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

CHANGELOG.md

@@ -23,6 +23,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ### Fixed
 
 - Possible nil dereference panic in `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace`. (#5965)
+- Use Gin's own `ClientIP` method to detect the client's IP, which supports custom proxy headers in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`. (#6095)
 
 ### Removed
 

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,91 @@ 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)
-	}
+	for _, tt := range []struct {
+		name                  string
+		requestModifierFn     func(r *http.Request)
+		httpServerRequestOpts HTTPServerRequestOptions
 
-	srv := httptest.NewServer(http.HandlerFunc(handler))
-	defer srv.Close()
+		wantClientIP string
+	}{
+		{
+			name:         "with a client IP from the network",
+			wantClientIP: "1.2.3.4",
+		},
+		{
+			name: "with a client IP from x-forwarded-for header",
+			requestModifierFn: func(r *http.Request) {
+				r.Header.Add("X-Forwarded-For", "5.6.7.8")
+			},
+			wantClientIP: "5.6.7.8",
+		},
+		{
+			name: "with a cl;ient IP in options",
+			requestModifierFn: func(r *http.Request) {
+				r.Header.Add("X-Forwarded-For", "5.6.7.8")
+			},
+			httpServerRequestOpts: HTTPServerRequestOptions{
+				HTTPClientIP: "9.8.7.6",
+			},
+			wantClientIP: "9.8.7.6",
+		},
+	} {
+		t.Run(tt.name, 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)
+			}
 
-	srvURL, err := url.Parse(srv.URL)
-	require.NoError(t, err)
-	srvPort, err := strconv.ParseInt(srvURL.Port(), 10, 32)
-	require.NoError(t, err)
+			srv := httptest.NewServer(http.HandlerFunc(handler))
+			defer srv.Close()
 
-	resp, err := srv.Client().Get(srv.URL)
-	require.NoError(t, err)
-	require.NoError(t, resp.Body.Close())
+			srvURL, err := url.Parse(srv.URL)
+			require.NoError(t, err)
+			srvPort, err := strconv.ParseInt(srvURL.Port(), 10, 32)
+			require.NoError(t, err)
 
-	req := <-got
-	peer, peerPort := splitHostPort(req.RemoteAddr)
+			req, err := http.NewRequest(http.MethodGet, srv.URL, nil)
+			require.NoError(t, err)
 
-	const user = "alice"
-	req.SetBasicAuth(user, "pswrd")
+			if tt.requestModifierFn != nil {
+				tt.requestModifierFn(req)
+			}
 
-	const clientIP = "127.0.0.5"
-	req.Header.Add("X-Forwarded-For", clientIP)
+			resp, err := srv.Client().Do(req)
+			require.NoError(t, err)
+			require.NoError(t, resp.Body.Close())
 
-	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", "/"),
-		},
-		HTTPServerRequest("", req))
+			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")
+			}
+
+			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", tt.wantClientIP),
+					attribute.String("net.protocol.version", "1.1"),
+					attribute.String("http.target", "/"),
+				},
+				HTTPServerRequest("", got, tt.httpServerRequestOpts))
+		})
+	}
 }
 
 func TestHTTPServerRequestMetrics(t *testing.T) {
@@ -227,7 +272,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 +301,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

@@ -32,8 +32,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()

instrumentation/github.com/gin-gonic/gin/otelgin/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++
 	}