diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dcd57e796a..5fb721d22f9 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv.go b/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv.go index 9f2833b190a..2569f0e0c16 100644 --- a/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv.go +++ b/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++ } diff --git a/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv_test.go b/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv_test.go index 868b09da277..b58f40d0c56 100644 --- a/instrumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv_test.go +++ b/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,14 +301,14 @@ 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)) } @@ -265,7 +316,7 @@ func TestHTTPServerName(t *testing.T) { 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"), diff --git a/instrumentation/github.com/emicklei/go-restful/otelrestful/restful.go b/instrumentation/github.com/emicklei/go-restful/otelrestful/restful.go index 42c7d913ad6..70cab16b144 100644 --- a/instrumentation/github.com/emicklei/go-restful/otelrestful/restful.go +++ b/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 != "" { diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go b/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go index 1affd4d6ca5..ed4cf29e0ff 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go +++ b/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() diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace_test.go b/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace_test.go index ecef39e3aeb..ac690ede1a5 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace_test.go +++ b/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() diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv.go b/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv.go index 8cfe7e3dc0b..73c75016a6c 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv.go +++ b/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++ } diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv_test.go b/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv_test.go index 868b09da277..b58f40d0c56 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/internal/semconvutil/httpconv_test.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/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,14 +301,14 @@ 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)) } @@ -265,7 +316,7 @@ func TestHTTPServerName(t *testing.T) { 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"), diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go b/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go index fb8698422dc..9121c1ccb85 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go @@ -12,6 +12,7 @@ import ( "net/http/httptest" "strconv" "testing" + "time" "github.com/gin-gonic/gin" "github.com/stretchr/testify/assert" @@ -321,3 +322,92 @@ func TestWithGinFilter(t *testing.T) { assert.Len(t, sr.Ended(), 1) }) } + +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(otelgin.Middleware("foobar", otelgin.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", + )) +} diff --git a/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv.go b/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv.go index 525ee31ffe8..d190e88da41 100644 --- a/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv.go +++ b/instrumentation/github.com/gorilla/mux/otelmux/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++ } diff --git a/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv_test.go b/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv_test.go index 868b09da277..b58f40d0c56 100644 --- a/instrumentation/github.com/gorilla/mux/otelmux/internal/semconvutil/httpconv_test.go +++ b/instrumentation/github.com/gorilla/mux/otelmux/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,14 +301,14 @@ 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)) } @@ -265,7 +316,7 @@ func TestHTTPServerName(t *testing.T) { 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"), diff --git a/instrumentation/github.com/gorilla/mux/otelmux/mux.go b/instrumentation/github.com/gorilla/mux/otelmux/mux.go index c7b2355eca8..1dd65c2446d 100644 --- a/instrumentation/github.com/gorilla/mux/otelmux/mux.go +++ b/instrumentation/github.com/gorilla/mux/otelmux/mux.go @@ -142,7 +142,7 @@ func (tw traceware) ServeHTTP(w http.ResponseWriter, r *http.Request) { } opts := []trace.SpanStartOption{ - trace.WithAttributes(semconvutil.HTTPServerRequest(tw.service, r)...), + trace.WithAttributes(semconvutil.HTTPServerRequest(tw.service, r, semconvutil.HTTPServerRequestOptions{})...), trace.WithSpanKind(trace.SpanKindServer), } diff --git a/instrumentation/github.com/labstack/echo/otelecho/echo.go b/instrumentation/github.com/labstack/echo/otelecho/echo.go index 2771ebc5d9e..0dd1091a602 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/echo.go +++ b/instrumentation/github.com/labstack/echo/otelecho/echo.go @@ -60,7 +60,7 @@ func Middleware(service string, opts ...Option) echo.MiddlewareFunc { }() ctx := cfg.Propagators.Extract(savedCtx, propagation.HeaderCarrier(request.Header)) opts := []oteltrace.SpanStartOption{ - oteltrace.WithAttributes(semconvutil.HTTPServerRequest(service, request)...), + oteltrace.WithAttributes(semconvutil.HTTPServerRequest(service, request, semconvutil.HTTPServerRequestOptions{})...), oteltrace.WithSpanKind(oteltrace.SpanKindServer), } if path := c.Path(); path != "" { diff --git a/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv.go b/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv.go index d9384c3e3fb..1c43d26e77d 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv.go +++ b/instrumentation/github.com/labstack/echo/otelecho/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++ } diff --git a/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv_test.go b/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv_test.go index 868b09da277..b58f40d0c56 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil/httpconv_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/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,14 +301,14 @@ 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)) } @@ -265,7 +316,7 @@ func TestHTTPServerName(t *testing.T) { 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"), diff --git a/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv.go b/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv.go index ae5563010b6..b882da5adc2 100644 --- a/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv.go +++ b/instrumentation/gopkg.in/macaron.v1/otelmacaron/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++ } diff --git a/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv_test.go b/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv_test.go index 868b09da277..b58f40d0c56 100644 --- a/instrumentation/gopkg.in/macaron.v1/otelmacaron/internal/semconvutil/httpconv_test.go +++ b/instrumentation/gopkg.in/macaron.v1/otelmacaron/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,14 +301,14 @@ 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)) } @@ -265,7 +316,7 @@ func TestHTTPServerName(t *testing.T) { 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"), diff --git a/instrumentation/gopkg.in/macaron.v1/otelmacaron/macaron.go b/instrumentation/gopkg.in/macaron.v1/otelmacaron/macaron.go index bda8143fe93..17e86da2757 100644 --- a/instrumentation/gopkg.in/macaron.v1/otelmacaron/macaron.go +++ b/instrumentation/gopkg.in/macaron.v1/otelmacaron/macaron.go @@ -33,7 +33,7 @@ func Middleware(service string, opts ...Option) macaron.Handler { ctx := cfg.Propagators.Extract(savedCtx, propagation.HeaderCarrier(c.Req.Header)) opts := []oteltrace.SpanStartOption{ - oteltrace.WithAttributes(semconvutil.HTTPServerRequest(service, c.Req.Request)...), + oteltrace.WithAttributes(semconvutil.HTTPServerRequest(service, c.Req.Request, semconvutil.HTTPServerRequestOptions{})...), oteltrace.WithSpanKind(oteltrace.SpanKindServer), } // TODO: span name should be router template not the actual request path, eg /user/:id vs /user/123 diff --git a/instrumentation/net/http/httptrace/otelhttptrace/httptrace.go b/instrumentation/net/http/httptrace/otelhttptrace/httptrace.go index a1230c36abc..b3150331000 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/httptrace.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/httptrace.go @@ -54,7 +54,10 @@ func Extract(ctx context.Context, req *http.Request, opts ...Option) ([]attribut c := newConfig(opts) ctx = c.propagators.Extract(ctx, propagation.HeaderCarrier(req.Header)) - attrs := append(semconvutil.HTTPServerRequest("", req), semconvutil.NetTransport("tcp")) + attrs := append( + semconvutil.HTTPServerRequest("", req, semconvutil.HTTPServerRequestOptions{}), + semconvutil.NetTransport("tcp"), + ) if req.ContentLength > 0 { a := semconv.HTTPRequestContentLength(int(req.ContentLength)) attrs = append(attrs, a) diff --git a/instrumentation/net/http/httptrace/otelhttptrace/httptrace_test.go b/instrumentation/net/http/httptrace/otelhttptrace/httptrace_test.go index faf81b85640..e9a5a73e71f 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/httptrace_test.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/httptrace_test.go @@ -78,6 +78,7 @@ func TestRoundtrip(t *testing.T) { semconv.HTTPSchemeKey: "http", semconv.HTTPTargetKey: "/", semconv.HTTPRequestContentLengthKey: "3", + semconv.HTTPClientIPKey: hp[0], semconv.NetSockPeerAddrKey: hp[0], semconv.NetTransportKey: "ip_tcp", semconv.UserAgentOriginalKey: "Go-http-client/1.1", diff --git a/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv.go b/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv.go index 811d4f3fe21..f19d5629012 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/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++ } diff --git a/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv_test.go b/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv_test.go index 868b09da277..b58f40d0c56 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/internal/semconvutil/httpconv_test.go +++ b/instrumentation/net/http/httptrace/otelhttptrace/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,14 +301,14 @@ 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)) } @@ -265,7 +316,7 @@ func TestHTTPServerName(t *testing.T) { 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"), diff --git a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go index c999b05e675..09563c95bd1 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/v1.20.0.go @@ -36,7 +36,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{}) } // ResponseTraceAttrs returns trace attributes for telemetry from an HTTP response. diff --git a/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go b/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go index a73bb06e90e..fe1e53ef23b 100644 --- a/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go +++ b/instrumentation/net/http/otelhttp/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++ } diff --git a/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv_test.go index 868b09da277..b58f40d0c56 100644 --- a/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/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,14 +301,14 @@ 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)) } @@ -265,7 +316,7 @@ func TestHTTPServerName(t *testing.T) { 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"), diff --git a/internal/shared/semconvutil/httpconv.go.tmpl b/internal/shared/semconvutil/httpconv.go.tmpl index 3a675411290..0e299edb67f 100644 --- a/internal/shared/semconvutil/httpconv.go.tmpl +++ b/internal/shared/semconvutil/httpconv.go.tmpl @@ -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++ } diff --git a/internal/shared/semconvutil/httpconv_test.go.tmpl b/internal/shared/semconvutil/httpconv_test.go.tmpl index 868b09da277..b58f40d0c56 100644 --- a/internal/shared/semconvutil/httpconv_test.go.tmpl +++ b/internal/shared/semconvutil/httpconv_test.go.tmpl @@ -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,14 +301,14 @@ 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)) } @@ -265,7 +316,7 @@ func TestHTTPServerName(t *testing.T) { 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"),