diff --git a/hscontrol/policy/pm.go b/hscontrol/policy/pm.go index f59626131..dbb63078a 100644 --- a/hscontrol/policy/pm.go +++ b/hscontrol/policy/pm.go @@ -63,12 +63,19 @@ type PolicyManager interface { // avoid a tailnet-wide recompute on every ordinary reconnect. NodeNeedsPeerRecompute(node types.NodeView) bool + // NodeIsViaTarget reports whether a grant steers traffic through the node. + NodeIsViaTarget(node types.NodeView) bool + // ViaRoutesForPeer computes via grant effects for a viewer-peer pair. // It returns which routes should be included (peer is via-designated for viewer) // and excluded (steered to a different peer). When no via grants apply, // 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. + // The callback runs under the policy lock and must remain lock-free. + SetRouterAvailable(available func(types.NodeID) bool) + // NodeCapMap returns compiled policy capabilities for the given node, // or nil when none target it. The map is a defensive clone; its values // are shared and read-only. It describes the node's own capabilities, diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 92b7463bb..565b4bf3a 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -118,6 +118,8 @@ type PolicyManager struct { // nodeAttrsPending mirrors len(nodeAttrsChanged) > 0 so the drain, // called on every dispatched change, skips pm.mu when idle. nodeAttrsPending atomic.Bool + + routerAvailable func(types.NodeID) bool } // filterAndPolicy combines the compiled filter rules with policy content for hashing. @@ -468,6 +470,22 @@ func (pm *PolicyManager) NodeNeedsPeerRecompute(node types.NodeView) bool { return true } + return pm.isViaTargetLocked(node) +} + +// NodeIsViaTarget reports whether a grant steers traffic through node. +func (pm *PolicyManager) NodeIsViaTarget(node types.NodeView) bool { + if pm == nil || !node.Valid() { + return false + } + + pm.mu.RLock() + defer pm.mu.RUnlock() + + return pm.isViaTargetLocked(node) +} + +func (pm *PolicyManager) isViaTargetLocked(node types.NodeView) bool { for tag := range pm.viaTargetTags { if node.HasTag(string(tag)) { return true @@ -1434,6 +1452,20 @@ 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) { + if pm == nil { + return + } + + pm.mu.Lock() + defer pm.mu.Unlock() + + pm.routerAvailable = available +} + // ViaRoutesForPeer computes via grant effects for a viewer-peer pair. // For each via grant where the viewer matches the source, it checks whether the // peer advertises any of the grant's destination prefixes. If the peer has the @@ -1607,9 +1639,9 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via 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 peer (the via-group primary) keeps the prefix in - // Include. The others move to Exclude. This mirrors HA - // primary election scoped to the via tag group. + // lowest-ID available peer keeps the prefix in Include. The others + // move to Exclude. With no available router, the lowest ID keeps the + // prefix so the viewer still gets exactly one route. // // Unlike the global [tailcfg.Node.PrimaryRoutes] election // (routes/primary.go), which picks one primary across ALL @@ -1639,12 +1671,21 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via continue } - var viaPrimaryID types.NodeID + var viaPrimaryID, viaFallbackID types.NodeID for _, viaTag := range grant.Via { for _, node := range pm.nodes.All() { if node.HasTag(string(viaTag)) && slices.Contains(node.SubnetRoutes(), included) { + if viaFallbackID == 0 || node.ID() < viaFallbackID { + viaFallbackID = node.ID() + } + + if pm.routerAvailable != nil && + !pm.routerAvailable(node.ID()) { + continue + } + if viaPrimaryID == 0 || node.ID() < viaPrimaryID { viaPrimaryID = node.ID() } @@ -1652,6 +1693,10 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via } } + if viaPrimaryID == 0 { + viaPrimaryID = viaFallbackID + } + if viaPrimaryID != 0 && peer.ID() != viaPrimaryID { result.Include = slices.DeleteFunc( result.Include, diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index dacfcfe65..9f65a929e 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -2754,6 +2754,80 @@ func TestViaRoutesForPeer(t *testing.T) { require.Empty(t, result.Include) require.Equal(t, []netip.Prefix{mp("10.0.0.0/24")}, result.Exclude) }) + + t.Run("via_election_skips_unavailable_routers", func(t *testing.T) { + t.Parallel() + + route := mp("10.0.0.0/24") + nodes := types.Nodes{ + { + ID: 1, + Hostname: "viewer", + IPv4: ap("100.64.0.1"), + User: new(users[0]), + UserID: new(users[0].ID), + Tags: []string{"tag:group-a"}, + Hostinfo: &tailcfg.Hostinfo{}, + }, + } + + for id := types.NodeID(2); id <= 4; id++ { + nodes = append(nodes, &types.Node{ + ID: id, + Hostname: fmt.Sprintf("router-%d", id), + IPv4: ap(fmt.Sprintf("100.64.0.%d", id)), + User: new(users[0]), + UserID: new(users[0].ID), + Tags: []string{"tag:router-a"}, + Hostinfo: &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{route}}, + ApprovedRoutes: []netip.Prefix{route}, + }) + } + + pol := `{ + "tagOwners": { + "tag:router-a": ["user1@"], + "tag:group-a": ["user1@"] + }, + "grants": [{ + "src": ["tag:group-a"], + "dst": ["10.0.0.0/24"], + "ip": ["*"], + "via": ["tag:router-a"] + }] + }` + pm, err := NewPolicyManager([]byte(pol), users, nodes.ViewSlice()) + require.NoError(t, err) + + for _, tt := range []struct { + name string + available []types.NodeID + 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: "whole group down", want: 2}, + {name: "lowest ID recovers", available: []types.NodeID{2, 3, 4}, want: 2}, + } { + t.Run(tt.name, func(t *testing.T) { + pm.SetRouterAvailable(func(id types.NodeID) bool { + return slices.Contains(tt.available, id) + }) + + for _, router := range nodes[1:] { + result := pm.ViaRoutesForPeer(nodes[0].View(), router.View()) + if router.ID == tt.want { + require.Equal(t, []netip.Prefix{route}, result.Include) + require.Empty(t, result.Exclude) + } else { + require.Empty(t, result.Include) + require.Equal(t, []netip.Prefix{route}, result.Exclude) + } + } + }) + } + }) } // TestBuildPeerMap_AutogroupInternetMakesExitNodeVisible reproduces diff --git a/hscontrol/servertest/grants_test.go b/hscontrol/servertest/grants_test.go index 40e0d89e5..f3c0257ba 100644 --- a/hscontrol/servertest/grants_test.go +++ b/hscontrol/servertest/grants_test.go @@ -3,10 +3,13 @@ package servertest_test import ( "context" "net/netip" + "slices" "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/tailcfg" @@ -1113,3 +1116,136 @@ func hasDstRules(matches []filtertype.Match) bool { return false } + +func TestGrantViaSubnetFailover(t *testing.T) { + t.Parallel() + + route := netip.MustParsePrefix("10.0.0.0/24") + setup := func( + t *testing.T, + ) (*servertest.TestServer, map[string]*servertest.TestClient) { + t.Helper() + + srv := servertest.NewServer(t) + user := srv.CreateUser(t, "via-ha-user") + + changed, err := srv.State().SetPolicy([]byte(`{ + "tagOwners": { + "tag:other": ["via-ha-user@"], + "tag:router": ["via-ha-user@"], + "tag:client": ["via-ha-user@"] + }, + "grants": [{ + "src": ["tag:client"], + "dst": ["10.0.0.0/24"], + "ip": ["*"], + "via": ["tag:router"] + }], + "autoApprovers": { + "routes": { + "10.0.0.0/24": ["tag:other", "tag:router"] + } + } + }`)) + require.NoError(t, err) + + if changed { + changes, err := srv.State().ReloadPolicy() + require.NoError(t, err) + srv.App.Change(changes...) + } + + clients := map[string]*servertest.TestClient{} + + // Register the router outside the via group first to hold global primary. + for _, c := range []struct{ name, tag string }{ + {"other", "tag:other"}, + {"router-1", "tag:router"}, + {"router-2", "tag:router"}, + {"client", "tag:client"}, + } { + clients[c.name] = servertest.NewClient(t, srv, c.name, + servertest.WithUser(user), servertest.WithTags(c.tag)) + } + + for _, name := range []string{"other", "router-1", "router-2"} { + clients[name].Direct().SetHostinfo(&tailcfg.Hostinfo{ + BackendLogID: "servertest-" + name, + Hostname: name, + RoutableIPs: []netip.Prefix{route}, + }) + + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + _ = clients[name].Direct().SendUpdate(ctx) + + cancel() + } + + return srv, clients + } + + onlyHolder := func(want string) func(*netmap.NetworkMap) bool { + return func(nm *netmap.NetworkMap) bool { + var holders []string + + for _, p := range nm.Peers { + if slices.Contains(p.AllowedIPs().AsSlice(), route) { + holders = append(holders, p.Hostinfo().Hostname()) + } + } + + return slices.Equal(holders, []string{want}) + } + } + + setExpiry := func(t *testing.T, srv *servertest.TestServer, id types.NodeID, expiry *time.Time) { + t.Helper() + + _, c, err := srv.State().SetNodeExpiry(id, expiry) + require.NoError(t, err) + srv.App.Change(c) + } + + for _, mode := range []string{"offline", "unhealthy", "expired"} { + t.Run(mode, func(t *testing.T) { + t.Parallel() + + srv, clients := setup(t) + clients["client"].WaitForCondition(t, "route on router-1", + 15*time.Second, onlyHolder("router-1")) + otherID := findNodeID(t, srv, "other") + router1ID := findNodeID(t, srv, "router-1") + require.Contains(t, srv.State().GetNodePrimaryRoutes(otherID), route) + + switch mode { + case "offline": + clients["router-1"].Disconnect(t) + case "unhealthy": + changed := srv.State().SetNodeHealth(router1ID, false) + require.True(t, changed, "a via health change must trigger a recompute") + srv.App.Change(change.PolicyChange()) + case "expired": + setExpiry(t, srv, router1ID, new(time.Now().Add(-time.Minute))) + } + + clients["client"].WaitForCondition(t, "route moves to router-2", + 30*time.Second, onlyHolder("router-2")) + assert.Contains(t, srv.State().GetNodePrimaryRoutes(otherID), route) + + switch mode { + case "offline": + clients["router-1"].Reconnect(t) + case "unhealthy": + changed := srv.State().SetNodeHealth(router1ID, true) + require.True(t, changed, "a via recovery must trigger a recompute") + srv.App.Change(change.PolicyChange()) + case "expired": + setExpiry(t, srv, router1ID, nil) + } + + clients["client"].WaitForCondition(t, "route returns to router-1", + 30*time.Second, onlyHolder("router-1")) + assert.Contains(t, srv.State().GetNodePrimaryRoutes(otherID), route) + }) + } +} diff --git a/hscontrol/servertest/via_ha_compat_test.go b/hscontrol/servertest/via_ha_compat_test.go index 97f24f405..e72e4e0b9 100644 --- a/hscontrol/servertest/via_ha_compat_test.go +++ b/hscontrol/servertest/via_ha_compat_test.go @@ -222,22 +222,14 @@ func runViaHACompat(t *testing.T, c *testcapture.Capture) { // keyed ///. A listed check that matches // fails, so a fix must delete its entry. var viaHAFailoverSkipReasons = map[string]string{ - "via-ha-failover/V1/client-a/route-owner": viaOfflineRouterKept, - "via-ha-failover/V9/client-a/route-owner": viaOfflineRouterKept, - "via-ha-failover/V2/client-a/route-owner": viaLowestIDAfterRestart, "via-ha-failover/V10/client-a/route-owner": viaLowestIDAfterRestart, - "via-ha-failover/V5/client-a/route-owner": viaExpiredRouterKept, - "via-ha-failover/V5b/client-a/route-owner": viaExpiredRouterKept, - "via-ha-failover/V12/client-a/route-owner": readvertisedRouterTakesRoute, "via-ha-failover/V12/client-b/route-owner": readvertisedRouterTakesRoute, } const ( - viaOfflineRouterKept = "VIA_OFFLINE_ROUTER_KEPT: headscale keeps a via route on an offline router while another router of the via group is online; SaaS moves it to the online one" - viaExpiredRouterKept = "VIA_EXPIRED_ROUTER_KEPT: headscale keeps a via route on a router whose key expired while another router of the via group is live; SaaS moves it to the live one" 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" ) diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index ca4656750..926352f55 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -206,14 +206,18 @@ type work struct { // updateChanges reports whether an in-place update moved a peer-visibility // input (which also re-elects routes) or only a route-election input. // Peer visibility depends on what the policy reads; election additionally -// depends on online and health state, treating unknown online as offline. +// depends on online, expiry and health state, treating unknown online as +// offline. Fields are compared, not [servesRoutes]: an expiry that passed +// before the write reads the same on both sides. func updateChanges(pre, post *types.Node) (bool, bool) { preView, postView := pre.View(), post.View() if postView.HasPolicyChange(preView) || postView.HasNetworkChanges(preView) { return true, true } - return false, pre.Online() != post.Online() || pre.Unhealthy != post.Unhealthy + return false, pre.Online() != post.Online() || + !preView.Expiry().GetOr(time.Time{}).Equal(postView.Expiry().GetOr(time.Time{})) || + pre.Unhealthy != post.Unhealthy } // PutNode adds or updates a node in the store. @@ -778,9 +782,15 @@ func snapshotFromNodes( return newSnap } -// onlineAdvertisers maps each non-exit prefix to the IDs of online nodes -// that approve it. Nodes are visited in the order of ids, so the per-prefix -// slices preserve that order. +// servesRoutes reports whether n can carry its routes. Expiry is tested +// apart from Online because an expired node keeps its control session. +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( nodes map[types.NodeID]types.Node, ids []types.NodeID, @@ -789,7 +799,7 @@ func onlineAdvertisers( for _, id := range ids { n := nodes[id] - if n.IsOnline == nil || !*n.IsOnline { + if !servesRoutes(&n) { continue } @@ -1085,6 +1095,14 @@ 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 { + n, ok := s.data.Load().nodesByID[id] + + return ok && servesRoutes(&n) && !n.Unhealthy +} + // PrimaryRoutes returns the snapshot's prefix→primary map. The map is // owned by the snapshot and must not be mutated; it is safe to read // concurrently because snapshots are immutable once published. diff --git a/hscontrol/state/node_store_test.go b/hscontrol/state/node_store_test.go index 9c41b5ac2..9b1a57c6b 100644 --- a/hscontrol/state/node_store_test.go +++ b/hscontrol/state/node_store_test.go @@ -1910,7 +1910,7 @@ var nodeFieldImpact = map[string]string{ "MachineKey": "payload", "NodeKey": "payload", "DiscoKey": "payload", "Endpoints": "payload", "Hostname": "payload", "GivenName": "payload", "RegisterMethod": "payload", "AuthKeyID": "payload", "AuthKey": "payload", - "Expiry": "payload", "LastSeen": "payload", "CreatedAt": "payload", + "Expiry": "election", "LastSeen": "payload", "CreatedAt": "payload", "UpdatedAt": "payload", "DeletedAt": "payload", "ActiveSessions": "payload", "SessionEpoch": "payload", } @@ -1966,6 +1966,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: "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_test.go b/hscontrol/state/primaries_test.go index 404adfa0d..e09f1cf6a 100644 --- a/hscontrol/state/primaries_test.go +++ b/hscontrol/state/primaries_test.go @@ -4,6 +4,7 @@ import ( "net/netip" "slices" "testing" + "time" "github.com/juanfont/headscale/hscontrol/types" "github.com/puzpuzpuz/xsync/v4" @@ -238,6 +239,25 @@ func TestPrimaries_AllUnhealthyPreservesPrevious(t *testing.T) { f.requirePrimary(prefix, 2) } +func TestPrimaries_ExpiredRouterNotElected(t *testing.T) { + // Expiry alone takes a router out of the election, even while its + // cached online flag still says connected. + prefix := mp("10.0.0.0/24") + f := newPrimariesFixture(t, 1, 2) + f.advertise(1, prefix) + f.advertise(2, prefix) + f.requirePrimary(prefix, 1) + require.True(t, f.ns.IsRouterAvailable(1)) + + 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)) +} + func TestPrimaries_ExitRouteNotElected(t *testing.T) { // Exit routes (0.0.0.0/0, ::/0) are not subject to HA primary // election — every approved exit-route advertiser keeps it. diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index aab3eadf0..06c16f69c 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -372,6 +372,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) nodeStore.Start() s := &State{ @@ -1692,7 +1693,7 @@ func (s *State) IsNodeHealthy(id types.NodeID) bool { } // SetNodeHealth flips the runtime health bit for one node and reports -// whether the resulting primary-route assignment changed, so the HA +// whether peers need to recompute global or via routes, so the HA // prober can decide whether to fan out a PolicyChange. true means // healthy; false means unhealthy. An unhealthy mark is dropped when // the node is no longer an HA candidate (offline or no approved @@ -1708,7 +1709,7 @@ func (s *State) SetNodeHealth(id types.NodeID, healthy bool) bool { // BatchSetNodeHealth applies a set of health updates atomically: the // election runs once after every flag has been flipped, so observers // never see an intermediate snapshot. Returns true when the -// primary-route assignment differs from before the batch so callers +// global primary routes or a via target's health changed, so callers // can gate a single PolicyChange dispatch. Map value true = healthy; // false = unhealthy (gated as in [State.SetNodeHealth]). // @@ -1744,7 +1745,18 @@ func (s *State) BatchSetNodeHealth(updates map[types.NodeID]bool) bool { s.nodeStore.UpdateNodes(fns) - return !maps.Equal(prevRoutes, s.nodeStore.PrimaryRoutes()) + if !maps.Equal(prevRoutes, s.nodeStore.PrimaryRoutes()) { + return true + } + + for id := range fns { + nv, ok := s.nodeStore.GetNode(id) + if ok && nv.Unhealthy() == !updates[id] && s.polMan.NodeIsViaTarget(nv) { + return true + } + } + + return false } // healthSetter returns an UpdateNodeFunc that flips n.Unhealthy to