Unverified Commit 79be3105 authored by Russell Cohen's avatar Russell Cohen Committed by GitHub
Browse files

Share HTTP connectors between providers and clients (#2876) 

## Motivation and Context
Clients using separate connectors is mostly confusing and troublesome
for customers.

## Description
Change the behavior of `ConfigLoader::http_connector` to set both the
client & credential provider HTTP connector.

**Note**: It is still possible to separate control clients for the
credential provider. Because `HttpConnector` is used, the timeout
settings can still be late-bound.

## Testing
Unit tests.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
parent 1fbafc83

CHANGELOG.next.toml

+6 −0
Original line number Diff line number Diff line
@@ -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 `<service>::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.

aws/rust-runtime/aws-config/src/default_provider/app_name.rs

+3 −6
Original line number Diff line number Diff line
@@ -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()

aws/rust-runtime/aws-config/src/lib.rs

+135 −55
Original line number Diff line number Diff line
@@ -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<bool>,

        use_dual_stack: Option<bool>,

        time_source: Option<SharedTimeSource>,

        env: Option<Env>,

        fs: Option<Fs>,

    }



    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 connector_fn = |settings:  &ConnectorSettings, sleep: Option<SharedAsyncSleep>| {

        ///   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 smithy_connector = hyper_ext::Adapter::builder()

        ///   let mut 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);

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

                );

            }

        }

    }

}

aws/rust-runtime/aws-config/src/provider_config.rs

+21 −13
Original line number Diff line number Diff line
@@ -150,6 +150,21 @@ impl ProviderConfig { 
        }

    }



    /// Initializer for ConfigBag to avoid possibly setting incorrect defaults.

    pub(crate) fn init(time_source: SharedTimeSource, sleep: Option<SharedAsyncSleep>) -> 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<HttpConnector>) -> Self {

        ProviderConfig {

            connector: HttpConnector::Prebuilt(Some(connector)),

            connector: connector.into(),

            ..self

        }

    }