From e007ad6df510629da6e501ed55247496915f213f Mon Sep 17 00:00:00 2001 From: Ying Mao Date: Mon, 3 Oct 2022 23:28:11 -0400 Subject: [PATCH] [Response Ops][Alerting] Fixing bug with using `runSoon` on pre-8.x rule (#142505) * Running task using scheduled task id. Adding functional test * dont run if rule is disable * Fixing i18n --- .../server/rules_client/rules_client.ts | 41 ++++++- .../rules_client/tests/run_soon.test.ts | 32 +++++- .../spaces_only/tests/alerting/index.ts | 1 + .../spaces_only/tests/alerting/run_soon.ts | 104 ++++++++++++++++++ 4 files changed, 171 insertions(+), 7 deletions(-) create mode 100644 x-pack/test/alerting_api_integration/spaces_only/tests/alerting/run_soon.ts 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 89ce20b59ae9b..7374868a11dbb 100644 --- a/x-pack/plugins/alerting/server/rules_client/rules_client.ts +++ b/x-pack/plugins/alerting/server/rules_client/rules_client.ts @@ -41,7 +41,11 @@ import { InvalidateAPIKeyResult as SecurityPluginInvalidateAPIKeyResult, } from '@kbn/security-plugin/server'; import { EncryptedSavedObjectsClient } from '@kbn/encrypted-saved-objects-plugin/server'; -import { TaskManagerStartContract, TaskStatus } from '@kbn/task-manager-plugin/server'; +import { + ConcreteTaskInstance, + TaskManagerStartContract, + TaskStatus, +} from '@kbn/task-manager-plugin/server'; import { IEvent, IEventLogClient, @@ -2977,9 +2981,27 @@ export class RulesClient { this.ruleTypeRegistry.ensureRuleTypeEnabled(attributes.alertTypeId); - const taskDoc = attributes.scheduledTaskId - ? await this.taskManager.get(attributes.scheduledTaskId) - : null; + // Check that the rule is enabled + if (!attributes.enabled) { + return i18n.translate('xpack.alerting.rulesClient.runSoon.disabledRuleError', { + defaultMessage: 'Error running rule: rule is disabled', + }); + } + + let taskDoc: ConcreteTaskInstance | null = null; + try { + taskDoc = attributes.scheduledTaskId + ? await this.taskManager.get(attributes.scheduledTaskId) + : null; + } catch (err) { + return i18n.translate('xpack.alerting.rulesClient.runSoon.getTaskError', { + defaultMessage: 'Error running rule: {errMessage}', + values: { + errMessage: err.message, + }, + }); + } + if ( taskDoc && (taskDoc.status === TaskStatus.Claiming || taskDoc.status === TaskStatus.Running) @@ -2989,7 +3011,16 @@ export class RulesClient { }); } - await this.taskManager.runSoon(id); + try { + await this.taskManager.runSoon(attributes.scheduledTaskId ? attributes.scheduledTaskId : id); + } catch (err) { + return i18n.translate('xpack.alerting.rulesClient.runSoon.runSoonError', { + defaultMessage: 'Error running rule: {errMessage}', + values: { + errMessage: err.message, + }, + }); + } } public async listAlertTypes() { diff --git a/x-pack/plugins/alerting/server/rules_client/tests/run_soon.test.ts b/x-pack/plugins/alerting/server/rules_client/tests/run_soon.test.ts index bea66a31ead25..138b167549c09 100644 --- a/x-pack/plugins/alerting/server/rules_client/tests/run_soon.test.ts +++ b/x-pack/plugins/alerting/server/rules_client/tests/run_soon.test.ts @@ -59,7 +59,7 @@ describe('runSoon()', () => { consumer: 'myApp', schedule: { interval: '10s' }, alertTypeId: 'myType', - enabled: false, + enabled: true, apiKey: 'MTIzOmFiYw==', apiKeyOwner: 'elastic', actions: [ @@ -179,7 +179,7 @@ describe('runSoon()', () => { }); test('does not run a rule if that rule is already running', async () => { - taskManager.get.mockResolvedValue({ + taskManager.get.mockResolvedValueOnce({ id: '1', scheduledAt: new Date(), attempts: 0, @@ -196,4 +196,32 @@ describe('runSoon()', () => { expect(message).toBe('Rule is already running'); expect(taskManager.runSoon).not.toHaveBeenCalled(); }); + + test('does not run a rule if that rule is disabled', async () => { + unsecuredSavedObjectsClient.get.mockResolvedValue({ + ...existingRule, + attributes: { + ...existingRule.attributes, + enabled: false, + }, + }); + const message = await rulesClient.runSoon({ id: '1' }); + expect(message).toBe('Error running rule: rule is disabled'); + expect(taskManager.get).not.toHaveBeenCalled(); + expect(taskManager.runSoon).not.toHaveBeenCalled(); + }); + + test('gracefully handles errors getting task document', async () => { + taskManager.get.mockRejectedValueOnce(new Error('oh no!')); + const message = await rulesClient.runSoon({ id: '1' }); + expect(message).toBe('Error running rule: oh no!'); + expect(taskManager.runSoon).not.toHaveBeenCalled(); + }); + + test('gracefully handles errors calling runSoon', async () => { + taskManager.runSoon.mockRejectedValueOnce(new Error('fail!')); + const message = await rulesClient.runSoon({ id: '1' }); + expect(message).toBe('Error running rule: fail!'); + expect(taskManager.runSoon).toHaveBeenCalled(); + }); }); diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/index.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/index.ts index 49e652d9a9a4e..e09cf3121adec 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/index.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/index.ts @@ -49,6 +49,7 @@ export default function alertingTests({ loadTestFile, getService }: FtrProviderC loadTestFile(require.resolve('./bulk_edit')); loadTestFile(require.resolve('./capped_action_type')); loadTestFile(require.resolve('./scheduled_task_id')); + loadTestFile(require.resolve('./run_soon')); // Do not place test files here, due to https://github.com/elastic/kibana/issues/123059 // note that this test will destroy existing spaces diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/run_soon.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/run_soon.ts new file mode 100644 index 0000000000000..050c220ab1b0f --- /dev/null +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/run_soon.ts @@ -0,0 +1,104 @@ +/* + * 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 expect from '@kbn/expect'; +import { getUrlPrefix, getTestRuleData, ObjectRemover } from '../../../common/lib'; +import { FtrProviderContext } from '../../../common/ftr_provider_context'; + +const LOADED_RULE_ID = '74f3e6d7-b7bb-477d-ac28-92ee22728e6e'; + +// eslint-disable-next-line import/no-default-export +export default function createRunSoonTests({ getService }: FtrProviderContext) { + const supertest = getService('supertest'); + const retry = getService('retry'); + const es = getService('es'); + const esArchiver = getService('esArchiver'); + + describe('runSoon', () => { + const objectRemover = new ObjectRemover(supertest); + + before(async () => { + await esArchiver.load('x-pack/test/functional/es_archives/rules_scheduled_task_id'); + }); + + afterEach(async () => { + await objectRemover.removeAll(); + }); + + after(async () => { + await esArchiver.unload('x-pack/test/functional/es_archives/rules_scheduled_task_id'); + }); + + it('should successfully run rule where scheduled task id is different than rule id', async () => { + await retry.try(async () => { + // Sometimes the rule may already be running. Try until it isn't + const response = await supertest + .post(`${getUrlPrefix(``)}/internal/alerting/rule/${LOADED_RULE_ID}/_run_soon`) + .set('kbn-xsrf', 'foo'); + expect(response.status).to.eql(204); + }); + }); + + it('should successfully run rule where scheduled task id is same as rule id', async () => { + const response = await supertest + .post(`${getUrlPrefix(``)}/api/alerting/rule`) + .set('kbn-xsrf', 'foo') + .send(getTestRuleData()); + + expect(response.status).to.eql(200); + objectRemover.add('default', response.body.id, 'rule', 'alerting'); + + const runSoonResponse = await supertest + .post(`${getUrlPrefix(``)}/internal/alerting/rule/${response.body.id}/_run_soon`) + .set('kbn-xsrf', 'foo'); + expect(runSoonResponse.status).to.eql(204); + }); + + it('should return message when task does not exist for rule', async () => { + const response = await supertest + .post(`${getUrlPrefix(``)}/api/alerting/rule`) + .set('kbn-xsrf', 'foo') + .send(getTestRuleData()); + + expect(response.status).to.eql(200); + objectRemover.add('default', response.body.id, 'rule', 'alerting'); + + await es.delete({ + id: `task:${response.body.id}`, + index: '.kibana_task_manager', + }); + + const runSoonResponse = await supertest + .post(`${getUrlPrefix(``)}/internal/alerting/rule/${response.body.id}/_run_soon`) + .set('kbn-xsrf', 'foo'); + expect(runSoonResponse.status).to.eql(200); + expect(runSoonResponse.text).to.eql( + `Error running rule: Saved object [task/${response.body.id}] not found` + ); + }); + + it('should return message when rule is disabled', async () => { + const response = await supertest + .post(`${getUrlPrefix(``)}/api/alerting/rule`) + .set('kbn-xsrf', 'foo') + .send(getTestRuleData()); + + expect(response.status).to.eql(200); + objectRemover.add('default', response.body.id, 'rule', 'alerting'); + + await supertest + .post(`${getUrlPrefix(``)}/api/alerting/rule/${response.body.id}/_disable`) + .set('kbn-xsrf', 'foo'); + + const runSoonResponse = await supertest + .post(`${getUrlPrefix(``)}/internal/alerting/rule/${response.body.id}/_run_soon`) + .set('kbn-xsrf', 'foo'); + expect(runSoonResponse.status).to.eql(200); + expect(runSoonResponse.text).to.eql(`Error running rule: rule is disabled`); + }); + }); +}