Adds new SavedObjectsRespository error type for 404 that do not originate from Elasticsearch responses #107301

TinaHeiligers · 2021-07-30T15:30:05Z

Summary

There may be cases where a 404 can be returned during calls to the Saved Objects Repository for reasons other than the saved object not being found.

The Saved Objects service should distinguish between a Saved Object is not found and other unknown reasons that do not originate from responses to an Elasticsearch call.

This PR adds an explicit 503 NotFoundEsUnavailable Error that will wrap 404 errors from get, update and delete requests when the response does not contain the x-elastic-product: Elasticsearch header.

As a follow up to this initial implementation, we need to:

Add general product check to all responses from the SO repository calls to ES Add general product check to all responses from the SO repository calls to ES. #107343.
Add an integration test that uses a proxy to mimic the behavior of the client when the response does not pass the product check.Add integration test to ensure the SavedObjectsRepository handles responses correctly #107246
Handle document not found and index not found response conditions consistently Saved Objects repository: only throw a 404 error when the document isn't found #107944

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

Risk Matrix

Risk	Probability	Severity	Mitigation/Notes
False positives— An intermediate proxy doesn't forward `x-elastic-product` header .	Low	Low	As of 7.15.0 Kibana won't start unless the required headers are present. For 7.14 patches, the type of error returned could be a 404 or a 503 but we will still get an error

For maintainers

This was checked for breaking API changes and was labeled appropriately

TinaHeiligers

Comments to reviewers.

TinaHeiligers · 2021-07-30T19:19:14Z

src/core/server/saved_objects/service/lib/errors.ts

+    const notFoundError = this.createGenericNotFoundError(type, id);
+    return this.decorateEsUnavailableError(
+      new Error(`${notFoundError.message}`),
+      `x-elastic-product not present or not recognized`


Copied self-review comment:

I'm not sure this is the best text string to use as a descriptor for the "missing" header. I took inspiration from the client's team but made it a lot more specific. We need to get Product's input on wording if we choose to follow this implementation.

TinaHeiligers · 2021-07-30T19:20:09Z

src/core/server/saved_objects/service/lib/errors.ts

+  public static isNotFoundEsUnavailableError(error: Error | DecoratedError) {
+    return (
+      isSavedObjectsClientError(error) &&
+      error[code] === CODE_ES_UNAVAILABLE &&


Copied self-review comment:

Specifically set a status code of 503 to indicate an ES availability error. This overrides the 404 we would otherwise have thrown.

TinaHeiligers · 2021-07-30T19:25:49Z

src/core/server/saved_objects/service/lib/repository.ts

+        throw SavedObjectsErrorHelpers.createGenericNotFoundError(type, id);
+      } else {
+        // throw if we can't verify the response is from Elasticsearch
+        throw SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError(type, id);


Self-review:
createGenericNotFoundEsUnavailableError will return a 503 error that adds the message from a NotFoundError to the EsUnavailableError. The decorated error that's thrown will assert true to a check for isEsUnavailableError, since the latter returns a boolean for the statusCode being 503.
If a saved object type and id are provided, the error will have the form:

{ message: 'x-elastic-product not present or not recognized: Saved object [foo/bar] not found', statusCode: 503 }

Otherwise, the error will have the form:

{ message: 'x-elastic-product not present or not recognized: Not Found', statusCode: 503 }

optional nit: I had to read createGenericNotFoundEsUnavailableError implementation to understand what error it will create. Maybe we should pass an error reason to decorateEsUnavailableError explicitly?

decorateEsUnavailableError(error, `Saved object [${type}/${id}] not found`);

// throw if we can't verify the response is from Elasticsearch

Will we add such a helper for every possible error response?

@joshdover which question did you give the thumbs up for in #107301 (comment)? i.e.

Maybe we should pass an error reason to decorateEsUnavailableError explicitly?

or

Will we add such a helper for every possible error response?

I'm not sure which way we want to go eventually, as long as we provide enough info to easily figure out the root cause of an error when it's thrown.

Maybe we should pass an error reason to decorateEsUnavailableError explicitly?

We would still need to provide an error to pass to decorateEsUnavailableError:

public static decorateEsUnavailableError(error: Error, reason?: string) { return decorate(error, CODE_ES_UNAVAILABLE, 503, reason); }

which is what we're doing in createGenericNotFoundEsUnavailableError.

TinaHeiligers

Code comments.

src/core/server/saved_objects/service/lib/repository.ts

TinaHeiligers · 2021-08-02T14:52:03Z

@elasticmachine merge upstream

…lable

YulNaumenko

LGTM! Awesome work! I've tested this locally by simulating situation with ES restart during the Kibana was still running - as a result all scheduled rules continue running after Kibana got back to the healthy state.
This PR should resolve this Alerting issue with no actions from the our side

joshdover · 2021-08-03T16:22:25Z

@mshustov regarding your comment regarding using this header before our version check uses it:

Question: Might it lead to false positives if an intermediate proxy doesn't forward x-elastic-product header? In v7.14, we do allow Kibana starts even if x-elastic-product header is missing. Kibana won't start unless the header presents starting from v7.15 #105557
Does it mean that we cannot backport this change to v7.14.1?

In addition to @TinaHeiligers's note that this check only applies to 404, it also seems quite possible that we will have #105557 fixed for the 7.14.1 release which makes me less concerned about this. Though maybe we should consider delaying a backport until that change is also backported.

TinaHeiligers · 2021-08-03T16:25:03Z

Though maybe we should consider delaying a backport until that change is also backported

@joshdover do you mean the 7.14.1 backport or both backports to 7.14.1 and 7.x (7.15)?

mshustov · 2021-08-03T16:42:47Z

ack: going to review tomorrow (04 Aug.)

src/core/server/elasticsearch/client/mocks.ts

joshdover · 2021-08-03T16:30:16Z

src/core/server/saved_objects/service/lib/errors.ts

+    );
+  }
+
+  public static isNotFoundEsUnavailableError(error: Error | DecoratedError) {


Do we need this API? If we want plugins to treat this error as any other ES not found issue, I think it'd be ok to only include the factory function createGenericNotFoundEsUnavailableError so that plugins don't waste time trying to figure out how they should handle this error in a different way.

I'm actually using the API within the repository to verify error types here. It also ensures we stay consistent with repository error type checks, such as isNotFoundError, with the additional check on the error reason.

isNotFoundEsUnavailableError isn't an error itself, it returns a boolean.

Right, but as a plugin developer, I'd want to handle each error exposed on SavedObjectsErrorHelpers.is* in order to make my code bulletproof. I'd expect that each of these is*Error methods to only correspond to one error type and not overlap, but now that is no longer the case since both isEsUnavailableError and isNotFoundEsUnavailableError could return true on the same error. I'd also expect that each error type would correspond to something different I could do to handle this, however the underlying problem for both of these errors is the same and there's really nothing different I can/should do as a developer for the isNotFoundEsUnavailableError case vs the isEsUnavailable one.

Are you planning to add similar new error guards for each method as part of #107343? Personally, I think we should only expose isEsUnavailable for all of these situations to signal to plugins that they should handle this error in the same way for every method.

Personally, I think we should only expose isEsUnavailable for all of these situations to signal to plugins that they should handle this error in the same way for every method.

That makes sense. I've removed isNotFoundEsUnavailableError.

joshdover · 2021-08-03T16:32:51Z

src/core/server/saved_objects/service/lib/repository.test.js

+          elasticsearchClientMock.createSuccessTransportRequestPromise(
+            { found: false },
+            undefined,
+            { 'x-elastic-product': 'Elasticsearch' }


Maybe we should include this header as the default in elasticsearchClientMock.createSuccessTransportRequestPromise() (or even better might be in the createApiResponse function that this function calls) and only override it when needed for these tests. That would better represent the expected typical conditions in our mocks rather than the exceptional case.

I'd prefer to handle that in the follow up issue (that I'm working on right now anyway), as it's going to become the typical conditions when we add the checks to the rest of the SO repository methods.

src/core/server/saved_objects/service/lib/repository.ts

joshdover · 2021-08-03T16:40:00Z

src/core/server/saved_objects/service/lib/repository.ts

+      .then((res) => {
+        const indexNotFound = res.statusCode === 404;
+        const esServerSupported = isSupportedEsServer(res.headers);
+        // check if we have the elasticsearch header when doc.found is not true (false, undefined or null) and if we do, ensure it is Elasticsearchq


Suggested change

// check if we have the elasticsearch header when doc.found is not true (false, undefined or null) and if we do, ensure it is Elasticsearchq

// check if we have the elasticsearch header when index is not found and if we do, ensure it is Elasticsearch

joshdover · 2021-08-03T16:47:55Z

do you mean the 7.14.1 backport or both backports to 7.14.1 and 7.x (7.15)?

I just mean delaying the backport to 7.14.1

TinaHeiligers · 2021-08-03T20:47:55Z

src/core/server/elasticsearch/client/mocks.ts

@@ -141,9 +141,10 @@ export type MockedTransportRequestPromise<T> = TransportRequestPromise<T> & {

 const createSuccessTransportRequestPromise = <T>(
  body: T,
-  { statusCode = 200 }: { statusCode?: number } = {}
+  { statusCode = 200 }: { statusCode?: number } = {},
+  headers?: Record<string, any>


@joshdover thanks for pointing the type issue out!

nit:

headers?: Record<string, string | string[]>

see

kibana/src/core/server/http/router/headers.ts

Lines 40 to 41 in b20e9c2

export type Headers = { [header in KnownHeaders]?: string | string[] | undefined } & {

[header: string]: string | string[] | undefined;

TinaHeiligers · 2021-08-09T22:26:50Z

jenkins test this please

TinaHeiligers · 2021-08-09T23:43:31Z

@elasticmachine merge upstream

…lable

TinaHeiligers · 2021-08-10T00:48:26Z

Summary of PR status:
Initial implementation of extra check to make sure we get a response from elasticsearch before throwing 404’s. This handles the immediate need for the Alerting team, where they need to make sure we get a response from elasticsearch before deleting an alert that isn’t found.

@dover @mshustov @rudolf I’ve created the following issues to follow up on from this work:

Implement similar checks in all the methods (Add general product check to all responses from the SO repository calls to ES. #107343)
Add integration test as a smoke screen to make sure the behavior is as expected. (Add integration test to ensure the SavedObjectsRepository handles responses correctly #107246)
Improve on the way we interpret 404 responses and change to throwing 500 errors when the index isn’t found. (Saved Objects repository: only throw a 404 error when the document isn't found #107944)

Please let me know if I’ve missed something or if there are remaining issues to tackle before moving forward with the PR.

I’m hoping we can a fix in before 7.15 FF but, depending on priorities, don't know if there's enough time to get through all the follow up issues before FF.

kibanamachine · 2021-08-10T02:03:19Z

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`core`	2455	2458	+3

API count missing comments

id	before	after	diff
`core`	1166	1169	+3

History

💔 Build #143899 failed 2af49d3
💚 Build #143817 succeeded 2af49d3
💚 Build #143530 succeeded 9cbbb57
💚 Build #143334 succeeded 81d7571
💔 Build #143299 failed 35cf9d0

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

TinaHeiligers · 2021-08-10T13:16:48Z

~~Note to self: backport manually to 7.14.1.~~
Update: #107663 only landed in 7.16 and we decided not to backport as far back as 7.14.1.
The changes are in for 7.15, which is already a minor ahead of the Kibana-wide startup product check.

…nate from Elasticsearch responses (elastic#107301) Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-08-10T13:19:54Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…nate from Elasticsearch responses (#107301) (#108037) Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Christiane (Tina) Heiligers <[email protected]>

ymao1 · 2021-09-23T18:16:03Z

@TinaHeiligers Was this backported to 7.14? The tag says 7.14 but I can't find a backport PR. Wondering because we are seeing a saved object not found error for a rule in 7.14.1 in a cluster that seems to have some proxy connectivity issues.

TinaHeiligers · 2021-09-23T20:56:58Z

@ymao1 no, it wasn't backported to 7.14.1. To follow up on this, 7.15 was around the corner, and there was still work that needed to be done to wrap it up.
We also needed to wait for #107663 to land, which was only backported to 7.16.

TinaHeiligers added 2 commits July 30, 2021 08:08

Adds generic not found ES unavailable error, updates ES client mock

81d435d

Adds response header check

98a785f

TinaHeiligers added release_note:enhancement Feature:Saved Objects v7.15.0 v7.14.1 labels Jul 30, 2021

TinaHeiligers added 2 commits July 30, 2021 08:49

clean up

7c52b54

Updates server API docs to include new error type

e173b77

TinaHeiligers commented Jul 30, 2021

View reviewed changes

Cleans up commented out code

5614cda

TinaHeiligers commented Jul 30, 2021

View reviewed changes

src/core/server/saved_objects/service/lib/repository.ts Outdated Show resolved Hide resolved

TinaHeiligers mentioned this pull request Jul 30, 2021

Adds explicit SavedObjectsRespository error type for 404 that do not originate from Elasticsearch responses #107104

Closed

2 tasks

TinaHeiligers marked this pull request as ready for review July 30, 2021 22:26

TinaHeiligers requested a review from a team as a code owner July 30, 2021 22:26

TinaHeiligers requested review from joshdover, YulNaumenko and mshustov July 30, 2021 22:26

TinaHeiligers mentioned this pull request Jul 31, 2021

Add general product check to all responses from the SO repository calls to ES. #107343

Closed

Merge branch 'master' into so-errors/identify-not-found-form-esunavai…

7d00fdd

…lable

YulNaumenko approved these changes Aug 2, 2021

View reviewed changes

joshdover reviewed Aug 3, 2021

View reviewed changes

TinaHeiligers added 3 commits August 3, 2021 10:12

removes case sensititivy check on headers

e45920d

Fixes code comment

449dda6

Improves elasticsearch client mocks types

b20e9c2

TinaHeiligers commented Aug 3, 2021

View reviewed changes

Merge branch 'master' into so-errors/identify-not-found-form-esunavai…

2f76296

…lable

mshustov approved these changes Aug 10, 2021

View reviewed changes

TinaHeiligers added auto-backport Deprecated - use backport:version if exact versions are needed v7.14.1 and removed v7.14.1 auto-backport Deprecated - use backport:version if exact versions are needed labels Aug 10, 2021

TinaHeiligers merged commit cdf90aa into elastic:master Aug 10, 2021

TinaHeiligers deleted the so-errors/identify-not-found-form-esunavailable branch August 10, 2021 13:16

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 10, 2021

Adds new SavedObjectsRespository error type for 404 that do not origi…

0fca204

…nate from Elasticsearch responses (elastic#107301) Co-authored-by: Kibana Machine <[email protected]>

kibanamachine mentioned this pull request Aug 10, 2021

[7.x] Adds new SavedObjectsRespository error type for 404 that do not originate from Elasticsearch responses (#107301) #108037

Merged

TinaHeiligers added v7.14.1 and removed auto-backport Deprecated - use backport:version if exact versions are needed labels Aug 10, 2021

YulNaumenko mentioned this pull request Aug 10, 2021

[Alerting] Should we retry alerting tasks that fail with Saved object not found errors #100764

Closed

TinaHeiligers mentioned this pull request Aug 10, 2021

Gracefully handle elasticsearch client version check #105557

Closed

TinaHeiligers mentioned this pull request Aug 16, 2021

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

Merged

2 tasks

This was referenced Aug 24, 2021

[SavedObjects] Integration tests for unsupported product 404 responses #109755

Merged

Fixes bugs for product check in update and delete #109979

Merged

TinaHeiligers removed the v7.14.1 label Sep 23, 2021

TinaHeiligers mentioned this pull request Oct 22, 2021

Removes custom header check on 404 responses, includes es client ProductNotSupportedError in EsUnavailableError conditional #116029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds new SavedObjectsRespository error type for 404 that do not originate from Elasticsearch responses #107301

Adds new SavedObjectsRespository error type for 404 that do not originate from Elasticsearch responses #107301

TinaHeiligers commented Jul 30, 2021 •

edited

Loading

TinaHeiligers left a comment

TinaHeiligers Jul 30, 2021

TinaHeiligers Jul 30, 2021

TinaHeiligers Jul 30, 2021

mshustov Aug 4, 2021

TinaHeiligers Aug 4, 2021

TinaHeiligers Aug 8, 2021

TinaHeiligers left a comment

TinaHeiligers commented Aug 2, 2021

YulNaumenko left a comment

joshdover commented Aug 3, 2021

TinaHeiligers commented Aug 3, 2021

mshustov commented Aug 3, 2021

joshdover Aug 3, 2021

TinaHeiligers Aug 3, 2021 •

edited

Loading

joshdover Aug 9, 2021

TinaHeiligers Aug 9, 2021 •

edited

Loading

joshdover Aug 3, 2021

TinaHeiligers Aug 3, 2021

joshdover Aug 3, 2021

joshdover commented Aug 3, 2021

TinaHeiligers Aug 3, 2021

mshustov Aug 4, 2021

TinaHeiligers Aug 4, 2021

TinaHeiligers commented Aug 9, 2021

TinaHeiligers commented Aug 9, 2021

TinaHeiligers commented Aug 10, 2021

kibanamachine commented Aug 10, 2021

API count

API count missing comments

TinaHeiligers commented Aug 10, 2021 •

edited

Loading

kibanamachine commented Aug 10, 2021

ymao1 commented Sep 23, 2021

TinaHeiligers commented Sep 23, 2021 •

edited

Loading

	// check if we have the elasticsearch header when doc.found is not true (false, undefined or null) and if we do, ensure it is Elasticsearchq
	// check if we have the elasticsearch header when index is not found and if we do, ensure it is Elasticsearch

	export type Headers = { [header in KnownHeaders]?: string \| string[] \| undefined } & {
	[header: string]: string \| string[] \| undefined;

Adds new SavedObjectsRespository error type for 404 that do not originate from Elasticsearch responses #107301

Adds new SavedObjectsRespository error type for 404 that do not originate from Elasticsearch responses #107301

Conversation

TinaHeiligers commented Jul 30, 2021 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

TinaHeiligers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Aug 2, 2021

YulNaumenko left a comment

Choose a reason for hiding this comment

joshdover commented Aug 3, 2021

TinaHeiligers commented Aug 3, 2021

mshustov commented Aug 3, 2021

Choose a reason for hiding this comment

TinaHeiligers Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdover commented Aug 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers commented Aug 9, 2021

TinaHeiligers commented Aug 9, 2021

TinaHeiligers commented Aug 10, 2021

kibanamachine commented Aug 10, 2021

💚 Build Succeeded

Metrics [docs]

API count

API count missing comments

History

TinaHeiligers commented Aug 10, 2021 • edited Loading

kibanamachine commented Aug 10, 2021

💚 Backport successful

ymao1 commented Sep 23, 2021

TinaHeiligers commented Sep 23, 2021 • edited Loading

TinaHeiligers commented Jul 30, 2021 •

edited

Loading

TinaHeiligers Aug 3, 2021 •

edited

Loading

TinaHeiligers Aug 9, 2021 •

edited

Loading

TinaHeiligers commented Aug 10, 2021 •

edited

Loading

TinaHeiligers commented Sep 23, 2021 •

edited

Loading