Unverified Commit 8e0b4404 authored by John DiSanti's avatar John DiSanti Committed by GitHub
Browse files

Improve timeout config ergonomics and add SDK default timeouts (#1740) 

* Overhaul timeout configuration
* Improve resiliency re-exports
* Refactor Smithy client retry config handling in builder
* Remove non-standard timeout env vars and profile keys
* Break operation customization codegen into its own file
* Move operation customization into a submodule
* Improve operation customization re-exports
* Add integration tests for read and connect timeouts
* Fix connector timeout config in the Smithy `Client` builder
* Rename `HttpSettings` to `ConnectorSettings`
parent 42a84576

CHANGELOG.next.toml

+32 −0
Original line number Diff line number Diff line
@@ -21,3 +21,35 @@ message = "Implement support for pure Python request middleware. Improve idiomat 
references = ["smithy-rs#1734"]

meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "server" }

author = "crisidev"



[[aws-sdk-rust]]

message = """

The SDK, by default, now times out if socket connect or time to first byte read takes longer than

3.1 seconds. There are a large number of breaking changes that come with this change that may

affect you if you customize the client configuration at all.

See [the upgrade guide](https://github.com/awslabs/aws-sdk-rust/issues/622) for information

on how to configure timeouts, and how to resolve compilation issues after upgrading.

"""

references = ["smithy-rs#1740", "smithy-rs#256"]

meta = { "breaking" = true, "tada" = false, "bug" = false }

author = "jdisanti"



[[aws-sdk-rust]]

message = """

Setting connect/read timeouts with `SdkConfig` now works. Previously, these timeout config values

were lost during connector creation, so the only reliable way to set them was to manually override

the HTTP connector.

"""

references = ["smithy-rs#1740", "smithy-rs#256"]

meta = { "breaking" = false, "tada" = false, "bug" = true }

author = "jdisanti"



[[smithy-rs]]

message = """

A large list of breaking changes were made to accomodate default timeouts in the AWS SDK.

See [the smithy-rs upgrade guide](https://github.com/awslabs/smithy-rs/issues/1760) for a full list

of breaking changes and how to resolve them.

"""

references = ["smithy-rs#1740", "smithy-rs#256"]

meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }

author = "jdisanti"

aws/rust-runtime/aws-config/external-types.toml

+4 −3
Original line number Diff line number Diff line
@@ -8,12 +8,13 @@ allowed_external_types = [ 
   "aws_smithy_client::erase::DynConnector",

   "aws_smithy_client::erase::boxclone::BoxCloneService",

   "aws_smithy_client::http_connector::HttpConnector",

   "aws_smithy_client::http_connector::HttpSettings",

   "aws_smithy_client::http_connector::ConnectorSettings",

   "aws_smithy_http::body::SdkBody",

   "aws_smithy_http::result::SdkError",

   "aws_smithy_types::retry::RetryConfig*",

   "aws_smithy_types::retry",

   "aws_smithy_types::retry::*",

   "aws_smithy_types::timeout",

   "aws_smithy_types::timeout::config::Config",

   "aws_smithy_types::timeout::config::TimeoutConfig",

   "aws_smithy_types::timeout::error::ConfigError",

   "aws_types::*",

   "http::response::Response",

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

+10 −11
Original line number Diff line number Diff line
@@ -5,11 +5,10 @@ 


//! Functionality related to creating new HTTP Connectors



use std::sync::Arc;



use aws_smithy_async::rt::sleep::AsyncSleep;

use aws_smithy_client::erase::DynConnector;

use aws_smithy_client::http_connector::HttpSettings;

use aws_smithy_client::http_connector::ConnectorSettings;

use std::sync::Arc;



// unused when all crate features are disabled

/// Unwrap an [`Option<DynConnector>`](aws_smithy_client::erase::DynConnector), and panic with a helpful error message if it's `None`
@@ -19,41 +18,41 @@ pub(crate) fn expect_connector(connector: Option<DynConnector>) -> DynConnector 


#[cfg(any(feature = "rustls", feature = "native-tls"))]

fn base(

    settings: &HttpSettings,

    settings: &ConnectorSettings,

    sleep: Option<Arc<dyn AsyncSleep>>,

) -> aws_smithy_client::hyper_ext::Builder {

    let mut hyper =

        aws_smithy_client::hyper_ext::Adapter::builder().timeout(&settings.http_timeout_config);

        aws_smithy_client::hyper_ext::Adapter::builder().connector_settings(settings.clone());

    if let Some(sleep) = sleep {

        hyper = hyper.sleep_impl(sleep);

    }

    hyper

}



/// Given `HttpSettings` and an `AsyncSleep`, create a `DynConnector` from defaults depending on what cargo features are activated.

/// Given `ConnectorSettings` and an `AsyncSleep`, create a `DynConnector` from defaults depending on what cargo features are activated.

#[cfg(feature = "rustls")]

pub fn default_connector(

    settings: &HttpSettings,

    settings: &ConnectorSettings,

    sleep: Option<Arc<dyn AsyncSleep>>,

) -> Option<DynConnector> {

    let hyper = base(settings, sleep).build(aws_smithy_client::conns::https());

    Some(DynConnector::new(hyper))

}



/// Given `HttpSettings` and an `AsyncSleep`, create a `DynConnector` from defaults depending on what cargo features are activated.

/// Given `ConnectorSettings` and an `AsyncSleep`, create a `DynConnector` from defaults depending on what cargo features are activated.

#[cfg(all(not(feature = "rustls"), feature = "native-tls"))]

pub fn default_connector(

    settings: &HttpSettings,

    settings: &ConnectorSettings,

    sleep: Option<Arc<dyn AsyncSleep>>,

) -> Option<DynConnector> {

    let hyper = base(settings, sleep).build(aws_smithy_client::conns::native_tls());

    Some(DynConnector::new(hyper))

}



/// Given `HttpSettings` and an `AsyncSleep`, create a `DynConnector` from defaults depending on what cargo features are activated.

/// Given `ConnectorSettings` and an `AsyncSleep`, create a `DynConnector` from defaults depending on what cargo features are activated.

#[cfg(not(any(feature = "rustls", feature = "native-tls")))]

pub fn default_connector(

    _settings: &HttpSettings,

    _settings: &ConnectorSettings,

    _sleep: Option<Arc<dyn AsyncSleep>>,

) -> Option<DynConnector> {

    None

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

+16 −72
Original line number Diff line number Diff line
@@ -3,95 +3,39 @@ 
 * SPDX-License-Identifier: Apache-2.0

 */



use aws_smithy_types::timeout;



use crate::environment::timeout_config::EnvironmentVariableTimeoutConfigProvider;

use crate::profile;

use crate::provider_config::ProviderConfig;

use aws_sdk_sso::config::timeout::TimeoutConfig;

use std::time::Duration;



const SDK_DEFAULT_CONNECT_TIMEOUT: Duration = Duration::from_millis(3100);



/// Default [`timeout::Config`] Provider chain

/// Default [`TimeoutConfig`] provider chain

///

/// Unlike other credentials and region, [`timeout::Config`] has no related `TimeoutConfigProvider` trait. Instead,

/// Unlike other credentials and region, [`TimeoutConfig`] has no related `TimeoutConfigProvider` trait. Instead,

/// a builder struct is returned which has a similar API.

///

/// This provider will check the following sources in order:

/// 1. [Environment variables](EnvironmentVariableTimeoutConfigProvider)

/// 2. [Profile file](crate::profile::timeout_config::ProfileFileTimeoutConfigProvider) (`~/.aws/config`)

///

/// # Example

///

/// ```no_run

/// # use std::error::Error;

/// # #[tokio::main]

/// # async fn main() {

/// use aws_config::default_provider::timeout_config;

///

/// // Load a timeout config from a specific profile

/// let timeout_config = timeout_config::default_provider()

///     .profile_name("other_profile")

///     .timeout_config()

///     .await;

/// let config = aws_config::from_env()

///     // Override the timeout config set by the default profile

///     .timeout_config(timeout_config)

///     .load()

///     .await;

/// // instantiate a service client:

/// // <my_aws_service>::Client::new(&config);

/// # }

/// ```

pub fn default_provider() -> Builder {

    Builder::default()

}



/// Builder for [`timeout::Config`](aws_smithy_types::timeout::Config) that checks the environment variables and AWS profile files for configuration

/// Builder for [`TimeoutConfig`] that resolves the default timeout configuration

#[non_exhaustive]

#[derive(Debug, Default)]

pub struct Builder {

    env_provider: EnvironmentVariableTimeoutConfigProvider,

    profile_file: profile::timeout_config::Builder,

}

pub struct Builder;



impl Builder {

    /// Configure the default chain

    ///

    /// Exposed for overriding the environment when unit-testing providers

    pub fn configure(mut self, configuration: &ProviderConfig) -> Self {

        self.env_provider =

            EnvironmentVariableTimeoutConfigProvider::new_with_env(configuration.env());

        self.profile_file = self.profile_file.configure(configuration);

    pub fn configure(self, _configuration: &ProviderConfig) -> Self {

        self

    }



    /// Override the profile name used by this provider

    pub fn profile_name(mut self, name: &str) -> Self {

        self.profile_file = self.profile_file.profile_name(name);

        self

    }



    /// Attempt to create a [`timeout::Config`](aws_smithy_types::timeout::Config) from following sources in order:

    /// 1. [Environment variables](crate::environment::timeout_config::EnvironmentVariableTimeoutConfigProvider)

    /// 2. [Profile file](crate::profile::timeout_config::ProfileFileTimeoutConfigProvider)

    ///

    /// Precedence is considered on a per-field basis. If no timeout is specified, requests will never time out.

    ///

    /// # Panics

    ///

    /// This will panic if:

    /// - a timeout is set to `NaN`, a negative number, or infinity

    /// - a timeout can't be parsed as a floating point number

    pub async fn timeout_config(self) -> timeout::Config {

        // Both of these can return errors due to invalid config settings and we want to surface those as early as possible

        // hence, we'll panic if any config values are invalid (missing values are OK though)

        // We match this instead of unwrapping so we can print the error with the `Display` impl instead of the `Debug` impl that unwrap uses

        let builder_from_env = match self.env_provider.timeout_config() {

            Ok(timeout_config_builder) => timeout_config_builder,

            Err(err) => panic!("{}", err),

        };

        let builder_from_profile = match self.profile_file.build().timeout_config().await {

            Ok(timeout_config_builder) => timeout_config_builder,

            Err(err) => panic!("{}", err),

        };



        builder_from_env.take_unset_from(builder_from_profile)

    /// Resolve default timeout configuration

    pub async fn timeout_config(self) -> TimeoutConfig {

        // TODO(https://github.com/awslabs/smithy-rs/issues/1732): Implement complete timeout defaults specification

        TimeoutConfig::builder()

            .connect_timeout(SDK_DEFAULT_CONNECT_TIMEOUT)

            .build()

    }

}

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

+10 −2
Original line number Diff line number Diff line
@@ -61,11 +61,15 @@ use tower::{Service, ServiceExt}; 


use crate::http_credential_provider::HttpCredentialProvider;

use crate::provider_config::ProviderConfig;

use aws_smithy_client::http_connector::ConnectorSettings;

use aws_types::os_shim_internal::Env;

use http::header::InvalidHeaderValue;

use std::time::Duration;

use tokio::sync::OnceCell;



const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(5);

const DEFAULT_CONNECT_TIMEOUT: Duration = Duration::from_secs(2);



// URL from https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-metadata-endpoint-v2.html

const BASE_HOST: &str = "http://169.254.170.2";

const ENV_RELATIVE_URI: &str = "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI";
@@ -164,8 +168,12 @@ impl Provider { 
        };

        let http_provider = HttpCredentialProvider::builder()

            .configure(&provider_config)

            .connect_timeout(builder.connect_timeout)

            .read_timeout(builder.read_timeout)

            .connector_settings(

                ConnectorSettings::builder()

                    .connect_timeout(DEFAULT_CONNECT_TIMEOUT)

                    .read_timeout(DEFAULT_READ_TIMEOUT)

                    .build(),

            )

            .build("EcsContainer", uri);

        Provider::Configured(http_provider)

    }