From 996504b3a1169898ceeaa2e349a589efeca4cde8 Mon Sep 17 00:00:00 2001 From: Evan Torrie Date: Wed, 13 Oct 2021 15:36:45 -0700 Subject: [PATCH 1/6] Fix IPv6 handling errors in semconv.NetAttributesFromHTTPRequest fixes #2283 --- CHANGELOG.md | 4 + semconv/v1.4.0/http.go | 80 ++++++++-------- semconv/v1.4.0/http_test.go | 179 ++++++++++++++++++++++++++++++++++-- 3 files changed, 209 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab09da29b7f..18bcd2a0a89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,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) +### Fixed + +- `semconv.NetAttributesFromHTTPRequest()` correctly handles IPv6 addresses. (#) + ## [1.0.1] - 2021-10-01 ### Fixed diff --git a/semconv/v1.4.0/http.go b/semconv/v1.4.0/http.go index 569e3c6450d..52f521fba5c 100644 --- a/semconv/v1.4.0/http.go +++ b/semconv/v1.4.0/http.go @@ -52,30 +52,7 @@ func NetAttributesFromHTTPRequest(network string, request *http.Request) []attri } peerName, peerIP, peerPort := "", "", 0 - { - hostPart := request.RemoteAddr - 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 = "", "" - } - } - } - } + peerIP, peerName, peerPort = hostIPNamePort(request.RemoteAddr) if peerName != "" { attrs = append(attrs, NetPeerNameKey.String(peerName)) } @@ -88,27 +65,9 @@ func NetAttributesFromHTTPRequest(network string, request *http.Request) []attri hostIP, hostName, hostPort := "", "", 0 for _, someHost := range []string{request.Host, request.Header.Get("Host"), request.URL.Host} { - hostPart := "" - if idx := strings.LastIndex(someHost, ":"); idx >= 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 - } + hostIP, hostName, hostPort = hostIPNamePort(someHost) + if hostIP != "" || hostName != "" || hostPort != 0 { break - } else { - hostPort = 0 } } if hostIP != "" { @@ -124,6 +83,39 @@ 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) { + hostPart, portPart := hostWithPort, "" + if idx := strings.LastIndexByte(hostWithPort, ':'); idx >= 0 { + ipv6WithPort := idx > 0 && hostWithPort[0] == '[' && hostWithPort[idx-1] == ']' + ipv4WithPort := idx > 0 && strings.IndexByte(hostWithPort[0:idx-1], ':') < 0 + if ipv6WithPort || ipv4WithPort { + hostPart, portPart, _ = net.SplitHostPort(hostWithPort) + } else if idx == 0 { + hostPart, portPart = "", hostWithPort[idx+1:] + } + } + if hostPart != "" { + if parsedIP := net.ParseIP(hostPart); parsedIP != nil { + ip = parsedIP.String() + } else { + name = hostPart + } + } + if portPart != "" { + numPort, err := strconv.ParseUint(portPart, 10, 16) + if err == nil { + port = (int)(numPort) + } else { + port = 0 + } + } + return +} + // EndUserAttributesFromHTTPRequest generates attributes of the // enduser namespace as specified by the OpenTelemetry specification // for a span. diff --git a/semconv/v1.4.0/http_test.go b/semconv/v1.4.0/http_test.go index 99498705c42..64d3994d014 100644 --- a/semconv/v1.4.0/http_test.go +++ b/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 { - r := testRequest(tc.method, tc.requestURI, tc.proto, tc.remoteAddr, tc.host, tc.url, tc.header, noTLS) - got := NetAttributesFromHTTPRequest(tc.network, r) - assertElementsMatch(t, tc.expected, got, "testcase %d - %s", idx, tc.name) + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + r := testRequest(tc.method, tc.requestURI, tc.proto, tc.remoteAddr, tc.host, tc.url, tc.header, noTLS) + got := NetAttributesFromHTTPRequest(tc.network, r) + if diff := cmp.Diff( + tc.expected, + got, + cmp.AllowUnexported(attribute.Value{})); diff != "" { + t.Fatalf("attributes differ: diff %+v,", diff) + } + }) } } From e674a99984b4263eb45321d39f94bffa1a63e020 Mon Sep 17 00:00:00 2001 From: Evan Torrie Date: Wed, 13 Oct 2021 15:39:46 -0700 Subject: [PATCH 2/6] Enter PR number in CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 18bcd2a0a89..0df875de25b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed -- `semconv.NetAttributesFromHTTPRequest()` correctly handles IPv6 addresses. (#) +- `semconv.NetAttributesFromHTTPRequest()` correctly handles IPv6 addresses. (#2285) ## [1.0.1] - 2021-10-01 From d4017e22f8046dd991b1d757d1295a8ad6b76d24 Mon Sep 17 00:00:00 2001 From: Evan Torrie Date: Wed, 13 Oct 2021 15:48:59 -0700 Subject: [PATCH 3/6] Remove unnecessary creation and then assignment Standardize order of checks for IP, Name, Port --- semconv/v1.4.0/http.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/semconv/v1.4.0/http.go b/semconv/v1.4.0/http.go index 52f521fba5c..30ddf757879 100644 --- a/semconv/v1.4.0/http.go +++ b/semconv/v1.4.0/http.go @@ -51,14 +51,13 @@ func NetAttributesFromHTTPRequest(network string, request *http.Request) []attri attrs = append(attrs, NetTransportOther) } - peerName, peerIP, peerPort := "", "", 0 - peerIP, peerName, peerPort = hostIPNamePort(request.RemoteAddr) - if peerName != "" { - attrs = append(attrs, NetPeerNameKey.String(peerName)) - } + peerIP, peerName, peerPort := hostIPNamePort(request.RemoteAddr) if peerIP != "" { attrs = append(attrs, NetPeerIPKey.String(peerIP)) } + if peerName != "" { + attrs = append(attrs, NetPeerNameKey.String(peerName)) + } if peerPort != 0 { attrs = append(attrs, NetPeerPortKey.Int(peerPort)) } From 1335150666a721b8b87ecc85fbad0eb873270cd4 Mon Sep 17 00:00:00 2001 From: Evan Torrie Date: Wed, 13 Oct 2021 23:59:18 -0700 Subject: [PATCH 4/6] Assume happy path when parsing host and port i.e. assume net.SplitHostPort(input) will succeed --- semconv/v1.4.0/http.go | 43 ++++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 27 deletions(-) diff --git a/semconv/v1.4.0/http.go b/semconv/v1.4.0/http.go index 30ddf757879..2d4721ff041 100644 --- a/semconv/v1.4.0/http.go +++ b/semconv/v1.4.0/http.go @@ -19,7 +19,6 @@ import ( "net" "net/http" "strconv" - "strings" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" @@ -59,10 +58,10 @@ func NetAttributesFromHTTPRequest(network string, request *http.Request) []attri attrs = append(attrs, NetPeerNameKey.String(peerName)) } if peerPort != 0 { - attrs = append(attrs, NetPeerPortKey.Int(peerPort)) + attrs = append(attrs, NetPeerPortKey.Int64(int64(peerPort))) } - hostIP, hostName, hostPort := "", "", 0 + hostIP, hostName, hostPort := "", "", uint64(0) for _, someHost := range []string{request.Host, request.Header.Get("Host"), request.URL.Host} { hostIP, hostName, hostPort = hostIPNamePort(someHost) if hostIP != "" || hostName != "" || hostPort != 0 { @@ -76,7 +75,7 @@ func NetAttributesFromHTTPRequest(network string, request *http.Request) []attri attrs = append(attrs, NetHostNameKey.String(hostName)) } if hostPort != 0 { - attrs = append(attrs, NetHostPortKey.Int(hostPort)) + attrs = append(attrs, NetHostPortKey.Int64(int64(hostPort))) } return attrs @@ -86,31 +85,21 @@ func NetAttributesFromHTTPRequest(network string, request *http.Request) []attri // 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) { - hostPart, portPart := hostWithPort, "" - if idx := strings.LastIndexByte(hostWithPort, ':'); idx >= 0 { - ipv6WithPort := idx > 0 && hostWithPort[0] == '[' && hostWithPort[idx-1] == ']' - ipv4WithPort := idx > 0 && strings.IndexByte(hostWithPort[0:idx-1], ':') < 0 - if ipv6WithPort || ipv4WithPort { - hostPart, portPart, _ = net.SplitHostPort(hostWithPort) - } else if idx == 0 { - hostPart, portPart = "", hostWithPort[idx+1:] - } +func hostIPNamePort(hostWithPort string) (ip string, name string, port uint64) { + var ( + hostPart, portPart string + err error + ) + if hostPart, portPart, err = net.SplitHostPort(hostWithPort); err != nil { + hostPart, portPart = hostWithPort, "" } - if hostPart != "" { - if parsedIP := net.ParseIP(hostPart); parsedIP != nil { - ip = parsedIP.String() - } else { - name = hostPart - } + if parsedIP := net.ParseIP(hostPart); parsedIP != nil { + ip = parsedIP.String() + } else { + name = hostPart } - if portPart != "" { - numPort, err := strconv.ParseUint(portPart, 10, 16) - if err == nil { - port = (int)(numPort) - } else { - port = 0 - } + if port, err = strconv.ParseUint(portPart, 10, 16); err != nil { + port = 0 } return } From 19584489831da86e3a36ffe97316f4cc1dc85d1b Mon Sep 17 00:00:00 2001 From: Evan Torrie Date: Thu, 14 Oct 2021 01:09:28 -0700 Subject: [PATCH 5/6] Get rid of uint64 for port --- semconv/v1.4.0/http.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/semconv/v1.4.0/http.go b/semconv/v1.4.0/http.go index 2d4721ff041..34c4b0bdc50 100644 --- a/semconv/v1.4.0/http.go +++ b/semconv/v1.4.0/http.go @@ -58,10 +58,10 @@ func NetAttributesFromHTTPRequest(network string, request *http.Request) []attri attrs = append(attrs, NetPeerNameKey.String(peerName)) } if peerPort != 0 { - attrs = append(attrs, NetPeerPortKey.Int64(int64(peerPort))) + attrs = append(attrs, NetPeerPortKey.Int(peerPort)) } - hostIP, hostName, hostPort := "", "", uint64(0) + hostIP, hostName, hostPort := "", "", 0 for _, someHost := range []string{request.Host, request.Header.Get("Host"), request.URL.Host} { hostIP, hostName, hostPort = hostIPNamePort(someHost) if hostIP != "" || hostName != "" || hostPort != 0 { @@ -75,7 +75,7 @@ func NetAttributesFromHTTPRequest(network string, request *http.Request) []attri attrs = append(attrs, NetHostNameKey.String(hostName)) } if hostPort != 0 { - attrs = append(attrs, NetHostPortKey.Int64(int64(hostPort))) + attrs = append(attrs, NetHostPortKey.Int(hostPort)) } return attrs @@ -85,9 +85,10 @@ func NetAttributesFromHTTPRequest(network string, request *http.Request) []attri // 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 uint64) { +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 { @@ -98,8 +99,8 @@ func hostIPNamePort(hostWithPort string) (ip string, name string, port uint64) { } else { name = hostPart } - if port, err = strconv.ParseUint(portPart, 10, 16); err != nil { - port = 0 + if parsedPort, err = strconv.ParseUint(portPart, 10, 16); err == nil { + port = int(parsedPort) } return } From 69b88ab9bb05ddf82238c99471b23e3c32ff1d73 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 18 Oct 2021 13:02:34 -0700 Subject: [PATCH 6/6] Fix git merge of main by adding back strings import --- semconv/v1.4.0/http.go | 1 + 1 file changed, 1 insertion(+) diff --git a/semconv/v1.4.0/http.go b/semconv/v1.4.0/http.go index 87c27e53c26..7340b229e2a 100644 --- a/semconv/v1.4.0/http.go +++ b/semconv/v1.4.0/http.go @@ -19,6 +19,7 @@ import ( "net" "net/http" "strconv" + "strings" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes"