From 05fc17bc85ab8e7e5fb373094d2269480930471b Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 25 Sep 2026 17:36:26 +0000 Subject: [PATCH] state: decide whole-peer updates at each caller, not in persist persistNodeAndRefreshPolicy no longer fabricates NodeAdded; RenameNode and SetNodeTags add it since both are peer visible. --- hscontrol/state/persist_test.go | 158 ++++++++++++++++++++++++++++++++ hscontrol/state/state.go | 28 ++++-- 2 files changed, 177 insertions(+), 9 deletions(-) diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 921618578..233eb15da 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -10,6 +10,7 @@ import ( "github.com/juanfont/headscale/hscontrol/db" "github.com/juanfont/headscale/hscontrol/policy" "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/types/change" "github.com/juanfont/headscale/hscontrol/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -713,3 +714,160 @@ func TestSingleUsePreAuthKeyUsedInNodeStore(t *testing.T) { second := register(t) assert.Equal(t, first.ID(), second.ID(), "second key re-registers the same node") } + +// TestPersistNodeAndRefreshPolicyEmptyForPayloadOnlyChange proves +// persistNodeAndRefreshPolicy returns an empty change when the write did not +// touch anything policy reads, so each caller can tell a genuinely empty +// write apart and pick its own wire change (see +// TestPersistCallerChangeDecisions). +func TestPersistNodeAndRefreshPolicyEmptyForPayloadOnlyChange(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + view, ok := s.nodeStore.UpdateNode(nodeID, func(n *types.Node) { + n.Hostinfo = &tailcfg.Hostinfo{Hostname: "payload-only"} + }) + require.True(t, ok) + + _, c, err := s.persistNodeAndRefreshPolicy(view) + require.NoError(t, err) + assert.True(t, c.IsEmpty(), "a payload-only write must not fabricate a change") +} + +// TestPersistCallerChangeDecisions proves each persistNodeAndRefreshPolicy +// caller picks its own wire change when persist reports none. A caller that +// returns nothing silently stops a node's peers from learning about it, so +// every case here checks the exact change, not just that persist succeeded. +func TestPersistCallerChangeDecisions(t *testing.T) { + taggingPolicy := `{ + "tagOwners": {"tag:ci": ["persist-user@"]}, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }` + + tests := []struct { + name string + policy string + setup func(t *testing.T, s *State, nodeID types.NodeID) + run func(t *testing.T, s *State, nodeID types.NodeID) change.Change + wantType string + wantOriginNode bool + wantPeersChanged bool + }{ + { + name: "RenameNode resends the whole node when the rename does not affect policy", + run: func(t *testing.T, s *State, nodeID types.NodeID) change.Change { + t.Helper() + + _, c, err := s.RenameNode(nodeID, "renamed") + require.NoError(t, err) + + return c + }, + wantType: "peers", + wantOriginNode: true, + wantPeersChanged: true, + }, + { + name: "SetNodeTags reports a policy change for a real tag assignment", + policy: taggingPolicy, + run: func(t *testing.T, s *State, nodeID types.NodeID) change.Change { + t.Helper() + + _, c, err := s.SetNodeTags(nodeID, []string{"tag:ci"}) + require.NoError(t, err) + + return c + }, + wantType: "policy", + wantOriginNode: true, + wantPeersChanged: false, + }, + { + name: "SetNodeTags resends the whole node when re-applying identical tags", + policy: taggingPolicy, + setup: func(t *testing.T, s *State, nodeID types.NodeID) { + t.Helper() + + _, _, err := s.SetNodeTags(nodeID, []string{"tag:ci"}) + require.NoError(t, err) + }, + run: func(t *testing.T, s *State, nodeID types.NodeID) change.Change { + t.Helper() + + _, c, err := s.SetNodeTags(nodeID, []string{"tag:ci"}) + require.NoError(t, err) + + return c + }, + wantType: "peers", + wantOriginNode: true, + wantPeersChanged: true, + }, + { + name: "SetApprovedRoutes keeps reporting a policy change (Task 6 narrows this)", + run: func(t *testing.T, s *State, nodeID types.NodeID) change.Change { + t.Helper() + + _, c, err := s.SetApprovedRoutes(nodeID, []netip.Prefix{netip.MustParsePrefix("10.0.0.0/8")}) + require.NoError(t, err) + + return c + }, + wantType: "policy", + wantOriginNode: false, + wantPeersChanged: false, + }, + { + name: "SaveNode reports no change for a payload-only save", + run: func(t *testing.T, s *State, nodeID types.NodeID) change.Change { + t.Helper() + + current, ok := s.nodeStore.GetNode(nodeID) + require.True(t, ok) + + n := current.AsStruct() + n.Hostinfo = &tailcfg.Hostinfo{Hostname: "saved-payload"} + + _, c, err := s.SaveNode(n.View()) + require.NoError(t, err) + + return c + }, + wantType: "empty", + wantOriginNode: false, + wantPeersChanged: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + if tt.policy != "" { + _, err := s.SetPolicy([]byte(tt.policy)) + require.NoError(t, err) + } + + if tt.setup != nil { + tt.setup(t, s, nodeID) + } + + c := tt.run(t, s, nodeID) + + assert.Equal(t, tt.wantType, c.Type()) + + if tt.wantOriginNode { + assert.Equal(t, nodeID, c.OriginNode) + } else { + assert.Zero(t, c.OriginNode) + } + + if tt.wantPeersChanged { + assert.Equal(t, []types.NodeID{nodeID}, c.PeersChanged) + } else { + assert.Empty(t, c.PeersChanged) + } + }) + } +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 726ce26d9..edbaf415f 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -577,10 +577,6 @@ func (s *State) persistNodeAndRefreshPolicy(node types.NodeView) (types.NodeView return fresh, change.Change{}, fmt.Errorf("updating policy manager after node save: %w", err) } - if c.IsEmpty() { - c = change.NodeAdded(node.ID()) - } - return fresh, c, nil } @@ -1009,6 +1005,12 @@ func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView, return nodeView, c, err } + if c.IsEmpty() { + // Tags are peer visible (tag owner resolution, ACLs); resend the + // whole node even when re-applying the same tags didn't move policy. + c = change.NodeAdded(nodeID) + } + // Set OriginNode so the mapper knows to include self info for this node. // When tags change, persistNodeAndRefreshPolicy returns PolicyChange which doesn't set OriginNode, // so the mapper's self-update check fails and the node never sees its new tags. @@ -1080,7 +1082,17 @@ func (s *State) RenameNode(nodeID types.NodeID, newName string) (types.NodeView, } } - return s.persistNodeAndRefreshPolicy(view) + nodeView, c, err := s.persistNodeAndRefreshPolicy(view) + if err != nil { + return nodeView, c, err + } + + if c.IsEmpty() { + // A rename is peer visible; resend the whole node. + c = change.NodeAdded(nodeID) + } + + return nodeView, c, nil } // BackfillNodeIPs assigns IP addresses to nodes that don't have them. @@ -3351,10 +3363,8 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest // leaves the node untouched, so skip the full-row UPDATE and the O(n) // policy SetNodes scan that persistNodeAndRefreshPolicy performs. // - // On the MapRequest path we deliberately bypass persistNodeAndRefreshPolicy's - // synthetic NodeAdded fallback: persistence must not fabricate a wire - // notification. We persist the row directly and refresh the policy - // manager only when node inputs visible to the policy actually changed + // Otherwise we persist the row directly and refresh the policy manager + // only when node inputs visible to the policy actually changed // (structural Hostinfo or routes), letting updatePolicyManagerNodes // decide whether matchers changed. policyChange := change.Change{}