From 81827679ac653e31a4ec2b9f13796a59cd8cabba Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 5 Jun 2023 13:15:21 -0400 Subject: [PATCH 01/29] Initial commit --- .../task_manager/server/config.test.ts | 3 + x-pack/plugins/task_manager/server/config.ts | 1 + .../server/ephemeral_task_lifecycle.test.ts | 1 + .../managed_configuration.test.ts | 1 + .../configuration_statistics.test.ts | 1 + .../monitoring_stats_stream.test.ts | 1 + .../task_manager/server/plugin.test.ts | 1 + x-pack/plugins/task_manager/server/plugin.ts | 1 + .../server/polling_lifecycle.test.ts | 1 + x-pack/plugins/task_manager/server/task.ts | 19 ++- .../task_manager/server/task_pool.test.ts | 1 + .../task_manager/server/task_store.test.ts | 31 ++++ .../plugins/task_manager/server/task_store.ts | 81 +++++++--- .../server/task_type_dictionary.test.ts | 15 +- .../server/task_type_dictionary.ts | 28 +++- .../task_manager/server/task_validator.ts | 146 ++++++++++++++++++ 16 files changed, 296 insertions(+), 36 deletions(-) create mode 100644 x-pack/plugins/task_manager/server/task_validator.ts diff --git a/x-pack/plugins/task_manager/server/config.test.ts b/x-pack/plugins/task_manager/server/config.test.ts index d2679efc5042a5..f9a8f17e1d6f62 100644 --- a/x-pack/plugins/task_manager/server/config.test.ts +++ b/x-pack/plugins/task_manager/server/config.test.ts @@ -12,6 +12,7 @@ describe('config validation', () => { const config: Record = {}; expect(configSchema.validate(config)).toMatchInlineSnapshot(` Object { + "allow_reading_invalid_state": false, "ephemeral_tasks": Object { "enabled": false, "request_capacity": 10, @@ -64,6 +65,7 @@ describe('config validation', () => { const config: Record = {}; expect(configSchema.validate(config)).toMatchInlineSnapshot(` Object { + "allow_reading_invalid_state": false, "ephemeral_tasks": Object { "enabled": false, "request_capacity": 10, @@ -114,6 +116,7 @@ describe('config validation', () => { }; expect(configSchema.validate(config)).toMatchInlineSnapshot(` Object { + "allow_reading_invalid_state": false, "ephemeral_tasks": Object { "enabled": false, "request_capacity": 10, diff --git a/x-pack/plugins/task_manager/server/config.ts b/x-pack/plugins/task_manager/server/config.ts index dffc9677c7c776..3f8c1a9345fca1 100644 --- a/x-pack/plugins/task_manager/server/config.ts +++ b/x-pack/plugins/task_manager/server/config.ts @@ -137,6 +137,7 @@ export const configSchema = schema.object( exclude_task_types: schema.arrayOf(schema.string(), { defaultValue: [] }), authenticate_background_task_utilization: schema.boolean({ defaultValue: true }), }), + allow_reading_invalid_state: schema.boolean({ defaultValue: false }), }, { validate: (config) => { diff --git a/x-pack/plugins/task_manager/server/ephemeral_task_lifecycle.test.ts b/x-pack/plugins/task_manager/server/ephemeral_task_lifecycle.test.ts index 586fa9db5d4469..a12ce476ec2f73 100644 --- a/x-pack/plugins/task_manager/server/ephemeral_task_lifecycle.test.ts +++ b/x-pack/plugins/task_manager/server/ephemeral_task_lifecycle.test.ts @@ -50,6 +50,7 @@ describe('EphemeralTaskLifecycle', () => { poll_interval: 6000000, version_conflict_threshold: 80, request_capacity: 1000, + allow_reading_invalid_state: false, monitored_aggregated_stats_refresh_rate: 5000, monitored_stats_required_freshness: 5000, monitored_stats_running_average_window: 50, diff --git a/x-pack/plugins/task_manager/server/integration_tests/managed_configuration.test.ts b/x-pack/plugins/task_manager/server/integration_tests/managed_configuration.test.ts index 4b024ac1e7e1be..5ba49664895cd9 100644 --- a/x-pack/plugins/task_manager/server/integration_tests/managed_configuration.test.ts +++ b/x-pack/plugins/task_manager/server/integration_tests/managed_configuration.test.ts @@ -43,6 +43,7 @@ describe('managed configuration', () => { max_workers: 10, max_attempts: 9, poll_interval: 3000, + allow_reading_invalid_state: false, version_conflict_threshold: 80, monitored_aggregated_stats_refresh_rate: 60000, monitored_stats_health_verbose_log: { diff --git a/x-pack/plugins/task_manager/server/monitoring/configuration_statistics.test.ts b/x-pack/plugins/task_manager/server/monitoring/configuration_statistics.test.ts index 665b212d951c07..62303b925fb3f6 100644 --- a/x-pack/plugins/task_manager/server/monitoring/configuration_statistics.test.ts +++ b/x-pack/plugins/task_manager/server/monitoring/configuration_statistics.test.ts @@ -16,6 +16,7 @@ describe('Configuration Statistics Aggregator', () => { max_workers: 10, max_attempts: 9, poll_interval: 6000000, + allow_reading_invalid_state: false, version_conflict_threshold: 80, monitored_stats_required_freshness: 6000000, request_capacity: 1000, diff --git a/x-pack/plugins/task_manager/server/monitoring/monitoring_stats_stream.test.ts b/x-pack/plugins/task_manager/server/monitoring/monitoring_stats_stream.test.ts index 23312b79cec8ba..0dcd87ab150bfe 100644 --- a/x-pack/plugins/task_manager/server/monitoring/monitoring_stats_stream.test.ts +++ b/x-pack/plugins/task_manager/server/monitoring/monitoring_stats_stream.test.ts @@ -20,6 +20,7 @@ describe('createMonitoringStatsStream', () => { max_workers: 10, max_attempts: 9, poll_interval: 6000000, + allow_reading_invalid_state: false, version_conflict_threshold: 80, monitored_stats_required_freshness: 6000000, request_capacity: 1000, diff --git a/x-pack/plugins/task_manager/server/plugin.test.ts b/x-pack/plugins/task_manager/server/plugin.test.ts index 637f8a2b952f41..aca39e1678a23e 100644 --- a/x-pack/plugins/task_manager/server/plugin.test.ts +++ b/x-pack/plugins/task_manager/server/plugin.test.ts @@ -43,6 +43,7 @@ const pluginInitializerContextParams = { poll_interval: 3000, version_conflict_threshold: 80, request_capacity: 1000, + allow_reading_invalid_state: false, monitored_aggregated_stats_refresh_rate: 5000, monitored_stats_health_verbose_log: { enabled: false, diff --git a/x-pack/plugins/task_manager/server/plugin.ts b/x-pack/plugins/task_manager/server/plugin.ts index fe6b6b4120ff03..e771138c7da396 100644 --- a/x-pack/plugins/task_manager/server/plugin.ts +++ b/x-pack/plugins/task_manager/server/plugin.ts @@ -212,6 +212,7 @@ export class TaskManagerPlugin definitions: this.definitions, taskManagerId: `kibana:${this.taskManagerId!}`, adHocTaskCounter: this.adHocTaskCounter, + allowReadingInvalidState: this.config.allow_reading_invalid_state, }); const managedConfiguration = createManagedConfiguration({ diff --git a/x-pack/plugins/task_manager/server/polling_lifecycle.test.ts b/x-pack/plugins/task_manager/server/polling_lifecycle.test.ts index 10c377cc8e8948..3f4907749aa111 100644 --- a/x-pack/plugins/task_manager/server/polling_lifecycle.test.ts +++ b/x-pack/plugins/task_manager/server/polling_lifecycle.test.ts @@ -48,6 +48,7 @@ describe('TaskPollingLifecycle', () => { poll_interval: 6000000, version_conflict_threshold: 80, request_capacity: 1000, + allow_reading_invalid_state: false, monitored_aggregated_stats_refresh_rate: 5000, monitored_stats_health_verbose_log: { enabled: false, diff --git a/x-pack/plugins/task_manager/server/task.ts b/x-pack/plugins/task_manager/server/task.ts index a9f49b661a7001..edde074ca4e02c 100644 --- a/x-pack/plugins/task_manager/server/task.ts +++ b/x-pack/plugins/task_manager/server/task.ts @@ -5,7 +5,7 @@ * 2.0. */ -import { schema, TypeOf } from '@kbn/config-schema'; +import { schema, TypeOf, ObjectType } from '@kbn/config-schema'; import { Interval, isInterval, parseIntervalAsMillisecond } from './lib/intervals'; import { isErr, tryAsResult } from './lib/result_type'; @@ -138,6 +138,15 @@ export const taskDefinitionSchema = schema.object( min: 0, }) ), + stateSchemaByVersion: schema.maybe( + schema.recordOf( + schema.string(), + schema.object({ + schema: schema.any(), + up: schema.any(), + }) + ) + ), }, { validate({ timeout }) { @@ -158,6 +167,13 @@ export type TaskDefinition = TypeOf & { * and an optional cancel function which cancels the task. */ createTaskRunner: TaskRunCreatorFunction; + stateSchemaByVersion?: Record< + number, + { + schema: ObjectType; + up: (state: Record) => Record; + } + >; }; export enum TaskStatus { @@ -248,6 +264,7 @@ export interface TaskInstance { // this can be fixed by supporting generics in the future // eslint-disable-next-line @typescript-eslint/no-explicit-any state: Record; + stateVersion?: number; /** * The serialized traceparent string of the current APM transaction or span. diff --git a/x-pack/plugins/task_manager/server/task_pool.test.ts b/x-pack/plugins/task_manager/server/task_pool.test.ts index 10e440184ab2eb..cfcd0f4d8644c6 100644 --- a/x-pack/plugins/task_manager/server/task_pool.test.ts +++ b/x-pack/plugins/task_manager/server/task_pool.test.ts @@ -422,6 +422,7 @@ describe('TaskPool', () => { type: '', title: '', timeout: '5m', + getLatestStateSchema: () => undefined, createTaskRunner: jest.fn(), }; }, diff --git a/x-pack/plugins/task_manager/server/task_store.test.ts b/x-pack/plugins/task_manager/server/task_store.test.ts index aa194fc2231e24..b194f42f6bfcfb 100644 --- a/x-pack/plugins/task_manager/server/task_store.test.ts +++ b/x-pack/plugins/task_manager/server/task_store.test.ts @@ -5,6 +5,7 @@ * 2.0. */ +import { schema } from '@kbn/config-schema'; import { Client } from '@elastic/elasticsearch'; import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; import _ from 'lodash'; @@ -49,6 +50,14 @@ const taskDefinitions = new TaskTypeDictionary(mockLogger()); taskDefinitions.registerTaskDefinitions({ report: { title: 'report', + stateSchemaByVersion: { + 1: { + schema: schema.object({ + foo: schema.string(), + }), + up: (doc) => doc, + }, + }, createTaskRunner: jest.fn(), }, dernstraight: { @@ -74,6 +83,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); }); @@ -118,6 +128,7 @@ describe('TaskStore', () => { scope: undefined, startedAt: null, state: '{"foo":"bar"}', + stateVersion: 1, status: 'idle', taskType: 'report', user: undefined, @@ -140,6 +151,7 @@ describe('TaskStore', () => { scope: undefined, startedAt: null, state: { foo: 'bar' }, + stateVersion: 1, status: 'idle', taskType: 'report', user: undefined, @@ -244,6 +256,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); }); @@ -313,6 +326,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); }); @@ -411,6 +425,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); }); @@ -458,6 +473,7 @@ describe('TaskStore', () => { scope: undefined, startedAt: null, state: JSON.stringify(task.state), + stateVersion: 1, status: task.status, taskType: task.taskType, user: undefined, @@ -475,6 +491,7 @@ describe('TaskStore', () => { startedAt: null, user: undefined, version: '123', + stateVersion: 1, }); }); @@ -514,6 +531,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); }); @@ -555,6 +573,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); }); @@ -589,6 +608,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); }); @@ -623,6 +643,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); }); @@ -635,6 +656,7 @@ describe('TaskStore', () => { id: randomId(), params: { hello: 'world' }, state: { foo: 'bar' }, + stateVersion: 1, taskType: 'report', attempts: 3, status: 'idle' as TaskStatus, @@ -680,6 +702,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); }); @@ -745,6 +768,7 @@ describe('TaskStore', () => { id: randomId(), params: { hello: 'world' }, state: { foo: 'bar' }, + stateVersion: 1, taskType: 'report', attempts: 3, status: status as TaskStatus, @@ -772,6 +796,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); expect(await store.getLifecycle(task.id)).toEqual(status); @@ -792,6 +817,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); expect(await store.getLifecycle(randomId())).toEqual(TaskLifecycleResult.NotFound); @@ -810,6 +836,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); return expect(store.getLifecycle(randomId())).rejects.toThrow('Bad Request'); @@ -828,6 +855,7 @@ describe('TaskStore', () => { definitions: taskDefinitions, savedObjectsRepository: savedObjectsClient, adHocTaskCounter, + allowReadingInvalidState: false, }); }); @@ -849,6 +877,7 @@ describe('TaskStore', () => { scheduledAt: '2019-02-12T21:01:22.479Z', startedAt: null, state: '{"foo":"bar"}', + stateVersion: 1, status: 'idle', taskType: 'report', traceparent: 'apmTraceparent', @@ -888,6 +917,7 @@ describe('TaskStore', () => { scheduledAt: '2019-02-12T21:01:22.479Z', startedAt: null, state: '{"foo":"bar"}', + stateVersion: 1, status: 'idle', taskType: 'report', traceparent: 'apmTraceparent', @@ -909,6 +939,7 @@ describe('TaskStore', () => { scope: undefined, startedAt: null, state: { foo: 'bar' }, + stateVersion: 1, status: 'idle', taskType: 'report', user: undefined, diff --git a/x-pack/plugins/task_manager/server/task_store.ts b/x-pack/plugins/task_manager/server/task_store.ts index a06ee7b918a7b4..5cf2bb13755a6e 100644 --- a/x-pack/plugins/task_manager/server/task_store.ts +++ b/x-pack/plugins/task_manager/server/task_store.ts @@ -36,6 +36,7 @@ import { import { TaskTypeDictionary } from './task_type_dictionary'; import { AdHocTaskCounter } from './lib/adhoc_task_counter'; +import { TaskValidator } from './task_validator'; export interface StoreOpts { esClient: ElasticsearchClient; @@ -45,6 +46,7 @@ export interface StoreOpts { savedObjectsRepository: ISavedObjectsRepository; serializer: ISavedObjectsSerializer; adHocTaskCounter: AdHocTaskCounter; + allowReadingInvalidState: boolean; } export interface SearchOpts { @@ -104,6 +106,7 @@ export class TaskStore { private savedObjectsRepository: ISavedObjectsRepository; private serializer: ISavedObjectsSerializer; private adHocTaskCounter: AdHocTaskCounter; + private readonly taskValidator: TaskValidator; /** * Constructs a new TaskStore. @@ -122,6 +125,10 @@ export class TaskStore { this.serializer = opts.serializer; this.savedObjectsRepository = opts.savedObjectsRepository; this.adHocTaskCounter = opts.adHocTaskCounter; + this.taskValidator = new TaskValidator({ + definitions: opts.definitions, + allowReadingInvalidState: opts.allowReadingInvalidState, + }); this.esClientWithoutRetries = opts.esClient.child({ // Timeouts are retried and make requests timeout after (requestTimeout * (1 + maxRetries)) // The poller doesn't need retry logic because it will try again at the next polling cycle @@ -150,9 +157,13 @@ export class TaskStore { let savedObject; try { + const validatedTaskInstance = this.taskValidator.getValidatedTaskInstance( + taskInstance, + 'write' + ); savedObject = await this.savedObjectsRepository.create( 'task', - taskInstanceToAttributes(taskInstance), + taskInstanceToAttributes(validatedTaskInstance), { id: taskInstance.id, refresh: false } ); if (get(taskInstance, 'schedule.interval', null) == null) { @@ -163,7 +174,8 @@ export class TaskStore { throw e; } - return savedObjectToConcreteTaskInstance(savedObject); + const result = savedObjectToConcreteTaskInstance(savedObject); + return this.taskValidator.getValidatedTaskInstance(result, 'read'); } /** @@ -174,9 +186,13 @@ export class TaskStore { public async bulkSchedule(taskInstances: TaskInstance[]): Promise { const objects = taskInstances.map((taskInstance) => { this.definitions.ensureHas(taskInstance.taskType); + const validatedTaskInstance = this.taskValidator.getValidatedTaskInstance( + taskInstance, + 'write' + ); return { type: 'task', - attributes: taskInstanceToAttributes(taskInstance), + attributes: taskInstanceToAttributes(validatedTaskInstance), id: taskInstance.id, }; }); @@ -197,7 +213,10 @@ export class TaskStore { throw e; } - return savedObjects.saved_objects.map((so) => savedObjectToConcreteTaskInstance(so)); + return savedObjects.saved_objects.map((so) => { + const taskInstance = savedObjectToConcreteTaskInstance(so); + return this.taskValidator.getValidatedTaskInstance(taskInstance, 'read'); + }); } /** @@ -223,7 +242,8 @@ export class TaskStore { * @returns {Promise} */ public async update(doc: ConcreteTaskInstance): Promise { - const attributes = taskInstanceToAttributes(doc); + const validatedTaskInstance = this.taskValidator.getValidatedTaskInstance(doc, 'write'); + const attributes = taskInstanceToAttributes(validatedTaskInstance); let updatedSavedObject; try { @@ -241,13 +261,14 @@ export class TaskStore { throw e; } - return savedObjectToConcreteTaskInstance( + const taskInstance = savedObjectToConcreteTaskInstance( // The SavedObjects update api forces a Partial on the `attributes` on the response, // but actually returns the whole object that is passed to it, so as we know we're // passing in the whole object, this is safe to do. // This is far from ideal, but unless we change the SavedObjectsClient this is the best we can do { ...updatedSavedObject, attributes: defaults(updatedSavedObject.attributes, attributes) } ); + return this.taskValidator.getValidatedTaskInstance(taskInstance, 'read'); } /** @@ -259,7 +280,8 @@ export class TaskStore { */ public async bulkUpdate(docs: ConcreteTaskInstance[]): Promise { const attributesByDocId = docs.reduce((attrsById, doc) => { - attrsById.set(doc.id, taskInstanceToAttributes(doc)); + const validatedTaskInstance = this.taskValidator.getValidatedTaskInstance(doc, 'write'); + attrsById.set(doc.id, taskInstanceToAttributes(validatedTaskInstance)); return attrsById; }, new Map()); @@ -283,21 +305,22 @@ export class TaskStore { } return updatedSavedObjects.map((updatedSavedObject) => { - return updatedSavedObject.error !== undefined - ? asErr({ - type: 'task', - id: updatedSavedObject.id, - error: updatedSavedObject.error, - }) - : asOk( - savedObjectToConcreteTaskInstance({ - ...updatedSavedObject, - attributes: defaults( - updatedSavedObject.attributes, - attributesByDocId.get(updatedSavedObject.id)! - ), - }) - ); + if (updatedSavedObject.error !== undefined) { + return asErr({ + type: 'task', + id: updatedSavedObject.id, + error: updatedSavedObject.error, + }); + } + + const taskInstance = savedObjectToConcreteTaskInstance({ + ...updatedSavedObject, + attributes: defaults( + updatedSavedObject.attributes, + attributesByDocId.get(updatedSavedObject.id)! + ), + }); + return asOk(this.taskValidator.getValidatedTaskInstance(taskInstance, 'read')); }); } @@ -346,7 +369,8 @@ export class TaskStore { this.errors$.next(e); throw e; } - return savedObjectToConcreteTaskInstance(result); + const taskInstance = savedObjectToConcreteTaskInstance(result); + return this.taskValidator.getValidatedTaskInstance(taskInstance, 'read'); } /** @@ -369,7 +393,12 @@ export class TaskStore { if (task.error) { return asErr({ id: task.id, type: task.type, error: task.error }); } - return asOk(savedObjectToConcreteTaskInstance(task)); + const taskInstance = savedObjectToConcreteTaskInstance(task); + const validatedTaskInstance = this.taskValidator.getValidatedTaskInstance( + taskInstance, + 'read' + ); + return asOk(validatedTaskInstance); }); } @@ -413,7 +442,9 @@ export class TaskStore { // @ts-expect-error @elastic/elasticsearch _source is optional .map((doc) => this.serializer.rawToSavedObject(doc)) .map((doc) => omit(doc, 'namespace') as SavedObject) - .map(savedObjectToConcreteTaskInstance), + .map((doc) => savedObjectToConcreteTaskInstance(doc)) + .map((doc) => this.taskValidator.getValidatedTaskInstance(doc, 'read')) + .filter((doc): doc is ConcreteTaskInstance => !!doc), }; } catch (e) { this.errors$.next(e); diff --git a/x-pack/plugins/task_manager/server/task_type_dictionary.test.ts b/x-pack/plugins/task_manager/server/task_type_dictionary.test.ts index 039403816da5e4..a00832895dd451 100644 --- a/x-pack/plugins/task_manager/server/task_type_dictionary.test.ts +++ b/x-pack/plugins/task_manager/server/task_type_dictionary.test.ts @@ -9,7 +9,7 @@ import { get } from 'lodash'; import { RunContext, TaskDefinition } from './task'; import { mockLogger } from './test_utils'; import { - sanitizeTaskDefinitions, + sanitizeAndAugmentTaskDefinitions, TaskDefinitionRegistry, TaskTypeDictionary, } from './task_type_dictionary'; @@ -55,16 +55,17 @@ describe('taskTypeDictionary', () => { definitions = new TaskTypeDictionary(mockLogger()); }); - describe('sanitizeTaskDefinitions', () => {}); + describe('sanitizeAndAugmentTaskDefinitions', () => {}); it('provides tasks with defaults', () => { const taskDefinitions = getMockTaskDefinitions({ numTasks: 3 }); - const result = sanitizeTaskDefinitions(taskDefinitions); + const result = sanitizeAndAugmentTaskDefinitions(taskDefinitions); expect(result).toMatchInlineSnapshot(` Array [ Object { "createTaskRunner": [Function], "description": "one super cool task", + "stateSchemaByVersion": undefined, "timeout": "5m", "title": "Test", "type": "test_task_type_0", @@ -72,6 +73,7 @@ describe('taskTypeDictionary', () => { Object { "createTaskRunner": [Function], "description": "one super cool task", + "stateSchemaByVersion": undefined, "timeout": "5m", "title": "Test", "type": "test_task_type_1", @@ -79,6 +81,7 @@ describe('taskTypeDictionary', () => { Object { "createTaskRunner": [Function], "description": "one super cool task", + "stateSchemaByVersion": undefined, "timeout": "5m", "title": "Test", "type": "test_task_type_2", @@ -108,7 +111,7 @@ describe('taskTypeDictionary', () => { }, }; - return sanitizeTaskDefinitions(taskDefinitions); + return sanitizeAndAugmentTaskDefinitions(taskDefinitions); }; expect(runsanitize).toThrowErrorMatchingInlineSnapshot( @@ -135,7 +138,7 @@ describe('taskTypeDictionary', () => { }, }; - return sanitizeTaskDefinitions(taskDefinitions); + return sanitizeAndAugmentTaskDefinitions(taskDefinitions); }; expect(runsanitize).toThrowErrorMatchingInlineSnapshot( @@ -162,7 +165,7 @@ describe('taskTypeDictionary', () => { }, }; - return sanitizeTaskDefinitions(taskDefinitions); + return sanitizeAndAugmentTaskDefinitions(taskDefinitions); }; expect(runsanitize).toThrowErrorMatchingInlineSnapshot( diff --git a/x-pack/plugins/task_manager/server/task_type_dictionary.ts b/x-pack/plugins/task_manager/server/task_type_dictionary.ts index f2b6d5b9153b1e..e6fbb7731130a5 100644 --- a/x-pack/plugins/task_manager/server/task_type_dictionary.ts +++ b/x-pack/plugins/task_manager/server/task_type_dictionary.ts @@ -5,6 +5,7 @@ * 2.0. */ +import { ObjectType } from '@kbn/config-schema'; import { Logger } from '@kbn/core/server'; import { TaskDefinition, taskDefinitionSchema, TaskRunCreatorFunction } from './task'; import { CONCURRENCY_ALLOW_LIST_BY_TASK_TYPE } from './constants'; @@ -65,6 +66,13 @@ export interface TaskRegisterDefinition { * The default value, if not given, is 0. */ maxConcurrency?: number; + stateSchemaByVersion?: Record< + number, + { + schema: ObjectType; + up: (state: Record) => Record; + } + >; } /** @@ -138,7 +146,7 @@ export class TaskTypeDictionary { } try { - for (const definition of sanitizeTaskDefinitions(taskDefinitions)) { + for (const definition of sanitizeAndAugmentTaskDefinitions(taskDefinitions)) { this.definitions.set(definition.type, definition); } } catch (e) { @@ -153,8 +161,20 @@ export class TaskTypeDictionary { * * @param taskDefinitions - The Kibana task definitions dictionary */ -export function sanitizeTaskDefinitions(taskDefinitions: TaskDefinitionRegistry): TaskDefinition[] { - return Object.entries(taskDefinitions).map(([type, rawDefinition]) => { - return taskDefinitionSchema.validate({ type, ...rawDefinition }) as TaskDefinition; +export function sanitizeAndAugmentTaskDefinitions( + taskDefinitions: TaskDefinitionRegistry +): TaskDefinition[] { + return Object.entries(taskDefinitions).map(([type, rawDefinition]): TaskDefinition => { + const validatedDefinition = taskDefinitionSchema.validate({ + type, + ...rawDefinition, + }); + return { + ...validatedDefinition, + createTaskRunner: rawDefinition.createTaskRunner, + // validatedDefinition contains Record and we want Record + // so reverting to rawDefinition. + stateSchemaByVersion: rawDefinition.stateSchemaByVersion, + }; }); } diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts new file mode 100644 index 00000000000000..f8701e1ea49b3b --- /dev/null +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -0,0 +1,146 @@ +/* + * 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 { isEmpty, max } from 'lodash'; +import { type ObjectType } from '@kbn/config-schema'; +import { TaskTypeDictionary } from './task_type_dictionary'; +import type { TaskInstance, ConcreteTaskInstance, TaskDefinition } from './task'; + +interface TaskValidatorOpts { + allowReadingInvalidState: boolean; + definitions: TaskTypeDictionary; +} + +type LatestStateSchema = + | undefined + | { + schema: ObjectType; + version: number; + up: (state: Record) => Record; + }; + +export class TaskValidator { + private readonly definitions: TaskTypeDictionary; + private readonly allowReadingInvalidState: boolean; + + constructor({ definitions, allowReadingInvalidState }: TaskValidatorOpts) { + this.definitions = definitions; + this.allowReadingInvalidState = allowReadingInvalidState; + } + + public getValidatedTaskInstance(task: T, mode: 'read' | 'write'): T { + // In the scenario the task is unused / deprecated and Kibana needs to manipulate the task + // we'll do a pass-through for those + if (!this.definitions.has(task.taskType)) { + return task; + } + + const taskTypeDef = this.definitions.get(task.taskType); + const lastestStateSchema = this.getLatestSchema(taskTypeDef); + + // TODO: Remove once all task types report their state schema + if (!lastestStateSchema) { + return task; + } + + if (mode === 'read') { + let state = task.state; + try { + state = this.getValidatedStateSchema( + this.migrateTaskState(task.state, task.stateVersion, taskTypeDef, lastestStateSchema), + task.taskType, + lastestStateSchema, + 'ignore' + ); + } catch (e) { + if (!this.allowReadingInvalidState) { + throw e; + } + // TODO Log / telemetry + } + + return { + ...task, + state, + }; + } + + return { + ...task, + state: this.getValidatedStateSchema(task.state, task.taskType, lastestStateSchema, 'forbid'), + stateVersion: lastestStateSchema?.version, + }; + } + + private migrateTaskState( + state: ConcreteTaskInstance['state'], + currentVersion: number | undefined, + taskTypeDef: TaskDefinition, + lastestStateSchema: LatestStateSchema + ) { + if (!lastestStateSchema || (currentVersion && currentVersion >= lastestStateSchema.version)) { + return state; + } + + let migratedState = state; + for (let i = currentVersion || 1; i <= lastestStateSchema.version; i++) { + if (!taskTypeDef.stateSchemaByVersion || !taskTypeDef.stateSchemaByVersion[`${i}`]) { + throw new Error( + `[TaskValidator] state schema for ${taskTypeDef.type} missing version: ${i}` + ); + } + migratedState = taskTypeDef.stateSchemaByVersion[i].up(migratedState); + try { + taskTypeDef.stateSchemaByVersion[i].schema.validate(migratedState); + } catch (e) { + throw new Error( + `[TaskValidator] failed to migrate to version ${i} because the data returned from the up migration doesn't match the schema: ${e.message}` + ); + } + } + + return migratedState; + } + + private getValidatedStateSchema( + state: ConcreteTaskInstance['state'], + taskType: string, + latestStateSchema: LatestStateSchema, + unknowns: 'forbid' | 'ignore' + ): ConcreteTaskInstance['state'] { + if (isEmpty(state)) { + return {}; + } + + if (!latestStateSchema) { + throw new Error( + `[TaskValidator] stateSchemaByVersion not defined for task type: ${taskType}` + ); + } + + return latestStateSchema.schema.extendsDeep({ unknowns }).validate(state); + } + + private getLatestSchema(taskTypeDef: TaskDefinition): LatestStateSchema { + if (!taskTypeDef.stateSchemaByVersion) { + return; + } + + const versions = Object.keys(taskTypeDef.stateSchemaByVersion).map((v) => parseInt(v, 10)); + const latest = max(versions); + + if (latest === undefined) { + return; + } + + return { + version: latest, + schema: taskTypeDef.stateSchemaByVersion[latest].schema, + up: taskTypeDef.stateSchemaByVersion[latest].up, + }; + } +} From 2c9d7384d96b354ebbe672838fe3d055fe6bf638 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 5 Jun 2023 13:20:40 -0400 Subject: [PATCH 02/29] Add note to set config default value back to true --- x-pack/plugins/task_manager/server/config.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugins/task_manager/server/config.ts b/x-pack/plugins/task_manager/server/config.ts index 3f8c1a9345fca1..052fcb6297b4f6 100644 --- a/x-pack/plugins/task_manager/server/config.ts +++ b/x-pack/plugins/task_manager/server/config.ts @@ -137,6 +137,7 @@ export const configSchema = schema.object( exclude_task_types: schema.arrayOf(schema.string(), { defaultValue: [] }), authenticate_background_task_utilization: schema.boolean({ defaultValue: true }), }), + // TODO: Set default value back to `true` allow_reading_invalid_state: schema.boolean({ defaultValue: false }), }, { From cac7f080df11f736da65ec2d7c6cfffe7958dd23 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 5 Jun 2023 13:23:55 -0400 Subject: [PATCH 03/29] Remove reference to x-pack/plugins/task_manager/server/task_pool.test.ts --- x-pack/plugins/task_manager/server/task_pool.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugins/task_manager/server/task_pool.test.ts b/x-pack/plugins/task_manager/server/task_pool.test.ts index cfcd0f4d8644c6..10e440184ab2eb 100644 --- a/x-pack/plugins/task_manager/server/task_pool.test.ts +++ b/x-pack/plugins/task_manager/server/task_pool.test.ts @@ -422,7 +422,6 @@ describe('TaskPool', () => { type: '', title: '', timeout: '5m', - getLatestStateSchema: () => undefined, createTaskRunner: jest.fn(), }; }, From 1412eb16d961bf7117d86914101d020e2d169386 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 5 Jun 2023 14:10:58 -0400 Subject: [PATCH 04/29] Add some task validator jest tests --- .../server/task_validator.test.ts | 208 ++++++++++++++++++ 1 file changed, 208 insertions(+) create mode 100644 x-pack/plugins/task_manager/server/task_validator.test.ts diff --git a/x-pack/plugins/task_manager/server/task_validator.test.ts b/x-pack/plugins/task_manager/server/task_validator.test.ts new file mode 100644 index 00000000000000..e179471f73893e --- /dev/null +++ b/x-pack/plugins/task_manager/server/task_validator.test.ts @@ -0,0 +1,208 @@ +/* + * 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 { taskManagerMock } from './mocks'; +import { mockLogger } from './test_utils'; +import { TaskValidator } from './task_validator'; +import { TaskTypeDictionary } from './task_type_dictionary'; + +const fooTaskDefinition = { + title: 'Foo', + description: '', + createTaskRunner() { + return { + async run() { + return { + state: {}, + }; + }, + }; + }, +}; + +describe('TaskValidator', () => { + describe('getValidatedTaskInstance()', () => { + it(`should return the task as-is whenever the task definition isn't in the definitions dictionary`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ foo: fooTaskDefinition }); + const taskValidator = new TaskValidator({ + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask(); + const result = taskValidator.getValidatedTaskInstance(task, 'read'); + expect(result).toEqual(task); + }); + + it(`should validate the state schema on write`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + }, + }, + }); + const taskValidator = new TaskValidator({ + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ state: { foo: 'bar' } }); + const { stateVersion, ...result } = taskValidator.getValidatedTaskInstance(task, 'write'); + expect(result).toEqual(task); + expect(stateVersion).toEqual(1); + }); + + it(`should fail to validate the state schema on write when unknown fields are present`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + }, + }, + }); + const taskValidator = new TaskValidator({ + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ state: { foo: 'foo', bar: 'bar' } }); + expect(() => + taskValidator.getValidatedTaskInstance(task, 'write') + ).toThrowErrorMatchingInlineSnapshot(`"[bar]: definition for this key is missing"`); + }); + + it(`should validate the state schema on read`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + }, + }, + }); + const taskValidator = new TaskValidator({ + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'bar' } }); + const result = taskValidator.getValidatedTaskInstance(task, 'read'); + expect(result).toEqual(task); + }); + + it(`should remove unknown fields when reading`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + }, + }, + }); + const taskValidator = new TaskValidator({ + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ + stateVersion: 1, + state: { foo: 'foo', bar: 'bar' }, + }); + const result = taskValidator.getValidatedTaskInstance(task, 'read'); + expect(result.state).toEqual({ foo: 'foo' }); + }); + + it(`should migrate state when reading from an older version`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + 2: { + up: (state) => ({ ...state, baz: 'baz' }), + schema: schema.object({ + foo: schema.string(), + baz: schema.string(), + }), + }, + }, + }, + }); + const taskValidator = new TaskValidator({ + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'foo' } }); + const result = taskValidator.getValidatedTaskInstance(task, 'read'); + expect(result.state).toEqual({ foo: 'foo', baz: 'baz' }); + }); + + it(`should throw during the migration phase if a schema version is missing`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + 3: { + up: (state) => ({ ...state, baz: 'baz' }), + schema: schema.object({ + foo: schema.string(), + baz: schema.string(), + }), + }, + }, + }, + }); + const taskValidator = new TaskValidator({ + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'foo' } }); + expect(() => + taskValidator.getValidatedTaskInstance(task, 'read') + ).toThrowErrorMatchingInlineSnapshot( + `"[TaskValidator] state schema for foo missing version: 2"` + ); + }); + }); +}); From d40c58a00454f6d821c56812067c6c403b7a5a4a Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 5 Jun 2023 14:44:39 -0400 Subject: [PATCH 05/29] Debug log validation failures --- x-pack/plugins/task_manager/server/plugin.ts | 1 + .../plugins/task_manager/server/task_store.test.ts | 13 +++++++++++++ x-pack/plugins/task_manager/server/task_store.ts | 4 +++- .../task_manager/server/task_validator.test.ts | 7 +++++++ .../plugins/task_manager/server/task_validator.ts | 13 ++++++++++--- 5 files changed, 34 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/task_manager/server/plugin.ts b/x-pack/plugins/task_manager/server/plugin.ts index e771138c7da396..3bf6bd13608710 100644 --- a/x-pack/plugins/task_manager/server/plugin.ts +++ b/x-pack/plugins/task_manager/server/plugin.ts @@ -213,6 +213,7 @@ export class TaskManagerPlugin taskManagerId: `kibana:${this.taskManagerId!}`, adHocTaskCounter: this.adHocTaskCounter, allowReadingInvalidState: this.config.allow_reading_invalid_state, + logger: this.logger, }); const managedConfiguration = createManagedConfiguration({ diff --git a/x-pack/plugins/task_manager/server/task_store.test.ts b/x-pack/plugins/task_manager/server/task_store.test.ts index b194f42f6bfcfb..19e179d5e890a7 100644 --- a/x-pack/plugins/task_manager/server/task_store.test.ts +++ b/x-pack/plugins/task_manager/server/task_store.test.ts @@ -76,6 +76,7 @@ describe('TaskStore', () => { beforeAll(() => { store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, @@ -249,6 +250,7 @@ describe('TaskStore', () => { childEsClient = elasticsearchServiceMock.createClusterClient().asInternalUser; esClient.child.mockReturnValue(childEsClient as unknown as Client); store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, @@ -319,6 +321,7 @@ describe('TaskStore', () => { beforeAll(() => { esClient = elasticsearchServiceMock.createClusterClient().asInternalUser; store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, @@ -418,6 +421,7 @@ describe('TaskStore', () => { beforeAll(() => { esClient = elasticsearchServiceMock.createClusterClient().asInternalUser; store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, @@ -524,6 +528,7 @@ describe('TaskStore', () => { beforeAll(() => { store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, @@ -566,6 +571,7 @@ describe('TaskStore', () => { beforeAll(() => { store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, @@ -601,6 +607,7 @@ describe('TaskStore', () => { beforeAll(() => { store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, @@ -636,6 +643,7 @@ describe('TaskStore', () => { beforeAll(() => { store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, @@ -695,6 +703,7 @@ describe('TaskStore', () => { beforeAll(() => { store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, @@ -789,6 +798,7 @@ describe('TaskStore', () => { })); const store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, @@ -810,6 +820,7 @@ describe('TaskStore', () => { ); const store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, @@ -829,6 +840,7 @@ describe('TaskStore', () => { ); const store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, @@ -848,6 +860,7 @@ describe('TaskStore', () => { beforeAll(() => { store = new TaskStore({ + logger: mockLogger(), index: 'tasky', taskManagerId: '', serializer, diff --git a/x-pack/plugins/task_manager/server/task_store.ts b/x-pack/plugins/task_manager/server/task_store.ts index 5cf2bb13755a6e..ac6476a03ad9ce 100644 --- a/x-pack/plugins/task_manager/server/task_store.ts +++ b/x-pack/plugins/task_manager/server/task_store.ts @@ -13,7 +13,7 @@ import { omit, defaults, get } from 'lodash'; import { SavedObjectError } from '@kbn/core-saved-objects-common'; import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; -import type { SavedObjectsBulkDeleteResponse } from '@kbn/core/server'; +import type { SavedObjectsBulkDeleteResponse, Logger } from '@kbn/core/server'; import { SavedObject, @@ -47,6 +47,7 @@ export interface StoreOpts { serializer: ISavedObjectsSerializer; adHocTaskCounter: AdHocTaskCounter; allowReadingInvalidState: boolean; + logger: Logger; } export interface SearchOpts { @@ -126,6 +127,7 @@ export class TaskStore { this.savedObjectsRepository = opts.savedObjectsRepository; this.adHocTaskCounter = opts.adHocTaskCounter; this.taskValidator = new TaskValidator({ + logger: opts.logger, definitions: opts.definitions, allowReadingInvalidState: opts.allowReadingInvalidState, }); diff --git a/x-pack/plugins/task_manager/server/task_validator.test.ts b/x-pack/plugins/task_manager/server/task_validator.test.ts index e179471f73893e..4d97eed63cd955 100644 --- a/x-pack/plugins/task_manager/server/task_validator.test.ts +++ b/x-pack/plugins/task_manager/server/task_validator.test.ts @@ -31,6 +31,7 @@ describe('TaskValidator', () => { const definitions = new TaskTypeDictionary(mockLogger()); definitions.registerTaskDefinitions({ foo: fooTaskDefinition }); const taskValidator = new TaskValidator({ + logger: mockLogger(), definitions, allowReadingInvalidState: false, }); @@ -55,6 +56,7 @@ describe('TaskValidator', () => { }, }); const taskValidator = new TaskValidator({ + logger: mockLogger(), definitions, allowReadingInvalidState: false, }); @@ -80,6 +82,7 @@ describe('TaskValidator', () => { }, }); const taskValidator = new TaskValidator({ + logger: mockLogger(), definitions, allowReadingInvalidState: false, }); @@ -105,6 +108,7 @@ describe('TaskValidator', () => { }, }); const taskValidator = new TaskValidator({ + logger: mockLogger(), definitions, allowReadingInvalidState: false, }); @@ -129,6 +133,7 @@ describe('TaskValidator', () => { }, }); const taskValidator = new TaskValidator({ + logger: mockLogger(), definitions, allowReadingInvalidState: false, }); @@ -163,6 +168,7 @@ describe('TaskValidator', () => { }, }); const taskValidator = new TaskValidator({ + logger: mockLogger(), definitions, allowReadingInvalidState: false, }); @@ -194,6 +200,7 @@ describe('TaskValidator', () => { }, }); const taskValidator = new TaskValidator({ + logger: mockLogger(), definitions, allowReadingInvalidState: false, }); diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts index f8701e1ea49b3b..7802e6072dc9c2 100644 --- a/x-pack/plugins/task_manager/server/task_validator.ts +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -6,13 +6,15 @@ */ import { isEmpty, max } from 'lodash'; -import { type ObjectType } from '@kbn/config-schema'; +import type { Logger } from '@kbn/core/server'; +import type { ObjectType } from '@kbn/config-schema'; import { TaskTypeDictionary } from './task_type_dictionary'; import type { TaskInstance, ConcreteTaskInstance, TaskDefinition } from './task'; interface TaskValidatorOpts { allowReadingInvalidState: boolean; definitions: TaskTypeDictionary; + logger: Logger; } type LatestStateSchema = @@ -24,10 +26,12 @@ type LatestStateSchema = }; export class TaskValidator { + private readonly logger: Logger; private readonly definitions: TaskTypeDictionary; private readonly allowReadingInvalidState: boolean; - constructor({ definitions, allowReadingInvalidState }: TaskValidatorOpts) { + constructor({ definitions, allowReadingInvalidState, logger }: TaskValidatorOpts) { + this.logger = logger; this.definitions = definitions; this.allowReadingInvalidState = allowReadingInvalidState; } @@ -60,7 +64,10 @@ export class TaskValidator { if (!this.allowReadingInvalidState) { throw e; } - // TODO Log / telemetry + this.logger.debug( + `[${task.taskType}][${task.id}] State validation failure, but allowing to proceed given allow_reading_invalid_state is true: ${e.message}` + ); + // TODO telemetry } return { From ba040efcc91c6c418a1944f88fa63168cf8912ba Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Tue, 6 Jun 2023 06:55:20 -0400 Subject: [PATCH 06/29] Move getLatestStateSchema out of class --- .../task_manager/server/task_validator.ts | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts index 7802e6072dc9c2..28a520443a0d4e 100644 --- a/x-pack/plugins/task_manager/server/task_validator.ts +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -44,7 +44,7 @@ export class TaskValidator { } const taskTypeDef = this.definitions.get(task.taskType); - const lastestStateSchema = this.getLatestSchema(taskTypeDef); + const lastestStateSchema = getLatestStateSchema(taskTypeDef); // TODO: Remove once all task types report their state schema if (!lastestStateSchema) { @@ -131,23 +131,23 @@ export class TaskValidator { return latestStateSchema.schema.extendsDeep({ unknowns }).validate(state); } +} - private getLatestSchema(taskTypeDef: TaskDefinition): LatestStateSchema { - if (!taskTypeDef.stateSchemaByVersion) { - return; - } - - const versions = Object.keys(taskTypeDef.stateSchemaByVersion).map((v) => parseInt(v, 10)); - const latest = max(versions); +function getLatestStateSchema(taskTypeDef: TaskDefinition): LatestStateSchema { + if (!taskTypeDef.stateSchemaByVersion) { + return; + } - if (latest === undefined) { - return; - } + const versions = Object.keys(taskTypeDef.stateSchemaByVersion).map((v) => parseInt(v, 10)); + const latest = max(versions); - return { - version: latest, - schema: taskTypeDef.stateSchemaByVersion[latest].schema, - up: taskTypeDef.stateSchemaByVersion[latest].up, - }; + if (latest === undefined) { + return; } + + return { + version: latest, + schema: taskTypeDef.stateSchemaByVersion[latest].schema, + up: taskTypeDef.stateSchemaByVersion[latest].up, + }; } From c471e16dcf00722c9ed4b92ff72711a840c99cff Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Tue, 6 Jun 2023 07:00:50 -0400 Subject: [PATCH 07/29] Fix some jest tests --- .../server/task_validator.test.ts | 19 ++++++++++++++++++- .../task_manager/server/task_validator.ts | 8 ++++---- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/task_manager/server/task_validator.test.ts b/x-pack/plugins/task_manager/server/task_validator.test.ts index 4d97eed63cd955..9cfa96aac483f0 100644 --- a/x-pack/plugins/task_manager/server/task_validator.test.ts +++ b/x-pack/plugins/task_manager/server/task_validator.test.ts @@ -29,7 +29,6 @@ describe('TaskValidator', () => { describe('getValidatedTaskInstance()', () => { it(`should return the task as-is whenever the task definition isn't in the definitions dictionary`, () => { const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ foo: fooTaskDefinition }); const taskValidator = new TaskValidator({ logger: mockLogger(), definitions, @@ -66,6 +65,24 @@ describe('TaskValidator', () => { expect(stateVersion).toEqual(1); }); + it(`should fail to validate the state schema when the task type doesn't have stateSchemaByVersion defined`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: fooTaskDefinition, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ state: { foo: 'bar' } }); + expect(() => + taskValidator.getValidatedTaskInstance(task, 'write') + ).toThrowErrorMatchingInlineSnapshot( + `"[TaskValidator] stateSchemaByVersion not defined for task type: foo"` + ); + }); + it(`should fail to validate the state schema on write when unknown fields are present`, () => { const definitions = new TaskTypeDictionary(mockLogger()); definitions.registerTaskDefinitions({ diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts index 28a520443a0d4e..63e18817b25ee1 100644 --- a/x-pack/plugins/task_manager/server/task_validator.ts +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -46,10 +46,10 @@ export class TaskValidator { const taskTypeDef = this.definitions.get(task.taskType); const lastestStateSchema = getLatestStateSchema(taskTypeDef); - // TODO: Remove once all task types report their state schema - if (!lastestStateSchema) { - return task; - } + // // TODO: Remove once all task types report their state schema + // if (!lastestStateSchema) { + // return task; + // } if (mode === 'read') { let state = task.state; From 532d9bd39d60ae57f86cfaa72a3b79ff8f7a61f8 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Tue, 6 Jun 2023 08:14:26 -0400 Subject: [PATCH 08/29] Fix tests --- x-pack/plugins/task_manager/server/task_validator.test.ts | 2 +- x-pack/plugins/task_manager/server/task_validator.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/task_manager/server/task_validator.test.ts b/x-pack/plugins/task_manager/server/task_validator.test.ts index 9cfa96aac483f0..ba3d0b5351fc58 100644 --- a/x-pack/plugins/task_manager/server/task_validator.test.ts +++ b/x-pack/plugins/task_manager/server/task_validator.test.ts @@ -65,7 +65,7 @@ describe('TaskValidator', () => { expect(stateVersion).toEqual(1); }); - it(`should fail to validate the state schema when the task type doesn't have stateSchemaByVersion defined`, () => { + it.skip(`should fail to validate the state schema when the task type doesn't have stateSchemaByVersion defined`, () => { const definitions = new TaskTypeDictionary(mockLogger()); definitions.registerTaskDefinitions({ foo: fooTaskDefinition, diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts index 63e18817b25ee1..28a520443a0d4e 100644 --- a/x-pack/plugins/task_manager/server/task_validator.ts +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -46,10 +46,10 @@ export class TaskValidator { const taskTypeDef = this.definitions.get(task.taskType); const lastestStateSchema = getLatestStateSchema(taskTypeDef); - // // TODO: Remove once all task types report their state schema - // if (!lastestStateSchema) { - // return task; - // } + // TODO: Remove once all task types report their state schema + if (!lastestStateSchema) { + return task; + } if (mode === 'read') { let state = task.state; From 100a7e3b606dc312c234ff59badbe3da9a336e42 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Wed, 7 Jun 2023 11:41:04 -0400 Subject: [PATCH 09/29] Code improvements and comments --- .../plugins/task_manager/server/task_store.ts | 1 + .../server/task_validator.test.ts | 377 +++++++++++------- .../task_manager/server/task_validator.ts | 11 +- 3 files changed, 238 insertions(+), 151 deletions(-) diff --git a/x-pack/plugins/task_manager/server/task_store.ts b/x-pack/plugins/task_manager/server/task_store.ts index ac6476a03ad9ce..be23052d07aa69 100644 --- a/x-pack/plugins/task_manager/server/task_store.ts +++ b/x-pack/plugins/task_manager/server/task_store.ts @@ -244,6 +244,7 @@ export class TaskStore { * @returns {Promise} */ public async update(doc: ConcreteTaskInstance): Promise { + // TODO: Should we force validation when claiming tasks? State could fail validation const validatedTaskInstance = this.taskValidator.getValidatedTaskInstance(doc, 'write'); const attributes = taskInstanceToAttributes(validatedTaskInstance); diff --git a/x-pack/plugins/task_manager/server/task_validator.test.ts b/x-pack/plugins/task_manager/server/task_validator.test.ts index ba3d0b5351fc58..c0c368575cd6ba 100644 --- a/x-pack/plugins/task_manager/server/task_validator.test.ts +++ b/x-pack/plugins/task_manager/server/task_validator.test.ts @@ -27,7 +27,7 @@ const fooTaskDefinition = { describe('TaskValidator', () => { describe('getValidatedTaskInstance()', () => { - it(`should return the task as-is whenever the task definition isn't in the definitions dictionary`, () => { + it(`should return the task as-is whenever the task definition isn't defined`, () => { const definitions = new TaskTypeDictionary(mockLogger()); const taskValidator = new TaskValidator({ logger: mockLogger(), @@ -39,32 +39,6 @@ describe('TaskValidator', () => { expect(result).toEqual(task); }); - it(`should validate the state schema on write`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), - }, - }, - }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, - }); - const task = taskManagerMock.createTask({ state: { foo: 'bar' } }); - const { stateVersion, ...result } = taskValidator.getValidatedTaskInstance(task, 'write'); - expect(result).toEqual(task); - expect(stateVersion).toEqual(1); - }); - it.skip(`should fail to validate the state schema when the task type doesn't have stateSchemaByVersion defined`, () => { const definitions = new TaskTypeDictionary(mockLogger()); definitions.registerTaskDefinitions({ @@ -83,150 +57,259 @@ describe('TaskValidator', () => { ); }); - it(`should fail to validate the state schema on write when unknown fields are present`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), + describe('mode=write', () => { + it(`should validate the state schema`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, }, }, - }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ state: { foo: 'bar' } }); + const { stateVersion, ...result } = taskValidator.getValidatedTaskInstance(task, 'write'); + expect(result).toEqual(task); + expect(stateVersion).toEqual(1); }); - const task = taskManagerMock.createTask({ state: { foo: 'foo', bar: 'bar' } }); - expect(() => - taskValidator.getValidatedTaskInstance(task, 'write') - ).toThrowErrorMatchingInlineSnapshot(`"[bar]: definition for this key is missing"`); - }); - it(`should validate the state schema on read`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), + it(`should fail to validate the state schema when unknown fields are present`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, }, }, - }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ state: { foo: 'foo', bar: 'bar' } }); + expect(() => + taskValidator.getValidatedTaskInstance(task, 'write') + ).toThrowErrorMatchingInlineSnapshot(`"[bar]: definition for this key is missing"`); }); - const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'bar' } }); - const result = taskValidator.getValidatedTaskInstance(task, 'read'); - expect(result).toEqual(task); }); - it(`should remove unknown fields when reading`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), + describe('mode=read', () => { + it(`should validate the state schema`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, }, }, - }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, - }); - const task = taskManagerMock.createTask({ - stateVersion: 1, - state: { foo: 'foo', bar: 'bar' }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'bar' } }); + const result = taskValidator.getValidatedTaskInstance(task, 'read'); + expect(result).toEqual(task); }); - const result = taskValidator.getValidatedTaskInstance(task, 'read'); - expect(result.state).toEqual({ foo: 'foo' }); - }); - it(`should migrate state when reading from an older version`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), + it(`should fail validation when the state schema doesn't match the state data`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, }, - 2: { - up: (state) => ({ ...state, baz: 'baz' }), - schema: schema.object({ - foo: schema.string(), - baz: schema.string(), - }), + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: true } }); + expect(() => + taskValidator.getValidatedTaskInstance(task, 'read') + ).toThrowErrorMatchingInlineSnapshot( + `"[foo]: expected value of type [string] but got [boolean]"` + ); + }); + + it(`should return original state when the state is invalid and allowReadingInvalidState is true`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, }, }, - }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: true, + }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: true } }); + const result = taskValidator.getValidatedTaskInstance(task, 'read'); + expect(result.state).toEqual({ foo: true }); }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, + + it(`should remove unknown fields`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + }, + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ + stateVersion: 1, + state: { foo: 'foo', bar: 'bar' }, + }); + const result = taskValidator.getValidatedTaskInstance(task, 'read'); + expect(result.state).toEqual({ foo: 'foo' }); }); - const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'foo' } }); - const result = taskValidator.getValidatedTaskInstance(task, 'read'); - expect(result.state).toEqual({ foo: 'foo', baz: 'baz' }); - }); - it(`should throw during the migration phase if a schema version is missing`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), + it(`should migrate state when reading from a document without stateVersion`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => ({ ...state, baz: 'baz' }), + schema: schema.object({ + foo: schema.string(), + baz: schema.string(), + }), + }, }, - 3: { - up: (state) => ({ ...state, baz: 'baz' }), - schema: schema.object({ - foo: schema.string(), - baz: schema.string(), - }), + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ stateVersion: undefined, state: { foo: 'foo' } }); + const result = taskValidator.getValidatedTaskInstance(task, 'read'); + expect(result.state).toEqual({ foo: 'foo', baz: 'baz' }); + }); + + it(`should migrate state when reading from an older version`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + 2: { + up: (state) => ({ ...state, baz: 'baz' }), + schema: schema.object({ + foo: schema.string(), + baz: schema.string(), + }), + }, }, }, - }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'foo' } }); + const result = taskValidator.getValidatedTaskInstance(task, 'read'); + expect(result.state).toEqual({ foo: 'foo', baz: 'baz' }); }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, + + it(`should throw during the migration phase if a schema version is missing`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + 3: { + up: (state) => ({ ...state, baz: 'baz' }), + schema: schema.object({ + foo: schema.string(), + baz: schema.string(), + }), + }, + }, + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'foo' } }); + expect(() => + taskValidator.getValidatedTaskInstance(task, 'read') + ).toThrowErrorMatchingInlineSnapshot( + `"[TaskValidator] state schema for foo missing version: 2"` + ); }); - const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'foo' } }); - expect(() => - taskValidator.getValidatedTaskInstance(task, 'read') - ).toThrowErrorMatchingInlineSnapshot( - `"[TaskValidator] state schema for foo missing version: 2"` - ); }); }); }); diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts index 28a520443a0d4e..87410801c74c2e 100644 --- a/x-pack/plugins/task_manager/server/task_validator.ts +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -37,16 +37,18 @@ export class TaskValidator { } public getValidatedTaskInstance(task: T, mode: 'read' | 'write'): T { - // In the scenario the task is unused / deprecated and Kibana needs to manipulate the task + // In the scenario the task is unused / deprecated and Kibana needs to manipulate the task, // we'll do a pass-through for those if (!this.definitions.has(task.taskType)) { return task; } const taskTypeDef = this.definitions.get(task.taskType); + // TODO: Cache by type const lastestStateSchema = getLatestStateSchema(taskTypeDef); - // TODO: Remove once all task types report their state schema + // TODO: Remove once all task types have defined their state schema. + // Otherwise, failures on read / write would occur. (don't forget to unskip test) if (!lastestStateSchema) { return task; } @@ -65,9 +67,8 @@ export class TaskValidator { throw e; } this.logger.debug( - `[${task.taskType}][${task.id}] State validation failure, but allowing to proceed given allow_reading_invalid_state is true: ${e.message}` + `[${task.taskType}][${task.id}] Failed to validate the task's state. Allowing read operation to proceed because allow_reading_invalid_state is true. Error: ${e.message}` ); - // TODO telemetry } return { @@ -76,6 +77,7 @@ export class TaskValidator { }; } + // We are doing a write operation which must validate against the latest state schema return { ...task, state: this.getValidatedStateSchema(task.state, task.taskType, lastestStateSchema, 'forbid'), @@ -129,6 +131,7 @@ export class TaskValidator { ); } + // TODO: Don't extendsDeep all the time return latestStateSchema.schema.extendsDeep({ unknowns }).validate(state); } } From 5dd7c142be6ea6ea10b9358d11aa60b9a8de65a1 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Wed, 7 Jun 2023 12:57:29 -0400 Subject: [PATCH 10/29] Revert some code --- .../server/task_type_dictionary.test.ts | 12 +++++------ .../server/task_type_dictionary.ts | 20 ++++--------------- 2 files changed, 10 insertions(+), 22 deletions(-) diff --git a/x-pack/plugins/task_manager/server/task_type_dictionary.test.ts b/x-pack/plugins/task_manager/server/task_type_dictionary.test.ts index a00832895dd451..18fa5fddd750d8 100644 --- a/x-pack/plugins/task_manager/server/task_type_dictionary.test.ts +++ b/x-pack/plugins/task_manager/server/task_type_dictionary.test.ts @@ -9,7 +9,7 @@ import { get } from 'lodash'; import { RunContext, TaskDefinition } from './task'; import { mockLogger } from './test_utils'; import { - sanitizeAndAugmentTaskDefinitions, + sanitizeTaskDefinitions, TaskDefinitionRegistry, TaskTypeDictionary, } from './task_type_dictionary'; @@ -55,10 +55,10 @@ describe('taskTypeDictionary', () => { definitions = new TaskTypeDictionary(mockLogger()); }); - describe('sanitizeAndAugmentTaskDefinitions', () => {}); + describe('sanitizeTaskDefinitions', () => {}); it('provides tasks with defaults', () => { const taskDefinitions = getMockTaskDefinitions({ numTasks: 3 }); - const result = sanitizeAndAugmentTaskDefinitions(taskDefinitions); + const result = sanitizeTaskDefinitions(taskDefinitions); expect(result).toMatchInlineSnapshot(` Array [ @@ -111,7 +111,7 @@ describe('taskTypeDictionary', () => { }, }; - return sanitizeAndAugmentTaskDefinitions(taskDefinitions); + return sanitizeTaskDefinitions(taskDefinitions); }; expect(runsanitize).toThrowErrorMatchingInlineSnapshot( @@ -138,7 +138,7 @@ describe('taskTypeDictionary', () => { }, }; - return sanitizeAndAugmentTaskDefinitions(taskDefinitions); + return sanitizeTaskDefinitions(taskDefinitions); }; expect(runsanitize).toThrowErrorMatchingInlineSnapshot( @@ -165,7 +165,7 @@ describe('taskTypeDictionary', () => { }, }; - return sanitizeAndAugmentTaskDefinitions(taskDefinitions); + return sanitizeTaskDefinitions(taskDefinitions); }; expect(runsanitize).toThrowErrorMatchingInlineSnapshot( diff --git a/x-pack/plugins/task_manager/server/task_type_dictionary.ts b/x-pack/plugins/task_manager/server/task_type_dictionary.ts index e6fbb7731130a5..30a4ee169f4e6d 100644 --- a/x-pack/plugins/task_manager/server/task_type_dictionary.ts +++ b/x-pack/plugins/task_manager/server/task_type_dictionary.ts @@ -146,7 +146,7 @@ export class TaskTypeDictionary { } try { - for (const definition of sanitizeAndAugmentTaskDefinitions(taskDefinitions)) { + for (const definition of sanitizeTaskDefinitions(taskDefinitions)) { this.definitions.set(definition.type, definition); } } catch (e) { @@ -161,20 +161,8 @@ export class TaskTypeDictionary { * * @param taskDefinitions - The Kibana task definitions dictionary */ -export function sanitizeAndAugmentTaskDefinitions( - taskDefinitions: TaskDefinitionRegistry -): TaskDefinition[] { - return Object.entries(taskDefinitions).map(([type, rawDefinition]): TaskDefinition => { - const validatedDefinition = taskDefinitionSchema.validate({ - type, - ...rawDefinition, - }); - return { - ...validatedDefinition, - createTaskRunner: rawDefinition.createTaskRunner, - // validatedDefinition contains Record and we want Record - // so reverting to rawDefinition. - stateSchemaByVersion: rawDefinition.stateSchemaByVersion, - }; +export function sanitizeTaskDefinitions(taskDefinitions: TaskDefinitionRegistry): TaskDefinition[] { + return Object.entries(taskDefinitions).map(([type, rawDefinition]) => { + return taskDefinitionSchema.validate({ type, ...rawDefinition }) as TaskDefinition; }); } From 50c00783a17a547aaef2bfee88250dcf4f7fc421 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Wed, 7 Jun 2023 14:11:24 -0400 Subject: [PATCH 11/29] Fix failed jest test --- .../plugins/task_manager/server/task_type_dictionary.test.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/x-pack/plugins/task_manager/server/task_type_dictionary.test.ts b/x-pack/plugins/task_manager/server/task_type_dictionary.test.ts index 18fa5fddd750d8..039403816da5e4 100644 --- a/x-pack/plugins/task_manager/server/task_type_dictionary.test.ts +++ b/x-pack/plugins/task_manager/server/task_type_dictionary.test.ts @@ -65,7 +65,6 @@ describe('taskTypeDictionary', () => { Object { "createTaskRunner": [Function], "description": "one super cool task", - "stateSchemaByVersion": undefined, "timeout": "5m", "title": "Test", "type": "test_task_type_0", @@ -73,7 +72,6 @@ describe('taskTypeDictionary', () => { Object { "createTaskRunner": [Function], "description": "one super cool task", - "stateSchemaByVersion": undefined, "timeout": "5m", "title": "Test", "type": "test_task_type_1", @@ -81,7 +79,6 @@ describe('taskTypeDictionary', () => { Object { "createTaskRunner": [Function], "description": "one super cool task", - "stateSchemaByVersion": undefined, "timeout": "5m", "title": "Test", "type": "test_task_type_2", From b9319da01ef635085d9b7804c5ebea4fb695e236 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 8 Jun 2023 07:12:07 -0400 Subject: [PATCH 12/29] Support specifying whether to validate or not when calling update and bulkUpdate --- .../server/buffered_task_store.mock.ts | 21 ++++++++++++ .../server/buffered_task_store.test.ts | 10 +++--- .../server/buffered_task_store.ts | 11 ++++--- .../server/lib/bulk_operation_buffer.test.ts | 12 ++++--- .../server/lib/bulk_operation_buffer.ts | 3 +- .../server/lib/retryable_bulk_update.test.ts | 19 +++++------ .../server/lib/retryable_bulk_update.ts | 4 ++- .../task_manager/server/polling_lifecycle.ts | 3 ++ .../server/task_running/task_runner.test.ts | 4 +-- .../server/task_scheduling.test.ts | 6 ++-- .../task_manager/server/task_scheduling.ts | 18 +++++++---- .../task_manager/server/task_store.test.ts | 12 ++++--- .../plugins/task_manager/server/task_store.ts | 32 +++++++++++++------ 13 files changed, 105 insertions(+), 50 deletions(-) create mode 100644 x-pack/plugins/task_manager/server/buffered_task_store.mock.ts diff --git a/x-pack/plugins/task_manager/server/buffered_task_store.mock.ts b/x-pack/plugins/task_manager/server/buffered_task_store.mock.ts new file mode 100644 index 00000000000000..efdea5a49bc384 --- /dev/null +++ b/x-pack/plugins/task_manager/server/buffered_task_store.mock.ts @@ -0,0 +1,21 @@ +/* + * 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 type { PublicMethodsOf } from '@kbn/utility-types'; +import { BufferedTaskStore } from './buffered_task_store'; + +const createBufferedTaskStoreMock = () => { + const mocked: jest.Mocked> = { + update: jest.fn(), + remove: jest.fn(), + }; + return mocked; +}; + +export const bufferedTaskStoreMock = { + create: createBufferedTaskStoreMock, +}; diff --git a/x-pack/plugins/task_manager/server/buffered_task_store.test.ts b/x-pack/plugins/task_manager/server/buffered_task_store.test.ts index 935192f7cc48e4..53fc13a19ef9bb 100644 --- a/x-pack/plugins/task_manager/server/buffered_task_store.test.ts +++ b/x-pack/plugins/task_manager/server/buffered_task_store.test.ts @@ -14,7 +14,7 @@ describe('Buffered Task Store', () => { test('proxies the TaskStore for `maxAttempts` and `remove`', async () => { const taskStore = taskStoreMock.create(); taskStore.bulkUpdate.mockResolvedValue([]); - const bufferedStore = new BufferedTaskStore(taskStore, {}); + const bufferedStore = new BufferedTaskStore(taskStore, { validate: true }); bufferedStore.remove('1'); expect(taskStore.remove).toHaveBeenCalledWith('1'); @@ -23,19 +23,19 @@ describe('Buffered Task Store', () => { describe('update', () => { test("proxies the TaskStore's `bulkUpdate`", async () => { const taskStore = taskStoreMock.create(); - const bufferedStore = new BufferedTaskStore(taskStore, {}); + const bufferedStore = new BufferedTaskStore(taskStore, { validate: true }); const task = taskManagerMock.createTask(); taskStore.bulkUpdate.mockResolvedValue([asOk(task)]); expect(await bufferedStore.update(task)).toMatchObject(task); - expect(taskStore.bulkUpdate).toHaveBeenCalledWith([task]); + expect(taskStore.bulkUpdate).toHaveBeenCalledWith([task], { validate: true }); }); test('handles partially successfull bulkUpdates resolving each call appropriately', async () => { const taskStore = taskStoreMock.create(); - const bufferedStore = new BufferedTaskStore(taskStore, {}); + const bufferedStore = new BufferedTaskStore(taskStore, { validate: true }); const tasks = [ taskManagerMock.createTask(), @@ -79,7 +79,7 @@ describe('Buffered Task Store', () => { test('handles multiple items with the same id', async () => { const taskStore = taskStoreMock.create(); - const bufferedStore = new BufferedTaskStore(taskStore, {}); + const bufferedStore = new BufferedTaskStore(taskStore, { validate: true }); const duplicateIdTask = taskManagerMock.createTask(); const tasks = [ diff --git a/x-pack/plugins/task_manager/server/buffered_task_store.ts b/x-pack/plugins/task_manager/server/buffered_task_store.ts index 0e24e8e49e43f2..78b779a69d1619 100644 --- a/x-pack/plugins/task_manager/server/buffered_task_store.ts +++ b/x-pack/plugins/task_manager/server/buffered_task_store.ts @@ -17,10 +17,13 @@ const DEFAULT_BUFFER_MAX_DURATION = 50; export class BufferedTaskStore implements Updatable { private bufferedUpdate: Operation; constructor(private readonly taskStore: TaskStore, options: BufferOptions) { - this.bufferedUpdate = createBuffer((docs) => taskStore.bulkUpdate(docs), { - bufferMaxDuration: DEFAULT_BUFFER_MAX_DURATION, - ...options, - }); + this.bufferedUpdate = createBuffer( + (docs) => taskStore.bulkUpdate(docs, { validate: options.validate }), + { + bufferMaxDuration: DEFAULT_BUFFER_MAX_DURATION, + ...options, + } + ); } public async update(doc: ConcreteTaskInstance): Promise { diff --git a/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.test.ts b/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.test.ts index 1832e1235aaf63..8b82ac4030c6ab 100644 --- a/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.test.ts +++ b/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.test.ts @@ -44,7 +44,7 @@ describe('Bulk Operation Buffer', () => { return Promise.resolve([incrementAttempts(task1), incrementAttempts(task2)]); }); - const bufferedUpdate = createBuffer(bulkUpdate); + const bufferedUpdate = createBuffer(bulkUpdate, { validate: true }); const task1 = createTask(); const task2 = createTask(); @@ -62,7 +62,7 @@ describe('Bulk Operation Buffer', () => { }); const bufferMaxDuration = 50; - const bufferedUpdate = createBuffer(bulkUpdate, { bufferMaxDuration }); + const bufferedUpdate = createBuffer(bulkUpdate, { bufferMaxDuration, validate: true }); const task1 = createTask(); const task2 = createTask(); @@ -102,6 +102,7 @@ describe('Bulk Operation Buffer', () => { const bufferedUpdate = createBuffer(bulkUpdate, { bufferMaxDuration, bufferMaxOperations: 2, + validate: true, }); const task1 = createTask(); @@ -135,6 +136,7 @@ describe('Bulk Operation Buffer', () => { const bufferedUpdate = createBuffer(bulkUpdate, { bufferMaxDuration, bufferMaxOperations: 3, + validate: true, }); const task1 = createTask(); @@ -173,7 +175,7 @@ describe('Bulk Operation Buffer', () => { } ); - const bufferedUpdate = createBuffer(bulkUpdate); + const bufferedUpdate = createBuffer(bulkUpdate, { validate: true }); const task1 = createTask(); const task2 = createTask(); @@ -195,7 +197,7 @@ describe('Bulk Operation Buffer', () => { return Promise.reject(new Error('bulkUpdate is an illusion')); }); - const bufferedUpdate = createBuffer(bulkUpdate); + const bufferedUpdate = createBuffer(bulkUpdate, { validate: true }); const task1 = createTask(); const task2 = createTask(); @@ -238,7 +240,7 @@ describe('Bulk Operation Buffer', () => { const logger = mockLogger(); - const bufferedUpdate = createBuffer(bulkUpdate, { logger }); + const bufferedUpdate = createBuffer(bulkUpdate, { logger, validate: true }); const task1 = createTask(); const task2 = createTask(); diff --git a/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.ts b/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.ts index 10bec75c846849..f83cde1b6a3515 100644 --- a/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.ts +++ b/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.ts @@ -16,6 +16,7 @@ export interface BufferOptions { bufferMaxDuration?: number; bufferMaxOperations?: number; logger?: Logger; + validate: boolean; } export interface Entity { @@ -39,7 +40,7 @@ const FLUSH = true; export function createBuffer( bulkOperation: BulkOperation, - { bufferMaxDuration = 0, bufferMaxOperations = Number.MAX_VALUE, logger }: BufferOptions = {} + { bufferMaxDuration = 0, bufferMaxOperations = Number.MAX_VALUE, logger, validate }: BufferOptions ): Operation { const flushBuffer = new Subject(); diff --git a/x-pack/plugins/task_manager/server/lib/retryable_bulk_update.test.ts b/x-pack/plugins/task_manager/server/lib/retryable_bulk_update.test.ts index aefcae67ecdd54..1eada39cb35392 100644 --- a/x-pack/plugins/task_manager/server/lib/retryable_bulk_update.test.ts +++ b/x-pack/plugins/task_manager/server/lib/retryable_bulk_update.test.ts @@ -32,25 +32,26 @@ describe('retryableBulkUpdate()', () => { }); it('should call getTasks with taskIds', async () => { - await retryableBulkUpdate({ taskIds, getTasks, filter, map, store }); + await retryableBulkUpdate({ taskIds, getTasks, filter, map, store, validate: false }); expect(getTasks).toHaveBeenCalledWith(taskIds); }); it('should filter tasks returned from getTasks', async () => { filter.mockImplementation((task) => task.id === '2'); - await retryableBulkUpdate({ taskIds, getTasks, filter, map, store }); + await retryableBulkUpdate({ taskIds, getTasks, filter, map, store, validate: false }); expect(filter).toHaveBeenCalledTimes(3); // Map happens after filter expect(map).toHaveBeenCalledTimes(1); - expect(store.bulkUpdate).toHaveBeenCalledWith([tasks[1]]); + expect(store.bulkUpdate).toHaveBeenCalledWith([tasks[1]], { validate: false }); }); it('should map tasks returned from getTasks', async () => { map.mockImplementation((task) => ({ ...task, status: TaskStatus.Claiming })); - await retryableBulkUpdate({ taskIds, getTasks, filter, map, store }); + await retryableBulkUpdate({ taskIds, getTasks, filter, map, store, validate: false }); expect(map).toHaveBeenCalledTimes(3); expect(store.bulkUpdate).toHaveBeenCalledWith( - tasks.map((task) => ({ ...task, status: TaskStatus.Claiming })) + tasks.map((task) => ({ ...task, status: TaskStatus.Claiming })), + { validate: false } ); }); @@ -71,9 +72,9 @@ describe('retryableBulkUpdate()', () => { ]); getTasks.mockResolvedValueOnce([tasks[0]].map((task) => asOk(task))); store.bulkUpdate.mockResolvedValueOnce(tasks.map((task) => asOk(task))); - await retryableBulkUpdate({ taskIds, getTasks, filter, map, store }); + await retryableBulkUpdate({ taskIds, getTasks, filter, map, store, validate: false }); expect(store.bulkUpdate).toHaveBeenCalledTimes(2); - expect(store.bulkUpdate).toHaveBeenNthCalledWith(2, [tasks[0]]); + expect(store.bulkUpdate).toHaveBeenNthCalledWith(2, [tasks[0]], { validate: false }); }); it('should skip updating tasks that cannot be found', async () => { @@ -86,7 +87,7 @@ describe('retryableBulkUpdate()', () => { }), asOk(tasks[2]), ]); - await retryableBulkUpdate({ taskIds, getTasks, filter, map, store }); - expect(store.bulkUpdate).toHaveBeenCalledWith([tasks[0], tasks[2]]); + await retryableBulkUpdate({ taskIds, getTasks, filter, map, store, validate: false }); + expect(store.bulkUpdate).toHaveBeenCalledWith([tasks[0], tasks[2]], { validate: false }); }); }); diff --git a/x-pack/plugins/task_manager/server/lib/retryable_bulk_update.ts b/x-pack/plugins/task_manager/server/lib/retryable_bulk_update.ts index 3c832e31af4da0..d50fb88d909c3f 100644 --- a/x-pack/plugins/task_manager/server/lib/retryable_bulk_update.ts +++ b/x-pack/plugins/task_manager/server/lib/retryable_bulk_update.ts @@ -19,6 +19,7 @@ export interface RetryableBulkUpdateOpts { filter: (task: ConcreteTaskInstance) => boolean; map: (task: ConcreteTaskInstance) => ConcreteTaskInstance; store: TaskStore; + validate: boolean; } export async function retryableBulkUpdate({ @@ -27,6 +28,7 @@ export async function retryableBulkUpdate({ filter, map, store, + validate, }: RetryableBulkUpdateOpts): Promise { const resultMap: Record = {}; @@ -42,7 +44,7 @@ export async function retryableBulkUpdate({ }, []) .filter(filter) .map(map); - const bulkUpdateResult = await store.bulkUpdate(tasksToUpdate); + const bulkUpdateResult = await store.bulkUpdate(tasksToUpdate, { validate }); for (const result of bulkUpdateResult) { const taskId = getId(result); resultMap[taskId] = result; diff --git a/x-pack/plugins/task_manager/server/polling_lifecycle.ts b/x-pack/plugins/task_manager/server/polling_lifecycle.ts index c60a8af2c92084..f664cf4a10e73e 100644 --- a/x-pack/plugins/task_manager/server/polling_lifecycle.ts +++ b/x-pack/plugins/task_manager/server/polling_lifecycle.ts @@ -116,6 +116,9 @@ export class TaskPollingLifecycle { this.bufferedStore = new BufferedTaskStore(this.store, { bufferMaxOperations: config.max_workers, logger, + // Buffer is used to remove and update tasks internally and operations + // should succeed regardless if the task state is invalid. + validate: false, }); this.pool = new TaskPool({ diff --git a/x-pack/plugins/task_manager/server/task_running/task_runner.test.ts b/x-pack/plugins/task_manager/server/task_running/task_runner.test.ts index 5812384c66c8eb..178964402c4680 100644 --- a/x-pack/plugins/task_manager/server/task_running/task_runner.test.ts +++ b/x-pack/plugins/task_manager/server/task_running/task_runner.test.ts @@ -23,10 +23,10 @@ import moment from 'moment'; import { TaskDefinitionRegistry, TaskTypeDictionary } from '../task_type_dictionary'; import { mockLogger } from '../test_utils'; import { throwRetryableError, throwUnrecoverableError } from './errors'; -import { taskStoreMock } from '../task_store.mock'; import apm from 'elastic-apm-node'; import { executionContextServiceMock } from '@kbn/core/server/mocks'; import { usageCountersServiceMock } from '@kbn/usage-collection-plugin/server/usage_counters/usage_counters_service.mock'; +import { bufferedTaskStoreMock } from '../buffered_task_store.mock'; import { calculateDelay, TASK_MANAGER_RUN_TRANSACTION_TYPE, @@ -1654,7 +1654,7 @@ describe('TaskManagerRunner', () => { const instance = mockInstance(opts.instance); - const store = taskStoreMock.create(); + const store = bufferedTaskStoreMock.create(); const usageCounter = usageCountersServiceMock.createSetupContract().createUsageCounter('test'); store.update.mockResolvedValue(instance); diff --git a/x-pack/plugins/task_manager/server/task_scheduling.test.ts b/x-pack/plugins/task_manager/server/task_scheduling.test.ts index 7f59f9f3625547..1a87bd59ec036f 100644 --- a/x-pack/plugins/task_manager/server/task_scheduling.test.ts +++ b/x-pack/plugins/task_manager/server/task_scheduling.test.ts @@ -538,7 +538,8 @@ describe('TaskScheduling', () => { status: TaskStatus.Idle, runAt: expect.any(Date), scheduledAt: expect.any(Date), - }) + }), + { validate: false } ); expect(mockTaskStore.get).toHaveBeenCalledWith(id); expect(result).toEqual({ id }); @@ -560,7 +561,8 @@ describe('TaskScheduling', () => { status: TaskStatus.Idle, runAt: expect.any(Date), scheduledAt: expect.any(Date), - }) + }), + { validate: false } ); expect(mockTaskStore.get).toHaveBeenCalledWith(id); expect(result).toEqual({ id }); diff --git a/x-pack/plugins/task_manager/server/task_scheduling.ts b/x-pack/plugins/task_manager/server/task_scheduling.ts index 86bf91048253f7..85c346d52da05f 100644 --- a/x-pack/plugins/task_manager/server/task_scheduling.ts +++ b/x-pack/plugins/task_manager/server/task_scheduling.ts @@ -159,6 +159,7 @@ export class TaskScheduling { getTasks: async (ids) => await this.bulkGetTasksHelper(ids), filter: (task) => !!task.enabled, map: (task) => ({ ...task, enabled: false }), + validate: false, }); } @@ -174,6 +175,7 @@ export class TaskScheduling { } return { ...task, enabled: true }; }, + validate: false, }); } @@ -208,6 +210,7 @@ export class TaskScheduling { return { ...task, schedule, runAt: new Date(newRunAtInMs) }; }, + validate: false, }); } @@ -229,12 +232,15 @@ export class TaskScheduling { public async runSoon(taskId: string): Promise { const task = await this.getNonRunningTask(taskId); try { - await this.store.update({ - ...task, - status: TaskStatus.Idle, - scheduledAt: new Date(), - runAt: new Date(), - }); + await this.store.update( + { + ...task, + status: TaskStatus.Idle, + scheduledAt: new Date(), + runAt: new Date(), + }, + { validate: false } + ); } catch (e) { if (e.statusCode === 409) { this.logger.debug( diff --git a/x-pack/plugins/task_manager/server/task_store.test.ts b/x-pack/plugins/task_manager/server/task_store.test.ts index 19e179d5e890a7..19c7332e1c13ea 100644 --- a/x-pack/plugins/task_manager/server/task_store.test.ts +++ b/x-pack/plugins/task_manager/server/task_store.test.ts @@ -462,7 +462,7 @@ describe('TaskStore', () => { } ); - const result = await store.update(task); + const result = await store.update(task, { validate: true }); expect(savedObjectsClient.update).toHaveBeenCalledWith( 'task', @@ -518,7 +518,9 @@ describe('TaskStore', () => { const firstErrorPromise = store.errors$.pipe(first()).toPromise(); savedObjectsClient.update.mockRejectedValue(new Error('Failure')); - await expect(store.update(task)).rejects.toThrowErrorMatchingInlineSnapshot(`"Failure"`); + await expect( + store.update(task, { validate: true }) + ).rejects.toThrowErrorMatchingInlineSnapshot(`"Failure"`); expect(await firstErrorPromise).toMatchInlineSnapshot(`[Error: Failure]`); }); }); @@ -559,9 +561,9 @@ describe('TaskStore', () => { const firstErrorPromise = store.errors$.pipe(first()).toPromise(); savedObjectsClient.bulkUpdate.mockRejectedValue(new Error('Failure')); - await expect(store.bulkUpdate([task])).rejects.toThrowErrorMatchingInlineSnapshot( - `"Failure"` - ); + await expect( + store.bulkUpdate([task], { validate: true }) + ).rejects.toThrowErrorMatchingInlineSnapshot(`"Failure"`); expect(await firstErrorPromise).toMatchInlineSnapshot(`[Error: Failure]`); }); }); diff --git a/x-pack/plugins/task_manager/server/task_store.ts b/x-pack/plugins/task_manager/server/task_store.ts index be23052d07aa69..1c8085b7950ca3 100644 --- a/x-pack/plugins/task_manager/server/task_store.ts +++ b/x-pack/plugins/task_manager/server/task_store.ts @@ -243,10 +243,14 @@ export class TaskStore { * @param {TaskDoc} doc * @returns {Promise} */ - public async update(doc: ConcreteTaskInstance): Promise { - // TODO: Should we force validation when claiming tasks? State could fail validation - const validatedTaskInstance = this.taskValidator.getValidatedTaskInstance(doc, 'write'); - const attributes = taskInstanceToAttributes(validatedTaskInstance); + public async update( + doc: ConcreteTaskInstance, + options: { validate: boolean } + ): Promise { + const taskInstance = options.validate + ? this.taskValidator.getValidatedTaskInstance(doc, 'write') + : doc; + const attributes = taskInstanceToAttributes(taskInstance); let updatedSavedObject; try { @@ -264,14 +268,14 @@ export class TaskStore { throw e; } - const taskInstance = savedObjectToConcreteTaskInstance( + const result = savedObjectToConcreteTaskInstance( // The SavedObjects update api forces a Partial on the `attributes` on the response, // but actually returns the whole object that is passed to it, so as we know we're // passing in the whole object, this is safe to do. // This is far from ideal, but unless we change the SavedObjectsClient this is the best we can do { ...updatedSavedObject, attributes: defaults(updatedSavedObject.attributes, attributes) } ); - return this.taskValidator.getValidatedTaskInstance(taskInstance, 'read'); + return options.validate ? this.taskValidator.getValidatedTaskInstance(result, 'read') : result; } /** @@ -281,10 +285,15 @@ export class TaskStore { * @param {Array} docs * @returns {Promise>} */ - public async bulkUpdate(docs: ConcreteTaskInstance[]): Promise { + public async bulkUpdate( + docs: ConcreteTaskInstance[], + options: { validate: boolean } + ): Promise { const attributesByDocId = docs.reduce((attrsById, doc) => { - const validatedTaskInstance = this.taskValidator.getValidatedTaskInstance(doc, 'write'); - attrsById.set(doc.id, taskInstanceToAttributes(validatedTaskInstance)); + const taskInstance = options.validate + ? this.taskValidator.getValidatedTaskInstance(doc, 'write') + : doc; + attrsById.set(doc.id, taskInstanceToAttributes(taskInstance)); return attrsById; }, new Map()); @@ -323,7 +332,10 @@ export class TaskStore { attributesByDocId.get(updatedSavedObject.id)! ), }); - return asOk(this.taskValidator.getValidatedTaskInstance(taskInstance, 'read')); + const result = options.validate + ? this.taskValidator.getValidatedTaskInstance(taskInstance, 'read') + : taskInstance; + return asOk(result); }); } From 25961fff29fb0297b312ac665019d3d27d9e3e5f Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 8 Jun 2023 07:27:33 -0400 Subject: [PATCH 13/29] Use cache to save processing time --- .../task_manager/server/task_validator.ts | 32 ++++++++++++++++--- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts index 87410801c74c2e..48073967103b94 100644 --- a/x-pack/plugins/task_manager/server/task_validator.ts +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -5,7 +5,7 @@ * 2.0. */ -import { isEmpty, max } from 'lodash'; +import { isEmpty, max, memoize } from 'lodash'; import type { Logger } from '@kbn/core/server'; import type { ObjectType } from '@kbn/config-schema'; import { TaskTypeDictionary } from './task_type_dictionary'; @@ -29,11 +29,22 @@ export class TaskValidator { private readonly logger: Logger; private readonly definitions: TaskTypeDictionary; private readonly allowReadingInvalidState: boolean; + private readonly cachedGetLatestStateSchema: (taskTypeDef: TaskDefinition) => LatestStateSchema; + private readonly cachedExtendSchema: typeof extendSchema; constructor({ definitions, allowReadingInvalidState, logger }: TaskValidatorOpts) { this.logger = logger; this.definitions = definitions; this.allowReadingInvalidState = allowReadingInvalidState; + this.cachedGetLatestStateSchema = memoize( + getLatestStateSchema, + (taskTypeDef) => taskTypeDef.type + ); + this.cachedExtendSchema = memoize( + extendSchema, + // We need to cache two outcomes per task type (unknowns: ignore and unknowns: forbid) + (options) => `${options.taskType}|unknowns:${options.unknowns}` + ); } public getValidatedTaskInstance(task: T, mode: 'read' | 'write'): T { @@ -44,8 +55,7 @@ export class TaskValidator { } const taskTypeDef = this.definitions.get(task.taskType); - // TODO: Cache by type - const lastestStateSchema = getLatestStateSchema(taskTypeDef); + const lastestStateSchema = this.cachedGetLatestStateSchema(taskTypeDef); // TODO: Remove once all task types have defined their state schema. // Otherwise, failures on read / write would occur. (don't forget to unskip test) @@ -131,11 +141,23 @@ export class TaskValidator { ); } - // TODO: Don't extendsDeep all the time - return latestStateSchema.schema.extendsDeep({ unknowns }).validate(state); + return this.cachedExtendSchema({ unknowns, taskType, latestStateSchema }).validate(state); } } +function extendSchema(options: { + latestStateSchema: LatestStateSchema; + unknowns: 'forbid' | 'ignore'; + taskType: string; +}) { + if (!options.latestStateSchema) { + throw new Error( + `[TaskValidator] stateSchemaByVersion not defined for task type: ${options.taskType}` + ); + } + return options.latestStateSchema.schema.extendsDeep({ unknowns: options.unknowns }); +} + function getLatestStateSchema(taskTypeDef: TaskDefinition): LatestStateSchema { if (!taskTypeDef.stateSchemaByVersion) { return; From a80dd7fdb90180d977fe4a476ac87bda022d72c1 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 8 Jun 2023 08:32:36 -0400 Subject: [PATCH 14/29] Add task store tests --- .../task_manager/server/task_store.test.ts | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/x-pack/plugins/task_manager/server/task_store.test.ts b/x-pack/plugins/task_manager/server/task_store.test.ts index 19c7332e1c13ea..c8f1abd12995cd 100644 --- a/x-pack/plugins/task_manager/server/task_store.test.ts +++ b/x-pack/plugins/task_manager/server/task_store.test.ts @@ -499,6 +499,62 @@ describe('TaskStore', () => { }); }); + test(`doesn't go through validation process to inject stateVersion when validate:false`, async () => { + const task = { + runAt: mockedDate, + scheduledAt: mockedDate, + startedAt: null, + retryAt: null, + id: 'task:324242', + params: { hello: 'world' }, + state: { foo: 'bar' }, + taskType: 'report', + attempts: 3, + status: 'idle' as TaskStatus, + version: '123', + ownerId: null, + traceparent: 'myTraceparent', + }; + + savedObjectsClient.update.mockImplementation( + async (type: string, id: string, attributes: SavedObjectAttributes) => { + return { + id, + type, + attributes, + references: [], + version: '123', + }; + } + ); + + const result = await store.update(task, { validate: false }); + + expect(savedObjectsClient.update).toHaveBeenCalledWith( + 'task', + task.id, + { + attempts: task.attempts, + schedule: undefined, + params: JSON.stringify(task.params), + retryAt: null, + runAt: task.runAt.toISOString(), + scheduledAt: mockedDate.toISOString(), + scope: undefined, + startedAt: null, + state: JSON.stringify(task.state), + status: task.status, + taskType: task.taskType, + user: undefined, + ownerId: null, + traceparent: 'myTraceparent', + }, + { version: '123', refresh: false } + ); + + expect(result.stateVersion).toBeUndefined(); + }); + test('pushes error from saved objects client to errors$', async () => { const task = { runAt: mockedDate, @@ -542,6 +598,62 @@ describe('TaskStore', () => { }); }); + test(`doesn't validate whenever validate:false is passed-in`, async () => { + const task = { + runAt: mockedDate, + scheduledAt: mockedDate, + startedAt: null, + retryAt: null, + id: 'task:324242', + params: { hello: 'world' }, + state: { foo: 'bar' }, + taskType: 'report', + attempts: 3, + status: 'idle' as TaskStatus, + version: '123', + ownerId: null, + traceparent: '', + }; + + savedObjectsClient.bulkUpdate.mockResolvedValue({ + saved_objects: [ + { + id: '324242', + type: 'task', + attributes: { + ...task, + state: '{"foo":"bar"}', + params: '{"hello":"world"}', + }, + references: [], + version: '123', + }, + ], + }); + + const result = await store.bulkUpdate([task], { validate: false }); + + expect(savedObjectsClient.bulkUpdate).toHaveBeenCalledWith( + [ + { + id: 'task:324242', + type: 'task', + version: '123', + attributes: { + ..._.omit(task, 'version', 'id'), + state: '{"foo":"bar"}', + params: '{"hello":"world"}', + runAt: task.runAt.toISOString(), + scheduledAt: task.scheduledAt.toISOString(), + }, + }, + ], + { refresh: false } + ); + + expect(result[0].tag === 'ok' && result[0].value.stateVersion).toBeUndefined(); + }); + test('pushes error from saved objects client to errors$', async () => { const task = { runAt: mockedDate, From f86a81e1e56381a76c5c964807694f00d7b31c63 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 8 Jun 2023 10:01:38 -0400 Subject: [PATCH 15/29] Fix task store tests --- .../task_manager/server/task_store.test.ts | 77 +++++++------------ 1 file changed, 27 insertions(+), 50 deletions(-) diff --git a/x-pack/plugins/task_manager/server/task_store.test.ts b/x-pack/plugins/task_manager/server/task_store.test.ts index c8f1abd12995cd..392b445978564c 100644 --- a/x-pack/plugins/task_manager/server/task_store.test.ts +++ b/x-pack/plugins/task_manager/server/task_store.test.ts @@ -26,13 +26,32 @@ import { mockLogger } from './test_utils'; import { AdHocTaskCounter } from './lib/adhoc_task_counter'; import { asErr } from './lib/result_type'; +const mockGetValidatedTaskInstance = jest.fn(); +jest.mock('./task_validator', () => { + return { + TaskValidator: jest.fn().mockImplementation(() => { + return { + getValidatedTaskInstance: mockGetValidatedTaskInstance, + }; + }), + }; +}); + const savedObjectsClient = savedObjectsRepositoryMock.create(); const serializer = savedObjectsServiceMock.createSerializer(); const adHocTaskCounter = new AdHocTaskCounter(); const randomId = () => `id-${_.random(1, 20)}`; -beforeEach(() => jest.resetAllMocks()); +beforeEach(() => { + jest.resetAllMocks(); + jest.requireMock('./task_validator').TaskValidator.mockImplementation(() => { + return { + getValidatedTaskInstance: mockGetValidatedTaskInstance, + }; + }); + mockGetValidatedTaskInstance.mockImplementation((task) => task); +}); const mockedDate = new Date('2019-02-12T21:01:22.479Z'); // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -129,7 +148,6 @@ describe('TaskStore', () => { scope: undefined, startedAt: null, state: '{"foo":"bar"}', - stateVersion: 1, status: 'idle', taskType: 'report', user: undefined, @@ -152,7 +170,6 @@ describe('TaskStore', () => { scope: undefined, startedAt: null, state: { foo: 'bar' }, - stateVersion: 1, status: 'idle', taskType: 'report', user: undefined, @@ -464,6 +481,9 @@ describe('TaskStore', () => { const result = await store.update(task, { validate: true }); + expect(mockGetValidatedTaskInstance).toHaveBeenCalledTimes(2); + expect(mockGetValidatedTaskInstance).toHaveBeenNthCalledWith(1, task, 'write'); + expect(mockGetValidatedTaskInstance).toHaveBeenNthCalledWith(2, task, 'read'); expect(savedObjectsClient.update).toHaveBeenCalledWith( 'task', task.id, @@ -477,7 +497,6 @@ describe('TaskStore', () => { scope: undefined, startedAt: null, state: JSON.stringify(task.state), - stateVersion: 1, status: task.status, taskType: task.taskType, user: undefined, @@ -495,7 +514,6 @@ describe('TaskStore', () => { startedAt: null, user: undefined, version: '123', - stateVersion: 1, }); }); @@ -528,31 +546,9 @@ describe('TaskStore', () => { } ); - const result = await store.update(task, { validate: false }); - - expect(savedObjectsClient.update).toHaveBeenCalledWith( - 'task', - task.id, - { - attempts: task.attempts, - schedule: undefined, - params: JSON.stringify(task.params), - retryAt: null, - runAt: task.runAt.toISOString(), - scheduledAt: mockedDate.toISOString(), - scope: undefined, - startedAt: null, - state: JSON.stringify(task.state), - status: task.status, - taskType: task.taskType, - user: undefined, - ownerId: null, - traceparent: 'myTraceparent', - }, - { version: '123', refresh: false } - ); + await store.update(task, { validate: false }); - expect(result.stateVersion).toBeUndefined(); + expect(mockGetValidatedTaskInstance).not.toHaveBeenCalled(); }); test('pushes error from saved objects client to errors$', async () => { @@ -631,27 +627,9 @@ describe('TaskStore', () => { ], }); - const result = await store.bulkUpdate([task], { validate: false }); + await store.bulkUpdate([task], { validate: false }); - expect(savedObjectsClient.bulkUpdate).toHaveBeenCalledWith( - [ - { - id: 'task:324242', - type: 'task', - version: '123', - attributes: { - ..._.omit(task, 'version', 'id'), - state: '{"foo":"bar"}', - params: '{"hello":"world"}', - runAt: task.runAt.toISOString(), - scheduledAt: task.scheduledAt.toISOString(), - }, - }, - ], - { refresh: false } - ); - - expect(result[0].tag === 'ok' && result[0].value.stateVersion).toBeUndefined(); + expect(mockGetValidatedTaskInstance).not.toHaveBeenCalled(); }); test('pushes error from saved objects client to errors$', async () => { @@ -1044,7 +1022,6 @@ describe('TaskStore', () => { scheduledAt: '2019-02-12T21:01:22.479Z', startedAt: null, state: '{"foo":"bar"}', - stateVersion: 1, status: 'idle', taskType: 'report', traceparent: 'apmTraceparent', From 3e3447acbb85daa87f1bc4a70dc0017ddf95458f Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 8 Jun 2023 10:05:31 -0400 Subject: [PATCH 16/29] Add task validator test --- .../task_manager/server/task_store.test.ts | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/x-pack/plugins/task_manager/server/task_store.test.ts b/x-pack/plugins/task_manager/server/task_store.test.ts index 392b445978564c..078769882f913d 100644 --- a/x-pack/plugins/task_manager/server/task_store.test.ts +++ b/x-pack/plugins/task_manager/server/task_store.test.ts @@ -1116,4 +1116,50 @@ describe('TaskStore', () => { expect(adHocTaskCounter.count).toEqual(0); }); }); + + describe('TaskValidator', () => { + test(`should pass allowReadingInvalidState:false accordingly`, () => { + const logger = mockLogger(); + + new TaskStore({ + logger, + index: 'tasky', + taskManagerId: '', + serializer, + esClient: elasticsearchServiceMock.createClusterClient().asInternalUser, + definitions: taskDefinitions, + savedObjectsRepository: savedObjectsClient, + adHocTaskCounter, + allowReadingInvalidState: false, + }); + + expect(jest.requireMock('./task_validator').TaskValidator).toHaveBeenCalledWith({ + logger, + definitions: taskDefinitions, + allowReadingInvalidState: false, + }); + }); + + test(`should pass allowReadingInvalidState:true accordingly`, () => { + const logger = mockLogger(); + + new TaskStore({ + logger, + index: 'tasky', + taskManagerId: '', + serializer, + esClient: elasticsearchServiceMock.createClusterClient().asInternalUser, + definitions: taskDefinitions, + savedObjectsRepository: savedObjectsClient, + adHocTaskCounter, + allowReadingInvalidState: true, + }); + + expect(jest.requireMock('./task_validator').TaskValidator).toHaveBeenCalledWith({ + logger, + definitions: taskDefinitions, + allowReadingInvalidState: true, + }); + }); + }); }); From 214bae38d89409d4f5527887f46ce9c4988146d1 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 8 Jun 2023 10:14:07 -0400 Subject: [PATCH 17/29] Onboard alerts_invalidate_api_keys task type --- .../invalidate_pending_api_keys/task.ts | 46 ++++++++++++++----- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/x-pack/plugins/alerting/server/invalidate_pending_api_keys/task.ts b/x-pack/plugins/alerting/server/invalidate_pending_api_keys/task.ts index f1e97c9fd93d02..1e624bcd807cf7 100644 --- a/x-pack/plugins/alerting/server/invalidate_pending_api_keys/task.ts +++ b/x-pack/plugins/alerting/server/invalidate_pending_api_keys/task.ts @@ -12,6 +12,7 @@ import { KibanaRequest, SavedObjectsClientContract, } from '@kbn/core/server'; +import { schema, TypeOf } from '@kbn/config-schema'; import { EncryptedSavedObjectsClient } from '@kbn/encrypted-saved-objects-plugin/server'; import { InvalidateAPIKeysParams, SecurityPluginStart } from '@kbn/security-plugin/server'; import { @@ -29,6 +30,22 @@ import { InvalidatePendingApiKey } from '../types'; const TASK_TYPE = 'alerts_invalidate_api_keys'; export const TASK_ID = `Alerts-${TASK_TYPE}`; +const stateSchemaByVersion = { + 1: { + up: (state: Record) => ({ + runs: state.runs || 0, + total_invalidated: state.total_invalidated || 0, + }), + schema: schema.object({ + runs: schema.number(), + total_invalidated: schema.number(), + }), + }, +}; + +const latestSchema = stateSchemaByVersion[1].schema; +type LatestTaskStateSchema = TypeOf; + const invalidateAPIKeys = async ( params: InvalidateAPIKeysParams, securityPluginStart?: SecurityPluginStart @@ -65,17 +82,21 @@ export async function scheduleApiKeyInvalidatorTask( ) { const interval = config.invalidateApiKeysTask.interval; try { + const state: LatestTaskStateSchema = { + runs: 0, + total_invalidated: 0, + }; await taskManager.ensureScheduled({ id: TASK_ID, taskType: TASK_TYPE, schedule: { interval, }, - state: {}, + state, params: {}, }); } catch (e) { - logger.debug(`Error scheduling task, received ${e.message}`); + logger.error(`Error scheduling ${TASK_ID} task, received ${e.message}`); } } @@ -88,6 +109,7 @@ function registerApiKeyInvalidatorTaskDefinition( taskManager.registerTaskDefinitions({ [TASK_TYPE]: { title: 'Invalidate alert API Keys', + stateSchemaByVersion, createTaskRunner: taskRunner(logger, coreStartServices, config), }, }); @@ -117,7 +139,7 @@ function taskRunner( config: AlertingConfig ) { return ({ taskInstance }: RunContext) => { - const { state } = taskInstance; + const state = taskInstance.state as LatestTaskStateSchema; return { async run() { let totalInvalidated = 0; @@ -159,22 +181,24 @@ function taskRunner( hasApiKeysPendingInvalidation = apiKeysToInvalidate.total > PAGE_SIZE; } while (hasApiKeysPendingInvalidation); + const updatedState: LatestTaskStateSchema = { + runs: (state.runs || 0) + 1, + total_invalidated: totalInvalidated, + }; return { - state: { - runs: (state.runs || 0) + 1, - total_invalidated: totalInvalidated, - }, + state: updatedState, schedule: { interval: config.invalidateApiKeysTask.interval, }, }; } catch (e) { logger.warn(`Error executing alerting apiKey invalidation task: ${e.message}`); + const updatedState: LatestTaskStateSchema = { + runs: state.runs + 1, + total_invalidated: totalInvalidated, + }; return { - state: { - runs: (state.runs || 0) + 1, - total_invalidated: totalInvalidated, - }, + state: updatedState, schedule: { interval: config.invalidateApiKeysTask.interval, }, From b5f12950bd976aa3cc889fa47ca5cc430e11efce Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 8 Jun 2023 11:28:21 -0400 Subject: [PATCH 18/29] Fix the validate calls in the buffered store --- .../server/buffered_task_store.test.ts | 52 ++++++++++---- .../server/buffered_task_store.ts | 22 +++++- .../server/lib/bulk_operation_buffer.test.ts | 12 ++-- .../server/lib/bulk_operation_buffer.ts | 3 +- .../task_manager/server/polling_lifecycle.ts | 3 - .../server/task_running/task_runner.test.ts | 37 ++++++---- .../server/task_running/task_runner.ts | 69 ++++++++++--------- .../task_manager/server/task_store.mock.ts | 3 + .../plugins/task_manager/server/task_store.ts | 2 +- 9 files changed, 129 insertions(+), 74 deletions(-) diff --git a/x-pack/plugins/task_manager/server/buffered_task_store.test.ts b/x-pack/plugins/task_manager/server/buffered_task_store.test.ts index 53fc13a19ef9bb..5cf79ab335da3b 100644 --- a/x-pack/plugins/task_manager/server/buffered_task_store.test.ts +++ b/x-pack/plugins/task_manager/server/buffered_task_store.test.ts @@ -14,7 +14,7 @@ describe('Buffered Task Store', () => { test('proxies the TaskStore for `maxAttempts` and `remove`', async () => { const taskStore = taskStoreMock.create(); taskStore.bulkUpdate.mockResolvedValue([]); - const bufferedStore = new BufferedTaskStore(taskStore, { validate: true }); + const bufferedStore = new BufferedTaskStore(taskStore, {}); bufferedStore.remove('1'); expect(taskStore.remove).toHaveBeenCalledWith('1'); @@ -23,19 +23,45 @@ describe('Buffered Task Store', () => { describe('update', () => { test("proxies the TaskStore's `bulkUpdate`", async () => { const taskStore = taskStoreMock.create(); - const bufferedStore = new BufferedTaskStore(taskStore, { validate: true }); + const bufferedStore = new BufferedTaskStore(taskStore, {}); const task = taskManagerMock.createTask(); taskStore.bulkUpdate.mockResolvedValue([asOk(task)]); - expect(await bufferedStore.update(task)).toMatchObject(task); - expect(taskStore.bulkUpdate).toHaveBeenCalledWith([task], { validate: true }); + expect(await bufferedStore.update(task, { validate: true })).toMatchObject(task); + expect(taskStore.bulkUpdate).toHaveBeenCalledWith([task], { validate: false }); + + expect(taskStore.taskValidator.getValidatedTaskInstance).toHaveBeenCalledTimes(2); + expect(taskStore.taskValidator.getValidatedTaskInstance).toHaveBeenNthCalledWith( + 1, + task, + 'write' + ); + expect(taskStore.taskValidator.getValidatedTaskInstance).toHaveBeenNthCalledWith( + 2, + task, + 'read' + ); + }); + + test(`doesn't validate when specified`, async () => { + const taskStore = taskStoreMock.create(); + const bufferedStore = new BufferedTaskStore(taskStore, {}); + + const task = taskManagerMock.createTask(); + + taskStore.bulkUpdate.mockResolvedValue([asOk(task)]); + + expect(await bufferedStore.update(task, { validate: false })).toMatchObject(task); + expect(taskStore.bulkUpdate).toHaveBeenCalledWith([task], { validate: false }); + + expect(taskStore.taskValidator.getValidatedTaskInstance).not.toHaveBeenCalled(); }); test('handles partially successfull bulkUpdates resolving each call appropriately', async () => { const taskStore = taskStoreMock.create(); - const bufferedStore = new BufferedTaskStore(taskStore, { validate: true }); + const bufferedStore = new BufferedTaskStore(taskStore, {}); const tasks = [ taskManagerMock.createTask(), @@ -58,9 +84,9 @@ describe('Buffered Task Store', () => { ]); const results = [ - bufferedStore.update(tasks[0]), - bufferedStore.update(tasks[1]), - bufferedStore.update(tasks[2]), + bufferedStore.update(tasks[0], { validate: true }), + bufferedStore.update(tasks[1], { validate: true }), + bufferedStore.update(tasks[2], { validate: true }), ]; expect(await results[0]).toMatchObject(tasks[0]); expect(results[1]).rejects.toMatchInlineSnapshot(` @@ -79,7 +105,7 @@ describe('Buffered Task Store', () => { test('handles multiple items with the same id', async () => { const taskStore = taskStoreMock.create(); - const bufferedStore = new BufferedTaskStore(taskStore, { validate: true }); + const bufferedStore = new BufferedTaskStore(taskStore, {}); const duplicateIdTask = taskManagerMock.createTask(); const tasks = [ @@ -105,10 +131,10 @@ describe('Buffered Task Store', () => { ]); const results = [ - bufferedStore.update(tasks[0]), - bufferedStore.update(tasks[1]), - bufferedStore.update(tasks[2]), - bufferedStore.update(tasks[3]), + bufferedStore.update(tasks[0], { validate: true }), + bufferedStore.update(tasks[1], { validate: true }), + bufferedStore.update(tasks[2], { validate: true }), + bufferedStore.update(tasks[3], { validate: true }), ]; expect(await results[0]).toMatchObject(tasks[0]); expect(results[1]).rejects.toMatchInlineSnapshot(` diff --git a/x-pack/plugins/task_manager/server/buffered_task_store.ts b/x-pack/plugins/task_manager/server/buffered_task_store.ts index 78b779a69d1619..a5f2e57028df34 100644 --- a/x-pack/plugins/task_manager/server/buffered_task_store.ts +++ b/x-pack/plugins/task_manager/server/buffered_task_store.ts @@ -18,7 +18,12 @@ export class BufferedTaskStore implements Updatable { private bufferedUpdate: Operation; constructor(private readonly taskStore: TaskStore, options: BufferOptions) { this.bufferedUpdate = createBuffer( - (docs) => taskStore.bulkUpdate(docs, { validate: options.validate }), + // Setting validate: false because we'll validate per update call + // + // Ideally we could accumulate the "validate" options and pass them + // to .bulkUpdate per doc, but the required changes to the bulk_operation_buffer + // to track the values are high and deffered for now. + (docs) => taskStore.bulkUpdate(docs, { validate: false }), { bufferMaxDuration: DEFAULT_BUFFER_MAX_DURATION, ...options, @@ -26,8 +31,19 @@ export class BufferedTaskStore implements Updatable { ); } - public async update(doc: ConcreteTaskInstance): Promise { - return unwrapPromise(this.bufferedUpdate(doc)); + public async update( + doc: ConcreteTaskInstance, + options: { validate: boolean } + ): Promise { + let docToUpdate = doc; + if (options.validate) { + docToUpdate = this.taskStore.taskValidator.getValidatedTaskInstance(doc, 'write'); + } + let result = await unwrapPromise(this.bufferedUpdate(docToUpdate)); + if (options.validate) { + result = this.taskStore.taskValidator.getValidatedTaskInstance(result, 'read'); + } + return result; } public async remove(id: string): Promise { diff --git a/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.test.ts b/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.test.ts index 8b82ac4030c6ab..99bb7c5b84274d 100644 --- a/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.test.ts +++ b/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.test.ts @@ -44,7 +44,7 @@ describe('Bulk Operation Buffer', () => { return Promise.resolve([incrementAttempts(task1), incrementAttempts(task2)]); }); - const bufferedUpdate = createBuffer(bulkUpdate, { validate: true }); + const bufferedUpdate = createBuffer(bulkUpdate, {}); const task1 = createTask(); const task2 = createTask(); @@ -62,7 +62,7 @@ describe('Bulk Operation Buffer', () => { }); const bufferMaxDuration = 50; - const bufferedUpdate = createBuffer(bulkUpdate, { bufferMaxDuration, validate: true }); + const bufferedUpdate = createBuffer(bulkUpdate, { bufferMaxDuration }); const task1 = createTask(); const task2 = createTask(); @@ -102,7 +102,6 @@ describe('Bulk Operation Buffer', () => { const bufferedUpdate = createBuffer(bulkUpdate, { bufferMaxDuration, bufferMaxOperations: 2, - validate: true, }); const task1 = createTask(); @@ -136,7 +135,6 @@ describe('Bulk Operation Buffer', () => { const bufferedUpdate = createBuffer(bulkUpdate, { bufferMaxDuration, bufferMaxOperations: 3, - validate: true, }); const task1 = createTask(); @@ -175,7 +173,7 @@ describe('Bulk Operation Buffer', () => { } ); - const bufferedUpdate = createBuffer(bulkUpdate, { validate: true }); + const bufferedUpdate = createBuffer(bulkUpdate, {}); const task1 = createTask(); const task2 = createTask(); @@ -197,7 +195,7 @@ describe('Bulk Operation Buffer', () => { return Promise.reject(new Error('bulkUpdate is an illusion')); }); - const bufferedUpdate = createBuffer(bulkUpdate, { validate: true }); + const bufferedUpdate = createBuffer(bulkUpdate, {}); const task1 = createTask(); const task2 = createTask(); @@ -240,7 +238,7 @@ describe('Bulk Operation Buffer', () => { const logger = mockLogger(); - const bufferedUpdate = createBuffer(bulkUpdate, { logger, validate: true }); + const bufferedUpdate = createBuffer(bulkUpdate, { logger }); const task1 = createTask(); const task2 = createTask(); diff --git a/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.ts b/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.ts index f83cde1b6a3515..874592c4e5d01e 100644 --- a/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.ts +++ b/x-pack/plugins/task_manager/server/lib/bulk_operation_buffer.ts @@ -16,7 +16,6 @@ export interface BufferOptions { bufferMaxDuration?: number; bufferMaxOperations?: number; logger?: Logger; - validate: boolean; } export interface Entity { @@ -40,7 +39,7 @@ const FLUSH = true; export function createBuffer( bulkOperation: BulkOperation, - { bufferMaxDuration = 0, bufferMaxOperations = Number.MAX_VALUE, logger, validate }: BufferOptions + { bufferMaxDuration = 0, bufferMaxOperations = Number.MAX_VALUE, logger }: BufferOptions ): Operation { const flushBuffer = new Subject(); diff --git a/x-pack/plugins/task_manager/server/polling_lifecycle.ts b/x-pack/plugins/task_manager/server/polling_lifecycle.ts index f664cf4a10e73e..c60a8af2c92084 100644 --- a/x-pack/plugins/task_manager/server/polling_lifecycle.ts +++ b/x-pack/plugins/task_manager/server/polling_lifecycle.ts @@ -116,9 +116,6 @@ export class TaskPollingLifecycle { this.bufferedStore = new BufferedTaskStore(this.store, { bufferMaxOperations: config.max_workers, logger, - // Buffer is used to remove and update tasks internally and operations - // should succeed regardless if the task state is invalid. - validate: false, }); this.pool = new TaskPool({ diff --git a/x-pack/plugins/task_manager/server/task_running/task_runner.test.ts b/x-pack/plugins/task_manager/server/task_running/task_runner.test.ts index 178964402c4680..be47be3a84266c 100644 --- a/x-pack/plugins/task_manager/server/task_running/task_runner.test.ts +++ b/x-pack/plugins/task_manager/server/task_running/task_runner.test.ts @@ -432,17 +432,20 @@ describe('TaskManagerRunner', () => { `[Error: type: Bad Request]` ); - expect(store.update).toHaveBeenCalledWith({ - ...mockInstance({ - id, - attempts: initialAttempts + 1, - schedule: undefined, - }), - status: TaskStatus.Idle, - startedAt: null, - retryAt: null, - ownerId: null, - }); + expect(store.update).toHaveBeenCalledWith( + { + ...mockInstance({ + id, + attempts: initialAttempts + 1, + schedule: undefined, + }), + status: TaskStatus.Idle, + startedAt: null, + retryAt: null, + ownerId: null, + }, + { validate: false } + ); }); test(`it doesnt try to increment a task's attempts when markTaskAsRunning fails for version conflict`, async () => { @@ -834,7 +837,9 @@ describe('TaskManagerRunner', () => { await runner.run(); expect(store.update).toHaveBeenCalledTimes(1); - expect(store.update).toHaveBeenCalledWith(expect.objectContaining({ runAt })); + expect(store.update).toHaveBeenCalledWith(expect.objectContaining({ runAt }), { + validate: true, + }); }); test('reschedules tasks that return a schedule', async () => { @@ -862,7 +867,9 @@ describe('TaskManagerRunner', () => { await runner.run(); expect(store.update).toHaveBeenCalledTimes(1); - expect(store.update).toHaveBeenCalledWith(expect.objectContaining({ runAt })); + expect(store.update).toHaveBeenCalledWith(expect.objectContaining({ runAt }), { + validate: true, + }); }); test(`doesn't reschedule recurring tasks that throw an unrecoverable error`, async () => { @@ -936,7 +943,9 @@ describe('TaskManagerRunner', () => { await runner.run(); expect(store.update).toHaveBeenCalledTimes(1); - expect(store.update).toHaveBeenCalledWith(expect.objectContaining({ runAt })); + expect(store.update).toHaveBeenCalledWith(expect.objectContaining({ runAt }), { + validate: true, + }); }); test('removes non-recurring tasks after they complete', async () => { diff --git a/x-pack/plugins/task_manager/server/task_running/task_runner.ts b/x-pack/plugins/task_manager/server/task_running/task_runner.ts index 858d89f46d70ae..0252c565c5d0f1 100644 --- a/x-pack/plugins/task_manager/server/task_running/task_runner.ts +++ b/x-pack/plugins/task_manager/server/task_running/task_runner.ts @@ -89,7 +89,7 @@ export interface TaskRunning { } export interface Updatable { - update(doc: ConcreteTaskInstance): Promise; + update(doc: ConcreteTaskInstance, options: { validate: boolean }): Promise; remove(id: string): Promise; } @@ -395,27 +395,30 @@ export class TaskManagerRunner implements TaskRunner { } this.instance = asReadyToRun( - (await this.bufferedTaskStore.update({ - ...taskWithoutEnabled(taskInstance), - status: TaskStatus.Running, - startedAt: now, - attempts, - retryAt: - (this.instance.task.schedule - ? maxIntervalFromDate( - now, - this.instance.task.schedule.interval, - this.definition.timeout - ) - : this.getRetryDelay({ - attempts, - // Fake an error. This allows retry logic when tasks keep timing out - // and lets us set a proper "retryAt" value each time. - error: new Error('Task timeout'), - addDuration: this.definition.timeout, - })) ?? null, - // This is a safe convertion as we're setting the startAt above - })) as ConcreteTaskInstanceWithStartedAt + (await this.bufferedTaskStore.update( + { + ...taskWithoutEnabled(taskInstance), + status: TaskStatus.Running, + startedAt: now, + attempts, + retryAt: + (this.instance.task.schedule + ? maxIntervalFromDate( + now, + this.instance.task.schedule.interval, + this.definition.timeout + ) + : this.getRetryDelay({ + attempts, + // Fake an error. This allows retry logic when tasks keep timing out + // and lets us set a proper "retryAt" value each time. + error: new Error('Task timeout'), + addDuration: this.definition.timeout, + })) ?? null, + // This is a safe convertion as we're setting the startAt above + }, + { validate: false } + )) as ConcreteTaskInstanceWithStartedAt ); const timeUntilClaimExpiresAfterUpdate = @@ -476,14 +479,17 @@ export class TaskManagerRunner implements TaskRunner { private async releaseClaimAndIncrementAttempts(): Promise> { return promiseResult( - this.bufferedTaskStore.update({ - ...taskWithoutEnabled(this.instance.task), - status: TaskStatus.Idle, - attempts: this.instance.task.attempts + 1, - startedAt: null, - retryAt: null, - ownerId: null, - }) + this.bufferedTaskStore.update( + { + ...taskWithoutEnabled(this.instance.task), + status: TaskStatus.Idle, + attempts: this.instance.task.attempts + 1, + startedAt: null, + retryAt: null, + ownerId: null, + }, + { validate: false } + ) ); } @@ -580,7 +586,8 @@ export class TaskManagerRunner implements TaskRunner { ownerId: null, }, taskWithoutEnabled(this.instance.task) - ) + ), + { validate: true } ) ); } diff --git a/x-pack/plugins/task_manager/server/task_store.mock.ts b/x-pack/plugins/task_manager/server/task_store.mock.ts index 23818bc9435084..5042d0bc94ffb0 100644 --- a/x-pack/plugins/task_manager/server/task_store.mock.ts +++ b/x-pack/plugins/task_manager/server/task_store.mock.ts @@ -14,6 +14,9 @@ interface TaskStoreOptions { export const taskStoreMock = { create({ index = '', taskManagerId = '' }: TaskStoreOptions = {}) { const mocked = { + taskValidator: { + getValidatedTaskInstance: jest.fn().mockImplementation((task) => task), + }, convertToSavedObjectIds: jest.fn(), update: jest.fn(), remove: jest.fn(), diff --git a/x-pack/plugins/task_manager/server/task_store.ts b/x-pack/plugins/task_manager/server/task_store.ts index 1c8085b7950ca3..f7dc5a4f6a3c22 100644 --- a/x-pack/plugins/task_manager/server/task_store.ts +++ b/x-pack/plugins/task_manager/server/task_store.ts @@ -100,6 +100,7 @@ export class TaskStore { public readonly index: string; public readonly taskManagerId: string; public readonly errors$ = new Subject(); + public readonly taskValidator: TaskValidator; private esClient: ElasticsearchClient; private esClientWithoutRetries: ElasticsearchClient; @@ -107,7 +108,6 @@ export class TaskStore { private savedObjectsRepository: ISavedObjectsRepository; private serializer: ISavedObjectsSerializer; private adHocTaskCounter: AdHocTaskCounter; - private readonly taskValidator: TaskValidator; /** * Constructs a new TaskStore. From 341a2cb0458439e095456ef9ab9e667a4897cc0c Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 8 Jun 2023 12:07:56 -0400 Subject: [PATCH 19/29] Add docs --- x-pack/plugins/task_manager/README.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/x-pack/plugins/task_manager/README.md b/x-pack/plugins/task_manager/README.md index dd6565ae16d524..c7ca9e5eeea082 100644 --- a/x-pack/plugins/task_manager/README.md +++ b/x-pack/plugins/task_manager/README.md @@ -92,6 +92,20 @@ export class Plugin { // can add significant load to the ES cluster, so please use this configuration only when absolutly necesery. maxConcurrency: 1, + // To ensure the validity of task state during read and write operations, utilize the stateSchemaByVersion configuration. This functionality validates the state before executing a task. Make sure to define the schema property using the @kbn/config-schema plugin, specifically as an ObjectType (schema.object) at the top level. + stateSchemaByVersion: { + 1: { + schema: schema.object({ + count: schema.number(), + }), + up: (state) => { + return { + count: state.count || 0, + }; + }, + } + } + // The createTaskRunner function / method returns an object that is responsible for // performing the work of the task. context: { taskInstance }, is documented below. createTaskRunner(context) { From a2854fdca590da217351d2e501d330d02db15912 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 8 Jun 2023 13:22:48 -0400 Subject: [PATCH 20/29] Final changes --- x-pack/plugins/task_manager/server/config.test.ts | 6 +++--- x-pack/plugins/task_manager/server/config.ts | 3 +-- x-pack/plugins/task_manager/server/task_validator.ts | 6 +----- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/task_manager/server/config.test.ts b/x-pack/plugins/task_manager/server/config.test.ts index f9a8f17e1d6f62..4eca3e971de542 100644 --- a/x-pack/plugins/task_manager/server/config.test.ts +++ b/x-pack/plugins/task_manager/server/config.test.ts @@ -12,7 +12,7 @@ describe('config validation', () => { const config: Record = {}; expect(configSchema.validate(config)).toMatchInlineSnapshot(` Object { - "allow_reading_invalid_state": false, + "allow_reading_invalid_state": true, "ephemeral_tasks": Object { "enabled": false, "request_capacity": 10, @@ -65,7 +65,7 @@ describe('config validation', () => { const config: Record = {}; expect(configSchema.validate(config)).toMatchInlineSnapshot(` Object { - "allow_reading_invalid_state": false, + "allow_reading_invalid_state": true, "ephemeral_tasks": Object { "enabled": false, "request_capacity": 10, @@ -116,7 +116,7 @@ describe('config validation', () => { }; expect(configSchema.validate(config)).toMatchInlineSnapshot(` Object { - "allow_reading_invalid_state": false, + "allow_reading_invalid_state": true, "ephemeral_tasks": Object { "enabled": false, "request_capacity": 10, diff --git a/x-pack/plugins/task_manager/server/config.ts b/x-pack/plugins/task_manager/server/config.ts index 052fcb6297b4f6..2025b44d5b50d7 100644 --- a/x-pack/plugins/task_manager/server/config.ts +++ b/x-pack/plugins/task_manager/server/config.ts @@ -137,8 +137,7 @@ export const configSchema = schema.object( exclude_task_types: schema.arrayOf(schema.string(), { defaultValue: [] }), authenticate_background_task_utilization: schema.boolean({ defaultValue: true }), }), - // TODO: Set default value back to `true` - allow_reading_invalid_state: schema.boolean({ defaultValue: false }), + allow_reading_invalid_state: schema.boolean({ defaultValue: true }), }, { validate: (config) => { diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts index 48073967103b94..b6eed0c4c2a302 100644 --- a/x-pack/plugins/task_manager/server/task_validator.ts +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -5,7 +5,7 @@ * 2.0. */ -import { isEmpty, max, memoize } from 'lodash'; +import { max, memoize } from 'lodash'; import type { Logger } from '@kbn/core/server'; import type { ObjectType } from '@kbn/config-schema'; import { TaskTypeDictionary } from './task_type_dictionary'; @@ -131,10 +131,6 @@ export class TaskValidator { latestStateSchema: LatestStateSchema, unknowns: 'forbid' | 'ignore' ): ConcreteTaskInstance['state'] { - if (isEmpty(state)) { - return {}; - } - if (!latestStateSchema) { throw new Error( `[TaskValidator] stateSchemaByVersion not defined for task type: ${taskType}` From 09d924d50e8284b6593a99d3b42ccc31bf4e11f9 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 8 Jun 2023 13:38:39 -0400 Subject: [PATCH 21/29] Add GH issue link --- x-pack/plugins/task_manager/server/task_validator.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts index b6eed0c4c2a302..9e090875671d5f 100644 --- a/x-pack/plugins/task_manager/server/task_validator.ts +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -58,6 +58,7 @@ export class TaskValidator { const lastestStateSchema = this.cachedGetLatestStateSchema(taskTypeDef); // TODO: Remove once all task types have defined their state schema. + // https://github.com/elastic/kibana/issues/159347 // Otherwise, failures on read / write would occur. (don't forget to unskip test) if (!lastestStateSchema) { return task; From 31d33cde4788c2957c799062245e4b08918a5a70 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Tue, 13 Jun 2023 09:41:06 -0400 Subject: [PATCH 22/29] Add jest integration tests --- .../server/integration_tests/lib/index.ts | 9 + .../integration_tests/lib/inject_task.ts | 32 +++ .../lib/setup_test_servers.ts | 52 ++++ .../task_state_validation.test.ts | 258 ++++++++++++++++++ 4 files changed, 351 insertions(+) create mode 100644 x-pack/plugins/task_manager/server/integration_tests/lib/index.ts create mode 100644 x-pack/plugins/task_manager/server/integration_tests/lib/inject_task.ts create mode 100644 x-pack/plugins/task_manager/server/integration_tests/lib/setup_test_servers.ts create mode 100644 x-pack/plugins/task_manager/server/integration_tests/task_state_validation.test.ts diff --git a/x-pack/plugins/task_manager/server/integration_tests/lib/index.ts b/x-pack/plugins/task_manager/server/integration_tests/lib/index.ts new file mode 100644 index 00000000000000..e27d2aee383494 --- /dev/null +++ b/x-pack/plugins/task_manager/server/integration_tests/lib/index.ts @@ -0,0 +1,9 @@ +/* + * 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. + */ + +export { injectTask } from './inject_task'; +export { setupTestServers } from './setup_test_servers'; diff --git a/x-pack/plugins/task_manager/server/integration_tests/lib/inject_task.ts b/x-pack/plugins/task_manager/server/integration_tests/lib/inject_task.ts new file mode 100644 index 00000000000000..2624c234dd526f --- /dev/null +++ b/x-pack/plugins/task_manager/server/integration_tests/lib/inject_task.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 { type ElasticsearchClient } from '@kbn/core/server'; +import { type ConcreteTaskInstance } from '../../task'; + +export async function injectTask( + esClient: ElasticsearchClient, + { id, ...task }: ConcreteTaskInstance +) { + const soId = `task:${id}`; + await esClient.index({ + id: soId, + index: '.kibana_task_manager', + document: { + references: [], + type: 'task', + updated_at: new Date().toISOString(), + task: { + ...task, + state: JSON.stringify(task.state), + params: JSON.stringify(task.params), + runAt: task.runAt.toISOString(), + scheduledAt: task.scheduledAt.toISOString(), + }, + }, + }); +} diff --git a/x-pack/plugins/task_manager/server/integration_tests/lib/setup_test_servers.ts b/x-pack/plugins/task_manager/server/integration_tests/lib/setup_test_servers.ts new file mode 100644 index 00000000000000..9bb4fb4a5ee317 --- /dev/null +++ b/x-pack/plugins/task_manager/server/integration_tests/lib/setup_test_servers.ts @@ -0,0 +1,52 @@ +/* + * 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 { createTestServers, createRootWithCorePlugins } from '@kbn/core-test-helpers-kbn-server'; + +export async function setupTestServers(settings = {}) { + const { startES } = createTestServers({ + adjustTimeout: (t) => jest.setTimeout(t), + settings: { + es: { + license: 'trial', + }, + }, + }); + + const esServer = await startES(); + + const root = createRootWithCorePlugins( + { + logging: { + root: { + level: 'warn', + }, + loggers: [ + { + name: 'plugins.taskManager', + level: 'all', + }, + ], + }, + }, + { oss: false } + ); + + await root.preboot(); + const coreSetup = await root.setup(); + const coreStart = await root.start(); + + return { + esServer, + kibanaServer: { + root, + coreSetup, + coreStart, + stop: async () => await root.shutdown(), + }, + }; +} diff --git a/x-pack/plugins/task_manager/server/integration_tests/task_state_validation.test.ts b/x-pack/plugins/task_manager/server/integration_tests/task_state_validation.test.ts new file mode 100644 index 00000000000000..180afb79e81eec --- /dev/null +++ b/x-pack/plugins/task_manager/server/integration_tests/task_state_validation.test.ts @@ -0,0 +1,258 @@ +/* + * 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 { + type TestElasticsearchUtils, + type TestKibanaUtils, +} from '@kbn/core-test-helpers-kbn-server'; +import { schema } from '@kbn/config-schema'; +import { TaskStatus } from '../task'; +import { type TaskPollingLifecycleOpts } from '../polling_lifecycle'; +import { type TaskClaimingOpts } from '../queries/task_claiming'; +import { TaskManagerPlugin, type TaskManagerStartContract } from '../plugin'; +import { injectTask, setupTestServers } from './lib'; + +const { TaskPollingLifecycle: TaskPollingLifecycleMock } = jest.requireMock('../polling_lifecycle'); +jest.mock('../polling_lifecycle', () => { + const actual = jest.requireActual('../polling_lifecycle'); + return { + ...actual, + TaskPollingLifecycle: jest.fn().mockImplementation((opts) => { + return new actual.TaskPollingLifecycle(opts); + }), + }; +}); + +const mockTaskTypeRunFn = jest.fn(); +const mockCreateTaskRunner = jest.fn(); +const mockTaskType = { + title: '', + description: '', + stateSchemaByVersion: { + 1: { + up: (state: Record) => ({ foo: state.foo || '' }), + schema: schema.object({ + foo: schema.string(), + }), + }, + 2: { + up: (state: Record) => ({ ...state, bar: state.bar || '' }), + schema: schema.object({ + foo: schema.string(), + bar: schema.string(), + }), + }, + 3: { + up: (state: Record) => ({ ...state, baz: state.baz || '' }), + schema: schema.object({ + foo: schema.string(), + bar: schema.string(), + baz: schema.string(), + }), + }, + }, + createTaskRunner: mockCreateTaskRunner.mockImplementation(() => ({ + run: mockTaskTypeRunFn, + })), +}; +jest.mock('../queries/task_claiming', () => { + const actual = jest.requireActual('../queries/task_claiming'); + return { + ...actual, + TaskClaiming: jest.fn().mockImplementation((opts: TaskClaimingOpts) => { + // We need to register here because once the class is instantiated, adding + // definitions won't get claimed because of "partitionIntoClaimingBatches". + opts.definitions.registerTaskDefinitions({ + fooType: mockTaskType, + }); + return new actual.TaskClaiming(opts); + }), + }; +}); + +const taskManagerStartSpy = jest.spyOn(TaskManagerPlugin.prototype, 'start'); + +describe('task state validation', () => { + let esServer: TestElasticsearchUtils; + let kibanaServer: TestKibanaUtils; + let taskManagerPlugin: TaskManagerStartContract; + let pollingLifecycleOpts: TaskPollingLifecycleOpts; + + beforeAll(async () => { + const setupResult = await setupTestServers(); + esServer = setupResult.esServer; + kibanaServer = setupResult.kibanaServer; + + expect(taskManagerStartSpy).toHaveBeenCalledTimes(1); + taskManagerPlugin = taskManagerStartSpy.mock.results[0].value; + + expect(TaskPollingLifecycleMock).toHaveBeenCalledTimes(1); + pollingLifecycleOpts = TaskPollingLifecycleMock.mock.calls[0][0]; + }); + + afterAll(async () => { + if (kibanaServer) { + await kibanaServer.stop(); + } + if (esServer) { + await esServer.stop(); + } + }); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + afterEach(async () => { + await taskManagerPlugin.removeIfExists('foo'); + }); + + it('should drop unknown fields from the task state', async () => { + const taskRunnerPromise = new Promise((resolve) => { + mockTaskTypeRunFn.mockImplementation(() => { + setTimeout(resolve, 0); + return { state: {} }; + }); + }); + + await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { + id: 'foo', + taskType: 'fooType', + params: { foo: true }, + state: { foo: 'test', bar: 'test', baz: 'test', invalidProperty: 'invalid' }, + stateVersion: 4, + runAt: new Date(), + enabled: true, + scheduledAt: new Date(), + attempts: 0, + status: TaskStatus.Idle, + startedAt: null, + retryAt: null, + ownerId: null, + }); + + await taskRunnerPromise; + + expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); + const call = mockCreateTaskRunner.mock.calls[0][0]; + expect(call.taskInstance.state).toEqual({ + foo: 'test', + bar: 'test', + baz: 'test', + }); + }); + + it('should fail to update the task if the task runner returns an unknown property in the state', async () => { + const errorLogSpy = jest.spyOn(pollingLifecycleOpts.logger, 'error'); + const taskRunnerPromise = new Promise((resolve) => { + mockTaskTypeRunFn.mockImplementation(() => { + setTimeout(resolve, 0); + return { state: { invalidField: true, foo: 'test', bar: 'test', baz: 'test' } }; + }); + }); + + await taskManagerPlugin.schedule({ + id: 'foo', + taskType: 'fooType', + params: {}, + state: { foo: 'test', bar: 'test', baz: 'test' }, + schedule: { interval: '1d' }, + }); + + await taskRunnerPromise; + + expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); + const call = mockCreateTaskRunner.mock.calls[0][0]; + expect(call.taskInstance.state).toEqual({ + foo: 'test', + bar: 'test', + baz: 'test', + }); + expect(errorLogSpy).toHaveBeenCalledWith( + 'Task fooType "foo" failed: Error: [invalidField]: definition for this key is missing', + expect.anything() + ); + }); + + it('should migrate the task state', async () => { + const taskRunnerPromise = new Promise((resolve) => { + mockTaskTypeRunFn.mockImplementation(() => { + setTimeout(resolve, 0); + return { state: {} }; + }); + }); + + await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { + id: 'foo', + taskType: 'fooType', + params: { foo: true }, + state: {}, + runAt: new Date(), + enabled: true, + scheduledAt: new Date(), + attempts: 0, + status: TaskStatus.Idle, + startedAt: null, + retryAt: null, + ownerId: null, + }); + + await taskRunnerPromise; + + expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); + const call = mockCreateTaskRunner.mock.calls[0][0]; + expect(call.taskInstance.state).toEqual({ + foo: '', + bar: '', + baz: '', + }); + }); + + it('should debug log by default when reading an invalid task state', async () => { + const debugLogSpy = jest.spyOn(pollingLifecycleOpts.logger, 'debug'); + const taskRunnerPromise = new Promise((resolve) => { + mockTaskTypeRunFn.mockImplementation(() => { + setTimeout(resolve, 0); + return { state: {} }; + }); + }); + + await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { + id: 'foo', + taskType: 'fooType', + params: { foo: true }, + state: { foo: true, bar: 'test', baz: 'test' }, + stateVersion: 4, + runAt: new Date(), + enabled: true, + scheduledAt: new Date(), + attempts: 0, + status: TaskStatus.Idle, + startedAt: null, + retryAt: null, + ownerId: null, + }); + + await taskRunnerPromise; + + expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); + const call = mockCreateTaskRunner.mock.calls[0][0]; + expect(call.taskInstance.state).toEqual({ + foo: true, + bar: 'test', + baz: 'test', + }); + + expect(debugLogSpy).toHaveBeenCalledWith( + `[fooType][foo] Failed to validate the task's state. Allowing read operation to proceed because allow_reading_invalid_state is true. Error: [foo]: expected value of type [string] but got [boolean]` + ); + }); + + // it('should fail the task run when setting allow_reading_invalid_state:false and reading an invalid state', async () => { + + // }); +}); From ce6c637b9a3f04afa96a41672dc0364ed69ef674 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Tue, 13 Jun 2023 10:15:48 -0400 Subject: [PATCH 23/29] Add jest integration test for config changes --- .../server/integration_tests/lib/index.ts | 1 + .../server/integration_tests/lib/retry.ts | 29 ++ .../lib/setup_test_servers.ts | 26 +- .../task_state_validation.test.ts | 352 +++++++++++------- 4 files changed, 255 insertions(+), 153 deletions(-) create mode 100644 x-pack/plugins/task_manager/server/integration_tests/lib/retry.ts diff --git a/x-pack/plugins/task_manager/server/integration_tests/lib/index.ts b/x-pack/plugins/task_manager/server/integration_tests/lib/index.ts index e27d2aee383494..ab7f23c98e06ff 100644 --- a/x-pack/plugins/task_manager/server/integration_tests/lib/index.ts +++ b/x-pack/plugins/task_manager/server/integration_tests/lib/index.ts @@ -7,3 +7,4 @@ export { injectTask } from './inject_task'; export { setupTestServers } from './setup_test_servers'; +export { retry } from './retry'; diff --git a/x-pack/plugins/task_manager/server/integration_tests/lib/retry.ts b/x-pack/plugins/task_manager/server/integration_tests/lib/retry.ts new file mode 100644 index 00000000000000..17aa83c6479bf2 --- /dev/null +++ b/x-pack/plugins/task_manager/server/integration_tests/lib/retry.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. + */ + +interface RetryOpts { + times: number; + intervalMs: number; +} + +export async function retry( + cb: () => Promise, + options: RetryOpts = { times: 60, intervalMs: 500 } +) { + let attempt = 1; + while (true) { + try { + return await cb(); + } catch (e) { + if (attempt >= options.times) { + throw e; + } + } + attempt++; + await new Promise((resolve) => setTimeout(resolve, options.intervalMs)); + } +} diff --git a/x-pack/plugins/task_manager/server/integration_tests/lib/setup_test_servers.ts b/x-pack/plugins/task_manager/server/integration_tests/lib/setup_test_servers.ts index 9bb4fb4a5ee317..7ded9629eb31ee 100644 --- a/x-pack/plugins/task_manager/server/integration_tests/lib/setup_test_servers.ts +++ b/x-pack/plugins/task_manager/server/integration_tests/lib/setup_test_servers.ts @@ -5,6 +5,7 @@ * 2.0. */ +import deepmerge from 'deepmerge'; import { createTestServers, createRootWithCorePlugins } from '@kbn/core-test-helpers-kbn-server'; export async function setupTestServers(settings = {}) { @@ -20,19 +21,22 @@ export async function setupTestServers(settings = {}) { const esServer = await startES(); const root = createRootWithCorePlugins( - { - logging: { - root: { - level: 'warn', - }, - loggers: [ - { - name: 'plugins.taskManager', - level: 'all', + deepmerge( + { + logging: { + root: { + level: 'warn', }, - ], + loggers: [ + { + name: 'plugins.taskManager', + level: 'all', + }, + ], + }, }, - }, + settings + ), { oss: false } ); diff --git a/x-pack/plugins/task_manager/server/integration_tests/task_state_validation.test.ts b/x-pack/plugins/task_manager/server/integration_tests/task_state_validation.test.ts index 180afb79e81eec..ac72a726850ea7 100644 --- a/x-pack/plugins/task_manager/server/integration_tests/task_state_validation.test.ts +++ b/x-pack/plugins/task_manager/server/integration_tests/task_state_validation.test.ts @@ -14,7 +14,7 @@ import { TaskStatus } from '../task'; import { type TaskPollingLifecycleOpts } from '../polling_lifecycle'; import { type TaskClaimingOpts } from '../queries/task_claiming'; import { TaskManagerPlugin, type TaskManagerStartContract } from '../plugin'; -import { injectTask, setupTestServers } from './lib'; +import { injectTask, setupTestServers, retry } from './lib'; const { TaskPollingLifecycle: TaskPollingLifecycleMock } = jest.requireMock('../polling_lifecycle'); jest.mock('../polling_lifecycle', () => { @@ -77,182 +77,250 @@ jest.mock('../queries/task_claiming', () => { const taskManagerStartSpy = jest.spyOn(TaskManagerPlugin.prototype, 'start'); describe('task state validation', () => { - let esServer: TestElasticsearchUtils; - let kibanaServer: TestKibanaUtils; - let taskManagerPlugin: TaskManagerStartContract; - let pollingLifecycleOpts: TaskPollingLifecycleOpts; + describe('allow_reading_invalid_state: true', () => { + let esServer: TestElasticsearchUtils; + let kibanaServer: TestKibanaUtils; + let taskManagerPlugin: TaskManagerStartContract; + let pollingLifecycleOpts: TaskPollingLifecycleOpts; - beforeAll(async () => { - const setupResult = await setupTestServers(); - esServer = setupResult.esServer; - kibanaServer = setupResult.kibanaServer; + beforeAll(async () => { + const setupResult = await setupTestServers(); + esServer = setupResult.esServer; + kibanaServer = setupResult.kibanaServer; - expect(taskManagerStartSpy).toHaveBeenCalledTimes(1); - taskManagerPlugin = taskManagerStartSpy.mock.results[0].value; + expect(taskManagerStartSpy).toHaveBeenCalledTimes(1); + taskManagerPlugin = taskManagerStartSpy.mock.results[0].value; - expect(TaskPollingLifecycleMock).toHaveBeenCalledTimes(1); - pollingLifecycleOpts = TaskPollingLifecycleMock.mock.calls[0][0]; - }); + expect(TaskPollingLifecycleMock).toHaveBeenCalledTimes(1); + pollingLifecycleOpts = TaskPollingLifecycleMock.mock.calls[0][0]; + }); - afterAll(async () => { - if (kibanaServer) { - await kibanaServer.stop(); - } - if (esServer) { - await esServer.stop(); - } - }); + afterAll(async () => { + if (kibanaServer) { + await kibanaServer.stop(); + } + if (esServer) { + await esServer.stop(); + } + }); - beforeEach(() => { - jest.clearAllMocks(); - }); + beforeEach(() => { + jest.clearAllMocks(); + }); - afterEach(async () => { - await taskManagerPlugin.removeIfExists('foo'); - }); + afterEach(async () => { + await taskManagerPlugin.removeIfExists('foo'); + }); - it('should drop unknown fields from the task state', async () => { - const taskRunnerPromise = new Promise((resolve) => { - mockTaskTypeRunFn.mockImplementation(() => { - setTimeout(resolve, 0); - return { state: {} }; + it('should drop unknown fields from the task state', async () => { + const taskRunnerPromise = new Promise((resolve) => { + mockTaskTypeRunFn.mockImplementation(() => { + setTimeout(resolve, 0); + return { state: {} }; + }); }); - }); - await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { - id: 'foo', - taskType: 'fooType', - params: { foo: true }, - state: { foo: 'test', bar: 'test', baz: 'test', invalidProperty: 'invalid' }, - stateVersion: 4, - runAt: new Date(), - enabled: true, - scheduledAt: new Date(), - attempts: 0, - status: TaskStatus.Idle, - startedAt: null, - retryAt: null, - ownerId: null, - }); + await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { + id: 'foo', + taskType: 'fooType', + params: { foo: true }, + state: { foo: 'test', bar: 'test', baz: 'test', invalidProperty: 'invalid' }, + stateVersion: 4, + runAt: new Date(), + enabled: true, + scheduledAt: new Date(), + attempts: 0, + status: TaskStatus.Idle, + startedAt: null, + retryAt: null, + ownerId: null, + }); - await taskRunnerPromise; + await taskRunnerPromise; - expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); - const call = mockCreateTaskRunner.mock.calls[0][0]; - expect(call.taskInstance.state).toEqual({ - foo: 'test', - bar: 'test', - baz: 'test', + expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); + const call = mockCreateTaskRunner.mock.calls[0][0]; + expect(call.taskInstance.state).toEqual({ + foo: 'test', + bar: 'test', + baz: 'test', + }); }); - }); - it('should fail to update the task if the task runner returns an unknown property in the state', async () => { - const errorLogSpy = jest.spyOn(pollingLifecycleOpts.logger, 'error'); - const taskRunnerPromise = new Promise((resolve) => { - mockTaskTypeRunFn.mockImplementation(() => { - setTimeout(resolve, 0); - return { state: { invalidField: true, foo: 'test', bar: 'test', baz: 'test' } }; + it('should fail to update the task if the task runner returns an unknown property in the state', async () => { + const errorLogSpy = jest.spyOn(pollingLifecycleOpts.logger, 'error'); + const taskRunnerPromise = new Promise((resolve) => { + mockTaskTypeRunFn.mockImplementation(() => { + setTimeout(resolve, 0); + return { state: { invalidField: true, foo: 'test', bar: 'test', baz: 'test' } }; + }); }); - }); - await taskManagerPlugin.schedule({ - id: 'foo', - taskType: 'fooType', - params: {}, - state: { foo: 'test', bar: 'test', baz: 'test' }, - schedule: { interval: '1d' }, - }); + await taskManagerPlugin.schedule({ + id: 'foo', + taskType: 'fooType', + params: {}, + state: { foo: 'test', bar: 'test', baz: 'test' }, + schedule: { interval: '1d' }, + }); - await taskRunnerPromise; + await taskRunnerPromise; - expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); - const call = mockCreateTaskRunner.mock.calls[0][0]; - expect(call.taskInstance.state).toEqual({ - foo: 'test', - bar: 'test', - baz: 'test', + expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); + const call = mockCreateTaskRunner.mock.calls[0][0]; + expect(call.taskInstance.state).toEqual({ + foo: 'test', + bar: 'test', + baz: 'test', + }); + expect(errorLogSpy).toHaveBeenCalledWith( + 'Task fooType "foo" failed: Error: [invalidField]: definition for this key is missing', + expect.anything() + ); }); - expect(errorLogSpy).toHaveBeenCalledWith( - 'Task fooType "foo" failed: Error: [invalidField]: definition for this key is missing', - expect.anything() - ); - }); - it('should migrate the task state', async () => { - const taskRunnerPromise = new Promise((resolve) => { - mockTaskTypeRunFn.mockImplementation(() => { - setTimeout(resolve, 0); - return { state: {} }; + it('should migrate the task state', async () => { + const taskRunnerPromise = new Promise((resolve) => { + mockTaskTypeRunFn.mockImplementation(() => { + setTimeout(resolve, 0); + return { state: {} }; + }); }); - }); - await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { - id: 'foo', - taskType: 'fooType', - params: { foo: true }, - state: {}, - runAt: new Date(), - enabled: true, - scheduledAt: new Date(), - attempts: 0, - status: TaskStatus.Idle, - startedAt: null, - retryAt: null, - ownerId: null, + await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { + id: 'foo', + taskType: 'fooType', + params: { foo: true }, + state: {}, + runAt: new Date(), + enabled: true, + scheduledAt: new Date(), + attempts: 0, + status: TaskStatus.Idle, + startedAt: null, + retryAt: null, + ownerId: null, + }); + + await taskRunnerPromise; + + expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); + const call = mockCreateTaskRunner.mock.calls[0][0]; + expect(call.taskInstance.state).toEqual({ + foo: '', + bar: '', + baz: '', + }); }); - await taskRunnerPromise; + it('should debug log by default when reading an invalid task state', async () => { + const debugLogSpy = jest.spyOn(pollingLifecycleOpts.logger, 'debug'); + const taskRunnerPromise = new Promise((resolve) => { + mockTaskTypeRunFn.mockImplementation(() => { + setTimeout(resolve, 0); + return { state: {} }; + }); + }); + + await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { + id: 'foo', + taskType: 'fooType', + params: { foo: true }, + state: { foo: true, bar: 'test', baz: 'test' }, + stateVersion: 4, + runAt: new Date(), + enabled: true, + scheduledAt: new Date(), + attempts: 0, + status: TaskStatus.Idle, + startedAt: null, + retryAt: null, + ownerId: null, + }); + + await taskRunnerPromise; - expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); - const call = mockCreateTaskRunner.mock.calls[0][0]; - expect(call.taskInstance.state).toEqual({ - foo: '', - bar: '', - baz: '', + expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); + const call = mockCreateTaskRunner.mock.calls[0][0]; + expect(call.taskInstance.state).toEqual({ + foo: true, + bar: 'test', + baz: 'test', + }); + + expect(debugLogSpy).toHaveBeenCalledWith( + `[fooType][foo] Failed to validate the task's state. Allowing read operation to proceed because allow_reading_invalid_state is true. Error: [foo]: expected value of type [string] but got [boolean]` + ); }); }); - it('should debug log by default when reading an invalid task state', async () => { - const debugLogSpy = jest.spyOn(pollingLifecycleOpts.logger, 'debug'); - const taskRunnerPromise = new Promise((resolve) => { - mockTaskTypeRunFn.mockImplementation(() => { - setTimeout(resolve, 0); - return { state: {} }; + describe('allow_reading_invalid_state: false', () => { + let esServer: TestElasticsearchUtils; + let kibanaServer: TestKibanaUtils; + let taskManagerPlugin: TaskManagerStartContract; + let pollingLifecycleOpts: TaskPollingLifecycleOpts; + + beforeAll(async () => { + const setupResult = await setupTestServers({ + xpack: { + task_manager: { + allow_reading_invalid_state: false, + }, + }, }); + esServer = setupResult.esServer; + kibanaServer = setupResult.kibanaServer; + + expect(taskManagerStartSpy).toHaveBeenCalledTimes(1); + taskManagerPlugin = taskManagerStartSpy.mock.results[0].value; + + expect(TaskPollingLifecycleMock).toHaveBeenCalledTimes(1); + pollingLifecycleOpts = TaskPollingLifecycleMock.mock.calls[0][0]; }); - await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { - id: 'foo', - taskType: 'fooType', - params: { foo: true }, - state: { foo: true, bar: 'test', baz: 'test' }, - stateVersion: 4, - runAt: new Date(), - enabled: true, - scheduledAt: new Date(), - attempts: 0, - status: TaskStatus.Idle, - startedAt: null, - retryAt: null, - ownerId: null, + afterAll(async () => { + if (kibanaServer) { + await kibanaServer.stop(); + } + if (esServer) { + await esServer.stop(); + } }); - await taskRunnerPromise; + beforeEach(() => { + jest.clearAllMocks(); + }); - expect(mockCreateTaskRunner).toHaveBeenCalledTimes(1); - const call = mockCreateTaskRunner.mock.calls[0][0]; - expect(call.taskInstance.state).toEqual({ - foo: true, - bar: 'test', - baz: 'test', + afterEach(async () => { + await taskManagerPlugin.removeIfExists('foo'); }); - expect(debugLogSpy).toHaveBeenCalledWith( - `[fooType][foo] Failed to validate the task's state. Allowing read operation to proceed because allow_reading_invalid_state is true. Error: [foo]: expected value of type [string] but got [boolean]` - ); - }); + it('should fail the task run when setting allow_reading_invalid_state:false and reading an invalid state', async () => { + const errorLogSpy = jest.spyOn(pollingLifecycleOpts.logger, 'error'); - // it('should fail the task run when setting allow_reading_invalid_state:false and reading an invalid state', async () => { + await injectTask(kibanaServer.coreStart.elasticsearch.client.asInternalUser, { + id: 'foo', + taskType: 'fooType', + params: { foo: true }, + state: { foo: true, bar: 'test', baz: 'test' }, + stateVersion: 4, + runAt: new Date(), + enabled: true, + scheduledAt: new Date(), + attempts: 0, + status: TaskStatus.Idle, + startedAt: null, + retryAt: null, + ownerId: null, + }); - // }); + await retry(async () => { + expect(errorLogSpy).toHaveBeenCalledWith( + `Failed to poll for work: Error: [foo]: expected value of type [string] but got [boolean]` + ); + }); + + expect(mockCreateTaskRunner).not.toHaveBeenCalled(); + }); + }); }); From cee90832e6ffc0b09876b6d9d10fb4c447c0b3b9 Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Tue, 13 Jun 2023 14:23:07 +0000 Subject: [PATCH 24/29] [CI] Auto-commit changed files from 'node scripts/lint_ts_projects --fix' --- x-pack/plugins/task_manager/tsconfig.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/task_manager/tsconfig.json b/x-pack/plugins/task_manager/tsconfig.json index 35e024db1f87d1..20fdb90611518f 100644 --- a/x-pack/plugins/task_manager/tsconfig.json +++ b/x-pack/plugins/task_manager/tsconfig.json @@ -19,7 +19,8 @@ "@kbn/es-types", "@kbn/apm-utils", "@kbn/core-saved-objects-common", - "@kbn/core-saved-objects-utils-server" + "@kbn/core-saved-objects-utils-server", + "@kbn/core-test-helpers-kbn-server" ], "exclude": [ "target/**/*", From 8fda328e038280fc6b77a1248a1c1f397ec240ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20C=C3=B4t=C3=A9?= Date: Thu, 22 Jun 2023 12:39:46 -0400 Subject: [PATCH 25/29] Update x-pack/plugins/task_manager/server/task_validator.ts Co-authored-by: Ying Mao --- x-pack/plugins/task_manager/server/task_validator.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts index 9e090875671d5f..8d7c0ed2f326cf 100644 --- a/x-pack/plugins/task_manager/server/task_validator.ts +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -55,7 +55,7 @@ export class TaskValidator { } const taskTypeDef = this.definitions.get(task.taskType); - const lastestStateSchema = this.cachedGetLatestStateSchema(taskTypeDef); + const latestStateSchema = this.cachedGetLatestStateSchema(taskTypeDef); // TODO: Remove once all task types have defined their state schema. // https://github.com/elastic/kibana/issues/159347 From 0d2e54d75d78399734a468550147a116683e3baf Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 22 Jun 2023 12:42:43 -0400 Subject: [PATCH 26/29] Finalize typo fix --- .../task_manager/server/task_validator.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts index 8d7c0ed2f326cf..5d58b80eedf8f2 100644 --- a/x-pack/plugins/task_manager/server/task_validator.ts +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -60,7 +60,7 @@ export class TaskValidator { // TODO: Remove once all task types have defined their state schema. // https://github.com/elastic/kibana/issues/159347 // Otherwise, failures on read / write would occur. (don't forget to unskip test) - if (!lastestStateSchema) { + if (!latestStateSchema) { return task; } @@ -68,9 +68,9 @@ export class TaskValidator { let state = task.state; try { state = this.getValidatedStateSchema( - this.migrateTaskState(task.state, task.stateVersion, taskTypeDef, lastestStateSchema), + this.migrateTaskState(task.state, task.stateVersion, taskTypeDef, latestStateSchema), task.taskType, - lastestStateSchema, + latestStateSchema, 'ignore' ); } catch (e) { @@ -91,8 +91,8 @@ export class TaskValidator { // We are doing a write operation which must validate against the latest state schema return { ...task, - state: this.getValidatedStateSchema(task.state, task.taskType, lastestStateSchema, 'forbid'), - stateVersion: lastestStateSchema?.version, + state: this.getValidatedStateSchema(task.state, task.taskType, latestStateSchema, 'forbid'), + stateVersion: latestStateSchema?.version, }; } @@ -100,14 +100,14 @@ export class TaskValidator { state: ConcreteTaskInstance['state'], currentVersion: number | undefined, taskTypeDef: TaskDefinition, - lastestStateSchema: LatestStateSchema + latestStateSchema: LatestStateSchema ) { - if (!lastestStateSchema || (currentVersion && currentVersion >= lastestStateSchema.version)) { + if (!latestStateSchema || (currentVersion && currentVersion >= latestStateSchema.version)) { return state; } let migratedState = state; - for (let i = currentVersion || 1; i <= lastestStateSchema.version; i++) { + for (let i = currentVersion || 1; i <= latestStateSchema.version; i++) { if (!taskTypeDef.stateSchemaByVersion || !taskTypeDef.stateSchemaByVersion[`${i}`]) { throw new Error( `[TaskValidator] state schema for ${taskTypeDef.type} missing version: ${i}` From 6b9125a36ca94608c0cee0181616e394643223f4 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 22 Jun 2023 13:00:13 -0400 Subject: [PATCH 27/29] Pass options.validate to getValidatedTaskInstance to avoid having ternary operators everywhere --- .../server/buffered_task_store.ts | 16 +++++------ .../plugins/task_manager/server/task_store.ts | 22 ++++++++------- .../server/task_validator.test.ts | 27 +++++++++++++++++++ .../task_manager/server/task_validator.ts | 10 ++++++- 4 files changed, 55 insertions(+), 20 deletions(-) diff --git a/x-pack/plugins/task_manager/server/buffered_task_store.ts b/x-pack/plugins/task_manager/server/buffered_task_store.ts index a5f2e57028df34..e0e782028d438b 100644 --- a/x-pack/plugins/task_manager/server/buffered_task_store.ts +++ b/x-pack/plugins/task_manager/server/buffered_task_store.ts @@ -35,15 +35,13 @@ export class BufferedTaskStore implements Updatable { doc: ConcreteTaskInstance, options: { validate: boolean } ): Promise { - let docToUpdate = doc; - if (options.validate) { - docToUpdate = this.taskStore.taskValidator.getValidatedTaskInstance(doc, 'write'); - } - let result = await unwrapPromise(this.bufferedUpdate(docToUpdate)); - if (options.validate) { - result = this.taskStore.taskValidator.getValidatedTaskInstance(result, 'read'); - } - return result; + const docToUpdate = this.taskStore.taskValidator.getValidatedTaskInstance(doc, 'write', { + validate: options.validate, + }); + const result = await unwrapPromise(this.bufferedUpdate(docToUpdate)); + return this.taskStore.taskValidator.getValidatedTaskInstance(result, 'read', { + validate: options.validate, + }); } public async remove(id: string): Promise { diff --git a/x-pack/plugins/task_manager/server/task_store.ts b/x-pack/plugins/task_manager/server/task_store.ts index f7dc5a4f6a3c22..3684b10403b68d 100644 --- a/x-pack/plugins/task_manager/server/task_store.ts +++ b/x-pack/plugins/task_manager/server/task_store.ts @@ -247,9 +247,9 @@ export class TaskStore { doc: ConcreteTaskInstance, options: { validate: boolean } ): Promise { - const taskInstance = options.validate - ? this.taskValidator.getValidatedTaskInstance(doc, 'write') - : doc; + const taskInstance = this.taskValidator.getValidatedTaskInstance(doc, 'write', { + validate: options.validate, + }); const attributes = taskInstanceToAttributes(taskInstance); let updatedSavedObject; @@ -275,7 +275,9 @@ export class TaskStore { // This is far from ideal, but unless we change the SavedObjectsClient this is the best we can do { ...updatedSavedObject, attributes: defaults(updatedSavedObject.attributes, attributes) } ); - return options.validate ? this.taskValidator.getValidatedTaskInstance(result, 'read') : result; + return this.taskValidator.getValidatedTaskInstance(result, 'read', { + validate: options.validate, + }); } /** @@ -290,9 +292,9 @@ export class TaskStore { options: { validate: boolean } ): Promise { const attributesByDocId = docs.reduce((attrsById, doc) => { - const taskInstance = options.validate - ? this.taskValidator.getValidatedTaskInstance(doc, 'write') - : doc; + const taskInstance = this.taskValidator.getValidatedTaskInstance(doc, 'write', { + validate: options.validate, + }); attrsById.set(doc.id, taskInstanceToAttributes(taskInstance)); return attrsById; }, new Map()); @@ -332,9 +334,9 @@ export class TaskStore { attributesByDocId.get(updatedSavedObject.id)! ), }); - const result = options.validate - ? this.taskValidator.getValidatedTaskInstance(taskInstance, 'read') - : taskInstance; + const result = this.taskValidator.getValidatedTaskInstance(taskInstance, 'read', { + validate: options.validate, + }); return asOk(result); }); } diff --git a/x-pack/plugins/task_manager/server/task_validator.test.ts b/x-pack/plugins/task_manager/server/task_validator.test.ts index c0c368575cd6ba..a24a0ea094b5f3 100644 --- a/x-pack/plugins/task_manager/server/task_validator.test.ts +++ b/x-pack/plugins/task_manager/server/task_validator.test.ts @@ -39,6 +39,33 @@ describe('TaskValidator', () => { expect(result).toEqual(task); }); + it(`should return the task as-is whenever the validate:false option is passed-in`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + }, + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask(); + const result = taskValidator.getValidatedTaskInstance(task, 'read', { validate: false }); + expect(result).toEqual(task); + }); + + // TODO: Remove skip once all task types have defined their state schema. + // https://github.com/elastic/kibana/issues/159347 it.skip(`should fail to validate the state schema when the task type doesn't have stateSchemaByVersion defined`, () => { const definitions = new TaskTypeDictionary(mockLogger()); definitions.registerTaskDefinitions({ diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts index 5d58b80eedf8f2..9f3b236bb0b33d 100644 --- a/x-pack/plugins/task_manager/server/task_validator.ts +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -47,7 +47,15 @@ export class TaskValidator { ); } - public getValidatedTaskInstance(task: T, mode: 'read' | 'write'): T { + public getValidatedTaskInstance( + task: T, + mode: 'read' | 'write', + options: { validate: boolean } = { validate: true } + ): T { + if (!options.validate) { + return task; + } + // In the scenario the task is unused / deprecated and Kibana needs to manipulate the task, // we'll do a pass-through for those if (!this.definitions.has(task.taskType)) { From 9187c46cab5acd69ca584cc08f45ce24e39adf24 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 22 Jun 2023 13:53:07 -0400 Subject: [PATCH 28/29] Fix failing jest tests --- .../task_manager/server/buffered_task_store.test.ts | 10 +++++++--- .../plugins/task_manager/server/task_store.test.ts | 12 ++++++++---- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/x-pack/plugins/task_manager/server/buffered_task_store.test.ts b/x-pack/plugins/task_manager/server/buffered_task_store.test.ts index 5cf79ab335da3b..335244df9b5b9c 100644 --- a/x-pack/plugins/task_manager/server/buffered_task_store.test.ts +++ b/x-pack/plugins/task_manager/server/buffered_task_store.test.ts @@ -36,12 +36,14 @@ describe('Buffered Task Store', () => { expect(taskStore.taskValidator.getValidatedTaskInstance).toHaveBeenNthCalledWith( 1, task, - 'write' + 'write', + { validate: true } ); expect(taskStore.taskValidator.getValidatedTaskInstance).toHaveBeenNthCalledWith( 2, task, - 'read' + 'read', + { validate: true } ); }); @@ -56,7 +58,9 @@ describe('Buffered Task Store', () => { expect(await bufferedStore.update(task, { validate: false })).toMatchObject(task); expect(taskStore.bulkUpdate).toHaveBeenCalledWith([task], { validate: false }); - expect(taskStore.taskValidator.getValidatedTaskInstance).not.toHaveBeenCalled(); + expect(taskStore.taskValidator.getValidatedTaskInstance).toHaveBeenCalledWith(task, 'write', { + validate: false, + }); }); test('handles partially successfull bulkUpdates resolving each call appropriately', async () => { diff --git a/x-pack/plugins/task_manager/server/task_store.test.ts b/x-pack/plugins/task_manager/server/task_store.test.ts index 078769882f913d..452bb0aea376d0 100644 --- a/x-pack/plugins/task_manager/server/task_store.test.ts +++ b/x-pack/plugins/task_manager/server/task_store.test.ts @@ -482,8 +482,12 @@ describe('TaskStore', () => { const result = await store.update(task, { validate: true }); expect(mockGetValidatedTaskInstance).toHaveBeenCalledTimes(2); - expect(mockGetValidatedTaskInstance).toHaveBeenNthCalledWith(1, task, 'write'); - expect(mockGetValidatedTaskInstance).toHaveBeenNthCalledWith(2, task, 'read'); + expect(mockGetValidatedTaskInstance).toHaveBeenNthCalledWith(1, task, 'write', { + validate: true, + }); + expect(mockGetValidatedTaskInstance).toHaveBeenNthCalledWith(2, task, 'read', { + validate: true, + }); expect(savedObjectsClient.update).toHaveBeenCalledWith( 'task', task.id, @@ -548,7 +552,7 @@ describe('TaskStore', () => { await store.update(task, { validate: false }); - expect(mockGetValidatedTaskInstance).not.toHaveBeenCalled(); + expect(mockGetValidatedTaskInstance).toHaveBeenCalledWith(task, 'write', { validate: false }); }); test('pushes error from saved objects client to errors$', async () => { @@ -629,7 +633,7 @@ describe('TaskStore', () => { await store.bulkUpdate([task], { validate: false }); - expect(mockGetValidatedTaskInstance).not.toHaveBeenCalled(); + expect(mockGetValidatedTaskInstance).toHaveBeenCalledWith(task, 'write', { validate: false }); }); test('pushes error from saved objects client to errors$', async () => { From 4ee1471d197d5d6acca6fa87fcc23367a22d5c8e Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Fri, 23 Jun 2023 09:20:23 -0400 Subject: [PATCH 29/29] Split the functions in taskvalidator --- .../server/buffered_task_store.test.ts | 18 +- .../server/buffered_task_store.ts | 4 +- .../task_manager/server/task_store.mock.ts | 3 +- .../task_manager/server/task_store.test.ts | 27 +- .../plugins/task_manager/server/task_store.ts | 34 +- .../server/task_validator.test.ts | 497 ++++++++++-------- .../task_manager/server/task_validator.ts | 66 ++- 7 files changed, 364 insertions(+), 285 deletions(-) diff --git a/x-pack/plugins/task_manager/server/buffered_task_store.test.ts b/x-pack/plugins/task_manager/server/buffered_task_store.test.ts index 335244df9b5b9c..08d84c8236cf12 100644 --- a/x-pack/plugins/task_manager/server/buffered_task_store.test.ts +++ b/x-pack/plugins/task_manager/server/buffered_task_store.test.ts @@ -32,17 +32,14 @@ describe('Buffered Task Store', () => { expect(await bufferedStore.update(task, { validate: true })).toMatchObject(task); expect(taskStore.bulkUpdate).toHaveBeenCalledWith([task], { validate: false }); - expect(taskStore.taskValidator.getValidatedTaskInstance).toHaveBeenCalledTimes(2); - expect(taskStore.taskValidator.getValidatedTaskInstance).toHaveBeenNthCalledWith( - 1, + expect(taskStore.taskValidator.getValidatedTaskInstanceForUpdating).toHaveBeenCalledTimes(1); + expect(taskStore.taskValidator.getValidatedTaskInstanceFromReading).toHaveBeenCalledTimes(1); + expect(taskStore.taskValidator.getValidatedTaskInstanceForUpdating).toHaveBeenCalledWith( task, - 'write', { validate: true } ); - expect(taskStore.taskValidator.getValidatedTaskInstance).toHaveBeenNthCalledWith( - 2, + expect(taskStore.taskValidator.getValidatedTaskInstanceFromReading).toHaveBeenCalledWith( task, - 'read', { validate: true } ); }); @@ -58,9 +55,10 @@ describe('Buffered Task Store', () => { expect(await bufferedStore.update(task, { validate: false })).toMatchObject(task); expect(taskStore.bulkUpdate).toHaveBeenCalledWith([task], { validate: false }); - expect(taskStore.taskValidator.getValidatedTaskInstance).toHaveBeenCalledWith(task, 'write', { - validate: false, - }); + expect(taskStore.taskValidator.getValidatedTaskInstanceForUpdating).toHaveBeenCalledWith( + task, + { validate: false } + ); }); test('handles partially successfull bulkUpdates resolving each call appropriately', async () => { diff --git a/x-pack/plugins/task_manager/server/buffered_task_store.ts b/x-pack/plugins/task_manager/server/buffered_task_store.ts index e0e782028d438b..4135164c1e442e 100644 --- a/x-pack/plugins/task_manager/server/buffered_task_store.ts +++ b/x-pack/plugins/task_manager/server/buffered_task_store.ts @@ -35,11 +35,11 @@ export class BufferedTaskStore implements Updatable { doc: ConcreteTaskInstance, options: { validate: boolean } ): Promise { - const docToUpdate = this.taskStore.taskValidator.getValidatedTaskInstance(doc, 'write', { + const docToUpdate = this.taskStore.taskValidator.getValidatedTaskInstanceForUpdating(doc, { validate: options.validate, }); const result = await unwrapPromise(this.bufferedUpdate(docToUpdate)); - return this.taskStore.taskValidator.getValidatedTaskInstance(result, 'read', { + return this.taskStore.taskValidator.getValidatedTaskInstanceFromReading(result, { validate: options.validate, }); } diff --git a/x-pack/plugins/task_manager/server/task_store.mock.ts b/x-pack/plugins/task_manager/server/task_store.mock.ts index 5042d0bc94ffb0..861f7d60bd221e 100644 --- a/x-pack/plugins/task_manager/server/task_store.mock.ts +++ b/x-pack/plugins/task_manager/server/task_store.mock.ts @@ -15,7 +15,8 @@ export const taskStoreMock = { create({ index = '', taskManagerId = '' }: TaskStoreOptions = {}) { const mocked = { taskValidator: { - getValidatedTaskInstance: jest.fn().mockImplementation((task) => task), + getValidatedTaskInstanceFromReading: jest.fn().mockImplementation((task) => task), + getValidatedTaskInstanceForUpdating: jest.fn().mockImplementation((task) => task), }, convertToSavedObjectIds: jest.fn(), update: jest.fn(), diff --git a/x-pack/plugins/task_manager/server/task_store.test.ts b/x-pack/plugins/task_manager/server/task_store.test.ts index 452bb0aea376d0..2261b8ac8f2ee5 100644 --- a/x-pack/plugins/task_manager/server/task_store.test.ts +++ b/x-pack/plugins/task_manager/server/task_store.test.ts @@ -26,12 +26,14 @@ import { mockLogger } from './test_utils'; import { AdHocTaskCounter } from './lib/adhoc_task_counter'; import { asErr } from './lib/result_type'; -const mockGetValidatedTaskInstance = jest.fn(); +const mockGetValidatedTaskInstanceFromReading = jest.fn(); +const mockGetValidatedTaskInstanceForUpdating = jest.fn(); jest.mock('./task_validator', () => { return { TaskValidator: jest.fn().mockImplementation(() => { return { - getValidatedTaskInstance: mockGetValidatedTaskInstance, + getValidatedTaskInstanceFromReading: mockGetValidatedTaskInstanceFromReading, + getValidatedTaskInstanceForUpdating: mockGetValidatedTaskInstanceForUpdating, }; }), }; @@ -47,10 +49,12 @@ beforeEach(() => { jest.resetAllMocks(); jest.requireMock('./task_validator').TaskValidator.mockImplementation(() => { return { - getValidatedTaskInstance: mockGetValidatedTaskInstance, + getValidatedTaskInstanceFromReading: mockGetValidatedTaskInstanceFromReading, + getValidatedTaskInstanceForUpdating: mockGetValidatedTaskInstanceForUpdating, }; }); - mockGetValidatedTaskInstance.mockImplementation((task) => task); + mockGetValidatedTaskInstanceFromReading.mockImplementation((task) => task); + mockGetValidatedTaskInstanceForUpdating.mockImplementation((task) => task); }); const mockedDate = new Date('2019-02-12T21:01:22.479Z'); @@ -481,11 +485,12 @@ describe('TaskStore', () => { const result = await store.update(task, { validate: true }); - expect(mockGetValidatedTaskInstance).toHaveBeenCalledTimes(2); - expect(mockGetValidatedTaskInstance).toHaveBeenNthCalledWith(1, task, 'write', { + expect(mockGetValidatedTaskInstanceForUpdating).toHaveBeenCalledTimes(1); + expect(mockGetValidatedTaskInstanceFromReading).toHaveBeenCalledTimes(1); + expect(mockGetValidatedTaskInstanceForUpdating).toHaveBeenCalledWith(task, { validate: true, }); - expect(mockGetValidatedTaskInstance).toHaveBeenNthCalledWith(2, task, 'read', { + expect(mockGetValidatedTaskInstanceFromReading).toHaveBeenCalledWith(task, { validate: true, }); expect(savedObjectsClient.update).toHaveBeenCalledWith( @@ -552,7 +557,9 @@ describe('TaskStore', () => { await store.update(task, { validate: false }); - expect(mockGetValidatedTaskInstance).toHaveBeenCalledWith(task, 'write', { validate: false }); + expect(mockGetValidatedTaskInstanceForUpdating).toHaveBeenCalledWith(task, { + validate: false, + }); }); test('pushes error from saved objects client to errors$', async () => { @@ -633,7 +640,9 @@ describe('TaskStore', () => { await store.bulkUpdate([task], { validate: false }); - expect(mockGetValidatedTaskInstance).toHaveBeenCalledWith(task, 'write', { validate: false }); + expect(mockGetValidatedTaskInstanceForUpdating).toHaveBeenCalledWith(task, { + validate: false, + }); }); test('pushes error from saved objects client to errors$', async () => { diff --git a/x-pack/plugins/task_manager/server/task_store.ts b/x-pack/plugins/task_manager/server/task_store.ts index 3684b10403b68d..d3c84e2c4e5619 100644 --- a/x-pack/plugins/task_manager/server/task_store.ts +++ b/x-pack/plugins/task_manager/server/task_store.ts @@ -159,10 +159,8 @@ export class TaskStore { let savedObject; try { - const validatedTaskInstance = this.taskValidator.getValidatedTaskInstance( - taskInstance, - 'write' - ); + const validatedTaskInstance = + this.taskValidator.getValidatedTaskInstanceForUpdating(taskInstance); savedObject = await this.savedObjectsRepository.create( 'task', taskInstanceToAttributes(validatedTaskInstance), @@ -177,7 +175,7 @@ export class TaskStore { } const result = savedObjectToConcreteTaskInstance(savedObject); - return this.taskValidator.getValidatedTaskInstance(result, 'read'); + return this.taskValidator.getValidatedTaskInstanceFromReading(result); } /** @@ -188,10 +186,8 @@ export class TaskStore { public async bulkSchedule(taskInstances: TaskInstance[]): Promise { const objects = taskInstances.map((taskInstance) => { this.definitions.ensureHas(taskInstance.taskType); - const validatedTaskInstance = this.taskValidator.getValidatedTaskInstance( - taskInstance, - 'write' - ); + const validatedTaskInstance = + this.taskValidator.getValidatedTaskInstanceForUpdating(taskInstance); return { type: 'task', attributes: taskInstanceToAttributes(validatedTaskInstance), @@ -217,7 +213,7 @@ export class TaskStore { return savedObjects.saved_objects.map((so) => { const taskInstance = savedObjectToConcreteTaskInstance(so); - return this.taskValidator.getValidatedTaskInstance(taskInstance, 'read'); + return this.taskValidator.getValidatedTaskInstanceFromReading(taskInstance); }); } @@ -247,7 +243,7 @@ export class TaskStore { doc: ConcreteTaskInstance, options: { validate: boolean } ): Promise { - const taskInstance = this.taskValidator.getValidatedTaskInstance(doc, 'write', { + const taskInstance = this.taskValidator.getValidatedTaskInstanceForUpdating(doc, { validate: options.validate, }); const attributes = taskInstanceToAttributes(taskInstance); @@ -275,7 +271,7 @@ export class TaskStore { // This is far from ideal, but unless we change the SavedObjectsClient this is the best we can do { ...updatedSavedObject, attributes: defaults(updatedSavedObject.attributes, attributes) } ); - return this.taskValidator.getValidatedTaskInstance(result, 'read', { + return this.taskValidator.getValidatedTaskInstanceFromReading(result, { validate: options.validate, }); } @@ -292,7 +288,7 @@ export class TaskStore { options: { validate: boolean } ): Promise { const attributesByDocId = docs.reduce((attrsById, doc) => { - const taskInstance = this.taskValidator.getValidatedTaskInstance(doc, 'write', { + const taskInstance = this.taskValidator.getValidatedTaskInstanceForUpdating(doc, { validate: options.validate, }); attrsById.set(doc.id, taskInstanceToAttributes(taskInstance)); @@ -334,7 +330,7 @@ export class TaskStore { attributesByDocId.get(updatedSavedObject.id)! ), }); - const result = this.taskValidator.getValidatedTaskInstance(taskInstance, 'read', { + const result = this.taskValidator.getValidatedTaskInstanceFromReading(taskInstance, { validate: options.validate, }); return asOk(result); @@ -387,7 +383,7 @@ export class TaskStore { throw e; } const taskInstance = savedObjectToConcreteTaskInstance(result); - return this.taskValidator.getValidatedTaskInstance(taskInstance, 'read'); + return this.taskValidator.getValidatedTaskInstanceFromReading(taskInstance); } /** @@ -411,10 +407,8 @@ export class TaskStore { return asErr({ id: task.id, type: task.type, error: task.error }); } const taskInstance = savedObjectToConcreteTaskInstance(task); - const validatedTaskInstance = this.taskValidator.getValidatedTaskInstance( - taskInstance, - 'read' - ); + const validatedTaskInstance = + this.taskValidator.getValidatedTaskInstanceFromReading(taskInstance); return asOk(validatedTaskInstance); }); } @@ -460,7 +454,7 @@ export class TaskStore { .map((doc) => this.serializer.rawToSavedObject(doc)) .map((doc) => omit(doc, 'namespace') as SavedObject) .map((doc) => savedObjectToConcreteTaskInstance(doc)) - .map((doc) => this.taskValidator.getValidatedTaskInstance(doc, 'read')) + .map((doc) => this.taskValidator.getValidatedTaskInstanceFromReading(doc)) .filter((doc): doc is ConcreteTaskInstance => !!doc), }; } catch (e) { diff --git a/x-pack/plugins/task_manager/server/task_validator.test.ts b/x-pack/plugins/task_manager/server/task_validator.test.ts index a24a0ea094b5f3..52822adf6f49fe 100644 --- a/x-pack/plugins/task_manager/server/task_validator.test.ts +++ b/x-pack/plugins/task_manager/server/task_validator.test.ts @@ -26,7 +26,7 @@ const fooTaskDefinition = { }; describe('TaskValidator', () => { - describe('getValidatedTaskInstance()', () => { + describe('getValidatedTaskInstanceFromReading()', () => { it(`should return the task as-is whenever the task definition isn't defined`, () => { const definitions = new TaskTypeDictionary(mockLogger()); const taskValidator = new TaskValidator({ @@ -35,7 +35,7 @@ describe('TaskValidator', () => { allowReadingInvalidState: false, }); const task = taskManagerMock.createTask(); - const result = taskValidator.getValidatedTaskInstance(task, 'read'); + const result = taskValidator.getValidatedTaskInstanceFromReading(task); expect(result).toEqual(task); }); @@ -60,7 +60,7 @@ describe('TaskValidator', () => { allowReadingInvalidState: false, }); const task = taskManagerMock.createTask(); - const result = taskValidator.getValidatedTaskInstance(task, 'read', { validate: false }); + const result = taskValidator.getValidatedTaskInstanceFromReading(task, { validate: false }); expect(result).toEqual(task); }); @@ -78,265 +78,320 @@ describe('TaskValidator', () => { }); const task = taskManagerMock.createTask({ state: { foo: 'bar' } }); expect(() => - taskValidator.getValidatedTaskInstance(task, 'write') + taskValidator.getValidatedTaskInstanceFromReading(task) ).toThrowErrorMatchingInlineSnapshot( `"[TaskValidator] stateSchemaByVersion not defined for task type: foo"` ); }); - describe('mode=write', () => { - it(`should validate the state schema`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), - }, + it(`should validate the state schema`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), }, }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, - }); - const task = taskManagerMock.createTask({ state: { foo: 'bar' } }); - const { stateVersion, ...result } = taskValidator.getValidatedTaskInstance(task, 'write'); - expect(result).toEqual(task); - expect(stateVersion).toEqual(1); + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'bar' } }); + const result = taskValidator.getValidatedTaskInstanceFromReading(task); + expect(result).toEqual(task); + }); - it(`should fail to validate the state schema when unknown fields are present`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), - }, + it(`should fail validation when the state schema doesn't match the state data`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), }, }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, - }); - const task = taskManagerMock.createTask({ state: { foo: 'foo', bar: 'bar' } }); - expect(() => - taskValidator.getValidatedTaskInstance(task, 'write') - ).toThrowErrorMatchingInlineSnapshot(`"[bar]: definition for this key is missing"`); + }, }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: true } }); + expect(() => + taskValidator.getValidatedTaskInstanceFromReading(task) + ).toThrowErrorMatchingInlineSnapshot( + `"[foo]: expected value of type [string] but got [boolean]"` + ); }); - describe('mode=read', () => { - it(`should validate the state schema`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), - }, + it(`should return original state when the state is invalid and allowReadingInvalidState is true`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), }, }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, - }); - const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'bar' } }); - const result = taskValidator.getValidatedTaskInstance(task, 'read'); - expect(result).toEqual(task); + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: true, }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: true } }); + const result = taskValidator.getValidatedTaskInstanceFromReading(task); + expect(result.state).toEqual({ foo: true }); + }); - it(`should fail validation when the state schema doesn't match the state data`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), - }, + it(`should remove unknown fields`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), }, }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, - }); - const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: true } }); - expect(() => - taskValidator.getValidatedTaskInstance(task, 'read') - ).toThrowErrorMatchingInlineSnapshot( - `"[foo]: expected value of type [string] but got [boolean]"` - ); + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ + stateVersion: 1, + state: { foo: 'foo', bar: 'bar' }, }); + const result = taskValidator.getValidatedTaskInstanceFromReading(task); + expect(result.state).toEqual({ foo: 'foo' }); + }); - it(`should return original state when the state is invalid and allowReadingInvalidState is true`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), - }, + it(`should migrate state when reading from a document without stateVersion`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => ({ ...state, baz: 'baz' }), + schema: schema.object({ + foo: schema.string(), + baz: schema.string(), + }), }, }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: true, - }); - const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: true } }); - const result = taskValidator.getValidatedTaskInstance(task, 'read'); - expect(result.state).toEqual({ foo: true }); + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, }); + const task = taskManagerMock.createTask({ stateVersion: undefined, state: { foo: 'foo' } }); + const result = taskValidator.getValidatedTaskInstanceFromReading(task); + expect(result.state).toEqual({ foo: 'foo', baz: 'baz' }); + }); - it(`should remove unknown fields`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), - }, + it(`should migrate state when reading from an older version`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + 2: { + up: (state) => ({ ...state, baz: 'baz' }), + schema: schema.object({ + foo: schema.string(), + baz: schema.string(), + }), }, }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, - }); - const task = taskManagerMock.createTask({ - stateVersion: 1, - state: { foo: 'foo', bar: 'bar' }, - }); - const result = taskValidator.getValidatedTaskInstance(task, 'read'); - expect(result.state).toEqual({ foo: 'foo' }); + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'foo' } }); + const result = taskValidator.getValidatedTaskInstanceFromReading(task); + expect(result.state).toEqual({ foo: 'foo', baz: 'baz' }); + }); - it(`should migrate state when reading from a document without stateVersion`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => ({ ...state, baz: 'baz' }), - schema: schema.object({ - foo: schema.string(), - baz: schema.string(), - }), - }, + it(`should throw during the migration phase if a schema version is missing`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + 3: { + up: (state) => ({ ...state, baz: 'baz' }), + schema: schema.object({ + foo: schema.string(), + baz: schema.string(), + }), }, }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, - }); - const task = taskManagerMock.createTask({ stateVersion: undefined, state: { foo: 'foo' } }); - const result = taskValidator.getValidatedTaskInstance(task, 'read'); - expect(result.state).toEqual({ foo: 'foo', baz: 'baz' }); + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'foo' } }); + expect(() => + taskValidator.getValidatedTaskInstanceFromReading(task) + ).toThrowErrorMatchingInlineSnapshot( + `"[TaskValidator] state schema for foo missing version: 2"` + ); + }); + }); + + describe('getValidatedTaskInstanceForUpdating()', () => { + it(`should return the task as-is whenever the task definition isn't defined`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, }); + const task = taskManagerMock.createTask(); + const result = taskValidator.getValidatedTaskInstanceForUpdating(task); + expect(result).toEqual(task); + }); - it(`should migrate state when reading from an older version`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), - }, - 2: { - up: (state) => ({ ...state, baz: 'baz' }), - schema: schema.object({ - foo: schema.string(), - baz: schema.string(), - }), - }, + it(`should return the task as-is whenever the validate:false option is passed-in`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), }, }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, - }); - const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'foo' } }); - const result = taskValidator.getValidatedTaskInstance(task, 'read'); - expect(result.state).toEqual({ foo: 'foo', baz: 'baz' }); + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, }); + const task = taskManagerMock.createTask(); + const result = taskValidator.getValidatedTaskInstanceForUpdating(task, { validate: false }); + expect(result).toEqual(task); + }); - it(`should throw during the migration phase if a schema version is missing`, () => { - const definitions = new TaskTypeDictionary(mockLogger()); - definitions.registerTaskDefinitions({ - foo: { - ...fooTaskDefinition, - stateSchemaByVersion: { - 1: { - up: (state) => state, - schema: schema.object({ - foo: schema.string(), - }), - }, - 3: { - up: (state) => ({ ...state, baz: 'baz' }), - schema: schema.object({ - foo: schema.string(), - baz: schema.string(), - }), - }, + // TODO: Remove skip once all task types have defined their state schema. + // https://github.com/elastic/kibana/issues/159347 + it.skip(`should fail to validate the state schema when the task type doesn't have stateSchemaByVersion defined`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: fooTaskDefinition, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ state: { foo: 'bar' } }); + expect(() => + taskValidator.getValidatedTaskInstanceForUpdating(task) + ).toThrowErrorMatchingInlineSnapshot( + `"[TaskValidator] stateSchemaByVersion not defined for task type: foo"` + ); + }); + + it(`should validate the state schema`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), }, }, - }); - const taskValidator = new TaskValidator({ - logger: mockLogger(), - definitions, - allowReadingInvalidState: false, - }); - const task = taskManagerMock.createTask({ stateVersion: 1, state: { foo: 'foo' } }); - expect(() => - taskValidator.getValidatedTaskInstance(task, 'read') - ).toThrowErrorMatchingInlineSnapshot( - `"[TaskValidator] state schema for foo missing version: 2"` - ); + }, }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ state: { foo: 'bar' } }); + const { stateVersion, ...result } = taskValidator.getValidatedTaskInstanceForUpdating(task); + expect(result).toEqual(task); + expect(stateVersion).toEqual(1); + }); + + it(`should fail to validate the state schema when unknown fields are present`, () => { + const definitions = new TaskTypeDictionary(mockLogger()); + definitions.registerTaskDefinitions({ + foo: { + ...fooTaskDefinition, + stateSchemaByVersion: { + 1: { + up: (state) => state, + schema: schema.object({ + foo: schema.string(), + }), + }, + }, + }, + }); + const taskValidator = new TaskValidator({ + logger: mockLogger(), + definitions, + allowReadingInvalidState: false, + }); + const task = taskManagerMock.createTask({ state: { foo: 'foo', bar: 'bar' } }); + expect(() => + taskValidator.getValidatedTaskInstanceForUpdating(task) + ).toThrowErrorMatchingInlineSnapshot(`"[bar]: definition for this key is missing"`); }); }); }); diff --git a/x-pack/plugins/task_manager/server/task_validator.ts b/x-pack/plugins/task_manager/server/task_validator.ts index 9f3b236bb0b33d..61d9a903dd5b4c 100644 --- a/x-pack/plugins/task_manager/server/task_validator.ts +++ b/x-pack/plugins/task_manager/server/task_validator.ts @@ -47,9 +47,8 @@ export class TaskValidator { ); } - public getValidatedTaskInstance( + public getValidatedTaskInstanceFromReading( task: T, - mode: 'read' | 'write', options: { validate: boolean } = { validate: true } ): T { if (!options.validate) { @@ -72,28 +71,51 @@ export class TaskValidator { return task; } - if (mode === 'read') { - let state = task.state; - try { - state = this.getValidatedStateSchema( - this.migrateTaskState(task.state, task.stateVersion, taskTypeDef, latestStateSchema), - task.taskType, - latestStateSchema, - 'ignore' - ); - } catch (e) { - if (!this.allowReadingInvalidState) { - throw e; - } - this.logger.debug( - `[${task.taskType}][${task.id}] Failed to validate the task's state. Allowing read operation to proceed because allow_reading_invalid_state is true. Error: ${e.message}` - ); + let state = task.state; + try { + state = this.getValidatedStateSchema( + this.migrateTaskState(task.state, task.stateVersion, taskTypeDef, latestStateSchema), + task.taskType, + latestStateSchema, + 'ignore' + ); + } catch (e) { + if (!this.allowReadingInvalidState) { + throw e; } + this.logger.debug( + `[${task.taskType}][${task.id}] Failed to validate the task's state. Allowing read operation to proceed because allow_reading_invalid_state is true. Error: ${e.message}` + ); + } - return { - ...task, - state, - }; + return { + ...task, + state, + }; + } + + public getValidatedTaskInstanceForUpdating( + task: T, + options: { validate: boolean } = { validate: true } + ): T { + if (!options.validate) { + return task; + } + + // In the scenario the task is unused / deprecated and Kibana needs to manipulate the task, + // we'll do a pass-through for those + if (!this.definitions.has(task.taskType)) { + return task; + } + + const taskTypeDef = this.definitions.get(task.taskType); + const latestStateSchema = this.cachedGetLatestStateSchema(taskTypeDef); + + // TODO: Remove once all task types have defined their state schema. + // https://github.com/elastic/kibana/issues/159347 + // Otherwise, failures on read / write would occur. (don't forget to unskip test) + if (!latestStateSchema) { + return task; } // We are doing a write operation which must validate against the latest state schema