-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-5696: Integrate AWS SDK v4 in MongoDB.Driver.Authentication.AWS #2064
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
Changes from all commits
3c7211a
7fa6e8b
6623348
e709b97
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,8 +14,6 @@ | |
| */ | ||
|
|
||
| using System; | ||
| using Amazon.Runtime; | ||
| using Amazon.Runtime.CredentialManagement; | ||
| using FluentAssertions; | ||
| using MongoDB.Bson; | ||
| using MongoDB.TestHelpers.XunitExtensions; | ||
|
|
@@ -64,105 +62,5 @@ public void Ecs_should_fill_AWS_CONTAINER_CREDENTIALS_RELATIVE_URI() | |
| var awsContainerUri = Environment.GetEnvironmentVariable("AWS_CONTAINER_CREDENTIALS_RELATIVE_URI") ?? Environment.GetEnvironmentVariable("AWS_CONTAINER_CREDENTIALS_FULL_URI"); | ||
| (awsContainerUri != null).Should().Be(isEcs); | ||
| } | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sanych-sun Do you agree with removing this test?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm OK with dropping the test as long as there is no our code involved. |
||
| [Fact] | ||
| public void AwsSdk_should_support_all_required_handlers() | ||
| { | ||
| var credentialsGeneratorsDelegatesEnumerator = FallbackCredentialsFactory.CredentialsGenerators.GetEnumerator(); | ||
|
|
||
| // AppConfigAWSCredentials | ||
| AWSCredentials credentials = null; | ||
| if (Type.GetType("Amazon.Runtime.AppConfigAWSCredentials, AWSSDK.Core", throwOnError: false) != null) // app.config/web.config does not present on windows | ||
| { | ||
| var appConfigAWSCredentialsException = Record.Exception(() => RunTestCase()); | ||
| // app.config/web.config case is based on ConfigurationManager. This is not configured for this test | ||
| appConfigAWSCredentialsException.Message.Should().Contain("The app.config/web.config files for the application did not contain credential information"); | ||
| } | ||
|
|
||
| // AssumeRoleWithWebIdentityCredentials.FromEnvironmentVariables() | ||
| var exception = Record.Exception(() => RunTestCase()); | ||
| if (Environment.GetEnvironmentVariable("AWS_WEB_IDENTITY_TOKEN_FILE") != null) | ||
| { | ||
| // aws-web-identity-credentials is configured | ||
| exception.Should().BeNull(); | ||
| credentials.Should().BeOfType<AssumeRoleWithWebIdentityCredentials>(); | ||
| } | ||
| else | ||
| { | ||
| // otherwise fail | ||
| exception.Message.Should().Contain("webIdentityTokenFile"); | ||
| } | ||
|
|
||
| // GetAWSCredentials (Profile) | ||
| exception = Record.Exception(() => RunTestCase()); | ||
| if (IsWithAwsProfileOnMachine()) | ||
| { | ||
| // current machine contains configured aws profile, which may include: | ||
| // 1. BasicAWSCredentials (aws_access_key_id and aws_secret_access_key) | ||
| // 2. SessionAWSCredentials (aws_access_key_id, aws_secret_access_key, aws_session_token) | ||
| exception.Should().BeNull(); | ||
| credentials.Should().Match(x => x is BasicAWSCredentials || x is SessionAWSCredentials); | ||
| } | ||
| else | ||
| { | ||
| // otherwise fail | ||
| exception.Message.Should().Contain("Credential").And.Subject.Should().Contain("profile"); | ||
| } | ||
|
|
||
| // EnvironmentVariablesAWSCredentials | ||
| exception = Record.Exception(() => RunTestCase()); | ||
| if (Environment.GetEnvironmentVariable("AWS_ACCESS_KEY_ID") != null && Environment.GetEnvironmentVariable("AWS_SECRET_ACCESS_KEY") != null) | ||
| { | ||
| // environment variables code path | ||
| exception.Should().BeNull(); | ||
| credentials.Should().BeOfType<EnvironmentVariablesAWSCredentials>(); | ||
| } | ||
| else | ||
| { | ||
| // otherwise fail | ||
| exception.Message.Should().Contain("The environment variables").And.Subject.Contains("were not set with AWS credentials"); | ||
| } | ||
|
|
||
| // ECSEC2CredentialsWrapper | ||
| exception = Record.Exception(() => RunTestCase()); | ||
| if (Environment.GetEnvironmentVariable("AWS_CONTAINER_CREDENTIALS_RELATIVE_URI") != null || Environment.GetEnvironmentVariable("AWS_CONTAINER_CREDENTIALS_FULL_URI") != null) | ||
| { | ||
| exception.Should().BeNull(); | ||
| credentials.Should().BeOfType<ECSTaskCredentials>(); | ||
| } | ||
| else | ||
| { | ||
| exception.Should().BeNull(); | ||
| credentials.GetType().Name.Should().Contain("DefaultInstanceProfileAWSCredentials"); // EC2 case | ||
| } | ||
|
|
||
| credentialsGeneratorsDelegatesEnumerator.MoveNext().Should().BeFalse(); // no more handlers | ||
|
|
||
| bool IsWithAwsProfileOnMachine() | ||
| { | ||
| var credentialProfileChain = new CredentialProfileStoreChain(); | ||
| if (credentialProfileChain.TryGetProfile(Environment.GetEnvironmentVariable("AWS_PROFILE") ?? "default", out var profile)) | ||
| { | ||
| try | ||
| { | ||
| _ = profile.GetAWSCredentials(credentialProfileChain); | ||
| return true; | ||
| } | ||
| catch | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| void RunTestCase() | ||
| { | ||
| credentials = null; | ||
| credentialsGeneratorsDelegatesEnumerator.MoveNext().Should().BeTrue(); | ||
| credentials = credentialsGeneratorsDelegatesEnumerator.Current(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
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/GetCredentialsAsyncmethods. There is alsoResolveIdentity(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 ownDefaultAWSCredentialsIdentityResolverinstance and callResolveIdentity(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this, I'll create a follow up ticket.
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.
Created: https://jira.mongodb.org/browse/CSHARP-6145