From f46ea3d1df4e2cc6f6938dfb3a93a39581fcdfc5 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 21 Oct 2026 11:15:28 +0000 Subject: [PATCH] mapper: skip responses left empty by visibility filtering A peer change hidden from a recipient still produced a frame carrying only ControlTime, one per sender per recipient. Drop it once the response is final. --- hscontrol/mapper/batcher.go | 44 +++++++++++--- hscontrol/mapper/batcher_bench_test.go | 20 +++++++ hscontrol/mapper/batcher_test.go | 79 +++++++++++++++++++++++++- hscontrol/mapper/batcher_unit_test.go | 28 +++++++++ hscontrol/mapper/builder.go | 5 +- hscontrol/mapper/mapper.go | 10 +++- hscontrol/mapper/mapper_test.go | 5 ++ 7 files changed, 175 insertions(+), 16 deletions(-) diff --git a/hscontrol/mapper/batcher.go b/hscontrol/mapper/batcher.go index f8efdc21d..0c19f0941 100644 --- a/hscontrol/mapper/batcher.go +++ b/hscontrol/mapper/batcher.go @@ -3,6 +3,7 @@ package mapper import ( "errors" "fmt" + "reflect" "slices" "sync" "sync/atomic" @@ -159,24 +160,49 @@ func generateMapResponse( removed = nc.computePeerDiff(peerIDs) } + // Visibility filtering can leave a broadcast with nothing for this + // recipient; the frame would only cost encode, write and client decode. + if emptyMapResponse(resp) { + resp = nil + } + return withRemovals(resp, removed), nil } -// 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. +// emptyMapResponse reports whether resp carries nothing but ControlTime, +// which every built response has and clients only sample for clock skew. +// Any other non-zero field counts, an empty slice or map included, and so +// does a field headscale does not set today: unknown is sent, not dropped. +// Per response on the hot path, so it must stay allocation-free; see +// [BenchmarkEmptyMapResponse]. +func emptyMapResponse(resp *tailcfg.MapResponse) bool { + stripped := *resp + stripped.ControlTime = nil + + return reflect.ValueOf(stripped).IsZero() +} + +// withRemovals returns resp, if any, 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} + var resps []*tailcfg.MapResponse + + if len(removed) > 0 { + resps = append(resps, &tailcfg.MapResponse{PeersRemoved: removed}) } - return []*tailcfg.MapResponse{{PeersRemoved: removed}, resp} + if resp != nil { + resps = append(resps, resp) + } + + return resps } // handleNodeChange generates and sends the [tailcfg.MapResponse] values for a given node and [change.Change]. diff --git a/hscontrol/mapper/batcher_bench_test.go b/hscontrol/mapper/batcher_bench_test.go index b95fd5e63..98d9e4177 100644 --- a/hscontrol/mapper/batcher_bench_test.go +++ b/hscontrol/mapper/batcher_bench_test.go @@ -65,6 +65,26 @@ func BenchmarkMultiChannelSend(b *testing.B) { } } +// BenchmarkEmptyMapResponse pins that the per-response emptiness check +// does not allocate, on a frame it drops and on one it keeps. +func BenchmarkEmptyMapResponse(b *testing.B) { + for _, tt := range []struct { + name string + resp *tailcfg.MapResponse + }{ + {"empty", testMapResponse()}, + {"peers", testMapResponseWithPeers(1, 2, 3)}, + } { + b.Run(tt.name, func(b *testing.B) { + b.ReportAllocs() + + for range b.N { + _ = emptyMapResponse(tt.resp) + } + }) + } +} + // BenchmarkComputePeerDiff measures the cost of computing peer diffs at scale. func BenchmarkComputePeerDiff(b *testing.B) { for _, peerCount := range []int{10, 100, 1000} { diff --git a/hscontrol/mapper/batcher_test.go b/hscontrol/mapper/batcher_test.go index 3145e05f8..41b2af080 100644 --- a/hscontrol/mapper/batcher_test.go +++ b/hscontrol/mapper/batcher_test.go @@ -1071,8 +1071,10 @@ func TestBatcherWorkQueueBatching(t *testing.T) { t.Logf("Update %d: nil update", updateCount) } case <-timeout: - // Expected: 5 explicit changes + 1 initial from AddNode + 1 NodeOnline from wrapper = 7 updates - expectedUpdates := 7 + // Expected: 5 explicit changes + 1 initial from AddNode = 6 updates. + // The wrapper's NodeOnline is about this node itself, which is + // not among its peers, so it leaves nothing to send. + expectedUpdates := 6 t.Logf("Received %d updates from %d changes (expected %d)", updateCount, 5, expectedUpdates) @@ -2976,3 +2978,76 @@ func TestSSHPolicySentOnlyWhenChanged(t *testing.T) { assert.Nil(t, frame.SSHPolicy, "connection %d already holds the policy", i) } } + +// TestInvisiblePeerChangeSendsNoFrame pins that a broadcast whose every +// changed peer is hidden from the recipient produces no frame for it, while +// the same change still reaches a recipient that sees the peer, and content +// that does not depend on seeing the peer still reaches everyone. +func TestInvisiblePeerChangeSendsNoFrame(t *testing.T) { + testData, cleanup := setupBatcherWithTestData(t, NewBatcherAndMapper, 2, 2, normalBufferSize) + defer cleanup() + + b := testData.Batcher.Batcher + + _, err := testData.State.SetPolicy([]byte( + `{"acls":[{"action":"accept","src":["autogroup:member"],"dst":["autogroup:self:*"]}]}`, + )) + require.NoError(t, err) + + // sender and sibling share a user; other belongs to the second user, so + // it cannot see either. + sender, sibling, other := &testData.Nodes[0], &testData.Nodes[1], &testData.Nodes[2] + + for _, n := range []*node{sender, sibling, other} { + require.NoError(t, b.AddNode(n.n.ID, n.ch, tailcfg.CapabilityVersion(100), nil)) + require.NotNil(t, expectReceive(t, n.ch, "initial map").Node) + } + + deliver := func(to *node, c change.Change) { + t.Helper() + + nc, ok := b.nodes.Load(to.n.ID) + require.True(t, ok) + + nc.workMu.Lock() + defer nc.workMu.Unlock() + + require.NoError(t, handleNodeChange(nc, b.mapper, c)) + } + + peerChanges := []struct { + name string + c change.Change + carries func(*tailcfg.MapResponse) bool + }{ + {"changed", change.NodeAdded(sender.n.ID), func(r *tailcfg.MapResponse) bool { + return len(r.PeersChanged) == 1 && r.PeersChanged[0].ID == sender.n.ID.NodeID() + }}, + {"patched", change.NodeOnline(sender.n.ID), func(r *tailcfg.MapResponse) bool { + return len(r.PeersChangedPatch) == 1 && r.PeersChangedPatch[0].NodeID == sender.n.ID.NodeID() + }}, + } + + for _, tt := range peerChanges { + deliver(other, tt.c) + expectNoReceive(t, other.ch, 20*time.Millisecond, tt.name+": other cannot see sender") + + deliver(sibling, tt.c) + assert.True(t, tt.carries(expectReceive(t, sibling.ch, tt.name+": sibling sees sender")), + "%s: sibling must get the peer", tt.name) + } + + deliver(other, change.PeersRemoved(sender.n.ID)) + assert.NotEmpty(t, expectReceive(t, other.ch, "removal").PeersRemoved) + + deliver(other, change.PolicyChange()) + assert.NotNil(t, expectReceive(t, other.ch, "policy").PacketFilters) + + _, _, err = testData.State.RenameNode(other.n.ID, "renamed") + require.NoError(t, err) + deliver(other, change.NodeAdded(other.n.ID)) + assert.NotNil(t, expectReceive(t, other.ch, "self").Node) + + deliver(other, change.PingNode(other.n.ID, &tailcfg.PingRequest{URL: "https://ping.test"})) + assert.NotNil(t, expectReceive(t, other.ch, "ping").PingRequest) +} diff --git a/hscontrol/mapper/batcher_unit_test.go b/hscontrol/mapper/batcher_unit_test.go index 7995e346b..991ab6780 100644 --- a/hscontrol/mapper/batcher_unit_test.go +++ b/hscontrol/mapper/batcher_unit_test.go @@ -1277,3 +1277,31 @@ func TestMultiChannelSend_SSHPolicyPerConnection(t *testing.T) { assert.Nil(t, expectReceive(t, chA, "a").SSHPolicy, "a already holds this policy") assert.Same(t, data.SSHPolicy, expectReceive(t, chB, "b").SSHPolicy, "b holds an older policy") } + +func TestEmptyMapResponse(t *testing.T) { + now := time.Now() + + tests := []struct { + name string + resp tailcfg.MapResponse + want bool + }{ + {"zero", tailcfg.MapResponse{}, true}, + {"control_time_only", tailcfg.MapResponse{ControlTime: &now}, true}, + {"self", tailcfg.MapResponse{Node: &tailcfg.Node{}}, false}, + {"peers_changed", tailcfg.MapResponse{PeersChanged: []*tailcfg.Node{{}}}, false}, + {"empty_slice_counts", tailcfg.MapResponse{PeersChanged: []*tailcfg.Node{}}, false}, + {"peers_removed", tailcfg.MapResponse{PeersRemoved: ids(1)}, false}, + {"patch", tailcfg.MapResponse{PeersChangedPatch: []*tailcfg.PeerChange{{}}}, false}, + {"packet_filters", tailcfg.MapResponse{PacketFilters: map[string][]tailcfg.FilterRule{"base": nil}}, false}, + {"ping", tailcfg.MapResponse{PingRequest: &tailcfg.PingRequest{}}, false}, + {"keepalive", tailcfg.MapResponse{KeepAlive: true}, false}, + {"debug", tailcfg.MapResponse{Debug: &tailcfg.Debug{}}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, emptyMapResponse(&tt.resp)) + }) + } +} diff --git a/hscontrol/mapper/builder.go b/hscontrol/mapper/builder.go index 2f3e14b23..5ea2f5a45 100644 --- a/hscontrol/mapper/builder.go +++ b/hscontrol/mapper/builder.go @@ -345,8 +345,9 @@ func (b *MapResponseBuilder) Build() (*tailcfg.MapResponse, error) { return nil, multierr.New(b.errs...) } - // Dumps show the generated response; an unchanged SSHPolicy is - // dropped later, per connection, before it reaches the wire. + // Dumps show the generated response; [generateMapResponse] drops it + // if it is left empty, and an unchanged SSHPolicy is dropped later, + // per connection, before it reaches the wire. if debugDumpMapResponsePath != "" { writeDebugMapResponse(b.resp, b.debugType, b.nodeID) } diff --git a/hscontrol/mapper/mapper.go b/hscontrol/mapper/mapper.go index 76c8c770f..c3fb74faa 100644 --- a/hscontrol/mapper/mapper.go +++ b/hscontrol/mapper/mapper.go @@ -401,9 +401,13 @@ func (m *mapper) buildFromChange( builder.WithPeers(peers) } else { if len(resp.PeersChanged) > 0 { - peers := m.state.ListPeers(nodeID, resp.PeersChanged...) - builder.WithUserProfiles(peers) - builder.WithPeerChanges(peers) + // Visibility may leave none of the changed peers for this + // recipient; the profiles would then carry only its own + // user, which it already holds. + if peers := m.state.ListPeers(nodeID, resp.PeersChanged...); peers.Len() > 0 { + builder.WithUserProfiles(peers) + builder.WithPeerChanges(peers) + } } if len(resp.PeersRemoved) > 0 { diff --git a/hscontrol/mapper/mapper_test.go b/hscontrol/mapper/mapper_test.go index 60e1548a1..12f6fd140 100644 --- a/hscontrol/mapper/mapper_test.go +++ b/hscontrol/mapper/mapper_test.go @@ -1017,6 +1017,9 @@ func TestGenerateMapResponseDebugOnlyOnFullMaps(t *testing.T) { Regions: map[tailcfg.DERPRegionID]*tailcfg.DERPRegion{999: {RegionID: 999}}, }, }, + // Without these the DERP map and DNS config renders carry + // nothing and no frame is generated at all. + TailcfgDNSConfig: &tailcfg.DNSConfig{Proxied: true}, Tuning: types.Tuning{ NodeStoreBatchSize: state.TestBatchSize, NodeStoreBatchTimeout: state.TestBatchTimeout, @@ -1035,6 +1038,8 @@ func TestGenerateMapResponseDebugOnlyOnFullMaps(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { _ = s.Close() }) + s.SetDERPMap(cfg.DERP.DERPMap) + _, err = s.SetPolicy([]byte(`{"acls":[{"action":"accept","src":["*"],"dst":["*:*"]}]}`)) require.NoError(t, err)