Unverified Commit 353d81c5 authored by John DiSanti's avatar John DiSanti Committed by GitHub
Browse files

Improve manual config experience for SDK retries and timeouts (#1603) 

* Remove `Default` implementation from `RetryConfig`
* Add use case integration tests
* Panic when retries/timeouts are enabled without a `sleep_impl`
* Combine the sleep, retry, and timeout customizations
* Add `sleep_impl` validation to the Smithy client builder
parent 422f5777

CHANGELOG.next.toml

+63 −1
Original line number Diff line number Diff line
@@ -10,3 +10,65 @@ 
# references = ["smithy-rs#920"]

# meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"}

# author = "rcoh"



[[aws-sdk-rust]]

message = "`aws_config::RetryConfig` no longer implements `Default`, and its `new` function has been replaced with `standard`."

references = ["smithy-rs#1603", "aws-sdk-rust#586"]

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

author = "jdisanti"



[[smithy-rs]]

message = "`aws_smithy_types::RetryConfig` no longer implements `Default`, and its `new` function has been replaced with `standard`."

references = ["smithy-rs#1603", "aws-sdk-rust#586"]

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

author = "jdisanti"



[[aws-sdk-rust]]

message = """

Direct configuration of `aws_config::SdkConfig` now defaults to retries being disabled.

If you're using `aws_config::load_from_env()` or `aws_config::from_env()` to configure

the SDK, then you are NOT affected by this change. If you use `SdkConfig::builder()` to

configure the SDK, then you ARE affected by this change and should set the retry config

on that builder.

"""

references = ["smithy-rs#1603", "aws-sdk-rust#586"]

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

author = "jdisanti"



[[aws-sdk-rust]]

message = """

Client creation now panics if retries or timeouts are enabled without an async sleep

implementation set on the SDK config.

If you're using the Tokio runtime and have the `rt-tokio` feature enabled (which is enabled by default),

then you shouldn't notice this change at all.

Otherwise, if using something other than Tokio as the async runtime, the `AsyncSleep` trait must be implemented,

and that implementation given to the config builder via the `sleep_impl` method. Alternatively, retry can be

explicitly turned off by setting the retry config to `RetryConfig::disabled()`, which will result in successful

client creation without an async sleep implementation.

"""

references = ["smithy-rs#1603", "aws-sdk-rust#586"]

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

author = "jdisanti"



[[smithy-rs]]

message = """

Client creation now panics if retries or timeouts are enabled without an async sleep implementation.

If you're using the Tokio runtime and have the `rt-tokio` feature enabled (which is enabled by default),

then you shouldn't notice this change at all.

Otherwise, if using something other than Tokio as the async runtime, the `AsyncSleep` trait must be implemented,

and that implementation given to the config builder via the `sleep_impl` method. Alternatively, retry can be

explicitly turned off by setting `max_attempts` to 1, which will result in successful client creation without an

async sleep implementation.

"""

references = ["smithy-rs#1603", "aws-sdk-rust#586"]

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

author = "jdisanti"



[[smithy-rs]]

message = """

The `default_async_sleep` method on the `Client` builder has been removed. The default async sleep is

wired up by default if none is provided.

"""

references = ["smithy-rs#1603", "aws-sdk-rust#586"]

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

author = "jdisanti"

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

+4 −8
Original line number Diff line number Diff line
@@ -399,7 +399,7 @@ mod test { 
            .retry_config()

            .await;



        let expected_retry_config = RetryConfig::new();

        let expected_retry_config = RetryConfig::standard();



        assert_eq!(actual_retry_config, expected_retry_config);

        // This is redundant but it's really important to make sure that
@@ -420,7 +420,7 @@ mod test { 
            .retry_config()

            .await;



        let expected_retry_config = RetryConfig::new();

        let expected_retry_config = RetryConfig::standard();



        assert_eq!(actual_retry_config, expected_retry_config)

    }
@@ -447,9 +447,7 @@ retry_mode = standard 
            .retry_config()

            .await;



        let expected_retry_config = RetryConfig::new()

            .with_max_attempts(1)

            .with_retry_mode(RetryMode::Standard);

        let expected_retry_config = RetryConfig::standard().with_max_attempts(1);



        assert_eq!(actual_retry_config, expected_retry_config)

    }
@@ -480,9 +478,7 @@ retry_mode = standard 
            .retry_config()

            .await;



        let expected_retry_config = RetryConfig::new()

            .with_max_attempts(42)

            .with_retry_mode(RetryMode::Standard);

        let expected_retry_config = RetryConfig::standard().with_max_attempts(42);



        assert_eq!(actual_retry_config, expected_retry_config)

    }

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

+1 −1
Original line number Diff line number Diff line
@@ -76,7 +76,7 @@ impl Builder { 
    /// Attempt to create a [RetryConfig](aws_smithy_types::retry::RetryConfig) from following sources in order:

    /// 1. [Environment variables](crate::environment::retry_config::EnvironmentVariableRetryConfigProvider)

    /// 2. [Profile file](crate::profile::retry_config::ProfileFileRetryConfigProvider)

    /// 3. [RetryConfig::default()](aws_smithy_types::retry::RetryConfig::default)

    /// 3. [RetryConfig::standard()](aws_smithy_types::retry::RetryConfig::standard)

    ///

    /// Precedence is considered on a per-field basis

    ///

aws/rust-runtime/aws-config/src/environment/retry_config.rs

+3 −5
Original line number Diff line number Diff line
@@ -102,7 +102,7 @@ mod test { 
                .retry_config_builder()

                .unwrap()

                .build(),

            RetryConfig::new().with_max_attempts(88)

            RetryConfig::standard().with_max_attempts(88)

        );

    }
@@ -123,7 +123,7 @@ mod test { 
                .retry_config_builder()

                .unwrap()

                .build(),

            RetryConfig::new().with_retry_mode(RetryMode::Standard)

            RetryConfig::standard()

        );

    }
@@ -137,9 +137,7 @@ mod test { 
            .retry_config_builder()

            .unwrap()

            .build(),

            RetryConfig::new()

                .with_max_attempts(13)

                .with_retry_mode(RetryMode::Standard)

            RetryConfig::standard().with_max_attempts(13)

        );

    }

aws/rust-runtime/aws-config/src/imds/client.rs

+8 −8
Original line number Diff line number Diff line
@@ -744,11 +744,8 @@ impl<T, E> ClassifyResponse<SdkSuccess<T>, SdkError<E>> for ImdsErrorPolicy { 


#[cfg(test)]

pub(crate) mod test {

    use std::collections::HashMap;

    use std::error::Error;

    use std::io;

    use std::time::{Duration, UNIX_EPOCH};



    use crate::imds::client::{Client, EndpointMode, ImdsErrorPolicy};

    use crate::provider_config::ProviderConfig;

    use aws_smithy_async::rt::sleep::TokioSleep;

    use aws_smithy_client::erase::DynConnector;

    use aws_smithy_client::test_connection::{capture_request, TestConnection};
@@ -760,11 +757,12 @@ pub(crate) mod test { 
    use http::header::USER_AGENT;

    use http::Uri;

    use serde::Deserialize;

    use std::collections::HashMap;

    use std::error::Error;

    use std::io;

    use std::time::{Duration, UNIX_EPOCH};

    use tracing_test::traced_test;



    use crate::imds::client::{Client, EndpointMode, ImdsErrorPolicy};

    use crate::provider_config::ProviderConfig;



    const TOKEN_A: &str = "AQAEAFTNrA4eEGx0AQgJ1arIq_Cc-t4tWt3fB0Hd8RKhXlKc5ccvhg==";

    const TOKEN_B: &str = "alternatetoken==";
@@ -918,6 +916,7 @@ pub(crate) mod test { 
        let client = super::Client::builder()

            .configure(

                &ProviderConfig::no_configuration()

                    .with_sleep(TokioSleep::new())

                    .with_http_connector(DynConnector::new(connection.clone()))

                    .with_time_source(TimeSource::manual(&time_source)),

            )
@@ -1134,6 +1133,7 @@ pub(crate) mod test { 
    async fn check(test_case: ImdsConfigTest) {

        let (server, watcher) = capture_request(None);

        let provider_config = ProviderConfig::no_configuration()

            .with_sleep(TokioSleep::new())

            .with_env(Env::from(test_case.env))

            .with_fs(Fs::from_map(test_case.fs))

            .with_http_connector(DynConnector::new(server));