GODRIVER-3567 Add optional AWS SDK v2-based MONGODB-AWS authenticator submodule - #2508
GODRIVER-3567 Add optional AWS SDK v2-based MONGODB-AWS authenticator submodule#2508qingyang-hu wants to merge 9 commits into
Conversation
|
There is an existing patch(es) for this commit SHA: Please note that the status that is posted is not in the context of this PR but rather the (latest) existing patch and that may affect some tests that may depend on the particular PR. If your tests do not rely on any PR-specific values (like base or head branch name) then your tests will report the same status. If you would like a patch to run in the context of this PR and abort the other(s), comment 'evergreen retry'. |
github-actions
Bot
added
the
review-priority-normal
There was a problem hiding this comment.
[nit] We should use github.com/stretchr/testify/require to match the rest of the driver test suite.
There was a problem hiding this comment.
[question] This test intentionally skips assume-role and regular. We should update this comment to reflect that and also why we skip these cases.
🧪 Performance ResultsCommit SHA: 55baa9dThe following benchmark tests for version 6a68b897d1a0f3000705417e had statistically significant changes (i.e., |z-score| > 1.96):
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
API Change Report./v2/mongo/optionscompatible changes(*AutoEncryptionOptions).SetAWSCredentialsProvider: added ./v2/x/mongo/driver/mongocrypt/optionscompatible changesMongoCryptOptions.AWSCredentialsProvider: added |
| provider := aws.CredentialsProviderFunc(func(context.Context) (aws.Credentials, error) { | ||
| return aws.Credentials{}, nil |
There was a problem hiding this comment.
[blocking] It's unclear to me why we would need to go through the entire compile check with this separate option. We should revert these changes and simply rely on ext/awsauth/test and ext/awsauth/examples.
There was a problem hiding this comment.
It was used to check compatibility for higher Go versions. Reverted since we bumped the version to 1.25.
There was a problem hiding this comment.
It was used to check compatibility for higher Go versions. Reverted since we bumped the version to 1.25.
I thought we decided to keep 1.20 as the first go runtime for awsauth? In any event, we don't need to include this in the compile check here. We should add ext/awsauth/test/compilecheck/compile_check_test.go and just assert this:
var _ options.AWSCredentialsProvider = (*awsauth.CredentialsProvider)(nil)
awsauth will pass this use case if it implements options.AWSCredentialsProvider , per the original compile check test.
There was a problem hiding this comment.
It was a sensible suggestion.
I've updated the version to 1.25 in cf91914.
The assertions have also been added to ext/awsauth/test/aws_test.go while I revert compile_check_test.go.
Let me know if we need any more adjustments.
| // not use this file except in compliance with the License. You may obtain | ||
| // a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| package awsauthtest |
There was a problem hiding this comment.
[blocking] We should add a test/compilecheck/ package here and tie it into the compile-check evergreen task:
var _ options.AWSCredentialsProvider = (*awsauth.CredentialsProvider)(nil)There was a problem hiding this comment.
I've added them at L24-L27.
5b6adba to
55baa9d
Compare
| // not use this file except in compliance with the License. You may obtain | ||
| // a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| package awsauthtest |
| }, nil | ||
| } | ||
|
|
||
| func TestClientSideEncryptionProse_26_custom_aws_credentials(t *testing.T) { |
There was a problem hiding this comment.
[suggestion] Give how large the CSFLE test suite has become, I suggest that we move this to it's own file (just like prose test 27): internal/integration/client_side_encryption_prose_26_test.go. Additionally, IMO subtests make it harder to read the code. Consider doing 1 test per case:
TestCSE_26_CustomAWS_Case1_CE_WithCredProvidersAndIncorrectKMSProviders(t *testing.T) {}
TestCSE_26_CustomAWS_Case2_CE_WithCredProviders(t *testing.T) {}
TestCSE_26_CustomAWS_Case3_AE_WithCredProvidersAndIncorrectKMSProviders(t *testing.T) {}
TestCSE_26_CustomAWS_Case4_CE_WithCredProvidersAndEnvVars(t *testing.T) {}| opts := options.Client().ApplyURI(mtest.ClusterURI()) | ||
| integtest.AddTestServerAPIVersion(opts) | ||
| keyVaultClient, err := mongo.Connect(opts) | ||
| assert.NoErrorf(mt, err, "error on Connect: %v", err) |
There was a problem hiding this comment.
[nit] We should use the require package instead of assert. See GODRIVER-3708 for more info
| }) | ||
| _, err = clientEncryption.CreateDataKey(context.Background(), "aws", dkOpts) | ||
| assert.NoErrorf(mt, err, "unexpected error %v", err) | ||
| assert.Equal(mt, 1, provider.cnt, "expected credential provider to be called once") |
There was a problem hiding this comment.
[nit] This should be >= 1 per the spec:
Assert the datakey was created and that the custom credential provider was called at least once.
| }) | ||
| _, err = clientEncryption.CreateDataKey(context.Background(), "aws", dkOpts) | ||
| assert.NoErrorf(mt, err, "unexpected error %v", err) | ||
| assert.Equal(mt, 1, provider.cnt, "expected credential provider to be called once") |
There was a problem hiding this comment.
[nit] This should be >= 1 per the spec:
Assert the datakey was created and that the custom credential provider was called at least once.
GODRIVER-3567
GODRIVER-3615
Summary
This PR allows users to plug a custom AWS SDK v2 credentials provider into the driver's MONGODB-AWS authentication mechanism without forcing the AWS SDK as a dependency on the core driver.
Additionally, EKS Pod Identity and other AWS SDK-native authentication are made possible by the
ext/awsauthsubmodule as required in GODRIVER-3571.The custom provider overrides environment variables and all automatic sources, superseded only by explicit URI credentials as required in GODRIVER-3615.
Background & Motivation
This PR contains two stacked commits.
881bc5f:
New public interface in mongo/options (clientoptions.go)
AWSCredentialsProviderinterface (Retrieve(ctx) (AWSCredentials, error)) and anAWSCredentialsstruct alias.WSCredentialsProviderfield intoCredential, and intoClientEncryptionOptions/AutoEncryptionOptions.New
ext/awsauthsub moduleNewCredentialsProvider(aws.CredentialsProvider)adapts an AWS SDK v2 provider to the driver'soptions.AWSCredentialsProviderinterface. The adapter relies onAWSCredentialsbeing a struct type alias soaws.Credentialsconverts directly.Internal adapter (internal/credutil/aws_options_provider.go
AWSOptionsProviderbridges the publicoptions.AWSCredentialsProviderto the driver's internalcredentials.Providertype.bbedca6:
Notes
Instead of the v1.0.0 specified in the original design, the ext/awsauth submodule depends on aws-sdk-go-v2 v1.28.0 in this PR. v1.28.0 is the earliest version that is compatible with the current Credentials struct.