-
Notifications
You must be signed in to change notification settings - Fork 181
RUST-1529 Use AWS SDK to get credentials #1435
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to mention this regarding the testing commit I provided, but we should leave line 226 (patchable: false
) in config.yml commented out while this PR is being worked on. patchable: false
disables tests from running on every commit to a pull request branch - normally, we don't need to run the AWS authentication tests that frequently, but since we're making AWS-related changes we should be running the tests on this PR.
let aws_credential = { | ||
// Limit scope of this variable to avoid holding onto the lock for the duration of | ||
// authenticate_stream. | ||
let cached_credential = CACHED_CREDENTIAL.lock().await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the SDK handle credential caching? We should make sure the new implementation still caches credentials as outlined in the spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up on this — as we discussed, the SDK does implement credential caching. I’ve linked the relevant documentation here in case it's helpful for future reference!
@@ -246,6 +297,20 @@ impl AwsCredential { | |||
} | |||
} | |||
|
|||
fn from_sdk_creds( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be feasible to replace AwsCredential
in the code with aws_credential_types::Credentials
(i.e. the type returned from provide_credentials
)? That could remove some of the type translation we're doing here; it looks like they have very similar fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to keep AwsCredential
for now since we’re still using compute_authorization_header(...)
, which relies on it. Once that method is removed in the next PR, I can look into fully switching over to aws_credential_types::Credentials
.
To make the transition easier later, I’ll update this PR to type creds
as aws_credential_types::Credentials
and then map it into AwsCredential
before using compute_authorization_header(...)
. That should make it easier to remove from_sdk_creds(...)
and AwsCredential
in the follow-up.
Also, I missed that another file uses AwsCredential::get
, so I’ve moved the credential-fetching logic into a public method to support that use case!
…redential_types::Credential
Replace existing implementation for getting AWS credentials by using the AWS SDK.
Currently, we have 5 places where we search for AWS credentials:
A custom AWS credential provider if the driver supports itFor the sake of testing, the existing implementation was replaced instead of being hidden behind a feature flag.
Work to be done
compute_authorization_header(...)
)