From b4d04a065fd384ca7f57891a2bb87e1ff5205fb6 Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Thu, 29 Apr 2021 23:27:00 -0400 Subject: [PATCH] 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 --- control/controlclient/auto.go | 74 ++++++++++---------- control/controlclient/client.go | 77 +++++++++++++++++++++ control/controlclient/controlclient_test.go | 2 +- control/controlclient/direct.go | 7 -- control/controlclient/status.go | 27 ++++++-- ipn/ipnlocal/local.go | 24 +++++-- 6 files changed, 152 insertions(+), 59 deletions(-) create mode 100644 control/controlclient/client.go diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index f42d6e715..077b6ad38 100644 --- a/control/controlclient/auto.go +++ b/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() } diff --git a/control/controlclient/client.go b/control/controlclient/client.go new file mode 100644 index 000000000..403c4e5a7 --- /dev/null +++ b/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) +} diff --git a/control/controlclient/controlclient_test.go b/control/controlclient/controlclient_test.go index c7b5715c8..6c06b5d7d 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", "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) diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index 816501bd5..5e522ed93 100644 --- a/control/controlclient/direct.go +++ b/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()") diff --git a/control/controlclient/status.go b/control/controlclient/status.go index 833da3484..e99b229df 100644 --- a/control/controlclient/status.go +++ b/control/controlclient/status.go @@ -18,7 +18,17 @@ import ( // 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. diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 8813be3dd..99fa195be 100644 --- a/ipn/ipnlocal/local.go +++ b/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) }