otelaws: Add finalize middleware after instead of before

CHANGELOG.md

@@ -12,6 +12,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 - Added support for providing `endpoint`, `pollingIntervalMs` and `initialSamplingRate` using environment variable `OTEL_TRACES_SAMPLER_ARG` in `go.opentelemetry.io/contrib/samples/jaegerremote`. (#6310)
 
+### Fixed
+
+- Fix broken AWS presigned URLs when using instrumentation in `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws`. (#5975)
+
 <!-- Released section -->
 <!-- Don't change this section unless doing release -->
 

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)
 }

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,77 @@ 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,
+	})
+
+	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"