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..9572538bb 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 } diff --git a/hscontrol/state/node_store_test.go b/hscontrol/state/node_store_test.go index ddeb8dd27..5e6cb48d2 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}, @@ -1899,9 +1906,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 +1960,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}, diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 0a583142c..b01c82d32 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. diff --git a/hscontrol/types/node_test.go b/hscontrol/types/node_test.go index 3a1a92b21..30291fbf1 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) } }) }