Skip to content

Commit

Permalink
[SIEM] Check ML Job status on ML Rule execution (#61715) (#61898)
Browse files Browse the repository at this point in the history
* Move isMlRule helper to a more general location

And use it during rule execution as well.

* Add error message back to rule error status

This was unintentionally removed in a previous merge commit.

* Expose mlClient as part of ML's Setup contract

This allows dependent plugins to leverage the exposed services without
having to define their own ml paths, e.g. "ml.jobs"

* Move ML Job predicates to common folder

These are pure functions and used on both the client and server.

* WIP: Check ML Job status on ML Rule execution

This works, but unfortunately it pushes this executor function to a
complexity of 25. We're gonna refactor this next.

* Move isMlRule and RuleType to common

These are used on both the frontend and the backend, and can be shared.

* Refactor Signal Rule executor to use RuleStatusService

RuleStatusService holds the logic for updating the current status as
well as adding an error status. It leverages a simple
RuleStatusSavedObjectClient to handle the communication with
SavedObjects.

This removes the need for our specialized 'writeError', 'writeGap', and
'writeSuccess' functions, which duplicated much of the rule status
logic and code. It also fixes a bug with gap failures, with should have
been treated the same as other failures.

NB that an error does not necessarily prevent the rule from running, as
in the case of a gap or an ML Job not running.

This also adds a buildRuleMessage helper to reduce the noise of
generating logs/messages, and to make them more consistent.

* Remove unneeded 'async' keywords

We're not awaiting here, so we can just return the promise.

* Make buildRuleStatusAttributes synchronous

We weren't doing anything async here, and in fact the returning of a
promise was causing a bug when we tried to spread it into our attributes
object.

* Fix incorrectly-named RuleStatus attributes

This mapping could be done within the ruleStatusService, but it
lives outside it for now.

Also renames the object holding these values to the more general
'result,' as creationSuccess implies it always succeeds.

* Move our rule message helpers to a separate file

Adds some tests, as well.

* Refactor how rule status objects interact

Only ruleStatusSavedObjectsClient receives a savedObjectsClient, the
other functions receive the ruleStatusSavedObjectsClient

* pluralizes savedObjects in ruleStatusSavedObjectsClient
* Backfills tests

* Handle adding multiple errors during a single rule execution

We were storing state in our RuleStatusClient, and consequently could
get into a situation where that state did not reflect reality, and we
would incorrectly try to delete a SavedObject that had already been
deleted.

Rather than try to store the _correct_ state in the service, we remove
state entirely and just fetch our statuses on each action.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
  • Loading branch information
rylnd and elasticmachine authored Mar 31, 2020
1 parent 8fbe161 commit c0b2598
Show file tree
Hide file tree
Showing 36 changed files with 702 additions and 383 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { isJobStarted, isJobLoading, isJobFailed } from './';
import { isJobStarted, isJobLoading, isJobFailed } from './ml_helpers';

describe('isJobStarted', () => {
test('returns false if only jobState is enabled', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { RuleType } from './types';

// Based on ML Job/Datafeed States from x-pack/legacy/plugins/ml/common/constants/states.js
const enabledStates = ['started', 'opened'];
const loadingStates = ['starting', 'stopping', 'opening', 'closing'];
Expand All @@ -20,3 +22,5 @@ export const isJobLoading = (jobState: string, datafeedState: string): boolean =
export const isJobFailed = (jobState: string, datafeedState: string): boolean => {
return failureStates.includes(jobState) || failureStates.includes(datafeedState);
};

export const isMlRule = (ruleType: RuleType) => ruleType === 'machine_learning';
8 changes: 8 additions & 0 deletions x-pack/legacy/plugins/siem/common/detection_engine/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,17 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import * as t from 'io-ts';

import { AlertAction } from '../../../../../plugins/alerting/common';

export type RuleAlertAction = Omit<AlertAction, 'actionTypeId'> & {
action_type_id: string;
};

export const RuleTypeSchema = t.keyof({
query: null,
saved_query: null,
machine_learning: null,
});
export type RuleType = t.TypeOf<typeof RuleTypeSchema>;
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ import styled from 'styled-components';
import React, { useState, useCallback } from 'react';
import { EuiFlexGroup, EuiFlexItem, EuiLoadingSpinner, EuiSwitch } from '@elastic/eui';
import { SiemJob } from '../types';
import { isJobLoading, isJobStarted, isJobFailed } from '../../ml/helpers';
import {
isJobLoading,
isJobFailed,
isJobStarted,
} from '../../../../common/detection_engine/ml_helpers';

const StaticSwitch = styled(EuiSwitch)`
.euiSwitch__thumb,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,7 @@

import * as t from 'io-ts';

export const RuleTypeSchema = t.keyof({
query: null,
saved_query: null,
machine_learning: null,
});
export type RuleType = t.TypeOf<typeof RuleTypeSchema>;
import { RuleTypeSchema } from '../../../../common/detection_engine/types';

/**
* Params is an "record", since it is a type of AlertActionParams which is action templates.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ import {
ReturnRulesStatuses,
} from './use_rule_status';
import * as api from './api';
import { RuleType } from '../rules/types';
import { Rule } from '../rules/types';

jest.mock('./api');

const testRule = {
const testRule: Rule = {
actions: [
{
group: 'fake group',
Expand Down Expand Up @@ -57,7 +57,7 @@ const testRule = {
threat: [],
throttle: null,
to: 'now',
type: 'query' as RuleType,
type: 'query',
updated_at: 'mm/dd/yyyyTHH:MM:sssz',
updated_by: 'mockUser',
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ import styled from 'styled-components';

import { esFilters } from '../../../../../../../../../../src/plugins/data/public';

import { RuleType } from '../../../../../../common/detection_engine/types';
import { tacticsOptions, techniquesOptions } from '../../../mitre/mitre_tactics_techniques';

import * as i18n from './translations';
import { BuildQueryBarDescription, BuildThreatDescription, ListItems } from './types';
import { SeverityBadge } from '../severity_badge';
import ListTreeIcon from './assets/list_tree_icon.svg';
import { RuleType } from '../../../../../containers/detection_engine/rules';
import { assertUnreachable } from '../../../../../lib/helpers';

const NoteDescriptionContainer = styled(EuiFlexItem)`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
esFilters,
FilterManager,
} from '../../../../../../../../../../src/plugins/data/public';
import { RuleType } from '../../../../../containers/detection_engine/rules';
import { RuleType } from '../../../../../../common/detection_engine/types';
import { DEFAULT_TIMELINE_TITLE } from '../../../../../components/timeline/translations';
import { useKibana } from '../../../../../lib/kibana';
import { IMitreEnterpriseAttack } from '../../types';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ import { EuiBadge, EuiIcon, EuiLink, EuiToolTip } from '@elastic/eui';
import { useKibana } from '../../../../../lib/kibana';
import { SiemJob } from '../../../../../components/ml_popover/types';
import { ListItems } from './types';
import { isJobStarted } from '../../../../../components/ml/helpers';
import { ML_JOB_STARTED, ML_JOB_STOPPED } from './translations';
import { isJobStarted } from '../../../../../../common/detection_engine/ml_helpers';

enum MessageLevels {
info = 'info',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ import {
EuiText,
} from '@elastic/eui';

import { isMlRule } from '../../../../../../common/detection_engine/ml_helpers';
import { RuleType } from '../../../../../../common/detection_engine/types';
import { FieldHook } from '../../../../../shared_imports';
import { RuleType } from '../../../../../containers/detection_engine/rules/types';
import * as i18n from './translations';
import { isMlRule } from '../../helpers';

const MlCardDescription = ({ hasValidLicense = false }: { hasValidLicense?: boolean }) => (
<EuiText size="s">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ import deepEqual from 'fast-deep-equal';
import { IIndexPattern } from '../../../../../../../../../../src/plugins/data/public';
import { useFetchIndexPatterns } from '../../../../../containers/detection_engine/rules';
import { DEFAULT_INDEX_KEY } from '../../../../../../common/constants';
import { isMlRule } from '../../../../../../common/detection_engine/ml_helpers';
import { DEFAULT_TIMELINE_TITLE } from '../../../../../components/timeline/translations';
import { useMlCapabilities } from '../../../../../components/ml_popover/hooks/use_ml_capabilities';
import { useUiSetting$ } from '../../../../../lib/kibana';
import { setFieldValue, isMlRule } from '../../helpers';
import { setFieldValue } from '../../helpers';
import { DefineStepRule, RuleStep, RuleStepProps } from '../../types';
import { StepRuleDescription } from '../description_step';
import { QueryBarDefineRule } from '../query_bar';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { isEmpty } from 'lodash/fp';
import React from 'react';

import { esKuery } from '../../../../../../../../../../src/plugins/data/public';
import { isMlRule } from '../../../../../../common/detection_engine/ml_helpers';
import { FieldValueQueryBar } from '../query_bar';
import {
ERROR_CODE,
Expand All @@ -19,7 +20,6 @@ import {
ValidationFunc,
} from '../../../../../shared_imports';
import { CUSTOM_QUERY_REQUIRED, INVALID_CUSTOM_QUERY, INDEX_HELPER_TEXT } from './translations';
import { isMlRule } from '../../helpers';

export const schema: FormSchema = {
index: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ import {
NOTIFICATION_THROTTLE_RULE,
NOTIFICATION_THROTTLE_NO_ACTIONS,
} from '../../../../../common/constants';
import { NewRule, RuleType } from '../../../../containers/detection_engine/rules';
import { transformAlertToRuleAction } from '../../../../../common/detection_engine/transform_actions';
import { RuleType } from '../../../../../common/detection_engine/types';
import { isMlRule } from '../../../../../common/detection_engine/ml_helpers';
import { NewRule } from '../../../../containers/detection_engine/rules';

import {
AboutStepRule,
Expand All @@ -25,7 +27,6 @@ import {
AboutStepRuleJson,
ActionsStepRuleJson,
} from '../types';
import { isMlRule } from '../helpers';

export const getTimeTypeValue = (time: string): { unit: string; value: number } => {
const timeObj = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@ import moment from 'moment';
import memoizeOne from 'memoize-one';
import { useLocation } from 'react-router-dom';

import { RuleAlertAction } from '../../../../common/detection_engine/types';
import { RuleAlertAction, RuleType } from '../../../../common/detection_engine/types';
import { isMlRule } from '../../../../common/detection_engine/ml_helpers';
import { transformRuleToAlertAction } from '../../../../common/detection_engine/transform_actions';
import { Filter } from '../../../../../../../../src/plugins/data/public';
import { Rule, RuleType } from '../../../containers/detection_engine/rules';
import { Rule } from '../../../containers/detection_engine/rules';
import { FormData, FormHook, FormSchema } from '../../../shared_imports';
import {
AboutStepRule,
Expand Down Expand Up @@ -214,8 +215,6 @@ export const setFieldValue = (
}
});

export const isMlRule = (ruleType: RuleType) => ruleType === 'machine_learning';

export const redirectToDetections = (
isSignalIndexExists: boolean | null,
isAuthenticated: boolean | null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@
*/

import { AlertAction } from '../../../../../../../plugins/alerting/common';
import { RuleAlertAction } from '../../../../common/detection_engine/types';
import { RuleAlertAction, RuleType } from '../../../../common/detection_engine/types';
import { Filter } from '../../../../../../../../src/plugins/data/common';
import { RuleType } from '../../../containers/detection_engine/rules/types';
import { FieldValueQueryBar } from './components/query_bar';
import { FormData, FormHook } from '../../../shared_imports';
import { FieldValueTimeline } from './components/pick_timeline';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,7 @@ import {
isRuleStatusFindTypes,
isRuleStatusSavedObjectType,
} from '../../rules/types';
import {
OutputRuleAlertRest,
ImportRuleAlertRest,
RuleAlertParamsRest,
RuleType,
} from '../../types';
import { OutputRuleAlertRest, ImportRuleAlertRest, RuleAlertParamsRest } from '../../types';
import {
createBulkErrorObject,
BulkError,
Expand Down Expand Up @@ -300,5 +295,3 @@ export const getTupleDuplicateErrorsAndUniqueRules = (

return [Array.from(errors.values()), Array.from(rulesAcc.values())];
};

export const isMlRule = (ruleType: RuleType) => ruleType === 'machine_learning';
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import * as t from 'io-ts';
import { Either, left, fold } from 'fp-ts/lib/Either';
import { pipe } from 'fp-ts/lib/pipeable';

import { isMlRule } from '../../../../../../common/detection_engine/ml_helpers';
import {
dependentRulesSchema,
RequiredRulesSchema,
Expand Down Expand Up @@ -47,7 +48,7 @@ export const addQueryFields = (typeAndTimelineOnly: TypeAndTimelineOnly): t.Mixe
};

export const addMlFields = (typeAndTimelineOnly: TypeAndTimelineOnly): t.Mixed[] => {
if (typeAndTimelineOnly.type === 'machine_learning') {
if (isMlRule(typeAndTimelineOnly.type)) {
return [
t.exact(t.type({ anomaly_threshold: dependentRulesSchema.props.anomaly_threshold })),
t.exact(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ import {
} from '../../../../../../../../src/core/server';
import { ILicense } from '../../../../../../../plugins/licensing/server';
import { MINIMUM_ML_LICENSE } from '../../../../common/constants';
import { RuleType } from '../../../../common/detection_engine/types';
import { isMlRule } from '../../../../common/detection_engine/ml_helpers';
import { BadRequestError } from '../errors/bad_request_error';
import { RuleType } from '../types';
import { isMlRule } from './rules/utils';

export interface OutputError {
message: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export interface IRuleStatusFindType {
saved_objects: IRuleStatusSavedObject[];
}

export type RuleStatusString = 'succeeded' | 'failed' | 'going to run' | 'executing';
export type RuleStatusString = 'succeeded' | 'failed' | 'going to run';

export interface HapiReadableStream extends Readable {
hapi: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,15 @@
*/

import { SignalSourceHit, SignalSearchResponse } from '../types';
import { Logger } from 'kibana/server';
import {
Logger,
SavedObject,
SavedObjectsFindResponse,
} from '../../../../../../../../../src/core/server';
import { loggingServiceMock } from '../../../../../../../../../src/core/server/mocks';
import { RuleTypeParams, OutputRuleAlertRest } from '../../types';
import { IRuleStatusAttributes } from '../../rules/types';
import { ruleStatusSavedObjectType } from '../../../../saved_objects';

export const sampleRuleAlertParams = (
maxSignals?: number | undefined,
Expand Down Expand Up @@ -373,4 +379,34 @@ export const sampleRule = (): Partial<OutputRuleAlertRest> => {
};
};

export const exampleRuleStatus: () => SavedObject<IRuleStatusAttributes> = () => ({
type: ruleStatusSavedObjectType,
id: '042e6d90-7069-11ea-af8b-0f8ae4fa817e',
attributes: {
alertId: 'f4b8e31d-cf93-4bde-a265-298bde885cd7',
statusDate: '2020-03-27T22:55:59.517Z',
status: 'succeeded',
lastFailureAt: null,
lastSuccessAt: '2020-03-27T22:55:59.517Z',
lastFailureMessage: null,
lastSuccessMessage: 'succeeded',
gap: null,
bulkCreateTimeDurations: [],
searchAfterTimeDurations: [],
lastLookBackDate: null,
},
references: [],
updated_at: '2020-03-27T22:55:59.577Z',
version: 'WzgyMiwxXQ==',
});

export const exampleFindRuleStatusResponse: (
mockStatuses: Array<SavedObject<IRuleStatusAttributes>>
) => SavedObjectsFindResponse<IRuleStatusAttributes> = (mockStatuses = [exampleRuleStatus()]) => ({
total: 1,
per_page: 6,
page: 1,
saved_objects: mockStatuses,
});

export const mockLogger: Logger = loggingServiceMock.createLogger();
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { RuleStatusSavedObjectsClient } from '../rule_status_saved_objects_client';

const createMockRuleStatusSavedObjectsClient = (): jest.Mocked<RuleStatusSavedObjectsClient> => ({
find: jest.fn(),
create: jest.fn(),
update: jest.fn(),
delete: jest.fn(),
});

export const ruleStatusSavedObjectsClientMock = {
create: createMockRuleStatusSavedObjectsClient,
};
Loading

0 comments on commit c0b2598

Please sign in to comment.