-
Notifications
You must be signed in to change notification settings - Fork 13
chore: Separate AwsCredentials from AwsConfig #716
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
Conversation
@@ -228,14 +230,14 @@ async fn request( | |||
} | |||
|
|||
fn build_get_secret_signed_headers( | |||
aws_config: &AwsConfig, | |||
aws_credentials: &AwsCredentials, | |||
region: String, |
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 don't see a region to pass region separately in the old code. It should be the same as the region in aws_config.
In the new code, removing aws_config
since region is the only field needed from aws_config
.
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.
secrets can be cross region, see: #594
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.
Got it. Let me add back aws_config
param.
f749307
to
11c1c77
Compare
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.
Pull Request Overview
This PR separates AWS credential fields from AwsConfig and introduces a new struct, AwsCredentials, to simplify upcoming changes involving lazy loading of credentials. Key changes include updating function signatures in decrypt.rs to accept an AwsCredentials parameter, updating helper functions to use AwsCredentials, and modifying tests to use the new structure.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
bottlecap/src/secrets/decrypt.rs | Updated resolve_secrets and helper functions to use AwsCredentials |
bottlecap/src/proxy/mod.rs | Removed credential fields from test AwsConfig instances |
bottlecap/src/proxy/interceptor.rs | Adjusted test AwsConfig instantiation to account for extracted fields |
bottlecap/src/lifecycle/invocation/span_inferrer.rs | Removed credential fields in test AwsConfig instantiation |
bottlecap/src/lifecycle/invocation/processor.rs | Removed credential fields in test AwsConfig instantiation |
bottlecap/src/config/aws.rs | Removed credential fields from AwsConfig and added the AwsCredentials struct |
Comments suppressed due to low confidence (3)
bottlecap/src/secrets/decrypt.rs:45
- Ensure that the logic comparing empty credential fields in aws_credentials aligns with how credentials are initialized elsewhere, to avoid false negatives when credentials are intentionally empty.
if aws_credentials.aws_secret_access_key.is_empty()
bottlecap/src/secrets/decrypt.rs:20
- [nitpick] Consider whether the use of a mutable reference for AwsCredentials in resolve_secrets is necessary, or if the function could accept an immutable reference to improve safety and concurrency.
pub async fn resolve_secrets(
bottlecap/src/config/aws.rs:41
- [nitpick] Adding brief documentation comments for the fields in AwsCredentials would improve clarity for future maintainers.
pub struct AwsCredentials {
@@ -400,13 +400,8 @@ mod tests { | |||
|
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.
Since credential fields are now removed from AwsConfig, ensure that any test or sample initialization that previously provided dummy credential values is updated to either create an AwsCredentials instance or clearly document that these values are not required.
// Note: Credentials are not required for this test case as it does not involve AWS authentication. |
Copilot uses AI. Check for mistakes.
Problem
Right now
AwsConfig
has a lot of fields, including the ones related to credential:The next PR #717 wants to lazily load API key and the credentials. To do that, for the resolver function
resolve_secrets()
, I need to change the paramaws_config
from&AwsConfig
toArc<RwLock<AwsConfig>>
. Becauseaws_config
is passed to many places, this change involves updating lots of functions, which is formidable.This PR
Separates these credential-related fields out from
AwsConfig
and creates a new structAwsCredentials
Thus, the next PR will only need to change the param
aws_credentials
from&AwsCredentials
toArc<RwLock<AwsCredentials>>
. Becauseaws_credentials
is not used in lots of places, the next PR becomes easier.https://datadoghq.atlassian.net/issues/SVLS-6996
https://datadoghq.atlassian.net/issues/SVLS-6998