CSHARP-5696: Integrate AWS SDK v4 in MongoDB.Driver.Authentication.AWS - #2064
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the MongoDB AWS authentication helper package to use AWS SDK for .NET v4 credential resolution, replacing deprecated v3 APIs and aligning the driver with the new SDK credential provider entry points.
Changes:
- Bumped
AWSSDK.SecurityTokendependency from v3 to v4. - Switched fallback credential resolution from
FallbackCredentialsFactorytoDefaultAWSCredentialsIdentityResolver, adding caching for the resolved provider. - Removed an AWS SDK handler-coverage test that depended on v3 internals.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/Communication/Security/AwsAuthenticationTests.cs | Removes a v3-specific credential provider chain test. |
| src/MongoDB.Driver.Authentication.AWS/MongoDB.Driver.Authentication.AWS.csproj | Updates the AWS SDK SecurityToken package reference to v4. |
| src/MongoDB.Driver.Authentication.AWS/CredentialsSources/AWSFallbackCredentialsSource.cs | Replaces v3 fallback factory with v4 identity resolver and adds provider caching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _lock.Wait(cancellationToken); | ||
| try | ||
| { | ||
| // returns cached credentials source immediately. Only if cached source unavailable, makes quite heavy steps | ||
| credentialsSource = FallbackCredentialsFactory.GetCredentials(); | ||
| // resolving the credentials source walks the full provider chain, so cache it and reuse until reset | ||
| credentialsSource = _cachedCredentialsSource ??= DefaultAWSCredentialsIdentityResolver.GetCredentials(null); |
| await _lock.WaitAsync(cancellationToken).ConfigureAwait(false); | ||
| try | ||
| { | ||
| // returns cached credentials source immediately. Only if cached source unavailable, makes quite heavy steps | ||
| credentialsSource = FallbackCredentialsFactory.GetCredentials(); | ||
| // resolving the credentials source walks the full provider chain, so cache it and reuse until reset | ||
| credentialsSource = _cachedCredentialsSource ??= await DefaultAWSCredentialsIdentityResolver.GetCredentialsAsync(null).ConfigureAwait(false); |
| [Fact] | ||
| public void Ecs_should_fill_AWS_CONTAINER_CREDENTIALS_RELATIVE_URI() | ||
| { | ||
| var isEcs = Environment.GetEnvironmentVariable("AWS_ECS_ENABLED") != null; | ||
| var awsContainerUri = Environment.GetEnvironmentVariable("AWS_CONTAINER_CREDENTIALS_RELATIVE_URI") ?? Environment.GetEnvironmentVariable("AWS_CONTAINER_CREDENTIALS_FULL_URI"); |
There was a problem hiding this comment.
So... I'm not sure we need this test. For a couple of reasons:
- We don't have those methods used here anymore.
FallbackCredentialsFactory.CredentialsGeneratorsis obsolete and its closest candidateAWSConfigs.AWSCredentialsGeneratorsis write only, so we can't read the default handlers - This test was essentially verifying that we get the "expected" chain of handlers to retrieve AWS credentials. Should we worry that the official SDK actually uses our "expected" chain of handlers?
- We actually verify that the correct credentials are chosen in our CI matrix with our
run-aws-auth-test-with-...tasks.
| var awsContainerUri = Environment.GetEnvironmentVariable("AWS_CONTAINER_CREDENTIALS_RELATIVE_URI") ?? Environment.GetEnvironmentVariable("AWS_CONTAINER_CREDENTIALS_FULL_URI"); | ||
| (awsContainerUri != null).Should().Be(isEcs); | ||
| } | ||
|
|
There was a problem hiding this comment.
I added the following as an answer to a Copilot comment, but I think it's worth to add it here as well.
So... I'm not sure we need this test. For a couple of reasons:
- We don't have those methods used here anymore.
FallbackCredentialsFactory.CredentialsGeneratorsis obsolete and its closest candidateAWSConfigs.AWSCredentialsGeneratorsis write only, so we can't read the default handlers. - This test was essentially verifying that we get the "expected" chain of handlers to retrieve AWS credentials. Should we worry that the official SDK actually uses our "expected" chain of handlers?
- We actually verify that the correct credentials are chosen in our CI matrix with our
run-aws-auth-test-with-...tasks.
There was a problem hiding this comment.
It seemed we were testing third-party library internals so I am supporting dropping the test. Unless there is a very good reason for doing so. @sanych-sun thoughts?
There was a problem hiding this comment.
@sanych-sun Do you agree with removing this test?
There was a problem hiding this comment.
I'm OK with dropping the test as long as there is no our code involved.
|
Assigned |
Co-authored-by: BossDarkReaper <8506424+BossDarkReaper@users.noreply.github.com>
adelinowona
left a comment
There was a problem hiding this comment.
You should also update the AGENTS.md files in the project for any reference to AWS SDK v3 stuff
| var awsContainerUri = Environment.GetEnvironmentVariable("AWS_CONTAINER_CREDENTIALS_RELATIVE_URI") ?? Environment.GetEnvironmentVariable("AWS_CONTAINER_CREDENTIALS_FULL_URI"); | ||
| (awsContainerUri != null).Should().Be(isEcs); | ||
| } | ||
|
|
There was a problem hiding this comment.
It seemed we were testing third-party library internals so I am supporting dropping the test. Unless there is a very good reason for doing so. @sanych-sun thoughts?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/MongoDB.Driver.Authentication.AWS/CredentialsSources/AWSFallbackCredentialsSource.cs:51
ResetCache()is now a no-op, but it’s still called fromAWSSaslMechanism.TryHandleAuthenticationExceptionto drop cached AWS credentials after an auth failure. With this implementation, the driver can no longer force a re-resolution/refresh on the next attempt.
Either wire ResetCache() to the AWS SDK v4 cache-invalidation mechanism (if available), or change the driver-side contract/callers so the method name and behavior stay consistent.
public void ResetCache()
{
// No-op: DefaultAWSCredentialsIdentityResolver owns credential caching and invalidates on
// environment/config changes.
}
src/MongoDB.Driver.Authentication.AWS/CredentialsSources/AWSFallbackCredentialsSource.cs:43
GetCredentialsAsyncaccepts aCancellationTokenbut the token is only checked once and isn’t propagated into the AWS SDK calls. If credential resolution or IMDS/ECS calls block, callers can’t cancel the in-flight operation.
If the AWS SDK v4 APIs expose overloads that accept a CancellationToken, prefer using those; otherwise consider documenting this limitation (or reworking the implementation so cancellation can interrupt the underlying work).
This issue also appears on line 47 of the same file.
public async Task<AWSCredentials> GetCredentialsAsync(CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();
var credentialsSource = await DefaultAWSCredentialsIdentityResolver.GetCredentialsAsync(null).ConfigureAwait(false);
var immutableCredentials = await credentialsSource.GetCredentialsAsync().ConfigureAwait(false);
| } | ||
|
|
||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| var credentialsSource = await DefaultAWSCredentialsIdentityResolver.GetCredentialsAsync(null).ConfigureAwait(false); |
There was a problem hiding this comment.
Also this probably doesn't have to block this PR any further (can be investigated in a separate ticket) but copilot had a comment about how our cancellationToken isn't passed to the AWS SDK. Upon inspecting the SDK code, it will use its own default cancellationToken of 35 seconds if we use the DefaultAWSCredentialsIdentityResolver.GetCredentials/GetCredentialsAsync methods. There is also ResolveIdentity(IClientConfig, CancellationToken) / ResolveIdentityAsync(...) methods which are instance methods that do take a cancellationToken so if we really wanted to fix this and honor our cancellationToken, we could create our own DefaultAWSCredentialsIdentityResolver instance and call ResolveIdentity(IClientConfig, CancellationToken) / ResolveIdentityAsync(...) so we can pass our cancellationToken.
Also worth investigating if IAWSCredentialsSource needs to be disposable since its dispose currently does nothing.
There was a problem hiding this comment.
I agree with this, I'll create a follow up ticket.
There was a problem hiding this comment.
https://jira.mongodb.org/browse/CSHARP-5696
Migrates
MongoDB.Driver.Authentication.AWSto AWS SDK for .NET v4 (AWSSDK.SecurityToken4.0.100.2), replacing the deprecatedFallbackCredentialsFactorywithDefaultAWSCredentialsIdentityResolver. Targeted for the next major release.Supersedes #2054 (from @BossDarkReaper).