diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index a1eba8bce..57cd9303a 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -16,6 +16,7 @@ import ( qt "github.com/frankban/quicktest" "github.com/google/go-cmp/cmp" + "tailscale.com/health/healthmsg" "tailscale.com/ipn" "tailscale.com/ipn/ipnstate" "tailscale.com/tstest" @@ -1143,3 +1144,15 @@ func TestCleanUpArgs(t *testing.T) { c.Assert(got, qt.DeepEquals, tt.want) } } + +func TestUpWorthWarning(t *testing.T) { + if !upWorthyWarning(healthmsg.WarnAcceptRoutesOff) { + t.Errorf("WarnAcceptRoutesOff of %q should be worth warning", healthmsg.WarnAcceptRoutesOff) + } + if !upWorthyWarning(healthmsg.TailscaleSSHOnBut + "some problem") { + t.Errorf("want true for SSH problems") + } + if upWorthyWarning("not in map poll") { + t.Errorf("want false for other misc errors") + } +} diff --git a/cmd/tailscale/cli/status.go b/cmd/tailscale/cli/status.go index 830b2f000..8bc325847 100644 --- a/cmd/tailscale/cli/status.go +++ b/cmd/tailscale/cli/status.go @@ -128,18 +128,21 @@ func runStatus(ctx context.Context, args []string) error { return err } - // print health check information prior to checking LocalBackend state as - // it may provide an explanation to the user if we choose to exit early - if len(st.Health) > 0 { + printHealth := func() { printf("# Health check:\n") for _, m := range st.Health { printf("# - %s\n", m) } - outln() } description, ok := isRunningOrStarting(st) if !ok { + // print health check information if we're in a weird state, as it might + // provide context about why we're in that weird state. + if len(st.Health) > 0 && (st.BackendState == ipn.Starting.String() || st.BackendState == ipn.NoState.String()) { + printHealth() + outln() + } outln(description) os.Exit(1) } @@ -214,6 +217,10 @@ func runStatus(ctx context.Context, args []string) error { } } Stdout.Write(buf.Bytes()) + if len(st.Health) > 0 { + outln() + printHealth() + } return nil } diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 6d4b28359..213ce14f8 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -25,6 +25,7 @@ import ( shellquote "github.com/kballard/go-shellquote" "github.com/peterbourgon/ff/v3/ffcli" qrcode "github.com/skip2/go-qrcode" + "tailscale.com/health/healthmsg" "tailscale.com/ipn" "tailscale.com/ipn/ipnstate" "tailscale.com/net/tsaddr" @@ -495,7 +496,7 @@ func runUp(ctx context.Context, args []string) (retErr error) { defer func() { if retErr == nil { - checkSSHUpWarnings(ctx) + checkUpWarnings(ctx) } }() @@ -678,25 +679,39 @@ func runUp(ctx context.Context, args []string) (retErr error) { } } -func checkSSHUpWarnings(ctx context.Context) { - if !upArgs.runSSH { - return - } - st, err := localClient.Status(ctx) +// upWorthWarning reports whether the health check message s is worth warning +// about during "tailscale up". Many of the health checks are noisy or confusing +// or very ephemeral and happen especially briefly at startup. +// +// TODO(bradfitz): change the server to send typed warnings with metadata about +// the health check, rather than just a string. +func upWorthyWarning(s string) bool { + return strings.Contains(s, healthmsg.TailscaleSSHOnBut) || + strings.Contains(s, healthmsg.WarnAcceptRoutesOff) +} + +func checkUpWarnings(ctx context.Context) { + st, err := localClient.StatusWithoutPeers(ctx) if err != nil { // Ignore. Don't spam more. return } - if len(st.Health) == 0 { + var warn []string + for _, w := range st.Health { + if upWorthyWarning(w) { + warn = append(warn, w) + } + } + if len(warn) == 0 { return } - if len(st.Health) == 1 && strings.Contains(st.Health[0], "SSH") { - printf("%s\n", st.Health[0]) + if len(warn) == 1 { + printf("%s\n", warn[0]) return } - printf("# Health check:\n") - for _, m := range st.Health { - printf(" - %s\n", m) + printf("# Health check warnings:\n") + for _, m := range warn { + printf("# - %s\n", m) } } @@ -1040,3 +1055,15 @@ func exitNodeIP(p *ipn.Prefs, st *ipnstate.Status) (ip netip.Addr) { } return } + +func anyPeerAdvertisingRoutes(st *ipnstate.Status) bool { + for _, ps := range st.Peer { + if ps.PrimaryRoutes == nil { + continue + } + if ps.PrimaryRoutes.Len() > 0 { + return true + } + } + return false +} diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 5772e88fd..5bdb73e9b 100644 --- a/cmd/tailscale/depaware.txt +++ b/cmd/tailscale/depaware.txt @@ -51,6 +51,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep tailscale.com/derp/derphttp from tailscale.com/net/netcheck tailscale.com/disco from tailscale.com/derp tailscale.com/envknob from tailscale.com/cmd/tailscale/cli+ + tailscale.com/health/healthmsg from tailscale.com/cmd/tailscale/cli tailscale.com/hostinfo from tailscale.com/net/interfaces+ tailscale.com/ipn from tailscale.com/cmd/tailscale/cli+ tailscale.com/ipn/ipnstate from tailscale.com/cmd/tailscale/cli+ diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 0439e1e78..5d7a7a57f 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -194,6 +194,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de tailscale.com/doctor/routetable from tailscale.com/ipn/ipnlocal tailscale.com/envknob from tailscale.com/control/controlclient+ tailscale.com/health from tailscale.com/control/controlclient+ + tailscale.com/health/healthmsg from tailscale.com/ipn/ipnlocal tailscale.com/hostinfo from tailscale.com/control/controlclient+ tailscale.com/ipn from tailscale.com/ipn/ipnlocal+ tailscale.com/ipn/ipnlocal from tailscale.com/ssh/tailssh+ diff --git a/health/healthmsg/healthmsg.go b/health/healthmsg/healthmsg.go new file mode 100644 index 000000000..0ede81419 --- /dev/null +++ b/health/healthmsg/healthmsg.go @@ -0,0 +1,14 @@ +// Copyright (c) 2022 Tailscale Inc & AUTHORS All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package healthmsg contains some constants for health messages. +// +// It's a leaf so both the server and CLI can depend on it without bringing too +// much in to the CLI binary. +package healthmsg + +const ( + WarnAcceptRoutesOff = "Some peers are advertising routes but --accept-routes is false" + TailscaleSSHOnBut = "Tailscale SSH enabled, but " // + ... something from caller +) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 9aa9e8ca2..a4e2c1101 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -35,6 +35,7 @@ import ( "tailscale.com/doctor/routetable" "tailscale.com/envknob" "tailscale.com/health" + "tailscale.com/health/healthmsg" "tailscale.com/hostinfo" "tailscale.com/ipn" "tailscale.com/ipn/ipnstate" @@ -585,19 +586,23 @@ func (b *LocalBackend) updateStatus(sb *ipnstate.StatusBuilder, extraLocked func s.CurrentTailnet.MagicDNSSuffix = b.netMap.MagicDNSSuffix() s.CurrentTailnet.MagicDNSEnabled = b.netMap.DNS.Proxied s.CurrentTailnet.Name = b.netMap.Domain - if prefs := b.pm.CurrentPrefs(); prefs.Valid() && !prefs.ExitNodeID().IsZero() { - if exitPeer, ok := b.netMap.PeerWithStableID(prefs.ExitNodeID()); ok { - var online = false - if exitPeer.Online != nil { - online = *exitPeer.Online - } - s.ExitNodeStatus = &ipnstate.ExitNodeStatus{ - ID: prefs.ExitNodeID(), - Online: online, - TailscaleIPs: exitPeer.Addresses, + if prefs := b.pm.CurrentPrefs(); prefs.Valid() { + if !prefs.RouteAll() && b.netMap.AnyPeersAdvertiseRoutes() { + s.Health = append(s.Health, healthmsg.WarnAcceptRoutesOff) + } + if !prefs.ExitNodeID().IsZero() { + if exitPeer, ok := b.netMap.PeerWithStableID(prefs.ExitNodeID()); ok { + var online = false + if exitPeer.Online != nil { + online = *exitPeer.Online + } + s.ExitNodeStatus = &ipnstate.ExitNodeStatus{ + ID: prefs.ExitNodeID(), + Online: online, + TailscaleIPs: exitPeer.Addresses, + } } } - } } }) @@ -2172,12 +2177,12 @@ func (b *LocalBackend) sshOnButUnusableHealthCheckMessageLocked() (healthMessage isAdmin := hasCapability(nm, tailcfg.CapabilityAdmin) if !isAdmin { - return "Tailscale SSH enabled, but access controls don't allow anyone to access this device. Ask your admin to update your tailnet's ACLs to allow access." + return healthmsg.TailscaleSSHOnBut + "access controls don't allow anyone to access this device. Ask your admin to update your tailnet's ACLs to allow access." } if !isDefault { - return "Tailscale SSH enabled, but access controls don't allow anyone to access this device. Update your tailnet's ACLs to allow access." + return healthmsg.TailscaleSSHOnBut + "access controls don't allow anyone to access this device. Update your tailnet's ACLs to allow access." } - return "Tailscale SSH enabled, but access controls don't allow anyone to access this device. Update your tailnet's ACLs at https://tailscale.com/s/ssh-policy" + return healthmsg.TailscaleSSHOnBut + "access controls don't allow anyone to access this device. Update your tailnet's ACLs at https://tailscale.com/s/ssh-policy" } func (b *LocalBackend) isDefaultServerLocked() bool { diff --git a/types/netmap/netmap.go b/types/netmap/netmap.go index 1961d9ce1..2d6a32154 100644 --- a/types/netmap/netmap.go +++ b/types/netmap/netmap.go @@ -84,6 +84,16 @@ type NetworkMap struct { UserProfiles map[tailcfg.UserID]tailcfg.UserProfile } +// AnyPeersAdvertiseRoutes reports whether any peer is advertising non-exit node routes. +func (nm *NetworkMap) AnyPeersAdvertiseRoutes() bool { + for _, p := range nm.Peers { + if len(p.PrimaryRoutes) > 0 { + return true + } + } + return false +} + // PeerByTailscaleIP returns a peer's Node based on its Tailscale IP. // // If nm is nil or no peer is found, ok is false.