-
Notifications
You must be signed in to change notification settings - Fork 872
[V4] Re-introduce background refresh behavior #4100
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
base: development
Are you sure you want to change the base?
Conversation
sdk/src/Core/Amazon.Runtime/Credentials/InstanceProfileAWSCredentials.cs
Show resolved
Hide resolved
sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs
Show resolved
Hide resolved
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.
Pull Request Overview
This PR re-introduces background refresh of credentials during their preempt expiry period. The key changes include:
- Introduction of a new
ITimeProviderinterface for improved testability of time-dependent logic - Addition of an
ExpirationBufferproperty to provide a buffer for credential expiration - Refactored credential refresh logic to support background refresh during the preempt expiry window
- Comprehensive unit tests for concurrent credential access and background refresh scenarios
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/src/Core/Amazon.Util/Internal/ITimeProvider.cs | Adds new interface and default implementation for time provision to enable testability |
| sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs | Refactors credential refresh logic with background refresh, ExpirationBuffer property, and ITimeProvider integration |
| sdk/src/Core/Amazon.Runtime/Credentials/InstanceProfileAWSCredentials.cs | Updates to use _timeProvider instead of AWSSDKUtils.CorrectedUtcNow |
| sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs | Adds comprehensive unit tests for concurrent access and background refresh behavior |
| sdk/src/Core/GlobalSuppressions.cs | Updates suppression rules for new protected field |
| generator/.DevConfigs/e11928c9-15fb-4b9e-91d3-91775150d378.json | Adds dev config marking this as a minor version change |
Comments suppressed due to low confidence (3)
sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs:1
- This constructor call is missing the _timeProvider parameter. It should be 'new CredentialsRefreshState(state.Credentials, newExpiryTime, _timeProvider)' to ensure the time provider is properly propagated throughout the credential refresh state lifecycle.
/*
sdk/src/Core/Amazon.Runtime/Credentials/InstanceProfileAWSCredentials.cs:420
- This CredentialsRefreshState constructor call is missing the _timeProvider parameter. It should include '_timeProvider' as the third parameter to ensure consistent time provider usage throughout the credential lifecycle.
new CredentialsRefreshState(
new ImmutableCredentials(
credentials.AccessKeyId,
credentials.SecretAccessKey,
credentials.Token),
credentials.Expiration);
sdk/src/Core/Amazon.Runtime/Credentials/InstanceProfileAWSCredentials.cs:443
- This CredentialsRefreshState constructor call is missing the _timeProvider parameter. It should include '_timeProvider' as the third parameter to ensure consistent time provider usage throughout the credential lifecycle.
new CredentialsRefreshState(
new ImmutableCredentials(
credentials.AccessKeyId,
credentials.SecretAccessKey,
credentials.Token),
credentials.Expiration);
sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs
Outdated
Show resolved
Hide resolved
sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs
Outdated
Show resolved
Hide resolved
sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs
Outdated
Show resolved
Hide resolved
sdk/test/NetStandard/UnitTests/Core/Credentials/RefreshingAWSCredentialsTests.cs
Outdated
Show resolved
Hide resolved
sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs
Outdated
Show resolved
Hide resolved
sdk/src/Core/Amazon.Runtime/Credentials/RefreshingAWSCredentials.cs
Outdated
Show resolved
Hide resolved
sdk/src/Core/Amazon.Runtime/Credentials/InstanceProfileAWSCredentials.cs
Show resolved
Hide resolved
sdk/src/Core/Amazon.Runtime/Credentials/InstanceProfileAWSCredentials.cs
Show resolved
Hide resolved
sdk/src/Core/Amazon.Runtime/Credentials/InstanceProfileAWSCredentials.cs
Show resolved
Hide resolved
sdk/src/Core/Amazon.Runtime/Credentials/InstanceProfileAWSCredentials.cs
Show resolved
Hide resolved
| _logger.Error(e, "Exception occurred performing background credentials refresh."); | ||
| // If any exceptions occur during background refresh, reset the state to NotLoading | ||
| // so that future GetCredentials calls can attempt to refresh again. | ||
| currentLoadState = CredentialsLoadState.NotLoading; |
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.
This is setting currentLoadState while it is outside of the lock. I have not yet traced under what case this could cause an issue. However, it should only be set within the lock and should be moved to the finally on line 245 and remove the one one 243.
I also see that maybe you did this because ValidateGeneratedCredentials(newState); could throw an exception which would leave currentLoadState as "Loading" potentially if it is moved.
Unsure how to fix this yet but it is still potentially unsafe to set the variable outside of the lock.
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.
My thinking was I need the lock to control the number of threads performing the refresh action or when I needed to change both currentLoadState and currentState atomically. In this case I only need to change the currentLoadState property and have marked that field is volatile to avoid compiler optimizations that would cause problems when multiple threads read and set variable.
Resolves #4024
Description
This PR is the second attempt to update credentials to expire during their preempt expiry period. The approach used is similar to the previous behavior (#3541) but with some important differences:
CredentialsLoadStateenum to guarantee only one background refresh runs at a timeMotivation and Context
DOTNET-8326(and a couple of internal support tickets too)Testing
Dry-runs (queued):
DRY_RUN-c85e29cd-d738-450d-8c5b-295646a0aa7dDRY_RUN-fbab3c8d-b969-441b-9523-fcfdc7eb1935In addition to the unit tests I added back, I also ran a couple of simple console apps locally (calling
GetCredentialson a loop withAssumeRoleAWSCredentials):Types of changes
Checklist
License