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

contrib/internal/httptrace/httptrace.go

@@ -57,8 +57,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()))
+	for k, v := range genClientIPSpanTags(r) {
+		opts = append([]ddtrace.StartSpanOption{tracer.Tag(k, v)}, opts...)
 	}
 	if spanctx, err := tracer.Extract(tracer.HTTPHeadersCarrier(r.Header)); err == nil {
 		opts = append(opts, tracer.ChildOf(spanctx))
@@ -90,8 +90,31 @@ func ippref(s string) *netaddr.IPPrefix {
 	return nil
 }
 
+// genClientIPSpanTags generates the client IP related key/value map of tags that need to be added to the span.
+func genClientIPSpanTags(r *http.Request) map[string]string {
+	tags := map[string]string{}
+	ip, matches := getClientIP(r)
+	if matches == nil {
+		if ip.IsValid() {
+			tags[ext.HTTPClientIP] = ip.String()
+		}
+		return tags
+	}
+	sb := strings.Builder{}
+	for hdr, ip := range matches {
+		tags[ext.HTTPRequestHeaders+"."+hdr] = ip
+		sb.WriteString(hdr + ":" + ip + ",")
+	}
+	hdrList := sb.String()
+	tags[ext.MultipleIPHeaders] = hdrList[:len(hdrList)-1]
+	return tags
+}
+
 // getClientIP attempts to find the client IP address in the given request r.
-func getClientIP(r *http.Request) netaddr.IP {
+// If several IP headers are present in the request, the returned IP is invalid and the map gets filled with the
+// header/ip pairs for all IP headers found in the request. Otherwise, the returned map is nil.
+// See https://datadoghq.atlassian.net/wiki/spaces/APS/pages/2118779066/Client+IP+addresses+resolution
+func getClientIP(r *http.Request) (netaddr.IP, map[string]string) {
 	ipHeaders := defaultIPHeaders
 	if len(clientIPHeader) > 0 {
 		ipHeaders = []string{clientIPHeader}
@@ -108,17 +131,26 @@ func getClientIP(r *http.Request) netaddr.IP {
 		}
 		return netaddr.IP{}
 	}
+	matches := map[string]string{}
+	var matchedIP netaddr.IP
 	for _, hdr := range ipHeaders {
 		if v := r.Header.Get(hdr); v != "" {
+			matches[hdr] = v
 			if ip := check(v); ip.IsValid() {
-				return ip
+				matchedIP = ip
 			}
 		}
 	}
+	if len(matches) == 1 {
+		return matchedIP, nil
+	}
+	if len(matches) > 1 {
+		return netaddr.IP{}, matches
+	}
 	if remoteIP := parseIP(r.RemoteAddr); remoteIP.IsValid() && isGlobal(remoteIP) {
-		return remoteIP
+		return remoteIP, nil
 	}
-	return netaddr.IP{}
+	return netaddr.IP{}, nil
 }
 
 func parseIP(s string) netaddr.IP {

contrib/internal/httptrace/httptrace_test.go

@@ -32,11 +32,12 @@ func TestStartRequestSpan(t *testing.T) {
 }
 
 type IPTestCase struct {
-	name           string
-	remoteAddr     string
-	headers        map[string]string
-	expectedIP     netaddr.IP
-	clientIPHeader string
+	name            string
+	remoteAddr      string
+	headers         map[string]string
+	expectedIP      netaddr.IP
+	expectedMatches map[string]string
+	clientIPHeader  string
 }
 
 func genIPTestCases() []IPTestCase {
@@ -107,14 +108,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{},
+			expectedMatches: map[string]string{"x-forwarded-for": "127.0.0.1", "forwarded-for": ipv4Global},
 		},
 		{
-			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{},
+			expectedMatches: map[string]string{"x-forwarded-for": "127.0.0.1", "forwarded-for": ipv4Global},
 		},
 		{
 			name:       "invalid-ipv6",
@@ -127,14 +130,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{},
+			expectedMatches: map[string]string{"x-forwarded-for": "2001:0db8:2001:zzzz::", "forwarded-for": ipv6Global},
 		},
 		{
-			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{},
+			expectedMatches: map[string]string{"x-forwarded-for": "2001:0db8:2001:zzzz::", "forwarded-for": ipv6Global},
 		},
 	}, tcs...)
 	tcs = append([]IPTestCase{
@@ -175,7 +180,10 @@ 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())
+			ip, matches := getClientIP(&r)
+			require.Equal(t, tc.expectedIP, ip)
+			require.Equal(t, tc.expectedMatches, matches)
+			genClientIPSpanTags(&http.Request{Header: header, RemoteAddr: tc.remoteAddr})
 		})
 	}
 }

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.