From 5676d201d61f3fa4218cfcd064bd1d5a4d8d8e9c Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sat, 26 Nov 2022 12:19:16 -0800 Subject: [PATCH] ipn: add a WatchIPNBus option bit to subscribe to EngineStatus changes So GUI clients don't need to poll for it. We still poll internally (for now!) but that's still cheaper. And will get much cheaper later, without having to modify clients once they start sending this bit. Change-Id: I36647b701c8d1fe197677e5eb76f6894e8ff79f7 Signed-off-by: Brad Fitzpatrick --- client/tailscale/localclient.go | 11 +++-------- ipn/backend.go | 11 +++++++++++ ipn/ipnlocal/local.go | 32 +++++++++++++++++++++++++++++++- ipn/localapi/localapi.go | 11 ++++++++++- 4 files changed, 55 insertions(+), 10 deletions(-) diff --git a/client/tailscale/localclient.go b/client/tailscale/localclient.go index f058277ca..2e2f6800f 100644 --- a/client/tailscale/localclient.go +++ b/client/tailscale/localclient.go @@ -990,13 +990,6 @@ func (lc *LocalClient) DebugDERPRegion(ctx context.Context, regionIDOrCode strin return decodeJSON[*ipnstate.DebugDERPRegionReport](body) } -// WatchIPNMask are filtering options for LocalClient.WatchIPNBus. -// -// The zero value is a valid WatchOpt that means to watch everything. -// -// TODO(bradfitz): flesh out. -type WatchIPNMask uint64 - // WatchIPNBus subscribes to the IPN notification bus. It returns a watcher // once the bus is connected successfully. // @@ -1005,7 +998,9 @@ type WatchIPNMask uint64 // // The returned IPNBusWatcher's Close method must be called when done to release // resources. -func (lc *LocalClient) WatchIPNBus(ctx context.Context, mask WatchIPNMask) (*IPNBusWatcher, error) { +// +// A default set of ipn.Notify messages are returned but the set can be modified by mask. +func (lc *LocalClient) WatchIPNBus(ctx context.Context, mask ipn.NotifyWatchOpt) (*IPNBusWatcher, error) { req, err := http.NewRequestWithContext(ctx, "GET", "http://"+apitype.LocalAPIHost+"/localapi/v0/watch-ipn-bus?mask="+fmt.Sprint(mask), nil) diff --git a/ipn/backend.go b/ipn/backend.go index 74b8ac239..fd64eaef5 100644 --- a/ipn/backend.go +++ b/ipn/backend.go @@ -52,6 +52,17 @@ type EngineStatus struct { LivePeers map[key.NodePublic]ipnstate.PeerStatusLite } +// NotifyWatchOpt is a bitmask of options about what type of Notify messages +// to subscribe to. +type NotifyWatchOpt uint64 + +const ( + // NotifyWatchEngineUpdates, if set, causes Engine updates to be sent to the + // client either regularly or when they change, without having to ask for + // each one via RequestEngineStatus. + NotifyWatchEngineUpdates NotifyWatchOpt = 1 << iota +) + // Notify is a communication from a backend (e.g. tailscaled) to a frontend // (cmd/tailscale, iOS, macOS, Win Tasktray). // In any given notification, any or all of these may be nil, meaning diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index b2776153f..01c7ea436 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1702,7 +1702,7 @@ func (b *LocalBackend) readPoller() { // Failure to consume many notifications in a row will result in dropped // notifications. There is currently (2022-11-22) no mechanism provided to // detect when a message has been dropped. -func (b *LocalBackend) WatchNotifications(ctx context.Context, fn func(roNotify *ipn.Notify) (keepGoing bool)) { +func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWatchOpt, fn func(roNotify *ipn.Notify) (keepGoing bool)) { handle := new(mapSetHandle) ch := make(chan *ipn.Notify, 128) @@ -1715,6 +1715,21 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, fn func(roNotify b.mu.Unlock() }() + // The GUI clients want to know when peers become active or inactive. + // They've historically got this information by polling for it, which is + // wasteful. As a step towards making it efficient, they now set this + // NotifyWatchEngineUpdates bit to ask for us to send it to them only on + // change. That's not yet (as of 2022-11-26) plumbed everywhere in + // tailscaled yet, so just do the polling here. This ends up causing all IPN + // bus watchers to get the notification every 2 seconds instead of just the + // GUI client's bus watcher, but in practice there's only 1 total connection + // anyway. And if we're polling, at least the client isn't making a new HTTP + // request every 2 seconds. + // TODO(bradfitz): plumb this further and only send a Notify on change. + if mask&ipn.NotifyWatchEngineUpdates != 0 { + go b.pollRequestEngineStatus(ctx) + } + for { select { case <-ctx.Done(): @@ -1727,6 +1742,21 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, fn func(roNotify } } +// pollRequestEngineStatus calls b.RequestEngineStatus every 2 seconds until ctx +// is done. +func (b *LocalBackend) pollRequestEngineStatus(ctx context.Context) { + ticker := time.NewTicker(2 * time.Second) + defer ticker.Stop() + for { + select { + case <-ticker.C: + b.RequestEngineStatus() + case <-ctx.Done(): + return + } + } +} + // send delivers n to the connected frontend and any API watchers from // LocalBackend.WatchNotifications (via the LocalAPI). // diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index 1b528595d..93170ea3f 100644 --- a/ipn/localapi/localapi.go +++ b/ipn/localapi/localapi.go @@ -602,8 +602,17 @@ func (h *Handler) serveWatchIPNBus(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") f.Flush() + var mask ipn.NotifyWatchOpt + if s := r.FormValue("mask"); s != "" { + v, err := strconv.ParseUint(s, 10, 64) + if err != nil { + http.Error(w, "bad mask", http.StatusBadRequest) + return + } + mask = ipn.NotifyWatchOpt(v) + } ctx := r.Context() - h.b.WatchNotifications(ctx, func(roNotify *ipn.Notify) (keepGoing bool) { + h.b.WatchNotifications(ctx, mask, func(roNotify *ipn.Notify) (keepGoing bool) { js, err := json.Marshal(roNotify) if err != nil { h.logf("json.Marshal: %v", err)