state: patch relogins and gate endpoint broadcasts

Relogin sent as a peer patch with endpoints preserved; endpoint-only deltas broadcast only on useful (non-STUN) changes.
2026-07-08 00:50:20 +09:00 · 2026-06-15 08:18:59 +00:00
parent 4da06925d0
commit a5ef3aff15
3 changed files with 272 additions and 17 deletions
@@ -111,3 +111,88 @@ func TestEndpointStorageInNodeStore(t *testing.T) {
 		}
 	}
 }
+
+// TestEndpointBroadcastWorthy verifies the gate that decides whether an
+// endpoint-only delta is worth fanning out to peers as an incremental
+// PeersChangedPatch. A delta that only adds STUN-derived endpoints (or only
+// removes endpoints) is suppressed: it is churny and unlikely to be useful,
+// and disco's callMeMaybe re-derives STUN paths anyway. Only deltas that
+// introduce a genuinely useful (non-STUN) endpoint are broadcast-worthy.
+func TestEndpointBroadcastWorthy(t *testing.T) {
+	local := netip.MustParseAddrPort("192.168.1.5:41641")
+	local2 := netip.MustParseAddrPort("192.168.1.6:41641")
+	stun := netip.MustParseAddrPort("203.0.113.7:41641")
+	stun2 := netip.MustParseAddrPort("203.0.113.8:41641")
+	portmap := netip.MustParseAddrPort("198.51.100.9:41641")
+
+	tests := []struct {
+		name    string
+		stored  []netip.AddrPort
+		newEPs  []netip.AddrPort
+		newType []tailcfg.EndpointType
+		want    bool
+	}{
+		{
+			name:    "adds only a STUN endpoint - suppress",
+			stored:  []netip.AddrPort{local},
+			newEPs:  []netip.AddrPort{local, stun},
+			newType: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointSTUN},
+			want:    false,
+		},
+		{
+			name:    "adds only STUN4LocalPort - suppress",
+			stored:  []netip.AddrPort{local},
+			newEPs:  []netip.AddrPort{local, stun},
+			newType: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointSTUN4LocalPort},
+			want:    false,
+		},
+		{
+			name:    "adds a useful local endpoint - broadcast",
+			stored:  []netip.AddrPort{stun},
+			newEPs:  []netip.AddrPort{stun, local},
+			newType: []tailcfg.EndpointType{tailcfg.EndpointSTUN, tailcfg.EndpointLocal},
+			want:    true,
+		},
+		{
+			name:    "adds a useful portmapped endpoint - broadcast",
+			stored:  []netip.AddrPort{local},
+			newEPs:  []netip.AddrPort{local, portmap},
+			newType: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointPortmapped},
+			want:    true,
+		},
+		{
+			name:    "pure shrink, no additions - suppress",
+			stored:  []netip.AddrPort{local, local2},
+			newEPs:  []netip.AddrPort{local},
+			newType: []tailcfg.EndpointType{tailcfg.EndpointLocal},
+			want:    false,
+		},
+		{
+			name:   "nil types (older client) adding endpoint - broadcast",
+			stored: []netip.AddrPort{local},
+			newEPs: []netip.AddrPort{local, local2},
+			want:   true,
+		},
+		{
+			name:    "only STUN endpoints churn (replace one STUN with another) - suppress",
+			stored:  []netip.AddrPort{local, stun},
+			newEPs:  []netip.AddrPort{local, stun2},
+			newType: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointSTUN},
+			want:    false,
+		},
+		{
+			name:    "mixed add: one STUN and one useful - broadcast",
+			stored:  []netip.AddrPort{local},
+			newEPs:  []netip.AddrPort{local, stun, local2},
+			newType: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointSTUN, tailcfg.EndpointLocal},
+			want:    true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := endpointBroadcastWorthy(tt.stored, tt.newEPs, tt.newType)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
@@ -325,3 +325,93 @@ func TestReauthRejectsNodeKeyClaimedByAnotherMachine(t *testing.T) {
 	require.Error(t, err,
 		"re-auth claiming a NodeKey bound to another machine must be rejected")
 }
+
+// TestReauthPreservesEndpointsWhenClientOmitsThem proves the re-auth/update
+// path keeps a node's live WireGuard endpoints when the originating
+// RegisterRequest carried none. Web/OIDC relogins report endpoints via
+// MapRequest, not register, so RegData.Endpoints is empty; wiping the stored
+// endpoints would advertise the re-keyed node to peers endpoint-less, which
+// drives head/unstable tailscale clients into one-way disco-deafness.
+func TestReauthPreservesEndpointsWhenClientOmitsThem(t *testing.T) {
+	dbPath := t.TempDir() + "/headscale.db"
+	cfg := persistTestConfig(dbPath)
+
+	database, err := db.NewHeadscaleDatabase(cfg)
+	require.NoError(t, err)
+
+	user := database.CreateUserForTest("user")
+	require.NoError(t, database.Close())
+
+	s, err := NewState(cfg)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = s.Close() })
+
+	machine := key.NewMachine()
+	endpoints := []netip.AddrPort{
+		netip.MustParseAddrPort("192.168.1.5:41641"),
+		netip.MustParseAddrPort("10.0.0.5:41641"),
+	}
+
+	// Node is registered and has reported live endpoints (as after its first
+	// MapRequest).
+	node, err := s.createAndSaveNewNode(newNodeParams{
+		User:           *user,
+		MachineKey:     machine.Public(),
+		NodeKey:        key.NewNode().Public(),
+		DiscoKey:       key.NewDisco().Public(),
+		Hostname:       "node",
+		Endpoints:      endpoints,
+		RegisterMethod: util.RegisterMethodCLI,
+	})
+	require.NoError(t, err)
+	require.Equal(t, endpoints, node.Endpoints().AsSlice(),
+		"precondition: node has live endpoints")
+
+	// Node re-authenticates, rotating its NodeKey. The RegisterRequest carries
+	// no endpoints.
+	updated, err := s.applyAuthNodeUpdate(authNodeUpdateParams{
+		ExistingNode: node,
+		RegData: &types.RegistrationData{
+			MachineKey: machine.Public(),
+			NodeKey:    key.NewNode().Public(),
+			Hostname:   "node",
+			Hostinfo:   &tailcfg.Hostinfo{},
+			Endpoints:  nil,
+		},
+		ValidHostinfo:  &tailcfg.Hostinfo{},
+		Hostname:       "node",
+		User:           user,
+		RegisterMethod: util.RegisterMethodCLI,
+	})
+	require.NoError(t, err)
+
+	assert.Equal(t, endpoints, updated.Endpoints().AsSlice(),
+		"re-auth without reported endpoints must preserve the node's live endpoints")
+}
+
+// TestReauthChange covers the decision both re-auth paths share: a same-user
+// relogin must be an incremental peer patch (so the tailscale client takes its
+// fast patch path), never a whole-node add (which strands a re-keyed,
+// momentarily-endpoint-less peer disco-deaf); a policy change forces a full
+// recompute; a new node is a whole-node add.
+func TestReauthChange(t *testing.T) {
+	n := types.Node{
+		ID:       7,
+		NodeKey:  key.NewNode().Public(),
+		DiscoKey: key.NewDisco().Public(),
+	}
+	node := n.View()
+
+	relogin := reauthChange(node, true, false)
+	assert.Len(t, relogin.PeerPatches, 1, "relogin must be a peer patch")
+	assert.Empty(t, relogin.PeersChanged, "relogin must not be a whole-node add")
+
+	added := reauthChange(node, false, false)
+	assert.Empty(t, added.PeerPatches)
+	assert.Len(t, added.PeersChanged, 1, "a new node must be a whole-node add")
+
+	pol := reauthChange(node, true, true)
+	assert.Empty(t, pol.PeerPatches, "a policy change must not be a peer patch")
+	assert.Empty(t, pol.PeersChanged)
+	assert.False(t, pol.IsEmpty(), "a policy change must be non-empty")
+}
@@ -1615,7 +1615,14 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView
 			params.ValidHostinfo,
 		)

-		node.Endpoints = regData.Endpoints
+		// Preserve the node's live endpoints when the register request carried
+		// none. Web/OIDC relogins report endpoints via MapRequest, not register,
+		// so RegData.Endpoints is empty; clearing the stored set would advertise
+		// the re-keyed node with no way for peers to reach it. The first
+		// MapRequest restores the live set.
+		if len(regData.Endpoints) > 0 {
+			node.Endpoints = regData.Endpoints
+		}
 		// Do NOT reset IsOnline here. Online status is managed exclusively by
 		// [State.Connect]/[State.Disconnect] in the poll session lifecycle.
 		// Resetting it during re-registration causes a false offline blip: the
@@ -2115,14 +2122,12 @@ func (s *State) HandleNodeFromAuthPath(
 		return finalNode, change.NodeAdded(finalNode.ID()), fmt.Errorf("updating policy manager nodes: %w", err)
 	}

-	var c change.Change
-	if !usersChange.IsEmpty() || !nodesChange.IsEmpty() {
-		c = change.PolicyChange()
-	} else {
-		c = change.NodeAdded(finalNode.ID())
-	}
+	policyChanged := !usersChange.IsEmpty() || !nodesChange.IsEmpty()

-	return finalNode, c, nil
+	// nodeExistsForSameUser is true only for a same-user relogin; a tag->user
+	// conversion is excluded, as it changes the peer's User — a structural
+	// change peers must see in full, not a key-rotation patch.
+	return finalNode, reauthChange(finalNode, nodeExistsForSameUser, policyChanged), nil
 }

 // createNewNodeFromAuth creates a new node during auth callback.
@@ -2448,14 +2453,27 @@ func (s *State) HandleNodeFromPreAuthKey(
 		return finalNode, change.NodeAdded(finalNode.ID()), fmt.Errorf("updating policy manager nodes: %w", err)
 	}

-	var c change.Change
-	if !usersChange.IsEmpty() || !nodesChange.IsEmpty() {
-		c = change.PolicyChange()
-	} else {
-		c = change.NodeAdded(finalNode.ID())
-	}
+	policyChanged := !usersChange.IsEmpty() || !nodesChange.IsEmpty()

-	return finalNode, c, nil
+	return finalNode, reauthChange(finalNode, existsSameUser, policyChanged), nil
+}
+
+// reauthChange returns the [change.Change] to broadcast after an authentication
+// that updated or created a node.
+//
+// A pure relogin (isRelogin: an existing node, same user, with only its NodeKey
+// rotated) is sent as a minimal incremental peer patch via [change.NodeKeyRotated]
+// rather than re-advertising the whole node. A policy change forces a full
+// recompute; any other (new) node is a whole-node add.
+func reauthChange(node types.NodeView, isRelogin, policyChanged bool) change.Change {
+	switch {
+	case policyChanged:
+		return change.PolicyChange()
+	case isRelogin:
+		return change.NodeKeyRotated(node)
+	default:
+		return change.NodeAdded(node.ID())
+	}
 }

 // updatePolicyManagerUsers updates the policy manager with current users.
@@ -2656,8 +2674,13 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 	updatedNode, ok := s.nodeStore.UpdateNode(id, func(currentNode *types.Node) {
 		peerChange := currentNode.PeerChangeFromMapRequest(req)

-		// Track what specifically changed
-		endpointChanged = peerChange.Endpoints != 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)

@@ -2841,6 +2864,63 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 	return buildMapRequestChangeResponse(id, updatedNode, hostinfoChanged, endpointChanged, derpChanged)
 }

+// endpointBroadcastWorthy reports whether an endpoint-only delta is worth
+// fanning out to peers as an incremental PeersChangedPatch. A delta that only
+// adds STUN-derived endpoints — or only removes endpoints — is suppressed:
+// bare STUN endpoints are unlikely to be open and churn a lot (the client
+// re-derives those paths over disco anyway), and a pure shrink is not worth
+// 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.
+//
+// Limitation: headscale stores bare []netip.AddrPort with no per-endpoint
+// type, so we can only classify the *new* request's endpoints (via the
+// parallel newTypes slice). We therefore gate on whether any newly-added
+// endpoint (present in new, absent from stored) is useful (non-STUN). When
+// newTypes is absent or shorter than newEPs (older clients), the unknown
+// endpoints are treated as useful, preserving the pre-existing always-broadcast
+// behaviour and never hiding a genuinely new endpoint.
+func endpointBroadcastWorthy(
+	stored, newEPs []netip.AddrPort,
+	newTypes []tailcfg.EndpointType,
+) bool {
+	storedSet := make(map[netip.AddrPort]struct{}, len(stored))
+	for _, ep := range stored {
+		storedSet[ep] = struct{}{}
+	}
+
+	for i, ep := range newEPs {
+		if _, ok := storedSet[ep]; ok {
+			// Already known to peers; not a newly-added endpoint.
+			continue
+		}
+
+		// A newly-added endpoint with no type information (older client)
+		// is treated as useful so we never hide a genuinely new endpoint.
+		t := tailcfg.EndpointUnknownType
+		if i < len(newTypes) {
+			t = newTypes[i]
+		}
+
+		if isUsefulEndpointType(t) {
+			return true
+		}
+	}
+
+	return false
+}
+
+// isUsefulEndpointType reports whether an endpoint type is worth eagerly
+// broadcasting to peers. STUN-derived endpoints are excluded because they are
+// churny and unlikely to be directly reachable; magicsock's disco handles
+// establishing those paths.
+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.
 func buildMapRequestChangeResponse(