From e9505e54329847ae414e18a656ee8a54714d52a8 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Fri, 3 May 2024 10:59:22 -0400 Subject: [PATCH] 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 Change-Id: I41d79ebd61d64829a3a9e70586ce56f62d24ccfd --- ipn/ipnlocal/local.go | 3 +-- ipn/ipnlocal/local_test.go | 5 +++-- ipn/ipnlocal/network-lock_test.go | 19 ++++++++++--------- ipn/ipnlocal/peerapi_test.go | 9 +++++---- ipn/ipnlocal/profiles.go | 10 ++++++---- ipn/ipnlocal/profiles_test.go | 23 ++++++++++++----------- ipn/ipnlocal/serve_test.go | 3 ++- ipn/ipnlocal/ssh_test.go | 3 ++- 8 files changed, 41 insertions(+), 34 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 8c8e5ea76..fab8a76c1 100644 --- 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 index 88373106a..25ea71636 100644 --- 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 index 85da13d8b..34f739b0f 100644 --- 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 index 1e63a396e..1552cc210 100644 --- 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 index 4034f4f7b..8a092f3a1 100644 --- 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 index 101aeb9c9..6a8e15751 100644 --- 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 index 800e5a89e..686f49db3 100644 --- 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 index c4fe992c7..6e93b34f0 100644 --- 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))