Skip to content
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

Improve documentation around client/credential caching #2775

Closed
stanhu opened this issue Sep 4, 2024 · 3 comments · Fixed by #2781
Closed

Improve documentation around client/credential caching #2775

stanhu opened this issue Sep 4, 2024 · 3 comments · Fixed by #2781
Assignees
Labels
documentation This is a problem with documentation. feature-request A feature should be added or improved. p3 This is a minor priority issue queued This issues is on the AWS team's backlog

Comments

@stanhu
Copy link

stanhu commented Sep 4, 2024

Describe the issue

In https://docs.aws.amazon.com/sdk-for-go/v1/developer-guide/sessions.html, the guidance for caching s3.Session was clear:

You should cache sessions when possible. This is because creating a new session loads all configuration values from the environment and configuration files each time the session is created. Sharing the session value across all of your service clients ensures the configuration is loaded the fewest number of times.

In https://aws.github.io/aws-sdk-go-v2/docs/migrating/ and https://aws.github.io/aws-sdk-go-v2/docs/making-requests/, I'm not clear on what could be cached. We have a long-lived application that might make hundreds of S3 calls over time, and we want to avoid hitting STS limits. If I understand correctly:

import "context"
import "github.com/aws/aws-sdk-go-v2/config"
import "github.com/aws/aws-sdk-go-v2/service/s3"

// ...

cfg, err := config.LoadDefaultConfig(context.TODO())
if err != nil {
	panic(err)
}

client := s3.NewFromConfig(cfg)

In this example:

1.config.LoadDefaultConfig: I think this might make a HTTP request to the instance metadata or STS endpoint if no static credentials are configured.
2. s3.Client: This creates a new S3 client and loads the credentials from the config.

Is it okay to cache s3.Client indefinitely? I presume that when the credentials are expired, it will automatically refresh them.

It should also be okay to cache config.LoadDefaultConfig to avoid making an HTTP call, but it might just be easier to cache s3.Client.

It'd be nice to update the documentation, especially in the migration guide, about this behavior.

Links

  1. https://aws.github.io/aws-sdk-go-v2/docs/configuring-sdk/
  2. https://aws.github.io/aws-sdk-go-v2/docs/migrating
  3. https://aws.github.io/aws-sdk-go-v2/docs/making-requests/

AWS Go SDK V2 Module Versions Used

	github.com/aws/aws-sdk-go v1.55.5
	github.com/aws/aws-sdk-go-v2 v1.30.3
	github.com/aws/aws-sdk-go-v2/config v1.27.27
	github.com/aws/aws-sdk-go-v2/credentials v1.17.27
	github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.17.10
	github.com/aws/aws-sdk-go-v2/service/s3 v1.58.3
	github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.3 // indirect
	github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.11 // indirect
	github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.15 // indirect
	github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.15 // indirect
	github.com/aws/aws-sdk-go-v2/internal/ini v1.8.0 // indirect
	github.com/aws/aws-sdk-go-v2/internal/v4a v1.3.15 // indirect
	github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3 // indirect
	github.com/aws/aws-sdk-go-v2/service/internal/checksum v1.3.17 // indirect
	github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.17 // indirect
	github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.17.15 // indirect
	github.com/aws/aws-sdk-go-v2/service/sso v1.22.4 // indirect
	github.com/aws/aws-sdk-go-v2/service/ssooidc v1.26.4 // indirect
	github.com/aws/aws-sdk-go-v2/service/sts v1.30.3 // indirect
	github.com/aws/smithy-go v1.20.3 // indirect
@stanhu stanhu added documentation This is a problem with documentation. needs-triage This issue or PR still needs to be triaged. labels Sep 4, 2024
@RanVaknin
Copy link
Contributor

RanVaknin commented Sep 5, 2024

Hi @stanhu,

Our developer guide talks about this:

All credential providers passed to or returned by LoadDefaultConfig are wrapped in a CredentialsCache automatically. This enables caching and concurrency safe credential access. If you explicitly configure a provider on aws.Config directly you must explicitly wrap the provider with this type using NewCredentialsCache.

Regarding your question:

Is it okay to cache s3.Client indefinitely? I presume that when the credentials are expired, it will automatically refresh them.

If you are using LoadDefaultConfig then yes. The same is true for Go SDK v1's session.NewSession(). Both instruct the SDK to use the default credential provider chain, which automatically comes with a cache that handles credential rotation for any of the providers. In your case, the SDK would make calls to IMDS to refresh credentials 5 min before the credentials' expiration window.

This isn't covered in the migration guide because the functionality in v1 and v2 is the same with regards to how credentials are rotated. The only difference is the re-naming of the config object, which in v1 was confusingly named "session" even though it does not really represent a session but rather a config object.

I suppose we can add another section in the migration guide about the credential rotation behavior.

Thanks,
Ran~

@RanVaknin RanVaknin self-assigned this Sep 5, 2024
@RanVaknin RanVaknin added feature-request A feature should be added or improved. p3 This is a minor priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Sep 5, 2024
@stanhu
Copy link
Author

stanhu commented Sep 5, 2024

@RanVaknin Thanks, that helps clarify things!

I suppose we can add another section in the migration guide about the credential rotation behavior.

Yes, that would be helpful.

@RanVaknin RanVaknin added the queued This issues is on the AWS team's backlog label Sep 6, 2024
@lucix-aws lucix-aws linked a pull request Sep 11, 2024 that will close this issue
Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is a problem with documentation. feature-request A feature should be added or improved. p3 This is a minor priority issue queued This issues is on the AWS team's backlog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants