From e21defa448d9bdf3de46201ab84cc88ef1c4e90e Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 20 Jan 2021 17:23:02 +0000 Subject: [PATCH] [Task Manager] Reject invalid Timeout values in Task Type Definitions (#88602) This PR adds the following: 1. We now validate the interval passed to `timeout` when a task type definition is registered. 2. replaces usage of `Joi` with `schema-type` --- .../task_manager/server/lib/intervals.ts | 35 ++++-- .../task_manager/server/lib/result_type.ts | 8 ++ x-pack/plugins/task_manager/server/task.ts | 101 +++++++++--------- .../server/task_running/task_runner.test.ts | 54 +--------- .../server/task_running/task_runner.ts | 19 +--- .../server/task_type_dictionary.test.ts | 63 ++++++++++- .../server/task_type_dictionary.ts | 32 ++---- 7 files changed, 157 insertions(+), 155 deletions(-) diff --git a/x-pack/plugins/task_manager/server/lib/intervals.ts b/x-pack/plugins/task_manager/server/lib/intervals.ts index da04dffa4b5d..b7945ff25d08 100644 --- a/x-pack/plugins/task_manager/server/lib/intervals.ts +++ b/x-pack/plugins/task_manager/server/lib/intervals.ts @@ -12,6 +12,19 @@ export enum IntervalCadence { Hour = 'h', Day = 'd', } + +// Once Babel is updated ot support Typescript 4.x templated types, we can use +// this more accurate and safer compile-time valdiation +// export type Interval = `${number}${IntervalCadence}`; +export type Interval = string; + +export function isInterval(interval: Interval | string): interval is Interval { + const numericAsStr: string = interval.slice(0, -1); + const numeric: number = parseInt(numericAsStr, 10); + const cadence: IntervalCadence | string = interval.slice(-1); + return !(!isCadence(cadence) || isNaN(numeric) || numeric <= 0 || !isNumeric(numericAsStr)); +} + const VALID_CADENCE = new Set(Object.values(IntervalCadence)); const CADENCE_IN_MS: Record = { [IntervalCadence.Second]: 1000, @@ -24,7 +37,7 @@ function isCadence(cadence: IntervalCadence | string): cadence is IntervalCadenc return VALID_CADENCE.has(cadence as IntervalCadence); } -export function asInterval(ms: number): string { +export function asInterval(ms: number): Interval { const secondsRemainder = ms % 1000; const minutesRemainder = ms % 60000; return secondsRemainder ? `${ms}ms` : minutesRemainder ? `${ms / 1000}s` : `${ms / 60000}m`; @@ -34,9 +47,9 @@ export function asInterval(ms: number): string { * Returns a date that is the specified interval from now. Currently, * only minute-intervals and second-intervals are supported. * - * @param {string} interval - An interval of the form `Nm` such as `5m` + * @param {Interval} interval - An interval of the form `Nm` such as `5m` */ -export function intervalFromNow(interval?: string): Date | undefined { +export function intervalFromNow(interval?: Interval): Date | undefined { if (interval === undefined) { return; } @@ -48,9 +61,9 @@ export function intervalFromNow(interval?: string): Date | undefined { * only minute-intervals and second-intervals are supported. * * @param {Date} date - The date to add interval to - * @param {string} interval - An interval of the form `Nm` such as `5m` + * @param {Interval} interval - An interval of the form `Nm` such as `5m` */ -export function intervalFromDate(date: Date, interval?: string): Date | undefined { +export function intervalFromDate(date: Date, interval?: Interval): Date | undefined { if (interval === undefined) { return; } @@ -59,9 +72,11 @@ export function intervalFromDate(date: Date, interval?: string): Date | undefine export function maxIntervalFromDate( date: Date, - ...intervals: Array + ...intervals: Array ): Date | undefined { - const maxSeconds = Math.max(...intervals.filter(isString).map(parseIntervalAsSecond)); + const maxSeconds = Math.max( + ...intervals.filter(isString).map((interval) => parseIntervalAsSecond(interval as Interval)) + ); if (!isNaN(maxSeconds)) { return secondsFromDate(date, maxSeconds); } @@ -91,14 +106,14 @@ export function secondsFromDate(date: Date, secs: number): Date { /** * Verifies that the specified interval matches our expected format. * - * @param {string} interval - An interval such as `5m` or `10s` + * @param {Interval} interval - An interval such as `5m` or `10s` * @returns {number} The interval as seconds */ -export const parseIntervalAsSecond = memoize((interval: string): number => { +export const parseIntervalAsSecond = memoize((interval: Interval): number => { return Math.round(parseIntervalAsMillisecond(interval) / 1000); }); -export const parseIntervalAsMillisecond = memoize((interval: string): number => { +export const parseIntervalAsMillisecond = memoize((interval: Interval): number => { const numericAsStr: string = interval.slice(0, -1); const numeric: number = parseInt(numericAsStr, 10); const cadence: IntervalCadence | string = interval.slice(-1); diff --git a/x-pack/plugins/task_manager/server/lib/result_type.ts b/x-pack/plugins/task_manager/server/lib/result_type.ts index d21c17d3bb5b..cd1d417c7949 100644 --- a/x-pack/plugins/task_manager/server/lib/result_type.ts +++ b/x-pack/plugins/task_manager/server/lib/result_type.ts @@ -39,6 +39,14 @@ export function isErr(result: Result): result is Err { return !isOk(result); } +export function tryAsResult(fn: () => T): Result { + try { + return asOk(fn()); + } catch (e) { + return asErr(e); + } +} + export async function promiseResult(future: Promise): Promise> { try { return asOk(await future); diff --git a/x-pack/plugins/task_manager/server/task.ts b/x-pack/plugins/task_manager/server/task.ts index e832a95ac3ca..9e2a2a2074a8 100644 --- a/x-pack/plugins/task_manager/server/task.ts +++ b/x-pack/plugins/task_manager/server/task.ts @@ -4,7 +4,9 @@ * you may not use this file except in compliance with the Elastic License. */ -import Joi from 'joi'; +import { schema, TypeOf } from '@kbn/config-schema'; +import { Interval, isInterval, parseIntervalAsMillisecond } from './lib/intervals'; +import { isErr, tryAsResult } from './lib/result_type'; /* * Type definitions and validations for tasks. @@ -83,17 +85,8 @@ export interface FailedTaskResult { status: TaskStatus.Failed; } -export const validateRunResult = Joi.object({ - runAt: Joi.date().optional(), - schedule: Joi.object().optional(), - error: Joi.object().optional(), - state: Joi.object().optional(), -}).optional(); - export type RunFunction = () => Promise; - export type CancelFunction = () => Promise; - export interface CancellableTask { run: RunFunction; cancel?: CancelFunction; @@ -101,40 +94,53 @@ export interface CancellableTask { export type TaskRunCreatorFunction = (context: RunContext) => CancellableTask; +export const taskDefinitionSchema = schema.object( + { + /** + * A unique identifier for the type of task being defined. + */ + type: schema.string(), + /** + * A brief, human-friendly title for this task. + */ + title: schema.maybe(schema.string()), + /** + * An optional more detailed description of what this task does. + */ + description: schema.maybe(schema.string()), + /** + * How long, in minutes or seconds, the system should wait for the task to complete + * before it is considered to be timed out. (e.g. '5m', the default). If + * the task takes longer than this, Kibana will send it a kill command and + * the task will be re-attempted. + */ + timeout: schema.string({ + defaultValue: '5m', + }), + /** + * Up to how many times the task should retry when it fails to run. This will + * default to the global variable. + */ + maxAttempts: schema.maybe( + schema.number({ + min: 1, + }) + ), + }, + { + validate({ timeout }) { + if (!isInterval(timeout) || isErr(tryAsResult(() => parseIntervalAsMillisecond(timeout)))) { + return `Invalid timeout "${timeout}". Timeout must be of the form "{number}{cadance}" where number is an integer. Example: 5m.`; + } + }, + } +); + /** * Defines a task which can be scheduled and run by the Kibana * task manager. */ -export interface TaskDefinition { - /** - * A unique identifier for the type of task being defined. - */ - type: string; - - /** - * A brief, human-friendly title for this task. - */ - title: string; - - /** - * An optional more detailed description of what this task does. - */ - description?: string; - - /** - * How long, in minutes or seconds, the system should wait for the task to complete - * before it is considered to be timed out. (e.g. '5m', the default). If - * the task takes longer than this, Kibana will send it a kill command and - * the task will be re-attempted. - */ - timeout?: string; - - /** - * Up to how many times the task should retry when it fails to run. This will - * default to the global variable. - */ - maxAttempts?: number; - +export type TaskDefinition = TypeOf & { /** * Function that customizes how the task should behave when the task fails. This * function can return `true`, `false` or a Date. True will tell task manager @@ -149,17 +155,7 @@ export interface TaskDefinition { * and an optional cancel function which cancels the task. */ createTaskRunner: TaskRunCreatorFunction; -} - -export const validateTaskDefinition = Joi.object({ - type: Joi.string().required(), - title: Joi.string().optional(), - description: Joi.string().optional(), - timeout: Joi.string().default('5m'), - maxAttempts: Joi.number().min(1).optional(), - createTaskRunner: Joi.func().required(), - getRetry: Joi.func().optional(), -}).default(); +}; export enum TaskStatus { Idle = 'idle', @@ -174,12 +170,11 @@ export enum TaskLifecycleResult { } export type TaskLifecycle = TaskStatus | TaskLifecycleResult; - export interface IntervalSchedule { /** * An interval in minutes (e.g. '5m'). If specified, this is a recurring task. * */ - interval: string; + interval: Interval; } /* 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 3777d89ce63d..77434d2b6559 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 @@ -10,10 +10,10 @@ import { secondsFromNow } from '../lib/intervals'; import { asOk, asErr } from '../lib/result_type'; import { TaskManagerRunner, TaskRunResult } from '../task_running'; import { TaskEvent, asTaskRunEvent, asTaskMarkRunningEvent, TaskRun } from '../task_events'; -import { ConcreteTaskInstance, TaskStatus, TaskDefinition, SuccessfulRunResult } from '../task'; +import { ConcreteTaskInstance, TaskStatus } from '../task'; import { SavedObjectsErrorHelpers } from '../../../../../src/core/server'; import moment from 'moment'; -import { TaskTypeDictionary } from '../task_type_dictionary'; +import { TaskDefinitionRegistry, TaskTypeDictionary } from '../task_type_dictionary'; import { mockLogger } from '../test_utils'; import { throwUnrecoverableError } from './errors'; @@ -41,24 +41,6 @@ describe('TaskManagerRunner', () => { expect(runner.toString()).toEqual('bar "foo"'); }); - test('warns if the task returns an unexpected result', async () => { - await allowsReturnType(undefined); - await allowsReturnType({}); - await allowsReturnType({ - runAt: new Date(), - }); - await allowsReturnType({ - error: new Error('Dang it!'), - }); - await allowsReturnType({ - state: { shazm: true }, - }); - await disallowsReturnType('hm....'); - await disallowsReturnType({ - whatIsThis: '?!!?', - }); - }); - test('queues a reattempt if the task fails', async () => { const initialAttempts = _.random(0, 2); const id = Date.now().toString(); @@ -1121,7 +1103,7 @@ describe('TaskManagerRunner', () => { interface TestOpts { instance?: Partial; - definitions?: Record>; + definitions?: TaskDefinitionRegistry; onTaskEvent?: (event: TaskEvent) => void; } @@ -1196,34 +1178,4 @@ describe('TaskManagerRunner', () => { instance, }; } - - async function testReturn(result: unknown, shouldBeValid: boolean) { - const { runner, logger } = testOpts({ - definitions: { - bar: { - title: 'Bar!', - createTaskRunner: () => ({ - run: async () => result as SuccessfulRunResult, - }), - }, - }, - }); - - await runner.run(); - - if (shouldBeValid) { - expect(logger.warn).not.toHaveBeenCalled(); - } else { - expect(logger.warn).toHaveBeenCalledTimes(1); - expect(logger.warn.mock.calls[0][0]).toMatch(/invalid task result/i); - } - } - - function allowsReturnType(result: unknown) { - return testReturn(result, true); - } - - function disallowsReturnType(result: unknown) { - return testReturn(result, false); - } }); 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 d281a65da332..704386d88ea3 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 @@ -13,7 +13,6 @@ import { Logger } from 'src/core/server'; import apm from 'elastic-apm-node'; import { performance } from 'perf_hooks'; -import Joi from 'joi'; import { identity, defaults, flow } from 'lodash'; import { Middleware } from '../lib/middleware'; @@ -36,7 +35,6 @@ import { FailedRunResult, FailedTaskResult, TaskDefinition, - validateRunResult, TaskStatus, } from '../task'; import { TaskTypeDictionary } from '../task_type_dictionary'; @@ -311,20 +309,9 @@ export class TaskManagerRunner implements TaskRunner { private validateResult( result?: SuccessfulRunResult | FailedRunResult | void ): Result { - const { error } = Joi.validate(result, validateRunResult); - - if (error) { - this.logger.warn(`Invalid task result for ${this}: ${error.message}`); - return asErr({ - error: new Error(`Invalid task result for ${this}: ${error.message}`), - state: {}, - }); - } - if (!result) { - return asOk(EMPTY_RUN_RESULT); - } - - return isFailedRunResult(result) ? asErr({ ...result, error: result.error }) : asOk(result); + return isFailedRunResult(result) + ? asErr({ ...result, error: result.error }) + : asOk(result || EMPTY_RUN_RESULT); } private shouldTryToScheduleRetry(): boolean { 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 e1d6ef17f5f9..bd532c38725d 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 @@ -6,7 +6,7 @@ import { get } from 'lodash'; import { RunContext, TaskDefinition } from './task'; -import { sanitizeTaskDefinitions } from './task_type_dictionary'; +import { sanitizeTaskDefinitions, TaskDefinitionRegistry } from './task_type_dictionary'; interface Opts { numTasks: number; @@ -73,8 +73,9 @@ describe('taskTypeDictionary', () => { it('throws a validation exception for invalid task definition', () => { const runsanitize = () => { - const taskDefinitions = { + const taskDefinitions: TaskDefinitionRegistry = { some_kind_of_task: { + // @ts-ignore fail: 'extremely', // cause a validation failure type: 'breaky_task', title: 'Test XYZ', @@ -94,6 +95,62 @@ describe('taskTypeDictionary', () => { return sanitizeTaskDefinitions(taskDefinitions); }; - expect(runsanitize).toThrowError(); + expect(runsanitize).toThrowErrorMatchingInlineSnapshot( + `"[fail]: definition for this key is missing"` + ); + }); + + it('throws a validation exception for invalid timeout on task definition', () => { + const runsanitize = () => { + const taskDefinitions: TaskDefinitionRegistry = { + some_kind_of_task: { + title: 'Test XYZ', + timeout: '15 days', + description: `Actually this won't work`, + createTaskRunner() { + return { + async run() { + return { + state: {}, + }; + }, + }; + }, + }, + }; + + return sanitizeTaskDefinitions(taskDefinitions); + }; + + expect(runsanitize).toThrowErrorMatchingInlineSnapshot( + `"Invalid timeout \\"15 days\\". Timeout must be of the form \\"{number}{cadance}\\" where number is an integer. Example: 5m."` + ); + }); + + it('throws a validation exception for invalid floating point timeout on task definition', () => { + const runsanitize = () => { + const taskDefinitions: TaskDefinitionRegistry = { + some_kind_of_task: { + title: 'Test XYZ', + timeout: '1.5h', + description: `Actually this won't work`, + createTaskRunner() { + return { + async run() { + return { + state: {}, + }; + }, + }; + }, + }, + }; + + return sanitizeTaskDefinitions(taskDefinitions); + }; + + expect(runsanitize).toThrowErrorMatchingInlineSnapshot( + `"Invalid timeout \\"1.5h\\". Timeout must be of the form \\"{number}{cadance}\\" where number is an integer. Example: 5m."` + ); }); }); 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 451b5dd7cad5..c66b117bde88 100644 --- a/x-pack/plugins/task_manager/server/task_type_dictionary.ts +++ b/x-pack/plugins/task_manager/server/task_type_dictionary.ts @@ -3,23 +3,13 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -import Joi from 'joi'; -import { TaskDefinition, validateTaskDefinition } from './task'; +import { TaskDefinition, taskDefinitionSchema } from './task'; import { Logger } from '../../../../src/core/server'; -/* - * The TaskManager is the public interface into the task manager system. This glues together - * all of the disparate modules in one integration point. The task manager operates in two different ways: - * - * - pre-init, it allows middleware registration, but disallows task manipulation - * - post-init, it disallows middleware registration, but allows task manipulation - * - * Due to its complexity, this is mostly tested by integration tests (see readme). - */ - -/** - * The public interface into the task manager system. - */ +export type TaskDefinitionRegistry = Record< + string, + Omit & Pick, 'timeout'> +>; export class TaskTypeDictionary { private definitions = new Map(); private logger: Logger; @@ -57,7 +47,7 @@ export class TaskTypeDictionary { * Method for allowing consumers to register task definitions into the system. * @param taskDefinitions - The Kibana task definitions dictionary */ - public registerTaskDefinitions(taskDefinitions: Record>) { + public registerTaskDefinitions(taskDefinitions: TaskDefinitionRegistry) { const duplicate = Object.keys(taskDefinitions).find((type) => this.definitions.has(type)); if (duplicate) { throw new Error(`Task ${duplicate} is already defined!`); @@ -79,10 +69,8 @@ export class TaskTypeDictionary { * * @param taskDefinitions - The Kibana task definitions dictionary */ -export function sanitizeTaskDefinitions( - taskDefinitions: Record> -): TaskDefinition[] { - return Object.entries(taskDefinitions).map(([type, rawDefinition]) => - Joi.attempt({ type, ...rawDefinition }, validateTaskDefinition) - ); +export function sanitizeTaskDefinitions(taskDefinitions: TaskDefinitionRegistry): TaskDefinition[] { + return Object.entries(taskDefinitions).map(([type, rawDefinition]) => { + return taskDefinitionSchema.validate({ type, ...rawDefinition }) as TaskDefinition; + }); }