From 1bd62737b9de257fd093aa45f9d65e6d4d702732 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Mon, 28 Sep 2026 09:19:52 +0000 Subject: [PATCH] mapper: send removed peers as their own delta Removals derived from a policy change, full update or reconnect rode a response whose DNSConfig, SSHPolicy, Node or Peers force a full rebuild. Updates tailscale/tailscale#15660 --- CHANGELOG.md | 1 + hscontrol/capver/capver.go | 10 +- hscontrol/mapper/batcher.go | 153 +++++++++++++++++----------- hscontrol/mapper/batcher_test.go | 89 ++++++++++++++++ hscontrol/mapper/mapper.go | 15 +-- hscontrol/mapper/mapper_test.go | 22 ++-- hscontrol/servertest/issues_test.go | 3 + 7 files changed, 210 insertions(+), 83 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c824954e..154fd7f2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -116,6 +116,7 @@ clients, and how to run the same setup without Nix. - Improve systemd service file hardening [#3341](https://github.com/juanfont/headscale/pull/3341) - Fix `headscale users destroy`/`rename` reporting "multiple users match query" when no user matches; an ambiguous match now lists the matching users [#3476](https://github.com/juanfont/headscale/pull/3476) - Deleting a user that still owns nodes now lists the nodes (ID and hostname) that must be deleted first [#3475](https://github.com/juanfont/headscale/pull/3475) +- Fix deleted nodes, and peers hidden by a policy change, staying listed in the Tailscale Android app; removed peers are now sent as their own incremental map update [#3492](https://github.com/juanfont/headscale/pull/3492) - Headscale now requires Go 1.27 to build - `headscale preauthkeys create --user` accepts a user name as well as an ID - `derp.paths` files may be Tailscale JSON or HuJSON DERP maps as well as YAML diff --git a/hscontrol/capver/capver.go b/hscontrol/capver/capver.go index a4c535e88..7b19fc325 100644 --- a/hscontrol/capver/capver.go +++ b/hscontrol/capver/capver.go @@ -15,14 +15,22 @@ import ( // minVersionParts is the minimum number of version parts needed for major.minor. const minVersionParts = 2 +// fullNetmapRemovalsCapVer is the capability version of Tailscale main when +// clients started reporting peers a full netmap drops as removed +// (tailscale/tailscale#15660); v1.104 is the first release at or above it. +const fullNetmapRemovalsCapVer tailcfg.CapabilityVersion = 148 + // CanOldCodeBeCleanedUp is called at server startup to panic when // [MinSupportedCapabilityVersion] has crossed a threshold at which a // backwards-compat emit path can be deleted. Each entry pairs a // [tailcfg.CapabilityVersion] threshold with the message identifying -// the code to remove; today there are none. +// the code to remove. // // All capability-version-gated cleanups should be registered here. func CanOldCodeBeCleanedUp() { + if MinSupportedCapabilityVersion >= fullNetmapRemovalsCapVer { + panic("the tailscale/tailscale#15660 compat can be cleaned up: grep for it (hscontrol/mapper/batcher.go and its tests)") + } } func tailscaleVersSorted() []string { diff --git a/hscontrol/mapper/batcher.go b/hscontrol/mapper/batcher.go index ddeb8597a..afe39a673 100644 --- a/hscontrol/mapper/batcher.go +++ b/hscontrol/mapper/batcher.go @@ -78,13 +78,18 @@ type nodeConnection interface { updateSentPeers(resp *tailcfg.MapResponse) } -// generateMapResponse generates a [tailcfg.MapResponse] for the given [types.NodeID] based on the provided [change.Change]. -func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*tailcfg.MapResponse, error) { +// generateMapResponse generates the [tailcfg.MapResponse] values to send, in +// order, for the given [types.NodeID] based on the provided [change.Change]. +func generateMapResponse( + nc nodeConnection, + mapper *mapper, + r change.Change, +) ([]*tailcfg.MapResponse, error) { nodeID := nc.nodeID() version := nc.version() if r.IsEmpty() { - return nil, nil //nolint:nilnil // Empty response means nothing to send + return nil, nil } if nodeID == 0 { @@ -97,7 +102,7 @@ func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*t // Handle self-only responses if r.IsSelfOnly() && r.TargetNode != nodeID { - return nil, nil //nolint:nilnil // No response needed for other nodes when self-only + return nil, nil } // Check if this is a self-update (the changed node is the receiving node). @@ -106,7 +111,8 @@ func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*t isSelfUpdate := r.OriginNode != 0 && r.OriginNode == nodeID var ( - mapResp *tailcfg.MapResponse + resp *tailcfg.MapResponse + removed []tailcfg.NodeID err error ) @@ -122,51 +128,60 @@ func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*t currentPeerIDs = append(currentPeerIDs, peer.ID().NodeID()) } - removedPeers := nc.computePeerDiff(currentPeerIDs) + removed = nc.computePeerDiff(currentPeerIDs) // 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) + resp, err = mapper.policyChangeResponse(nodeID, version, currentPeers, isSelfUpdate) } else if isSelfUpdate { // Non-policy self-update: just send the self node info - mapResp, err = mapper.selfMapResponse(nodeID, version) + resp, err = mapper.selfMapResponse(nodeID, version) } else { - mapResp, err = mapper.buildFromChange(nodeID, version, &r) + resp, err = mapper.buildFromChange(nodeID, version, &r) } if err != nil { return nil, fmt.Errorf("generating map response for nodeID %d: %w", nodeID, err) } - // When a full update (SendAllPeers=true) produces zero visible peers - // (e.g., a restrictive policy isolates this node), the resulting - // [tailcfg.MapResponse] has Peers: []*tailcfg.Node{} (empty non-nil slice). - // - // The Tailscale client only treats Peers as a full authoritative - // replacement when len(Peers) > 0 (controlclient/map.go:462). - // An empty Peers slice is indistinguishable from a delta response, - // so the client silently preserves its existing peer state. - // - // This matters when a [change.FullUpdate] replaces a pending - // [change.PolicyChange] in the batcher ([Batcher.addToBatch] - // short-circuits on [change.HasFull]). The [change.PolicyChange] - // would have computed PeersRemoved via - // [multiChannelNodeConn.computePeerDiff], but the [change.FullUpdate] - // path uses [MapResponseBuilder.WithPeers] which sets Peers: []. - // - // Fix: when a full update results in zero peers, compute the diff - // against lastSentPeers and add explicit PeersRemoved entries so - // the client correctly clears its stale peer state. - if mapResp != nil && r.SendAllPeers && len(mapResp.Peers) == 0 { - removedPeers := nc.computePeerDiff(nil) - if len(removedPeers) > 0 { - mapResp.PeersRemoved = removedPeers - } + if resp == nil { + return nil, nil } - return mapResp, nil + // A full peer list replaces the node's peer set, so peers missing from + // it are removals. Clients take Peers as authoritative only when it is + // non-empty, so a full update that isolates a node (e.g. + // [Batcher.addToBatch] replacing a pending [change.PolicyChange] with a + // [change.FullUpdate]) needs them sent explicitly. + if resp.Peers != nil { + peerIDs := make([]tailcfg.NodeID, 0, len(resp.Peers)) + for _, peer := range resp.Peers { + peerIDs = append(peerIDs, peer.ID) + } + + removed = nc.computePeerDiff(peerIDs) + } + + return withRemovals(resp, removed), nil } -// handleNodeChange generates and sends a [tailcfg.MapResponse] for a given node and [change.Change]. +// withRemovals returns resp and the peers removed from the node's view as the +// responses to send. The removals lead, in a response of their own: clients +// before Tailscale v1.104 only report removed peers to IPN bus watchers opted +// out of full netmaps (the Android app since 1.100) from responses they apply +// as deltas, and resp can carry DNSConfig, SSHPolicy, Node or Peers, which +// force a full rebuild. +// +// TODO(kradalby): when the tailscale/tailscale#15660 compat goes (see +// capver.CanOldCodeBeCleanedUp), set resp.PeersRemoved and return resp alone. +func withRemovals(resp *tailcfg.MapResponse, removed []tailcfg.NodeID) []*tailcfg.MapResponse { + if len(removed) == 0 { + return []*tailcfg.MapResponse{resp} + } + + return []*tailcfg.MapResponse{{PeersRemoved: removed}, resp} +} + +// handleNodeChange generates and sends the [tailcfg.MapResponse] values for a given node and [change.Change]. func handleNodeChange(nc nodeConnection, mapper *mapper, r change.Change) error { if nc == nil { return ErrNodeConnectionNil @@ -176,35 +191,31 @@ func handleNodeChange(nc nodeConnection, mapper *mapper, r change.Change) error log.Debug().Caller().Uint64(zf.NodeID, nodeID.Uint64()).Str(zf.Reason, r.Reason).Msg("node change processing started") - data, err := generateMapResponse(nc, mapper, r) + resps, err := generateMapResponse(nc, mapper, r) if err != nil { return fmt.Errorf("generating map response for node %d: %w", nodeID, err) } - if data == nil { - // No data to send is valid for some response types - return nil - } + for _, resp := range resps { + err = nc.send(resp) + if err != nil { + // If the node has no active connections, the data was not + // delivered. Do not update lastSentPeers — recording phantom + // peer state would corrupt future computePeerDiff calculations, + // causing the node to miss peer additions or removals after + // reconnection. + if errors.Is(err, errNoActiveConnections) { + return nil + } - // Send the map response - err = nc.send(data) - if err != nil { - // If the node has no active connections, the data was not - // delivered. Do not update lastSentPeers — recording phantom - // peer state would corrupt future computePeerDiff calculations, - // causing the node to miss peer additions or removals after - // reconnection. - if errors.Is(err, errNoActiveConnections) { - return nil + return fmt.Errorf("sending map response to node %d: %w", nodeID, err) } - return fmt.Errorf("sending map response to node %d: %w", nodeID, err) + // Update peer tracking only after confirmed delivery to at + // least one active connection. + nc.updateSentPeers(resp) } - // Update peer tracking only after confirmed delivery to at - // least one active connection. - nc.updateSentPeers(data) - return nil } @@ -337,6 +348,29 @@ func (b *Batcher) AddNode( // path, and under workMu so a concurrent async bundle for this node // cannot interleave its own lastSentPeers update. nodeConn.workMu.Lock() + + // Peers removed while the node had no stream: the initial map is a + // full rebuild, so follow it with the removal as a delta, see + // [withRemovals]. + // + // TODO(kradalby): delete with the tailscale/tailscale#15660 compat, see + // capver.CanOldCodeBeCleanedUp. + peerIDs := make([]tailcfg.NodeID, 0, len(initialMap.Peers)) + for _, peer := range initialMap.Peers { + peerIDs = append(peerIDs, peer.ID) + } + + removed := make([]types.NodeID, 0) + for _, peerID := range nodeConn.computePeerDiff(peerIDs) { + removed = append(removed, types.NodeID(peerID)) //nolint:gosec // NodeID types are equivalent + } + + if len(removed) > 0 { + rm := change.PeersRemoved(removed...) + rm.TargetNode = id + b.AddWork(rm) + } + nodeConn.updateSentPeers(initialMap) nodeConn.workMu.Unlock() @@ -506,9 +540,12 @@ func (b *Batcher) worker(workerID int) { // node waits until the initial map is sent. nc.workMu.Lock() - var err error - - result.mapResponse, err = generateMapResponse(nc, b.mapper, w.changes[0]) + resps, err := generateMapResponse(nc, b.mapper, w.changes[0]) + if len(resps) > 0 { + // The initial map must be the stream's first frame; AddNode + // sends any removals ahead of it after it instead. + result.mapResponse = resps[len(resps)-1] + } result.err = err if result.err != nil { diff --git a/hscontrol/mapper/batcher_test.go b/hscontrol/mapper/batcher_test.go index c16e147ce..8a6cb3b51 100644 --- a/hscontrol/mapper/batcher_test.go +++ b/hscontrol/mapper/batcher_test.go @@ -19,6 +19,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "tailscale.com/tailcfg" + "tailscale.com/types/netmap" ) var errNodeNotFoundAfterAdd = errors.New("node not found after adding to batcher") @@ -2358,3 +2359,91 @@ func TestAddWorkPeersRemovedNotTreatedAsEmpty(t *testing.T) { require.Equal(t, 3, countNodesPending(lb.b), "surviving 3 nodes must have the removal pending") } + +// TestHandleNodeChangeSendsRemovalsAsDelta checks that peers missing from a +// response listing the node's complete peer set go out first as a response +// of their own that clients can apply as a delta, and that the full-set +// response never carries them. Clients only report removals to IPN bus +// watchers opted out of full netmaps from delta responses. +// +// TODO(kradalby): with the tailscale/tailscale#15660 compat gone, removals +// ride the full-set response; adapt this and TestHandleNodeChangeRetryAfterRemoval. +func TestHandleNodeChangeSendsRemovalsAsDelta(t *testing.T) { + tests := []struct { + name string + nodes int + ch change.Change + }{ + {"policy_change", 2, change.PolicyChange()}, + {"full_update", 2, change.FullUpdate()}, + // Clients ignore an empty Peers list, so an isolating full update + // relies on the removal entirely. + {"full_update_isolated", 1, change.FullUpdate()}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testData, cleanup := setupBatcherWithTestData(t, NewBatcherAndMapper, 1, tt.nodes, normalBufferSize) + defer cleanup() + + gone := tailcfg.NodeID(999) + + mc := newMockNodeConnection(testData.Nodes[0].n.ID) + mc.peers.Store(gone, struct{}{}) + + for i := range testData.Nodes[1:] { + mc.peers.Store(testData.Nodes[i+1].n.ID.NodeID(), struct{}{}) + } + + require.NoError(t, handleNodeChange(mc, testData.Batcher.mapper, tt.ch)) + + sent := mc.getSent() + require.Len(t, sent, 2) + + assert.Equal(t, []tailcfg.NodeID{gone}, sent[0].PeersRemoved) + _, ok := netmap.MutationsFromMapResponse(sent[0], time.Time{}) + assert.True(t, ok, "removal must be applicable as a delta: %+v", sent[0]) + + assert.Empty(t, sent[1].PeersRemoved, "full-set response must not carry removals") + + _, tracked := mc.peers.Load(gone) + assert.False(t, tracked, "removed peer must leave lastSentPeers") + }) + } +} + +// TestHandleNodeChangeRetryAfterRemoval checks that a change retried after +// its removal was delivered but its content was not does not repeat the +// removal. +func TestHandleNodeChangeRetryAfterRemoval(t *testing.T) { + testData, cleanup := setupBatcherWithTestData(t, NewBatcherAndMapper, 1, 2, normalBufferSize) + defer cleanup() + + gone := tailcfg.NodeID(999) + + mc := newMockNodeConnection(testData.Nodes[0].n.ID) + mc.peers.Store(gone, struct{}{}) + + var sends int + + mc.sendFn = func(resp *tailcfg.MapResponse) error { + sends++ + if sends == 2 { + return errNoReadyConnections + } + + mc.sent = append(mc.sent, resp) + + return nil + } + + err := handleNodeChange(mc, testData.Batcher.mapper, change.PolicyChange()) + require.ErrorIs(t, err, errNoReadyConnections) + require.NoError(t, handleNodeChange(mc, testData.Batcher.mapper, change.PolicyChange())) + + sent := mc.getSent() + require.Len(t, sent, 2, "removal once, then the retried content") + assert.Equal(t, []tailcfg.NodeID{gone}, sent[0].PeersRemoved) + assert.Empty(t, sent[1].PeersRemoved) + assert.NotEmpty(t, sent[1].PacketFilters) +} diff --git a/hscontrol/mapper/mapper.go b/hscontrol/mapper/mapper.go index f64464240..b136681ae 100644 --- a/hscontrol/mapper/mapper.go +++ b/hscontrol/mapper/mapper.go @@ -304,8 +304,8 @@ func (m *mapper) selfMapResponse( } // policyChangeResponse creates a [tailcfg.MapResponse] for policy changes. -// It sends: -// - PeersRemoved for peers that are no longer visible after the policy change +// Peers no longer visible after the change are sent separately, see +// [handleNodeChange]. It sends: // - PeersChanged for remaining peers (their AllowedIPs may have changed due to policy) // - Updated PacketFilters // - Updated SSHPolicy (SSH rules may reference users/groups that changed) @@ -325,7 +325,6 @@ func (m *mapper) selfMapResponse( func (m *mapper) policyChangeResponse( nodeID types.NodeID, capVer tailcfg.CapabilityVersion, - removedPeers []tailcfg.NodeID, currentPeers views.Slice[types.NodeView], includeSelf bool, ) (*tailcfg.MapResponse, error) { @@ -340,16 +339,6 @@ func (m *mapper) policyChangeResponse( builder = builder.WithSelfNode() } - if len(removedPeers) > 0 { - // Convert [tailcfg.NodeID] to [types.NodeID] for [MapResponseBuilder.WithPeersRemoved] - removedIDs := make([]types.NodeID, len(removedPeers)) - for i, id := range removedPeers { - removedIDs[i] = types.NodeID(id) //nolint:gosec // NodeID types are equivalent - } - - builder.WithPeersRemoved(removedIDs...) - } - // Send remaining peers in PeersChanged - their AllowedIPs may have // changed due to the policy update (e.g., different routes allowed). // Cross-user peers must also carry their user profile, otherwise the diff --git a/hscontrol/mapper/mapper_test.go b/hscontrol/mapper/mapper_test.go index 9ab4125bd..7f85cc6ee 100644 --- a/hscontrol/mapper/mapper_test.go +++ b/hscontrol/mapper/mapper_test.go @@ -872,11 +872,13 @@ func TestBackfillNodeIPsReachesBackfilledNode(t *testing.T) { var self *tailcfg.Node for _, ch := range change.FilterForNode(target, cs) { - resp, err := generateMapResponse(nc, m, ch) + resps, err := generateMapResponse(nc, m, ch) require.NoError(t, err) - if resp != nil && resp.Node != nil { - self = resp.Node + for _, resp := range resps { + if resp.Node != nil { + self = resp.Node + } } } @@ -963,16 +965,14 @@ func TestFailedExpiryOfPrimaryAnnouncesBackup(t *testing.T) { var backupRoutes []netip.Prefix for _, ch := range change.FilterForNode(client, []change.Change{c}) { - resp, err := generateMapResponse(nc, m, ch) + resps, err := generateMapResponse(nc, m, ch) require.NoError(t, err) - if resp == nil { - continue - } - - for _, p := range append(resp.Peers, resp.PeersChanged...) { - if p.ID == backup.NodeID() { - backupRoutes = p.PrimaryRoutes + for _, resp := range resps { + for _, p := range append(resp.Peers, resp.PeersChanged...) { + if p.ID == backup.NodeID() { + backupRoutes = p.PrimaryRoutes + } } } } diff --git a/hscontrol/servertest/issues_test.go b/hscontrol/servertest/issues_test.go index 7b0a10901..d505a4753 100644 --- a/hscontrol/servertest/issues_test.go +++ b/hscontrol/servertest/issues_test.go @@ -1000,6 +1000,9 @@ func TestIssuesIdentity(t *testing.T) { // SSHPolicy, the removal forces a full netmap rebuild, which never tells IPN // bus watchers opted out of full netmaps (the Android app since 1.100) that // the peer is gone, so they keep showing it. +// +// TODO(kradalby): with the tailscale/tailscale#15660 compat gone, removals +// ride full rebuilds; assert they reach the netmap instead. func TestPeerRemovedAsDelta(t *testing.T) { t.Parallel()