From 1ef109c0426ecaf8f6d90afadbdcf374676f932a Mon Sep 17 00:00:00 2001 From: Luis Neto Date: Tue, 3 Dec 2024 04:56:28 -0800 Subject: [PATCH] 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 | 3 + diagnostic/network/collector_unix.go | 4 + diagnostic/network/collector_unix_test.go | 68 +++++++++--- diagnostic/network/collector_utils.go | 25 +++-- diagnostic/network/collector_windows.go | 28 +++-- diagnostic/network/collector_windows_test.go | 105 ++++++++++++++++--- 6 files changed, 186 insertions(+), 47 deletions(-) diff --git a/diagnostic/network/collector.go b/diagnostic/network/collector.go index 5b742060..8a3a0fd9 100644 --- a/diagnostic/network/collector.go +++ b/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 diff --git a/diagnostic/network/collector_unix.go b/diagnostic/network/collector_unix.go index 60cfdf89..2db2d262 100644 --- a/diagnostic/network/collector_unix.go +++ b/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 } diff --git a/diagnostic/network/collector_unix_test.go b/diagnostic/network/collector_unix_test.go index 73b7d7dd..5ec231a3 100644 --- a/diagnostic/network/collector_unix_test.go +++ b/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 * * *`, - nil, - true, +someletters * * * +4 172.68.101.121 (172.68.101.121) 12.874 ms 15.517 ms 15.311 ms `, + []*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, - 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), + }, + ), + }, }, { "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) - } + require.NoError(t, err) + assert.Equal(t, test.expectedHops, hops) }) } } diff --git a/diagnostic/network/collector_utils.go b/diagnostic/network/collector_utils.go index 4505dd91..f897fda6 100644 --- a/diagnostic/network/collector_utils.go +++ b/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) diff --git a/diagnostic/network/collector_windows.go b/diagnostic/network/collector_windows.go index d590ac53..fe91a9de 100644 --- a/diagnostic/network/collector_windows.go +++ b/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 } diff --git a/diagnostic/network/collector_windows_test.go b/diagnostic/network/collector_windows_test.go index c591d2cb..3338a8bc 100644 --- a/diagnostic/network/collector_windows_test.go +++ b/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, - 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), + }, + ), + }, }, { "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, - 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), + }, + ), + }, }, { "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) - } + require.NoError(t, err) + assert.Equal(t, test.expectedHops, hops) }) } }