controlclient: extract a Client interface and rename Client->Auto.

This will let us create a mock or fake Client implementation for use
with ipn.Backend.

Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>

control/controlclient/auto.go

@@ -2,11 +2,6 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-// Package controlclient implements the client for the Tailscale
-// control plane.
-//
-// It handles authentication, port picking, and collects the local
-// network configuration.
 package controlclient
 
 import (
@@ -45,8 +40,9 @@ func (g *LoginGoal) sendLogoutError(err error) {
 	}
 }
 
-// Client connects to a tailcontrol server for a node.
-type Client struct {
+// Auto connects to a tailcontrol server for a node.
+// It's a concrete implementation of the Client interface.
+type Auto struct {
 	direct   *Direct // our interface to the server APIs
 	timeNow  func() time.Time
 	logf     logger.Logf
@@ -79,8 +75,8 @@ type Client struct {
 	mapDone    chan struct{}   // when closed, map goroutine is done
 }
 
-// New creates and starts a new Client.
-func New(opts Options) (*Client, error) {
+// New creates and starts a new Auto.
+func New(opts Options) (*Auto, error) {
 	c, err := NewNoStart(opts)
 	if c != nil {
 		c.Start()
@@ -88,8 +84,8 @@ func New(opts Options) (*Client, error) {
 	return c, err
 }
 
-// NewNoStart creates a new Client, but without calling Start on it.
-func NewNoStart(opts Options) (*Client, error) {
+// NewNoStart creates a new Auto, but without calling Start on it.
+func NewNoStart(opts Options) (*Auto, error) {
 	direct, err := NewDirect(opts)
 	if err != nil {
 		return nil, err
@@ -100,7 +96,7 @@ func NewNoStart(opts Options) (*Client, error) {
 	if opts.TimeNow == nil {
 		opts.TimeNow = time.Now
 	}
-	c := &Client{
+	c := &Auto{
 		direct:   direct,
 		timeNow:  opts.TimeNow,
 		logf:     opts.Logf,
@@ -116,7 +112,7 @@ func NewNoStart(opts Options) (*Client, error) {
 
 }
 
-func (c *Client) onHealthChange(sys health.Subsystem, err error) {
+func (c *Auto) onHealthChange(sys health.Subsystem, err error) {
 	if sys == health.SysOverall {
 		return
 	}
@@ -127,7 +123,7 @@ func (c *Client) onHealthChange(sys health.Subsystem, err error) {
 // SetPaused controls whether HTTP activity should be paused.
 //
 // The client can be paused and unpaused repeatedly, unlike Start and Shutdown, which can only be used once.
-func (c *Client) SetPaused(paused bool) {
+func (c *Auto) SetPaused(paused bool) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
 	if paused == c.paused {
@@ -149,7 +145,7 @@ func (c *Client) SetPaused(paused bool) {
 // Start starts the client's goroutines.
 //
 // It should only be called for clients created by NewNoStart.
-func (c *Client) Start() {
+func (c *Auto) Start() {
 	go c.authRoutine()
 	go c.mapRoutine()
 }
@@ -159,7 +155,7 @@ func (c *Client) Start() {
 // streaming response open), or start a new streaming one if necessary.
 //
 // It should be called whenever there's something new to tell the server.
-func (c *Client) sendNewMapRequest() {
+func (c *Auto) sendNewMapRequest() {
 	c.mu.Lock()
 
 	// If we're not already streaming a netmap, or if we're already stuck
@@ -198,7 +194,7 @@ func (c *Client) sendNewMapRequest() {
 	}()
 }
 
-func (c *Client) cancelAuth() {
+func (c *Auto) cancelAuth() {
 	c.mu.Lock()
 	if c.authCancel != nil {
 		c.authCancel()
@@ -209,7 +205,7 @@ func (c *Client) cancelAuth() {
 	c.mu.Unlock()
 }
 
-func (c *Client) cancelMapLocked() {
+func (c *Auto) cancelMapLocked() {
 	if c.mapCancel != nil {
 		c.mapCancel()
 	}
@@ -218,13 +214,13 @@ func (c *Client) cancelMapLocked() {
 	}
 }
 
-func (c *Client) cancelMapUnsafely() {
+func (c *Auto) cancelMapUnsafely() {
 	c.mu.Lock()
 	c.cancelMapLocked()
 	c.mu.Unlock()
 }
 
-func (c *Client) cancelMapSafely() {
+func (c *Auto) cancelMapSafely() {
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
@@ -260,7 +256,7 @@ func (c *Client) cancelMapSafely() {
 	}
 }
 
-func (c *Client) authRoutine() {
+func (c *Auto) authRoutine() {
 	defer close(c.authDone)
 	bo := backoff.NewBackoff("authRoutine", c.logf, 30*time.Second)
 
@@ -379,7 +375,7 @@ func (c *Client) authRoutine() {
 
 // Expiry returns the credential expiration time, or the zero time if
 // the expiration time isn't known. Used in tests only.
-func (c *Client) Expiry() *time.Time {
+func (c *Auto) Expiry() *time.Time {
 	c.mu.Lock()
 	defer c.mu.Unlock()
 	return c.expiry
@@ -387,21 +383,21 @@ func (c *Client) Expiry() *time.Time {
 
 // Direct returns the underlying direct client object. Used in tests
 // only.
-func (c *Client) Direct() *Direct {
+func (c *Auto) Direct() *Direct {
 	return c.direct
 }
 
 // unpausedChanLocked returns a new channel that is closed when the
-// current Client pause is unpaused.
+// current Auto pause is unpaused.
 //
 // c.mu must be held
-func (c *Client) unpausedChanLocked() <-chan struct{} {
+func (c *Auto) unpausedChanLocked() <-chan struct{} {
 	unpaused := make(chan struct{})
 	c.unpauseWaiters = append(c.unpauseWaiters, unpaused)
 	return unpaused
 }
 
-func (c *Client) mapRoutine() {
+func (c *Auto) mapRoutine() {
 	defer close(c.mapDone)
 	bo := backoff.NewBackoff("mapRoutine", c.logf, 30*time.Second)
 
@@ -523,7 +519,7 @@ func (c *Client) mapRoutine() {
 	}
 }
 
-func (c *Client) AuthCantContinue() bool {
+func (c *Auto) AuthCantContinue() bool {
 	if c == nil {
 		return true
 	}
@@ -534,13 +530,13 @@ func (c *Client) AuthCantContinue() bool {
 }
 
 // SetStatusFunc sets fn as the callback to run on any status change.
-func (c *Client) SetStatusFunc(fn func(Status)) {
+func (c *Auto) SetStatusFunc(fn func(Status)) {
 	c.mu.Lock()
 	c.statusFunc = fn
 	c.mu.Unlock()
 }
 
-func (c *Client) SetHostinfo(hi *tailcfg.Hostinfo) {
+func (c *Auto) SetHostinfo(hi *tailcfg.Hostinfo) {
 	if hi == nil {
 		panic("nil Hostinfo")
 	}
@@ -553,7 +549,7 @@ func (c *Client) SetHostinfo(hi *tailcfg.Hostinfo) {
 	c.sendNewMapRequest()
 }
 
-func (c *Client) SetNetInfo(ni *tailcfg.NetInfo) {
+func (c *Auto) SetNetInfo(ni *tailcfg.NetInfo) {
 	if ni == nil {
 		panic("nil NetInfo")
 	}
@@ -566,7 +562,7 @@ func (c *Client) SetNetInfo(ni *tailcfg.NetInfo) {
 	c.sendNewMapRequest()
 }
 
-func (c *Client) sendStatus(who string, err error, url string, nm *netmap.NetworkMap) {
+func (c *Auto) sendStatus(who string, err error, url string, nm *netmap.NetworkMap) {
 	c.mu.Lock()
 	state := c.state
 	loggedIn := c.loggedIn
@@ -611,7 +607,7 @@ func (c *Client) sendStatus(who string, err error, url string, nm *netmap.Networ
 	c.mu.Unlock()
 }
 
-func (c *Client) Login(t *tailcfg.Oauth2Token, flags LoginFlags) {
+func (c *Auto) Login(t *tailcfg.Oauth2Token, flags LoginFlags) {
 	c.logf("client.Login(%v, %v)", t != nil, flags)
 
 	c.mu.Lock()
@@ -625,7 +621,7 @@ func (c *Client) Login(t *tailcfg.Oauth2Token, flags LoginFlags) {
 	c.cancelAuth()
 }
 
-func (c *Client) StartLogout() {
+func (c *Auto) StartLogout() {
 	c.logf("client.StartLogout()")
 
 	c.mu.Lock()
@@ -636,7 +632,7 @@ func (c *Client) StartLogout() {
 	c.cancelAuth()
 }
 
-func (c *Client) Logout(ctx context.Context) error {
+func (c *Auto) Logout(ctx context.Context) error {
 	c.logf("client.Logout()")
 
 	errc := make(chan error, 1)
@@ -668,14 +664,14 @@ func (c *Client) Logout(ctx context.Context) error {
 //
 // The localPort field is unused except for integration tests in
 // another repo.
-func (c *Client) UpdateEndpoints(localPort uint16, endpoints []tailcfg.Endpoint) {
+func (c *Auto) UpdateEndpoints(localPort uint16, endpoints []tailcfg.Endpoint) {
 	changed := c.direct.SetEndpoints(localPort, endpoints)
 	if changed {
 		c.sendNewMapRequest()
 	}
 }
 
-func (c *Client) Shutdown() {
+func (c *Auto) Shutdown() {
 	c.logf("client.Shutdown()")
 
 	c.mu.Lock()
@@ -701,17 +697,17 @@ func (c *Client) Shutdown() {
 
 // NodePublicKey returns the node public key currently in use. This is
 // used exclusively in tests.
-func (c *Client) TestOnlyNodePublicKey() wgkey.Key {
+func (c *Auto) TestOnlyNodePublicKey() wgkey.Key {
 	priv := c.direct.GetPersist()
 	return priv.PrivateNodeKey.Public()
 }
 
-func (c *Client) TestOnlySetAuthKey(authkey string) {
+func (c *Auto) TestOnlySetAuthKey(authkey string) {
 	c.direct.mu.Lock()
 	defer c.direct.mu.Unlock()
 	c.direct.authKey = authkey
 }
 
-func (c *Client) TestOnlyTimeNow() time.Time {
+func (c *Auto) TestOnlyTimeNow() time.Time {
 	return c.timeNow()
 }

control/controlclient/client.go

@@ -0,0 +1,77 @@
+// Copyright (c) 2020 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package controlclient implements the client for the Tailscale
+// control plane.
+//
+// It handles authentication, port picking, and collects the local
+// network configuration.
+package controlclient
+
+import (
+	"context"
+
+	"tailscale.com/tailcfg"
+)
+
+type LoginFlags int
+
+const (
+	LoginDefault     = LoginFlags(0)
+	LoginInteractive = LoginFlags(1 << iota) // force user login and key refresh
+)
+
+// Client represents a client connection to the control server.
+// Currently this is done through a pair of polling https requests in
+// the Auto client, but that might change eventually.
+type Client interface {
+	// SetStatusFunc provides a callback to call when control sends us
+	// a message.
+	SetStatusFunc(func(Status))
+	// Shutdown closes this session, which should not be used any further
+	// afterwards.
+	Shutdown()
+	// Login begins an interactive or non-interactive login process.
+	// Client will eventually call the Status callback with either a
+	// LoginFinished flag (on success) or an auth URL (if further
+	// interaction is needed).
+	Login(*tailcfg.Oauth2Token, LoginFlags)
+	// StartLogout starts an asynchronous logout process.
+	// When it finishes, the Status callback will be called while
+	// AuthCantContinue()==true.
+	StartLogout()
+	// Logout starts a synchronous logout process. It doesn't return
+	// until the logout operation has been completed.
+	Logout(context.Context) error
+	// SetPaused pauses or unpauses the controlclient activity as much
+	// as possible, without losing its internal state, to minimize
+	// unnecessary network activity.
+	// TODO: It might be better to simply shutdown the controlclient and
+	// make a new one when it's time to unpause.
+	SetPaused(bool)
+	// AuthCantContinue returns whether authentication is blocked. If it
+	// is, you either need to visit the auth URL (previously sent in a
+	// Status callback) or call the Login function appropriately.
+	// TODO: this probably belongs in the Status itself instead.
+	AuthCantContinue() bool
+	// SetHostinfo changes the Hostinfo structure that will be sent in
+	// subsequent node registration requests.
+	// TODO: a server-side change would let us simply upload this
+	// in a separate http request. It has nothing to do with the rest of
+	// the state machine.
+	SetHostinfo(*tailcfg.Hostinfo)
+	// SetNetinfo changes the NetIinfo structure that will be sent in
+	// subsequent node registration requests.
+	// TODO: a server-side change would let us simply upload this
+	// in a separate http request. It has nothing to do with the rest of
+	// the state machine.
+	SetNetInfo(*tailcfg.NetInfo)
+	// UpdateEndpoints changes the Endpoint structure that will be sent
+	// in subsequent node registration requests.
+	// TODO: localPort seems to be obsolete, remove it.
+	// TODO: a server-side change would let us simply upload this
+	// in a separate http request. It has nothing to do with the rest of
+	// the state machine.
+	UpdateEndpoints(localPort uint16, endpoints []tailcfg.Endpoint)
+}

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", "Persist", "NetMap", "Hostinfo", "State"}
+	equalHandles := []string{"LoginFinished", "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/direct.go

@@ -251,13 +251,6 @@ func (c *Direct) GetPersist() persist.Persist {
 	return c.persist
 }
 
-type LoginFlags int
-
-const (
-	LoginDefault     = LoginFlags(0)
-	LoginInteractive = LoginFlags(1 << iota) // force user login and key refresh
-)
-
 func (c *Direct) TryLogout(ctx context.Context) error {
 	c.logf("direct.TryLogout()")
 

control/controlclient/status.go

@@ -18,7 +18,17 @@
 
 // State is the high-level state of the client. It is used only in
 // unit tests for proper sequencing, don't depend on it anywhere else.
-// TODO(apenwarr): eliminate 'state', as it's now obsolete.
+//
+// TODO(apenwarr): eliminate the state, as it's now obsolete.
+//
+// apenwarr: Historical note: controlclient.Auto was originally
+// intended to be the state machine for the whole tailscale client, but that
+// turned out to not be the right abstraction layer, and it moved to
+// ipn.Backend. Since ipn.Backend now has a state machine, it would be
+// much better if controlclient could be a simple stateless API. But the
+// current server-side API (two interlocking polling https calls) makes that
+// very hard to implement. A server side API change could untangle this and
+// remove all the statefulness.
 type State int
 
 const (
@@ -55,13 +65,18 @@ func (s State) String() string {
 
 type Status struct {
 	_             structs.Incomparable
-	LoginFinished *empty.Message
+	LoginFinished *empty.Message // nonempty when login finishes
 	Err           string
-	URL           string
-	Persist       *persist.Persist   // locally persisted configuration
+	URL           string             // interactive URL to visit to finish logging in
 	NetMap        *netmap.NetworkMap // server-pushed configuration
-	Hostinfo      *tailcfg.Hostinfo  // current Hostinfo data
-	State         State
+
+	// The internal state should not be exposed outside this
+	// package, but we have some automated tests elsewhere that need to
+	// use them. Please don't use these fields.
+	// TODO(apenwarr): Unexport or remove these.
+	State    State
+	Persist  *persist.Persist  // locally persisted configuration
+	Hostinfo *tailcfg.Hostinfo // current Hostinfo data
 }
 
 // Equal reports whether s and s2 are equal.

ipn/ipnlocal/local.go

@@ -83,6 +83,7 @@ type LocalBackend struct {
 	keyLogf               logger.Logf        // for printing list of peers on change
 	statsLogf             logger.Logf        // for printing peers stats on change
 	e                     wgengine.Engine
+	ccGen                 clientGen // function for producing controlclient
 	store                 ipn.StateStore
 	backendLogID          string
 	unregisterLinkMon     func()
@@ -99,7 +100,7 @@ type LocalBackend struct {
 	mu             sync.Mutex
 	httpTestClient *http.Client // for controlclient. nil by default, used by tests.
 	notify         func(ipn.Notify)
-	cc             *controlclient.Client
+	cc             controlclient.Client
 	stateKey       ipn.StateKey // computed in part from user-provided value
 	userID         string       // current controlling user ID (for Windows, primarily)
 	prefs          *ipn.Prefs
@@ -140,9 +141,23 @@ type LocalBackend struct {
 	statusChanged *sync.Cond
 }
 
+type clientGen func(controlclient.Options) (controlclient.Client, error)
+
 // NewLocalBackend returns a new LocalBackend that is ready to run,
 // but is not actually running.
 func NewLocalBackend(logf logger.Logf, logid string, store ipn.StateStore, e wgengine.Engine) (*LocalBackend, error) {
+	// TODO(apenwarr): change controlclient.New to return controlclient.Client?
+	// Then we could avoid this wrapper, at the expense of external tests
+	// having to typecast in the interface.
+	ccWrap := func(opts controlclient.Options) (controlclient.Client, error) {
+		return controlclient.New(opts)
+	}
+	return NewLocalBackendWithClientGen(logf, logid, store, e, ccWrap)
+}
+
+// NewLocalBackend returns a new LocalBackend that is ready to run,
+// but is not actually running.
+func NewLocalBackendWithClientGen(logf logger.Logf, logid string, store ipn.StateStore, e wgengine.Engine, ccGen clientGen) (*LocalBackend, error) {
 	if e == nil {
 		panic("ipn.NewLocalBackend: wgengine must not be nil")
 	}
@@ -165,6 +180,7 @@ func NewLocalBackend(logf logger.Logf, logid string, store ipn.StateStore, e wge
 		keyLogf:        logger.LogOnChange(logf, 5*time.Minute, time.Now),
 		statsLogf:      logger.LogOnChange(logf, 5*time.Minute, time.Now),
 		e:              e,
+		ccGen:          ccGen,
 		store:          store,
 		backendLogID:   logid,
 		state:          ipn.NoState,
@@ -742,7 +758,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 		debugFlags = append([]string{"netstack"}, debugFlags...)
 	}
 
-	cc, err := controlclient.New(controlclient.Options{
+	cc, err := b.ccGen(controlclient.Options{
 		GetMachinePrivateKey: b.createGetMachinePrivateKeyFunc(),
 		Logf:                 logger.WithPrefix(b.logf, "control: "),
 		Persist:              *persistv,
@@ -2210,10 +2226,6 @@ func (b *LocalBackend) ResetForClientDisconnect() {
 // Logout tells the controlclient that we want to log out, and
 // transitions the local engine to the logged-out state without
 // waiting for controlclient to be in that state.
-//
-// NOTE(apenwarr): No easy way to persist logged-out status.
-//  Maybe that's for the better; if someone logs out accidentally,
-//  rebooting will fix it.
 func (b *LocalBackend) Logout() {
 	b.logout(context.Background(), false)
 }