Feature: add support for aws-chunked content encoding #1501

Velfi · 2022-06-27T17:27:26Z

Motivation and Context

Supports the flexible checksums feature. When sending streaming requests we need to include checksums as trailers and we need aws-chunked content encoding to do that.

Description

This provides functionality specified in the flexible checksums RFC

Testing

I wrote a test

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.

github-actions · 2022-06-27T20:20:50Z

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.

rcoh

looking good so far! the state machine is very clean, just a couple spots that could be cleaned up / clarified

rcoh · 2022-06-27T20:45:14Z

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

+                        let total_length_of_trailers_in_bytes =
+                            this.options.trailer_lens.iter().sum();
+
+                        Poll::Ready(Some(Ok(trailers_as_aws_chunked_bytes(
+                            total_length_of_trailers_in_bytes,
+                            trailers,


is there any check that these trailers actually match this.options.trailer_lens?

I added one

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

rcoh · 2022-06-27T20:53:12Z

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

+    pub fn with_trailer_len(mut self, trailer_len: u64) -> Self {
+        self.trailer_lens.push(trailer_len);
+        self
+    }


I wonder if this API will be awkward to use if you have a vec of trail lengths already...

I updated the API so you can input a vec when creating the Options

rcoh · 2022-06-27T20:53:53Z

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

+#[derive(Debug, Default)]
+#[non_exhaustive]
+pub struct AwsChunkedBodyOptions {
+    /// The total size of the stream. Because we only support unsigned encoding
+    /// this implies that there will only be a single chunk containing the
+    /// underlying payload.
+    pub stream_length: u64,
+    /// The length of each trailer sent within an `AwsChunkedBody`. Necessary in
+    /// order to correctly calculate the total size of the body accurately.
+    pub trailer_lens: Vec<u64>,
+}


don't personally love Default on this because Default on this is a sort of broken empty state which probably isn't right

It's not necessarily broken, it just applies to a stream with no body or trailers. Do you have a better API in mind?

I refactored the API, let me know what you think

rcoh · 2022-06-27T20:54:42Z

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

+        SizeHint::with_exact(
+            self.encoded_length()
+                .expect("Requests made with aws-chunked encoding must have known size")
+                as u64,
+        )
+    }


I wonder if we should also have a double check on this vs. the size_hint on the inner body

We can only check that the encoded body is larger (due to chunk prefix, CRLF, trailers) than the size hint of the inner body but I can't think of how to test that other than by constructing a body that lies about its size.

rcoh · 2022-06-27T20:55:23Z

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

+    pub fn with_stream_length(mut self, stream_length: u64) -> Self {
+        self.stream_length = stream_length;
+        self
+    }


one possible default could be to have stream_length delegate to the inner size_hint() if unset...guess that's kind of tricky with trailers though?

yeah size hint doesn't account for trailers

CHANGELOG.next.toml

update: AwsChunkedBody to note we only support single chunks remove: unnecessary `Option`s remove: overly smart total_length_of_trailers_in_bytes in trailers_as_aws_chunked_bytes update: use "where"-style declaration for `impl<Inner> Body for AwsChunkedBody<Inner>` add: helpful data to trace logging add: trailer len double check in AwsChunkedBody::poll_data add: test for trailer len double check add: assert to size_hint fix: incorrect body emitted when body is empty add: test for empty encoded body

Co-authored-by: John DiSanti <[email protected]>

github-actions · 2022-06-28T21:56:33Z

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.

jdisanti · 2022-06-28T22:10:00Z

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

+                for value in values {
+                    total += value.len() + ",".len();
+                }


Does this match the actual serialization behavior for multiple values? I thought that hyper would add multiple of the same header name with a different value, but I'm not 100% sure.

I'm not sure if it matches what Hyper would do in this situation but it I did test it with S3 and it accepted it. I can test an upload the hyper-style headers if you like.

I switched this to work more similarly to how hyper serializes headers. The serialization is done manually.

jdisanti · 2022-06-28T22:11:27Z

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

@@ -276,6 +282,30 @@ where
    len
 }

+fn total_rendered_length_of_trailers(header_map: Option<&HeaderMap>) -> u64 {


Seems like we should have a unit test that serializes the header map with the http crate and compares its length against this function. Especially with multiple headers of the same name.

I added a test that ensures our trailer serializer and trailer length calculator stay in sync

rcoh

overall this looks great! I'd like to see more exhaustive testing (eg. an inner body that contains multiple chunks eg.)

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

update: when aws-chunked formatting a `HeaderMap`, header names with multiple values will be written out one value per line remove: unnecessary AwsChunkedBodyOptions::stream_length method add: trace fields refactor: make inserting the final body CRLF more explicit add: test to ensure trailer encoding and trailer len calculation stay in sync

…in sync for empty HeaderMap

github-actions · 2022-06-29T20:48:42Z

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

jdisanti · 2022-06-29T22:02:31Z

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

+    /// not equal to the actual length of the trailers returned by the inner `http::Body`
+    /// implementor. These trailer lengths are necessary in order to correctly calculate the total
+    /// size of the body for setting the content length header.
+    ReportedTrailerLengthMismatch { actual: u64, expected: u64 },


I see @rcoh suggested making an error for this, but I think this is a case where we have a bug in our code, and there's little to no value for a customer in matching on this error. It's a case that simply should not happen if the code is correct, so an assertion was warranted.

On the flip side, with an assertion, it would silently allow the lengths to mismatch when compiled in release mode, which would likely result in a server-side error response. I think this is OK though, since a bug with this should not have been released in the first place if it was adequately tested.

We might want to proptest header names/values for the size prediction vs. the rendering to be absolutely certain they cannot mismatch. I think they can right now in the case where a header value has Unicode characters in it since the length predictor function is using string length instead of byte length.

Another thing we could consider is just getting rid of the length prediction entirely. Just render the trailers to a BytesMut when the length is initially needed, and store that BytesMut in the AwsChunkedBody for later.

@jdisanti only debug_assert doesn't fail in release mode

I added a new error for when stream_length is set incorrectly

fix: don't convert to str before getting len of HeaderValue

github-actions · 2022-06-30T17:03:58Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-06-30T20:37:31Z

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.

…unks add: double check that stream_length used to create an `AwsChunkedBody` is correct.

github-actions · 2022-06-30T22:32:27Z

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.

rcoh

LGTM!

rcoh · 2022-07-05T13:48:04Z

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

+    pin_project! {
+        struct SputteringBody {
+            parts: Vec<Option<Bytes>>,
+            cursor: usize,
+            delay_in_millis: u64,
+        }
+    }
+
+    impl SputteringBody {
+        fn len(&self) -> usize {
+            self.parts.iter().flat_map(|b| b).map(|b| b.len()).sum()
+        }
+    }
+


this is cool!

github-actions · 2022-07-05T16:06: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.

Velfi force-pushed the feature/aws-chunked-content-encoding branch from ccde60a to f803a9e Compare June 27, 2022 17:28

add: support for unsigned aws-chunked content encoding

1c69093

Velfi force-pushed the feature/aws-chunked-content-encoding branch from f803a9e to 1c69093 Compare June 27, 2022 17:29

Velfi added 2 commits June 27, 2022 12:31

Merge branch 'main' into feature/aws-chunked-content-encoding

d7c71b0

add: aws-smithy-checksums to CrateSet.key

74ef713

rcoh requested changes Jun 27, 2022

View reviewed changes

Velfi mentioned this pull request Jun 27, 2022

Feature: Flexible checksums #1482

Closed

2 tasks

jdisanti reviewed Jun 27, 2022

View reviewed changes

CHANGELOG.next.toml Outdated Show resolved Hide resolved

CHANGELOG.next.toml Outdated Show resolved Hide resolved

Velfi and others added 4 commits June 28, 2022 16:38

Update CHANGELOG.next.toml

c2512cd

Co-authored-by: John DiSanti <[email protected]>

Merge branch 'main' into feature/aws-chunked-content-encoding

2e8d027

update: CHANGELOG.next.toml message

6cab554

jdisanti reviewed Jun 28, 2022

View reviewed changes

rcoh reviewed Jun 29, 2022

View reviewed changes

Velfi added 4 commits June 29, 2022 14:37

udpate: emit error instead of panicking for incorrect trailer length

f25a10a

add: test ensuring trailer serialization and length calculation stay …

fee1750

…in sync for empty HeaderMap

Merge branch 'main' into feature/aws-chunked-content-encoding

df7f352

jdisanti reviewed Jun 29, 2022

View reviewed changes

aws/rust-runtime/aws-http/src/content_encoding.rs Outdated Show resolved Hide resolved

aws/rust-runtime/aws-http/src/content_encoding.rs Outdated Show resolved Hide resolved

jdisanti reviewed Jun 29, 2022

View reviewed changes

update: avoid repeated allocations in trailers_as_aws_chunked_bytes

8a1054c

fix: don't convert to str before getting len of HeaderValue

Merge branch 'main' into feature/aws-chunked-content-encoding

ebefe0d

add: aws-chunked body test for inner bodies that emit intermittent ch…

6501601

…unks add: double check that stream_length used to create an `AwsChunkedBody` is correct.

rcoh approved these changes Jul 5, 2022

View reviewed changes

Merge branch 'main' into feature/aws-chunked-content-encoding

3dfe669

Velfi merged commit b77c66c into main Jul 5, 2022

Velfi deleted the feature/aws-chunked-content-encoding branch July 5, 2022 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: add support for aws-chunked content encoding #1501

Feature: add support for aws-chunked content encoding #1501

Velfi commented Jun 27, 2022

github-actions bot commented Jun 27, 2022

rcoh left a comment

rcoh Jun 27, 2022

Velfi Jun 28, 2022

rcoh Jun 27, 2022

Velfi Jun 28, 2022

rcoh Jun 27, 2022

Velfi Jun 28, 2022

Velfi Jun 28, 2022

rcoh Jun 27, 2022

Velfi Jun 28, 2022

rcoh Jun 27, 2022

Velfi Jun 28, 2022

github-actions bot commented Jun 28, 2022

jdisanti Jun 28, 2022

Velfi Jun 29, 2022

Velfi Jun 29, 2022

jdisanti Jun 28, 2022

Velfi Jun 29, 2022

rcoh left a comment

github-actions bot commented Jun 29, 2022

jdisanti Jun 29, 2022

jdisanti Jun 29, 2022 •

edited

Loading

rcoh Jun 30, 2022

Velfi Jun 30, 2022

github-actions bot commented Jun 30, 2022

github-actions bot commented Jun 30, 2022

github-actions bot commented Jun 30, 2022

rcoh left a comment

rcoh Jul 5, 2022

github-actions bot commented Jul 5, 2022

Feature: add support for aws-chunked content encoding #1501

Feature: add support for aws-chunked content encoding #1501

Conversation

Velfi commented Jun 27, 2022

Motivation and Context

Description

Testing

Checklist

github-actions bot commented Jun 27, 2022

rcoh left a 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

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

github-actions bot commented Jun 28, 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

rcoh left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 29, 2022

Choose a reason for hiding this comment

jdisanti Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 30, 2022

github-actions bot commented Jun 30, 2022

github-actions bot commented Jun 30, 2022

rcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 5, 2022

jdisanti Jun 29, 2022 •

edited

Loading