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

@@ -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

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)
+	}
+}