ipn/ipnlocal: plumb health.Tracker into profileManager constructor

Setting the field after-the-fact wasn't working because we could migrate prefs on creation, which would set health status for auto updates. Updates #11986 Signed-off-by: Andrew Dunham <andrew@du.nham.ca> Change-Id: I41d79ebd61d64829a3a9e70586ce56f62d24ccfd
tailscale · May 3, 2024 · e9505e5 · e9505e5
1 parent e42c439
commit e9505e5
Show file tree

Hide file tree

Showing 8 changed files with 41 additions and 34 deletions.
diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
@@ -364,11 +364,10 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo
 	if loginFlags&controlclient.LocalBackendStartKeyOSNeutral != 0 {
 		goos = ""
 	}
-	pm, err := newProfileManagerWithGOOS(store, logf, goos)
+	pm, err := newProfileManagerWithGOOS(store, logf, sys.HealthTracker(), goos)
 	if err != nil {
 		return nil, err
 	}
-	pm.health = sys.HealthTracker()
 	if sds, ok := store.(ipn.StateStoreDialerSetter); ok {
 		sds.SetDialer(dialer.SystemDial)
 	}

diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go
@@ -29,6 +29,7 @@ import (
 	"tailscale.com/control/controlclient"
 	"tailscale.com/drive"
 	"tailscale.com/drive/driveimpl"
+	"tailscale.com/health"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/store/mem"
 	"tailscale.com/net/netcheck"
@@ -1849,7 +1850,7 @@ func TestSetExitNodeIDPolicy(t *testing.T) {
 			if test.prefs == nil {
 				test.prefs = ipn.NewPrefs()
 			}
-			pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+			pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 			pm.prefs = test.prefs.View()
 			b.netMap = test.nm
 			b.pm = pm
@@ -2131,7 +2132,7 @@ func TestApplySysPolicy(t *testing.T) {
 					wantPrefs.ControlURL = ipn.DefaultControlURL
 				}
 
-				pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+				pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 				pm.prefs = usePrefs.View()
 
 				b := newTestBackend(t)

diff --git a/ipn/ipnlocal/network-lock_test.go b/ipn/ipnlocal/network-lock_test.go
@@ -17,6 +17,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"tailscale.com/control/controlclient"
+	"tailscale.com/health"
 	"tailscale.com/hostinfo"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/store/mem"
@@ -148,7 +149,7 @@ func TestTKAEnablementFlow(t *testing.T) {
 	temp := t.TempDir()
 
 	cc := fakeControlClient(t, client)
-	pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+	pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 	must.Do(pm.SetPrefs((&ipn.Prefs{
 		Persist: &persist.Persist{
 			PrivateNodeKey: nodePriv,
@@ -188,7 +189,7 @@ func TestTKADisablementFlow(t *testing.T) {
 	nlPriv := key.NewNLPrivate()
 	key := tka.Key{Kind: tka.Key25519, Public: nlPriv.Public().Verifier(), Votes: 2}
 
-	pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+	pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 	must.Do(pm.SetPrefs((&ipn.Prefs{
 		Persist: &persist.Persist{
 			PrivateNodeKey: nodePriv,
@@ -380,7 +381,7 @@ func TestTKASync(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			nodePriv := key.NewNode()
 			nlPriv := key.NewNLPrivate()
-			pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+			pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 			must.Do(pm.SetPrefs((&ipn.Prefs{
 				Persist: &persist.Persist{
 					PrivateNodeKey: nodePriv,
@@ -602,7 +603,7 @@ func TestTKADisable(t *testing.T) {
 	disablementSecret := bytes.Repeat([]byte{0xa5}, 32)
 	nlPriv := key.NewNLPrivate()
 
-	pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+	pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 	must.Do(pm.SetPrefs((&ipn.Prefs{
 		Persist: &persist.Persist{
 			PrivateNodeKey: nodePriv,
@@ -693,7 +694,7 @@ func TestTKASign(t *testing.T) {
 	toSign := key.NewNode()
 	nlPriv := key.NewNLPrivate()
 
-	pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+	pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 	must.Do(pm.SetPrefs((&ipn.Prefs{
 		Persist: &persist.Persist{
 			PrivateNodeKey: nodePriv,
@@ -782,7 +783,7 @@ func TestTKAForceDisable(t *testing.T) {
 	nlPriv := key.NewNLPrivate()
 	key := tka.Key{Kind: tka.Key25519, Public: nlPriv.Public().Verifier(), Votes: 2}
 
-	pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+	pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 	must.Do(pm.SetPrefs((&ipn.Prefs{
 		Persist: &persist.Persist{
 			PrivateNodeKey: nodePriv,
@@ -877,7 +878,7 @@ func TestTKAAffectedSigs(t *testing.T) {
 	// toSign := key.NewNode()
 	nlPriv := key.NewNLPrivate()
 
-	pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+	pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 	must.Do(pm.SetPrefs((&ipn.Prefs{
 		Persist: &persist.Persist{
 			PrivateNodeKey: nodePriv,
@@ -1010,7 +1011,7 @@ func TestTKARecoverCompromisedKeyFlow(t *testing.T) {
 	cosignPriv := key.NewNLPrivate()
 	compromisedPriv := key.NewNLPrivate()
 
-	pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+	pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 	must.Do(pm.SetPrefs((&ipn.Prefs{
 		Persist: &persist.Persist{
 			PrivateNodeKey: nodePriv,
@@ -1101,7 +1102,7 @@ func TestTKARecoverCompromisedKeyFlow(t *testing.T) {
 
 	// Cosign using the cosigning key.
 	{
-		pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+		pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 		must.Do(pm.SetPrefs((&ipn.Prefs{
 			Persist: &persist.Persist{
 				PrivateNodeKey: nodePriv,

diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go
@@ -26,6 +26,7 @@ import (
 	"tailscale.com/appc"
 	"tailscale.com/appc/appctest"
 	"tailscale.com/client/tailscale/apitype"
+	"tailscale.com/health"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/store/mem"
 	"tailscale.com/tailcfg"
@@ -642,7 +643,7 @@ func TestPeerAPIReplyToDNSQueries(t *testing.T) {
 	h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345")
 
 	eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0)
-	pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+	pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 	h.ps = &peerAPIServer{
 		b: &LocalBackend{
 			e:     eng,
@@ -692,7 +693,7 @@ func TestPeerAPIPrettyReplyCNAME(t *testing.T) {
 		h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345")
 
 		eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0)
-		pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+		pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 		var a *appc.AppConnector
 		if shouldStore {
 			a = appc.NewAppConnector(t.Logf, &appctest.RouteCollector{}, &appc.RouteInfo{}, fakeStoreRoutes)
@@ -764,7 +765,7 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) {
 
 		rc := &appctest.RouteCollector{}
 		eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0)
-		pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+		pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 		var a *appc.AppConnector
 		if shouldStore {
 			a = appc.NewAppConnector(t.Logf, rc, &appc.RouteInfo{}, fakeStoreRoutes)
@@ -827,7 +828,7 @@ func TestPeerAPIReplyToDNSQueriesAreObservedWithCNAMEFlattening(t *testing.T) {
 
 		rc := &appctest.RouteCollector{}
 		eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0)
-		pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+		pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 		var a *appc.AppConnector
 		if shouldStore {
 			a = appc.NewAppConnector(t.Logf, rc, &appc.RouteInfo{}, fakeStoreRoutes)

diff --git a/ipn/ipnlocal/profiles.go b/ipn/ipnlocal/profiles.go
@@ -485,7 +485,8 @@ func (pm *profileManager) CurrentPrefs() ipn.PrefsView {
 
 // ReadStartupPrefsForTest reads the startup prefs from disk. It is only used for testing.
 func ReadStartupPrefsForTest(logf logger.Logf, store ipn.StateStore) (ipn.PrefsView, error) {
-	pm, err := newProfileManager(store, logf)
+	ht := new(health.Tracker) // in tests, don't care about the health status
+	pm, err := newProfileManager(store, logf, ht)
 	if err != nil {
 		return ipn.PrefsView{}, err
 	}
@@ -494,8 +495,8 @@ func ReadStartupPrefsForTest(logf logger.Logf, store ipn.StateStore) (ipn.PrefsV
 
 // newProfileManager creates a new ProfileManager using the provided StateStore.
 // It also loads the list of known profiles from the StateStore.
-func newProfileManager(store ipn.StateStore, logf logger.Logf) (*profileManager, error) {
-	return newProfileManagerWithGOOS(store, logf, envknob.GOOS())
+func newProfileManager(store ipn.StateStore, logf logger.Logf, health *health.Tracker) (*profileManager, error) {
+	return newProfileManagerWithGOOS(store, logf, health, envknob.GOOS())
 }
 
 func readAutoStartKey(store ipn.StateStore, goos string) (ipn.StateKey, error) {
@@ -528,7 +529,7 @@ func readKnownProfiles(store ipn.StateStore) (map[ipn.ProfileID]*ipn.LoginProfil
 	return knownProfiles, nil
 }
 
-func newProfileManagerWithGOOS(store ipn.StateStore, logf logger.Logf, goos string) (*profileManager, error) {
+func newProfileManagerWithGOOS(store ipn.StateStore, logf logger.Logf, ht *health.Tracker, goos string) (*profileManager, error) {
 	logf = logger.WithPrefix(logf, "pm: ")
 	stateKey, err := readAutoStartKey(store, goos)
 	if err != nil {
@@ -544,6 +545,7 @@ func newProfileManagerWithGOOS(store ipn.StateStore, logf logger.Logf, goos stri
 		store:         store,
 		knownProfiles: knownProfiles,
 		logf:          logf,
+		health:        ht,
 	}
 
 	if stateKey != "" {

diff --git a/ipn/ipnlocal/profiles_test.go b/ipn/ipnlocal/profiles_test.go
@@ -12,6 +12,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"tailscale.com/clientupdate"
+	"tailscale.com/health"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/store/mem"
 	"tailscale.com/tailcfg"
@@ -24,7 +25,7 @@ import (
 func TestProfileCurrentUserSwitch(t *testing.T) {
 	store := new(mem.Store)
 
-	pm, err := newProfileManagerWithGOOS(store, logger.Discard, "linux")
+	pm, err := newProfileManagerWithGOOS(store, logger.Discard, new(health.Tracker), "linux")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -61,7 +62,7 @@ func TestProfileCurrentUserSwitch(t *testing.T) {
 		t.Fatalf("CurrentPrefs() = %v, want emptyPrefs", pm.CurrentPrefs().Pretty())
 	}
 
-	pm, err = newProfileManagerWithGOOS(store, logger.Discard, "linux")
+	pm, err = newProfileManagerWithGOOS(store, logger.Discard, new(health.Tracker), "linux")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -79,7 +80,7 @@ func TestProfileCurrentUserSwitch(t *testing.T) {
 func TestProfileList(t *testing.T) {
 	store := new(mem.Store)
 
-	pm, err := newProfileManagerWithGOOS(store, logger.Discard, "linux")
+	pm, err := newProfileManagerWithGOOS(store, logger.Discard, new(health.Tracker), "linux")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -283,7 +284,7 @@ func TestProfileDupe(t *testing.T) {
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			store := new(mem.Store)
-			pm, err := newProfileManagerWithGOOS(store, logger.Discard, "linux")
+			pm, err := newProfileManagerWithGOOS(store, logger.Discard, new(health.Tracker), "linux")
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -316,7 +317,7 @@ func TestProfileDupe(t *testing.T) {
 func TestProfileManagement(t *testing.T) {
 	store := new(mem.Store)
 
-	pm, err := newProfileManagerWithGOOS(store, logger.Discard, "linux")
+	pm, err := newProfileManagerWithGOOS(store, logger.Discard, new(health.Tracker), "linux")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -414,7 +415,7 @@ func TestProfileManagement(t *testing.T) {
 	t.Logf("Recreate profile manager from store")
 	// Recreate the profile manager to ensure that it can load the profiles
 	// from the store at startup.
-	pm, err = newProfileManagerWithGOOS(store, logger.Discard, "linux")
+	pm, err = newProfileManagerWithGOOS(store, logger.Discard, new(health.Tracker), "linux")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -430,7 +431,7 @@ func TestProfileManagement(t *testing.T) {
 	t.Logf("Recreate profile manager from store after deleting default profile")
 	// Recreate the profile manager to ensure that it can load the profiles
 	// from the store at startup.
-	pm, err = newProfileManagerWithGOOS(store, logger.Discard, "linux")
+	pm, err = newProfileManagerWithGOOS(store, logger.Discard, new(health.Tracker), "linux")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -472,7 +473,7 @@ func TestProfileManagement(t *testing.T) {
 			t.Fatal("SetPrefs failed to save auto-update setting")
 		}
 		// Re-load profiles to trigger migration for invalid auto-update value.
-		pm, err = newProfileManagerWithGOOS(store, logger.Discard, "linux")
+		pm, err = newProfileManagerWithGOOS(store, logger.Discard, new(health.Tracker), "linux")
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -494,7 +495,7 @@ func TestProfileManagementWindows(t *testing.T) {
 
 	store := new(mem.Store)
 
-	pm, err := newProfileManagerWithGOOS(store, logger.Discard, "windows")
+	pm, err := newProfileManagerWithGOOS(store, logger.Discard, new(health.Tracker), "windows")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -565,7 +566,7 @@ func TestProfileManagementWindows(t *testing.T) {
 	t.Logf("Recreate profile manager from store, should reset prefs")
 	// Recreate the profile manager to ensure that it can load the profiles
 	// from the store at startup.
-	pm, err = newProfileManagerWithGOOS(store, logger.Discard, "windows")
+	pm, err = newProfileManagerWithGOOS(store, logger.Discard, new(health.Tracker), "windows")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -590,7 +591,7 @@ func TestProfileManagementWindows(t *testing.T) {
 	}
 
 	// Recreate the profile manager to ensure that it starts with test profile.
-	pm, err = newProfileManagerWithGOOS(store, logger.Discard, "windows")
+	pm, err = newProfileManagerWithGOOS(store, logger.Discard, new(health.Tracker), "windows")
 	if err != nil {
 		t.Fatal(err)
 	}

diff --git a/ipn/ipnlocal/serve_test.go b/ipn/ipnlocal/serve_test.go
@@ -24,6 +24,7 @@ import (
 	"testing"
 	"time"
 
+	"tailscale.com/health"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/store/mem"
 	"tailscale.com/tailcfg"
@@ -686,7 +687,7 @@ func newTestBackend(t *testing.T) *LocalBackend {
 	dir := t.TempDir()
 	b.SetVarRoot(dir)
 
-	pm := must.Get(newProfileManager(new(mem.Store), logf))
+	pm := must.Get(newProfileManager(new(mem.Store), logf, new(health.Tracker)))
 	pm.currentProfile = &ipn.LoginProfile{ID: "id0"}
 	b.pm = pm
 

diff --git a/ipn/ipnlocal/ssh_test.go b/ipn/ipnlocal/ssh_test.go
@@ -10,6 +10,7 @@ import (
 	"reflect"
 	"testing"
 
+	"tailscale.com/health"
 	"tailscale.com/ipn/store/mem"
 	"tailscale.com/tailcfg"
 	"tailscale.com/util/must"
@@ -49,7 +50,7 @@ type fakeSSHServer struct {
 }
 
 func TestGetSSHUsernames(t *testing.T) {
-	pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+	pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker)))
 	b := &LocalBackend{pm: pm, store: pm.Store()}
 	b.sshServer = fakeSSHServer{}
 	res, err := b.getSSHUsernames(new(tailcfg.C2NSSHUsernamesRequest))