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

consolidate http client implementation(s) #3866

Merged
merged 19 commits into from
Oct 14, 2024
Merged

consolidate http client implementation(s) #3866

merged 19 commits into from
Oct 14, 2024

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Oct 7, 2024

Motivation and Context

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.

Copy link

github-actions bot commented Oct 7, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Oct 7, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Oct 8, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Oct 8, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Oct 8, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Oct 8, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Oct 9, 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.

Comment on lines -7 to -10
default = "deny"
unlicensed = "deny"
copyleft = "deny"
allow-osi-fsf-free = "neither"
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we keep these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated cargo-deny in the docker image to try and fix the panic happening due to some edge case triggered by aws-smithy-http-client. These are all the default now and an error on latest version. See EmbarkStudios/cargo-deny#611

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 step forward!

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@aajtodd aajtodd merged commit e616942 into hyper1 Oct 14, 2024
40 of 44 checks passed
@aajtodd aajtodd deleted the http-client branch October 14, 2024 17:32
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