Skip to content

Bump AWS SDK version#868

Merged
sam-berning merged 4 commits into
awslabs:developfrom
sam-berning:aws-sdk-bump
Apr 17, 2025
Merged

Bump AWS SDK version#868
sam-berning merged 4 commits into
awslabs:developfrom
sam-berning:aws-sdk-bump

Conversation

@sam-berning

Copy link
Copy Markdown
Contributor

Issue #, if available:

Description of changes:

Bump version of AWS SDK to leverage the updated HTTPS stack with hyper 1.x and aws-lc. See awslabs/aws-sdk-rust#1257.

Also migrates from aws-smithy-experimental to aws-smithy-https-client.

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

@sam-berning sam-berning marked this pull request as draft April 14, 2025 18:42
See awslabs/aws-sdk-rust#1257.

Also migrates from aws-smithy-experimental to aws-smithy-https-client.

Signed-off-by: Sam Berning <bernings@amazon.com>
Comment thread tuftool/Cargo.toml

@ginglis13 ginglis13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM - verified FIPS testing following #828 (comment)

@sam-berning sam-berning marked this pull request as ready for review April 16, 2025 18:00
Comment thread deny.toml
Comment on lines +77 to +78
# failure-server is an integ test package that pulls in legacy http libraries
{ name = "failure-server" },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this case, we're only skipping it for bans but not for licenses, right? That seems like a workable solution.

@cbgbt cbgbt Apr 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we're ignoring the failure-server branch, I wonder if we can add an explicit ban on ring?

We do an explicit ban on crates in bottlerocket-os/bottlerocket here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In this case, we're only skipping it for bans but not for licenses, right? That seems like a workable solution.

Yes, this only skips it for bans.

If we're ignoring the failure-server branch, I wonder if we can add an explicit ban on ring?

The feature set of reqwest that we're using in tuftool still depends on ring: https://github.com/seanmonstar/reqwest/blob/master/Cargo.toml#L52. We'd need to remove that dependency before we can deny ring.

Comment thread deny.toml Outdated
Comment on lines 72 to 73
# noxious-client is using an older version of reqwest
{ name = "reqwest", version = "=0.11" },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might be able to remove this now that we're skipping failure-server?

Comment thread Cargo.lock Outdated
Comment on lines +3339 to +3258
"aws-smithy-http",
"aws-smithy-http 0.60.11",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see that we have multiple aws-smithy-http's here but no corresponding skip in deny.toml. Any idea why?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, that's really strange, I'm not sure how that wasn't flagged by cargo-deny! The 0.60.x version comes from tough-kms's dev-dependencies, where we specifically use that version. I'll bump it to 0.62.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've noticed sometimes the aws-sdk-rust crates will get ahead of themselves and pull multiple aws-smithy-https. You might be able to shake it by pulling newer (but not the newest) SDK crates.

Comment thread tough/src/lib.rs
/// Steps 0 and 1 of the client application, which load the current root metadata file based on a
/// trusted root metadata file.
#[expect(clippy::too_many_arguments)]
#[allow(clippy::needless_continue)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you use expect instead of allow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hm, tried this and apparently not. There seems to be something odd with the macos Github actions runner, it doesn't register a needless_continue in this function so it will fail the expect (and it's also had a bunch of other strange failures on this PR). I couldn't replicate this on my mac, but switching this back to allow to pass the workflows.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This suggests to me that the Mac actions runner must be using an older rust toolchain.

I think it points out to me that we probably don't want this to fail if you are using an older rust toolchain, so I'm OK to leave it as-is.

Signed-off-by: Sam Berning <bernings@amazon.com>
Signed-off-by: Sam Berning <bernings@amazon.com>
Signed-off-by: Sam Berning <bernings@amazon.com>
@sam-berning sam-berning merged commit 728889a into awslabs:develop Apr 17, 2025
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.

5 participants