Merge branch 'main' into Issue_6976

Author: arshukla98

CHANGELOG.md

@@ -16,6 +16,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Add `WithAttributes` option to set instrumentation scope attributes on the created `log.Logger` in `go.opentelemetry.io/contrib/bridges/otellogrus`. (#6966)
 - Add `WithAttributes` option to set instrumentation scope attributes on the created `log.Logger` in `go.opentelemetry.io/contrib/bridges/otellogr`. (#6967)
 - Add the `WithGinMetricAttributes` option to allow setting dynamic, per-request metric attributes based on `*gin.Context` in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`. (#6932)
+- 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)
 
 ### Changed
 

instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconv/bench_test.go

@@ -43,6 +43,6 @@ func BenchmarkHTTPServerRequest(b *testing.B) {
 	b.ReportAllocs()
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		benchHTTPServerRequestResults = serv.RequestTraceAttrs("", req)
+		benchHTTPServerRequestResults = serv.RequestTraceAttrs("", req, RequestTraceAttrsOpts{})
 	}
 }

instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconv/env.go

@@ -61,9 +61,9 @@ type HTTPServer struct {
 //
 // If the primary server name is not known, server should be an empty string.
 // The req Host will be used to determine the server instead.
-func (s HTTPServer) RequestTraceAttrs(server string, req *http.Request) []attribute.KeyValue {
+func (s HTTPServer) RequestTraceAttrs(server string, req *http.Request, opts RequestTraceAttrsOpts) []attribute.KeyValue {
 	if s.duplicate {
-		return append(OldHTTPServer{}.RequestTraceAttrs(server, req), CurrentHTTPServer{}.RequestTraceAttrs(server, req)...)
+		return append(OldHTTPServer{}.RequestTraceAttrs(server, req), CurrentHTTPServer{}.RequestTraceAttrs(server, req, opts)...)
 	}
 	return OldHTTPServer{}.RequestTraceAttrs(server, req)
 }

instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconv/env_test.go

@@ -42,7 +42,7 @@ func TestHTTPServerDoesNotPanic(t *testing.T) {
 				req, err := http.NewRequest("GET", "http://example.com", nil)
 				require.NoError(t, err)
 
-				_ = tt.server.RequestTraceAttrs("stuff", req)
+				_ = tt.server.RequestTraceAttrs("stuff", req, RequestTraceAttrsOpts{})
 				_ = tt.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200})
 				tt.server.RecordMetrics(context.Background(), ServerMetricData{
 					ServerName: "stuff",
@@ -250,7 +250,7 @@ func BenchmarkRecordMetrics(b *testing.B) {
 	for _, bm := range benchmarks {
 		b.Run(bm.name, func(b *testing.B) {
 			req, _ := http.NewRequest("GET", "http://example.com", nil)
-			_ = bm.server.RequestTraceAttrs("stuff", req)
+			_ = bm.server.RequestTraceAttrs("stuff", req, RequestTraceAttrsOpts{})
 			_ = bm.server.ResponseTraceAttrs(ResponseTelemetry{StatusCode: 200})
 			ctx := context.Background()
 			b.ReportAllocs()

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

@@ -20,6 +20,11 @@ import (
 	semconvNew "go.opentelemetry.io/otel/semconv/v1.26.0"
 )
 
+type RequestTraceAttrsOpts struct {
+	// If set, this is used as value for the "http.client_ip" attribute.
+	HTTPClientIP string
+}
+
 type CurrentHTTPServer struct{}
 
 // RequestTraceAttrs returns trace attributes for an HTTP request received by a
@@ -38,7 +43,7 @@ type CurrentHTTPServer struct{}
 //
 // If the primary server name is not known, server should be an empty string.
 // The req Host will be used to determine the server instead.
-func (n CurrentHTTPServer) RequestTraceAttrs(server string, req *http.Request) []attribute.KeyValue {
+func (n CurrentHTTPServer) RequestTraceAttrs(server string, req *http.Request, opts RequestTraceAttrsOpts) []attribute.KeyValue {
 	count := 3 // ServerAddress, Method, Scheme
 
 	var host string
@@ -65,7 +70,8 @@ func (n CurrentHTTPServer) RequestTraceAttrs(server string, req *http.Request) [
 
 	scheme := n.scheme(req.TLS != nil)
 
-	if peer, peerPort := SplitHostPort(req.RemoteAddr); peer != "" {
+	peer, peerPort := SplitHostPort(req.RemoteAddr)
+	if peer != "" {
 		// The Go HTTP server sets RemoteAddr to "IP:port", this will not be a
 		// file-path that would be interpreted with a sock family.
 		count++
@@ -79,7 +85,17 @@ func (n CurrentHTTPServer) RequestTraceAttrs(server string, req *http.Request) [
 		count++
 	}
 
-	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 != "" {
 		count++
 	}

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

@@ -263,7 +263,7 @@ func TestRequestTraceAttrs_HTTPRoute(t *testing.T) {
 	req.Pattern = "/high/cardinality/path/{id}"
 
 	var found bool
-	for _, attr := range (CurrentHTTPServer{}).RequestTraceAttrs("", req) {
+	for _, attr := range (CurrentHTTPServer{}).RequestTraceAttrs("", req, RequestTraceAttrsOpts{}) {
 		if attr.Key != "http.route" {
 			continue
 		}
@@ -272,3 +272,54 @@ func TestRequestTraceAttrs_HTTPRoute(t *testing.T) {
 	}
 	require.True(t, found)
 }
+
+func TestRequestTraceAttrs_ClientIP(t *testing.T) {
+	for _, tt := range []struct {
+		name              string
+		requestModifierFn func(r *http.Request)
+		requestTraceOpts  RequestTraceAttrsOpts
+
+		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 client IP in options",
+			requestModifierFn: func(r *http.Request) {
+				r.Header.Add("X-Forwarded-For", "5.6.7.8")
+			},
+			requestTraceOpts: RequestTraceAttrsOpts{
+				HTTPClientIP: "9.8.7.6",
+			},
+			wantClientIP: "9.8.7.6",
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			req := httptest.NewRequest("GET", "/example", nil)
+			req.RemoteAddr = "1.2.3.4:5678"
+
+			if tt.requestModifierFn != nil {
+				tt.requestModifierFn(req)
+			}
+
+			var found bool
+			for _, attr := range (CurrentHTTPServer{}).RequestTraceAttrs("", req, tt.requestTraceOpts) {
+				if attr.Key != "client.address" {
+					continue
+				}
+				found = true
+				assert.Equal(t, tt.wantClientIP, attr.Value.AsString())
+			}
+			require.True(t, found)
+		})
+	}
+}

instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconv/test/common_test.go

@@ -67,5 +67,5 @@ func testTraceRequest(t *testing.T, serv semconv.HTTPServer, want func(testServe
 		clientIP:   clientIP,
 	}
 
-	assert.ElementsMatch(t, want(srvReq), serv.RequestTraceAttrs("", req))
+	assert.ElementsMatch(t, want(srvReq), serv.RequestTraceAttrs("", req, semconv.RequestTraceAttrsOpts{}))
 }

instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconv/v1.20.0.go

@@ -38,7 +38,7 @@ type OldHTTPServer struct{}
 // If the primary server name is not known, server should be an empty string.
 // The req Host will be used to determine the server instead.
 func (o OldHTTPServer) RequestTraceAttrs(server string, req *http.Request) []attribute.KeyValue {
-	return semconvutil.HTTPServerRequest(server, req)
+	return semconvutil.HTTPServerRequest(server, req, semconvutil.HTTPServerRequestOptions{})
 }
 
 func (o OldHTTPServer) NetworkTransportAttr(network string) attribute.KeyValue {

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

@@ -16,6 +16,11 @@ import (
 	semconv "go.opentelemetry.io/otel/semconv/v1.20.0"
 )
 
+type HTTPServerRequestOptions struct {
+	// If set, this is used as value for the "http.client_ip" attribute.
+	HTTPClientIP string
+}
+
 // HTTPClientResponse returns trace attributes for an HTTP response received by a
 // client from a server. It will return the following attributes if the related
 // values are defined in resp: "http.status.code",
@@ -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++
 	}