-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
preserve 401 errors from legacy es client #71234
preserve 401 errors from legacy es client #71234
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions:
- Should this be considered a fix? Should I label it
release_note:fix
instead ofskip
? - Do we want to backport this lower than
7.9
? - Do we want to add a FTR test for that (unsure how to trigger a
401
error from ES from an FTR test?)
const authenticationError = LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError( | ||
new (esErrors.AuthenticationException as any)('Authentication Exception', { | ||
body: { error: { header: { 'WWW-Authenticate': 'authenticate header' } } }, | ||
statusCode: 401, | ||
}) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈 but as the ClusterClient
is already mocked in the test suite, I couldn't 'demock' it to throw the error directly from the Client
itself. Also the behavior from the client is tested in cluster_client.test.ts
so I guess this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could throw this exception from a route handler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I could throw the same error directly from the handler instead of mocking the return of callAsCurrentUser
, but forging the error would still be as ugly.
I'd say yes. If someone encounters an error, they could find this bug fix in docs.
we backport bug fixes 2 versions back. we can backport at least to 7.8. I don't see any 7.7 releases scheduled. @joshdover
That would require a separate setup. The integration test that you added is sufficient. |
Yeah let's backport to 7.8 to catch the next patch release. Most likely will not make it for 7.8.1 though and I don't see any reason to block/delay that release for this fix due to the lack of complaints for this bug. |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/ml/data_frame_analytics/regression_creation·ts.machine learning data frame analytics regression creation electrical grid stability displays the include fields selectionStandard Out
Stack Trace
Build metrics
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I'm fine to keep it in master to avoid divergence, even considering we are going to deprecate this logic in 8.0
* preserve 401 errors from legacy es client * use exact import to resolve mocked import issue
* preserve 401 errors from legacy es client * use exact import to resolve mocked import issue # Conflicts: # src/core/server/elasticsearch/legacy/cluster_client.test.ts # src/core/server/http/integration_tests/core_service.test.mocks.ts
* preserve 401 errors from legacy es client (#71234) * preserve 401 errors from legacy es client * use exact import to resolve mocked import issue # Conflicts: # src/core/server/elasticsearch/legacy/cluster_client.test.ts # src/core/server/http/integration_tests/core_service.test.mocks.ts * adapt for 7.8 * use specific import to avoid cyclic deps
Summary
Forward 401 errors returned from the legacy es client directly instead of replacing them with 500 as it is done with every other errors during routing.
'Fix an issue causing
unauthorized
response (401) from elasticsearch to not being properly forwarded by Kibana, causing thewww-authenticate
header and 401 status code not being present in the http response.'Checklist