Feature: Flexible checksums #1482

Velfi · 2022-06-21T20:03:12Z

EDIT (2022/06/27)

At @rcoh 's suggestion, I've broken this PR up into smaller PRs to make review easier:

I'll open more PRs for remaining functionality once those get merged.

Motivation and Context

Supports the flexible checksums feature. Without this feature, we can't update our S3 models.

Description

See the flexible checksums RFC for details on what this PR accomplishes.

Testing

I wrote several tests to test the new functionality on multiple different levels

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

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

add: base64::encoded_length add: ChecksumBody add: ChecksumValidatedBody update: signer to support streaming payloads with trailers add: checksum inlineables add: AwsChunkedEncodingBody + options struct update: make append_merge_header_maps pub

add: test for response checksum validation add: ByteStream map method add: get_parts_mut method to operation::Output update: support formatting the string type in RustCodegenDecorator.kt update: S3 models

rcoh

I wasn't able to review this all in one sitting, but will revisit tomorrow. Incredible work on this!

aws/rust-runtime/aws-http/Cargo.toml

rcoh · 2022-06-21T20:34:58Z

aws/rust-runtime/aws-http/src/content_encoding.rs

+                        .chunk_length
+                        .or(this.options.stream_length)
+                        .unwrap_or_default();
+                    Poll::Ready(Some(Ok(prefix_with_chunk_size(data, total_chunk_size))))


this adds a significant amount of copying—how hard would it be to refactor it to send the chunk size and the chunk separately?

I'm also a little confused about how this works—we never switch back into WritingChunkSize AFAICT

aws/rust-runtime/aws-http/src/content_encoding.rs

aws/rust-runtime/aws-inlineable/src/body_with_checksum.rs

update: ChecksumValidatedBody::poll_inner to log at the appropriate level rename: checksum_header_name_to_checksum_algorithm to header_name_to_algorithm rename: checksum_algorithm_to_checksum_header_name to algorithm_to_header_name fix: use new ClientCodegenContext instead of CodegenContext

…ksums

fix: prefer static over const declaration for `HeaderName`s fix: add missing param to TestProtocolTraitImplGenerator.generateTraitImpls

add: tests for wrapped body retryability refactor: swap pin-project for pin-project-lite refactor: swap tempfile for temp-file in integration tests

…ksums

github-actions · 2022-06-23T19:10:01Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

github-actions · 2022-06-23T20:05:08Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

aws/rust-runtime/aws-http/src/content_encoding.rs

rcoh · 2022-06-27T14:07:27Z

aws/rust-runtime/aws-http/src/content_encoding.rs

+                    *this.state = AwsChunkedBodyState::WritingChunk;
+                    let total_chunk_size = this
+                        .options
+                        .chunk_length
+                        .or(this.options.stream_length)
+                        .unwrap_or_default();
+                    Poll::Ready(Some(Ok(prefix_with_chunk_size(data, total_chunk_size))))


this code seems to straddle two lines between supporting multiple chunks in some ways but not in others–it makes it pretty confusing to follow. I think we should maybe remove chunk_size as a parameter entirely?

rcoh · 2022-06-27T14:12:14Z

aws/sdk/integration-tests/s3/tests/checksums.rs

+/// Test connection for the movies IT
+/// headers are signed with actual creds, at some point we could replace them with verifiable test
+/// credentials, but there are plenty of other tests that target signing


out of date comment

rcoh · 2022-06-27T14:12:26Z

aws/sdk/integration-tests/s3/tests/checksums.rs

+// static INIT_LOGGER: std::sync::Once = std::sync::Once::new();
+// fn init_logger() {
+//     INIT_LOGGER.call_once(|| {
+//         tracing_subscriber::fmt::init();
+//     });
+// }


is this useful?

rcoh · 2022-06-27T14:14:02Z

aws/sdk/integration-tests/s3/tests/checksums.rs

+async fn collect_body_into_string(mut body: aws_smithy_http::body::SdkBody) -> String {
+    use bytes::Buf;
+    use bytes_utils::SegmentedBuf;
+    use http_body::Body;
+    use std::io::Read;
+
+    let mut output = SegmentedBuf::new();
+    while let Some(buf) = body.data().await {
+        output.push(buf.unwrap());
+    }
+
+    let mut output_text = String::new();
+    output
+        .reader()
+        .read_to_string(&mut output_text)
+        .expect("Doesn't cause IO errors");
+
+    output_text
+}


you can use str::from_utf8(&body.collect().into_bytes())

rcoh · 2022-06-27T14:17:54Z

rust-runtime/aws-smithy-checksums/src/body.rs

+    fn poll_trailers(
+        self: Pin<&mut Self>,
+        cx: &mut Context<'_>,
+    ) -> Poll<Result<Option<HeaderMap<HeaderValue>>, Self::Error>> {
+        let this = self.project();
+        match (
+            this.checksum.headers(),
+            http_body::Body::poll_trailers(this.inner, cx),
+        ) {
+            // If everything is ready, return trailers, merging them if we have more than one map
+            (Ok(outer_trailers), Poll::Ready(Ok(inner_trailers))) => {
+                let trailers = match (outer_trailers, inner_trailers) {
+                    // Values from the inner trailer map take precedent over values from the outer map
+                    (Some(outer), Some(inner)) => Some(append_merge_header_maps(inner, outer)),
+                    // If only one or neither produced trailers, just combine the `Option`s with `or`
+                    (outer, inner) => outer.or(inner),
+                };
+                Poll::Ready(Ok(trailers))
+            }
+            // If the inner poll is Ok but the outer body's checksum callback encountered an error,
+            // return the error
+            (Err(e), Poll::Ready(Ok(_))) => Poll::Ready(Err(e)),
+            // Otherwise return the result of the inner poll.
+            // It may be pending or it may be ready with an error.
+            (_, inner_poll) => inner_poll,
+        }
+    }


this is really cool! (how checksum body uses structured trailers then AWS chunked writes the trailers into the body)

rcoh · 2022-06-27T14:19:16Z

rust-runtime/aws-smithy-checksums/src/body.rs

+    pub fn checksum_mismatch(expected: Bytes, actual: Bytes) -> Self {
+        Self::ChecksumMismatch { expected, actual }
+    }


given the Display impl, I wonder if we should have a max length on these fields

rcoh · 2022-06-27T14:20:52Z

rust-runtime/aws-smithy-checksums/src/lib.rs

+pub trait Checksum: Send + Sync {
+    /// Given a slice of bytes, update this checksum's internal state.
+    fn update(&mut self, bytes: &[u8]) -> Result<(), BoxError>;
+    /// Either return this checksum as a `HeaderMap` containing one HTTP header, or return an error
+    /// describing why checksum calculation failed.
+    fn headers(&self) -> Result<Option<HeaderMap<HeaderValue>>, BoxError>;
+    /// Return the `HeaderName` used to represent this checksum algorithm
+    fn header_name(&self) -> HeaderName;
+    /// "Finalize" this checksum, returning the calculated value as `Bytes` or an error that
+    /// occurred during checksum calculation. To print this value in a human-readable hexadecimal
+    /// format, you can print it using Rust's builtin [formatter].
+    ///
+    /// _**NOTE:** typically, "finalizing" a checksum in Rust will take ownership of the checksum
+    /// struct. In this method, we clone the checksum's state before finalizing because checksums
+    /// may be used in a situation where taking ownership is not possible._
+    ///
+    /// [formatter]: https://doc.rust-lang.org/std/fmt/trait.UpperHex.html
+    fn finalize(&self) -> Result<Bytes, BoxError>;
+    /// Return the size of this checksum algorithms resulting checksum, in bytes. For example, the
+    /// CRC32 checksum algorithm calculates a 32 bit checksum, so a CRC32 checksum struct
+    /// implementing this trait method would return 4.
+    fn size(&self) -> u64;
+}


nice, I like this design! Have we already gone back and forth on this? I might consider splitting out HttpChecksum as `CheckSum + headers/header_name

rcoh · 2022-06-27T14:21:27Z

rust-runtime/aws-smithy-http/src/body.rs

@@ -6,7 +6,7 @@
 use bytes::Bytes;
 use http::{HeaderMap, HeaderValue};
 use http_body::{Body, SizeHint};
-use pin_project::pin_project;
+use pin_project_lite::pin_project;


thank you for doing this!!

rcoh · 2022-06-27T14:23:51Z

rust-runtime/aws-smithy-http/src/byte_stream.rs

+pin_project! {
+    #[doc = "Stream of binary data"]
+    #[doc = ""]
+    #[doc = "`ByteStream` wraps a stream of binary data for ease of use."]
+    #[doc = ""]
+    #[doc = "## Getting data out of a `ByteStream`"]
+    #[doc = ""]
+    #[doc = "`ByteStream` provides two primary mechanisms for accessing the data:"]
+    #[doc = "1. With `.collect()`:"]
+    #[doc = ""]
+    #[doc = "    [`.collect()`](crate::byte_stream::ByteStream::collect) reads the complete ByteStream into memory and stores it in `AggregatedBytes`,"]
+    #[doc = "    a non-contiguous ByteBuffer."]
+    #[doc = "    ```no_run"]
+    #[doc = "    use aws_smithy_http::byte_stream::{ByteStream, AggregatedBytes};"]
+    #[doc = "    use aws_smithy_http::body::SdkBody;"]
+    #[doc = "    use bytes::Buf;"]
+    #[doc = "    async fn example() {"]
+    #[doc = "       let stream = ByteStream::new(SdkBody::from(\"hello! This is some data\"));"]
+    #[doc = "       // Load data from the stream into memory:"]


hmm, are you sure this is required here? There are other places where we use pin_project_lite and doc comments:

https://github.com/awslabs/smithy-rs/blob/a0539e20b069a7de021c84521d8f3c7ba098ad6d/rust-runtime/aws-smithy-async/src/future/fn_stream.rs#L16

Velfi · 2022-06-27T17:33:55Z

I converted this into a draft and will split it into several smaller PRs.

Velfi · 2022-07-21T14:08:29Z

Closing this in favor of #1561

Velfi added 2 commits June 15, 2022 14:39

add: HttpRequestChecksumDecorator

70f4a45

add: base64::encoded_length add: ChecksumBody add: ChecksumValidatedBody update: signer to support streaming payloads with trailers add: checksum inlineables add: AwsChunkedEncodingBody + options struct update: make append_merge_header_maps pub

feature: add checksum validation for responses

73099b0

add: test for response checksum validation add: ByteStream map method add: get_parts_mut method to operation::Output update: support formatting the string type in RustCodegenDecorator.kt update: S3 models

Velfi requested a review from a team as a code owner June 21, 2022 20:03

Merge branch 'main' into feature/flexible-checksums

3f3d4fa

rcoh reviewed Jun 21, 2022

View reviewed changes

Velfi added 6 commits June 22, 2022 13:36

Merge remote-tracking branch 'origin/main' into feature/flexible-chec…

f840eae

…ksums

remove: unused import

90be9e8

fix: prefer static over const declaration for `HeaderName`s fix: add missing param to TestProtocolTraitImplGenerator.generateTraitImpls

fix: wrapped body retryability

189a705

add: tests for wrapped body retryability refactor: swap pin-project for pin-project-lite refactor: swap tempfile for temp-file in integration tests

Merge remote-tracking branch 'origin/main' into feature/flexible-chec…

8c48a9d

…ksums

add: missing param to generateTraitImpls override

950f15f

Velfi requested a review from a team as a code owner June 23, 2022 18:48

Velfi added 2 commits June 23, 2022 14:52

remove: TODOs

ec793fa

remove: zeldas

2821782

rcoh requested changes Jun 27, 2022

View reviewed changes

rcoh reviewed Jun 27, 2022

View reviewed changes

Velfi marked this pull request as draft June 27, 2022 17:33

Velfi mentioned this pull request Jul 20, 2022

feature: support flexible checksums #1561

Merged

2 tasks

Velfi closed this Jul 21, 2022

Velfi deleted the feature/flexible-checksums branch July 21, 2022 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Flexible checksums #1482

Feature: Flexible checksums #1482

Velfi commented Jun 21, 2022 •

edited

Loading

rcoh left a comment

rcoh Jun 21, 2022

github-actions bot commented Jun 23, 2022

github-actions bot commented Jun 23, 2022

rcoh Jun 27, 2022

rcoh Jun 27, 2022

rcoh Jun 27, 2022

rcoh Jun 27, 2022

rcoh Jun 27, 2022

rcoh Jun 27, 2022

rcoh Jun 27, 2022

rcoh Jun 27, 2022

rcoh Jun 27, 2022

Velfi commented Jun 27, 2022

Velfi commented Jul 21, 2022

Feature: Flexible checksums #1482

Feature: Flexible checksums #1482

Conversation

Velfi commented Jun 21, 2022 • edited Loading

EDIT (2022/06/27)

Motivation and Context

Description

Testing

Checklist

rcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 23, 2022

github-actions bot commented Jun 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Velfi commented Jun 27, 2022

Velfi commented Jul 21, 2022

Velfi commented Jun 21, 2022 •

edited

Loading