From d7746d02dbae2b568afaf6b19531dea8fc7bb306 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Fri, 17 Jan 2020 14:48:03 -0500 Subject: [PATCH 1/7] Cleanup action task params saved objects after use --- .../plugins/actions/server/lib/task_runner_factory.ts | 7 +++++++ x-pack/legacy/plugins/actions/server/plugin.ts | 1 + 2 files changed, 8 insertions(+) diff --git a/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts b/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts index 2dc3d1161399e..164361220007f 100644 --- a/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts +++ b/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts @@ -6,6 +6,7 @@ import { ActionExecutorContract } from './action_executor'; import { ExecutorError } from './executor_error'; +import { ActionsCoreStart } from '../shim'; import { RunContext } from '../../../../../plugins/task_manager/server'; import { PluginStartContract as EncryptedSavedObjectsStartContract } from '../../../../../plugins/encrypted_saved_objects/server'; import { ActionTaskParams, GetBasePathFunction, SpaceIdToNamespaceFunction } from '../types'; @@ -14,6 +15,7 @@ export interface TaskRunnerContext { encryptedSavedObjectsPlugin: EncryptedSavedObjectsStartContract; spaceIdToNamespace: SpaceIdToNamespaceFunction; getBasePath: GetBasePathFunction; + getScopedSavedObjectsClient: ActionsCoreStart['savedObjects']['getScopedSavedObjectsClient']; } export class TaskRunnerFactory { @@ -43,6 +45,7 @@ export class TaskRunnerFactory { encryptedSavedObjectsPlugin, spaceIdToNamespace, getBasePath, + getScopedSavedObjectsClient, } = this.taskRunnerContext!; return { @@ -93,6 +96,10 @@ export class TaskRunnerFactory { executorResult.data, executorResult.retry == null ? false : executorResult.retry ); + } else if (executorResult.status === 'ok') { + // Cleanup action_task_params object now that we're done with it + const savedObjectsClient = getScopedSavedObjectsClient(fakeRequest); + await savedObjectsClient.delete('action_task_params', actionTaskParamsId); } }, }; diff --git a/x-pack/legacy/plugins/actions/server/plugin.ts b/x-pack/legacy/plugins/actions/server/plugin.ts index ffc4a9cf90e54..640e97dd00afb 100644 --- a/x-pack/legacy/plugins/actions/server/plugin.ts +++ b/x-pack/legacy/plugins/actions/server/plugin.ts @@ -161,6 +161,7 @@ export class Plugin { encryptedSavedObjectsPlugin: plugins.encryptedSavedObjects, getBasePath, spaceIdToNamespace, + getScopedSavedObjectsClient: core.savedObjects.getScopedSavedObjectsClient, }); const executeFn = createExecuteFunction({ From beb9b3cc289ab0de9b068f36982429f1e0ba4250 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Fri, 17 Jan 2020 14:53:26 -0500 Subject: [PATCH 2/7] Fix jest tests --- .../plugins/actions/server/lib/task_runner_factory.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.test.ts b/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.test.ts index ad2b74da0d7d4..09806f67479c7 100644 --- a/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.test.ts +++ b/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.test.ts @@ -67,11 +67,15 @@ const taskRunnerFactoryInitializerParams = { spaceIdToNamespace, encryptedSavedObjectsPlugin: mockedEncryptedSavedObjectsPlugin, getBasePath: jest.fn().mockReturnValue(undefined), + getScopedSavedObjectsClient: jest.fn().mockReturnValue(savedObjectsClientMock.create()), }; beforeEach(() => { jest.resetAllMocks(); actionExecutorInitializerParams.getServices.mockReturnValue(services); + taskRunnerFactoryInitializerParams.getScopedSavedObjectsClient.mockReturnValue( + savedObjectsClientMock.create() + ); }); test(`throws an error if factory isn't initialized`, () => { From 0cec412f79d2acb42abf4dc240f1502a6bae97c9 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 20 Jan 2020 09:00:29 -0500 Subject: [PATCH 3/7] Add integration test to ensure object gets cleaned up --- x-pack/legacy/plugins/actions/mappings.json | 3 ++ .../actions/server/create_execute_function.ts | 1 + .../legacy/plugins/actions/server/plugin.ts | 1 + x-pack/legacy/plugins/actions/server/types.ts | 1 + .../common/fixtures/plugins/alerts/index.ts | 9 ++--- .../common/lib/task_manager_utils.ts | 33 +++++++++++++++++++ .../tests/alerting/alerts.ts | 2 ++ .../spaces_only/tests/alerting/alerts_base.ts | 5 +++ 8 files changed, 51 insertions(+), 4 deletions(-) diff --git a/x-pack/legacy/plugins/actions/mappings.json b/x-pack/legacy/plugins/actions/mappings.json index a9c4d80b00af1..22b6fc3308d66 100644 --- a/x-pack/legacy/plugins/actions/mappings.json +++ b/x-pack/legacy/plugins/actions/mappings.json @@ -27,6 +27,9 @@ }, "apiKey": { "type": "binary" + }, + "createdAt": { + "type": "date" } } } diff --git a/x-pack/legacy/plugins/actions/server/create_execute_function.ts b/x-pack/legacy/plugins/actions/server/create_execute_function.ts index ddd8b1df2327b..d3bf8d9a31b6a 100644 --- a/x-pack/legacy/plugins/actions/server/create_execute_function.ts +++ b/x-pack/legacy/plugins/actions/server/create_execute_function.ts @@ -56,6 +56,7 @@ export function createExecuteFunction({ actionId: id, params, apiKey, + createdAt: new Date().toISOString(), }); await taskManager.schedule({ diff --git a/x-pack/legacy/plugins/actions/server/plugin.ts b/x-pack/legacy/plugins/actions/server/plugin.ts index 640e97dd00afb..23c18a9b202bf 100644 --- a/x-pack/legacy/plugins/actions/server/plugin.ts +++ b/x-pack/legacy/plugins/actions/server/plugin.ts @@ -86,6 +86,7 @@ export class Plugin { plugins.encryptedSavedObjects.registerType({ type: 'action_task_params', attributesToEncrypt: new Set(['apiKey']), + attributesToExcludeFromAAD: new Set(['createdAt']), }); const actionExecutor = new ActionExecutor(); diff --git a/x-pack/legacy/plugins/actions/server/types.ts b/x-pack/legacy/plugins/actions/server/types.ts index 6a6fb7d660cbb..2fe8036327fba 100644 --- a/x-pack/legacy/plugins/actions/server/types.ts +++ b/x-pack/legacy/plugins/actions/server/types.ts @@ -93,4 +93,5 @@ export interface ActionTaskParams extends SavedObjectAttributes { actionId: string; params: Record; apiKey?: string; + createdAt: string; } diff --git a/x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/index.ts b/x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/index.ts index b5d201c1682bd..00d0f597346d9 100644 --- a/x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/index.ts +++ b/x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/index.ts @@ -62,8 +62,8 @@ export default function(kibana: any) { encrypted: schema.string(), }), }, - async executor({ config, secrets, params, services }: ActionTypeExecutorOptions) { - return await services.callCluster('index', { + async executor({ config, secrets, params, services, actionId }: ActionTypeExecutorOptions) { + await services.callCluster('index', { index: params.index, refresh: 'wait_for', body: { @@ -74,6 +74,7 @@ export default function(kibana: any) { source: 'action:test.index-record', }, }); + return { status: 'ok', actionId }; }, }; const failingActionType: ActionType = { @@ -141,7 +142,7 @@ export default function(kibana: any) { reference: schema.string(), }), }, - async executor({ params, services }: ActionTypeExecutorOptions) { + async executor({ params, services, actionId }: ActionTypeExecutorOptions) { // Call cluster let callClusterSuccess = false; let callClusterError; @@ -186,8 +187,8 @@ export default function(kibana: any) { }, }); return { + actionId, status: 'ok', - actionId: '', }; }, }; diff --git a/x-pack/test/alerting_api_integration/common/lib/task_manager_utils.ts b/x-pack/test/alerting_api_integration/common/lib/task_manager_utils.ts index b72960b162e76..f0ea5838bb50d 100644 --- a/x-pack/test/alerting_api_integration/common/lib/task_manager_utils.ts +++ b/x-pack/test/alerting_api_integration/common/lib/task_manager_utils.ts @@ -43,4 +43,37 @@ export class TaskManagerUtils { } }); } + + async waitForActionTaskParamsToBeCleanedUp(createdAtFilter: Date): Promise { + return await this.retry.try(async () => { + const searchResult = await this.es.search({ + index: '.kibana', + body: { + query: { + bool: { + must: [ + { + term: { + type: 'action_task_params', + }, + }, + { + range: { + 'action_task_params.createdAt': { + gte: createdAtFilter, + }, + }, + }, + ], + }, + }, + }, + }); + if (searchResult.hits.total.value) { + throw new Error( + `Expected 0 action_task_params objects but received ${searchResult.hits.total.value}` + ); + } + }); + } } diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/alerts.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/alerts.ts index 551498e22d5c8..5540d129518b4 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/alerts.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/alerts.ts @@ -147,6 +147,8 @@ export default function alertTests({ getService }: FtrProviderContext) { reference, source: 'action:test.index-record', }); + + await taskManagerUtils.waitForActionTaskParamsToBeCleanedUp(testStart); break; default: throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`); diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/alerts_base.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/alerts_base.ts index d9a58851afb31..3c60d2779720a 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/alerts_base.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/alerts_base.ts @@ -16,6 +16,7 @@ import { ObjectRemover, AlertUtils, ensureDatetimeIsWithinRange, + TaskManagerUtils, } from '../../../common/lib'; // eslint-disable-next-line import/no-default-export @@ -24,6 +25,7 @@ export function alertTests({ getService }: FtrProviderContext, space: Space) { const es = getService('legacyEs'); const retry = getService('retry'); const esTestIndexTool = new ESTestIndexTool(es, retry); + const taskManagerUtils = new TaskManagerUtils(es, retry); function getAlertingTaskById(taskId: string) { return supertestWithoutAuth @@ -73,6 +75,7 @@ export function alertTests({ getService }: FtrProviderContext, space: Space) { }); it('should schedule task, run alert and schedule actions', async () => { + const testStart = new Date(); const reference = alertUtils.generateReference(); const response = await alertUtils.createAlwaysFiringAction({ reference }); const alertId = response.body.id; @@ -121,6 +124,8 @@ export function alertTests({ getService }: FtrProviderContext, space: Space) { reference, source: 'action:test.index-record', }); + + await taskManagerUtils.waitForActionTaskParamsToBeCleanedUp(testStart); }); it('should reschedule failing alerts using the alerting interval and not the Task Manager retry logic', async () => { From 044929982077f2e0fd6f46122914d3588d6efbe2 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 20 Jan 2020 09:26:40 -0500 Subject: [PATCH 4/7] Add unit tests --- .../server/create_execute_function.test.ts | 8 +++ .../server/lib/action_executor.mock.ts | 2 +- .../server/lib/task_runner_factory.test.ts | 55 ++++++++++++++++++- .../actions/server/lib/task_runner_factory.ts | 14 ++++- 4 files changed, 74 insertions(+), 5 deletions(-) diff --git a/x-pack/legacy/plugins/actions/server/create_execute_function.test.ts b/x-pack/legacy/plugins/actions/server/create_execute_function.test.ts index 7dbcfce5ee335..e172373abce8b 100644 --- a/x-pack/legacy/plugins/actions/server/create_execute_function.test.ts +++ b/x-pack/legacy/plugins/actions/server/create_execute_function.test.ts @@ -4,14 +4,21 @@ * you may not use this file except in compliance with the Elastic License. */ +import sinon from 'sinon'; import { taskManagerMock } from '../../../../plugins/task_manager/server/task_manager.mock'; import { createExecuteFunction } from './create_execute_function'; import { savedObjectsClientMock } from '../../../../../src/core/server/mocks'; +let fakeTimer: sinon.SinonFakeTimers; const mockTaskManager = taskManagerMock.start(); const savedObjectsClient = savedObjectsClientMock.create(); const getBasePath = jest.fn(); +beforeAll(() => { + fakeTimer = sinon.useFakeTimers(); +}); +afterAll(() => fakeTimer.restore()); + beforeEach(() => jest.resetAllMocks()); describe('execute()', () => { @@ -62,6 +69,7 @@ describe('execute()', () => { actionId: '123', params: { baz: false }, apiKey: Buffer.from('123:abc').toString('base64'), + createdAt: '1970-01-01T00:00:00.000Z', }); }); diff --git a/x-pack/legacy/plugins/actions/server/lib/action_executor.mock.ts b/x-pack/legacy/plugins/actions/server/lib/action_executor.mock.ts index 73e5e96ab24ed..b4419cd761bbe 100644 --- a/x-pack/legacy/plugins/actions/server/lib/action_executor.mock.ts +++ b/x-pack/legacy/plugins/actions/server/lib/action_executor.mock.ts @@ -9,7 +9,7 @@ import { ActionExecutorContract } from './action_executor'; const createActionExecutorMock = () => { const mocked: jest.Mocked = { initialize: jest.fn(), - execute: jest.fn(), + execute: jest.fn().mockResolvedValue({ status: 'ok', actionId: '' }), }; return mocked; }; diff --git a/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.test.ts b/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.test.ts index 09806f67479c7..907881dddefbf 100644 --- a/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.test.ts +++ b/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.test.ts @@ -65,16 +65,17 @@ const actionExecutorInitializerParams = { }; const taskRunnerFactoryInitializerParams = { spaceIdToNamespace, + logger: loggingServiceMock.create().get(), encryptedSavedObjectsPlugin: mockedEncryptedSavedObjectsPlugin, getBasePath: jest.fn().mockReturnValue(undefined), - getScopedSavedObjectsClient: jest.fn().mockReturnValue(savedObjectsClientMock.create()), + getScopedSavedObjectsClient: jest.fn().mockReturnValue(services.savedObjectsClient), }; beforeEach(() => { jest.resetAllMocks(); actionExecutorInitializerParams.getServices.mockReturnValue(services); taskRunnerFactoryInitializerParams.getScopedSavedObjectsClient.mockReturnValue( - savedObjectsClientMock.create() + services.savedObjectsClient ); }); @@ -141,6 +142,56 @@ test('executes the task by calling the executor with proper parameters', async ( }); }); +test('cleans up action_task_params object', async () => { + const taskRunner = taskRunnerFactory.create({ + taskInstance: mockedTaskInstance, + }); + + mockedActionExecutor.execute.mockResolvedValueOnce({ status: 'ok', actionId: '2' }); + spaceIdToNamespace.mockReturnValueOnce('namespace-test'); + mockedEncryptedSavedObjectsPlugin.getDecryptedAsInternalUser.mockResolvedValueOnce({ + id: '3', + type: 'action_task_params', + attributes: { + actionId: '2', + params: { baz: true }, + apiKey: Buffer.from('123:abc').toString('base64'), + }, + references: [], + }); + + await taskRunner.run(); + + expect(services.savedObjectsClient.delete).toHaveBeenCalledWith('action_task_params', '3'); +}); + +test('runs successfully when cleanup fails and logs the error', async () => { + const taskRunner = taskRunnerFactory.create({ + taskInstance: mockedTaskInstance, + }); + + mockedActionExecutor.execute.mockResolvedValueOnce({ status: 'ok', actionId: '2' }); + spaceIdToNamespace.mockReturnValueOnce('namespace-test'); + mockedEncryptedSavedObjectsPlugin.getDecryptedAsInternalUser.mockResolvedValueOnce({ + id: '3', + type: 'action_task_params', + attributes: { + actionId: '2', + params: { baz: true }, + apiKey: Buffer.from('123:abc').toString('base64'), + }, + references: [], + }); + services.savedObjectsClient.delete.mockRejectedValueOnce(new Error('Fail')); + + await taskRunner.run(); + + expect(services.savedObjectsClient.delete).toHaveBeenCalledWith('action_task_params', '3'); + expect(taskRunnerFactoryInitializerParams.logger.error).toHaveBeenCalledWith( + 'Failed to cleanup action_task_params object [id="3"]: Fail' + ); +}); + test('throws an error with suggested retry logic when return status is error', async () => { const taskRunner = taskRunnerFactory.create({ taskInstance: mockedTaskInstance, diff --git a/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts b/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts index 164361220007f..72a6208d44228 100644 --- a/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts +++ b/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts @@ -7,11 +7,13 @@ import { ActionExecutorContract } from './action_executor'; import { ExecutorError } from './executor_error'; import { ActionsCoreStart } from '../shim'; +import { Logger } from '../../../../../../src/core/server'; import { RunContext } from '../../../../../plugins/task_manager/server'; import { PluginStartContract as EncryptedSavedObjectsStartContract } from '../../../../../plugins/encrypted_saved_objects/server'; import { ActionTaskParams, GetBasePathFunction, SpaceIdToNamespaceFunction } from '../types'; export interface TaskRunnerContext { + logger: Logger; encryptedSavedObjectsPlugin: EncryptedSavedObjectsStartContract; spaceIdToNamespace: SpaceIdToNamespaceFunction; getBasePath: GetBasePathFunction; @@ -42,6 +44,7 @@ export class TaskRunnerFactory { const { actionExecutor } = this; const { + logger, encryptedSavedObjectsPlugin, spaceIdToNamespace, getBasePath, @@ -98,8 +101,15 @@ export class TaskRunnerFactory { ); } else if (executorResult.status === 'ok') { // Cleanup action_task_params object now that we're done with it - const savedObjectsClient = getScopedSavedObjectsClient(fakeRequest); - await savedObjectsClient.delete('action_task_params', actionTaskParamsId); + try { + const savedObjectsClient = getScopedSavedObjectsClient(fakeRequest); + await savedObjectsClient.delete('action_task_params', actionTaskParamsId); + } catch (e) { + // Log error only, we shouldn't make the action type execute again because of an error here + logger.error( + `Failed to cleanup action_task_params object [id="${actionTaskParamsId}"]: ${e.message}` + ); + } } }, }; From f8533549a8f0c8622dbe2b36f413818b724e4625 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 20 Jan 2020 09:30:00 -0500 Subject: [PATCH 5/7] Fix comment --- x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts b/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts index 72a6208d44228..25714e49e339b 100644 --- a/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts +++ b/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts @@ -105,7 +105,7 @@ export class TaskRunnerFactory { const savedObjectsClient = getScopedSavedObjectsClient(fakeRequest); await savedObjectsClient.delete('action_task_params', actionTaskParamsId); } catch (e) { - // Log error only, we shouldn't make the action type execute again because of an error here + // Log error only, we shouldn't fail the task because of an error here (if ever there's retry logic) logger.error( `Failed to cleanup action_task_params object [id="${actionTaskParamsId}"]: ${e.message}` ); From f0a03854817d88f45ccda7190ecb2376a0faac9c Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 20 Jan 2020 11:41:36 -0500 Subject: [PATCH 6/7] Re-use updated_at instead of creating createdAt --- x-pack/legacy/plugins/actions/mappings.json | 3 --- .../actions/server/create_execute_function.test.ts | 8 -------- .../plugins/actions/server/create_execute_function.ts | 1 - x-pack/legacy/plugins/actions/server/plugin.ts | 2 +- x-pack/legacy/plugins/actions/server/types.ts | 1 - .../common/lib/task_manager_utils.ts | 2 +- 6 files changed, 2 insertions(+), 15 deletions(-) diff --git a/x-pack/legacy/plugins/actions/mappings.json b/x-pack/legacy/plugins/actions/mappings.json index 22b6fc3308d66..a9c4d80b00af1 100644 --- a/x-pack/legacy/plugins/actions/mappings.json +++ b/x-pack/legacy/plugins/actions/mappings.json @@ -27,9 +27,6 @@ }, "apiKey": { "type": "binary" - }, - "createdAt": { - "type": "date" } } } diff --git a/x-pack/legacy/plugins/actions/server/create_execute_function.test.ts b/x-pack/legacy/plugins/actions/server/create_execute_function.test.ts index e172373abce8b..7dbcfce5ee335 100644 --- a/x-pack/legacy/plugins/actions/server/create_execute_function.test.ts +++ b/x-pack/legacy/plugins/actions/server/create_execute_function.test.ts @@ -4,21 +4,14 @@ * you may not use this file except in compliance with the Elastic License. */ -import sinon from 'sinon'; import { taskManagerMock } from '../../../../plugins/task_manager/server/task_manager.mock'; import { createExecuteFunction } from './create_execute_function'; import { savedObjectsClientMock } from '../../../../../src/core/server/mocks'; -let fakeTimer: sinon.SinonFakeTimers; const mockTaskManager = taskManagerMock.start(); const savedObjectsClient = savedObjectsClientMock.create(); const getBasePath = jest.fn(); -beforeAll(() => { - fakeTimer = sinon.useFakeTimers(); -}); -afterAll(() => fakeTimer.restore()); - beforeEach(() => jest.resetAllMocks()); describe('execute()', () => { @@ -69,7 +62,6 @@ describe('execute()', () => { actionId: '123', params: { baz: false }, apiKey: Buffer.from('123:abc').toString('base64'), - createdAt: '1970-01-01T00:00:00.000Z', }); }); diff --git a/x-pack/legacy/plugins/actions/server/create_execute_function.ts b/x-pack/legacy/plugins/actions/server/create_execute_function.ts index d3bf8d9a31b6a..ddd8b1df2327b 100644 --- a/x-pack/legacy/plugins/actions/server/create_execute_function.ts +++ b/x-pack/legacy/plugins/actions/server/create_execute_function.ts @@ -56,7 +56,6 @@ export function createExecuteFunction({ actionId: id, params, apiKey, - createdAt: new Date().toISOString(), }); await taskManager.schedule({ diff --git a/x-pack/legacy/plugins/actions/server/plugin.ts b/x-pack/legacy/plugins/actions/server/plugin.ts index 23c18a9b202bf..10d24c7f38311 100644 --- a/x-pack/legacy/plugins/actions/server/plugin.ts +++ b/x-pack/legacy/plugins/actions/server/plugin.ts @@ -86,7 +86,6 @@ export class Plugin { plugins.encryptedSavedObjects.registerType({ type: 'action_task_params', attributesToEncrypt: new Set(['apiKey']), - attributesToExcludeFromAAD: new Set(['createdAt']), }); const actionExecutor = new ActionExecutor(); @@ -159,6 +158,7 @@ export class Plugin { actionTypeRegistry: actionTypeRegistry!, }); taskRunnerFactory!.initialize({ + logger, encryptedSavedObjectsPlugin: plugins.encryptedSavedObjects, getBasePath, spaceIdToNamespace, diff --git a/x-pack/legacy/plugins/actions/server/types.ts b/x-pack/legacy/plugins/actions/server/types.ts index 2fe8036327fba..6a6fb7d660cbb 100644 --- a/x-pack/legacy/plugins/actions/server/types.ts +++ b/x-pack/legacy/plugins/actions/server/types.ts @@ -93,5 +93,4 @@ export interface ActionTaskParams extends SavedObjectAttributes { actionId: string; params: Record; apiKey?: string; - createdAt: string; } diff --git a/x-pack/test/alerting_api_integration/common/lib/task_manager_utils.ts b/x-pack/test/alerting_api_integration/common/lib/task_manager_utils.ts index f0ea5838bb50d..3a1d035a023c2 100644 --- a/x-pack/test/alerting_api_integration/common/lib/task_manager_utils.ts +++ b/x-pack/test/alerting_api_integration/common/lib/task_manager_utils.ts @@ -59,7 +59,7 @@ export class TaskManagerUtils { }, { range: { - 'action_task_params.createdAt': { + updated_at: { gte: createdAtFilter, }, }, From ce26bccf5a46d97436ac077e3a9721f8006a1e5c Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 20 Jan 2020 11:45:06 -0500 Subject: [PATCH 7/7] Consider null/undefined returned from executor as success as well --- .../actions/server/lib/task_runner_factory.ts | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts b/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts index 25714e49e339b..61d6519659309 100644 --- a/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts +++ b/x-pack/legacy/plugins/actions/server/lib/task_runner_factory.ts @@ -91,6 +91,7 @@ export class TaskRunnerFactory { actionId, request: fakeRequest, }); + if (executorResult.status === 'error') { // Task manager error handler only kicks in when an error thrown (at this time) // So what we have to do is throw when the return status is `error`. @@ -99,17 +100,17 @@ export class TaskRunnerFactory { executorResult.data, executorResult.retry == null ? false : executorResult.retry ); - } else if (executorResult.status === 'ok') { - // Cleanup action_task_params object now that we're done with it - try { - const savedObjectsClient = getScopedSavedObjectsClient(fakeRequest); - await savedObjectsClient.delete('action_task_params', actionTaskParamsId); - } catch (e) { - // Log error only, we shouldn't fail the task because of an error here (if ever there's retry logic) - logger.error( - `Failed to cleanup action_task_params object [id="${actionTaskParamsId}"]: ${e.message}` - ); - } + } + + // Cleanup action_task_params object now that we're done with it + try { + const savedObjectsClient = getScopedSavedObjectsClient(fakeRequest); + await savedObjectsClient.delete('action_task_params', actionTaskParamsId); + } catch (e) { + // Log error only, we shouldn't fail the task because of an error here (if ever there's retry logic) + logger.error( + `Failed to cleanup action_task_params object [id="${actionTaskParamsId}"]: ${e.message}` + ); } }, };