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 <bradfitz@tailscale.com>

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+

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)
 

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)

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 {

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.

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)
 	}