Gin exporter: support reading Client IP from custom headers (and make…

… sure proxy is trusted)

With Gin, it's possible to configure the server to read the Client IP from custom headers; examples include `X-Real-Ip` or `CF-Connecting-IP`. This PR makes it possible to set as span attribute the same IP that Gin reads.

Additionally, it makes sure that headers such as "X-Forwarded-For" are used only if Gin is configured to trust the upstream server

PS: Also fixed unit tests, where there were assertions inside handlers, which are executed in separate goroutines

instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go

@@ -66,11 +66,26 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {
 			c.Request = c.Request.WithContext(savedCtx)
 		}()
 		ctx := cfg.Propagators.Extract(savedCtx, propagation.HeaderCarrier(c.Request.Header))
+
+		// Gin's ClientIP method can detect the client's IP from various headers set by proxies, and it's configurable
+		// However, semconvutil supports the X-Forwarded-For header only
+		// So we need to temporarily set the client's IP in the X-Forwarded-For header, before restoring it after the call
+		originalHeader := c.Request.Header.Get("X-Forwarded-For")
+		c.Request.Header.Set("X-Forwarded-For", c.ClientIP())
+
 		opts := []oteltrace.SpanStartOption{
 			oteltrace.WithAttributes(semconvutil.HTTPServerRequest(service, c.Request)...),
 			oteltrace.WithAttributes(semconv.HTTPRoute(c.FullPath())),
 			oteltrace.WithSpanKind(oteltrace.SpanKindServer),
 		}
+
+		// Restore the previous value for X-Forwarded-For
+		if originalHeader == "" {
+			c.Request.Header.Del("X-Forwarded-For")
+		} else {
+			c.Request.Header.Set("X-Forwarded-For", originalHeader)
+		}
+
 		var spanName string
 		if cfg.SpanNameFormatter == nil {
 			spanName = c.FullPath()

instrumentation/github.com/gin-gonic/gin/otelgin/gintrace_test.go

@@ -10,12 +10,17 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
+	"time"
 
 	"github.com/gin-gonic/gin"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"go.opentelemetry.io/otel"
+	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/propagation"
+	sdktrace "go.opentelemetry.io/otel/sdk/trace"
+	"go.opentelemetry.io/otel/sdk/trace/tracetest"
 	"go.opentelemetry.io/otel/trace"
 	"go.opentelemetry.io/otel/trace/noop"
 
@@ -32,8 +37,9 @@ func TestGetSpanNotInstrumented(t *testing.T) {
 		// Assert we don't have a span on the context.
 		span := trace.SpanFromContext(c.Request.Context())
 		ok := !span.SpanContext().IsValid()
-		assert.True(t, ok)
-		_, _ = c.Writer.Write([]byte("ok"))
+		if !ok {
+			c.Status(http.StatusInternalServerError)
+		}
 	})
 	r := httptest.NewRequest("GET", "/ping", nil)
 	w := httptest.NewRecorder()
@@ -60,13 +66,20 @@ func TestPropagationWithGlobalPropagators(t *testing.T) {
 
 	router := gin.New()
 	router.Use(Middleware("foobar", WithTracerProvider(provider)))
+	resCh := make(chan trace.Span, 1)
 	router.GET("/user/:id", func(c *gin.Context) {
-		span := trace.SpanFromContext(c.Request.Context())
-		assert.Equal(t, sc.TraceID(), span.SpanContext().TraceID())
-		assert.Equal(t, sc.SpanID(), span.SpanContext().SpanID())
+		resCh <- trace.SpanFromContext(c.Request.Context())
 	})
 
 	router.ServeHTTP(w, r)
+
+	select {
+	case span := <-resCh:
+		assert.Equal(t, sc.TraceID(), span.SpanContext().TraceID())
+		assert.Equal(t, sc.SpanID(), span.SpanContext().SpanID())
+	case <-time.After(5 * time.Second):
+		t.Fatal("did not receive signal in 5s")
+	}
 }
 
 func TestPropagationWithCustomPropagators(t *testing.T) {
@@ -87,11 +100,104 @@ func TestPropagationWithCustomPropagators(t *testing.T) {
 
 	router := gin.New()
 	router.Use(Middleware("foobar", WithTracerProvider(provider), WithPropagators(b3)))
+	resCh := make(chan trace.Span, 1)
 	router.GET("/user/:id", func(c *gin.Context) {
-		span := trace.SpanFromContext(c.Request.Context())
-		assert.Equal(t, sc.TraceID(), span.SpanContext().TraceID())
-		assert.Equal(t, sc.SpanID(), span.SpanContext().SpanID())
+		resCh <- trace.SpanFromContext(c.Request.Context())
 	})
 
 	router.ServeHTTP(w, r)
+
+	select {
+	case span := <-resCh:
+		assert.Equal(t, sc.TraceID(), span.SpanContext().TraceID())
+		assert.Equal(t, sc.SpanID(), span.SpanContext().SpanID())
+	case <-time.After(5 * time.Second):
+		t.Fatal("did not receive signal in 5s")
+	}
+}
+
+func TestClientIP(t *testing.T) {
+	testFn := func(requestFn func(r *http.Request), ginFn func(router *gin.Engine), expect string) func(t *testing.T) {
+		return func(t *testing.T) {
+			r := httptest.NewRequest("GET", "/ping", nil)
+			r.RemoteAddr = "1.2.3.4:5678"
+
+			if requestFn != nil {
+				requestFn(r)
+			}
+
+			sr := tracetest.NewSpanRecorder()
+			provider := sdktrace.NewTracerProvider()
+			provider.RegisterSpanProcessor(sr)
+
+			doneCh := make(chan struct{})
+			router := gin.New()
+
+			if ginFn != nil {
+				ginFn(router)
+			}
+
+			router.Use(Middleware("foobar", WithTracerProvider(provider)))
+			router.GET("/ping", func(c *gin.Context) {
+				close(doneCh)
+			})
+
+			w := httptest.NewRecorder()
+			router.ServeHTTP(w, r)
+			response := w.Result() //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
+			assert.Equal(t, http.StatusOK, response.StatusCode)
+
+			select {
+			case <-doneCh:
+				// nop
+			case <-time.After(5 * time.Second):
+				t.Fatal("did not receive signal in 5s")
+			}
+
+			res := sr.Ended()
+			require.Len(t, res, 1)
+
+			got := make(map[attribute.Key]attribute.Value, len(res[0].Attributes()))
+			for _, a := range res[0].Attributes() {
+				got[a.Key] = a.Value
+			}
+
+			require.NotEmpty(t, got["http.client_ip"])
+			assert.Equal(t, expect, got["http.client_ip"].AsString())
+		}
+	}
+
+	t.Run("no header", testFn(nil, nil, "1.2.3.4"))
+
+	t.Run("header is not trusted", testFn(
+		func(r *http.Request) {
+			r.Header.Set("X-Forwarded-For", "9.8.7.6")
+		},
+		func(router *gin.Engine) {
+			router.SetTrustedProxies(nil)
+		},
+		"1.2.3.4",
+	))
+
+	t.Run("client IP in X-Forwarded-For header", testFn(
+		func(r *http.Request) {
+			r.Header.Set("X-Forwarded-For", "9.8.7.6")
+		},
+		func(router *gin.Engine) {
+			router.SetTrustedProxies([]string{"0.0.0.0/0"})
+		},
+		"9.8.7.6",
+	))
+
+	t.Run("client IP in X-Custom-IP", testFn(
+		func(r *http.Request) {
+			r.Header.Set("X-Forwarded-For", "2.3.2.3") // not used
+			r.Header.Set("X-Custom-IP", "9.8.7.6")
+		},
+		func(router *gin.Engine) {
+			router.RemoteIPHeaders = []string{"X-Custom-IP", "X-Forwarded-For"}
+			router.SetTrustedProxies([]string{"0.0.0.0/0"})
+		},
+		"9.8.7.6",
+	))
 }

instrumentation/github.com/gin-gonic/gin/otelgin/go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/stretchr/testify v1.9.0
 	go.opentelemetry.io/contrib/propagators/b3 v1.29.0
 	go.opentelemetry.io/otel v1.29.0
+	go.opentelemetry.io/otel/sdk v1.29.0
 	go.opentelemetry.io/otel/trace v1.29.0
 )
 
@@ -26,6 +27,7 @@ require (
 	github.com/go-playground/universal-translator v0.18.1 // indirect
 	github.com/go-playground/validator/v10 v10.22.0 // indirect
 	github.com/goccy/go-json v0.10.3 // indirect
+	github.com/google/uuid v1.6.0 // indirect
 	github.com/json-iterator/go v1.1.12 // indirect
 	github.com/klauspost/cpuid/v2 v2.2.8 // indirect
 	github.com/leodido/go-urn v1.4.0 // indirect

instrumentation/github.com/gin-gonic/gin/otelgin/go.sum

@@ -34,6 +34,8 @@ github.com/goccy/go-json v0.10.3/go.mod h1:oq7eo15ShAhp70Anwd5lgX2pLfOS3QCiwU/PU
 github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
 github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
 github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
+github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
+github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM=
 github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo=
 github.com/klauspost/cpuid/v2 v2.0.9/go.mod h1:FInQzS24/EEf25PyTYn52gqo7WaD8xa0213Md/qVLRg=
@@ -71,6 +73,8 @@ go.opentelemetry.io/otel v1.29.0 h1:PdomN/Al4q/lN6iBJEN3AwPvUiHPMlt93c8bqTG5Llw=
 go.opentelemetry.io/otel v1.29.0/go.mod h1:N/WtXPs1CNCUEx+Agz5uouwCba+i+bJGFicT8SR4NP8=
 go.opentelemetry.io/otel/metric v1.29.0 h1:vPf/HFWTNkPu1aYeIsc98l4ktOQaL6LeSoeV2g+8YLc=
 go.opentelemetry.io/otel/metric v1.29.0/go.mod h1:auu/QWieFVWx+DmQOUMgj0F8LHWdgalxXqvp7BII/W8=
+go.opentelemetry.io/otel/sdk v1.29.0 h1:vkqKjk7gwhS8VaWb0POZKmIEDimRCMsopNYnriHyryo=
+go.opentelemetry.io/otel/sdk v1.29.0/go.mod h1:pM8Dx5WKnvxLCb+8lG1PRNIDxu9g9b9g59Qr7hfAAok=
 go.opentelemetry.io/otel/trace v1.29.0 h1:J/8ZNK4XgR7a21DZUAsbF8pZ5Jcw1VhACmnYt39JTi4=
 go.opentelemetry.io/otel/trace v1.29.0/go.mod h1:eHl3w0sp3paPkYstJOmAimxhiFXPg+MMTlEh3nsQgWQ=
 golang.org/x/arch v0.10.0 h1:S3huipmSclq3PJMNe76NGwkBR504WFkQ5dhzWzP8ZW8=