diff --git a/hscontrol/poll.go b/hscontrol/poll.go index 929d06a1..689669d9 100644 --- a/hscontrol/poll.go +++ b/hscontrol/poll.go @@ -182,12 +182,12 @@ func (m *mapSession) serveLongPoll() { // handler ran late is exactly such a session: if it kept its session // acquired on this path, the surviving session's release could never // take the node offline (the relogin flake). - // A deleted node cannot reconnect, so waiting for it only delays the - // client's next map request, and with it the re-authentication signal - // it needs. See: https://github.com/juanfont/headscale/issues/3410 - _, nodeExists := m.h.state.GetNodeByID(m.node.ID) + // A deleted or expired node cannot return online through a map + // reconnect, so release its session without the reconnect grace. + // See: https://github.com/juanfont/headscale/issues/3410 + node, nodeExists := m.h.state.GetNodeByID(m.node.ID) - if !stillConnected && nodeExists { + if !stillConnected && nodeExists && !node.IsExpired() { // Wait up to 10 seconds for the node to reconnect. // 10 seconds was arbitrary chosen as a reasonable time to reconnect. ticker := time.NewTicker(time.Second) diff --git a/hscontrol/servertest/lifecycle_test.go b/hscontrol/servertest/lifecycle_test.go index c1531550..eefc7e92 100644 --- a/hscontrol/servertest/lifecycle_test.go +++ b/hscontrol/servertest/lifecycle_test.go @@ -4,12 +4,14 @@ import ( "context" "fmt" "math/rand/v2" + "net/netip" "strings" "testing" "time" "github.com/juanfont/headscale/hscontrol/servertest" "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/types/change" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "tailscale.com/types/netmap" @@ -122,6 +124,214 @@ func TestConnectionLifecycle(t *testing.T) { }) } +// TestNodeExpiryPreservesControlConnection exercises the same-key map repoll +// that controlclient.Auto performs while the backend is in NeedsLogin. +func TestNodeExpiryPreservesControlConnection(t *testing.T) { + t.Parallel() + + for _, scheduled := range []bool{false, true} { + t.Run(fmt.Sprintf("scheduled=%v", scheduled), func(t *testing.T) { + t.Parallel() + h := servertest.NewHarness(t, 2, + servertest.WithServerOptions(servertest.WithBatchDelay(10*time.Millisecond)), + ) + client, observer := h.Client(0), h.Client(1) + id := findNodeID(t, h.Server, client.Name) + node, ok := h.Server.State().GetNodeByID(id) + require.True(t, ok) + + epoch, nodeKey := node.SessionEpoch(), node.NodeKey() + require.True(t, node.IsOnline().Get()) + + lastCheck := time.Now() + + expiry := lastCheck + if scheduled { + expiry = lastCheck.Add(time.Second) + } + + node, c, err := h.Server.State().SetNodeExpiry(id, &expiry) + require.NoError(t, err) + require.Equal(t, scheduled, node.IsOnline().Get()) + h.Server.App.Change(c) + + if scheduled { + require.Eventually(t, func() bool { return time.Now().After(expiry) }, + 3*time.Second, 10*time.Millisecond) + + _, changes, changed := h.Server.State().ExpireExpiredNodes(lastCheck) + require.True(t, changed) + h.Server.App.Change(changes...) + } + + require.EventuallyWithT(t, func(c *assert.CollectT) { + current, found := h.Server.State().GetNodeByID(id) + if !assert.True(c, found) { + return + } + + assert.False(c, current.IsOnline().Get()) + assert.Equal(c, 1, current.ActiveSessions()) + assert.Equal(c, epoch, current.SessionEpoch()) + assert.True(c, h.Server.App.MapBatcher().IsConnected(id)) + assert.True(c, client.Netmap().SelfNode.Expired()) + assert.False(c, client.Netmap().SelfNode.Online().Get()) + + peer, found := observer.PeerByName(client.Name) + if assert.True(c, found) { + assert.True(c, peer.Expired()) + assert.False(c, peer.Online().Get()) + } + }, 5*time.Second, 10*time.Millisecond, "expiry must take the node offline without ending its control session") + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + require.NoError(t, client.RestartPoll(ctx)) + require.EventuallyWithT(t, func(c *assert.CollectT) { + current, found := h.Server.State().GetNodeByID(id) + if !assert.True(c, found) { + return + } + + assert.Greater(c, current.SessionEpoch(), epoch) + assert.Equal(c, 1, current.ActiveSessions()) + assert.Equal(c, nodeKey, current.NodeKey()) + assert.False(c, current.IsOnline().Get(), "an expired-key repoll cannot bring a node online") + assert.True(c, h.Server.App.MapBatcher().IsConnected(id)) + }, 5*time.Second, 10*time.Millisecond, "expired-key repoll must preserve offline status") + + // Restoring expiry must reach the client through that same stream. + current, _ := h.Server.State().GetNodeByID(id) + epoch = current.SessionEpoch() + _, c, err = h.Server.State().SetNodeExpiry(id, nil) + require.NoError(t, err) + h.Server.App.Change(c) + require.EventuallyWithT(t, func(c *assert.CollectT) { + current, found := h.Server.State().GetNodeByID(id) + if !assert.True(c, found) { + return + } + + assert.True(c, current.IsOnline().Get()) + assert.Equal(c, epoch, current.SessionEpoch()) + assert.Equal(c, nodeKey, current.NodeKey()) + assert.Equal(c, 1, current.ActiveSessions()) + assert.False(c, client.Netmap().SelfNode.Expired()) + assert.True(c, client.Netmap().SelfNode.Online().Get()) + + peer, found := observer.PeerByName(client.Name) + if assert.True(c, found) { + assert.False(c, peer.Expired()) + assert.True(c, peer.Online().Get()) + } + }, 5*time.Second, 10*time.Millisecond, "restoring expiry must recover both clients without another login or poll") + }) + } +} + +func TestRestoredExpirySurvivesQueuedChanges(t *testing.T) { + t.Parallel() + + for _, full := range []bool{false, true} { + t.Run(fmt.Sprintf("full=%v", full), func(t *testing.T) { + t.Parallel() + h := servertest.NewHarness(t, 1, + servertest.WithServerOptions(servertest.WithBatchDelay(10*time.Millisecond)), + ) + client := h.Client(0) + node := h.Server.State().ListNodes().At(0) + past := time.Now() + _, expired, err := h.Server.State().SetNodeExpiry(node.ID(), &past) + require.NoError(t, err) + + future := past.Add(time.Hour) + _, restored, err := h.Server.State().SetNodeExpiry(node.ID(), &future) + require.NoError(t, err) + + changes := []change.Change{expired, restored} + if full { + changes = append(changes, change.FullUpdate()) + } + + h.Server.App.Change(changes...) + client.WaitForCondition(t, "restored expiry delivered", 5*time.Second, + func(nm *netmap.NetworkMap) bool { return nm.SelfKeyExpiry().Equal(future) }) + // Observe beyond delivery: the old worker cancelled the stream only + // after sending its map, even when that map had the restored expiry. + require.Never(t, func() bool { return len(h.ConnectedClients()) == 0 }, + 200*time.Millisecond, 10*time.Millisecond, "a queued expiry must not close the restored stream") + + current, found := h.Server.State().GetNodeByID(node.ID()) + require.True(t, found) + require.True(t, current.IsOnline().Get()) + require.Equal(t, node.SessionEpoch(), current.SessionEpoch()) + }) + } +} + +func TestNodeExpiryRouteFailover(t *testing.T) { + t.Parallel() + + for _, scheduled := range []bool{false, true} { + t.Run(fmt.Sprintf("scheduled=%v", scheduled), func(t *testing.T) { + t.Parallel() + h := servertest.NewHarness(t, 3, + servertest.WithServerOptions(servertest.WithBatchDelay(10*time.Millisecond)), + ) + route := netip.MustParsePrefix("10.70.0.0/24") + primary := advertiseAndApproveRoute(t, h.Server, h.Client(0), route) + standby := advertiseAndApproveRoute(t, h.Server, h.Client(1), route) + require.Contains(t, h.Server.State().GetNodePrimaryRoutes(primary), route) + + lastCheck := time.Now() + + expiry := lastCheck + if scheduled { + expiry = lastCheck.Add(time.Second) + } + + _, c, err := h.Server.State().SetNodeExpiry(primary, &expiry) + require.NoError(t, err) + h.Server.App.Change(c) + + if scheduled { + require.Eventually(t, func() bool { return time.Now().After(expiry) }, + 3*time.Second, 10*time.Millisecond) + + _, changes, _ := h.Server.State().ExpireExpiredNodes(lastCheck) + h.Server.App.Change(changes...) + } + + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Empty(c, h.Server.State().GetNodePrimaryRoutes(primary)) + assert.Contains(c, h.Server.State().GetNodePrimaryRoutes(standby), route) + assert.True(c, h.Server.App.MapBatcher().IsConnected(primary)) + + peer, found := h.Client(2).PeerByName(h.Client(1).Name) + if assert.True(c, found) { + assert.Contains(c, peer.PrimaryRoutes().AsSlice(), route) + } + }, 5*time.Second, 10*time.Millisecond, "expiry must move the route to the standby while preserving control connectivity") + + _, c, err = h.Server.State().SetNodeExpiry(primary, nil) + require.NoError(t, err) + h.Server.App.Change(c) + _, c, err = h.Server.State().SetNodeExpiry(standby, &expiry) + require.NoError(t, err) + h.Server.App.Change(c) + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Contains(c, h.Server.State().GetNodePrimaryRoutes(primary), route) + + peer, found := h.Client(2).PeerByName(h.Client(0).Name) + if assert.True(c, found) { + assert.Contains(c, peer.PrimaryRoutes().AsSlice(), route) + } + }, 5*time.Second, 10*time.Millisecond, "restored router must be eligible for failover without reconnecting") + }) + } +} + // TestLogoutReloginAllClientsConverge is an in-process reproduction of the // flaky integration tests TestAuthKeyLogoutAndReloginSameUser, // TestAuthWebFlowLogoutAndReloginSameUser and diff --git a/hscontrol/state/connect_test.go b/hscontrol/state/connect_test.go index 235d092c..b41c86ee 100644 --- a/hscontrol/state/connect_test.go +++ b/hscontrol/state/connect_test.go @@ -1,8 +1,10 @@ package state import ( + "fmt" "net/netip" "testing" + "time" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/types/change" @@ -282,3 +284,50 @@ func TestDisconnectOutOfOrderSessionsCannotStrandNodeOnline(t *testing.T) { require.True(t, known) assert.False(t, online, "node must be offline after its last session is released") } + +func TestExpiredNodeSessionAccounting(t *testing.T) { + for _, expiry := range []*time.Time{nil, new(time.Time{}), new(time.Now().Add(time.Hour))} { + t.Run(fmt.Sprint(expiry), func(t *testing.T) { + _, s, id := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + _, first := s.Connect(id) + past := time.Now() + node, _, err := s.SetNodeExpiry(id, &past) + require.NoError(t, err) + require.False(t, node.IsOnline().Get()) + require.Equal(t, 1, node.ActiveSessions()) + + changes, second := s.Connect(id) + require.Greater(t, second, first) + + for _, c := range changes { + for _, patch := range c.PeerPatches { + if patch.Online != nil { + require.False(t, *patch.Online) + } + } + } + + node, ok := s.GetNodeByID(id) + require.True(t, ok) + require.False(t, node.IsOnline().Get()) + require.Equal(t, 2, node.ActiveSessions()) + + _, err = s.Disconnect(id, second) + require.NoError(t, err) + + node, _, err = s.SetNodeExpiry(id, expiry) + require.NoError(t, err) + require.True(t, node.IsOnline().Get(), "restoring a key with a live session restores online state") + require.Equal(t, 1, node.ActiveSessions()) + + _, err = s.Disconnect(id, first) + require.NoError(t, err) + node, _, err = s.SetNodeExpiry(id, expiry) + require.NoError(t, err) + require.False(t, node.IsOnline().Get(), "restoring expiry cannot connect a disconnected node") + require.Zero(t, node.ActiveSessions()) + }) + } +} diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index 08d4ad0a..617a5007 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -210,10 +210,7 @@ func updateChanges(pre, post *types.Node) (bool, bool) { return true, true } - wasOnline := pre.IsOnline != nil && *pre.IsOnline - isOnline := post.IsOnline != nil && *post.IsOnline - - return false, wasOnline != isOnline || pre.Unhealthy != post.Unhealthy + return false, pre.Online() != post.Online() || pre.Unhealthy != post.Unhealthy } // PutNode adds or updates a node in the store. diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index ddb6af25..6703b39e 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -634,10 +634,11 @@ func (s *State) DeleteNode(node types.NodeView) (change.Change, error) { return c, nil } -// Connect marks a node connected and returns the resulting changes +// Connect acquires a control session and returns the resulting changes // plus a session epoch identifying this poll session. Every Connect // acquires one live session; the caller must release it with exactly // one [State.Disconnect] call once the session ends (see poll.go). +// An expired key can keep polling control, but cannot make the node online. func (s *State) Connect(id types.NodeID) ([]change.Change, uint64) { prevRoutes := s.nodeStore.PrimaryRoutes() @@ -649,7 +650,7 @@ func (s *State) Connect(id types.NodeID) ([]change.Change, uint64) { n.SessionEpoch++ epoch = n.SessionEpoch n.ActiveSessions++ - n.IsOnline = new(true) + n.IsOnline = new(n.ShouldBeOnline()) n.Unhealthy = false }) if !ok { @@ -660,6 +661,9 @@ func (s *State) Connect(id types.NodeID) ([]change.Change, uint64) { // 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()) + } log.Info().EmbedObject(node).Msg("node connected") @@ -680,7 +684,7 @@ func (s *State) Connect(id types.NodeID) ([]change.Change, uint64) { } // Disconnect releases one poll session previously acquired by -// [State.Connect] and marks the node offline only when that was its +// [State.Connect] and marks the node offline when that was its // last live session. Sessions are counted rather than compared by // epoch: overlapping sessions for one node — a rapid reconnect, or a // cancelled map request whose handler ran late — release in any order @@ -707,7 +711,7 @@ func (s *State) Disconnect(id types.NodeID, epoch uint64) ([]change.Change, erro now := time.Now() n.LastSeen = &now - n.IsOnline = new(false) + n.IsOnline = new(n.ShouldBeOnline()) // Offline nodes are not HA candidates; drop any stale // Unhealthy bit so it does not surface in DebugRoutes. n.Unhealthy = false @@ -721,7 +725,7 @@ func (s *State) Disconnect(id types.NodeID, epoch uint64) ([]change.Change, erro log.Debug(). Uint64("disconnect_epoch", epoch). Int("active_sessions", node.ActiveSessions()). - Msg("session released, other sessions keep node online") + Msg("session released, other control sessions remain") return nil, nil } @@ -906,13 +910,20 @@ func (s *State) ListEphemeralNodes() views.Slice[types.NodeView] { // SetNodeExpiry updates the expiration time for a node. // If expiry is nil, the node's expiry is disabled (node will never expire). func (s *State) SetNodeExpiry(nodeID types.NodeID, expiry *time.Time) (types.NodeView, change.Change, error) { + var onlineChanged bool + // Update [NodeStore] before database to ensure consistency. The [NodeStore] update // is blocking and will be the source of truth for the batcher. The database update // must make the exact same change. If the database update fails, the [NodeStore] // change will remain, but since we return an error, no change notification will be // sent to the batcher, preventing inconsistent state propagation. n, ok := s.nodeStore.UpdateNode(nodeID, func(node *types.Node) { + wasOnline := node.Online() node.Expiry = expiry + // Control stays connected in NeedsLogin so an expiry extension can + // recover the client, but an expired key is not online. + node.IsOnline = new(node.ShouldBeOnline()) + onlineChanged = wasOnline != node.Online() }) if !ok { @@ -931,8 +942,11 @@ func (s *State) SetNodeExpiry(nodeID types.NodeID, expiry *time.Time) (types.Nod return n, change.Change{}, fmt.Errorf("updating policy manager after setting expiry: %w", err) } - if c.IsEmpty() { - c = change.NodeAdded(n.ID()) + // Resolve expiry and online status together from the current snapshot + // when the mapper sends the change, including after a rapid restoration. + c = c.Merge(change.NodeAdded(n.ID())) + if onlineChanged && s.polMan.NodeNeedsPeerRecompute(n) { + c = c.Merge(change.PolicyChange()) } return n, c, nil @@ -1113,7 +1127,8 @@ func (s *State) ExpireExpiredNodes(lastCheck time.Time) (time.Time, []change.Cha // while this function is running by using a consistent timestamp for the next check started := time.Now() - var updates []change.Change + nodeUpdates := make(map[types.NodeID]UpdateNodeFunc) + expiredNodes := make(map[types.NodeID]bool) for _, node := range s.nodeStore.ListNodes().All() { //nolint:unqueryvet // NodeStore.ListNodes not a SQL query if !node.Valid() { @@ -1122,9 +1137,34 @@ func (s *State) ExpireExpiredNodes(lastCheck time.Time) (time.Time, []change.Cha // Why check After(lastCheck): We only want to notify about nodes that // expired since the last check to avoid duplicate notifications - if node.IsExpired() && node.Expiry().Valid() && node.Expiry().Get().After(lastCheck) { - updates = append(updates, change.KeyExpiryFor(node.ID(), node.Expiry().Get())) + if !node.IsExpired() || !node.Expiry().Get().After(lastCheck) { + continue } + + nodeUpdates[node.ID()] = func(n *types.Node) { + // The key may have been restored since the snapshot was read. + if !n.IsExpired() || !n.Expiry.After(lastCheck) { + return + } + + expiredNodes[n.ID] = n.Online() + n.IsOnline = new(n.ShouldBeOnline()) + } + } + + // Publish simultaneous expirations together so route election sees all + // unavailable nodes in one snapshot. + s.nodeStore.UpdateNodes(nodeUpdates) + + updates := make([]change.Change, 0, len(expiredNodes)) + + for id, wasOnline := range expiredNodes { + c := change.NodeAdded(id) + if current, ok := s.nodeStore.GetNode(id); ok && wasOnline && s.polMan.NodeNeedsPeerRecompute(current) { + c = c.Merge(change.PolicyChange()) + } + + updates = append(updates, c) } if len(updates) > 0 { @@ -1403,8 +1443,7 @@ var haHealthUpdates = promauto.NewCounterVec(prometheus.CounterOpts{ func healthSetter(healthy bool) UpdateNodeFunc { return func(n *types.Node) { if !healthy { - online := n.IsOnline != nil && *n.IsOnline - if !online || len(n.AllApprovedRoutes()) == 0 { + if !n.Online() || len(n.AllApprovedRoutes()) == 0 { haHealthUpdates.WithLabelValues("rejected").Inc() return @@ -1805,12 +1844,8 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView 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 - // change notification triggers a map regeneration showing the node as - // offline to peers, even though [State.Connect] will immediately set it - // back to true. + // Preserve online state during re-registration so a live node does + // not appear offline before the client restarts its map stream. node.LastSeen = new(time.Now()) // On conversion (tagged → user) we set the new register method. @@ -2696,10 +2731,8 @@ func (s *State) HandleNodeFromPreAuthKey( node.AuthKey = pak node.AuthKeyID = &pak.ID - // 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 - // to peers. + // Preserve online state during re-registration so a live node does + // not appear offline before the client restarts its map stream. node.LastSeen = new(time.Now()) // Tagged nodes keep their existing expiry (disabled). diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 78228124..1c1f2206 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -181,6 +181,8 @@ type Node struct { UpdatedAt time.Time DeletedAt *time.Time + // IsOnline caches [Node.ShouldBeOnline]; read it through [Node.Online]. + // Every writer must derive it, so online means the same thing everywhere. IsOnline *bool `gorm:"-"` // Unhealthy excludes the node from primary route election while @@ -189,10 +191,9 @@ type Node struct { // ActiveSessions counts live poll sessions for this node. // [State.Connect] increments it and every session release - // ([State.Disconnect]) decrements it, so the node goes offline - // exactly when its last session ends — regardless of the order in - // which overlapping sessions' cleanups run. Never persisted, like - // SessionEpoch. + // ([State.Disconnect]) decrements it. Releasing the last session + // takes the node offline; expiry can take it offline while sessions + // remain. Never persisted, like SessionEpoch. ActiveSessions int `gorm:"-"` // SessionEpoch identifies a poll session generation; Connect bumps @@ -228,6 +229,21 @@ func (node *Node) IsExpired() bool { return time.Since(*node.Expiry) > 0 } +// Online reports the node's last known connectivity. Unknown counts as +// offline. Use [Node.ShouldBeOnline] to derive the value, not to read it. +func (node *Node) Online() bool { + return node.IsOnline != nil && *node.IsOnline +} + +// ShouldBeOnline derives what [Node.IsOnline] must hold from its two inputs: +// a live control session and an unexpired node key. An expired client keeps +// polling control to receive auth updates, so a session alone is not enough. +// Call it only from a NodeStore write closure, where ActiveSessions is +// stable; elsewhere read [Node.Online]. +func (node *Node) ShouldBeOnline() bool { + return node.ActiveSessions > 0 && !node.IsExpired() +} + // IsEphemeral returns if the node is registered as an Ephemeral node. // https://tailscale.com/docs/features/ephemeral-nodes func (node *Node) IsEphemeral() bool { @@ -917,6 +933,15 @@ func (nv NodeView) IsExpired() bool { return nv.ж.IsExpired() } +// Online reports the node's last known connectivity. +func (nv NodeView) Online() bool { + if !nv.Valid() { + return false + } + + return nv.ж.Online() +} + // IsEphemeral returns if the node is registered as an Ephemeral node. // https://tailscale.com/docs/features/ephemeral-nodes func (nv NodeView) IsEphemeral() bool {