From b297ad771e7c3e323136cac2a91377fb2b8b40a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mazeau?= Date: Thu, 23 Jun 2022 15:41:40 +0200 Subject: [PATCH] contrib/internal/httptrace: write _dd.multiple-ip-headers span tag (#1346) --- contrib/internal/httptrace/httptrace.go | 52 +++++++++++--------- contrib/internal/httptrace/httptrace_test.go | 48 +++++++++++++----- ddtrace/ext/tags.go | 10 ++++ 3 files changed, 75 insertions(+), 35 deletions(-) diff --git a/contrib/internal/httptrace/httptrace.go b/contrib/internal/httptrace/httptrace.go index 8d9fdca727..1e3ae32d3e 100644 --- a/contrib/internal/httptrace/httptrace.go +++ b/contrib/internal/httptrace/httptrace.go @@ -39,12 +39,14 @@ var ( "true-client-ip", } clientIPHeader = os.Getenv("DD_TRACE_CLIENT_IP_HEADER") + collectIP = os.Getenv("DD_TRACE_CLIENT_IP_HEADER_DISABLED") != "true" ) // StartRequestSpan starts an HTTP request span with the standard list of HTTP request span tags (http.method, http.url, // http.useragent). Any further span start option can be added with opts. func StartRequestSpan(r *http.Request, opts ...ddtrace.StartSpanOption) (tracer.Span, context.Context) { // Append our span options before the given ones so that the caller can "overwrite" them. + // TODO(): rework span start option handling (https://github.com/DataDog/dd-trace-go/issues/1352) opts = append([]ddtrace.StartSpanOption{ tracer.SpanType(ext.SpanTypeWeb), tracer.Tag(ext.HTTPMethod, r.Method), @@ -57,8 +59,8 @@ func StartRequestSpan(r *http.Request, opts ...ddtrace.StartSpanOption) (tracer. tracer.Tag("http.host", r.Host), }, opts...) } - if ip := getClientIP(r); ip.IsValid() { - opts = append(opts, tracer.Tag(ext.HTTPClientIP, ip.String())) + if collectIP { + opts = append(genClientIPSpanTags(r), opts...) } if spanctx, err := tracer.Extract(tracer.HTTPHeadersCarrier(r.Header)); err == nil { opts = append(opts, tracer.ChildOf(spanctx)) @@ -90,35 +92,41 @@ func ippref(s string) *netaddr.IPPrefix { return nil } -// getClientIP attempts to find the client IP address in the given request r. -func getClientIP(r *http.Request) netaddr.IP { +// genClientIPSpanTags generates the client IP related tags that need to be added to the span. +// See https://datadoghq.atlassian.net/wiki/spaces/APS/pages/2118779066/Client+IP+addresses+resolution +func genClientIPSpanTags(r *http.Request) []ddtrace.StartSpanOption { ipHeaders := defaultIPHeaders if len(clientIPHeader) > 0 { ipHeaders = []string{clientIPHeader} } - check := func(s string) netaddr.IP { - for _, ipstr := range strings.Split(s, ",") { - ip := parseIP(strings.TrimSpace(ipstr)) - if !ip.IsValid() { - continue - } - if isGlobal(ip) { - return ip - } - } - return netaddr.IP{} - } + var headers []string + var ips []string + var opts []ddtrace.StartSpanOption for _, hdr := range ipHeaders { if v := r.Header.Get(hdr); v != "" { - if ip := check(v); ip.IsValid() { - return ip - } + headers = append(headers, hdr) + ips = append(ips, v) } } - if remoteIP := parseIP(r.RemoteAddr); remoteIP.IsValid() && isGlobal(remoteIP) { - return remoteIP + if len(ips) == 0 { + if remoteIP := parseIP(r.RemoteAddr); remoteIP.IsValid() && isGlobal(remoteIP) { + opts = append(opts, tracer.Tag(ext.HTTPClientIP, remoteIP.String())) + } + } else if len(ips) == 1 { + for _, ipstr := range strings.Split(ips[0], ",") { + ip := parseIP(strings.TrimSpace(ipstr)) + if ip.IsValid() && isGlobal(ip) { + opts = append(opts, tracer.Tag(ext.HTTPClientIP, ip.String())) + break + } + } + } else { + for i := range ips { + opts = append(opts, tracer.Tag(ext.HTTPRequestHeaders+"."+headers[i], ips[i])) + } + opts = append(opts, tracer.Tag(ext.MultipleIPHeaders, strings.Join(headers, ","))) } - return netaddr.IP{} + return opts } func parseIP(s string) netaddr.IP { diff --git a/contrib/internal/httptrace/httptrace_test.go b/contrib/internal/httptrace/httptrace_test.go index 391d5f8dfa..d1dce63be6 100644 --- a/contrib/internal/httptrace/httptrace_test.go +++ b/contrib/internal/httptrace/httptrace_test.go @@ -16,6 +16,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" ) @@ -36,6 +38,7 @@ type IPTestCase struct { remoteAddr string headers map[string]string expectedIP netaddr.IP + multiHeaders string clientIPHeader string } @@ -107,14 +110,16 @@ func genIPTestCases() []IPTestCase { expectedIP: netaddr.MustParseIP(ipv4Global), }, { - name: "invalid-ipv4-recover-multi-header-1", - headers: map[string]string{"x-forwarded-for": "127..0.0.1", "forwarded-for": ipv4Global}, - expectedIP: netaddr.MustParseIP(ipv4Global), + name: "ipv4-multi-header-1", + headers: map[string]string{"x-forwarded-for": "127.0.0.1", "forwarded-for": ipv4Global}, + expectedIP: netaddr.IP{}, + multiHeaders: "x-forwarded-for,forwarded-for", }, { - name: "invalid-ipv4-recover-multi-header-2", - headers: map[string]string{"forwarded-for": ipv4Global, "x-forwarded-for": "127..0.0.1"}, - expectedIP: netaddr.MustParseIP(ipv4Global), + name: "ipv4-multi-header-2", + headers: map[string]string{"forwarded-for": ipv4Global, "x-forwarded-for": "127.0.0.1"}, + expectedIP: netaddr.IP{}, + multiHeaders: "x-forwarded-for,forwarded-for", }, { name: "invalid-ipv6", @@ -127,14 +132,16 @@ func genIPTestCases() []IPTestCase { expectedIP: netaddr.MustParseIP(ipv6Global), }, { - name: "invalid-ipv6-recover-multi-header-1", - headers: map[string]string{"x-forwarded-for": "2001:0db8:2001:zzzz::", "forwarded-for": ipv6Global}, - expectedIP: netaddr.MustParseIP(ipv6Global), + name: "ipv6-multi-header-1", + headers: map[string]string{"x-forwarded-for": "2001:0db8:2001:zzzz::", "forwarded-for": ipv6Global}, + expectedIP: netaddr.IP{}, + multiHeaders: "x-forwarded-for,forwarded-for", }, { - name: "invalid-ipv6-recover-multi-header-2", - headers: map[string]string{"forwarded-for": ipv6Global, "x-forwarded-for": "2001:0db8:2001:zzzz::"}, - expectedIP: netaddr.MustParseIP(ipv6Global), + name: "ipv6-multi-header-2", + headers: map[string]string{"forwarded-for": ipv6Global, "x-forwarded-for": "2001:0db8:2001:zzzz::"}, + expectedIP: netaddr.IP{}, + multiHeaders: "x-forwarded-for,forwarded-for", }, }, tcs...) tcs = append([]IPTestCase{ @@ -175,7 +182,22 @@ func TestIPHeaders(t *testing.T) { } r := http.Request{Header: header, RemoteAddr: tc.remoteAddr} clientIPHeader = tc.clientIPHeader - require.Equal(t, tc.expectedIP.String(), getClientIP(&r).String()) + cfg := ddtrace.StartSpanConfig{} + for _, opt := range genClientIPSpanTags(&r) { + opt(&cfg) + } + if tc.expectedIP.IsValid() { + require.Equal(t, tc.expectedIP.String(), cfg.Tags[ext.HTTPClientIP]) + require.Nil(t, cfg.Tags[ext.MultipleIPHeaders]) + } else { + require.Nil(t, cfg.Tags[ext.HTTPClientIP]) + if tc.multiHeaders != "" { + require.Equal(t, tc.multiHeaders, cfg.Tags[ext.MultipleIPHeaders]) + for hdr, ip := range tc.headers { + require.Equal(t, ip, cfg.Tags[ext.HTTPRequestHeaders+"."+hdr]) + } + } + } }) } } diff --git a/ddtrace/ext/tags.go b/ddtrace/ext/tags.go index b76024f54a..5799c79503 100644 --- a/ddtrace/ext/tags.go +++ b/ddtrace/ext/tags.go @@ -42,6 +42,16 @@ const ( // HTTPClientIP sets the HTTP client IP tag. HTTPClientIP = "http.client_ip" + // MultipleIPHeaders sets the multiple ip header tag used internally to tell the backend an error occurred when + // retrieving an HTTP request client IP. + // See https://datadoghq.atlassian.net/wiki/spaces/APS/pages/2118779066/Client+IP+addresses+resolution + MultipleIPHeaders = "_dd.multiple-ip-headers" + + // HTTPRequestHeaders sets the HTTP request headers partial tag + // This tag is meant to be composed, i.e http.request.headers.headerX, http.request.headers.headerY, etc... + // See https://datadoghq.atlassian.net/wiki/spaces/APMINT/pages/2302444638/DD+TRACE+HEADER+TAGS + HTTPRequestHeaders = "http.request.headers" + // SpanName is a pseudo-key for setting a span's operation name by means of // a tag. It is mostly here to facilitate vendor-agnostic frameworks like Opentracing // and OpenCensus.