From 544d8d0ab80510602ee0069e69e42a41655a537e Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 30 Apr 2021 07:23:22 -0700 Subject: [PATCH] ipn/ipnlocal: remove NewLocalBackendWithClientGen This removes the NewLocalBackendWithClientGen constructor added in b4d04a065fd384ca7f57891a2bb87e1ff5205fb6 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 --- ipn/ipnlocal/local.go | 44 +++++++++++++++++++++++++------------- ipn/ipnlocal/state_test.go | 12 +++++------ 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index db983b1ce..582f13cac 100644 --- a/ipn/ipnlocal/local.go +++ b/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, diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index 7bb3a88b6..2f860c566 100644 --- a/ipn/ipnlocal/state_test.go +++ b/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 := ¬ifyThrottler{t: t} notifies.expect(0)