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

Aws sdk rust #469

Merged
merged 1 commit into from
Jul 26, 2022
Merged

Aws sdk rust #469

merged 1 commit into from
Jul 26, 2022

Conversation

rpkelly
Copy link
Contributor

@rpkelly rpkelly commented Jun 9, 2022

Issue #, if available:
#428

Description of changes:

  • Changed tough-ssm to use aws sdk rust
  • Changed tough-kms to use aws sdk rust
  • Changed tuftool to use aws sdk rust
  • Updated root directory to support above changes

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@rpkelly rpkelly marked this pull request as ready for review June 10, 2022 00:09
Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

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

nice work

None => Region::default(),
},
)
let config = aws_config::from_env();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does from_env already use the profile if environment variables are not present? Although the behavior was previously not commented, I think it would help to have some comments here explaining why from_env happens first, then things are overridden by the profile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.rs/aws-config/0.13.0/aws_config/fn.from_env.html it creates a ConfigLoader, which is a builder for the SdkConfig. Looking at the source, it's the same as ConfigLoader::default()

https://docs.rs/aws-config/0.13.0/aws_config/struct.ConfigLoader.html .from_env() is how the docs always refer to creating the builder for the SdkConfig, by default it looks in your environment and default profile, and the overrides are if you're doing anything else.

Copy link
Contributor

@etungsten etungsten Jul 26, 2022

Choose a reason for hiding this comment

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

I think Matt meant adding code comments for the explanation.

edit: Since ConfigLoader does document that the default chain implementation goes through from_env first, I think it's ok to document that here.

tough-kms/src/client.rs Outdated Show resolved Hide resolved
tough-kms/src/lib.rs Show resolved Hide resolved
tough-kms/Cargo.toml Outdated Show resolved Hide resolved
None => Region::default(),
},
)
let config = aws_config::from_env();
Copy link
Contributor

Choose a reason for hiding this comment

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

Request same documentation here about why we do from_env first then override values with the profile.

tough-ssm/src/lib.rs Show resolved Hide resolved
@@ -15,7 +10,7 @@ use tough::schema::key::Key;
use tough_kms::KmsKeySource;
use tough_kms::KmsSigningAlgorithm::RsassaPssSha256;

/// Deserialize base64 to `bytes::Bytes`
/// Deserialize base64 to `byt/es::Bytes`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Deserialize base64 to `byt/es::Bytes`
/// Deserialize base64 to `bytes::Bytes`

Is this a typo?

Copy link
Contributor

@etungsten etungsten left a comment

Choose a reason for hiding this comment

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

Is there anything we can do to test these changes? I believe at a minimum you should try this out in a Bottlerocket host (after bottlerocket-os/bottlerocket#2300 is merged) and see the update flow still works.

@etungsten etungsten self-requested a review July 26, 2022 19:15
Copy link
Contributor

@etungsten etungsten left a comment

Choose a reason for hiding this comment

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

Richard pointed out to me offline that github actions runs the integration tests. After looking through the integration test, I'm satisfied with the coverage.

Update tough-kms to use aws sdk rust

Update tough-ssm to use aws-sdk-rust

Update tuftool to AWS SDK Rust
@rpkelly rpkelly merged commit 973c94c into awslabs:develop Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants