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

Flexible Checksums Updates #3845

Merged
merged 33 commits into from
Oct 2, 2024
Merged

Conversation

landonxjames
Copy link
Contributor

@landonxjames landonxjames commented Sep 22, 2024

Motivation and Context

Updates to flexible checksum functionality to align with the upcoming spec updates.

Note: this PR is to a feature branch since some of the changes here would be breaking before service deployments to align with the updated spec.

Description

  • Add two new SdkConfig fields response_checksum_validation and request_checksum_calculation that indicate the user's preferred checksum behavior
  • Use Crc32 as the default checksum algorithm
  • Update the checksum calculating/validating interceptors to respect the new SdkConfig settings and default checksum
  • Add two new interceptors (one for requests and one for responses) that each mutate the input to set the default checksum values. Note that these cannot be done in the existing inlineable interceptors because they require access to both runtime values and values from the modeled checksum trait

Testing

Added new suite of tests as modeled in the SEP in HttpChecksumTest.kt

There are a few CI tests failing an all of those are currently expected:

  • Check PR semver compliance: I marked an enum in aws-smithy-checksums as non-exhaustive. That crate is not yet 1.0 so this change should be fine (and is necessary since we will likely support different checksum types in the future).
  • Canary: This fails on MPU since the upload_part contains the default crc32 checksum but create_multipart_upload does not specify a checksum. S3 will fix this behavior before we deploy.
  • check-aws-sdk-smoketest-unit-tests: This fails on an S3 protocol test with missing required header: 'content-md5'. This should be fixed by bumping to the newest smithy version (in a separate PR) since MD5 is now deprecated.

Checklist

  • For changes to the smithy-rs codegen or runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "client," "server," or both in the applies_to key.
  • For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "aws-sdk-rust" in the applies_to key.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@landonxjames landonxjames force-pushed the landonxjames/flex branch 8 times, most recently from cf331e4 to 3175d0c Compare September 22, 2024 21:34
Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this, leaving comments accumulated so far. Will circle back.

@@ -39,20 +47,49 @@ internal fun RuntimeConfig.awsInlineableHttpRequestChecksum() =
CargoDependency.smithyHttp(this),
CargoDependency.smithyRuntimeApiClient(this),
CargoDependency.smithyTypes(this),
AwsCargoDependency.awsSigv4(this),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need awsSigv4 crate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The http_request_checksum.rs inlineable uses the sigv4 crate. In previous contexts where it was used that crate was pulled in as a dependency by other decorators. When I started writing the codegen tests for this with a generic model that crate was not being pulled in. Since it is necessary for the inlineable to work I added it as a dependency here.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
@smithy-lang smithy-lang deleted a comment from github-actions bot Sep 25, 2024
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@ysaito1001
Copy link
Contributor

ysaito1001 commented Sep 27, 2024

Add two new interceptors (one for requests and one for responses) that each mutate the input to set the default checksum values. Note that these cannot be done in the existing inlineable interceptors because they require access to both runtime values and values from the modeled checksum trait

I'm thinking whether we can somehow avoid adding new interceptors, since we'd want all related logic to be bundled in one place. What if we augment existing interceptors in aws-inlineable/src/http_request_checksum.rs by introducing another generic parameter for mutating checksum, i.e.,

pub(crate) struct RequestChecksumInterceptor<AP, CM> {
    algorithm_provider: AP,
    checksum_mutator: CM, // this is new, similar to the above `AP`
}

In the impl block, we call the CM closure:

impl<AP, CM> Intercept for RequestChecksumInterceptor<AP, CM>
where
    AP: Fn(&Input) -> Result<Option<ChecksumAlgorithm>, BoxError> + Send + Sync,
    CM: Fn(&mut Input, &ConfigBag) -> Result<(), BoxError> + Send + Sync, // this is new
{
...
    fn modify_before_serialization(
        &self,
        context: &mut BeforeSerializationInterceptorContextMut<'_>,
        _runtime_components: &RuntimeComponents,
        cfg: &mut ConfigBag,
    ) -> Result<(), BoxError> {
        (self.checksum_mutator)(context.input_mut(), cfg)
    }
...
}

To wire things up, where we instantiate this struct in codegen, give this code to the closure for CM (we can borrow the idea from how to pass the argument for the closure AP). At that place, we should have access to things like checksumTrait.isRequestChecksumRequired.

The same argument goes for aws-inlineable/src/http_response_checksum.rs.

I haven't verified if this idea compiles or works. But the net benefit is that we don't have to render new interceptor structs per operation.

FYI, I'm ok with merging what we have and iterate on this to avoid churn (new interceptors are private so we can change later we want to).

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@landonxjames
Copy link
Contributor Author

Good suggestion @ysaito1001, implemented in d682c8d and 6c9c3da

Copy link

github-actions bot commented Oct 2, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@aajtodd aajtodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. One non-blocking question.

##[allow(clippy::wildcard_in_or_patterns)]
match response_checksum_validation {
#{ResponseChecksumValidation}::WhenRequired => {}
#{ResponseChecksumValidation}::WhenSupported | _ => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why | _ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The {Response | Request}ChecksumValidation enums are marked non-exhaustive as a safety measure so have to handle that case somehow. Since these should always be set before this point that case shouldn't actually be encountered, but if it is we treat it the same as the default (WhenSupported).

@landonxjames landonxjames merged commit 76f8a55 into feature/flexible-checksums Oct 2, 2024
42 of 44 checks passed
@landonxjames landonxjames deleted the landonxjames/flex branch October 2, 2024 21:57
@Velfi Velfi changed the title Flexible Checksums Updatess Flexible Checksums Updates Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants