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

refactor hyper1 client builder to allow multiple TLS providers and add new config #3914

Merged
merged 13 commits into from
Nov 20, 2024

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Nov 15, 2024

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 (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.

@aajtodd aajtodd requested review from a team as code owners November 15, 2024 21:06
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

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.

Nicely refactored!

sleep_impl: Option<SharedAsyncSleep>,
client_builder: Option<hyper_util::client::legacy::Builder>,
enable_tcp_nodelay: bool,
interface: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this?

@aajtodd aajtodd merged commit 9833eb2 into hyper1 Nov 20, 2024
36 of 44 checks passed
@aajtodd aajtodd deleted the atodd/http-client-builder branch November 20, 2024 17:55
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