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

Lazy initialize the default HTTP client #3262

Merged
merged 8 commits into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,16 @@
# message = "Fix typos in module documentation for generated crates"
# references = ["smithy-rs#920"]
# meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"}
# author = "rcoh"
# author = "rcoh"

[[aws-sdk-rust]]
message = "Loading native TLS trusted certs for the default HTTP client now only occurs if the default HTTP client is not overridden in config."
references = ["smithy-rs#3262"]
meta = { "breaking" = false, "tada" = false, "bug" = true }
author = "jdisanti"

[[smithy-rs]]
message = "Loading native TLS trusted certs for the default HTTP client now only occurs if the default HTTP client is not overridden in config."
references = ["smithy-rs#3262"]
meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "client" }
author = "jdisanti"
6 changes: 6 additions & 0 deletions aws/rust-runtime/aws-config/src/imds/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -990,12 +990,18 @@ pub(crate) mod test {
use crate::imds::client::ImdsError;
use aws_smithy_types::error::display::DisplayErrorContext;
use std::time::SystemTime;
let (_guard, _) = capture_test_logs();

let client = Client::builder()
// 240.* can never be resolved
.endpoint("http://240.0.0.0")
.expect("valid uri")
.build();
// preload the connector so that doesn't impact timing
let _resp = client
.get("/latest/metadata")
.await
.expect_err("240.0.0.0 will never resolve");
jdisanti marked this conversation as resolved.
Show resolved Hide resolved
let now = SystemTime::now();
let resp = client
.get("/latest/metadata")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,19 @@
use aws_credential_types::provider::SharedCredentialsProvider;
use aws_credential_types::Credentials;
use aws_smithy_async::rt::sleep::{SharedAsyncSleep, TokioSleep};
use aws_smithy_runtime::client::http::test_util::NeverClient;
use aws_smithy_runtime::test_util::capture_test_logs::capture_test_logs;
use aws_smithy_runtime_api::client::result::SdkError;
use aws_smithy_types::timeout::TimeoutConfig;
use aws_types::region::Region;
use aws_types::SdkConfig;
use std::time::Duration;
use tokio::time::Instant;

/// Use a 5 second operation timeout on SdkConfig and a 0ms connect timeout on the service config
/// Use a 5 second operation timeout on SdkConfig and a 0ms operation timeout on the service config
#[tokio::test]
async fn timeouts_can_be_set_by_service() {
let (_guard, _) = capture_test_logs();
let sdk_config = SdkConfig::builder()
.credentials_provider(SharedCredentialsProvider::new(Credentials::for_tests()))
.region(Region::from_static("us-east-1"))
Expand All @@ -25,6 +28,7 @@ async fn timeouts_can_be_set_by_service() {
.operation_timeout(Duration::from_secs(5))
.build(),
)
.http_client(NeverClient::new())
// ip that
.endpoint_url(
// Emulate a connect timeout error by hitting an unroutable IP
Expand All @@ -34,7 +38,7 @@ async fn timeouts_can_be_set_by_service() {
let config = aws_sdk_s3::config::Builder::from(&sdk_config)
.timeout_config(
TimeoutConfig::builder()
.connect_timeout(Duration::from_secs(0))
.operation_timeout(Duration::from_secs(0))
.build(),
)
.build();
Expand All @@ -48,8 +52,8 @@ async fn timeouts_can_be_set_by_service() {
.await
.expect_err("unroutable IP should timeout");
match err {
SdkError::DispatchFailure(err) => assert!(err.is_timeout()),
// if the connect timeout is not respected, this times out after 1 second because of the operation timeout with `SdkError::Timeout`
SdkError::TimeoutError(_err) => { /* ok */ }
// if the connect timeout is not respected, this times out after 5 seconds because of the operation timeout with `SdkError::Timeout`
_other => panic!("unexpected error: {:?}", _other),
}
// there should be a 0ms timeout, we gotta set some stuff up. Just want to make sure
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
Validate the base client configuration.

This gets called upon client construction. The full config may not be available at
this time (hence why it has [`RuntimeComponentsBuilder`] as an argument rather
than [`RuntimeComponents`]). Any error returned here will become a panic
in the client constructor.

[`RuntimeComponentsBuilder`]: crate::client::runtime_components::RuntimeComponentsBuilder
[`RuntimeComponents`]: crate::client::runtime_components::RuntimeComponents
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Validate the final client configuration.

This gets called immediately after the [`Intercept::read_before_execution`] trait hook
when the final configuration has been resolved. Any error returned here will
cause the operation to return that error.

[`Intercept::read_before_execution`]: crate::client::interceptors::Intercept::read_before_execution
43 changes: 41 additions & 2 deletions rust-runtime/aws-smithy-runtime-api/src/client/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,13 @@
//! [`tower`]: https://crates.io/crates/tower
//! [`aws-smithy-runtime`]: https://crates.io/crates/aws-smithy-runtime

use crate::box_error::BoxError;
use crate::client::orchestrator::{HttpRequest, HttpResponse};
use crate::client::result::ConnectorError;
use crate::client::runtime_components::sealed::ValidateConfig;
use crate::client::runtime_components::RuntimeComponents;
use crate::client::runtime_components::{RuntimeComponents, RuntimeComponentsBuilder};
use crate::impl_shared_conversions;
use aws_smithy_types::config_bag::ConfigBag;
use std::fmt;
use std::sync::Arc;
use std::time::Duration;
Expand Down Expand Up @@ -143,6 +145,26 @@ pub trait HttpClient: Send + Sync + fmt::Debug {
settings: &HttpConnectorSettings,
components: &RuntimeComponents,
) -> SharedHttpConnector;

#[doc = include_str!("../../rustdoc/validate_base_client_config.md")]
fn validate_base_client_config(
&self,
runtime_components: &RuntimeComponentsBuilder,
cfg: &ConfigBag,
) -> Result<(), BoxError> {
let _ = (runtime_components, cfg);
Ok(())
}

#[doc = include_str!("../../rustdoc/validate_final_config.md")]
fn validate_final_config(
&self,
runtime_components: &RuntimeComponents,
cfg: &ConfigBag,
) -> Result<(), BoxError> {
let _ = (runtime_components, cfg);
Ok(())
}
}

/// Shared HTTP client for use across multiple clients and requests.
Expand Down Expand Up @@ -170,7 +192,24 @@ impl HttpClient for SharedHttpClient {
}
}

impl ValidateConfig for SharedHttpClient {}
impl ValidateConfig for SharedHttpClient {
fn validate_base_client_config(
&self,
runtime_components: &super::runtime_components::RuntimeComponentsBuilder,
cfg: &aws_smithy_types::config_bag::ConfigBag,
) -> Result<(), crate::box_error::BoxError> {
self.selector
.validate_base_client_config(runtime_components, cfg)
}

fn validate_final_config(
&self,
runtime_components: &RuntimeComponents,
cfg: &aws_smithy_types::config_bag::ConfigBag,
) -> Result<(), crate::box_error::BoxError> {
self.selector.validate_final_config(runtime_components, cfg)
}
}

impl_shared_conversions!(convert SharedHttpClient from HttpClient using SharedHttpClient::new);

Expand Down
15 changes: 2 additions & 13 deletions rust-runtime/aws-smithy-runtime-api/src/client/identity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,7 @@ pub trait ResolveCachedIdentity: fmt::Debug + Send + Sync {
config_bag: &'a ConfigBag,
) -> IdentityFuture<'a>;

/// Validate the base client configuration for this implementation.
///
/// This gets called upon client construction. The full config may not be available at
/// this time (hence why it has [`RuntimeComponentsBuilder`] as an argument rather
/// than [`RuntimeComponents`]). Any error returned here will become a panic
/// in the client constructor.
#[doc = include_str!("../../rustdoc/validate_base_client_config.md")]
fn validate_base_client_config(
&self,
runtime_components: &RuntimeComponentsBuilder,
Expand All @@ -79,13 +74,7 @@ pub trait ResolveCachedIdentity: fmt::Debug + Send + Sync {
Ok(())
}

/// Validate the final client configuration for this implementation.
///
/// This gets called immediately after the [`Intercept::read_before_execution`] trait hook
/// when the final configuration has been resolved. Any error returned here will
/// cause the operation to return that error.
///
/// [`Intercept::read_before_execution`]: crate::client::interceptors::Intercept::read_before_execution
#[doc = include_str!("../../rustdoc/validate_final_config.md")]
fn validate_final_config(
&self,
runtime_components: &RuntimeComponents,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,7 @@ pub(crate) mod sealed {
/// This trait can be used to validate that certain required components or config values
/// are available, and provide an error with helpful instructions if they are not.
pub trait ValidateConfig: fmt::Debug + Send + Sync {
/// Validate the base client configuration.
///
/// This gets called upon client construction. The full config may not be available at
/// this time (hence why it has [`RuntimeComponentsBuilder`] as an argument rather
/// than [`RuntimeComponents`]). Any error returned here will become a panic
/// in the client constructor.
#[doc = include_str!("../../rustdoc/validate_base_client_config.md")]
fn validate_base_client_config(
&self,
runtime_components: &RuntimeComponentsBuilder,
Expand All @@ -59,11 +54,7 @@ pub(crate) mod sealed {
Ok(())
}

/// Validate the final client configuration.
///
/// This gets called immediately after the [`Intercept::read_before_execution`] trait hook
/// when the final configuration has been resolved. Any error returned here will
/// cause the operation to return that error.
#[doc = include_str!("../../rustdoc/validate_final_config.md")]
fn validate_final_config(
&self,
runtime_components: &RuntimeComponents,
Expand Down
101 changes: 65 additions & 36 deletions rust-runtime/aws-smithy-runtime/src/client/http/hyper_014.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,12 @@ use aws_smithy_runtime_api::client::http::{
};
use aws_smithy_runtime_api::client::orchestrator::{HttpRequest, HttpResponse};
use aws_smithy_runtime_api::client::result::ConnectorError;
use aws_smithy_runtime_api::client::runtime_components::RuntimeComponents;
use aws_smithy_runtime_api::client::runtime_components::{
RuntimeComponents, RuntimeComponentsBuilder,
};
use aws_smithy_runtime_api::shared::IntoShared;
use aws_smithy_types::body::SdkBody;
use aws_smithy_types::config_bag::ConfigBag;
use aws_smithy_types::error::display::DisplayErrorContext;
use aws_smithy_types::retry::ErrorKind;
use h2::Reason;
Expand All @@ -36,38 +39,42 @@ use tokio::io::{AsyncRead, AsyncWrite};
mod default_connector {
use aws_smithy_async::rt::sleep::SharedAsyncSleep;
use aws_smithy_runtime_api::client::http::HttpConnectorSettings;
use hyper_0_14::client::HttpConnector;
use hyper_rustls::HttpsConnector;

// Creating a `with_native_roots` HTTP client takes 300ms on OS X. Cache this so that we
// don't need to repeatedly incur that cost.
static HTTPS_NATIVE_ROOTS: once_cell::sync::Lazy<
pub(crate) static HTTPS_NATIVE_ROOTS: once_cell::sync::Lazy<
hyper_rustls::HttpsConnector<hyper_0_14::client::HttpConnector>,
> = once_cell::sync::Lazy::new(|| {
> = once_cell::sync::Lazy::new(default_tls);

fn default_tls() -> HttpsConnector<HttpConnector> {
use hyper_rustls::ConfigBuilderExt;
hyper_rustls::HttpsConnectorBuilder::new()
.with_tls_config(
rustls::ClientConfig::builder()
.with_cipher_suites(&[
// TLS1.3 suites
rustls::cipher_suite::TLS13_AES_256_GCM_SHA384,
rustls::cipher_suite::TLS13_AES_128_GCM_SHA256,
// TLS1.2 suites
rustls::cipher_suite::TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
rustls::cipher_suite::TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
rustls::cipher_suite::TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
rustls::cipher_suite::TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
rustls::cipher_suite::TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,
])
.with_safe_default_kx_groups()
.with_safe_default_protocol_versions()
.expect("Error with the TLS configuration. Please file a bug report under https://github.com/smithy-lang/smithy-rs/issues.")
.with_native_roots()
.with_no_client_auth()
)
.https_or_http()
.enable_http1()
.enable_http2()
.build()
});
.with_tls_config(
rustls::ClientConfig::builder()
.with_cipher_suites(&[
// TLS1.3 suites
rustls::cipher_suite::TLS13_AES_256_GCM_SHA384,
rustls::cipher_suite::TLS13_AES_128_GCM_SHA256,
// TLS1.2 suites
rustls::cipher_suite::TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
rustls::cipher_suite::TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
rustls::cipher_suite::TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
rustls::cipher_suite::TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
rustls::cipher_suite::TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,
])
.with_safe_default_kx_groups()
.with_safe_default_protocol_versions()
.expect("Error with the TLS configuration. Please file a bug report under https://github.com/smithy-lang/smithy-rs/issues.")
.with_native_roots()
.with_no_client_auth()
)
.https_or_http()
.enable_http1()
.enable_http2()
.build()
}

pub(super) fn base(
settings: &HttpConnectorSettings,
Expand Down Expand Up @@ -474,6 +481,20 @@ where
C::Future: Unpin + Send + 'static,
C::Error: Into<BoxError>,
{
fn validate_base_client_config(
&self,
_: &RuntimeComponentsBuilder,
_: &ConfigBag,
) -> Result<(), BoxError> {
// Initialize the TCP connector at this point so that native certs load
// at client initialization time instead of upon first request. We do it
// here rather than at construction so that it won't run if this is not
// the selected HTTP client for the base config (for example, if this was
// the default HTTP client, and it was overridden by a later plugin).
let _ = (self.tcp_connector_fn)();
Ok(())
}

fn http_connector(
&self,
settings: &HttpConnectorSettings,
Expand All @@ -490,7 +511,14 @@ where
.connector_settings(settings.clone());
builder.set_sleep_impl(components.sleep_impl());

let start = components.time_source().map(|ts| ts.now());
let tcp_connector = (self.tcp_connector_fn)();
let end = components.time_source().map(|ts| ts.now());
if let (Some(start), Some(end)) = (start, end) {
if let Ok(elapsed) = end.duration_since(start) {
tracing::debug!("new TCP connector created in {:?}", elapsed);
}
}
let connector = SharedHttpConnector::new(builder.build(tcp_connector));
cache.insert(key.clone(), connector);
}
Expand Down Expand Up @@ -535,10 +563,12 @@ impl HyperClientBuilder {
self
}

/// Create a [`HyperConnector`] with the default rustls HTTPS implementation.
/// Create a hyper client with the default rustls HTTPS implementation.
///
/// This client will immediately load trusted certificates.
jdisanti marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(feature = "tls-rustls")]
pub fn build_https(self) -> SharedHttpClient {
self.build(default_connector::https())
self.build_with_fn(default_connector::https)
}

/// Create a [`SharedHttpClient`] from this builder and a given connector.
Expand All @@ -555,14 +585,9 @@ impl HyperClientBuilder {
C::Future: Unpin + Send + 'static,
C::Error: Into<BoxError>,
{
SharedHttpClient::new(HyperClient {
connector_cache: RwLock::new(HashMap::new()),
client_builder: self.client_builder.unwrap_or_default(),
tcp_connector_fn: move || tcp_connector.clone(),
})
self.build_with_fn(move || tcp_connector.clone())
}

#[cfg(all(test, feature = "test-util"))]
fn build_with_fn<C, F>(self, tcp_connector_fn: F) -> SharedHttpClient
where
F: Fn() -> C + Send + Sync + 'static,
Expand Down Expand Up @@ -952,6 +977,7 @@ mod timeout_middleware {
mod test {
use super::*;
use crate::client::http::test_util::NeverTcpConnector;
use aws_smithy_async::time::SystemTimeSource;
use aws_smithy_runtime_api::client::runtime_components::RuntimeComponentsBuilder;
use http::Uri;
use hyper_0_14::client::connect::{Connected, Connection};
Expand Down Expand Up @@ -993,7 +1019,10 @@ mod test {
];

// Kick off thousands of parallel tasks that will try to create a connector
let components = RuntimeComponentsBuilder::for_tests().build().unwrap();
let components = RuntimeComponentsBuilder::for_tests()
.with_time_source(Some(SystemTimeSource::new()))
.build()
.unwrap();
let mut handles = Vec::new();
for setting in &settings {
for _ in 0..1000 {
Expand Down
Loading