feat: create wrapper client for aws config for customizable defaults #6095
+287
−98
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/try |
|
Okay, starting a try! I'll update this comment once it's running...\n |
Dependency Review✅ No vulnerabilities or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a custom AWS config wrapper to support configurable defaults (adaptive retry, increased attempts, fallback region) and proactive validation of credentials, and updates dependent crates to use the new wrapper.
- Introduces the new crate si-aws-config with a custom configuration loader and caller identity validation.
- Updates si-data-ssm, si-data-acmpca, data-warehouse-stream-client, and associated Buck and Cargo files to remove direct aws_config usage in favor of si-aws-config.
- Adjusts error handling in client constructors across multiple modules to propagate errors from the new wrapper.
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| third-party/rust/Cargo.toml | Added aws-sdk-sts dependency with specified features. |
| lib/si-data-ssm/src/lib.rs | Updated SSM client initialization to use si-aws-config and proper error propagation. |
| lib/si-data-ssm/Cargo.toml | Replaced direct aws-config dependency with si-aws-config. |
| lib/si-data-ssm/BUCK | Updated Buck dependency reference from aws-config to si-aws-config. |
| lib/si-data-acmpca/src/lib.rs | Updated ACM PCA client error handling to use si-aws-config and propagate errors. |
| lib/si-data-acmpca/Cargo.toml | Replaced direct aws-config dependency with si-aws-config. |
| lib/si-data-acmpca/BUCK | Updated Buck dependency reference to use si-aws-config. |
| lib/si-aws-config/src/lib.rs | New implementation of the AWS config wrapper with adaptive retry, fallback region, and immediate credential validation. |
| lib/si-aws-config/Cargo.toml | New Cargo configuration for the si-aws-config crate. |
| lib/si-aws-config/BUCK | Buck build file for the new si-aws-config crate. |
| lib/innit-server/src/server.rs | Updated usage of ParameterStoreClient and PrivateCertManagerClient to propagate errors. |
| lib/innit-client/src/lib.rs | Updated certificate generation to use the new error propagation style. |
| lib/forklift-server/src/server/app/billing_events.rs | Adjusted DataWarehouseStreamClient initialization to propagate errors properly. |
| lib/data-warehouse-stream-client/src/lib.rs | Updated Firehose client initialization to use si-aws-config with error propagation. |
| lib/data-warehouse-stream-client/Cargo.toml | Replaced direct aws-config dependency with si-aws-config. |
| lib/data-warehouse-stream-client/BUCK | Updated Buck dependency reference to use si-aws-config. |
| dev/Tiltfile | Added auto_init flag to resource configuration. |
| Cargo.toml | Added si-aws-config to workspace members and updated dependency versions. |
Comments suppressed due to low confidence (2)
lib/si-data-ssm/src/lib.rs:52
- [nitpick] The error message contains duplicate wording ('Error error'). Consider revising it to 'AWS Config Error: {0}' for clarity.
#[error("AWS Config Error error: {0}")]
lib/si-data-acmpca/src/lib.rs:71
- [nitpick] The error message contains redundant wording ('Error error'). Consider revising it to 'AWS Config Error: {0}' for improved clarity.
#[error("AWS Config Error error: {0}")]
fnichol
previously approved these changes
May 15, 2025
| let sts = aws_sdk_sts::Client::new(&config); | ||
| let ident = sts | ||
| .get_caller_identity() | ||
| .send() |
There was a problem hiding this comment.
Was this default behavior before? Because I like that we can roughly confirm our identity this way!
There was a problem hiding this comment.
No, the default behavior is to gather credentials from various places in the environment and then carry on. It makes no effort to validate the creds until the first API call made using them. Here we'll at least test them on startup.
6017cf0 to
814e205
Compare
johnrwatson
approved these changes
May 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a wrapper for the aws_config crate so we can supply different defaults (adaptive retry, more attempts, fallback region) and so we can validate the credentials are good as soon as we create them (rather than waiting until they are first used).