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

Improves not found response handling in the saved objects repository #108749

4 changes: 2 additions & 2 deletions src/core/server/elasticsearch/client/mocks.ts
Original file line number Diff line number Diff line change
@@ -142,7 +142,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[]> = { 'x-elastic-product': 'Elasticsearch' }
): MockedTransportRequestPromise<ApiResponse<T>> => {
const response = createApiResponse({ body, statusCode, headers });
const promise = Promise.resolve(response);
@@ -163,7 +163,7 @@ function createApiResponse<TResponse = Record<string, any>>(
return {
body: {} as any,
statusCode: 200,
headers: {},
headers: { 'x-elastic-product': 'Elasticsearch' },
lukeelmers marked this conversation as resolved.
Show resolved Hide resolved
warnings: [],
meta: {} as any,
...opts,
Original file line number Diff line number Diff line change
@@ -12,6 +12,6 @@ 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';
};
Original file line number Diff line number Diff line change
@@ -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 };
@@ -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 () => {
Original file line number Diff line number Diff line change
@@ -7,13 +7,17 @@
*/

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

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 {
getSavedObjectFromSource,
rawDocExistsInNamespace,
isNotFoundFromUnsupportedServer,
} from './internal_utils';
import type {
ISavedObjectsPointInTimeFinder,
SavedObjectsCreatePointInTimeFinderOptions,
@@ -198,6 +202,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
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
@@ -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 availablefor all operations (e.g. mget)
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
type: string | null = null,
id: string | null = null
) {
const notFoundError = this.createGenericNotFoundError(type, id);
return this.decorateEsUnavailableError(
new Error(`${notFoundError.message}`),
33 changes: 33 additions & 0 deletions src/core/server/saved_objects/service/lib/internal_utils.test.ts
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@ import {
getBulkOperationError,
getSavedObjectFromSource,
rawDocExistsInNamespace,
isNotFoundFromUnsupportedServer,
} from './internal_utils';
import { ALL_NAMESPACES_STRING } from './utils';

@@ -241,3 +242,35 @@ describe('#rawDocExistsInNamespace', () => {
});
});
});

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

const result = isNotFoundFromUnsupportedServer(rawResponse);
expect(result).toBeTruthy();
lukeelmers marked this conversation as resolved.
Show resolved Hide resolved
});

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).toBeFalsy();
});

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

const result = isNotFoundFromUnsupportedServer(rawResponse);
expect(result).toBeFalsy();
});
});
17 changes: 17 additions & 0 deletions src/core/server/saved_objects/service/lib/internal_utils.ts
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@ import type { SavedObject } from '../../types';
import { decodeRequestVersion, encodeHitVersion } from '../../version';
import { SavedObjectsErrorHelpers } from './errors';
import { ALL_NAMESPACES_STRING, SavedObjectsUtils } from './utils';
import { isSupportedEsServer } from '../../../elasticsearch';

/**
* Checks the raw response of a bulk operation and returns an error if necessary.
@@ -141,3 +142,19 @@ export function rawDocExistsInNamespace(
namespaces?.includes(ALL_NAMESPACES_STRING);
return existsInNamespace ?? false;
}

/**
* 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: {
Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Aug 16, 2021

Choose a reason for hiding this comment

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

I'm in two minds about where this helper should go and considered moving it to the elasticsearch service but went with internal_utils as we're really only using it within the SO service right now.

I'm also open to suggestions for the name (naming is hard...)!

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I don't have strong feelings one way or the other on whether this is internal or in the ES service. If it was used anywhere else I'd say put it in the ES service though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the method since we're both thinking it should belong to the elasticsearch service. While ATM it's only used in the SO service, other folks might have to implement something similar.

statusCode: number | null;
headers: Record<string, string | string[]> | null;
}): boolean => {
return args.statusCode === 404 && !isSupportedEsServer(args.headers);
};
Loading