Skip to content

Commit

Permalink
implement user-configurable retry classifiers (#2977)
Browse files Browse the repository at this point in the history
[Read the RFC here](#3018)

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->
#2417

## Description
<!--- Describe your changes in detail -->
Exactly what it says on the tin. I have a related RFC to publish that
goes into more depth.

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
I wrote an integration test that ensures a custom retry classifier can
be set and is called.

## 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._
  • Loading branch information
Velfi authored Oct 10, 2023
1 parent d1ad937 commit 96a9f84
Show file tree
Hide file tree
Showing 43 changed files with 1,190 additions and 615 deletions.
20 changes: 20 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -303,3 +303,23 @@ message = "Our algorithm for converting identifiers to `snake_case` has been upd
references = ["smithy-rs#3037", "aws-sdk-rust#756"]
meta = { "breaking" = true, "tada" = false, "bug" = true, "target" = "all" }
author = "rcoh"

[[smithy-rs]]
message = """
Retry classifiers are now configurable at the service and operation levels. Users may also define their own custom retry classifiers.
For more information, see the [guide](https://github.com/awslabs/smithy-rs/discussions/3050).
"""
references = ["smithy-rs#2417", "smithy-rs#3018"]
meta = { "breaking" = true, "tada" = true, "bug" = false, "target" = "client" }
author = "Velfi"

[[aws-sdk-rust]]
message = """
Retry classifiers are now configurable at the service and operation levels. Users may also define their own custom retry classifiers.
For more information, see the [guide](https://github.com/awslabs/smithy-rs/discussions/3050).
"""
references = ["smithy-rs#2417", "smithy-rs#3018"]
meta = { "breaking" = true, "tada" = true, "bug" = false }
author = "Velfi"
46 changes: 22 additions & 24 deletions aws/rust-runtime/aws-config/src/http_credential_provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,19 @@ use aws_credential_types::Credentials;
use aws_smithy_http::body::SdkBody;
use aws_smithy_http::result::SdkError;
use aws_smithy_runtime::client::orchestrator::operation::Operation;
use aws_smithy_runtime::client::retries::classifier::{
HttpStatusCodeClassifier, SmithyErrorClassifier,
use aws_smithy_runtime::client::retries::classifiers::{
HttpStatusCodeClassifier, TransientErrorClassifier,
};
use aws_smithy_runtime_api::client::http::HttpConnectorSettings;
use aws_smithy_runtime_api::client::interceptors::context::{Error, InterceptorContext};
use aws_smithy_runtime_api::client::orchestrator::{
HttpResponse, OrchestratorError, SensitiveOutput,
};
use aws_smithy_runtime_api::client::retries::{ClassifyRetry, RetryClassifiers, RetryReason};
use aws_smithy_runtime_api::client::retries::classifiers::ClassifyRetry;
use aws_smithy_runtime_api::client::retries::classifiers::RetryAction;
use aws_smithy_runtime_api::client::runtime_plugin::StaticRuntimePlugin;
use aws_smithy_types::config_bag::Layer;
use aws_smithy_types::retry::{ErrorKind, RetryConfig};
use aws_smithy_types::retry::RetryConfig;
use aws_smithy_types::timeout::TimeoutConfig;
use http::header::{ACCEPT, AUTHORIZATION};
use http::{HeaderValue, Response};
Expand Down Expand Up @@ -88,18 +89,6 @@ impl Builder {
) -> HttpCredentialProvider {
let provider_config = self.provider_config.unwrap_or_default();

// The following errors are retryable:
// - Socket errors
// - Networking timeouts
// - 5xx errors
// - Non-parseable 200 responses.
let retry_classifiers = RetryClassifiers::new()
.with_classifier(HttpCredentialRetryClassifier)
// Socket errors and network timeouts
.with_classifier(SmithyErrorClassifier::<Error>::new())
// 5xx errors
.with_classifier(HttpStatusCodeClassifier::default());

let mut builder = Operation::builder()
.service_name("HttpCredentialProvider")
.operation_name("LoadCredentials")
Expand All @@ -123,7 +112,16 @@ impl Builder {
if let Some(sleep_impl) = provider_config.sleep_impl() {
builder = builder
.standard_retry(&RetryConfig::standard())
.retry_classifiers(retry_classifiers)
// The following errors are retryable:
// - Socket errors
// - Networking timeouts
// - 5xx errors
// - Non-parseable 200 responses.
.retry_classifier(HttpCredentialRetryClassifier)
// Socket errors and network timeouts
.retry_classifier(TransientErrorClassifier::<Error>::new())
// 5xx errors
.retry_classifier(HttpStatusCodeClassifier::default())
.sleep_impl(sleep_impl);
} else {
builder = builder.no_retry();
Expand Down Expand Up @@ -192,11 +190,11 @@ impl ClassifyRetry for HttpCredentialRetryClassifier {
"HttpCredentialRetryClassifier"
}

fn classify_retry(&self, ctx: &InterceptorContext) -> Option<RetryReason> {
let output_or_error = ctx.output_or_error()?;
fn classify_retry(&self, ctx: &InterceptorContext) -> RetryAction {
let output_or_error = ctx.output_or_error();
let error = match output_or_error {
Ok(_) => return None,
Err(err) => err,
Some(Ok(_)) | None => return RetryAction::NoActionIndicated,
Some(Err(err)) => err,
};

// Retry non-parseable 200 responses
Expand All @@ -206,11 +204,11 @@ impl ClassifyRetry for HttpCredentialRetryClassifier {
.zip(ctx.response().map(HttpResponse::status))
{
if matches!(err, CredentialsError::Unhandled { .. }) && status.is_success() {
return Some(RetryReason::Error(ErrorKind::ServerError));
return RetryAction::server_error();
}
}

None
RetryAction::NoActionIndicated
}
}

Expand Down Expand Up @@ -308,7 +306,7 @@ mod test {
}

#[tokio::test]
async fn explicit_error_not_retriable() {
async fn explicit_error_not_retryable() {
let http_client = StaticReplayClient::new(vec![ReplayEvent::new(
Request::builder()
.uri(Uri::from_static("http://localhost:1234/some-creds"))
Expand Down
52 changes: 25 additions & 27 deletions aws/rust-runtime/aws-config/src/imds/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ use aws_smithy_runtime::client::retries::strategy::StandardRetryStrategy;
use aws_smithy_runtime_api::client::auth::AuthSchemeOptionResolverParams;
use aws_smithy_runtime_api::client::endpoint::{EndpointResolver, EndpointResolverParams};
use aws_smithy_runtime_api::client::interceptors::context::InterceptorContext;
use aws_smithy_runtime_api::client::orchestrator::{
Future, HttpResponse, OrchestratorError, SensitiveOutput,
use aws_smithy_runtime_api::client::orchestrator::{Future, OrchestratorError, SensitiveOutput};
use aws_smithy_runtime_api::client::retries::classifiers::{
ClassifyRetry, RetryAction, SharedRetryClassifier,
};
use aws_smithy_runtime_api::client::retries::{ClassifyRetry, RetryClassifiers, RetryReason};
use aws_smithy_runtime_api::client::runtime_components::RuntimeComponentsBuilder;
use aws_smithy_runtime_api::client::runtime_plugin::{RuntimePlugin, SharedRuntimePlugin};
use aws_smithy_types::config_bag::{FrozenLayer, Layer};
use aws_smithy_types::endpoint::Endpoint;
use aws_smithy_types::retry::{ErrorKind, RetryConfig};
use aws_smithy_types::retry::RetryConfig;
use aws_smithy_types::timeout::TimeoutConfig;
use aws_types::os_shim_internal::Env;
use http::Uri;
Expand Down Expand Up @@ -250,9 +250,7 @@ impl ImdsCommonRuntimePlugin {
.with_http_client(config.http_client())
.with_endpoint_resolver(Some(endpoint_resolver))
.with_interceptor(UserAgentInterceptor::new())
.with_retry_classifiers(Some(
RetryClassifiers::new().with_classifier(ImdsResponseRetryClassifier),
))
.with_retry_classifier(SharedRetryClassifier::new(ImdsResponseRetryClassifier))
.with_retry_strategy(Some(StandardRetryStrategy::new(retry_config)))
.with_time_source(Some(config.time_source()))
.with_sleep_impl(config.sleep_impl()),
Expand Down Expand Up @@ -548,32 +546,26 @@ impl EndpointResolver for ImdsEndpointResolver {
#[derive(Clone, Debug)]
struct ImdsResponseRetryClassifier;

impl ImdsResponseRetryClassifier {
fn classify(response: &HttpResponse) -> Option<RetryReason> {
let status = response.status();
match status {
_ if status.is_server_error() => Some(RetryReason::Error(ErrorKind::ServerError)),
// 401 indicates that the token has expired, this is retryable
_ if status.as_u16() == 401 => Some(RetryReason::Error(ErrorKind::ServerError)),
// This catch-all includes successful responses that fail to parse. These should not be retried.
_ => None,
}
}
}

impl ClassifyRetry for ImdsResponseRetryClassifier {
fn name(&self) -> &'static str {
"ImdsResponseRetryClassifier"
}

fn classify_retry(&self, ctx: &InterceptorContext) -> Option<RetryReason> {
fn classify_retry(&self, ctx: &InterceptorContext) -> RetryAction {
if let Some(response) = ctx.response() {
Self::classify(response)
let status = response.status();
match status {
_ if status.is_server_error() => RetryAction::server_error(),
// 401 indicates that the token has expired, this is retryable
_ if status.as_u16() == 401 => RetryAction::server_error(),
// This catch-all includes successful responses that fail to parse. These should not be retried.
_ => RetryAction::NoActionIndicated,
}
} else {
// Don't retry timeouts for IMDS, or else it will take ~30 seconds for the default
// credentials provider chain to fail to provide credentials.
// Also don't retry non-responses.
None
RetryAction::NoActionIndicated
}
}
}
Expand All @@ -596,7 +588,7 @@ pub(crate) mod test {
use aws_smithy_runtime_api::client::orchestrator::{
HttpRequest, HttpResponse, OrchestratorError,
};
use aws_smithy_runtime_api::client::retries::ClassifyRetry;
use aws_smithy_runtime_api::client::retries::classifiers::{ClassifyRetry, RetryAction};
use aws_smithy_types::error::display::DisplayErrorContext;
use aws_types::os_shim_internal::{Env, Fs};
use http::header::USER_AGENT;
Expand Down Expand Up @@ -915,21 +907,27 @@ pub(crate) mod test {
http_client.assert_requests_match(&[]);
}

/// Successful responses should classify as `RetryKind::Unnecessary`
/// The classifier should return `None` when classifying a successful response.
#[test]
fn successful_response_properly_classified() {
let mut ctx = InterceptorContext::new(Input::doesnt_matter());
ctx.set_output_or_error(Ok(Output::doesnt_matter()));
ctx.set_response(imds_response("").map(|_| SdkBody::empty()));
let classifier = ImdsResponseRetryClassifier;
assert_eq!(None, classifier.classify_retry(&ctx));
assert_eq!(
RetryAction::NoActionIndicated,
classifier.classify_retry(&ctx)
);

// Emulate a failure to parse the response body (using an io error since it's easy to construct in a test)
let mut ctx = InterceptorContext::new(Input::doesnt_matter());
ctx.set_output_or_error(Err(OrchestratorError::connector(ConnectorError::io(
io::Error::new(io::ErrorKind::BrokenPipe, "fail to parse").into(),
))));
assert_eq!(None, classifier.classify_retry(&ctx));
assert_eq!(
RetryAction::NoActionIndicated,
classifier.classify_retry(&ctx)
);
}

// since tokens are sent as headers, the tokens need to be valid header values
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"name": "empty-config",
"name": "invalid-config",
"docs": "config was invalid",
"result": {
"ErrorContains": "could not parse profile file"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"name": "e2e-assume-role",
"name": "retry-on-error",
"docs": "end to end successful role assumption",
"result": {
"Ok": {
Expand Down
2 changes: 1 addition & 1 deletion aws/rust-runtime/aws-runtime/src/retries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@
*/

/// Classifiers that can inspect a response and determine if it should be retried.
pub mod classifier;
pub mod classifiers;
Loading

0 comments on commit 96a9f84

Please sign in to comment.