Skip to content

Commit

Permalink
Make connection recording tests less senstive to semver hazards (#3786)
Browse files Browse the repository at this point in the history
## Motivation and Context
A preparatory PR that relaxes test verification of the connection
recording tests.

## Description
This PR is a preparatory step for upcoming changes to the `UserAgent`,
which will introduce new header values in `x-amz-user-agent`, such as
`ua/2.0` (user agent metadata) and `m/A` (business metrics).

However, the introduction of new header values will cause the following
pain points:
- we have to update many connection recording tests to make them pass
again (i.e. the very places updated in this PR)
- check for semver hazards [fail to
pass](https://github.com/smithy-lang/smithy-rs/actions/runs/10209305234/job/28247956895).
This is much the same as we encountered in [content length enforcement
tests](#3523). This
creates a chicken-and-egg problem: tests need to be updated for the PRs
to pass CI, but the "released SDKs" in the `aws-sdk-rust` repository
won't implement the new `UserAgent` header values until the PRs are
merged and released.

To prevent recurring issues with headers affecting connection recording
tests (hence semver checks), this PR preemptively updates the connection
recording tests. Specifically, it adjusts them to ignore certain
headers, ensuring that updates to the `x-amz-user-agent` header do not
trigger semver hazards in subsequent PRs.

**Questions**:
- This PR modifies the connection recording tests to skip verification
of the `x-amz-user-agent` and `authorization` headers. Consequently, we
no longer test the SigV4 signature match in `aws/sdk/integration-tests`.
Although we continue to run canary tests in CI, it would be beneficial
to maintain at least one integration test for verifying the correctness
of the SigV4 signature. This helps in detecting potential bugs affecting
SigV4 signature correctness early on.
To address this, I’ve added [an
awsSdkIntegrationTest](https://github.com/smithy-lang/smithy-rs/blob/f513b924dc0e624d9810889b7177cb4eda6709d2/aws/sdk-codegen/src/test/kotlin/software/amazon/smithy/rustsdk/SigV4AuthDecoratorTest.kt#L72)
that excludes the `UserAgentInterceptor` and checks the `Signature`
value in the `authorization` header. The question is, do we want to keep
this test? If future header updates cause semver hazards to fail, this
test would also be affected. We would then need to repeat the process we
are going through with this PR: update the test, release the change to
aws-sdk-rust, and only then can we make subsequent changes without
breaing semver hazards.

- I've removed the commented-out tests and their associated connection
recording files from `request_information_headers.rs` as part of
cleanup, since there were no explanatory comments. Let me know if we
want to restore these tests, and I will do so along with a comment
explaining their purpose.

## Testing
- Existing tests in CI

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
  • Loading branch information
ysaito1001 authored Aug 14, 2024
1 parent 8733038 commit 8a78e6e
Show file tree
Hide file tree
Showing 18 changed files with 129 additions and 88 deletions.
4 changes: 2 additions & 2 deletions aws/rust-runtime/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,5 @@ async fn sso_bearer_auth() {
let item = &response.items.unwrap()[0];
assert_eq!("somespacename", item.name);

replay.full_validate("application/json").await.unwrap();
replay.relaxed_validate("application/json").await.unwrap();
}
5 changes: 2 additions & 3 deletions aws/sdk/integration-tests/kms/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use aws_sdk_kms::operation::RequestId;
use aws_smithy_runtime::client::http::test_util::{ReplayEvent, StaticReplayClient};
use aws_smithy_runtime_api::client::result::SdkError;
use aws_smithy_types::body::SdkBody;
use http::header::AUTHORIZATION;
use http::Uri;
use kms::config::{Config, Credentials, Region};

Expand Down Expand Up @@ -90,7 +89,7 @@ async fn generate_random() {
.sum::<u32>(),
8562
);
http_client.assert_requests_match(&[]);
http_client.relaxed_requests_match();
}

#[tokio::test]
Expand Down Expand Up @@ -166,5 +165,5 @@ async fn generate_random_keystore_not_found() {
inner.request_id(),
Some("bfe81a0a-9a08-4e71-9910-cdb5ab6ea3b6")
);
http_client.assert_requests_match(&[AUTHORIZATION.as_str()]);
http_client.relaxed_requests_match();
}
2 changes: 1 addition & 1 deletion aws/sdk/integration-tests/qldbsession/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,5 @@ async fn signv4_use_correct_service_name() {
.await
.expect("request should succeed");

http_client.assert_requests_match(&[]);
http_client.assert_requests_match(&["authorization"]);
}
6 changes: 5 additions & 1 deletion aws/sdk/integration-tests/s3/tests/checksums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,11 @@ async fn test_checksum_on_streaming_response(
.await
.unwrap();

http_client.assert_requests_match(&["x-amz-checksum-mode", AUTHORIZATION.as_str()]);
http_client.assert_requests_match(&[
"x-amz-checksum-mode",
"x-amz-user-agent",
AUTHORIZATION.as_str(),
]);

res
}
Expand Down
17 changes: 12 additions & 5 deletions aws/sdk/integration-tests/s3/tests/content-length-enforcement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@ async fn test_content_length_enforcement_is_not_applied_to_head_request() {
.await
.expect("content length enforcement must not apply to HEAD requests");

// The body returned will be empty, so we pass an empty string to full_validate.
// That way, it'll do a string equality check on the empty strings.
http_client.full_validate("").await.unwrap();
// The body returned will be empty, so we pass an empty string for `media_type` to
// `validate_body_and_headers_except`. That way, it'll do a string equality check on the empty
// strings.
http_client.relaxed_validate("").await.unwrap();
}

#[tokio::test]
Expand Down Expand Up @@ -57,7 +58,10 @@ async fn test_content_length_enforcement_get_request_short() {
// This will fail with a content-length mismatch error.
let content_length_err = output.body.collect().await.unwrap_err();

http_client.full_validate("application/text").await.unwrap();
http_client
.relaxed_validate("application/text")
.await
.unwrap();
assert_eq!(
DisplayErrorContext(content_length_err).to_string(),
"streaming error: Invalid Content-Length: Expected 9999 bytes but 10000 bytes were received (Error { kind: StreamingError(ContentLengthError { expected: 9999, received: 10000 }) })"
Expand Down Expand Up @@ -91,7 +95,10 @@ async fn test_content_length_enforcement_get_request_long() {
// This will fail with a content-length mismatch error.
let content_length_err = output.body.collect().await.unwrap_err();

http_client.full_validate("application/text").await.unwrap();
http_client
.relaxed_validate("application/text")
.await
.unwrap();
assert_eq!(
DisplayErrorContext(content_length_err).to_string(),
"streaming error: Invalid Content-Length: Expected 10001 bytes but 10000 bytes were received (Error { kind: StreamingError(ContentLengthError { expected: 10001, received: 10000 }) })"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use aws_sdk_s3::Config;
use aws_sdk_s3::{config::Credentials, config::Region, types::ObjectAttributes, Client};
use aws_smithy_runtime::client::http::test_util::{ReplayEvent, StaticReplayClient};
use aws_smithy_types::body::SdkBody;
use http::header::AUTHORIZATION;

const RESPONSE_BODY_XML: &[u8] = b"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<GetObjectAttributesResponse xmlns=\"http://s3.amazonaws.com/doc/2006-03-01/\"><Checksum><ChecksumSHA1>e1AsOh9IyGCa4hLN+2Od7jlnP14=</ChecksumSHA1></Checksum></GetObjectAttributesResponse>";

Expand Down Expand Up @@ -60,5 +59,5 @@ async fn ignore_invalid_xml_body_root() {
.await
.unwrap();

http_client.assert_requests_match(&[AUTHORIZATION.as_str()]);
http_client.relaxed_requests_match();
}
20 changes: 5 additions & 15 deletions aws/sdk/integration-tests/s3/tests/naughty-string-metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,20 +79,10 @@ async fn test_s3_signer_with_naughty_string_metadata() {

let _ = builder.send().await.unwrap();

// As long as a request can be extracted and the `Authorization` header exits, we're good.
// We cannot compare a signature in the `Authorization` header between expected and actual
// because the signature is subject to change as we update the `x-amz-user-agent` header, e.g.
// due to the introduction of a new metric.
let expected_req = rcvr.expect_request();
let auth_header = expected_req
.headers()
.get("Authorization")
.unwrap()
.to_owned();

// This is a snapshot test taken from a known working test result
let snapshot_signature =
"Signature=a5115604df66219874a9e5a8eab4c9f7a28c992ab2d918037a285756c019f3b2";
assert!(
auth_header .contains(snapshot_signature),
"authorization header signature did not match expected signature: got {}, expected it to contain {}",
auth_header,
snapshot_signature
);
let _ = expected_req.headers().get("Authorization").unwrap();
}
8 changes: 4 additions & 4 deletions aws/sdk/integration-tests/s3/tests/no_auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ async fn list_objects() {
dbg!(result).expect("success");

http_client
.validate_body_and_headers(None, "application/xml")
.relaxed_validate("application/xml")
.await
.unwrap();
}
Expand Down Expand Up @@ -65,7 +65,7 @@ async fn list_objects_v2() {
dbg!(result).expect("success");

http_client
.validate_body_and_headers(None, "application/xml")
.relaxed_validate("application/xml")
.await
.unwrap();
}
Expand Down Expand Up @@ -96,7 +96,7 @@ async fn head_object() {
dbg!(result).expect("success");

http_client
.validate_body_and_headers(None, "application/xml")
.relaxed_validate("application/xml")
.await
.unwrap();
}
Expand Down Expand Up @@ -127,7 +127,7 @@ async fn get_object() {
dbg!(result).expect("success");

http_client
.validate_body_and_headers(None, "application/xml")
.relaxed_validate("application/xml")
.await
.unwrap();
}
13 changes: 1 addition & 12 deletions aws/sdk/integration-tests/s3/tests/normalize-uri-path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,18 +34,7 @@ async fn test_operation_should_not_normalize_uri_path() {
.unwrap();

let request = rx.expect_request();
let actual_auth =
std::str::from_utf8(request.headers().get("authorization").unwrap().as_bytes()).unwrap();

let actual_uri = request.uri();
let expected_uri = "https://test-bucket-ad7c9f01-7f7b-4669-b550-75cc6d4df0f1.s3.us-east-1.amazonaws.com/a/.././b.txt?x-id=PutObject";
assert_eq!(actual_uri, expected_uri);

let expected_sig = "Signature=2ac540538c84dc2616d92fb51d4fc6146ccd9ccc1ee85f518a1a686c5ef97b86";
assert!(
actual_auth.contains(expected_sig),
"authorization header signature did not match expected signature: expected {} but not found in {}",
expected_sig,
actual_auth,
);
assert_eq!(expected_uri, actual_uri);
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,23 +36,12 @@ async fn test_s3_signer_query_string_with_all_valid_chars() {
.send()
.await;

// As long as a request can be extracted and the `Authorization` header exits, we're good.
// We cannot compare a signature in the `Authorization` header between expected and actual
// because the signature is subject to change as we update the `x-amz-user-agent` header, e.g.
// due to the introduction of a new metric.
let expected_req = rcvr.expect_request();
let auth_header = expected_req
.headers()
.get("Authorization")
.unwrap()
.to_owned();

// This is a snapshot test taken from a known working test result
let snapshot_signature =
"Signature=9a931d20606f93fa4e5553602866a9b5ccac2cd42b54ae5a4b17e4614fb443ce";
assert!(
auth_header
.contains(snapshot_signature),
"authorization header signature did not match expected signature: got {}, expected it to contain {}",
auth_header,
snapshot_signature
);
let _ = expected_req.headers().get("Authorization").unwrap();
}

// This test can help identify individual characters that break the signing of query strings. This
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,13 @@ async fn three_retries_and_then_success() {
let resp = resp.expect("valid e2e test");
assert_eq!(resp.name(), Some("test-bucket"));
http_client
.full_validate("application/xml")
.relaxed_validate("application/xml")
.await
.expect("failed")
.unwrap();
}
//

// TODO(simulate time): Currently commented out since the test is work in progress.
// Consider using `tick_advance_time_and_sleep` to simulate client and server times.
// // # Client makes 3 separate SDK operation invocations
// // # All succeed on first attempt.
// // # Fast network, latency + server time is less than one second.
Expand Down Expand Up @@ -190,7 +192,9 @@ async fn three_retries_and_then_success() {
// assert_eq!(resp.name(), Some("test-bucket"));
// conn.full_validate(MediaType::Xml).await.expect("failed")
// }
//

// TODO(simulate time): Currently commented out since the test is work in progress.
// Consider using `tick_advance_time_and_sleep` to simulate client and server times.
// // # One SDK operation invocation.
// // # Client retries 3 times, successful response on 3rd attempt.
// // # Slow network, one way latency is 2 seconds.
Expand Down
3 changes: 2 additions & 1 deletion aws/sdk/integration-tests/s3/tests/signing-it.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use aws_sdk_s3::config::{Credentials, Region};
use aws_sdk_s3::{Client, Config};
use aws_smithy_runtime::client::http::test_util::{ReplayEvent, StaticReplayClient};
use aws_smithy_types::body::SdkBody;
use http::header::AUTHORIZATION;

#[tokio::test]
async fn test_signer() {
Expand Down Expand Up @@ -37,5 +38,5 @@ async fn test_signer() {
.send()
.await;

http_client.assert_requests_match(&[]);
http_client.assert_requests_match(&[AUTHORIZATION.as_str()]);
}
3 changes: 2 additions & 1 deletion aws/sdk/integration-tests/s3control/tests/signing-it.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use aws_sdk_s3control::config::{Credentials, Region};
use aws_sdk_s3control::{Client, Config};
use aws_smithy_runtime::client::http::test_util::{ReplayEvent, StaticReplayClient};
use aws_smithy_types::body::SdkBody;
use http::header::AUTHORIZATION;

#[tokio::test]
async fn test_signer() {
Expand Down Expand Up @@ -39,5 +40,5 @@ async fn test_signer() {
.await
.expect_err("empty response");

http_client.assert_requests_match(&[]);
http_client.assert_requests_match(&[AUTHORIZATION.as_str()]);
}
Loading

0 comments on commit 8a78e6e

Please sign in to comment.