From 2ae41a570b12bfb755a343124517890692e4e562 Mon Sep 17 00:00:00 2001 From: Jiawei Wu Date: Mon, 9 Sep 2024 20:08:24 -0700 Subject: [PATCH] Address comments --- .../validation/validate_flapping/v1.test.ts | 57 ++++++++ .../rule/methods/update/update_rule.ts | 11 +- ...orm_rule_attributes_to_rule_domain.test.ts | 11 ++ .../transform_rule_domain_to_rule.test.ts | 16 +++ ...orm_rule_domain_to_rule_attributes.test.ts | 122 ++++++++++++++++++ .../tests/alerting/group1/create.ts | 2 +- .../tests/alerting/group2/update.ts | 48 ++++++- 7 files changed, 258 insertions(+), 9 deletions(-) create mode 100644 x-pack/plugins/alerting/common/routes/rule/validation/validate_flapping/v1.test.ts create mode 100644 x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_domain_to_rule_attributes.test.ts diff --git a/x-pack/plugins/alerting/common/routes/rule/validation/validate_flapping/v1.test.ts b/x-pack/plugins/alerting/common/routes/rule/validation/validate_flapping/v1.test.ts new file mode 100644 index 0000000000000..de5ef6acf7390 --- /dev/null +++ b/x-pack/plugins/alerting/common/routes/rule/validation/validate_flapping/v1.test.ts @@ -0,0 +1,57 @@ +/* + * 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 { validateFlapping } from './v1'; + +describe('validateFlapping', () => { + test('should error if look back window exceeds the lower bound', () => { + const result = validateFlapping({ + look_back_window: 0, + status_change_threshold: 10, + }); + + expect(result).toEqual('look back window must be between 2 and 20'); + }); + + test('should error if look back window exceeds the upper bound', () => { + const result = validateFlapping({ + look_back_window: 50, + status_change_threshold: 10, + }); + + expect(result).toEqual('look back window must be between 2 and 20'); + }); + + test('should error if status change threshold exceeds the lower bound', () => { + const result = validateFlapping({ + look_back_window: 10, + status_change_threshold: 1, + }); + + expect(result).toEqual('status change threshold must be between 2 and 20'); + }); + + test('should error if status change threshold exceeds the upper bound', () => { + const result = validateFlapping({ + look_back_window: 10, + status_change_threshold: 50, + }); + + expect(result).toEqual('status change threshold must be between 2 and 20'); + }); + + test('should error if status change threshold is greater than the look back window', () => { + const result = validateFlapping({ + look_back_window: 10, + status_change_threshold: 15, + }); + + expect(result).toEqual( + 'lookBackWindow (10) must be equal to or greater than statusChangeThreshold (15)' + ); + }); +}); diff --git a/x-pack/plugins/alerting/server/application/rule/methods/update/update_rule.ts b/x-pack/plugins/alerting/server/application/rule/methods/update/update_rule.ts index 885b5ed12769b..5908c16d61855 100644 --- a/x-pack/plugins/alerting/server/application/rule/methods/update/update_rule.ts +++ b/x-pack/plugins/alerting/server/application/rule/methods/update/update_rule.ts @@ -51,6 +51,11 @@ const validateCanUpdateFlapping = ( return; } + // If updated flapping is undefined then don't do anything, it's not being updated + if (updateFlapping === undefined) { + return; + } + // If both versions are falsy, allow it even if its changing between undefined and null if (!originalFlapping && !updateFlapping) { return; @@ -154,12 +159,6 @@ async function updateWithOCC( validateCanUpdateFlapping(isFlappingEnabled, originalFlapping, initialData.flapping); - if (!isFlappingEnabled && !isEqual(originalFlapping, initialData.flapping)) { - throw Boom.badRequest( - `Error updating rule: can not update rule flapping if global flapping is disabled` - ); - } - let validationPayload: ValidateScheduleLimitResult = null; if (enabled && schedule.interval !== data.schedule.interval) { validationPayload = await validateScheduleLimit({ diff --git a/x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_attributes_to_rule_domain.test.ts b/x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_attributes_to_rule_domain.test.ts index 7b6fcb18ae219..6a4e1824172ca 100644 --- a/x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_attributes_to_rule_domain.test.ts +++ b/x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_attributes_to_rule_domain.test.ts @@ -92,6 +92,10 @@ describe('transformRuleAttributesToRuleDomain', () => { updatedBy: 'user', apiKey: MOCK_API_KEY, apiKeyOwner: 'user', + flapping: { + lookBackWindow: 20, + statusChangeThreshold: 20, + }, }; it('transforms the actions correctly', () => { @@ -106,6 +110,13 @@ describe('transformRuleAttributesToRuleDomain', () => { isSystemAction ); + expect(res.flapping).toMatchInlineSnapshot(` + Object { + "lookBackWindow": 20, + "statusChangeThreshold": 20, + } + `); + expect(res.actions).toMatchInlineSnapshot(` Array [ Object { diff --git a/x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_domain_to_rule.test.ts b/x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_domain_to_rule.test.ts index 9789e3603385c..dba50d17612ad 100644 --- a/x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_domain_to_rule.test.ts +++ b/x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_domain_to_rule.test.ts @@ -67,6 +67,10 @@ describe('transformRuleDomainToRule', () => { updatedBy: 'user', apiKey: MOCK_API_KEY, apiKeyOwner: 'user', + flapping: { + lookBackWindow: 20, + statusChangeThreshold: 20, + }, }; it('should transform rule domain to rule', () => { @@ -100,6 +104,10 @@ describe('transformRuleDomainToRule', () => { revision: 0, updatedBy: 'user', apiKeyOwner: 'user', + flapping: { + lookBackWindow: 20, + statusChangeThreshold: 20, + }, }); }); @@ -135,6 +143,10 @@ describe('transformRuleDomainToRule', () => { revision: 0, updatedBy: 'user', apiKeyOwner: 'user', + flapping: { + lookBackWindow: 20, + statusChangeThreshold: 20, + }, }); }); @@ -172,6 +184,10 @@ describe('transformRuleDomainToRule', () => { revision: 0, updatedBy: 'user', apiKeyOwner: 'user', + flapping: { + lookBackWindow: 20, + statusChangeThreshold: 20, + }, }); }); }); diff --git a/x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_domain_to_rule_attributes.test.ts b/x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_domain_to_rule_attributes.test.ts new file mode 100644 index 0000000000000..73c39532b9ab9 --- /dev/null +++ b/x-pack/plugins/alerting/server/application/rule/transforms/transform_rule_domain_to_rule_attributes.test.ts @@ -0,0 +1,122 @@ +/* + * 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 { RuleDomain } from '../types'; +import { transformRuleDomainToRuleAttributes } from './transform_rule_domain_to_rule_attributes'; + +describe('transformRuleDomainToRuleAttributes', () => { + const MOCK_API_KEY = Buffer.from('123:abc').toString('base64'); + + const defaultAction = { + id: '1', + uuid: 'test-uuid', + group: 'default', + actionTypeId: 'test', + params: {}, + }; + + const rule: RuleDomain<{}> = { + id: 'test', + enabled: false, + name: 'my rule name', + tags: ['foo'], + alertTypeId: 'myType', + consumer: 'myApp', + schedule: { interval: '1m' }, + actions: [defaultAction], + params: {}, + mapped_params: {}, + createdBy: 'user', + createdAt: new Date('2019-02-12T21:01:22.479Z'), + updatedAt: new Date('2019-02-12T21:01:22.479Z'), + legacyId: 'legacyId', + muteAll: false, + mutedInstanceIds: [], + snoozeSchedule: [], + scheduledTaskId: 'task-123', + executionStatus: { + lastExecutionDate: new Date('2019-02-12T21:01:22.479Z'), + status: 'pending' as const, + }, + throttle: null, + notifyWhen: null, + revision: 0, + updatedBy: 'user', + apiKey: MOCK_API_KEY, + apiKeyOwner: 'user', + flapping: { + lookBackWindow: 20, + statusChangeThreshold: 20, + }, + }; + + test('should transform rule domain to rule attribute', () => { + const result = transformRuleDomainToRuleAttributes({ + rule, + actionsWithRefs: [ + { + group: 'default', + actionRef: 'action_0', + actionTypeId: 'test', + uuid: 'test-uuid', + params: {}, + }, + ], + params: { + legacyId: 'test', + paramsWithRefs: {}, + }, + }); + + expect(result).toMatchInlineSnapshot(` + Object { + "actions": Array [ + Object { + "actionRef": "action_0", + "actionTypeId": "test", + "group": "default", + "params": Object {}, + "uuid": "test-uuid", + }, + ], + "alertTypeId": "myType", + "apiKey": "MTIzOmFiYw==", + "apiKeyOwner": "user", + "consumer": "myApp", + "createdAt": "2019-02-12T21:01:22.479Z", + "createdBy": "user", + "enabled": false, + "executionStatus": Object { + "lastExecutionDate": "2019-02-12T21:01:22.479Z", + "status": "pending", + }, + "flapping": Object { + "lookBackWindow": 20, + "statusChangeThreshold": 20, + }, + "legacyId": "test", + "muteAll": false, + "mutedInstanceIds": Array [], + "name": "my rule name", + "notifyWhen": null, + "params": Object {}, + "revision": 0, + "schedule": Object { + "interval": "1m", + }, + "scheduledTaskId": "task-123", + "snoozeSchedule": Array [], + "tags": Array [ + "foo", + ], + "throttle": null, + "updatedAt": "2019-02-12T21:01:22.479Z", + "updatedBy": "user", + } + `); + }); +}); diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group1/create.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group1/create.ts index b66fa7e9d15d7..ca680a8a6bf70 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group1/create.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group1/create.ts @@ -636,7 +636,7 @@ export default function createAlertTests({ getService }: FtrProviderContext) { await resetRulesSettings(supertest, 'space1'); }); - it('should allow flapping to be updated', async () => { + it('should allow flapping to be created', async () => { const response = await supertest .post(`${getUrlPrefix(Spaces.space1.id)}/api/alerting/rule`) .set('kbn-xsrf', 'foo') diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group2/update.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group2/update.ts index a812d650e58a6..0a618d9f87e7e 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group2/update.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group2/update.ts @@ -294,13 +294,53 @@ export default function createUpdateTests({ getService }: FtrProviderContext) { }, }) .expect(400); + }); + + it('should allow rule to be updated when global flapping is off if not updating flapping', async () => { + const response = await supertest + .post(`${getUrlPrefix(Spaces.space1.id)}/api/alerting/rule`) + .set('kbn-xsrf', 'foo') + .send( + getTestRuleData({ + flapping: { + look_back_window: 5, + status_change_threshold: 5, + }, + }) + ); + + objectRemover.add(Spaces.space1.id, response.body.id, 'rule', 'alerting'); + + await supertest + .post(`${getUrlPrefix(Spaces.space1.id)}/internal/alerting/rules/settings/_flapping`) + .set('kbn-xsrf', 'foo') + .send({ + enabled: false, + look_back_window: 5, + status_change_threshold: 5, + }); - // Can still update when global flapping is off if not updating flapping await supertest .put(`${getUrlPrefix(Spaces.space1.id)}/api/alerting/rule/${response.body.id}`) .set('kbn-xsrf', 'foo') .send({ - name: 'bcd', + name: 'updated name 1', + tags: ['foo'], + params: { + foo: true, + }, + schedule: { interval: '12s' }, + actions: [], + throttle: '1m', + notify_when: 'onThrottleInterval', + }) + .expect(200); + + await supertest + .put(`${getUrlPrefix(Spaces.space1.id)}/api/alerting/rule/${response.body.id}`) + .set('kbn-xsrf', 'foo') + .send({ + name: 'updated name 2', tags: ['foo'], params: { foo: true, @@ -309,6 +349,10 @@ export default function createUpdateTests({ getService }: FtrProviderContext) { actions: [], throttle: '1m', notify_when: 'onThrottleInterval', + flapping: { + look_back_window: 5, + status_change_threshold: 5, + }, }) .expect(200); });