Gin exporter: support reading Client IP from custom headers (and make sure proxy is trusted)

instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv.go

@@ -54,6 +54,11 @@ func HTTPClientStatus(code int) (codes.Code, string) {
 	return hc.ClientStatus(code)
 }
 
+type HTTPServerRequestOptions struct {
+	// If set, this is used as value for the "http.client_ip" attribute.
+	HTTPClientIP string
+}
+
 // HTTPServerRequest returns trace attributes for an HTTP request received by a
 // server.
 //
@@ -75,8 +80,8 @@ func HTTPClientStatus(code int) (codes.Code, string) {
 // "http.target", "net.host.name". The following attributes are returned if
 // they related values are defined in req: "net.host.port", "net.sock.peer.addr",
 // "net.sock.peer.port", "user_agent.original", "http.client_ip".
-func HTTPServerRequest(server string, req *http.Request) []attribute.KeyValue {
-	return hc.ServerRequest(server, req)
+func HTTPServerRequest(server string, req *http.Request, opts HTTPServerRequestOptions) []attribute.KeyValue {
+	return hc.ServerRequest(server, req, opts)
 }
 
 // HTTPServerRequestMetrics returns metric attributes for an HTTP request received by a
@@ -305,7 +310,7 @@ func (c *httpConv) ClientRequestMetrics(req *http.Request) []attribute.KeyValue
 // related values are defined in req: "net.host.port", "net.sock.peer.addr",
 // "net.sock.peer.port", "user_agent.original", "http.client_ip",
 // "net.protocol.name", "net.protocol.version".
-func (c *httpConv) ServerRequest(server string, req *http.Request) []attribute.KeyValue {
+func (c *httpConv) ServerRequest(server string, req *http.Request, opts HTTPServerRequestOptions) []attribute.KeyValue {
 	/* The following semantic conventions are returned if present:
 	http.method             string
 	http.scheme             string
@@ -358,7 +363,17 @@ func (c *httpConv) ServerRequest(server string, req *http.Request) []attribute.K
 		n++
 	}
 
-	clientIP := serverClientIP(req.Header.Get("X-Forwarded-For"))
+	// For client IP, use, in order:
+	// 1. The value passed in the options
+	// 2. The value in the X-Forwarded-For header
+	// 3. The peer address
+	clientIP := opts.HTTPClientIP
+	if clientIP == "" {
+		clientIP = serverClientIP(req.Header.Get("X-Forwarded-For"))
+		if clientIP == "" {
+			clientIP = peer
+		}
+	}
 	if clientIP != "" {
 		n++
 	}

instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv_test.go

@@ -12,6 +12,7 @@ import (
 	"net/url"
 	"strconv"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -165,47 +166,83 @@ func TestHTTPClientRequestRequired(t *testing.T) {
 }
 
 func TestHTTPServerRequest(t *testing.T) {
-	got := make(chan *http.Request, 1)
-	handler := func(w http.ResponseWriter, r *http.Request) {
-		got <- r
-		w.WriteHeader(http.StatusOK)
-	}
+	testFn := func(requestModifierFn func(r *http.Request), opts HTTPServerRequestOptions, expectClientIP string) func(t *testing.T) {
+		return func(t *testing.T) {
+			reqCh := make(chan *http.Request, 1)
+			handler := func(w http.ResponseWriter, r *http.Request) {
+				r.RemoteAddr = "1.2.3.4:5678"
+				reqCh <- r
+				w.WriteHeader(http.StatusOK)
+			}
 
-	srv := httptest.NewServer(http.HandlerFunc(handler))
-	defer srv.Close()
+			srv := httptest.NewServer(http.HandlerFunc(handler))
+			defer srv.Close()
 
-	srvURL, err := url.Parse(srv.URL)
-	require.NoError(t, err)
-	srvPort, err := strconv.ParseInt(srvURL.Port(), 10, 32)
-	require.NoError(t, err)
+			srvURL, err := url.Parse(srv.URL)
+			require.NoError(t, err)
+			srvPort, err := strconv.ParseInt(srvURL.Port(), 10, 32)
+			require.NoError(t, err)
 
-	resp, err := srv.Client().Get(srv.URL)
-	require.NoError(t, err)
-	require.NoError(t, resp.Body.Close())
+			req, err := http.NewRequest(http.MethodGet, srv.URL, nil)
+			require.NoError(t, err)
 
-	req := <-got
-	peer, peerPort := splitHostPort(req.RemoteAddr)
+			if requestModifierFn != nil {
+				requestModifierFn(req)
+			}
 
-	const user = "alice"
-	req.SetBasicAuth(user, "pswrd")
+			resp, err := srv.Client().Do(req)
+			require.NoError(t, err)
+			require.NoError(t, resp.Body.Close())
 
-	const clientIP = "127.0.0.5"
-	req.Header.Add("X-Forwarded-For", clientIP)
+			var got *http.Request
+			select {
+			case got = <-reqCh:
+				// All good
+			case <-time.After(5 * time.Second):
+				t.Fatal("Did not receive a signal in 5s")
+			}
 
-	assert.ElementsMatch(t,
-		[]attribute.KeyValue{
-			attribute.String("http.method", "GET"),
-			attribute.String("http.scheme", "http"),
-			attribute.String("net.host.name", srvURL.Hostname()),
-			attribute.Int("net.host.port", int(srvPort)),
-			attribute.String("net.sock.peer.addr", peer),
-			attribute.Int("net.sock.peer.port", peerPort),
-			attribute.String("user_agent.original", "Go-http-client/1.1"),
-			attribute.String("http.client_ip", clientIP),
-			attribute.String("net.protocol.version", "1.1"),
-			attribute.String("http.target", "/"),
+			peer, peerPort := splitHostPort(got.RemoteAddr)
+
+			const user = "alice"
+			got.SetBasicAuth(user, "pswrd")
+
+			assert.ElementsMatch(t,
+				[]attribute.KeyValue{
+					attribute.String("http.method", "GET"),
+					attribute.String("http.scheme", "http"),
+					attribute.String("net.host.name", srvURL.Hostname()),
+					attribute.Int("net.host.port", int(srvPort)),
+					attribute.String("net.sock.peer.addr", peer),
+					attribute.Int("net.sock.peer.port", peerPort),
+					attribute.String("user_agent.original", "Go-http-client/1.1"),
+					attribute.String("http.client_ip", expectClientIP),
+					attribute.String("net.protocol.version", "1.1"),
+					attribute.String("http.target", "/"),
+				},
+				HTTPServerRequest("", got, opts))
+		}
+	}
+
+	t.Run("client IP from network", testFn(nil, HTTPServerRequestOptions{}, "1.2.3.4"))
+
+	t.Run("client IP from X-Forwarded-For header", testFn(
+		func(r *http.Request) {
+			r.Header.Add("X-Forwarded-For", "5.6.7.8")
 		},
-		HTTPServerRequest("", req))
+		HTTPServerRequestOptions{},
+		"5.6.7.8",
+	))
+
+	t.Run("set client IP in options", testFn(
+		func(r *http.Request) {
+			r.Header.Add("X-Forwarded-For", "5.6.7.8")
+		},
+		HTTPServerRequestOptions{
+			HTTPClientIP: "9.8.7.6",
+		},
+		"9.8.7.6",
+	))
 }
 
 func TestHTTPServerRequestMetrics(t *testing.T) {
@@ -227,7 +264,13 @@ func TestHTTPServerRequestMetrics(t *testing.T) {
 	require.NoError(t, err)
 	require.NoError(t, resp.Body.Close())
 
-	req := <-got
+	var req *http.Request
+	select {
+	case req = <-got:
+		// All good
+	case <-time.After(5 * time.Second):
+		t.Fatal("did not receive a signal in 5s")
+	}
 
 	assert.ElementsMatch(t,
 		[]attribute.KeyValue{
@@ -250,22 +293,22 @@ func TestHTTPServerName(t *testing.T) {
 	)
 	portStr := strconv.Itoa(port)
 	server := host + ":" + portStr
-	assert.NotPanics(t, func() { got = HTTPServerRequest(server, req) })
+	assert.NotPanics(t, func() { got = HTTPServerRequest(server, req, HTTPServerRequestOptions{}) })
 	assert.Contains(t, got, attribute.String("net.host.name", host))
 	assert.Contains(t, got, attribute.Int("net.host.port", port))
 
 	req = &http.Request{Host: "alt.host.name:" + portStr}
 	// The server parameter does not include a port, ServerRequest should use
 	// the port in the request Host field.
-	assert.NotPanics(t, func() { got = HTTPServerRequest(host, req) })
+	assert.NotPanics(t, func() { got = HTTPServerRequest(host, req, HTTPServerRequestOptions{}) })
 	assert.Contains(t, got, attribute.String("net.host.name", host))
 	assert.Contains(t, got, attribute.Int("net.host.port", port))
 }
 
 func TestHTTPServerRequestFailsGracefully(t *testing.T) {
 	req := new(http.Request)
 	var got []attribute.KeyValue
-	assert.NotPanics(t, func() { got = HTTPServerRequest("", req) })
+	assert.NotPanics(t, func() { got = HTTPServerRequest("", req, HTTPServerRequestOptions{}) })
 	want := []attribute.KeyValue{
 		attribute.String("http.method", "GET"),
 		attribute.String("http.scheme", "http"),

instrumentation/github.com/emicklei/go-restful/otelrestful/restful.go

@@ -43,7 +43,7 @@ func OTelFilter(service string, opts ...Option) restful.FilterFunction {
 		spanName := route
 
 		opts := []oteltrace.SpanStartOption{
-			oteltrace.WithAttributes(semconvutil.HTTPServerRequest(service, r)...),
+			oteltrace.WithAttributes(semconvutil.HTTPServerRequest(service, r, semconvutil.HTTPServerRequestOptions{})...),
 			oteltrace.WithSpanKind(oteltrace.SpanKindServer),
 		}
 		if route != "" {

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

@@ -66,11 +66,17 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {
 			c.Request = c.Request.WithContext(savedCtx)
 		}()
 		ctx := cfg.Propagators.Extract(savedCtx, propagation.HeaderCarrier(c.Request.Header))
+
+		serverRequestOpts := semconvutil.HTTPServerRequestOptions{
+			// Gin's ClientIP method can detect the client's IP from various headers set by proxies, and it's configurable
+			HTTPClientIP: c.ClientIP(),
+		}
 		opts := []oteltrace.SpanStartOption{
-			oteltrace.WithAttributes(semconvutil.HTTPServerRequest(service, c.Request)...),
+			oteltrace.WithAttributes(semconvutil.HTTPServerRequest(service, c.Request, serverRequestOpts)...),
 			oteltrace.WithAttributes(semconv.HTTPRoute(c.FullPath())),
 			oteltrace.WithSpanKind(oteltrace.SpanKindServer),
 		}
+
 		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()
@@ -95,3 +101,92 @@ func TestPropagationWithCustomPropagators(t *testing.T) {
 
 	router.ServeHTTP(w, r)
 }
+
+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) {
+			//nolint:errcheck
+			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) {
+			//nolint:errcheck
+			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"}
+			//nolint:errcheck
+			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.1 // 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