From df86073f540bca54afbb55d8c239ab704d5adeca Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 21 Oct 2026 16:41:31 +0000 Subject: [PATCH] policy/v2, state: elect primaries by oldest control session A router that reconnects joins as a standby; one that kept its session takes its routes back once available again, as on Tailscale. Both the global and the via election use it, in place of anti-flap stickiness. --- docs/ref/routes.md | 4 + hscontrol/policy/pm.go | 5 +- hscontrol/policy/v2/policy.go | 39 +++-- hscontrol/policy/v2/policy_test.go | 24 ++- hscontrol/servertest/connect_race_test.go | 8 +- hscontrol/servertest/expiry_compat_test.go | 14 +- hscontrol/servertest/grants_test.go | 18 +- hscontrol/servertest/ha_health_test.go | 23 +-- hscontrol/servertest/ha_property_test.go | 66 +++----- hscontrol/servertest/via_ha_compat_test.go | 13 +- hscontrol/state/node_store.go | 56 +++--- hscontrol/state/node_store_test.go | 1 + hscontrol/state/primaries_property_test.go | 108 ++++++------ hscontrol/state/primaries_test.go | 118 +++++++++++-- hscontrol/state/state.go | 9 +- integration/route_test.go | 187 +++++++++++++++------ 16 files changed, 441 insertions(+), 252 deletions(-) diff --git a/docs/ref/routes.md b/docs/ref/routes.md index eace30cb1..438515ff7 100644 --- a/docs/ref/routes.md +++ b/docs/ref/routes.md @@ -345,6 +345,10 @@ availability](https://tailscale.com/docs/how-to/set-up-high-availability#subnet- This feature is enabled by default when at least two nodes advertise the same prefix. See the configuration options `node.routes.ha` in the [configuration file](configuration.md) for details. +Of the routers that are online, healthy and not expired, the one connected to Headscale the longest serves a route. A +router that reconnects becomes a standby, while a router that stays connected takes its routes back once it is approved, +healthy or unexpired again. When none is available, the route stays with the router that served it last. + ## Troubleshooting ### Enable IP forwarding diff --git a/hscontrol/policy/pm.go b/hscontrol/policy/pm.go index dbb63078a..c71dc88df 100644 --- a/hscontrol/policy/pm.go +++ b/hscontrol/policy/pm.go @@ -72,9 +72,10 @@ type PolicyManager interface { // both fields are empty and the caller falls back to existing behavior. ViaRoutesForPeer(viewer, peer types.NodeView) types.ViaRouteResult - // SetRouterAvailable sets the via election's runtime availability check. + // SetRouterSession sets the via election's runtime check: when a + // router's control session began and whether it may win. // The callback runs under the policy lock and must remain lock-free. - SetRouterAvailable(available func(types.NodeID) bool) + SetRouterSession(session func(types.NodeID) (time.Time, bool)) // NodeCapMap returns compiled policy capabilities for the given node, // or nil when none target it. The map is a defensive clone; its values diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 565b4bf3a..5bc05d4f0 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -119,7 +119,7 @@ type PolicyManager struct { // called on every dispatched change, skips pm.mu when idle. nodeAttrsPending atomic.Bool - routerAvailable func(types.NodeID) bool + routerSession func(types.NodeID) (time.Time, bool) } // filterAndPolicy combines the compiled filter rules with policy content for hashing. @@ -1452,10 +1452,12 @@ func (pm *PolicyManager) NodeCanApproveRoute(node types.NodeView, route netip.Pr return false } -// SetRouterAvailable installs the availability check used by the via election. -// Runtime status changes bypass SetNodes; nil treats every router as available. -// The callback runs with pm.mu held and must remain a lock-free snapshot read. -func (pm *PolicyManager) SetRouterAvailable(available func(types.NodeID) bool) { +// SetRouterSession installs the session check used by the via election: when +// a router's control session began and whether it may win. Runtime status +// changes bypass SetNodes; nil treats every router as available, with one +// session age. The callback runs with pm.mu held and must remain a lock-free +// snapshot read. +func (pm *PolicyManager) SetRouterSession(session func(types.NodeID) (time.Time, bool)) { if pm == nil { return } @@ -1463,7 +1465,7 @@ func (pm *PolicyManager) SetRouterAvailable(available func(types.NodeID) bool) { pm.mu.Lock() defer pm.mu.Unlock() - pm.routerAvailable = available + pm.routerSession = session } // ViaRoutesForPeer computes via grant effects for a viewer-peer pair. @@ -1638,8 +1640,9 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via // When neither condition is met, per-viewer via steering applies. if len(result.Include) > 0 || len(result.Exclude) > 0 { // Multi-router via election: when a via grant's tag matches - // multiple peers advertising the same prefix, only the - // lowest-ID available peer keeps the prefix in Include. The others + // multiple peers advertising the same prefix, only the available + // peer with the oldest control session (lower ID on a tie) keeps + // the prefix in Include, as the global election picks. The others // move to Exclude. With no available router, the lowest ID keeps the // prefix so the viewer still gets exactly one route. // @@ -1671,7 +1674,10 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via continue } - var viaPrimaryID, viaFallbackID types.NodeID + var ( + viaPrimaryID, viaFallbackID types.NodeID + viaPrimarySince time.Time + ) for _, viaTag := range grant.Via { for _, node := range pm.nodes.All() { @@ -1681,13 +1687,18 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via viaFallbackID = node.ID() } - if pm.routerAvailable != nil && - !pm.routerAvailable(node.ID()) { - continue + var since time.Time + + if pm.routerSession != nil { + var ok bool + if since, ok = pm.routerSession(node.ID()); !ok { + continue + } } - if viaPrimaryID == 0 || node.ID() < viaPrimaryID { - viaPrimaryID = node.ID() + if viaPrimaryID == 0 || since.Before(viaPrimarySince) || + (since.Equal(viaPrimarySince) && node.ID() < viaPrimaryID) { + viaPrimaryID, viaPrimarySince = node.ID(), since } } } diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index c3579edcf..39de16873 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -2797,20 +2797,26 @@ func TestViaRoutesForPeer(t *testing.T) { pm, err := NewPolicyManager([]byte(pol), users, nodes.ViewSlice()) require.NoError(t, err) + // sessions maps each available router to when its control session + // began. + at := func(sec int64) time.Time { return time.Unix(sec, 0) } + for _, tt := range []struct { - name string - available []types.NodeID - want types.NodeID + name string + sessions map[types.NodeID]time.Time + want types.NodeID }{ - {name: "all available", available: []types.NodeID{2, 3, 4}, want: 2}, - {name: "lowest ID down", available: []types.NodeID{3, 4}, want: 3}, - {name: "only highest ID left", available: []types.NodeID{4}, want: 4}, + {name: "all available", sessions: map[types.NodeID]time.Time{2: at(1), 3: at(1), 4: at(1)}, want: 2}, + {name: "lowest ID down", sessions: map[types.NodeID]time.Time{3: at(1), 4: at(1)}, want: 3}, + {name: "only highest ID left", sessions: map[types.NodeID]time.Time{4: at(1)}, want: 4}, {name: "whole group down", want: 2}, - {name: "lowest ID recovers", available: []types.NodeID{2, 3, 4}, want: 2}, + {name: "oldest session wins", sessions: map[types.NodeID]time.Time{2: at(3), 3: at(1), 4: at(2)}, want: 3}, } { t.Run(tt.name, func(t *testing.T) { - pm.SetRouterAvailable(func(id types.NodeID) bool { - return slices.Contains(tt.available, id) + pm.SetRouterSession(func(id types.NodeID) (time.Time, bool) { + since, ok := tt.sessions[id] + + return since, ok }) for _, router := range nodes[1:] { diff --git a/hscontrol/servertest/connect_race_test.go b/hscontrol/servertest/connect_race_test.go index ce1f9e339..f94ea19b7 100644 --- a/hscontrol/servertest/connect_race_test.go +++ b/hscontrol/servertest/connect_race_test.go @@ -37,10 +37,10 @@ func TestConnectDisconnectRace(t *testing.T) { // real noise/poll path. After this, [state.NodeStore] + primaryRoutes already // have the node, and [state.State.Connect] has been called once. // - // Only c2 advertises the route. [tailcfg.NodeView.PrimaryRoutes] preserves a current - // primary across changes (anti-flap, see primary.go), so if both - // nodes were advertising, c1 (lower NodeID) would stay primary and - // the test could never observe the route slipping out of c2's + // Only c2 advertises the route. The election keeps a route on the + // router with the oldest control session, so if both nodes were + // advertising, c1 (connected first) would stay primary and the test + // could never observe the route slipping out of c2's // [tailcfg.NodeView.PrimaryRoutes] — it would never have been there in the first place. c1 := servertest.NewClient(t, srv, "race-r1", servertest.WithUser(user)) c2 := servertest.NewClient(t, srv, "race-r2", servertest.WithUser(user)) diff --git a/hscontrol/servertest/expiry_compat_test.go b/hscontrol/servertest/expiry_compat_test.go index faf002b06..c16226a0e 100644 --- a/hscontrol/servertest/expiry_compat_test.go +++ b/hscontrol/servertest/expiry_compat_test.go @@ -36,11 +36,6 @@ var expirySkipReasons = map[string]string{ "expiry/C2-expire-r1/r1/paths-r2": expiredPeerPaths, "expiry/C2-expire-r1/r1/self": selfExpiredFlags, - "expiry/C3-restore-r1/r1/routes-observer": restoredRouterTakesRoute, - "expiry/C3-restore-r1/r1/routes-r2": restoredRouterTakesRoute, - "expiry/C3-restore-r1/r2/routes-observer": restoredRouterTakesRoute, - "expiry/C3-restore-r1/r2/routes-r1": restoredRouterTakesRoute, - "expiry/C4-expire-r2/r2/online-observer": expiredPeerOffline, "expiry/C4-expire-r2/r2/online-r1": expiredPeerOffline, "expiry/C4-expire-r2/r2/paths-observer": expiredPeerPaths, @@ -114,11 +109,10 @@ var expirySkipReasons = map[string]string{ } const ( - selfExpiredFlags = "SELF_EXPIRED_FLAGS: headscale sends an expired node its own entry with MachineAuthorized false and Expired set; SaaS keeps MachineAuthorized true and leaves Expired out" - expiredPeerPaths = "EXPIRED_PEER_PATHS: headscale sends peers an expired node's endpoints and home DERP; SaaS leaves both out" - clientExpiryRefused = "CLIENT_EXPIRY_REFUSED: headscale refuses a client's request for an earlier key expiry; SaaS stores it, and the node expires then" - nameserversFromConfig = "NAMESERVERS_FROM_CONFIG: headscale reads nameservers from its config and cannot change them while running; SaaS pushes a change to every node, an expired one too" - restoredRouterTakesRoute = "RESTORED_ROUTER_TAKES_ROUTE: SaaS hands a route back to a router whose key expiry is lifted, taking it from the standby that served it meanwhile; headscale keeps the standby" + selfExpiredFlags = "SELF_EXPIRED_FLAGS: headscale sends an expired node its own entry with MachineAuthorized false and Expired set; SaaS keeps MachineAuthorized true and leaves Expired out" + expiredPeerPaths = "EXPIRED_PEER_PATHS: headscale sends peers an expired node's endpoints and home DERP; SaaS leaves both out" + clientExpiryRefused = "CLIENT_EXPIRY_REFUSED: headscale refuses a client's request for an earlier key expiry; SaaS stores it, and the node expires then" + nameserversFromConfig = "NAMESERVERS_FROM_CONFIG: headscale reads nameservers from its config and cannot change them while running; SaaS pushes a change to every node, an expired one too" ) // TestNodeExpiryCompat replays key expiry against SaaS: an expired router diff --git a/hscontrol/servertest/grants_test.go b/hscontrol/servertest/grants_test.go index f3c0257ba..1af6b2f58 100644 --- a/hscontrol/servertest/grants_test.go +++ b/hscontrol/servertest/grants_test.go @@ -1243,8 +1243,22 @@ func TestGrantViaSubnetFailover(t *testing.T) { setExpiry(t, srv, router1ID, nil) } - clients["client"].WaitForCondition(t, "route returns to router-1", - 30*time.Second, onlyHolder("router-1")) + // router-1 keeps its control session through health and expiry + // changes, so it is older than router-2 and takes the route + // back; a reconnect opens a newer one. + want := "router-1" + if mode == "offline" { + want = "router-2" + } + + clients["client"].WaitForCondition(t, "route settles on "+want, + 30*time.Second, func(nm *netmap.NetworkMap) bool { + router1Online := slices.ContainsFunc(nm.Peers, func(p tailcfg.NodeView) bool { + return p.Hostinfo().Hostname() == "router-1" && p.Online().Get() + }) + + return router1Online && onlyHolder(want)(nm) + }) assert.Contains(t, srv.State().GetNodePrimaryRoutes(otherID), route) }) } diff --git a/hscontrol/servertest/ha_health_test.go b/hscontrol/servertest/ha_health_test.go index 230afc665..ff16b777d 100644 --- a/hscontrol/servertest/ha_health_test.go +++ b/hscontrol/servertest/ha_health_test.go @@ -121,9 +121,10 @@ func TestHAHealthProbe_UnhealthyFailover(t *testing.T) { assert.NotContains(t, primaries1, route, "node 1 should not be primary") } -// TestHAHealthProbe_RecoveryNoFlap verifies that marking an unhealthy -// node healthy again does NOT cause it to reclaim primary (stability). -func TestHAHealthProbe_RecoveryNoFlap(t *testing.T) { +// TestHAHealthProbe_RecoveryReturnsRoute verifies that marking an unhealthy +// node healthy again hands it the route back: it kept the older control +// session. +func TestHAHealthProbe_RecoveryReturnsRoute(t *testing.T) { t.Parallel() srv := servertest.NewServer(t) @@ -145,12 +146,12 @@ func TestHAHealthProbe_RecoveryNoFlap(t *testing.T) { primaries := srv.State().GetNodePrimaryRoutes(nodeID2) require.Contains(t, primaries, route, "node 2 should be primary") - // Recovery: node 1 healthy again. Node 2 should STAY primary. + // Recovery: node 1 healthy again, with the older session. changed := srv.State().SetNodeHealth(nodeID1, true) - assert.False(t, changed, "recovery should not change primaries (no flap)") + assert.True(t, changed, "recovery should hand the route back") - primaries = srv.State().GetNodePrimaryRoutes(nodeID2) - assert.Contains(t, primaries, route, "node 2 should remain primary after recovery") + primaries = srv.State().GetNodePrimaryRoutes(nodeID1) + assert.Contains(t, primaries, route, "node 1 should be primary again after recovery") } // TestHAHealthProbe_ConnectClearsUnhealthy verifies that reconnecting @@ -290,9 +291,8 @@ func TestHAHealthProbe_SetUnhealthyNoRoutesIsNoOp(t *testing.T) { // [HAHealthProber.ProbeOnce], the timeout path observes the reconnect // and bails out instead of installing a spurious Unhealthy bit. // -// Without the guard, the primary fails over to the standby and the -// anti-flap election preserves that choice even after the original -// primary is fully back online. +// Without the guard, the stale timeout marks the reconnected primary +// unhealthy and the route fails over to the standby. func TestHAHealthProbe_ReconnectDuringProbeKeepsHealthy(t *testing.T) { t.Parallel() @@ -445,7 +445,8 @@ func TestHAHealthProbe_DeferralScopedToPrefix(t *testing.T) { assert.True(t, srv.State().IsNodeHealthy(officeID1)) assert.True(t, srv.State().IsNodeHealthy(labID1), "a deferral on another prefix must not withhold the lab node's recovery") - assert.Contains(t, srv.State().GetNodePrimaryRoutes(labID2), labRoute) + assert.Contains(t, srv.State().GetNodePrimaryRoutes(labID1), labRoute, + "the recovered lab node has the older session and takes its route back") } func TestHAHealthProbe_LastAdvertiserRecovers(t *testing.T) { diff --git a/hscontrol/servertest/ha_property_test.go b/hscontrol/servertest/ha_property_test.go index 16e6cc0a2..cad6213f8 100644 --- a/hscontrol/servertest/ha_property_test.go +++ b/hscontrol/servertest/ha_property_test.go @@ -27,8 +27,8 @@ type haPropClient struct { name string route netip.Prefix // connected mirrors the prober's view (mapBatcher.IsConnected). - // Tracked alongside the runtime read so anti-flap invariants can - // reason about the candidate set the prober actually evaluates. + // Tracked alongside the runtime read so the invariants can reason + // about the candidate set the prober actually evaluates. connected bool // freshSinceReconnect is true between a successful Reconnect and // the prober's second observation of the new SessionEpoch — the @@ -138,7 +138,7 @@ func readPrimaries( // checkHAInvariants walks every prefix → primary mapping in the live // snapshot and asserts the six properties documented in the test // header. prevPrimaries is the snapshot taken before the just-applied -// op so anti-flap can compare moves. +// op, the primary that keeps a prefix while no router is available. // //nolint:gocyclo // invariant checker over several independent properties func checkHAInvariants( @@ -146,7 +146,6 @@ func checkHAInvariants( srv *servertest.TestServer, clients []*haPropClient, prevPrimaries map[netip.Prefix]types.NodeID, - skipAntiFlap bool, ) { rt.Helper() @@ -246,31 +245,17 @@ func checkHAInvariants( ) } - // Invariant 5 (anti-flap): if the previous snapshot already had - // a primary for this prefix AND that primary is still a healthy, - // online, advertising candidate, the new snapshot must keep it. - // Skipped on settle reads where we re-check without an op in - // between — the prober may run in a deferred batch and the - // election may legitimately move once on its own timeline. - if !skipAntiFlap { - if prev, hadPrev := prevPrimaries[prefix]; hadPrev && prev != owner { - prevNV, exists := st.GetNodeByID(prev) - if exists && prevNV.Valid() { - prevOnline, prevKnown := prevNV.IsOnline().GetOk() - prevApproved := prevNV.AllApprovedRoutes() - - stillCandidate := prevKnown && prevOnline && - slices.Contains(prevApproved, prefix) - stillHealthy := st.IsNodeHealthy(prev) - - if stillCandidate && stillHealthy { - rt.Fatalf( - "prefix %s flapped primary %d -> %d "+ - "while previous primary was still a healthy candidate", - prefix, prev, owner, - ) - } - } + // Invariant 5 (oldest session): no healthy online candidate + // opened its control session before the primary's. A router + // that reconnects joins as a standby; one that keeps its + // session wins again once healthy. + for _, c := range advertisersByPrefix[prefix] { + cv, _ := st.GetNodeByID(c) + if st.IsNodeHealthy(c) && cv.SessionStart().Before(nv.SessionStart()) { + rt.Fatalf( + "prefix %s primary %d holds it over %d, whose session is older", + prefix, owner, c, + ) } } } @@ -487,17 +472,6 @@ func TestHAProberProperty(t *testing.T) { prev := snapshotPrimaries(rt, srv, clients) - // Anti-flap is meaningful only across operations that do - // not themselves mutate the candidate set or per-node - // health. Connect/Disconnect both mutate IsOnline (via - // state.Connect, or via the asynchronous grace-period - // state.Disconnect), and the resulting election move is - // expected, not a flap. ProbeOnce, by contrast, must keep - // the primary on the previous owner whenever that owner - // is still a candidate — this is what the reconnect- - // during-probe and dual-disconnect guards preserve. - isProberOp := op == 2 - switch op { case 0: // ClientDisconnect idx := idxGen.Draw(rt, fmt.Sprintf("disc_idx_%d", step)) @@ -506,8 +480,8 @@ func TestHAProberProperty(t *testing.T) { // Refuse to disconnect the last connected node. The // prober's HANodes gate requires ≥2 online candidates // per prefix; with only one node left, no probe runs - // and the prober-driven invariants (anti-flap, - // reconnect-defer) become unfalsifiable. Keeping at + // and the prober-driven invariants (health + // failover, reconnect-defer) become unfalsifiable. Keeping at // least two candidates is enough to exercise every // failure shape we care about. connectedCount := 0 @@ -615,15 +589,13 @@ func TestHAProberProperty(t *testing.T) { // to wait on; the no-op step still verifies that two // consecutive reads (one snapshot, the same snapshot) // stay consistent. This is invariant 3: stable - // conditions = stable primary. We skip the anti-flap - // comparison so a primary that legitimately changed - // in the prior op is not re-flagged here. - checkHAInvariants(rt, srv, clients, prev, true) + // conditions = stable primary. + checkHAInvariants(rt, srv, clients, prev) continue } - checkHAInvariants(rt, srv, clients, prev, !isProberOp) + checkHAInvariants(rt, srv, clients, prev) } }) } diff --git a/hscontrol/servertest/via_ha_compat_test.go b/hscontrol/servertest/via_ha_compat_test.go index e72e4e0b9..e51bf0781 100644 --- a/hscontrol/servertest/via_ha_compat_test.go +++ b/hscontrol/servertest/via_ha_compat_test.go @@ -221,18 +221,7 @@ func runViaHACompat(t *testing.T, c *testcapture.Capture) { // viaHAFailoverSkipReasons lists checks where headscale differs from SaaS, // keyed ///. A listed check that matches // fails, so a fix must delete its entry. -var viaHAFailoverSkipReasons = map[string]string{ - "via-ha-failover/V2/client-a/route-owner": viaLowestIDAfterRestart, - "via-ha-failover/V10/client-a/route-owner": viaLowestIDAfterRestart, - - "via-ha-failover/V12/client-a/route-owner": readvertisedRouterTakesRoute, - "via-ha-failover/V12/client-b/route-owner": readvertisedRouterTakesRoute, -} - -const ( - viaLowestIDAfterRestart = "VIA_LOWEST_ID_AFTER_RESTART: headscale serves a via route from the group's lowest-ID router once it reconnects; SaaS keeps the router that served while it was down" - readvertisedRouterTakesRoute = "READVERTISED_ROUTER_TAKES_ROUTE: SaaS hands a route back to an HA router that withdrew and re-advertised it, taking it from the router that served it meanwhile; headscale keeps that router" -) +var viaHAFailoverSkipReasons = map[string]string{} // viaHAFailoverViewers are the clients whose views are compared: one reaches // the route only through the via group, the other through the global diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index 52cfc8b46..a1231c47b 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -169,8 +169,8 @@ type Snapshot struct { allNodes []types.NodeView // routes maps each prefix to its current primary advertiser. The - // previous assignment is carried over when still valid so the - // primary does not flap on every unrelated batch. + // next election reads it to keep a prefix where it was while no + // router is available. routes map[netip.Prefix]types.NodeID isPrimaryRoute map[types.NodeID]bool } @@ -217,7 +217,8 @@ func updateChanges(pre, post *types.Node) (bool, bool) { return false, pre.Online() != post.Online() || !preView.Expiry().GetOr(time.Time{}).Equal(postView.Expiry().GetOr(time.Time{})) || - pre.Unhealthy != post.Unhealthy + pre.Unhealthy != post.Unhealthy || + !pre.SessionStart.Equal(post.SessionStart) } // PutNode adds or updates a node in the store. @@ -727,8 +728,8 @@ func snapshotFromNodes( if reuseRoutes { routes, isPrimaryRoute = prev.routes, prev.isPrimaryRoute } else { - // Carrying the previous assignment forward lets a still-valid - // primary survive unrelated batches. + // The previous assignment keeps a prefix mapped while no router + // is available. var prevRoutes map[netip.Prefix]types.NodeID if prev != nil { prevRoutes = prev.routes @@ -818,8 +819,9 @@ func advertisers( // electPrimaryRoutes picks the primary router for each non-exit prefix any // node approves. A router is available when it is online, unexpired and -// healthy. The previous primary is preserved while it is available -// (anti-flap); otherwise the lowest-NodeID available router wins. +// healthy. The available router with the oldest control session wins, the +// lower NodeID on a tie: a router that reconnects joins as a standby, and +// one that keeps its session takes its routes back once available again. // // With no router available the prefix stays mapped, so peers keep the route // and use it once its router returns: the previous primary keeps it while it @@ -843,22 +845,24 @@ func electPrimaryRoutes( routes := make(map[netip.Prefix]types.NodeID, len(approvers)) for prefix, ids := range approvers { + var oldest types.NodeID + + // ids ascend, so a tie keeps the lower ID. + for _, id := range ids { + if available(id) && (oldest == 0 || + nodes[id].SessionStart.Before(nodes[oldest].SessionStart)) { + oldest = id + } + } + + if oldest != 0 { + routes[prefix] = oldest + + continue + } + cur, hasPrev := prev[prefix] - hasPrev = hasPrev && slices.Contains(ids, cur) - - if hasPrev && available(cur) { - routes[prefix] = cur - - continue - } - - if i := slices.IndexFunc(ids, available); i >= 0 { - routes[prefix] = ids[i] - - continue - } - - if hasPrev { + if hasPrev && slices.Contains(ids, cur) { routes[prefix] = cur continue @@ -1095,12 +1099,12 @@ func (s *NodeStore) IsNodeHealthy(id types.NodeID) bool { return !n.Unhealthy } -// IsRouterAvailable reports whether id may win a primary election: online, -// unexpired and healthy. -func (s *NodeStore) IsRouterAvailable(id types.NodeID) bool { +// RouterSession reports when id's control session began and whether id may +// win a primary election: online, unexpired and healthy. +func (s *NodeStore) RouterSession(id types.NodeID) (time.Time, bool) { n, ok := s.data.Load().nodesByID[id] - return ok && servesRoutes(&n) && !n.Unhealthy + return n.SessionStart, ok && servesRoutes(&n) && !n.Unhealthy } // PrimaryRoutes returns the snapshot's prefix→primary map. The map is diff --git a/hscontrol/state/node_store_test.go b/hscontrol/state/node_store_test.go index 1fa3993d5..ddeb8dd27 100644 --- a/hscontrol/state/node_store_test.go +++ b/hscontrol/state/node_store_test.go @@ -1967,6 +1967,7 @@ func TestUpdateChangesReportsRelationFields(t *testing.T) { {name: "online", mutate: func(n *types.Node) { n.IsOnline = new(true) }, wantElection: true}, {name: "unhealthy", mutate: func(n *types.Node) { n.Unhealthy = true }, wantElection: true}, {name: "expiry", mutate: func(n *types.Node) { n.Expiry = new(time.Now().Add(-time.Minute)) }, wantElection: true}, + {name: "session start", mutate: func(n *types.Node) { n.SessionStart = time.Now() }, wantElection: true}, {name: "endpoint", mutate: func(n *types.Node) { n.Endpoints = []netip.AddrPort{netip.MustParseAddrPort("192.0.2.1:1")} }}, {name: "lastseen", mutate: func(n *types.Node) { n.LastSeen = new(time.Now()) }}, } diff --git a/hscontrol/state/primaries_property_test.go b/hscontrol/state/primaries_property_test.go index bca17ac92..a8472b6e3 100644 --- a/hscontrol/state/primaries_property_test.go +++ b/hscontrol/state/primaries_property_test.go @@ -6,6 +6,7 @@ import ( "net/netip" "slices" "testing" + "time" "github.com/juanfont/headscale/hscontrol/types" "pgregory.net/rapid" @@ -23,7 +24,10 @@ import ( // is the one inside snapshotFromNodes; the model's job is to predict // the same answer from a separate, deliberately direct implementation. type primariesModel struct { + // connected mirrors IsOnline; since holds the step at which the + // node's current session opened. connected map[types.NodeID]bool + since map[types.NodeID]int // announced mirrors Hostinfo.RoutableIPs — what the client says // it can route. The election does not look at this directly; it // is used to recompute the effective set whenever ApprovedRoutes @@ -40,16 +44,17 @@ type primariesModel struct { prefixes map[types.NodeID][]netip.Prefix unhealthy map[types.NodeID]bool - // primary[p] is the current primary for prefix p. The - // implementation preserves the current primary across changes to - // avoid flapping, so the model has to track this across - // operations rather than recompute a fresh choice each time. + // primary[p] is the current primary for prefix p. With no router + // available the implementation keeps the previous primary, so the + // model has to track this across operations rather than recompute + // a fresh choice each time. primary map[netip.Prefix]types.NodeID } func newPrimariesModel() *primariesModel { return &primariesModel{ connected: map[types.NodeID]bool{}, + since: map[types.NodeID]int{}, announced: map[types.NodeID][]netip.Prefix{}, approved: map[types.NodeID][]netip.Prefix{}, prefixes: map[types.NodeID][]netip.Prefix{}, @@ -120,6 +125,15 @@ func (m *primariesModel) approversByPrefix() map[netip.Prefix][]types.NodeID { return out } +// connect mirrors a session opening on node n at step. +func (m *primariesModel) connect(n types.NodeID, step int) { + if !m.connected[n] { + m.since[n] = step + } + + m.connected[n] = true +} + // available reports whether n may win an election: connected and healthy. func (m *primariesModel) available(n types.NodeID) bool { return m.connected[n] && !m.unhealthy[n] @@ -138,19 +152,25 @@ func (m *primariesModel) updatePrimaries() { } for p, nodes := range approvers { + // The oldest session wins; nodes ascend, so a tie keeps the + // lower ID. + var oldest types.NodeID + + for _, n := range nodes { + if m.available(n) && (oldest == 0 || m.since[n] < m.since[oldest]) { + oldest = n + } + } + + if oldest != 0 { + m.primary[p] = oldest + + continue + } + cur, hasPrev := m.primary[p] hasPrev = hasPrev && slices.Contains(nodes, cur) - if hasPrev && m.available(cur) { - continue - } - - if i := slices.IndexFunc(nodes, m.available); i >= 0 { - m.primary[p] = nodes[i] - - continue - } - // No router available: the previous primary keeps the prefix, // else the lowest-ID approver the prober has not declared // unreachable; with neither the prefix is unmapped. @@ -189,8 +209,8 @@ func samePrefixSet(a, b []netip.Prefix) bool { // checkPrimariesProperties asserts every rule we expect of the snapshot's // primaries map given the model. prevSnapshotPrimaries is the snapshot's -// PrimaryRoutes() reading taken before the just-applied op, used to -// catch flap regressions that move primary off a still-eligible owner. +// PrimaryRoutes() reading taken before the just-applied op, the primary +// that may stay unhealthy while no router is available. func checkPrimariesProperties( rt *rapid.T, ns *NodeStore, @@ -327,21 +347,15 @@ func checkPrimariesProperties( ) } - // Anti-flap: if the previous snapshot already had a primary - // for this prefix AND that primary is still a healthy - // candidate after the op, the election must keep it. Flapping - // the primary for an unrelated reason violates the contract - // the integration tests rely on (HA failover only moves on - // loss of candidacy or health). - if prev, hadPrev := prevSnapshotPrimaries[prefix]; hadPrev { - stillCandidate := slices.Contains(candidates, prev) - stillHealthy := !m.unhealthy[prev] - - if stillCandidate && stillHealthy && owner != prev { + // Oldest session: no available router opened its control + // session before the primary's. A router that reconnects joins + // as a standby; one that keeps its session wins again. + for _, c := range approversByPrefix[prefix] { + cv, _ := ns.GetNode(c) + if m.available(c) && cv.SessionStart().Before(nv.SessionStart()) { rt.Fatalf( - "prefix %s flapped primary %d -> %d "+ - "while previous primary was still a healthy candidate", - prefix, prev, owner, + "prefix %s primary %d holds it over %d, whose session is older", + prefix, owner, c, ) } } @@ -447,10 +461,20 @@ func TestPrimaryRoutesProperty(t *testing.T) { for step := range opCount { op := rapid.IntRange(0, 8).Draw(rt, fmt.Sprintf("op_%d", step)) + // connect opens a session, as State.Connect does, when the + // node is offline. + connect := func(n *types.Node) { + if !n.Online() { + n.SessionStart = time.Unix(int64(step), 0) + } + + n.IsOnline = new(true) + } + // Snapshot primaries before applying the op so the - // anti-flap invariant has a stable reference. Reading - // after the model has already changed would compare a - // stale snapshot to a moved model. + // healthy-preference check knows the previous primary. + // Reading after the model has already changed would compare + // a stale snapshot to a moved model. prevPrimaries := snapshotPrimariesCopy(ns) switch op { @@ -459,7 +483,7 @@ func TestPrimaryRoutesProperty(t *testing.T) { prefs := prefixSubsetGen.Draw(rt, fmt.Sprintf("prefs_%d", step)) ns.UpdateNode(id, func(n *types.Node) { - n.IsOnline = new(true) + connect(n) n.Unhealthy = false n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: prefs} n.ApprovedRoutes = prefs @@ -468,12 +492,7 @@ func TestPrimaryRoutesProperty(t *testing.T) { m.announced[id] = prefs m.approved[id] = prefs m.recomputeEffective(id) - - if len(prefs) == 0 { - delete(m.connected, id) - } else { - m.connected[id] = true - } + m.connect(id, step) delete(m.unhealthy, id) @@ -503,7 +522,7 @@ func TestPrimaryRoutesProperty(t *testing.T) { prefs := prefixSubsetGen.Draw(rt, fmt.Sprintf("prefs_%d", step)) ns.UpdateNode(id, func(n *types.Node) { - n.IsOnline = new(true) + connect(n) n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: prefs} n.ApprovedRoutes = prefs }) @@ -511,12 +530,7 @@ func TestPrimaryRoutesProperty(t *testing.T) { m.announced[id] = prefs m.approved[id] = prefs m.recomputeEffective(id) - - if len(prefs) == 0 { - delete(m.connected, id) - } else { - m.connected[id] = true - } + m.connect(id, step) case 5: // BatchProbeResults — atomic health flips per cycle. // Mirrors State.BatchSetNodeHealth: per-node diff --git a/hscontrol/state/primaries_test.go b/hscontrol/state/primaries_test.go index 165616ecc..f287b4d29 100644 --- a/hscontrol/state/primaries_test.go +++ b/hscontrol/state/primaries_test.go @@ -24,6 +24,24 @@ func mp(prefix string) netip.Prefix { type primariesFixture struct { t *testing.T ns *NodeStore + + // clock orders the control sessions advertise and approveRoutes open. + clock time.Time +} + +// session returns an update that opens a control session, as State.Connect +// does, when the node is offline: each opens later than the one before. +func (f *primariesFixture) session() func(*types.Node) { + f.clock = f.clock.Add(time.Second) + start := f.clock + + return func(n *types.Node) { + if !n.Online() { + n.SessionStart = start + } + + n.IsOnline = new(true) + } } func newPrimariesFixture(t *testing.T, ids ...types.NodeID) *primariesFixture { @@ -45,8 +63,10 @@ func newPrimariesFixture(t *testing.T, ids ...types.NodeID) *primariesFixture { // prefs argument leaves the node online but advertising nothing. func (f *primariesFixture) advertise(id types.NodeID, prefs ...netip.Prefix) { f.t.Helper() + + connect := f.session() f.ns.UpdateNode(id, func(n *types.Node) { - n.IsOnline = new(true) + connect(n) n.Unhealthy = false n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: prefs} n.ApprovedRoutes = prefs @@ -58,8 +78,10 @@ func (f *primariesFixture) advertise(id types.NodeID, prefs ...netip.Prefix) { // Unhealthy. func (f *primariesFixture) approveRoutes(id types.NodeID, prefs ...netip.Prefix) { f.t.Helper() + + connect := f.session() f.ns.UpdateNode(id, func(n *types.Node) { - n.IsOnline = new(true) + connect(n) n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: prefs} n.ApprovedRoutes = prefs }) @@ -147,10 +169,10 @@ func TestPrimaries_OverlappingRoutesLowerIDWins(t *testing.T) { f.requireNodeRoutes(2) } -func TestPrimaries_AntiFlapPreservesCurrentPrimary(t *testing.T) { +func TestPrimaries_ReconnectJoinsAsStandby(t *testing.T) { // A primary that disappears (advertiser leaves the set) should - // trigger failover. When the original primary returns, the new - // primary keeps the assignment — anti-flap. + // trigger failover. When the original primary returns on a new + // session, the new primary's session is older and keeps the route. f := newPrimariesFixture(t, 1, 2) f.advertise(1, mp("192.168.1.0/24")) f.advertise(2, mp("192.168.1.0/24")) @@ -276,15 +298,75 @@ func TestPrimaries_UnhealthyTriggersFailover(t *testing.T) { f.requirePrimary(mp("192.168.1.0/24"), 2) } -func TestPrimaries_RecoveryFromUnhealthyNoFlap(t *testing.T) { - f := newPrimariesFixture(t, 1, 2) - f.advertise(1, mp("192.168.1.0/24")) - f.advertise(2, mp("192.168.1.0/24")) - f.unhealthy(1) - f.requirePrimary(mp("192.168.1.0/24"), 2) +func TestPrimaries_OldestSessionWins(t *testing.T) { + prefix := mp("192.168.1.0/24") + withdraw := func(f *primariesFixture, id types.NodeID) { + f.ns.UpdateNode(id, func(n *types.Node) { n.Hostinfo = &tailcfg.Hostinfo{} }) + } - f.healthy(1) - f.requirePrimary(mp("192.168.1.0/24"), 2) + tests := []struct { + name string + setup func(*primariesFixture) + want types.NodeID + }{ + { + name: "older session over lower ID", + setup: func(f *primariesFixture) { + f.advertise(2, prefix) + f.advertise(1, prefix) + }, + want: 2, + }, + { + name: "healthy again takes the route back", + setup: func(f *primariesFixture) { + f.advertise(1, prefix) + f.advertise(2, prefix) + f.unhealthy(1) + f.healthy(1) + }, + want: 1, + }, + { + name: "approved again takes the route back", + setup: func(f *primariesFixture) { + f.advertise(1, prefix) + f.advertise(2, prefix) + f.approveRoutes(1) + f.approveRoutes(1, prefix) + }, + want: 1, + }, + { + name: "advertised again takes the route back", + setup: func(f *primariesFixture) { + f.advertise(1, prefix) + f.advertise(2, prefix) + withdraw(f, 1) + f.requirePrimary(prefix, 2) + f.approveRoutes(1, prefix) + }, + want: 1, + }, + { + name: "reconnected router stays standby", + setup: func(f *primariesFixture) { + f.advertise(1, prefix) + f.advertise(2, prefix) + f.disconnect(1) + f.advertise(1, prefix) + }, + want: 2, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newPrimariesFixture(t, 1, 2) + tt.setup(f) + f.requirePrimary(prefix, tt.want) + }) + } } func TestPrimaries_AllUnhealthyKeepsAPrimary(t *testing.T) { @@ -332,15 +414,19 @@ func TestPrimaries_ExpiredRouterNotElected(t *testing.T) { f.advertise(1, prefix) f.advertise(2, prefix) f.requirePrimary(prefix, 1) - require.True(t, f.ns.IsRouterAvailable(1)) + _, ok := f.ns.RouterSession(1) + require.True(t, ok) f.ns.UpdateNode(1, func(n *types.Node) { n.Expiry = new(time.Now().Add(-time.Minute)) }) f.requirePrimary(prefix, 2) - require.False(t, f.ns.IsRouterAvailable(1)) - require.True(t, f.ns.IsRouterAvailable(2)) + _, ok = f.ns.RouterSession(1) + require.False(t, ok) + + _, ok = f.ns.RouterSession(2) + require.True(t, ok) } func TestPrimaries_ExitRouteNotElected(t *testing.T) { diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 3a251e322..dc181a8f2 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -325,7 +325,7 @@ func NewState(cfg *types.Config) (*State, error) { batchTimeout := cmp.Or(cfg.Tuning.NodeStoreBatchTimeout, defaultNodeStoreBatchTimeout) nodeStore := NewNodeStore(nodes, policyPeersFunc(polMan), batchSize, batchTimeout) - polMan.SetRouterAvailable(nodeStore.IsRouterAvailable) + polMan.SetRouterSession(nodeStore.RouterSession) nodeStore.Start() // Last: NewLRU starts the TTL sweeper goroutine, which only Close stops. @@ -752,6 +752,13 @@ func (s *State) Connect(id types.NodeID) ([]change.Change, uint64) { node, ok := s.nodeStore.UpdateNode(id, func(n *types.Node) { n.SessionEpoch++ epoch = n.SessionEpoch + + // A reconnect within poll.go's offline grace keeps the age: peers + // never saw the node leave, so a blip does not move its routes. + if n.ActiveSessions == 0 { + n.SessionStart = time.Now() + } + n.ActiveSessions++ n.IsOnline = new(n.ShouldBeOnline()) n.Unhealthy = false diff --git a/integration/route_test.go b/integration/route_test.go index d3cf5e889..0c0aed931 100644 --- a/integration/route_test.go +++ b/integration/route_test.go @@ -311,6 +311,8 @@ func TestHASubnetRouterFailover(t *testing.T) { client := allClients[3] + orderRouterSessions(t, headscale, subRouter1, subRouter2, subRouter3) + t.Logf("%s (%s) picked as client", client.Hostname(), client.MustID()) t.Logf("=== Initial Route Advertisement - Setting up HA configuration with 3 routers ===") t.Logf("[%s] Starting test section", time.Now().Format(TimestampFormat)) @@ -1027,7 +1029,7 @@ func TestHASubnetRouterFailover(t *testing.T) { t.Logf("[%s] Starting test section", time.Now().Format(TimestampFormat)) t.Logf(" Current state: Router 1 STANDBY, Router 2 STANDBY, Router 3 PRIMARY") t.Logf(" Action: Disabling route approval on router 3 (route still advertised but not approved)") - t.Logf(" Expected: Router 1 (%s) should become new PRIMARY (lowest ID with approved route)", subRouter1.Hostname()) + t.Logf(" Expected: Router 1 (%s) should become new PRIMARY (came back before router 2, so its session is older)", subRouter1.Hostname()) t.Logf(" Expected: Router 2 (%s) remains STANDBY", subRouter2.Hostname()) t.Logf(" Expected: Router 3 (%s) goes to advertised-only state (no longer serving)", subRouter3.Hostname()) _, err = headscale.ApproveRoutes(mustParseID(MustFindNode(subRouter3.Hostname(), nodes).Id), []netip.Prefix{}) @@ -1194,14 +1196,15 @@ func TestHASubnetRouterFailover(t *testing.T) { checkFailureAndPrintRoutes(t, client) - // enable the route of subnet router 1, no change expected + // Re-enable the route of subnet router 1. Its control session is older + // than router 2's (it came back first), so it takes the route back. t.Logf("=== ROUTE RE-ENABLE TEST: Re-approving route on router 1 (%s) ===", subRouter1.Hostname()) t.Logf("[%s] Starting test section", time.Now().Format(TimestampFormat)) t.Logf(" Current state: Router 1 advertised-only, Router 2 PRIMARY, Router 3 advertised-only") t.Logf(" Action: Re-enabling route approval on router 1") - t.Logf(" Expected: Router 2 (%s) remains PRIMARY (stability - no unnecessary flapping)", subRouter2.Hostname()) - t.Logf(" Expected: Router 1 (%s) becomes STANDBY (approved but not primary)", subRouter1.Hostname()) - t.Logf(" Expected: HA fully restored with Router 2 PRIMARY and Router 1 STANDBY") + t.Logf(" Expected: Router 1 (%s) becomes PRIMARY (older control session)", subRouter1.Hostname()) + t.Logf(" Expected: Router 2 (%s) becomes STANDBY", subRouter2.Hostname()) + t.Logf(" Expected: HA fully restored with Router 1 PRIMARY and Router 2 STANDBY") r1Node := MustFindNode(subRouter1.Hostname(), nodes) _, err = headscale.ApproveRoutes( @@ -1215,10 +1218,10 @@ func TestHASubnetRouterFailover(t *testing.T) { assert.NoError(c, err) assert.Len(c, nodes, 6) - requireNodeRouteCountWithCollect(c, MustFindNode(subRouter1.Hostname(), nodes), 1, 1, 0) - requireNodeRouteCountWithCollect(c, MustFindNode(subRouter2.Hostname(), nodes), 1, 1, 1) + requireNodeRouteCountWithCollect(c, MustFindNode(subRouter1.Hostname(), nodes), 1, 1, 1) + requireNodeRouteCountWithCollect(c, MustFindNode(subRouter2.Hostname(), nodes), 1, 1, 0) requireNodeRouteCountWithCollect(c, MustFindNode(subRouter3.Hostname(), nodes), 1, 0, 0) - }, propagationTime, 200*time.Millisecond, "Re-enable verification: Router 1 approved as STANDBY, Router 2 remains PRIMARY (no flapping), full HA restored") + }, propagationTime, 200*time.Millisecond, "Re-enable verification: Router 1 PRIMARY, Router 2 STANDBY, full HA restored") // Verify that the route is announced from subnet router 1 assert.EventuallyWithT(t, func(c *assert.CollectT) { @@ -1237,33 +1240,33 @@ func TestHASubnetRouterFailover(t *testing.T) { return } - assert.Nil(c, srs1PeerStatus.PrimaryRoutes) - assert.NotNil(c, srs2PeerStatus.PrimaryRoutes) + assert.NotNil(c, srs1PeerStatus.PrimaryRoutes) + assert.Nil(c, srs2PeerStatus.PrimaryRoutes) assert.Nil(c, srs3PeerStatus.PrimaryRoutes) - if srs2PeerStatus.PrimaryRoutes != nil { + if srs1PeerStatus.PrimaryRoutes != nil { assert.Contains(c, - srs2PeerStatus.PrimaryRoutes.AsSlice(), + srs1PeerStatus.PrimaryRoutes.AsSlice(), pref, ) } - }, propagationTime, 200*time.Millisecond, "Verifying Router 2 remains PRIMARY after Router 1 route re-enabled") + }, propagationTime, 200*time.Millisecond, "Verifying Router 1 is PRIMARY after its route is re-enabled") assert.EventuallyWithT(t, func(c *assert.CollectT) { - assertCurlDockerHostname(c, client, weburl, "Verifying client can reach webservice through router 2 after route re-enable") - }, propagationTime, 200*time.Millisecond, "Verifying client can reach webservice through router 2 after route re-enable") + assertCurlDockerHostname(c, client, weburl, "Verifying client can reach webservice through router 1 after route re-enable") + }, propagationTime, 200*time.Millisecond, "Verifying client can reach webservice through router 1 after route re-enable") assert.EventuallyWithT(t, func(c *assert.CollectT) { tr, err := client.Traceroute(webip) assert.NoError(c, err) - ip, err := subRouter2.IPv4() - if !assert.NoError(c, err, "failed to get IPv4 for subRouter2") { + ip, err := subRouter1.IPv4() + if !assert.NoError(c, err, "failed to get IPv4 for subRouter1") { return } assertTracerouteViaIPWithCollect(c, tr, ip) - }, propagationTime, 200*time.Millisecond, "Verifying traceroute still goes through router 2 after route re-enable") + }, propagationTime, 200*time.Millisecond, "Verifying traceroute goes through router 1 after route re-enable") // Validate primary routes table state after router 1 re-approval validatePrimaryRoutes(t, headscale, &types.DebugRoutes{ @@ -1273,19 +1276,20 @@ func TestHASubnetRouterFailover(t *testing.T) { // Router 3 route is still not approved }, PrimaryRoutes: map[string]types.NodeID{ - pref.String(): types.NodeID(mustParseID(MustFindNode(subRouter2.Hostname(), nodes).Id)), + pref.String(): types.NodeID(mustParseID(MustFindNode(subRouter1.Hostname(), nodes).Id)), }, - }, "Router 2 should remain primary after router 1 re-approval") + }, "Router 1 should be primary after router 1 re-approval") checkFailureAndPrintRoutes(t, client) - // Enable route on node 3, we now have all routes re-enabled + // Enable route on node 3, we now have all routes re-enabled. Router 3 + // never reconnected, so its session is the oldest and it takes the route. t.Logf("=== ROUTE RE-ENABLE TEST: Re-approving route on router 3 (%s) - Full HA Restoration ===", subRouter3.Hostname()) t.Logf("[%s] Starting test section", time.Now().Format(TimestampFormat)) - t.Logf(" Current state: Router 1 STANDBY, Router 2 PRIMARY, Router 3 advertised-only") + t.Logf(" Current state: Router 1 PRIMARY, Router 2 STANDBY, Router 3 advertised-only") t.Logf(" Action: Re-enabling route approval on router 3") - t.Logf(" Expected: Router 2 (%s) remains PRIMARY (stability preferred)", subRouter2.Hostname()) - t.Logf(" Expected: Routers 1 & 3 are both STANDBY") + t.Logf(" Expected: Router 3 (%s) becomes PRIMARY (oldest control session)", subRouter3.Hostname()) + t.Logf(" Expected: Routers 1 & 2 are both STANDBY") t.Logf(" Expected: Full HA restored with all 3 routers available") r3Node := MustFindNode(subRouter3.Hostname(), nodes) @@ -1300,11 +1304,11 @@ func TestHASubnetRouterFailover(t *testing.T) { assert.NoError(c, err) assert.Len(c, nodes, 6) require.GreaterOrEqual(t, len(nodes), 3, "need at least 3 nodes to avoid panic") - // After router 3 re-approval: Router 2 remains PRIMARY, Routers 1&3 are STANDBY + // After router 3 re-approval: Router 3 is PRIMARY, Routers 1&2 are STANDBY // SubnetRoutes should only show routes for PRIMARY node (actively serving) requireNodeRouteCountWithCollect(c, nodes[0], 1, 1, 0) // Router 1: STANDBY (available, approved, but not serving) - requireNodeRouteCountWithCollect(c, nodes[1], 1, 1, 1) // Router 2: PRIMARY (available, approved, and serving) - requireNodeRouteCountWithCollect(c, nodes[2], 1, 1, 0) // Router 3: STANDBY (available, approved, but not serving) + requireNodeRouteCountWithCollect(c, nodes[1], 1, 1, 0) // Router 2: STANDBY (available, approved, but not serving) + requireNodeRouteCountWithCollect(c, nodes[2], 1, 1, 1) // Router 3: PRIMARY (available, approved, and serving) }, propagationTime, 200*time.Millisecond, "Waiting for route state after router 3 re-approval") // Validate primary routes table state after router 3 re-approval @@ -1315,9 +1319,9 @@ func TestHASubnetRouterFailover(t *testing.T) { types.NodeID(mustParseID(MustFindNode(subRouter3.Hostname(), nodes).Id)): {pref}, }, PrimaryRoutes: map[string]types.NodeID{ - pref.String(): types.NodeID(mustParseID(MustFindNode(subRouter2.Hostname(), nodes).Id)), + pref.String(): types.NodeID(mustParseID(MustFindNode(subRouter3.Hostname(), nodes).Id)), }, - }, "Router 2 should remain primary after router 3 re-approval") + }, "Router 3 should be primary after router 3 re-approval") checkFailureAndPrintRoutes(t, client) } @@ -2166,6 +2170,38 @@ func MustFindNode(hostname string, nodes []*clientv1.Node) *clientv1.Node { panic("node not found") } +// orderRouterSessions gives each router after the first a new control +// session, one after another, so the oldest session, which the primary +// election prefers, follows the order given. Clients log in concurrently, +// which leaves that order to chance. +func orderRouterSessions(t *testing.T, headscale ControlServer, routers ...TailscaleClient) { + t.Helper() + + waitOnline := func(r TailscaleClient, online bool) { + t.Helper() + require.EventuallyWithT(t, func(c *assert.CollectT) { + nodes, err := headscale.ListNodes() + assert.NoError(c, err) + assert.Equal(c, online, MustFindNode(r.Hostname(), nodes).Online) + }, integrationutil.HAConvergeTimeout, 500*time.Millisecond, + "%s should have online=%t", r.Hostname(), online) + } + + for _, r := range routers[1:] { + require.NoError(t, r.Down()) + } + + // A session ends only once the server's reconnect grace has passed. + for _, r := range routers[1:] { + waitOnline(r, false) + } + + for _, r := range routers[1:] { + require.NoError(t, r.Up()) + waitOnline(r, true) + } +} + // TestAutoApproveMultiNetwork tests auto approving of routes // by setting up two networks where network1 has three subnet // routers: @@ -3691,6 +3727,8 @@ func TestHASubnetRouterPingFailover(t *testing.T) { subRouter2 := allClients[1] client := allClients[2] + orderRouterSessions(t, headscale, subRouter1, subRouter2) + t.Logf("Router 1: %s, Router 2: %s, Client: %s", subRouter1.Hostname(), subRouter2.Hostname(), client.Hostname()) @@ -3827,32 +3865,31 @@ func TestHASubnetRouterPingFailover(t *testing.T) { _, _, err = subRouter1.Execute(iptablesDel) require.NoError(t, err, "failed to remove iptables rule") - // Wait for the prober to detect recovery. + // Wait for the prober to detect recovery. Router 1 kept its control + // session, the older one, so it takes the route back. assert.EventuallyWithT(t, func(c *assert.CollectT) { pr, err := headscale.PrimaryRoutes() assert.NoError(c, err) - // Router 1 should be healthy again but NOT primary (no flapping). assert.Equal(c, map[string]types.NodeID{ - pref.String(): nodeID2, - }, pr.PrimaryRoutes, "router 2 should remain primary (no flapping)") + pref.String(): nodeID1, + }, pr.PrimaryRoutes, "router 1 should be primary again") assert.Empty(c, pr.UnhealthyNodes, "no nodes should be unhealthy after recovery") - }, propagationTime, 1*time.Second, "waiting for recovery without flapping") + }, propagationTime, 1*time.Second, "waiting for recovery") - // Traffic should still go through router 2 (stability). assert.EventuallyWithT(t, func(c *assert.CollectT) { tr, err := client.Traceroute(webip) assert.NoError(c, err) - ip, err := subRouter2.IPv4() + ip, err := subRouter1.IPv4() if !assert.NoError(c, err) { return } assertTracerouteViaIPWithCollect(c, tr, ip) - }, propagationTime, 200*time.Millisecond, "traceroute should still go through router 2 after recovery") + }, propagationTime, 200*time.Millisecond, "traceroute should go through router 1 after recovery") } // TestHASubnetRouterFailoverBothOffline reproduces issue #3203: @@ -3931,6 +3968,8 @@ func TestHASubnetRouterFailoverBothOffline(t *testing.T) { subRouter2 := allClients[1] client := allClients[2] + orderRouterSessions(t, headscale, subRouter1, subRouter2) + t.Logf("Router 1: %s, Router 2: %s, Client: %s", subRouter1.Hostname(), subRouter2.Hostname(), client.Hostname()) @@ -3969,7 +4008,7 @@ func TestHASubnetRouterFailoverBothOffline(t *testing.T) { nodeID1 := types.NodeID(mustParseID(MustFindNode(subRouter1.Hostname(), nodes).Id)) nodeID2 := types.NodeID(mustParseID(MustFindNode(subRouter2.Hostname(), nodes).Id)) - // Sanity: r1 starts as primary (lower NodeID). + // Sanity: r1 starts as primary (older session). assert.EventuallyWithT(t, func(c *assert.CollectT) { pr, err := headscale.PrimaryRoutes() assert.NoError(c, err) @@ -4146,6 +4185,8 @@ func TestHASubnetRouterFailoverBothOfflineCablePull(t *testing.T) { subRouter2 := allClients[1] client := allClients[2] + orderRouterSessions(t, headscale, subRouter1, subRouter2) + for _, r := range []TailscaleClient{subRouter1, subRouter2} { _, _, err = r.Execute([]string{ "tailscale", "set", "--advertise-routes=" + pref.String(), @@ -4290,7 +4331,7 @@ func TestHASubnetRouterFailoverBothOfflineCablePull(t *testing.T) { // disconnect leaves the long-poll TCP half-open at the peer. // // Phases: -// 1. r1 starts as primary (lowest NodeID). +// 1. r1 starts as primary (older session). // 2. r1 alone fails and recovers — failover to r2, then traffic // resumes when r1 returns. // 3. r2 alone fails and recovers — failover, then traffic resumes. @@ -4298,9 +4339,10 @@ func TestHASubnetRouterFailoverBothOfflineCablePull(t *testing.T) { // 4a. r1 down → r2 promoted. // 4b. r2 down → primary must NOT flap to offline r1. // 4c. r2 up → r2 primary again, traffic resumes. +// 4d. r1 up → the older session is primary. // 5. Simultaneous dual failure. -// 5a. r1 + r2 down → primary must NOT flap to offline r1. -// 5b. both up → primary stays r2, traffic resumes. +// 5a. r1 + r2 down → primary must not move. +// 5b. both up → the older session is primary, traffic resumes. // // The no-flap assertions in 4b and 5a are the regression barriers // for #3203. Phases 2/3 are functional checks (failover works, @@ -4370,6 +4412,8 @@ func TestHASubnetRouterFailoverDockerDisconnect(t *testing.T) { subRouter2 := allClients[1] client := allClients[2] + orderRouterSessions(t, headscale, subRouter1, subRouter2) + for _, r := range []TailscaleClient{subRouter1, subRouter2} { _, _, err = r.Execute([]string{ "tailscale", "set", "--advertise-routes=" + pref.String(), @@ -4430,8 +4474,7 @@ func TestHASubnetRouterFailoverDockerDisconnect(t *testing.T) { } // requirePrimaryStable asserts primary == want for the entire - // window. Catches transient flaps and verifies anti-flap on - // prev-primary return. + // window. Catches transient flaps. requirePrimaryStable := func(want types.NodeID, window time.Duration, msg string) { t.Helper() require.Never(t, func() bool { @@ -4446,8 +4489,47 @@ func TestHASubnetRouterFailoverDockerDisconnect(t *testing.T) { }, window, 1*time.Second, msg) } + // requireOlderSessionPrimary blocks until both routers are online and + // healthy and the one with the older control session is primary, and + // returns it. Whether a cable pull ends a session depends on when the + // server notices the dead connection, so the test reads the order. + requireOlderSessionPrimary := func(msg string) types.NodeID { + t.Helper() + + var want types.NodeID + + assert.EventuallyWithT(t, func(c *assert.CollectT) { + store, err := headscale.DebugNodeStore() + if !assert.NoError(c, err) { + return + } + + r1, r2 := store[nodeID1], store[nodeID2] + for _, n := range []*types.Node{&r1, &r2} { + assert.True(c, n.Online(), "%s: node %d online", msg, n.ID) + assert.False(c, n.Unhealthy, "%s: node %d healthy", msg, n.ID) + } + + want = nodeID1 + if r2.SessionStart.Before(r1.SessionStart) { + want = nodeID2 + } + + pr, err := headscale.PrimaryRoutes() + if !assert.NoError(c, err) { + return + } + + assert.Equal(c, map[string]types.NodeID{ + pref.String(): want, + }, pr.PrimaryRoutes, msg) + }, propagationTime, 1*time.Second, msg) + + return want + } + // ============================================================ - // Phase 1: initial state — r1 (lowest NodeID) is primary. + // Phase 1: initial state — r1 (older session) is primary. // ============================================================ t.Log("=== Phase 1: initial state — r1 should be primary. ===") requirePrimary(nodeID1, "phase 1: r1 primary at start") @@ -4510,28 +4592,31 @@ func TestHASubnetRouterFailoverDockerDisconnect(t *testing.T) { requirePrimary(nodeID2, "phase 4c: r2 primary after reconnect") requireTrafficWorks("phase 4c: client reaches webservice via r2 after recovery") - t.Log("=== Phase 4d: reconnect r1, traffic should still flow. ===") + t.Log("=== Phase 4d: reconnect r1, the older session should be primary. ===") require.NoError(t, subRouter1.ReconnectToNetwork(usernet1), "phase 4d: docker reconnect r1") + + prev := requireOlderSessionPrimary("phase 4d: older session primary after r1 returns") + requireTrafficWorks("phase 4d: client still reaches webservice") // ============================================================ // Phase 5: simultaneous dual failure (whole-segment outage). - // prev going in is r2 — primary must not flap to offline r1. + // The primary going in must keep the route while both are down. // ============================================================ t.Log("=== Phase 5a: cable-pull r1 and r2 simultaneously. ===") require.NoError(t, subRouter1.DisconnectFromNetwork(usernet1), "phase 5a: docker disconnect r1") require.NoError(t, subRouter2.DisconnectFromNetwork(usernet1), "phase 5a: docker disconnect r2") - requirePrimaryStable(nodeID2, flapWindow, - "phase 5a: primary must not flap to offline r1 (issue #3203)") + requirePrimaryStable(prev, flapWindow, + "phase 5a: primary must not move while both routers are down (issue #3203)") - t.Log("=== Phase 5b: reconnect both, r2 should remain primary. ===") + t.Log("=== Phase 5b: reconnect both, the older session should be primary. ===") require.NoError(t, subRouter1.ReconnectToNetwork(usernet1), "phase 5b: docker reconnect r1") require.NoError(t, subRouter2.ReconnectToNetwork(usernet1), "phase 5b: docker reconnect r2") - requirePrimary(nodeID2, "phase 5b: r2 primary after both reconnect") - requireTrafficWorks("phase 5b: client reaches webservice via r2") + requireOlderSessionPrimary("phase 5b: older session primary after both reconnect") + requireTrafficWorks("phase 5b: client reaches webservice") }