diff --git a/hscontrol/mapper/batcher_concurrency_test.go b/hscontrol/mapper/batcher_concurrency_test.go index 0ced070d2..066d1ac43 100644 --- a/hscontrol/mapper/batcher_concurrency_test.go +++ b/hscontrol/mapper/batcher_concurrency_test.go @@ -273,7 +273,7 @@ func TestAddToBatchDifferential(t *testing.T) { lb.b.nodes.Store(99, nil) } - endpoint := change.EndpointOrDERPUpdate(1, &tailcfg.PeerChange{ + endpoint := change.EndpointOrDERPUpdate(&tailcfg.PeerChange{ NodeID: 1, DERPRegion: 7, Online: new(true), }) ping := change.PingNode(2, &tailcfg.PingRequest{URL: "https://example.com/one"}) diff --git a/hscontrol/servertest/mapstream_compat_test.go b/hscontrol/servertest/mapstream_compat_test.go index 39d3b2fd0..c798fe2cd 100644 --- a/hscontrol/servertest/mapstream_compat_test.go +++ b/hscontrol/servertest/mapstream_compat_test.go @@ -59,26 +59,9 @@ var mapStreamSkipReasons = map[string]string{ "map-delivery/S3.5-remove/db1/frames": policyResponseToEveryNode, "map-delivery/S3.5-restore/db1/frames": policyResponseToEveryNode, - "map-delivery/S2.1/laptop/frames": ownReportEchoed, - "map-delivery/S2.3/laptop/frames": ownReportEchoed, - "map-delivery/S2.5/laptop/self-endpoints": ownReportEchoed, - "map-delivery/S2.5/laptop/endpoints-gap-bystander": restartEndpointsKept, "map-delivery/S2.5/laptop/endpoints-gap-svc": restartEndpointsKept, - "map-delivery/S3.1/appc/frames": reconnectAnnounced, - "map-delivery/S3.1/bystander/frames": reconnectAnnounced, - "map-delivery/S3.1/router/frames": reconnectAnnounced, - "map-delivery/S3.1/sshd/frames": reconnectAnnounced, - "map-delivery/S3.1/svc/frames": reconnectAnnounced, - - "map-delivery/S3.4/appc/frames": routerReconnectRecompute, - "map-delivery/S3.4/bystander/frames": routerReconnectRecompute, - "map-delivery/S3.4/db1/frames": routerReconnectRecompute, - "map-delivery/S3.4/laptop/frames": routerReconnectRecompute, - "map-delivery/S3.4/sshd/frames": routerReconnectRecompute, - "map-delivery/S3.4/svc/frames": routerReconnectRecompute, - "map-delivery/S3.6/bystander/peers": sameUserPeers, "map-delivery/S3.6/laptop/peers": sameUserPeers, @@ -111,20 +94,13 @@ var mapStreamSkipReasons = map[string]string{ "policy-generation/G4/thor-1/peer-owners": sameUserPeers, "policy-generation/G4/thor-2/peer-owners": sameUserPeers, - "user-role/S1-reconnect/own-a/frames": reconnectAnnounced, - "user-role/S1-reconnect/own-b/frames": reconnectAnnounced, - "user-role/S2-reconnect/own-a/frames": reconnectAnnounced, - "user-role/S0/tag-a/device-user": taggedDeviceUser, "user-role/S0/tag-b/device-user": taggedDeviceUser, } const ( policyResponseToEveryNode = "POLICY_RESPONSE_TO_EVERY_NODE: headscale sends every connected node a policy response when the policy, a node's tags or approved routes, or the set of nodes change; SaaS sends only the nodes whose peers or filter change" - ownReportEchoed = "OWN_REPORT_ECHOED: headscale sends a node its own endpoint or DERP report back; SaaS sends it nothing, so its own entry keeps the endpoints of its last full map" restartEndpointsKept = "RESTART_ENDPOINTS_KEPT: headscale keeps a restarted node's last endpoints at its peers until it reports new ones; SaaS sends peers the node without endpoints until then" - reconnectAnnounced = "RECONNECT_ANNOUNCED: headscale tells peers a node is online after a stream gap of a few seconds; SaaS sends them nothing" - routerReconnectRecompute = "ROUTER_RECONNECT_RECOMPUTE: headscale sends every node a policy response when a subnet router's stream comes back after a few seconds; SaaS sends nothing" taggedDeviceUser = "TAGGED_DEVICE_USER: SaaS shows a tagged device's user as the login that made its key; headscale shows tagged-devices" ) diff --git a/hscontrol/state/connect_test.go b/hscontrol/state/connect_test.go index 71fc06c30..4af3d36e8 100644 --- a/hscontrol/state/connect_test.go +++ b/hscontrol/state/connect_test.go @@ -210,6 +210,40 @@ func TestConnectDisconnectSubnetRouterEmitsPolicyChangeNotFull(t *testing.T) { }) } +// TestReconnectWhileOnlineAnnouncesNothing pins that a stream which comes +// back while an earlier session still holds the node online, as in the +// reconnect grace, tells peers nothing and recomputes nothing, as on Tailscale +// SaaS; once the node went offline, the next connect announces it again. +func TestReconnectWhileOnlineAnnouncesNothing(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) + + cs, first := s.Connect(nodeID) + require.True(t, forcesPeerRecompute(cs)) + + cs, second := s.Connect(nodeID) + assert.Empty(t, cs, "a reconnect while online must change nothing peers see") + + cs, err := s.Disconnect(nodeID, first) + require.NoError(t, err) + require.Empty(t, cs) + + cs, err = s.Disconnect(nodeID, second) + require.NoError(t, err) + require.True(t, hasPeerPatch(cs)) + + cs, _ = s.Connect(nodeID) + assert.True(t, hasPeerPatch(cs), "a node that went offline must be announced online") + assert.True(t, forcesPeerRecompute(cs)) +} + // drainSessions releases poll sessions on nodeID until the node goes // offline, returning the changes from the final release (the one that // emits the offline notifications). Fails the test if the node is diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 5d78674d9..7bd910b4d 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -528,8 +528,9 @@ func TestReauthPreservesEndpointsWhenClientOmitsThem(t *testing.T) { // TestReauthChange covers the decision both re-auth paths share: a keys-only // 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. +// momentarily-endpoint-less peer disco-deaf); a relogin peers read as before +// sends nothing; 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, @@ -538,24 +539,28 @@ func TestReauthChange(t *testing.T) { } 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") + keys := reauthChange(node, reloginKeys, false) + assert.Len(t, keys.PeerPatches, 1, "relogin must be a peer patch") + assert.Empty(t, keys.PeersChanged, "relogin must not be a whole-node add") - added := reauthChange(node, false, false) + assert.True(t, reauthChange(node, reloginNone, false).IsEmpty(), + "a relogin peers read as before must send nothing") + + added := reauthChange(node, reloginWhole, 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) + pol := reauthChange(node, reloginKeys, 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") } -// TestOnlyKeysChanged covers which relogins may ride the key-rotation patch: +// TestReloginChanged covers which relogins may ride the key-rotation patch: // one that flips Expired or changes Hostinfo peers read must not, as -// [tailcfg.PeerChange] carries neither (#3531). -func TestOnlyKeysChanged(t *testing.T) { +// [tailcfg.PeerChange] carries neither (#3531); one on the same keys and +// expiry needs nothing at all. +func TestReloginChanged(t *testing.T) { now := time.Now() past, future := now.Add(-time.Minute), now.Add(time.Hour) @@ -575,20 +580,27 @@ func TestOnlyKeysChanged(t *testing.T) { withOSVersion.OSVersion = "6.1" withService.Services = []tailcfg.Service{{Proto: tailcfg.PeerAPI4, Port: 4242}} + same := node(future, hi) + renewedNode := same.AsStruct() + renewedNode.Expiry = new(future.Add(time.Hour)) + renewed := renewedNode.View() + tests := []struct { name string before, after types.NodeView - want bool + want relogin }{ - {name: "keys", before: node(future, hi), after: node(future, hi), want: true}, - {name: "hostinfo peers do not read", before: node(future, hi), after: node(future, withOSVersion), want: true}, - {name: "un-expired", before: node(past, hi), after: node(future, hi)}, - {name: "hostinfo peers read", before: node(future, hi), after: node(future, withService)}, + {name: "keys", before: node(future, hi), after: node(future, hi), want: reloginKeys}, + {name: "hostinfo peers do not read", before: node(future, hi), after: node(future, withOSVersion), want: reloginKeys}, + {name: "un-expired", before: node(past, hi), after: node(future, hi), want: reloginWhole}, + {name: "hostinfo peers read", before: node(future, hi), after: node(future, withService), want: reloginWhole}, + {name: "same keys", before: same, after: same, want: reloginNone}, + {name: "same keys, new expiry", before: same, after: renewed, want: reloginKeys}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, onlyKeysChanged(tt.before, tt.after, now)) + assert.Equal(t, tt.want, reloginChanged(tt.before, tt.after, now)) }) } } diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 94c34b315..7a5a8211a 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -826,9 +826,16 @@ func (s *State) Connect(id types.NodeID) ([]change.Change, uint64) { // Reconnecting clears Unhealthy: the node just proved basic // connectivity by completing the Noise handshake. - var epoch uint64 + var ( + epoch uint64 + wasOnline bool + ) node, ok := s.nodeStore.UpdateNode(id, func(n *types.Node) { + // A stream that comes back within the reconnect grace finds the + // node still online: peers saw no change, as on Tailscale SaaS. + wasOnline = n.IsOnline != nil && *n.IsOnline + n.SessionEpoch++ epoch = n.SessionEpoch @@ -846,17 +853,22 @@ func (s *State) Connect(id types.NodeID) ([]change.Change, uint64) { return nil, 0 } + log.Info().EmbedObject(node).Msg("node connected") + // A node coming online sends a lightweight online peer patch. Subnet // routers, relay targets, and via targets get their full peer recompute // from the gated PolicyChange below, so no full update is needed here. - c := []change.Change{change.NodeOnline(node.ID())} - if !node.Online() { - c[0] = change.NodeAdded(node.ID()) + var c []change.Change + + switch { + case !node.Online(): + c = append(c, change.NodeAdded(node.ID())) + case !wasOnline: + c = append(c, change.NodeOnline(node.ID())) } - log.Info().EmbedObject(node).Msg("node connected") - - if !maps.Equal(prevRoutes, s.nodeStore.PrimaryRoutes()) { + routesMoved := !maps.Equal(prevRoutes, s.nodeStore.PrimaryRoutes()) + if routesMoved { c = append(c, change.NodeAdded(id)) } @@ -865,7 +877,7 @@ func (s *State) Connect(id types.NodeID) ([]change.Change, uint64) { // 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) { + if (!wasOnline || routesMoved) && s.polMan.NodeNeedsPeerRecompute(node) { c = append(c, change.PolicyChange()) } @@ -2964,7 +2976,7 @@ func (s *State) HandleNodeFromAuthPath( var ( finalNode types.NodeView before types.NodeView - keysOnly bool + changed relogin ) if nodeExists && !existingNode.IsTagged() { @@ -2975,7 +2987,7 @@ func (s *State) HandleNodeFromAuthPath( return types.NodeView{}, s.policyChangeSince(genBefore), err } - keysOnly = onlyKeysChanged(before, finalNode, time.Now()) + changed = reloginChanged(before, finalNode, time.Now()) } else if nodeExists { updateParams.ExistingNode = existingNode updateParams.IsConvertFromTag = true @@ -3034,10 +3046,10 @@ func (s *State) HandleNodeFromAuthPath( policyChanged := !usersChange.IsEmpty() || !nodesChange.IsEmpty() - // keysOnly is set only for a same-user relogin; a tag->user conversion is + // changed is set 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 s.approveRoutesAfterRegistration(before, finalNode, reauthChange(finalNode, keysOnly, policyChanged)) + return s.approveRoutesAfterRegistration(before, finalNode, reauthChange(finalNode, changed, policyChanged)) } func sortedPrefixes(p []netip.Prefix) []netip.Prefix { @@ -3321,7 +3333,7 @@ func (s *State) HandleNodeFromPreAuthKey( var ( finalNode types.NodeView - keysOnly bool + changed relogin ) var before types.NodeView @@ -3336,7 +3348,7 @@ func (s *State) HandleNodeFromPreAuthKey( return types.NodeView{}, s.policyChangeSince(genBefore), err } - keysOnly = onlyKeysChanged(before, finalNode, time.Now()) + changed = reloginChanged(before, finalNode, time.Now()) } else { // Node does not exist for this user with this machine key. // For a user-owned key, check whether the machine key is already held @@ -3436,7 +3448,7 @@ func (s *State) HandleNodeFromPreAuthKey( policyChanged := !usersChange.IsEmpty() || !nodesChange.IsEmpty() - return s.approveRoutesAfterRegistration(before, finalNode, reauthChange(finalNode, keysOnly, policyChanged)) + return s.approveRoutesAfterRegistration(before, finalNode, reauthChange(finalNode, changed, policyChanged)) } // approveRoutesAfterRegistration runs route auto-approval after a @@ -3747,32 +3759,61 @@ func (s *State) undoReregistration(prior, written *types.Node) { } } +// relogin is what a same-user relogin changed that its peers read. +type relogin int + +const ( + // reloginWhole: something [change.NodeKeyRotated] cannot patch. + reloginWhole relogin = iota + // reloginKeys: only what [change.NodeKeyRotated] patches. + reloginKeys + // reloginNone: nothing, as when a client logs in again on its node key; + // Tailscale SaaS sends its peers nothing then. + reloginNone +) + // reauthChange returns the [change.Change] to broadcast after an authentication -// that updated or created a node. keysOnly marks a same-user relogin that -// changed nothing peers read besides its keys; see [onlyKeysChanged]. +// that updated or created a node; r is [reloginWhole] for any but a same-user +// relogin. // -// Such a relogin is sent as the minimal peer patch [change.NodeKeyRotated]. -// Anything else is a whole-node add, and a policy change forces a full -// recompute. -func reauthChange(node types.NodeView, keysOnly, policyChanged bool) change.Change { +// A keys-only relogin is sent as the minimal peer patch +// [change.NodeKeyRotated]. Anything else is a whole-node add, and a policy +// change forces a full recompute. +func reauthChange(node types.NodeView, r relogin, policyChanged bool) change.Change { switch { case policyChanged: return change.PolicyChange() - case keysOnly: + case r == reloginNone: + return change.Change{} + case r == reloginKeys: return change.NodeKeyRotated(node) default: return change.NodeAdded(node.ID()) } } -// onlyKeysChanged reports whether peers read before and after the same, apart -// from what [change.NodeKeyRotated] patches. [tailcfg.PeerChange] carries -// neither [tailcfg.Node.Expired] nor Hostinfo: peers left holding an expired -// node drop its WireGuard handshakes, and the relogin stored the Hostinfo, so -// no later MapRequest shows the change. -func onlyKeysChanged(before, after types.NodeView, now time.Time) bool { - return before.IsExpiredAt(now) == after.IsExpiredAt(now) && - peerHostinfoEqual(before.Hostinfo(), after.Hostinfo()) +// reloginChanged compares what peers read before and after a relogin. +// [tailcfg.PeerChange] carries neither [tailcfg.Node.Expired] nor Hostinfo: +// peers left holding an expired node drop its WireGuard handshakes, and the +// relogin stored the Hostinfo, so no later MapRequest shows the change. +func reloginChanged(before, after types.NodeView, now time.Time) relogin { + expiry := func(n types.NodeView) time.Time { + e, _ := n.Expiry().GetOk() + + return e + } + + switch { + case before.IsExpiredAt(now) != after.IsExpiredAt(now) || + !peerHostinfoEqual(before.Hostinfo(), after.Hostinfo()): + return reloginWhole + case before.NodeKey() == after.NodeKey() && before.DiscoKey() == after.DiscoKey() && + expiry(before).Equal(expiry(after)) && + views.SliceEqual(before.Endpoints(), after.Endpoints()): + return reloginNone + default: + return reloginKeys + } } // updatePolicyManagerUsers pushes the current user list into the policy @@ -4302,7 +4343,7 @@ func buildMapRequestChangeResponse( patch.DERPRegion = currentDERP } - return change.EndpointOrDERPUpdate(id, patch) + return change.EndpointOrDERPUpdate(patch) } return change.Change{} diff --git a/hscontrol/types/change/change.go b/hscontrol/types/change/change.go index d9053b3fd..4e1561ffc 100644 --- a/hscontrol/types/change/change.go +++ b/hscontrol/types/change/change.go @@ -595,13 +595,12 @@ func KeyExpiryFor(id types.NodeID, expiry time.Time) Change { return c } -// EndpointOrDERPUpdate returns a [Change] for when a node's endpoints or DERP region changes. -// The [Change.OriginNode] field enables self-update detection by the mapper. -func EndpointOrDERPUpdate(id types.NodeID, patch *tailcfg.PeerChange) Change { - c := PeerPatched("endpoint/DERP update", patch) - c.OriginNode = id - - return c +// EndpointOrDERPUpdate returns a [Change] for when a node reports new +// endpoints or a new DERP region. It has no [Change.OriginNode]: the node +// already knows what it reported, so, as on Tailscale SaaS, only its peers +// are sent the patch. +func EndpointOrDERPUpdate(patch *tailcfg.PeerChange) Change { + return PeerPatched("endpoint/DERP update", patch) } // NodeKeyRotated returns a [Change] for a node re-logging in: its NodeKey (and