Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Ingest Manager] Handle Legacy ES client errors #76865

Merged
merged 9 commits into from
Sep 10, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,19 @@
*/

import Boom from 'boom';
import { errors } from 'elasticsearch';
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';

const LegacyESErrors = errors as Record<string, any>;
type ITestEsErrorsFnParams = [errorCode: string, error: any, bodyMessage: string];

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

async function testEsErrorsFn(...args: ITestEsErrorsFnParams) {
const [, error, bodyMessage] = args;
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 casesWithPathResponse: ITestEsErrorsFnParams[] = errorCodes.map((errorCode) => [
errorCode,
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',
]);
const casesWithOtherMeta: ITestEsErrorsFnParams[] = errorCodes.map((errorCode) => [
errorCode,
new LegacyESErrors[errorCode]('the root message', {
other: '/path/to/call',
props: 'response is here',
}),
'the root message',
]);
const casesWithoutMeta: ITestEsErrorsFnParams[] = errorCodes.map((errorCode) => [
errorCode,
new LegacyESErrors[errorCode]('some message'),
'some message',
]);

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', () => {
it('502: RegistryError', async () => {
const error = new RegistryError('xyz');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,46 @@
* 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,
KibanaRequest,
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<IKibanaResponse>;

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 is based on BadRequest values observed while debugging https://github.com/elastic/kibana/issues/75862

export class IngestManagerError extends Error {
constructor(message?: string) {
super(message);
this.name = this.constructor.name; // for stack traces
}
interface LegacyESClientError {
message: string;
stack: string;
status: number;
displayName: string;
path?: string;
query?: string | undefined;
body?: {
error: object;
status: number;
};
statusCode?: number;
response?: string;
}
export const isLegacyESClientError = (error: any): error is LegacyESClientError => {
return error instanceof LegacyESErrors._Abstract;
};

const getHTTPResponseCode = (error: IngestManagerError): number => {
if (error instanceof RegistryError) {
Expand All @@ -48,6 +61,21 @@ export const defaultIngestErrorHandler: IngestErrorHandler = async ({
response,
}: IngestErrorHandlerParams): Promise<IKibanaResponse> => {
const logger = appContextService.getLogger();
if (isLegacyESClientError(error)) {
// there was a problem communicating with ES (e.g. via `callCluster`)
// only log the message
logger.error(error.message);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not log the more elaborate message that is constructed below, and passed to response.customError()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No real reason not to. I considered it at one point. Happy to update it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be helpful to have the same level of detail in both the log and the returned message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally. Just pushed fdaf588

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

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 || error.status,
body: { message },
});
}

// our "expected" errors
if (error instanceof IngestManagerError) {
Expand Down Expand Up @@ -76,9 +104,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 {}
20 changes: 20 additions & 0 deletions x-pack/plugins/ingest_manager/server/errors/index.ts
Original file line number Diff line number Diff line change
@@ -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 {}
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,12 @@ async function installPipeline({
body: pipeline.contentForInstallation,
};
if (pipeline.extension === 'yml') {
callClusterParams.headers = { ['Content-Type']: 'application/yaml' };
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',
};
}

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