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

Revamp errors in aws-smithy-http #1884

Merged
merged 5 commits into from
Nov 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 18 additions & 14 deletions aws/rust-runtime/aws-config/src/http_credential_provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl HttpCredentialProvider {
let credentials = self.client.call(self.operation(auth)).await;
match credentials {
Ok(creds) => Ok(creds),
Err(SdkError::ServiceError { err, .. }) => Err(err),
Err(SdkError::ServiceError(context)) => Err(context.into_err()),
Err(other) => Err(CredentialsError::unhandled(other)),
}
}
Expand Down Expand Up @@ -176,13 +176,20 @@ impl ClassifyRetry<SdkSuccess<Credentials>, SdkError<CredentialsError>>
RetryKind::Error(ErrorKind::TransientError)
}
// non-parseable 200s
Err(SdkError::ServiceError {
err: CredentialsError::Unhandled { .. },
raw,
}) if raw.http().status().is_success() => RetryKind::Error(ErrorKind::ServerError),
Err(SdkError::ServiceError(context))
if matches!(context.err(), CredentialsError::Unhandled { .. })
&& context.raw().http().status().is_success() =>
{
RetryKind::Error(ErrorKind::ServerError)
}
// 5xx errors
Err(SdkError::ServiceError { raw, .. } | SdkError::ResponseError { raw, .. })
if raw.http().status().is_server_error() =>
Err(SdkError::ResponseError(context))
if context.raw().http().status().is_server_error() =>
{
RetryKind::Error(ErrorKind::ServerError)
}
Err(SdkError::ServiceError(context))
if context.raw().http().status().is_server_error() =>
{
RetryKind::Error(ErrorKind::ServerError)
}
Expand Down Expand Up @@ -219,10 +226,10 @@ mod test {
raw: operation::Response::new(resp.map(SdkBody::from)),
parsed: creds,
}),
Err(err) => Err(SdkError::ServiceError {
Err(err) => Err(SdkError::service_error(
err,
raw: operation::Response::new(resp.map(SdkBody::from)),
}),
operation::Response::new(resp.map(SdkBody::from)),
)),
}
}

Expand Down Expand Up @@ -278,10 +285,7 @@ mod test {
assert!(
matches!(
sdk_error,
SdkError::ServiceError {
err: CredentialsError::ProviderError { .. },
..
}
SdkError::ServiceError(ref context) if matches!(context.err(), CredentialsError::ProviderError { .. })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess all of CredentialsError:: are not part of this refactoring? Because CredentialsError::ProviderError looks like a struct variant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CredentialsError is refactored in #1922

),
"should be provider error: {}",
sdk_error
Expand Down
80 changes: 47 additions & 33 deletions aws/rust-runtime/aws-config/src/imds/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,23 +206,26 @@ impl Client {
.call(operation)
.await
.map_err(|err| match err {
SdkError::ConstructionFailure(err) => match err.downcast::<ImdsError>() {
Ok(token_failure) => *token_failure,
Err(other) => ImdsError::Unexpected(other),
},
SdkError::TimeoutError(err) => ImdsError::IoError(err),
SdkError::DispatchFailure(err) => ImdsError::IoError(err.into()),
SdkError::ResponseError { err, .. } => ImdsError::IoError(err),
SdkError::ServiceError {
err: InnerImdsError::BadStatus,
raw,
} => ImdsError::ErrorResponse {
response: raw.into_parts().0,
SdkError::ConstructionFailure(_) if err.source().is_some() => {
match err.into_source().map(|e| e.downcast::<ImdsError>()) {
Ok(Ok(token_failure)) => *token_failure,
Ok(Err(err)) => ImdsError::Unexpected(err),
Err(err) => ImdsError::Unexpected(err.into()),
}
}
SdkError::ConstructionFailure(_) => ImdsError::Unexpected(err.into()),
SdkError::ServiceError(context) => match context.err() {
InnerImdsError::InvalidUtf8 => {
ImdsError::Unexpected("IMDS returned invalid UTF-8".into())
}
InnerImdsError::BadStatus => ImdsError::ErrorResponse {
response: context.into_raw().into_parts().0,
},
},
SdkError::ServiceError {
err: InnerImdsError::InvalidUtf8,
..
} => ImdsError::Unexpected("IMDS returned invalid UTF-8".into()),
SdkError::TimeoutError(_)
| SdkError::DispatchFailure(_)
| SdkError::ResponseError(_) => ImdsError::IoError(err.into()),
_ => ImdsError::Unexpected(err.into()),
})
}

Expand Down Expand Up @@ -735,9 +738,8 @@ impl<T, E> ClassifyRetry<SdkSuccess<T>, SdkError<E>> for ImdsResponseRetryClassi
fn classify_retry(&self, response: Result<&SdkSuccess<T>, &SdkError<E>>) -> RetryKind {
match response {
Ok(_) => RetryKind::Unnecessary,
Err(SdkError::ResponseError { raw, .. }) | Err(SdkError::ServiceError { raw, .. }) => {
Self::classify(raw)
}
Err(SdkError::ResponseError(context)) => Self::classify(context.raw()),
Err(SdkError::ServiceError(context)) => Self::classify(context.raw()),
_ => RetryKind::UnretryableFailure,
}
}
Expand All @@ -764,6 +766,21 @@ pub(crate) mod test {
use std::time::{Duration, UNIX_EPOCH};
use tracing_test::traced_test;

macro_rules! assert_full_error_contains {
($err:expr, $contains:expr) => {
let err = $err;
let message = format!(
"{}",
aws_smithy_types::error::display::DisplayErrorContext(&err)
);
assert!(
message.contains($contains),
"Error message '{message}' didn't contain text '{}'",
$contains
);
};
}
Comment on lines +769 to +782
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we extract this to a shared utils crate? It seems useful.

Copy link
Collaborator Author

@jdisanti jdisanti Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely considered it, but I didn't want to create a whole crate just for this macro when there are only a few places it would be used outside of aws-smithy-http.

It almost seems like we should replace the aws-smithy-protocol-test crate with a general purpose testing crate.


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

Expand Down Expand Up @@ -1027,7 +1044,7 @@ pub(crate) mod test {
)]);
let client = make_client(&connection).await;
let err = client.get("/latest/metadata").await.expect_err("no token");
assert!(format!("{}", err).contains("forbidden"), "{}", err);
assert_full_error_contains!(err, "forbidden");
connection.assert_requests_match(&[]);
}

Expand All @@ -1050,10 +1067,10 @@ pub(crate) mod test {
);

// Emulate a failure to parse the response body (using an io error since it's easy to construct in a test)
let failure = SdkError::<()>::ResponseError {
err: Box::new(io::Error::new(io::ErrorKind::BrokenPipe, "fail to parse")),
raw: response_200(),
};
let failure = SdkError::<()>::response_error(
io::Error::new(io::ErrorKind::BrokenPipe, "fail to parse"),
response_200(),
);
assert_eq!(
RetryKind::UnretryableFailure,
classifier.classify_retry(Err::<&SdkSuccess<()>, _>(&failure))
Expand All @@ -1069,7 +1086,7 @@ pub(crate) mod test {
)]);
let client = make_client(&connection).await;
let err = client.get("/latest/metadata").await.expect_err("no token");
assert!(format!("{}", err).contains("Invalid Token"), "{}", err);
assert_full_error_contains!(err, "Invalid Token");
connection.assert_requests_match(&[]);
}

Expand All @@ -1090,7 +1107,7 @@ pub(crate) mod test {
]);
let client = make_client(&connection).await;
let err = client.get("/latest/metadata").await.expect_err("no token");
assert!(format!("{}", err).contains("invalid UTF-8"), "{}", err);
assert_full_error_contains!(err, "invalid UTF-8");
connection.assert_requests_match(&[]);
}

Expand All @@ -1099,6 +1116,7 @@ pub(crate) mod test {
#[cfg(any(feature = "rustls", feature = "native-tls"))]
async fn one_second_connect_timeout() {
use crate::imds::client::ImdsError;
use aws_smithy_types::error::display::DisplayErrorContext;
use std::time::SystemTime;

let client = Client::builder()
Expand All @@ -1124,7 +1142,8 @@ pub(crate) mod test {
time_elapsed
);
match resp {
ImdsError::FailedToLoadToken(err) if format!("{}", err).contains("timeout") => {} // ok,
ImdsError::FailedToLoadToken(err)
if format!("{}", DisplayErrorContext(&err)).contains("timeout") => {} // ok,
other => panic!(
"wrong error, expected construction failure with TimedOutError inside: {}",
other
Expand Down Expand Up @@ -1182,12 +1201,7 @@ pub(crate) mod test {
test, test_case.docs
),
(Err(substr), Err(err)) => {
assert!(
format!("{}", err).contains(substr),
"`{}` did not contain `{}`",
err,
substr
);
assert_full_error_contains!(err, substr);
return;
}
(Ok(_uri), Err(e)) => panic!(
Expand Down
10 changes: 7 additions & 3 deletions aws/rust-runtime/aws-config/src/imds/credentials.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use crate::imds::client::{ImdsError, LazyClient};
use crate::json_credentials::{parse_json_credentials, JsonCredentials, RefreshableCredentials};
use crate::provider_config::ProviderConfig;
use aws_smithy_client::SdkError;
use aws_smithy_types::error::display::DisplayErrorContext;
use aws_types::credentials::{future, CredentialsError, ProvideCredentials};
use aws_types::os_shim_internal::Env;
use aws_types::{credentials, Credentials};
Expand Down Expand Up @@ -137,9 +138,12 @@ impl ImdsCredentialsProvider {
);
Err(CredentialsError::not_loaded("received 404 from IMDS"))
}
Err(ImdsError::FailedToLoadToken(SdkError::DispatchFailure(err))) => Err(
CredentialsError::not_loaded(format!("could not communicate with imds: {}", err)),
),
Err(ImdsError::FailedToLoadToken(ref err @ SdkError::DispatchFailure(_))) => {
Err(CredentialsError::not_loaded(format!(
"could not communicate with IMDS: {}",
DisplayErrorContext(&err)
)))
}
Err(other) => Err(CredentialsError::provider_error(other)),
}
}
Expand Down
28 changes: 14 additions & 14 deletions aws/rust-runtime/aws-config/src/sts/assume_role.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use std::time::Duration;

use crate::meta::credentials::LazyCachingCredentialsProvider;
use crate::provider_config::ProviderConfig;
use aws_smithy_types::error::display::DisplayErrorContext;
use tracing::Instrument;

/// Credentials provider that uses credentials provided by another provider to assume a role
Expand Down Expand Up @@ -253,21 +254,20 @@ impl Inner {
);
super::util::into_credentials(assumed.credentials, "AssumeRoleProvider")
}
Err(SdkError::ServiceError { err, raw }) => {
match err.kind {
Err(SdkError::ServiceError(ref context))
if matches!(
context.err().kind,
AssumeRoleErrorKind::RegionDisabledException(_)
| AssumeRoleErrorKind::MalformedPolicyDocumentException(_) => {
return Err(CredentialsError::invalid_configuration(
SdkError::ServiceError { err, raw },
))
}
_ => {}
}
tracing::warn!(error = ?err.message(), "STS refused to grant assume role");
Err(CredentialsError::provider_error(SdkError::ServiceError {
err,
raw,
}))
| AssumeRoleErrorKind::MalformedPolicyDocumentException(_)
) =>
{
Err(CredentialsError::invalid_configuration(
assumed.err().unwrap(),
))
}
Err(SdkError::ServiceError(ref context)) => {
tracing::warn!(error = %DisplayErrorContext(context.err()), "STS refused to grant assume role");
Err(CredentialsError::provider_error(assumed.err().unwrap()))
}
Err(err) => Err(CredentialsError::provider_error(err)),
}
Expand Down
10 changes: 5 additions & 5 deletions aws/rust-runtime/aws-config/src/test_case.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use aws_types::os_shim_internal::{Env, Fs};
use serde::Deserialize;

use crate::connector::default_connector;
use aws_smithy_types::error::display::DisplayErrorContext;
use std::collections::HashMap;
use std::error::Error;
use std::fmt::Debug;
Expand Down Expand Up @@ -100,12 +101,11 @@ where
assert_eq!(expected, &actual.into(), "incorrect result was returned")
}
(Err(err), GenericTestResult::ErrorContains(substr)) => {
let message = format!("{}", DisplayErrorContext(&err));
assert!(
format!("{}", err).contains(substr),
"`{}` did not contain `{}`",
err,
substr
)
message.contains(substr),
"`{message}` did not contain `{substr}`"
);
}
(Err(actual_error), GenericTestResult::Ok(expected_creds)) => panic!(
"expected credentials ({:?}) but an error was returned: {}",
Expand Down
15 changes: 7 additions & 8 deletions aws/rust-runtime/aws-endpoint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ pub use partition::Partition;
pub use partition::PartitionResolver;
use std::collections::HashMap;

use aws_smithy_http::endpoint::Error as EndpointError;
use aws_smithy_http::endpoint::error::ResolveEndpointError;
use aws_smithy_http::endpoint::{apply_endpoint, EndpointPrefix, ResolveEndpoint};
use aws_smithy_http::middleware::MapRequest;
use aws_smithy_http::operation::Request;
Expand Down Expand Up @@ -54,19 +54,18 @@ impl EndpointShim {
}

impl ResolveEndpoint<Params> for EndpointShim {
fn resolve_endpoint(
&self,
params: &Params,
) -> Result<SmithyEndpoint, aws_smithy_http::endpoint::Error> {
fn resolve_endpoint(&self, params: &Params) -> Result<SmithyEndpoint, ResolveEndpointError> {
let aws_endpoint = self
.0
.resolve_endpoint(
params
.region
.as_ref()
.ok_or_else(|| EndpointError::message("no region in params"))?,
.ok_or_else(|| ResolveEndpointError::message("no region in params"))?,
)
.map_err(|err| EndpointError::message("failure resolving endpoint").with_cause(err))?;
.map_err(|err| {
ResolveEndpointError::message("failure resolving endpoint").with_source(err)
})?;
let uri = aws_endpoint.endpoint().uri();
let mut auth_scheme =
HashMap::from([("name".to_string(), Document::String("sigv4".into()))]);
Expand Down Expand Up @@ -134,7 +133,7 @@ impl MapRequest for AwsEndpointStage {
// its place
return Err(AwsEndpointStageError::EndpointResolutionError(std::mem::replace(
e,
aws_smithy_http::endpoint::Error::message("the original error was directly returned")
ResolveEndpointError::message("the original error was directly returned")
).into()));
}
};
Expand Down
16 changes: 8 additions & 8 deletions aws/rust-runtime/aws-http/src/retry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,10 @@ mod test {
err: E,
raw: http::Response<&'static str>,
) -> Result<SdkSuccess<()>, SdkError<E>> {
Err(SdkError::ServiceError {
Err(SdkError::service_error(
err,
raw: operation::Response::new(raw.map(SdkBody::from)),
})
operation::Response::new(raw.map(SdkBody::from)),
))
}

#[test]
Expand Down Expand Up @@ -263,10 +263,10 @@ mod test {
let policy = AwsResponseRetryClassifier::new();
assert_eq!(
policy.classify_retry(
Result::<SdkSuccess<()>, SdkError<UnmodeledError>>::Err(SdkError::ResponseError {
err: Box::new(UnmodeledError),
raw: operation::Response::new(http::Response::new("OK").map(SdkBody::from)),
})
Result::<SdkSuccess<()>, SdkError<UnmodeledError>>::Err(SdkError::response_error(
UnmodeledError,
operation::Response::new(http::Response::new("OK").map(SdkBody::from)),
))
.as_ref()
),
RetryKind::Error(ErrorKind::TransientError)
Expand All @@ -276,7 +276,7 @@ mod test {
#[test]
fn test_timeout_error() {
let policy = AwsResponseRetryClassifier::new();
let err: Result<(), SdkError<UnmodeledError>> = Err(SdkError::TimeoutError("blah".into()));
let err: Result<(), SdkError<UnmodeledError>> = Err(SdkError::timeout_error("blah"));
assert_eq!(
policy.classify_retry(err.as_ref()),
RetryKind::Error(ErrorKind::TransientError)
Expand Down
Loading