cmd/tailscale: make "up", "status" warn if routes and --accept-routes off

Example output:

    # Health check:
    #     - Some peers are advertising routes but --accept-routes is false

Also, move "tailscale status" health checks to the bottom, where they
won't be lost in large netmaps.

Updates #2053
Updates #6266

Change-Id: I5ae76a0cd69a452ce70063875cd7d974bfeb8f1a
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

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")
+	}
+}

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
 }
 

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
+}

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+

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+

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
+)

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,7 +586,11 @@ 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 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 {
@@ -597,7 +602,7 @@ func (b *LocalBackend) updateStatus(sb *ipnstate.StatusBuilder, extraLocked func
 							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 {

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.