From 205a43ca21c6cb991d32984c0ae3edc77f3c03fc Mon Sep 17 00:00:00 2001 From: Zacqary Adam Xeper Date: Wed, 16 Nov 2022 16:02:36 -0600 Subject: [PATCH] =?UTF-8?q?[Alert=20Summaries]=20[BE]=20Move=20=E2=80=9CNo?= =?UTF-8?q?tify=20When=E2=80=9D=20and=20throttle=20from=20rule=20to=20acti?= =?UTF-8?q?on=20(#144130)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add frequency props to actions and validate them on edit/create * Nullify undefined throttle * Update schema to allow for frequency param on actions * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Commit missing file * Fix types * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * Fix types * Fix jest * Fix validating global freq params * Add tests for create and edit * Reset legacy api * Make notify and throttle optional in route schemas * Fix tests * Split missing frequency test cases Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 0c864fe479e332aa92bf189f0123661cd52262ae) --- .../plugins/alerting/common/alert_summary.ts | 2 +- x-pack/plugins/alerting/common/rule.ts | 9 +- .../alerting/common/rule_notify_when_type.ts | 2 +- .../lib/alert_summary_from_event_log.ts | 2 +- .../alerting/server/routes/create_rule.ts | 17 +- .../server/routes/lib/actions_schema.ts | 29 +++ .../alerting/server/routes/lib/index.ts | 2 + .../server/routes/lib/rewrite_actions.ts | 32 ++++ .../server/routes/lib/rewrite_request_case.ts | 2 +- .../server/routes/lib/rewrite_rule.ts | 10 +- .../alerting/server/routes/update_rule.ts | 16 +- .../server/rules_client/rules_client.ts | 63 +++++- .../server/rules_client/tests/create.test.ts | 180 ++++++++++++++++++ .../server/rules_client/tests/update.test.ts | 177 +++++++++++++++++ .../server/task_runner/execution_handler.ts | 2 +- .../server/task_runner/task_runner.ts | 4 +- x-pack/plugins/alerting/server/types.ts | 9 +- .../normalization/rule_converters.ts | 4 +- .../rule_schema/model/rule_schemas.ts | 16 +- 19 files changed, 533 insertions(+), 45 deletions(-) create mode 100644 x-pack/plugins/alerting/server/routes/lib/actions_schema.ts create mode 100644 x-pack/plugins/alerting/server/routes/lib/rewrite_actions.ts diff --git a/x-pack/plugins/alerting/common/alert_summary.ts b/x-pack/plugins/alerting/common/alert_summary.ts index f9675e64a7f9..25b00538c16b 100644 --- a/x-pack/plugins/alerting/common/alert_summary.ts +++ b/x-pack/plugins/alerting/common/alert_summary.ts @@ -20,7 +20,7 @@ export interface AlertSummary { ruleTypeId: string; consumer: string; muteAll: boolean; - throttle: string | null; + throttle?: string | null; enabled: boolean; statusStartDate: string; statusEndDate: string; diff --git a/x-pack/plugins/alerting/common/rule.ts b/x-pack/plugins/alerting/common/rule.ts index bd479b96f9b1..b13c996e5bf7 100644 --- a/x-pack/plugins/alerting/common/rule.ts +++ b/x-pack/plugins/alerting/common/rule.ts @@ -75,6 +75,11 @@ export interface RuleAction { id: string; actionTypeId: string; params: RuleActionParams; + frequency?: { + summary: boolean; + notifyWhen: RuleNotifyWhenType; + throttle: string | null; + }; } export interface RuleAggregations { @@ -123,9 +128,9 @@ export interface Rule { updatedAt: Date; apiKey: string | null; apiKeyOwner: string | null; - throttle: string | null; + throttle?: string | null; muteAll: boolean; - notifyWhen: RuleNotifyWhenType | null; + notifyWhen?: RuleNotifyWhenType | null; mutedInstanceIds: string[]; executionStatus: RuleExecutionStatus; monitoring?: RuleMonitoring; diff --git a/x-pack/plugins/alerting/common/rule_notify_when_type.ts b/x-pack/plugins/alerting/common/rule_notify_when_type.ts index 700c87acdbdb..4be2e35f2d39 100644 --- a/x-pack/plugins/alerting/common/rule_notify_when_type.ts +++ b/x-pack/plugins/alerting/common/rule_notify_when_type.ts @@ -5,7 +5,7 @@ * 2.0. */ -const RuleNotifyWhenTypeValues = [ +export const RuleNotifyWhenTypeValues = [ 'onActionGroupChange', 'onActiveAlert', 'onThrottleInterval', diff --git a/x-pack/plugins/alerting/server/lib/alert_summary_from_event_log.ts b/x-pack/plugins/alerting/server/lib/alert_summary_from_event_log.ts index f1aedf078800..12ea8df57eb8 100644 --- a/x-pack/plugins/alerting/server/lib/alert_summary_from_event_log.ts +++ b/x-pack/plugins/alerting/server/lib/alert_summary_from_event_log.ts @@ -31,7 +31,7 @@ export function alertSummaryFromEventLog(params: AlertSummaryFromEventLogParams) statusEndDate: dateEnd, status: 'OK', muteAll: rule.muteAll, - throttle: rule.throttle, + throttle: rule.throttle ?? null, enabled: rule.enabled, lastRun: undefined, errorMessages: [], diff --git a/x-pack/plugins/alerting/server/routes/create_rule.ts b/x-pack/plugins/alerting/server/routes/create_rule.ts index 1b114dc54d26..8d418f8497cb 100644 --- a/x-pack/plugins/alerting/server/routes/create_rule.ts +++ b/x-pack/plugins/alerting/server/routes/create_rule.ts @@ -11,9 +11,11 @@ import { CreateOptions } from '../rules_client'; import { RewriteRequestCase, RewriteResponseCase, + rewriteActions, handleDisabledApiKeysError, verifyAccessAndContext, countUsageOfPredefinedIds, + actionsSchema, rewriteRuleLastRun, } from './lib'; import { @@ -31,20 +33,13 @@ export const bodySchema = schema.object({ enabled: schema.boolean({ defaultValue: true }), consumer: schema.string(), tags: schema.arrayOf(schema.string(), { defaultValue: [] }), - throttle: schema.nullable(schema.string({ validate: validateDurationSchema })), + throttle: schema.nullable(schema.maybe(schema.string({ validate: validateDurationSchema }))), params: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }), schedule: schema.object({ interval: schema.string({ validate: validateDurationSchema }), }), - actions: schema.arrayOf( - schema.object({ - group: schema.string(), - id: schema.string(), - params: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }), - }), - { defaultValue: [] } - ), - notify_when: schema.string({ validate: validateNotifyWhenType }), + actions: actionsSchema, + notify_when: schema.maybe(schema.string({ validate: validateNotifyWhenType })), }); const rewriteBodyReq: RewriteRequestCase['data']> = ({ @@ -56,6 +51,7 @@ const rewriteBodyReq: RewriteRequestCase['data']> alertTypeId, notifyWhen, }); + const rewriteBodyRes: RewriteResponseCase> = ({ actions, alertTypeId, @@ -132,6 +128,7 @@ export const createRuleRoute = ({ router, licenseState, usageCounter }: RouteOpt await rulesClient.create({ data: rewriteBodyReq({ ...rule, + actions: rewriteActions(rule.actions), notify_when: rule.notify_when as RuleNotifyWhenType, }), options: { id: params?.id }, diff --git a/x-pack/plugins/alerting/server/routes/lib/actions_schema.ts b/x-pack/plugins/alerting/server/routes/lib/actions_schema.ts new file mode 100644 index 000000000000..d89873a48c2e --- /dev/null +++ b/x-pack/plugins/alerting/server/routes/lib/actions_schema.ts @@ -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 { schema } from '@kbn/config-schema'; +import { validateDurationSchema } from '../../lib'; + +export const actionsSchema = schema.arrayOf( + schema.object({ + group: schema.string(), + id: schema.string(), + params: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }), + frequency: schema.maybe( + schema.object({ + summary: schema.boolean(), + notify_when: schema.oneOf([ + schema.literal('onActionGroupChange'), + schema.literal('onActiveAlert'), + schema.literal('onThrottleInterval'), + ]), + throttle: schema.nullable(schema.string({ validate: validateDurationSchema })), + }) + ), + }), + { defaultValue: [] } +); diff --git a/x-pack/plugins/alerting/server/routes/lib/index.ts b/x-pack/plugins/alerting/server/routes/lib/index.ts index cda768e7b363..387a6f11a5e5 100644 --- a/x-pack/plugins/alerting/server/routes/lib/index.ts +++ b/x-pack/plugins/alerting/server/routes/lib/index.ts @@ -18,5 +18,7 @@ export type { } from './rewrite_request_case'; export { verifyAccessAndContext } from './verify_access_and_context'; export { countUsageOfPredefinedIds } from './count_usage_of_predefined_ids'; +export { rewriteActions } from './rewrite_actions'; +export { actionsSchema } from './actions_schema'; export { rewriteRule, rewriteRuleLastRun } from './rewrite_rule'; export { rewriteNamespaces } from './rewrite_namespaces'; diff --git a/x-pack/plugins/alerting/server/routes/lib/rewrite_actions.ts b/x-pack/plugins/alerting/server/routes/lib/rewrite_actions.ts new file mode 100644 index 000000000000..37b39548c610 --- /dev/null +++ b/x-pack/plugins/alerting/server/routes/lib/rewrite_actions.ts @@ -0,0 +1,32 @@ +/* + * 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 { CamelToSnake, RewriteRequestCase } from './rewrite_request_case'; +import { RuleAction } from '../../types'; + +type ReqRuleAction = Omit & { + frequency?: { + [K in keyof NonNullable as CamelToSnake]: NonNullable< + RuleAction['frequency'] + >[K]; + }; +}; +export const rewriteActions: ( + actions?: ReqRuleAction[] +) => Array> = (actions) => { + const rewriteFrequency: RewriteRequestCase> = ({ + notify_when: notifyWhen, + ...rest + }) => ({ ...rest, notifyWhen }); + if (!actions) return []; + return actions.map( + (action) => + ({ + ...action, + ...(action.frequency ? { frequency: rewriteFrequency(action.frequency) } : {}), + } as RuleAction) + ); +}; diff --git a/x-pack/plugins/alerting/server/routes/lib/rewrite_request_case.ts b/x-pack/plugins/alerting/server/routes/lib/rewrite_request_case.ts index 4eb352d2b2b8..5cd64ebf5273 100644 --- a/x-pack/plugins/alerting/server/routes/lib/rewrite_request_case.ts +++ b/x-pack/plugins/alerting/server/routes/lib/rewrite_request_case.ts @@ -71,7 +71,7 @@ export type RewriteResponseCase = ( * * For more details see this PR comment: https://github.com/microsoft/TypeScript/pull/40336#issuecomment-686723087 */ -type CamelToSnake = string extends T +export type CamelToSnake = string extends T ? string : T extends `${infer C0}${infer C1}${infer R}` ? `${C0 extends Uppercase ? '_' : ''}${Lowercase}${C1 extends Uppercase diff --git a/x-pack/plugins/alerting/server/routes/lib/rewrite_rule.ts b/x-pack/plugins/alerting/server/routes/lib/rewrite_rule.ts index 270db64812b2..47dc3a78c278 100644 --- a/x-pack/plugins/alerting/server/routes/lib/rewrite_rule.ts +++ b/x-pack/plugins/alerting/server/routes/lib/rewrite_rule.ts @@ -56,11 +56,19 @@ export const rewriteRule = ({ last_execution_date: executionStatus.lastExecutionDate, last_duration: executionStatus.lastDuration, }, - actions: actions.map(({ group, id, actionTypeId, params }) => ({ + actions: actions.map(({ group, id, actionTypeId, params, frequency }) => ({ group, id, params, connector_type_id: actionTypeId, + ...(frequency + ? { + frequency: { + ...frequency, + notify_when: frequency.notifyWhen, + }, + } + : {}), })), ...(lastRun ? { last_run: rewriteRuleLastRun(lastRun) } : {}), ...(nextRun ? { next_run: nextRun } : {}), diff --git a/x-pack/plugins/alerting/server/routes/update_rule.ts b/x-pack/plugins/alerting/server/routes/update_rule.ts index 2b7f6b3c98b3..c998d5eb50a5 100644 --- a/x-pack/plugins/alerting/server/routes/update_rule.ts +++ b/x-pack/plugins/alerting/server/routes/update_rule.ts @@ -15,6 +15,8 @@ import { RewriteResponseCase, RewriteRequestCase, handleDisabledApiKeysError, + rewriteActions, + actionsSchema, rewriteRuleLastRun, } from './lib'; import { @@ -35,17 +37,10 @@ const bodySchema = schema.object({ schedule: schema.object({ interval: schema.string({ validate: validateDurationSchema }), }), - throttle: schema.nullable(schema.string({ validate: validateDurationSchema })), + throttle: schema.nullable(schema.maybe(schema.string({ validate: validateDurationSchema }))), params: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }), - actions: schema.arrayOf( - schema.object({ - group: schema.string(), - id: schema.string(), - params: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }), - }), - { defaultValue: [] } - ), - notify_when: schema.string({ validate: validateNotifyWhenType }), + actions: actionsSchema, + notify_when: schema.maybe(schema.string({ validate: validateNotifyWhenType })), }); const rewriteBodyReq: RewriteRequestCase> = (result) => { @@ -137,6 +132,7 @@ export const updateRuleRoute = ( id, data: { ...rule, + actions: rewriteActions(rule.actions), notify_when: rule.notify_when as RuleNotifyWhenType, }, }) diff --git a/x-pack/plugins/alerting/server/rules_client/rules_client.ts b/x-pack/plugins/alerting/server/rules_client/rules_client.ts index 811dee819e58..a22311440936 100644 --- a/x-pack/plugins/alerting/server/rules_client/rules_client.ts +++ b/x-pack/plugins/alerting/server/rules_client/rules_client.ts @@ -396,8 +396,8 @@ export interface UpdateOptions { schedule: IntervalSchedule; actions: NormalizedAlertAction[]; params: Params; - throttle: string | null; - notifyWhen: RuleNotifyWhenType | null; + throttle?: string | null; + notifyWhen?: RuleNotifyWhenType | null; }; } @@ -691,7 +691,7 @@ export class RulesClient { throw Boom.badRequest(`Error creating rule: could not create API key - ${error.message}`); } - await this.validateActions(ruleType, data.actions); + await this.validateActions(ruleType, data); // Throw error if schedule interval is less than the minimum and we are enforcing it const intervalInMs = parseDuration(data.schedule.interval); @@ -711,7 +711,8 @@ export class RulesClient { const createTime = Date.now(); const lastRunTimestamp = new Date(); const legacyId = Semver.lt(this.kibanaVersion, '8.0.0') ? id : null; - const notifyWhen = getRuleNotifyWhenType(data.notifyWhen, data.throttle); + const notifyWhen = getRuleNotifyWhenType(data.notifyWhen ?? null, data.throttle ?? null); + const throttle = data.throttle ?? null; const rawRule: RawRule = { ...data, @@ -727,6 +728,7 @@ export class RulesClient { muteAll: false, mutedInstanceIds: [], notifyWhen, + throttle, executionStatus: getRuleExecutionStatusPending(lastRunTimestamp.toISOString()), monitoring: getDefaultMonitoring(lastRunTimestamp.toISOString()), }; @@ -1870,7 +1872,7 @@ export class RulesClient { // Validate const validatedAlertTypeParams = validateRuleTypeParams(data.params, ruleType.validate?.params); - await this.validateActions(ruleType, data.actions); + await this.validateActions(ruleType, data); // Throw error if schedule interval is less than the minimum and we are enforcing it const intervalInMs = parseDuration(data.schedule.interval); @@ -1899,7 +1901,7 @@ export class RulesClient { } const apiKeyAttributes = this.apiKeyAsAlertAttributes(createdAPIKey, username); - const notifyWhen = getRuleNotifyWhenType(data.notifyWhen, data.throttle); + const notifyWhen = getRuleNotifyWhenType(data.notifyWhen ?? null, data.throttle ?? null); let updatedObject: SavedObject; const createAttributes = this.updateMeta({ @@ -2429,7 +2431,7 @@ export class RulesClient { for (const operation of operations) { switch (operation.field) { case 'actions': - await this.validateActions(ruleType, operation.value); + await this.validateActions(ruleType, { ...attributes, actions: operation.value }); ruleActions = applyBulkEditOperation(operation, ruleActions); break; case 'snoozeSchedule': @@ -2539,7 +2541,7 @@ export class RulesClient { // get notifyWhen const notifyWhen = getRuleNotifyWhenType( - attributes.notifyWhen, + attributes.notifyWhen ?? null, attributes.throttle ?? null ); @@ -3904,8 +3906,23 @@ export class RulesClient { private async validateActions( alertType: UntypedNormalizedRuleType, - actions: NormalizedAlertAction[] + data: Pick & { actions: NormalizedAlertAction[] } ): Promise { + const { actions, notifyWhen, throttle } = data; + const hasNotifyWhen = typeof notifyWhen !== 'undefined'; + const hasThrottle = typeof throttle !== 'undefined'; + let usesRuleLevelFreqParams; + if (hasNotifyWhen && hasThrottle) usesRuleLevelFreqParams = true; + else if (!hasNotifyWhen && !hasThrottle) usesRuleLevelFreqParams = false; + else { + throw Boom.badRequest( + i18n.translate('xpack.alerting.rulesClient.usesValidGlobalFreqParams.oneUndefined', { + defaultMessage: + 'Rule-level notifyWhen and throttle must both be defined or both be undefined', + }) + ); + } + if (actions.length === 0) { return; } @@ -3948,6 +3965,34 @@ export class RulesClient { }) ); } + + // check for actions using frequency params if the rule has rule-level frequency params defined + if (usesRuleLevelFreqParams) { + const actionsWithFrequency = actions.filter((action) => Boolean(action.frequency)); + if (actionsWithFrequency.length) { + throw Boom.badRequest( + i18n.translate('xpack.alerting.rulesClient.validateActions.mixAndMatchFreqParams', { + defaultMessage: + 'Cannot specify per-action frequency params when notify_when and throttle are defined at the rule level: {groups}', + values: { + groups: actionsWithFrequency.map((a) => a.group).join(', '), + }, + }) + ); + } + } else { + const actionsWithoutFrequency = actions.filter((action) => !action.frequency); + if (actionsWithoutFrequency.length) { + throw Boom.badRequest( + i18n.translate('xpack.alerting.rulesClient.validateActions.notAllActionsWithFreq', { + defaultMessage: 'Actions missing frequency parameters: {groups}', + values: { + groups: actionsWithoutFrequency.map((a) => a.group).join(', '), + }, + }) + ); + } + } } private async extractReferences< diff --git a/x-pack/plugins/alerting/server/rules_client/tests/create.test.ts b/x-pack/plugins/alerting/server/rules_client/tests/create.test.ts index aad483f09fe9..4d9b84e53a2f 100644 --- a/x-pack/plugins/alerting/server/rules_client/tests/create.test.ts +++ b/x-pack/plugins/alerting/server/rules_client/tests/create.test.ts @@ -2661,4 +2661,184 @@ describe('create()', () => { expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); expect(taskManager.schedule).not.toHaveBeenCalled(); }); + + test('throws error when mixing and matching global and per-action frequency values', async () => { + rulesClient = new RulesClient({ + ...rulesClientParams, + minimumScheduleInterval: { value: '1m', enforce: true }, + }); + ruleTypeRegistry.get.mockImplementation(() => ({ + id: '123', + name: 'Test', + actionGroups: [{ id: 'default', name: 'Default' }], + recoveryActionGroup: RecoveredActionGroup, + defaultActionGroupId: 'default', + minimumLicenseRequired: 'basic', + isExportable: true, + async executor() {}, + producer: 'alerts', + useSavedObjectReferences: { + extractReferences: jest.fn(), + injectReferences: jest.fn(), + }, + })); + + const data = getMockData({ + notifyWhen: 'onActionGroupChange', + actions: [ + { + group: 'default', + id: '1', + params: { + foo: true, + }, + frequency: { + summary: false, + notifyWhen: 'onActionGroupChange', + throttle: null, + }, + }, + { + group: 'default', + id: '2', + params: { + foo: true, + }, + frequency: { + summary: false, + notifyWhen: 'onActionGroupChange', + throttle: null, + }, + }, + ], + }); + await expect(rulesClient.create({ data })).rejects.toThrowErrorMatchingInlineSnapshot( + `"Cannot specify per-action frequency params when notify_when and throttle are defined at the rule level: default, default"` + ); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); + expect(taskManager.schedule).not.toHaveBeenCalled(); + + const data2 = getMockData({ + notifyWhen: null, + actions: [ + { + group: 'default', + id: '1', + params: { + foo: true, + }, + frequency: { + summary: false, + notifyWhen: 'onActionGroupChange', + throttle: null, + }, + }, + { + group: 'default', + id: '2', + params: { + foo: true, + }, + }, + ], + }); + await expect(rulesClient.create({ data: data2 })).rejects.toThrowErrorMatchingInlineSnapshot( + `"Cannot specify per-action frequency params when notify_when and throttle are defined at the rule level: default"` + ); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); + expect(taskManager.schedule).not.toHaveBeenCalled(); + }); + + test('throws error when neither global frequency nor action frequency are defined', async () => { + rulesClient = new RulesClient({ + ...rulesClientParams, + minimumScheduleInterval: { value: '1m', enforce: true }, + }); + ruleTypeRegistry.get.mockImplementation(() => ({ + id: '123', + name: 'Test', + actionGroups: [{ id: 'default', name: 'Default' }], + recoveryActionGroup: RecoveredActionGroup, + defaultActionGroupId: 'default', + minimumLicenseRequired: 'basic', + isExportable: true, + async executor() {}, + producer: 'alerts', + useSavedObjectReferences: { + extractReferences: jest.fn(), + injectReferences: jest.fn(), + }, + })); + + const data = getMockData({ + notifyWhen: undefined, + throttle: undefined, + actions: [ + { + group: 'default', + id: '1', + params: { + foo: true, + }, + }, + ], + }); + await expect(rulesClient.create({ data })).rejects.toThrowErrorMatchingInlineSnapshot( + `"Actions missing frequency parameters: default"` + ); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); + expect(taskManager.schedule).not.toHaveBeenCalled(); + }); + test('throws error when some actions are missing frequency params', async () => { + rulesClient = new RulesClient({ + ...rulesClientParams, + minimumScheduleInterval: { value: '1m', enforce: true }, + }); + ruleTypeRegistry.get.mockImplementation(() => ({ + id: '123', + name: 'Test', + actionGroups: [{ id: 'default', name: 'Default' }], + recoveryActionGroup: RecoveredActionGroup, + defaultActionGroupId: 'default', + minimumLicenseRequired: 'basic', + isExportable: true, + async executor() {}, + producer: 'alerts', + useSavedObjectReferences: { + extractReferences: jest.fn(), + injectReferences: jest.fn(), + }, + })); + + const data = getMockData({ + notifyWhen: undefined, + throttle: undefined, + actions: [ + { + group: 'default', + id: '1', + params: { + foo: true, + }, + frequency: { + summary: false, + notifyWhen: 'onActionGroupChange', + throttle: null, + }, + }, + { + group: 'default', + id: '2', + params: { + foo: true, + }, + }, + ], + }); + await expect(rulesClient.create({ data })).rejects.toThrowErrorMatchingInlineSnapshot( + `"Actions missing frequency parameters: default"` + ); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); + expect(taskManager.schedule).not.toHaveBeenCalled(); + }); }); diff --git a/x-pack/plugins/alerting/server/rules_client/tests/update.test.ts b/x-pack/plugins/alerting/server/rules_client/tests/update.test.ts index 9071099ed3aa..ae566c107862 100644 --- a/x-pack/plugins/alerting/server/rules_client/tests/update.test.ts +++ b/x-pack/plugins/alerting/server/rules_client/tests/update.test.ts @@ -1652,6 +1652,183 @@ describe('update()', () => { expect(taskManager.bulkUpdateSchedules).not.toHaveBeenCalled(); }); + test('throws error when mixing and matching global and per-action frequency values', async () => { + const alertId = uuid.v4(); + const taskId = uuid.v4(); + + mockApiCalls(alertId, taskId, { interval: '1m' }, { interval: '1m' }); + await expect( + rulesClient.update({ + id: alertId, + data: { + schedule: { interval: '1m' }, + name: 'abc', + tags: ['foo'], + params: { + bar: true, + }, + throttle: null, + notifyWhen: 'onActionGroupChange', + actions: [ + { + group: 'default', + id: '1', + params: { + foo: true, + }, + frequency: { + summary: false, + notifyWhen: 'onActionGroupChange', + throttle: null, + }, + }, + { + group: 'default', + id: '2', + params: { + foo: true, + }, + frequency: { + summary: false, + notifyWhen: 'onActionGroupChange', + throttle: null, + }, + }, + ], + }, + }) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Cannot specify per-action frequency params when notify_when and throttle are defined at the rule level: default, default"` + ); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); + expect(taskManager.schedule).not.toHaveBeenCalled(); + + await expect( + rulesClient.update({ + id: alertId, + data: { + schedule: { interval: '1m' }, + name: 'abc', + tags: ['foo'], + params: { + bar: true, + }, + throttle: null, + notifyWhen: null, + actions: [ + { + group: 'default', + id: '1', + params: { + foo: true, + }, + frequency: { + summary: false, + notifyWhen: 'onActionGroupChange', + throttle: null, + }, + }, + { + group: 'default', + id: '2', + params: { + foo: true, + }, + }, + ], + }, + }) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Cannot specify per-action frequency params when notify_when and throttle are defined at the rule level: default"` + ); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); + expect(taskManager.schedule).not.toHaveBeenCalled(); + }); + + test('throws error when neither global frequency nor action frequency are defined', async () => { + const alertId = uuid.v4(); + const taskId = uuid.v4(); + + mockApiCalls(alertId, taskId, { interval: '1m' }, { interval: '1m' }); + + await expect( + rulesClient.update({ + id: alertId, + data: { + schedule: { interval: '1m' }, + name: 'abc', + tags: ['foo'], + params: { + bar: true, + }, + notifyWhen: undefined, + throttle: undefined, + actions: [ + { + group: 'default', + id: '1', + params: { + foo: true, + }, + }, + ], + }, + }) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Actions missing frequency parameters: default"` + ); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); + expect(taskManager.schedule).not.toHaveBeenCalled(); + }); + + test('throws error when when some actions are missing frequency params', async () => { + const alertId = uuid.v4(); + const taskId = uuid.v4(); + + mockApiCalls(alertId, taskId, { interval: '1m' }, { interval: '1m' }); + + await expect( + rulesClient.update({ + id: alertId, + data: { + schedule: { interval: '1m' }, + name: 'abc', + tags: ['foo'], + params: { + bar: true, + }, + notifyWhen: undefined, + throttle: undefined, + actions: [ + { + group: 'default', + id: '1', + params: { + foo: true, + }, + frequency: { + summary: false, + notifyWhen: 'onActionGroupChange', + throttle: null, + }, + }, + { + group: 'default', + id: '2', + params: { + foo: true, + }, + }, + ], + }, + }) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Actions missing frequency parameters: default"` + ); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); + expect(taskManager.schedule).not.toHaveBeenCalled(); + }); + test('logs when update of schedule of an alerts underlying task fails', async () => { const alertId = uuid.v4(); const taskId = uuid.v4(); diff --git a/x-pack/plugins/alerting/server/task_runner/execution_handler.ts b/x-pack/plugins/alerting/server/task_runner/execution_handler.ts index 7c993879779e..7bb672533433 100644 --- a/x-pack/plugins/alerting/server/task_runner/execution_handler.ts +++ b/x-pack/plugins/alerting/server/task_runner/execution_handler.ts @@ -383,7 +383,7 @@ export class ExecutionHandler< } = this; const muted = mutedAlertIdsSet!.has(alertId); - const throttled = alert.isThrottled(throttle); + const throttled = alert.isThrottled(throttle ?? null); if (muted) { if ( diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner.ts b/x-pack/plugins/alerting/server/task_runner/task_runner.ts index a06bd01cb9ce..3fc70537f1bc 100644 --- a/x-pack/plugins/alerting/server/task_runner/task_runner.ts +++ b/x-pack/plugins/alerting/server/task_runner/task_runner.ts @@ -218,8 +218,8 @@ export class TaskRunner< alertTypeId: ruleTypeId, consumer, schedule, - throttle, - notifyWhen, + throttle = null, + notifyWhen = null, name, tags, createdBy, diff --git a/x-pack/plugins/alerting/server/types.ts b/x-pack/plugins/alerting/server/types.ts index a7244e6a02c5..c2af1555cfa3 100644 --- a/x-pack/plugins/alerting/server/types.ts +++ b/x-pack/plugins/alerting/server/types.ts @@ -209,6 +209,11 @@ export interface RawRuleAction extends SavedObjectAttributes { actionRef: string; actionTypeId: string; params: RuleActionParams; + frequency?: { + summary: boolean; + notifyWhen: RuleNotifyWhenType; + throttle: string | null; + }; } export interface RuleMeta extends SavedObjectAttributes { @@ -268,8 +273,8 @@ export interface RawRule extends SavedObjectAttributes { updatedAt: string; apiKey: string | null; apiKeyOwner: string | null; - throttle: string | null; - notifyWhen: RuleNotifyWhenType | null; + throttle?: string | null; + notifyWhen?: RuleNotifyWhenType | null; muteAll: boolean; mutedInstanceIds: string[]; meta?: RuleMeta; diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_converters.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_converters.ts index ec3dcccf56ea..e15c88ccf3aa 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_converters.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_converters.ts @@ -467,10 +467,10 @@ export const convertPatchAPIToInternalSchema = ( : existingRule.actions, throttle: nextParams.throttle ? transformToAlertThrottle(nextParams.throttle) - : existingRule.throttle, + : existingRule.throttle ?? null, notifyWhen: nextParams.throttle ? transformToNotifyWhen(nextParams.throttle) - : existingRule.notifyWhen, + : existingRule.notifyWhen ?? null, }; }; diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_schema/model/rule_schemas.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_schema/model/rule_schemas.ts index 5da3f3749da5..1be28852dade 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_schema/model/rule_schemas.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_schema/model/rule_schemas.ts @@ -284,7 +284,7 @@ export const allRuleTypes = t.union([ t.literal(NEW_TERMS_RULE_TYPE_ID), ]); -export const internalRuleCreate = t.type({ +const internalRuleCreateRequired = t.type({ name: RuleName, tags: RuleTagArray, alertTypeId: allRuleTypes, @@ -295,12 +295,18 @@ export const internalRuleCreate = t.type({ enabled: IsRuleEnabled, actions: RuleActionArrayCamel, params: ruleParams, +}); +const internalRuleCreateOptional = t.partial({ throttle: t.union([RuleActionThrottle, t.null]), notifyWhen, }); +export const internalRuleCreate = t.intersection([ + internalRuleCreateOptional, + internalRuleCreateRequired, +]); export type InternalRuleCreate = t.TypeOf; -export const internalRuleUpdate = t.type({ +const internalRuleUpdateRequired = t.type({ name: RuleName, tags: RuleTagArray, schedule: t.type({ @@ -308,7 +314,13 @@ export const internalRuleUpdate = t.type({ }), actions: RuleActionArrayCamel, params: ruleParams, +}); +const internalRuleUpdateOptional = t.partial({ throttle: t.union([RuleActionThrottle, t.null]), notifyWhen, }); +export const internalRuleUpdate = t.intersection([ + internalRuleUpdateOptional, + internalRuleUpdateRequired, +]); export type InternalRuleUpdate = t.TypeOf;