From 3515b009c22d1d6b299cead0fde6e3f9d1068d59 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 15 Jul 2026 12:09:45 +0000 Subject: [PATCH] ipn/ipnext, ipn/ipnlocal, feature/conn25: pass peer seq to AllowedIPs hook The ExtraWireGuardAllowedIPs hook was called once per peer on every authReconfig, so each netmap delta paid an O(n) scan over all peers even when conn25 (the only implementer) wasn't configured and every call returned nothing. Invert the API: the hook now receives an iter.Seq2 of the current peers and returns the extra prefixes keyed by node ID. An idle extension returns nil without iterating, so the unconfigured case does no per-peer work at all. With this, the runtime.DidRange analysis (see the ts_rangehook test) no longer reports the updateRouteManagerExtras peer scan on netmap deltas. Updates #12542 Signed-off-by: Brad Fitzpatrick Change-Id: I9181e77416fa22f4c904620d42e9bcb934165216 --- feature/conn25/conn25.go | 13 ++++++++--- ipn/ipnext/ipnext.go | 38 ++++++++++++++++++++----------- ipn/ipnlocal/node_backend.go | 22 ++++++++++-------- ipn/ipnlocal/node_backend_test.go | 17 ++++++++------ 4 files changed, 57 insertions(+), 33 deletions(-) diff --git a/feature/conn25/conn25.go b/feature/conn25/conn25.go index 33695ec12..ae0764433 100644 --- a/feature/conn25/conn25.go +++ b/feature/conn25/conn25.go @@ -14,6 +14,7 @@ import ( "errors" "fmt" "io" + "iter" "net/http" "net/netip" "slices" @@ -267,11 +268,17 @@ func (e *extension) installHooks(dph *datapathHandler) error { }) // Tell WireGuard what Transit IPs belong to which connector peers. - e.host.Hooks().ExtraWireGuardAllowedIPs.Set(func(k key.NodePublic) views.Slice[netip.Prefix] { + e.host.Hooks().ExtraWireGuardAllowedIPs.Set(func(peers iter.Seq2[tailcfg.NodeID, key.NodePublic]) map[tailcfg.NodeID][]netip.Prefix { if !e.conn25.isConfigured() { - return views.Slice[netip.Prefix]{} + return nil } - return e.extraWireGuardAllowedIPs(k) + var extras map[tailcfg.NodeID][]netip.Prefix + for id, k := range peers { + if pfxs := e.extraWireGuardAllowedIPs(k); pfxs.Len() > 0 { + mak.Set(&extras, id, pfxs.AsSlice()) + } + } + return extras }) return nil diff --git a/ipn/ipnext/ipnext.go b/ipn/ipnext/ipnext.go index 30436e1ff..261428721 100644 --- a/ipn/ipnext/ipnext.go +++ b/ipn/ipnext/ipnext.go @@ -430,29 +430,41 @@ type Hooks struct { // See [filter.Filter] for details on how these hooks are invoked. Filter FilterHooks - // ExtraWireGuardAllowedIPs is called with each peer's public key - // from the initial [wgcfg.Config], and returns a view of prefixes to - // append to each peer's AllowedIPs. + // ExtraWireGuardAllowedIPs is called with a sequence of peers whose + // extra AllowedIPs the caller wants (re)computed, and returns + // prefixes to append to those peers' AllowedIPs, keyed by node ID. // - // The extra AllowedIPs are added after the [router.Config] is generated, but - // before the WireGuard config is sent to the engine, so the extra IPs are - // given to WireGuard, but not the OS routing table. + // The sequence is not necessarily all peers: callers may pass any + // subset (such as only the peers changed by a netmap delta), and + // the returned map's meaning is scoped to the peers presented. A + // peer absent from the returned map has no extra AllowedIPs. As of + // 2026-07-15 the only caller passes all current peers on each + // reconfig, but extensions must not rely on that. // - // The prefixes returned from the hook should not contain duplicates, either - // internally, or with netmap peer prefixes. Returned prefixes should only + // An extension with nothing to add should return nil without + // iterating peers; that keeps steady-state netmap deltas free of + // per-peer work when the extension is idle. The peers sequence is + // only valid during the call. + // + // The extra AllowedIPs are given to WireGuard, but not the OS + // routing table. + // + // The returned prefixes should not contain duplicates, either + // internally, or with netmap peer prefixes. They should only // contain host routes, and not contain default or subnet routes. - // Subsequent calls that return an unchanged set of prefixes for a given peer, - // should return the prefixes in the same order for that peer, - // to prevent configuration churn. + // Subsequent calls that return an unchanged set of prefixes for a + // given peer should return the prefixes in the same order for that + // peer, to prevent configuration churn. // - // The returned slice should not be mutated by the extension after it is returned. + // The returned map and slices should not be mutated by the + // extension after they are returned. // // The hook is called with LocalBackend's mutex locked. // // TODO(#17858): This hook may not be needed and can possibly be replaced by // new hooks that fit into the new architecture that make use of new // WireGuard APIs. - ExtraWireGuardAllowedIPs feature.Hook[func(key.NodePublic) views.Slice[netip.Prefix]] + ExtraWireGuardAllowedIPs feature.Hook[func(peers iter.Seq2[tailcfg.NodeID, key.NodePublic]) map[tailcfg.NodeID][]netip.Prefix] // ExtraRouterConfigRoutes returns a view of prefixes to append to [router.Config.Routes]. // diff --git a/ipn/ipnlocal/node_backend.go b/ipn/ipnlocal/node_backend.go index 2098ebadd..ffc820be8 100644 --- a/ipn/ipnlocal/node_backend.go +++ b/ipn/ipnlocal/node_backend.go @@ -6,6 +6,7 @@ package ipnlocal import ( "cmp" "context" + "iter" "maps" "net/netip" "slices" @@ -943,22 +944,23 @@ func (nb *nodeBackend) updateRouteManagerPrefs(p routePrefs) routemanager.PeersW return res.AllowedIPs } -// updateRouteManagerExtras pushes each peer's extra WireGuard-only -// allowed IPs into the route manager, obtained by calling fn (the -// [ipnext.Hooks.ExtraWireGuardAllowedIPs] hook) with each peer's -// public key. +// updateRouteManagerExtras pushes extra WireGuard-only allowed IPs +// into the route manager, obtained by calling fn (the +// [ipnext.Hooks.ExtraWireGuardAllowedIPs] hook) with a sequence of +// the current peers. // // It returns the peers whose allowed source prefixes changed as a // result. -func (nb *nodeBackend) updateRouteManagerExtras(fn func(key.NodePublic) views.Slice[netip.Prefix]) routemanager.PeersWithRouteChanges { +func (nb *nodeBackend) updateRouteManagerExtras(fn func(peers iter.Seq2[tailcfg.NodeID, key.NodePublic]) map[tailcfg.NodeID][]netip.Prefix) routemanager.PeersWithRouteChanges { nb.mu.Lock() defer nb.mu.Unlock() - var extras map[tailcfg.NodeID][]netip.Prefix - for id, p := range nb.peers { - if pfxs := fn(p.Key()); pfxs.Len() > 0 { - mak.Set(&extras, id, pfxs.AsSlice()) + extras := fn(func(yield func(tailcfg.NodeID, key.NodePublic) bool) { + for id, p := range nb.peers { + if !yield(id, p.Key()) { + return + } } - } + }) rt := nb.routeMgr.Begin() rt.SetExtraAllowedIPs(extras) res := rt.Commit() diff --git a/ipn/ipnlocal/node_backend_test.go b/ipn/ipnlocal/node_backend_test.go index adad0c32b..03b54fe65 100644 --- a/ipn/ipnlocal/node_backend_test.go +++ b/ipn/ipnlocal/node_backend_test.go @@ -6,6 +6,7 @@ package ipnlocal import ( "context" "errors" + "iter" "maps" "net/netip" "slices" @@ -17,7 +18,6 @@ import ( "tailscale.com/tstest" "tailscale.com/types/key" "tailscale.com/types/netmap" - "tailscale.com/types/views" "tailscale.com/util/dnsname" "tailscale.com/util/eventbus" "tailscale.com/util/mak" @@ -524,11 +524,14 @@ func TestNodeBackendRouteManagerExtras(t *testing.T) { nb.SetNetMap(&netmap.NetworkMap{Peers: []tailcfg.NodeView{p1}}) transit := netip.MustParsePrefix("fe80::1234/128") - extrasFor := func(k key.NodePublic) views.Slice[netip.Prefix] { - if k == p1.Key() { - return views.SliceOf([]netip.Prefix{transit}) + extrasFor := func(peers iter.Seq2[tailcfg.NodeID, key.NodePublic]) map[tailcfg.NodeID][]netip.Prefix { + var extras map[tailcfg.NodeID][]netip.Prefix + for id, k := range peers { + if k == p1.Key() { + mak.Set(&extras, id, []netip.Prefix{transit}) + } } - return views.Slice[netip.Prefix]{} + return extras } // Installing extras reports the peer's allowed prefixes as @@ -550,8 +553,8 @@ func TestNodeBackendRouteManagerExtras(t *testing.T) { } // A hook that no longer returns extras removes them. - changed = nb.updateRouteManagerExtras(func(key.NodePublic) views.Slice[netip.Prefix] { - return views.Slice[netip.Prefix]{} + changed = nb.updateRouteManagerExtras(func(iter.Seq2[tailcfg.NodeID, key.NodePublic]) map[tailcfg.NodeID][]netip.Prefix { + return nil }) if len(changed) != 1 { t.Errorf("clearing extras changed = %v; want just %v", changed, p1.Key())