From 96b1ed1e7b0c1613290df1801ff6ba8474a33ce5 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Mon, 25 Nov 2024 08:13:43 +0100 Subject: [PATCH 1/2] chore(deps): update module github.com/bytedance/sonic to v1.12.5 (#6368) --- .../github.com/gin-gonic/gin/otelgin/example/go.mod | 2 +- .../github.com/gin-gonic/gin/otelgin/example/go.sum | 4 ++-- instrumentation/github.com/gin-gonic/gin/otelgin/go.mod | 2 +- instrumentation/github.com/gin-gonic/gin/otelgin/go.sum | 4 ++-- instrumentation/github.com/gin-gonic/gin/otelgin/test/go.mod | 2 +- instrumentation/github.com/gin-gonic/gin/otelgin/test/go.sum | 4 ++-- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/example/go.mod b/instrumentation/github.com/gin-gonic/gin/otelgin/example/go.mod index ac376495d46..b546a549e55 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/example/go.mod +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/example/go.mod @@ -17,7 +17,7 @@ require ( ) require ( - github.com/bytedance/sonic v1.12.4 // indirect + github.com/bytedance/sonic v1.12.5 // indirect github.com/bytedance/sonic/loader v0.2.1 // indirect github.com/cloudwego/base64x v0.1.4 // indirect github.com/cloudwego/iasm v0.2.0 // indirect diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/example/go.sum b/instrumentation/github.com/gin-gonic/gin/otelgin/example/go.sum index f45d89a681b..41087c5811e 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/example/go.sum +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/example/go.sum @@ -1,5 +1,5 @@ -github.com/bytedance/sonic v1.12.4 h1:9Csb3c9ZJhfUWeMtpCDCq6BUoH5ogfDFLUgQ/jG+R0k= -github.com/bytedance/sonic v1.12.4/go.mod h1:B8Gt/XvtZ3Fqj+iSKMypzymZxw/FVwgIGKzMzT9r/rk= +github.com/bytedance/sonic v1.12.5 h1:hoZxY8uW+mT+OpkcUWw4k0fDINtOcVavEsGfzwzFU/w= +github.com/bytedance/sonic v1.12.5/go.mod h1:B8Gt/XvtZ3Fqj+iSKMypzymZxw/FVwgIGKzMzT9r/rk= github.com/bytedance/sonic/loader v0.1.1/go.mod h1:ncP89zfokxS5LZrJxl5z0UJcsk4M4yY2JpfqGeCtNLU= github.com/bytedance/sonic/loader v0.2.1 h1:1GgorWTqf12TA8mma4DDSbaQigE2wOgQo7iCjjJv3+E= github.com/bytedance/sonic/loader v0.2.1/go.mod h1:ncP89zfokxS5LZrJxl5z0UJcsk4M4yY2JpfqGeCtNLU= diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/go.mod b/instrumentation/github.com/gin-gonic/gin/otelgin/go.mod index a45e6216ccc..06efaeee2ac 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/go.mod +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/go.mod @@ -13,7 +13,7 @@ require ( ) require ( - github.com/bytedance/sonic v1.12.4 // indirect + github.com/bytedance/sonic v1.12.5 // indirect github.com/bytedance/sonic/loader v0.2.1 // indirect github.com/cloudwego/base64x v0.1.4 // indirect github.com/cloudwego/iasm v0.2.0 // indirect diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/go.sum b/instrumentation/github.com/gin-gonic/gin/otelgin/go.sum index 29db99e1171..df05f5d559c 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/go.sum +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/go.sum @@ -1,5 +1,5 @@ -github.com/bytedance/sonic v1.12.4 h1:9Csb3c9ZJhfUWeMtpCDCq6BUoH5ogfDFLUgQ/jG+R0k= -github.com/bytedance/sonic v1.12.4/go.mod h1:B8Gt/XvtZ3Fqj+iSKMypzymZxw/FVwgIGKzMzT9r/rk= +github.com/bytedance/sonic v1.12.5 h1:hoZxY8uW+mT+OpkcUWw4k0fDINtOcVavEsGfzwzFU/w= +github.com/bytedance/sonic v1.12.5/go.mod h1:B8Gt/XvtZ3Fqj+iSKMypzymZxw/FVwgIGKzMzT9r/rk= github.com/bytedance/sonic/loader v0.1.1/go.mod h1:ncP89zfokxS5LZrJxl5z0UJcsk4M4yY2JpfqGeCtNLU= github.com/bytedance/sonic/loader v0.2.1 h1:1GgorWTqf12TA8mma4DDSbaQigE2wOgQo7iCjjJv3+E= github.com/bytedance/sonic/loader v0.2.1/go.mod h1:ncP89zfokxS5LZrJxl5z0UJcsk4M4yY2JpfqGeCtNLU= diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/test/go.mod b/instrumentation/github.com/gin-gonic/gin/otelgin/test/go.mod index ee969a92ed7..28e56a0d15b 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/test/go.mod +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/test/go.mod @@ -12,7 +12,7 @@ require ( ) require ( - github.com/bytedance/sonic v1.12.4 // indirect + github.com/bytedance/sonic v1.12.5 // indirect github.com/bytedance/sonic/loader v0.2.1 // indirect github.com/cloudwego/base64x v0.1.4 // indirect github.com/cloudwego/iasm v0.2.0 // indirect diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/test/go.sum b/instrumentation/github.com/gin-gonic/gin/otelgin/test/go.sum index dbe41cd1685..c861ebd2ce8 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/test/go.sum +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/test/go.sum @@ -1,5 +1,5 @@ -github.com/bytedance/sonic v1.12.4 h1:9Csb3c9ZJhfUWeMtpCDCq6BUoH5ogfDFLUgQ/jG+R0k= -github.com/bytedance/sonic v1.12.4/go.mod h1:B8Gt/XvtZ3Fqj+iSKMypzymZxw/FVwgIGKzMzT9r/rk= +github.com/bytedance/sonic v1.12.5 h1:hoZxY8uW+mT+OpkcUWw4k0fDINtOcVavEsGfzwzFU/w= +github.com/bytedance/sonic v1.12.5/go.mod h1:B8Gt/XvtZ3Fqj+iSKMypzymZxw/FVwgIGKzMzT9r/rk= github.com/bytedance/sonic/loader v0.1.1/go.mod h1:ncP89zfokxS5LZrJxl5z0UJcsk4M4yY2JpfqGeCtNLU= github.com/bytedance/sonic/loader v0.2.1 h1:1GgorWTqf12TA8mma4DDSbaQigE2wOgQo7iCjjJv3+E= github.com/bytedance/sonic/loader v0.2.1/go.mod h1:ncP89zfokxS5LZrJxl5z0UJcsk4M4yY2JpfqGeCtNLU= From b508e33e7d8c2140cac4e9530ccd4e81b68be079 Mon Sep 17 00:00:00 2001 From: Jack She Date: Mon, 25 Nov 2024 19:29:31 +1100 Subject: [PATCH 2/2] otelaws: Add finalize middleware after instead of before (#5975) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes https://github.com/open-telemetry/opentelemetry-go-contrib/issues/3368. ## Summary In any presigned URL flow, the AWS SDK skips all `Finalize` middlewares after the [PresignHTTPMiddleware](https://github.com/aws/aws-sdk-go-v2/blob/a2747951bd66c0b687a0a483195843b73f6448bb/internal/v4a/presign_middleware.go#L36-L38). So, if we move context propagation after the `PresignHTTPMiddleware`, we will not include context propagation headers in presigned URL requests, which should prevent the reported issue with signatures. ## Context The existing middleware in this package adds propagation headers (e.g. `traceparent`) to outgoing AWS requests, presumably to be used in things like AWS X-Ray. However, when making requests for presigned URLs, the AWS SDK adds a bunch of middleware of its own to requests. The most important of these is the [PresignHTTPMiddleware](https://github.com/aws/aws-sdk-go-v2/blob/a2747951bd66c0b687a0a483195843b73f6448bb/internal/v4a/presign_middleware.go#L36-L38). This does a lot of things, but for the purposes of this PR/issue, there are two particularly relevant things it does: ### Incorporating request headers into the presigned URL signature When it is built, the middleware receives a presigner. Then, when it is actually run, it receives a `*smithyHTTP.Request`, and it's entirely up to the presigner if the headers on the `*smithyHTTP.Request` are incorporated in the calculation of the presigned URL's signature. If we look in the [internals of the default v4 signer](https://github.com/aws/aws-sdk-go-v2/blob/main/internal/v4a/v4a.go#L305), it appears to just incorporate all existing headers and use them to sign the URL. This leads to the problem described in the above issue: > The URL generated from code with **otelaws middlewares** has two values `host` and `traceparent`, while the one without **otelaws middlewares** has only `host`. As observed by the comments in that issue, disabling the context propagation solves this issue, which makes sense as that removes the `traceparent` header from the signature calculation. This leads us to the second relevant behaviour of the `PresignHTTPMiddleware`. ### Short circuiting remaining middleware From the comments of the `PresignHTTPMiddleware`: ```golang // PresignHTTPRequestMiddleware provides the Finalize middleware for creating a // presigned URL for an HTTP request. // // Will short circuit the middleware stack and not forward onto the next // Finalize handler. ``` In addition to this, the Deserialize middlewares get cleared, as seen [here](https://github.com/aws/aws-sdk-go-v2/blob/a2747951bd66c0b687a0a483195843b73f6448bb/service/s3/api_client.go#L904). So, in the presigned URL flow, the last middleware that will get called will be the `PresignHTTPRequestMiddleware`. This is actually useful for us now, as if we just ensure our context propagation middleware comes last in the `Finalize` step, then we can be sure it will never run in the presign flow. Simply shifting from `middleware.Before` to `middleware.After` does just that. I've also renamed the function and moved it so that its order reflects the order of middleware execution (i.e. [finalize before deserialize](https://aws.github.io/aws-sdk-go-v2/docs/middleware/)) --------- Co-authored-by: Robert PajÄ…k --- CHANGELOG.md | 1 + .../aws/aws-sdk-go-v2/otelaws/aws.go | 36 +++--- .../aws/aws-sdk-go-v2/otelaws/aws_test.go | 110 +++++++++++++++++- 3 files changed, 126 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f022916f572..1846b0348d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed +- Fix broken AWS presigned URLs when using instrumentation in `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws`. (#5975) - Fixed the value for configuring the OTLP exporter to use `grpc` instead of `grpc/protobuf` in `go.opentelemetry.io/contrib/config`. (#6338) - Allow marshaling types in `go.opentelemetry.io/contrib/config`. (#6347) 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) } 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 91c03aec27c..ed64437990a 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,16 @@ 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 +37,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 +51,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 +64,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 +75,105 @@ func Test_otelMiddlewares_finalizeMiddleware(t *testing.T) { assert.Contains(t, input.Header[key], value) } +func Test_otelMiddlewares_finalizeMiddlewareAfter_Noop(t *testing.T) { + stack := middleware.Stack{ + Finalize: middleware.NewFinalizeStep(), + } + + propagator := mockPropagator{ + injectKey: "mock-key", + injectValue: "mock-value", + } + + m := otelMiddlewares{ + propagator: propagator, + } + + err := m.finalizeMiddlewareAfter(&stack) + require.NoError(t, err) + + // Non request input should trigger noop + input := &struct{}{} + + next := middleware.HandlerFunc(func(ctx context.Context, input interface{}) (output interface{}, metadata middleware.Metadata, err error) { + return nil, middleware.Metadata{}, nil + }) + + _, _, err = stack.Finalize.HandleMiddleware(context.Background(), input, next) + assert.NoError(t, err) +} + +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, + }) + + err := stack.Finalize.Add(presignedHTTPMiddleware, middleware.After) + require.NoError(t, err) + + 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, "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) + 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"