[NETPATH-314] Fix Traceroute Latency (#29520)

pkg/networkpath/traceroute/tcp/utils.go

@@ -151,31 +151,30 @@ func listenPackets(icmpConn rawConnWrapper, tcpConn rawConnWrapper, timeout time
 	var icmpIP net.IP
 	var tcpIP net.IP
 	var icmpCode layers.ICMPv4TypeCode
+	var tcpFinished time.Time
+	var icmpFinished time.Time
 	var port uint16
 	wg.Add(2)
 	ctx, cancel := context.WithTimeout(context.Background(), timeout)
 	defer cancel()
 	go func() {
 		defer wg.Done()
 		defer cancel()
-		tcpIP, port, _, tcpErr = handlePackets(ctx, tcpConn, "tcp", localIP, localPort, remoteIP, remotePort, seqNum)
+		tcpIP, port, _, tcpFinished, tcpErr = handlePackets(ctx, tcpConn, "tcp", localIP, localPort, remoteIP, remotePort, seqNum)
 	}()
 	go func() {
 		defer wg.Done()
 		defer cancel()
-		icmpIP, _, icmpCode, icmpErr = handlePackets(ctx, icmpConn, "icmp", localIP, localPort, remoteIP, remotePort, seqNum)
+		icmpIP, _, icmpCode, icmpFinished, icmpErr = handlePackets(ctx, icmpConn, "icmp", localIP, localPort, remoteIP, remotePort, seqNum)
 	}()
 	wg.Wait()
-	// TODO: while this is okay, we
-	// should do this more cleanly
-	finished := time.Now()
 
 	if tcpErr != nil && icmpErr != nil {
 		_, tcpCanceled := tcpErr.(canceledError)
 		_, icmpCanceled := icmpErr.(canceledError)
 		if icmpCanceled && tcpCanceled {
 			log.Trace("timed out waiting for responses")
-			return net.IP{}, 0, 0, finished, nil
+			return net.IP{}, 0, 0, time.Time{}, nil
 		}
 		if tcpErr != nil {
 			log.Errorf("TCP listener error: %s", tcpErr.Error())
@@ -184,34 +183,34 @@ func listenPackets(icmpConn rawConnWrapper, tcpConn rawConnWrapper, timeout time
 			log.Errorf("ICMP listener error: %s", icmpErr.Error())
 		}
 
-		return net.IP{}, 0, 0, finished, multierr.Append(fmt.Errorf("tcp error: %w", tcpErr), fmt.Errorf("icmp error: %w", icmpErr))
+		return net.IP{}, 0, 0, time.Time{}, multierr.Append(fmt.Errorf("tcp error: %w", tcpErr), fmt.Errorf("icmp error: %w", icmpErr))
 	}
 
 	// if there was an error for TCP, but not
 	// ICMP, return the ICMP response
 	if tcpErr != nil {
-		return icmpIP, port, icmpCode, finished, nil
+		return icmpIP, port, icmpCode, icmpFinished, nil
 	}
 
 	// return the TCP response
-	return tcpIP, port, 0, finished, nil
+	return tcpIP, port, 0, tcpFinished, nil
 }
 
 // handlePackets in its current implementation should listen for the first matching
 // packet on the connection and then return. If no packet is received within the
 // timeout or if the listener is canceled, it should return a canceledError
-func handlePackets(ctx context.Context, conn rawConnWrapper, listener string, localIP net.IP, localPort uint16, remoteIP net.IP, remotePort uint16, seqNum uint32) (net.IP, uint16, layers.ICMPv4TypeCode, error) {
+func handlePackets(ctx context.Context, conn rawConnWrapper, listener string, localIP net.IP, localPort uint16, remoteIP net.IP, remotePort uint16, seqNum uint32) (net.IP, uint16, layers.ICMPv4TypeCode, time.Time, error) {
 	buf := make([]byte, 1024)
 	for {
 		select {
 		case <-ctx.Done():
-			return net.IP{}, 0, 0, canceledError("listener canceled")
+			return net.IP{}, 0, 0, time.Time{}, canceledError("listener canceled")
 		default:
 		}
 		now := time.Now()
 		err := conn.SetReadDeadline(now.Add(time.Millisecond * 100))
 		if err != nil {
-			return net.IP{}, 0, 0, fmt.Errorf("failed to read: %w", err)
+			return net.IP{}, 0, 0, time.Time{}, fmt.Errorf("failed to read: %w", err)
 		}
 		header, packet, _, err := conn.ReadFrom(buf)
 		if err != nil {
@@ -220,8 +219,12 @@ func handlePackets(ctx context.Context, conn rawConnWrapper, listener string, lo
 					continue
 				}
 			}
-			return net.IP{}, 0, 0, err
+			return net.IP{}, 0, 0, time.Time{}, err
 		}
+		// once we have a packet, take a timestamp to know when
+		// the response was received, if it matches, we will
+		// return this timestamp
+		received := time.Now()
 		// TODO: remove listener constraint and parse all packets
 		// in the same function return a succinct struct here
 		if listener == "icmp" {
@@ -231,7 +234,7 @@ func handlePackets(ctx context.Context, conn rawConnWrapper, listener string, lo
 				continue
 			}
 			if icmpMatch(localIP, localPort, remoteIP, remotePort, seqNum, icmpResponse) {
-				return icmpResponse.SrcIP, 0, icmpResponse.TypeCode, nil
+				return icmpResponse.SrcIP, 0, icmpResponse.TypeCode, received, nil
 			}
 		} else if listener == "tcp" {
 			tcpResp, err := parseTCP(header, packet)
@@ -240,10 +243,10 @@ func handlePackets(ctx context.Context, conn rawConnWrapper, listener string, lo
 				continue
 			}
 			if tcpMatch(localIP, localPort, remoteIP, remotePort, seqNum, tcpResp) {
-				return tcpResp.SrcIP, uint16(tcpResp.TCPResponse.SrcPort), 0, nil
+				return tcpResp.SrcIP, uint16(tcpResp.TCPResponse.SrcPort), 0, received, nil
 			}
 		} else {
-			return net.IP{}, 0, 0, fmt.Errorf("unsupported listener type")
+			return net.IP{}, 0, 0, received, fmt.Errorf("unsupported listener type")
 		}
 	}
 }
@@ -258,7 +261,6 @@ func parseICMP(header *ipv4.Header, payload []byte) (*icmpResponse, error) {
 
 	if header.Protocol != IPProtoICMP || header.Version != 4 ||
 		header.Src == nil || header.Dst == nil {
-		log.Errorf("invalid IP header for ICMP packet")
 		return nil, fmt.Errorf("invalid IP header for ICMP packet: %+v", header)
 	}
 	icmpResponse.SrcIP = header.Src
@@ -312,7 +314,6 @@ func parseTCP(header *ipv4.Header, payload []byte) (*tcpResponse, error) {
 
 	if header.Protocol != IPProtoTCP || header.Version != 4 ||
 		header.Src == nil || header.Dst == nil {
-		log.Errorf("invalid IP header for TCP packet")
 		return nil, fmt.Errorf("invalid IP header for TCP packet: %+v", header)
 	}
 	tcpResponse.SrcIP = header.Src

pkg/networkpath/traceroute/tcp/utils_test.go

@@ -26,6 +26,9 @@ import (
 var (
 	srcIP = net.ParseIP("1.2.3.4")
 	dstIP = net.ParseIP("5.6.7.8")
+
+	innerSrcIP = net.ParseIP("10.0.0.1")
+	innerDstIP = net.ParseIP("192.168.1.1")
 )
 
 type (
@@ -47,6 +50,8 @@ type (
 )
 
 func Test_handlePackets(t *testing.T) {
+	_, tcpBytes := createMockTCPPacket(createMockIPv4Header(dstIP, srcIP, 6), createMockTCPLayer(443, 12345, 28394, 28395, true, true, true))
+
 	tt := []struct {
 		description string
 		// input
@@ -121,13 +126,47 @@ func Test_handlePackets(t *testing.T) {
 			listener: "tcp",
 			errMsg:   "canceled",
 		},
+		{
+			description: "successful ICMP parsing returns IP, port, and type code",
+			ctxTimeout:  500 * time.Millisecond,
+			conn: &mockRawConn{
+				header:  createMockIPv4Header(srcIP, dstIP, 1),
+				payload: createMockICMPPacket(createMockICMPLayer(layers.ICMPv4CodeTTLExceeded), createMockIPv4Layer(innerSrcIP, innerDstIP, layers.IPProtocolTCP), createMockTCPLayer(12345, 443, 28394, 12737, true, true, true), false),
+			},
+			localIP:          innerSrcIP,
+			localPort:        12345,
+			remoteIP:         innerDstIP,
+			remotePort:       443,
+			seqNum:           28394,
+			listener:         "icmp",
+			expectedIP:       srcIP,
+			expectedPort:     0,
+			expectedTypeCode: layers.ICMPv4CodeTTLExceeded,
+		},
+		{
+			description: "successful TCP parsing returns IP, port, and type code",
+			ctxTimeout:  500 * time.Millisecond,
+			conn: &mockRawConn{
+				header:  createMockIPv4Header(dstIP, srcIP, 6),
+				payload: tcpBytes,
+			},
+			localIP:          srcIP,
+			localPort:        12345,
+			remoteIP:         dstIP,
+			remotePort:       443,
+			seqNum:           28394,
+			listener:         "tcp",
+			expectedIP:       dstIP,
+			expectedPort:     443,
+			expectedTypeCode: 0,
+		},
 	}
 
 	for _, test := range tt {
 		t.Run(test.description, func(t *testing.T) {
 			ctx, cancel := context.WithTimeout(context.Background(), test.ctxTimeout)
 			defer cancel()
-			actualIP, actualPort, actualTypeCode, err := handlePackets(ctx, test.conn, test.listener, test.localIP, test.localPort, test.remoteIP, test.remotePort, test.seqNum)
+			actualIP, actualPort, actualTypeCode, _, err := handlePackets(ctx, test.conn, test.listener, test.localIP, test.localPort, test.remoteIP, test.remotePort, test.seqNum)
 			if test.errMsg != "" {
 				require.Error(t, err)
 				assert.True(t, strings.Contains(err.Error(), test.errMsg))
@@ -142,8 +181,6 @@ func Test_handlePackets(t *testing.T) {
 }
 
 func Test_parseICMP(t *testing.T) {
-	innerSrcIP := net.ParseIP("10.0.0.1")
-	innerDstIP := net.ParseIP("192.168.1.1")
 	ipv4Header := createMockIPv4Header(srcIP, dstIP, 1)
 	icmpLayer := createMockICMPLayer(layers.ICMPv4CodeTTLExceeded)
 	innerIPv4Layer := createMockIPv4Layer(innerSrcIP, innerDstIP, layers.IPProtocolTCP)

releasenotes/notes/network-path-latency-fix-575efe1aa26c250b.yaml

@@ -0,0 +1,11 @@
+# Each section from every release note are combined when the
+# CHANGELOG.rst is rendered. So the text needs to be worded so that
+# it does not depend on any information only available in another
+# section. This may mean repeating some details, but each section
+# must be readable independently of the other.
+#
+# Each section note must be formatted as reStructuredText.
+---
+fixes:
+  - |
+    Fixes an issue where TCP traceroute latency was not being calculated correctly.