Skip to content

Commit

Permalink
Improves not found response handling in the saved objects repository (#…
Browse files Browse the repository at this point in the history
…108749) (#108954)

Co-authored-by: Christiane (Tina) Heiligers <[email protected]>
  • Loading branch information
kibanamachine and TinaHeiligers authored Aug 17, 2021
1 parent 416e0ba commit 6acf24a
Show file tree
Hide file tree
Showing 13 changed files with 452 additions and 78 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@
<b>Signature:</b>

```typescript
static createGenericNotFoundEsUnavailableError(type: string, id: string): DecoratedError;
static createGenericNotFoundEsUnavailableError(type?: string | null, id?: string | null): DecoratedError;
```

## Parameters

| Parameter | Type | Description |
| --- | --- | --- |
| type | <code>string</code> | |
| id | <code>string</code> | |
| type | <code>string &#124; null</code> | |
| id | <code>string &#124; null</code> | |

<b>Returns:</b>

Expand Down
5 changes: 3 additions & 2 deletions src/core/server/elasticsearch/client/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { TransportRequestPromise } from '@elastic/elasticsearch/lib/Transport';
import type { DeeplyMockedKeys } from '@kbn/utility-types/jest';
import { ElasticsearchClient } from './types';
import { ICustomClusterClient } from './cluster_client';
import { PRODUCT_RESPONSE_HEADER } from '../supported_server_response_check';

const createInternalClientMock = (
res?: MockedTransportRequestPromise<unknown>
Expand Down Expand Up @@ -142,7 +143,7 @@ export type MockedTransportRequestPromise<T> = TransportRequestPromise<T> & {
const createSuccessTransportRequestPromise = <T>(
body: T,
{ statusCode = 200 }: { statusCode?: number } = {},
headers?: Record<string, string | string[]>
headers: Record<string, string | string[]> = { [PRODUCT_RESPONSE_HEADER]: 'Elasticsearch' }
): MockedTransportRequestPromise<ApiResponse<T>> => {
const response = createApiResponse({ body, statusCode, headers });
const promise = Promise.resolve(response);
Expand All @@ -163,7 +164,7 @@ function createApiResponse<TResponse = Record<string, any>>(
return {
body: {} as any,
statusCode: 200,
headers: {},
headers: { [PRODUCT_RESPONSE_HEADER]: 'Elasticsearch' },
warnings: [],
meta: {} as any,
...opts,
Expand Down
6 changes: 5 additions & 1 deletion src/core/server/elasticsearch/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,8 @@ export type {
DeleteDocumentResponse,
} from './client';
export { getRequestDebugMeta, getErrorMessage } from './client';
export { isSupportedEsServer } from './supported_server_response_check';
export {
isSupportedEsServer,
isNotFoundFromUnsupportedServer,
PRODUCT_RESPONSE_HEADER,
} from './supported_server_response_check';
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* 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 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { isNotFoundFromUnsupportedServer } from './supported_server_response_check';

describe('#isNotFoundFromUnsupportedServer', () => {
it('returns true with not found response from unsupported server', () => {
const rawResponse = {
statusCode: 404,
headers: {},
};

const result = isNotFoundFromUnsupportedServer(rawResponse);
expect(result).toBe(true);
});

it('returns false with not found response from supported server', async () => {
const rawResponse = {
statusCode: 404,
headers: { 'x-elastic-product': 'Elasticsearch' },
};

const result = isNotFoundFromUnsupportedServer(rawResponse);
expect(result).toBe(false);
});

it('returns false when not a 404', async () => {
const rawResponse = {
statusCode: 200,
headers: { 'x-elastic-product': 'Elasticsearch' },
};

const result = isNotFoundFromUnsupportedServer(rawResponse);
expect(result).toBe(false);
});
});
18 changes: 17 additions & 1 deletion src/core/server/elasticsearch/supported_server_response_check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,22 @@ export const PRODUCT_RESPONSE_HEADER = 'x-elastic-product';
* @returns boolean
*/
// This check belongs to the elasticsearch service as a dedicated helper method.
export const isSupportedEsServer = (headers: Record<string, string> | null) => {
export const isSupportedEsServer = (headers: Record<string, string | string[]> | null) => {
return !!headers && headers[PRODUCT_RESPONSE_HEADER] === 'Elasticsearch';
};

/**
* Check to ensure that a 404 response does not come from Elasticsearch
*
* WARNING: This is a hack to work around for 404 responses returned from a proxy.
* We're aiming to minimise the risk of data loss when consumers act on Not Found errors
*
* @param response response from elasticsearch client call
* @returns boolean 'true' if the status code is 404 and the Elasticsearch product header is missing/unexpected value
*/
export const isNotFoundFromUnsupportedServer = (args: {
statusCode: number | null;
headers: Record<string, string | string[]> | null;
}): boolean => {
return args.statusCode === 404 && !isSupportedEsServer(args.headers);
};
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { savedObjectsPointInTimeFinderMock } from './point_in_time_finder.mock';
import { savedObjectsRepositoryMock } from './repository.mock';
import { PointInTimeFinder } from './point_in_time_finder';
import { ISavedObjectsRepository } from './repository';
import { SavedObjectsErrorHelpers } from './errors';

const SPACES = ['default', 'another-space'];
const VERSION_PROPS = { _seq_no: 1, _primary_term: 1 };
Expand Down Expand Up @@ -318,6 +319,23 @@ describe('collectMultiNamespaceReferences', () => {
// obj3 is excluded from the results
]);
});
it(`handles 404 responses that don't come from Elasticsearch`, async () => {
const createEsUnavailableNotFoundError = () => {
return SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError();
};
const obj1 = { type: MULTI_NAMESPACE_OBJ_TYPE_1, id: 'id-1' };
const params = setup([obj1]);
client.mget.mockReturnValueOnce(
elasticsearchClientMock.createSuccessTransportRequestPromise(
{ docs: [] },
{ statusCode: 404 },
{}
)
);
await expect(() => collectMultiNamespaceReferences(params)).rejects.toThrowError(
createEsUnavailableNotFoundError()
);
});

describe('legacy URL aliases', () => {
it('uses the PointInTimeFinder to search for legacy URL aliases', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@
*/

import * as esKuery from '@kbn/es-query';

import { isNotFoundFromUnsupportedServer } from '../../../elasticsearch';
import { LegacyUrlAlias, LEGACY_URL_ALIAS_TYPE } from '../../object_types';
import type { ISavedObjectTypeRegistry } from '../../saved_objects_type_registry';
import type { SavedObjectsSerializer } from '../../serialization';
import type { SavedObject, SavedObjectsBaseOptions } from '../../types';
import { SavedObjectsErrorHelpers } from './errors';
import { getRootFields } from './included_fields';
import { getSavedObjectFromSource, rawDocExistsInNamespace } from './internal_utils';
import type {
Expand Down Expand Up @@ -198,6 +199,15 @@ async function getObjectsAndReferences({
{ body: { docs: makeBulkGetDocs(bulkGetObjects) } },
{ ignore: [404] }
);
// exit early if we can't verify a 404 response is from Elasticsearch
if (
isNotFoundFromUnsupportedServer({
statusCode: bulkGetResponse.statusCode,
headers: bulkGetResponse.headers,
})
) {
throw SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError();
}
const newObjectsToGet = new Set<string>();
for (let i = 0; i < bulkGetObjects.length; i++) {
// For every element in bulkGetObjects, there should be a matching element in bulkGetResponse.body.docs
Expand Down
6 changes: 5 additions & 1 deletion src/core/server/saved_objects/service/lib/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,11 @@ export class SavedObjectsErrorHelpers {
return isSavedObjectsClientError(error) && error[code] === CODE_GENERAL_ERROR;
}

public static createGenericNotFoundEsUnavailableError(type: string, id: string) {
public static createGenericNotFoundEsUnavailableError(
// type and id not available in all operations (e.g. mget)
type: string | null = null,
id: string | null = null
) {
const notFoundError = this.createGenericNotFoundError(type, id);
return this.decorateEsUnavailableError(
new Error(`${notFoundError.message}`),
Expand Down
Loading

0 comments on commit 6acf24a

Please sign in to comment.