From 7053e1956217a9620c4ae828f26a1460e016a8ff Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 30 Aug 2023 11:09:36 -0700 Subject: [PATCH] control/controlclient: delete Status.Log{in,out}Finished They were entirely redundant and 1:1 with the status field so this turns them into methods instead. Updates #cleanup Updates #1909 Change-Id: I7d939750749edf7dae4c97566bbeb99f2f75adbc Signed-off-by: Brad Fitzpatrick --- cmd/tailscaled/depaware.txt | 2 +- control/controlclient/auto.go | 20 ++++--------- control/controlclient/controlclient_test.go | 9 +----- control/controlclient/status.go | 32 ++++++++++++++------- ipn/ipnlocal/local.go | 6 ++-- ipn/ipnlocal/state_test.go | 5 ++-- 6 files changed, 33 insertions(+), 41 deletions(-) diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 9b99a11e9..c04391ec3 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -302,7 +302,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de tailscale.com/tstime/rate from tailscale.com/wgengine/filter+ tailscale.com/tsweb/varz from tailscale.com/cmd/tailscaled tailscale.com/types/dnstype from tailscale.com/ipn/ipnlocal+ - tailscale.com/types/empty from tailscale.com/control/controlclient+ + tailscale.com/types/empty from tailscale.com/ipn+ tailscale.com/types/flagtype from tailscale.com/cmd/tailscaled tailscale.com/types/ipproto from tailscale.com/net/flowtrack+ tailscale.com/types/key from tailscale.com/control/controlbase+ diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index 38c56dee0..6f354bdd3 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -17,7 +17,6 @@ import ( "tailscale.com/net/sockstats" "tailscale.com/tailcfg" "tailscale.com/tstime" - "tailscale.com/types/empty" "tailscale.com/types/key" "tailscale.com/types/logger" "tailscale.com/types/netmap" @@ -646,13 +645,6 @@ func (c *Auto) sendStatus(who string, err error, url string, nm *netmap.NetworkM c.logf("[v1] sendStatus: %s: %v", who, state) var p *persist.PersistView - var loginFin, logoutFin *empty.Message - if state == StateAuthenticated { - loginFin = new(empty.Message) - } - if state == StateNotAuthenticated { - logoutFin = new(empty.Message) - } if nm != nil && loggedIn && synced { p = ptr.To(c.direct.GetPersist()) } else { @@ -661,13 +653,11 @@ func (c *Auto) sendStatus(who string, err error, url string, nm *netmap.NetworkM nm = nil } new := Status{ - LoginFinished: loginFin, - LogoutFinished: logoutFin, - URL: url, - Persist: p, - NetMap: nm, - Err: err, - state: state, + URL: url, + Persist: p, + NetMap: nm, + Err: err, + state: state, } c.observer.SetControlClientStatus(new) diff --git a/control/controlclient/controlclient_test.go b/control/controlclient/controlclient_test.go index 7a245cad9..e02b784ef 100644 --- a/control/controlclient/controlclient_test.go +++ b/control/controlclient/controlclient_test.go @@ -6,8 +6,6 @@ package controlclient import ( "reflect" "testing" - - "tailscale.com/types/empty" ) func fieldsOf(t reflect.Type) (fields []string) { @@ -21,7 +19,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", "LogoutFinished", "Err", "URL", "NetMap", "Persist", "state"} + equalHandles := []string{"Err", "URL", "NetMap", "Persist", "state"} 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) @@ -61,11 +59,6 @@ func TestStatusEqual(t *testing.T) { &Status{state: StateAuthenticated}, false, }, - { - &Status{LoginFinished: nil}, - &Status{LoginFinished: new(empty.Message)}, - false, - }, } for i, tt := range tests { got := tt.a.Equal(tt.b) diff --git a/control/controlclient/status.go b/control/controlclient/status.go index b5f478449..7f425b546 100644 --- a/control/controlclient/status.go +++ b/control/controlclient/status.go @@ -8,7 +8,6 @@ import ( "fmt" "reflect" - "tailscale.com/types/empty" "tailscale.com/types/netmap" "tailscale.com/types/persist" "tailscale.com/types/structs" @@ -62,12 +61,10 @@ func (s State) String() string { } type Status struct { - _ structs.Incomparable - LoginFinished *empty.Message // nonempty when login finishes - LogoutFinished *empty.Message // nonempty when logout finishes - Err error - URL string // interactive URL to visit to finish logging in - NetMap *netmap.NetworkMap // server-pushed configuration + _ structs.Incomparable + Err error + URL string // interactive URL to visit to finish logging in + NetMap *netmap.NetworkMap // server-pushed configuration Persist *persist.PersistView // locally persisted configuration @@ -78,22 +75,35 @@ type Status struct { state State } +// LoginFinished reports whether the controlclient is in its "StateAuthenticated" +// state where it's in a happy register state but not yet in a map poll. +// +// TODO(bradfitz): delete this and everything around Status.state. +func (s *Status) LoginFinished() bool { return s.state == StateAuthenticated } + +// LogoutFinished reports whether the controlclient is in its "StateNotAuthenticated" +// state where we don't want to be logged in. +// +// TODO(bradfitz): delete this and everything around Status.state. +func (s *Status) LogoutFinished() bool { return s.state == StateNotAuthenticated } + // StateForTest returns the internal state of s for tests only. func (s *Status) StateForTest() State { return s.state } +// SetStateForTest sets the internal state of s for tests only. +func (s *Status) SetStateForTest(state State) { s.state = state } + // Equal reports whether s and s2 are equal. func (s *Status) Equal(s2 *Status) bool { if s == nil && s2 == nil { return true } 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 && + s.state == s2.state && reflect.DeepEqual(s.Persist, s2.Persist) && - reflect.DeepEqual(s.NetMap, s2.NetMap) && - s.state == s2.state + reflect.DeepEqual(s.NetMap, s2.NetMap) } func (s Status) String() string { diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 9b3f99319..3cc60a035 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -969,7 +969,7 @@ func (b *LocalBackend) SetControlClientStatus(st controlclient.Status) { b.blockEngineUpdates(false) } - if st.LoginFinished != nil && wasBlocked { + if st.LoginFinished() && wasBlocked { // Auth completed, unblock the engine b.blockEngineUpdates(false) b.authReconfig() @@ -979,7 +979,7 @@ func (b *LocalBackend) SetControlClientStatus(st controlclient.Status) { // Lock b once and do only the things that require locking. b.mu.Lock() - if st.LogoutFinished != nil { + if st.LogoutFinished() { if p := b.pm.CurrentPrefs(); !p.Persist().Valid() || p.Persist().UserProfile().LoginName() == "" { b.mu.Unlock() return @@ -1017,7 +1017,7 @@ func (b *LocalBackend) SetControlClientStatus(st controlclient.Status) { b.authURL = st.URL b.authURLSticky = st.URL } - if wasBlocked && st.LoginFinished != nil { + if wasBlocked && st.LoginFinished() { // Interactive login finished successfully (URL visited). // After an interactive login, the user always wants // WantRunning. diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index de98f517d..b047e1fff 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -19,7 +19,6 @@ import ( "tailscale.com/tailcfg" "tailscale.com/tsd" "tailscale.com/tstest" - "tailscale.com/types/empty" "tailscale.com/types/key" "tailscale.com/types/logger" "tailscale.com/types/logid" @@ -171,9 +170,9 @@ func (cc *mockControl) send(err error, url string, loginFinished bool, nm *netma Err: err, } if loginFinished { - s.LoginFinished = &empty.Message{} + s.SetStateForTest(controlclient.StateAuthenticated) } else if url == "" && err == nil && nm == nil { - s.LogoutFinished = &empty.Message{} + s.SetStateForTest(controlclient.StateNotAuthenticated) } cc.opts.Observer.SetControlClientStatus(s) }