health, ipn/ipnlocal: move more health warning code into health.Tracker

In prep for making health warnings rich objects with metadata rather
than a bunch of strings, start moving it all into the same place.

We'll still ultimately need the stringified form for the CLI and
LocalAPI for compatibility but we'll next convert all these warnings
into Warnables that have severity levels and such, and legacy
stringification will just be something each Warnable thing can do.

Updates #4136

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

health/health.go

@@ -23,6 +23,7 @@ import (
 	"tailscale.com/util/mak"
 	"tailscale.com/util/multierr"
 	"tailscale.com/util/set"
+	"tailscale.com/version"
 )
 
 var (
@@ -72,6 +73,9 @@ type Tracker struct {
 	watchers set.HandleSet[func(Subsystem, error)] // opt func to run if error state changes
 	timer    *time.Timer
 
+	latestVersion   *tailcfg.ClientVersion // or nil
+	checkForUpdates bool
+
 	inMapPoll               bool
 	inMapPollSince          time.Time
 	lastMapPollEndedAt      time.Time
@@ -543,7 +547,37 @@ func (t *Tracker) SetAuthRoutineInError(err error) {
 	}
 	t.mu.Lock()
 	defer t.mu.Unlock()
+	if err == nil && t.lastLoginErr == nil {
+		return
+	}
 	t.lastLoginErr = err
+	t.selfCheckLocked()
+}
+
+// SetLatestVersion records the latest version of the Tailscale client.
+// v can be nil if unknown.
+func (t *Tracker) SetLatestVersion(v *tailcfg.ClientVersion) {
+	if t.nil() {
+		return
+	}
+	t.mu.Lock()
+	defer t.mu.Unlock()
+	t.latestVersion = v
+	t.selfCheckLocked()
+}
+
+// SetCheckForUpdates sets whether the client wants to check for updates.
+func (t *Tracker) SetCheckForUpdates(v bool) {
+	if t.nil() {
+		return
+	}
+	t.mu.Lock()
+	defer t.mu.Unlock()
+	if t.checkForUpdates == v {
+		return
+	}
+	t.checkForUpdates = v
+	t.selfCheckLocked()
 }
 
 func (t *Tracker) timerSelfCheck() {
@@ -567,6 +601,23 @@ func (t *Tracker) selfCheckLocked() {
 	t.setLocked(SysOverall, t.overallErrorLocked())
 }
 
+// AppendWarnings appends all current health warnings to dst and returns the
+// result.
+func (t *Tracker) AppendWarnings(dst []string) []string {
+	err := t.OverallError()
+	if err == nil {
+		return dst
+	}
+	if me, ok := err.(multierr.Error); ok {
+		for _, err := range me.Errors() {
+			dst = append(dst, err.Error())
+		}
+	} else {
+		dst = append(dst, err.Error())
+	}
+	return dst
+}
+
 // OverallError returns a summary of the health state.
 //
 // If there are multiple problems, the error will be of type
@@ -609,42 +660,76 @@ var errNetworkDown = errors.New("network down")
 var errNotInMapPoll = errors.New("not in map poll")
 var errNoDERPHome = errors.New("no DERP home")
 var errNoUDP4Bind = errors.New("no udp4 bind")
+var errUnstable = errors.New("This is an unstable (development) version of Tailscale; frequent updates and bugs are likely")
 
 func (t *Tracker) overallErrorLocked() error {
+	var errs []error
+	add := func(err error) {
+		if err != nil {
+			errs = append(errs, err)
+		}
+	}
+	merged := func() error {
+		return multierr.New(errs...)
+	}
+
+	if t.checkForUpdates {
+		if cv := t.latestVersion; cv != nil && !cv.RunningLatest && cv.LatestVersion != "" {
+			if cv.UrgentSecurityUpdate {
+				add(fmt.Errorf("Security update available: %v -> %v, run `tailscale update` or `tailscale set --auto-update` to update", version.Short(), cv.LatestVersion))
+			} else {
+				add(fmt.Errorf("Update available: %v -> %v, run `tailscale update` or `tailscale set --auto-update` to update", version.Short(), cv.LatestVersion))
+			}
+		}
+	}
+	if version.IsUnstableBuild() {
+		add(errUnstable)
+	}
+
 	if v, ok := t.anyInterfaceUp.Get(); ok && !v {
-		return errNetworkDown
+		add(errNetworkDown)
+		return merged()
 	}
 	if t.localLogConfigErr != nil {
-		return t.localLogConfigErr
+		add(t.localLogConfigErr)
+		return merged()
 	}
 	if !t.ipnWantRunning {
-		return fmt.Errorf("state=%v, wantRunning=%v", t.ipnState, t.ipnWantRunning)
+		add(fmt.Errorf("state=%v, wantRunning=%v", t.ipnState, t.ipnWantRunning))
+		return merged()
 	}
 	if t.lastLoginErr != nil {
-		return fmt.Errorf("not logged in, last login error=%v", t.lastLoginErr)
+		add(fmt.Errorf("not logged in, last login error=%v", t.lastLoginErr))
+		return merged()
 	}
 	now := time.Now()
 	if !t.inMapPoll && (t.lastMapPollEndedAt.IsZero() || now.Sub(t.lastMapPollEndedAt) > 10*time.Second) {
-		return errNotInMapPoll
+		add(errNotInMapPoll)
+		return merged()
 	}
 	const tooIdle = 2*time.Minute + 5*time.Second
 	if d := now.Sub(t.lastStreamedMapResponse).Round(time.Second); d > tooIdle {
-		return t.networkErrorfLocked("no map response in %v", d)
+		add(t.networkErrorfLocked("no map response in %v", d))
+		return merged()
 	}
 	if !t.derpHomeless {
 		rid := t.derpHomeRegion
 		if rid == 0 {
-			return errNoDERPHome
+			add(errNoDERPHome)
+			return merged()
 		}
 		if !t.derpRegionConnected[rid] {
-			return t.networkErrorfLocked("not connected to home DERP region %v", rid)
+			add(t.networkErrorfLocked("not connected to home DERP region %v", rid))
+			return merged()
 		}
 		if d := now.Sub(t.derpRegionLastFrame[rid]).Round(time.Second); d > tooIdle {
-			return t.networkErrorfLocked("haven't heard from home DERP region %v in %v", rid, d)
+			add(t.networkErrorfLocked("haven't heard from home DERP region %v in %v", rid, d))
+			return merged()
 		}
 	}
 	if t.udp4Unbound {
-		return errNoUDP4Bind
+		add(errNoUDP4Bind)
+		return merged()
 	}
 
 	// TODO: use
@@ -653,7 +738,6 @@ func (t *Tracker) overallErrorLocked() error {
 	_ = t.lastStreamedMapResponse
 	_ = t.lastMapRequestHeard
 
-	var errs []error
 	for i := range t.MagicSockReceiveFuncs {
 		f := &t.MagicSockReceiveFuncs[i]
 		if f.missing {

ipn/ipnlocal/local.go

@@ -368,6 +368,7 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo
 	if err != nil {
 		return nil, err
 	}
+	pm.health = sys.HealthTracker()
 	if sds, ok := store.(ipn.StateStoreDialerSetter); ok {
 		sds.SetDialer(dialer.SystemDial)
 	}
@@ -770,30 +771,14 @@ func (b *LocalBackend) UpdateStatus(sb *ipnstate.StatusBuilder) {
 		s.AuthURL = b.authURLSticky
 		if prefs := b.pm.CurrentPrefs(); prefs.Valid() && prefs.AutoUpdate().Check {
 			s.ClientVersion = b.lastClientVersion
-			if cv := b.lastClientVersion; cv != nil && !cv.RunningLatest && cv.LatestVersion != "" {
-				if cv.UrgentSecurityUpdate {
-					s.Health = append(s.Health, fmt.Sprintf("Security update available: %v -> %v, run `tailscale update` or `tailscale set --auto-update` to update", version.Short(), cv.LatestVersion))
-				} else {
-					s.Health = append(s.Health, fmt.Sprintf("Update available: %v -> %v, run `tailscale update` or `tailscale set --auto-update` to update", version.Short(), cv.LatestVersion))
-				}
-			}
-		}
-		if err := b.health.OverallError(); err != nil {
-			switch e := err.(type) {
-			case multierr.Error:
-				for _, err := range e.Errors() {
-					s.Health = append(s.Health, err.Error())
-				}
-			default:
-				s.Health = append(s.Health, err.Error())
-			}
 		}
+		s.Health = b.health.AppendWarnings(s.Health)
+
+		// TODO(bradfitz): move this health check into a health.Warnable
+		// and remove from here.
 		if m := b.sshOnButUnusableHealthCheckMessageLocked(); m != "" {
 			s.Health = append(s.Health, m)
 		}
-		if version.IsUnstableBuild() {
-			s.Health = append(s.Health, "This is an unstable (development) version of Tailscale; frequent updates and bugs are likely")
-		}
 		if b.netMap != nil {
 			s.CertDomains = append([]string(nil), b.netMap.DNS.CertDomains...)
 			s.MagicDNSSuffix = b.netMap.MagicDNSSuffix()
@@ -2517,6 +2502,7 @@ func (b *LocalBackend) tellClientToBrowseToURL(url string) {
 func (b *LocalBackend) onClientVersion(v *tailcfg.ClientVersion) {
 	b.mu.Lock()
 	b.lastClientVersion = v
+	b.health.SetLatestVersion(v)
 	b.mu.Unlock()
 	b.send(ipn.Notify{ClientVersion: v})
 }

ipn/ipnlocal/profiles.go

@@ -16,6 +16,7 @@ import (
 
 	"tailscale.com/clientupdate"
 	"tailscale.com/envknob"
+	"tailscale.com/health"
 	"tailscale.com/ipn"
 	"tailscale.com/types/logger"
 	"tailscale.com/util/clientmetric"
@@ -30,8 +31,9 @@ var debug = envknob.RegisterBool("TS_DEBUG_PROFILES")
 //
 // It is not safe for concurrent use.
 type profileManager struct {
-	store ipn.StateStore
+	store  ipn.StateStore
-	logf  logger.Logf
+	logf   logger.Logf
+	health *health.Tracker
 
 	currentUserID  ipn.WindowsUserID
 	knownProfiles  map[ipn.ProfileID]*ipn.LoginProfile // always non-nil
@@ -102,6 +104,7 @@ func (pm *profileManager) SetCurrentUserID(uid ipn.WindowsUserID) error {
 	}
 	pm.currentProfile = prof
 	pm.prefs = prefs
+	pm.updateHealth()
 	return nil
 }
 
@@ -285,6 +288,7 @@ func newUnusedID(knownProfiles map[ipn.ProfileID]*ipn.LoginProfile) (ipn.Profile
 // is not new.
 func (pm *profileManager) setPrefsLocked(clonedPrefs ipn.PrefsView) error {
 	pm.prefs = clonedPrefs
+	pm.updateHealth()
 	if pm.currentProfile.ID == "" {
 		return nil
 	}
@@ -336,6 +340,7 @@ func (pm *profileManager) SwitchProfile(id ipn.ProfileID) error {
 		return err
 	}
 	pm.prefs = prefs
+	pm.updateHealth()
 	pm.currentProfile = kp
 	return pm.setAsUserSelectedProfileLocked()
 }
@@ -443,12 +448,20 @@ func (pm *profileManager) writeKnownProfiles() error {
 	return pm.WriteState(ipn.KnownProfilesStateKey, b)
 }
 
+func (pm *profileManager) updateHealth() {
+	if !pm.prefs.Valid() {
+		return
+	}
+	pm.health.SetCheckForUpdates(pm.prefs.AutoUpdate().Check)
+}
+
 // NewProfile creates and switches to a new unnamed profile. The new profile is
 // not persisted until SetPrefs is called with a logged-in user.
 func (pm *profileManager) NewProfile() {
 	metricNewProfile.Add(1)
 
 	pm.prefs = defaultPrefs
+	pm.updateHealth()
 	pm.currentProfile = &ipn.LoginProfile{}
 }