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

Improve error messaging for auth scheme selection #3277

Merged
merged 6 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
17 changes: 17 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,23 @@ references = ["aws-sdk-rust#975", "smithy-rs#3269"]
meta = { "breaking" = false, "tada" = true, "bug" = false }
author = "jdisanti"

[[aws-sdk-rust]]
message = """Add `test_credentials` to `ConfigLoader` in `aws_config`. This allows the following pattern during tests:

```rust
async fn main() {
let conf = aws_config::defaults(BehaviorVersion::latest())
.test_credentials()
.await;
}
```

This is designed for unit tests and using local mocks like DynamoDB Local and LocalStack with the SDK.
"""
meta = { "breaking" = false, "tada" = true, "bug" = false }
author = "rcoh"
references = ["smithy-rs#3279", "aws-sdk-rust#971"]

jdisanti marked this conversation as resolved.
Show resolved Hide resolved
[[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"]
Expand Down
10 changes: 5 additions & 5 deletions aws/sdk/integration-tests/dynamodb/tests/auth_scheme_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
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]
Expand All @@ -24,9 +25,8 @@ async fn auth_scheme_error() {
.send()
.await
.expect_err("there is no credential provider, so this must fail");
let err = DisplayErrorContext(&err).to_string();
let expected = "\"sigv4\" wasn't an option because there was no identity resolver for it. Be sure to set an identity";
if !err.contains(expected) {
panic!("expected '{}'\nto contain '{}'", err, expected);
}
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"
);
}
65 changes: 38 additions & 27 deletions rust-runtime/aws-smithy-runtime/src/client/orchestrator/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,19 @@ impl fmt::Display for NoMatchingAuthSchemeError {
// 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 is a bug. Please file an issue.");
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 is a bug. Please file an issue.");
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;
Expand All @@ -47,24 +51,28 @@ impl fmt::Display for NoMatchingAuthSchemeError {
for item in explored.items() {
write!(
f,
" \"{}\" wasn't an option because ",
" \"{}\" 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;
"it has auth config in its endpoint config, but this scheme wasn't listed in that endpoint config."
},
ExploreResult::NotExplored => unreachable!(),
})?;
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.")?;
Expand Down Expand Up @@ -609,7 +617,8 @@ mod tests {
fn friendly_error_messages() {
let err = NoMatchingAuthSchemeError(ExploredList::default());
assert_eq!(
"no auth options are available. This is a bug. Please file an issue.",
"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()
);

Expand All @@ -625,8 +634,8 @@ mod tests {
let err = NoMatchingAuthSchemeError(list);
assert_eq!(
"failed to select an auth scheme to sign the request with. \
\"SigV4\" wasn't an option because there was no identity resolver for it. \
\"SigV4a\" wasn't an option because there was no identity resolver for it. \
\"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()
Expand All @@ -645,9 +654,10 @@ mod tests {
let err = NoMatchingAuthSchemeError(list);
assert_eq!(
"failed to select an auth scheme to sign the request with. \
\"SigV4\" wasn't an option because there was no identity resolver for it. \
\"SigV4a\" wasn't an option because it has auth config in its endpoint config, \
but this scheme wasn't listed in that endpoint config. \
\"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()
Expand All @@ -662,8 +672,9 @@ mod tests {
let err = NoMatchingAuthSchemeError(list);
assert_eq!(
"failed to select an auth scheme to sign the request with. \
\"SigV4a\" wasn't an option because it has auth config in its endpoint config, \
but this scheme wasn't listed in that endpoint config. \
\"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()
);
Expand Down
2 changes: 2 additions & 0 deletions rust-runtime/aws-smithy-runtime/src/test_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,5 @@

/// Utility for capturing and displaying logs during a unit test.
pub mod capture_test_logs;

mod assertions;
57 changes: 57 additions & 0 deletions rust-runtime/aws-smithy-runtime/src/test_util/assertions.rs
Original file line number Diff line number Diff line change
@@ -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"))
);
}
}