diff --git a/hscontrol/state/maprequest_test.go b/hscontrol/state/maprequest_test.go index e448d48ff..3eb3685e9 100644 --- a/hscontrol/state/maprequest_test.go +++ b/hscontrol/state/maprequest_test.go @@ -1117,3 +1117,158 @@ func BenchmarkUpdateNodeFromMapRequest(b *testing.B) { } }) } + +// autoApproveTestPolicy lets aa-user's nodes see each other and +// auto-approve subnets under 10.55.0.0/16 and exit routes; aa-other's +// node only reaches the internet, so it sees an aa-user node only once +// that node is an approved exit node. +const autoApproveTestPolicy = `{ + "acls": [ + {"action": "accept", "src": ["aa-user@"], "dst": ["aa-user@:*", "10.55.0.0/16:*"]}, + {"action": "accept", "src": ["aa-other@"], "dst": ["autogroup:internet:*"]} + ], + "autoApprovers": { + "routes": {"10.55.0.0/16": ["aa-user@"]}, + "exitNode": ["aa-user@"] + }}` + +// newAutoApproveTestState returns a State under autoApproveTestPolicy with +// three aa-user nodes followed by one aa-other node. prepare edits the +// database rows before the State loads them, and the returned counter +// counts peer builds from then on. +func newAutoApproveTestState( + t *testing.T, + prepare func(nodes []*types.Node), +) (*State, []types.NodeID, *atomic.Int64) { + t.Helper() + + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(t, err) + + user := database.CreateUserForTest("aa-user") + other := database.CreateUserForTest("aa-other") + nodes := database.CreateRegisteredNodesForTest(user, 3, "aa-node") + nodes = append(nodes, database.CreateRegisteredNodeForTest(other, "aa-other-node")) + + if prepare != nil { + prepare(nodes) + + for _, n := range nodes { + require.NoError(t, database.DB.Save(n).Error) + } + } + + require.NoError(t, database.Close()) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + _, err = s.SetPolicy([]byte(autoApproveTestPolicy)) + require.NoError(t, err) + + ids := make([]types.NodeID, 0, len(nodes)) + for _, n := range nodes { + ids = append(ids, n.ID) + } + + return s, ids, countStatePeerBuilds(t, s) +} + +func checkAutoApproveAdjacency(t *testing.T, s *State) { + t.Helper() + + users, err := s.ListAllUsers() + require.NoError(t, err) + + checkAdjacencyMatchesFullBuild(t, s.nodeStore, autoApproveTestPolicy, users) +} + +func routeMapRequest(t *testing.T, s *State, id types.NodeID, routes ...netip.Prefix) tailcfg.MapRequest { + t.Helper() + + nv, ok := s.GetNodeByID(id) + require.True(t, ok) + + return tailcfg.MapRequest{ + NodeKey: nv.NodeKey(), + DiscoKey: nv.DiscoKey(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: nv.Hostname(), + RoutableIPs: routes, + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + }, + } +} + +// countNodeRowUpdates counts UPDATEs of the nodes table from now on. +func countNodeRowUpdates(t *testing.T, s *State) *atomic.Int64 { + t.Helper() + + var n atomic.Int64 + + gdb := s.DB().DB + name := t.Name() + "_count_node_updates" + err := gdb.Callback().Update().After("gorm:update").Register(name, func(tx *gorm.DB) { + if tx.Statement != nil && tx.Statement.Table == "nodes" { + n.Add(1) + } + }) + require.NoError(t, err) + t.Cleanup(func() { _ = gdb.Callback().Update().Remove(name) }) + + return &n +} + +// TestMapRequestAutoApprovalIsOneWrite pins that a map request whose +// announced route the policy auto-approves lands in one route-changing +// NodeStore write (one peer build) and one row update, persists the +// approval, and reports the policy change the new route causes. +func TestMapRequestAutoApprovalIsOneWrite(t *testing.T) { + s, ids, builds := newAutoApproveTestState(t, nil) + route := netip.MustParsePrefix("10.55.1.0/24") + + req := routeMapRequest(t, s, ids[0], route) + rowUpdates := countNodeRowUpdates(t, s) + + c, err := s.UpdateNodeFromMapRequest(ids[0], req) + require.NoError(t, err) + + assert.Equal(t, int64(1), builds.Load(), "peer builds for one map request") + assert.Equal(t, int64(1), rowUpdates.Load(), "node row updates for one map request") + assert.Equal(t, "policy", c.Type()) + + row, err := s.DB().GetNodeByID(ids[0]) + require.NoError(t, err) + assert.Contains(t, row.ApprovedRoutes, route, "the approval must be persisted") + + nv, ok := s.GetNodeByID(ids[0]) + require.True(t, ok) + assert.Contains(t, nv.SubnetRoutes(), route) + + checkAutoApproveAdjacency(t, s) +} + +// TestMapRequestWithdrawingRoutesClearsUnhealthy pins that a node whose +// announced set shrinks to empty stops being an unhealthy HA candidate. +func TestMapRequestWithdrawingRoutesClearsUnhealthy(t *testing.T) { + route := netip.MustParsePrefix("10.55.1.0/24") + s, ids, _ := newAutoApproveTestState(t, func(nodes []*types.Node) { + nodes[0].Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{route}} + nodes[0].ApprovedRoutes = []netip.Prefix{route} + }) + + _, ok := s.nodeStore.UpdateNode(ids[0], func(n *types.Node) { n.Unhealthy = true }) + require.True(t, ok) + + _, err := s.UpdateNodeFromMapRequest(ids[0], routeMapRequest(t, s, ids[0])) + require.NoError(t, err) + + nv, ok := s.GetNodeByID(ids[0]) + require.True(t, ok) + assert.Empty(t, nv.AllApprovedRoutes()) + assert.False(t, nv.Unhealthy(), "a node with no approved routes is no HA candidate") +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index b02ce6f3a..926df8bf0 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -3399,15 +3399,17 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest } if routeChange { - // Always apply the route approval result so routes are - // cleared when auto-approvers are removed from the policy, - // even if the policy evaluation itself detected no change. log.Info(). Uint64(zf.NodeID, id.Uint64()). Strs(zf.OldApprovedRoutes, util.PrefixesToString(currentNode.ApprovedRoutes)). Strs(zf.NewApprovedRoutes, util.PrefixesToString(autoApprovedRoutes)). Bool(zf.RouteChanged, routeChange). Msg("applying route approval results") + + // Approving in this write keeps one request at one NodeStore + // write, one peer build and one row update. Persisting is + // already due: route approval only runs on a Hostinfo change. + currentNode.ApprovedRoutes = autoApprovedRoutes } // AllApprovedRoutes is announced ∩ approved; a Hostinfo @@ -3423,27 +3425,6 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest return change.Change{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, id) } - if routeChange { - log.Debug(). - Uint64(zf.NodeID, id.Uint64()). - Strs(zf.AutoApprovedRoutes, util.PrefixesToString(autoApprovedRoutes)). - Msg("Persisting auto-approved routes from MapRequest") - - // [State.SetApprovedRoutes] will update both database and PrimaryRoutes table - // TODO(kradalby): approval should ride the map request write above. - // Writing it separately costs a second NodeStore write and a second - // peer-map rebuild for one request. - _, c, err := s.SetApprovedRoutes(id, autoApprovedRoutes) - if err != nil { - return change.Change{}, fmt.Errorf("persisting auto-approved routes: %w", err) - } - - // If [State.SetApprovedRoutes] resulted in a policy change, return it - if !c.IsEmpty() { - return c, nil - } - } // Continue with the rest of the processing using the updated node - // SubnetRoutes = announced ∩ approved, so a Hostinfo update can // move a primary without ever touching ApprovedRoutes. The pre/post // snapshot diff catches that. @@ -3482,7 +3463,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest // Only refresh the policy manager when something it depends on // might have moved. Endpoint/key/DERP/LastSeen-only updates do not // affect policy evaluation and are deliberately skipped here. - if delta.peerHostinfoChanged || delta.routesChanged { + if delta.peerHostinfoChanged || delta.routesChanged || routeChange { policyChange, err = s.updatePolicyManagerNodes(genBefore) if err != nil { return nodeWriteFailed(id, policyChange.Merge(nodeRouteChange)), fmt.Errorf("updating policy manager after node save: %w", err) @@ -3503,6 +3484,12 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest // map responses. c := buildMapRequestChangeResponse(id, updatedNode, delta) + // Approval moved no effective route; resend the node so peers hold + // its current state. + if routeChange { + c = c.Merge(change.NodeAdded(id)) + } + // One trace line per classified request so a "peer cannot reach me" // report can be matched to the classification that narrowed it. log.Trace().