feat: migrate and apply bucketEndpointMiddleware #552

srchase · 2019-12-12T17:26:41Z

Migrates and applies bucketEndpointMiddleware

Enables the following configuration options on S3 Clients:

bucketEndpoint
forcePathStyle
useAccelerateEndpoint
useDualstackEndpoint

Applies to all S3 operations except CreateBucket, DeleteBucket and ListBuckets.

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

aws-sdk-js-automation · 2019-12-12T17:30:52Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

packages/middleware-bucket-endpoint/src/bucketEndpointMiddleware.ts

aws-sdk-js-automation · 2019-12-16T18:52:55Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

AllanZhengYP · 2019-12-18T00:26:08Z

packages/middleware-bucket-endpoint/src/configurations.ts

+    : false;
+  const useDualstackEndpoint = input.useDualstackEndpoint
+    ? input.useDualstackEndpoint
+    : false;


All of these can be change to

const { bucketEndpoint = false, forcePathStyle = false, //others } = input;

Note that defaulting only applies if the left-hand side is undefined - not if it's otherwise falsey like the existing code. May or may not be what you want.

AllanZhengYP · 2019-12-18T00:28:55Z

packages/middleware-bucket-endpoint/src/configurations.ts

+  preformedBucketEndpoint?: boolean;
+  useAccelerateEndpoint?: boolean;
+  useDualstackEndpoint?: boolean;
+}


Can you add document block for each of these configs? They should be documented in API reference

AllanZhengYP · 2019-12-18T00:36:44Z

packages/middleware-bucket-endpoint/src/bucketEndpointMiddleware.ts

+  ): Promise<BuildHandlerOutput<Output>> => {
+    const { Bucket: bucketName } = args.input;
+    let replaceBucketInPath =
+      options.preformedBucketEndpoint || options.bucketEndpoint;


I don't get the difference of preformedBucketEndpoint and bucketEndpoint, we might need to document them

I removed preformedBucketEndpoint. only bucketEndpoint is needed now that we combined the input and config options.

AllanZhengYP · 2019-12-18T00:40:26Z

packages/middleware-bucket-endpoint/src/bucketEndpointMiddleware.ts

+          accelerateEndpoint: options.useAccelerateEndpoint,
+          dualstackEndpoint: options.useDualstackEndpoint,
+          pathStyleEndpoint: options.forcePathStyle,
+          sslCompatible: request.protocol === "https:"


This key should be tlsCompatible as we use tls everywhere now

changed to tlsCompatible

AllanZhengYP · 2019-12-18T00:42:15Z

packages/middleware-bucket-endpoint/src/bucketEndpointMiddleware.ts

+        request.path = request.path.replace(/^(\/)?[^\/]+/, "");
+        if (request.path === "") {
+          request.path = "/";
+        }


What's the rationale for removing the path? I think we need more doc/comments on this.

Reading it, it's removing the bucket name from the path as the second-part of transforming from path-style to hostname-style bucket possibly started by bucketHostname(), e.g. from s3.amazonaws.com/my-bucket/my-key to my-bucket.s3.amazonaws.com/my-key, possibly by whatever happened before this middleware.

What's unclear to me at this point is why is the bucket name already in the request path? My understanding was that the path-style was being deprecated, so I would expect the request path to already only be the key.

@simonbuchan Even though path-style is deprecated, the API model shipped by S3 service team is still path-style. So SDK needs to do the heavy lifting to move the bucket name from path to the hostname.

aws-sdk-js-automation · 2019-12-18T20:24:42Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-sdk-js-automation · 2019-12-18T20:29:13Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-sdk-js-automation · 2019-12-19T19:11:01Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

feat: migrate and apply middleware feat: remove $ input options fix: remove preformedBucket option

aws-sdk-js-automation · 2019-12-19T19:39:34Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

lock · 2019-12-26T20:11:30Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

srchase added the smithy-codegen Changes regarding supporting smithy model. Will be merged to smithy-codegen branch label Dec 12, 2019

srchase requested a review from AllanZhengYP December 12, 2019 17:26

AllanZhengYP reviewed Dec 12, 2019

View reviewed changes

packages/middleware-bucket-endpoint/src/bucketEndpointMiddleware.ts Outdated Show resolved Hide resolved

srchase force-pushed the bucket-endpoint-middleware branch from cf33f63 to e1426dd Compare December 16, 2019 18:49

srchase requested a review from AllanZhengYP December 16, 2019 21:00

AllanZhengYP reviewed Dec 18, 2019

View reviewed changes

srchase force-pushed the bucket-endpoint-middleware branch from e1426dd to e5fb3fe Compare December 18, 2019 20:20

srchase requested a review from AllanZhengYP December 18, 2019 20:23

srchase force-pushed the bucket-endpoint-middleware branch from e5fb3fe to 03a8d87 Compare December 18, 2019 20:25

AllanZhengYP approved these changes Dec 19, 2019

View reviewed changes

feat: rename package

186b6c3

feat: migrate and apply middleware feat: remove $ input options fix: remove preformedBucket option

srchase force-pushed the bucket-endpoint-middleware branch from b4ff89c to 186b6c3 Compare December 19, 2019 19:36

srchase merged commit 7480f74 into aws:smithy-codegen Dec 19, 2019

srchase deleted the bucket-endpoint-middleware branch December 19, 2019 19:42

lock bot locked as resolved and limited conversation to collaborators Dec 26, 2019

feat: migrate and apply bucketEndpointMiddleware #552

feat: migrate and apply bucketEndpointMiddleware #552

Uh oh!

Conversation

srchase commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-sdk-js-automation commented Dec 12, 2019

AWS CodeBuild CI Report

Uh oh!

Uh oh!

aws-sdk-js-automation commented Dec 16, 2019

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AllanZhengYP Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-sdk-js-automation commented Dec 18, 2019

AWS CodeBuild CI Report

Uh oh!

aws-sdk-js-automation commented Dec 18, 2019

AWS CodeBuild CI Report

Uh oh!

aws-sdk-js-automation commented Dec 19, 2019

AWS CodeBuild CI Report

Uh oh!

aws-sdk-js-automation commented Dec 19, 2019

AWS CodeBuild CI Report

Uh oh!

lock bot commented Dec 26, 2019

Uh oh!

Uh oh!

srchase commented Dec 12, 2019 •

edited

Loading

AllanZhengYP Dec 18, 2019 •

edited

Loading