From bf5da4babeebb586732d85280641c9f2672ee852 Mon Sep 17 00:00:00 2001 From: Jack She Date: Sat, 3 Aug 2024 01:12:16 +1000 Subject: [PATCH 1/5] move finalize middleware after --- .../aws/aws-sdk-go-v2/otelaws/aws.go | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws.go b/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws.go index 5709db82cf4..42ea8ecab10 100644 --- a/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws.go +++ b/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws.go @@ -83,6 +83,23 @@ func (m otelMiddlewares) initializeMiddlewareAfter(stack *middleware.Stack) erro middleware.After) } +func (m otelMiddlewares) finalizeMiddlewareAfter(stack *middleware.Stack) error { + return stack.Finalize.Add(middleware.FinalizeMiddlewareFunc("OTelFinalizeMiddleware", func( + ctx context.Context, in middleware.FinalizeInput, next middleware.FinalizeHandler) ( + out middleware.FinalizeOutput, metadata middleware.Metadata, err error, + ) { + // Propagate the Trace information by injecting it into the HTTP request. + switch req := in.Request.(type) { + case *smithyhttp.Request: + m.propagator.Inject(ctx, propagation.HeaderCarrier(req.Header)) + default: + } + + return next.HandleFinalize(ctx, in) + }), + middleware.After) +} + func (m otelMiddlewares) deserializeMiddleware(stack *middleware.Stack) error { return stack.Deserialize.Add(middleware.DeserializeMiddlewareFunc("OTelDeserializeMiddleware", func( ctx context.Context, in middleware.DeserializeInput, next middleware.DeserializeHandler) ( @@ -108,23 +125,6 @@ func (m otelMiddlewares) deserializeMiddleware(stack *middleware.Stack) error { middleware.Before) } -func (m otelMiddlewares) finalizeMiddleware(stack *middleware.Stack) error { - return stack.Finalize.Add(middleware.FinalizeMiddlewareFunc("OTelFinalizeMiddleware", func( - ctx context.Context, in middleware.FinalizeInput, next middleware.FinalizeHandler) ( - out middleware.FinalizeOutput, metadata middleware.Metadata, err error, - ) { - // Propagate the Trace information by injecting it into the HTTP request. - switch req := in.Request.(type) { - case *smithyhttp.Request: - m.propagator.Inject(ctx, propagation.HeaderCarrier(req.Header)) - default: - } - - return next.HandleFinalize(ctx, in) - }), - middleware.Before) -} - func spanName(serviceID, operation string) string { spanName := serviceID if operation != "" { @@ -155,5 +155,5 @@ func AppendMiddlewares(apiOptions *[]func(*middleware.Stack) error, opts ...Opti propagator: cfg.TextMapPropagator, attributeSetter: cfg.AttributeSetter, } - *apiOptions = append(*apiOptions, m.initializeMiddlewareBefore, m.initializeMiddlewareAfter, m.finalizeMiddleware, m.deserializeMiddleware) + *apiOptions = append(*apiOptions, m.initializeMiddlewareBefore, m.initializeMiddlewareAfter, m.finalizeMiddlewareAfter, m.deserializeMiddleware) } From f25e56e4afcf7af64df2c967d2c532e1f60c1274 Mon Sep 17 00:00:00 2001 From: Jack She Date: Fri, 15 Nov 2024 14:46:52 +1100 Subject: [PATCH 2/5] add test --- .../aws/aws-sdk-go-v2/otelaws/aws_test.go | 80 ++++++++++++++++++- 1 file changed, 77 insertions(+), 3 deletions(-) diff --git a/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go b/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go index 46edfbd82a7..9de0f1ffe1b 100644 --- a/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go +++ b/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go @@ -7,12 +7,15 @@ import ( "context" "net/http" "testing" + "time" "github.com/aws/smithy-go/middleware" smithyhttp "github.com/aws/smithy-go/transport/http" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/aws/aws-sdk-go-v2/aws" + awsSignerV4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" "go.opentelemetry.io/otel/propagation" ) @@ -33,7 +36,7 @@ func (p mockPropagator) Fields() []string { return []string{} } -func Test_otelMiddlewares_finalizeMiddleware(t *testing.T) { +func Test_otelMiddlewares_finalizeMiddlewareAfter(t *testing.T) { stack := middleware.Stack{ Finalize: middleware.NewFinalizeStep(), } @@ -47,7 +50,7 @@ func Test_otelMiddlewares_finalizeMiddleware(t *testing.T) { propagator: propagator, } - err := m.finalizeMiddleware(&stack) + err := m.finalizeMiddlewareAfter(&stack) require.NoError(t, err) input := &smithyhttp.Request{ @@ -60,7 +63,8 @@ func Test_otelMiddlewares_finalizeMiddleware(t *testing.T) { return nil, middleware.Metadata{}, nil }) - _, _, _ = stack.Finalize.HandleMiddleware(context.Background(), input, next) + _, _, err = stack.Finalize.HandleMiddleware(context.Background(), input, next) + require.NoError(t, err) // Assert header has been updated with injected values key := http.CanonicalHeaderKey(propagator.injectKey) @@ -70,6 +74,76 @@ func Test_otelMiddlewares_finalizeMiddleware(t *testing.T) { assert.Contains(t, input.Header[key], value) } +type mockCredentialsProvider struct{} + +func (mockCredentialsProvider) Retrieve(context.Context) (aws.Credentials, error) { + return aws.Credentials{}, nil +} + +type mockHTTPPresigner struct{} + +func (f mockHTTPPresigner) PresignHTTP( + ctx context.Context, credentials aws.Credentials, r *http.Request, + payloadHash string, service string, region string, signingTime time.Time, + optFns ...func(*awsSignerV4.SignerOptions), +) ( + url string, signedHeader http.Header, err error, +) { + return "mock-url", nil, nil +} + +func Test_otelMiddlewares_presignedRequests(t *testing.T) { + stack := middleware.Stack{ + Finalize: middleware.NewFinalizeStep(), + } + + presignedHTTPMiddleware := awsSignerV4.NewPresignHTTPRequestMiddleware(awsSignerV4.PresignHTTPRequestMiddlewareOptions{ + CredentialsProvider: mockCredentialsProvider{}, + Presigner: mockHTTPPresigner{}, + LogSigning: false, + }) + + stack.Finalize.Add(presignedHTTPMiddleware, middleware.After) + + propagator := mockPropagator{ + injectKey: "mock-key", + injectValue: "mock-value", + } + + m := otelMiddlewares{ + propagator: propagator, + } + + err := m.finalizeMiddlewareAfter(&stack) + require.NoError(t, err) + + input := &smithyhttp.Request{ + Request: &http.Request{ + Header: http.Header{}, + }, + } + + next := middleware.HandlerFunc(func(ctx context.Context, input interface{}) (output interface{}, metadata middleware.Metadata, err error) { + return nil, middleware.Metadata{}, nil + }) + + ctx := awsSignerV4.SetPayloadHash(context.Background(), "mock-hash") + url, _, err := stack.Finalize.HandleMiddleware(ctx, input, next) + + // verify we actually went through the presign flow + require.NoError(t, err) + presignedReq, ok := url.(*awsSignerV4.PresignedHTTPRequest) + require.True(t, ok) + require.Equal(t, presignedReq.URL, "mock-url") + + // Assert header has NOT been updated with injected values, as the presign middleware should short circuit + key := http.CanonicalHeaderKey(propagator.injectKey) + value := propagator.injectValue + + assert.NotContains(t, input.Header, key) + assert.NotContains(t, input.Header[key], value) +} + func Test_Span_name(t *testing.T) { serviceID1 := "" serviceID2 := "ServiceID" From 6a68bf4949e7d6ec8293929eb130159a9b950acf Mon Sep 17 00:00:00 2001 From: Jack She Date: Fri, 15 Nov 2024 16:59:25 +1100 Subject: [PATCH 3/5] linting issues --- .../github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go b/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go index 9de0f1ffe1b..c2deb004ac5 100644 --- a/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go +++ b/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go @@ -16,6 +16,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" awsSignerV4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" + "go.opentelemetry.io/otel/propagation" ) @@ -103,7 +104,8 @@ func Test_otelMiddlewares_presignedRequests(t *testing.T) { LogSigning: false, }) - stack.Finalize.Add(presignedHTTPMiddleware, middleware.After) + err := stack.Finalize.Add(presignedHTTPMiddleware, middleware.After) + require.NoError(t, err) propagator := mockPropagator{ injectKey: "mock-key", @@ -134,7 +136,7 @@ func Test_otelMiddlewares_presignedRequests(t *testing.T) { require.NoError(t, err) presignedReq, ok := url.(*awsSignerV4.PresignedHTTPRequest) require.True(t, ok) - require.Equal(t, presignedReq.URL, "mock-url") + require.Equal(t, "mock-url", presignedReq.URL) // Assert header has NOT been updated with injected values, as the presign middleware should short circuit key := http.CanonicalHeaderKey(propagator.injectKey) From 659de52fe973ed3901a9275fcbf49085fc816bc3 Mon Sep 17 00:00:00 2001 From: Jack She Date: Fri, 15 Nov 2024 17:01:01 +1100 Subject: [PATCH 4/5] fix another error --- .../github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go b/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go index 6277a21f39e..da44ef3d7d4 100644 --- a/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go +++ b/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go @@ -116,7 +116,7 @@ func Test_otelMiddlewares_presignedRequests(t *testing.T) { propagator: propagator, } - err := m.finalizeMiddlewareAfter(&stack) + err = m.finalizeMiddlewareAfter(&stack) require.NoError(t, err) input := &smithyhttp.Request{ From 082cb53f45cebeb2cc44c0c66f9673802b2feca2 Mon Sep 17 00:00:00 2001 From: Jack She Date: Sat, 16 Nov 2024 18:53:04 +1100 Subject: [PATCH 5/5] add changelog entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29e95a2c4cb..c2511cd3881 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +- Fix broken AWS presigned URLs when using instrumentation in `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws`. (#5975) +