From ef0d7402706112472b4d87bc0dff25e74b588d26 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sun, 19 Jun 2022 18:14:45 -0700 Subject: [PATCH] control/controlclient: remove Client.SetStatusFunc It can't change at runtime. Make it an option. Signed-off-by: Brad Fitzpatrick --- control/controlclient/auto.go | 52 ++++++++++++++++----------------- control/controlclient/client.go | 3 -- control/controlclient/direct.go | 3 ++ ipn/ipnlocal/local.go | 2 +- ipn/ipnlocal/state_test.go | 11 ++++--- 5 files changed, 34 insertions(+), 37 deletions(-) diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index 4ff50f6cf..3ec734388 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -6,6 +6,7 @@ import ( "context" + "errors" "fmt" "net/http" "sync" @@ -46,17 +47,17 @@ func (g *LoginGoal) sendLogoutError(err error) { // 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 - expiry *time.Time - closed bool - newMapCh chan struct{} // readable when we must restart a map request + direct *Direct // our interface to the server APIs + timeNow func() time.Time + logf logger.Logf + expiry *time.Time + closed bool + newMapCh chan struct{} // readable when we must restart a map request + statusFunc func(Status) // called to update Client status; always non-nil unregisterHealthWatch func() - mu sync.Mutex // mutex guards the following fields - statusFunc func(Status) // called to update Client status + mu sync.Mutex // mutex guards the following fields paused bool // whether we should stop making HTTP requests unpauseWaiters []chan struct{} @@ -92,6 +93,9 @@ func NewNoStart(opts Options) (*Auto, error) { if err != nil { return nil, err } + if opts.Status == nil { + return nil, errors.New("missing required Options.Status") + } if opts.Logf == nil { opts.Logf = func(fmt string, args ...any) {} } @@ -99,13 +103,14 @@ func NewNoStart(opts Options) (*Auto, error) { opts.TimeNow = time.Now } c := &Auto{ - direct: direct, - timeNow: opts.TimeNow, - logf: opts.Logf, - newMapCh: make(chan struct{}, 1), - quit: make(chan struct{}), - authDone: make(chan struct{}), - mapDone: make(chan struct{}), + direct: direct, + timeNow: opts.TimeNow, + logf: opts.Logf, + newMapCh: make(chan struct{}, 1), + quit: make(chan struct{}), + authDone: make(chan struct{}), + mapDone: make(chan struct{}), + statusFunc: opts.Status, } c.authCtx, c.authCancel = context.WithCancel(context.Background()) c.mapCtx, c.mapCancel = context.WithCancel(context.Background()) @@ -533,13 +538,6 @@ func (c *Auto) AuthCantContinue() bool { return !c.loggedIn && (c.loginGoal == nil || c.loginGoal.url != "") } -// SetStatusFunc sets fn as the callback to run on any status change. -func (c *Auto) SetStatusFunc(fn func(Status)) { - c.mu.Lock() - c.statusFunc = fn - c.mu.Unlock() -} - func (c *Auto) SetHostinfo(hi *tailcfg.Hostinfo) { if hi == nil { panic("nil Hostinfo") @@ -567,10 +565,13 @@ func (c *Auto) SetNetInfo(ni *tailcfg.NetInfo) { func (c *Auto) sendStatus(who string, err error, url string, nm *netmap.NetworkMap) { c.mu.Lock() + if c.closed { + c.mu.Unlock() + return + } state := c.state loggedIn := c.loggedIn synced := c.synced - statusFunc := c.statusFunc c.inSendStatus++ c.mu.Unlock() @@ -601,9 +602,7 @@ func (c *Auto) sendStatus(who string, err error, url string, nm *netmap.NetworkM State: state, Err: err, } - if statusFunc != nil { - statusFunc(new) - } + c.statusFunc(new) c.mu.Lock() c.inSendStatus-- @@ -684,7 +683,6 @@ func (c *Auto) Shutdown() { direct := c.direct if !closed { c.closed = true - c.statusFunc = nil } c.mu.Unlock() diff --git a/control/controlclient/client.go b/control/controlclient/client.go index 844f72d5e..aac8bf468 100644 --- a/control/controlclient/client.go +++ b/control/controlclient/client.go @@ -28,9 +28,6 @@ // 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() diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index 2de629e68..df950abbb 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -108,6 +108,9 @@ type Options struct { PopBrowserURL func(url string) // optional func to open browser Dialer *tsdial.Dialer // non-nil + // Status is called when there's a change in status. + Status func(Status) + // KeepSharerAndUserSplit controls whether the client // understands Node.Sharer. If false, the Sharer is mapped to the User. KeepSharerAndUserSplit bool diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index a95673b25..fab967a14 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1056,6 +1056,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error { Pinger: b, PopBrowserURL: b.tellClientToBrowseToURL, Dialer: b.Dialer(), + Status: b.setClientStatus, // Don't warn about broken Linux IP forwarding when // netstack is being used. @@ -1075,7 +1076,6 @@ func (b *LocalBackend) Start(opts ipn.Options) error { cc.UpdateEndpoints(endpoints) } - cc.SetStatusFunc(b.setClientStatus) b.e.SetNetInfoCallback(b.setNetInfo) b.mu.Lock() diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index ddd60a401..f7dfdebe5 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -114,10 +114,6 @@ func (cc *mockControl) logf(format string, args ...any) { cc.logfActual(format, args...) } -func (cc *mockControl) SetStatusFunc(fn func(controlclient.Status)) { - cc.statusFunc = fn -} - func (cc *mockControl) populateKeys() (newKeys bool) { cc.mu.Lock() defer cc.mu.Unlock() @@ -295,12 +291,15 @@ func TestStateMachine(t *testing.T) { } t.Cleanup(e.Close) - cc := newMockControl(t) - t.Cleanup(func() { cc.preventLog.Set(true) }) // hacky way to pacify issue 3020 b, err := NewLocalBackend(logf, "logid", store, nil, e, 0) if err != nil { t.Fatalf("NewLocalBackend: %v", err) } + + cc := newMockControl(t) + cc.statusFunc = b.setClientStatus + t.Cleanup(func() { cc.preventLog.Set(true) }) // hacky way to pacify issue 3020 + b.SetControlClientGetterForTesting(func(opts controlclient.Options) (controlclient.Client, error) { cc.mu.Lock() cc.opts = opts