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 fad18a7fb..877978729 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -451,6 +451,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 @@ -648,6 +655,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())