TUN-8762: fix argument order when invoking tracert and modify network info output parsing.

## Summary

The windows code path has three bugs:

* the -4 and -6 option cannot be passed in the last position
* since there are some lines in the command output that are not parsable the collection fails to parse any kind of output
* the timeout hop is not correctly parsed

This PR also guards the parsing code against empty domains

Closes TUN-8762

diagnostic/network/collector.go

@@ -2,11 +2,14 @@ package diagnostic
 
 import (
 	"context"
+	"errors"
 	"time"
 )
 
 const MicrosecondsFactor = 1000.0
 
+var ErrEmptyDomain = errors.New("domain must not be empty")
+
 // For now only support ICMP is provided.
 type IPVersion int
 

diagnostic/network/collector_unix.go

@@ -68,7 +68,11 @@ func DecodeLine(text string) (*Hop, error) {
 			rtts = append(rtts, time.Duration(rtt*MicrosecondsFactor))
 		}
 	}
+
 	domain, _ = strings.CutSuffix(domain, " ")
+	if domain == "" {
+		return nil, ErrEmptyDomain
+	}
 
 	return NewHop(uint8(index), domain, rtts), nil
 }

diagnostic/network/collector_unix_test.go

@@ -20,23 +20,68 @@ func TestDecode(t *testing.T) {
 		name         string
 		text         string
 		expectedHops []*diagnostic.Hop
-		expectErr    bool
 	}{
 		{
 			"repeated hop index parse failure",
 			`1  172.68.101.121 (172.68.101.121)  12.874 ms  15.517 ms  15.311 ms
 2  172.68.101.121 (172.68.101.121)  12.874 ms  15.517 ms  15.311 ms
-someletters * * *`,
+someletters * * *
-			nil,
+4  172.68.101.121 (172.68.101.121)  12.874 ms  15.517 ms  15.311 ms `,
-			true,
+			[]*diagnostic.Hop{
+				diagnostic.NewHop(
+					uint8(1),
+					"172.68.101.121 (172.68.101.121)",
+					[]time.Duration{
+						time.Duration(12874),
+						time.Duration(15517),
+						time.Duration(15311),
+					},
+				),
+				diagnostic.NewHop(
+					uint8(2),
+					"172.68.101.121 (172.68.101.121)",
+					[]time.Duration{
+						time.Duration(12874),
+						time.Duration(15517),
+						time.Duration(15311),
+					},
+				),
+				diagnostic.NewHop(
+					uint8(4),
+					"172.68.101.121 (172.68.101.121)",
+					[]time.Duration{
+						time.Duration(12874),
+						time.Duration(15517),
+						time.Duration(15311),
+					},
+				),
+			},
 		},
 		{
 			"hop index parse failure",
 			`1  172.68.101.121 (172.68.101.121)  12.874 ms  15.517 ms  15.311 ms
 2  172.68.101.121 (172.68.101.121)  12.874 ms  15.517 ms  15.311 ms
 someletters 8.8.8.8 8.8.8.9 abc ms 0.456 ms 0.789 ms`,
-			nil,
+			[]*diagnostic.Hop{
-			true,
+				diagnostic.NewHop(
+					uint8(1),
+					"172.68.101.121 (172.68.101.121)",
+					[]time.Duration{
+						time.Duration(12874),
+						time.Duration(15517),
+						time.Duration(15311),
+					},
+				),
+				diagnostic.NewHop(
+					uint8(2),
+					"172.68.101.121 (172.68.101.121)",
+					[]time.Duration{
+						time.Duration(12874),
+						time.Duration(15517),
+						time.Duration(15311),
+					},
+				),
+			},
 		},
 		{
 			"missing rtt",
@@ -61,7 +106,6 @@ someletters 8.8.8.8 8.8.8.9 abc ms 0.456 ms 0.789 ms`,
 					},
 				),
 			},
-			false,
 		},
 		{
 			"simple example ipv4",
@@ -89,7 +133,6 @@ someletters 8.8.8.8 8.8.8.9 abc ms 0.456 ms 0.789 ms`,
 				),
 				diagnostic.NewTimeoutHop(uint8(3)),
 			},
-			false,
 		},
 		{
 			"simple example ipv6",
@@ -115,7 +158,6 @@ someletters 8.8.8.8 8.8.8.9 abc ms 0.456 ms 0.789 ms`,
 					},
 				),
 			},
-			false,
 		},
 	}
 
@@ -124,12 +166,8 @@ someletters 8.8.8.8 8.8.8.9 abc ms 0.456 ms 0.789 ms`,
 			t.Parallel()
 
 			hops, err := diagnostic.Decode(strings.NewReader(test.text), diagnostic.DecodeLine)
-			if test.expectErr {
-				require.Error(t, err)
-			} else {
 			require.NoError(t, err)
 			assert.Equal(t, test.expectedHops, hops)
-			}
 		})
 	}
 }

diagnostic/network/collector_utils.go

@@ -10,7 +10,7 @@ import (
 
 type DecodeLineFunc func(text string) (*Hop, error)
 
-func decodeNetworkOutputToFile(command *exec.Cmd, fn DecodeLineFunc) ([]*Hop, string, error) {
+func decodeNetworkOutputToFile(command *exec.Cmd, decodeLine DecodeLineFunc) ([]*Hop, string, error) {
 	stdout, err := command.StdoutPipe()
 	if err != nil {
 		return nil, "", fmt.Errorf("error piping traceroute's output: %w", err)
@@ -26,32 +26,41 @@ func decodeNetworkOutputToFile(command *exec.Cmd, fn DecodeLineFunc) ([]*Hop, st
 	// otherwise the process can become a zombie
 	buf := bytes.NewBuffer([]byte{})
 	tee := io.TeeReader(stdout, buf)
-	hops, err := Decode(tee, fn)
+	hops, err := Decode(tee, decodeLine)
+	// regardless of success of the decoding
+	// consume all output to have available in buf
+	_, _ = io.ReadAll(tee)
 
 	if werr := command.Wait(); werr != nil {
 		return nil, "", fmt.Errorf("error finishing traceroute: %w", werr)
 	}
 
 	if err != nil {
-		// consume all output to have available in buf
-		io.ReadAll(tee)
-		// This is already a TracerouteError no need to wrap it
 		return nil, buf.String(), err
 	}
 
 	return hops, "", nil
 }
 
-func Decode(reader io.Reader, fn DecodeLineFunc) ([]*Hop, error) {
+func Decode(reader io.Reader, decodeLine DecodeLineFunc) ([]*Hop, error) {
 	scanner := bufio.NewScanner(reader)
 	scanner.Split(bufio.ScanLines)
 
 	var hops []*Hop
+
 	for scanner.Scan() {
 		text := scanner.Text()
-		hop, err := fn(text)
+		if text == "" {
+			continue
+		}
+
+		hop, err := decodeLine(text)
 		if err != nil {
-			return nil, fmt.Errorf("error decoding output line: %w", err)
+			// This continue is here on the error case because there are lines at the start and end
+			// that may not be parsable. (check windows tracert output)
+			// The skip is here because aside from the start and end lines the other lines should
+			// always be parsable without errors.
+			continue
 		}
 
 		hops = append(hops, hop)

diagnostic/network/collector_windows.go

@@ -14,7 +14,13 @@ import (
 type NetworkCollectorImpl struct{}
 
 func (tracer *NetworkCollectorImpl) Collect(ctx context.Context, options TraceOptions) ([]*Hop, string, error) {
+	ipversion := "-4"
+	if !options.useV4 {
+		ipversion = "-6"
+	}
+
 	args := []string{
+		ipversion,
 		"-w",
 		strconv.FormatInt(int64(options.timeout.Seconds()), 10),
 		"-h",
@@ -23,17 +29,14 @@ func (tracer *NetworkCollectorImpl) Collect(ctx context.Context, options TraceOp
 		"-d",
 		options.address,
 	}
-	if options.useV4 {
-		args = append(args, "-4")
-	} else {
-		args = append(args, "-6")
-	}
 	command := exec.CommandContext(ctx, "tracert.exe", args...)
 
 	return decodeNetworkOutputToFile(command, DecodeLine)
 }
 
 func DecodeLine(text string) (*Hop, error) {
+	const requestTimedOut = "Request timed out."
+
 	fields := strings.Fields(text)
 	parts := []string{}
 	filter := func(s string) bool { return s != "*" && s != "ms" }
@@ -49,10 +52,6 @@ func DecodeLine(text string) (*Hop, error) {
 		return nil, fmt.Errorf("couldn't parse index from timeout hop: %w", err)
 	}
 
-	if len(parts) == 1 {
-		return NewTimeoutHop(uint8(index)), nil
-	}
-
 	domain := ""
 	rtts := []time.Duration{}
 
@@ -66,6 +65,17 @@ func DecodeLine(text string) (*Hop, error) {
 			rtts = append(rtts, time.Duration(rtt*MicrosecondsFactor))
 		}
 	}
+
 	domain, _ = strings.CutSuffix(domain, " ")
+	// If the domain is equal to "Request timed out." then we build a
+	// timeout hop.
+	if domain == requestTimedOut {
+		return NewTimeoutHop(uint8(index)), nil
+	}
+
+	if domain == "" {
+		return nil, ErrEmptyDomain
+	}
+
 	return NewHop(uint8(index), domain, rtts), nil
 }

diagnostic/network/collector_windows_test.go

@@ -20,23 +20,105 @@ func TestDecode(t *testing.T) {
 		name         string
 		text         string
 		expectedHops []*diagnostic.Hop
-		expectErr    bool
 	}{
+
+		{
+			"tracert output",
+			`
+Tracing route to region2.v2.argotunnel.com [198.41.200.73]
+over a maximum of 5 hops:
+
+  1    10 ms    <1 ms     1 ms  192.168.64.1
+  2    27 ms    14 ms     5 ms  192.168.1.254
+  3     *        *        *     Request timed out.
+  4     *        *        *     Request timed out.
+  5    27 ms     5 ms     5 ms  195.8.30.245
+
+Trace complete.
+`,
+			[]*diagnostic.Hop{
+				diagnostic.NewHop(
+					uint8(1),
+					"192.168.64.1",
+					[]time.Duration{
+						time.Duration(10000),
+						time.Duration(1000),
+						time.Duration(1000),
+					},
+				),
+				diagnostic.NewHop(
+					uint8(2),
+					"192.168.1.254",
+					[]time.Duration{
+						time.Duration(27000),
+						time.Duration(14000),
+						time.Duration(5000),
+					},
+				),
+				diagnostic.NewTimeoutHop(uint8(3)),
+				diagnostic.NewTimeoutHop(uint8(4)),
+				diagnostic.NewHop(
+					uint8(5),
+					"195.8.30.245",
+					[]time.Duration{
+						time.Duration(27000),
+						time.Duration(5000),
+						time.Duration(5000),
+					},
+				),
+			},
+		},
 		{
 			"repeated hop index parse failure",
 			`1	12.874 ms	15.517 ms	15.311 ms	172.68.101.121 (172.68.101.121)  
 2	12.874 ms	15.517 ms	15.311 ms	172.68.101.121 (172.68.101.121)  
 someletters * * *`,
-			nil,
+			[]*diagnostic.Hop{
-			true,
+				diagnostic.NewHop(
+					uint8(1),
+					"172.68.101.121 (172.68.101.121)",
+					[]time.Duration{
+						time.Duration(12874),
+						time.Duration(15517),
+						time.Duration(15311),
+					},
+				),
+				diagnostic.NewHop(
+					uint8(2),
+					"172.68.101.121 (172.68.101.121)",
+					[]time.Duration{
+						time.Duration(12874),
+						time.Duration(15517),
+						time.Duration(15311),
+					},
+				),
+			},
 		},
 		{
 			"hop index parse failure",
 			`1	12.874 ms	15.517 ms	15.311 ms	172.68.101.121 (172.68.101.121)  
 2	12.874 ms	15.517 ms	15.311 ms	172.68.101.121 (172.68.101.121)  
 someletters abc ms 0.456 ms 0.789 ms 8.8.8.8 8.8.8.9`,
-			nil,
+			[]*diagnostic.Hop{
-			true,
+				diagnostic.NewHop(
+					uint8(1),
+					"172.68.101.121 (172.68.101.121)",
+					[]time.Duration{
+						time.Duration(12874),
+						time.Duration(15517),
+						time.Duration(15311),
+					},
+				),
+				diagnostic.NewHop(
+					uint8(2),
+					"172.68.101.121 (172.68.101.121)",
+					[]time.Duration{
+						time.Duration(12874),
+						time.Duration(15517),
+						time.Duration(15311),
+					},
+				),
+			},
 		},
 		{
 			"missing rtt",
@@ -61,13 +143,12 @@ someletters abc ms 0.456 ms 0.789 ms 8.8.8.8 8.8.8.9`,
 					},
 				),
 			},
-			false,
 		},
 		{
 			"simple example ipv4",
 			`1	12.874 ms	15.517 ms	15.311 ms	172.68.101.121 (172.68.101.121)  
 2	12.874 ms	15.517 ms	15.311 ms	172.68.101.121 (172.68.101.121)  
-3  * * *`,
+3  * * * Request timed out.`,
 			[]*diagnostic.Hop{
 				diagnostic.NewHop(
 					uint8(1),
@@ -89,7 +170,6 @@ someletters abc ms 0.456 ms 0.789 ms 8.8.8.8 8.8.8.9`,
 				),
 				diagnostic.NewTimeoutHop(uint8(3)),
 			},
-			false,
 		},
 		{
 			"simple example ipv6",
@@ -115,7 +195,6 @@ someletters abc ms 0.456 ms 0.789 ms 8.8.8.8 8.8.8.9`,
 					},
 				),
 			},
-			false,
 		},
 	}
 
@@ -124,12 +203,8 @@ someletters abc ms 0.456 ms 0.789 ms 8.8.8.8 8.8.8.9`,
 			t.Parallel()
 
 			hops, err := diagnostic.Decode(strings.NewReader(test.text), diagnostic.DecodeLine)
-			if test.expectErr {
-				require.Error(t, err)
-			} else {
 			require.NoError(t, err)
 			assert.Equal(t, test.expectedHops, hops)
-			}
 		})
 	}
 }