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._

Author: jdisanti

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"

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"
+    );
+}

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");
+                    "<unknown>"
+                }
+            })?;
+        }
+        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<Item = &ExploredAuthOption> {
+        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.");
+        }
+    }
 }

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;

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::<String>()
+            .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"))
+        );
+    }
+}