-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Security Solution][Endpoint] Better error handling for ES errors #266524
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
Changes from all commits
df526b7
9bae854
1d0c5ab
a1cf606
4021140
1ade2e2
3ea526a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
|
|
||
| import { AgentNotFoundError } from '@kbn/fleet-plugin/server'; | ||
| import { | ||
| AgentPolicyNotFoundError, | ||
| PackagePolicyNotFoundError, | ||
| } from '@kbn/fleet-plugin/server/errors'; | ||
| import { errors } from '@elastic/elasticsearch'; | ||
| import { EndpointError } from '../../../common/endpoint/errors'; | ||
| import { NotFoundError } from '../errors'; | ||
| import { catchAndWrapError, wrapErrorIfNeeded } from './wrap_errors'; | ||
|
|
||
| describe('wrapErrorIfNeeded', () => { | ||
| it('returns the same instance when already an EndpointError', () => { | ||
| const original = new EndpointError('already wrapped'); | ||
| expect(wrapErrorIfNeeded(original)).toBe(original); | ||
| }); | ||
|
|
||
| it('wraps a plain Error in EndpointError', () => { | ||
| const original = new Error('plain error'); | ||
| const result = wrapErrorIfNeeded(original); | ||
|
|
||
| expect(result).toBeInstanceOf(EndpointError); | ||
| expect(result.message).toBe('plain error'); | ||
| expect(result.meta).toBe(original); | ||
| }); | ||
|
|
||
| it('applies messagePrefix to a plain Error', () => { | ||
| const original = new Error('something went wrong'); | ||
| const result = wrapErrorIfNeeded(original, 'context'); | ||
|
|
||
| expect(result).toBeInstanceOf(EndpointError); | ||
| expect(result.message).toBe('context: something went wrong'); | ||
| }); | ||
|
|
||
| describe('Fleet Not Found errors', () => { | ||
| it('wraps AgentNotFoundError in NotFoundError', () => { | ||
| const original = new AgentNotFoundError('agent not found'); | ||
| const result = wrapErrorIfNeeded(original); | ||
|
|
||
| expect(result).toBeInstanceOf(NotFoundError); | ||
| expect(result.message).toBe('agent not found'); | ||
| expect(result.meta).toBe(original); | ||
| }); | ||
|
|
||
| it('wraps AgentPolicyNotFoundError in NotFoundError', () => { | ||
| const original = new AgentPolicyNotFoundError('policy not found'); | ||
| const result = wrapErrorIfNeeded(original); | ||
|
|
||
| expect(result).toBeInstanceOf(NotFoundError); | ||
| expect(result.message).toBe('policy not found'); | ||
| }); | ||
|
|
||
| it('wraps PackagePolicyNotFoundError in NotFoundError', () => { | ||
| const original = new PackagePolicyNotFoundError('package policy not found'); | ||
| const result = wrapErrorIfNeeded(original); | ||
|
|
||
| expect(result).toBeInstanceOf(NotFoundError); | ||
| expect(result.message).toBe('package policy not found'); | ||
| }); | ||
|
|
||
| it('applies messagePrefix to Fleet Not Found errors', () => { | ||
| const original = new AgentNotFoundError('agent-123'); | ||
| const result = wrapErrorIfNeeded(original, 'fetch agent'); | ||
|
|
||
| expect(result).toBeInstanceOf(NotFoundError); | ||
| expect(result.message).toBe('fetch agent: agent-123'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Elasticsearch errors', () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const buildResponseError = (body: any, statusCode = 404) => | ||
| new errors.ResponseError({ | ||
| body, | ||
| statusCode, | ||
| headers: {}, | ||
| warnings: null, | ||
| // @ts-expect-error | ||
| meta: { | ||
| body, | ||
| statusCode, | ||
| headers: {}, | ||
| context: {}, | ||
| request: { | ||
| params: { method: 'GET', path: '/_search', querystring: {}, body: undefined }, | ||
| options: {}, | ||
| id: 'test-request', | ||
| }, | ||
| attempts: 1, | ||
| aborted: false, | ||
| } as unknown as errors.ResponseError['meta'], | ||
| }); | ||
|
|
||
| it('wraps ElasticsearchClientError in EndpointError', () => { | ||
| const esError = buildResponseError({ error: { type: 'index_not_found_exception' } }); | ||
| const result = wrapErrorIfNeeded(esError); | ||
|
|
||
| expect(result).toBeInstanceOf(EndpointError); | ||
| }); | ||
|
|
||
| it('builds a descriptive message from ES response reason fields', () => { | ||
| const esError = buildResponseError({ | ||
| error: { | ||
| type: 'index_not_found_exception', | ||
| reason: 'no such index [.fleet-agents]', | ||
| index: '.fleet-agents', | ||
| }, | ||
| }); | ||
| const result = wrapErrorIfNeeded(esError); | ||
|
|
||
| expect(result.message).toContain('no such index [.fleet-agents]'); | ||
| }); | ||
|
|
||
| it('applies messagePrefix to ES errors', () => { | ||
| const esError = buildResponseError({ | ||
| error: { type: 'search_phase_execution_exception', reason: 'shard failed' }, | ||
| }); | ||
| const result = wrapErrorIfNeeded(esError, 'search failed'); | ||
|
|
||
| expect(result.message).toMatch(/^search failed:/); | ||
| }); | ||
|
|
||
| it('populates debug.es_request with request parameters', () => { | ||
| const esError = buildResponseError({ error: { reason: 'not found' } }); | ||
| const result = wrapErrorIfNeeded(esError); | ||
|
|
||
| expect(result.debug).toBeDefined(); | ||
| expect(result.debug.es_request).toMatchObject({ | ||
| method: 'GET', | ||
| path: '/_search', | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('catchAndWrapError', () => { | ||
| it('rejects with an EndpointError wrapping the original error', async () => { | ||
| const original = new Error('async failure'); | ||
| await expect(Promise.reject(original).catch(catchAndWrapError)).rejects.toBeInstanceOf( | ||
| EndpointError | ||
| ); | ||
| }); | ||
|
|
||
| it('rejects with the same EndpointError when already one', async () => { | ||
| const original = new EndpointError('already wrapped'); | ||
| await expect(Promise.reject(original).catch(catchAndWrapError)).rejects.toBe(original); | ||
| }); | ||
|
|
||
| describe('withMessage', () => { | ||
| it('rejects with an EndpointError whose message includes the custom prefix', async () => { | ||
| const original = new Error('downstream failure'); | ||
| await expect( | ||
| Promise.reject(original).catch(catchAndWrapError.withMessage('custom prefix')) | ||
| ).rejects.toMatchObject({ | ||
| message: 'custom prefix: downstream failure', | ||
| }); | ||
| }); | ||
|
|
||
| it('wraps Fleet Not Found errors in NotFoundError with prefix', async () => { | ||
| const original = new AgentNotFoundError('not found'); | ||
| await expect( | ||
| Promise.reject(original).catch(catchAndWrapError.withMessage('get agent')) | ||
| ).rejects.toBeInstanceOf(NotFoundError); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,17 +5,22 @@ | |
| * 2.0. | ||
| */ | ||
|
|
||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||
|
|
||
| import { AgentNotFoundError } from '@kbn/fleet-plugin/server'; | ||
| import { | ||
| AgentPolicyNotFoundError, | ||
| PackagePolicyNotFoundError, | ||
| } from '@kbn/fleet-plugin/server/errors'; | ||
| import { NotFoundError } from '../errors'; | ||
| import { errors, type DiagnosticResult } from '@elastic/elasticsearch'; | ||
| import { isPlainObject } from 'lodash'; | ||
| import { EndpointError } from '../../../common/endpoint/errors'; | ||
| import { NotFoundError } from '../errors'; | ||
|
|
||
| /** | ||
| * Will wrap the given Error with `EndpointError`, which will help getting a good picture of where in | ||
| * our code the error originated (better stack trace). | ||
| * our code the error originated (better stack trace). It will also process some known error types | ||
| * and build a more descriptive error message and add additional `debug` details to the error object. | ||
| */ | ||
| export const wrapErrorIfNeeded = <E extends EndpointError = EndpointError>( | ||
| error: Error, | ||
|
|
@@ -25,7 +30,79 @@ export const wrapErrorIfNeeded = <E extends EndpointError = EndpointError>( | |
| return error as E; | ||
| } | ||
|
|
||
| const message = `${messagePrefix ? `${messagePrefix}: ` : ''}${error.message}`; | ||
| let debug: EndpointError['debug']; | ||
| let message = `${messagePrefix ? `${messagePrefix}: ` : ''}${error.message}`; | ||
|
|
||
| try { | ||
| // Process known error Types and retrieve additional data not normally output to logs | ||
| if (error instanceof errors.ElasticsearchClientError) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be worth adding a small |
||
| const esError = error as { meta?: DiagnosticResult; body?: any }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of typin we could probably go with if (error instanceof errors.ResponseError) {
const { params } = error.meta.meta.request;
debug = {
es_request: {
method: params.method,
...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That will not work. Here I'm checking that the Error is one emitted by the Elastic search client (the base Error class) and that Error definition does not have a see |
||
|
|
||
| debug = { | ||
| es_request: { | ||
| method: esError.meta?.meta?.request?.params?.method, | ||
| path: esError.meta?.meta?.request?.params?.path, | ||
| querystring: esError.meta?.meta?.request?.params?.querystring, | ||
| body: esError.meta?.meta?.request?.params?.body, | ||
| }, | ||
| es_response: { | ||
| body: esError.body, | ||
| }, | ||
| }; | ||
|
|
||
| // Since this is an elasticsearch client error, lets build a better error message | ||
| // that is based on the Elasticsearch error response body | ||
|
|
||
| const queue: any[] = [debug.es_response.body]; | ||
| let newMessage = ''; | ||
|
|
||
| // The most common Elasticsearch error response structure seems to be something like: | ||
| // { | ||
| // error?: { | ||
| // type: string; // e.g., 'index_not_found_exception' | ||
| // reason: string; // Human-readable message | ||
| // caused_by?: { | ||
| // type?: string; | ||
| // reason?: string; | ||
| // caused_by?: { ... } // Recursive chain | ||
| // }; | ||
| // root_cause?: Array<{ // Array of root causes | ||
| // type?: string; | ||
| // reason?: string; | ||
| // }>; | ||
| // }; | ||
| // status?: number; // HTTP status code | ||
| // } | ||
| // So we'll loop through all this data and grab the string values for 'reason' | ||
| while (queue.length > 0) { | ||
| const record = queue.shift(); | ||
|
|
||
| if (Array.isArray(record)) { | ||
| queue.push(...record); | ||
| } else if (isPlainObject(record)) { | ||
| Object.entries(record).forEach(([key, value]) => { | ||
| if (isPlainObject(value) || Array.isArray(value)) { | ||
| queue.push(value); | ||
| } else if (key === 'reason') { | ||
| newMessage += (newMessage.length > 0 ? ' > ' : '') + value; | ||
|
|
||
| if (record.index) { | ||
| newMessage += ` (index: ${record.index})`; | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| if (newMessage.length > 0) { | ||
| message = `${ | ||
| messagePrefix ? `${messagePrefix}: ` : '' | ||
| }Elasticsearch error encountered: ${newMessage}`; | ||
| } | ||
| } | ||
| } catch (_) { | ||
| /* best effort - failures are ignored */ | ||
| } | ||
|
|
||
| // Check for known "Not Found" errors and wrap them with our own `NotFoundError`, which will enable | ||
| // the correct HTTP status code to be used if it is thrown during processing of an API route | ||
|
|
@@ -37,7 +114,10 @@ export const wrapErrorIfNeeded = <E extends EndpointError = EndpointError>( | |
| return new NotFoundError(message, error) as E; | ||
| } | ||
|
|
||
| return new EndpointError(message, error) as E; | ||
| const err = new EndpointError(message, error) as E; | ||
| err.debug = debug; | ||
|
|
||
| return err; | ||
| }; | ||
|
|
||
| interface CatchAndWrapError { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we could go with something like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EndpointErroris generic and also used by the UI, so I rather keepdebugproperty set atany. Although with this change was triggered by the lack of detail while investigating a recent issue that originated from a ES error, thedebugproperty is really to capture anything else that one might like to include when an error of this type if thrown.