From 55bb7314f22565d10ab1a9c9d7cc75a303d0f2cb Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 28 Aug 2023 15:27:39 -0700 Subject: [PATCH] control/controlclient: replace a status func with Observer interface For now the method has only one interface (the same as the func it's replacing) but it will grow, eventually with the goal to remove the controlclient.Status type for most purposes. Updates #1909 Change-Id: I715c8bf95e3f5943055a94e76af98d988558a2f2 Signed-off-by: Brad Fitzpatrick --- control/controlclient/auto.go | 24 ++++++++++++------------ control/controlclient/direct.go | 11 +++++++++-- ipn/ipnlocal/local.go | 8 ++++---- ipn/ipnlocal/network-lock_test.go | 8 +++++++- ipn/ipnlocal/state_test.go | 4 ++-- 5 files changed, 34 insertions(+), 21 deletions(-) diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index 5ada58552..62da8d13c 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -138,14 +138,14 @@ type updateGen int64 // 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 - clock tstime.Clock - logf logger.Logf - expiry *time.Time - closed bool - updateCh chan struct{} // readable when we should inform the server of a change - newMapCh chan struct{} // readable when we must restart a map request - statusFunc func(Status) // called to update Client status; always non-nil + direct *Direct // our interface to the server APIs + clock tstime.Clock + logf logger.Logf + expiry *time.Time + closed bool + updateCh chan struct{} // readable when we should inform the server of a change + newMapCh chan struct{} // readable when we must restart a map request + observer Observer // called to update Client status; always non-nil unregisterHealthWatch func() @@ -194,8 +194,8 @@ func NewNoStart(opts Options) (_ *Auto, err error) { } }() - if opts.Status == nil { - return nil, errors.New("missing required Options.Status") + if opts.Observer == nil { + return nil, errors.New("missing required Options.Observer") } if opts.Logf == nil { opts.Logf = func(fmt string, args ...any) {} @@ -213,7 +213,7 @@ func NewNoStart(opts Options) (_ *Auto, err error) { authDone: make(chan struct{}), mapDone: make(chan struct{}), updateDone: make(chan struct{}), - statusFunc: opts.Status, + observer: opts.Observer, } c.authCtx, c.authCancel = context.WithCancel(context.Background()) c.authCtx = sockstats.WithSockStats(c.authCtx, sockstats.LabelControlClientAuto, opts.Logf) @@ -669,7 +669,7 @@ func (c *Auto) sendStatus(who string, err error, url string, nm *netmap.NetworkM State: state, Err: err, } - c.statusFunc(new) + c.observer.SetControlClientStatus(new) c.mu.Lock() c.inSendStatus-- diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index a4523f7a5..f999e4c50 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -101,6 +101,12 @@ type Direct struct { lastPingURL string // last PingRequest.URL received, for dup suppression } +// Observer is implemented by users of the control client (such as LocalBackend) +// to get notified of changes in the control client's status. +type Observer interface { + SetControlClientStatus(Status) +} + type Options struct { Persist persist.Persist // initial persistent data GetMachinePrivateKey func() (key.MachinePrivate, error) // returns the machine key to use @@ -122,8 +128,9 @@ type Options struct { Dialer *tsdial.Dialer // non-nil C2NHandler http.Handler // or nil - // Status is called when there's a change in status. - Status func(Status) + // Observer is called when there's a change in status to report + // from the control client. + Observer Observer // SkipIPForwardingCheck declares that the host's IP // forwarding works and should not be double-checked by the diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index caac3977c..056e099f2 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -888,9 +888,9 @@ func (b *LocalBackend) SetDecompressor(fn func() (controlclient.Decompressor, er b.newDecompressor = fn } -// setClientStatus is the callback invoked by the control client whenever it posts a new status. +// SetControlClientStatus is the callback invoked by the control client whenever it posts a new status. // Among other things, this is where we update the netmap, packet filters, DNS and DERP maps. -func (b *LocalBackend) setClientStatus(st controlclient.Status) { +func (b *LocalBackend) SetControlClientStatus(st controlclient.Status) { // The following do not depend on any data for which we need to lock b. if st.Err != nil { // TODO(crawshaw): display in the UI. @@ -947,7 +947,7 @@ func (b *LocalBackend) setClientStatus(st controlclient.Status) { // Call ourselves with the current status again; the logic in // setClientStatus will take care of updating the expired field // of peers in the netmap. - b.setClientStatus(st) + b.SetControlClientStatus(st) }) } } @@ -1457,7 +1457,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error { OnClientVersion: b.onClientVersion, OnControlTime: b.em.onControlTime, Dialer: b.Dialer(), - Status: b.setClientStatus, + Observer: b, C2NHandler: http.HandlerFunc(b.handleC2N), DialPlan: &b.dialPlan, // pointer because it can't be copied diff --git a/ipn/ipnlocal/network-lock_test.go b/ipn/ipnlocal/network-lock_test.go index e9325ea13..f5eb57101 100644 --- a/ipn/ipnlocal/network-lock_test.go +++ b/ipn/ipnlocal/network-lock_test.go @@ -29,6 +29,12 @@ import ( "tailscale.com/util/must" ) +type observerFunc func(controlclient.Status) + +func (f observerFunc) SetControlClientStatus(s controlclient.Status) { + f(s) +} + func fakeControlClient(t *testing.T, c *http.Client) *controlclient.Auto { hi := hostinfo.New() ni := tailcfg.NetInfo{LinkType: "wired"} @@ -43,7 +49,7 @@ func fakeControlClient(t *testing.T, c *http.Client) *controlclient.Auto { }, HTTPTestClient: c, NoiseTestClient: c, - Status: func(controlclient.Status) {}, + Observer: observerFunc(func(controlclient.Status) {}), } cc, err := controlclient.NewNoStart(opts) diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index c924abd5a..de98f517d 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -162,7 +162,7 @@ func (cc *mockControl) send(err error, url string, loginFinished bool, nm *netma cc.authBlocked = false cc.mu.Unlock() } - if cc.opts.Status != nil { + if cc.opts.Observer != nil { pv := cc.persist.View() s := controlclient.Status{ URL: url, @@ -175,7 +175,7 @@ func (cc *mockControl) send(err error, url string, loginFinished bool, nm *netma } else if url == "" && err == nil && nm == nil { s.LogoutFinished = &empty.Message{} } - cc.opts.Status(s) + cc.opts.Observer.SetControlClientStatus(s) } }