Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RAM][SECURITYSOLUTION][ALERTS] - Show warning that custom action intervals cannot be shorter than the rule's check interval #155684

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { getTimeTypeValue } from './time_type_value';

describe('getTimeTypeValue', () => {
[
{ interval: '1ms', value: 1, unit: 'ms' },
{ interval: '0s', value: 0, unit: 's' },
{ interval: '3s', value: 3, unit: 's' },
{ interval: '5m', value: 5, unit: 'm' },
{ interval: '7h', value: 7, unit: 'h' },
{ interval: '10d', value: 10, unit: 'd' },
].forEach(({ interval, value, unit }) => {
it(`should correctly return time duration and time unit when 'interval' is ${interval}`, () => {
const { value: actualValue, unit: actualUnit } = getTimeTypeValue(interval);
expect(actualValue).toBe(value);
expect(actualUnit).toBe(unit);
});
});

[
{ interval: '-1ms', value: 1, unit: 'ms' },
{ interval: '-3s', value: 3, unit: 's' },
{ interval: '-5m', value: 5, unit: 'm' },
{ interval: '-7h', value: 7, unit: 'h' },
{ interval: '-10d', value: 10, unit: 'd' },
{ interval: '-1', value: 1, unit: 'ms' },
{ interval: '-3', value: 3, unit: 'ms' },
{ interval: '-5', value: 5, unit: 'ms' },
{ interval: '-7', value: 7, unit: 'ms' },
{ interval: '-10', value: 10, unit: 'ms' },
].forEach(({ interval, value, unit }) => {
it(`should correctly return positive time duration and time unit when 'interval' is negative (${interval})`, () => {
const { value: actualValue, unit: actualUnit } = getTimeTypeValue(interval);
expect(actualValue).toBe(value);
expect(actualUnit).toBe(unit);
});
});

[
{ interval: 'ms', value: 0, unit: 'ms' },
{ interval: 's', value: 0, unit: 's' },
{ interval: 'm', value: 0, unit: 'm' },
{ interval: 'h', value: 0, unit: 'h' },
{ interval: 'd', value: 0, unit: 'd' },
{ interval: '-ms', value: 0, unit: 'ms' },
{ interval: '-s', value: 0, unit: 's' },
{ interval: '-m', value: 0, unit: 'm' },
{ interval: '-h', value: 0, unit: 'h' },
{ interval: '-d', value: 0, unit: 'd' },
].forEach(({ interval, value, unit }) => {
it(`should correctly return time duration equal to '0' when 'interval' does not specify time duration (${interval})`, () => {
const { value: actualValue, unit: actualUnit } = getTimeTypeValue(interval);
expect(actualValue).toBe(value);
expect(actualUnit).toBe(unit);
});
});

[
{ interval: '0', value: 0, unit: 'ms' },
{ interval: '1', value: 1, unit: 'ms' },
{ interval: '3', value: 3, unit: 'ms' },
{ interval: '5', value: 5, unit: 'ms' },
{ interval: '7', value: 7, unit: 'ms' },
{ interval: '10', value: 10, unit: 'ms' },
].forEach(({ interval, value, unit }) => {
it(`should correctly return time unit set to 'ms' as a default value when 'interval' does not specify it (${interval})`, () => {
const { value: actualValue, unit: actualUnit } = getTimeTypeValue(interval);
expect(actualValue).toBe(value);
expect(actualUnit).toBe(unit);
});
});

[
{ interval: '1f', value: 1, unit: 'ms' },
{ interval: '-3r', value: 3, unit: 'ms' },
{ interval: 'p', value: 0, unit: 'ms' },
].forEach(({ interval, value, unit }) => {
it(`should correctly return time unit set to 'ms' as a default value when data is invalid (${interval})`, () => {
const { value: actualValue, unit: actualUnit } = getTimeTypeValue(interval);
expect(actualValue).toBe(value);
expect(actualUnit).toBe(unit);
});
});
});
29 changes: 29 additions & 0 deletions x-pack/plugins/security_solution/common/utils/time_type_value.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { isEmpty } from 'lodash/fp';
import type { Unit } from '@kbn/datemath';

export const getTimeTypeValue = (time: string): { unit: Unit; value: number } => {
Copy link
Contributor

@maximpn maximpn Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be really helpful to have a comment with an expected time format.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

I worked last year on creating a TimeDuration io-ts type and added there some explanation of how the type works, that could be of inspiration:
https://github.com/elastic/kibana/blob/main/packages/kbn-securitysolution-io-ts-types/src/time_duration/index.ts

Copy link
Contributor Author

@e40pud e40pud Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved this method from another file x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_creation/helpers.ts :-)

If I will have time before the release to address the concerns that you put, I will look into this as well.

const timeObj: { unit: Unit; value: number } = {
unit: 'ms',
value: 0,
};
const filterTimeVal = time.match(/\d+/g);
const filterTimeType = time.match(/[a-zA-Z]+/g);
Copy link
Contributor

@maximpn maximpn Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some reason to match all letters here and check separately for s, m, h and d below? What if there is time string like 1abch or 20mms?

It could be one regex like

/^(\d+)?(ms|s|m|h|d)?$/

to match an expected format strictly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Maxim here, with the additional doubt of why defaulting to 0ms if the only accepted Units are actually ['s', 'm', 'h', 'd']

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

I just moved this method from another file x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_creation/helpers.ts :-)

If I will have time before the release to address the concerns that you put, I will look into this as well.

if (!isEmpty(filterTimeVal) && filterTimeVal != null && !isNaN(Number(filterTimeVal[0]))) {
timeObj.value = Number(filterTimeVal[0]);
}
if (
!isEmpty(filterTimeType) &&
filterTimeType != null &&
['s', 'm', 'h', 'd'].includes(filterTimeType[0])
) {
timeObj.unit = filterTimeType[0] as Unit;
}
return timeObj;
};
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import type {
DefineStepRule,
} from '../../../../detections/pages/detection_engine/rules/types';
import {
getTimeTypeValue,
formatDefineStepData,
formatScheduleStepData,
formatAboutStepData,
Expand All @@ -43,56 +42,6 @@ import { getThreatMock } from '../../../../../common/detection_engine/schemas/ty
import type { Threat, Threats } from '@kbn/securitysolution-io-ts-alerting-types';

describe('helpers', () => {
describe('getTimeTypeValue', () => {
test('returns timeObj with value 0 if no time value found', () => {
const result = getTimeTypeValue('m');

expect(result).toEqual({ unit: 'm', value: 0 });
});

test('returns timeObj with unit set to default unit value of "ms" if no expected time type found', () => {
const result = getTimeTypeValue('5l');

expect(result).toEqual({ unit: 'ms', value: 5 });
});

test('returns timeObj with unit of s and value 5 when time is 5s ', () => {
const result = getTimeTypeValue('5s');

expect(result).toEqual({ unit: 's', value: 5 });
});

test('returns timeObj with unit of m and value 5 when time is 5m ', () => {
const result = getTimeTypeValue('5m');

expect(result).toEqual({ unit: 'm', value: 5 });
});

test('returns timeObj with unit of h and value 5 when time is 5h ', () => {
const result = getTimeTypeValue('5h');

expect(result).toEqual({ unit: 'h', value: 5 });
});

test('returns timeObj with value of 5 when time is float like 5.6m ', () => {
const result = getTimeTypeValue('5m');

expect(result).toEqual({ unit: 'm', value: 5 });
});

test('returns timeObj with value of 0 and unit of "ms" if random string passed in', () => {
const result = getTimeTypeValue('random');

expect(result).toEqual({ unit: 'ms', value: 0 });
});

test('returns timeObj with unit of d and value 5 when time is 5d ', () => {
const result = getTimeTypeValue('5d');

expect(result).toEqual({ unit: 'd', value: 5 });
});
});

describe('filterEmptyThreats', () => {
let mockThreat: Threat;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
/* eslint-disable complexity */

import { has, isEmpty } from 'lodash/fp';
import type { Unit } from '@kbn/datemath';
import moment from 'moment';
import deepmerge from 'deepmerge';
import omit from 'lodash/omit';
Expand All @@ -25,6 +24,7 @@ import type {
Type,
} from '@kbn/securitysolution-io-ts-alerting-types';
import { ENDPOINT_LIST_ID } from '@kbn/securitysolution-list-constants';
import { getTimeTypeValue } from '../../../../../common/utils/time_type_value';
import { assertUnreachable } from '../../../../../common/utility_types';
import {
transformAlertToRuleAction,
Expand All @@ -49,26 +49,6 @@ import type { RuleCreateProps } from '../../../../../common/api/detection_engine
import { DEFAULT_SUPPRESSION_MISSING_FIELDS_STRATEGY } from '../../../../../common/api/detection_engine/model/rule_schema';
import { stepActionsDefaultValue } from '../../../../detections/components/rules/step_rule_actions';

export const getTimeTypeValue = (time: string): { unit: Unit; value: number } => {
const timeObj: { unit: Unit; value: number } = {
unit: 'ms',
value: 0,
};
const filterTimeVal = time.match(/\d+/g);
const filterTimeType = time.match(/[a-zA-Z]+/g);
if (!isEmpty(filterTimeVal) && filterTimeVal != null && !isNaN(Number(filterTimeVal[0]))) {
timeObj.value = Number(filterTimeVal[0]);
}
if (
!isEmpty(filterTimeType) &&
filterTimeType != null &&
['s', 'm', 'h', 'd'].includes(filterTimeType[0])
) {
timeObj.unit = filterTimeType[0] as Unit;
}
return timeObj;
};

export interface RuleFields {
anomalyThreshold: unknown;
machineLearningJobId: unknown;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,7 @@ const CreateRulePageComponent: React.FC = () => {
isLoading={isCreateRuleLoading || loading || isStartingJobs}
actionMessageParams={actionMessageParams}
summaryActionMessageParams={actionMessageParams}
ruleScheduleInterval={scheduleStepData.interval}
ruleType={ruleType}
form={actionsStepForm}
/>
Expand Down Expand Up @@ -735,6 +736,7 @@ const CreateRulePageComponent: React.FC = () => {
isStartingJobs,
loading,
ruleType,
scheduleStepData.interval,
submitRuleDisabled,
submitRuleEnabled,
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ const EditRulePageComponent: FC<{ rule: Rule }> = ({ rule }) => {
isUpdateView
actionMessageParams={actionMessageParams}
summaryActionMessageParams={actionMessageParams}
ruleScheduleInterval={scheduleStepData.interval}
ruleType={rule?.type}
form={actionsStepForm}
key="actionsStep"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import type {
import { SecurityConnectorFeatureId } from '@kbn/actions-plugin/common';
import { FormattedMessage } from '@kbn/i18n-react';
import { AlertConsumers } from '@kbn/rule-data-utils';
import { getTimeTypeValue } from '../../../../../common/utils/time_type_value';
import { NOTIFICATION_DEFAULT_FREQUENCY } from '../../../../../common/constants';
import type { FieldHook } from '../../../../shared_imports';
import { useFormContext } from '../../../../shared_imports';
Expand Down Expand Up @@ -81,6 +82,7 @@ interface Props {
field: FieldHook;
messageVariables: ActionVariables;
summaryMessageVariables: ActionVariables;
ruleScheduleInterval?: string;
}

const DEFAULT_ACTION_GROUP_ID = 'default';
Expand Down Expand Up @@ -116,6 +118,7 @@ export const RuleActionsField: React.FC<Props> = ({
field,
messageVariables,
summaryMessageVariables,
ruleScheduleInterval,
}) => {
const [fieldErrors, setFieldErrors] = useState<string | null>(null);
const form = useFormContext();
Expand Down Expand Up @@ -144,6 +147,14 @@ export const RuleActionsField: React.FC<Props> = ({
[actions]
);

const minimumThrottleInterval = useMemo<[number, string] | undefined>(() => {
if (ruleScheduleInterval != null) {
const { unit: intervalUnit, value: intervalValue } = getTimeTypeValue(ruleScheduleInterval);
return [intervalValue, intervalUnit];
}
return ruleScheduleInterval;
}, [ruleScheduleInterval]);

const setActionIdByIndex = useCallback(
(id: string, index: number) => {
const updatedActions = [...(actions as Array<Partial<RuleAction>>)];
Expand Down Expand Up @@ -253,6 +264,7 @@ export const RuleActionsField: React.FC<Props> = ({
notifyWhenSelectOptions: NOTIFY_WHEN_OPTIONS,
defaultRuleFrequency: NOTIFICATION_DEFAULT_FREQUENCY,
disableErrorMessages: !isFormValidated,
minimumThrottleInterval,
}),
[
actions,
Expand All @@ -265,6 +277,7 @@ export const RuleActionsField: React.FC<Props> = ({
setAlertActionsProperty,
setActionAlertsFilterProperty,
isFormValidated,
minimumThrottleInterval,
]
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
*/

import moment from 'moment';
import { getTimeTypeValue } from '../../../../../common/utils/time_type_value';

import type { TimeframePreviewOptions } from '../../../pages/detection_engine/rules/types';
import { getTimeTypeValue } from '../../../../detection_engine/rule_creation_ui/pages/rule_creation/helpers';

export const usePreviewInvocationCount = ({
timeframeOptions,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ describe('StepRuleActions', () => {

return (
<StepRuleActions
ruleScheduleInterval={'5m'}
actionMessageParams={actionMessageParams}
summaryActionMessageParams={actionMessageParams}
isLoading={false}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ interface StepRuleActionsProps extends RuleStepProps {
actionMessageParams: ActionVariables;
summaryActionMessageParams: ActionVariables;
ruleType?: Type;
ruleScheduleInterval: string;
form: FormHook<ActionsStepRule>;
}

Expand Down Expand Up @@ -79,6 +80,7 @@ const StepRuleActionsComponent: FC<StepRuleActionsProps> = ({
actionMessageParams,
summaryActionMessageParams,
ruleType,
ruleScheduleInterval,
form,
}) => {
const {
Expand All @@ -96,11 +98,12 @@ const StepRuleActionsComponent: FC<StepRuleActionsProps> = ({
componentProps={{
messageVariables: actionMessageParams,
summaryMessageVariables: summaryActionMessageParams,
ruleScheduleInterval,
}}
/>
</>
),
[actionMessageParams, summaryActionMessageParams]
[actionMessageParams, ruleScheduleInterval, summaryActionMessageParams]
);
const displayResponseActionsOptions = useMemo(() => {
if (isQueryRule(ruleType)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type { ActionResult } from '@kbn/actions-plugin/server';
import type { RuleActionFrequency, RuleAction } from '@kbn/alerting-plugin/common';
import type { ActionTypeRegistryContract } from '@kbn/triggers-actions-ui-plugin/public';
import { FormattedMessage } from '@kbn/i18n-react';
import { getTimeTypeValue } from '../../../../detection_engine/rule_creation_ui/pages/rule_creation/helpers';
import { getTimeTypeValue } from '../../../../../common/utils/time_type_value';
import * as i18n from './translations';

const DescriptionLine = ({ children }: { children: React.ReactNode }) => (
Expand Down