diff --git a/hscontrol/servertest/expiry_compat_test.go b/hscontrol/servertest/expiry_compat_test.go index a87394e8a..faf002b06 100644 --- a/hscontrol/servertest/expiry_compat_test.go +++ b/hscontrol/servertest/expiry_compat_test.go @@ -78,37 +78,30 @@ var expirySkipReasons = map[string]string{ "expiry/A5-advertise/laptop/online-observer": expiredPeerOffline, "expiry/A5-advertise/laptop/paths-observer": expiredPeerPaths, - "expiry/A5-advertise/laptop/routes-observer": expiredRouterUnserved, "expiry/A5-advertise/laptop/self": selfExpiredFlags, "expiry/A7-dns/laptop/dns": nameserversFromConfig, "expiry/A7-dns/laptop/online-observer": expiredPeerOffline, "expiry/A7-dns/laptop/paths-observer": expiredPeerPaths, - "expiry/A7-dns/laptop/routes-observer": expiredRouterUnserved, "expiry/A7-dns/laptop/self": selfExpiredFlags, "expiry/A7-dns/observer/dns": nameserversFromConfig, "expiry/A8-policy/laptop/online-observer": expiredPeerOffline, "expiry/A8-policy/laptop/paths-observer": expiredPeerPaths, - "expiry/A8-policy/laptop/routes-observer": expiredRouterUnserved, "expiry/A8-policy/laptop/self": selfExpiredFlags, "expiry/A9-ssh/laptop/online-observer": expiredPeerOffline, "expiry/A9-ssh/laptop/paths-observer": expiredPeerPaths, - "expiry/A9-ssh/laptop/routes-observer": expiredRouterUnserved, "expiry/A9-ssh/laptop/self": selfExpiredFlags, "expiry/A10-reconnect/laptop/online-observer": expiredPeerOffline, "expiry/A10-reconnect/laptop/paths-observer": expiredPeerPaths, - "expiry/A10-reconnect/laptop/routes-observer": expiredRouterUnserved, "expiry/A10-reconnect/laptop/self": selfExpiredFlags, "expiry/A6a-expire-again/laptop/online-observer": expiredPeerOffline, "expiry/A6a-expire-again/laptop/online-peer1": expiredPeerOffline, "expiry/A6a-expire-again/laptop/paths-observer": expiredPeerPaths, "expiry/A6a-expire-again/laptop/paths-peer1": expiredPeerPaths, - "expiry/A6a-expire-again/laptop/routes-observer": expiredRouterUnserved, - "expiry/A6a-expire-again/laptop/routes-peer1": expiredRouterUnserved, "expiry/A6a-expire-again/laptop/self": selfExpiredFlags, "expiry/A6b-rotate-disco/laptop/online-observer": expiredPeerOffline, @@ -117,9 +110,6 @@ var expirySkipReasons = map[string]string{ "expiry/A6b-rotate-disco/laptop/paths-observer": expiredPeerPaths, "expiry/A6b-rotate-disco/laptop/paths-peer1": expiredPeerPaths, "expiry/A6b-rotate-disco/laptop/paths-peer3": expiredPeerPaths, - "expiry/A6b-rotate-disco/laptop/routes-observer": expiredRouterUnserved, - "expiry/A6b-rotate-disco/laptop/routes-peer1": expiredRouterUnserved, - "expiry/A6b-rotate-disco/laptop/routes-peer3": expiredRouterUnserved, "expiry/A6b-rotate-disco/laptop/self": selfExpiredFlags, } diff --git a/hscontrol/servertest/ha_property_test.go b/hscontrol/servertest/ha_property_test.go index 565e36af2..16e6cc0a2 100644 --- a/hscontrol/servertest/ha_property_test.go +++ b/hscontrol/servertest/ha_property_test.go @@ -153,8 +153,8 @@ func checkHAInvariants( st := srv.State() primaries := readPrimaries(rt, srv, clients) - // Collect every prefix advertised by the fleet so the all-down - // honesty check can iterate them, even prefixes with zero current + // Collect every prefix advertised by the fleet so the no-router + // check can iterate them, even prefixes with zero current // advertisers. prefixSet := make(map[netip.Prefix]struct{}) for _, c := range clients { @@ -167,7 +167,10 @@ func checkHAInvariants( // prober's mapBatcher.IsConnected view is a separate gate the // prober consults before dispatching a probe, not part of the // election input. + // approversByPrefix also holds the offline routers, which keep a + // prefix while no candidate can serve it. advertisersByPrefix := make(map[netip.Prefix][]types.NodeID) + approversByPrefix := make(map[netip.Prefix][]types.NodeID) for _, c := range clients { nv, ok := st.GetNodeByID(c.id) @@ -176,12 +179,15 @@ func checkHAInvariants( } online, known := nv.IsOnline().GetOk() - if !known || !online { - continue - } for _, p := range nv.AllApprovedRoutes() { - if p == c.route { + if p != c.route { + continue + } + + approversByPrefix[p] = append(approversByPrefix[p], c.id) + + if known && online { advertisersByPrefix[p] = append(advertisersByPrefix[p], c.id) } } @@ -207,20 +213,24 @@ func checkHAInvariants( ) } - // Invariant 1: primary IsOnline in the NodeStore view. - // state.Disconnect's 10s grace can leave a recently-disconnected - // node with IsOnline=true; the structural rule still requires - // the bit before the snapshot may keep it as primary. + // Invariant 1: primary IsOnline in the NodeStore view while any + // candidate is healthy. state.Disconnect's 10s grace can leave a + // recently-disconnected node with IsOnline=true; the structural + // rule still requires the bit before the snapshot may keep it as + // primary over a healthy candidate. + healthyCandidate := slices.ContainsFunc(advertisersByPrefix[prefix], st.IsNodeHealthy) + online, known := nv.IsOnline().GetOk() - if !known || !online { + if (!known || !online) && healthyCandidate { rt.Fatalf( - "prefix %s primary %d is not online (IsOnline=%v known=%v)", - prefix, owner, online, known, + "prefix %s primary %d is not online (IsOnline=%v known=%v) "+ + "but %v has a healthy candidate", + prefix, owner, online, known, advertisersByPrefix[prefix], ) } - // Invariant 2: primary advertises the prefix and is among the - // snapshot's candidate set. + // Invariant 2: primary advertises the prefix and, while any + // candidate is healthy, is among the snapshot's candidate set. approved := nv.AllApprovedRoutes() if !slices.Contains(approved, prefix) { rt.Fatalf( @@ -229,7 +239,7 @@ func checkHAInvariants( ) } - if !slices.Contains(advertisersByPrefix[prefix], owner) { + if healthyCandidate && !slices.Contains(advertisersByPrefix[prefix], owner) { rt.Fatalf( "prefix %s primary %d not in advertiser set %v", prefix, owner, advertisersByPrefix[prefix], @@ -265,72 +275,49 @@ func checkHAInvariants( } } - // Invariant 6 (all-down honesty): if every candidate of a prefix is - // either offline OR unhealthy, the snapshot must either drop the - // primary OR keep one that is still a candidate from the same set - // (the all-unhealthy preserve-prev rule). A primary pointed at a - // non-candidate after every candidate has gone dark would point - // peers at a node the prober has already declared unreachable. + // Invariant 6 (no router available): with no healthy online + // candidate the prefix stays mapped. The previous primary keeps it + // while it still approves the prefix; otherwise the primary must be + // a router the prober has not declared unreachable, or there is none. for prefix := range prefixSet { owner, hasPrimary := primaries[prefix] - if !hasPrimary { - continue - } - candidates := advertisersByPrefix[prefix] - - // Empty candidates with a primary present trips invariant 2 - // above; reaching this branch with len(candidates) == 0 means - // the snapshot is internally inconsistent. - if len(candidates) == 0 { - rt.Fatalf( - "prefix %s has primary %d but no advertiser in the snapshot", - prefix, owner, - ) - } - anyHealthy := slices.ContainsFunc(candidates, st.IsNodeHealthy) // Healthy preference: a primary should not be unhealthy when a // healthy candidate exists. Asserting this through the real // prober → State → NodeStore seam catches any divergence the // unit-level model would miss. - if anyHealthy && !st.IsNodeHealthy(owner) { + if hasPrimary && anyHealthy && !st.IsNodeHealthy(owner) { rt.Fatalf( "prefix %s primary %d unhealthy but %v has a healthy candidate", prefix, owner, candidates, ) } - // All-unhealthy guards: with every candidate unhealthy, the - // election has exactly two correct choices: - // - preserve prev primary when it is still a candidate - // - leave the prefix unmapped (no candidate fallback) - // - // Any other outcome points peers at a node already declared - // unreachable. - if !anyHealthy { - prevOwner, hadPrev := prevPrimaries[prefix] - prevStillCandidate := hadPrev && - slices.Contains(candidates, prevOwner) + if anyHealthy { + continue + } - if prevStillCandidate && owner != prevOwner { - rt.Fatalf( - "prefix %s all-unhealthy election picked %d, "+ - "but prev primary %d is still a candidate — "+ - "preserve-prev rule violated", - prefix, owner, prevOwner, - ) - } + prevOwner, hadPrev := prevPrimaries[prefix] + prevStillApproves := hadPrev && + slices.Contains(approversByPrefix[prefix], prevOwner) - if !prevStillCandidate && (!hadPrev || owner != prevOwner) { - rt.Fatalf( - "prefix %s all-unhealthy fallback elected %d "+ - "but prev primary %v is gone; election "+ - "must leave the prefix unmapped", - prefix, owner, prevOwner, - ) - } + if prevStillApproves && owner != prevOwner { + rt.Fatalf( + "prefix %s with no router available elected %d, "+ + "but prev primary %d still approves it", + prefix, owner, prevOwner, + ) + } + + if !prevStillApproves && hasPrimary && owner != prevOwner && + !st.IsNodeHealthy(owner) { + rt.Fatalf( + "prefix %s with no router available moved to %d, "+ + "which the prober declared unreachable", + prefix, owner, + ) } } } @@ -367,14 +354,14 @@ func snapshotPrimaries( // so a reconnect mid-cycle does not install a stale Unhealthy. // // - The batched BatchSetNodeHealth is observable through the -// all-unhealthy preserve-prev invariant: per-call writes would +// no-router preserve-prev invariant: per-call writes would // publish an intermediate "one unhealthy, one healthy" snapshot, // leaving primary on a non-prev node after both flips land. // -// - The all-unhealthy fallback that leaves prefixes unmapped is -// covered by the honesty invariant: with every candidate -// unhealthy AND prev gone from the candidate set, the prefix -// must stay unmapped instead of pinned to any candidate. +// - The fallback with no router available is covered by the +// no-router invariant: the previous primary keeps the prefix +// while it still approves it, and the fallback never moves it +// to a router the prober declared unreachable. // // Caveat: TestClient is a real controlclient.Direct that responds to // PingRequest over Noise, so the timeout path of the prober rarely diff --git a/hscontrol/servertest/lifecycle_compat_test.go b/hscontrol/servertest/lifecycle_compat_test.go index b04fa5e19..715cc22c1 100644 --- a/hscontrol/servertest/lifecycle_compat_test.go +++ b/hscontrol/servertest/lifecycle_compat_test.go @@ -51,8 +51,6 @@ var lifecycleSkipReasons = map[string]string{ "route-approval/b9/served": approvalOnSameKeyReauth, "route-approval/b9/self-routes": approvalOnSameKeyReauth, - "route-approval/r1/served": loggedOutRouterUnserved, - "ephemeral-lifecycle/E1-logout/eph-logout/outcome": logoutAnswerUnauthorized, "ephemeral-lifecycle/E1-logout/eph-logout-user/outcome": logoutAnswerUnauthorized, @@ -81,7 +79,6 @@ const ( approvalOnHostinfo = "APPROVAL_ON_HOSTINFO: headscale auto-approves when a router with no approved routes sends a Hostinfo change that keeps its routes; SaaS does not" approvalOnPolicy = "APPROVAL_ON_POLICY: headscale auto-approves announced routes when a policy is set; SaaS waits for the router's next route change or key rotation" approvalOnSameKeyReauth = "APPROVAL_ON_SAME_KEY_REAUTH: headscale auto-approves on a re-auth that keeps the node key; SaaS does not" - loggedOutRouterUnserved = "LOGGED_OUT_ROUTER_UNSERVED: headscale stops serving a logged-out router's routes; SaaS keeps them on the expired, offline peer" ) const ( @@ -93,8 +90,7 @@ const ( // Used by reauthSkipReasons. const ( - expiredRouterUnserved = "EXPIRED_ROUTER_UNSERVED: headscale stops serving an expired router's routes; SaaS keeps them in the peer's AllowedIPs and PrimaryRoutes" - expiredPeerOffline = "EXPIRED_PEER_OFFLINE: headscale shows an expired peer offline while it is connected; SaaS shows it online" + expiredPeerOffline = "EXPIRED_PEER_OFFLINE: headscale shows an expired peer offline while it is connected; SaaS shows it online" ) // TestRouteApprovalCompat replays a user-owned router through admin diff --git a/hscontrol/servertest/reauth_compat_test.go b/hscontrol/servertest/reauth_compat_test.go index a7b58681d..03c26d2c7 100644 --- a/hscontrol/servertest/reauth_compat_test.go +++ b/hscontrol/servertest/reauth_compat_test.go @@ -37,7 +37,6 @@ import ( // /[/]/. A listed check that matches fails, so a // fix must delete its entry. var reauthSkipReasons = map[string]string{ - "owner-routes/r-expired-retag/expire/served": expiredRouterUnserved, "owner-routes/r-expired-retag/expire/online": expiredPeerOffline, "owner-routes/r-expired-retag/owner/node-key": expiredKeyAccepted, diff --git a/hscontrol/servertest/routes_test.go b/hscontrol/servertest/routes_test.go index 5c2bdacdb..6c3f8529a 100644 --- a/hscontrol/servertest/routes_test.go +++ b/hscontrol/servertest/routes_test.go @@ -307,11 +307,14 @@ func TestRoutes(t *testing.T) { 20*time.Second, 200*time.Millisecond, "r2 should take over as primary after r1 offline") - // 2. Take r2 offline. With both routers gone, no primary should remain. + // 2. Take r2 offline. With both routers gone, r2 keeps the route. r2.Disconnect(t) - assert.Eventually(t, func() bool { return !hasRoute(r1ID) && !hasRoute(r2ID) }, - 20*time.Second, 200*time.Millisecond, - "no primary should be assigned while both routers are offline") + assert.Eventually(t, func() bool { + nv, ok := srv.State().GetNodeByID(r2ID) + + return ok && !nv.IsOnline().Get() && hasRoute(r2ID) && !hasRoute(r1ID) + }, 20*time.Second, 200*time.Millisecond, + "the last primary keeps the route while both routers are offline") // 3. Reconnect r2 (cable plugged back in). r2.Reconnect(t) diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index 926352f55..52cfc8b46 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -788,18 +788,19 @@ func servesRoutes(n *types.Node) bool { return n.Online() && !n.IsExpired() } -// onlineAdvertisers maps each non-exit prefix to the IDs of online, -// unexpired nodes that approve it. Nodes are visited in the order of ids, so -// the per-prefix slices preserve that order. -func onlineAdvertisers( +// advertisers maps each non-exit prefix to the IDs of nodes that approve it +// and pass keep. Nodes are visited in the order of ids, so the per-prefix +// slices preserve that order. +func advertisers( nodes map[types.NodeID]types.Node, ids []types.NodeID, + keep func(*types.Node) bool, ) map[netip.Prefix][]types.NodeID { - advertisers := make(map[netip.Prefix][]types.NodeID) + out := make(map[netip.Prefix][]types.NodeID) for _, id := range ids { n := nodes[id] - if !servesRoutes(&n) { + if !keep(&n) { continue } @@ -808,65 +809,63 @@ func onlineAdvertisers( continue } - advertisers[p] = append(advertisers[p], id) + out[p] = append(out[p], id) } } - return advertisers + return out } -// electPrimaryRoutes picks the primary advertiser for each non-exit -// prefix. Inputs are restricted to online nodes that advertise the -// prefix. The previous primary is preserved when it is still online -// and healthy (anti-flap); otherwise the lowest-NodeID healthy -// advertiser wins. When every advertiser is unhealthy the previous -// primary is preserved only if still a candidate — falling back to -// any other candidate would point peers at a node the prober has -// already declared unreachable, so leaving the prefix unmapped is -// preferred until a probe cycle finds one that responds. +// 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. +// +// 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 +// still approves the prefix, else the lowest-NodeID approving router does. +// That fallback never picks a router the prober declared unreachable; with +// only such routers left the prefix is unmapped until a probe cycle finds one +// that responds. func electPrimaryRoutes( nodes map[types.NodeID]types.Node, prev map[netip.Prefix]types.NodeID, ) (map[netip.Prefix]types.NodeID, map[types.NodeID]bool) { - advertisers := onlineAdvertisers(nodes, slices.Sorted(maps.Keys(nodes))) + approvers := advertisers(nodes, slices.Sorted(maps.Keys(nodes)), + func(*types.Node) bool { return true }) - routes := make(map[netip.Prefix]types.NodeID, len(advertisers)) - for prefix, candidates := range advertisers { - if cur, ok := prev[prefix]; ok && - slices.Contains(candidates, cur) && - !nodes[cur].Unhealthy { + available := func(id types.NodeID) bool { + n := nodes[id] + + return servesRoutes(&n) && !n.Unhealthy + } + healthy := func(id types.NodeID) bool { return !nodes[id].Unhealthy } + + routes := make(map[netip.Prefix]types.NodeID, len(approvers)) + for prefix, ids := range approvers { + cur, hasPrev := prev[prefix] + hasPrev = hasPrev && slices.Contains(ids, cur) + + if hasPrev && available(cur) { routes[prefix] = cur + continue } - var ( - selected types.NodeID - found bool - ) + if i := slices.IndexFunc(ids, available); i >= 0 { + routes[prefix] = ids[i] - for _, c := range candidates { - if !nodes[c].Unhealthy { - selected = c - found = true - - break - } + continue } - // All-unhealthy fallback: preserve the previous primary only - // when it is still a candidate. Falling back to any candidate - // would point peers at a node the prober has already declared - // unreachable; leaving the prefix unmapped is honest until a - // probe cycle picks one that responds. - if !found { - if cur, ok := prev[prefix]; ok && slices.Contains(candidates, cur) { - selected = cur - found = true - } + if hasPrev { + routes[prefix] = cur + + continue } - if found { - routes[prefix] = selected + if i := slices.IndexFunc(ids, healthy); i >= 0 { + routes[prefix] = ids[i] } } @@ -1061,9 +1060,10 @@ func (s *NodeStore) PrimaryRoutesForNode(id types.NodeID) []netip.Prefix { func (s *NodeStore) HANodes() map[netip.Prefix][]types.NodeID { snap := s.data.Load() - advertisers := onlineAdvertisers( + online := advertisers( snap.nodesByID, slices.Sorted(maps.Keys(snap.nodesByID)), + servesRoutes, ) unhealthy := func(id types.NodeID) bool { @@ -1072,7 +1072,7 @@ func (s *NodeStore) HANodes() map[netip.Prefix][]types.NodeID { out := make(map[netip.Prefix][]types.NodeID) - for p, ids := range advertisers { + for p, ids := range online { if len(ids) < 2 && !slices.ContainsFunc(ids, unhealthy) { continue } diff --git a/hscontrol/state/primaries_property_test.go b/hscontrol/state/primaries_property_test.go index 6507e2350..bca17ac92 100644 --- a/hscontrol/state/primaries_property_test.go +++ b/hscontrol/state/primaries_property_test.go @@ -91,11 +91,23 @@ func (m *primariesModel) recomputeEffective(id types.NodeID) { func (m *primariesModel) advertisersByPrefix() map[netip.Prefix][]types.NodeID { out := map[netip.Prefix][]types.NodeID{} - for n, prefs := range m.prefixes { - if !m.connected[n] { - continue + for p, nodes := range m.approversByPrefix() { + for _, n := range nodes { + if m.connected[n] { + out[p] = append(out[p], n) + } } + } + return out +} + +// approversByPrefix returns every node that approves each prefix, connected +// or not, sorted by NodeID. +func (m *primariesModel) approversByPrefix() map[netip.Prefix][]types.NodeID { + out := map[netip.Prefix][]types.NodeID{} + + for n, prefs := range m.prefixes { for _, p := range prefs { out[p] = append(out[p], n) } @@ -108,78 +120,57 @@ func (m *primariesModel) advertisersByPrefix() map[netip.Prefix][]types.NodeID { return out } +// 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] +} + // updatePrimaries reapplies the algorithm to recompute the primary // for each prefix. Called after every operation. func (m *primariesModel) updatePrimaries() { - advertisers := m.advertisersByPrefix() + approvers := m.approversByPrefix() - // Drop primaries for prefixes that no longer have any advertiser. + // Drop primaries for prefixes no node approves any more. for p := range m.primary { - if _, ok := advertisers[p]; !ok { + if _, ok := approvers[p]; !ok { delete(m.primary, p) } } - for p, nodes := range advertisers { - if cur, ok := m.primary[p]; ok { - if slices.Contains(nodes, cur) && !m.unhealthy[cur] { - continue - } + for p, nodes := range approvers { + cur, hasPrev := m.primary[p] + hasPrev = hasPrev && slices.Contains(nodes, cur) + + if hasPrev && m.available(cur) { + continue } - var ( - selected types.NodeID - found bool - ) + if i := slices.IndexFunc(nodes, m.available); i >= 0 { + m.primary[p] = nodes[i] - for _, n := range nodes { - if !m.unhealthy[n] { - selected = n - found = true - - break - } + continue } - // All-unhealthy fallback: preserve the previous primary if it - // is still a candidate, otherwise leave the prefix unmapped. - // Choosing any candidate would point peers at a node already - // declared unreachable; the model mirrors that policy. - if !found && len(nodes) >= 1 { - if cur, ok := m.primary[p]; ok && slices.Contains(nodes, cur) { - selected = cur - found = true - } + // 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. + if hasPrev { + continue } - if found { - m.primary[p] = selected + if i := slices.IndexFunc(nodes, func(n types.NodeID) bool { + return !m.unhealthy[n] + }); i >= 0 { + m.primary[p] = nodes[i] } else { delete(m.primary, p) } } } -// allPrefixes returns every prefix mentioned by any connected node. +// allPrefixes returns every prefix any node approves. func (m *primariesModel) allPrefixes() []netip.Prefix { - seen := map[netip.Prefix]bool{} - - for n, prefs := range m.prefixes { - if !m.connected[n] { - continue - } - - for _, p := range prefs { - seen[p] = true - } - } - - out := make([]netip.Prefix, 0, len(seen)) - for p := range seen { - out = append(out, p) - } - - return out + return slices.Collect(maps.Keys(m.approversByPrefix())) } func samePrefixSet(a, b []netip.Prefix) bool { @@ -233,10 +224,9 @@ func checkPrimariesProperties( } } - // Every prefix that has at least one connected advertiser must - // have a primary in the snapshot: a prefix silently losing its - // primary after a disconnect/reconnect cycle would leave peers - // without a route. + // Every prefix the model maps must have the same primary in the + // snapshot: a prefix silently losing its primary after a + // disconnect/reconnect cycle would leave peers without a route. for _, p := range m.allPrefixes() { want, expectExists := m.primary[p] if !expectExists { @@ -246,7 +236,7 @@ func checkPrimariesProperties( got, ok := ns.PrimaryRouteFor(p) if !ok { rt.Fatalf( - "prefix %s has at least one advertiser in the model but no primary in NodeStore", + "prefix %s has a primary in the model but none in NodeStore", p, ) } @@ -261,7 +251,7 @@ func checkPrimariesProperties( // Structural invariants on the live snapshot, independent of the // model. These catch shapes the model alone cannot — e.g. an owner - // that is offline but still has the prefix attributed to it. + // that no longer approves the prefix attributed to it. snapshotPrimaries := ns.PrimaryRoutes() // A primary that owns ≥1 prefix in `routes` must also light up @@ -283,9 +273,11 @@ func checkPrimariesProperties( } } - // Per-owner structural checks: every primary must currently be - // online and must currently advertise the prefix it owns. + // Per-owner structural checks: every primary must currently + // advertise the prefix it owns, and be available whenever any of + // its approvers is. advertisersByPrefix := m.advertisersByPrefix() + approversByPrefix := m.approversByPrefix() for prefix, owner := range snapshotPrimaries { nv, ok := ns.GetNode(owner) @@ -296,13 +288,14 @@ func checkPrimariesProperties( ) } - // Primary is online: an offline node cannot move packets, so - // election must never leave one as the snapshot's primary. + // An offline primary is only allowed while no approver could + // move packets for the prefix. online, known := nv.IsOnline().GetOk() - if !known || !online { + if (!known || !online) && + slices.ContainsFunc(approversByPrefix[prefix], m.available) { rt.Fatalf( - "prefix %s primary %d is not online", - prefix, owner, + "prefix %s primary %d is not online but %v had an available router", + prefix, owner, approversByPrefix[prefix], ) } @@ -317,21 +310,17 @@ func checkPrimariesProperties( ) } - // Healthy preference: if any candidate for this prefix is - // healthy, the elected primary must also be healthy. Leaving - // the prefix unmapped is fine; pointing at an unhealthy - // candidate while a healthy one was available is not. + // Healthy preference: if any approver for this prefix is + // healthy, the elected primary must also be healthy, unless it + // is the previous primary kept while no router is available. + // Leaving the prefix unmapped is fine; moving it to an + // unhealthy router while a healthy one was there is not. candidates := advertisersByPrefix[prefix] - anyHealthy := false + anyHealthy := slices.ContainsFunc(approversByPrefix[prefix], func(n types.NodeID) bool { + return !m.unhealthy[n] + }) - for _, c := range candidates { - if !m.unhealthy[c] { - anyHealthy = true - break - } - } - - if anyHealthy && m.unhealthy[owner] { + if anyHealthy && m.unhealthy[owner] && owner != prevSnapshotPrimaries[prefix] { rt.Fatalf( "prefix %s primary %d is unhealthy but %v had a healthy candidate", prefix, owner, candidates, diff --git a/hscontrol/state/primaries_test.go b/hscontrol/state/primaries_test.go index e09f1cf6a..165616ecc 100644 --- a/hscontrol/state/primaries_test.go +++ b/hscontrol/state/primaries_test.go @@ -172,13 +172,98 @@ func TestPrimaries_ClearRoutesDropsPrimary(t *testing.T) { f.requireNoPrimary(mp("192.168.1.0/24")) } -func TestPrimaries_DisconnectDropsLastAdvertiserPrimary(t *testing.T) { +func TestPrimaries_DisconnectKeepsLastRouterPrimary(t *testing.T) { f := newPrimariesFixture(t, 1) f.advertise(1, mp("192.168.1.0/24")) f.requirePrimary(mp("192.168.1.0/24"), 1) f.disconnect(1) - f.requireNoPrimary(mp("192.168.1.0/24")) + f.requirePrimary(mp("192.168.1.0/24"), 1) +} + +func TestPrimaries_NoRouterAvailable(t *testing.T) { + prefix := mp("10.0.0.0/24") + expire := func(f *primariesFixture, id types.NodeID) { + f.ns.UpdateNode(id, func(n *types.Node) { + n.Expiry = new(time.Now().Add(-time.Minute)) + }) + } + unapprove := func(f *primariesFixture, id types.NodeID) { + f.ns.UpdateNode(id, func(n *types.Node) { n.ApprovedRoutes = nil }) + } + + tests := []struct { + name string + setup func(*primariesFixture) + want types.NodeID + }{ + { + name: "expired router keeps its route", + setup: func(f *primariesFixture) { + f.advertise(1, prefix) + expire(f, 1) + }, + want: 1, + }, + { + name: "previous primary kept over a lower ID", + setup: func(f *primariesFixture) { + f.advertise(1, prefix) + f.advertise(2, prefix) + f.disconnect(1) + f.disconnect(2) + }, + want: 2, + }, + { + name: "lowest ID approver once the previous primary is gone", + setup: func(f *primariesFixture) { + f.advertise(1, prefix) + f.advertise(2, prefix) + f.advertise(3, prefix) + f.disconnect(2) + f.disconnect(3) + f.disconnect(1) + unapprove(f, 1) + }, + want: 2, + }, + { + name: "unhealthy router passed over", + setup: func(f *primariesFixture) { + f.advertise(1, prefix) + f.advertise(2, prefix) + f.advertise(3, prefix) + f.disconnect(2) + f.unhealthy(1) + f.disconnect(3) + unapprove(f, 3) + }, + want: 2, + }, + { + name: "only unhealthy routers left", + setup: func(f *primariesFixture) { + f.advertise(1, prefix) + f.advertise(3, prefix) + f.unhealthy(1) + unapprove(f, 3) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newPrimariesFixture(t, 1, 2, 3) + tt.setup(f) + + if tt.want == 0 { + f.requireNoPrimary(prefix) + } else { + f.requirePrimary(prefix, tt.want) + } + }) + } } func TestPrimaries_UnhealthyTriggersFailover(t *testing.T) { @@ -269,10 +354,9 @@ func TestPrimaries_ExitRouteNotElected(t *testing.T) { } func TestPrimaries_BothOfflineThenOneReturns(t *testing.T) { - // With two HA advertisers, dropping both then bringing one back - // used to leave the prefix without any primary. The snapshot - // recomputes primaries on every NodeStore write, so the - // returning advertiser must be elected. + // With two HA advertisers both offline, the last primary keeps the + // prefix. The snapshot recomputes primaries on every NodeStore + // write, so the first advertiser to return must be elected. prefix := mp("10.0.0.0/24") f := newPrimariesFixture(t, 1, 2) f.advertise(1, prefix) @@ -283,10 +367,10 @@ func TestPrimaries_BothOfflineThenOneReturns(t *testing.T) { f.requirePrimary(prefix, 2) f.disconnect(2) - f.requireNoPrimary(prefix) - - f.advertise(2, prefix) f.requirePrimary(prefix, 2) + + f.advertise(1, prefix) + f.requirePrimary(prefix, 1) } func TestPrimaries_HANodes(t *testing.T) { diff --git a/integration/route_test.go b/integration/route_test.go index 8e4e2ebb2..e66d433d8 100644 --- a/integration/route_test.go +++ b/integration/route_test.go @@ -3996,15 +3996,21 @@ func TestHASubnetRouterFailoverBothOffline(t *testing.T) { }, pr.PrimaryRoutes, "r2 should be primary after r1 offline") }, propagationTime, 500*time.Millisecond, "waiting for failover to r2") - t.Log("=== Step 2: r2 also goes offline. No primary should remain. ===") + t.Log("=== Step 2: r2 also goes offline. r2 keeps the route. ===") require.NoError(t, subRouter2.Down()) assert.EventuallyWithT(t, func(c *assert.CollectT) { + current, err := headscale.ListNodes() + assert.NoError(c, err) + assert.False(c, MustFindNode(subRouter2.Hostname(), current).Online, "r2 should be offline") + pr, err := headscale.PrimaryRoutes() assert.NoError(c, err) - assert.Empty(c, pr.PrimaryRoutes, - "no primary should be assigned while both routers are offline") + assert.Equal(c, map[string]types.NodeID{ + pref.String(): nodeID2, + }, pr.PrimaryRoutes, + "the last primary keeps the route while both routers are offline") }, propagationTime, 500*time.Millisecond, "waiting for both routers to be offline") t.Log("=== Step 3: r2 returns. ===")