From 5228cb1a408e29b67d72720894c80ac489f11daa Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 3 Jun 2026 13:55:42 +0000 Subject: [PATCH] change: drop subnet-router full update, use policy change Don't send a full update when a subnet router goes up or down; the gated policy change already recomputes peers and is a smaller payload. Updates #3293 --- hscontrol/state/connect_test.go | 48 +++++++++++++++++++++++++++ hscontrol/types/change/change.go | 26 +++++---------- hscontrol/types/change/change_test.go | 41 +++++++++++++++++++++++ 3 files changed, 97 insertions(+), 18 deletions(-) diff --git a/hscontrol/state/connect_test.go b/hscontrol/state/connect_test.go index b1ded3ab..3a60d1b3 100644 --- a/hscontrol/state/connect_test.go +++ b/hscontrol/state/connect_test.go @@ -166,3 +166,51 @@ func TestConnectDisconnectSubnetRouterForcesRecompute(t *testing.T) { "subnet router disconnect must force a peer recompute") }) } + +// TestConnectDisconnectSubnetRouterEmitsPolicyChangeNotFull pins how a subnet +// router forces that recompute: through the gated change.PolicyChange() (a +// runtime peer recompute) and the lightweight online/offline peer patch, not a +// full update. policyChangeResponse is a strict subset of a full update yet +// still carries primary-route failover, so the heavier FullUpdate that the +// online/offline change once emitted for subnet routers is unnecessary. +func TestConnectDisconnectSubnetRouterEmitsPolicyChangeNotFull(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + route := netip.MustParsePrefix("10.0.0.0/24") + _, ok := s.nodeStore.UpdateNode(nodeID, func(n *types.Node) { + n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{route}} + n.ApprovedRoutes = []netip.Prefix{route} + }) + require.True(t, ok) + + assertRecomputeNotFull := func(t *testing.T, cs []change.Change) { + t.Helper() + + assert.NotEmpty(t, runtimePeerComputationReasons(cs), + "subnet router must still drive a runtime peer recompute") + assert.True(t, hasPeerPatch(cs), + "subnet router should still emit the lightweight online/offline patch") + + for _, c := range cs { + assert.Falsef(t, c.IsFull(), + "subnet router recompute must be a PolicyChange, not a full update: %q", c.Reason) + } + } + + t.Run("connect", func(t *testing.T) { + cs, epoch := s.Connect(nodeID) + require.NotZero(t, epoch) + + assertRecomputeNotFull(t, cs) + }) + + t.Run("disconnect", func(t *testing.T) { + _, epoch := s.Connect(nodeID) + + cs, err := s.Disconnect(nodeID, epoch) + require.NoError(t, err) + + assertRecomputeNotFull(t, cs) + }) +} diff --git a/hscontrol/types/change/change.go b/hscontrol/types/change/change.go index 5b3c933f..77d0f083 100644 --- a/hscontrol/types/change/change.go +++ b/hscontrol/types/change/change.go @@ -456,29 +456,19 @@ func NodeRemoved(id types.NodeID) Change { return PeersRemoved(id) } -// NodeOnlineFor returns a [Change] for when a node comes online. -// If the node is a subnet router, a full update is sent instead of a patch. +// NodeOnlineFor returns the [Change] for a node coming online: a lightweight +// [NodeOnline] peer patch. Subnet routers, relay targets, and via targets get +// their full peer recompute from the gated [PolicyChange] that State.Connect +// emits, so no full update is needed here. func NodeOnlineFor(node types.NodeView) Change { - if node.IsSubnetRouter() { - c := FullUpdate() - c.Reason = "subnet router online" - - return c - } - return NodeOnline(node.ID()) } -// NodeOfflineFor returns a [Change] for when a node goes offline. -// If the node is a subnet router, a full update is sent instead of a patch. +// NodeOfflineFor returns the [Change] for a node going offline: a lightweight +// [NodeOffline] peer patch. As with [NodeOnlineFor], subnet routers and other +// recompute-forcing nodes rely on the gated [PolicyChange] from State.Disconnect +// for the peer recompute, so no full update is needed here. func NodeOfflineFor(node types.NodeView) Change { - if node.IsSubnetRouter() { - c := FullUpdate() - c.Reason = "subnet router offline" - - return c - } - return NodeOffline(node.ID()) } diff --git a/hscontrol/types/change/change_test.go b/hscontrol/types/change/change_test.go index c5086267..14fe077a 100644 --- a/hscontrol/types/change/change_test.go +++ b/hscontrol/types/change/change_test.go @@ -1,11 +1,13 @@ package change import ( + "net/netip" "reflect" "testing" "github.com/juanfont/headscale/hscontrol/types" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "tailscale.com/tailcfg" ) @@ -612,3 +614,42 @@ func TestUniqueNodeIDs(t *testing.T) { }) } } + +func TestNodeOnlineOfflineForSubnetRouter(t *testing.T) { + route := netip.MustParsePrefix("10.0.0.0/24") + router := types.Node{ + ID: 1, + Hostinfo: &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{route}}, + ApprovedRoutes: []netip.Prefix{route}, + } + view := router.View() + require.True(t, view.IsSubnetRouter(), "test node must be a subnet router") + + tests := []struct { + name string + got Change + wantOnline bool + }{ + {name: "online", got: NodeOnlineFor(view), wantOnline: true}, + {name: "offline", got: NodeOfflineFor(view), wantOnline: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // A subnet router's online/offline transition rides the lightweight + // peer patch, not a full update: the gated PolicyChange that + // State.Connect/Disconnect emit owns the netmap recompute. + assert.False(t, tt.got.IsFull(), + "subnet router online/offline must be a peer patch, not a full update") + + require.NotEmpty(t, tt.got.PeerPatches, + "expected an online/offline peer patch") + + patch := tt.got.PeerPatches[0] + assert.Equal(t, view.ID().NodeID(), patch.NodeID) + + require.NotNil(t, patch.Online) + assert.Equal(t, tt.wantOnline, *patch.Online) + }) + } +}