ipn/ipnlocal: do not reset the netmap and packet filter in (*LocalBackend).Start()

Resetting LocalBackend's netmap without also unconfiguring wgengine to reset routes, DNS, and the killswitch
firewall rules may cause connectivity issues until a new netmap is received.

In some cases, such as when bootstrap DNS servers are inaccessible due to network restrictions or other reasons,
or if the control plane is experiencing issues, this can result in a complete loss of connectivity until the user disconnects
and reconnects to Tailscale.

As LocalBackend handles state resets in (*LocalBackend).resetForProfileChangeLockedOnEntry(), and this includes
resetting the netmap, resetting the current netmap in (*LocalBackend).Start() is not necessary.
Moreover, it's harmful if (*LocalBackend).Start() is called more than once for the same profile.

In this PR, we update resetForProfileChangeLockedOnEntry() to reset the packet filter and remove
the redundant resetting of the netmap and packet filter from Start(). We also update the state machine
tests and revise comments that became inaccurate due to previous test updates.

Updates tailscale/corp#27173

Signed-off-by: Nick Khyl <nickk@tailscale.com>

ipn/ipnlocal/local.go

@@ -2380,12 +2380,10 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 	}
 	b.applyPrefsToHostinfoLocked(hostinfo, prefs)
 
-	b.setNetMapLocked(nil)
 	persistv := prefs.Persist().AsStruct()
 	if persistv == nil {
 		persistv = new(persist.Persist)
 	}
-	b.updateFilterLocked(nil, ipn.PrefsView{})
 
 	if b.portpoll != nil {
 		b.portpollOnce.Do(func() {
@@ -7531,6 +7529,7 @@ func (b *LocalBackend) resetForProfileChangeLockedOnEntry(unlock unlockOnce) err
 		return nil
 	}
 	b.setNetMapLocked(nil) // Reset netmap.
+	b.updateFilterLocked(nil, ipn.PrefsView{})
 	// Reset the NetworkMap in the engine
 	b.e.SetNetworkMap(new(netmap.NetworkMap))
 	if prevCC := b.resetControlClientLocked(); prevCC != nil {

ipn/ipnlocal/local_test.go

@@ -1510,6 +1510,15 @@ func TestReconfigureAppConnector(t *testing.T) {
 func TestBackfillAppConnectorRoutes(t *testing.T) {
 	// Create backend with an empty app connector.
 	b := newTestBackend(t)
+	// newTestBackend creates a backend with a non-nil netmap,
+	// but this test requires a nil netmap.
+	// Otherwise, instead of backfilling, [LocalBackend.reconfigAppConnectorLocked]
+	// uses the domains and routes from netmap's [appctype.AppConnectorAttr].
+	// Additionally, a non-nil netmap makes reconfigAppConnectorLocked
+	// asynchronous, resulting in a flaky test.
+	// Therefore, we set the netmap to nil to simulate a fresh backend start
+	// or a profile switch where the netmap is not yet available.
+	b.setNetMapLocked(nil)
 	if err := b.Start(ipn.Options{}); err != nil {
 		t.Fatal(err)
 	}

ipn/ipnlocal/state_test.go

@@ -735,12 +735,10 @@ func TestStateMachine(t *testing.T) {
 		// b.Shutdown() explicitly ourselves.
 		previousCC.assertShutdown(false)
 
-		// Note: unpause happens because ipn needs to get at least one netmap
-		//  on startup, otherwise UIs can't show the node list, login
-		//  name, etc when in state ipn.Stopped.
-		//  Arguably they shouldn't try. But they currently do.
 		nn := notifies.drain(2)
-		cc.assertCalls("New", "Login")
+		// We already have a netmap for this node,
+		// and WantRunning is false, so cc should be paused.
+		cc.assertCalls("New", "Login", "pause")
 		c.Assert(nn[0].Prefs, qt.IsNotNil)
 		c.Assert(nn[1].State, qt.IsNotNil)
 		c.Assert(nn[0].Prefs.WantRunning(), qt.IsFalse)
@@ -751,7 +749,11 @@ func TestStateMachine(t *testing.T) {
 	// When logged in but !WantRunning, ipn leaves us unpaused to retrieve
 	// the first netmap. Simulate that netmap being received, after which
 	// it should pause us, to avoid wasting CPU retrieving unnecessarily
-	// additional netmap updates.
+	// additional netmap updates. Since our LocalBackend instance already
+	// has a netmap, we will reset it to nil to simulate the first netmap
+	// retrieval.
+	b.setNetMapLocked(nil)
+	cc.assertCalls("unpause")
 	//
 	// TODO: really the various GUIs and prefs should be refactored to
 	//  not require the netmap structure at all when starting while
@@ -853,7 +855,7 @@ func TestStateMachine(t *testing.T) {
 	// The last test case is the most common one: restarting when both
 	// logged in and WantRunning.
 	t.Logf("\n\nStart5")
-	notifies.expect(1)
+	notifies.expect(2)
 	c.Assert(b.Start(ipn.Options{}), qt.IsNil)
 	{
 		// NOTE: cc.Shutdown() is correct here, since we didn't call
@@ -861,30 +863,32 @@ func TestStateMachine(t *testing.T) {
 		previousCC.assertShutdown(false)
 		cc.assertCalls("New", "Login")
 
-		nn := notifies.drain(1)
+		nn := notifies.drain(2)
 		cc.assertCalls()
 		c.Assert(nn[0].Prefs, qt.IsNotNil)
 		c.Assert(nn[0].Prefs.LoggedOut(), qt.IsFalse)
 		c.Assert(nn[0].Prefs.WantRunning(), qt.IsTrue)
-		c.Assert(b.State(), qt.Equals, ipn.NoState)
+		// We're logged in and have a valid netmap, so we should
+		// be in the Starting state.
+		c.Assert(nn[1].State, qt.IsNotNil)
+		c.Assert(*nn[1].State, qt.Equals, ipn.Starting)
+		c.Assert(b.State(), qt.Equals, ipn.Starting)
 	}
 
 	// Control server accepts our valid key from before.
 	t.Logf("\n\nLoginFinished5")
-	notifies.expect(1)
+	notifies.expect(0)
 	cc.send(nil, "", true, &netmap.NetworkMap{
 		SelfNode: (&tailcfg.Node{MachineAuthorized: true}).View(),
 	})
 	{
-		nn := notifies.drain(1)
+		notifies.drain(0)
 		cc.assertCalls()
 		// NOTE: No LoginFinished message since no interactive
 		// login was needed.
-		c.Assert(nn[0].State, qt.IsNotNil)
-		c.Assert(ipn.Starting, qt.Equals, *nn[0].State)
 		// NOTE: No prefs change this time. WantRunning stays true.
 		// We were in Starting in the first place, so that doesn't
-		// change either.
+		// change either, so we don't expect any notifications.
 		c.Assert(ipn.Starting, qt.Equals, b.State())
 	}
 	t.Logf("\n\nExpireKey")