Unmute TokenAuthIntegTests test#61714
Merged
jkakavas merged 3 commits intoelastic:masterfrom Sep 1, 2020
Merged
Conversation
@ywangd made an awesome analysis on why this test is failing, over at elastic#55816 (comment) This change makes it so that we use the same client to perform a refresh of a token, as we use to subsequently attempt to authenticate with the refreshed token. This ensures the tests are failing and is a good approximation of how we expect the same client doing the refresh, to also perform the subsequent authentication in real life uses. The errors we were seeing from users have dissappeared after elastic#55114 so we deem our behavior safe.
Collaborator
|
Pinging @elastic/es-security (:Security/Authentication) |
Contributor
Author
|
If we feel we should look into changing the behavior, we can do that and track this in another issue while we are still running the tests as is. |
ywangd
approved these changes
Sep 1, 2020
Member
ywangd
left a comment
There was a problem hiding this comment.
LGTM.
Just writing down my reasonings for the approval:
- For this edge case to happen, every request needs to be round-robined to different nodes, even when all requests are from the same client. This might be the case for Cloud where proxies are employed. But even then, this requires external facing network to work fast enough to beat shard sync among internal nodes. All these just sound unlikely to happen.
- Dirty read is a more general problem. It feels unjustified to fix it for a particular use case.
- No new failure has been reported after #55114.
So I am happy for us to refactor the test as the fix.
Contributor
Author
|
@elasticmachine update branch |
jkakavas
added a commit
to jkakavas/elasticsearch
that referenced
this pull request
Sep 3, 2020
This reverts commit 4886342. In master, we use the rest client which round robins between cluster nodes and thus the same client might refresh the token in a node and then immediately attempt to authenticate against another. Occasionally this still triggers the issue at hand.
jkakavas
added a commit
that referenced
this pull request
Sep 3, 2020
This reverts commit 4886342. In master, we use the rest client which round robins between cluster nodes and thus the same client might refresh the token in a node and then immediately attempt to authenticate against another. Occasionally this still triggers the issue at hand.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@ywangd made an awesome analysis on why this test is failing, over
at #55816 (comment)
This change makes it so that we use the same client to perform a
refresh of a token, as we use to subsequently attempt to authenticate
with the refreshed token. This ensures the tests are failing and is
a good approximation of how we expect the same client doing the
refresh, to also perform the subsequent authentication in real life
uses.
The errors we were seeing from users have disappeared after #55114
so we deem our behavior safe.