ipn/ipnlocal: remove NewLocalBackendWithClientGen

This removes the NewLocalBackendWithClientGen constructor added in
b4d04a065f and instead adds
LocalBackend.SetControlClientGetterForTesting, mirroring
LocalBackend.SetHTTPTestClient. NewLocalBackendWithClientGen was
weird in being exported but taking an unexported type. This was noted
during code review:

https://github.com/tailscale/tailscale/pull/1818#discussion_r623155669

which ended in:

"I'll leave it for y'all to clean up if you find some way to do it elegantly."

This is more idiomatic.

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

ipn/ipnlocal/local.go

@@ -83,7 +83,6 @@ 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,6 +98,7 @@ type LocalBackend struct {
 	// The mutex protects the following elements.
 	mu             sync.Mutex
 	httpTestClient *http.Client // for controlclient. nil by default, used by tests.
+	ccGen          clientGen    // function for producing controlclient; lazily populated
 	notify         func(ipn.Notify)
 	cc             controlclient.Client
 	stateKey       ipn.StateKey // computed in part from user-provided value
@@ -141,23 +141,13 @@ type LocalBackend struct {
 	statusChanged *sync.Cond
 }
 
+// clientGen is a func that creates a control plane client.
+// It's the type used by LocalBackend.SetControlClientGetterForTesting.
 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")
 	}
@@ -180,7 +170,6 @@ func NewLocalBackendWithClientGen(logf logger.Logf, logid string, store ipn.Stat
 		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,
@@ -614,6 +603,31 @@ func (b *LocalBackend) SetHTTPTestClient(c *http.Client) {
 	b.httpTestClient = c
 }
 
+// SetControlClientGetterForTesting sets the func that creates a
+// control plane client. It can be called at most once, before Start.
+func (b *LocalBackend) SetControlClientGetterForTesting(newControlClient func(controlclient.Options) (controlclient.Client, error)) {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	if b.ccGen != nil {
+		panic("invalid use of SetControlClientGetterForTesting after Start")
+	}
+	b.ccGen = newControlClient
+}
+
+func (b *LocalBackend) getNewControlClientFunc() clientGen {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	if b.ccGen == nil {
+		// Initialize it rather than just returning the
+		// default to make any future call to
+		// SetControlClientGetterForTesting panic.
+		b.ccGen = func(opts controlclient.Options) (controlclient.Client, error) {
+			return controlclient.New(opts)
+		}
+	}
+	return b.ccGen
+}
+
 // startIsNoopLocked reports whether a Start call on this LocalBackend
 // with the provided Start Options would be a useless no-op.
 //
@@ -765,7 +779,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 		debugFlags = append([]string{"netstack"}, debugFlags...)
 	}
 
-	cc, err := b.ccGen(controlclient.Options{
+	cc, err := b.getNewControlClientFunc()(controlclient.Options{
 		GetMachinePrivateKey: b.createGetMachinePrivateKeyFunc(),
 		Logf:                 logger.WithPrefix(b.logf, "control: "),
 		Persist:              *persistv,

ipn/ipnlocal/state_test.go

@@ -278,7 +278,11 @@ func TestStateMachine(t *testing.T) {
 	}
 
 	cc := newMockControl()
-	ccGen := func(opts controlclient.Options) (controlclient.Client, error) {
+	b, err := NewLocalBackend(logf, "logid", store, e)
+	if err != nil {
+		t.Fatalf("NewLocalBackend: %v", err)
+	}
+	b.SetControlClientGetterForTesting(func(opts controlclient.Options) (controlclient.Client, error) {
 		cc.mu.Lock()
 		cc.opts = opts
 		cc.logf = opts.Logf
@@ -289,11 +293,7 @@ func TestStateMachine(t *testing.T) {
 		cc.logf("ccGen: new mockControl.")
 		cc.called("New")
 		return cc, nil
-	}
-	b, err := NewLocalBackendWithClientGen(logf, "logid", store, e, ccGen)
-	if err != nil {
-		t.Fatalf("NewLocalBackend: %v", err)
-	}
+	})
 
 	notifies := &notifyThrottler{t: t}
 	notifies.expect(0)