Skip to content

Commit 2bfd4a9

Browse files
Supplementingelasticmachinekibanamachine
authored andcommitted
[Fleet] Add graceful error handling for invalid sortField on GET api/fleet/agents (elastic#239017)
## Summary Closes elastic/ingest-dev#5992 - Added error handling for all fleet endpoints that use ES. ES errors will now bubble up and be properly returned along with their status code response from ES. This will stop errors from being `500` when they are actually something else, we just werent properly handling them at the top level. - Also added additional test coverage to verify the handler will throw/succeed when its supposed to, and added some simple integration tests to verify the entire flow works as intended. <img width="811" height="236" alt="image" src="https://github.com/user-attachments/assets/dbc3b831-6984-427a-9762-399ad3b8e1b9" /> ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: kibanamachine <[email protected]>
1 parent 56d977a commit 2bfd4a9

File tree

4 files changed

+301
-2
lines changed

4 files changed

+301
-2
lines changed

x-pack/platform/plugins/shared/fleet/server/errors/handlers.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ import {
2626
AgentPolicyNameExistsError,
2727
ConcurrentInstallOperationError,
2828
FleetError,
29+
FleetElasticsearchValidationError,
30+
isESClientError,
2931
PackageUnsupportedMediaTypeError,
3032
RegistryConnectionError,
3133
RegistryError,
@@ -231,6 +233,12 @@ export const defaultFleetErrorHandler: IngestErrorHandler = async ({
231233
error,
232234
response,
233235
}: IngestErrorHandlerParams): Promise<IKibanaResponse> => {
234-
const options = fleetErrorToResponseOptions(error);
236+
// Convert ALL Elasticsearch errors to Fleet errors (preserving original status codes)
237+
let processedError = error;
238+
if (isESClientError(error)) {
239+
processedError = new FleetElasticsearchValidationError(error);
240+
}
241+
242+
const options = fleetErrorToResponseOptions(processedError);
235243
return response.customError(options);
236244
};

x-pack/platform/plugins/shared/fleet/server/errors/index.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,5 +241,20 @@ export class ArtifactsElasticsearchError extends FleetError {
241241
}
242242
}
243243

244+
export class FleetElasticsearchValidationError extends FleetErrorWithStatusCode {
245+
constructor(esError: Error) {
246+
let statusCode: number | undefined;
247+
const message = esError.message;
248+
249+
// Extract the original ES status code and ensure we have meta with statusCode
250+
if (isESClientError(esError)) {
251+
statusCode = esError.meta.statusCode;
252+
}
253+
254+
// Pass through the ES error message, status code, and original error as meta
255+
super(message, statusCode, esError);
256+
}
257+
}
258+
244259
export class FleetFilesClientError extends FleetError {}
245260
export class FleetFileNotFound extends FleetFilesClientError {}

x-pack/platform/plugins/shared/fleet/server/routes/agent/handlers.test.ts

Lines changed: 270 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,16 @@
66
*/
77

88
import { coreMock, httpServerMock } from '@kbn/core/server/mocks';
9+
import { errors } from '@elastic/elasticsearch';
910

1011
import { getAgentStatusForAgentPolicy } from '../../services/agents/status';
12+
import { fetchAndAssignAgentMetrics } from '../../services/agents/agent_metrics';
1113

12-
import { getAgentStatusForAgentPolicyHandler, getAvailableVersionsHandler } from './handlers';
14+
import {
15+
getAgentStatusForAgentPolicyHandler,
16+
getAvailableVersionsHandler,
17+
getAgentsHandler,
18+
} from './handlers';
1319

1420
jest.mock('../../services/agents/versions', () => {
1521
return {
@@ -30,7 +36,270 @@ jest.mock('../../services/agents/status', () => ({
3036
getAgentStatusForAgentPolicy: jest.fn(),
3137
}));
3238

39+
jest.mock('../../services/agents/agent_metrics', () => ({
40+
fetchAndAssignAgentMetrics: jest.fn(),
41+
}));
42+
3343
describe('Handlers', () => {
44+
// Helper function to create mock Elasticsearch errors
45+
const createMockESError = (errorBody: any, statusCode: number = 400) => {
46+
const error = new Error('ResponseError') as any;
47+
error.meta = {
48+
body: errorBody,
49+
statusCode,
50+
};
51+
Object.setPrototypeOf(error, errors.ResponseError.prototype);
52+
return error;
53+
};
54+
55+
describe('getAgentsHandler', () => {
56+
let mockAgentClient: any;
57+
let mockContext: any;
58+
let mockResponse: any;
59+
60+
beforeEach(() => {
61+
mockAgentClient = {
62+
asCurrentUser: {
63+
listAgents: jest.fn(),
64+
},
65+
};
66+
67+
mockContext = {
68+
core: Promise.resolve(coreMock.createRequestHandlerContext()),
69+
fleet: Promise.resolve({
70+
agentClient: mockAgentClient,
71+
}),
72+
};
73+
74+
mockResponse = httpServerMock.createResponseFactory();
75+
(fetchAndAssignAgentMetrics as jest.Mock).mockClear();
76+
});
77+
78+
it('should handle successful agent listing', async () => {
79+
const mockAgents = [
80+
{ id: 'agent1', enrolled_at: '2023-01-01' },
81+
{ id: 'agent2', enrolled_at: '2023-01-02' },
82+
];
83+
84+
mockAgentClient.asCurrentUser.listAgents.mockResolvedValue({
85+
agents: mockAgents,
86+
total: 2,
87+
page: 1,
88+
perPage: 20,
89+
});
90+
91+
const request = {
92+
query: {
93+
page: 1,
94+
perPage: 20,
95+
sortField: 'enrolled_at',
96+
sortOrder: 'desc',
97+
},
98+
};
99+
100+
await getAgentsHandler(mockContext, request as any, mockResponse);
101+
102+
expect(mockResponse.ok).toHaveBeenCalledWith({
103+
body: {
104+
items: mockAgents,
105+
total: 2,
106+
page: 1,
107+
perPage: 20,
108+
},
109+
});
110+
});
111+
112+
it('should let ES parsing errors bubble up to global error handler', async () => {
113+
const elasticsearchError = createMockESError({
114+
error: {
115+
type: 'parsing_exception',
116+
reason: 'Invalid query syntax',
117+
},
118+
});
119+
120+
mockAgentClient.asCurrentUser.listAgents.mockRejectedValue(elasticsearchError);
121+
122+
const request = {
123+
query: {
124+
sortField: 'invalid_field',
125+
},
126+
};
127+
128+
await expect(getAgentsHandler(mockContext, request as any, mockResponse)).rejects.toEqual(
129+
elasticsearchError
130+
);
131+
});
132+
133+
it('should let ES argument errors bubble up to global error handler', async () => {
134+
const elasticsearchError = createMockESError({
135+
error: {
136+
type: 'illegal_argument_exception',
137+
reason: 'No mapping found for [non_existent_field] in order to sort on',
138+
},
139+
});
140+
141+
mockAgentClient.asCurrentUser.listAgents.mockRejectedValue(elasticsearchError);
142+
143+
const request = {
144+
query: {
145+
sortField: 'non_existent_field',
146+
},
147+
};
148+
149+
await expect(getAgentsHandler(mockContext, request as any, mockResponse)).rejects.toEqual(
150+
elasticsearchError
151+
);
152+
});
153+
154+
it('should let search_phase_execution_exception errors bubble up to global error handler', async () => {
155+
const elasticsearchError = createMockESError({
156+
error: {
157+
type: 'search_phase_execution_exception',
158+
reason: 'Unknown field [bad_field]',
159+
},
160+
});
161+
162+
mockAgentClient.asCurrentUser.listAgents.mockRejectedValue(elasticsearchError);
163+
164+
const request = {
165+
query: {
166+
sortField: 'bad_field',
167+
},
168+
};
169+
170+
await expect(getAgentsHandler(mockContext, request as any, mockResponse)).rejects.toEqual(
171+
elasticsearchError
172+
);
173+
});
174+
175+
it('should let field mapping errors bubble up to global error handler', async () => {
176+
const elasticsearchError = createMockESError({
177+
error: {
178+
type: 'some_error_type',
179+
reason: 'No mapping found for field [invalid_field]',
180+
},
181+
});
182+
183+
mockAgentClient.asCurrentUser.listAgents.mockRejectedValue(elasticsearchError);
184+
185+
const request = {
186+
query: {
187+
sortField: 'invalid_field',
188+
},
189+
};
190+
191+
await expect(getAgentsHandler(mockContext, request as any, mockResponse)).rejects.toEqual(
192+
elasticsearchError
193+
);
194+
});
195+
196+
it('should let unknown field errors bubble up to global error handler', async () => {
197+
const elasticsearchError = createMockESError({
198+
error: {
199+
type: 'some_error_type',
200+
reason: 'Unknown field [mystery_field] in sort criteria',
201+
},
202+
});
203+
204+
mockAgentClient.asCurrentUser.listAgents.mockRejectedValue(elasticsearchError);
205+
206+
const request = {
207+
query: {
208+
sortField: 'mystery_field',
209+
},
210+
};
211+
212+
await expect(getAgentsHandler(mockContext, request as any, mockResponse)).rejects.toEqual(
213+
elasticsearchError
214+
);
215+
});
216+
217+
it('should re-throw non-validation errors', async () => {
218+
const systemError = new Error('System error');
219+
220+
mockAgentClient.asCurrentUser.listAgents.mockRejectedValue(systemError);
221+
222+
const request = {
223+
query: {
224+
sortField: 'enrolled_at',
225+
},
226+
};
227+
228+
await expect(getAgentsHandler(mockContext, request as any, mockResponse)).rejects.toThrow(
229+
'System error'
230+
);
231+
});
232+
233+
it('should re-throw elasticsearch errors that are not validation errors', async () => {
234+
const elasticsearchError = createMockESError({
235+
error: {
236+
type: 'cluster_block_exception',
237+
reason: 'Cluster is read-only',
238+
},
239+
});
240+
241+
mockAgentClient.asCurrentUser.listAgents.mockRejectedValue(elasticsearchError);
242+
243+
const request = {
244+
query: {
245+
sortField: 'enrolled_at',
246+
},
247+
};
248+
249+
await expect(getAgentsHandler(mockContext, request as any, mockResponse)).rejects.toEqual(
250+
elasticsearchError
251+
);
252+
});
253+
254+
it('should let root_cause errors bubble up to global error handler', async () => {
255+
const elasticsearchError = createMockESError({
256+
error: {
257+
type: 'search_phase_execution_exception',
258+
reason: 'all shards failed',
259+
root_cause: [
260+
{
261+
type: 'illegal_argument_exception',
262+
reason: 'No mapping found for [hostname] in order to sort on',
263+
},
264+
],
265+
},
266+
});
267+
268+
mockAgentClient.asCurrentUser.listAgents.mockRejectedValue(elasticsearchError);
269+
270+
const request = {
271+
query: {
272+
sortField: 'hostname',
273+
},
274+
};
275+
276+
await expect(getAgentsHandler(mockContext, request as any, mockResponse)).rejects.toEqual(
277+
elasticsearchError
278+
);
279+
});
280+
281+
it('should let errors with missing reasons bubble up to global error handler', async () => {
282+
const elasticsearchError = createMockESError({
283+
error: {
284+
type: 'parsing_exception',
285+
// no reason provided
286+
},
287+
});
288+
289+
mockAgentClient.asCurrentUser.listAgents.mockRejectedValue(elasticsearchError);
290+
291+
const request = {
292+
query: {
293+
sortField: 'invalid_field',
294+
},
295+
};
296+
297+
await expect(getAgentsHandler(mockContext, request as any, mockResponse)).rejects.toEqual(
298+
elasticsearchError
299+
);
300+
});
301+
});
302+
34303
describe('getAgentStatusForAgentPolicyHandler', () => {
35304
it.each([
36305
{ requested: 'policy-id-1', called: ['policy-id-1'] },

x-pack/platform/test/fleet_api_integration/apis/agents/list.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,13 @@ export default function ({ getService }: FtrProviderContext) {
101101
expect(agent.access_api_key_id).to.eql('api-key-2');
102102
});
103103

104+
it('should return a 400 when given an invalid "sortField" value', async () => {
105+
await supertest
106+
.get(`/api/fleet/agents?sortField=invalid_field`)
107+
.set('kbn-xsrf', 'xxxx')
108+
.expect(400);
109+
});
110+
104111
it('should return a 200 when given sort options', async () => {
105112
const { body: apiResponse } = await supertest
106113
.get(`/api/fleet/agents?sortField=last_checkin&sortOrder=desc`)

0 commit comments

Comments
 (0)