diff --git a/hscontrol/state/connect_test.go b/hscontrol/state/connect_test.go new file mode 100644 index 00000000..b1ded3ab --- /dev/null +++ b/hscontrol/state/connect_test.go @@ -0,0 +1,168 @@ +package state + +import ( + "net/netip" + "testing" + + "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/types/change" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "tailscale.com/tailcfg" +) + +// runtimePeerComputationReasons returns the Reason of every change in cs that +// carries RequiresRuntimePeerComputation. Each such change makes the batcher +// rebuild a full netmap (packet filters, SSH policy, peer serialization) for +// every connected node, so it is the expensive fan-out the issue is about. +func runtimePeerComputationReasons(cs []change.Change) []string { + var reasons []string + + for _, c := range cs { + if c.RequiresRuntimePeerComputation { + reasons = append(reasons, c.Reason) + } + } + + return reasons +} + +// hasPeerPatch reports whether any change carries a lightweight PeerChange +// patch (e.g. the online/offline indicator). This is the cheap notification a +// reconnect should produce instead of a full runtime recompute. +func hasPeerPatch(cs []change.Change) bool { + for _, c := range cs { + if len(c.PeerPatches) > 0 { + return true + } + } + + return false +} + +// forcesPeerRecompute reports whether any change makes peers rebuild a full +// netmap, whether via a full update (subnet-router path) or a runtime peer +// computation (relay/via path). +func forcesPeerRecompute(cs []change.Change) bool { + for _, c := range cs { + if c.IsFull() || c.RequiresRuntimePeerComputation { + return true + } + } + + return false +} + +// TestConnectDisconnectOrdinaryNodeNoRuntimeRecompute asserts that an ordinary +// node coming online or going offline only sends the lightweight online/offline +// peer patch and does not trigger a runtime peer recompute. +// +// State.Connect and State.Disconnect gate change.PolicyChange() (which sets +// RequiresRuntimePeerComputation, forcing the batcher to rebuild a full netmap +// for every connected node) on NodeNeedsPeerRecompute. An ordinary node is +// neither a subnet router, a relay target, nor a via target, so the gate is +// false and no recompute is emitted. +// +// Emitting that recompute unconditionally turned each reconnect into O(N) full +// netmap rebuilds (and a reconnect storm into O(N^2)), which saturated CPU +// after the v0.28 -> v0.29 upgrade. +func TestConnectDisconnectOrdinaryNodeNoRuntimeRecompute(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + t.Run("connect", func(t *testing.T) { + cs, epoch := s.Connect(nodeID) + require.NotZero(t, epoch, "Connect should return a session epoch") + + assert.True(t, hasPeerPatch(cs), + "Connect should still emit a lightweight online peer patch") + + reasons := runtimePeerComputationReasons(cs) + assert.Empty(t, reasons, + "ordinary node connect must not trigger a runtime peer recompute; "+ + "got RequiresRuntimePeerComputation changes: %v", reasons) + }) + + t.Run("disconnect", func(t *testing.T) { + // Connect first so Disconnect's epoch check passes. + _, epoch := s.Connect(nodeID) + + cs, err := s.Disconnect(nodeID, epoch) + require.NoError(t, err) + + assert.True(t, hasPeerPatch(cs), + "Disconnect should still emit a lightweight offline peer patch") + + reasons := runtimePeerComputationReasons(cs) + assert.Empty(t, reasons, + "ordinary node disconnect must not trigger a runtime peer recompute; "+ + "got RequiresRuntimePeerComputation changes: %v", reasons) + }) +} + +// TestConnectDisconnectRelayTargetTriggersRecompute locks the cap/relay case: +// a relay target is not a subnet router, so the only thing that makes its +// connect/disconnect emit a runtime peer recompute is the +// NodeNeedsPeerRecompute gate. Peers must still receive that recompute so they +// drop a stale PeerRelay allocation when the relay goes offline. +func TestConnectDisconnectRelayTargetTriggersRecompute(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + // A cap/relay grant whose destination resolves to the node's owning + // user makes the node a relay target without making it a subnet router. + relayPolicy := `{"grants":[{"src":["*"],"dst":["persist-user@"],"app":{"tailscale.com/cap/relay":[{}]}}]}` + _, err := s.SetPolicy([]byte(relayPolicy)) + require.NoError(t, err) + + t.Run("connect", func(t *testing.T) { + cs, epoch := s.Connect(nodeID) + require.NotZero(t, epoch) + + assert.NotEmpty(t, runtimePeerComputationReasons(cs), + "relay-target node connect must trigger a runtime peer recompute") + }) + + t.Run("disconnect", func(t *testing.T) { + _, epoch := s.Connect(nodeID) + + cs, err := s.Disconnect(nodeID, epoch) + require.NoError(t, err) + + assert.NotEmpty(t, runtimePeerComputationReasons(cs), + "relay-target node disconnect must trigger a runtime peer recompute") + }) +} + +// TestConnectDisconnectSubnetRouterForcesRecompute guards that a subnet router +// still forces peers to recompute on connect/disconnect (primary-route +// failover changes their AllowedIPs), so the gate does not over-suppress. +func TestConnectDisconnectSubnetRouterForcesRecompute(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) + + t.Run("connect", func(t *testing.T) { + cs, epoch := s.Connect(nodeID) + require.NotZero(t, epoch) + + assert.True(t, forcesPeerRecompute(cs), + "subnet router connect must force a peer recompute") + }) + + t.Run("disconnect", func(t *testing.T) { + _, epoch := s.Connect(nodeID) + + cs, err := s.Disconnect(nodeID, epoch) + require.NoError(t, err) + + assert.True(t, forcesPeerRecompute(cs), + "subnet router disconnect must force a peer recompute") + }) +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index dce769b7..4daf18f3 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -594,9 +594,14 @@ func (s *State) Connect(id types.NodeID) ([]change.Change, uint64) { c = append(c, change.NodeAdded(id)) } - // Coming online may re-enable cap/relay grants and identity-based - // aliases targeting this node, so peers need a fresh netmap. - c = append(c, change.PolicyChange()) + // Only a node whose online state changes what peers compute (a subnet + // router, relay target, or via target) needs a full peer recompute. + // An ordinary node coming online just sends the lightweight online + // patch above; emitting a PolicyChange for it would force every peer + // to rebuild its netmap on every reconnect. + if s.polMan.NodeNeedsPeerRecompute(node) { + c = append(c, change.PolicyChange()) + } return c, epoch } @@ -648,13 +653,15 @@ func (s *State) Disconnect(id types.NodeID, epoch uint64) ([]change.Change, erro c = change.Change{} } - // Going offline can affect policy compilation beyond subnet routes - // (cap/relay grants, tag/group aliases, via routes), so peers need - // a fresh netmap regardless of whether the primary moved. - // - // TODO(kradalby): fires one full netmap recompute per peer on - // every connect/disconnect. Coalesce in mapper/batcher.go:addToBatch. - cs := []change.Change{change.NodeOfflineFor(node), c, change.PolicyChange()} + // Only a node whose online state changes what peers compute (a subnet + // router, relay target, or via target) needs a full peer recompute. + // An ordinary node going offline just sends the lightweight offline + // patch; emitting a PolicyChange for it would force every peer to + // rebuild its netmap on every disconnect. + cs := []change.Change{change.NodeOfflineFor(node), c} + if s.polMan.NodeNeedsPeerRecompute(node) { + cs = append(cs, change.PolicyChange()) + } return cs, nil }