From bd4e785a9d9d900b734b8e66c92125e1b97a9028 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Madrigal=20=F0=9F=90=A7?= <599908+Madrigal@users.noreply.github.com> Date: Wed, 16 Oct 2024 19:19:46 -0400 Subject: [PATCH 1/4] Update presign post URL resolution to use the exact result from EndpointResolverV2 --- .../smithy/aws/go/codegen/SdkGoTypes.java | 1 + .../customization/s3/StoreResolvedUri.java | 43 +++++++++++++++++++ ...mithy.go.codegen.integration.GoIntegration | 1 + internal/context/context.go | 12 ++++++ service/s3/endpoints.go | 2 + service/s3/presign_post.go | 18 +------- service/s3/presign_post_test.go | 37 +++++++++++++--- 7 files changed, 91 insertions(+), 23 deletions(-) create mode 100644 codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/s3/StoreResolvedUri.java diff --git a/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/SdkGoTypes.java b/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/SdkGoTypes.java index c91a3f4c2aa..5a2114f1b7f 100644 --- a/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/SdkGoTypes.java +++ b/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/SdkGoTypes.java @@ -72,6 +72,7 @@ public static final class Smithy { public static final class Context { public static final Symbol SetS3Backend = AwsGoDependency.INTERNAL_CONTEXT.valueSymbol("SetS3Backend"); + public static final Symbol SetS3ResolvedUri = AwsGoDependency.INTERNAL_CONTEXT.valueSymbol("SetS3ResolvedUri"); } public static final class Endpoints { diff --git a/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/s3/StoreResolvedUri.java b/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/s3/StoreResolvedUri.java new file mode 100644 index 00000000000..d2fc9683112 --- /dev/null +++ b/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/s3/StoreResolvedUri.java @@ -0,0 +1,43 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.smithy.aws.go.codegen.customization.s3; + +import software.amazon.smithy.aws.go.codegen.AwsGoDependency; +import software.amazon.smithy.aws.go.codegen.customization.S3ModelUtils; +import software.amazon.smithy.go.codegen.GoSettings; +import software.amazon.smithy.go.codegen.GoWriter; +import software.amazon.smithy.go.codegen.integration.GoIntegration; +import software.amazon.smithy.model.Model; + +import java.util.Map; + +/** + * Stores the endpoint resolved by EndpointResolverV2 + */ +public class StoreResolvedUri implements GoIntegration { + @Override + public void renderPostEndpointResolutionHook(GoSettings settings, GoWriter writer, Model model) { + if (!S3ModelUtils.isServiceS3(model, settings.getService(model))) return; + writer.writeGoTemplate( + """ + ctx = $internalContext:L.$setFunc:L(ctx, endpt.URI.String()) + """, + Map.of( + "internalContext", AwsGoDependency.INTERNAL_CONTEXT.getAlias(), + "setFunc", "SetS3ResolvedURI") + ); + } +} diff --git a/codegen/smithy-aws-go-codegen/src/main/resources/META-INF/services/software.amazon.smithy.go.codegen.integration.GoIntegration b/codegen/smithy-aws-go-codegen/src/main/resources/META-INF/services/software.amazon.smithy.go.codegen.integration.GoIntegration index 7c2810d24f8..193e594ce79 100644 --- a/codegen/smithy-aws-go-codegen/src/main/resources/META-INF/services/software.amazon.smithy.go.codegen.integration.GoIntegration +++ b/codegen/smithy-aws-go-codegen/src/main/resources/META-INF/services/software.amazon.smithy.go.codegen.integration.GoIntegration @@ -74,6 +74,7 @@ software.amazon.smithy.aws.go.codegen.customization.RemoveDefaults software.amazon.smithy.aws.go.codegen.customization.auth.S3ExpressAuthScheme software.amazon.smithy.aws.go.codegen.customization.S3BucketContext software.amazon.smithy.aws.go.codegen.customization.s3.ExpressDefaultChecksum +software.amazon.smithy.aws.go.codegen.customization.s3.StoreResolvedUri software.amazon.smithy.aws.go.codegen.customization.auth.GlobalAnonymousOption software.amazon.smithy.aws.go.codegen.customization.CloudFrontKVSSigV4a software.amazon.smithy.aws.go.codegen.customization.BackfillProtocolTestServiceTrait diff --git a/internal/context/context.go b/internal/context/context.go index f0c283d3942..cd7cf0ae2bb 100644 --- a/internal/context/context.go +++ b/internal/context/context.go @@ -10,6 +10,7 @@ import ( type s3BackendKey struct{} type checksumInputAlgorithmKey struct{} type clockSkew struct{} +type s3resolvedUri struct{} const ( // S3BackendS3Express identifies the S3Express backend @@ -50,3 +51,14 @@ func GetAttemptSkewContext(ctx context.Context) time.Duration { x, _ := middleware.GetStackValue(ctx, clockSkew{}).(time.Duration) return x } + +// SetS3ResolvedURI sets the URI as resolved by the EndpointResolverV2 +func SetS3ResolvedURI(ctx context.Context, value string) context.Context { + return middleware.WithStackValue(ctx, s3resolvedUri{}, value) +} + +// GetS3ResolvedURI gets the URI as resolved by EndpointResolverV2 +func GetS3ResolvedURI(ctx context.Context) string { + v, _ := middleware.GetStackValue(ctx, s3resolvedUri{}).(string) + return v +} diff --git a/service/s3/endpoints.go b/service/s3/endpoints.go index 361e4201b94..c37184fc030 100644 --- a/service/s3/endpoints.go +++ b/service/s3/endpoints.go @@ -5849,6 +5849,8 @@ func (m *resolveEndpointV2Middleware) HandleFinalize(ctx context.Context, in mid rscheme.SignerProperties.SetAll(&o.SignerProperties) } + ctx = internalcontext.SetS3ResolvedURI(ctx, endpt.URI.String()) + backend := s3cust.GetPropertiesBackend(&endpt.Properties) ctx = internalcontext.SetS3Backend(ctx, backend) diff --git a/service/s3/presign_post.go b/service/s3/presign_post.go index 6bdbcde6687..c22d7e1c817 100644 --- a/service/s3/presign_post.go +++ b/service/s3/presign_post.go @@ -8,7 +8,6 @@ import ( "encoding/hex" "encoding/json" "fmt" - "net/url" "strings" "time" @@ -211,10 +210,6 @@ func (s *presignPostRequestMiddleware) HandleFinalize( ) ( out middleware.FinalizeOutput, metadata middleware.Metadata, err error, ) { - req, ok := in.Request.(*smithyhttp.Request) - if !ok { - return out, metadata, fmt.Errorf("unexpected request middleware type %T", in.Request) - } input := getOperationInput(ctx) asS3Put, ok := input.(*PutObjectInput) @@ -230,8 +225,7 @@ func (s *presignPostRequestMiddleware) HandleFinalize( return out, metadata, fmt.Errorf("PutObject input does not have a key input") } - httpReq := req.Build(ctx) - u := httpReq.URL.String() + uri := internalcontext.GetS3ResolvedURI(ctx) signingName := awsmiddleware.GetSigningName(ctx) signingRegion := awsmiddleware.GetSigningRegion(ctx) @@ -265,22 +259,14 @@ func (s *presignPostRequestMiddleware) HandleFinalize( } } - // Other middlewares may set default values on the URL on the path or as query params. Remove them - baseURL := toBaseURL(u) - out.Result = &PresignedPostRequest{ - URL: baseURL, + URL: uri, Values: fields, } return out, metadata, nil } -func toBaseURL(fullURL string) string { - a, _ := url.Parse(fullURL) - return a.Scheme + "://" + a.Host -} - // Adapted from existing PresignConverter middleware func (c presignPostConverter) ConvertToPresignMiddleware(stack *middleware.Stack, options Options) (err error) { stack.Build.Remove("UserAgent") diff --git a/service/s3/presign_post_test.go b/service/s3/presign_post_test.go index baf50a2aaf8..22eb79db602 100644 --- a/service/s3/presign_post_test.go +++ b/service/s3/presign_post_test.go @@ -19,11 +19,13 @@ func TestPresignPutObject(t *testing.T) { defer mockTime(fixedTime)() cases := map[string]struct { - input PutObjectInput - options []func(*PresignPostOptions) - expectedExpires time.Time - expectedURL string - region string + input PutObjectInput + options []func(*PresignPostOptions) + expectedExpires time.Time + expectedURL string + region string + pathStyleEnabled bool + BaseEndpoint string }{ "sample": { input: PutObjectInput{ @@ -66,6 +68,23 @@ func TestPresignPutObject(t *testing.T) { }, expectedURL: "https://mfzwi23gnjvgw.mrap.accesspoint.s3-global.amazonaws.com", }, + "use path style bucket hosting pattern": { + input: PutObjectInput{ + Bucket: aws.String("bucket"), + Key: aws.String("key"), + }, + expectedURL: "https://s3.us-west-2.amazonaws.com/bucket", + pathStyleEnabled: true, + }, + "use path style bucket with custom baseEndpoint": { + input: PutObjectInput{ + Bucket: aws.String("bucket"), + Key: aws.String("key"), + }, + expectedURL: "https://s3.custom-domain.com/bucket", + pathStyleEnabled: true, + BaseEndpoint: "https://s3.custom-domain.com", + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { @@ -81,8 +100,12 @@ func TestPresignPutObject(t *testing.T) { return aws.NopRetryer{} }, } - - presignClient := NewPresignClient(NewFromConfig(cfg)) + presignClient := NewPresignClient(NewFromConfig(cfg, func(options *Options) { + options.UsePathStyle = tc.pathStyleEnabled + if tc.BaseEndpoint != "" { + options.BaseEndpoint = aws.String(tc.BaseEndpoint) + } + })) postObject, err := presignClient.PresignPostObject(ctx, &tc.input, tc.options...) if err != nil { t.Error(err) From 13b584601f0cf6b9673fdf31c7fa7f857a8141c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Madrigal=20=F0=9F=90=A7?= <599908+Madrigal@users.noreply.github.com> Date: Wed, 16 Oct 2024 19:23:49 -0400 Subject: [PATCH 2/4] Add changelog --- .changelog/5702f383099a47a5860f5ab25cda67a1.json | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changelog/5702f383099a47a5860f5ab25cda67a1.json diff --git a/.changelog/5702f383099a47a5860f5ab25cda67a1.json b/.changelog/5702f383099a47a5860f5ab25cda67a1.json new file mode 100644 index 00000000000..8b452bf4490 --- /dev/null +++ b/.changelog/5702f383099a47a5860f5ab25cda67a1.json @@ -0,0 +1,8 @@ +{ + "id": "5702f383-099a-47a5-860f-5ab25cda67a1", + "type": "bugfix", + "description": "Update presign post URL resolution to use the exact result from EndpointResolverV2", + "modules": [ + "service/s3" + ] +} \ No newline at end of file From 83f942c6f3eb93a1c8c23fca6b5c3979b899f882 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Madrigal=20=F0=9F=90=A7?= <599908+Madrigal@users.noreply.github.com> Date: Wed, 16 Oct 2024 19:38:52 -0400 Subject: [PATCH 3/4] Fix lint error and remove unused import --- .../software/amazon/smithy/aws/go/codegen/SdkGoTypes.java | 1 - internal/context/context.go | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/SdkGoTypes.java b/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/SdkGoTypes.java index 5a2114f1b7f..c91a3f4c2aa 100644 --- a/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/SdkGoTypes.java +++ b/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/SdkGoTypes.java @@ -72,7 +72,6 @@ public static final class Smithy { public static final class Context { public static final Symbol SetS3Backend = AwsGoDependency.INTERNAL_CONTEXT.valueSymbol("SetS3Backend"); - public static final Symbol SetS3ResolvedUri = AwsGoDependency.INTERNAL_CONTEXT.valueSymbol("SetS3ResolvedUri"); } public static final class Endpoints { diff --git a/internal/context/context.go b/internal/context/context.go index cd7cf0ae2bb..5e0520c4fc1 100644 --- a/internal/context/context.go +++ b/internal/context/context.go @@ -10,7 +10,7 @@ import ( type s3BackendKey struct{} type checksumInputAlgorithmKey struct{} type clockSkew struct{} -type s3resolvedUri struct{} +type s3resolvedURI struct{} const ( // S3BackendS3Express identifies the S3Express backend @@ -54,11 +54,11 @@ func GetAttemptSkewContext(ctx context.Context) time.Duration { // SetS3ResolvedURI sets the URI as resolved by the EndpointResolverV2 func SetS3ResolvedURI(ctx context.Context, value string) context.Context { - return middleware.WithStackValue(ctx, s3resolvedUri{}, value) + return middleware.WithStackValue(ctx, s3resolvedURI{}, value) } // GetS3ResolvedURI gets the URI as resolved by EndpointResolverV2 func GetS3ResolvedURI(ctx context.Context) string { - v, _ := middleware.GetStackValue(ctx, s3resolvedUri{}).(string) + v, _ := middleware.GetStackValue(ctx, s3resolvedURI{}).(string) return v } From de356cf8aed1a2b0e85933f41a3d4a0567ace0a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Madrigal=20=F0=9F=90=A7?= <599908+Madrigal@users.noreply.github.com> Date: Wed, 23 Oct 2024 13:15:57 -0400 Subject: [PATCH 4/4] Move context operation to service/s3 and add more tests based on PR feedback --- .../customization/s3/StoreResolvedUri.java | 5 ++-- internal/context/context.go | 12 ---------- service/s3/endpoints.go | 2 +- service/s3/presign_post.go | 2 +- service/s3/presign_post_test.go | 23 +++++++++++++++++++ service/s3/uri_context.go | 23 +++++++++++++++++++ 6 files changed, 50 insertions(+), 17 deletions(-) create mode 100644 service/s3/uri_context.go diff --git a/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/s3/StoreResolvedUri.java b/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/s3/StoreResolvedUri.java index d2fc9683112..341349a8aef 100644 --- a/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/s3/StoreResolvedUri.java +++ b/codegen/smithy-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/customization/s3/StoreResolvedUri.java @@ -33,11 +33,10 @@ public void renderPostEndpointResolutionHook(GoSettings settings, GoWriter write if (!S3ModelUtils.isServiceS3(model, settings.getService(model))) return; writer.writeGoTemplate( """ - ctx = $internalContext:L.$setFunc:L(ctx, endpt.URI.String()) + ctx = $setFunc:L(ctx, endpt.URI.String()) """, Map.of( - "internalContext", AwsGoDependency.INTERNAL_CONTEXT.getAlias(), - "setFunc", "SetS3ResolvedURI") + "setFunc", "setS3ResolvedURI") ); } } diff --git a/internal/context/context.go b/internal/context/context.go index 5e0520c4fc1..f0c283d3942 100644 --- a/internal/context/context.go +++ b/internal/context/context.go @@ -10,7 +10,6 @@ import ( type s3BackendKey struct{} type checksumInputAlgorithmKey struct{} type clockSkew struct{} -type s3resolvedURI struct{} const ( // S3BackendS3Express identifies the S3Express backend @@ -51,14 +50,3 @@ func GetAttemptSkewContext(ctx context.Context) time.Duration { x, _ := middleware.GetStackValue(ctx, clockSkew{}).(time.Duration) return x } - -// SetS3ResolvedURI sets the URI as resolved by the EndpointResolverV2 -func SetS3ResolvedURI(ctx context.Context, value string) context.Context { - return middleware.WithStackValue(ctx, s3resolvedURI{}, value) -} - -// GetS3ResolvedURI gets the URI as resolved by EndpointResolverV2 -func GetS3ResolvedURI(ctx context.Context) string { - v, _ := middleware.GetStackValue(ctx, s3resolvedURI{}).(string) - return v -} diff --git a/service/s3/endpoints.go b/service/s3/endpoints.go index c37184fc030..6ff0cc6ed40 100644 --- a/service/s3/endpoints.go +++ b/service/s3/endpoints.go @@ -5849,7 +5849,7 @@ func (m *resolveEndpointV2Middleware) HandleFinalize(ctx context.Context, in mid rscheme.SignerProperties.SetAll(&o.SignerProperties) } - ctx = internalcontext.SetS3ResolvedURI(ctx, endpt.URI.String()) + ctx = setS3ResolvedURI(ctx, endpt.URI.String()) backend := s3cust.GetPropertiesBackend(&endpt.Properties) ctx = internalcontext.SetS3Backend(ctx, backend) diff --git a/service/s3/presign_post.go b/service/s3/presign_post.go index c22d7e1c817..491ed2e5da5 100644 --- a/service/s3/presign_post.go +++ b/service/s3/presign_post.go @@ -225,7 +225,7 @@ func (s *presignPostRequestMiddleware) HandleFinalize( return out, metadata, fmt.Errorf("PutObject input does not have a key input") } - uri := internalcontext.GetS3ResolvedURI(ctx) + uri := getS3ResolvedURI(ctx) signingName := awsmiddleware.GetSigningName(ctx) signingRegion := awsmiddleware.GetSigningRegion(ctx) diff --git a/service/s3/presign_post_test.go b/service/s3/presign_post_test.go index 22eb79db602..afafc684edb 100644 --- a/service/s3/presign_post_test.go +++ b/service/s3/presign_post_test.go @@ -33,6 +33,12 @@ func TestPresignPutObject(t *testing.T) { Key: aws.String("key"), }, }, + "bucket and key have the same value": { + input: PutObjectInput{ + Bucket: aws.String("bucket"), + Key: aws.String("bucket"), + }, + }, "expires override": { input: PutObjectInput{ Bucket: aws.String("bucket"), @@ -76,6 +82,14 @@ func TestPresignPutObject(t *testing.T) { expectedURL: "https://s3.us-west-2.amazonaws.com/bucket", pathStyleEnabled: true, }, + "use path style bucket and key have the same value ": { + input: PutObjectInput{ + Bucket: aws.String("value"), + Key: aws.String("value"), + }, + expectedURL: "https://s3.us-west-2.amazonaws.com/value", + pathStyleEnabled: true, + }, "use path style bucket with custom baseEndpoint": { input: PutObjectInput{ Bucket: aws.String("bucket"), @@ -85,6 +99,15 @@ func TestPresignPutObject(t *testing.T) { pathStyleEnabled: true, BaseEndpoint: "https://s3.custom-domain.com", }, + "use path style bucket with custom baseEndpoint with path": { + input: PutObjectInput{ + Bucket: aws.String("bucket"), + Key: aws.String("key"), + }, + BaseEndpoint: "https://my-custom-domain.com/path_my_path", + pathStyleEnabled: true, + expectedURL: "https://my-custom-domain.com/path_my_path/bucket", + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { diff --git a/service/s3/uri_context.go b/service/s3/uri_context.go new file mode 100644 index 00000000000..0e664c59ce0 --- /dev/null +++ b/service/s3/uri_context.go @@ -0,0 +1,23 @@ +package s3 + +// This contains helper methods to set resolver URI into the context object. If they are ever used for +// something other than S3, they should be moved to internal/context/context.go + +import ( + "context" + + "github.com/aws/smithy-go/middleware" +) + +type s3resolvedURI struct{} + +// setS3ResolvedURI sets the URI as resolved by the EndpointResolverV2 +func setS3ResolvedURI(ctx context.Context, value string) context.Context { + return middleware.WithStackValue(ctx, s3resolvedURI{}, value) +} + +// getS3ResolvedURI gets the URI as resolved by EndpointResolverV2 +func getS3ResolvedURI(ctx context.Context) string { + v, _ := middleware.GetStackValue(ctx, s3resolvedURI{}).(string) + return v +}