ipnlocal: don't assume NeedsLogin immediately after StartLogout().

Previously, there was no server round trip required to log out, so when
you asked ipnlocal to Logout(), it could clear the netmap immediately
and switch to NeedsLogin state.

In v1.8, we added a true Logout operation. ipn.Logout() would trigger
an async cc.StartLogout() and *also* immediately switch to NeedsLogin.
Unfortunately, some frontends would see NeedsLogin and immediately
trigger a new StartInteractiveLogin() operation, before the
controlclient auth state machine actually acted on the Logout command,
thus accidentally invalidating the entire logout operation, retaining
the netmap, and violating the user's expectations.

Instead, add a new LogoutFinished signal from controlclient
(paralleling LoginFinished) and, upon starting a logout, don't update
the ipn state machine until it's received.

Updates: #1918 (BUG-2)
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>

control/controlclient/auto.go

@@ -576,9 +576,12 @@ func (c *Auto) sendStatus(who string, err error, url string, nm *netmap.NetworkM
 	c.logf("[v1] sendStatus: %s: %v", who, state)
 
 	var p *persist.Persist
-	var fin *empty.Message
+	var loginFin, logoutFin *empty.Message
 	if state == StateAuthenticated {
-		fin = new(empty.Message)
+		loginFin = new(empty.Message)
+	}
+	if state == StateNotAuthenticated {
+		logoutFin = new(empty.Message)
 	}
 	if nm != nil && loggedIn && synced {
 		pp := c.direct.GetPersist()
@@ -589,12 +592,13 @@ func (c *Auto) sendStatus(who string, err error, url string, nm *netmap.NetworkM
 		nm = nil
 	}
 	new := Status{
-		LoginFinished: fin,
-		URL:           url,
-		Persist:       p,
-		NetMap:        nm,
-		Hostinfo:      hi,
-		State:         state,
+		LoginFinished:  loginFin,
+		LogoutFinished: logoutFin,
+		URL:            url,
+		Persist:        p,
+		NetMap:         nm,
+		Hostinfo:       hi,
+		State:          state,
 	}
 	if err != nil {
 		new.Err = err.Error()

control/controlclient/controlclient_test.go

@@ -22,7 +22,7 @@ func fieldsOf(t reflect.Type) (fields []string) {
 
 func TestStatusEqual(t *testing.T) {
 	// Verify that the Equal method stays in sync with reality
-	equalHandles := []string{"LoginFinished", "Err", "URL", "NetMap", "State", "Persist", "Hostinfo"}
+	equalHandles := []string{"LoginFinished", "LogoutFinished", "Err", "URL", "NetMap", "State", "Persist", "Hostinfo"}
 	if have := fieldsOf(reflect.TypeOf(Status{})); !reflect.DeepEqual(have, equalHandles) {
 		t.Errorf("Status.Equal check might be out of sync\nfields: %q\nhandled: %q\n",
 			have, equalHandles)

control/controlclient/status.go

@@ -64,11 +64,12 @@ func (s State) String() string {
 }
 
 type Status struct {
-	_             structs.Incomparable
-	LoginFinished *empty.Message // nonempty when login finishes
-	Err           string
-	URL           string             // interactive URL to visit to finish logging in
-	NetMap        *netmap.NetworkMap // server-pushed configuration
+	_              structs.Incomparable
+	LoginFinished  *empty.Message // nonempty when login finishes
+	LogoutFinished *empty.Message // nonempty when logout finishes
+	Err            string
+	URL            string             // interactive URL to visit to finish logging in
+	NetMap         *netmap.NetworkMap // server-pushed configuration
 
 	// The internal state should not be exposed outside this
 	// package, but we have some automated tests elsewhere that need to
@@ -86,6 +87,7 @@ func (s *Status) Equal(s2 *Status) bool {
 	}
 	return s != nil && s2 != nil &&
 		(s.LoginFinished == nil) == (s2.LoginFinished == nil) &&
+		(s.LogoutFinished == nil) == (s2.LogoutFinished == nil) &&
 		s.Err == s2.Err &&
 		s.URL == s2.URL &&
 		reflect.DeepEqual(s.Persist, s2.Persist) &&

ipn/ipnlocal/local.go

@@ -453,6 +453,13 @@ func (b *LocalBackend) setClientStatus(st controlclient.Status) {
 	// Lock b once and do only the things that require locking.
 	b.mu.Lock()
 
+	if st.LogoutFinished != nil {
+		// Since we're logged out now, our netmap cache is invalid.
+		// Since st.NetMap==nil means "netmap is unchanged", there is
+		// no other way to represent this change.
+		b.setNetMapLocked(nil)
+	}
+
 	prefs := b.prefs
 	stateKey := b.stateKey
 	netMap := b.netMap
@@ -650,6 +657,12 @@ func (b *LocalBackend) getNewControlClientFunc() clientGen {
 // startIsNoopLocked reports whether a Start call on this LocalBackend
 // with the provided Start Options would be a useless no-op.
 //
+// TODO(apenwarr): we shouldn't need this.
+//  The state machine is now nearly clean enough where it can accept a new
+//  connection while in any state, not just Running, and on any platform.
+//  We'd want to add a few more tests to state_test.go to ensure this continues
+//  to work as expected.
+//
 // b.mu must be held.
 func (b *LocalBackend) startIsNoopLocked(opts ipn.Options) bool {
 	// Options has 5 fields; check all of them:
@@ -2326,7 +2339,6 @@ func (b *LocalBackend) LogoutSync(ctx context.Context) error {
 func (b *LocalBackend) logout(ctx context.Context, sync bool) error {
 	b.mu.Lock()
 	cc := b.cc
-	b.setNetMapLocked(nil)
 	b.mu.Unlock()
 
 	b.EditPrefs(&ipn.MaskedPrefs{
@@ -2353,10 +2365,6 @@ func (b *LocalBackend) logout(ctx context.Context, sync bool) error {
 		cc.StartLogout()
 	}
 
-	b.mu.Lock()
-	b.setNetMapLocked(nil)
-	b.mu.Unlock()
-
 	b.stateMachine()
 	return err
 }

ipn/ipnlocal/state_test.go

@@ -140,6 +140,8 @@ func (cc *mockControl) send(err error, url string, loginFinished bool, nm *netma
 		}
 		if loginFinished {
 			s.LoginFinished = &empty.Message{}
+		} else if url == "" && err == nil && nm == nil {
+			s.LogoutFinished = &empty.Message{}
 		}
 		cc.statusFunc(s)
 	}
@@ -563,24 +565,25 @@ func TestStateMachine(t *testing.T) {
 	b.Logout()
 	{
 		nn := notifies.drain(2)
-		// BUG: now is not the time to unpause.
-		c.Assert([]string{"unpause", "StartLogout"}, qt.DeepEquals, cc.getCalls())
+		c.Assert([]string{"pause", "StartLogout"}, qt.DeepEquals, cc.getCalls())
 		c.Assert(nn[0].State, qt.Not(qt.IsNil))
 		c.Assert(nn[1].Prefs, qt.Not(qt.IsNil))
-		c.Assert(ipn.NeedsLogin, qt.Equals, *nn[0].State)
+		c.Assert(ipn.Stopped, qt.Equals, *nn[0].State)
 		c.Assert(nn[1].Prefs.LoggedOut, qt.IsTrue)
 		c.Assert(nn[1].Prefs.WantRunning, qt.IsFalse)
-		c.Assert(ipn.NeedsLogin, qt.Equals, b.State())
+		c.Assert(ipn.Stopped, qt.Equals, b.State())
 	}
 
 	// Let's make the logout succeed.
 	t.Logf("\n\nLogout (async) - succeed")
-	notifies.expect(0)
+	notifies.expect(1)
 	cc.setAuthBlocked(true)
 	cc.send(nil, "", false, nil)
 	{
-		notifies.drain(0)
-		c.Assert(cc.getCalls(), qt.HasLen, 0)
+		nn := notifies.drain(1)
+		c.Assert([]string{"unpause"}, qt.DeepEquals, cc.getCalls())
+		c.Assert(nn[0].State, qt.Not(qt.IsNil))
+		c.Assert(ipn.NeedsLogin, qt.Equals, *nn[0].State)
 		c.Assert(b.Prefs().LoggedOut, qt.IsTrue)
 		c.Assert(b.Prefs().WantRunning, qt.IsFalse)
 		c.Assert(ipn.NeedsLogin, qt.Equals, b.State())