From fd217a39fad9394cb9fedaa7cfe1057c286a6615 Mon Sep 17 00:00:00 2001 From: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Date: Sat, 11 Mar 2023 00:12:52 -0500 Subject: [PATCH] [8.7] [Response Ops][Alerting] Delete `unrecognized` tasks when enabling a rule (#152975) (#153152) # Backport This will backport the following commits from `main` to `8.7`: - [[Response Ops][Alerting] Delete `unrecognized` tasks when enabling a rule (#152975)](https://github.com/elastic/kibana/pull/152975) ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) Co-authored-by: Ying Mao --- .../rules_client/methods/bulk_enable.ts | 18 +- .../server/rules_client/methods/enable.ts | 10 +- .../rules_client/tests/bulk_enable.test.ts | 242 +++++++++++++++++- .../server/rules_client/tests/enable.test.ts | 47 ++++ .../alerting/server/rules_client/tests/lib.ts | 17 ++ .../alerting/group4/scheduled_task_id.ts | 32 +++ .../rules_scheduled_task_id/data.json | 77 +++++- 7 files changed, 435 insertions(+), 8 deletions(-) diff --git a/x-pack/plugins/alerting/server/rules_client/methods/bulk_enable.ts b/x-pack/plugins/alerting/server/rules_client/methods/bulk_enable.ts index 4ce189c6caca6..03643ccc02ac2 100644 --- a/x-pack/plugins/alerting/server/rules_client/methods/bulk_enable.ts +++ b/x-pack/plugins/alerting/server/rules_client/methods/bulk_enable.ts @@ -10,7 +10,7 @@ import { KueryNode, nodeBuilder } from '@kbn/es-query'; import { SavedObjectsBulkUpdateObject } from '@kbn/core/server'; import { withSpan } from '@kbn/apm-utils'; import { Logger } from '@kbn/core/server'; -import { TaskManagerStartContract } from '@kbn/task-manager-plugin/server'; +import { TaskManagerStartContract, TaskStatus } from '@kbn/task-manager-plugin/server'; import { RawRule, IntervalSchedule } from '../../types'; import { convertRuleIdsToKueryNode } from '../../lib'; import { ruleAuditEvent, RuleAuditAction } from '../common/audit_events'; @@ -36,10 +36,18 @@ const getShouldScheduleTask = async ( if (!scheduledTaskId) return true; try { // make sure scheduledTaskId exist - await withSpan({ name: 'getShouldScheduleTask', type: 'rules' }, () => - context.taskManager.get(scheduledTaskId) - ); - return false; + return await withSpan({ name: 'getShouldScheduleTask', type: 'rules' }, async () => { + const task = await context.taskManager.get(scheduledTaskId); + + // Check whether task status is unrecognized. If so, we want to delete + // this task and create a fresh one + if (task.status === TaskStatus.Unrecognized) { + await context.taskManager.removeIfExists(scheduledTaskId); + return true; + } + + return false; + }); } catch (err) { return true; } diff --git a/x-pack/plugins/alerting/server/rules_client/methods/enable.ts b/x-pack/plugins/alerting/server/rules_client/methods/enable.ts index 1aee25fa5adbc..6e93dee530665 100644 --- a/x-pack/plugins/alerting/server/rules_client/methods/enable.ts +++ b/x-pack/plugins/alerting/server/rules_client/methods/enable.ts @@ -5,6 +5,7 @@ * 2.0. */ +import { TaskStatus } from '@kbn/task-manager-plugin/server'; import { RawRule, IntervalSchedule } from '../../types'; import { resetMonitoringLastRun, getNextRun } from '../../lib'; import { WriteOperations, AlertingAuthorizationEntity } from '../../authorization'; @@ -110,7 +111,14 @@ async function enableWithOCC(context: RulesClientContext, { id }: { id: string } if (attributes.scheduledTaskId) { // If scheduledTaskId defined in rule SO, make sure it exists try { - await context.taskManager.get(attributes.scheduledTaskId); + const task = await context.taskManager.get(attributes.scheduledTaskId); + + // Check whether task status is unrecognized. If so, we want to delete + // this task and create a fresh one + if (task.status === TaskStatus.Unrecognized) { + await context.taskManager.removeIfExists(attributes.scheduledTaskId); + scheduledTaskIdToCreate = id; + } } catch (err) { scheduledTaskIdToCreate = id; } diff --git a/x-pack/plugins/alerting/server/rules_client/tests/bulk_enable.test.ts b/x-pack/plugins/alerting/server/rules_client/tests/bulk_enable.test.ts index eaa302e41ac85..0aeca36a2b458 100644 --- a/x-pack/plugins/alerting/server/rules_client/tests/bulk_enable.test.ts +++ b/x-pack/plugins/alerting/server/rules_client/tests/bulk_enable.test.ts @@ -30,6 +30,7 @@ import { returnedRule1, returnedRule2, } from './test_helpers'; +import { TaskStatus } from '@kbn/task-manager-plugin/server'; jest.mock('../../invalidate_pending_api_keys/bulk_mark_api_keys_for_invalidation', () => ({ bulkMarkApiKeysForInvalidation: jest.fn(), @@ -379,7 +380,7 @@ describe('bulkEnableRules', () => { }); describe('taskManager', () => { - test('should return task id if deleting task failed', async () => { + test('should return task id if enabling task failed', async () => { unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({ saved_objects: [enabledRule1, enabledRule2], }); @@ -487,6 +488,245 @@ describe('bulkEnableRules', () => { ); expect(logger.error).toBeCalledTimes(0); }); + + test('should schedule task when scheduledTaskId is defined but task with that ID does not', async () => { + // One rule gets the task successfully, one rule doesn't so only one task should be scheduled + taskManager.get.mockRejectedValueOnce(new Error('Failed to get task!')); + taskManager.schedule.mockResolvedValueOnce({ + id: 'id1', + taskType: 'alerting:fakeType', + scheduledAt: new Date(), + attempts: 1, + status: TaskStatus.Idle, + runAt: new Date(), + startedAt: null, + retryAt: null, + state: {}, + params: {}, + ownerId: null, + }); + unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({ + saved_objects: [enabledRule1, enabledRule2], + }); + + const result = await rulesClient.bulkEnableRules({ ids: ['id1', 'id2'] }); + + expect(taskManager.schedule).toHaveBeenCalledTimes(1); + expect(taskManager.schedule).toHaveBeenCalledWith({ + id: 'id1', + taskType: `alerting:fakeType`, + params: { + alertId: 'id1', + spaceId: 'default', + consumer: 'fakeConsumer', + }, + schedule: { + interval: '5m', + }, + enabled: true, + state: { + alertInstances: {}, + alertTypeState: {}, + previousStartedAt: null, + }, + scope: ['alerting'], + }); + + expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + id: 'id1', + attributes: expect.objectContaining({ + enabled: true, + }), + }), + expect.objectContaining({ + id: 'id2', + attributes: expect.objectContaining({ + enabled: true, + }), + }), + ]), + { overwrite: true } + ); + + expect(result).toStrictEqual({ + errors: [], + rules: [returnedRule1, returnedRule2], + total: 2, + taskIdsFailedToBeEnabled: [], + }); + }); + + test('should schedule task when scheduledTaskId is not defined', async () => { + encryptedSavedObjects.createPointInTimeFinderDecryptedAsInternalUser = jest + .fn() + .mockResolvedValueOnce({ + close: jest.fn(), + find: function* asyncGenerator() { + yield { + saved_objects: [ + { + ...disabledRule1, + attributes: { ...disabledRule1.attributes, scheduledTaskId: null }, + }, + disabledRule2, + ], + }; + }, + }); + taskManager.schedule.mockResolvedValueOnce({ + id: 'id1', + taskType: 'alerting:fakeType', + scheduledAt: new Date(), + attempts: 1, + status: TaskStatus.Idle, + runAt: new Date(), + startedAt: null, + retryAt: null, + state: {}, + params: {}, + ownerId: null, + }); + unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({ + saved_objects: [enabledRule1, enabledRule2], + }); + const result = await rulesClient.bulkEnableRules({ ids: ['id1', 'id2'] }); + + expect(taskManager.schedule).toHaveBeenCalledTimes(1); + expect(taskManager.schedule).toHaveBeenCalledWith({ + id: 'id1', + taskType: `alerting:fakeType`, + params: { + alertId: 'id1', + spaceId: 'default', + consumer: 'fakeConsumer', + }, + schedule: { + interval: '5m', + }, + enabled: true, + state: { + alertInstances: {}, + alertTypeState: {}, + previousStartedAt: null, + }, + scope: ['alerting'], + }); + + expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + id: 'id1', + attributes: expect.objectContaining({ + enabled: true, + }), + }), + expect.objectContaining({ + id: 'id2', + attributes: expect.objectContaining({ + enabled: true, + }), + }), + ]), + { overwrite: true } + ); + + expect(result).toStrictEqual({ + errors: [], + rules: [returnedRule1, returnedRule2], + total: 2, + taskIdsFailedToBeEnabled: [], + }); + }); + + test('should schedule task when task with scheduledTaskId exists but is unrecognized', async () => { + taskManager.get.mockResolvedValueOnce({ + id: 'task-123', + taskType: 'alerting:123', + scheduledAt: new Date(), + attempts: 1, + status: TaskStatus.Unrecognized, + runAt: new Date(), + startedAt: null, + retryAt: null, + state: {}, + params: { + alertId: '1', + }, + ownerId: null, + enabled: false, + }); + taskManager.schedule.mockResolvedValueOnce({ + id: 'id1', + taskType: 'alerting:fakeType', + scheduledAt: new Date(), + attempts: 1, + status: TaskStatus.Idle, + runAt: new Date(), + startedAt: null, + retryAt: null, + state: {}, + params: {}, + ownerId: null, + }); + unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({ + saved_objects: [enabledRule1, enabledRule2], + }); + + const result = await rulesClient.bulkEnableRules({ ids: ['id1', 'id2'] }); + + expect(taskManager.removeIfExists).toHaveBeenCalledTimes(1); + expect(taskManager.removeIfExists).toHaveBeenCalledWith('id1'); + expect(taskManager.schedule).toHaveBeenCalledTimes(1); + expect(taskManager.schedule).toHaveBeenCalledWith({ + id: 'id1', + taskType: `alerting:fakeType`, + params: { + alertId: 'id1', + spaceId: 'default', + consumer: 'fakeConsumer', + }, + schedule: { + interval: '5m', + }, + enabled: true, + state: { + alertInstances: {}, + alertTypeState: {}, + previousStartedAt: null, + }, + scope: ['alerting'], + }); + + expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + id: 'id1', + attributes: expect.objectContaining({ + enabled: true, + }), + }), + expect.objectContaining({ + id: 'id2', + attributes: expect.objectContaining({ + enabled: true, + }), + }), + ]), + { overwrite: true } + ); + + expect(result).toStrictEqual({ + errors: [], + rules: [returnedRule1, returnedRule2], + total: 2, + taskIdsFailedToBeEnabled: [], + }); + }); }); describe('auditLogger', () => { diff --git a/x-pack/plugins/alerting/server/rules_client/tests/enable.test.ts b/x-pack/plugins/alerting/server/rules_client/tests/enable.test.ts index 9cb6c356edaed..13826b720fa76 100644 --- a/x-pack/plugins/alerting/server/rules_client/tests/enable.test.ts +++ b/x-pack/plugins/alerting/server/rules_client/tests/enable.test.ts @@ -536,6 +536,53 @@ describe('enable()', () => { }); }); + test('schedules task when task with scheduledTaskId exists but is unrecognized', async () => { + taskManager.schedule.mockResolvedValueOnce({ + id: '1', + taskType: 'alerting:123', + scheduledAt: new Date(), + attempts: 1, + status: TaskStatus.Idle, + runAt: new Date(), + startedAt: null, + retryAt: null, + state: {}, + params: {}, + ownerId: null, + }); + taskManager.get.mockResolvedValue({ ...mockTask, status: TaskStatus.Unrecognized }); + await rulesClient.enable({ id: '1' }); + expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled(); + expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { + namespace: 'default', + }); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(2); + expect(taskManager.bulkEnable).not.toHaveBeenCalled(); + expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123'); + expect(taskManager.schedule).toHaveBeenCalledWith({ + id: '1', + taskType: `alerting:myType`, + params: { + alertId: '1', + spaceId: 'default', + consumer: 'myApp', + }, + schedule: { + interval: '10s', + }, + enabled: true, + state: { + alertInstances: {}, + alertTypeState: {}, + previousStartedAt: null, + }, + scope: ['alerting'], + }); + expect(unsecuredSavedObjectsClient.update).toHaveBeenNthCalledWith(2, 'alert', '1', { + scheduledTaskId: '1', + }); + }); + test('throws error when scheduling task fails', async () => { encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ ...existingRule, diff --git a/x-pack/plugins/alerting/server/rules_client/tests/lib.ts b/x-pack/plugins/alerting/server/rules_client/tests/lib.ts index 7a17c2dc105ad..6328708f4bae0 100644 --- a/x-pack/plugins/alerting/server/rules_client/tests/lib.ts +++ b/x-pack/plugins/alerting/server/rules_client/tests/lib.ts @@ -9,6 +9,7 @@ import { taskManagerMock } from '@kbn/task-manager-plugin/server/mocks'; import { IEventLogClient } from '@kbn/event-log-plugin/server'; import { actionsClientMock } from '@kbn/actions-plugin/server/mocks'; import { eventLogClientMock } from '@kbn/event-log-plugin/server/mocks'; +import { TaskStatus } from '@kbn/task-manager-plugin/server'; import { ConstructorOptions } from '../rules_client'; import { RuleTypeRegistry } from '../../rule_type_registry'; import { RecoveredActionGroup } from '../../../common'; @@ -51,6 +52,22 @@ export function getBeforeSetup( rulesClientParams.createAPIKey.mockResolvedValue({ apiKeysEnabled: false }); rulesClientParams.getUserName.mockResolvedValue('elastic'); taskManager.runSoon.mockResolvedValue({ id: '' }); + taskManager.get.mockResolvedValue({ + id: 'task-123', + taskType: 'alerting:123', + scheduledAt: new Date(), + attempts: 1, + status: TaskStatus.Idle, + runAt: new Date(), + startedAt: null, + retryAt: null, + state: {}, + params: { + alertId: '1', + }, + ownerId: null, + enabled: false, + }); taskManager.bulkRemoveIfExist.mockResolvedValue({ statuses: [{ id: 'taskId', type: 'alert', success: true }], }); diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group4/scheduled_task_id.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group4/scheduled_task_id.ts index d8570564d32d2..b7ad0e47947e1 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group4/scheduled_task_id.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group4/scheduled_task_id.ts @@ -23,6 +23,7 @@ export default function createScheduledTaskIdTests({ getService }: FtrProviderCo const supertest = getService('supertest'); const supertestWithoutAuth = getService('supertestWithoutAuth'); const esArchiver = getService('esArchiver'); + const retry = getService('retry'); describe('scheduled task id', () => { const objectRemover = new ObjectRemover(supertest); @@ -116,5 +117,36 @@ export default function createScheduledTaskIdTests({ getService }: FtrProviderCo }); expect(taskRecord.task.enabled).to.eql(true); }); + + it('deletes associated task for rule if task is unrecognized', async () => { + const RULE_ID = '46be60d4-ae63-48ed-ab6f-f4d9b4defacf'; + // We've archived a disabled rule with a scheduled task ID that references + // a task with a removed task type. Task manager will mark the task as unrecognized. + // When we enable the rule, the unrecognized task should be removed and a new + // task created in its place + + // scheduled task should exist and be unrecognized + await retry.try(async () => { + const taskRecordLoaded = await getScheduledTask(RULE_ID); + expect(taskRecordLoaded.task.status).to.equal('unrecognized'); + }); + + // enable the rule + await supertestWithoutAuth + .post(`${getUrlPrefix(``)}/api/alerting/rule/${RULE_ID}/_enable`) + .set('kbn-xsrf', 'foo'); + await retry.try(async () => { + const response = await supertestWithoutAuth.get( + `${getUrlPrefix(``)}/api/alerting/rule/${RULE_ID}` + ); + + expect(response.status).to.eql(200); + expect(response.body.enabled).to.be(true); + }); + + // new scheduled task should exist with ID and status should not be unrecognized + const newTaskRecordLoaded = await getScheduledTask(RULE_ID); + expect(newTaskRecordLoaded.task.status).not.to.equal('unrecognized'); + }); }); } diff --git a/x-pack/test/functional/es_archives/rules_scheduled_task_id/data.json b/x-pack/test/functional/es_archives/rules_scheduled_task_id/data.json index 159380a281de5..25032f09eeb51 100644 --- a/x-pack/test/functional/es_archives/rules_scheduled_task_id/data.json +++ b/x-pack/test/functional/es_archives/rules_scheduled_task_id/data.json @@ -71,4 +71,79 @@ "updated_at": "2021-11-05T16:21:37.629Z" } } -} \ No newline at end of file +} + +{ + "type": "doc", + "value": { + "id": "alert:46be60d4-ae63-48ed-ab6f-f4d9b4defacf", + "index": ".kibana_1", + "source": { + "alert": { + "actions": [ + ], + "alertTypeId": "example.always-firing", + "apiKey": "QIUT8u0/kbOakEHSj50jDpVR90MrqOxanEscboYOoa8PxQvcA5jfHash+fqH3b+KNjJ1LpnBcisGuPkufY9j1e32gKzwGZV5Bfys87imHvygJvIM8uKiFF8bQ8Y4NTaxOJO9fAmZPrFy07ZcQMCAQz+DUTgBFqs=", + "apiKeyOwner": "elastic", + "consumer": "alerts", + "createdAt": "2020-06-17T15:35:38.497Z", + "createdBy": "elastic", + "enabled": false, + "muteAll": false, + "mutedInstanceIds": [ + ], + "name": "always-firing-alert", + "params": { + }, + "schedule": { + "interval": "1m" + }, + "scheduledTaskId": "46be60d4-ae63-48ed-ab6f-f4d9b4defacf", + "tags": [ + ], + "throttle": null, + "updatedBy": "elastic" + }, + "migrationVersion": { + "alert": "7.16.0" + }, + "references": [ + ], + "type": "alert", + "updated_at": "2020-06-17T15:35:39.839Z" + } + } +} + +{ + "type": "doc", + "value": { + "id": "task:46be60d4-ae63-48ed-ab6f-f4d9b4defacf", + "index": ".kibana_task_manager_1", + "source": { + "migrationVersion": { + "task": "7.16.0" + }, + "task": { + "attempts": 0, + "ownerId": null, + "params": "{\"alertId\":\"46be60d4-ae63-48ed-ab6f-f4d9b4defacf\",\"spaceId\":\"default\"}", + "retryAt": null, + "runAt": "2021-11-05T16:21:52.148Z", + "schedule": { + "interval": "1m" + }, + "scheduledAt": "2021-11-05T15:28:42.055Z", + "scope": [ + "alerting" + ], + "startedAt": null, + "status": "idle", + "taskType": "sampleTaskRemovedType" + }, + "references": [], + "type": "task", + "updated_at": "2021-11-05T16:21:37.629Z" + } + } +}