From 59f3ff12a70e15fb351630ef1324d09cbdf5f335 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 9 Sep 2026 14:45:32 +0000 Subject: [PATCH] state: classify map requests before broadcasting Each request is reduced to the narrowest change it justifies, so a keepalive or endpoint bump no longer resends the whole node to peers. Updates #3417 --- hscontrol/state/endpoint_test.go | 17 + hscontrol/state/maprequest.go | 154 ++++- hscontrol/state/maprequest_noop_test.go | 74 -- hscontrol/state/maprequest_test.go | 875 ++++++++++++++++++++++++ hscontrol/state/state.go | 329 +++++---- hscontrol/types/node.go | 5 +- 6 files changed, 1239 insertions(+), 215 deletions(-) delete mode 100644 hscontrol/state/maprequest_noop_test.go diff --git a/hscontrol/state/endpoint_test.go b/hscontrol/state/endpoint_test.go index 21b83e5b..c0a4594e 100644 --- a/hscontrol/state/endpoint_test.go +++ b/hscontrol/state/endpoint_test.go @@ -187,6 +187,23 @@ func TestEndpointBroadcastWorthy(t *testing.T) { newType: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointSTUN, tailcfg.EndpointLocal}, want: true, }, + { + // Peers hold no endpoints for this node yet, and a + // suppressed delta is never resent, so the first set is + // the only one they would ever get. + name: "first endpoints ever, STUN only - broadcast", + stored: nil, + newEPs: []netip.AddrPort{stun}, + newType: []tailcfg.EndpointType{tailcfg.EndpointSTUN}, + want: true, + }, + { + name: "no stored and none announced - suppress", + stored: nil, + newEPs: nil, + newType: nil, + want: false, + }, } for _, tt := range tests { diff --git a/hscontrol/state/maprequest.go b/hscontrol/state/maprequest.go index b8f91be1..86895009 100644 --- a/hscontrol/state/maprequest.go +++ b/hscontrol/state/maprequest.go @@ -6,10 +6,78 @@ package state import ( "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/util/zlog/zf" + "github.com/rs/zerolog" "github.com/rs/zerolog/log" "tailscale.com/tailcfg" ) +// mapRequestDelta carries the classified facts extracted from one MapRequest +// against the currently-stored node. It separates the raw wire-level peer +// change (what the client actually sent) from the broadcast/persist/policy +// decisions made from it, so that each downstream decision (broadcast, persist, +// policy refresh, relation rebuild) sees only its own input. +// +// Construction happens once per MapRequest inside the NodeStore write callback +// (so comparisons run against serialized current state); classification happens +// after the write succeeds. Splitting these avoids redoing comparisons and +// avoids having the broadcast classifier depend on incidental branch order. +type mapRequestDelta struct { + // peerChange is the wire-level delta produced by + // [types.Node.PeerChangeFromMapRequest] - LastSeen is always stamped. + peerChange tailcfg.PeerChange + + // hostinfoChanged reports whether anything in Hostinfo changed apart + // from PreferredDERP (tracked by derpChanged) and DERP latency jitter. + // It gates storing and persisting the new Hostinfo. + hostinfoChanged bool + + // peerHostinfoChanged reports whether a Hostinfo field that peers read + // changed (see [peerHostinfo]). Only that forces a whole-node resend. + peerHostinfoChanged bool + + // routesChanged reports whether announced routes (RoutableIPs) + // changed. Routes are policy and election inputs, so they are tracked + // on their own. + routesChanged bool + + // derpChanged reports whether PreferredDERP changed. oldDERP/newDERP + // carry the values; DERP zero means "unchanged" on the wire, so + // clearing DERP to zero cannot be sent as a patch and forces a + // whole-peer update. + derpChanged bool + oldDERP, newDERP tailcfg.DERPRegionID + + // endpointBroadcast reports whether the endpoint delta is worth fanning + // out (i.e., a newly-added useful non-STUN endpoint). Storage still + // happens regardless; this gates only broadcast. + endpointBroadcast bool + + // keyChanged and discoKeyChanged report whether the node's wire keys + // changed. Key patches already carry the resulting endpoints/expiry, + // so a key change subsumes endpoint/DERP patches. + keyChanged bool + discoKeyChanged bool + + // persistWorthy reports whether the request carries data that should + // hit the database. LastSeen-only updates are not persist-worthy. + persistWorthy bool +} + +// MarshalZerologObject implements [zerolog.LogObjectMarshaler]. +func (d mapRequestDelta) MarshalZerologObject(e *zerolog.Event) { + e.Bool("hostinfo.changed", d.hostinfoChanged). + Bool("hostinfo.peer_changed", d.peerHostinfoChanged). + Bool("routes.changed", d.routesChanged). + Bool("derp.changed", d.derpChanged). + Int("derp.old", int(d.oldDERP)). + Int("derp.new", int(d.newDERP)). + Bool("endpoint.broadcast", d.endpointBroadcast). + Bool("key.changed", d.keyChanged). + Bool("disco_key.changed", d.discoKeyChanged). + Bool("persist", d.persistWorthy) +} + // netInfoFromMapRequest determines the correct [tailcfg.NetInfo] to use. // Returns the [tailcfg.NetInfo] that should be used for this request. func netInfoFromMapRequest( @@ -26,8 +94,8 @@ func netInfoFromMapRequest( if currentHostinfo != nil && currentHostinfo.NetInfo != nil { log.Debug(). Caller(). - Uint64("node.id", nodeID.Uint64()). - Int64("preferredDERP", currentHostinfo.NetInfo.PreferredDERP.Int64()). + Uint64(zf.NodeID, nodeID.Uint64()). + Int64(zf.DERP, currentHostinfo.NetInfo.PreferredDERP.Int64()). Msg("using NetInfo from previous Hostinfo in MapRequest") return currentHostinfo.NetInfo @@ -43,9 +111,87 @@ func netInfoFromMapRequest( log.Debug(). Caller(). - Uint64("node.id", nodeID.Uint64()). - Str("node.hostname", hostname). + Uint64(zf.NodeID, nodeID.Uint64()). + Str(zf.Hostname, hostname). Msg("node sent update but has no NetInfo in request or database") return nil } + +// hostinfoDERP returns the PreferredDERP value from a Hostinfo, or 0 when +// either pointer is nil. 0 is the wire "unchanged" sentinel. +func hostinfoDERP(hi *tailcfg.Hostinfo) tailcfg.DERPRegionID { + if hi == nil || hi.NetInfo == nil { + return 0 + } + + return hi.NetInfo.PreferredDERP +} + +// hostinfoEqual reports whether two Hostinfo values are the same apart from +// PreferredDERP, which the caller tracks on its own, and DERP latency +// jitter, which [tailcfg.NetInfo.BasicallyEqual] skips. +func hostinfoEqual(oldHI, newHI *tailcfg.Hostinfo) bool { + if oldHI == nil || newHI == nil { + return oldHI == newHI + } + + if !netInfoEqualIgnoringDERP(oldHI.NetInfo, newHI.NetInfo) { + return false + } + + // NetInfo is compared above; drop it so nil-vs-empty inside it does + // not leak through reflect.DeepEqual. + oldCopy := *oldHI + oldCopy.NetInfo = nil + newCopy := *newHI + newCopy.NetInfo = nil + + return oldCopy.Equal(&newCopy) +} + +// peerHostinfo keeps the Hostinfo fields another node's client reads from +// a peer: what tailscale status shows, PeerAPI services, SSH known hosts, +// exit-node location, app-connector eligibility, and the routes this server +// feeds into policy. Everything else is stored but never fanned out, and a +// peer's NetInfo is never read at all. +func peerHostinfo(hi *tailcfg.Hostinfo) *tailcfg.Hostinfo { + if hi == nil { + return nil + } + + return &tailcfg.Hostinfo{ + Hostname: hi.Hostname, + OS: hi.OS, + Services: hi.Services, + SSH_HostKeys: hi.SSH_HostKeys, + Location: hi.Location, + AppConnector: hi.AppConnector, + RoutableIPs: hi.RoutableIPs, + } +} + +// peerHostinfoEqual reports whether the fields peers read are unchanged. +func peerHostinfoEqual(oldHI, newHI *tailcfg.Hostinfo) bool { + return peerHostinfo(oldHI).Equal(peerHostinfo(newHI)) +} + +// netInfoEqualIgnoringDERP compares two NetInfo values via +// [tailcfg.NetInfo.BasicallyEqual] after zeroing PreferredDERP on both, so +// that DERP-only changes do not appear here (they are tracked separately). +func netInfoEqualIgnoringDERP(old, current *tailcfg.NetInfo) bool { + if old == nil && current == nil { + return true + } + + if (old == nil) != (current == nil) { + return false + } + + oldCopy := *old + oldCopy.PreferredDERP = 0 + currentCopy := *current + currentCopy.PreferredDERP = 0 + + return oldCopy.BasicallyEqual(¤tCopy) +} diff --git a/hscontrol/state/maprequest_noop_test.go b/hscontrol/state/maprequest_noop_test.go deleted file mode 100644 index ff2f8517..00000000 --- a/hscontrol/state/maprequest_noop_test.go +++ /dev/null @@ -1,74 +0,0 @@ -package state - -import ( - "strings" - "sync/atomic" - "testing" - - "github.com/stretchr/testify/require" - "gorm.io/gorm" - "tailscale.com/tailcfg" -) - -// TestNoOpMapRequestSkipsPersist ensures an identical, no-op MapRequest does -// not issue a database UPDATE (nor the O(n) policy SetNodes scan that follows -// persistNodeAndRefreshPolicy). The node state is unchanged, so persisting is pure waste on -// the hot map-request path. -func TestNoOpMapRequestSkipsPersist(t *testing.T) { - _, s, nodeID := persistTestSetup(t) - t.Cleanup(func() { _ = s.Close() }) - - var nodeUpdateCount atomic.Int64 - - gdb := s.DB().DB - cbName := "noop_count_node_updates" - err := gdb.Callback().Update().After("gorm:update").Register(cbName, func(tx *gorm.DB) { - if tx.Statement == nil { - return - } - - if tx.Statement.Table == "nodes" || - strings.Contains(strings.ToLower(tx.Statement.SQL.String()), "update \"nodes\"") { - nodeUpdateCount.Add(1) - } - }) - require.NoError(t, err) - t.Cleanup(func() { _ = gdb.Callback().Update().Remove(cbName) }) - - nv, ok := s.GetNodeByID(nodeID) - require.True(t, ok, "node should exist in NodeStore") - - stored := nv.AsStruct() - - req := tailcfg.MapRequest{ - NodeKey: stored.NodeKey, - DiscoKey: stored.DiscoKey, - Hostinfo: &tailcfg.Hostinfo{ - Hostname: stored.Hostname, - NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, - }, - } - - // First request establishes the Hostinfo/DERP state (expected to persist). - _, err = s.UpdateNodeFromMapRequest(nodeID, req) - require.NoError(t, err) - - nodeUpdateCount.Store(0) - - // Second request is value-identical: a no-op. - req2 := tailcfg.MapRequest{ - NodeKey: stored.NodeKey, - DiscoKey: stored.DiscoKey, - Hostinfo: &tailcfg.Hostinfo{ - Hostname: stored.Hostname, - NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, - }, - } - - _, err = s.UpdateNodeFromMapRequest(nodeID, req2) - require.NoError(t, err) - - require.Equalf(t, int64(0), nodeUpdateCount.Load(), - "no-op MapRequest should not issue any nodes-table UPDATE, got %d", - nodeUpdateCount.Load()) -} diff --git a/hscontrol/state/maprequest_test.go b/hscontrol/state/maprequest_test.go index d835afbc..e437448e 100644 --- a/hscontrol/state/maprequest_test.go +++ b/hscontrol/state/maprequest_test.go @@ -1,12 +1,21 @@ package state import ( + "net/netip" + "strings" + "sync" + "sync/atomic" "testing" + "time" "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/types/change" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gorm.io/gorm" "tailscale.com/tailcfg" + "tailscale.com/types/key" + "tailscale.com/types/opt" ) func TestNetInfoFromMapRequest(t *testing.T) { @@ -133,3 +142,869 @@ func TestNetInfoPreservationInRegistrationFlow(t *testing.T) { assert.Equal(t, tailcfg.DERPRegionID(7), result.PreferredDERP, "Should preserve DERP region from existing node") }) } + +// TestNoOpMapRequestSkipsPersist ensures an identical, no-op MapRequest does +// not issue a database UPDATE (nor the O(n) policy SetNodes scan that follows +// persistNodeAndRefreshPolicy). The node state is unchanged, so persisting is pure waste on +// the hot map-request path. +func TestNoOpMapRequestSkipsPersist(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + var nodeUpdateCount atomic.Int64 + + gdb := s.DB().DB + cbName := "noop_count_node_updates" + err := gdb.Callback().Update().After("gorm:update").Register(cbName, func(tx *gorm.DB) { + if tx.Statement == nil { + return + } + + if tx.Statement.Table == "nodes" || + strings.Contains(strings.ToLower(tx.Statement.SQL.String()), "update \"nodes\"") { + nodeUpdateCount.Add(1) + } + }) + require.NoError(t, err) + t.Cleanup(func() { _ = gdb.Callback().Update().Remove(cbName) }) + + nv, ok := s.GetNodeByID(nodeID) + require.True(t, ok, "node should exist in NodeStore") + + stored := nv.AsStruct() + + req := tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: stored.Hostname, + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + }, + } + + // First request establishes the Hostinfo/DERP state (expected to persist). + _, err = s.UpdateNodeFromMapRequest(nodeID, req) + require.NoError(t, err) + + nodeUpdateCount.Store(0) + + // Second request is value-identical: a no-op. + req2 := tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: stored.Hostname, + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + }, + } + + _, err = s.UpdateNodeFromMapRequest(nodeID, req2) + require.NoError(t, err) + + require.Equalf(t, int64(0), nodeUpdateCount.Load(), + "no-op MapRequest should not issue any nodes-table UPDATE, got %d", + nodeUpdateCount.Load()) +} + +// TestNoOpMapRequestEmitsNoPeerChange ensures an identical, no-op MapRequest +// does not emit a peer-visible change. +func TestNoOpMapRequestEmitsNoPeerChange(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + nv, ok := s.GetNodeByID(nodeID) + require.True(t, ok, "node should exist in NodeStore") + + stored := nv.AsStruct() + + req := func() tailcfg.MapRequest { + return tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: stored.Hostname, + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + }, + } + } + + // First request establishes the Hostinfo/DERP state. + _, err := s.UpdateNodeFromMapRequest(nodeID, req()) + require.NoError(t, err) + + beforeSeen := mustLastSeen(t, s, nodeID) + + // Second request is value-identical: a no-op. + c, err := s.UpdateNodeFromMapRequest(nodeID, req()) + require.NoError(t, err) + + require.Truef(t, c.IsEmpty(), + "no-op MapRequest must not emit a change, got reason=%q type=%q peersChanged=%v", + c.Reason, c.Type(), c.PeersChanged) + + require.True(t, mustLastSeen(t, s, nodeID).After(beforeSeen), + "no-op request must still stamp LastSeen in the NodeStore") +} + +// TestSTUNOnlyEndpointUpdateEmitsNoPeerChange ensures endpoint churn that +// endpointBroadcastWorthy suppresses stays suppressed instead of escalating +// to a whole-peer change. +func TestSTUNOnlyEndpointUpdateEmitsNoPeerChange(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + nv, ok := s.GetNodeByID(nodeID) + require.True(t, ok, "node should exist in NodeStore") + + stored := nv.AsStruct() + + hi := func() *tailcfg.Hostinfo { + return &tailcfg.Hostinfo{ + Hostname: stored.Hostname, + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + } + } + + local := netip.MustParseAddrPort("192.168.1.5:41641") + + // First request establishes the Hostinfo/DERP state and a useful + // endpoint, so the delta below is churn on a set peers already hold + // rather than the node's first endpoints. + _, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: hi(), + Endpoints: []netip.AddrPort{local}, + EndpointTypes: []tailcfg.EndpointType{tailcfg.EndpointLocal}, + }) + require.NoError(t, err) + + // Second request adds a single STUN-derived endpoint and nothing else. + c, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: hi(), + Endpoints: []netip.AddrPort{local, netip.MustParseAddrPort("198.51.100.7:41641")}, + EndpointTypes: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointSTUN}, + }) + require.NoError(t, err) + + require.Truef(t, c.IsEmpty(), + "suppressed STUN-only endpoint delta must not emit a change, got reason=%q type=%q peersChanged=%v", + c.Reason, c.Type(), c.PeersChanged) + + after, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + require.Contains(t, after.Endpoints().AsSlice(), netip.MustParseAddrPort("198.51.100.7:41641"), + "suppressed endpoint must still be stored") +} + +// TestFirstEndpointsReachPeersEvenWhenSTUNOnly pins that the first endpoint +// set a node announces is always broadcast. Peers hold no endpoints for it +// yet, and a suppressed delta is never resent, so suppressing the first set +// leaves peers with no direct path for the life of the node. +func TestFirstEndpointsReachPeersEvenWhenSTUNOnly(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + nv, ok := s.GetNodeByID(nodeID) + require.True(t, ok, "node should exist in NodeStore") + require.Empty(t, nv.Endpoints().AsSlice(), "precondition: node starts with no endpoints") + + stored := nv.AsStruct() + stun := netip.MustParseAddrPort("198.51.100.7:41641") + + c, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Endpoints: []netip.AddrPort{stun}, + EndpointTypes: []tailcfg.EndpointType{tailcfg.EndpointSTUN}, + }) + require.NoError(t, err) + + require.Falsef(t, c.IsEmpty(), + "a node's first endpoints must reach peers, got reason=%q type=%q", c.Reason, c.Type()) + require.Len(t, c.PeerPatches, 1, "expected a single endpoint patch") + require.Contains(t, c.PeerPatches[0].Endpoints, stun, + "the patch must carry the announced endpoint") +} + +// TestMapRequestDERPLatencyJitterEmitsNoPeerChange pins the wiring between +// [State.UpdateNodeFromMapRequest] and the Hostinfo comparison: DERP latency +// is transport diagnostics that no peer reads, so jitter must not reach the +// classifier as a change. hostinfoEqual covers the comparison itself; this +// covers the path. +func TestMapRequestDERPLatencyJitterEmitsNoPeerChange(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + nv, ok := s.GetNodeByID(nodeID) + require.True(t, ok, "node should exist in NodeStore") + + stored := nv.AsStruct() + + hi := func(latency float64) *tailcfg.Hostinfo { + return &tailcfg.Hostinfo{ + Hostname: stored.Hostname, + NetInfo: &tailcfg.NetInfo{ + PreferredDERP: 1, + DERPLatency: map[string]float64{"1-v4": latency}, + }, + } + } + + // Establish the Hostinfo, DERP region and a first latency sample. + _, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: hi(0.010), + }) + require.NoError(t, err) + + // Only the latency sample moves. + c, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: hi(0.025), + }) + require.NoError(t, err) + + require.Truef(t, c.IsEmpty(), + "DERP latency jitter must not emit a change, got reason=%q type=%q peersChanged=%v", + c.Reason, c.Type(), c.PeersChanged) + + after, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + require.Equal(t, 1, int(after.Hostinfo().NetInfo().PreferredDERP()), + "the DERP region must survive a latency-only request") +} + +// TestMapRequestOmittedNetInfoIsNoChange pins that a MapRequest carrying +// Hostinfo with NetInfo omitted is compared against the preserved NetInfo, not +// against the bare request. Tailscale clients send NetInfo only when it +// changed, so classifying the omission as a Hostinfo change turns +// every routine map request into a database write, an O(n) policy rescan and a +// whole-peer broadcast. +func TestMapRequestOmittedNetInfoIsNoChange(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + var nodeUpdateCount atomic.Int64 + + gdb := s.DB().DB + cbName := "omitted_netinfo_count_node_updates" + err := gdb.Callback().Update().After("gorm:update").Register(cbName, func(tx *gorm.DB) { + if tx.Statement != nil && tx.Statement.Table == "nodes" { + nodeUpdateCount.Add(1) + } + }) + require.NoError(t, err) + t.Cleanup(func() { _ = gdb.Callback().Update().Remove(cbName) }) + + nv, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + stored := nv.AsStruct() + + // Establish Hostinfo with NetInfo. + _, err = s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: stored.Hostname, + OS: "linux", + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + }, + }) + require.NoError(t, err) + require.Positive(t, nodeUpdateCount.Load(), "first request must persist") + + nodeUpdateCount.Store(0) + + // Same Hostinfo, NetInfo omitted: the client is saying "unchanged". + c, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: stored.Hostname, + OS: "linux", + }, + }) + require.NoError(t, err) + + require.True(t, c.IsEmpty(), "omitted NetInfo must not broadcast, got %+v", c) + require.Equalf(t, int64(0), nodeUpdateCount.Load(), + "omitted NetInfo must not persist, got %d updates", nodeUpdateCount.Load()) + + after, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + require.Equal(t, tailcfg.DERPRegionID(1), hostinfoDERP(after.AsStruct().Hostinfo), + "stored NetInfo must survive a request that omits it") +} + +// TestMapRequestDERPClearToZeroIsStoredAndBroadcast pins that a node reporting +// PreferredDERP 0 has its home region cleared. tailcfg.PeerChange.DERPRegion +// zero means "unchanged" on the wire, so the clear cannot ride a patch and has +// to escalate to a whole-peer update. +func TestMapRequestDERPClearToZeroIsStoredAndBroadcast(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + var nodeUpdateCount atomic.Int64 + + gdb := s.DB().DB + cbName := "derp_clear_count_node_updates" + err := gdb.Callback().Update().After("gorm:update").Register(cbName, func(tx *gorm.DB) { + if tx.Statement != nil && tx.Statement.Table == "nodes" { + nodeUpdateCount.Add(1) + } + }) + require.NoError(t, err) + t.Cleanup(func() { _ = gdb.Callback().Update().Remove(cbName) }) + + nv, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + stored := nv.AsStruct() + + _, err = s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: stored.Hostname, + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + }, + }) + require.NoError(t, err) + + nodeUpdateCount.Store(0) + + c, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: stored.Hostname, + NetInfo: &tailcfg.NetInfo{PreferredDERP: 0}, + }, + }) + require.NoError(t, err) + + after, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + require.Equal(t, tailcfg.DERPRegionID(0), hostinfoDERP(after.AsStruct().Hostinfo), + "clearing PreferredDERP must be stored") + + require.Contains(t, c.PeersChanged, nodeID, + "clearing PreferredDERP cannot be a patch, got %+v", c) + require.Empty(t, c.PeerPatches, + "clearing PreferredDERP must not emit a DERP patch, got %+v", c) + require.Positive(t, nodeUpdateCount.Load(), "clearing PreferredDERP must be persisted") +} + +// TestMapRequestDERPOnlyChangeKeepsGivenName pins that a request changing only +// PreferredDERP does not re-derive GivenName. Peers only receive a DERP patch +// for such a request, so a rename here would never reach them. +func TestMapRequestDERPOnlyChangeKeepsGivenName(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + nv, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + stored := nv.AsStruct() + + req := func(derp tailcfg.DERPRegionID) tailcfg.MapRequest { + return tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: stored.Hostname, + NetInfo: &tailcfg.NetInfo{PreferredDERP: derp}, + }, + } + } + + _, err := s.UpdateNodeFromMapRequest(nodeID, req(1)) + require.NoError(t, err) + + // A collision-bumped name whose base is free again is exactly what the + // auto-derive path rewrites. + bumped := stored.Hostname + "-1" + _, ok = s.nodeStore.UpdateNode(nodeID, func(n *types.Node) { n.GivenName = bumped }) + require.True(t, ok) + + c, err := s.UpdateNodeFromMapRequest(nodeID, req(2)) + require.NoError(t, err) + require.NotEmpty(t, c.PeerPatches, "DERP-only change must be a patch, got %+v", c) + + after, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + require.Equal(t, bumped, after.GivenName(), "DERP-only change must not touch GivenName") +} + +func TestConcurrentMapRequestDERPUsesPersistedState(t *testing.T) { + for _, rotateKey := range []bool{false, true} { + for _, tt := range []struct { + name string + firstDERP, lastDERP tailcfg.DERPRegionID + }{ + {name: "newer region", firstDERP: 2, lastDERP: 3}, + {name: "newer clear", firstDERP: 2, lastDERP: 0}, + {name: "superseded clear", firstDERP: 0, lastDERP: 3}, + } { + name := tt.name + if rotateKey { + name += " with key rotation" + } + + t.Run(name, func(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + nv, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + req := tailcfg.MapRequest{ + NodeKey: nv.NodeKey(), + DiscoKey: nv.DiscoKey(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: nv.Hostname(), + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + }, + } + _, err := s.UpdateNodeFromMapRequest(nodeID, req) + require.NoError(t, err) + + if rotateKey { + req.NodeKey = key.NewNode().Public() + req.DiscoKey = key.NewDisco().Public() + } + + // Let both requests publish their NodeStore writes before + // either can persist. Both must then use the last region, + // regardless of which response reaches peers first. + s.persistMu.Lock() + unlockPersist := sync.OnceFunc(s.persistMu.Unlock) + + var wg sync.WaitGroup + + t.Cleanup(func() { + unlockPersist() + wg.Wait() + }) + + type result struct { + change change.Change + err error + } + + results := make(chan result, 2) + + for _, derp := range []tailcfg.DERPRegionID{tt.firstDERP, tt.lastDERP} { + request := req + request.Hostinfo = req.Hostinfo.Clone() + request.Hostinfo.NetInfo.PreferredDERP = derp + + wg.Go(func() { + c, err := s.UpdateNodeFromMapRequest(nodeID, request) + results <- result{change: c, err: err} + }) + + require.EventuallyWithT(t, func(c *assert.CollectT) { + stored, exists := s.GetNodeByID(nodeID) + require.True(c, exists) + require.Equal(c, derp, stored.Hostinfo().NetInfo().PreferredDERP()) + }, 5*time.Second, time.Millisecond, "request must update NodeStore before persisting") + } + + unlockPersist() + wg.Wait() + close(results) + + for result := range results { + require.NoError(t, result.err) + + if tt.lastDERP == 0 { + require.Contains(t, result.change.PeersChanged, nodeID, + "the current region is zero, so neither response can use a DERP patch") + require.Empty(t, result.change.PeerPatches) + + continue + } + + require.Empty(t, result.change.PeersChanged) + require.Len(t, result.change.PeerPatches, 1) + require.Equal(t, tt.lastDERP, result.change.PeerPatches[0].DERPRegion, + "neither response may restore a superseded DERP region") + } + + persisted, err := s.DB().GetNodeByID(nodeID) + require.NoError(t, err) + require.Equal(t, tt.lastDERP, persisted.Hostinfo.NetInfo.PreferredDERP) + + // A resend is still a no-op; correct delivery cannot depend + // on a future identical request repairing the peer's state. + req.Hostinfo = req.Hostinfo.Clone() + req.Hostinfo.NetInfo.PreferredDERP = tt.lastDERP + c, err := s.UpdateNodeFromMapRequest(nodeID, req) + require.NoError(t, err) + require.True(t, c.IsEmpty()) + }) + } + } +} + +// TestMapRequestPeerInvisibleHostinfoChangeIsStoredNotBroadcast pins that +// a Hostinfo field no peer reads is stored and persisted but does not +// resend the whole node to every peer. +func TestMapRequestPeerInvisibleHostinfoChangeIsStoredNotBroadcast(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + var nodeUpdateCount atomic.Int64 + + gdb := s.DB().DB + cbName := "peer_invisible_count_node_updates" + err := gdb.Callback().Update().After("gorm:update").Register(cbName, func(tx *gorm.DB) { + if tx.Statement != nil && tx.Statement.Table == "nodes" { + nodeUpdateCount.Add(1) + } + }) + require.NoError(t, err) + t.Cleanup(func() { _ = gdb.Callback().Update().Remove(cbName) }) + + nv, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + stored := nv.AsStruct() + + req := func(model string) tailcfg.MapRequest { + return tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: stored.Hostname, + OS: "linux", + DeviceModel: model, + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + }, + } + } + + _, err = s.UpdateNodeFromMapRequest(nodeID, req("a")) + require.NoError(t, err) + + nodeUpdateCount.Store(0) + + c, err := s.UpdateNodeFromMapRequest(nodeID, req("b")) + require.NoError(t, err) + require.True(t, c.IsEmpty(), "peers read no DeviceModel, got %+v", c) + require.Positive(t, nodeUpdateCount.Load(), "the new DeviceModel must be persisted") + + after, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + require.Equal(t, "b", after.Hostinfo().DeviceModel(), "the new DeviceModel must be stored") +} + +// TestMapRequestPeerVisibleHostinfoChangeIsBroadcast pins that a Hostinfo +// field peers read still resends the whole node. +func TestMapRequestPeerVisibleHostinfoChangeIsBroadcast(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + nv, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + stored := nv.AsStruct() + + req := func(os string) tailcfg.MapRequest { + return tailcfg.MapRequest{ + NodeKey: stored.NodeKey, + DiscoKey: stored.DiscoKey, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: stored.Hostname, + OS: os, + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + }, + } + } + + _, err := s.UpdateNodeFromMapRequest(nodeID, req("linux")) + require.NoError(t, err) + + c, err := s.UpdateNodeFromMapRequest(nodeID, req("windows")) + require.NoError(t, err) + require.Contains(t, c.PeersChanged, nodeID, "peers read OS, got %+v", c) +} + +func mustLastSeen(t *testing.T, s *State, id types.NodeID) time.Time { + t.Helper() + + nv, ok := s.GetNodeByID(id) + require.True(t, ok) + + seen, ok := nv.LastSeen().GetOk() + require.True(t, ok, "LastSeen must be stamped") + + return seen +} + +func TestHostinfoEqual(t *testing.T) { + t.Run("both nil", func(t *testing.T) { + require.True(t, hostinfoEqual(nil, nil)) + }) + + t.Run("one nil", func(t *testing.T) { + require.False(t, hostinfoEqual(&tailcfg.Hostinfo{}, nil)) + require.False(t, hostinfoEqual(nil, &tailcfg.Hostinfo{})) + }) + + t.Run("identical", func(t *testing.T) { + a := &tailcfg.Hostinfo{Hostname: "node1", OS: "linux"} + b := &tailcfg.Hostinfo{Hostname: "node1", OS: "linux"} + require.True(t, hostinfoEqual(a, b)) + }) + + t.Run("DERPLatency jitter is not a change", func(t *testing.T) { + a := &tailcfg.Hostinfo{ + Hostname: "node1", + NetInfo: &tailcfg.NetInfo{ + PreferredDERP: 1, + DERPLatency: map[string]float64{"1-v4": 0.010}, + }, + } + b := &tailcfg.Hostinfo{ + Hostname: "node1", + NetInfo: &tailcfg.NetInfo{ + PreferredDERP: 1, + DERPLatency: map[string]float64{"1-v4": 0.025}, // jitter + }, + } + require.True(t, hostinfoEqual(a, b), + "DERPLatency jitter must not count as a change") + }) + + t.Run("PreferredDERP change is not a change here", func(t *testing.T) { + a := &tailcfg.Hostinfo{ + Hostname: "node1", + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + } + b := &tailcfg.Hostinfo{ + Hostname: "node1", + NetInfo: &tailcfg.NetInfo{PreferredDERP: 2}, + } + require.True(t, hostinfoEqual(a, b), + "PreferredDERP change is tracked separately") + }) + + t.Run("hostname change", func(t *testing.T) { + a := &tailcfg.Hostinfo{Hostname: "node1"} + b := &tailcfg.Hostinfo{Hostname: "node2"} + require.False(t, hostinfoEqual(a, b)) + }) + + t.Run("route change", func(t *testing.T) { + a := &tailcfg.Hostinfo{Hostname: "node1", RoutableIPs: nil} + b := &tailcfg.Hostinfo{Hostname: "node1"} + // RoutableIPs nil vs nil: equal + require.True(t, hostinfoEqual(a, b)) + + // Add a route. + // (also tracked separately as routesChangedInput) + b.RoutableIPs = []netip.Prefix{netip.MustParsePrefix("10.0.0.0/24")} + require.False(t, hostinfoEqual(a, b)) + }) +} + +func TestPeerHostinfoEqual(t *testing.T) { + base := func() *tailcfg.Hostinfo { + return &tailcfg.Hostinfo{ + Hostname: "node", + OS: "linux", + DeviceModel: "laptop", + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1, WorkingUDP: opt.NewBool(true)}, + } + } + + tests := []struct { + name string + mutate func(*tailcfg.Hostinfo) + want bool + }{ + {name: "no change", mutate: func(*tailcfg.Hostinfo) {}, want: true}, + {name: "device model", mutate: func(hi *tailcfg.Hostinfo) { hi.DeviceModel = "desktop" }, want: true}, + {name: "shields up", mutate: func(hi *tailcfg.Hostinfo) { hi.ShieldsUp = true }, want: true}, + {name: "ipn version", mutate: func(hi *tailcfg.Hostinfo) { hi.IPNVersion = "1.99" }, want: true}, + {name: "netinfo udp", mutate: func(hi *tailcfg.Hostinfo) { hi.NetInfo.WorkingUDP = opt.NewBool(false) }, want: true}, + {name: "netinfo dropped", mutate: func(hi *tailcfg.Hostinfo) { hi.NetInfo = nil }, want: true}, + {name: "hostname", mutate: func(hi *tailcfg.Hostinfo) { hi.Hostname = "other" }, want: false}, + {name: "os", mutate: func(hi *tailcfg.Hostinfo) { hi.OS = "windows" }, want: false}, + { + name: "services", + mutate: func(hi *tailcfg.Hostinfo) { hi.Services = []tailcfg.Service{{Proto: "peerapi4", Port: 1}} }, + want: false, + }, + {name: "ssh host keys", mutate: func(hi *tailcfg.Hostinfo) { hi.SSH_HostKeys = []string{"ssh-ed25519 AAAA"} }, want: false}, + {name: "location", mutate: func(hi *tailcfg.Hostinfo) { hi.Location = &tailcfg.Location{Priority: 5} }, want: false}, + {name: "app connector", mutate: func(hi *tailcfg.Hostinfo) { hi.AppConnector = opt.NewBool(true) }, want: false}, + { + name: "routes", + mutate: func(hi *tailcfg.Hostinfo) { hi.RoutableIPs = []netip.Prefix{netip.MustParsePrefix("10.0.0.0/24")} }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + changed := base() + tt.mutate(changed) + require.Equal(t, tt.want, peerHostinfoEqual(base(), changed)) + }) + } + + require.True(t, peerHostinfoEqual(nil, nil)) + require.False(t, peerHostinfoEqual(nil, base())) +} + +func TestNetInfoEqualIgnoringDERP(t *testing.T) { + t.Run("both nil", func(t *testing.T) { + require.True(t, netInfoEqualIgnoringDERP(nil, nil)) + }) + + t.Run("ignores PreferredDERP", func(t *testing.T) { + a := &tailcfg.NetInfo{PreferredDERP: 1, WorkingUDP: opt.NewBool(true)} + b := &tailcfg.NetInfo{PreferredDERP: 9, WorkingUDP: opt.NewBool(true)} + require.True(t, netInfoEqualIgnoringDERP(a, b)) + }) + + t.Run("ignores DERPLatency", func(t *testing.T) { + a := &tailcfg.NetInfo{DERPLatency: map[string]float64{"1": 0.01}} + b := &tailcfg.NetInfo{DERPLatency: map[string]float64{"1": 0.99}} + require.True(t, netInfoEqualIgnoringDERP(a, b)) + }) + + t.Run("catches WorkingUDP change", func(t *testing.T) { + a := &tailcfg.NetInfo{WorkingUDP: opt.NewBool(true)} + b := &tailcfg.NetInfo{WorkingUDP: opt.NewBool(false)} + require.False(t, netInfoEqualIgnoringDERP(a, b)) + }) +} + +func TestHostinfoDERP(t *testing.T) { + require.Equal(t, tailcfg.DERPRegionID(0), hostinfoDERP(nil)) + require.Equal(t, tailcfg.DERPRegionID(0), hostinfoDERP(&tailcfg.Hostinfo{})) + require.Equal(t, tailcfg.DERPRegionID(0), hostinfoDERP(&tailcfg.Hostinfo{NetInfo: &tailcfg.NetInfo{}})) + require.Equal(t, tailcfg.DERPRegionID(5), hostinfoDERP(&tailcfg.Hostinfo{NetInfo: &tailcfg.NetInfo{PreferredDERP: 5}})) +} + +func TestBuildMapRequestChangeResponse(t *testing.T) { + node := types.Node{ + ID: 1, + NodeKey: key.NewNode().Public(), + DiscoKey: key.NewDisco().Public(), + Endpoints: []netip.AddrPort{netip.MustParseAddrPort("203.0.113.9:41641")}, + Hostinfo: &tailcfg.Hostinfo{NetInfo: &tailcfg.NetInfo{PreferredDERP: 2}}, + } + tests := []struct { + name string + delta mapRequestDelta + nodeDERP tailcfg.DERPRegionID + + wantEmpty bool + wantWholePeer bool + wantKeys bool + wantEndpoints bool + wantDERP tailcfg.DERPRegionID + }{ + { + name: "nothing peer visible", + delta: mapRequestDelta{}, + + wantEmpty: true, + }, + { + name: "peer visible hostinfo", + delta: mapRequestDelta{peerHostinfoChanged: true, endpointBroadcast: true}, + + wantWholePeer: true, + }, + { + name: "derp clear to zero", + delta: mapRequestDelta{derpChanged: true, oldDERP: 1, newDERP: 0}, + + wantWholePeer: true, + }, + { + name: "disco key and derp clear to zero", + delta: mapRequestDelta{discoKeyChanged: true, derpChanged: true, oldDERP: 1, newDERP: 0}, + + wantWholePeer: true, + }, + { + name: "disco key only", + delta: mapRequestDelta{discoKeyChanged: true}, + + wantKeys: true, + wantEndpoints: true, + }, + { + name: "disco key and derp move", + delta: mapRequestDelta{discoKeyChanged: true, derpChanged: true, oldDERP: 1, newDERP: 2}, + nodeDERP: 2, + + wantKeys: true, + wantEndpoints: true, + wantDERP: 2, + }, + { + name: "useful endpoint only", + delta: mapRequestDelta{endpointBroadcast: true}, + + wantEndpoints: true, + }, + { + name: "derp move only", + delta: mapRequestDelta{derpChanged: true, oldDERP: 1, newDERP: 2}, + nodeDERP: 2, + + wantDERP: 2, + }, + { + name: "endpoint and derp move", + delta: mapRequestDelta{endpointBroadcast: true, derpChanged: true, oldDERP: 1, newDERP: 2}, + nodeDERP: 2, + + wantEndpoints: true, + wantDERP: 2, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + current := node.Clone() + current.Hostinfo.NetInfo.PreferredDERP = tt.nodeDERP + c := buildMapRequestChangeResponse(node.ID, current.View(), tt.delta) + + if tt.wantEmpty { + require.True(t, c.IsEmpty(), "got %+v", c) + + return + } + + if tt.wantWholePeer { + require.Contains(t, c.PeersChanged, node.ID, "got %+v", c) + require.Empty(t, c.PeerPatches) + + return + } + + require.Empty(t, c.PeersChanged, "got %+v", c) + require.Len(t, c.PeerPatches, 1) + + patch := c.PeerPatches[0] + require.Equal(t, tt.wantKeys, patch.Key != nil && patch.DiscoKey != nil, "keys on patch") + require.Equal(t, tt.wantEndpoints, patch.Endpoints != nil, "endpoints on patch") + require.Equal(t, tt.wantDERP, patch.DERPRegion, "DERP on patch") + }) + } +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index e5adc8dc..2de8aea3 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -31,6 +31,7 @@ import ( "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/types/change" "github.com/juanfont/headscale/hscontrol/util" + "github.com/juanfont/headscale/hscontrol/util/zlog" "github.com/juanfont/headscale/hscontrol/util/zlog/zf" "github.com/puzpuzpuz/xsync/v4" "github.com/rs/zerolog" @@ -3051,25 +3052,22 @@ func isAutoDerivedGivenName(given, hostname string) bool { // // TODO(kradalby): This is essentially a patch update that could be sent directly to nodes, // which means we could shortcut the whole change thing if there are no other important updates. -// When a field is added to this function, remember to also add it to: -// - node.PeerChangeFromMapRequest -// - node.ApplyPeerChange -// - logTracePeerChange in poll.go. +// When a field is added to a MapRequest that is stored on the node, also add +// it to [types.Node.PeerChangeFromMapRequest], [types.Node.ApplyPeerChange], +// the mapRequestDelta classification, and (if the policy reads it) +// [types.NodeView.HasPolicyChange]. func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest) (change.Change, error) { //nolint:gocyclo // central map-request reconciliation; the sequential branch flow reads clearer as one function than split across helpers log.Trace(). Caller(). Uint64(zf.NodeID, id.Uint64()). - Interface("request", req). + EmbedObject(zlog.MapRequest(&req)). Msg("Processing MapRequest for node") var ( + delta mapRequestDelta routeChange bool - hostinfoChanged bool needsRouteApproval bool autoApprovedRoutes []netip.Prefix - endpointChanged bool - derpChanged bool - persistWorthy bool ) // Snapshot the primary assignment so we can tell whether the // Hostinfo + auto-approval that follows shifted any prefix. @@ -3078,49 +3076,84 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest // We need to ensure we update the node as it is in the [NodeStore] at // the time of the request. updatedNode, ok := s.nodeStore.UpdateNode(id, func(currentNode *types.Node) { - peerChange := currentNode.PeerChangeFromMapRequest(req) + // Capture the raw wire-level peer change. LastSeen is always + // stamped here, so classification tests must not rely on it. + delta.peerChange = currentNode.PeerChangeFromMapRequest(req) + delta.keyChanged = delta.peerChange.Key != nil + delta.discoKeyChanged = delta.peerChange.DiscoKey != nil - // Track what specifically changed. An endpoint delta is only - // broadcast-worthy when it adds a useful (non-STUN) endpoint; - // STUN-only churn and pure shrinks are suppressed to reduce peer - // churn (see endpointBroadcastWorthy). The new set is still stored - // via ApplyPeerChange below regardless of this decision. - endpointChanged = peerChange.Endpoints != nil && - endpointBroadcastWorthy(currentNode.Endpoints, req.Endpoints, req.EndpointTypes) - derpChanged = peerChange.DERPRegion != 0 - hostinfoChanged = !hostinfoEqual(currentNode.View(), req.Hostinfo) + // Normalize before classifying. A nil req.Hostinfo means the client + // did not send one (e.g., endpoint-only/lite requests); we must NOT + // clobber the stored Hostinfo with a shell containing only NetInfo. + // When Hostinfo is present but NetInfo is omitted (Tailscale >= 1.66 + // sends NetInfo only when it changed), the stored NetInfo is carried + // over. Every comparison below runs against this normalized value, + // otherwise an omitted NetInfo reads as a Hostinfo change and turns + // a routine map request into a whole-peer broadcast. + var newHostinfo *tailcfg.Hostinfo - // Get the correct NetInfo to use - netInfo := netInfoFromMapRequest(id, currentNode.Hostinfo, req.Hostinfo) if req.Hostinfo != nil { - req.Hostinfo.NetInfo = netInfo - } else { - req.Hostinfo = &tailcfg.Hostinfo{NetInfo: netInfo} + // Copy so the classification never mutates the caller's request. + hi := *req.Hostinfo + hi.NetInfo = netInfoFromMapRequest(id, currentNode.Hostinfo, req.Hostinfo) + newHostinfo = &hi } - // Re-check hostinfoChanged after potential NetInfo preservation - hostinfoChanged = !hostinfoEqual(currentNode.View(), req.Hostinfo) + // DERP comparison is independent of the rest of Hostinfo: + // PreferredDERP has its own wire patch representation, and + // DERP zero on the wire means "unchanged", so a clear-to-zero + // must be detected here and escalated to a whole-peer update + // during classification. A request without Hostinfo says nothing + // about DERP, so it compares equal. + storedDERP := hostinfoDERP(currentNode.Hostinfo) + requestedDERP := storedDERP - // A change carrying only an updated LastSeen is not worth a full-row - // database UPDATE plus the O(n) policy rescan persistNodeAndRefreshPolicy triggers: - // LastSeen is best-effort and rides along the next substantive write. - // PeerChangeFromMapRequest always stamps LastSeen, so test the other - // fields explicitly. - persistWorthy = peerChangePersistWorthy(peerChange) || hostinfoChanged - - // If there is no changes and nothing to save, - // return early. - if peerChangeEmpty(peerChange) && !hostinfoChanged { - return + if newHostinfo != nil { + requestedDERP = hostinfoDERP(newHostinfo) } - // Calculate route approval before [NodeStore] update to avoid calling View() inside callback + delta.oldDERP = storedDERP + delta.newDERP = requestedDERP + delta.derpChanged = requestedDERP != storedDERP + + // Endpoint broadcast-worthiness is gated separately from storage: + // the new set is always stored via ApplyPeerChange below, but only + // newly-added useful (non-STUN) endpoints justify a peer broadcast. + // STUN-only churn and pure shrinks are suppressed to keep peers' + // views stable. See endpointBroadcastWorthy. + delta.endpointBroadcast = delta.peerChange.Endpoints != nil && + endpointBroadcastWorthy(currentNode.Endpoints, req.Endpoints, req.EndpointTypes) + + // Routes are policy and election inputs, so they are compared on + // their own. Any other Hostinfo change is stored, but only fields + // peers read are worth resending the whole node for. + delta.routesChanged = newHostinfo != nil && + routesChanged(currentNode.View(), newHostinfo) + delta.hostinfoChanged = newHostinfo != nil && + !hostinfoEqual(currentNode.Hostinfo, newHostinfo) + delta.peerHostinfoChanged = newHostinfo != nil && + !peerHostinfoEqual(currentNode.Hostinfo, newHostinfo) + + // A change carrying only an updated LastSeen is not worth a + // full-row database UPDATE plus the O(n) policy rescan + // persistNodeAndRefreshPolicy triggers: LastSeen is best-effort and rides + // along the next substantive write. DERP is called out because + // peerChangePersistWorthy cannot see a clear-to-zero. + delta.persistWorthy = peerChangePersistWorthy(delta.peerChange) || + delta.hostinfoChanged || + delta.derpChanged + + hostinfoToStore := delta.hostinfoChanged || delta.derpChanged + + // Calculate route approval before [NodeStore] update to avoid + // calling View() inside callback var hasNewRoutes bool if hi := req.Hostinfo; hi != nil { hasNewRoutes = len(hi.RoutableIPs) > 0 } - needsRouteApproval = hostinfoChanged && (routesChanged(currentNode.View(), req.Hostinfo) || (hasNewRoutes && len(currentNode.ApprovedRoutes) == 0)) + needsRouteApproval = delta.hostinfoChanged && + (delta.routesChanged || (hasNewRoutes && len(currentNode.ApprovedRoutes) == 0)) if needsRouteApproval { // Extract announced routes from request var announcedRoutes []netip.Prefix @@ -3140,59 +3173,52 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest } // Log when routes change but approval doesn't - if hostinfoChanged && !routeChange { + if delta.routesChanged && !routeChange { if hi := req.Hostinfo; hi != nil { - if routesChanged(currentNode.View(), hi) { - log.Debug(). - Caller(). - Uint64(zf.NodeID, id.Uint64()). - Strs(zf.OldAnnouncedRoutes, util.PrefixesToString(currentNode.AnnouncedRoutes())). - Strs(zf.NewAnnouncedRoutes, util.PrefixesToString(hi.RoutableIPs)). - Strs(zf.ApprovedRoutes, util.PrefixesToString(currentNode.ApprovedRoutes)). - Bool(zf.RouteChanged, routeChange). - Msg("announced routes changed but approved routes did not") - } + log.Debug(). + Caller(). + Uint64(zf.NodeID, id.Uint64()). + Strs(zf.OldAnnouncedRoutes, util.PrefixesToString(currentNode.AnnouncedRoutes())). + Strs(zf.NewAnnouncedRoutes, util.PrefixesToString(hi.RoutableIPs)). + Strs(zf.ApprovedRoutes, util.PrefixesToString(currentNode.ApprovedRoutes)). + Bool(zf.RouteChanged, routeChange). + Msg("announced routes changed but approved routes did not") } } - currentNode.ApplyPeerChange(&peerChange) + currentNode.ApplyPeerChange(&delta.peerChange) - if hostinfoChanged { - // The node might not set NetInfo if it has not changed and if - // the full HostInfo object is overwritten, the information is lost. - // If there is no NetInfo, keep the previous one. - // From 1.66 the client only sends it if changed: - // https://github.com/tailscale/tailscale/commit/e1011f138737286ecf5123ff887a7a5800d129a2 - // TODO(kradalby): evaluate if we need better comparing of hostinfo - // before we take the changes. - // NetInfo preservation has already been handled above before early return check - currentNode.Hostinfo = req.Hostinfo - if req.Hostinfo != nil && req.Hostinfo.Hostname != "" { - // Preserve an admin-renamed GivenName: only auto-derive when the - // current GivenName is still what SanitizeHostname of the old - // Hostname would produce (possibly with a "-N" collision bump). - autoDerived := isAutoDerivedGivenName(currentNode.GivenName, currentNode.Hostname) + if hostinfoToStore { + currentNode.Hostinfo = newHostinfo + } - currentNode.Hostname = req.Hostinfo.Hostname - if autoDerived { - currentNode.GivenName = dnsname.SanitizeHostname(req.Hostinfo.Hostname) - // [NodeStore.UpdateNode] auto-bumps GivenName on collision. - } + // Only a real hostname change may re-derive GivenName: it is peer + // visible, so the whole node is resent and peers learn the name. + if newHostinfo != nil && newHostinfo.Hostname != "" && + newHostinfo.Hostname != currentNode.Hostname { + // Preserve an admin-renamed GivenName: only auto-derive + // when the current GivenName is still what + // SanitizeHostname of the old Hostname would produce + // (possibly with a "-N" collision bump). + autoDerived := isAutoDerivedGivenName(currentNode.GivenName, currentNode.Hostname) + + currentNode.Hostname = newHostinfo.Hostname + if autoDerived { + currentNode.GivenName = dnsname.SanitizeHostname(newHostinfo.Hostname) + // [NodeStore.UpdateNode] auto-bumps GivenName on collision. } + } - if routeChange { - // Apply pre-calculated route approval - // Always apply the route approval result to ensure consistency, - // regardless of whether the policy evaluation detected changes. - // This fixes the bug where routes weren't properly cleared when - // auto-approvers were removed from the policy. - log.Info(). - Uint64(zf.NodeID, id.Uint64()). - Strs(zf.OldApprovedRoutes, util.PrefixesToString(currentNode.ApprovedRoutes)). - Strs(zf.NewApprovedRoutes, util.PrefixesToString(autoApprovedRoutes)). - Bool(zf.RouteChanged, routeChange). - Msg("applying route approval results") - } + if routeChange { + // Always apply the route approval result so routes are + // cleared when auto-approvers are removed from the policy, + // even if the policy evaluation itself detected no change. + log.Info(). + Uint64(zf.NodeID, id.Uint64()). + Strs(zf.OldApprovedRoutes, util.PrefixesToString(currentNode.ApprovedRoutes)). + Strs(zf.NewApprovedRoutes, util.PrefixesToString(autoApprovedRoutes)). + Bool(zf.RouteChanged, routeChange). + Msg("applying route approval results") } // AllApprovedRoutes is announced ∩ approved; a Hostinfo @@ -3215,6 +3241,9 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest Msg("Persisting auto-approved routes from MapRequest") // [State.SetApprovedRoutes] will update both database and PrimaryRoutes table + // TODO(kradalby): approval should ride the map request write above. + // Writing it separately costs a second NodeStore write and a second + // peer-map rebuild for one request. _, c, err := s.SetApprovedRoutes(id, autoApprovedRoutes) if err != nil { return change.Change{}, fmt.Errorf("persisting auto-approved routes: %w", err) @@ -3246,15 +3275,32 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest // A no-op MapRequest (identical re-send / reconnect with matching state) // leaves the node untouched, so skip the full-row UPDATE and the O(n) // policy SetNodes scan that persistNodeAndRefreshPolicy performs. + // + // On the MapRequest path we deliberately bypass persistNodeAndRefreshPolicy's + // synthetic NodeAdded fallback: persistence must not fabricate a wire + // notification. We persist the row directly and refresh the policy + // manager only when node inputs visible to the policy actually changed + // (structural Hostinfo or routes), letting updatePolicyManagerNodes + // decide whether matchers changed. policyChange := change.Change{} - if persistWorthy { + if delta.persistWorthy { var err error - _, policyChange, err = s.persistNodeAndRefreshPolicy(updatedNode) + updatedNode, err = s.persistNode(updatedNode) if err != nil { return change.Change{}, fmt.Errorf("saving to database: %w", err) } + + // Only refresh the policy manager when something it depends on + // might have moved. Endpoint/key/DERP/LastSeen-only updates do not + // affect policy evaluation and are deliberately skipped here. + if delta.peerHostinfoChanged || delta.routesChanged { + policyChange, err = s.updatePolicyManagerNodes() + if err != nil { + return change.Change{}, fmt.Errorf("updating policy manager after node save: %w", err) + } + } } if !policyChange.IsEmpty() { @@ -3265,9 +3311,20 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest return nodeRouteChange, nil } - // Determine the most specific change type based on what actually changed. - // This allows us to send lightweight patch updates instead of full map responses. - return buildMapRequestChangeResponse(id, updatedNode, hostinfoChanged, endpointChanged, derpChanged) + // Determine the most specific change type from the classified delta. + // This allows us to send lightweight patch updates instead of full + // map responses. + c := buildMapRequestChangeResponse(id, updatedNode, delta) + + // One trace line per classified request so a "peer cannot reach me" + // report can be matched to the classification that narrowed it. + log.Trace(). + Uint64(zf.NodeID, id.Uint64()). + Str(zf.Type, c.Type()). + EmbedObject(delta). + Msg("classified MapRequest") + + return c, nil } // endpointBroadcastWorthy reports whether an endpoint-only delta is worth @@ -3278,9 +3335,9 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest // telling peers about. Suppressing this churn keeps peers' views stable. // // The decision is intentionally conservative: it gates the broadcast only, -// not storage. The node's full endpoint set (STUN included) is still stored -// and rides along the next substantive change or full MapResponse, so no -// reachable path is permanently hidden from peers. +// not storage. A suppressed delta is never resent on its own, so suppression +// is only safe once peers already hold an endpoint set to fall back on; the +// first set a node announces is therefore always broadcast. // // Limitation: headscale stores bare []netip.AddrPort with no per-endpoint // type, so we can only classify the *new* request's endpoints (via the @@ -3293,6 +3350,12 @@ func endpointBroadcastWorthy( stored, newEPs []netip.AddrPort, newTypes []tailcfg.EndpointType, ) bool { + // Peers hold no endpoints for this node yet, so the first set announced + // is the only one they would ever get. Type does not matter here. + if len(stored) == 0 { + return len(newEPs) > 0 + } + storedSet := make(map[netip.AddrPort]struct{}, len(stored)) for _, ep := range stored { storedSet[ep] = struct{}{} @@ -3327,52 +3390,60 @@ func isUsefulEndpointType(t tailcfg.EndpointType) bool { return t != tailcfg.EndpointSTUN && t != tailcfg.EndpointSTUN4LocalPort } -// buildMapRequestChangeResponse determines the appropriate response type for a [tailcfg.MapRequest] update. -// Hostinfo changes require a full update, while endpoint/DERP changes can use lightweight patches. +// buildMapRequestChangeResponse picks the narrowest broadcast for a processed +// MapRequest delta. Policy and route changes are handled by the caller. +// +// Two wire constraints shape the order: tailcfg.PeerChange.DERPRegion == 0 +// means "unchanged", so clearing DERP cannot ride a patch and needs a whole +// peer; and a key patch already carries endpoints, key expiry, and DERP, so a +// key change subsumes an endpoint or DERP patch in the same request. +// The delta decides which fields to send; their values come from the fresh +// node because concurrent requests may have superseded the captured values. func buildMapRequestChangeResponse( id types.NodeID, node types.NodeView, - hostinfoChanged, endpointChanged, derpChanged bool, -) (change.Change, error) { - // Hostinfo changes require NodeAdded (full update) as they may affect many fields. - if hostinfoChanged { - return change.NodeAdded(id), nil + delta mapRequestDelta, +) change.Change { + if delta.peerHostinfoChanged { + return change.NodeAdded(id) } - // Return specific change types for endpoint and/or DERP updates. - if endpointChanged || derpChanged { + var currentDERP tailcfg.DERPRegionID + + if delta.derpChanged { + if hi := node.Hostinfo(); hi.Valid() && hi.NetInfo().Valid() { + currentDERP = hi.NetInfo().PreferredDERP() + } + + if currentDERP == 0 { + return change.NodeAdded(id) + } + } + + if delta.keyChanged || delta.discoKeyChanged { + c := change.NodeKeyRotated(node) + if delta.derpChanged { + c.PeerPatches[0].DERPRegion = currentDERP + } + + return c + } + + if delta.endpointBroadcast || delta.derpChanged { patch := &tailcfg.PeerChange{NodeID: id.NodeID()} - if endpointChanged { + if delta.endpointBroadcast { patch.Endpoints = node.Endpoints().AsSlice() } - if derpChanged { - if hi := node.Hostinfo(); hi.Valid() { - if ni := hi.NetInfo(); ni.Valid() { - patch.DERPRegion = ni.PreferredDERP() - } - } + if delta.derpChanged { + patch.DERPRegion = currentDERP } - return change.EndpointOrDERPUpdate(id, patch), nil + return change.EndpointOrDERPUpdate(id, patch) } - return change.NodeAdded(id), nil -} - -func hostinfoEqual(oldNode types.NodeView, newHI *tailcfg.Hostinfo) bool { - if !oldNode.Valid() && newHI == nil { - return true - } - - if !oldNode.Valid() || newHI == nil { - return false - } - - old := oldNode.AsStruct().Hostinfo - - return old.Equal(newHI) + return change.Change{} } func routesChanged(oldNode types.NodeView, newHI *tailcfg.Hostinfo) bool { @@ -3392,16 +3463,6 @@ func routesChanged(oldNode types.NodeView, newHI *tailcfg.Hostinfo) bool { return !slices.Equal(oldRoutes, newRoutes) } -func peerChangeEmpty(peerChange tailcfg.PeerChange) bool { - return peerChange.Key == nil && - peerChange.DiscoKey == nil && - peerChange.Online == nil && - peerChange.Endpoints == nil && - peerChange.DERPRegion == 0 && - peerChange.LastSeen == nil && - peerChange.KeyExpiry == nil -} - // peerChangePersistWorthy reports whether a peer change carries anything that // warrants a database write. It deliberately ignores LastSeen, which // [Node.PeerChangeFromMapRequest] always stamps: a keepalive that only bumps diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 3609e3a9..78228124 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -623,9 +623,8 @@ func (node *Node) MarshalZerologObject(e *zerolog.Event) { // PeerChangeFromMapRequest takes a [tailcfg.MapRequest] and compares it to the node // to produce a [tailcfg.PeerChange] struct that can be used to updated the node and // inform peers about smaller changes to the node. -// When a field is added to this function, remember to also add it to: -// - [Node.ApplyPeerChange] -// - logTracePeerChange in poll.go. +// When a field is added to this function, also add it to +// [Node.ApplyPeerChange]. func (node *Node) PeerChangeFromMapRequest(req tailcfg.MapRequest) tailcfg.PeerChange { ret := tailcfg.PeerChange{ NodeID: tailcfg.NodeID(node.ID), //nolint:gosec // NodeID is bounded