Skip to content

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Aug 7, 2025

Motivation and Context

awslabs/aws-sdk-rust#169

Description

  • Adds support for automatic support for common proxy env vars (HTTP_PROXY, HTTPS_PROXY, ALL_PROXY, NO_PROXY and their lowercase versions) behind a new BehaviorVersion
  • relocates the TLS providers to their own modules, these are largely the same with the biggest change being wrap_connector returns dedicated connector types for rustls and s2n and the addition of new connect modules containing the proxy tunnel logic for each TLS provider.
    • This results in a bit of repeated logic to handle proxy tunneling but I couldn't find a way to unify the implementations and abstract out just the manual TLS handshake due to type system constraints
  • updates dependency versions in aws-smithy-http-client
  • added a new build_with_connector_fn that allows creating a SharedHttpClient by passing your own function for creating connectors. We didn't have a way to configure aws_smithy_http_client::Connector and turn it into a SharedHttpClient without going through http_client_fn. That works but it doesn't re-use the same caching logic for connectors as we do when going through aws_smithy_http_client::Builder. The alternative would be to make the client builder in aws-smithy-http-client behavior version aware. Doing it the way it is in the PR allows us to externalize this though and handle it in aws-smithy-runtime when creating the default https client

Testing

  • integration tests with mock proxy over localhost
  • tested against mitmproxy using different combinations of env variables and behavior versions

Checklist

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

@aajtodd aajtodd requested review from a team as code owners August 7, 2025 17:50
@aajtodd aajtodd force-pushed the aajtodd/http-proxy branch from 837996a to 8fe0f8f Compare August 8, 2025 12:29
Copy link

github-actions bot commented Aug 8, 2025

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link

github-actions bot commented Aug 8, 2025

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@landonxjames landonxjames left a comment

Choose a reason for hiding this comment

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

Looks good, one question about feature management, but not a blocker.

Feel like this should release concurrently with the change from #4255 because that also feels like it might be an unexpected enough change to warrant a BehaviorVersion (I know the behavior version can't actually get that change since it is just a compile time dependency feature, but still feels like it should be indicated somehow)

rustls-aws-lc = ["dep:rustls", "rustls?/aws_lc_rs", "dep:hyper-rustls", "default-client"]
rustls-aws-lc-fips = ["dep:rustls", "rustls?/fips", "dep:hyper-rustls", "default-client"]
s2n-tls = ["dep:s2n-tls", "dep:s2n-tls-hyper", "default-client"]
rustls-ring = ["dep:rustls", "rustls?/ring", "dep:hyper-rustls", "dep:tokio-rustls", "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.

Feels kind of weird adding tokio dependencies behind a flag that isn't rt-tokio. Would it make sense to introduce a rt-tokio flag that adds these deps and have these features enable that feature?

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 think it depends on whether the feature is bringing in the tokio runtime as a requirement vs using tokio IO traits. (for example hyper-rustls also depends on tokio)

@aajtodd
Copy link
Contributor Author

aajtodd commented Aug 11, 2025

Feel like this should release concurrently with the change from #4255 because that also feels like it might be an unexpected enough change to warrant a BehaviorVersion (I know the behavior version can't actually get that change since it is just a compile time dependency feature, but still feels like it should be indicated somehow)

Feature flags and behavior versions are not the best match but ya we can discuss what makes sense.

@ysaito1001
Copy link
Contributor

Feel like this should release concurrently with the change from #4255 because that also feels like it might be an unexpected enough change to warrant a BehaviorVersion (I know the behavior version can't actually get that change since it is just a compile time dependency feature, but still feels like it should be indicated somehow)

Feature flags and behavior versions are not the best match but ya we can discuss what makes sense.

Good point. My gut is this PR can go independently of PR4255; if we just upgraded rustls to 0.24 (say PR4255 didn't exist), we probably wouldn't add a new BehaviorVersion for a dependency upgrade.

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.

Awesome work. While dev guide could be useful, the API rustdoc seems good for now.

@aajtodd aajtodd force-pushed the aajtodd/http-proxy branch from 117a938 to b0afb34 Compare August 12, 2025 14:22
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 force-pushed the aajtodd/http-proxy branch from 715640b to 22c4386 Compare August 13, 2025 18:36
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@aajtodd aajtodd force-pushed the aajtodd/http-proxy branch from 22c4386 to 7b929c2 Compare August 14, 2025 15:37
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@aajtodd aajtodd merged commit ab69f19 into main Aug 14, 2025
45 checks passed
@aajtodd aajtodd deleted the aajtodd/http-proxy branch August 14, 2025 17:19
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.

3 participants