GODRIVER-3454 Support custom AWS credential provider.#2228
GODRIVER-3454 Support custom AWS credential provider.#2228qingyang-hu wants to merge 15 commits intomasterfrom
Conversation
🧪 Performance ResultsCommit SHA: a61c360The following benchmark tests for version 69866eb3dc65520007412f7b 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/drivercompatible changesAWSCredentialsProvider: added ./v2/x/mongo/driver/mongocrypt/optionscompatible changesMongoCryptOptions.AWSCredentialsProvider: added |
qingyang-hu
force-pushed
the
godriver3615
branch
12 times, most recently
from
a1b8f64 to
795b157
Compare
| }) | ||
| } | ||
|
|
||
| func TestCustomAwsCredentialsProse(t *testing.T) { |
There was a problem hiding this comment.
Put this aside from TestClientSideEncryptionProse so Setenv() can be used without being influenced by t.Parallel().
qingyang-hu
requested review from
Copilot,
matthewdale and
prestonvasquez
and removed request for
prestonvasquez
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for custom AWS credential providers, enabling applications to supply their own AWS credential retrieval logic for both authentication and client-side encryption. The implementation allows users to provide custom credential providers as a callback function that returns AWS credentials on demand.
Key changes:
- Added
AwsCredentialsProvidercallback field to credential options for connection authentication - Extended client-side encryption options to support custom credential providers via
CredentialProvidersmap - Refactored internal credential provider interface to use context-aware
Retrieve(context.Context)method consistently across all provider implementations
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
mongo/options/clientoptions.go |
Added AwsCredentialsProvider field and Credentials type to support custom AWS credential callbacks |
mongo/options/clientencryptionoptions.go |
Added CredentialProviders map and setter for custom provider configuration |
mongo/options/autoencryptionoptions.go |
Added CredentialProviders field and setter for auto-encryption provider configuration |
x/mongo/driver/topology/topology_options.go |
Converted user-provided credential provider to internal format during credential conversion |
x/mongo/driver/driver.go |
Added AwsCredentialsProvider field to Cred struct for internal provider storage |
x/mongo/driver/auth/mongodbaws.go |
Integrated custom credential provider into AWS authentication flow |
x/mongo/driver/mongocrypt/options/mongocrypt_options.go |
Added CredentialProviders field to MongoCrypt options |
x/mongo/driver/mongocrypt/mongocrypt.go |
Wired custom credential providers into MongoCrypt initialization |
mongo/client.go |
Converted credential providers for auto-encryption setup |
mongo/client_encryption.go |
Converted credential providers for client encryption setup |
internal/credproviders/aws_provider.go |
New provider implementation that wraps custom AWS credential callbacks |
internal/aws/types.go |
Added public Credentials type for custom provider return values |
internal/aws/credentials/credentials.go |
Unified provider interface to use context-aware Retrieve(context.Context) method |
internal/credproviders/*.go |
Updated all provider implementations to use context-aware retrieval |
x/mongo/driver/auth/mongodbaws_test.go |
Added tests for custom credential provider behavior |
internal/integration/client_side_encryption_prose_test.go |
Added integration tests for custom credential provider scenarios |
mongo/client_examples_test.go |
Added example demonstrating custom credential provider usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if a.credentials == nil || a.credentials.ExpirationCallback == nil { | ||
| return true | ||
| } |
There was a problem hiding this comment.
The IsExpired logic returns true when credentials are nil, which means the provider will always be considered expired before the first retrieval. This forces an immediate retrieval, but if ExpirationCallback is also nil after the first retrieval, it will always return true, causing credentials to be re-fetched on every call. Consider returning a.credentials == nil when ExpirationCallback is nil to avoid unnecessary re-fetching after initial retrieval.
| if a.credentials == nil || a.credentials.ExpirationCallback == nil { | |
| return true | |
| } | |
| if a.credentials == nil { | |
| return true | |
| } | |
| if a.credentials.ExpirationCallback == nil { | |
| return false | |
| } |
mongo/client.go
Outdated
| providers[k] = &credproviders.AwsProvider{ | ||
| Provider: func(ctx context.Context) (aws.Credentials, error) { | ||
| var creds aws.Credentials | ||
| c, err := fn(ctx) | ||
| if err != nil { | ||
| return creds, err | ||
| } | ||
| creds.AccessKeyID = c.AccessKeyID | ||
| creds.SecretAccessKey = c.SecretAccessKey | ||
| creds.SessionToken = c.SessionToken | ||
| creds.ExpirationCallback = c.ExpirationCallback | ||
| return creds, nil | ||
| }, | ||
| } |
There was a problem hiding this comment.
The closure over fn in the loop may capture the wrong value if the loop variable is reused. While this loop only processes the 'aws' key specifically, consider using a function parameter or declaring fn within the block to ensure correct closure capture: provider := fn before the closure.
mongo/client_encryption.go
Outdated
| providers[k] = &credproviders.AwsProvider{ | ||
| Provider: func(ctx context.Context) (aws.Credentials, error) { | ||
| var creds aws.Credentials | ||
| c, err := fn(ctx) | ||
| if err != nil { | ||
| return creds, err | ||
| } | ||
| creds.AccessKeyID = c.AccessKeyID | ||
| creds.SecretAccessKey = c.SecretAccessKey | ||
| creds.SessionToken = c.SessionToken | ||
| creds.ExpirationCallback = c.ExpirationCallback | ||
| return creds, nil | ||
| }, | ||
| } |
There was a problem hiding this comment.
The closure over fn in the loop may capture the wrong value if the loop variable is reused. While this loop only processes the 'aws' key specifically, consider using a function parameter or declaring fn within the block to ensure correct closure capture: provider := fn before the closure.
qingyang-hu
force-pushed
the
godriver3615
branch
2 times, most recently
from
1a5bc81 to
4fea597
Compare
mongo/options/clientoptions.go
Outdated
| } | ||
|
|
||
| // CredentialsProvider is the function type that returns AWS credentials. | ||
| type CredentialsProvider func(context.Context) (Credentials, error) |
There was a problem hiding this comment.
Thanks for this work @qingyang-hu! Before committing to a detailed review, I'd like to align with the team on the overall approach for providing custom credentials by clarifying that there are (at least) two approaches:
- The callback approach suggested in this PR
- The submodule to use the official aws-sdk-go-v2 as prescribed in GODRIVER-3567 and POC’d in PR #2057
Here is how I anticipate folks would use the submodule approach (see here for required POC updates):
// Custom credentials provider using the aws-sdk-go-v2
myProvider := aws.CredentialsProviderFunc(func(ctx context.Context) (aws.Credentials, error) {
return aws.Credentials{
AccessKeyID: "USER_ACCESS_KEY",
SecretAccessKey: "USER_SECRET_KEY",
SessionToken: "USER_SESSION_TOKEN",
Source: "CustomProvider",
}, nil
})
// Load AWS config with custom credentials provider using the aws-sdk-go-v2
awsCfg, _ := config.LoadDefaultConfig(
ctx,
config.WithCredentialsProvider(myProvider))
opts := options.Client().ApplyURI(uri)
// Extend options to include awsConfig using mongoaws
opts = mongoawsv2.WithAWSConfig(opts, awsConfig)
// Use with MongoDB
client, _ := mongo.Connect(opts)The pros of the submodule solution:
- Ideal for power users
- Zero AWS dependencies in the Go Driver while still getting official AWS SDK support
- The solution is future proof in that we can add mongoawsv3, mongoawsv4, etc. whenever needed
- Much better user experience than having to define an abstract credential object that could apply to any provider
- We can deprecate and no longer add to the AWS legacy code.
The Cons of the submodule solution:
- We have to maintain the legacy behavior until v3
- Requires an extra import when working with AWS
A potential concern is that the opaque any return type reduces compile-time type safety, but this is validated at connection time via mongo.Connect(), which is acceptable.
If we think custom credentials are rarely needed (power users only), then I suggest prioritizing GODRIVER-3567 (submodule approach) to avoid committing AWS-specific types to the stable API. Power users who need custom credentials likely already use AWS SDK v2 in their applications, so the dependency is acceptable. This prevents long-term technical debt from maintaining options.Credentials.
Otherwise, if credentials have broader appeal or users want to avoid AWS SDK v2 dependency, then this PR is acceptable imo. However, if we merge, I suggest we prioritize implementing GODRIVER-3567 afterward and clearly document it as the preferred approach for users already using AWS SDK v2 by deprecating options.Credentials.
IIUC, the SetCredentialProviders portion for CSFLE provides value regardless of which path we choose for connection authentication.
CC: @matthewdale
There was a problem hiding this comment.
Addressing the pros:
Ideal for power users
The user experience for both the callback and submodule approach are very similar.
Zero AWS dependencies in the Go Driver while still getting official AWS SDK support
That is true for the callback approach as well. Dependency management for the callback approach is arguably simpler because you don't have to worry about what version of the AWS SDK the "mongoawsv2" submodule depends on.
The solution is future proof in that we can add mongoawsv3, mongoawsv4, etc. whenever needed
I think we can address future AWS SDK changes in either approach. The structure of AWS credentials and credentials providers has barely changed over the lifetime of both AWS SDK major versions.
Much better user experience than having to define an abstract credential object that could apply to any provider
I agree with this. We should make the credential and credential provider types in this PR specific to AWS.
We can deprecate and no longer add to the AWS legacy code.
There are two sections of AWS-specific code in the Go Driver:
- Code to support the server's "MONGODB-AWS" auth API.
- Code to fetch AWS credentials from different environments.
Section 1 is specific to the MongoDB server API and is something we will have to continue maintaining as long as the server supports it, whether in the main Go Driver module or in an AWS-specific submodule. Section 2 is what we want to delegate to the AWS SDK.
Addressing the cons:
We have to maintain the legacy behavior until v3
I think this is true for either approach.
Requires an extra import when working with AWS
I agree with this con.
E.g. user experience using the callback approach (with the Go Driver types renamed to be AWS-specific):
myProvider := aws.CredentialsProviderFunc(func(ctx context.Context) (aws.Credentials, error) {
return aws.Credentials{
AccessKeyID: "USER_ACCESS_KEY",
SecretAccessKey: "USER_SECRET_KEY",
SessionToken: "USER_SESSION_TOKEN",
Source: "CustomProvider",
}, nil
})
// Load AWS config with custom credentials provider using the aws-sdk-go-v2
awsCfg, _ := config.LoadDefaultConfig(
context.Background(),
config.WithCredentialsProvider(myProvider))
opts := options.Client().
ApplyURI("mongodb://localhost:27017").
SetAuth(options.Credential{
AWSCredentialsProvider: func(ctx context.Context) (options.AWSCredentials, error) {
cfg, err := awsCfg.Credentials.Retrieve(ctx)
if err != nil {
return options.AWSCredentials{}, err
}
return options.AWSCredentials(cfg), nil
},
})
client, _ := mongo.Connect(opts)Overall, I think the callback approach requires fewer moving pieces and has an almost identical user experience. Also, it seems to align a little better with the API in other drivers (e.g. mongodb/node-mongodb-native#4373) considering DRIVERS-3194 was closed as a duplicate of DRIVERS-2903. I think we should move forward with the callback approach, amending this PR to make the credentials and credentials provider types AWS-specific.
There was a problem hiding this comment.
The user experience for both the callback and submodule approach are very similar.
I see it a bit differently. For example, if a caller needs to inject AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE to obtain credentials from the EKS Pod Identity Agent, the callback approach would require you the user to
- Read both the relative URI and token file from environment variables
- Make the HTTP request with the Authorization header set to the token value
- Parses the ECS response and returns credentials in the format the driver expects
- Pass this function to AwsCredentialsProvider (for auth) or SetCredentialProviders (for encryption)
// callback for to obtain credentials from the EKS Pod Identity Agent
func awsCredentialProvider(ctx context.Context) (options.Credentials, error) {
awsCfg, err := config.LoadDefaultConfig(ctx)
if err != nil {
return options.Credentials{}, err
}
creds, err := awsCfg.Credentials.Retrieve(ctx)
if err != nil {
return options.Credentials{}, err
}
return options.Credentials{
AccessKeyID: creds.AccessKeyID,
SecretAccessKey: creds.SecretAccessKey,
SessionToken: creds.SessionToken,
Source: creds.Source,
CanExpire: creds.CanExpire,
Expires: creds.Expires,
}, nil
}Shipping a standalone implementation upfront gives power users a supported hook, which is a much better UX:
// Load AWS config with default credential chain
// This automatically supports:
// - Environment variables
// - AWS_CONTAINER_CREDENTIALS_RELATIVE_URI
// - AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE (automatically read and sent)
// - EC2 instance metadata
// - ECS container credentials
// - etc.
awsCfg, err := config.LoadDefaultConfig(ctx)
if err != nil {
panic(err)
}
opts := options.Client().ApplyURI(uri)
// Extend options to include awsConfig using mongoawsv2
opts = mongoawsv2.WithAWSConfig(opts, awsCfg)
// Use with MongoDB - the AWS SDK will handle the token file automatically
client, err := mongo.Connect(opts)
if err != nil {
panic(err)
}That is true for the callback approach as well. Dependency management for the callback approach is arguably simpler because you don't have to worry about what version of the AWS SDK the "mongoawsv2" submodule depends on.
That's a fair point about avoiding a direct dependency. However, I'm concerned that copying SDK code (e.g., a signer) introduces security risks: if the SDK maintainers patch a CVE, we might not catch it in time and could continue shipping a vulnerable implementation. By relying directly on the SDK, we can benefit from their security updates automatically. Do you see a way to mitigate that risk with the current implementation?
We should make the credential and credential provider types in this PR specific to AWS.
From what I've seen, it seems like we'd be building and maintaining AWS-specific logic that essentially acts as a shim around the SDK. Is there a precedent or pattern you're pulling from that makes this the preferred approach? It seems like just relying on the SDK seems far more prudent.
Section 1 is specific to the MongoDB server API and is something we will have to continue maintaining as long as the server supports it
That's the longterm (v3) goal of GODRIVER-3567 and PR 2057, to containerize that logic into its own submodule, which would allow us to remove the AWS v1 legacy code from the driver. I realize this needs better documentation. I think this approach could give us a cleaner separation and make future maintenance easier. Does that address your concern?
Also, it seems to align a little better with the API in other drivers (e.g. mongodb/node-mongodb-native#4373) considering DRIVERS-3194 was closed as a duplicate of DRIVERS-2903
To clarify: DRIVERS-3194 was closed as a duplicate specifically because a submodule solution was expected to satisfy the requirements of DRIVERS-2903. The submodule approach was the intended path forward for addressing both tickets.
The callback solution is fair, but I think we should give strong consideration to the SDK solution as our end goal. If we implement the callback and then agree that the SDK solution is long-term better, we'd essentially need to turn around and deprecate it once we ship the submodule which feels like unnecessary churn.
To recap, my main concerns with the callback approach:
- We miss upstream CVE patches if we copy SDK code (not huge, given users can update Go toolchain locally)
- Power users still need workarounds for cases like EKS Pod Identity to avoid complex overhead
- We're already removing v1 AWS code (GODRIVER-3570), so this adds another deprecation cycle
For other drivers:
- Node uses an opt-in dependency for aws4 for signing, but plans to copy signing into the driver to reduce their overall dependency count as part of their serverless driver initiative, since the SigV4 signing algorithm is straightforward and spec-defined.
- Python relies on the SDK for all aspects of AWS Auth, but it is an opt-in dependency
- .NET also uses a standalone module that takes a dependency on the official AWS SDK, and will be fully integrated with this solution as of SHARP-4390
internal/aws/types.go
Outdated
| ) | ||
|
|
||
| // Credentials represents AWS credentials. | ||
| type Credentials struct { |
There was a problem hiding this comment.
We should rename this to AWSCredentials since it seems to be specific to the MONGODB-AWS auth mechanism.
mongo/options/clientoptions.go
Outdated
| } | ||
|
|
||
| // CredentialsProvider is the function type that returns AWS credentials. | ||
| type CredentialsProvider func(context.Context) (Credentials, error) |
There was a problem hiding this comment.
We should rename this to AWSCredentialsProvider since it seems to be specific to the MONGODB-AWS auth mechanism.
x/mongo/driver/driver.go
Outdated
| Props map[string]string | ||
| OIDCMachineCallback OIDCCallback | ||
| OIDCHumanCallback OIDCCallback | ||
| AwsCredentialsProvider func(context.Context) (aws.Credentials, error) |
There was a problem hiding this comment.
Initialisms in Go are conventionally all caps. This should be AWSCredentialsProvider.
qingyang-hu
force-pushed
the
godriver3615
branch
4 times, most recently
from
57dab25 to
61ee02f
Compare
qingyang-hu
force-pushed
the
godriver3615
branch
from
61ee02f to
ec2df09
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 44 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| nonce := make([]byte, 64) | ||
| copy(nonce, n.Nonce.Data) | ||
|
|
||
| writeReplies(resps, |
There was a problem hiding this comment.
The function writeReplies is called but not defined in this file. This function should be added to write replies to the channel, similar to other test files in the auth package. The function should iterate over the provided documents and write them to the channel using drivertest.MakeReply.
ext/awsauth/doc.go
Outdated
| // NewCredentialsProvider() adapts an AWS CredentialsProvider to be used in: | ||
| // | ||
| // ClientOptions | ||
| // ClinetEncryptionOptions |
There was a problem hiding this comment.
Typo in comment: "ClinetEncryptionOptions" should be "ClientEncryptionOptions".
| // ClinetEncryptionOptions | |
| // ClientEncryptionOptions |
| type awsCredentialsProvider struct { | ||
| provider options.AWSCredentialsProvider | ||
| } | ||
|
|
||
| func (p awsCredentialsProvider) Retrieve(ctx context.Context) (credentials.Value, error) { | ||
| creds, err := p.provider.Retrieve(ctx) | ||
| if err != nil { | ||
| return credentials.Value{}, err | ||
| } | ||
| return credentials.Value{ | ||
| AccessKeyID: creds.AccessKeyID, | ||
| SecretAccessKey: creds.SecretAccessKey, | ||
| SessionToken: creds.SessionToken, | ||
| Source: creds.Source, | ||
| CanExpire: creds.CanExpire, | ||
| Expires: creds.Expires, | ||
| AccountID: creds.AccountID, | ||
| ProviderName: "AwsProvider", | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
There is code duplication of the awsCredentialsProvider adapter type between this file and mongo/client_encryption.go. Both types have identical implementations that convert options.AWSCredentialsProvider to credentials.Provider. Consider extracting this to a shared internal package to avoid duplication and ensure consistency.
qingyang-hu
force-pushed
the
godriver3615
branch
3 times, most recently
from
46fb68a to
024f1ce
Compare
qingyang-hu
force-pushed
the
godriver3615
branch
from
024f1ce to
5f28ab0
Compare
| // This package allows credential providers and signers in v2 AWS SDK to be | ||
| // supplied to the MongoDB Go Driver for use with the MONGODB-AWS authentication | ||
| // mechanism. | ||
| // |
There was a problem hiding this comment.
[docs] We should add context about avoiding the AWS SDK depenency in the main driver:
| // | |
| // | |
| // This package is a separate module to avoid adding the AWS SDK as a dependency | |
| // of the main driver. Users who don't need AWS authentication won't need to | |
| // import the AWS SDK. | |
| // |
ext/awsauth/mongoaws_example_test.go
Outdated
| ) | ||
|
|
||
| func ExampleNewCredentialsProvider() { | ||
| awsCredentialProvider := NewCredentialsProvider(aws.NewConfig().Credentials) |
There was a problem hiding this comment.
[docs] Users would typically load credentials using LoadDefaultConfig, can we update examples to do that?
cfg, err := config.LoadDefaultConfig(context.TODO())
if err != nil {
panic(err)
}
awsCredentialProvider := NewCredentialsProvider(cfg.Credentials)
ext/awsauth/mongoaws.go
Outdated
| // NewCredentialsProvider returns a CredentialsProvider that wraps AWS | ||
| // CredentialsProvider. Provider is expected to not be nil. | ||
| func NewCredentialsProvider(provider aws.CredentialsProvider) *CredentialsProvider { | ||
| return &CredentialsProvider{ |
There was a problem hiding this comment.
[question] Does this need to return a pointer references? Whatever we decide, can we make NewSigner symmetric?
There was a problem hiding this comment.
Making the NewSigner() align with the NewCredentialsProvider(). Will also return an error to indicate the nil interface.
ext/awsauth/mongoaws.go
Outdated
|
|
||
| var _ options.AWSCredentialsProvider = (*CredentialsProvider)(nil) | ||
|
|
||
| // CredentialsProvider is an implementation of options.AWSCredentialsProvider in |
There was a problem hiding this comment.
[nit] This documentation is kind of missleading. CredentialsProvider is just a thin wrapper around aws.CredentialsProvider and doesn't directly provide "caching and concurrency safe credentials retrieval".
// CredentialsProvider adapts an AWS SDK v2 CredentialsProvider to the
// options.AWSCredentialsProvider interface used by the MongoDB Go Driver.
ext/awsauth/mongoaws.go
Outdated
| hash := sha256.Sum256([]byte(payload)) | ||
| payload = hex.EncodeToString(hash[:]) | ||
| } | ||
| r.Header.Set("X-Amz-Security-Token", creds.SessionToken) |
There was a problem hiding this comment.
AFAICT we don't unset X-Amz-Security-Token in aws_conv.go. We just use it to set another header. I think we should avoid sending an empty value, in case that has some connotation we aren't aware of.
| v.CanExpire = true | ||
| } |
There was a problem hiding this comment.
[question] I'm not sure I understand the logic here, shouldn't we discard the credentials if invalid? Why mark it as expirable?
There was a problem hiding this comment.
It's a failsafe mechanism to make the credentials expire if they are invalid. Positive CanExpire ensures the Expired() returns true with a zero Expires (see the logic here).
I'm adding a comment here.
| if err != nil { | ||
| return v, err | ||
| } | ||
| v.CanExpire = true |
There was a problem hiding this comment.
[question] Shouldn't this be in the expiration check block? What happens if expiration is 0?
There was a problem hiding this comment.
These credentials are expirable. Positive CanExpire ensures the Expired() returns true with a zero Expires (see the logic here). So 0 expiration makes the credentials expire.
| func (s *StaticProvider) Retrieve(_ context.Context) (credentials.Value, error) { | ||
| if !s.verified { | ||
| s.err = verify(s.Value) | ||
| s.ProviderName = staticProviderName | ||
| s.verified = true | ||
| } | ||
| return s.Value, s.err | ||
| } |
There was a problem hiding this comment.
[question] Is error caching necessary here?
func (s *StaticProvider) Retrieve(_ context.Context) (credentials.Value, error) {
s.ProviderName = staticProviderName
return s.Value, verify(s.Value)
}There was a problem hiding this comment.
Credentials might be retrieved more than once, and these values are static, so we cache the error as well as the value to avoid unnecessary computation.
| type serverMessage struct { | ||
| Nonce bson.Binary `bson:"s"` | ||
| Host string `bson:"h"` | ||
| type awsSaslAdapter struct { |
There was a problem hiding this comment.
Should we rename this file to aws_sasl_adapter.go?
x/mongo/driver/auth/mongodbaws.go
Outdated
|
|
||
| type awsSaslAdapter struct { | ||
| conversation *awsConversation | ||
| type builtInV4Signer struct { |
There was a problem hiding this comment.
[question] Do we need a wrapper for v4 signing? v4.Signer.Sign is only called in 1 place, here. Can't we just have that method implement awsSigner?
ext/awsauth/go.mod
Outdated
| @@ -0,0 +1,33 @@ | |||
| module go.mongodb.org/mongo-driver/v2/awsauth | |||
There was a problem hiding this comment.
We're planning to tag the initial module release with v0.1, so the module name should not include "v2", but should include the "ext" dir.
| module go.mongodb.org/mongo-driver/v2/awsauth | |
| module go.mongodb.org/mongo-driver/ext/awsauth |
ext/awsauth/mongoaws.go
Outdated
There was a problem hiding this comment.
Optional: Rename file to awsauth.go.
There was a problem hiding this comment.
Optional: Rename file to awsauth_example_test.go.
ext/awsauth/mongoaws_example_test.go
Outdated
| // 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 awsauth |
There was a problem hiding this comment.
Testable examples should always be in a *_test package. That has two benefits:
- The example code has to import the package under test (e.g.
awstesthere), so it looks more like the code a customer would write. - Testable example functions are rendered as full code files, including imports and a
mainblock, in the docs site.
| package awsauth | |
| package awsauth_test |
ext/awsauth/mongoaws_example_test.go
Outdated
| credential := options.Credential{ | ||
| AuthMechanism: "MONGODB-AWS", | ||
| AWSCredentialsProvider: awsCredentialProvider, | ||
| AWSSigner: awsSigner, |
There was a problem hiding this comment.
I'm concerned asking customers to specify both AWSCredentialsProvider and AWSSigner is going to create confusion. There's currently no observable behavior difference and no guidance on why a customer should specify it. Also, having to initialize and specify both a cred provider and a signer separately requires customers to write more code that could be avoided if the interfaces were combined.
We should either:
- Combine the
AWSCredentialsProviderandAWSSignerinterfaces and update the docs to say that we're planning to require customers to useawsauthfor signing in Go Driver v3. - Remove the
AWSSignerinterface and theawsauth.Signertype.
I prefer option 2 because it seems to be a better overall customer experience. While it would be nice to eventually remove the AWS v4 signing logic from the Go Driver, it doesn't seem worth a more confusing API.
ext/awsauth/mongoaws.go
Outdated
| func NewCredentialsProvider(credentialsProvider aws.CredentialsProvider) (*CredentialsProvider, error) { | ||
| if credentialsProvider == nil { | ||
| return nil, errors.New("nil provider") | ||
| } |
There was a problem hiding this comment.
We shouldn't require users to handle errors here. We should either defer this error check to Retrieve or let it panic.
qingyang-hu
force-pushed
the
godriver3615
branch
from
14b7932 to
a61c360
Compare
3 participants
GODRIVER-3454
GODRIVER-3615
Summary
awsSignerinterface to make the official AWS SDK migration easier in the future.Background & Motivation