Skip to content

Commit f16e4e7

Browse files
ethanndicksonethanndickson
committed
review
1 parent 9ccec94 commit f16e4e7

File tree

3 files changed

+104
-42
lines changed

3 files changed

+104
-42
lines changed

‎cli/cliui/agent.go

Lines changed: 61 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,13 @@ import (
77
"strconv"
88
"strings"
99
"time"
10+
"unicode"
1011

1112
"github.com/google/uuid"
1213
"golang.org/x/xerrors"
1314
"tailscale.com/tailcfg"
1415

16+
"github.com/coder/coder/v2/coderd/healthcheck/health"
1517
"github.com/coder/coder/v2/codersdk"
1618
"github.com/coder/coder/v2/codersdk/healthsdk"
1719
"github.com/coder/coder/v2/codersdk/workspacesdk"
@@ -351,72 +353,109 @@ func PeerDiagnostics(w io.Writer, d tailnet.PeerDiagnostics) {
351353
}
352354

353355
type ConnDiags struct {
354-
ConnInfo *workspacesdk.AgentConnectionInfo
356+
ConnInfo workspacesdk.AgentConnectionInfo
355357
PingP2P bool
356358
DisableDirect bool
357359
LocalNetInfo *tailcfg.NetInfo
358360
LocalInterfaces *healthsdk.InterfacesReport
359361
AgentNetcheck *healthsdk.AgentNetcheckReport
360362
ClientIPIsAWS bool
361363
AgentIPIsAWS bool
364+
Verbose bool
362365
// TODO: More diagnostics
363366
}
364367

365-
func ConnDiagnostics(w io.Writer, d ConnDiags) {
368+
func (d ConnDiags) Write(w io.Writer) {
369+
_, _ = fmt.Fprintln(w, "")
370+
general, client, agent := d.splitDiagnostics()
371+
for _, msg := range general {
372+
_, _ = fmt.Fprintln(w, msg)
373+
}
374+
if len(client) > 0 {
375+
_, _ = fmt.Fprint(w, "Possible client-side issues with direct connection:\n\n")
376+
for _, msg := range client {
377+
_, _ = fmt.Fprintf(w, " - %s\n\n", msg)
378+
}
379+
}
380+
if len(agent) > 0 {
381+
_, _ = fmt.Fprint(w, "Possible agent-side issues with direct connections:\n\n")
382+
for _, msg := range agent {
383+
_, _ = fmt.Fprintf(w, " - %s\n\n", msg)
384+
}
385+
}
386+
}
387+
388+
func (d ConnDiags) splitDiagnostics() (general, client, agent []string) {
366389
if d.PingP2P {
367-
_, _ = fmt.Fprint(w, "✔ You are connected directly (p2p)\n")
390+
general = append(general, "✔ You are connected directly (p2p)")
368391
} else {
369-
_, _ = fmt.Fprint(w, "❗ You are connected via a DERP relay, not directly (p2p)\n")
392+
general = append(general, "❗ You are connected via a DERP relay, not directly (p2p)")
370393
}
371394

372395
if d.AgentNetcheck != nil {
373396
for _, msg := range d.AgentNetcheck.Interfaces.Warnings {
374-
_, _ = fmt.Fprintf(w, "❗ Agent: %s\n", msg.Message)
397+
agent = append(agent, formatHealthMessage(msg))
375398
}
376399
}
377400

378401
if d.LocalInterfaces != nil {
379402
for _, msg := range d.LocalInterfaces.Warnings {
380-
_, _ = fmt.Fprintf(w, "❗ Client: %s\n", msg.Message)
403+
client = append(client, formatHealthMessage(msg))
381404
}
382405
}
383406

384-
if d.DisableDirect {
385-
_, _ = fmt.Fprint(w, "❗ Direct connections are disabled locally, by `--disable-direct` or `CODER_DISABLE_DIRECT`\n")
386-
return
407+
if d.PingP2P && !d.Verbose {
408+
return general, client, agent
387409
}
388410

389-
if d.ConnInfo != nil && d.ConnInfo.DisableDirectConnections {
390-
_, _ = fmt.Fprint(w, "❗ Your Coder administrator has blocked direct connections\n")
391-
return
411+
if d.DisableDirect {
412+
general = append(general, "❗ Direct connections are disabled locally, by `--disable-direct` or `CODER_DISABLE_DIRECT`")
413+
if !d.Verbose {
414+
return general, client, agent
415+
}
392416
}
393417

394-
if d.ConnInfo != nil && d.ConnInfo.DERPMap != nil {
395-
if !d.ConnInfo.DERPMap.HasSTUN() {
396-
_, _ = fmt.Fprint(w, "✘ The DERP map is not configured to use STUN, which will prevent direct connections from starting outside of local networks\n")
397-
} else if d.LocalNetInfo != nil && !d.LocalNetInfo.UDP {
398-
_, _ = fmt.Fprint(w, "❗ Client could not connect to STUN over UDP, which may be preventing a direct connection\n")
418+
if d.ConnInfo.DisableDirectConnections {
419+
general = append(general, "❗ Your Coder administrator has blocked direct connections")
420+
if !d.Verbose {
421+
return general, client, agent
399422
}
400423
}
401424

425+
if !d.ConnInfo.DERPMap.HasSTUN() {
426+
general = append(general, "✘ The DERP map is not configured to use STUN")
427+
} else if d.LocalNetInfo != nil && !d.LocalNetInfo.UDP {
428+
client = append(client, "✘ Client could not connect to STUN over UDP")
429+
}
430+
402431
if d.LocalNetInfo != nil && d.LocalNetInfo.MappingVariesByDestIP.EqualBool(true) {
403-
_, _ = fmt.Fprint(w, "❗ Client is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers\n")
432+
client = append(client, "❗ Client is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers")
404433
}
405434

406435
if d.AgentNetcheck != nil && d.AgentNetcheck.NetInfo != nil {
407436
if d.AgentNetcheck.NetInfo.MappingVariesByDestIP.EqualBool(true) {
408-
_, _ = fmt.Fprint(w, "❗ Agent is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers\n")
437+
agent = append(agent, "❗ Agent is potentially behind a hard NAT, as multiple endpoints were retrieved from different STUN servers")
409438
}
410439
if !d.AgentNetcheck.NetInfo.UDP {
411-
_, _ = fmt.Fprint(w, "❗ Agent could not connect to STUN over UDP, which may be preventing a direct connection\n")
440+
agent = append(agent, "✘ Agent could not connect to STUN over UDP")
412441
}
413442
}
414443

415444
if d.ClientIPIsAWS {
416-
_, _ = 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")
445+
client = append(client, "❗ Client IP address is within an AWS range (AWS uses hard NAT)")
417446
}
418447

419448
if d.AgentIPIsAWS {
420-
_, _ = 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")
449+
agent = append(agent, "❗ Agent IP address is within an AWS range (AWS uses hard NAT)")
450+
}
451+
return general, client, agent
452+
}
453+
454+
func formatHealthMessage(msg health.Message) string {
455+
if msg.Code != health.CodeInterfaceSmallMTU {
456+
return msg.Message
421457
}
458+
r := []rune(strings.Replace(msg.Message, ", which may cause problems with direct connections", "", -1))
459+
out := string(append([]rune{unicode.ToUpper(r[0])}, r[1:]...))
460+
return fmt.Sprintf("❗ %s, which may degrade the quality of direct connections", out)
422461
}

‎cli/cliui/agent_test.go

Lines changed: 34 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -686,7 +686,9 @@ func TestConnDiagnostics(t *testing.T) {
686686
{
687687
name: "Direct",
688688
diags: cliui.ConnDiags{
689-
ConnInfo: &workspacesdk.AgentConnectionInfo{},
689+
ConnInfo: workspacesdk.AgentConnectionInfo{
690+
DERPMap: &tailcfg.DERPMap{},
691+
},
690692
PingP2P: true,
691693
LocalNetInfo: &tailcfg.NetInfo{},
692694
},
@@ -697,7 +699,8 @@ func TestConnDiagnostics(t *testing.T) {
697699
{
698700
name: "DirectBlocked",
699701
diags: cliui.ConnDiags{
700-
ConnInfo: &workspacesdk.AgentConnectionInfo{
702+
ConnInfo: workspacesdk.AgentConnectionInfo{
703+
DERPMap: &tailcfg.DERPMap{},
701704
DisableDirectConnections: true,
702705
},
703706
},
@@ -709,20 +712,20 @@ func TestConnDiagnostics(t *testing.T) {
709712
{
710713
name: "NoStun",
711714
diags: cliui.ConnDiags{
712-
ConnInfo: &workspacesdk.AgentConnectionInfo{
715+
ConnInfo: workspacesdk.AgentConnectionInfo{
713716
DERPMap: &tailcfg.DERPMap{},
714717
},
715718
LocalNetInfo: &tailcfg.NetInfo{},
716719
},
717720
want: []string{
718721
`❗ You are connected via a DERP relay, not directly (p2p)`,
719-
`✘ The DERP map is not configured to use STUN, which will prevent direct connections from starting outside of local networks`,
722+
`✘ The DERP map is not configured to use STUN`,
720723
},
721724
},
722725
{
723726
name: "ClientHasStunNoUDP",
724727
diags: cliui.ConnDiags{
725-
ConnInfo: &workspacesdk.AgentConnectionInfo{
728+
ConnInfo: workspacesdk.AgentConnectionInfo{
726729
DERPMap: &tailcfg.DERPMap{
727730
Regions: map[int]*tailcfg.DERPRegion{
728731
999: {
@@ -741,13 +744,13 @@ func TestConnDiagnostics(t *testing.T) {
741744
},
742745
want: []string{
743746
`❗ You are connected via a DERP relay, not directly (p2p)`,
744-
`❗ Client could not connect to STUN over UDP, which may be preventing a direct connection`,
747+
`✘ Client could not connect to STUN over UDP`,
745748
},
746749
},
747750
{
748751
name: "AgentHasStunNoUDP",
749752
diags: cliui.ConnDiags{
750-
ConnInfo: &workspacesdk.AgentConnectionInfo{
753+
ConnInfo: workspacesdk.AgentConnectionInfo{
751754
DERPMap: &tailcfg.DERPMap{
752755
Regions: map[int]*tailcfg.DERPRegion{
753756
999: {
@@ -768,12 +771,15 @@ func TestConnDiagnostics(t *testing.T) {
768771
},
769772
want: []string{
770773
`❗ You are connected via a DERP relay, not directly (p2p)`,
771-
`❗ Agent could not connect to STUN over UDP, which may be preventing a direct connection`,
774+
`✘ Agent could not connect to STUN over UDP`,
772775
},
773776
},
774777
{
775778
name: "ClientHardNat",
776779
diags: cliui.ConnDiags{
780+
ConnInfo: workspacesdk.AgentConnectionInfo{
781+
DERPMap: &tailcfg.DERPMap{},
782+
},
777783
LocalNetInfo: &tailcfg.NetInfo{
778784
MappingVariesByDestIP: "true",
779785
},
@@ -786,7 +792,9 @@ func TestConnDiagnostics(t *testing.T) {
786792
{
787793
name: "AgentHardNat",
788794
diags: cliui.ConnDiags{
789-
ConnInfo: &workspacesdk.AgentConnectionInfo{},
795+
ConnInfo: workspacesdk.AgentConnectionInfo{
796+
DERPMap: &tailcfg.DERPMap{},
797+
},
790798
PingP2P: false,
791799
LocalNetInfo: &tailcfg.NetInfo{},
792800
AgentNetcheck: &healthsdk.AgentNetcheckReport{
@@ -801,6 +809,9 @@ func TestConnDiagnostics(t *testing.T) {
801809
{
802810
name: "AgentInterfaceWarnings",
803811
diags: cliui.ConnDiags{
812+
ConnInfo: workspacesdk.AgentConnectionInfo{
813+
DERPMap: &tailcfg.DERPMap{},
814+
},
804815
PingP2P: true,
805816
AgentNetcheck: &healthsdk.AgentNetcheckReport{
806817
Interfaces: healthsdk.InterfacesReport{
@@ -813,13 +824,16 @@ func TestConnDiagnostics(t *testing.T) {
813824
},
814825
},
815826
want: []string{
816-
`❗ Agent: network interface eth0 has MTU 1280, (less than 1378), which may cause problems with direct connections`,
827+
`❗ Network interface eth0 has MTU 1280, (less than 1378), which may degrade the quality of direct connections`,
817828
`✔ You are connected directly (p2p)`,
818829
},
819830
},
820831
{
821832
name: "LocalInterfaceWarnings",
822833
diags: cliui.ConnDiags{
834+
ConnInfo: workspacesdk.AgentConnectionInfo{
835+
DERPMap: &tailcfg.DERPMap{},
836+
},
823837
PingP2P: true,
824838
LocalInterfaces: &healthsdk.InterfacesReport{
825839
BaseReport: healthsdk.BaseReport{
@@ -830,30 +844,36 @@ func TestConnDiagnostics(t *testing.T) {
830844
},
831845
},
832846
want: []string{
833-
`❗ Client: network interface eth1 has MTU 1310, (less than 1378), which may cause problems with direct connections`,
847+
`❗ Network interface eth1 has MTU 1310, (less than 1378), which may degrade the quality of direct connections`,
834848
`✔ You are connected directly (p2p)`,
835849
},
836850
},
837851
{
838852
name: "ClientAWSIP",
839853
diags: cliui.ConnDiags{
854+
ConnInfo: workspacesdk.AgentConnectionInfo{
855+
DERPMap: &tailcfg.DERPMap{},
856+
},
840857
ClientIPIsAWS: true,
841858
AgentIPIsAWS: false,
842859
},
843860
want: []string{
844861
`❗ You are connected via a DERP relay, not directly (p2p)`,
845-
`❗ Client IP address is within an AWS range, which is known to cause problems with forming direct connections (AWS uses hard NAT)`,
862+
`❗ Client IP address is within an AWS range (AWS uses hard NAT)`,
846863
},
847864
},
848865
{
849866
name: "AgentAWSIP",
850867
diags: cliui.ConnDiags{
868+
ConnInfo: workspacesdk.AgentConnectionInfo{
869+
DERPMap: &tailcfg.DERPMap{},
870+
},
851871
ClientIPIsAWS: false,
852872
AgentIPIsAWS: true,
853873
},
854874
want: []string{
855875
`❗ You are connected via a DERP relay, not directly (p2p)`,
856-
`❗ Agent IP address is within an AWS range, which is known to cause problems with forming direct connections (AWS uses hard NAT)`,
876+
`❗ Agent IP address is within an AWS range (AWS uses hard NAT)`,
857877
},
858878
},
859879
}
@@ -864,7 +884,7 @@ func TestConnDiagnostics(t *testing.T) {
864884
r, w := io.Pipe()
865885
go func() {
866886
defer w.Close()
867-
cliui.ConnDiagnostics(w, tc.diags)
887+
tc.diags.Write(w)
868888
}()
869889
bytes, err := io.ReadAll(r)
870890
require.NoError(t, err)

‎cli/ping.go

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -158,21 +158,21 @@ func (r *RootCmd) ping() *serpent.Command {
158158
PingP2P: didP2p,
159159
DisableDirect: r.disableDirect,
160160
LocalNetInfo: ni,
161+
Verbose: r.verbose,
161162
}
162163

163164
awsRanges, err := cliutil.FetchAWSIPRanges(ctx, cliutil.AWSIPRangesURL)
164165
if err != nil {
165-
_, _ = fmt.Fprintf(inv.Stdout, "Failed to retrieve AWS IP ranges: %v\n", err)
166+
opts.Logger.Debug(inv.Context(), "failed to retrieve AWS IP ranges", slog.Error(err))
166167
}
167168

168169
connDiags.ClientIPIsAWS = isAWSIP(awsRanges, ni)
169170

170171
connInfo, err := client.AgentConnectionInfoGeneric(ctx)
171-
if err == nil {
172-
connDiags.ConnInfo = &connInfo
173-
} else {
174-
_, _ = fmt.Fprintf(inv.Stdout, "Failed to retrieve connection info from server: %v\n", err)
172+
if err != nil || connInfo.DERPMap == nil {
173+
return xerrors.Errorf("Failed to retrieve connection info from server: %v\n", err)
175174
}
175+
connDiags.ConnInfo = connInfo
176176
ifReport, err := healthsdk.RunInterfacesReport()
177177
if err == nil {
178178
connDiags.LocalInterfaces = &ifReport
@@ -193,7 +193,7 @@ func (r *RootCmd) ping() *serpent.Command {
193193
}
194194
}
195195

196-
cliui.ConnDiagnostics(inv.Stdout, connDiags)
196+
connDiags.Write(inv.Stdout)
197197
return nil
198198
},
199199
}
@@ -224,6 +224,9 @@ func (r *RootCmd) ping() *serpent.Command {
224224
}
225225

226226
func isAWSIP(awsRanges *cliutil.AWSIPRanges, ni *tailcfg.NetInfo) bool {
227+
if awsRanges == nil {
228+
return false
229+
}
227230
if ni.GlobalV4 != "" {
228231
ip, err := netip.ParseAddr(ni.GlobalV4)
229232
if err == nil && awsRanges.CheckIP(ip) {

0 commit comments

Comments
 (0)