Skip to content

Commit 96ffe74

Browse files
author
John Schulz
committed
[Ingest Manager] Handle Legacy ES client errors (#76865)
## Summary closes #75862 1. Use an HTTP status code if ES client error provides one. extended #75862 (comment) to all 4xx-5xx errors 1. Format Error message as described in #75862 (comment) & agreed to #75862 (comment) ### example Request/Response: ``` > 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}" } ``` ### example Kibana Logs: `[parse_exception] Failed to parse content to map` <details><summary>Used `test.each` to generate tests for each 4xx - 5xx error. Call each error 3 different ways.</summary> ``` defaultIngestErrorHandler use the HTTP error status code provided by LegacyESErrors ✓ 400 - with path & response (12ms) ✓ 401 - with path & response (5ms) ✓ 402 - with path & response (5ms) ✓ 403 - with path & response (6ms) ✓ 404 - with path & response (5ms) ✓ 405 - with path & response (17ms) ✓ 406 - with path & response (2ms) ✓ 407 - with path & response (3ms) ✓ 408 - with path & response (6ms) ✓ 409 - with path & response (5ms) ✓ 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 (1ms) ✓ 417 - with path & response (9ms) ✓ 418 - with path & response (1ms) ✓ 421 - with path & response (1ms) ✓ 426 - with path & response (1ms) ✓ 429 - with path & response (1ms) ✓ 450 - with path & response (1ms) ✓ 494 - with path & response (1ms) ✓ 497 - with path & response (1ms) ✓ 499 - with path & response (3ms) ✓ 500 - with path & response (2ms) ✓ 501 - with path & response (1ms) ✓ 502 - with path & response (2ms) ✓ 503 - with path & response (1ms) ✓ 504 - with path & response (1ms) ✓ 505 - with path & response (8ms) ✓ 506 - with path & response (2ms) ✓ 510 - with path & response (1ms) ✓ 400 - with other metadata (1ms) ✓ 401 - with other metadata (1ms) ✓ 402 - with other metadata (1ms) ✓ 403 - with other metadata (1ms) ✓ 404 - with other metadata (2ms) ✓ 405 - with other metadata (1ms) ✓ 406 - with other metadata (2ms) ✓ 407 - with other metadata (1ms) ✓ 408 - with other metadata (1ms) ✓ 409 - with other metadata (1ms) ✓ 410 - with other metadata (10ms) ✓ 411 - with other metadata (2ms) ✓ 412 - with other metadata (1ms) ✓ 413 - with other metadata (1ms) ✓ 414 - with other metadata (1ms) ✓ 415 - with other metadata (1ms) ✓ 416 - with other metadata (7ms) ✓ 417 - with other metadata (2ms) ✓ 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 (1ms) ✓ 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 (2ms) ✓ 506 - with other metadata (1ms) ✓ 510 - with other metadata (1ms) ✓ 400 - without metadata (1ms) ✓ 401 - without metadata (1ms) ✓ 402 - without metadata (10ms) ✓ 403 - without metadata (1ms) ✓ 404 - without metadata (1ms) ✓ 405 - without metadata (1ms) ✓ 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 (12ms) ✓ 416 - without metadata (1ms) ✓ 417 - without metadata (2ms) ✓ 418 - without metadata (1ms) ✓ 421 - without metadata (1ms) ✓ 426 - without metadata (2ms) ✓ 429 - without metadata (2ms) ✓ 450 - without metadata (3ms) ✓ 494 - without metadata (2ms) ✓ 497 - without metadata (2ms) ✓ 499 - without metadata (1ms) ✓ 500 - without metadata (1ms) ✓ 501 - without metadata (2ms) ✓ 502 - without metadata (1ms) ✓ 503 - without metadata (10ms) ✓ 504 - without metadata (2ms) ✓ 505 - without metadata (1ms) ✓ 506 - without metadata (2ms) ✓ 510 - without metadata (1ms) ``` </details> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Manual testing <details><summary><strong>Manual testing</strong></summary> #### Checkout the branch from elastic/package-storage#370 ``` git clone https://github.com/elastic/package-storage.git cd package-storage/ git switch -C update-aws-0.2.7-1598281986 origin/update-aws-0.2.7-1598281986 ``` #### start the stack using the registry from elastic/package-storage#370 ``` cd testing/environments/ docker-compose -f snapshot.yml pull docker-compose -f snapshot.yml -f local.yml up --force-recreate ``` #### Try to install the broken package ``` > curl --user elastic:changeme -X POST http://localhost:5601/api/ingest_manager/epm/packages/aws-0.2.7 -H 'kbn-xsrf: xyz' {"statusCode":500,"error":"Internal Server Error","message":"Bad Request"} ``` _observe the same error as #75862_ #### start only local registry with the broken package ``` # CTRL-C the stack (shell where you ran `docker-compose`) cd ../.. # back to package-storage root docker build . docker run -p 8080:8080 id_from_prior_step ``` #### start the stack from this PR, pointing at the local registry from prior step ``` yarn start --no-base-path --xpack.ingestManager.registryUrl=http://localhost:8080 yarn es snapshot --license=trial -E xpack.security.authc.api_key.enabled=true ``` #### Try to install the broken package ``` 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 response from /_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}" } ``` _observe new error format_ </details>
1 parent 14cbd78 commit 96ffe74

File tree

4 files changed

+119
-19
lines changed

4 files changed

+119
-19
lines changed

x-pack/plugins/ingest_manager/server/errors.test.ts renamed to x-pack/plugins/ingest_manager/server/errors/handlers.test.ts

Lines changed: 56 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,19 @@
55
*/
66

77
import Boom from 'boom';
8+
import { errors } from 'elasticsearch';
89
import { httpServerMock } from 'src/core/server/mocks';
9-
import { createAppContextStartContractMock } from './mocks';
10-
10+
import { createAppContextStartContractMock } from '../mocks';
11+
import { appContextService } from '../services';
1112
import {
1213
IngestManagerError,
1314
RegistryError,
1415
PackageNotFoundError,
1516
defaultIngestErrorHandler,
16-
} from './errors';
17-
import { appContextService } from './services';
17+
} from './index';
18+
19+
const LegacyESErrors = errors as Record<string, any>;
20+
type ITestEsErrorsFnParams = [errorCode: string, error: any, expectedMessage: string];
1821

1922
describe('defaultIngestErrorHandler', () => {
2023
let mockContract: ReturnType<typeof createAppContextStartContractMock>;
@@ -29,6 +32,55 @@ describe('defaultIngestErrorHandler', () => {
2932
appContextService.stop();
3033
});
3134

35+
async function testEsErrorsFn(...args: ITestEsErrorsFnParams) {
36+
const [, error, expectedMessage] = args;
37+
jest.clearAllMocks();
38+
const response = httpServerMock.createResponseFactory();
39+
await defaultIngestErrorHandler({ error, response });
40+
41+
// response
42+
expect(response.ok).toHaveBeenCalledTimes(0);
43+
expect(response.customError).toHaveBeenCalledTimes(1);
44+
expect(response.customError).toHaveBeenCalledWith({
45+
statusCode: error.status,
46+
body: { message: expectedMessage },
47+
});
48+
49+
// logging
50+
expect(mockContract.logger?.error).toHaveBeenCalledTimes(1);
51+
expect(mockContract.logger?.error).toHaveBeenCalledWith(expectedMessage);
52+
}
53+
54+
describe('use the HTTP error status code provided by LegacyESErrors', () => {
55+
const statusCodes = Object.keys(LegacyESErrors).filter((key) => /^\d+$/.test(key));
56+
const errorCodes = statusCodes.filter((key) => parseInt(key, 10) >= 400);
57+
const casesWithPathResponse: ITestEsErrorsFnParams[] = errorCodes.map((errorCode) => [
58+
errorCode,
59+
new LegacyESErrors[errorCode]('the root message', {
60+
path: '/path/to/call',
61+
response: 'response is here',
62+
}),
63+
'the root message response from /path/to/call: response is here',
64+
]);
65+
const casesWithOtherMeta: ITestEsErrorsFnParams[] = errorCodes.map((errorCode) => [
66+
errorCode,
67+
new LegacyESErrors[errorCode]('the root message', {
68+
other: '/path/to/call',
69+
props: 'response is here',
70+
}),
71+
'the root message',
72+
]);
73+
const casesWithoutMeta: ITestEsErrorsFnParams[] = errorCodes.map((errorCode) => [
74+
errorCode,
75+
new LegacyESErrors[errorCode]('some message'),
76+
'some message',
77+
]);
78+
79+
test.each(casesWithPathResponse)('%d - with path & response', testEsErrorsFn);
80+
test.each(casesWithOtherMeta)('%d - with other metadata', testEsErrorsFn);
81+
test.each(casesWithoutMeta)('%d - without metadata', testEsErrorsFn);
82+
});
83+
3284
describe('IngestManagerError', () => {
3385
it('502: RegistryError', async () => {
3486
const error = new RegistryError('xyz');

x-pack/plugins/ingest_manager/server/errors.ts renamed to x-pack/plugins/ingest_manager/server/errors/handlers.ts

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,33 +4,46 @@
44
* you may not use this file except in compliance with the Elastic License.
55
*/
66

7-
/* eslint-disable max-classes-per-file */
87
import Boom, { isBoom } from 'boom';
98
import {
109
RequestHandlerContext,
1110
KibanaRequest,
1211
IKibanaResponse,
1312
KibanaResponseFactory,
1413
} from 'src/core/server';
15-
import { appContextService } from './services';
14+
import { errors as LegacyESErrors } from 'elasticsearch';
15+
import { appContextService } from '../services';
16+
import { IngestManagerError, RegistryError, PackageNotFoundError } from './index';
1617

1718
type IngestErrorHandler = (
1819
params: IngestErrorHandlerParams
1920
) => IKibanaResponse | Promise<IKibanaResponse>;
20-
2121
interface IngestErrorHandlerParams {
2222
error: IngestManagerError | Boom | Error;
2323
response: KibanaResponseFactory;
2424
request?: KibanaRequest;
2525
context?: RequestHandlerContext;
2626
}
27+
// unsure if this is correct. would prefer to use something "official"
28+
// this type is based on BadRequest values observed while debugging https://github.com/elastic/kibana/issues/75862
2729

28-
export class IngestManagerError extends Error {
29-
constructor(message?: string) {
30-
super(message);
31-
this.name = this.constructor.name; // for stack traces
32-
}
30+
interface LegacyESClientError {
31+
message: string;
32+
stack: string;
33+
status: number;
34+
displayName: string;
35+
path?: string;
36+
query?: string | undefined;
37+
body?: {
38+
error: object;
39+
status: number;
40+
};
41+
statusCode?: number;
42+
response?: string;
3343
}
44+
export const isLegacyESClientError = (error: any): error is LegacyESClientError => {
45+
return error instanceof LegacyESErrors._Abstract;
46+
};
3447

3548
const getHTTPResponseCode = (error: IngestManagerError): number => {
3649
if (error instanceof RegistryError) {
@@ -48,6 +61,22 @@ export const defaultIngestErrorHandler: IngestErrorHandler = async ({
4861
response,
4962
}: IngestErrorHandlerParams): Promise<IKibanaResponse> => {
5063
const logger = appContextService.getLogger();
64+
if (isLegacyESClientError(error)) {
65+
// there was a problem communicating with ES (e.g. via `callCluster`)
66+
// only log the message
67+
const message =
68+
error?.path && error?.response
69+
? // if possible, return the failing endpoint and its response
70+
`${error.message} response from ${error.path}: ${error.response}`
71+
: error.message;
72+
73+
logger.error(message);
74+
75+
return response.customError({
76+
statusCode: error?.statusCode || error.status,
77+
body: { message },
78+
});
79+
}
5180

5281
// our "expected" errors
5382
if (error instanceof IngestManagerError) {
@@ -76,9 +105,3 @@ export const defaultIngestErrorHandler: IngestErrorHandler = async ({
76105
body: { message: error.message },
77106
});
78107
};
79-
80-
export class RegistryError extends IngestManagerError {}
81-
export class RegistryConnectionError extends RegistryError {}
82-
export class RegistryResponseError extends RegistryError {}
83-
export class PackageNotFoundError extends IngestManagerError {}
84-
export class PackageOutdatedError extends IngestManagerError {}
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License;
4+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
7+
/* eslint-disable max-classes-per-file */
8+
export { defaultIngestErrorHandler } from './handlers';
9+
10+
export class IngestManagerError extends Error {
11+
constructor(message?: string) {
12+
super(message);
13+
this.name = this.constructor.name; // for stack traces
14+
}
15+
}
16+
export class RegistryError extends IngestManagerError {}
17+
export class RegistryConnectionError extends RegistryError {}
18+
export class RegistryResponseError extends RegistryError {}
19+
export class PackageNotFoundError extends IngestManagerError {}
20+
export class PackageOutdatedError extends IngestManagerError {}

x-pack/plugins/ingest_manager/server/services/epm/elasticsearch/ingest_pipeline/install.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,12 @@ async function installPipeline({
143143
body: pipeline.contentForInstallation,
144144
};
145145
if (pipeline.extension === 'yml') {
146-
callClusterParams.headers = { ['Content-Type']: 'application/yaml' };
146+
callClusterParams.headers = {
147+
// pipeline is YAML
148+
'Content-Type': 'application/yaml',
149+
// but we want JSON responses (to extract error messages, status code, or other metadata)
150+
Accept: 'application/json',
151+
};
147152
}
148153

149154
// This uses the catch-all endpoint 'transport.request' because we have to explicitly

0 commit comments

Comments
 (0)