Fix IPv6 parsing errors in semconv.NetAttributesFromHTTPRequest (#2285)

* Fix IPv6 handling errors in semconv.NetAttributesFromHTTPRequest

fixes #2283

* Enter PR number in CHANGELOG

* Remove unnecessary creation and then assignment

Standardize order of checks for IP, Name, Port

* Assume happy path when parsing host and port

i.e. assume net.SplitHostPort(input) will succeed

* Get rid of uint64 for port

* Fix git merge of main by adding back strings import

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <codingalias@gmail.com>

CHANGELOG.md

@@ -23,6 +23,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Adds `otlptracegrpc.WithGRPCConn` and `otlpmetricgrpc.WithGRPCConn` for reusing existing gRPC connection. (#2002)
 - Added a new `schema` module to help parse Schema Files in OTEP 0152 format. (#2267)
 
+### Fixed
+
+- `semconv.NetAttributesFromHTTPRequest()` correctly handles IPv6 addresses. (#2285)
+
 ## [1.0.1] - 2021-10-01
 
 ### Fixed

semconv/v1.4.0/http.go

@@ -51,64 +51,22 @@ func NetAttributesFromHTTPRequest(network string, request *http.Request) []attri
 		attrs = append(attrs, NetTransportOther)
 	}
 
-	peerName, peerIP, peerPort := "", "", 0
+	peerIP, peerName, peerPort := hostIPNamePort(request.RemoteAddr)
-	{
+	if peerIP != "" {
-		hostPart := request.RemoteAddr
+		attrs = append(attrs, NetPeerIPKey.String(peerIP))
-		portPart := ""
-		if idx := strings.LastIndex(hostPart, ":"); idx >= 0 {
-			hostPart = request.RemoteAddr[:idx]
-			portPart = request.RemoteAddr[idx+1:]
-		}
-		if hostPart != "" {
-			if ip := net.ParseIP(hostPart); ip != nil {
-				peerIP = ip.String()
-			} else {
-				peerName = hostPart
-			}
-
-			if portPart != "" {
-				numPort, err := strconv.ParseUint(portPart, 10, 16)
-				if err == nil {
-					peerPort = (int)(numPort)
-				} else {
-					peerName, peerIP = "", ""
-				}
-			}
-		}
 	}
 	if peerName != "" {
 		attrs = append(attrs, NetPeerNameKey.String(peerName))
 	}
-	if peerIP != "" {
-		attrs = append(attrs, NetPeerIPKey.String(peerIP))
-	}
 	if peerPort != 0 {
 		attrs = append(attrs, NetPeerPortKey.Int(peerPort))
 	}
 
 	hostIP, hostName, hostPort := "", "", 0
 	for _, someHost := range []string{request.Host, request.Header.Get("Host"), request.URL.Host} {
-		hostPart := ""
+		hostIP, hostName, hostPort = hostIPNamePort(someHost)
-		if idx := strings.LastIndex(someHost, ":"); idx >= 0 {
+		if hostIP != "" || hostName != "" || hostPort != 0 {
-			strPort := someHost[idx+1:]
-			numPort, err := strconv.ParseUint(strPort, 10, 16)
-			if err == nil {
-				hostPort = (int)(numPort)
-			}
-			hostPart = someHost[:idx]
-		} else {
-			hostPart = someHost
-		}
-		if hostPart != "" {
-			ip := net.ParseIP(hostPart)
-			if ip != nil {
-				hostIP = ip.String()
-			} else {
-				hostName = hostPart
-			}
 			break
-		} else {
-			hostPort = 0
 		}
 	}
 	if hostIP != "" {
@@ -124,6 +82,30 @@ func NetAttributesFromHTTPRequest(network string, request *http.Request) []attri
 	return attrs
 }
 
+// hostIPNamePort extracts the IP address, name and (optional) port from hostWithPort.
+// It handles both IPv4 and IPv6 addresses. If the host portion is not recognized
+// as a valid IPv4 or IPv6 address, the `ip` result will be empty and the
+// host portion will instead be returned in `name`.
+func hostIPNamePort(hostWithPort string) (ip string, name string, port int) {
+	var (
+		hostPart, portPart string
+		parsedPort         uint64
+		err                error
+	)
+	if hostPart, portPart, err = net.SplitHostPort(hostWithPort); err != nil {
+		hostPart, portPart = hostWithPort, ""
+	}
+	if parsedIP := net.ParseIP(hostPart); parsedIP != nil {
+		ip = parsedIP.String()
+	} else {
+		name = hostPart
+	}
+	if parsedPort, err = strconv.ParseUint(portPart, 10, 16); err == nil {
+		port = int(parsedPort)
+	}
+	return
+}
+
 // EndUserAttributesFromHTTPRequest generates attributes of the
 // enduser namespace as specified by the OpenTelemetry specification
 // for a span.

semconv/v1.4.0/http_test.go

@@ -20,6 +20,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/assert"
 
 	"go.opentelemetry.io/otel/attribute"
@@ -131,7 +132,7 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 			},
 		},
 		{
-			name:       "with remote ip and port",
+			name:       "with remote ipv4 and port",
 			network:    "tcp",
 			method:     "GET",
 			requestURI: "/user/123",
@@ -148,6 +149,42 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 				attribute.Int("net.peer.port", 56),
 			},
 		},
+		{
+			name:       "with remote ipv6 and port",
+			network:    "tcp",
+			method:     "GET",
+			requestURI: "/user/123",
+			proto:      "HTTP/1.0",
+			remoteAddr: "[fe80::0202:b3ff:fe1e:8329]:56",
+			host:       "",
+			url: &url.URL{
+				Path: "/user/123",
+			},
+			header: nil,
+			expected: []attribute.KeyValue{
+				attribute.String("net.transport", "ip_tcp"),
+				attribute.String("net.peer.ip", "fe80::202:b3ff:fe1e:8329"),
+				attribute.Int("net.peer.port", 56),
+			},
+		},
+		{
+			name:       "with remote ipv4-in-v6 and port",
+			network:    "tcp",
+			method:     "GET",
+			requestURI: "/user/123",
+			proto:      "HTTP/1.0",
+			remoteAddr: "[::ffff:192.168.0.1]:56",
+			host:       "",
+			url: &url.URL{
+				Path: "/user/123",
+			},
+			header: nil,
+			expected: []attribute.KeyValue{
+				attribute.String("net.transport", "ip_tcp"),
+				attribute.String("net.peer.ip", "192.168.0.1"),
+				attribute.Int("net.peer.port", 56),
+			},
+		},
 		{
 			name:       "with remote name and port",
 			network:    "tcp",
@@ -167,7 +204,7 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 			},
 		},
 		{
-			name:       "with remote ip only",
+			name:       "with remote ipv4 only",
 			network:    "tcp",
 			method:     "GET",
 			requestURI: "/user/123",
@@ -183,6 +220,40 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 				attribute.String("net.peer.ip", "1.2.3.4"),
 			},
 		},
+		{
+			name:       "with remote ipv6 only",
+			network:    "tcp",
+			method:     "GET",
+			requestURI: "/user/123",
+			proto:      "HTTP/1.0",
+			remoteAddr: "fe80::0202:b3ff:fe1e:8329",
+			host:       "",
+			url: &url.URL{
+				Path: "/user/123",
+			},
+			header: nil,
+			expected: []attribute.KeyValue{
+				attribute.String("net.transport", "ip_tcp"),
+				attribute.String("net.peer.ip", "fe80::202:b3ff:fe1e:8329"),
+			},
+		},
+		{
+			name:       "with remote ipv4_in_v6 only",
+			network:    "tcp",
+			method:     "GET",
+			requestURI: "/user/123",
+			proto:      "HTTP/1.0",
+			remoteAddr: "::ffff:192.168.0.1", // section 2.5.5.2 of RFC4291
+			host:       "",
+			url: &url.URL{
+				Path: "/user/123",
+			},
+			header: nil,
+			expected: []attribute.KeyValue{
+				attribute.String("net.transport", "ip_tcp"),
+				attribute.String("net.peer.ip", "192.168.0.1"),
+			},
+		},
 		{
 			name:       "with remote name only",
 			network:    "tcp",
@@ -214,6 +285,7 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 			header: nil,
 			expected: []attribute.KeyValue{
 				attribute.String("net.transport", "ip_tcp"),
+				attribute.Int("net.peer.port", 56),
 			},
 		},
 		{
@@ -236,7 +308,7 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 			},
 		},
 		{
-			name:       "with host ip only",
+			name:       "with host ipv4 only",
 			network:    "tcp",
 			method:     "GET",
 			requestURI: "/user/123",
@@ -254,6 +326,25 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 				attribute.String("net.host.ip", "4.3.2.1"),
 			},
 		},
+		{
+			name:       "with host ipv6 only",
+			network:    "tcp",
+			method:     "GET",
+			requestURI: "/user/123",
+			proto:      "HTTP/1.0",
+			remoteAddr: "1.2.3.4:56",
+			host:       "fe80::0202:b3ff:fe1e:8329",
+			url: &url.URL{
+				Path: "/user/123",
+			},
+			header: nil,
+			expected: []attribute.KeyValue{
+				attribute.String("net.transport", "ip_tcp"),
+				attribute.String("net.peer.ip", "1.2.3.4"),
+				attribute.Int("net.peer.port", 56),
+				attribute.String("net.host.ip", "fe80::202:b3ff:fe1e:8329"),
+			},
+		},
 		{
 			name:       "with host name and port",
 			network:    "tcp",
@@ -275,7 +366,7 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 			},
 		},
 		{
-			name:       "with host ip and port",
+			name:       "with host ipv4 and port",
 			network:    "tcp",
 			method:     "GET",
 			requestURI: "/user/123",
@@ -294,6 +385,26 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 				attribute.Int("net.host.port", 78),
 			},
 		},
+		{
+			name:       "with host ipv6 and port",
+			network:    "tcp",
+			method:     "GET",
+			requestURI: "/user/123",
+			proto:      "HTTP/1.0",
+			remoteAddr: "1.2.3.4:56",
+			host:       "[fe80::202:b3ff:fe1e:8329]:78",
+			url: &url.URL{
+				Path: "/user/123",
+			},
+			header: nil,
+			expected: []attribute.KeyValue{
+				attribute.String("net.transport", "ip_tcp"),
+				attribute.String("net.peer.ip", "1.2.3.4"),
+				attribute.Int("net.peer.port", 56),
+				attribute.String("net.host.ip", "fe80::202:b3ff:fe1e:8329"),
+				attribute.Int("net.host.port", 78),
+			},
+		},
 		{
 			name:       "with host name and bogus port",
 			network:    "tcp",
@@ -314,7 +425,7 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 			},
 		},
 		{
-			name:       "with host ip and bogus port",
+			name:       "with host ipv4 and bogus port",
 			network:    "tcp",
 			method:     "GET",
 			requestURI: "/user/123",
@@ -332,6 +443,25 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 				attribute.String("net.host.ip", "4.3.2.1"),
 			},
 		},
+		{
+			name:       "with host ipv6 and bogus port",
+			network:    "tcp",
+			method:     "GET",
+			requestURI: "/user/123",
+			proto:      "HTTP/1.0",
+			remoteAddr: "1.2.3.4:56",
+			host:       "[fe80::202:b3ff:fe1e:8329]:qwerty",
+			url: &url.URL{
+				Path: "/user/123",
+			},
+			header: nil,
+			expected: []attribute.KeyValue{
+				attribute.String("net.transport", "ip_tcp"),
+				attribute.String("net.peer.ip", "1.2.3.4"),
+				attribute.Int("net.peer.port", 56),
+				attribute.String("net.host.ip", "fe80::202:b3ff:fe1e:8329"),
+			},
+		},
 		{
 			name:       "with empty host and port",
 			network:    "tcp",
@@ -348,6 +478,7 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 				attribute.String("net.transport", "ip_tcp"),
 				attribute.String("net.peer.ip", "1.2.3.4"),
 				attribute.Int("net.peer.port", 56),
+				attribute.Int("net.host.port", 80),
 			},
 		},
 		{
@@ -373,7 +504,7 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 			},
 		},
 		{
-			name:       "with host ip and port in url",
+			name:       "with host ipv4 and port in url",
 			network:    "tcp",
 			method:     "GET",
 			requestURI: "http://4.3.2.1:78/user/123",
@@ -393,11 +524,39 @@ func TestNetAttributesFromHTTPRequest(t *testing.T) {
 				attribute.Int("net.host.port", 78),
 			},
 		},
+		{
+			name:       "with host ipv6 and port in url",
+			network:    "tcp",
+			method:     "GET",
+			requestURI: "http://4.3.2.1:78/user/123",
+			proto:      "HTTP/1.0",
+			remoteAddr: "1.2.3.4:56",
+			host:       "",
+			url: &url.URL{
+				Host: "[fe80::202:b3ff:fe1e:8329]:78",
+				Path: "/user/123",
+			},
+			header: nil,
+			expected: []attribute.KeyValue{
+				attribute.String("net.transport", "ip_tcp"),
+				attribute.String("net.peer.ip", "1.2.3.4"),
+				attribute.Int("net.peer.port", 56),
+				attribute.String("net.host.ip", "fe80::202:b3ff:fe1e:8329"),
+				attribute.Int("net.host.port", 78),
+			},
+		},
 	}
-	for idx, tc := range testcases {
+	for _, tc := range testcases {
-		r := testRequest(tc.method, tc.requestURI, tc.proto, tc.remoteAddr, tc.host, tc.url, tc.header, noTLS)
+		t.Run(tc.name, func(t *testing.T) {
-		got := NetAttributesFromHTTPRequest(tc.network, r)
+			r := testRequest(tc.method, tc.requestURI, tc.proto, tc.remoteAddr, tc.host, tc.url, tc.header, noTLS)
-		assertElementsMatch(t, tc.expected, got, "testcase %d - %s", idx, tc.name)
+			got := NetAttributesFromHTTPRequest(tc.network, r)
+			if diff := cmp.Diff(
+				tc.expected,
+				got,
+				cmp.AllowUnexported(attribute.Value{})); diff != "" {
+				t.Fatalf("attributes differ: diff %+v,", diff)
+			}
+		})
 	}
 }