From 663bface8c59e8d23a205bbbbdc5c1018ebc49e8 Mon Sep 17 00:00:00 2001 From: John Schulz Date: Sat, 5 Sep 2020 16:44:58 -0400 Subject: [PATCH 1/6] WIP but handles ES transport.request errors ``` > curl --user elastic:changeme -X POST http://localhost:5601/api/ingest_manager/epm/packages/aws-0.2.7 -H 'kbn-xsrf: xyz' {"statusCode":400,"error":"Bad Request","message":"[parse_exception] Failed to parse content to map from ES at /_ingest/pipeline/logs-aws.cloudtrail-0.2.7: {\"error\":{\"root_cause\":[{\"type\":\"parse_exception\",\"reason\":\"Failed to parse content to map\"}],\"type\":\"parse_exception\",\"reason\":\"Failed to parse content to map\",\"caused_by\":{\"type\":\"json_parse_exception\",\"reason\":\"Duplicate field 'ListGroupsForUser'\\n at [Source: (byte[])\\\"---\\ndescription: Pipeline for AWS CloudTrail Logs\\nprocessors:\\n - set:\\n field: event.ingested\\n value: '{{_ingest.timestamp}}'\\n - rename:\\n field: \\\"message\\\"\\n target_field: \\\"event.original\\\"\\n - json:\\n field: \\\"event.original\\\"\\n target_field: \\\"json\\\"\\n - date:\\n field: \\\"json.eventTime\\\"\\n target_field: \\\"@timestamp\\\"\\n ignore_failure: true\\n formats:\\n - ISO8601\\n - rename:\\n field: \\\"json.eventVersion\\\"\\n target_field: \\\"aws.cloudtrail.event_versi\\\"[truncated 16425 bytes]; line: 489, column: 26]\"}},\"status\":400}"} ``` --- .../plugins/ingest_manager/server/errors.ts | 40 +++++++++++++++++++ .../elasticsearch/ingest_pipeline/install.ts | 5 ++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/ingest_manager/server/errors.ts b/x-pack/plugins/ingest_manager/server/errors.ts index 9829a4de23d7b..60356bfa5fb7c 100644 --- a/x-pack/plugins/ingest_manager/server/errors.ts +++ b/x-pack/plugins/ingest_manager/server/errors.ts @@ -25,6 +25,35 @@ interface IngestErrorHandlerParams { context?: RequestHandlerContext; } +// unsure if this is correct. would prefer to use something "official" +// this type & guard are based on values observed while debugging https://github.com/elastic/kibana/issues/75862 +interface LegacyESClientError { + statusCode: number; + message: string; + body: { + error: { + root_cause: [{ type: string; reason: string }]; + type: string; + caused_by: { type: string; reason: string }; + }; + status: number; + }; + path: string; + query: string | undefined; + response: string; +} + +export const isLegacyESClientError = (error: any): error is LegacyESClientError => { + return ( + 'statusCode' in error && + 'message' in error && + 'body' in error && + 'path' in error && + 'query' in error && + 'response' in error + ); +}; + export class IngestManagerError extends Error { constructor(message?: string) { super(message); @@ -48,6 +77,17 @@ export const defaultIngestErrorHandler: IngestErrorHandler = async ({ response, }: IngestErrorHandlerParams): Promise => { const logger = appContextService.getLogger(); + if (isLegacyESClientError(error)) { + // there was a problem communicating with ES (e.g. via `callCluster`) + // log full error + logger.error(error); + // return the endpoint that failed and the body it returned + const message = `${error.message} from ES at ${error.path}: ${error.response}`; + return response.customError({ + statusCode: error.statusCode, + body: { message }, + }); + } // our "expected" errors if (error instanceof IngestManagerError) { diff --git a/x-pack/plugins/ingest_manager/server/services/epm/elasticsearch/ingest_pipeline/install.ts b/x-pack/plugins/ingest_manager/server/services/epm/elasticsearch/ingest_pipeline/install.ts index 44e4eddfbbe6a..1ec0898ad7dd0 100644 --- a/x-pack/plugins/ingest_manager/server/services/epm/elasticsearch/ingest_pipeline/install.ts +++ b/x-pack/plugins/ingest_manager/server/services/epm/elasticsearch/ingest_pipeline/install.ts @@ -156,7 +156,10 @@ async function installPipeline({ body: pipeline.contentForInstallation, }; if (pipeline.extension === 'yml') { - callClusterParams.headers = { ['Content-Type']: 'application/yaml' }; + callClusterParams.headers = { + 'Content-Type': 'application/yaml', + Accept: 'application/json', + }; } // This uses the catch-all endpoint 'transport.request' because we have to explicitly From 8e260475b3e594f44b778056f43aec1c6c677350 Mon Sep 17 00:00:00 2001 From: John Schulz Date: Mon, 7 Sep 2020 07:07:16 -0400 Subject: [PATCH 2/6] Use code from legacy repo for type guard --- .../handlers.test.ts} | 7 ++-- .../server/{errors.ts => errors/handlers.ts} | 37 ++++--------------- .../ingest_manager/server/errors/index.ts | 20 ++++++++++ 3 files changed, 31 insertions(+), 33 deletions(-) rename x-pack/plugins/ingest_manager/server/{errors.test.ts => errors/handlers.test.ts} (97%) rename x-pack/plugins/ingest_manager/server/{errors.ts => errors/handlers.ts} (73%) create mode 100644 x-pack/plugins/ingest_manager/server/errors/index.ts diff --git a/x-pack/plugins/ingest_manager/server/errors.test.ts b/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts similarity index 97% rename from x-pack/plugins/ingest_manager/server/errors.test.ts rename to x-pack/plugins/ingest_manager/server/errors/handlers.test.ts index 70e3a3b4150ad..9da64556da081 100644 --- a/x-pack/plugins/ingest_manager/server/errors.test.ts +++ b/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts @@ -6,15 +6,14 @@ import Boom from 'boom'; import { httpServerMock } from 'src/core/server/mocks'; -import { createAppContextStartContractMock } from './mocks'; - +import { createAppContextStartContractMock } from '../mocks'; +import { appContextService } from '../services'; import { IngestManagerError, RegistryError, PackageNotFoundError, defaultIngestErrorHandler, -} from './errors'; -import { appContextService } from './services'; +} from './index'; describe('defaultIngestErrorHandler', () => { let mockContract: ReturnType; diff --git a/x-pack/plugins/ingest_manager/server/errors.ts b/x-pack/plugins/ingest_manager/server/errors/handlers.ts similarity index 73% rename from x-pack/plugins/ingest_manager/server/errors.ts rename to x-pack/plugins/ingest_manager/server/errors/handlers.ts index 60356bfa5fb7c..95e9aae55367f 100644 --- a/x-pack/plugins/ingest_manager/server/errors.ts +++ b/x-pack/plugins/ingest_manager/server/errors/handlers.ts @@ -4,7 +4,6 @@ * you may not use this file except in compliance with the Elastic License. */ -/* eslint-disable max-classes-per-file */ import Boom, { isBoom } from 'boom'; import { RequestHandlerContext, @@ -12,21 +11,21 @@ import { IKibanaResponse, KibanaResponseFactory, } from 'src/core/server'; -import { appContextService } from './services'; +import { errors as LegacyESErrors } from 'elasticsearch'; +import { appContextService } from '../services'; +import { IngestManagerError, RegistryError, PackageNotFoundError } from './index'; type IngestErrorHandler = ( params: IngestErrorHandlerParams ) => IKibanaResponse | Promise; - interface IngestErrorHandlerParams { error: IngestManagerError | Boom | Error; response: KibanaResponseFactory; request?: KibanaRequest; context?: RequestHandlerContext; } - // unsure if this is correct. would prefer to use something "official" -// this type & guard are based on values observed while debugging https://github.com/elastic/kibana/issues/75862 +// this type is based on BadRequest values observed while debugging https://github.com/elastic/kibana/issues/75862 interface LegacyESClientError { statusCode: number; message: string; @@ -44,23 +43,9 @@ interface LegacyESClientError { } export const isLegacyESClientError = (error: any): error is LegacyESClientError => { - return ( - 'statusCode' in error && - 'message' in error && - 'body' in error && - 'path' in error && - 'query' in error && - 'response' in error - ); + return error instanceof LegacyESErrors._Abstract; }; -export class IngestManagerError extends Error { - constructor(message?: string) { - super(message); - this.name = this.constructor.name; // for stack traces - } -} - const getHTTPResponseCode = (error: IngestManagerError): number => { if (error instanceof RegistryError) { return 502; // Bad Gateway @@ -79,10 +64,10 @@ export const defaultIngestErrorHandler: IngestErrorHandler = async ({ const logger = appContextService.getLogger(); if (isLegacyESClientError(error)) { // there was a problem communicating with ES (e.g. via `callCluster`) - // log full error - logger.error(error); + // only log the message + logger.error(error.message); // return the endpoint that failed and the body it returned - const message = `${error.message} from ES at ${error.path}: ${error.response}`; + const message = `${error.message} response from ${error.path}: ${error.response}`; return response.customError({ statusCode: error.statusCode, body: { message }, @@ -116,9 +101,3 @@ export const defaultIngestErrorHandler: IngestErrorHandler = async ({ body: { message: error.message }, }); }; - -export class RegistryError extends IngestManagerError {} -export class RegistryConnectionError extends RegistryError {} -export class RegistryResponseError extends RegistryError {} -export class PackageNotFoundError extends IngestManagerError {} -export class PackageOutdatedError extends IngestManagerError {} diff --git a/x-pack/plugins/ingest_manager/server/errors/index.ts b/x-pack/plugins/ingest_manager/server/errors/index.ts new file mode 100644 index 0000000000000..5e36a2ec9a884 --- /dev/null +++ b/x-pack/plugins/ingest_manager/server/errors/index.ts @@ -0,0 +1,20 @@ +/* + * 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. + */ + +/* eslint-disable max-classes-per-file */ +export { defaultIngestErrorHandler } from './handlers'; + +export class IngestManagerError extends Error { + constructor(message?: string) { + super(message); + this.name = this.constructor.name; // for stack traces + } +} +export class RegistryError extends IngestManagerError {} +export class RegistryConnectionError extends RegistryError {} +export class RegistryResponseError extends RegistryError {} +export class PackageNotFoundError extends IngestManagerError {} +export class PackageOutdatedError extends IngestManagerError {} From 154f82bf483b2483f150bcb2e243959fbede100a Mon Sep 17 00:00:00 2001 From: John Schulz Date: Tue, 8 Sep 2020 15:32:39 -0400 Subject: [PATCH 3/6] Add tests for all 4xx & 5xx errors from Legacy ES MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``` use the HTTP error status code provided by LegacyESErrors ✓ 400 - with meta (21ms) ✓ 401 - with meta (8ms) ✓ 402 - with meta (18ms) ✓ 403 - with meta (7ms) ✓ 404 - with meta (5ms) ✓ 405 - with meta (4ms) ✓ 406 - with meta (4ms) ✓ 407 - with meta (11ms) ✓ 408 - with meta (14ms) ✓ 409 - with meta (7ms) ✓ 410 - with meta (4ms) ✓ 411 - with meta (4ms) ✓ 412 - with meta (4ms) ✓ 413 - with meta (4ms) ✓ 414 - with meta (4ms) ✓ 415 - with meta (4ms) ✓ 416 - with meta (7ms) ✓ 417 - with meta (5ms) ✓ 418 - with meta (5ms) ✓ 421 - with meta (4ms) ✓ 426 - with meta (4ms) ✓ 429 - with meta (13ms) ✓ 450 - with meta (4ms) ✓ 494 - with meta (5ms) ✓ 497 - with meta (5ms) ✓ 499 - with meta (6ms) ✓ 500 - with meta (7ms) ✓ 501 - with meta (6ms) ✓ 502 - with meta (6ms) ✓ 503 - with meta (7ms) ✓ 504 - with meta (5ms) ✓ 505 - with meta (5ms) ✓ 506 - with meta (6ms) ✓ 510 - with meta (6ms) ✓ 400 - without meta (5ms) ✓ 401 - without meta (12ms) ✓ 402 - without meta (5ms) ✓ 403 - without meta (7ms) ✓ 404 - without meta (7ms) ✓ 405 - without meta (6ms) ✓ 406 - without meta (6ms) ✓ 407 - without meta (5ms) ✓ 408 - without meta (6ms) ✓ 409 - without meta (5ms) ✓ 410 - without meta (6ms) ✓ 411 - without meta (5ms) ✓ 412 - without meta (6ms) ✓ 413 - without meta (5ms) ✓ 414 - without meta (11ms) ✓ 415 - without meta (6ms) ✓ 416 - without meta (7ms) ✓ 417 - without meta (5ms) ✓ 418 - without meta (3ms) ✓ 421 - without meta (4ms) ✓ 426 - without meta (77ms) ✓ 429 - without meta (13ms) ✓ 450 - without meta (3ms) ✓ 494 - without meta (2ms) ✓ 497 - without meta (2ms) ✓ 499 - without meta (2ms) ✓ 500 - without meta (2ms) ✓ 501 - without meta (8ms) ✓ 502 - without meta (2ms) ✓ 503 - without meta (2ms) ✓ 504 - without meta (3ms) ✓ 505 - without meta (2ms) ✓ 506 - without meta (2ms) ✓ 510 - without meta (2ms) ``` --- .../server/errors/handlers.test.ts | 65 +++++++++++++++++++ .../ingest_manager/server/errors/handlers.ts | 31 +++++---- .../elasticsearch/ingest_pipeline/install.ts | 2 + 3 files changed, 84 insertions(+), 14 deletions(-) diff --git a/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts b/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts index 9da64556da081..8d97be86d629a 100644 --- a/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts +++ b/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts @@ -5,6 +5,7 @@ */ import Boom from 'boom'; +import { errors } from 'elasticsearch'; import { httpServerMock } from 'src/core/server/mocks'; import { createAppContextStartContractMock } from '../mocks'; import { appContextService } from '../services'; @@ -15,6 +16,7 @@ import { defaultIngestErrorHandler, } from './index'; +const LegacyESErrors = errors as Record; describe('defaultIngestErrorHandler', () => { let mockContract: ReturnType; beforeEach(async () => { @@ -28,6 +30,69 @@ describe('defaultIngestErrorHandler', () => { appContextService.stop(); }); + describe('use the HTTP error status code provided by LegacyESErrors', () => { + const statusCodes = Object.keys(LegacyESErrors).filter((key) => /^\d+$/.test(key)); + const errorCodes = statusCodes.filter((key) => parseInt(key, 10) >= 400); + const testCasesWithMeta = errorCodes.map((errorCode) => [ + errorCode, + LegacyESErrors[errorCode], + 'the root message', + { + path: '/path/to/call', + response: 'response is here', + }, + ]); + + test.each(testCasesWithMeta)( + '%d - with meta', + async (statusCode, StatusCodeError, message, meta) => { + jest.clearAllMocks(); + const error = new StatusCodeError(message, meta); + const response = httpServerMock.createResponseFactory(); + await defaultIngestErrorHandler({ error, response }); + + // response + expect(response.ok).toHaveBeenCalledTimes(0); + expect(response.customError).toHaveBeenCalledTimes(1); + expect(response.customError).toHaveBeenCalledWith({ + statusCode: error.status, + body: { message: `${error.message} response from ${error.path}: ${error.response}` }, + }); + + // logging + expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); + expect(mockContract.logger?.error).toHaveBeenCalledWith(error.message); + } + ); + + const testCasesWithoutMeta = errorCodes.map((errorCode) => [ + errorCode, + LegacyESErrors[errorCode], + 'the root message', + ]); + test.each(testCasesWithoutMeta)( + '%d - without meta', + async (statusCode, StatusCodeError, message) => { + jest.clearAllMocks(); + const error = new StatusCodeError(message); + const response = httpServerMock.createResponseFactory(); + await defaultIngestErrorHandler({ error, response }); + + // response + expect(response.ok).toHaveBeenCalledTimes(0); + expect(response.customError).toHaveBeenCalledTimes(1); + expect(response.customError).toHaveBeenCalledWith({ + statusCode: error.status, + body: { message: error.message }, + }); + + // logging + expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); + expect(mockContract.logger?.error).toHaveBeenCalledWith(error.message); + } + ); + }); + describe('IngestManagerError', () => { it('502: RegistryError', async () => { const error = new RegistryError('xyz'); diff --git a/x-pack/plugins/ingest_manager/server/errors/handlers.ts b/x-pack/plugins/ingest_manager/server/errors/handlers.ts index 95e9aae55367f..f8fc35d5fd867 100644 --- a/x-pack/plugins/ingest_manager/server/errors/handlers.ts +++ b/x-pack/plugins/ingest_manager/server/errors/handlers.ts @@ -26,22 +26,21 @@ interface IngestErrorHandlerParams { } // unsure if this is correct. would prefer to use something "official" // this type is based on BadRequest values observed while debugging https://github.com/elastic/kibana/issues/75862 + interface LegacyESClientError { - statusCode: number; message: string; - body: { - error: { - root_cause: [{ type: string; reason: string }]; - type: string; - caused_by: { type: string; reason: string }; - }; + stack: string; + status: number; + displayName: string; + path?: string; + query?: string | undefined; + body?: { + error: object; status: number; }; - path: string; - query: string | undefined; - response: string; + statusCode?: number; + response?: string; } - export const isLegacyESClientError = (error: any): error is LegacyESClientError => { return error instanceof LegacyESErrors._Abstract; }; @@ -66,10 +65,14 @@ export const defaultIngestErrorHandler: IngestErrorHandler = async ({ // there was a problem communicating with ES (e.g. via `callCluster`) // only log the message logger.error(error.message); - // return the endpoint that failed and the body it returned - const message = `${error.message} response from ${error.path}: ${error.response}`; + + const message = + error?.path && error?.response + ? // if possible, return the failing endpoint and its response + `${error.message} response from ${error.path}: ${error.response}` + : error.message; return response.customError({ - statusCode: error.statusCode, + statusCode: error?.statusCode || error.status, body: { message }, }); } diff --git a/x-pack/plugins/ingest_manager/server/services/epm/elasticsearch/ingest_pipeline/install.ts b/x-pack/plugins/ingest_manager/server/services/epm/elasticsearch/ingest_pipeline/install.ts index 1ec0898ad7dd0..878c6ea8f2804 100644 --- a/x-pack/plugins/ingest_manager/server/services/epm/elasticsearch/ingest_pipeline/install.ts +++ b/x-pack/plugins/ingest_manager/server/services/epm/elasticsearch/ingest_pipeline/install.ts @@ -157,7 +157,9 @@ async function installPipeline({ }; if (pipeline.extension === 'yml') { callClusterParams.headers = { + // pipeline is YAML 'Content-Type': 'application/yaml', + // but we want JSON responses (to extract error messages, status code, or other metadata) Accept: 'application/json', }; } From 3b454a683e9367032a4e4ed76d5ee3df8885d40a Mon Sep 17 00:00:00 2001 From: John Schulz Date: Tue, 8 Sep 2020 20:09:03 -0400 Subject: [PATCH 4/6] Have all test cases use the same function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``` use the HTTP error status code provided by LegacyESErrors ✓ 400 - with path & response (11ms) ✓ 401 - with path & response (7ms) ✓ 402 - with path & response (5ms) ✓ 403 - with path & response (7ms) ✓ 404 - with path & response (7ms) ✓ 405 - with path & response (20ms) ✓ 406 - with path & response (2ms) ✓ 407 - with path & response (3ms) ✓ 408 - with path & response (6ms) ✓ 409 - with path & response (8ms) ✓ 410 - with path & response (1ms) ✓ 411 - with path & response (1ms) ✓ 412 - with path & response (1ms) ✓ 413 - with path & response (1ms) ✓ 414 - with path & response (1ms) ✓ 415 - with path & response (1ms) ✓ 416 - with path & response (2ms) ✓ 417 - with path & response (13ms) ✓ 418 - with path & response (1ms) ✓ 421 - with path & response (1ms) ✓ 426 - with path & response (1ms) ✓ 429 - with path & response (2ms) ✓ 450 - with path & response (1ms) ✓ 494 - with path & response (1ms) ✓ 497 - with path & response (1ms) ✓ 499 - with path & response (1ms) ✓ 500 - with path & response (2ms) ✓ 501 - with path & response (1ms) ✓ 502 - with path & response (1ms) ✓ 503 - with path & response (1ms) ✓ 504 - with path & response (1ms) ✓ 505 - with path & response (10ms) ✓ 506 - with path & response (1ms) ✓ 510 - with path & response (2ms) ✓ 400 - with other metadata (1ms) ✓ 401 - with other metadata (1ms) ✓ 402 - with other metadata (1ms) ✓ 403 - with other metadata (1ms) ✓ 404 - with other metadata (1ms) ✓ 405 - with other metadata (1ms) ✓ 406 - with other metadata (1ms) ✓ 407 - with other metadata (1ms) ✓ 408 - with other metadata (2ms) ✓ 409 - with other metadata (1ms) ✓ 410 - with other metadata (10ms) ✓ 411 - with other metadata (1ms) ✓ 412 - with other metadata (1ms) ✓ 413 - with other metadata (1ms) ✓ 414 - with other metadata (1ms) ✓ 415 - with other metadata (1ms) ✓ 416 - with other metadata (10ms) ✓ 417 - with other metadata (1ms) ✓ 418 - with other metadata (1ms) ✓ 421 - with other metadata (1ms) ✓ 426 - with other metadata (1ms) ✓ 429 - with other metadata (1ms) ✓ 450 - with other metadata (1ms) ✓ 494 - with other metadata (11ms) ✓ 497 - with other metadata (2ms) ✓ 499 - with other metadata (1ms) ✓ 500 - with other metadata (1ms) ✓ 501 - with other metadata (1ms) ✓ 502 - with other metadata (1ms) ✓ 503 - with other metadata (1ms) ✓ 504 - with other metadata (2ms) ✓ 505 - with other metadata (1ms) ✓ 506 - with other metadata (1ms) ✓ 510 - with other metadata (1ms) ✓ 400 - without metadata (1ms) ✓ 401 - without metadata (1ms) ✓ 402 - without metadata (13ms) ✓ 403 - without metadata (1ms) ✓ 404 - without metadata (1ms) ✓ 405 - without metadata (2ms) ✓ 406 - without metadata (1ms) ✓ 407 - without metadata (1ms) ✓ 408 - without metadata (1ms) ✓ 409 - without metadata (1ms) ✓ 410 - without metadata (1ms) ✓ 411 - without metadata (1ms) ✓ 412 - without metadata (1ms) ✓ 413 - without metadata (1ms) ✓ 414 - without metadata (1ms) ✓ 415 - without metadata (2ms) ✓ 416 - without metadata (10ms) ✓ 417 - without metadata (2ms) ✓ 418 - without metadata (1ms) ✓ 421 - without metadata (1ms) ✓ 426 - without metadata (1ms) ✓ 429 - without metadata (1ms) ✓ 450 - without metadata (1ms) ✓ 494 - without metadata (1ms) ✓ 497 - without metadata (2ms) ✓ 499 - without metadata (1ms) ✓ 500 - without metadata (1ms) ✓ 501 - without metadata (1ms) ✓ 502 - without metadata (1ms) ✓ 503 - without metadata (11ms) ✓ 504 - without metadata (1ms) ✓ 505 - without metadata (1ms) ✓ 506 - without metadata (3ms) ✓ 510 - without metadata (1ms) ``` --- .../server/errors/handlers.test.ts | 87 ++++++++----------- 1 file changed, 36 insertions(+), 51 deletions(-) diff --git a/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts b/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts index 8d97be86d629a..3fc538e4b28cb 100644 --- a/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts +++ b/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts @@ -30,67 +30,52 @@ describe('defaultIngestErrorHandler', () => { appContextService.stop(); }); + async function testEsErrorsFn(errorCode: string, error: any, bodyMessage: string) { + jest.clearAllMocks(); + const response = httpServerMock.createResponseFactory(); + await defaultIngestErrorHandler({ error, response }); + + // response + expect(response.ok).toHaveBeenCalledTimes(0); + expect(response.customError).toHaveBeenCalledTimes(1); + expect(response.customError).toHaveBeenCalledWith({ + statusCode: error.status, + body: { message: bodyMessage }, + }); + + // logging + expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); + expect(mockContract.logger?.error).toHaveBeenCalledWith(error.message); + } + describe('use the HTTP error status code provided by LegacyESErrors', () => { const statusCodes = Object.keys(LegacyESErrors).filter((key) => /^\d+$/.test(key)); const errorCodes = statusCodes.filter((key) => parseInt(key, 10) >= 400); - const testCasesWithMeta = errorCodes.map((errorCode) => [ + const testCasesWithPathResponse = errorCodes.map((errorCode) => [ errorCode, - LegacyESErrors[errorCode], - 'the root message', - { + new LegacyESErrors[errorCode]('the root message', { path: '/path/to/call', response: 'response is here', - }, + }), + 'the root message response from /path/to/call: response is here', ]); - - test.each(testCasesWithMeta)( - '%d - with meta', - async (statusCode, StatusCodeError, message, meta) => { - jest.clearAllMocks(); - const error = new StatusCodeError(message, meta); - const response = httpServerMock.createResponseFactory(); - await defaultIngestErrorHandler({ error, response }); - - // response - expect(response.ok).toHaveBeenCalledTimes(0); - expect(response.customError).toHaveBeenCalledTimes(1); - expect(response.customError).toHaveBeenCalledWith({ - statusCode: error.status, - body: { message: `${error.message} response from ${error.path}: ${error.response}` }, - }); - - // logging - expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); - expect(mockContract.logger?.error).toHaveBeenCalledWith(error.message); - } - ); - - const testCasesWithoutMeta = errorCodes.map((errorCode) => [ + const testCasesWithOtherMeta = errorCodes.map((errorCode) => [ errorCode, - LegacyESErrors[errorCode], + new LegacyESErrors[errorCode]('the root message', { + other: '/path/to/call', + props: 'response is here', + }), 'the root message', ]); - test.each(testCasesWithoutMeta)( - '%d - without meta', - async (statusCode, StatusCodeError, message) => { - jest.clearAllMocks(); - const error = new StatusCodeError(message); - const response = httpServerMock.createResponseFactory(); - await defaultIngestErrorHandler({ error, response }); - - // response - expect(response.ok).toHaveBeenCalledTimes(0); - expect(response.customError).toHaveBeenCalledTimes(1); - expect(response.customError).toHaveBeenCalledWith({ - statusCode: error.status, - body: { message: error.message }, - }); - - // logging - expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); - expect(mockContract.logger?.error).toHaveBeenCalledWith(error.message); - } - ); + const testCasesWithoutMeta = errorCodes.map((errorCode) => [ + errorCode, + new LegacyESErrors[errorCode]('some message'), + 'some message', + ]); + + test.each(testCasesWithPathResponse)('%d - with path & response', testEsErrorsFn); + test.each(testCasesWithOtherMeta)('%d - with other metadata', testEsErrorsFn); + test.each(testCasesWithoutMeta)('%d - without metadata', testEsErrorsFn); }); describe('IngestManagerError', () => { From 9a06d7b3dec9f8e101ff21674ea5eb9b7bdd1826 Mon Sep 17 00:00:00 2001 From: John Schulz Date: Tue, 8 Sep 2020 20:46:46 -0400 Subject: [PATCH 5/6] Add types to test function --- .../server/errors/handlers.test.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts b/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts index 3fc538e4b28cb..8632ecbbea002 100644 --- a/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts +++ b/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts @@ -17,6 +17,8 @@ import { } from './index'; const LegacyESErrors = errors as Record; +type ITestEsErrorsFnParams = [errorCode: string, error: any, bodyMessage: string]; + describe('defaultIngestErrorHandler', () => { let mockContract: ReturnType; beforeEach(async () => { @@ -30,7 +32,8 @@ describe('defaultIngestErrorHandler', () => { appContextService.stop(); }); - async function testEsErrorsFn(errorCode: string, error: any, bodyMessage: string) { + async function testEsErrorsFn(...args: ITestEsErrorsFnParams) { + const [, error, bodyMessage] = args; jest.clearAllMocks(); const response = httpServerMock.createResponseFactory(); await defaultIngestErrorHandler({ error, response }); @@ -51,7 +54,7 @@ describe('defaultIngestErrorHandler', () => { describe('use the HTTP error status code provided by LegacyESErrors', () => { const statusCodes = Object.keys(LegacyESErrors).filter((key) => /^\d+$/.test(key)); const errorCodes = statusCodes.filter((key) => parseInt(key, 10) >= 400); - const testCasesWithPathResponse = errorCodes.map((errorCode) => [ + const casesWithPathResponse: ITestEsErrorsFnParams[] = errorCodes.map((errorCode) => [ errorCode, new LegacyESErrors[errorCode]('the root message', { path: '/path/to/call', @@ -59,7 +62,7 @@ describe('defaultIngestErrorHandler', () => { }), 'the root message response from /path/to/call: response is here', ]); - const testCasesWithOtherMeta = errorCodes.map((errorCode) => [ + const casesWithOtherMeta: ITestEsErrorsFnParams[] = errorCodes.map((errorCode) => [ errorCode, new LegacyESErrors[errorCode]('the root message', { other: '/path/to/call', @@ -67,15 +70,15 @@ describe('defaultIngestErrorHandler', () => { }), 'the root message', ]); - const testCasesWithoutMeta = errorCodes.map((errorCode) => [ + const casesWithoutMeta: ITestEsErrorsFnParams[] = errorCodes.map((errorCode) => [ errorCode, new LegacyESErrors[errorCode]('some message'), 'some message', ]); - test.each(testCasesWithPathResponse)('%d - with path & response', testEsErrorsFn); - test.each(testCasesWithOtherMeta)('%d - with other metadata', testEsErrorsFn); - test.each(testCasesWithoutMeta)('%d - without metadata', testEsErrorsFn); + test.each(casesWithPathResponse)('%d - with path & response', testEsErrorsFn); + test.each(casesWithOtherMeta)('%d - with other metadata', testEsErrorsFn); + test.each(casesWithoutMeta)('%d - without metadata', testEsErrorsFn); }); describe('IngestManagerError', () => { From fdaf5883cb7646601b98cb844bdfc79860bd0042 Mon Sep 17 00:00:00 2001 From: John Schulz Date: Wed, 9 Sep 2020 11:41:18 -0400 Subject: [PATCH 6/6] Use same message for logs & HTTP response --- .../plugins/ingest_manager/server/errors/handlers.test.ts | 8 ++++---- x-pack/plugins/ingest_manager/server/errors/handlers.ts | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts b/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts index 8632ecbbea002..361386a86d547 100644 --- a/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts +++ b/x-pack/plugins/ingest_manager/server/errors/handlers.test.ts @@ -17,7 +17,7 @@ import { } from './index'; const LegacyESErrors = errors as Record; -type ITestEsErrorsFnParams = [errorCode: string, error: any, bodyMessage: string]; +type ITestEsErrorsFnParams = [errorCode: string, error: any, expectedMessage: string]; describe('defaultIngestErrorHandler', () => { let mockContract: ReturnType; @@ -33,7 +33,7 @@ describe('defaultIngestErrorHandler', () => { }); async function testEsErrorsFn(...args: ITestEsErrorsFnParams) { - const [, error, bodyMessage] = args; + const [, error, expectedMessage] = args; jest.clearAllMocks(); const response = httpServerMock.createResponseFactory(); await defaultIngestErrorHandler({ error, response }); @@ -43,12 +43,12 @@ describe('defaultIngestErrorHandler', () => { expect(response.customError).toHaveBeenCalledTimes(1); expect(response.customError).toHaveBeenCalledWith({ statusCode: error.status, - body: { message: bodyMessage }, + body: { message: expectedMessage }, }); // logging expect(mockContract.logger?.error).toHaveBeenCalledTimes(1); - expect(mockContract.logger?.error).toHaveBeenCalledWith(error.message); + expect(mockContract.logger?.error).toHaveBeenCalledWith(expectedMessage); } describe('use the HTTP error status code provided by LegacyESErrors', () => { diff --git a/x-pack/plugins/ingest_manager/server/errors/handlers.ts b/x-pack/plugins/ingest_manager/server/errors/handlers.ts index f8fc35d5fd867..9f776565cf262 100644 --- a/x-pack/plugins/ingest_manager/server/errors/handlers.ts +++ b/x-pack/plugins/ingest_manager/server/errors/handlers.ts @@ -64,13 +64,14 @@ export const defaultIngestErrorHandler: IngestErrorHandler = async ({ if (isLegacyESClientError(error)) { // there was a problem communicating with ES (e.g. via `callCluster`) // only log the message - logger.error(error.message); - const message = error?.path && error?.response ? // if possible, return the failing endpoint and its response `${error.message} response from ${error.path}: ${error.response}` : error.message; + + logger.error(message); + return response.customError({ statusCode: error?.statusCode || error.status, body: { message },