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

enable hyper1 as default http client #3939

Merged
merged 17 commits into from
Dec 12, 2024
Merged

enable hyper1 as default http client #3939

merged 17 commits into from
Dec 12, 2024

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Dec 5, 2024

Motivation and Context

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.

@aajtodd aajtodd requested a review from a team as a code owner December 5, 2024 18:39
@aajtodd aajtodd requested a review from a team as a code owner December 5, 2024 19:02
Copy link

github-actions bot commented Dec 5, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Dec 5, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Dec 6, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Dec 6, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Dec 6, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

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

Great work updating various places extensively. Looks like need to get through some of CI checks but the changes look good structurally.

tools/ci-scripts/check-aws-config Show resolved Hide resolved
Comment on lines +60 to +62
// takes precedence over legacy connector if enabled
#[cfg(feature = "default-http-connector")]
let _default = aws_smithy_http_client::default_client();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is clever.

Copy link

github-actions bot commented Dec 6, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@aajtodd aajtodd merged commit ba89a32 into hyper1 Dec 12, 2024
40 of 44 checks passed
@aajtodd aajtodd deleted the hyper1-enable branch December 12, 2024 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants