Improve testing of STS credentials reloading#140114
Merged
DaveCTurner merged 6 commits intoelastic:mainfrom Jan 12, 2026
Merged
Improve testing of STS credentials reloading#140114DaveCTurner merged 6 commits intoelastic:mainfrom
DaveCTurner merged 6 commits intoelastic:mainfrom
Conversation
Today `CustomWebIdentityTokenCredentialsProviderTests` verifies that a S3 repository will reload credentials from STS, but fakes out the interactions with the environment variables and system properties via special test-only abstractions in the production code. This makes it hard to be confident that the real system behaves as expected. This commit replaces this suite with ones that verify the behaviour of a real Elasticsearch node, removing the need for the extra abstractions in the production code.
DaveCTurner
added
>test
Collaborator
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
elasticsearchmachine
added
the
Team:Distributed Coordination (obsolete)
Contributor
|
Is there third party test with real S3? |
Contributor
Author
|
No I don't think so, this is beyond what we have the tools to orchestrate in CI right now. |
mhl-b
reviewed
Jan 6, 2026
Comment on lines
118
to
129
| // doesn't matter if the current credentials all become invalid, because they're so close to expiry that the SDK is refreshing them | ||
| // (as confirmed by the success of the verify command) | ||
| dynamicCredentials.clearValidCredentials(); | ||
| assertVerifySuccess(repositoryName); | ||
|
|
||
| // if the refresh fails (incorrect token file contents) we keep on re-using the last good credentials | ||
| expectedWebIdentityTokenFileContents = randomAlphanumericOfLength(100); | ||
| assertVerifySuccess(repositoryName); | ||
|
|
||
| // if the last good credentials stop working then verification starts to fail | ||
| dynamicCredentials.clearValidCredentials(); | ||
| assertVerifyFailure(repositoryName); |
Contributor
There was a problem hiding this comment.
I don't get it. First you clear creds and make verify call to refresh them. How come after second clear creds call refresh stops working?
Contributor
There was a problem hiding this comment.
Oh wait, AWS client should refresh creds on the background, isn't it? Even clearing creds should issue a refresh on next s3 operation?
Contributor
Author
There was a problem hiding this comment.
Yes, it's refreshing these creds each time. The second one has an incorrect expectedWebIdentityTokenFileContents so the STS request fails (but the old creds remain valid)
Contributor
There was a problem hiding this comment.
I see now... expectedWebIdentityTokenFileContents is hooked into the fixture. It does look magical.
Do you mind to add another comment line saying how expectedWebIdentityTokenFileContents = randomAlphanumericOfLength(100) manipulates test?
mhl-b
approved these changes
Jan 6, 2026
Contributor
mhl-b
left a comment
mhl-b
left a comment
There was a problem hiding this comment.
LGTM, would be nice to have additional comment
Comment on lines
118
to
129
| // doesn't matter if the current credentials all become invalid, because they're so close to expiry that the SDK is refreshing them | ||
| // (as confirmed by the success of the verify command) | ||
| dynamicCredentials.clearValidCredentials(); | ||
| assertVerifySuccess(repositoryName); | ||
|
|
||
| // if the refresh fails (incorrect token file contents) we keep on re-using the last good credentials | ||
| expectedWebIdentityTokenFileContents = randomAlphanumericOfLength(100); | ||
| assertVerifySuccess(repositoryName); | ||
|
|
||
| // if the last good credentials stop working then verification starts to fail | ||
| dynamicCredentials.clearValidCredentials(); | ||
| assertVerifyFailure(repositoryName); |
Contributor
There was a problem hiding this comment.
I see now... expectedWebIdentityTokenFileContents is hooked into the fixture. It does look magical.
Do you mind to add another comment line saying how expectedWebIdentityTokenFileContents = randomAlphanumericOfLength(100) manipulates test?
szybia
added a commit
to szybia/elasticsearch
that referenced
this pull request
Jan 12, 2026
…i-project-tests * upstream/main: (23 commits) Fix `testAckListenerReceivesNacksIfPublicationTimesOut` (elastic#140514) Reduce priority of clear-cache tasks (elastic#139685) Add docs and tests about `StreamOutput` to memory (elastic#140365) ES|QL - dense_vector support for COUNT, PRESENT, ABSENT aggregator functions (elastic#139914) Add release notes for v9.2.4 release (elastic#140487) Add release notes for v9.1.10 release (elastic#140488) Add conncectors release notes for 9.1.10, 9.2.4 (elastic#140499) Add parameter support in PromQL query durations (elastic#139873) Improve testing of STS credentials reloading (elastic#140114) Fix zstd native binary publishing script to support newer versions (elastic#140485) Add FlattenedFieldBinaryVsSortedSetDocValuesSyntheticSourceIT (elastic#140489) Store fallback match only text fields in binary doc values (elastic#140189) [DiskBBQ] Use the new merge executor for intra-merge parallelism (elastic#139942) ESQL: introduce support for mapping-unavailable fields (elastic#140463) Add ESNextOSQVectorsScorerTests (elastic#140436) Disable high cardinality tests on release builds (elastic#140503) ESQL: TRange timezone support (elastic#139911) Directly compressing `StreamOutput` (elastic#140502) ES|QL - fix dense vector enrich bug (elastic#139774) Use CrossProjectModeDecider in RemoteClusterService (elastic#140481) ...
spinscale
pushed a commit
to spinscale/elasticsearch
that referenced
this pull request
Jan 21, 2026
Today `CustomWebIdentityTokenCredentialsProviderTests` verifies that a S3 repository will reload credentials from STS, but fakes out the interactions with the environment variables and system properties via special test-only abstractions in the production code. This makes it hard to be confident that the real system behaves as expected. This commit replaces this suite with ones that verify the behaviour of a real Elasticsearch node, removing the need for the extra abstractions in the production code.
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.
Today
CustomWebIdentityTokenCredentialsProviderTestsverifies that aS3 repository will reload credentials from STS, but fakes out the
interactions with the environment variables and system properties via
special test-only abstractions in the production code. This makes it
hard to be confident that the real system behaves as expected.
This commit replaces this suite with ones that verify the behaviour of a
real Elasticsearch node, removing the need for the extra abstractions in
the production code.