Port v1 SDK customization for s3 HTTP PUT request #2051

wty-Bryant · 2023-03-10T22:31:45Z

Add {Expect: 100-continue} http header customization middleware for s3 client HTTP PUT request larger than 2MB or with unknown-size body, solve #1451

…mization middleware for s3 client http PUT operation larger than 2MB or with unknown size Body.

aajtodd · 2023-03-13T19:01:45Z

...codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/S3100Continue.java

+
+    // returns true if service is either s3 or s3 control and needs s3 customization
+    private static boolean requiresS3Customization(Model model, ServiceShape service) {
+        return S3ModelUtils.isServiceS3(model, service) || S3ModelUtils.isServiceS3Control(model, service);


fix: Probably doesn't need to apply to s3control

aajtodd · 2023-03-13T19:04:10Z

...codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/S3100Continue.java

+    public List<RuntimeClientPlugin> getClientPlugins() {
+        return ListUtils.of(
+                RuntimeClientPlugin.builder()
+                        .servicePredicate(S3100Continue::requiresS3Customization)


fix: IIRC this builder can also specify an operation predicate, we should use that to restrict this to just the operations that we want to use it on (PutObject, UploadPart, etc)

aajtodd · 2023-03-13T19:05:26Z

config/go.mod

-	github.com/aws/aws-sdk-go-v2/service/sso v1.12.4
-	github.com/aws/aws-sdk-go-v2/service/ssooidc v1.14.4
-	github.com/aws/aws-sdk-go-v2/service/sts v1.18.5
+	github.com/aws/aws-sdk-go-v2 v1.17.6


fix: There are way too many changes to go.mod and go_module_metadata.go un-related to the intent of this PR. We should figure out why these changed/revert them. It makes it very difficult to follow the PR otherwise

aajtodd · 2023-03-13T19:13:58Z

service/internal/s3shared/s3100continue.go

+		return out, metadata, fmt.Errorf("unknown request type %T", req)
+	}
+
+	if req.Method == "PUT" && (req.ContentLength == -1 || (req.ContentLength == 0 && req.Body != nil) || req.ContentLength >= 1024*1024*2) {


comment: If we scope the customization correctly in codegen to only apply to specific operations we can probably remove the req.Method check here.

fix: Change limit (2MB) to a private constant in this file

aajtodd · 2023-03-13T19:19:29Z

service/s3/internal/customizations/s3100continue_test.go

+)
+
+// unit test for service/internal/s3shared/s3100continue.go
+func TestAdd100ContinueHttpHeader(t *testing.T) {


fix: Is there not a way to have the test live closer to where the code that defines it is (e.g. s3shared)? It would also seem like we don't really need to spinup a test server for this and can instead directly test the middleware (example from smithy-go).

If I put it under s3shared containing middleware, circular dependencies occur since s3shared and s3 depends on each other in that case due to the test.

Right but if you don't make the test depend on the s3 client and instead just test the middleware in isolation then it should be fine (see the example tests linked in previous comment)

...codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/S3100Continue.java

aajtodd · 2023-03-13T19:25:24Z

...codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/S3100Continue.java

+ * Add middleware, which adds {Expect: 100-continue} header for s3 client HTTP PUT request larger than 2MB
+ * or with unknown size streaming bodies, during operation builder step
+ */
+public class S3100Continue implements GoIntegration {


fix: Not sure where to make this comment but we ran into an issue with this header being set and signing. See this PR for more details. Can you verify that s3 put object requests with transfer acceleration enabled don't run into issues when this header is present? We may need to special case it in signing as we did in Kotlin.

I'm confused about the test design of this validation. Since UseAccelerate and SupportsAccelerate is configured in UpdateEndpoints at Serialize step in stack, is it possible to wire up inner middleware like processARNResource, s3ObjectLambdaEndpoint etc with current s3100Continue middleware in isolation in unit test to check issues?

No need to check it in a unit test. Just test the actual real S3 service with your customization enabled and UseAccelerate enabled (might need to configure it on the bucket).

…zation # Conflicts: # config/go.mod

…on, exempt Expect header from signV4

lucix-aws · 2023-03-20T18:56:19Z

service/s3/api_client.go

@@ -80,6 +80,11 @@ type Options struct {
 	// Configures the events that will be sent to the configured logger.
 	ClientLogMode aws.ClientLogMode

+	// The threshold ContentLength for HTTP PUT request to receive {Expect:
+	// 100-continue} header. When set to -1, this header will be opt out of the
+	// operation request; when set to 0, the thresholdwill be set to default 2MB


the threshold will (space)

aajtodd

Couple small fixes but this looks pretty close.

aajtodd · 2023-03-20T18:46:38Z

aws/signer/internal/v4/headers_test.go

+func TestIgnoredHeaders(t *testing.T) {
+	cases := map[string]struct {
+		Header        string
+		ExpectIgnored bool


nit: Slightly confusing to understand the expectations due to the inversion. Originally interpreted the test cases as "expect the header to be ignored" but that isn't how it's used. Might consider renaming or changing the assertion code

Agree, the initial ExcludeList rule within v4 is also a little confusing since it is valid when inner rule is invalid. I have changed the ExpectIgnored to be true if the header is ignored.

aajtodd · 2023-03-20T18:48:32Z

...codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/S3100Continue.java

+    public List<RuntimeClientPlugin> getClientPlugins() {
+        return ListUtils.of(
+                RuntimeClientPlugin.builder()
+                        .operationPredicate((model, service, operation) -> isS3Service(model, service) && operation.


fix: We probably really only care about the PutObject and UploadPart operations. The other APIs don't have large binary payloads where an Expect header makes any difference (and may not be supported by the service for those operations IDK).

Question: Can service and operation predicates be combined such that you can move the isS3Service check to servicePredicate(...)?

For service/operation predicates combination, if I put isS3Service within servicePredicate() , no matter I put that before or after the servicePredicate , it will put the add100Continue middleware within some operation that we don't want (outside s3 or within s3's other operation). Thus I think it might be better to keep the service check within operation predicate like S3GetBucketLocation

aajtodd · 2023-03-20T18:53:05Z

...codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/S3100Continue.java

+                                    .type(SymbolUtils.createValueSymbolBuilder("int64")
+                                            .putProperty(SymbolUtils.GO_UNIVERSE_TYPE, true)
+                                            .build())
+                                    .documentation("The threshold ContentLength for HTTP PUT request to receive {Expect: 100-continue} header. " +


nit:

The threshold ContentLength in bytes ...

aajtodd · 2023-03-20T18:54:36Z

...codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/S3100Continue.java

+                                            .putProperty(SymbolUtils.GO_UNIVERSE_TYPE, true)
+                                            .build())
+                                    .documentation("The threshold ContentLength for HTTP PUT request to receive {Expect: 100-continue} header. " +
+                                            "When set to -1, this header will be opt out of the operation request; when set to 0, the threshold" +


fix:

Setting to -1 will disable adding the Expect header to requests ...

aajtodd · 2023-03-20T18:57:48Z

service/internal/s3shared/s3100continue.go

+		return out, metadata, fmt.Errorf("unknown request type %T", req)
+	}
+
+	if req.ContentLength == -1 || (req.ContentLength == 0 && req.Body != nil) || req.ContentLength >= sizeLimit {


Question: When do we hit req.ContentLength == 0 && req.Body != nil case?

I haven't met such case in real code, it is judged only because the initial issue put forward that "Need to port the V1 SDK customization that set the Expect: 100-continue header for HTTP PUT operations that were larger 2 MB or streaming bodies where the content-length is unknown." According to http.Request comment about ContentLength, "For client requests, a value of 0 with a non-nil Body is also treated as unknown."

…zation

…cale to PutObject and UploadPart

aajtodd

Fix and ship.

aajtodd · 2023-03-21T01:32:06Z

.changelog/1d119df578fb4a57a2934d38ea28aaa5.json

+{
+    "id": "1d119df5-78fb-4a57-a293-4d38ea28aaa5",
+    "type": "feature",
+    "description": "enable s3100continue header config, exempt it from sigV4",


fix: There should only be one changelog entry for this entire PR/change.

aajtodd · 2023-03-21T01:34:51Z

...codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/S3100Continue.java

+    private static final String ADD_100Continue_Header = "add100Continue";
+    private static final String ADD_100Continue_Header_INTERNAL = "Add100Continue";
+    private static final String Continue_Client_Option = "ContinueHeaderThresholdBytes";
+    private static final Set<String> Put_Op_Set = new HashSet<>(Arrays.asList("PutObject", "UploadPart"));


nit: Use the full shape ID rather than just the name (e.g. com.amazonaws.s3#PutObject or whatever it actually is).

aajtodd · 2023-03-21T01:37:10Z

service/internal/s3shared/s3100continue.go

+)
+
+const s3100ContinueID = "S3100Continue"
+const defaultLimit int64 = 1024 * 1024 * 2


nit: default100ContinueThresholdBytes or similar.

…ration shape

…o add100ContinueCustomization

Tianyi Wang added 3 commits March 9, 2023 18:04

Needs debug for s3100continue middleware

9e7967f

Port aws-sdk-go-v1's {Expect: 100-continue} http request header custo…

3e03bb4

…mization middleware for s3 client http PUT operation larger than 2MB or with unknown size Body.

changelog to s3 100continue customzaiton commit

625a54a

wty-Bryant requested a review from a team as a code owner March 10, 2023 22:31

Tianyi Wang added 2 commits March 10, 2023 18:55

codegen updated module dependencies versions

ce5a431

Modify some comments in s3 100-continue middleware

13dfba5

aajtodd requested changes Mar 13, 2023

View reviewed changes

Tianyi Wang added 8 commits March 15, 2023 18:13

Modify some logic in s3 100-continue middleware PR

d66bf8d

Modify some logic in s3 100-continue middleware PR

59d9ba0

Merge remote-tracking branch 'origin/main' into add100ContinueCustomi…

b7f8dda

…zation # Conflicts: # config/go.mod

Add comment to s3100continue

afb1cdb

Merge branch 'main' into add100ContinueCustomization

b64beb3

Merge branch 'main' into add100ContinueCustomization

7f5e52c

Enable user configuration of Expect: s3100continue header customizati…

baf538a

…on, exempt Expect header from signV4

Add changlog to git push baf538a

e4cb9bc

wty-Bryant requested a review from aajtodd March 18, 2023 04:07

lucix-aws reviewed Mar 20, 2023

View reviewed changes

aajtodd requested changes Mar 20, 2023

View reviewed changes

Tianyi Wang added 3 commits March 20, 2023 18:07

Merge remote-tracking branch 'origin/main' into add100ContinueCustomi…

94a1e66

…zation

Change s3 100continue middleware codegen comment, shrink middleware s…

38963ce

…cale to PutObject and UploadPart

add changelog to last commit

83491fc

aajtodd approved these changes Mar 21, 2023

View reviewed changes

Tianyi Wang added 4 commits March 20, 2023 22:45

Change some variable name and use operation shape id to represent ope…

6174ff2

…ration shape

Merge remote-tracking branch 'origin/add100ContinueCustomization' int…

b94b5b7

…o add100ContinueCustomization

Keep one changelog for PR

3780faa

Keep one changelog for PR

c01aac6

wty-Bryant merged commit c93b5cc into main Mar 21, 2023

wty-Bryant deleted the add100ContinueCustomization branch April 10, 2023 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port v1 SDK customization for s3 HTTP PUT request #2051

Port v1 SDK customization for s3 HTTP PUT request #2051

wty-Bryant commented Mar 10, 2023

aajtodd Mar 13, 2023

aajtodd Mar 13, 2023

aajtodd Mar 13, 2023

aajtodd Mar 13, 2023

aajtodd Mar 13, 2023

wty-Bryant Mar 13, 2023

aajtodd Mar 13, 2023

aajtodd Mar 13, 2023

wty-Bryant Mar 15, 2023

aajtodd Mar 16, 2023

lucix-aws Mar 20, 2023

aajtodd left a comment

aajtodd Mar 20, 2023

wty-Bryant Mar 20, 2023

aajtodd Mar 20, 2023

aajtodd Mar 20, 2023

wty-Bryant Mar 20, 2023 •

edited

Loading

aajtodd Mar 20, 2023

aajtodd Mar 20, 2023

aajtodd Mar 20, 2023

wty-Bryant Mar 20, 2023 •

edited

Loading

aajtodd left a comment

aajtodd Mar 21, 2023

aajtodd Mar 21, 2023

aajtodd Mar 21, 2023

Port v1 SDK customization for s3 HTTP PUT request #2051

Port v1 SDK customization for s3 HTTP PUT request #2051

Conversation

wty-Bryant commented Mar 10, 2023

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

aajtodd 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

wty-Bryant Mar 20, 2023 • edited Loading

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

wty-Bryant Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

aajtodd 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

wty-Bryant Mar 20, 2023 •

edited

Loading

wty-Bryant Mar 20, 2023 •

edited

Loading