From 4fd766a431df0e16db1214cdf7b6ce8168ed1189 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 30 Sep 2026 11:45:30 +0000 Subject: [PATCH] policy/v2: leave the policy manager unchanged when a recompile fails updateLocked runs every fallible step before writing pm, so a failed SetUsers or SetNodes no longer leaves half a new filter live. --- hscontrol/policy/v2/policy.go | 104 ++++++++++---------- hscontrol/policy/v2/policy_test.go | 151 +++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+), 54 deletions(-) diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index edcfbf239..00d2404a1 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -238,20 +238,53 @@ func NewPolicyManager(b []byte, users []types.User, nodes views.Slice[types.Node } // updateLocked updates the filter rules based on the current policy and nodes. -// It must be called with the lock held. +// It must be called with the lock held. Every step that can fail runs before +// pm is written, so a caller that restores its own input on error leaves pm +// as it was, and a retry with the same input recompiles. func (pm *PolicyManager) updateLocked() (bool, error) { // Compile all grants once. Both global and per-node filter // rules are derived from these compiled grants. - pm.compiledGrants = pm.pol.compileGrants(pm.users, pm.nodes) - pm.userNodeIdx = buildUserNodeIndex(pm.nodes) - pm.needsPerNodeFilter = hasPerNodeGrants(pm.compiledGrants) - pm.viaTargetTags = collectViaTargetTags(pm.compiledGrants) + grants := pm.pol.compileGrants(pm.users, pm.nodes) - relayTargetIPs, err := collectRelayTargetIPs(pm.compiledGrants) + relayTargetIPs, err := collectRelayTargetIPs(grants) if err != nil { return false, fmt.Errorf("collecting relay target IPs: %w", err) } + // Order matters, tags might be used in autoapprovers, so we need to ensure + // that the map for tag owners is resolved before resolving autoapprovers. + // TODO(kradalby): Order might not matter after #2417 + tagMap, err := resolveTagOwners(pm.pol, pm.users, pm.nodes) + if err != nil { + return false, fmt.Errorf("resolving tag owners map: %w", err) + } + + autoMap, exitSet, err := resolveAutoApprovers(pm.pol, pm.users, pm.nodes) + if err != nil { + return false, fmt.Errorf("resolving auto approvers map: %w", err) + } + + // Fast path for the common steady-state shape: tailnet has no + // nodeAttrs entries and never had any. Skip the compile + per-node + // hash walk entirely. As soon as the operator adds a nodeAttrs + // entry pm.nodeAttrsHashes becomes non-empty and the gate opens. + refreshNodeAttrs := pm.pol == nil || + len(pm.pol.NodeAttrs) > 0 || + pm.pol.RandomizeClientPort || + len(pm.nodeAttrsHashes) > 0 + + var nodeAttrs map[types.NodeID]tailcfg.NodeCapMap + if refreshNodeAttrs { + nodeAttrs, err = pm.pol.compileNodeAttrs(pm.users, pm.nodes) + if err != nil { + return false, fmt.Errorf("compiling nodeAttrs: %w", err) + } + } + + pm.compiledGrants = grants + pm.userNodeIdx = buildUserNodeIndex(pm.nodes) + pm.needsPerNodeFilter = hasPerNodeGrants(pm.compiledGrants) + pm.viaTargetTags = collectViaTargetTags(pm.compiledGrants) pm.relayTargetIPs = relayTargetIPs var filter []tailcfg.FilterRule @@ -287,14 +320,6 @@ func (pm *PolicyManager) updateLocked() (bool, error) { pm.matchers = matcher.MatchesFromFilterRules(pm.filter) } - // Order matters, tags might be used in autoapprovers, so we need to ensure - // that the map for tag owners is resolved before resolving autoapprovers. - // TODO(kradalby): Order might not matter after #2417 - tagMap, err := resolveTagOwners(pm.pol, pm.users, pm.nodes) - if err != nil { - return false, fmt.Errorf("resolving tag owners map: %w", err) - } - tagOwnerMapHash := deephash.Hash(&tagMap) tagOwnerChanged := tagOwnerMapHash != pm.tagOwnerMapHash @@ -310,11 +335,6 @@ func (pm *PolicyManager) updateLocked() (bool, error) { pm.tagOwnerMap = tagMap pm.tagOwnerMapHash = tagOwnerMapHash - autoMap, exitSet, err := resolveAutoApprovers(pm.pol, pm.users, pm.nodes) - if err != nil { - return false, fmt.Errorf("resolving auto approvers map: %w", err) - } - autoApproveMapHash := deephash.Hash(&autoMap) autoApproveChanged := autoApproveMapHash != pm.autoApproveMapHash @@ -343,14 +363,13 @@ func (pm *PolicyManager) updateLocked() (bool, error) { pm.exitSet = exitSet pm.exitSetHash = exitSetHash - // Recompile per-node nodeAttrs CapMap and append the diff to + // Install the per-node nodeAttrs CapMap and append the diff to // pm.nodeAttrsChanged. The drain (NodesWithChangedCapMap) returns // the accumulated union of every change since the last drain; // SetUsers/SetNodes appending between SetPolicy and the drain // cannot lose the policy-reload diff. - err = pm.refreshNodeAttrsLocked() - if err != nil { - return false, err + if refreshNodeAttrs { + pm.refreshNodeAttrsLocked(nodeAttrs) } // Determine if we need to send updates to nodes @@ -593,15 +612,9 @@ func (pm *PolicyManager) SetPolicy(polB []byte) (bool, error) { changed, err := pm.updateLocked() if err != nil { - // updateLocked stops partway, so the rejected policy's filter may - // already be live; recompile the previous one. + // pm still holds what prev compiled to. pm.pol = prev - _, rerr := pm.updateLocked() - if rerr != nil { - log.Error().Err(rerr).Msg("restoring previous policy after rejected SetPolicy") - } - return false, err } @@ -864,8 +877,9 @@ func (pm *PolicyManager) SetUsers(users []types.User) (bool, bool, error) { policyChanged, err := pm.updateLocked() if err != nil { - // Keep the old list so a retry with the same input recompiles - // instead of being treated as unchanged. + // Keep the old list, which pm is still compiled from, so a retry + // with the same input recompiles instead of being treated as + // unchanged. pm.users = prev return false, false, err @@ -927,8 +941,8 @@ func (pm *PolicyManager) SetNodes(nodes views.Slice[types.NodeView]) (bool, erro // Recompile filter with the new node list needsUpdate, err := pm.updateLocked() if err != nil { - // Keep the old list so a retry with the same input recompiles - // instead of being treated as unchanged, as in SetUsers. The + // Keep the old list, which pm is still compiled from, so a + // retry with the same input recompiles, as in SetUsers. The // NodeStore writer only logs this error; the writing caller's // own SetNodes then retries and returns it. pm.nodes, pm.nodesByID = prevNodes, prevByID @@ -1883,7 +1897,7 @@ func resolveTagOwners(p *Policy, users types.Users, nodes views.Slice[types.Node return ret, nil } -// refreshNodeAttrsLocked recompiles the per-node nodeAttrs CapMap and +// refreshNodeAttrsLocked installs a compiled per-node nodeAttrs CapMap and // appends the IDs whose CapMap differs from the previous snapshot // (including newly-targeted nodes and nodes that lost all attrs) to // pm.nodeAttrsChanged. Append, not overwrite: a concurrent @@ -1891,23 +1905,7 @@ func resolveTagOwners(p *Policy, users types.Users, nodes views.Slice[types.Node // drain cannot clobber the policy-reload diff. // // Caller must hold pm.mu. -func (pm *PolicyManager) refreshNodeAttrsLocked() error { - // Fast path for the common steady-state shape: tailnet has no - // nodeAttrs entries and never had any. Skip the compile + per-node - // hash walk entirely. As soon as the operator adds a nodeAttrs - // entry pm.nodeAttrsHashes becomes non-empty and the gate opens. - if pm.pol != nil && - len(pm.pol.NodeAttrs) == 0 && - !pm.pol.RandomizeClientPort && - len(pm.nodeAttrsHashes) == 0 { - return nil - } - - newMap, err := pm.pol.compileNodeAttrs(pm.users, pm.nodes) - if err != nil { - return fmt.Errorf("compiling nodeAttrs: %w", err) - } - +func (pm *PolicyManager) refreshNodeAttrsLocked(newMap map[types.NodeID]tailcfg.NodeCapMap) { newHashes := make(map[types.NodeID]deephash.Sum, len(newMap)) for id, capMap := range newMap { newHashes[id] = deephash.Hash(&capMap) @@ -1936,8 +1934,6 @@ func (pm *PolicyManager) refreshNodeAttrsLocked() error { pm.nodeAttrsMap = newMap pm.nodeAttrsHashes = newHashes pm.nodeAttrsChanged = append(pm.nodeAttrsChanged, changed...) - - return nil } // NodeCapMap returns the policy-derived CapMap for the given node, or diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index e140453f8..67603f650 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -3016,3 +3016,154 @@ func TestSetNodesRetriesAfterFailedRecompile(t *testing.T) { require.True(t, changed, "the retry must recompile, not see the failed input as current") require.Equal(t, gen+1, pm.NodesGeneration()) } + +// TestFailedSetUsersKeepsCompiledPolicy renames alice to charlie while +// nodeAttrs still names alice, so the recompile resolves charlie's grant +// and then fails. A failed Set* must leave every compiled result as it was: +// a partial compile would hand alice's nodes charlie's port 22, and the +// reverse rename, which SetUsers sees as no change, would not clear it. +func TestFailedSetUsersKeepsCompiledPolicy(t *testing.T) { + users := types.Users{{ID: 1, Name: "alice"}, {ID: 2, Name: "bob"}} + nodes := types.Nodes{ + node("a1", "100.64.0.1", "fd7a:115c:a1e0::1", users[0]), + node("a2", "100.64.0.2", "fd7a:115c:a1e0::2", users[0]), + node("b1", "100.64.0.3", "fd7a:115c:a1e0::3", users[1]), + } + + for i, n := range nodes { + n.ID = types.NodeID(i + 1) //nolint:gosec + } + + pol := []byte(`{ + "acls": [ + {"action": "accept", "src": ["charlie@"], "dst": ["autogroup:self:22"]}, + {"action": "accept", "src": ["bob@"], "dst": ["bob@:*"]} + ], + "ssh": [{"action": "accept", "src": ["charlie@"], "dst": ["autogroup:self"], "users": ["root"]}], + "nodeAttrs": [{"target": ["alice@"], "attr": ["randomize-client-port"]}] + }`) + + pm, err := NewPolicyManager(pol, users, nodes.ViewSlice()) + require.NoError(t, err) + + type view struct { + filter []tailcfg.FilterRule + ssh *tailcfg.SSHPolicy + } + + read := func(pm *PolicyManager, n *types.Node) view { + t.Helper() + + f, err := pm.FilterForNode(n.View()) + require.NoError(t, err) + + ssh, err := pm.SSHPolicy("", n.View()) + require.NoError(t, err) + + return view{filter: f, ssh: ssh} + } + + before := read(pm, nodes[0]) + + renamed := slices.Clone(users) + renamed[0].Name = "charlie" + + _, _, err = pm.SetUsers(renamed) + require.Error(t, err) + require.Equal(t, before, read(pm, nodes[0]), "a failed SetUsers must not change compiled results") + + _, _, err = pm.SetUsers(users) + require.NoError(t, err) + require.Equal(t, before, read(pm, nodes[0]), "reversing the rename must serve the original results") + + b2 := node("b2", "100.64.0.4", "fd7a:115c:a1e0::4", users[1]) + b2.ID = 4 + grown := append(slices.Clone(nodes), b2) + + _, err = pm.SetNodes(grown.ViewSlice()) + require.NoError(t, err) + + fresh, err := NewPolicyManager(pol, users, grown.ViewSlice()) + require.NoError(t, err) + + for _, n := range grown { + require.Equal(t, read(fresh, n), read(pm, n), + "node %s after a later write must match a fresh compile", n.Hostname) + } +} + +// TestFailedSetNodesKeepsCompiledPolicy fails a SetNodes that retags a +// node, then retries it once the failure is gone. The failed write must +// leave the compiled filter as it was, and the retry must compile the new +// owners. +func TestFailedSetNodesKeepsCompiledPolicy(t *testing.T) { + users := types.Users{{ID: 1, Name: "alice"}, {ID: 2, Name: "bob"}} + nodes := types.Nodes{ + node("a1", "100.64.0.1", "fd7a:115c:a1e0::1", users[0]), + node("a2", "100.64.0.2", "fd7a:115c:a1e0::2", users[0]), + node("b1", "100.64.0.3", "fd7a:115c:a1e0::3", users[1]), + } + + for i, n := range nodes { + n.ID = types.NodeID(i + 1) //nolint:gosec + } + + pol := []byte(`{ + "tagOwners": {"tag:srv": ["alice@"]}, + "acls": [{"action": "accept", "src": ["alice@"], "dst": ["alice@:*"]}], + "ssh": [{"action": "accept", "src": ["alice@"], "dst": ["alice@"], "users": ["root"]}] + }`) + + pm, err := NewPolicyManager(pol, users, nodes.ViewSlice()) + require.NoError(t, err) + + read := func(pm *PolicyManager, n *types.Node) ([]tailcfg.FilterRule, *tailcfg.SSHPolicy) { + t.Helper() + + f, err := pm.FilterForNode(n.View()) + require.NoError(t, err) + + ssh, err := pm.SSHPolicy("", n.View()) + require.NoError(t, err) + + return f, ssh + } + + beforeFilter, beforeSSH := read(pm, nodes[0]) + beforeGlobal, _ := pm.Filter() + + retagged := slices.Clone(nodes) + tagged := nodes[1].Clone() + tagged.Tags = []string{"tag:srv"} + tagged.UserID, tagged.User = nil, nil + retagged[1] = tagged + + good := pm.pol.TagOwners + missing := Tag("tag:missing") + pm.pol.TagOwners = TagOwners{"tag:srv": Owners{&missing}} + + _, err = pm.SetNodes(retagged.ViewSlice()) + require.Error(t, err) + + pm.pol.TagOwners = good + + gotFilter, gotSSH := read(pm, nodes[0]) + require.Equal(t, beforeFilter, gotFilter, "a failed SetNodes must not change compiled results") + require.Equal(t, beforeSSH, gotSSH, "a failed SetNodes must not change compiled results") + + gotGlobal, _ := pm.Filter() + require.Equal(t, beforeGlobal, gotGlobal, "a failed SetNodes must not change compiled results") + + _, err = pm.SetNodes(retagged.ViewSlice()) + require.NoError(t, err) + + fresh, err := NewPolicyManager(pol, users, retagged.ViewSlice()) + require.NoError(t, err) + + for _, n := range retagged { + wantFilter, wantSSH := read(fresh, n) + gotFilter, gotSSH := read(pm, n) + require.Equal(t, wantFilter, gotFilter, "node %s filter after the retry", n.Hostname) + require.Equal(t, wantSSH, gotSSH, "node %s SSH after the retry", n.Hostname) + } +}