From 85d2621b7c1b5445044ef190a4a5051e6be8df3d Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Mon, 4 Dec 2023 11:27:39 -0800 Subject: [PATCH] Improve error messaging for auth scheme selection (#3277) This patch adds a stack-allocated list of explored auth schemes and the reason each didn't work to the auth orchestrator so that its error message is much easier to decipher. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --- CHANGELOG.next.toml | 12 + .../dynamodb/tests/auth_scheme_error.rs | 32 +++ .../src/client/orchestrator/auth.rs | 227 +++++++++++++++++- .../aws-smithy-runtime/src/test_util.rs | 2 + .../src/test_util/assertions.rs | 57 +++++ 5 files changed, 323 insertions(+), 7 deletions(-) create mode 100644 aws/sdk/integration-tests/dynamodb/tests/auth_scheme_error.rs create mode 100644 rust-runtime/aws-smithy-runtime/src/test_util/assertions.rs diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 1d3cad3a75..2c2289e1d4 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -49,3 +49,15 @@ This is designed for unit tests and using local mocks like DynamoDB Local and Lo meta = { "breaking" = false, "tada" = true, "bug" = false } author = "rcoh" references = ["smithy-rs#3279", "aws-sdk-rust#971"] + +[[aws-sdk-rust]] +message = "Improve the error messages for when auth fails to select an auth scheme for a request." +references = ["aws-sdk-rust#979", "smithy-rs#3277"] +meta = { "breaking" = false, "tada" = false, "bug" = false } +author = "jdisanti" + +[[smithy-rs]] +message = "Improve the error messages for when auth fails to select an auth scheme for a request." +references = ["smithy-rs#3277"] +meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client" } +author = "jdisanti" diff --git a/aws/sdk/integration-tests/dynamodb/tests/auth_scheme_error.rs b/aws/sdk/integration-tests/dynamodb/tests/auth_scheme_error.rs new file mode 100644 index 0000000000..702a3fd933 --- /dev/null +++ b/aws/sdk/integration-tests/dynamodb/tests/auth_scheme_error.rs @@ -0,0 +1,32 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +use aws_sdk_dynamodb::config::Region; +use aws_sdk_dynamodb::error::DisplayErrorContext; +use aws_sdk_dynamodb::{Client, Config}; +use aws_smithy_runtime::assert_str_contains; +use aws_smithy_runtime::client::http::test_util::capture_request; + +#[tokio::test] +async fn auth_scheme_error() { + let (http_client, _) = capture_request(None); + let config = Config::builder() + .behavior_version_latest() + .http_client(http_client) + .region(Region::new("us-west-2")) + // intentionally omitting credentials_provider + .build(); + let client = Client::from_conf(config); + + let err = client + .list_tables() + .send() + .await + .expect_err("there is no credential provider, so this must fail"); + assert_str_contains!( + DisplayErrorContext(&err).to_string(), + "\"sigv4\" wasn't a valid option because there was no identity resolver for it. Be sure to set an identity" + ); +} diff --git a/rust-runtime/aws-smithy-runtime/src/client/orchestrator/auth.rs b/rust-runtime/aws-smithy-runtime/src/client/orchestrator/auth.rs index 8c6f7000e4..98a39a9a43 100644 --- a/rust-runtime/aws-smithy-runtime/src/client/orchestrator/auth.rs +++ b/rust-runtime/aws-smithy-runtime/src/client/orchestrator/auth.rs @@ -20,18 +20,86 @@ use std::error::Error as StdError; use std::fmt; use tracing::trace; +#[derive(Debug)] +struct NoMatchingAuthSchemeError(ExploredList); + +impl fmt::Display for NoMatchingAuthSchemeError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let explored = &self.0; + + // Use the information we have about the auth options that were explored to construct + // as helpful of an error message as possible. + if explored.items().count() == 0 { + return f.write_str( + "no auth options are available. This can happen if there's \ + a problem with the service model, or if there is a codegen bug.", + ); + } + if explored + .items() + .all(|explored| matches!(explored.result, ExploreResult::NoAuthScheme)) + { + return f.write_str( + "no auth schemes are registered. This can happen if there's \ + a problem with the service model, or if there is a codegen bug.", + ); + } + + let mut try_add_identity = false; + let mut likely_bug = false; + f.write_str("failed to select an auth scheme to sign the request with.")?; + for item in explored.items() { + write!( + f, + " \"{}\" wasn't a valid option because ", + item.scheme_id.as_str() + )?; + f.write_str(match item.result { + ExploreResult::NoAuthScheme => { + likely_bug = true; + "no auth scheme was registered for it." + } + ExploreResult::NoIdentityResolver => { + try_add_identity = true; + "there was no identity resolver for it." + } + ExploreResult::MissingEndpointConfig => { + likely_bug = true; + "there is auth config in the endpoint config, but this scheme wasn't listed in it \ + (see https://github.com/smithy-lang/smithy-rs/discussions/3281 for more details)." + } + ExploreResult::NotExplored => { + debug_assert!(false, "this should be unreachable"); + "" + } + })?; + } + if try_add_identity { + f.write_str(" Be sure to set an identity, such as credentials, auth token, or other identity type that is required for this service.")?; + } else if likely_bug { + f.write_str(" This is likely a bug.")?; + } + if explored.truncated { + f.write_str(" Note: there were other auth schemes that were evaluated that weren't listed here.")?; + } + + Ok(()) + } +} + +impl StdError for NoMatchingAuthSchemeError {} + #[derive(Debug)] enum AuthOrchestrationError { - NoMatchingAuthScheme, + MissingEndpointConfig, BadAuthSchemeEndpointConfig(Cow<'static, str>), } impl fmt::Display for AuthOrchestrationError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::NoMatchingAuthScheme => f.write_str( - "no auth scheme matched auth scheme options. This is a bug. Please file an issue.", - ), + // This error is never bubbled up + Self::MissingEndpointConfig => f.write_str("missing endpoint config"), Self::BadAuthSchemeEndpointConfig(message) => f.write_str(message), } } @@ -59,6 +127,8 @@ pub(super) async fn orchestrate_auth( "orchestrating auth", ); + let mut explored = ExploredList::default(); + // Iterate over IDs of possibly-supported auth schemes for &scheme_id in options.as_ref() { // For each ID, try to resolve the corresponding auth scheme. @@ -95,16 +165,21 @@ pub(super) async fn orchestrate_auth( )?; return Ok(()); } - Err(AuthOrchestrationError::NoMatchingAuthScheme) => { + Err(AuthOrchestrationError::MissingEndpointConfig) => { + explored.push(scheme_id, ExploreResult::MissingEndpointConfig); continue; } Err(other_err) => return Err(other_err.into()), } + } else { + explored.push(scheme_id, ExploreResult::NoIdentityResolver); } + } else { + explored.push(scheme_id, ExploreResult::NoAuthScheme); } } - Err(AuthOrchestrationError::NoMatchingAuthScheme.into()) + Err(NoMatchingAuthSchemeError(explored).into()) } fn extract_endpoint_auth_scheme_config( @@ -135,10 +210,66 @@ fn extract_endpoint_auth_scheme_config( .and_then(Document::as_string); config_scheme_id == Some(scheme_id.as_str()) }) - .ok_or(AuthOrchestrationError::NoMatchingAuthScheme)?; + .ok_or(AuthOrchestrationError::MissingEndpointConfig)?; Ok(AuthSchemeEndpointConfig::from(Some(auth_scheme_config))) } +#[derive(Debug)] +enum ExploreResult { + NotExplored, + NoAuthScheme, + NoIdentityResolver, + MissingEndpointConfig, +} + +/// Information about an evaluated auth option. +/// This should be kept small so it can fit in an array on the stack. +#[derive(Debug)] +struct ExploredAuthOption { + scheme_id: AuthSchemeId, + result: ExploreResult, +} +impl Default for ExploredAuthOption { + fn default() -> Self { + Self { + scheme_id: AuthSchemeId::new(""), + result: ExploreResult::NotExplored, + } + } +} + +const MAX_EXPLORED_LIST_LEN: usize = 8; + +/// Stack allocated list of explored auth options for error messaging +#[derive(Default)] +struct ExploredList { + items: [ExploredAuthOption; MAX_EXPLORED_LIST_LEN], + len: usize, + truncated: bool, +} +impl ExploredList { + fn items(&self) -> impl Iterator { + self.items.iter().take(self.len) + } + + fn push(&mut self, scheme_id: AuthSchemeId, result: ExploreResult) { + if self.len + 1 >= self.items.len() { + self.truncated = true; + } else { + self.items[self.len] = ExploredAuthOption { scheme_id, result }; + self.len += 1; + } + } +} +impl fmt::Debug for ExploredList { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("ExploredList") + .field("items", &&self.items[0..self.len]) + .field("truncated", &self.truncated) + .finish() + } +} + #[cfg(all(test, feature = "test-util"))] mod tests { use super::*; @@ -481,4 +612,86 @@ mod tests { .unwrap() ); } + + #[test] + fn friendly_error_messages() { + let err = NoMatchingAuthSchemeError(ExploredList::default()); + assert_eq!( + "no auth options are available. This can happen if there's a problem with \ + the service model, or if there is a codegen bug.", + err.to_string() + ); + + let mut list = ExploredList::default(); + list.push( + AuthSchemeId::new("SigV4"), + ExploreResult::NoIdentityResolver, + ); + list.push( + AuthSchemeId::new("SigV4a"), + ExploreResult::NoIdentityResolver, + ); + let err = NoMatchingAuthSchemeError(list); + assert_eq!( + "failed to select an auth scheme to sign the request with. \ + \"SigV4\" wasn't a valid option because there was no identity resolver for it. \ + \"SigV4a\" wasn't a valid option because there was no identity resolver for it. \ + Be sure to set an identity, such as credentials, auth token, or other identity \ + type that is required for this service.", + err.to_string() + ); + + // It should prioritize the suggestion to try an identity before saying it's a bug + let mut list = ExploredList::default(); + list.push( + AuthSchemeId::new("SigV4"), + ExploreResult::NoIdentityResolver, + ); + list.push( + AuthSchemeId::new("SigV4a"), + ExploreResult::MissingEndpointConfig, + ); + let err = NoMatchingAuthSchemeError(list); + assert_eq!( + "failed to select an auth scheme to sign the request with. \ + \"SigV4\" wasn't a valid option because there was no identity resolver for it. \ + \"SigV4a\" wasn't a valid option because there is auth config in the endpoint \ + config, but this scheme wasn't listed in it (see \ + https://github.com/smithy-lang/smithy-rs/discussions/3281 for more details). \ + Be sure to set an identity, such as credentials, auth token, or other identity \ + type that is required for this service.", + err.to_string() + ); + + // Otherwise, it should suggest it's a bug + let mut list = ExploredList::default(); + list.push( + AuthSchemeId::new("SigV4a"), + ExploreResult::MissingEndpointConfig, + ); + let err = NoMatchingAuthSchemeError(list); + assert_eq!( + "failed to select an auth scheme to sign the request with. \ + \"SigV4a\" wasn't a valid option because there is auth config in the endpoint \ + config, but this scheme wasn't listed in it (see \ + https://github.com/smithy-lang/smithy-rs/discussions/3281 for more details). \ + This is likely a bug.", + err.to_string() + ); + + // Truncation should be indicated + let mut list = ExploredList::default(); + for _ in 0..=MAX_EXPLORED_LIST_LEN { + list.push( + AuthSchemeId::new("dontcare"), + ExploreResult::MissingEndpointConfig, + ); + } + let err = NoMatchingAuthSchemeError(list).to_string(); + if !err.contains( + "Note: there were other auth schemes that were evaluated that weren't listed here", + ) { + panic!("The error should indicate that the explored list was truncated."); + } + } } diff --git a/rust-runtime/aws-smithy-runtime/src/test_util.rs b/rust-runtime/aws-smithy-runtime/src/test_util.rs index b3ec5e5dfb..5edd1dfcbe 100644 --- a/rust-runtime/aws-smithy-runtime/src/test_util.rs +++ b/rust-runtime/aws-smithy-runtime/src/test_util.rs @@ -5,3 +5,5 @@ /// Utility for capturing and displaying logs during a unit test. pub mod capture_test_logs; + +mod assertions; diff --git a/rust-runtime/aws-smithy-runtime/src/test_util/assertions.rs b/rust-runtime/aws-smithy-runtime/src/test_util/assertions.rs new file mode 100644 index 0000000000..de35c053b4 --- /dev/null +++ b/rust-runtime/aws-smithy-runtime/src/test_util/assertions.rs @@ -0,0 +1,57 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +/// Asserts that a given string value `$str` contains a substring `$expected`. +/// +/// This macro can also take a custom panic message with formatting. +#[macro_export] +macro_rules! assert_str_contains { + ($str:expr, $expected:expr) => { + assert_str_contains!($str, $expected, "") + }; + ($str:expr, $expected:expr, $($fmt_args:tt)+) => {{ + let s = $str; + let expected = $expected; + if !s.contains(&expected) { + panic!( + "assertion failed: `str.contains(expected)`\n{:>8}: {expected}\n{:>8}: {s}\n{}", + "expected", + "str", + ::std::fmt::format(::std::format_args!($($fmt_args)+)), + ); + } + }}; +} + +#[cfg(test)] +mod tests { + use std::panic::{catch_unwind, UnwindSafe}; + + fn expect_panic(f: impl FnOnce() -> () + UnwindSafe) -> String { + *catch_unwind(f) + .expect_err("it should fail") + .downcast::() + .expect("it should be a string") + } + + #[test] + fn assert_str_contains() { + assert_str_contains!("foobar", "bar"); + assert_str_contains!("foobar", "o"); + + assert_eq!( + "assertion failed: `str.contains(expected)`\nexpected: not-in-it\n str: foobar\n", + expect_panic(|| assert_str_contains!("foobar", "not-in-it")) + ); + assert_eq!( + "assertion failed: `str.contains(expected)`\nexpected: not-in-it\n str: foobar\nsome custom message", + expect_panic(|| assert_str_contains!("foobar", "not-in-it", "some custom message")) + ); + assert_eq!( + "assertion failed: `str.contains(expected)`\nexpected: not-in-it\n str: foobar\nsome custom message with formatting", + expect_panic(|| assert_str_contains!("foobar", "not-in-it", "some custom message with {}", "formatting")) + ); + } +}