From c9d0970957d97fad1f34bd78897db67c280506ac Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 28 Oct 2022 16:51:55 -0700 Subject: [PATCH 1/6] Revamp errors in `aws-types` --- .../aws-config/src/meta/credentials/chain.rs | 10 +- .../aws-config/src/web_identity_token.rs | 3 +- .../imds_default_chain_error/test-case.json | 2 +- .../imds_disabled/test-case.json | 2 +- .../imds_no_iam_role/test-case.json | 2 +- .../credential_process_failure/test-case.json | 2 +- .../empty_config/test-case.json | 2 +- .../aws-types/src/credentials/mod.rs | 3 +- .../aws-types/src/credentials/provider.rs | 254 ++++++++++-------- 9 files changed, 154 insertions(+), 126 deletions(-) diff --git a/aws/rust-runtime/aws-config/src/meta/credentials/chain.rs b/aws/rust-runtime/aws-config/src/meta/credentials/chain.rs index 41d30caec0..30d42993e0 100644 --- a/aws/rust-runtime/aws-config/src/meta/credentials/chain.rs +++ b/aws/rust-runtime/aws-config/src/meta/credentials/chain.rs @@ -82,12 +82,12 @@ impl CredentialsProviderChain { tracing::debug!(provider = %name, "loaded credentials"); return Ok(credentials); } - Err(CredentialsError::CredentialsNotLoaded { context, .. }) => { - tracing::debug!(provider = %name, context = %context, "provider in chain did not provide credentials"); + Err(err @ CredentialsError::CredentialsNotLoaded(_)) => { + tracing::debug!(provider = %name, context = %DisplayErrorContext(&err), "provider in chain did not provide credentials"); } - Err(e) => { - tracing::warn!(provider = %name, error = %DisplayErrorContext(&e), "provider failed to provide credentials"); - return Err(e); + Err(err) => { + tracing::warn!(provider = %name, error = %DisplayErrorContext(&err), "provider failed to provide credentials"); + return Err(err); } } } diff --git a/aws/rust-runtime/aws-config/src/web_identity_token.rs b/aws/rust-runtime/aws-config/src/web_identity_token.rs index 3e6a1df9d4..b1edaec822 100644 --- a/aws/rust-runtime/aws-config/src/web_identity_token.rs +++ b/aws/rust-runtime/aws-config/src/web_identity_token.rs @@ -266,6 +266,7 @@ mod test { }; use aws_sdk_sts::Region; use aws_smithy_async::rt::sleep::TokioSleep; + use aws_smithy_types::error::display::DisplayErrorContext; use aws_types::credentials::CredentialsError; use aws_types::os_shim_internal::{Env, Fs}; use std::collections::HashMap; @@ -308,7 +309,7 @@ mod test { .await .expect_err("should fail, provider not loaded"); assert!( - format!("{}", err).contains("AWS_ROLE_ARN"), + format!("{}", DisplayErrorContext(&err)).contains("AWS_ROLE_ARN"), "`{}` did not contain expected string", err ); diff --git a/aws/rust-runtime/aws-config/test-data/default-provider-chain/imds_default_chain_error/test-case.json b/aws/rust-runtime/aws-config/test-data/default-provider-chain/imds_default_chain_error/test-case.json index c8224c50a3..da2abbfc5c 100644 --- a/aws/rust-runtime/aws-config/test-data/default-provider-chain/imds_default_chain_error/test-case.json +++ b/aws/rust-runtime/aws-config/test-data/default-provider-chain/imds_default_chain_error/test-case.json @@ -2,6 +2,6 @@ "name": "imds-default-chain", "docs": "IMDS isn't specifically configured but is loaded as part of the default chain. This has the exact same HTTP traffic as imds_no_iam_role, they are equivalent.", "result": { - "ErrorContains": "The credential provider was not enabled" + "ErrorContains": "the credential provider was not enabled" } } diff --git a/aws/rust-runtime/aws-config/test-data/default-provider-chain/imds_disabled/test-case.json b/aws/rust-runtime/aws-config/test-data/default-provider-chain/imds_disabled/test-case.json index 2b00bcf2ec..4712282501 100644 --- a/aws/rust-runtime/aws-config/test-data/default-provider-chain/imds_disabled/test-case.json +++ b/aws/rust-runtime/aws-config/test-data/default-provider-chain/imds_disabled/test-case.json @@ -2,6 +2,6 @@ "name": "imds-disabled", "docs": "when IMDS is disabled by an environment variable, it shouldn't be used as part of the default chain", "result": { - "ErrorContains": "The credential provider was not enabled" + "ErrorContains": "the credential provider was not enabled" } } diff --git a/aws/rust-runtime/aws-config/test-data/default-provider-chain/imds_no_iam_role/test-case.json b/aws/rust-runtime/aws-config/test-data/default-provider-chain/imds_no_iam_role/test-case.json index cd0cd29dc1..5c4a015f99 100644 --- a/aws/rust-runtime/aws-config/test-data/default-provider-chain/imds_no_iam_role/test-case.json +++ b/aws/rust-runtime/aws-config/test-data/default-provider-chain/imds_no_iam_role/test-case.json @@ -2,6 +2,6 @@ "name": "imds-token-fail", "docs": "attempts to acquire an IMDS token, but the instance doesn't have a role configured", "result": { - "ErrorContains": "The credential provider was not enabled" + "ErrorContains": "the credential provider was not enabled" } } diff --git a/aws/rust-runtime/aws-config/test-data/profile-provider/credential_process_failure/test-case.json b/aws/rust-runtime/aws-config/test-data/profile-provider/credential_process_failure/test-case.json index a905cc413e..16b75a9476 100644 --- a/aws/rust-runtime/aws-config/test-data/profile-provider/credential_process_failure/test-case.json +++ b/aws/rust-runtime/aws-config/test-data/profile-provider/credential_process_failure/test-case.json @@ -2,6 +2,6 @@ "name": "credential_process_failure", "docs": "credential_process handles the external process exiting with a non-zero exit code", "result": { - "ErrorContains": "An error occurred while loading credentials: Error retrieving credentials: external process exited with code exit status: 1" + "ErrorContains": "an error occurred while loading credentials: Error retrieving credentials: external process exited with code exit status: 1" } } diff --git a/aws/rust-runtime/aws-config/test-data/profile-provider/empty_config/test-case.json b/aws/rust-runtime/aws-config/test-data/profile-provider/empty_config/test-case.json index ca5828b757..d954e883ab 100644 --- a/aws/rust-runtime/aws-config/test-data/profile-provider/empty_config/test-case.json +++ b/aws/rust-runtime/aws-config/test-data/profile-provider/empty_config/test-case.json @@ -2,6 +2,6 @@ "name": "empty-config", "docs": "no config was defined", "result": { - "ErrorContains": "The credential provider was not enabled" + "ErrorContains": "the credential provider was not enabled" } } diff --git a/aws/rust-runtime/aws-types/src/credentials/mod.rs b/aws/rust-runtime/aws-types/src/credentials/mod.rs index 6f0e6f7d88..c77343651c 100644 --- a/aws/rust-runtime/aws-types/src/credentials/mod.rs +++ b/aws/rust-runtime/aws-types/src/credentials/mod.rs @@ -71,8 +71,9 @@ mod credentials_impl; mod provider; pub use credentials_impl::Credentials; +pub use provider::error; +pub use provider::error::CredentialsError; pub use provider::future; -pub use provider::CredentialsError; pub use provider::ProvideCredentials; pub use provider::Result; pub use provider::SharedCredentialsProvider; diff --git a/aws/rust-runtime/aws-types/src/credentials/provider.rs b/aws/rust-runtime/aws-types/src/credentials/provider.rs index 04a425680b..3811f6e1c4 100644 --- a/aws/rust-runtime/aws-types/src/credentials/provider.rs +++ b/aws/rust-runtime/aws-types/src/credentials/provider.rs @@ -4,147 +4,173 @@ */ use crate::Credentials; -use std::error::Error; -use std::fmt::{self, Debug, Display, Formatter}; use std::sync::Arc; -use std::time::Duration; -/// Error returned when credentials failed to load. -#[derive(Debug)] -#[non_exhaustive] -pub enum CredentialsError { - /// No credentials were available for this provider - #[non_exhaustive] - CredentialsNotLoaded { - /// Underlying cause of the error. - context: Box, - }, +/// Credentials provider errors +pub mod error { + use std::error::Error; + use std::fmt; + use std::time::Duration; - /// Loading credentials from this provider exceeded the maximum allowed duration - #[non_exhaustive] - ProviderTimedOut(Duration), + /// Details for [`CredentialsError::CredentialsNotLoaded`] + #[derive(Debug)] + pub struct CredentialsNotLoaded { + source: Box, + } - /// The provider was given an invalid configuration - /// - /// For example: - /// - syntax error in ~/.aws/config - /// - assume role profile that forms an infinite loop - #[non_exhaustive] - InvalidConfiguration { - /// Underlying cause of the error. - cause: Box, - }, + /// Details for [`CredentialsError::ProviderTimedOut`] + #[derive(Debug)] + pub struct ProviderTimedOut { + timeout_duration: Duration, + } - /// The provider experienced an error during credential resolution - /// - /// This may include errors like a 503 from STS or a file system error when attempting to - /// read a configuration file. - #[non_exhaustive] - ProviderError { - /// Underlying cause of the error. - cause: Box, - }, + impl ProviderTimedOut { + /// Returns the maximum allowed timeout duration that was exceeded + pub fn timeout_duration(&self) -> Duration { + self.timeout_duration + } + } - /// An unexpected error occurred during credential resolution - /// - /// If the error is something that can occur during expected usage of a provider, `ProviderError` - /// should be returned instead. Unhandled is reserved for exceptional cases, for example: - /// - Returned data not UTF-8 - /// - A provider returns data that is missing required fields + /// Details for [`CredentialsError::InvalidConfiguration`] + #[derive(Debug)] + pub struct InvalidConfiguration { + source: Box, + } + + /// Details for [`CredentialsError::ProviderError`] + #[derive(Debug)] + pub struct ProviderError { + source: Box, + } + + /// Details for [`CredentialsError::Unhandled`] + #[derive(Debug)] + pub struct Unhandled { + source: Box, + } + + /// Error returned when credentials failed to load. + #[derive(Debug)] #[non_exhaustive] - Unhandled { - /// Underlying cause of the error. - cause: Box, - }, -} + pub enum CredentialsError { + /// No credentials were available for this provider + CredentialsNotLoaded(CredentialsNotLoaded), -impl CredentialsError { - /// The credentials provider did not provide credentials - /// - /// This error indicates the credentials provider was not enable or no configuration was set. - /// This contrasts with [`invalid_configuration`](CredentialsError::InvalidConfiguration), indicating - /// that the provider was configured in some way, but certain settings were invalid. - pub fn not_loaded(context: impl Into>) -> Self { - CredentialsError::CredentialsNotLoaded { - context: context.into(), - } + /// Loading credentials from this provider exceeded the maximum allowed duration + ProviderTimedOut(ProviderTimedOut), + + /// The provider was given an invalid configuration + /// + /// For example: + /// - syntax error in ~/.aws/config + /// - assume role profile that forms an infinite loop + InvalidConfiguration(InvalidConfiguration), + + /// The provider experienced an error during credential resolution + /// + /// This may include errors like a 503 from STS or a file system error when attempting to + /// read a configuration file. + ProviderError(ProviderError), + + /// An unexpected error occurred during credential resolution + /// + /// If the error is something that can occur during expected usage of a provider, `ProviderError` + /// should be returned instead. Unhandled is reserved for exceptional cases, for example: + /// - Returned data not UTF-8 + /// - A provider returns data that is missing required fields + Unhandled(Unhandled), } - /// An unexpected error occurred loading credentials from this provider - /// - /// Unhandled errors should not occur during normal operation and should be reserved for exceptional - /// cases, such as a JSON API returning an output that was not parseable as JSON. - pub fn unhandled(cause: impl Into>) -> Self { - Self::Unhandled { - cause: cause.into(), + impl CredentialsError { + /// The credentials provider did not provide credentials + /// + /// This error indicates the credentials provider was not enable or no configuration was set. + /// This contrasts with [`invalid_configuration`](CredentialsError::InvalidConfiguration), indicating + /// that the provider was configured in some way, but certain settings were invalid. + pub fn not_loaded(source: impl Into>) -> Self { + CredentialsError::CredentialsNotLoaded(CredentialsNotLoaded { + source: source.into(), + }) } - } - /// The credentials provider returned an error - /// - /// Provider errors may occur during normal use of a credentials provider, e.g. a 503 when - /// retrieving credentials from IMDS. - pub fn provider_error(cause: impl Into>) -> Self { - Self::ProviderError { - cause: cause.into(), + /// An unexpected error occurred loading credentials from this provider + /// + /// Unhandled errors should not occur during normal operation and should be reserved for exceptional + /// cases, such as a JSON API returning an output that was not parseable as JSON. + pub fn unhandled(source: impl Into>) -> Self { + Self::Unhandled(Unhandled { + source: source.into(), + }) } - } - /// The provided configuration for a provider was invalid - pub fn invalid_configuration(cause: impl Into>) -> Self { - Self::InvalidConfiguration { - cause: cause.into(), + /// The credentials provider returned an error + /// + /// Provider errors may occur during normal use of a credentials provider, e.g. a 503 when + /// retrieving credentials from IMDS. + pub fn provider_error(source: impl Into>) -> Self { + Self::ProviderError(ProviderError { + source: source.into(), + }) + } + + /// The provided configuration for a provider was invalid + pub fn invalid_configuration( + source: impl Into>, + ) -> Self { + Self::InvalidConfiguration(InvalidConfiguration { + source: source.into(), + }) } - } - /// The credentials provider did not provide credentials within an allotted duration - pub fn provider_timed_out(context: Duration) -> Self { - Self::ProviderTimedOut(context) + /// The credentials provider did not provide credentials within an allotted duration + pub fn provider_timed_out(timeout_duration: Duration) -> Self { + Self::ProviderTimedOut(ProviderTimedOut { timeout_duration }) + } } -} -impl Display for CredentialsError { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - match self { - CredentialsError::CredentialsNotLoaded { context } => { - write!(f, "The credential provider was not enabled: {}", context) - } - CredentialsError::ProviderTimedOut(d) => write!( - f, - "Credentials provider timed out after {} seconds", - d.as_secs() - ), - CredentialsError::Unhandled { cause } => { - write!(f, "Unexpected credentials error: {}", cause) - } - CredentialsError::InvalidConfiguration { cause } => { - write!( + impl fmt::Display for CredentialsError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + CredentialsError::CredentialsNotLoaded(_) => { + write!(f, "the credential provider was not enabled") + } + CredentialsError::ProviderTimedOut(details) => write!( f, - "The credentials provider was not properly configured: {}", - cause - ) - } - CredentialsError::ProviderError { cause } => { - write!(f, "An error occurred while loading credentials: {}", cause) + "credentials provider timed out after {} seconds", + details.timeout_duration.as_secs() + ), + CredentialsError::InvalidConfiguration(_) => { + write!(f, "the credentials provider was not properly configured") + } + CredentialsError::ProviderError(_) => { + write!(f, "an error occurred while loading credentials") + } + CredentialsError::Unhandled(_) => { + write!(f, "unexpected credentials error") + } } } } -} -impl Error for CredentialsError { - fn source(&self) -> Option<&(dyn Error + 'static)> { - match self { - CredentialsError::Unhandled { cause } - | CredentialsError::ProviderError { cause } - | CredentialsError::InvalidConfiguration { cause } => Some(cause.as_ref() as _), - CredentialsError::CredentialsNotLoaded { context } => Some(context.as_ref() as _), - _ => None, + impl Error for CredentialsError { + fn source(&self) -> Option<&(dyn Error + 'static)> { + match self { + CredentialsError::CredentialsNotLoaded(details) => { + Some(details.source.as_ref() as _) + } + CredentialsError::ProviderTimedOut(_) => None, + CredentialsError::InvalidConfiguration(details) => { + Some(details.source.as_ref() as _) + } + CredentialsError::ProviderError(details) => Some(details.source.as_ref() as _), + CredentialsError::Unhandled(details) => Some(details.source.as_ref() as _), + } } } } /// Result type for credential providers. -pub type Result = std::result::Result; +pub type Result = std::result::Result; /// Convenience `ProvideCredentials` struct that implements the `ProvideCredentials` trait. pub mod future { @@ -181,7 +207,7 @@ pub mod future { } /// Asynchronous Credentials Provider -pub trait ProvideCredentials: Send + Sync + Debug { +pub trait ProvideCredentials: Send + Sync + std::fmt::Debug { /// Returns a future that provides credentials. fn provide_credentials<'a>(&'a self) -> future::ProvideCredentials<'a> where From 5b590372f03e01bc8a070b0359f3262de0abd00a Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 28 Oct 2022 17:13:32 -0700 Subject: [PATCH 2/6] Revamp errors in `aws-endpoint` --- aws/rust-runtime/aws-endpoint/src/lib.rs | 49 +++++++++++++++++------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/aws/rust-runtime/aws-endpoint/src/lib.rs b/aws/rust-runtime/aws-endpoint/src/lib.rs index 384a071595..a3f7f1d0b6 100644 --- a/aws/rust-runtime/aws-endpoint/src/lib.rs +++ b/aws/rust-runtime/aws-endpoint/src/lib.rs @@ -24,7 +24,6 @@ use http::header::HeaderName; use http::{HeaderValue, Uri}; use std::error::Error; use std::fmt; -use std::fmt::{Debug, Display, Formatter}; use std::str::FromStr; use std::sync::Arc; @@ -100,19 +99,41 @@ impl ResolveEndpoint for EndpointShim { pub struct AwsEndpointStage; #[derive(Debug)] -pub enum AwsEndpointStageError { +enum AwsEndpointStageErrorKind { NoEndpointResolver, - NoRegion, EndpointResolutionError(BoxError), } -impl Display for AwsEndpointStageError { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - Debug::fmt(self, f) +#[derive(Debug)] +pub struct AwsEndpointStageError { + kind: AwsEndpointStageErrorKind, +} + +impl fmt::Display for AwsEndpointStageError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use AwsEndpointStageErrorKind::*; + match &self.kind { + NoEndpointResolver => write!(f, "endpoint resolution failed: no endpoint resolver"), + EndpointResolutionError(_) => write!(f, "endpoint resolution failed"), + } + } +} + +impl Error for AwsEndpointStageError { + fn source(&self) -> Option<&(dyn Error + 'static)> { + use AwsEndpointStageErrorKind::*; + match &self.kind { + EndpointResolutionError(source) => Some(source.as_ref() as _), + NoEndpointResolver => None, + } } } -impl Error for AwsEndpointStageError {} +impl From for AwsEndpointStageError { + fn from(kind: AwsEndpointStageErrorKind) -> Self { + Self { kind } + } +} impl MapRequest for AwsEndpointStage { type Error = AwsEndpointStageError; @@ -121,7 +142,7 @@ impl MapRequest for AwsEndpointStage { request.augment(|mut http_req, props| { let endpoint_result = props .get_mut::() - .ok_or(AwsEndpointStageError::NoEndpointResolver)?; + .ok_or(AwsEndpointStageErrorKind::NoEndpointResolver)?; let endpoint = match endpoint_result { // downgrade the mut ref to a shared ref Ok(_endpoint) => props.get::() @@ -131,25 +152,25 @@ impl MapRequest for AwsEndpointStage { Err(e) => { // We need to own the error to return it, so take it and leave a stub error in // its place - return Err(AwsEndpointStageError::EndpointResolutionError(std::mem::replace( + return Err(AwsEndpointStageErrorKind::EndpointResolutionError(std::mem::replace( e, ResolveEndpointError::message("the original error was directly returned") - ).into())); + ).into()).into()); } }; let (uri, signing_scope_override, signing_service_override) = smithy_to_aws(endpoint) - .map_err(|err| AwsEndpointStageError::EndpointResolutionError(err))?; + .map_err(|err| AwsEndpointStageErrorKind::EndpointResolutionError(err))?; tracing::debug!(endpoint = ?endpoint, base_region = ?signing_scope_override, "resolved endpoint"); apply_endpoint(http_req.uri_mut(), &uri, props.get::()) - .map_err(|err|AwsEndpointStageError::EndpointResolutionError(err.into()))?; + .map_err(|err| AwsEndpointStageErrorKind::EndpointResolutionError(err.into()))?; for (header_name, header_values) in endpoint.headers() { http_req.headers_mut().remove(header_name); for value in header_values { http_req.headers_mut().insert( HeaderName::from_str(header_name) - .map_err(|err|AwsEndpointStageError::EndpointResolutionError(err.into()))?, + .map_err(|err| AwsEndpointStageErrorKind::EndpointResolutionError(err.into()))?, HeaderValue::from_str(value) - .map_err(|err|AwsEndpointStageError::EndpointResolutionError(err.into()))?, + .map_err(|err| AwsEndpointStageErrorKind::EndpointResolutionError(err.into()))?, ); } } From 944e9d3988f1ddba097c7deacfcac452ec5b3b04 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 28 Oct 2022 17:30:38 -0700 Subject: [PATCH 3/6] Revamp errors in `aws-http` --- aws/rust-runtime/aws-http/src/auth.rs | 34 ++++++++---------- aws/rust-runtime/aws-http/src/user_agent.rs | 40 ++++++++++++++++----- 2 files changed, 45 insertions(+), 29 deletions(-) diff --git a/aws/rust-runtime/aws-http/src/auth.rs b/aws/rust-runtime/aws-http/src/auth.rs index bab76b4bb5..2cb75e08cc 100644 --- a/aws/rust-runtime/aws-http/src/auth.rs +++ b/aws/rust-runtime/aws-http/src/auth.rs @@ -54,7 +54,7 @@ impl CredentialsStage { } // if we get another error class, there is probably something actually wrong that the user will // want to know about - Err(other) => return Err(CredentialsStageError::CredentialsLoadingError(other)), + Err(other) => return Err(other.into()), } Ok(request) } @@ -67,34 +67,28 @@ mod error { /// Failures that can occur in the credentials middleware. #[derive(Debug)] - pub enum CredentialsStageError { - /// No credentials provider was found in the property bag for the operation. - MissingCredentialsProvider, - /// Failed to load credentials with the credential provider in the property bag. - CredentialsLoadingError(CredentialsError), + pub struct CredentialsStageError { + source: CredentialsError, } - impl StdError for CredentialsStageError {} + impl StdError for CredentialsStageError { + fn source(&self) -> Option<&(dyn StdError + 'static)> { + Some(&self.source as _) + } + } impl fmt::Display for CredentialsStageError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - use CredentialsStageError::*; - match self { - MissingCredentialsProvider => { - write!(f, "No credentials provider in the property bag") - } - CredentialsLoadingError(err) => write!( - f, - "Failed to load credentials from the credentials provider: {}", - err - ), - } + write!( + f, + "failed to load credentials from the credentials provider" + ) } } impl From for CredentialsStageError { - fn from(err: CredentialsError) -> Self { - CredentialsStageError::CredentialsLoadingError(err) + fn from(source: CredentialsError) -> Self { + CredentialsStageError { source } } } } diff --git a/aws/rust-runtime/aws-http/src/user_agent.rs b/aws/rust-runtime/aws-http/src/user_agent.rs index 1a2fd7a185..22e7727c42 100644 --- a/aws/rust-runtime/aws-http/src/user_agent.rs +++ b/aws/rust-runtime/aws-http/src/user_agent.rs @@ -525,31 +525,53 @@ impl UserAgentStage { } } -/// Failures that can arise from the user agent middleware #[derive(Debug)] -pub enum UserAgentStageError { +enum UserAgentStageErrorKind { /// There was no [`AwsUserAgent`] in the property bag. UserAgentMissing, /// The formatted user agent string is not a valid HTTP header value. This indicates a bug. InvalidHeader(InvalidHeaderValue), } -impl Error for UserAgentStageError {} +/// Failures that can arise from the user agent middleware +#[derive(Debug)] +pub struct UserAgentStageError { + kind: UserAgentStageErrorKind, +} + +impl Error for UserAgentStageError { + fn source(&self) -> Option<&(dyn Error + 'static)> { + use UserAgentStageErrorKind::*; + match &self.kind { + InvalidHeader(source) => Some(source as _), + UserAgentMissing => None, + } + } +} impl fmt::Display for UserAgentStageError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::UserAgentMissing => write!(f, "User agent missing from property bag"), - Self::InvalidHeader(_) => { - write!(f, "Provided user agent header was invalid. This is a bug.") + use UserAgentStageErrorKind::*; + match self.kind { + UserAgentMissing => write!(f, "user agent missing from property bag"), + InvalidHeader(_) => { + write!(f, "provided user agent header was invalid (this is a bug)") } } } } +impl From for UserAgentStageError { + fn from(kind: UserAgentStageErrorKind) -> Self { + Self { kind } + } +} + impl From for UserAgentStageError { fn from(value: InvalidHeaderValue) -> Self { - UserAgentStageError::InvalidHeader(value) + Self { + kind: UserAgentStageErrorKind::InvalidHeader(value), + } } } @@ -564,7 +586,7 @@ impl MapRequest for UserAgentStage { request.augment(|mut req, conf| { let ua = conf .get::() - .ok_or(UserAgentStageError::UserAgentMissing)?; + .ok_or(UserAgentStageErrorKind::UserAgentMissing)?; req.headers_mut() .append(USER_AGENT, HeaderValue::try_from(ua.ua_header())?); req.headers_mut().append( From 3b5625b13e907fde61292d7c4d0181cdc62244b3 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 28 Oct 2022 17:40:35 -0700 Subject: [PATCH 4/6] Revamp errors in `aws-sig-auth` --- .../aws-sig-auth/src/middleware.rs | 78 +++++++++++-------- 1 file changed, 46 insertions(+), 32 deletions(-) diff --git a/aws/rust-runtime/aws-sig-auth/src/middleware.rs b/aws/rust-runtime/aws-sig-auth/src/middleware.rs index bd647565b3..1c8770df85 100644 --- a/aws/rust-runtime/aws-sig-auth/src/middleware.rs +++ b/aws/rust-runtime/aws-sig-auth/src/middleware.rs @@ -61,71 +61,80 @@ impl SigV4SigningStage { } #[derive(Debug)] -pub enum SigningStageError { +enum SigningStageErrorKind { MissingCredentials, MissingSigningRegion, MissingSigningService, MissingSigningConfig, - InvalidBodyType, SigningFailure(SigningError), } +#[derive(Debug)] +pub struct SigningStageError { + kind: SigningStageErrorKind, +} + impl Display for SigningStageError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - match self { - SigningStageError::MissingCredentials => { - write!(f, "No credentials in the property bag") + use SigningStageErrorKind::*; + match self.kind { + MissingCredentials => { + write!(f, "no credentials in the property bag") } - SigningStageError::MissingSigningRegion => { - write!(f, "No signing region in the property bag") + MissingSigningRegion => { + write!(f, "no signing region in the property bag") } - SigningStageError::MissingSigningService => { - write!(f, "No signing service in the property bag") + MissingSigningService => { + write!(f, "no signing service in the property bag") } - SigningStageError::MissingSigningConfig => { - write!(f, "No signing configuration in the property bag") + MissingSigningConfig => { + write!(f, "no signing configuration in the property bag") } - SigningStageError::InvalidBodyType => write!( - f, - "The request body could not be signed by this configuration" - ), - SigningStageError::SigningFailure(_) => write!(f, "Signing failed"), + SigningFailure(_) => write!(f, "signing failed"), } } } -impl From for SigningStageError { - fn from(error: SigningError) -> Self { - Self::SigningFailure(error) - } -} - impl Error for SigningStageError { fn source(&self) -> Option<&(dyn Error + 'static)> { - match &self { - SigningStageError::SigningFailure(err) => Some(err), + match &self.kind { + SigningStageErrorKind::SigningFailure(err) => Some(err), _ => None, } } } +impl From for SigningStageError { + fn from(kind: SigningStageErrorKind) -> Self { + Self { kind } + } +} + +impl From for SigningStageError { + fn from(error: SigningError) -> Self { + Self { + kind: SigningStageErrorKind::SigningFailure(error), + } + } +} + /// Extract a signing config from a [`PropertyBag`](aws_smithy_http::property_bag::PropertyBag) fn signing_config( config: &PropertyBag, ) -> Result<(&OperationSigningConfig, RequestConfig, Credentials), SigningStageError> { let operation_config = config .get::() - .ok_or(SigningStageError::MissingSigningConfig)?; + .ok_or(SigningStageErrorKind::MissingSigningConfig)?; let credentials = config .get::() - .ok_or(SigningStageError::MissingCredentials)? + .ok_or(SigningStageErrorKind::MissingCredentials)? .clone(); let region = config .get::() - .ok_or(SigningStageError::MissingSigningRegion)?; + .ok_or(SigningStageErrorKind::MissingSigningRegion)?; let signing_service = config .get::() - .ok_or(SigningStageError::MissingSigningService)?; + .ok_or(SigningStageErrorKind::MissingSigningService)?; let payload_override = config.get::>(); let request_config = RequestConfig { request_ts: config @@ -146,7 +155,7 @@ impl MapRequest for SigV4SigningStage { req.augment(|mut req, config| { let operation_config = config .get::() - .ok_or(SigningStageError::MissingSigningConfig)?; + .ok_or(SigningStageErrorKind::MissingSigningConfig)?; let (operation_config, request_config, creds) = match &operation_config.signing_requirements { SigningRequirements::Disabled => return Ok(req), @@ -160,7 +169,7 @@ impl MapRequest for SigV4SigningStage { let signature = self .signer .sign(operation_config, &request_config, &creds, &mut req) - .map_err(SigningStageError::SigningFailure)?; + .map_err(SigningStageErrorKind::SigningFailure)?; config.insert(signature); Ok(req) }) @@ -169,7 +178,9 @@ impl MapRequest for SigV4SigningStage { #[cfg(test)] mod test { - use crate::middleware::{SigV4SigningStage, Signature, SigningStageError}; + use crate::middleware::{ + SigV4SigningStage, Signature, SigningStageError, SigningStageErrorKind, + }; use crate::signer::{OperationSigningConfig, SigV4Signer}; use aws_endpoint::partition::endpoint::{Protocol, SignatureVersion}; use aws_endpoint::{AwsEndpointStage, Params}; @@ -254,7 +265,10 @@ mod test { // make sure we got the correct error types in any order assert!(errs.iter().all(|el| matches!( el, - SigningStageError::MissingCredentials | SigningStageError::MissingSigningConfig + SigningStageError { + kind: SigningStageErrorKind::MissingCredentials + | SigningStageErrorKind::MissingSigningConfig + } ))); let (req, _) = req.into_parts(); From a08d4233d88acc471bf35e417f2cf6038250cfa6 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 28 Oct 2022 17:50:45 -0700 Subject: [PATCH 5/6] Revamp errors in `aws-inlineable` --- .../aws-inlineable/src/presigning.rs | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/aws/rust-runtime/aws-inlineable/src/presigning.rs b/aws/rust-runtime/aws-inlineable/src/presigning.rs index 9a259d700c..5a97a19902 100644 --- a/aws/rust-runtime/aws-inlineable/src/presigning.rs +++ b/aws/rust-runtime/aws-inlineable/src/presigning.rs @@ -50,10 +50,8 @@ pub mod config { } } - /// `PresigningConfig` build errors. - #[non_exhaustive] #[derive(Debug)] - pub enum Error { + enum ErrorKind { /// Presigned requests cannot be valid for longer than one week. ExpiresInDurationTooLong, @@ -61,19 +59,31 @@ pub mod config { ExpiresInRequired, } + /// `PresigningConfig` build errors. + #[derive(Debug)] + pub struct Error { + kind: ErrorKind, + } + impl std::error::Error for Error {} impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Error::ExpiresInDurationTooLong => { + match self.kind { + ErrorKind::ExpiresInDurationTooLong => { write!(f, "`expires_in` must be no longer than one week") } - Error::ExpiresInRequired => write!(f, "`expires_in` is required"), + ErrorKind::ExpiresInRequired => write!(f, "`expires_in` is required"), } } } + impl From for Error { + fn from(kind: ErrorKind) -> Self { + Self { kind } + } + } + /// Builder used to create `PresigningConfig`. #[non_exhaustive] #[derive(Default, Debug)] @@ -134,9 +144,9 @@ pub mod config { /// Builds the `PresigningConfig`. This will error if `expires_in` is not /// given, or if it's longer than one week. pub fn build(self) -> Result { - let expires_in = self.expires_in.ok_or(Error::ExpiresInRequired)?; + let expires_in = self.expires_in.ok_or(ErrorKind::ExpiresInRequired)?; if expires_in > ONE_WEEK { - return Err(Error::ExpiresInDurationTooLong); + return Err(ErrorKind::ExpiresInDurationTooLong.into()); } Ok(PresigningConfig { start_time: self.start_time.unwrap_or_else(SystemTime::now), From 183794c4bfe92d57a75761e1d988a3dbe35d4515 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Tue, 15 Nov 2022 12:39:47 -0800 Subject: [PATCH 6/6] Incorporate feedback --- .../aws-config/src/imds/credentials.rs | 29 +++++++++++++++---- .../aws-sig-auth/src/middleware.rs | 8 +++-- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/aws/rust-runtime/aws-config/src/imds/credentials.rs b/aws/rust-runtime/aws-config/src/imds/credentials.rs index b9a1e053a1..3100fca568 100644 --- a/aws/rust-runtime/aws-config/src/imds/credentials.rs +++ b/aws/rust-runtime/aws-config/src/imds/credentials.rs @@ -13,11 +13,29 @@ 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}; use std::borrow::Cow; +use std::error::Error as StdError; +use std::fmt; + +#[derive(Debug)] +struct ImdsCommunicationError { + source: Box, +} + +impl fmt::Display for ImdsCommunicationError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "could not communicate with IMDS") + } +} + +impl StdError for ImdsCommunicationError { + fn source(&self) -> Option<&(dyn StdError + 'static)> { + Some(self.source.as_ref()) + } +} /// IMDSv2 Credentials Provider /// @@ -138,11 +156,10 @@ impl ImdsCredentialsProvider { ); Err(CredentialsError::not_loaded("received 404 from IMDS")) } - Err(ImdsError::FailedToLoadToken(ref err @ SdkError::DispatchFailure(_))) => { - Err(CredentialsError::not_loaded(format!( - "could not communicate with IMDS: {}", - DisplayErrorContext(&err) - ))) + Err(ImdsError::FailedToLoadToken(err @ SdkError::DispatchFailure(_))) => { + Err(CredentialsError::not_loaded(ImdsCommunicationError { + source: err.into(), + })) } Err(other) => Err(CredentialsError::provider_error(other)), } diff --git a/aws/rust-runtime/aws-sig-auth/src/middleware.rs b/aws/rust-runtime/aws-sig-auth/src/middleware.rs index 1c8770df85..135b96df20 100644 --- a/aws/rust-runtime/aws-sig-auth/src/middleware.rs +++ b/aws/rust-runtime/aws-sig-auth/src/middleware.rs @@ -97,9 +97,13 @@ impl Display for SigningStageError { impl Error for SigningStageError { fn source(&self) -> Option<&(dyn Error + 'static)> { + use SigningStageErrorKind as ErrorKind; match &self.kind { - SigningStageErrorKind::SigningFailure(err) => Some(err), - _ => None, + ErrorKind::SigningFailure(err) => Some(err), + ErrorKind::MissingCredentials + | ErrorKind::MissingSigningRegion + | ErrorKind::MissingSigningService + | ErrorKind::MissingSigningConfig => None, } } }