contrib/internal/httptrace: record errors in span tags while retrieving client IP

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 {

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])
+					}
+				}
+			}
 		})
 	}
 }

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.