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

TinaHeiligers · 2021-07-28T23:52:54Z

Summary

Resolves #102353
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.

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

Initial comments to reviewers.

TinaHeiligers · 2021-07-29T00:09:27Z

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`


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-29T00:10:06Z

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

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


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

joshdover

This is looking great. I really only have feedback on which error we'd like to raise in this case and subsequently, the condition we should use to throw this error.

An integration test for this would be really nice to have but it's not going to be simple. We'd need to setup a proxy server in front of Elasticsearch that allows us to 'hi-jack' the response to test this condition. I imagine this would be easier to accomplish in a jest integration test than with the FTR. As a starting place, I'd experiment with using a custom Hapi server with the h2o2 plugin (same one we use for the bath path proxy). We can also do this as a follow up if needed.

joshdover · 2021-07-29T11:01:34Z

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

@@ -202,4 +202,23 @@ export class SavedObjectsErrorHelpers {
  public static isGeneralError(error: Error | DecoratedError) {
    return isSavedObjectsClientError(error) && error[code] === CODE_GENERAL_ERROR;
  }
+
+  public static createGenericNotFoundEsUnavailableError(


Do we want to differentiate this error from isEsUnavailableError? I'm inclined to say that we want applications to handle this condition in the same way they would handle any other issue reaching Elasticsearch. I'm not sure if there's anything different a consumer would or can do in this case vs. any other isEsUnavailableError. Another benefit of using the existing error is that any existing code that is handling it will also handle this new case.

Curious what @mshustov thinks as well.

Agree, it's ES server is unavailable rather than a Kibana entity is not found.
A user reacts to them in different ways: in the case of a Kibana entity is not found, the user needs to adjust the request params and try again.
While in the case of ES server is unavailable, the error is not actionable: the user needs to contact an admin to address the availability problem.

Initially, I did try throwing isEsUnavailableError but that returns a boolean, not an actual Error or DecoratedError. If you mean that we should rather throw a decorated EsUnavailableError that will return true on checking that the error is isEsUnavailable, then we can do that.
If I'm mis-reading the types here, please let me know!.

f you mean that we should rather throw a decorated EsUnavailableError that will return true on checking that the error is isEsUnavailable, then we can do that.

Yeah, I was thinking about something like this. @joshdover Did you mean the same?

I initially replied with this but then deleted it again. I'm adding it back for more context on the error that I'm using:

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.

joshdover · 2021-07-29T11:03:11Z

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

    const indexNotFound = statusCode === 404;
-
+    // check if we have the elasticsearch header when doc.found is not true (false, undefined or null) and if we do, ensure it is Elasticsearch
+    if (!isFoundGetResponse(body) && !isEsHeaderValid(headers)) {


If we decide to use the same isEsUnavailable error (see comment above), then I think we should simplify this condition to only if (!isEsHeaderValid(headers)) to catch this issue more broadly. Same for the other repository functions.

isEsUnavailable does not return an error. It returns a boolean.

In addition, what we're hoping to accomplish with this work is to differentiate between a saved object not actually being found (404) vs the response returning a 404 because ES is unavailable.

For the initial work (this PR), we're only scoping the work to checking if the response is from ES when we get a 404, rather than a larger check on all or any responses.

We will need to implement something for ensuring all responses received are valid ES responses in the long run but IMO, is too risky to try and get in for a patch release.

joshdover · 2021-07-29T11:04:23Z

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

+ * @returns boolean
+ */
+const isEsHeaderValid = (headers: Record<string, unknown> | null) => {
+  return headers && headers['x-elastic-product']


I'm fairly certain this is the case, but have we validated that the casing of the header names are always normalized to lowercase?

Good question! I did do a cursory check but we could handle the case by casting it to lowercase before checking the condition to be safe.

mshustov · 2021-07-29T12:07:59Z

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

+    return (
+      isSavedObjectsClientError(error) &&
+      error[code] === CODE_ES_UNAVAILABLE &&
+      error.message.startsWith('x-elastic-product not present or not recognized:')


Do we need : at the end?

Nope. Good catch!

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

mshustov · 2021-07-29T12:42:04Z

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

+    // check if we have the elasticsearch header when doc.found is not true (false, undefined or null) and if we do, ensure it is Elasticsearch
+    const esHeaderFound = isEsHeaderValid(headers);
+
+    if (!isFoundGetResponse(body) && !esHeaderFound) {


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?

Might it lead to false positives if an intermediate proxy doesn't forward x-elastic-product header?

If the header is missing from the response then yes, we would get 503 errors rather than 404 (which is what we're getting now). We would, however, still be getting an error because ATM, the header check is only done if we are already getting a 404 response.

If we did backport the work to 7.14.1, then there is a chance we'd get false positives but I don't know how likely that would be. We rely on headers for other Kibana functionality (e.g. accessing internal indices). In what cases wouldn't a proxy forward response headers on?

…rove-response-to-not-found

TinaHeiligers · 2021-07-29T21:45:24Z

An integration test for this would be really nice to have but it's not going to be simple. We'd need to setup a proxy server in front of Elasticsearch that allows us to 'hi-jack' the response to test this condition. I imagine this would be easier to accomplish in a jest integration test than with the FTR. As a starting place, I'd experiment with using a custom Hapi server with the h2o2 plugin (same one we use for the bath path proxy). We can also do this as a follow up if needed.

++ to adding a jest integration test. I also agree adding one as a follow up would be better because, as you say, it's not going to be simple.

TinaHeiligers · 2021-07-30T14:17:11Z

@mshustov @joshdover I've either addressed or answered your initial comments. Could you please take another look at the PR? I've kept the scope small for this one in the hope that we can get it in for the 7.14.1 patch.
@YulNaumenko Would this work for your case? The changes have been applied to get, update and delete.

I'm not sure what's up with all those doc changes. I'll investigate and potentially open a new PR if needed. Converting back to draft to figure this out. Sorry folks!

TinaHeiligers · 2021-07-30T14:37:01Z

@elasticmachine merge upstream

…found

kibanamachine · 2021-07-30T18:33:45Z

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`core`	2363	2368	+5

API count missing comments

id	before	after	diff
`core`	1081	1086	+5

History

💚 Build #141974 succeeded 159068b
💚 Build #141824 succeeded 9a33d69
💔 Build #141811 failed e1e93eb
💔 Build #141532 failed e1e93eb
💔 Build #141522 failed df0c75c278ec938bb645bbe98cede4ed786e84da

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

TinaHeiligers · 2021-07-30T20:04:47Z

@joshdover @mshustov @YulNaumenko I've opened a new PR without all those weird doc changes. Sorry about that!

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

TinaHeiligers requested a review from joshdover July 28, 2021 23:53

TinaHeiligers commented Jul 29, 2021

View reviewed changes

TinaHeiligers added 6 commits July 28, 2021 17:44

Implements check on response from get request

fb9eeed

Adds headers to createApiResponse

854d0b0

Adds header check to update

cf5cf75

Adds generic not found es unavailable error, updates unit tests

e0886b2

Adds header check to client.delete

2671098

Updates API docs to add new NotFoundEsUnavailable SO repo error

e1e93eb

TinaHeiligers force-pushed the so-errors/improve-response-to-not-found branch from df0c75c to e1e93eb Compare July 29, 2021 01:56

joshdover reviewed Jul 29, 2021

View reviewed changes

mshustov reviewed Jul 29, 2021

View reviewed changes

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

mshustov reviewed Jul 29, 2021

View reviewed changes

TinaHeiligers added 4 commits July 29, 2021 08:46

Merge branch 'master' into so-errors/improve-response-to-not-found

c7315cb

Rebuilds API docs

95cbf5a

Merge branch 'master' of github.com:elastic/kibana into so-errors/imp…

a408e43

…rove-response-to-not-found

implements case-insensitive header check

8aef363

TinaHeiligers mentioned this pull request Jul 29, 2021

Add integration test to ensure the SavedObjectsRepository handles responses correctly #107246

Closed

TinaHeiligers added 2 commits July 29, 2021 15:17

Merge branch 'master' into so-errors/improve-response-to-not-found

ce70c77

removes typescript ignore

9a33d69

TinaHeiligers marked this pull request as ready for review July 30, 2021 01:24

TinaHeiligers requested a review from a team as a code owner July 30, 2021 01:24

YulNaumenko self-requested a review July 30, 2021 08:26

TinaHeiligers marked this pull request as draft July 30, 2021 14:23

Merge branch 'master' into so-errors/improve-response-to-not-found

159068b

TinaHeiligers mentioned this pull request Jul 30, 2021

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

Merged

2 tasks

Merge branch 'elastic:master' into so-errors/improve-response-to-not-…

8d14a73

…found

TinaHeiligers closed this Jul 30, 2021

TinaHeiligers deleted the so-errors/improve-response-to-not-found branch October 14, 2021 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TinaHeiligers commented Jul 28, 2021 •

edited

Loading

TinaHeiligers left a comment

TinaHeiligers Jul 29, 2021

TinaHeiligers Jul 29, 2021

joshdover left a comment

joshdover Jul 29, 2021

mshustov Jul 29, 2021 •

edited

Loading

TinaHeiligers Jul 29, 2021

mshustov Jul 29, 2021

TinaHeiligers Jul 29, 2021

joshdover Jul 29, 2021

TinaHeiligers Jul 29, 2021

TinaHeiligers Jul 29, 2021 •

edited

Loading

joshdover Jul 29, 2021

TinaHeiligers Jul 29, 2021

mshustov Jul 29, 2021

TinaHeiligers Jul 29, 2021

mshustov Jul 29, 2021

TinaHeiligers Jul 29, 2021 •

edited

Loading

TinaHeiligers commented Jul 29, 2021 •

edited

Loading

TinaHeiligers commented Jul 30, 2021 •

edited

Loading

TinaHeiligers commented Jul 30, 2021

kibanamachine commented Jul 30, 2021

API count

API count missing comments

TinaHeiligers commented Jul 30, 2021

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

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

Conversation

TinaHeiligers commented Jul 28, 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

joshdover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Jul 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers Jul 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers commented Jul 29, 2021 • edited Loading

TinaHeiligers commented Jul 30, 2021 • edited Loading

TinaHeiligers commented Jul 30, 2021

kibanamachine commented Jul 30, 2021

💚 Build Succeeded

Metrics [docs]

API count

API count missing comments

History

TinaHeiligers commented Jul 30, 2021

TinaHeiligers commented Jul 28, 2021 •

edited

Loading

mshustov Jul 29, 2021 •

edited

Loading

TinaHeiligers Jul 29, 2021 •

edited

Loading

TinaHeiligers Jul 29, 2021 •

edited

Loading

TinaHeiligers commented Jul 29, 2021 •

edited

Loading

TinaHeiligers commented Jul 30, 2021 •

edited

Loading