change default HTTPS client to hyper1 #4040
Merged
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
## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> * #3710 * #1925 * awslabs/aws-sdk-rust#977 ## Description This is the first of what I'm going to assume will be several PRs on the path to migrating us to hyper 1.x. The goal is to reach a desired state as far as crate organization, feature flags, and how this is all enabled by default (eventually). This first PR just moves existing HTTP client support around as prep work for what's to come. NOTE: This is all getting merged into a staging branch `hyper1` rather than `main` * Migrate `hyper-0.14` and `hyper-1.x` support into a new `aws-smithy-http-client` crate. * re-export `hyper 0.14` from `aws-smithy-runtime`. * Remove support from `aws-smithy-experimental` for hyper 1.x and leave breadcrumb for where it lives now. * Migrate `CaptureSmithyConnection` into `aws-smithy-runtime-api` so that it can be used by new crate without creating a circular dependency. Why a new crate? Several reasons: * The entire hyper and rustls ecosystem bifurcates on hyper 0.x. The resulting `Cargo.toml` is kind of a mess as a result. In a new crate we can isolate this ugliness as well as manage feature flags more meaningfully with this in mind. * Placing the HTTP client implementation in `aws-smithy-runtime` makes it a load bearing crate for all of the HTTP and TLS related feature flags we may wish to expose _in addition to it's own feature flags_. This sort of explodes with the aforementioned bifurcation. * I expect `aws-smithy-runtime` and generated SDKs to choose a default configuration but customers can pull in this new `aws-smithy-http-client` crate and enable different flags for specific use cases (e.g. FIPS). * A new crate may be a good place to explore new functionality (e.g. we've considered forking our own implementation of `hyper-util` legacy client, this gives us a natural place for that should we go down that route) ## Future Where are we going? * I want to relocate all of `aws-smithy-runtime/src/client/http/test_util` into the `aws-smithy-http-client` crate. These are utilities for testing with a mock/fake HTTP client and they make more sense in this new crate. This also allows us to update the utils to support the hyper/http 1.x ecosystem and feature gate the legacy ecosystem. We can re-export the legacy ecosystem test support from `aws-smithy-runtime` for now. * Update our internal usage of these test utils with the new 1.x ecosystem and deprecate the old APIs but leave them in place. * I expect we'll deprecate them with a plan to remove or at least feature gate differently in the future with a recommendation to upgrade. * I want to explore the tickets/use cases people have asked for to see what/if we can do anything better with the hyper 1.x API surface. In particular I think we may want to just expose our own builder type (new type around hyper-util builder). * Because `hyper-util` is not 1.x we can't expose the `HyperClientBuilder` like we did previously. I don't think we should even if it was 1.x though, we should offer a "default client" with knobs for all the things we do want to support directly. Anything not found there you have to bring your own client configured to your heart's content. * We need to explore if we can make `aws-lc` the default (at least on `unix`). * I want to add optional support for `s2n-tls` to `aws-smithy-http-client` and reconcile related crypto/tls feature flags with this in mind. * Need to figure out how we set the default for `aws-smithy-runtime` and generated clients to be hyper 1.x and add appropriate new flags, etc. * Update changelogs, versions, lockfiles, etc. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->
Previous PR(s):
* #3866
## Description
This PR migrates all of the HTTP related `test-util` feature to the new
`aws-smithy-http-client` crate.
* Re-export the relocated types from `aws-smithy-runtime`. I've tried to
ensure the feature gates line up correctly to what was there before. All
existing code depending on the `test-util` feature compiles and runs
still in `smithy-rs`.
* Updated the internals of all `test-util` code to be based on `hyper-1
/ http-1`.
* There were two functions `legacy_infallible::infallible_client_fn` and
`legacy_capture_request` that I had to just duplicate with the legacy
types to avoid a breaking change to them. These will be re-exported from
`aws-smithy-runtime` as the original types but otherwise not be used
going forward.
Next steps:
* Deprecate the old `test-util` APIs and update all of our usage of
`test-util` to be based on the new API's in `aws-smithy-http-client`. I
may ask for team to help in this effort as it should be relatively
straight forward migration work.
## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] For changes to the smithy-rs codegen or runtime crates, I have
created a changelog entry Markdown file in the `.changelog` directory,
specifying "client," "server," or both in the `applies_to` key.
- [x] For changes to the AWS SDK, generated SDK code, or SDK runtime
crates, I have created a changelog entry Markdown file in the
`.changelog` directory, specifying "aws-sdk-rust" in the `applies_to`
key.
----
_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
## Motivation and Context Follow up PR to #3888 ## Description * Update `aws/sdk/integration-tests` to use the `test-util` feature(s) directly from `aws-smithy-http-client` rather than `aws-smithy-runtime` * ~Refactor `aws-smithy-http` to use `http-1.x~ * Fixed an erroneous error message from `imds` credentials provider in passing while tracking down a test failure * Fixed a few tests that were not immune to environment variables (e.g. on dev desktop `AWS_EC2_METADATA_ENABLED=false` is set by default and was causing some test failures) I started pulling at some threads and found them to be quite load bearing (e.g. `inlineable/serialization_settings.rs`) and would require updating all of codegen to use `http-1.x`. ~This PR is a bit of a half step with some stop gaps put into place (e.g. the new `compat` feature of `aws-smithy-http` or inlining a function) to prevent the need to update everything in a single PR. Future PR's will continue where I left off here. ~ One lesson I learned the hard way was that our integration test `Cargo.toml` have to reflect codegen. I had updated them to all use `http = "1"` and got it all working only to realize codegen generates `http = "0.2.9"` and `http-1x = { package = "http", version = "1"}` and thus the integration tests have to look the same as that. ### Future * ~Continue to refactor `aws-smithy-http` to upgrade to `http-body-1.x`. ~ * Refactor codegen to update to `http-1.x` UPDATE: I reverted the update to http 1.x for `aws-smithy-http`. This needs to be done and planned with the server and I don't want it to unnecessarily block hyper 1. We will pursue the remaining hyper 1 changes and then make a decision on internal use of http and http-body pre 1.x usage. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
…d new config (#3914) ## Motivation and Context Yet another follow up to: #3866 ## Description * Refactor the hyper 1 implementation into multiple modules to make it (slightly) easier to navigate (separating TLS, DNS, and timeout concerns into new/separate modules) * Refactors the hyper 1 implementation to allow for additional TLS provider implementations (e.g. s2n). Before we just exposed a `CryptoMode` but that is only applicable to rustls. I've swapped this for a new `TlsProvider` concept * Refactored how connectors are built. Before we were taking a TLS connector and wrapping it (via `Connector`). But this is structurally awkward since our `Connector` is analogous to hyper_utils' [`HttpConnector`](https://docs.rs/hyper-util/latest/hyper_util/client/legacy/connect/struct.HttpConnector.html#) (or supposed to be anyway). The updated structure forces our `Connector` to build the `HttpConnector` (which is the lowest layer/TCP connector) and then (if enabled) wrap it in TLS, and then finally wrap it with the SDK timeouts. ``` // build function returns a connector that looks like this as far as layering goes: Connector<TlsProviderSelected>.build() -> SdkTimeouts(TlsConnector(HttpConnector)) ``` * Exposed additional builder setting for `TCP_NODELAY` (and defaulted it to true). * Exposed additional builder setting for binding to a specific NIC * Fixed feature flags (powerset build was failing locally due to invalid or misconfigured feature flag application) * Renamed TLS feature flags to be associated with the underlying TLS lib (e.g. `crypto-aws-lc` -> `rustls-aws-lc`) **NOTE**: Apologies in advance, the diff didn't quite come out as I wanted. `client.rs` replaced `hyper_1.rs` (several modules of `hyper_1.rs` were split out into new modules). Everything in `timeout.rs` and `dns.rs` did not change. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
## Motivation and Context * #1925 * awslabs/aws-sdk-rust#977 ## Description * Add a new `default-http-connector` feature to `aws-smithy-runtime` that enables hyper 1.x with rustls+aws-lc as the default HTTP client plugin. * If both the legacy `connector-hyper-014-x` feature is enabled the new feature takes precedence such that the default HTTP client plugin favors hyper 1 if both features are enabled. * Update codegen to enable the new `default-http-connector` feature instead of hyper 0.14 * NOTE: we can't break the existing `rustls` feature flag so it has instead been updated to enable the `default-http-connector` feature. * Importantly this still uses rustls as the TLS provider but the crypto provider now defaults to aws-lc (which is coincidentally what rustls now defaults to as well). This is likely to break _someone_ somewhere due to build requirements changing (e.g. CMake now be required on certain platforms or other build tools needed). * Updated `aws-config` to default to hyper1 for the default credentials chain. * We have two features in `aws-config`, `rustls` and `client-hyper`, that seem to be used entirely to enable the `aws-smithy-runtime` features `tls-rustls` and `connector-hyper-0-14-x` features (both are required for the default HTTP client plugin to work prior to this PR). I think the _intent_ behind these features was "the user is not providing an HTTP client explicitly so we need a default one to be available". I've added a new feature to `aws-config` for this, `default-http-connector` and updated the existing features to be synonyms. We don't use `rustls` or `hyper 0.14.x` directly in `aws-config` so I think this is a more clearly defined flag and conveys it's intent better. * Added a new `legacy-test-util` feature flag to `aws-smithy-runtime`. The rationale for this is that the `test-util` feature provides testing utilities for other things from `aws-smithy-runtime` but it also brings in the (now) legacy hyper 0.14 HTTP testing facilities. I've left `test-util` to mean what it does today and be backwards compatibile (for now anyway) and in future we can ship a (breaking) change to disable the legacy test utils by default (and by extension stop compiling the legacy hyper ecosystem in all of our tests) * Fixed an issue in `examples/pokemon-service-common` due to codegen no longer generating clients with the `aws-smithy-runtime/tls-rustls` feature enabled by default (they are using the `HyperClientBuilder::build_https()` method directly but relying on feature unification to enable the method) ## Questions * Bikeshed any feature flag names. e.g. `aws-config/default-http-connector` could be more generic like `default-providers` or something. Today we use it to mean "we need a default HTTP client" but you can imagine a future where we need other default runtime components to exist and be configured. Perhaps that is what we want from this flag? ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> * #1925 * awslabs/aws-sdk-rust#977 ## Description <!--- Describe your changes in detail --> Follow up to #3939 to change how we enable hyper1 as the default. * This approach adds a new cargo feature flag on generated SDK packages (`default-https-client`) and restores the `rustls` feature to mean what it did before. Both will remain enabled as `default` features for some undetermined amount of time with the intent to remove `rustls` as a default feature as a breaking change in the future. * Renamed feature flags to be consistent. We settled on `default-https-client`. This new flag when enabled and the appropriate min BMV is set will result in the new HTTPS stack being the default for clients. Otherwise we fallback to the old behavior (still requires `rustls` and `connector-hyper-0-14` features enabled) ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> Adds s2n-tls as an optional TLS provider to the hyper1 branch. Related issue: #2446 ## Description <!--- Describe your changes in detail --> Currently, smithy-rs clients use rustls as the TLS provider. This PR adds s2n-tls as an optional provider, allowing users to configure smithy-rs to secure their HTTP requests with s2n-tls. ## Testing <!--- Please describe in detail how you tested your changes --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> I added a new client smoke test for s2n-tls. I also added a test that makes sure s2n-tls is actually set as the provider when configured. I was looking for more tests that invoke rustls that I could add an equivalent s2n-tls test for, but was having trouble discovering them. If there are tests that make sense to have please let me know and I will add them! Some of the CI jobs are still failing. However, they appear to be unrelated to this change. These jobs also failed in the last hyper1 commit: - Verify TLS configuration [failed similarly in the last commit](https://github.com/smithy-lang/smithy-rs/actions/runs/12915992501/job/36020683438) - Check for semver hazards [failed similarly in the last commit](https://github.com/smithy-lang/smithy-rs/actions/runs/12915992496/job/36025552287) - Check PR semver compliance [failed similarly in the last commit](https://github.com/smithy-lang/smithy-rs/actions/runs/12915992521/job/36020685637) Also, [the canary will need to be run manually](https://github.com/smithy-lang/smithy-rs/actions/runs/12916010137/job/36020739770?pr=3965) if necessary for this change. ## Checklist <!--- If a checkbox below is not applicable, then please DELETE it rather than leaving it unchecked --> - [ ] For changes to the smithy-rs codegen or runtime crates, I have created a changelog entry Markdown file in the `.changelog` directory, specifying "client," "server," or both in the `applies_to` key. - [ ] For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the `.changelog` directory, specifying "aws-sdk-rust" in the `applies_to` key. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> * #1925 * awslabs/aws-sdk-rust#977 ## Description Introduces a new `TlsContext` that can be used to customize TLS configuration. Currently that only includes supplying custom root certificates. This PR also refactors the way caching works. Previously we cached complete connectors with default resolver when the connector settings were known to be defaulted. The reason given in code and past commits for this caching resolves around the fact that `hyper-rustls` `with_native_roots()` configuration option re-loads platform root certificates on each invocation. Rather than cache the connector, instead I've switched the caching to only cache loading the certificates. This reduces a bit of complexity and required context to understand when we can cache. ### Questions/Discussion 1. There are a number of places we call `unwrap`/`expect` in this code. It was this way previously but I've introduced a few more. This stems from building a connector not returning a result. I don't know that I love this but I also suppose I can understand why we chose this. Now is the time to consider whether we'd approach it differently or not though or we are simply saying a valid HTTPS configuration. 2. Loading native certs is all blocking file calls though and not async friendly. I haven't changed anything here but it caught my eye. It would be nice if this were more async friendly. The mitigation we chose to this seemed to be caching the connector and paying that cost once up front at construction time if possible. 3. Both rustls and s2n take in PEM encoded certs. Rustls (via `rustls-pki-types`) provides a way to actually parse and load the certs as an independent configuration object (trust store). The issue is it converts them to DER format which makes using this for s2n not possible. It would be nice if we could load and validate certs before passing them off to rustls/s2n in a uniform way. 4. Should `TlsContext` and/or `TrustStore` provider builders? Currently they just self mutate which is a pattern we use in some places (e.g. stalled stream protection config comes to mind). ## Testing New unit tests + existing TLS tests ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
ysaito1001
approved these changes
Mar 4, 2025
There was a problem hiding this comment.
Long-awaited change, delivered in such a well-organized manner. Excellent work!
Error results from cargo semver checks are all expected. Check for semver hazards cannot be run, as expected. We can force merge this to main using the bot user (although now that I think about this, the failure in Check for semver hazards will get in the way of smithy-rs prod release 🤔 )
We could potentially comment out this line in ci.yml as part of this PR and then once a smithy-rs release is done, we could comment in within the backport PR.
landonxjames
approved these changes
Mar 5, 2025
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
drganjoo
approved these changes
Mar 10, 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.
Motivation and Context
Description
Updates the default HTTPS client to be based off hyper 1.x, rustls, and aws-lc. See the GitHub discussion: awslabs/aws-sdk-rust#1257
This PR is a rollup of previously reviewed PRs:
Testing
Checklist
.changelogdirectory, specifying "client," "server," or both in theapplies_tokey..changelogdirectory, specifying "aws-sdk-rust" in theapplies_tokey.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.