From 2064dc20d4749fb84bdab7f1369fe81503cde03e Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Thu, 27 Jun 2024 09:36:29 -0700 Subject: [PATCH] health,ipn/ipnlocal: hide update warning when auto-updates are enabled (#12631) When auto-udpates are enabled, we don't need to nag users to update after a new release, before we release auto-updates. Updates https://github.com/tailscale/corp/issues/20081 Signed-off-by: Andrew Lytvynov --- health/health.go | 52 +++++++++++++++-------- health/health_test.go | 89 ++++++++++++++++++++++++++++++++++++++++ ipn/ipnlocal/profiles.go | 2 +- 3 files changed, 124 insertions(+), 19 deletions(-) diff --git a/health/health.go b/health/health.go index 437c3465d..c8cf3e15f 100644 --- a/health/health.go +++ b/health/health.go @@ -78,6 +78,7 @@ type Tracker struct { latestVersion *tailcfg.ClientVersion // or nil checkForUpdates bool + applyUpdates opt.Bool inMapPoll bool inMapPollSince time.Time @@ -782,17 +783,20 @@ func (t *Tracker) SetLatestVersion(v *tailcfg.ClientVersion) { t.selfCheckLocked() } -// SetCheckForUpdates sets whether the client wants to check for updates. -func (t *Tracker) SetCheckForUpdates(v bool) { +// SetAutoUpdatePrefs sets the client auto-update preferences. The arguments +// match the fields of ipn.AutoUpdatePrefs, but we cannot pass that struct +// directly due to a circular import. +func (t *Tracker) SetAutoUpdatePrefs(check bool, apply opt.Bool) { if t.nil() { return } t.mu.Lock() defer t.mu.Unlock() - if t.checkForUpdates == v { + if t.checkForUpdates == check && t.applyUpdates == apply { return } - t.checkForUpdates = v + t.checkForUpdates = check + t.applyUpdates = apply t.selfCheckLocked() } @@ -883,20 +887,14 @@ func (t *Tracker) multiErrLocked() error { func (t *Tracker) updateBuiltinWarnablesLocked() { t.updateWarmingUpWarnableLocked() - if t.checkForUpdates { - if cv := t.latestVersion; cv != nil && !cv.RunningLatest && cv.LatestVersion != "" { - if cv.UrgentSecurityUpdate { - t.setUnhealthyLocked(securityUpdateAvailableWarnable, Args{ - ArgCurrentVersion: version.Short(), - ArgAvailableVersion: cv.LatestVersion, - }) - } else { - t.setUnhealthyLocked(updateAvailableWarnable, Args{ - ArgCurrentVersion: version.Short(), - ArgAvailableVersion: cv.LatestVersion, - }) - } - } + if w, show := t.showUpdateWarnable(); show { + t.setUnhealthyLocked(w, Args{ + ArgCurrentVersion: version.Short(), + ArgAvailableVersion: t.latestVersion.LatestVersion, + }) + } else { + t.setHealthyLocked(updateAvailableWarnable) + t.setHealthyLocked(securityUpdateAvailableWarnable) } if version.IsUnstableBuild() { @@ -1070,6 +1068,24 @@ func (t *Tracker) updateWarmingUpWarnableLocked() { } } +func (t *Tracker) showUpdateWarnable() (*Warnable, bool) { + if !t.checkForUpdates { + return nil, false + } + cv := t.latestVersion + if cv == nil || cv.RunningLatest || cv.LatestVersion == "" { + return nil, false + } + if cv.UrgentSecurityUpdate { + return securityUpdateAvailableWarnable, true + } + // Only show update warning when auto-updates are off + if !t.applyUpdates.EqualBool(true) { + return updateAvailableWarnable, true + } + return nil, false +} + // ReceiveFuncStats tracks the calls made to a wireguard-go receive func. type ReceiveFuncStats struct { // name is the name of the receive func. diff --git a/health/health_test.go b/health/health_test.go index 9ef49b642..c5b7d865a 100644 --- a/health/health_test.go +++ b/health/health_test.go @@ -9,6 +9,9 @@ "slices" "testing" "time" + + "tailscale.com/tailcfg" + "tailscale.com/types/opt" ) func TestAppendWarnableDebugFlags(t *testing.T) { @@ -214,3 +217,89 @@ func TestCheckDependsOnAppearsInUnhealthyState(t *testing.T) { t.Fatalf("Expected DependsOn = %v in the unhealthy state, got: %v", wantDependsOn, us2.DependsOn) } } + +func TestShowUpdateWarnable(t *testing.T) { + tests := []struct { + desc string + check bool + apply opt.Bool + cv *tailcfg.ClientVersion + wantWarnable *Warnable + wantShow bool + }{ + { + desc: "nil CientVersion", + check: true, + cv: nil, + wantWarnable: nil, + wantShow: false, + }, + { + desc: "RunningLatest", + check: true, + cv: &tailcfg.ClientVersion{RunningLatest: true}, + wantWarnable: nil, + wantShow: false, + }, + { + desc: "no LatestVersion", + check: true, + cv: &tailcfg.ClientVersion{RunningLatest: false, LatestVersion: ""}, + wantWarnable: nil, + wantShow: false, + }, + { + desc: "show regular update", + check: true, + cv: &tailcfg.ClientVersion{RunningLatest: false, LatestVersion: "1.2.3"}, + wantWarnable: updateAvailableWarnable, + wantShow: true, + }, + { + desc: "show security update", + check: true, + cv: &tailcfg.ClientVersion{RunningLatest: false, LatestVersion: "1.2.3", UrgentSecurityUpdate: true}, + wantWarnable: securityUpdateAvailableWarnable, + wantShow: true, + }, + { + desc: "update check disabled", + check: false, + cv: &tailcfg.ClientVersion{RunningLatest: false, LatestVersion: "1.2.3"}, + wantWarnable: nil, + wantShow: false, + }, + { + desc: "hide update with auto-updates", + check: true, + apply: opt.NewBool(true), + cv: &tailcfg.ClientVersion{RunningLatest: false, LatestVersion: "1.2.3"}, + wantWarnable: nil, + wantShow: false, + }, + { + desc: "show security update with auto-updates", + check: true, + apply: opt.NewBool(true), + cv: &tailcfg.ClientVersion{RunningLatest: false, LatestVersion: "1.2.3", UrgentSecurityUpdate: true}, + wantWarnable: securityUpdateAvailableWarnable, + wantShow: true, + }, + } + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + tr := &Tracker{ + checkForUpdates: tt.check, + applyUpdates: tt.apply, + latestVersion: tt.cv, + } + gotWarnable, gotShow := tr.showUpdateWarnable() + if gotWarnable != tt.wantWarnable { + t.Errorf("got warnable: %v, want: %v", gotWarnable, tt.wantWarnable) + } + if gotShow != tt.wantShow { + t.Errorf("got show: %v, want: %v", gotShow, tt.wantShow) + } + }) + } +} diff --git a/ipn/ipnlocal/profiles.go b/ipn/ipnlocal/profiles.go index b3c688160..05286665e 100644 --- a/ipn/ipnlocal/profiles.go +++ b/ipn/ipnlocal/profiles.go @@ -448,7 +448,7 @@ func (pm *profileManager) updateHealth() { if !pm.prefs.Valid() { return } - pm.health.SetCheckForUpdates(pm.prefs.AutoUpdate().Check) + pm.health.SetAutoUpdatePrefs(pm.prefs.AutoUpdate().Check, pm.prefs.AutoUpdate().Apply) } // NewProfile creates and switches to a new unnamed profile. The new profile is