From ed0f4b9714afda0b74017c8b07e22f6a68d9c9e3 Mon Sep 17 00:00:00 2001 From: John Schulz Date: Fri, 4 Sep 2020 07:33:58 -0400 Subject: [PATCH] [Ingest Manager] Create default Error handler. Use on /setup & EPM routes (#74409) * Add default Error handler. Try on /*setup & /epm/* * Add return type. Rename interface * Export error handler type & add comments * use default error handler in installPackageHandler * (re)-add comment --- .../ingest_manager/server/errors.test.ts | 191 ++++++++++++++++++ .../plugins/ingest_manager/server/errors.ts | 56 ++++- .../server/routes/epm/handlers.ts | 66 ++---- .../server/routes/setup/handlers.ts | 41 +--- 4 files changed, 272 insertions(+), 82 deletions(-) create mode 100644 x-pack/plugins/ingest_manager/server/errors.test.ts diff --git a/x-pack/plugins/ingest_manager/server/errors.test.ts b/x-pack/plugins/ingest_manager/server/errors.test.ts new file mode 100644 index 0000000000000..70e3a3b4150ad --- /dev/null +++ b/x-pack/plugins/ingest_manager/server/errors.test.ts @@ -0,0 +1,191 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import Boom from 'boom'; +import { httpServerMock } from 'src/core/server/mocks'; +import { createAppContextStartContractMock } from './mocks'; + +import { + IngestManagerError, + RegistryError, + PackageNotFoundError, + defaultIngestErrorHandler, +} from './errors'; +import { appContextService } from './services'; + +describe('defaultIngestErrorHandler', () => { + let mockContract: ReturnType; + beforeEach(async () => { + // prevents `Logger not set.` and other appContext errors + mockContract = createAppContextStartContractMock(); + appContextService.start(mockContract); + }); + + afterEach(async () => { + jest.clearAllMocks(); + appContextService.stop(); + }); + + describe('IngestManagerError', () => { + it('502: RegistryError', async () => { + const error = new RegistryError('xyz'); + const response = httpServerMock.createResponseFactory(); + + await defaultIngestErrorHandler({ error, response }); + + // response + expect(response.ok).toHaveBeenCalledTimes(0); + expect(response.customError).toHaveBeenCalledTimes(1); + expect(response.customError).toHaveBeenCalledWith({ + statusCode: 502, + body: { message: error.message }, + }); + + // logging + expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); + expect(mockContract.logger?.error).toHaveBeenCalledWith(error.message); + }); + + it('404: PackageNotFoundError', async () => { + const error = new PackageNotFoundError('123'); + const response = httpServerMock.createResponseFactory(); + + await defaultIngestErrorHandler({ error, response }); + + // response + expect(response.ok).toHaveBeenCalledTimes(0); + expect(response.customError).toHaveBeenCalledTimes(1); + expect(response.customError).toHaveBeenCalledWith({ + statusCode: 404, + body: { message: error.message }, + }); + + // logging + expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); + expect(mockContract.logger?.error).toHaveBeenCalledWith(error.message); + }); + + it('400: IngestManagerError', async () => { + const error = new IngestManagerError('123'); + const response = httpServerMock.createResponseFactory(); + + await defaultIngestErrorHandler({ error, response }); + + // response + expect(response.ok).toHaveBeenCalledTimes(0); + expect(response.customError).toHaveBeenCalledTimes(1); + expect(response.customError).toHaveBeenCalledWith({ + statusCode: 400, + body: { message: error.message }, + }); + + // logging + expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); + expect(mockContract.logger?.error).toHaveBeenCalledWith(error.message); + }); + }); + + describe('Boom', () => { + it('500: constructor - one arg', async () => { + const error = new Boom('bam'); + const response = httpServerMock.createResponseFactory(); + + await defaultIngestErrorHandler({ error, response }); + + // response + expect(response.ok).toHaveBeenCalledTimes(0); + expect(response.customError).toHaveBeenCalledTimes(1); + expect(response.customError).toHaveBeenCalledWith({ + statusCode: 500, + body: { message: 'An internal server error occurred' }, + }); + + // logging + expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); + expect(mockContract.logger?.error).toHaveBeenCalledWith('An internal server error occurred'); + }); + + it('custom: constructor - 2 args', async () => { + const error = new Boom('Problem doing something', { + statusCode: 456, + }); + const response = httpServerMock.createResponseFactory(); + + await defaultIngestErrorHandler({ error, response }); + + // response + expect(response.ok).toHaveBeenCalledTimes(0); + expect(response.customError).toHaveBeenCalledTimes(1); + expect(response.customError).toHaveBeenCalledWith({ + statusCode: 456, + body: { message: error.message }, + }); + + // logging + expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); + expect(mockContract.logger?.error).toHaveBeenCalledWith('Problem doing something'); + }); + + it('400: Boom.badRequest', async () => { + const error = Boom.badRequest('nope'); + const response = httpServerMock.createResponseFactory(); + + await defaultIngestErrorHandler({ error, response }); + + // response + expect(response.ok).toHaveBeenCalledTimes(0); + expect(response.customError).toHaveBeenCalledTimes(1); + expect(response.customError).toHaveBeenCalledWith({ + statusCode: 400, + body: { message: error.message }, + }); + + // logging + expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); + expect(mockContract.logger?.error).toHaveBeenCalledWith('nope'); + }); + + it('404: Boom.notFound', async () => { + const error = Boom.notFound('sorry'); + const response = httpServerMock.createResponseFactory(); + + await defaultIngestErrorHandler({ error, response }); + + // response + expect(response.ok).toHaveBeenCalledTimes(0); + expect(response.customError).toHaveBeenCalledTimes(1); + expect(response.customError).toHaveBeenCalledWith({ + statusCode: 404, + body: { message: error.message }, + }); + + // logging + expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); + expect(mockContract.logger?.error).toHaveBeenCalledWith('sorry'); + }); + }); + + describe('all other errors', () => { + it('500', async () => { + const error = new Error('something'); + const response = httpServerMock.createResponseFactory(); + + await defaultIngestErrorHandler({ error, response }); + + // response + expect(response.ok).toHaveBeenCalledTimes(0); + expect(response.customError).toHaveBeenCalledTimes(1); + expect(response.customError).toHaveBeenCalledWith({ + statusCode: 500, + body: { message: error.message }, + }); + + // logging + expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); + expect(mockContract.logger?.error).toHaveBeenCalledWith(error); + }); + }); +}); diff --git a/x-pack/plugins/ingest_manager/server/errors.ts b/x-pack/plugins/ingest_manager/server/errors.ts index e6ef4a51284b0..9829a4de23d7b 100644 --- a/x-pack/plugins/ingest_manager/server/errors.ts +++ b/x-pack/plugins/ingest_manager/server/errors.ts @@ -5,6 +5,26 @@ */ /* eslint-disable max-classes-per-file */ +import Boom, { isBoom } from 'boom'; +import { + RequestHandlerContext, + KibanaRequest, + IKibanaResponse, + KibanaResponseFactory, +} from 'src/core/server'; +import { appContextService } from './services'; + +type IngestErrorHandler = ( + params: IngestErrorHandlerParams +) => IKibanaResponse | Promise; + +interface IngestErrorHandlerParams { + error: IngestManagerError | Boom | Error; + response: KibanaResponseFactory; + request?: KibanaRequest; + context?: RequestHandlerContext; +} + export class IngestManagerError extends Error { constructor(message?: string) { super(message); @@ -12,7 +32,7 @@ export class IngestManagerError extends Error { } } -export const getHTTPResponseCode = (error: IngestManagerError): number => { +const getHTTPResponseCode = (error: IngestManagerError): number => { if (error instanceof RegistryError) { return 502; // Bad Gateway } @@ -23,6 +43,40 @@ export const getHTTPResponseCode = (error: IngestManagerError): number => { return 400; // Bad Request }; +export const defaultIngestErrorHandler: IngestErrorHandler = async ({ + error, + response, +}: IngestErrorHandlerParams): Promise => { + const logger = appContextService.getLogger(); + + // our "expected" errors + if (error instanceof IngestManagerError) { + // only log the message + logger.error(error.message); + return response.customError({ + statusCode: getHTTPResponseCode(error), + body: { message: error.message }, + }); + } + + // handle any older Boom-based errors or the few places our app uses them + if (isBoom(error)) { + // only log the message + logger.error(error.output.payload.message); + return response.customError({ + statusCode: error.output.statusCode, + body: { message: error.output.payload.message }, + }); + } + + // not sure what type of error this is. log as much as possible + logger.error(error); + return response.customError({ + statusCode: 500, + body: { message: error.message }, + }); +}; + export class RegistryError extends IngestManagerError {} export class RegistryConnectionError extends RegistryError {} export class RegistryResponseError extends RegistryError {} diff --git a/x-pack/plugins/ingest_manager/server/routes/epm/handlers.ts b/x-pack/plugins/ingest_manager/server/routes/epm/handlers.ts index 6400d6e215f9b..6d7252ffec41a 100644 --- a/x-pack/plugins/ingest_manager/server/routes/epm/handlers.ts +++ b/x-pack/plugins/ingest_manager/server/routes/epm/handlers.ts @@ -32,7 +32,7 @@ import { getLimitedPackages, getInstallationObject, } from '../../services/epm/packages'; -import { IngestManagerError, getHTTPResponseCode } from '../../errors'; +import { IngestManagerError, defaultIngestErrorHandler } from '../../errors'; import { splitPkgKey } from '../../services/epm/registry'; export const getCategoriesHandler: RequestHandler< @@ -45,11 +45,8 @@ export const getCategoriesHandler: RequestHandler< response: res, }; return response.ok({ body }); - } catch (e) { - return response.customError({ - statusCode: 500, - body: { message: e.message }, - }); + } catch (error) { + return defaultIngestErrorHandler({ error, response }); } }; @@ -69,11 +66,8 @@ export const getListHandler: RequestHandler< return response.ok({ body, }); - } catch (e) { - return response.customError({ - statusCode: 500, - body: { message: e.message }, - }); + } catch (error) { + return defaultIngestErrorHandler({ error, response }); } }; @@ -87,11 +81,8 @@ export const getLimitedListHandler: RequestHandler = async (context, request, re return response.ok({ body, }); - } catch (e) { - return response.customError({ - statusCode: 500, - body: { message: e.message }, - }); + } catch (error) { + return defaultIngestErrorHandler({ error, response }); } }; @@ -112,11 +103,8 @@ export const getFileHandler: RequestHandler { const soClient = context.core.savedObjects.client; @@ -46,11 +46,8 @@ export const getFleetStatusHandler: RequestHandler = async (context, request, re return response.ok({ body, }); - } catch (e) { - return response.customError({ - statusCode: 500, - body: { message: e.message }, - }); + } catch (error) { + return defaultIngestErrorHandler({ error, response }); } }; @@ -70,44 +67,22 @@ export const createFleetSetupHandler: RequestHandler< return response.ok({ body: { isInitialized: true }, }); - } catch (e) { - return response.customError({ - statusCode: 500, - body: { message: e.message }, - }); + } catch (error) { + return defaultIngestErrorHandler({ error, response }); } }; export const ingestManagerSetupHandler: RequestHandler = async (context, request, response) => { const soClient = context.core.savedObjects.client; const callCluster = context.core.elasticsearch.legacy.client.callAsCurrentUser; - const logger = appContextService.getLogger(); + try { const body: PostIngestSetupResponse = { isInitialized: true }; await setupIngestManager(soClient, callCluster); return response.ok({ body, }); - } catch (e) { - if (e instanceof IngestManagerError) { - logger.error(e.message); - return response.customError({ - statusCode: getHTTPResponseCode(e), - body: { message: e.message }, - }); - } - if (e.isBoom) { - logger.error(e.output.payload.message); - return response.customError({ - statusCode: e.output.statusCode, - body: { message: e.output.payload.message }, - }); - } - logger.error(e.message); - logger.error(e.stack); - return response.customError({ - statusCode: 500, - body: { message: e.message }, - }); + } catch (error) { + return defaultIngestErrorHandler({ error, response }); } };