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

fix: cache credential resolution with the AWS credential provider #2987

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

rtyler
Copy link
Member

@rtyler rtyler commented Nov 12, 2024

object_store invokes get_credential on every invocation of a get/list/put/etc. The provider invocation for environment based credentials is practically zero-cost, so this has no/low overhead.

In the case of the AssumeRoleProvider or any provider which has some cost, such as an invocation of the AWS STS APIs, this can result in rate-limiting or service quota exhaustion.

In order to prevent this, the credentials are attempted to be cached only so long as they have no expired, which is defined in the aws_credential_types::Credential struct

Sponsored-by: Scribd Inc

@github-actions github-actions bot added the binding/rust Issues for the Rust crate label Nov 12, 2024
Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 84.14634% with 13 lines in your changes missing coverage. Please review.

Project coverage is 72.33%. Comparing base (b4eff70) to head (3a25235).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/aws/src/credentials.rs 84.14% 0 Missing and 13 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2987      +/-   ##
==========================================
+ Coverage   72.27%   72.33%   +0.06%     
==========================================
  Files         128      128              
  Lines       40341    40420      +79     
  Branches    40341    40420      +79     
==========================================
+ Hits        29155    29237      +82     
+ Misses       9335     9305      -30     
- Partials     1851     1878      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

`object_store` invokes `get_credential` on _every_ invocation of a
get/list/put/etc. The provider invocation for environment based
credentials is practically zero-cost, so this has no/low overhead.

In the case of the AssumeRoleProvider or any provider which has _some_
cost, such as an invocation of the AWS STS APIs, this can result in
rate-limiting or service quota exhaustion.

In order to prevent this, the credentials are attempted to be cached
only so long as they have no expired, which is defined in the
`aws_credential_types::Credential` struct

Signed-off-by: R. Tyler Croy <[email protected]>
Sponsored-by: Scribd Inc
Copy link
Contributor

@mightyshazam mightyshazam left a comment

Choose a reason for hiding this comment

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

LGTM

@rtyler rtyler added this pull request to the merge queue Nov 14, 2024
Merged via the queue into delta-io:main with commit 9982e9c Nov 14, 2024
22 checks passed
@rtyler rtyler deleted the credential-caching branch November 14, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants