[Ingest Manager] Create default Error handler. Use on /setup & EPM ro…

…utes (elastic#74409) (elastic#76754) * 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
pgayvallet · Sep 5, 2020 · b4309d6 · b4309d6
1 parent 0eea810
commit b4309d6
Show file tree

Hide file tree

Showing 4 changed files with 272 additions and 82 deletions.
diff --git a/x-pack/plugins/ingest_manager/server/errors.test.ts 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<typeof createAppContextStartContractMock>;
+  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
@@ -5,14 +5,34 @@
  */
 
 /* 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<IKibanaResponse>;
+
+interface IngestErrorHandlerParams {
+  error: IngestManagerError | Boom | Error;
+  response: KibanaResponseFactory;
+  request?: KibanaRequest;
+  context?: RequestHandlerContext;
+}
+
 export class IngestManagerError extends Error {
   constructor(message?: string) {
     super(message);
     this.name = this.constructor.name; // for stack traces
   }
 }
 
-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<IKibanaResponse> => {
+  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
@@ -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<TypeOf<typeof GetFileRequestSchema.p
       customResponseObj.headers = { 'Content-Type': contentType };
     }
     return response.custom(customResponseObj);
-  } catch (e) {
-    return response.customError({
-      statusCode: 500,
-      body: { message: e.message },
-    });
+  } catch (error) {
+    return defaultIngestErrorHandler({ error, response });
   }
 };
 
@@ -135,11 +123,8 @@ export const getInfoHandler: RequestHandler<TypeOf<typeof GetInfoRequestSchema.p
       response: res,
     };
     return response.ok({ body });
-  } catch (e) {
-    return response.customError({
-      statusCode: 500,
-      body: { message: e.message },
-    });
+  } catch (error) {
+    return defaultIngestErrorHandler({ error, response });
   }
 };
 
@@ -165,14 +150,12 @@ export const installPackageHandler: RequestHandler<
     };
     return response.ok({ body });
   } catch (e) {
+    // could have also done `return defaultIngestErrorHandler({ error: e, response })` at each of the returns,
+    // but doing it this way will log the outer/install errors before any inner/rollback errors
+    const defaultResult = await defaultIngestErrorHandler({ error: e, response });
     if (e instanceof IngestManagerError) {
-      logger.error(e);
-      return response.customError({
-        statusCode: getHTTPResponseCode(e),
-        body: { message: e.message },
-      });
+      return defaultResult;
     }
-
     // if there is an unknown server error, uninstall any package assets
     try {
       const installedPkg = await getInstallationObject({ savedObjectsClient, pkgName });
@@ -183,11 +166,7 @@ export const installPackageHandler: RequestHandler<
     } catch (error) {
       logger.error(`could not remove failed installation ${error}`);
     }
-    logger.error(e);
-    return response.customError({
-      statusCode: 500,
-      body: { message: e.message },
-    });
+    return defaultResult;
   }
 };
 
@@ -203,16 +182,7 @@ export const deletePackageHandler: RequestHandler<TypeOf<
       response: res,
     };
     return response.ok({ body });
-  } catch (e) {
-    if (e.isBoom) {
-      return response.customError({
-        statusCode: e.output.statusCode,
-        body: { message: e.output.payload.message },
-      });
-    }
-    return response.customError({
-      statusCode: 500,
-      body: { message: e.message },
-    });
+  } catch (error) {
+    return defaultIngestErrorHandler({ error, response });
   }
 };