From 9587dbc0611983cab56c1c33b479a59a9065f752 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Thu, 30 Nov 2023 15:03:42 -0800 Subject: [PATCH] Lazy initialize the default HTTP client (#3262) This change initializes the TLS trust certs at base client validation time rather than upon creation of the default HTTP client runtime plugin so that if the default is overridden, that initialization doesn't need to take place. This is especially helpful on MacOS where that initialization takes approximately 100 milliseconds. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: Russell Cohen --- CHANGELOG.next.toml | 12 +++ .../s3/tests/service_timeout_overrides.rs | 12 ++- .../rustdoc/validate_base_client_config.md | 9 ++ .../rustdoc/validate_final_config.md | 7 ++ .../aws-smithy-runtime-api/src/client/http.rs | 43 +++++++- .../src/client/identity.rs | 15 +-- .../src/client/runtime_components.rs | 13 +-- .../src/client/http/hyper_014.rs | 102 +++++++++++------- .../src/client/http/test_util/never.rs | 2 + 9 files changed, 149 insertions(+), 66 deletions(-) create mode 100644 rust-runtime/aws-smithy-runtime-api/rustdoc/validate_base_client_config.md create mode 100644 rust-runtime/aws-smithy-runtime-api/rustdoc/validate_final_config.md diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 27abec50c7..fb9213af12 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -11,6 +11,18 @@ # meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"} # 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" + [[aws-sdk-rust]] message = """Client creation now takes microseconds instead of milliseconds. Previously, it would take 2-3 milliseconds for each client instantiation due to time spent compiling regexes. diff --git a/aws/sdk/integration-tests/s3/tests/service_timeout_overrides.rs b/aws/sdk/integration-tests/s3/tests/service_timeout_overrides.rs index 9e4808118c..40a4fb0578 100644 --- a/aws/sdk/integration-tests/s3/tests/service_timeout_overrides.rs +++ b/aws/sdk/integration-tests/s3/tests/service_timeout_overrides.rs @@ -6,6 +6,8 @@ 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; @@ -13,9 +15,10 @@ 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")) @@ -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 @@ -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(); @@ -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 diff --git a/rust-runtime/aws-smithy-runtime-api/rustdoc/validate_base_client_config.md b/rust-runtime/aws-smithy-runtime-api/rustdoc/validate_base_client_config.md new file mode 100644 index 0000000000..4ac61d51b0 --- /dev/null +++ b/rust-runtime/aws-smithy-runtime-api/rustdoc/validate_base_client_config.md @@ -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 diff --git a/rust-runtime/aws-smithy-runtime-api/rustdoc/validate_final_config.md b/rust-runtime/aws-smithy-runtime-api/rustdoc/validate_final_config.md new file mode 100644 index 0000000000..768bda5dd5 --- /dev/null +++ b/rust-runtime/aws-smithy-runtime-api/rustdoc/validate_final_config.md @@ -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 diff --git a/rust-runtime/aws-smithy-runtime-api/src/client/http.rs b/rust-runtime/aws-smithy-runtime-api/src/client/http.rs index 1e90d40fb1..aa66f79699 100644 --- a/rust-runtime/aws-smithy-runtime-api/src/client/http.rs +++ b/rust-runtime/aws-smithy-runtime-api/src/client/http.rs @@ -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; @@ -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. @@ -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); diff --git a/rust-runtime/aws-smithy-runtime-api/src/client/identity.rs b/rust-runtime/aws-smithy-runtime-api/src/client/identity.rs index 669c4490ab..3632029e7b 100644 --- a/rust-runtime/aws-smithy-runtime-api/src/client/identity.rs +++ b/rust-runtime/aws-smithy-runtime-api/src/client/identity.rs @@ -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, @@ -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, diff --git a/rust-runtime/aws-smithy-runtime-api/src/client/runtime_components.rs b/rust-runtime/aws-smithy-runtime-api/src/client/runtime_components.rs index 0b0c58c321..19471ddf6b 100644 --- a/rust-runtime/aws-smithy-runtime-api/src/client/runtime_components.rs +++ b/rust-runtime/aws-smithy-runtime-api/src/client/runtime_components.rs @@ -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, @@ -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, diff --git a/rust-runtime/aws-smithy-runtime/src/client/http/hyper_014.rs b/rust-runtime/aws-smithy-runtime/src/client/http/hyper_014.rs index 344c60714a..1359beb4a3 100644 --- a/rust-runtime/aws-smithy-runtime/src/client/http/hyper_014.rs +++ b/rust-runtime/aws-smithy-runtime/src/client/http/hyper_014.rs @@ -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; @@ -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, - > = once_cell::sync::Lazy::new(|| { + > = once_cell::sync::Lazy::new(default_tls); + + fn default_tls() -> HttpsConnector { 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, @@ -474,6 +481,20 @@ where C::Future: Unpin + Send + 'static, C::Error: Into, { + 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, @@ -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); } @@ -535,10 +563,13 @@ impl HyperClientBuilder { self } - /// Create a [`HyperConnector`] with the default rustls HTTPS implementation. + /// Create a hyper client with the default rustls HTTPS implementation. + /// + /// The trusted certificates will be loaded later when this becomes the selected + /// HTTP client for a Smithy client. #[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. @@ -555,14 +586,9 @@ impl HyperClientBuilder { C::Future: Unpin + Send + 'static, C::Error: Into, { - 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(self, tcp_connector_fn: F) -> SharedHttpClient where F: Fn() -> C + Send + Sync + 'static, @@ -952,6 +978,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}; @@ -993,7 +1020,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 { diff --git a/rust-runtime/aws-smithy-runtime/src/client/http/test_util/never.rs b/rust-runtime/aws-smithy-runtime/src/client/http/test_util/never.rs index d61608cbeb..128579e384 100644 --- a/rust-runtime/aws-smithy-runtime/src/client/http/test_util/never.rs +++ b/rust-runtime/aws-smithy-runtime/src/client/http/test_util/never.rs @@ -146,6 +146,7 @@ async fn never_tcp_connector_plugs_into_hyper_014() { use super::*; use crate::client::http::hyper_014::HyperClientBuilder; use aws_smithy_async::rt::sleep::TokioSleep; + use aws_smithy_async::time::SystemTimeSource; use aws_smithy_runtime_api::client::runtime_components::RuntimeComponentsBuilder; use std::time::Duration; @@ -153,6 +154,7 @@ async fn never_tcp_connector_plugs_into_hyper_014() { let client = HyperClientBuilder::new().build(NeverTcpConnector::new()); let components = RuntimeComponentsBuilder::for_tests() .with_sleep_impl(Some(TokioSleep::new())) + .with_time_source(Some(SystemTimeSource::new())) .build() .unwrap(); let http_connector = client.http_connector(