From 3b4b9a443684858571edd2be70f407b3ad5de055 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 15 Jan 2026 13:56:48 +0000 Subject: [PATCH] hscontrol: fix tag updates not propagating to node self view When SetNodeTags changed a node's tags, the node's self view wasn't updated. The bug manifested as: the first SetNodeTags call updates the server but the client's self view doesn't update until a second call with the same tag. Root cause: Three issues combined to prevent self-updates: 1. SetNodeTags returned PolicyChange which doesn't set OriginNode, so the mapper's self-update check failed. 2. The Change.Merge function didn't preserve OriginNode, so when changes were batched together, OriginNode was lost. 3. generateMapResponse checked OriginNode only in buildFromChange(), but PolicyChange uses RequiresRuntimePeerComputation which bypasses that code path entirely and calls policyChangeResponse() instead. The fix addresses all three: - state.go: Set OriginNode on the returned change - change.go: Preserve OriginNode (and TargetNode) during merge - batcher.go: Pass isSelfUpdate to policyChangeResponse so the origin node gets both self info AND packet filters - mapper.go: Add includeSelf parameter to policyChangeResponse Fixes #2978 --- hscontrol/mapper/batcher.go | 12 ++++++++++- hscontrol/mapper/mapper.go | 9 ++++++++ hscontrol/state/state.go | 13 +++++++++++- hscontrol/types/change/change.go | 12 +++++++++++ hscontrol/types/change/change_test.go | 30 +++++++++++++++++++++++++++ 5 files changed, 74 insertions(+), 2 deletions(-) diff --git a/hscontrol/mapper/batcher.go b/hscontrol/mapper/batcher.go index 37f3cc24..0a1e30d0 100644 --- a/hscontrol/mapper/batcher.go +++ b/hscontrol/mapper/batcher.go @@ -92,6 +92,11 @@ func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*t return nil, nil //nolint:nilnil // No response needed for other nodes when self-only } + // Check if this is a self-update (the changed node is the receiving node). + // When true, ensure the response includes the node's self info so it sees + // its own attribute changes (e.g., tags changed via admin API). + isSelfUpdate := r.OriginNode != 0 && r.OriginNode == nodeID + var ( mapResp *tailcfg.MapResponse err error @@ -110,7 +115,12 @@ func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*t } removedPeers := nc.computePeerDiff(currentPeerIDs) - mapResp, err = mapper.policyChangeResponse(nodeID, version, removedPeers, currentPeers) + // Include self node when this is a self-update (e.g., node's own tags changed) + // so the node sees its updated self info along with new packet filters. + mapResp, err = mapper.policyChangeResponse(nodeID, version, removedPeers, currentPeers, isSelfUpdate) + } else if isSelfUpdate { + // Non-policy self-update: just send the self node info + mapResp, err = mapper.selfMapResponse(nodeID, version) } else { mapResp, err = mapper.buildFromChange(nodeID, version, &r) } diff --git a/hscontrol/mapper/mapper.go b/hscontrol/mapper/mapper.go index 759c9568..616d470f 100644 --- a/hscontrol/mapper/mapper.go +++ b/hscontrol/mapper/mapper.go @@ -187,13 +187,18 @@ func (m *mapper) selfMapResponse( // - PeersChanged for remaining peers (their AllowedIPs may have changed due to policy) // - Updated PacketFilters // - Updated SSHPolicy (SSH rules may reference users/groups that changed) +// - Optionally, the node's own self info (when includeSelf is true) // This avoids the issue where an empty Peers slice is interpreted by Tailscale // clients as "no change" rather than "no peers". +// When includeSelf is true, the node's self info is included so that a node +// whose own attributes changed (e.g., tags via admin API) sees its updated +// self info along with the new packet filters. func (m *mapper) policyChangeResponse( nodeID types.NodeID, capVer tailcfg.CapabilityVersion, removedPeers []tailcfg.NodeID, currentPeers views.Slice[types.NodeView], + includeSelf bool, ) (*tailcfg.MapResponse, error) { builder := m.NewMapResponseBuilder(nodeID). WithDebugType(policyResponseDebug). @@ -201,6 +206,10 @@ func (m *mapper) policyChangeResponse( WithPacketFilters(). WithSSHPolicy() + if includeSelf { + builder = builder.WithSelfNode() + } + if len(removedPeers) > 0 { // Convert tailcfg.NodeID to types.NodeID for WithPeersRemoved removedIDs := make([]types.NodeID, len(removedPeers)) diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 3b916ad6..0a5fabdb 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -719,7 +719,18 @@ func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView, return types.NodeView{}, change.Change{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, nodeID) } - return s.persistNodeToDB(n) + nodeView, c, err := s.persistNodeToDB(n) + if err != nil { + return nodeView, c, err + } + + // Set OriginNode so the mapper knows to include self info for this node. + // When tags change, persistNodeToDB returns PolicyChange which doesn't set OriginNode, + // so the mapper's self-update check fails and the node never sees its new tags. + // Setting OriginNode ensures the node gets a self-update with the new tags. + c.OriginNode = nodeID + + return nodeView, c, nil } // SetApprovedRoutes sets the network routes that a node is approved to advertise. diff --git a/hscontrol/types/change/change.go b/hscontrol/types/change/change.go index 307ef690..a76fb7c4 100644 --- a/hscontrol/types/change/change.go +++ b/hscontrol/types/change/change.go @@ -70,6 +70,18 @@ func (r Change) Merge(other Change) Change { merged.PeersRemoved = uniqueNodeIDs(append(r.PeersRemoved, other.PeersRemoved...)) merged.PeerPatches = append(r.PeerPatches, other.PeerPatches...) + // Preserve OriginNode for self-update detection. + // If either change has OriginNode set, keep it so the mapper + // can detect self-updates and send the node its own changes. + if merged.OriginNode == 0 { + merged.OriginNode = other.OriginNode + } + + // Preserve TargetNode for targeted responses. + if merged.TargetNode == 0 { + merged.TargetNode = other.TargetNode + } + if r.Reason != "" && other.Reason != "" && r.Reason != other.Reason { merged.Reason = r.Reason + "; " + other.Reason } else if other.Reason != "" { diff --git a/hscontrol/types/change/change_test.go b/hscontrol/types/change/change_test.go index 30330584..9f181dd6 100644 --- a/hscontrol/types/change/change_test.go +++ b/hscontrol/types/change/change_test.go @@ -233,6 +233,36 @@ func TestChange_Merge(t *testing.T) { r2: Change{Reason: "update"}, want: Change{Reason: "update"}, }, + { + name: "OriginNode preserved from first", + r1: Change{OriginNode: 42}, + r2: Change{IncludePolicy: true}, + want: Change{OriginNode: 42, IncludePolicy: true}, + }, + { + name: "OriginNode preserved from second when first is zero", + r1: Change{IncludePolicy: true}, + r2: Change{OriginNode: 42}, + want: Change{OriginNode: 42, IncludePolicy: true}, + }, + { + name: "OriginNode first wins when both set", + r1: Change{OriginNode: 1}, + r2: Change{OriginNode: 2}, + want: Change{OriginNode: 1}, + }, + { + name: "TargetNode preserved from first", + r1: Change{TargetNode: 42}, + r2: Change{IncludeSelf: true}, + want: Change{TargetNode: 42, IncludeSelf: true}, + }, + { + name: "TargetNode preserved from second when first is zero", + r1: Change{IncludeSelf: true}, + r2: Change{TargetNode: 42}, + want: Change{TargetNode: 42, IncludeSelf: true}, + }, } for _, tt := range tests {