From 9ccec941ec793d61e15e2b0f40849b58413a9cf9 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Tue, 27 Aug 2024 09:06:11 +0000 Subject: [PATCH 1/5] feat(cli): add aws check to ping p2p diagnostics --- cli/cliui/agent.go | 39 ++++++--- cli/cliui/agent_test.go | 74 +++++++++++++++++ cli/cliutil/awscheck.go | 114 ++++++++++++++++++++++++++ cli/cliutil/awscheck_internal_test.go | 95 +++++++++++++++++++++ cli/ping.go | 33 +++++++- 5 files changed, 344 insertions(+), 11 deletions(-) create mode 100644 cli/cliutil/awscheck.go create mode 100644 cli/cliutil/awscheck_internal_test.go diff --git a/cli/cliui/agent.go b/cli/cliui/agent.go index a0079d3b3c81e..dd71f964a65e0 100644 --- a/cli/cliui/agent.go +++ b/cli/cliui/agent.go @@ -357,10 +357,18 @@ type ConnDiags struct { LocalNetInfo *tailcfg.NetInfo LocalInterfaces *healthsdk.InterfacesReport AgentNetcheck *healthsdk.AgentNetcheckReport + ClientIPIsAWS bool + AgentIPIsAWS bool // TODO: More diagnostics } func ConnDiagnostics(w io.Writer, d ConnDiags) { + if d.PingP2P { + _, _ = fmt.Fprint(w, "✔ You are connected directly (p2p)\n") + } else { + _, _ = fmt.Fprint(w, "❗ You are connected via a DERP relay, not directly (p2p)\n") + } + if d.AgentNetcheck != nil { for _, msg := range d.AgentNetcheck.Interfaces.Warnings { _, _ = fmt.Fprintf(w, "❗ Agent: %s\n", msg.Message) @@ -373,12 +381,6 @@ func ConnDiagnostics(w io.Writer, d ConnDiags) { } } - if d.PingP2P { - _, _ = fmt.Fprint(w, "✔ You are connected directly (p2p)\n") - return - } - _, _ = fmt.Fprint(w, "❗ You are connected via a DERP relay, not directly (p2p)\n") - if d.DisableDirect { _, _ = fmt.Fprint(w, "❗ Direct connections are disabled locally, by `--disable-direct` or `CODER_DISABLE_DIRECT`\n") return @@ -389,15 +391,32 @@ func ConnDiagnostics(w io.Writer, d ConnDiags) { return } - if d.ConnInfo != nil && d.ConnInfo.DERPMap != nil && !d.ConnInfo.DERPMap.HasSTUN() { - _, _ = fmt.Fprint(w, "✘ The DERP map is not configured to use STUN, which will prevent direct connections from starting outside of local networks\n") + if d.ConnInfo != nil && d.ConnInfo.DERPMap != nil { + if !d.ConnInfo.DERPMap.HasSTUN() { + _, _ = fmt.Fprint(w, "✘ The DERP map is not configured to use STUN, which will prevent direct connections from starting outside of local networks\n") + } else if d.LocalNetInfo != nil && !d.LocalNetInfo.UDP { + _, _ = fmt.Fprint(w, "❗ Client could not connect to STUN over UDP, which may be preventing a direct connection\n") + } } if d.LocalNetInfo != nil && d.LocalNetInfo.MappingVariesByDestIP.EqualBool(true) { _, _ = fmt.Fprint(w, "❗ Client is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers\n") } - if d.AgentNetcheck != nil && d.AgentNetcheck.NetInfo != nil && d.AgentNetcheck.NetInfo.MappingVariesByDestIP.EqualBool(true) { - _, _ = fmt.Fprint(w, "❗ Agent is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers\n") + if d.AgentNetcheck != nil && d.AgentNetcheck.NetInfo != nil { + if d.AgentNetcheck.NetInfo.MappingVariesByDestIP.EqualBool(true) { + _, _ = fmt.Fprint(w, "❗ Agent is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers\n") + } + if !d.AgentNetcheck.NetInfo.UDP { + _, _ = fmt.Fprint(w, "❗ Agent could not connect to STUN over UDP, which may be preventing a direct connection\n") + } + } + + if d.ClientIPIsAWS { + _, _ = fmt.Fprint(w, "❗ Client IP address is within an AWS range, which is known to cause problems with forming direct connections (AWS uses hard NAT)\n") + } + + if d.AgentIPIsAWS { + _, _ = fmt.Fprint(w, "❗ Agent IP address is within an AWS range, which is known to cause problems with forming direct connections (AWS uses hard NAT)\n") } } diff --git a/cli/cliui/agent_test.go b/cli/cliui/agent_test.go index fced0c67ea1b9..f93309af2a77e 100644 --- a/cli/cliui/agent_test.go +++ b/cli/cliui/agent_test.go @@ -719,6 +719,58 @@ func TestConnDiagnostics(t *testing.T) { `✘ The DERP map is not configured to use STUN, which will prevent direct connections from starting outside of local networks`, }, }, + { + name: "ClientHasStunNoUDP", + diags: cliui.ConnDiags{ + ConnInfo: &workspacesdk.AgentConnectionInfo{ + DERPMap: &tailcfg.DERPMap{ + Regions: map[int]*tailcfg.DERPRegion{ + 999: { + Nodes: []*tailcfg.DERPNode{ + { + STUNPort: 1337, + }, + }, + }, + }, + }, + }, + LocalNetInfo: &tailcfg.NetInfo{ + UDP: false, + }, + }, + want: []string{ + `❗ You are connected via a DERP relay, not directly (p2p)`, + `❗ Client could not connect to STUN over UDP, which may be preventing a direct connection`, + }, + }, + { + name: "AgentHasStunNoUDP", + diags: cliui.ConnDiags{ + ConnInfo: &workspacesdk.AgentConnectionInfo{ + DERPMap: &tailcfg.DERPMap{ + Regions: map[int]*tailcfg.DERPRegion{ + 999: { + Nodes: []*tailcfg.DERPNode{ + { + STUNPort: 1337, + }, + }, + }, + }, + }, + }, + AgentNetcheck: &healthsdk.AgentNetcheckReport{ + NetInfo: &tailcfg.NetInfo{ + UDP: false, + }, + }, + }, + want: []string{ + `❗ You are connected via a DERP relay, not directly (p2p)`, + `❗ Agent could not connect to STUN over UDP, which may be preventing a direct connection`, + }, + }, { name: "ClientHardNat", diags: cliui.ConnDiags{ @@ -782,6 +834,28 @@ func TestConnDiagnostics(t *testing.T) { `✔ You are connected directly (p2p)`, }, }, + { + name: "ClientAWSIP", + diags: cliui.ConnDiags{ + ClientIPIsAWS: true, + AgentIPIsAWS: false, + }, + want: []string{ + `❗ You are connected via a DERP relay, not directly (p2p)`, + `❗ Client IP address is within an AWS range, which is known to cause problems with forming direct connections (AWS uses hard NAT)`, + }, + }, + { + name: "AgentAWSIP", + diags: cliui.ConnDiags{ + ClientIPIsAWS: false, + AgentIPIsAWS: true, + }, + want: []string{ + `❗ You are connected via a DERP relay, not directly (p2p)`, + `❗ Agent IP address is within an AWS range, which is known to cause problems with forming direct connections (AWS uses hard NAT)`, + }, + }, } for _, tc := range testCases { tc := tc diff --git a/cli/cliutil/awscheck.go b/cli/cliutil/awscheck.go new file mode 100644 index 0000000000000..20a5960a45fb2 --- /dev/null +++ b/cli/cliutil/awscheck.go @@ -0,0 +1,114 @@ +package cliutil + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/netip" + "time" + + "golang.org/x/xerrors" +) + +const AWSIPRangesURL = "https://ip-ranges.amazonaws.com/ip-ranges.json" + +type awsIPv4Prefix struct { + Prefix string `json:"ip_prefix"` + Region string `json:"region"` + Service string `json:"service"` + NetworkBorderGroup string `json:"network_border_group"` +} + +type awsIPv6Prefix struct { + Prefix string `json:"ipv6_prefix"` + Region string `json:"region"` + Service string `json:"service"` + NetworkBorderGroup string `json:"network_border_group"` +} + +type AWSIPRanges struct { + V4 []netip.Prefix + V6 []netip.Prefix +} + +type awsIPRangesResponse struct { + SyncToken string `json:"syncToken"` + CreateDate string `json:"createDate"` + IPV4Prefixes []awsIPv4Prefix `json:"prefixes"` + IPV6Prefixes []awsIPv6Prefix `json:"ipv6_prefixes"` +} + +func FetchAWSIPRanges(ctx context.Context, url string) (*AWSIPRanges, error) { + client := &http.Client{} + reqCtx, reqCancel := context.WithTimeout(ctx, 5*time.Second) + defer reqCancel() + req, _ := http.NewRequestWithContext(reqCtx, http.MethodGet, url, nil) + resp, err := client.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + b, _ := io.ReadAll(resp.Body) + return nil, xerrors.Errorf("unexpected status code %d: %s", resp.StatusCode, b) + } + + var body awsIPRangesResponse + err = json.NewDecoder(resp.Body).Decode(&body) + if err != nil { + return nil, xerrors.Errorf("json decode: %w", err) + } + + out := &AWSIPRanges{ + V4: make([]netip.Prefix, 0, len(body.IPV4Prefixes)), + V6: make([]netip.Prefix, 0, len(body.IPV6Prefixes)), + } + + for _, p := range body.IPV4Prefixes { + prefix, err := netip.ParsePrefix(p.Prefix) + if err != nil { + return nil, xerrors.Errorf("parse ip prefix: %w", err) + } + if prefix.Addr().Is6() { + return nil, xerrors.Errorf("ipv4 prefix contains ipv6 address: %s", p.Prefix) + } + out.V4 = append(out.V4, prefix) + } + + for _, p := range body.IPV6Prefixes { + prefix, err := netip.ParsePrefix(p.Prefix) + if err != nil { + return nil, xerrors.Errorf("parse ip prefix: %w", err) + } + if prefix.Addr().Is4() { + return nil, xerrors.Errorf("ipv6 prefix contains ipv4 address: %s", p.Prefix) + } + out.V6 = append(out.V6, prefix) + } + + return out, nil +} + +// CheckIP checks if the given IP address is an AWS IP. +func (r *AWSIPRanges) CheckIP(ip netip.Addr) bool { + if ip.IsLoopback() || ip.IsLinkLocalMulticast() || ip.IsLinkLocalUnicast() || ip.IsPrivate() { + return false + } + + if ip.Is4() { + for _, p := range r.V4 { + if p.Contains(ip) { + return true + } + } + } else { + for _, p := range r.V6 { + if p.Contains(ip) { + return true + } + } + } + return false +} diff --git a/cli/cliutil/awscheck_internal_test.go b/cli/cliutil/awscheck_internal_test.go new file mode 100644 index 0000000000000..8688bf51e7e28 --- /dev/null +++ b/cli/cliutil/awscheck_internal_test.go @@ -0,0 +1,95 @@ +package cliutil + +import ( + "context" + "net/http" + "net/http/httptest" + "net/netip" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/testutil" +) + +func TestIPV4Check(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + httpapi.Write(context.Background(), w, http.StatusOK, awsIPRangesResponse{ + IPV4Prefixes: []awsIPv4Prefix{ + { + Prefix: "3.24.0.0/14", + }, + { + Prefix: "15.230.15.29/32", + }, + { + Prefix: "47.128.82.100/31", + }, + }, + IPV6Prefixes: []awsIPv6Prefix{ + { + Prefix: "2600:9000:5206::/48", + }, + { + Prefix: "2406:da70:8800::/40", + }, + { + Prefix: "2600:1f68:5000::/40", + }, + }, + }) + })) + ctx := testutil.Context(t, testutil.WaitShort) + ranges, err := FetchAWSIPRanges(ctx, srv.URL) + require.NoError(t, err) + + t.Run("Private/IPV4", func(t *testing.T) { + t.Parallel() + ip, err := netip.ParseAddr("192.168.0.1") + require.NoError(t, err) + isAws := ranges.CheckIP(ip) + require.False(t, isAws) + }) + + t.Run("AWS/IPV4", func(t *testing.T) { + t.Parallel() + ip, err := netip.ParseAddr("3.25.61.113") + require.NoError(t, err) + isAws := ranges.CheckIP(ip) + require.True(t, isAws) + }) + + t.Run("NonAWS/IPV4", func(t *testing.T) { + t.Parallel() + ip, err := netip.ParseAddr("159.196.123.40") + require.NoError(t, err) + isAws := ranges.CheckIP(ip) + require.False(t, isAws) + }) + + t.Run("Private/IPV6", func(t *testing.T) { + t.Parallel() + ip, err := netip.ParseAddr("::1") + require.NoError(t, err) + isAws := ranges.CheckIP(ip) + require.False(t, isAws) + }) + + t.Run("AWS/IPV6", func(t *testing.T) { + t.Parallel() + ip, err := netip.ParseAddr("2600:9000:5206:0001:0000:0000:0000:0001") + require.NoError(t, err) + isAws := ranges.CheckIP(ip) + require.True(t, isAws) + }) + + t.Run("NonAWS/IPV6", func(t *testing.T) { + t.Parallel() + ip, err := netip.ParseAddr("2403:5807:885f:0:a544:49d4:58f8:aedf") + require.NoError(t, err) + isAws := ranges.CheckIP(ip) + require.False(t, isAws) + }) +} diff --git a/cli/ping.go b/cli/ping.go index e36a518b41c7a..e20a220849d4d 100644 --- a/cli/ping.go +++ b/cli/ping.go @@ -5,9 +5,11 @@ import ( "errors" "fmt" "net/http" + "net/netip" "time" "golang.org/x/xerrors" + "tailscale.com/tailcfg" "cdr.dev/slog" "cdr.dev/slog/sloggers/sloghuman" @@ -15,6 +17,7 @@ import ( "github.com/coder/pretty" "github.com/coder/coder/v2/cli/cliui" + "github.com/coder/coder/v2/cli/cliutil" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/healthsdk" "github.com/coder/coder/v2/codersdk/workspacesdk" @@ -150,11 +153,20 @@ func (r *RootCmd) ping() *serpent.Command { diags := conn.GetPeerDiagnostics() cliui.PeerDiagnostics(inv.Stdout, diags) + ni := conn.GetNetInfo() connDiags := cliui.ConnDiags{ PingP2P: didP2p, DisableDirect: r.disableDirect, - LocalNetInfo: conn.GetNetInfo(), + LocalNetInfo: ni, } + + awsRanges, err := cliutil.FetchAWSIPRanges(ctx, cliutil.AWSIPRangesURL) + if err != nil { + _, _ = fmt.Fprintf(inv.Stdout, "Failed to retrieve AWS IP ranges: %v\n", err) + } + + connDiags.ClientIPIsAWS = isAWSIP(awsRanges, ni) + connInfo, err := client.AgentConnectionInfoGeneric(ctx) if err == nil { connDiags.ConnInfo = &connInfo @@ -167,9 +179,11 @@ func (r *RootCmd) ping() *serpent.Command { } else { _, _ = fmt.Fprintf(inv.Stdout, "Failed to retrieve local interfaces report: %v\n", err) } + agentNetcheck, err := conn.Netcheck(ctx) if err == nil { connDiags.AgentNetcheck = &agentNetcheck + connDiags.AgentIPIsAWS = isAWSIP(awsRanges, agentNetcheck.NetInfo) } else { var sdkErr *codersdk.Error if errors.As(err, &sdkErr) && sdkErr.StatusCode() == http.StatusNotFound { @@ -178,6 +192,7 @@ func (r *RootCmd) ping() *serpent.Command { _, _ = fmt.Fprintf(inv.Stdout, "Failed to retrieve connection report from agent: %v\n", err) } } + cliui.ConnDiagnostics(inv.Stdout, connDiags) return nil }, @@ -207,3 +222,19 @@ func (r *RootCmd) ping() *serpent.Command { } return cmd } + +func isAWSIP(awsRanges *cliutil.AWSIPRanges, ni *tailcfg.NetInfo) bool { + if ni.GlobalV4 != "" { + ip, err := netip.ParseAddr(ni.GlobalV4) + if err == nil && awsRanges.CheckIP(ip) { + return true + } + } + if ni.GlobalV6 != "" { + ip, err := netip.ParseAddr(ni.GlobalV6) + if err == nil && awsRanges.CheckIP(ip) { + return true + } + } + return false +} From f16e4e72ab3a2dfef9a278437ef795271d59a82b Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Thu, 29 Aug 2024 08:17:49 +0000 Subject: [PATCH 2/5] review --- cli/cliui/agent.go | 83 ++++++++++++++++++++++++++++++----------- cli/cliui/agent_test.go | 48 +++++++++++++++++------- cli/ping.go | 15 +++++--- 3 files changed, 104 insertions(+), 42 deletions(-) diff --git a/cli/cliui/agent.go b/cli/cliui/agent.go index dd71f964a65e0..660d2af45ebe8 100644 --- a/cli/cliui/agent.go +++ b/cli/cliui/agent.go @@ -7,11 +7,13 @@ import ( "strconv" "strings" "time" + "unicode" "github.com/google/uuid" "golang.org/x/xerrors" "tailscale.com/tailcfg" + "github.com/coder/coder/v2/coderd/healthcheck/health" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/healthsdk" "github.com/coder/coder/v2/codersdk/workspacesdk" @@ -351,7 +353,7 @@ func PeerDiagnostics(w io.Writer, d tailnet.PeerDiagnostics) { } type ConnDiags struct { - ConnInfo *workspacesdk.AgentConnectionInfo + ConnInfo workspacesdk.AgentConnectionInfo PingP2P bool DisableDirect bool LocalNetInfo *tailcfg.NetInfo @@ -359,64 +361,101 @@ type ConnDiags struct { AgentNetcheck *healthsdk.AgentNetcheckReport ClientIPIsAWS bool AgentIPIsAWS bool + Verbose bool // TODO: More diagnostics } -func ConnDiagnostics(w io.Writer, d ConnDiags) { +func (d ConnDiags) Write(w io.Writer) { + _, _ = fmt.Fprintln(w, "") + general, client, agent := d.splitDiagnostics() + for _, msg := range general { + _, _ = fmt.Fprintln(w, msg) + } + if len(client) > 0 { + _, _ = fmt.Fprint(w, "Possible client-side issues with direct connection:\n\n") + for _, msg := range client { + _, _ = fmt.Fprintf(w, " - %s\n\n", msg) + } + } + if len(agent) > 0 { + _, _ = fmt.Fprint(w, "Possible agent-side issues with direct connections:\n\n") + for _, msg := range agent { + _, _ = fmt.Fprintf(w, " - %s\n\n", msg) + } + } +} + +func (d ConnDiags) splitDiagnostics() (general, client, agent []string) { if d.PingP2P { - _, _ = fmt.Fprint(w, "✔ You are connected directly (p2p)\n") + general = append(general, "✔ You are connected directly (p2p)") } else { - _, _ = fmt.Fprint(w, "❗ You are connected via a DERP relay, not directly (p2p)\n") + general = append(general, "❗ You are connected via a DERP relay, not directly (p2p)") } if d.AgentNetcheck != nil { for _, msg := range d.AgentNetcheck.Interfaces.Warnings { - _, _ = fmt.Fprintf(w, "❗ Agent: %s\n", msg.Message) + agent = append(agent, formatHealthMessage(msg)) } } if d.LocalInterfaces != nil { for _, msg := range d.LocalInterfaces.Warnings { - _, _ = fmt.Fprintf(w, "❗ Client: %s\n", msg.Message) + client = append(client, formatHealthMessage(msg)) } } - if d.DisableDirect { - _, _ = fmt.Fprint(w, "❗ Direct connections are disabled locally, by `--disable-direct` or `CODER_DISABLE_DIRECT`\n") - return + if d.PingP2P && !d.Verbose { + return general, client, agent } - if d.ConnInfo != nil && d.ConnInfo.DisableDirectConnections { - _, _ = fmt.Fprint(w, "❗ Your Coder administrator has blocked direct connections\n") - return + if d.DisableDirect { + general = append(general, "❗ Direct connections are disabled locally, by `--disable-direct` or `CODER_DISABLE_DIRECT`") + if !d.Verbose { + return general, client, agent + } } - if d.ConnInfo != nil && d.ConnInfo.DERPMap != nil { - if !d.ConnInfo.DERPMap.HasSTUN() { - _, _ = fmt.Fprint(w, "✘ The DERP map is not configured to use STUN, which will prevent direct connections from starting outside of local networks\n") - } else if d.LocalNetInfo != nil && !d.LocalNetInfo.UDP { - _, _ = fmt.Fprint(w, "❗ Client could not connect to STUN over UDP, which may be preventing a direct connection\n") + if d.ConnInfo.DisableDirectConnections { + general = append(general, "❗ Your Coder administrator has blocked direct connections") + if !d.Verbose { + return general, client, agent } } + if !d.ConnInfo.DERPMap.HasSTUN() { + general = append(general, "✘ The DERP map is not configured to use STUN") + } else if d.LocalNetInfo != nil && !d.LocalNetInfo.UDP { + client = append(client, "✘ Client could not connect to STUN over UDP") + } + if d.LocalNetInfo != nil && d.LocalNetInfo.MappingVariesByDestIP.EqualBool(true) { - _, _ = fmt.Fprint(w, "❗ Client is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers\n") + client = append(client, "❗ Client is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers") } if d.AgentNetcheck != nil && d.AgentNetcheck.NetInfo != nil { if d.AgentNetcheck.NetInfo.MappingVariesByDestIP.EqualBool(true) { - _, _ = fmt.Fprint(w, "❗ Agent is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers\n") + agent = append(agent, "❗ Agent is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers") } if !d.AgentNetcheck.NetInfo.UDP { - _, _ = fmt.Fprint(w, "❗ Agent could not connect to STUN over UDP, which may be preventing a direct connection\n") + agent = append(agent, "✘ Agent could not connect to STUN over UDP") } } if d.ClientIPIsAWS { - _, _ = fmt.Fprint(w, "❗ Client IP address is within an AWS range, which is known to cause problems with forming direct connections (AWS uses hard NAT)\n") + client = append(client, "❗ Client IP address is within an AWS range (AWS uses hard NAT)") } if d.AgentIPIsAWS { - _, _ = fmt.Fprint(w, "❗ Agent IP address is within an AWS range, which is known to cause problems with forming direct connections (AWS uses hard NAT)\n") + agent = append(agent, "❗ Agent IP address is within an AWS range (AWS uses hard NAT)") + } + return general, client, agent +} + +func formatHealthMessage(msg health.Message) string { + if msg.Code != health.CodeInterfaceSmallMTU { + return msg.Message } + r := []rune(strings.Replace(msg.Message, ", which may cause problems with direct connections", "", -1)) + out := string(append([]rune{unicode.ToUpper(r[0])}, r[1:]...)) + return fmt.Sprintf("❗ %s, which may degrade the quality of direct connections", out) } diff --git a/cli/cliui/agent_test.go b/cli/cliui/agent_test.go index f93309af2a77e..cd05d7164775a 100644 --- a/cli/cliui/agent_test.go +++ b/cli/cliui/agent_test.go @@ -686,7 +686,9 @@ func TestConnDiagnostics(t *testing.T) { { name: "Direct", diags: cliui.ConnDiags{ - ConnInfo: &workspacesdk.AgentConnectionInfo{}, + ConnInfo: workspacesdk.AgentConnectionInfo{ + DERPMap: &tailcfg.DERPMap{}, + }, PingP2P: true, LocalNetInfo: &tailcfg.NetInfo{}, }, @@ -697,7 +699,8 @@ func TestConnDiagnostics(t *testing.T) { { name: "DirectBlocked", diags: cliui.ConnDiags{ - ConnInfo: &workspacesdk.AgentConnectionInfo{ + ConnInfo: workspacesdk.AgentConnectionInfo{ + DERPMap: &tailcfg.DERPMap{}, DisableDirectConnections: true, }, }, @@ -709,20 +712,20 @@ func TestConnDiagnostics(t *testing.T) { { name: "NoStun", diags: cliui.ConnDiags{ - ConnInfo: &workspacesdk.AgentConnectionInfo{ + ConnInfo: workspacesdk.AgentConnectionInfo{ DERPMap: &tailcfg.DERPMap{}, }, LocalNetInfo: &tailcfg.NetInfo{}, }, want: []string{ `❗ You are connected via a DERP relay, not directly (p2p)`, - `✘ The DERP map is not configured to use STUN, which will prevent direct connections from starting outside of local networks`, + `✘ The DERP map is not configured to use STUN`, }, }, { name: "ClientHasStunNoUDP", diags: cliui.ConnDiags{ - ConnInfo: &workspacesdk.AgentConnectionInfo{ + ConnInfo: workspacesdk.AgentConnectionInfo{ DERPMap: &tailcfg.DERPMap{ Regions: map[int]*tailcfg.DERPRegion{ 999: { @@ -741,13 +744,13 @@ func TestConnDiagnostics(t *testing.T) { }, want: []string{ `❗ You are connected via a DERP relay, not directly (p2p)`, - `❗ Client could not connect to STUN over UDP, which may be preventing a direct connection`, + `✘ Client could not connect to STUN over UDP`, }, }, { name: "AgentHasStunNoUDP", diags: cliui.ConnDiags{ - ConnInfo: &workspacesdk.AgentConnectionInfo{ + ConnInfo: workspacesdk.AgentConnectionInfo{ DERPMap: &tailcfg.DERPMap{ Regions: map[int]*tailcfg.DERPRegion{ 999: { @@ -768,12 +771,15 @@ func TestConnDiagnostics(t *testing.T) { }, want: []string{ `❗ You are connected via a DERP relay, not directly (p2p)`, - `❗ Agent could not connect to STUN over UDP, which may be preventing a direct connection`, + `✘ Agent could not connect to STUN over UDP`, }, }, { name: "ClientHardNat", diags: cliui.ConnDiags{ + ConnInfo: workspacesdk.AgentConnectionInfo{ + DERPMap: &tailcfg.DERPMap{}, + }, LocalNetInfo: &tailcfg.NetInfo{ MappingVariesByDestIP: "true", }, @@ -786,7 +792,9 @@ func TestConnDiagnostics(t *testing.T) { { name: "AgentHardNat", diags: cliui.ConnDiags{ - ConnInfo: &workspacesdk.AgentConnectionInfo{}, + ConnInfo: workspacesdk.AgentConnectionInfo{ + DERPMap: &tailcfg.DERPMap{}, + }, PingP2P: false, LocalNetInfo: &tailcfg.NetInfo{}, AgentNetcheck: &healthsdk.AgentNetcheckReport{ @@ -801,6 +809,9 @@ func TestConnDiagnostics(t *testing.T) { { name: "AgentInterfaceWarnings", diags: cliui.ConnDiags{ + ConnInfo: workspacesdk.AgentConnectionInfo{ + DERPMap: &tailcfg.DERPMap{}, + }, PingP2P: true, AgentNetcheck: &healthsdk.AgentNetcheckReport{ Interfaces: healthsdk.InterfacesReport{ @@ -813,13 +824,16 @@ func TestConnDiagnostics(t *testing.T) { }, }, want: []string{ - `❗ Agent: network interface eth0 has MTU 1280, (less than 1378), which may cause problems with direct connections`, + `❗ Network interface eth0 has MTU 1280, (less than 1378), which may degrade the quality of direct connections`, `✔ You are connected directly (p2p)`, }, }, { name: "LocalInterfaceWarnings", diags: cliui.ConnDiags{ + ConnInfo: workspacesdk.AgentConnectionInfo{ + DERPMap: &tailcfg.DERPMap{}, + }, PingP2P: true, LocalInterfaces: &healthsdk.InterfacesReport{ BaseReport: healthsdk.BaseReport{ @@ -830,30 +844,36 @@ func TestConnDiagnostics(t *testing.T) { }, }, want: []string{ - `❗ Client: network interface eth1 has MTU 1310, (less than 1378), which may cause problems with direct connections`, + `❗ Network interface eth1 has MTU 1310, (less than 1378), which may degrade the quality of direct connections`, `✔ You are connected directly (p2p)`, }, }, { name: "ClientAWSIP", diags: cliui.ConnDiags{ + ConnInfo: workspacesdk.AgentConnectionInfo{ + DERPMap: &tailcfg.DERPMap{}, + }, ClientIPIsAWS: true, AgentIPIsAWS: false, }, want: []string{ `❗ You are connected via a DERP relay, not directly (p2p)`, - `❗ Client IP address is within an AWS range, which is known to cause problems with forming direct connections (AWS uses hard NAT)`, + `❗ Client IP address is within an AWS range (AWS uses hard NAT)`, }, }, { name: "AgentAWSIP", diags: cliui.ConnDiags{ + ConnInfo: workspacesdk.AgentConnectionInfo{ + DERPMap: &tailcfg.DERPMap{}, + }, ClientIPIsAWS: false, AgentIPIsAWS: true, }, want: []string{ `❗ You are connected via a DERP relay, not directly (p2p)`, - `❗ Agent IP address is within an AWS range, which is known to cause problems with forming direct connections (AWS uses hard NAT)`, + `❗ Agent IP address is within an AWS range (AWS uses hard NAT)`, }, }, } @@ -864,7 +884,7 @@ func TestConnDiagnostics(t *testing.T) { r, w := io.Pipe() go func() { defer w.Close() - cliui.ConnDiagnostics(w, tc.diags) + tc.diags.Write(w) }() bytes, err := io.ReadAll(r) require.NoError(t, err) diff --git a/cli/ping.go b/cli/ping.go index e20a220849d4d..f149cb6a33e2a 100644 --- a/cli/ping.go +++ b/cli/ping.go @@ -158,21 +158,21 @@ func (r *RootCmd) ping() *serpent.Command { PingP2P: didP2p, DisableDirect: r.disableDirect, LocalNetInfo: ni, + Verbose: r.verbose, } awsRanges, err := cliutil.FetchAWSIPRanges(ctx, cliutil.AWSIPRangesURL) if err != nil { - _, _ = fmt.Fprintf(inv.Stdout, "Failed to retrieve AWS IP ranges: %v\n", err) + opts.Logger.Debug(inv.Context(), "failed to retrieve AWS IP ranges", slog.Error(err)) } connDiags.ClientIPIsAWS = isAWSIP(awsRanges, ni) connInfo, err := client.AgentConnectionInfoGeneric(ctx) - if err == nil { - connDiags.ConnInfo = &connInfo - } else { - _, _ = fmt.Fprintf(inv.Stdout, "Failed to retrieve connection info from server: %v\n", err) + if err != nil || connInfo.DERPMap == nil { + return xerrors.Errorf("Failed to retrieve connection info from server: %v\n", err) } + connDiags.ConnInfo = connInfo ifReport, err := healthsdk.RunInterfacesReport() if err == nil { connDiags.LocalInterfaces = &ifReport @@ -193,7 +193,7 @@ func (r *RootCmd) ping() *serpent.Command { } } - cliui.ConnDiagnostics(inv.Stdout, connDiags) + connDiags.Write(inv.Stdout) return nil }, } @@ -224,6 +224,9 @@ func (r *RootCmd) ping() *serpent.Command { } func isAWSIP(awsRanges *cliutil.AWSIPRanges, ni *tailcfg.NetInfo) bool { + if awsRanges == nil { + return false + } if ni.GlobalV4 != "" { ip, err := netip.ParseAddr(ni.GlobalV4) if err == nil && awsRanges.CheckIP(ip) { From 99e64b4fe44e1269b52522182491149c8688990a Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Thu, 29 Aug 2024 08:19:48 +0000 Subject: [PATCH 3/5] remove hyphens --- cli/cliui/agent.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/cliui/agent.go b/cli/cliui/agent.go index 660d2af45ebe8..0658db55f5014 100644 --- a/cli/cliui/agent.go +++ b/cli/cliui/agent.go @@ -374,13 +374,13 @@ func (d ConnDiags) Write(w io.Writer) { if len(client) > 0 { _, _ = fmt.Fprint(w, "Possible client-side issues with direct connection:\n\n") for _, msg := range client { - _, _ = fmt.Fprintf(w, " - %s\n\n", msg) + _, _ = fmt.Fprintf(w, " %s\n\n", msg) } } if len(agent) > 0 { _, _ = fmt.Fprint(w, "Possible agent-side issues with direct connections:\n\n") for _, msg := range agent { - _, _ = fmt.Fprintf(w, " - %s\n\n", msg) + _, _ = fmt.Fprintf(w, " %s\n\n", msg) } } } From 55cda77ff43fbaf45b05ea482e76fea7808c6252 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Thu, 29 Aug 2024 08:32:34 +0000 Subject: [PATCH 4/5] add back hyphens --- cli/cliui/agent.go | 24 ++++++++++++------------ cli/cliui/agent_test.go | 18 +++++++++--------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/cli/cliui/agent.go b/cli/cliui/agent.go index 0658db55f5014..4380f86b232e3 100644 --- a/cli/cliui/agent.go +++ b/cli/cliui/agent.go @@ -374,13 +374,13 @@ func (d ConnDiags) Write(w io.Writer) { if len(client) > 0 { _, _ = fmt.Fprint(w, "Possible client-side issues with direct connection:\n\n") for _, msg := range client { - _, _ = fmt.Fprintf(w, " %s\n\n", msg) + _, _ = fmt.Fprintf(w, " - %s\n\n", msg) } } if len(agent) > 0 { _, _ = fmt.Fprint(w, "Possible agent-side issues with direct connections:\n\n") for _, msg := range agent { - _, _ = fmt.Fprintf(w, " %s\n\n", msg) + _, _ = fmt.Fprintf(w, " - %s\n\n", msg) } } } @@ -423,30 +423,30 @@ func (d ConnDiags) splitDiagnostics() (general, client, agent []string) { } if !d.ConnInfo.DERPMap.HasSTUN() { - general = append(general, "✘ The DERP map is not configured to use STUN") + general = append(general, "The DERP map is not configured to use STUN") } else if d.LocalNetInfo != nil && !d.LocalNetInfo.UDP { - client = append(client, "✘ Client could not connect to STUN over UDP") + client = append(client, "Client could not connect to STUN over UDP") } if d.LocalNetInfo != nil && d.LocalNetInfo.MappingVariesByDestIP.EqualBool(true) { - client = append(client, "❗ Client is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers") + client = append(client, "Client is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers") } if d.AgentNetcheck != nil && d.AgentNetcheck.NetInfo != nil { if d.AgentNetcheck.NetInfo.MappingVariesByDestIP.EqualBool(true) { - agent = append(agent, "❗ Agent is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers") + agent = append(agent, "Agent is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers") } if !d.AgentNetcheck.NetInfo.UDP { - agent = append(agent, "✘ Agent could not connect to STUN over UDP") + agent = append(agent, "Agent could not connect to STUN over UDP") } } - if d.ClientIPIsAWS { - client = append(client, "❗ Client IP address is within an AWS range (AWS uses hard NAT)") + if true { + client = append(client, "Client IP address is within an AWS range (AWS uses hard NAT)") } - if d.AgentIPIsAWS { - agent = append(agent, "❗ Agent IP address is within an AWS range (AWS uses hard NAT)") + if true { + agent = append(agent, "Agent IP address is within an AWS range (AWS uses hard NAT)") } return general, client, agent } @@ -457,5 +457,5 @@ func formatHealthMessage(msg health.Message) string { } r := []rune(strings.Replace(msg.Message, ", which may cause problems with direct connections", "", -1)) out := string(append([]rune{unicode.ToUpper(r[0])}, r[1:]...)) - return fmt.Sprintf("❗ %s, which may degrade the quality of direct connections", out) + return fmt.Sprintf("%s, which may degrade the quality of direct connections", out) } diff --git a/cli/cliui/agent_test.go b/cli/cliui/agent_test.go index cd05d7164775a..fcb416811227b 100644 --- a/cli/cliui/agent_test.go +++ b/cli/cliui/agent_test.go @@ -719,7 +719,7 @@ func TestConnDiagnostics(t *testing.T) { }, want: []string{ `❗ You are connected via a DERP relay, not directly (p2p)`, - `✘ The DERP map is not configured to use STUN`, + `The DERP map is not configured to use STUN`, }, }, { @@ -744,7 +744,7 @@ func TestConnDiagnostics(t *testing.T) { }, want: []string{ `❗ You are connected via a DERP relay, not directly (p2p)`, - `✘ Client could not connect to STUN over UDP`, + `Client could not connect to STUN over UDP`, }, }, { @@ -771,7 +771,7 @@ func TestConnDiagnostics(t *testing.T) { }, want: []string{ `❗ You are connected via a DERP relay, not directly (p2p)`, - `✘ Agent could not connect to STUN over UDP`, + `Agent could not connect to STUN over UDP`, }, }, { @@ -786,7 +786,7 @@ func TestConnDiagnostics(t *testing.T) { }, want: []string{ `❗ You are connected via a DERP relay, not directly (p2p)`, - `❗ Client is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers`, + `Client is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers`, }, }, { @@ -803,7 +803,7 @@ func TestConnDiagnostics(t *testing.T) { }, want: []string{ `❗ You are connected via a DERP relay, not directly (p2p)`, - `❗ Agent is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers`, + `Agent is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers`, }, }, { @@ -824,8 +824,8 @@ func TestConnDiagnostics(t *testing.T) { }, }, want: []string{ - `❗ Network interface eth0 has MTU 1280, (less than 1378), which may degrade the quality of direct connections`, `✔ You are connected directly (p2p)`, + `Network interface eth0 has MTU 1280, (less than 1378), which may degrade the quality of direct connections`, }, }, { @@ -844,8 +844,8 @@ func TestConnDiagnostics(t *testing.T) { }, }, want: []string{ - `❗ Network interface eth1 has MTU 1310, (less than 1378), which may degrade the quality of direct connections`, `✔ You are connected directly (p2p)`, + `Network interface eth1 has MTU 1310, (less than 1378), which may degrade the quality of direct connections`, }, }, { @@ -859,7 +859,7 @@ func TestConnDiagnostics(t *testing.T) { }, want: []string{ `❗ You are connected via a DERP relay, not directly (p2p)`, - `❗ Client IP address is within an AWS range (AWS uses hard NAT)`, + `Client IP address is within an AWS range (AWS uses hard NAT)`, }, }, { @@ -873,7 +873,7 @@ func TestConnDiagnostics(t *testing.T) { }, want: []string{ `❗ You are connected via a DERP relay, not directly (p2p)`, - `❗ Agent IP address is within an AWS range (AWS uses hard NAT)`, + `Agent IP address is within an AWS range (AWS uses hard NAT)`, }, }, } From 2e08d191fdf198615438ce61ad2b1d78d86d40e5 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Thu, 29 Aug 2024 09:04:41 +0000 Subject: [PATCH 5/5] fix test --- cli/cliui/agent.go | 19 ++++--------------- cli/cliui/agent_test.go | 4 ++-- cli/ping.go | 12 ++++++------ cli/ping_test.go | 1 + codersdk/healthsdk/interfaces.go | 2 +- 5 files changed, 14 insertions(+), 24 deletions(-) diff --git a/cli/cliui/agent.go b/cli/cliui/agent.go index 4380f86b232e3..9f528a6d69f20 100644 --- a/cli/cliui/agent.go +++ b/cli/cliui/agent.go @@ -7,13 +7,11 @@ import ( "strconv" "strings" "time" - "unicode" "github.com/google/uuid" "golang.org/x/xerrors" "tailscale.com/tailcfg" - "github.com/coder/coder/v2/coderd/healthcheck/health" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/healthsdk" "github.com/coder/coder/v2/codersdk/workspacesdk" @@ -394,13 +392,13 @@ func (d ConnDiags) splitDiagnostics() (general, client, agent []string) { if d.AgentNetcheck != nil { for _, msg := range d.AgentNetcheck.Interfaces.Warnings { - agent = append(agent, formatHealthMessage(msg)) + agent = append(agent, msg.Message) } } if d.LocalInterfaces != nil { for _, msg := range d.LocalInterfaces.Warnings { - client = append(client, formatHealthMessage(msg)) + client = append(client, msg.Message) } } @@ -441,21 +439,12 @@ func (d ConnDiags) splitDiagnostics() (general, client, agent []string) { } } - if true { + if d.ClientIPIsAWS { client = append(client, "Client IP address is within an AWS range (AWS uses hard NAT)") } - if true { + if d.AgentIPIsAWS { agent = append(agent, "Agent IP address is within an AWS range (AWS uses hard NAT)") } return general, client, agent } - -func formatHealthMessage(msg health.Message) string { - if msg.Code != health.CodeInterfaceSmallMTU { - return msg.Message - } - r := []rune(strings.Replace(msg.Message, ", which may cause problems with direct connections", "", -1)) - out := string(append([]rune{unicode.ToUpper(r[0])}, r[1:]...)) - return fmt.Sprintf("%s, which may degrade the quality of direct connections", out) -} diff --git a/cli/cliui/agent_test.go b/cli/cliui/agent_test.go index fcb416811227b..aeb41130d344e 100644 --- a/cli/cliui/agent_test.go +++ b/cli/cliui/agent_test.go @@ -817,7 +817,7 @@ func TestConnDiagnostics(t *testing.T) { Interfaces: healthsdk.InterfacesReport{ BaseReport: healthsdk.BaseReport{ Warnings: []health.Message{ - health.Messagef(health.CodeInterfaceSmallMTU, "network interface eth0 has MTU 1280, (less than 1378), which may cause problems with direct connections"), + health.Messagef(health.CodeInterfaceSmallMTU, "Network interface eth0 has MTU 1280, (less than 1378), which may degrade the quality of direct connections"), }, }, }, @@ -838,7 +838,7 @@ func TestConnDiagnostics(t *testing.T) { LocalInterfaces: &healthsdk.InterfacesReport{ BaseReport: healthsdk.BaseReport{ Warnings: []health.Message{ - health.Messagef(health.CodeInterfaceSmallMTU, "network interface eth1 has MTU 1310, (less than 1378), which may cause problems with direct connections"), + health.Messagef(health.CodeInterfaceSmallMTU, "Network interface eth1 has MTU 1310, (less than 1378), which may degrade the quality of direct connections"), }, }, }, diff --git a/cli/ping.go b/cli/ping.go index f149cb6a33e2a..cadb69b5c21f5 100644 --- a/cli/ping.go +++ b/cli/ping.go @@ -148,8 +148,8 @@ func (r *RootCmd) ping() *serpent.Command { break } } - ctx, cancel = context.WithTimeout(inv.Context(), 30*time.Second) - defer cancel() + diagCtx, diagCancel := context.WithTimeout(inv.Context(), 30*time.Second) + defer diagCancel() diags := conn.GetPeerDiagnostics() cliui.PeerDiagnostics(inv.Stdout, diags) @@ -161,16 +161,16 @@ func (r *RootCmd) ping() *serpent.Command { Verbose: r.verbose, } - awsRanges, err := cliutil.FetchAWSIPRanges(ctx, cliutil.AWSIPRangesURL) + awsRanges, err := cliutil.FetchAWSIPRanges(diagCtx, cliutil.AWSIPRangesURL) if err != nil { opts.Logger.Debug(inv.Context(), "failed to retrieve AWS IP ranges", slog.Error(err)) } connDiags.ClientIPIsAWS = isAWSIP(awsRanges, ni) - connInfo, err := client.AgentConnectionInfoGeneric(ctx) + connInfo, err := client.AgentConnectionInfoGeneric(diagCtx) if err != nil || connInfo.DERPMap == nil { - return xerrors.Errorf("Failed to retrieve connection info from server: %v\n", err) + return xerrors.Errorf("Failed to retrieve connection info from server: %w\n", err) } connDiags.ConnInfo = connInfo ifReport, err := healthsdk.RunInterfacesReport() @@ -180,7 +180,7 @@ func (r *RootCmd) ping() *serpent.Command { _, _ = fmt.Fprintf(inv.Stdout, "Failed to retrieve local interfaces report: %v\n", err) } - agentNetcheck, err := conn.Netcheck(ctx) + agentNetcheck, err := conn.Netcheck(diagCtx) if err == nil { connDiags.AgentNetcheck = &agentNetcheck connDiags.AgentIPIsAWS = isAWSIP(awsRanges, agentNetcheck.NetInfo) diff --git a/cli/ping_test.go b/cli/ping_test.go index 7a4dcee5e15f3..f4ce8aefa7a00 100644 --- a/cli/ping_test.go +++ b/cli/ping_test.go @@ -67,6 +67,7 @@ func TestPing(t *testing.T) { pty.ExpectMatch("pong from " + workspace.Name) pty.ExpectMatch("✔ received remote agent data from Coder networking coordinator") + pty.ExpectMatch("✔ You are connected directly (p2p)") cancel() <-cmdDone }) diff --git a/codersdk/healthsdk/interfaces.go b/codersdk/healthsdk/interfaces.go index 6f4365aaeefac..714e1ecbdb411 100644 --- a/codersdk/healthsdk/interfaces.go +++ b/codersdk/healthsdk/interfaces.go @@ -72,7 +72,7 @@ func generateInterfacesReport(st *interfaces.State) (report InterfacesReport) { report.Severity = health.SeverityWarning report.Warnings = append(report.Warnings, health.Messagef(health.CodeInterfaceSmallMTU, - "network interface %s has MTU %d (less than %d), which may cause problems with direct connections", iface.Name, iface.MTU, safeMTU), + "Network interface %s has MTU %d (less than %d), which may degrade the quality of direct connections", iface.Name, iface.MTU, safeMTU), ) } }