From 0e7515eab9ae88ee3ba533c4b6db75af5d33f911 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 09:40:25 +0000 Subject: [PATCH] state, policy/v2: classify only effective route changes Compare announced-and-approved routes only when immutable route inputs change. Publish Hostinfo without unnecessary peer adjacency rebuilds. --- hscontrol/policy/v2/policy.go | 12 +-- hscontrol/policy/v2/policy_test.go | 59 ++++++++++++ hscontrol/servertest/mapstream_compat_test.go | 6 -- hscontrol/state/node_store.go | 8 +- hscontrol/state/node_store_test.go | 95 ++++++++++++++++--- hscontrol/state/state.go | 2 +- hscontrol/types/node.go | 37 +++----- hscontrol/types/node_test.go | 64 +++++++++++-- 8 files changed, 215 insertions(+), 68 deletions(-) diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 5bc05d4f0..e8f1d7c9c 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -1182,7 +1182,7 @@ func (pm *PolicyManager) NodesGeneration() uint64 { func (pm *PolicyManager) cacheableLocked(node types.NodeView) bool { own, ok := pm.nodesByID[node.ID()] - return ok && !node.HasPolicyChange(own) && !node.HasNetworkChanges(own) + return ok && !node.HasPolicyChange(own) } // nodeIDViewMap indexes a slice of node views by node ID. On duplicate IDs the @@ -1212,14 +1212,6 @@ func (pm *PolicyManager) nodesHavePolicyAffectingChanges(newNodes views.Slice[ty if newNode.HasPolicyChange(oldNode) { return true } - - // Via grants and autogroup:self compile filter rules per-node - // that depend on the node's route state (SubnetRoutes, ExitRoutes). - // Route changes are policy-affecting in this context because they - // alter which filter rules get generated for the via-designated node. - if pm.needsPerNodeFilter && newNode.HasNetworkChanges(oldNode) { - return true - } } return false @@ -2005,7 +1997,7 @@ func (pm *PolicyManager) invalidateGlobalPolicyCache(newNodes views.Slice[types. continue } - if newNode.HasNetworkChanges(oldNode) { + if newNode.HasPolicyChange(oldNode) { pm.filterRulesMap.Delete(nodeID) pm.matchersForNodeMap.Delete(nodeID) } diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index c9c331bac..afda6b84d 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -4749,3 +4749,62 @@ func TestResolvedPolicyKeepsTagIndex(t *testing.T) { require.NotNil(t, pm.resolvedPol.nodesByTag) require.NotNil(t, pm.resolvedPol.resolvedGroups) } + +func TestSetNodesUnapprovedRoutesKeepGeneration(t *testing.T) { + users := types.Users{{ID: 1, Name: "user1"}} + route := netip.MustParsePrefix("10.77.0.0/24") + + for _, tt := range []struct { + name string + policy string + }{ + {name: "global", policy: `{"acls": [{"action": "accept", + "src": ["user1@"], "dst": ["10.77.0.0/24:*"]}]}`}, + {name: "self", policy: `{"acls": [{"action": "accept", + "src": ["user1@"], "dst": ["autogroup:self:*"]}]}`}, + {name: "via", policy: `{ + "tagOwners": {"tag:router": ["user1@"]}, + "grants": [{"src": ["user1@"], "dst": ["10.77.0.0/24"], + "ip": ["*"], "via": ["tag:router"]}] + }`}, + } { + t.Run(tt.name, func(t *testing.T) { + router := node("router", "100.64.0.1", "fd7a:115c:a1e0::1", users[0]) + router.ID = 1 + client := node("client", "100.64.0.2", "fd7a:115c:a1e0::2", users[0]) + client.ID = 2 + + if tt.name == "via" { + router.Tags = []string{"tag:router"} + } + + pm, err := NewPolicyManager( + []byte(tt.policy), + users, + types.Nodes{router, client}.ViewSlice(), + ) + require.NoError(t, err) + require.Equal(t, tt.name != "global", pm.needsPerNodeFilter) + gen := pm.NodesGeneration() + + announced := router.Clone() + announced.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{route}} + changed, err := pm.SetNodes(types.Nodes{announced, client}.ViewSlice()) + require.NoError(t, err) + require.False(t, changed, "unapproved routes are not policy inputs") + require.Equal(t, gen, pm.NodesGeneration()) + require.Equal( + t, + []netip.Prefix{route}, + pm.nodesByID[router.ID].AnnouncedRoutes(), + ) + + approved := announced.Clone() + approved.ApprovedRoutes = []netip.Prefix{route} + changed, err = pm.SetNodes(types.Nodes{approved, client}.ViewSlice()) + require.NoError(t, err) + require.True(t, changed, "approval changes served routes") + require.Equal(t, gen+1, pm.NodesGeneration()) + }) + } +} diff --git a/hscontrol/servertest/mapstream_compat_test.go b/hscontrol/servertest/mapstream_compat_test.go index 94aab2820..4eb58738c 100644 --- a/hscontrol/servertest/mapstream_compat_test.go +++ b/hscontrol/servertest/mapstream_compat_test.go @@ -114,11 +114,6 @@ var mapStreamSkipReasons = map[string]string{ "route-announce/RA9/srv/presence": policyResponseToEveryNode, "route-announce/RA9/wit/presence": policyResponseToEveryNode, - "route-announce/RA7/wit/presence": announcePolicyResend, - "route-announce/RA8/wit/presence": announcePolicyResend, - "route-announce/RA10/wit/presence": announcePolicyResend, - "route-announce/RA11/wit/presence": announcePolicyResend, - "route-announce/RA0/rtr/self-routes": selfRoutesReduced, "route-announce/RA1/rtr/self-routes": selfRoutesReduced, "route-announce/RA2/rtr/self-routes": selfRoutesReduced, @@ -177,7 +172,6 @@ const ( peerHostinfoWhole = "PEER_HOSTINFO_WHOLE: headscale sends peers a node's whole reported Hostinfo; SaaS sends only Hostname, OS, Services, the SSH host keys and AppConnector" peerServicesWhole = "PEER_SERVICES_WHOLE: headscale sends peers every service a node reports; SaaS leaves out peerapi-dns-proxy" removalSentToAll = "REMOVAL_SENT_TO_ALL: headscale tells every connected node that a deleted node is gone; SaaS tells only the nodes that had it as a peer" - announcePolicyResend = "ANNOUNCE_POLICY_RESEND: under an autogroup:self or via policy, headscale sends every connected node a policy response when a router changes only routes nobody approved; SaaS sends nothing to nodes that cannot see the router" withdrawnRouteEnabled = "WITHDRAWN_ROUTE_ENABLED: headscale's routes API keeps a route the device stopped advertising in enabledRoutes; SaaS lists only advertised routes as enabled" taggedDeviceUser = "TAGGED_DEVICE_USER: SaaS shows a tagged device's user as the login that made its key; headscale shows tagged-devices" ) diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index a1231c47b..3bd44f6c7 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -211,7 +211,7 @@ type work struct { // before the write reads the same on both sides. func updateChanges(pre, post *types.Node) (bool, bool) { preView, postView := pre.View(), post.View() - if postView.HasPolicyChange(preView) || postView.HasNetworkChanges(preView) { + if postView.HasPolicyChange(preView) { return true, true } @@ -259,6 +259,8 @@ func (s *NodeStore) PutNode(n types.Node) types.NodeView { } // UpdateNodeFunc is a function type that takes a pointer to a [types.Node] and modifies it. +// Referenced data, including Hostinfo and slices, is shared with published +// snapshots. Replace it instead of mutating it in place. type UpdateNodeFunc func(n *types.Node) // UpdateNode applies a function to modify a specific node in the @@ -537,7 +539,7 @@ func (s *NodeStore) applyBatch(batch []work) { } oldGivenName := n.GivenName - pre := n.Clone() + pre := n fn(&n) if n.GivenName != oldGivenName { @@ -550,7 +552,7 @@ func (s *NodeStore) applyBatch(batch []work) { w.pre[id] = pre.View() } - relation, election := updateChanges(pre, &n) + relation, election := updateChanges(&pre, &n) relationChanged = relationChanged || relation electionChanged = electionChanged || election } diff --git a/hscontrol/state/node_store_test.go b/hscontrol/state/node_store_test.go index ddeb8dd27..56c3f47a5 100644 --- a/hscontrol/state/node_store_test.go +++ b/hscontrol/state/node_store_test.go @@ -1580,8 +1580,8 @@ func TestRebuildPeerMapsAfterStopReturns(t *testing.T) { } // TestUpdateNodeRecomputesPeersOnlyForRelationInputs pins which fields make a -// write recompute peer adjacency: the inputs that force a peer-map rebuild -// (an announced but unapproved route is included on purpose). +// write recompute peer adjacency: only inputs read by the policy require +// a peer-map rebuild. func TestUpdateNodeRecomputesPeersOnlyForRelationInputs(t *testing.T) { subnet := netip.MustParsePrefix("10.77.0.0/24") @@ -1615,6 +1615,13 @@ func TestUpdateNodeRecomputesPeersOnlyForRelationInputs(t *testing.T) { mutate: func(n *types.Node) { n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{subnet}} }, + }, + { + name: "announced and approved route", + mutate: func(n *types.Node) { + n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{subnet}} + n.ApprovedRoutes = []netip.Prefix{subnet} + }, wantRecompute: true, }, {name: "user association", mutate: func(n *types.Node) { n.User = nil }, wantRecompute: true}, @@ -1675,6 +1682,53 @@ func TestUpdateNodeDiff(t *testing.T) { assert.False(t, before.Valid()) } +func TestUpdateNodeDiffSharesUnchangedRoutes(t *testing.T) { + node := createTestNode(1, 1, "user1", "node1") + peer := createTestNode(2, 2, "user2", "node2") + announced := make([]netip.Prefix, 2048) + + ip := netip.MustParseAddr("10.0.0.0") + for i := range announced { + announced[i] = netip.PrefixFrom(ip, 32) + ip = ip.Next() + } + + node.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: announced} + node.ApprovedRoutes = []netip.Prefix{announced[0]} + lastSeen := time.Now().Add(-time.Minute) + node.LastSeen = &lastSeen + + var peerBuilds atomic.Int64 + + store := NewNodeStore(types.Nodes{&node, &peer}, + func(nodes []types.NodeView) map[types.NodeID][]types.NodeID { + peerBuilds.Add(1) + + return allowAllPeersFunc(nodes) + }, TestBatchSize, TestBatchTimeout) + store.Start() + + defer store.Stop() + + peerBuilds.Store(0) + + published := store.data.Load() + now := time.Now() + before, after, ok := store.UpdateNodeDiff(1, func(n *types.Node) { + n.LastSeen = &now + }) + require.True(t, ok) + assert.Same(t, &before.AnnouncedRoutes()[0], &after.AnnouncedRoutes()[0]) + assert.Same(t, &published.nodesByID[1].Hostinfo.RoutableIPs[0], + &after.AnnouncedRoutes()[0]) + assert.Equal(t, lastSeen, before.LastSeen().Get()) + assert.Equal(t, lastSeen, *published.nodesByID[1].LastSeen) + assert.Equal(t, now, after.LastSeen().Get()) + assert.Zero(t, peerBuilds.Load()) + assert.Same(t, &published.peersByNode[1][0], &store.data.Load().peersByNode[1][0]) + assert.Equal(t, now, store.ListPeers(2).At(0).LastSeen().Get()) +} + // TestListPeersExcludesSelf proves a node is never returned among its own // peers, on both the snapshot path and the explicit peer-ID path. // @@ -1899,9 +1953,9 @@ func TestNodeStoreAdjacencyMatchesFullBuild(t *testing.T) { // - "payload": neither; a write can reuse the previous peer adjacency. // // A field missing here fails TestUpdateChangesCoversEveryNodeField: -// classify it, and if it is a relation input, teach HasPolicyChange or -// HasNetworkChanges about it before updateChanges can let NodeStore -// reuse peer adjacency across the write. +// classify it, and if it is a relation input, teach HasPolicyChange +// about it before updateChanges can let NodeStore reuse peer adjacency +// across the write. var nodeFieldImpact = map[string]string{ "ID": "relation", "IPv4": "relation", "IPv6": "relation", "UserID": "relation", "User": "relation", "Tags": "relation", @@ -1953,16 +2007,11 @@ func TestUpdateChangesReportsRelationFields(t *testing.T) { n.ApprovedRoutes = []netip.Prefix{pfx} }, wantRelation: true, wantElection: true}, { - // Announcing a route the policy has not approved does not - // change who can access it (SubnetRoutes/ExitRoutes, which - // gate HasPolicyChange, stay empty), but HasNetworkChanges - // tracks the raw announcement so a later approval sees a - // fresh Hostinfo rather than one NodeStore decided to reuse. + // Policy reads served routes; a later approval triggers its own rebuild. name: "announced but not approved route", mutate: func(n *types.Node) { n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{pfx}} }, - wantRelation: true, wantElection: true, }, {name: "online", mutate: func(n *types.Node) { n.IsOnline = new(true) }, wantElection: true}, {name: "unhealthy", mutate: func(n *types.Node) { n.Unhealthy = true }, wantElection: true}, @@ -2030,15 +2079,20 @@ func benchNodes(n int) ([]types.User, types.Nodes) { // (lastseen) should cost far fewer builds than a relation-changing one // (route, tag). func BenchmarkNodeStoreWrite(b *testing.B) { - users, nodes := benchNodes(617) - for _, kind := range []struct { - name string - mutate func(i int, n *types.Node) + name string + announced int + mutate func(i int, n *types.Node) }{ {name: "lastseen", mutate: func(_ int, n *types.Node) { n.LastSeen = new(time.Now()) }}, + { + name: "lastseen-many-routes", announced: 4096, + mutate: func(_ int, n *types.Node) { + n.LastSeen = new(time.Now()) + }, + }, {name: "route", mutate: func(i int, n *types.Node) { subnet := netip.PrefixFrom(netip.AddrFrom4([4]byte{10, byte(200 + i%50), 0, 0}), 24) //nolint:gosec n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{subnet}} @@ -2054,6 +2108,17 @@ func BenchmarkNodeStoreWrite(b *testing.B) { n.UserID, n.User = nil, nil }}, } { + users, nodes := benchNodes(617) + if kind.announced > 0 { + nodes[0].Hostinfo.RoutableIPs = make([]netip.Prefix, kind.announced) + + ip := netip.MustParseAddr("10.0.0.0") + for i := range kind.announced { + nodes[0].Hostinfo.RoutableIPs[i] = netip.PrefixFrom(ip, 32) + ip = ip.Next() + } + } + b.Run(fmt.Sprintf("%s/n=%d", kind.name, len(nodes)), func(b *testing.B) { var calls atomic.Int64 diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 7c3a850c5..22df631fe 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -3876,7 +3876,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest // update that shrinks the announced set can drop the node // out of HA candidacy without touching ApprovedRoutes. // Clear any stale Unhealthy bit in that case. - if len(currentNode.AllApprovedRoutes()) == 0 { + if currentNode.Unhealthy && len(currentNode.AllApprovedRoutes()) == 0 { currentNode.Unhealthy = false } }) diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 0a583142c..3e716c9e0 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -1198,31 +1198,6 @@ func (nv NodeView) IPsAsString() []string { return nv.ж.IPsAsString() } -// HasNetworkChanges checks if the node has network-related changes. -// Returns true if IPs, announced routes, or approved routes changed. -// This is primarily used for policy cache invalidation. Route slices -// are compared order-insensitively since clients may re-advertise the -// same routes in a different order. -func (nv NodeView) HasNetworkChanges(other NodeView) bool { - if !slices.Equal(nv.IPs(), other.IPs()) { - return true - } - - if !equalPrefixesUnordered(nv.AnnouncedRoutes(), other.AnnouncedRoutes()) { - return true - } - - if !equalPrefixesUnordered(nv.SubnetRoutes(), other.SubnetRoutes()) { - return true - } - - if !equalPrefixesUnordered(nv.ExitRoutes(), other.ExitRoutes()) { - return true - } - - return false -} - // equalPrefixesUnordered reports whether a and b contain the same // prefixes, order-independent. Inputs are cloned before sorting so // callers' slices are not mutated. @@ -1247,6 +1222,13 @@ func equalUnordered[E comparable](a, b []E, cmp func(E, E) int) bool { return slices.Equal(ac, bc) } +// Unchanged snapshots share immutable slices, so pointer equality avoids a scan. +func sameSlice[E comparable](a, b []E) bool { + aView, bView := views.SliceOf(a), views.SliceOf(b) + + return aView.MapKey() == bView.MapKey() || views.SliceEqual(aView, bView) +} + // HasPolicyChange reports whether the node has changes that affect // policy evaluation. Includes approved subnet routes because they act // as source identity in [Node.CanAccess] for subnet-to-subnet ACLs, @@ -1272,6 +1254,11 @@ func (nv NodeView) HasPolicyChange(other NodeView) bool { return true } + if sameSlice(nv.AnnouncedRoutes(), other.AnnouncedRoutes()) && + sameSlice(nv.ж.ApprovedRoutes, other.ж.ApprovedRoutes) { + return false + } + if !equalPrefixesUnordered(nv.SubnetRoutes(), other.SubnetRoutes()) { return true } diff --git a/hscontrol/types/node_test.go b/hscontrol/types/node_test.go index 3a1a92b21..3a561dd6f 100644 --- a/hscontrol/types/node_test.go +++ b/hscontrol/types/node_test.go @@ -687,9 +687,9 @@ func TestApplyPeerChange(t *testing.T) { } } -// TestHasNetworkChanges tests the [NodeView] method for detecting -// when a node's network properties have changed. -func TestHasNetworkChanges(t *testing.T) { +// TestHasPolicyChangeAddressesAndRoutes covers the addresses and served +// routes that policy evaluation reads. +func TestHasPolicyChangeAddressesAndRoutes(t *testing.T) { mustIPPtr := func(s string) *netip.Addr { ip := netip.MustParseAddr(s) return &ip @@ -759,7 +759,7 @@ func TestHasNetworkChanges(t *testing.T) { IPv4: mustIPPtr("100.64.0.1"), Hostinfo: &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{netip.MustParsePrefix("10.0.0.0/24")}}, }, - changed: true, + changed: false, }, { name: "RoutableIPs removed", @@ -773,7 +773,7 @@ func TestHasNetworkChanges(t *testing.T) { IPv4: mustIPPtr("100.64.0.1"), Hostinfo: &tailcfg.Hostinfo{}, }, - changed: true, + changed: false, }, { name: "RoutableIPs changed", @@ -787,7 +787,7 @@ func TestHasNetworkChanges(t *testing.T) { IPv4: mustIPPtr("100.64.0.1"), Hostinfo: &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{netip.MustParsePrefix("192.168.0.0/24")}}, }, - changed: true, + changed: false, }, { name: "SubnetRoutes added", @@ -886,9 +886,9 @@ func TestHasNetworkChanges(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := tt.new.View().HasNetworkChanges(tt.old.View()) + got := tt.new.View().HasPolicyChange(tt.old.View()) if got != tt.changed { - t.Errorf("HasNetworkChanges() = %v, want %v", got, tt.changed) + t.Errorf("HasPolicyChange() = %v, want %v", got, tt.changed) } }) } @@ -1076,6 +1076,54 @@ func TestNodeIsExpiredAt(t *testing.T) { }) } +func TestHasPolicyChangeSharedRouteInputs(t *testing.T) { + subnet := netip.MustParsePrefix("10.0.0.0/24") + exit := netip.MustParsePrefix("::/0") + base := Node{ + Hostinfo: &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{subnet, exit}}, + ApprovedRoutes: []netip.Prefix{subnet, exit}, + } + + for _, tt := range []struct { + name string + mutate func(*Node) + want bool + }{ + {name: "shared slices", mutate: func(*Node) {}}, + {name: "equal slices", mutate: func(n *Node) { + n.Hostinfo = n.Hostinfo.Clone() + n.ApprovedRoutes = []netip.Prefix{subnet, exit} + }}, + {name: "reordered slices", mutate: func(n *Node) { + n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{exit, subnet}} + n.ApprovedRoutes = []netip.Prefix{exit, subnet} + }}, + {name: "approval removed", mutate: func(n *Node) { + n.ApprovedRoutes = []netip.Prefix{subnet} + }, want: true}, + {name: "announcement removed", mutate: func(n *Node) { + n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{subnet}} + }, want: true}, + } { + t.Run(tt.name, func(t *testing.T) { + other := base + tt.mutate(&other) + require.Equal(t, tt.want, base.View().HasPolicyChange(other.View())) + }) + } + + shared := base + + var changed bool + + allocs := testing.AllocsPerRun(10, func() { + changed = base.View().HasPolicyChange(shared.View()) + }) + + require.False(t, changed) + require.Zero(t, allocs, "unchanged route inputs should not derive route lists") +} + func TestValidateHostinfo(t *testing.T) { t.Parallel()