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

Conversation

TinaHeiligers
Copy link
Contributor

Resolves #107343

This PR is to address the follow up on #107301, and verifies that 404 responses are from ES in the remaining saved object repository APIs where we explicitly ignore 404 and handle the error ourselves.
If the check fails, we throw a decorated EsUnavailableError instead of Not Found.

Checklist

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

* @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.

@lukeelmers lukeelmers self-requested a review August 16, 2021 19:01
@TinaHeiligers TinaHeiligers marked this pull request as ready for review August 16, 2021 21:21
@TinaHeiligers TinaHeiligers requested a review from a team as a code owner August 16, 2021 21:21
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Overall makes sense! Couple thoughts & questions.

* @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
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.

src/core/server/elasticsearch/client/mocks.ts Outdated Show resolved Hide resolved
elasticsearchClientMock.createSuccessTransportRequestPromise(
{ ...response },
{ statusCode: 404 },
{}
Copy link
Member

Choose a reason for hiding this comment

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

nit: If we aren't including the ES product header here, might be worth renaming this function for clarity so that it is obvious it is specific to only one type of error?

Or I guess alternatively you could attempt to make it more generic by, say, adding an expectedErrorResult as in bulkCreateError and conditionally exluding the header if the error is a genericNotFoundEsUnavailableError

Same nit applies to bulkGetMgetError and bulkUpdateMgetError

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.

might be worth renaming this function for clarity so that it is obvious it is specific to only one type of error

I went with the renaming option (e.g. https://github.com/elastic/kibana/pull/108749/files#diff-8a58e602e79e19512cd392387f38a79ba9782f60d0fc1596a90d5e9e18efb6bdR655) to make it super clear!

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

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Updates LGTM

@TinaHeiligers TinaHeiligers added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 17, 2021
@TinaHeiligers TinaHeiligers enabled auto-merge (squash) August 17, 2021 17:03
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@TinaHeiligers TinaHeiligers merged commit 3a74287 into elastic:master Aug 17, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x
7.16 The branch "7.16" is invalid or doesn't exist

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 108749

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Saved Objects release_note:enhancement v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add general product check to all responses from the SO repository calls to ES.
3 participants