Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement user-configurable retry classifiers #2977

Merged
merged 29 commits into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
7c4a63f
implement user-configurable retry classifiers
Velfi Sep 8, 2023
d994100
Merge branch 'main' into zhessler-user-configurable-retry-classifiers
Velfi Oct 4, 2023
c8c05d4
fix codegen test
Velfi Oct 4, 2023
425e5f4
fix KMS integration test
Velfi Oct 4, 2023
91ad658
update retry classifiers to respect results from higher-priority clas…
Velfi Oct 5, 2023
862d79d
fix test_retry_classifier_customization
Velfi Oct 5, 2023
b6bd15a
rename SmithyErrorClassifier to TransientErrorClassifier
Velfi Oct 5, 2023
ee3707a
fix unused import warning in codegen
Velfi Oct 5, 2023
f8abd87
fix more tests
Velfi Oct 5, 2023
e2da8a1
fix another test
Velfi Oct 6, 2023
cfea5f4
Merge branch 'main' into zhessler-user-configurable-retry-classifiers
Velfi Oct 6, 2023
f6be431
rename RetryAction variants
Velfi Oct 6, 2023
2baefa4
fix retry action docs
Velfi Oct 6, 2023
a5a94d8
respond to PR comments
Velfi Oct 6, 2023
10aca1b
Merge branch 'main' into zhessler-user-configurable-retry-classifiers
Velfi Oct 6, 2023
32ee53f
fix test broken by merge
Velfi Oct 6, 2023
55e2a5c
remove the unused FixedDelayRetryStrategy
Velfi Oct 6, 2023
8598891
flip RetryClassifierPriority
Velfi Oct 6, 2023
0be791d
fix missing classifier variant in standard retry strategy
Velfi Oct 9, 2023
2f221c6
fix sneaky extra writable
Velfi Oct 9, 2023
c41317e
remove unused import
Velfi Oct 9, 2023
33900bd
refactor retry actions and reasons
Velfi Oct 9, 2023
9e4ab0f
Merge branch 'main' into zhessler-user-configurable-retry-classifiers
Velfi Oct 9, 2023
28df537
fix test
Velfi Oct 10, 2023
e406aa1
Merge branch 'main' into zhessler-user-configurable-retry-classifiers
Velfi Oct 10, 2023
e8b28b0
add docs to generated retry classifier setters
Velfi Oct 10, 2023
76b2b8e
add retry classifier config override test
Velfi Oct 10, 2023
2402d1c
Merge branch 'main' into zhessler-user-configurable-retry-classifiers
Velfi Oct 10, 2023
918a2db
Merge branch 'main' into zhessler-user-configurable-retry-classifiers
Velfi Oct 10, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
Velfi marked this conversation as resolved.
Show resolved Hide resolved
Loading
Loading