Unverified Commit 95714395 authored by Zelda Hessler's avatar Zelda Hessler Committed by GitHub
Browse files

feature: make retry behavior user-configurable (#741)



* feature: add retry_config to aws_config::ConfigLoader and aws_config::default_provider
feature: add retry_config to aws_types::Config and aws_types::Builder
feature: add RetryConfig and RetryMode to smithy_types
feature: create EnvironmentVariableRetryConfigProvider
feature: create RetryConfigProviderChain
feature: create ProfileFileRetryConfigProvider
update: make smithy-types dep non-optional for aws-config
add: smithy-types dep to aws-types

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

Co-authored-by: default avatarJohn DiSanti <jdisanti@amazon.com>

* refactor: simplify configuration logic of retry_configs
update: use non-allocating string comparison for RetryMode::from_str
update: panic on setting invalid values for RetryConfig
remove: provider chain for retry_config
remove: ProvideRetryConfig trait and related functionality

* update: AwsFluentClientDecorator to work with retry_config
refactor: rename smithy_client::retry::Config.max_retries to max_attempts and fix code broken by this change
add: RetryConfigDecorator to smithy codegen with example and test
add: RetryConfigDecorator to decorators list
add: update SharedConfigDecorator to work with retry_config
add: prop getters to RetryConfig
add: From<RetryConfig> for smithy_client::retry::Config
update: RegionDecorator example of generated code
sort: decorators list alphabetically

* fix: clone moved valued in AwsFluentClientDecorator
update: imds client to refer to max attempts instead of max retries
fix: clippy lint about FromStr
add: RetryModeErr error struct for when FromStr fails
fix: code affected by added FromStr<RetryMode> trait usage

* formatting: run rustfmt

* format: use 1.53 version of fustfmt

* fix: smithy_client tests broken by max_attempts change

* fix: clarify some confusing counter logic around request attempts

* update: set_retry_config example code to be more helpful
fix: broken docs link

* add: missing PartialEq impl for RetryConfig
update: EnvironmentVariableRetryConfigProvider tests
remove: unused import

* update: CHANGELOGs

* update: Config builder decorators to match Config builder methods

* fix: old references to ProtocolConfig

* refactor: surface all retry_config errors in the default_provider
add: RetryConfigErr

* update: Changelog to not new semantics of max_attempts
update: Config::retry_config() example
fix: copy paste error
rename: RetryModeErr to RetryModeParseErr
update: note valid retry modes in error message
add: helper for creating RetryConfig that disables retries
update: use Cow<&str> for RetryConfigErr to save on allocations
add: FailedToParseMaxAttempts error when creating RetryConfig from invalid max_attempts
update: don't ignore invalid/unparseable max_attempts
update: note panic that can occur in retry_config::default_provider
remove: invalid/useless code from RetryConfigDecorator.kt
remove: inside baseball comments previously added to CHANGELOG

* disable: adaptive RetryMode tests

* fix: don't listen to the IDE, err is being used

* fix: don't listen to the IDE, err is being used

* fix: really struggling with this underscore

* fix: typo in doc comment example

* fix: typo in doc comment example
fix: outdated tests

* Update rust-runtime/smithy-client/src/retry.rs

Co-authored-by: default avatarRussell Cohen <russell.r.cohen@gmail.com>

* update: retry_config::default_provider to consider precedence per-field instead of per struct
add: RetryConfigBuilder to make the above possible
update: Env and Profile provider for RetryConfig to return RetryConfigBuilder
add: docs to generated retry_config builders
add: from_slice method to os_shim_internal::Fs

* update: use old ordering of decorators in AwsCodegenDecorator

* update: use old ordering of decorators in AwsCodegenDecorator
fix: os_shim_internal example not compiling
formatting: run ktlint
update: tests broken by RetryConfigDecorator.kt changes

* formatting: don't use * imports in kotlin

* fix: tests broken by stubConfigProject change

* Update codegen/src/test/kotlin/software/amazon/smithy/rust/RetryConfigProviderConfigTest.kt

Co-authored-by: default avatarRussell Cohen <russell.r.cohen@gmail.com>

* formatting: run ktlint

* add: back accidentally removed presigning decorator

Co-authored-by: default avatarJohn DiSanti <jdisanti@amazon.com>
Co-authored-by: default avatarRussell Cohen <russell.r.cohen@gmail.com>
parent e6220849
Loading
Loading
Loading
Loading
+7 −0
Original line number Diff line number Diff line
@@ -12,6 +12,11 @@ v0.25 (October 7th, 2021)
=========================
**Breaking changes**
- :warning: MSRV increased from 1.52.1 to 1.53.0 per our 3-behind MSRV policy.
- :warning: `smithy_client::retry::Config` field `max_retries` is renamed to `max_attempts`
  - This also brings a change to the semantics of the field. In the old version, setting `max_retries` to 3 would mean
    that up to 4 requests could occur (1 initial request and 3 retries). In the new version, setting `max_attempts` to 3
    would mean that up to 3 requests could occur (1 initial request and 2 retries).
- :warning: `smithy_client::retry::Config::with_max_retries` method is renamed to `with_max_attempts`
- :warning: Several classes in the codegen module were renamed and/or refactored (smithy-rs#735):
  - `ProtocolConfig` became `CodegenContext` and moved to `software.amazon.smithy.rust.codegen.smithy`
  - `HttpProtocolGenerator` became `ProtocolGenerator` and was refactored
@@ -22,7 +27,9 @@ v0.25 (October 7th, 2021)
- The `DispatchError` variant of `SdkError` now contains `ConnectorError` instead of `Box<dyn Error>` (#744).

**New this week**

- :bug: Fix an issue where `smithy-xml` may have generated invalid XML (smithy-rs#719)
- Add `RetryConfig` struct for configuring retry behavior (smithy-rs#725)
- :bug: Fix error when receiving empty event stream messages (smithy-rs#736)
- :bug: Fix bug in event stream receiver that could cause the last events in the response stream to be lost (smithy-rs#736)
- Add connect & HTTP read timeouts to IMDS, defaulting to 1 second
+11 −0
Original line number Diff line number Diff line
@@ -2,15 +2,26 @@ vNext (Month Day, Year)
=======================

**Breaking changes**

- :warning: MSRV increased from 1.52.1 to 1.53.0 per our 3-behind MSRV policy.
- `SmithyConnector` and `DynConnector` now return `ConnectorError` instead of `Box<dyn Error>`. If you have written a custom connector, it will need to be updated to return the new error type. (#744)
- The `DispatchError` variant of `SdkError` now contains `ConnectorError` instead of `Box<dyn Error>` (#744).

**Tasks to cut release**

- [ ] Bump MSRV on aws-sdk-rust, then delete this line.

**New This Week**

- :tada: Make retry behavior configurable
    - With env vars `AWS_MAX_ATTEMPTS` and `AWS_RETRY_MODE`
    - With `~/.aws/config` settings `max_attempts` and `retry_mode`
    - By calling the `with_retry_config` method on a `Config` and passing in a `RetryConfig`
    - Only the `Standard` retry mode is currently implemented. `Adaptive` retry mode will be implemented at a later
      date.
    - For more info, see the AWS Reference pages on configuring these settings:
        - [Setting global max attempts](https://docs.aws.amazon.com/sdkref/latest/guide/setting-global-max_attempts.html)
        - [Setting global retry mode](https://docs.aws.amazon.com/sdkref/latest/guide/setting-global-retry_mode.html)
- :tada: Add presigned request support and examples for S3 GetObject and PutObject (smithy-rs#731, aws-sdk-rust#139)
- :tada: Add presigned request support and example for Polly SynthesizeSpeech (smithy-rs#735, aws-sdk-rust#139)
- Add connect & HTTP read timeouts to IMDS, defaulting to 1 second
+2 −2
Original line number Diff line number Diff line
@@ -10,7 +10,7 @@ exclude = ["test-data/*", "integration-tests/*"]
default-provider = ["profile", "imds", "meta", "sts", "environment"]
profile = ["sts", "web-identity-token", "meta", "environment", "imds"]
meta = ["tokio/sync"]
imds = ["profile", "smithy-http", "smithy-types", "smithy-http-tower", "smithy-json", "tower", "aws-http", "meta"]
imds = ["profile", "smithy-http", "smithy-http-tower", "smithy-json", "tower", "aws-http", "meta"]
environment = ["meta"]
sts = ["aws-sdk-sts", "aws-hyper"]
web-identity-token = ["sts", "profile"]
@@ -28,6 +28,7 @@ default = ["default-provider", "rustls", "rt-tokio"]
aws-types = { path = "../../sdk/build/aws-sdk/aws-types" }
smithy-async = { path = "../../sdk/build/aws-sdk/smithy-async" }
smithy-client = { path = "../../sdk/build/aws-sdk/smithy-client" }
smithy-types = { path = "../../sdk/build/aws-sdk/smithy-types" }
tracing = { version = "0.1" }
tokio = { version = "1", features = ["sync"], optional = true }
aws-sdk-sts = { path = "../../sdk/build/aws-sdk/sts", optional = true }
@@ -37,7 +38,6 @@ aws-hyper = { path = "../../sdk/build/aws-sdk/aws-hyper", optional = true }

# imds
smithy-http = { path = "../../sdk/build/aws-sdk/smithy-http", optional = true }
smithy-types = { path = "../../sdk/build/aws-sdk/smithy-types", optional = true }
smithy-http-tower = { path = "../../sdk/build/aws-sdk/smithy-http-tower", optional = true }
tower = { version = "0.4.8", optional = true }
aws-http = { path = "../../sdk/build/aws-sdk/aws-http", optional = true }
+229 −15
Original line number Diff line number Diff line
@@ -7,18 +7,15 @@
//!
//! Unless specific configuration is required, these should be constructed via [`ConfigLoader`](crate::ConfigLoader).
//!
//!

/// Default region provider chain
pub mod region {
    use aws_types::region::Region;

    use crate::environment::region::EnvironmentVariableRegionProvider;
    use crate::meta::region::{ProvideRegion, RegionProviderChain};
    use crate::{imds, profile};

    use crate::provider_config::ProviderConfig;

    use aws_types::region::Region;
    use crate::{imds, profile};

    /// Default Region Provider chain
    ///
@@ -89,17 +86,109 @@ pub mod region {
    }
}

/// Default retry behavior configuration provider chain
pub mod retry_config {
    use smithy_types::retry::RetryConfig;

    use crate::environment::retry_config::EnvironmentVariableRetryConfigProvider;
    use crate::profile;
    use crate::provider_config::ProviderConfig;

    /// Default RetryConfig Provider chain
    ///
    /// Unlike other "providers" `RetryConfig` has no related `RetryConfigProvider` trait. Instead,
    /// a builder struct is returned which has a similar API.
    ///
    /// This provider will check the following sources in order:
    /// 1. [Environment variables](EnvironmentVariableRetryConfigProvider)
    /// 2. [Profile file](crate::profile::retry_config::ProfileFileRetryConfigProvider)
    ///
    /// # Example
    ///
    /// ```rust
    /// # use std::error::Error;
    /// # #[tokio::main]
    /// # async fn main() -> Result<(), Box<dyn Error>> {
    /// use aws_config::default_provider::retry_config;
    /// // Creating a RetryConfig from the default_provider already happens when loading a config from_env
    /// // This is only for illustration purposes
    /// let retry_config = retry_config::default_provider().retry_config().await;
    /// let config = aws_config::from_env().retry_config(retry_config).load().await;
    /// // instantiate a service client:
    /// // <my_aws_service>::Client::new(&config);
    /// #     Ok(())
    /// # }
    /// ```
    pub fn default_provider() -> Builder {
        Builder::default()
    }

    /// Builder for RetryConfig that checks the environment and aws profile for configuration
    #[derive(Default)]
    pub struct Builder {
        env_provider: EnvironmentVariableRetryConfigProvider,
        profile_file: profile::retry_config::Builder,
    }

    impl Builder {
        #[doc(hidden)]
        /// Configure the default chain
        ///
        /// Exposed for overriding the environment when unit-testing providers
        pub fn configure(mut self, configuration: &ProviderConfig) -> Self {
            self.env_provider =
                EnvironmentVariableRetryConfigProvider::new_with_env(configuration.env());
            self.profile_file = self.profile_file.configure(configuration);
            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 [RetryConfig](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()](smithy_types::retry::RetryConfig::default)
        ///
        /// Precedence is considered on a per-field basis
        ///
        /// # Panics
        ///
        /// - Panics if the `AWS_MAX_ATTEMPTS` env var or `max_attempts` profile var is set to 0
        /// - Panics if the `AWS_RETRY_MODE` env var or `retry_mode` profile var is set to "adaptive" (it's not yet supported)
        pub async fn retry_config(self) -> RetryConfig {
            // 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.retry_config_builder() {
                Ok(retry_config_builder) => retry_config_builder,
                Err(err) => panic!("{}", err),
            };
            let builder_from_profile = match self.profile_file.build().retry_config_builder().await
            {
                Ok(retry_config_builder) => retry_config_builder,
                Err(err) => panic!("{}", err),
            };

            builder_from_env.merge_with(builder_from_profile).build()
        }
    }
}

/// Default credentials provider chain
pub mod credentials {
    use std::borrow::Cow;

    use aws_types::credentials::{future, ProvideCredentials};

    use crate::environment::credentials::EnvironmentVariableCredentialsProvider;
    use crate::meta::credentials::{CredentialsProviderChain, LazyCachingCredentialsProvider};
    use crate::meta::region::ProvideRegion;
    use aws_types::credentials::{future, ProvideCredentials};

    use crate::provider_config::ProviderConfig;

    use std::borrow::Cow;

    #[cfg(any(feature = "rustls", feature = "native-tls"))]
    /// Default Credentials Provider chain
    ///
@@ -239,10 +328,11 @@ pub mod credentials {
                Some(provider) => provider.region().await,
                None => self.region_chain.build().region().await,
            };

            let conf = self.conf.unwrap_or_default().with_region(region);

            let profile_provider = self.profile_file_builder.configure(&conf).build();
            let env_provider = EnvironmentVariableCredentialsProvider::new_with_env(conf.env());
            let profile_provider = self.profile_file_builder.configure(&conf).build();
            let web_identity_token_provider = self.web_identity_builder.configure(&conf).build();
            let imds_provider = self.imds_builder.configure(&conf).build();

@@ -251,12 +341,23 @@ pub mod credentials {
                .or_else("WebIdentityToken", web_identity_token_provider)
                .or_else("Ec2InstanceMetadata", imds_provider);
            let cached_provider = self.credential_cache.configure(&conf).load(provider_chain);

            DefaultCredentialsChain(cached_provider.build())
        }
    }

    #[cfg(test)]
    mod test {
        use tracing_test::traced_test;

        use aws_types::credentials::ProvideCredentials;
        use aws_types::os_shim_internal::{Env, Fs};
        use smithy_types::retry::{RetryConfig, RetryMode};

        use crate::default_provider::credentials::DefaultCredentialsChain;
        use crate::default_provider::retry_config;
        use crate::provider_config::ProviderConfig;
        use crate::test_case::TestEnvironment;

        /// Test generation macro
        ///
@@ -307,11 +408,6 @@ pub mod credentials {
            };
        }

        use crate::default_provider::credentials::DefaultCredentialsChain;
        use crate::test_case::TestEnvironment;
        use aws_types::credentials::ProvideCredentials;
        use tracing_test::traced_test;

        make_test!(prefer_environment);
        make_test!(profile_static_keys);
        make_test!(web_identity_token_env);
@@ -347,5 +443,123 @@ pub mod credentials {
                .expect("creds should load");
            assert_eq!(creds.access_key_id(), "correct_key_secondary");
        }

        #[tokio::test]
        async fn test_returns_default_retry_config_from_empty_profile() {
            let env = Env::from_slice(&[("AWS_CONFIG_FILE", "config")]);
            let fs = Fs::from_slice(&[("config", "[default]\n")]);

            let provider_config = ProviderConfig::no_configuration().with_env(env).with_fs(fs);

            let actual_retry_config = retry_config::default_provider()
                .configure(&provider_config)
                .retry_config()
                .await;

            let expected_retry_config = RetryConfig::new();

            assert_eq!(actual_retry_config, expected_retry_config);
            // This is redundant but it's really important to make sure that
            // we're setting these exact values by default so we check twice
            assert_eq!(actual_retry_config.max_attempts(), 3);
            assert_eq!(actual_retry_config.mode(), RetryMode::Standard);
        }

        #[tokio::test]
        async fn test_no_retry_config_in_empty_profile() {
            let env = Env::from_slice(&[("AWS_CONFIG_FILE", "config")]);
            let fs = Fs::from_slice(&[("config", "[default]\n")]);

            let provider_config = ProviderConfig::no_configuration().with_env(env).with_fs(fs);

            let actual_retry_config = retry_config::default_provider()
                .configure(&provider_config)
                .retry_config()
                .await;

            let expected_retry_config = RetryConfig::new();

            assert_eq!(actual_retry_config, expected_retry_config)
        }

        #[tokio::test]
        async fn test_creation_of_retry_config_from_profile() {
            let env = Env::from_slice(&[("AWS_CONFIG_FILE", "config")]);
            // TODO standard is the default mode; this test would be better if it was setting it to adaptive mode
            // adaptive mode is currently unsupported so that would panic
            let fs = Fs::from_slice(&[(
                "config",
                // If the lines with the vars have preceding spaces, they don't get read
                r#"[default]
max_attempts = 1
retry_mode = standard
            "#,
            )]);

            let provider_config = ProviderConfig::no_configuration().with_env(env).with_fs(fs);

            let actual_retry_config = retry_config::default_provider()
                .configure(&provider_config)
                .retry_config()
                .await;

            let expected_retry_config = RetryConfig::new()
                .with_max_attempts(1)
                .with_retry_mode(RetryMode::Standard);

            assert_eq!(actual_retry_config, expected_retry_config)
        }

        #[tokio::test]
        async fn test_env_retry_config_takes_precedence_over_profile_retry_config() {
            let env = Env::from_slice(&[
                ("AWS_CONFIG_FILE", "config"),
                ("AWS_MAX_ATTEMPTS", "42"),
                ("AWS_RETRY_MODE", "standard"),
            ]);
            // TODO standard is the default mode; this test would be better if it was setting it to adaptive mode
            // adaptive mode is currently unsupported so that would panic
            let fs = Fs::from_slice(&[(
                "config",
                // If the lines with the vars have preceding spaces, they don't get read
                r#"[default]
max_attempts = 88
retry_mode = standard
            "#,
            )]);

            let provider_config = ProviderConfig::no_configuration().with_env(env).with_fs(fs);

            let actual_retry_config = retry_config::default_provider()
                .configure(&provider_config)
                .retry_config()
                .await;

            let expected_retry_config = RetryConfig::new()
                .with_max_attempts(42)
                .with_retry_mode(RetryMode::Standard);

            assert_eq!(actual_retry_config, expected_retry_config)
        }

        #[tokio::test]
        #[should_panic = "failed to parse max attempts set by aws profile: invalid digit found in string"]
        async fn test_invalid_profile_retry_config_panics() {
            let env = Env::from_slice(&[("AWS_CONFIG_FILE", "config")]);
            let fs = Fs::from_slice(&[(
                "config",
                // If the lines with the vars have preceding spaces, they don't get read
                r#"[default]
max_attempts = potato
            "#,
            )]);

            let provider_config = ProviderConfig::no_configuration().with_env(env).with_fs(fs);

            let _ = retry_config::default_provider()
                .configure(&provider_config)
                .retry_config()
                .await;
        }
    }
}
+4 −0
Original line number Diff line number Diff line
@@ -9,3 +9,7 @@ pub use credentials::EnvironmentVariableCredentialsProvider;
/// Load regions from the environment
pub mod region;
pub use region::EnvironmentVariableRegionProvider;

/// Load retry behavior configuration from the environment
pub mod retry_config;
pub use retry_config::EnvironmentVariableRetryConfigProvider;
Loading