From 8a7d35594df8f549aa1ccd89e626b101f15561a4 Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Thu, 20 May 2021 02:46:57 -0400 Subject: [PATCH] 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 --- control/controlclient/auto.go | 20 ++++++++++++-------- control/controlclient/controlclient_test.go | 2 +- control/controlclient/status.go | 12 +++++++----- ipn/ipnlocal/local.go | 18 +++++++++++++----- ipn/ipnlocal/state_test.go | 17 ++++++++++------- 5 files changed, 43 insertions(+), 26 deletions(-) diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index fd1823fa9..71aac5ff7 100644 --- a/control/controlclient/auto.go +++ b/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() diff --git a/control/controlclient/controlclient_test.go b/control/controlclient/controlclient_test.go index 6c06b5d7d..b7ae18570 100644 --- a/control/controlclient/controlclient_test.go +++ b/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) diff --git a/control/controlclient/status.go b/control/controlclient/status.go index e99b229df..3cbe9261d 100644 --- a/control/controlclient/status.go +++ b/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) && diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 6c00814a1..374ecce43 100644 --- a/ipn/ipnlocal/local.go +++ b/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 } diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index e5d1ba55b..0630eb6e9 100644 --- a/ipn/ipnlocal/state_test.go +++ b/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())