Skip to content

GODRIVER-4034 Update the built-in AWS credential interfaces to match aws-sdk-go-v2.#2481

Open
qingyang-hu wants to merge 3 commits into
mongodb:masterfrom
qingyang-hu:godriver3567
Open

GODRIVER-4034 Update the built-in AWS credential interfaces to match aws-sdk-go-v2.#2481
qingyang-hu wants to merge 3 commits into
mongodb:masterfrom
qingyang-hu:godriver3567

Conversation

@qingyang-hu

@qingyang-hu qingyang-hu commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

GODRIVER-4034

Summary

This ticket re-shapes the built-in AWS credential interfaces to match aws-sdk-go-v2:

Value drops ProviderName and gains the SDK-v2 fields: Source, CanExpire, Expires and AccountID. Exactly the field set specified for the Retrieve return struct in:

type AWSCredentialsProvider interface {
    Retrieve(ctx context.Context) (struct { // Retrieve together
		AccessKeyID     string
		SecretAccessKey string
		SessionToken    string
		Source          string
		CanExpire       bool
		Expires         time.Time
		AccountID       string
	}, error)
}

The old Provider interface had Retrieve() (Value, error) plus IsExpired() bool, with a separate ProviderWithContext shim for RetrieveWithContext(ctx).

These migrate into one method: Retrieve(ctx context.Context) (Value, error). Every concrete provider (assume_role, ec2, ecs, env, imds, static) is updated to the new signature.

A new Value.Expired() method derives expiry from Expires/CanExpire. Providers/chains no longer track expiry separately.

Background & Motivation

This is a prerequisite ticket with no-public-API changes only. It de-risks the feature by landing the mechanical interface alignment on its own. The follow-up PRs will add new surface: the options interface of credential/encryption setters and the ext/awsauth adapter.

@evergreen-ci-prod

Copy link
Copy Markdown

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'.

@mongodb-drivers-pr-bot

mongodb-drivers-pr-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

🧪 Performance Results

Commit SHA: d4b8d22

The following benchmark tests for version 6a59552f44f8d8000784c488 had statistically significant changes (i.e., |z-score| > 1.96):

Benchmark Measurement % Change Patch Value Stable Region H-Score Z-Score
BenchmarkMultiInsertSmallDocument total_mem_allocs -15.3612 2211050.0000 Avg: 2612337.1667
Med: 2679873.0000
Stdev: 155803.8854
0.8452 -2.5756
BenchmarkMultiInsertSmallDocument total_time_seconds -11.3315 1.0200 Avg: 1.1503
Med: 1.1694
Stdev: 0.0646
0.7958 -2.0190
BenchmarkMultiInsertSmallDocument total_bytes_allocated -8.8764 444248888.0000 Avg: 487523218.6667
Med: 489091920.0000
Stdev: 19286819.0518
0.7780 -2.2437

For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch.

@mongodb-drivers-pr-bot

mongodb-drivers-pr-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

API Change Report

./v2/x/mongo/driver

compatible changes

AWSCredentialsProvider: added
Cred.AWSCredentialsProvider: added

./v2/x/mongo/driver/auth/creds

incompatible changes

AWSCredentialProvider: removed
NewAWSCredentialProvider: removed

compatible changes

AWSCredentialDocSource: added
NewAWSCredentials: added

@qingyang-hu qingyang-hu changed the title GODRIVER-3567 GODRIVER-3567 Update the Retrieve interface. Jul 14, 2026
@qingyang-hu
qingyang-hu marked this pull request as ready for review July 14, 2026 15:02
@qingyang-hu
qingyang-hu requested a review from a team as a code owner July 14, 2026 15:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 updates the driver’s AWS/Azure credential plumbing to support context-aware credential retrieval, adds an AWS credentials provider hook on driver.Cred, and refactors the internal AWS auth flow to use the v4 signer directly.

Changes:

  • Add AWSCredentialsProvider support to x/mongo/driver.Cred and wire it into MongoDB-AWS authentication.
  • Refactor internal AWS/Azure credential providers to use Retrieve(ctx) / Get(ctx) and move expiration tracking into credentials.Value.
  • Update internal credentials caching/chain logic and adjust related tests.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
x/mongo/driver/driver.go Adds AWSCredentialsProvider to Cred and defines the provider interface.
x/mongo/driver/auth/mongodbaws.go Wires optional custom AWS provider into the provider chain and uses v4 signer path.
x/mongo/driver/auth/creds/credscaching_test.go Updates AWS credential caching test expectations for invalid/expired credentials.
x/mongo/driver/auth/creds/azurecreds.go Switches Azure credential retrieval to Credentials.Get(ctx).
x/mongo/driver/auth/creds/awscreds.go Switches AWS credential retrieval to Credentials.Get(ctx).
x/mongo/driver/auth/aws_sasl_adapter.go Refactors SASL client to sign requests via v4 signer instead of the old conversation wrapper.
internal/credproviders/static_provider.go Updates static provider to implement Retrieve(context.Context).
internal/credproviders/imds_provider.go Refactors Azure IMDS provider to return Value expiration via fields instead of internal state.
internal/credproviders/env_provider.go Refactors env provider to context signature and uses CanExpire/Expires semantics for invalid creds.
internal/credproviders/ecs_provider.go Refactors ECS provider to context signature and uses CanExpire/Expires in returned Value.
internal/credproviders/ec2_provider.go Refactors EC2 provider to context signature and uses CanExpire/Expires in returned Value.
internal/credproviders/assume_role_provider.go Refactors assume-role provider to context signature and uses CanExpire/Expires in returned Value.
internal/aws/signer/v4/v4.go Updates signer to call Credentials.Get(ctx) and adjusts documentation.
internal/aws/credentials/credentials.go Redesigns credentials caching around Value.Expired() and atomic storage; removes provider-side IsExpired.
internal/aws/credentials/credentials_test.go Updates credential tests for the new API and concurrency behavior.
internal/aws/credentials/chain_provider.go Updates chain provider to call Retrieve(ctx) and validate returned values.
internal/aws/credentials/chain_provider_test.go Updates chain provider tests for new API/behavior.
Comments suppressed due to low confidence (1)

x/mongo/driver/auth/aws_sasl_adapter.go:56

  • awsSaslAdapter.Next ignores the provided context, and finalMsg creates an http.Request without attaching the SASL context. Because the v4 signer fetches credentials using req.Context(), credential retrieval and signing cannot be canceled when the auth context is canceled.

Thread ctx through step/finalMsg and use http.NewRequestWithContext (or req.WithContext) so signer.Sign and credentials.Get observe cancellation.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread x/mongo/driver/driver.go
Comment on lines +87 to 90
// AWSCredentialsProvider is the interface used to retrieve AWS credentials.
type AWSCredentialsProvider interface {
Retrieve(ctx context.Context) (credentials.Value, error)
}
Comment on lines +153 to +156
val := v.(*Value)
if val == nil || !val.HasKeys() {
return Value{}, false
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it's unclear why HasKeys should indicate the validity of an AWS credential. This also highlights the danger of using the same credential type for AWS and Azure.

Is Copilot's suggestion an acceptable compromise?

Comment on lines +100 to +103
// Sign will set the request's Body to be the `body` parameter passed in. If an
// empty body parameter passed to Sign, the request's Body field will be set to
// nil. Its important to note that this functionality will not change the
// request's ContentLength of the request.
Comment on lines 91 to 93
// Validates that a single call to Retrieve is shared between two calls to Get
time.Sleep(10 * time.Millisecond)
stub.done <- struct{}{}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, "sleep synchronization" can easily lead to flaky test and should not be used if possible.

@qingyang-hu
qingyang-hu requested a review from matthewdale July 14, 2026 15:35

@matthewdale matthewdale left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to update the description of this PR to reflect the actual changes and call out that the public options change will be made in the PR that adds the ext/awsauth submodule. As-is, this PR is difficult to review because neither the PR description nor the GODRIVER-3567 definition-of-done suggest why some of the refactors are necessary.

Comment on lines 7 to 11
// Based on github.com/aws/aws-sdk-go by Amazon.com, Inc. with code from:
// - github.com/aws/aws-sdk-go/blob/v1.44.225/aws/credentials/chain_provider.go
// See THIRD-PARTY-NOTICES for original license terms

package credentials

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update the comments in these modified files to indicate that it's no longer a direct copy and has been somewhat significantly modified.

e.retrieved = true
if err != nil {
// Expire the credentials if invalid.
v.CanExpire = true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: Consider also setting v.Expires = time.Time{} to make it explicit that we're marking the expiry time in the past. Currently, that requires knowing that Expires is set to the zero value on line 46. That assumption can also break if the credentials.Value initialization ever changes.

Suggested change
v.CanExpire = true
v.CanExpire = true
v.Expires = time.Time{}


// GetCredentialsDoc generates Azure credentials.
func (p AzureCredentialProvider) GetCredentialsDoc(ctx context.Context) (bsoncore.Document, error) {
creds, err := p.cred.GetWithContext(ctx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems out-of-scope for this PR, but it's very confusing that an AzureCredentialProvider uses the same AWS credential type and logic used by the AWSCredentialProvider. Do you remember why we use the same cred type for both?

Edit: I see that change was originally made in #1215. Even though I reviewed that PR, it still seems surprising.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created GODRIVER-4035 to address the issue.

Comment thread x/mongo/driver/auth/mongodbaws.go Outdated
}

return &MongoDBAWSAuthenticator{
credentials: creds.NewAWSCredentialProvider(httpClient, providers...).Cred,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: Now that we've added the new AWSCredentialsProvider interface, the naming of AWSCredentialProvider creates a lot of confusion. Is it possible to rename that type, or move it to the mongocrypt package since it's only actually used to implement the kmsProvider interface?

@qingyang-hu qingyang-hu changed the title GODRIVER-3567 Update the Retrieve interface. GODRIVER-4034 Update the built-in AWS credential interfaces to match aws-sdk-go-v2. Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants