diff --git a/hscontrol/servertest/mapstream_compat_test.go b/hscontrol/servertest/mapstream_compat_test.go index ed7d5bf36..c798fe2cd 100644 --- a/hscontrol/servertest/mapstream_compat_test.go +++ b/hscontrol/servertest/mapstream_compat_test.go @@ -62,19 +62,6 @@ var mapStreamSkipReasons = map[string]string{ "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, @@ -107,10 +94,6 @@ 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, } @@ -118,8 +101,6 @@ var mapStreamSkipReasons = map[string]string{ 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" 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 7a27ff391..b7bcbbb0e 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -530,8 +530,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, @@ -540,24 +541,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) @@ -577,20 +582,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 2e1b6910a..d2d1701b8 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -771,9 +771,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 @@ -791,17 +798,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)) } @@ -810,7 +822,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()) } @@ -2869,7 +2881,7 @@ func (s *State) HandleNodeFromAuthPath( var ( finalNode types.NodeView before types.NodeView - keysOnly bool + changed relogin ) if nodeExists && !existingNode.IsTagged() { @@ -2880,7 +2892,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 @@ -2939,10 +2951,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 { @@ -3225,7 +3237,7 @@ func (s *State) HandleNodeFromPreAuthKey( var ( finalNode types.NodeView - keysOnly bool + changed relogin ) var before types.NodeView @@ -3240,7 +3252,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 @@ -3340,7 +3352,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 @@ -3644,32 +3656,61 @@ func (s *State) reregisterNodeWithPAK( return updatedNodeView, before, nil } +// 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