diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index eec4009c22..394665e33e 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -665,6 +665,12 @@ references = ["smithy-rs#2865"] meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "server" } author = "david-perez" +[[aws-sdk-rust]] +message = "**Behavior change**: Credential providers now share the HTTP connector used by the SDK. If you want to keep a separate connector for clients, use `::ConfigBuilder::http_connector` when constructing the client." +references = ["aws-sdk-rust#579", "aws-sdk-rust#338"] +meta = { "breaking" = true, "tada" = false, "bug" = false } +author = "rcoh" + [[smithy-rs]] message = """ [RestJson1](https://awslabs.github.io/smithy/2.0/aws/protocols/aws-restjson1-protocol.html#operation-error-serialization) server SDKs now serialize only the [shape name](https://smithy.io/2.0/spec/model.html#shape-id) in operation error responses. Previously (from versions 0.52.0 to 0.55.4), the full shape ID was rendered. diff --git a/aws/rust-runtime/aws-config/src/default_provider/app_name.rs b/aws/rust-runtime/aws-config/src/default_provider/app_name.rs index 7d91c6d363..041d0f8261 100644 --- a/aws/rust-runtime/aws-config/src/default_provider/app_name.rs +++ b/aws/rust-runtime/aws-config/src/default_provider/app_name.rs @@ -118,12 +118,9 @@ mod tests { async fn profile_name_override() { let fs = Fs::from_slice(&[("test_config", "[profile custom]\nsdk_ua_app_id = correct")]); let conf = crate::from_env() - .configure( - ProviderConfig::empty() - .with_fs(fs) - .with_sleep(InstantSleep) - .with_http_connector(no_traffic_connector()), - ) + .sleep_impl(InstantSleep) + .fs(fs) + .http_connector(no_traffic_connector()) .profile_name("custom") .profile_files( ProfileFiles::builder() diff --git a/aws/rust-runtime/aws-config/src/lib.rs b/aws/rust-runtime/aws-config/src/lib.rs index e7c035ea80..9185672577 100644 --- a/aws/rust-runtime/aws-config/src/lib.rs +++ b/aws/rust-runtime/aws-config/src/lib.rs @@ -161,6 +161,7 @@ mod loader { use aws_smithy_types::timeout::TimeoutConfig; use aws_types::app_name::AppName; use aws_types::docs_for; + use aws_types::os_shim_internal::{Env, Fs}; use aws_types::SdkConfig; use crate::connector::default_connector; @@ -205,6 +206,8 @@ mod loader { use_fips: Option, use_dual_stack: Option, time_source: Option, + env: Option, + fs: Option, } impl ConfigLoader { @@ -281,35 +284,43 @@ mod loader { self } - /// Override the [`HttpConnector`] for this [`ConfigLoader`]. The connector will be used when - /// sending operations. This **does not set** the HTTP connector used by config providers. - /// To change that connector, use [ConfigLoader::configure]. + /// Override the [`HttpConnector`] for this [`ConfigLoader`]. The connector will be used for + /// both AWS services and credential providers. When [`HttpConnector::ConnectorFn`] is used, + /// the connector will be lazily instantiated as needed based on the provided settings. /// + /// **Note**: In order to take advantage of late-configured timeout settings, you MUST use + /// [`HttpConnector::ConnectorFn`] + /// when configuring this connector. + /// + /// If you wish to use a separate connector when creating clients, use the client-specific config. /// ## Examples /// ```no_run - /// # #[cfg(feature = "client-hyper")] + /// # use aws_smithy_async::rt::sleep::SharedAsyncSleep; + /// use aws_smithy_client::http_connector::HttpConnector; + /// #[cfg(feature = "client-hyper")] /// # async fn create_config() { /// use std::time::Duration; /// use aws_smithy_client::{Client, hyper_ext}; /// use aws_smithy_client::erase::DynConnector; /// use aws_smithy_client::http_connector::ConnectorSettings; /// - /// let https_connector = hyper_rustls::HttpsConnectorBuilder::new() - /// .with_webpki_roots() - /// .https_only() - /// .enable_http1() - /// .enable_http2() - /// .build(); - /// let smithy_connector = hyper_ext::Adapter::builder() - /// // Optionally set things like timeouts as well - /// .connector_settings( - /// ConnectorSettings::builder() - /// .connect_timeout(Duration::from_secs(5)) - /// .build() - /// ) - /// .build(https_connector); + /// let connector_fn = |settings: &ConnectorSettings, sleep: Option| { + /// let https_connector = hyper_rustls::HttpsConnectorBuilder::new() + /// .with_webpki_roots() + /// // NOTE: setting `https_only()` will not allow this connector to work with IMDS. + /// .https_only() + /// .enable_http1() + /// .enable_http2() + /// .build(); + /// let mut smithy_connector = hyper_ext::Adapter::builder() + /// // Optionally set things like timeouts as well + /// .connector_settings(settings.clone()); + /// smithy_connector.set_sleep_impl(sleep); + /// Some(DynConnector::new(smithy_connector.build(https_connector))) + /// }; + /// let connector = HttpConnector::ConnectorFn(std::sync::Arc::new(connector_fn)); /// let sdk_config = aws_config::from_env() - /// .http_connector(smithy_connector) + /// .http_connector(connector) /// .load() /// .await; /// # } @@ -532,6 +543,9 @@ mod loader { /// let shared_config = aws_config::from_env().configure(provider_config).load().await; /// # } /// ``` + #[deprecated( + note = "Use setters on this builder instead. configure is very hard to use correctly." + )] pub fn configure(mut self, provider_config: ProviderConfig) -> Self { self.provider_config = Some(provider_config); self @@ -547,9 +561,35 @@ mod loader { /// This means that if you provide a region provider that does not return a region, no region will /// be set in the resulting [`SdkConfig`](aws_types::SdkConfig) pub async fn load(self) -> SdkConfig { + let http_connector = self + .http_connector + .unwrap_or_else(|| HttpConnector::ConnectorFn(Arc::new(default_connector))); + + let time_source = self.time_source.unwrap_or_default(); + + let sleep_impl = if self.sleep.is_some() { + self.sleep + } else { + if default_async_sleep().is_none() { + tracing::warn!( + "An implementation of AsyncSleep was requested by calling default_async_sleep \ + but no default was set. + This happened when ConfigLoader::load was called during Config construction. \ + You can fix this by setting a sleep_impl on the ConfigLoader before calling \ + load or by enabling the rt-tokio feature" + ); + } + default_async_sleep() + }; + let conf = self .provider_config - .unwrap_or_default() + .unwrap_or_else(|| { + ProviderConfig::init(time_source.clone(), sleep_impl.clone()) + .with_fs(self.fs.unwrap_or_default()) + .with_env(self.env.unwrap_or_default()) + .with_http_connector(http_connector.clone()) + }) .with_profile_config(self.profile_files_override, self.profile_name_override); let region = if let Some(provider) = self.region { provider.region().await @@ -579,21 +619,6 @@ mod loader { .await }; - let sleep_impl = if self.sleep.is_some() { - self.sleep - } else { - if default_async_sleep().is_none() { - tracing::warn!( - "An implementation of AsyncSleep was requested by calling default_async_sleep \ - but no default was set. - This happened when ConfigLoader::load was called during Config construction. \ - You can fix this by setting a sleep_impl on the ConfigLoader before calling \ - load or by enabling the rt-tokio feature" - ); - } - default_async_sleep() - }; - let timeout_config = if let Some(timeout_config) = self.timeout_config { timeout_config } else { @@ -603,10 +628,6 @@ mod loader { .await }; - let http_connector = self - .http_connector - .unwrap_or_else(|| HttpConnector::ConnectorFn(Arc::new(default_connector))); - let credentials_provider = match self.credentials_provider { CredentialsProviderOption::Set(provider) => Some(provider), CredentialsProviderOption::NotSet => { @@ -641,13 +662,11 @@ mod loader { use_dual_stack_provider(&conf).await }; - let ts = self.time_source.unwrap_or_default(); - let mut builder = SdkConfig::builder() .region(region) .retry_config(retry_config) .timeout_config(timeout_config) - .time_source(ts) + .time_source(time_source) .http_connector(http_connector); builder.set_app_name(app_name); @@ -661,18 +680,35 @@ mod loader { } } + #[cfg(test)] + impl ConfigLoader { + pub(crate) fn env(mut self, env: Env) -> Self { + self.env = Some(env); + self + } + + pub(crate) fn fs(mut self, fs: Fs) -> Self { + self.fs = Some(fs); + self + } + } + #[cfg(test)] mod test { use aws_credential_types::provider::ProvideCredentials; use aws_smithy_async::rt::sleep::TokioSleep; + use aws_smithy_async::time::{StaticTimeSource, TimeSource}; use aws_smithy_client::erase::DynConnector; use aws_smithy_client::never::NeverConnector; + use aws_smithy_client::test_connection::infallible_connection_fn; use aws_types::app_name::AppName; use aws_types::os_shim_internal::{Env, Fs}; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + use std::time::{SystemTime, UNIX_EPOCH}; use tracing_test::traced_test; use crate::profile::profile_file::{ProfileFileKind, ProfileFiles}; - use crate::provider_config::ProviderConfig; use crate::test_case::{no_traffic_connector, InstantSleep}; use crate::{from_env, ConfigLoader}; @@ -688,13 +724,10 @@ mod loader { let fs = Fs::from_slice(&[("test_config", "[profile custom]\nsdk-ua-app-id = correct")]); let loader = from_env() - .configure( - ProviderConfig::empty() - .with_sleep(TokioSleep::new()) - .with_env(env) - .with_fs(fs) - .with_http_connector(DynConnector::new(NeverConnector::new())), - ) + .sleep_impl(TokioSleep::new()) + .env(env) + .fs(fs) + .http_connector(DynConnector::new(NeverConnector::new())) .profile_name("custom") .profile_files( ProfileFiles::builder() @@ -734,11 +767,9 @@ mod loader { } fn base_conf() -> ConfigLoader { - from_env().configure( - ProviderConfig::empty() - .with_sleep(InstantSleep) - .with_http_connector(no_traffic_connector()), - ) + from_env() + .sleep_impl(InstantSleep) + .http_connector(no_traffic_connector()) } #[tokio::test] @@ -770,5 +801,54 @@ mod loader { assert!(config.credentials_cache().is_none()); assert!(config.credentials_provider().is_none()); } + + #[tokio::test] + async fn connector_is_shared() { + let num_requests = Arc::new(AtomicUsize::new(0)); + let movable = num_requests.clone(); + let conn = infallible_connection_fn(move |_req| { + movable.fetch_add(1, Ordering::Relaxed); + http::Response::new("ok!") + }); + let config = from_env().http_connector(conn.clone()).load().await; + config + .credentials_provider() + .unwrap() + .provide_credentials() + .await + .expect_err("no traffic is allowed"); + let num_requests = num_requests.load(Ordering::Relaxed); + assert!(num_requests > 0, "{}", num_requests); + } + + #[tokio::test] + async fn time_source_is_passed() { + #[derive(Debug)] + struct PanicTs; + impl TimeSource for PanicTs { + fn now(&self) -> SystemTime { + panic!("timesource-was-used") + } + } + let config = from_env() + .sleep_impl(InstantSleep) + .time_source(StaticTimeSource::new(UNIX_EPOCH)) + .http_connector(no_traffic_connector()) + .load() + .await; + // assert that the innards contain the customized fields + for inner in ["InstantSleep", "StaticTimeSource"] { + assert!( + format!("{:#?}", config.credentials_cache()).contains(inner), + "{:#?}", + config.credentials_cache() + ); + assert!( + format!("{:#?}", config.credentials_provider()).contains(inner), + "{:#?}", + config.credentials_cache() + ); + } + } } } diff --git a/aws/rust-runtime/aws-config/src/provider_config.rs b/aws/rust-runtime/aws-config/src/provider_config.rs index 3712376b42..f1caa75e51 100644 --- a/aws/rust-runtime/aws-config/src/provider_config.rs +++ b/aws/rust-runtime/aws-config/src/provider_config.rs @@ -150,6 +150,21 @@ impl ProviderConfig { } } + /// Initializer for ConfigBag to avoid possibly setting incorrect defaults. + pub(crate) fn init(time_source: SharedTimeSource, sleep: Option) -> Self { + Self { + parsed_profile: Default::default(), + profile_files: ProfileFiles::default(), + env: Env::default(), + fs: Fs::default(), + time_source, + connector: HttpConnector::Prebuilt(None), + sleep, + region: None, + profile_name_override: None, + } + } + /// Create a default provider config with the region region automatically loaded from the default chain. /// /// # Examples @@ -270,10 +285,7 @@ impl ProviderConfig { self.with_region(provider_chain.region().await) } - // these setters are doc(hidden) because they only exist for tests - - #[doc(hidden)] - pub fn with_fs(self, fs: Fs) -> Self { + pub(crate) fn with_fs(self, fs: Fs) -> Self { ProviderConfig { parsed_profile: Default::default(), fs, @@ -281,8 +293,7 @@ impl ProviderConfig { } } - #[cfg(test)] - pub fn with_env(self, env: Env) -> Self { + pub(crate) fn with_env(self, env: Env) -> Self { ProviderConfig { parsed_profile: Default::default(), env, @@ -303,14 +314,11 @@ impl ProviderConfig { /// Override the HTTPS connector for this configuration /// - /// **Warning**: Use of this method will prevent you from taking advantage of the HTTP connect timeouts. - /// Consider [`ProviderConfig::with_tcp_connector`]. - /// - /// # Stability - /// This method is expected to change to support HTTP configuration. - pub fn with_http_connector(self, connector: DynConnector) -> Self { + /// **Note**: In order to take advantage of late-configured timeout settings, use [`HttpConnector::ConnectorFn`] + /// when configuring this connector. + pub fn with_http_connector(self, connector: impl Into) -> Self { ProviderConfig { - connector: HttpConnector::Prebuilt(Some(connector)), + connector: connector.into(), ..self } }