state: auto-approve routes in the NodeStore writer, not on reload

Registration approval runs on the node the writer replaces. A policy
reload approves nothing, and a map request only when its announced
routes change, so neither re-grants a route an admin revoked.

hscontrol/servertest/issues_test.go

@@ -282,40 +282,8 @@ func TestIssuesRoutes(t *testing.T) {
 			selfOffersExit)
 	})
 
-	// A policy reload that auto-approves an advertised exit route goes
-	// through autoApproveNodes, a third producer of the same change.
-	t.Run("exit_route_auto_approved_on_reload_reaches_self", func(t *testing.T) {
-		t.Parallel()
-
-		srv := servertest.NewServer(t)
-		user := srv.CreateUser(t, "reloadexit-user")
-
-		exit := servertest.NewClient(t, srv, "reloadexit-node",
-			servertest.WithUser(user))
-		obs := servertest.NewClient(t, srv, "reloadexit-obs",
-			servertest.WithUser(user))
-
-		exit.WaitForPeers(t, 1, 10*time.Second)
-		advertiseRoutes(t, exit, obs, tsaddr.ExitRoutes())
-
-		_, err := srv.State().SetPolicyInDB(`{
-			"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}],
-			"autoApprovers": {"exitNode": ["reloadexit-user@"]}
-		}`)
-		require.NoError(t, err)
-
-		changes, err := srv.State().ReloadPolicy()
-		require.NoError(t, err)
-		srv.App.Change(changes...)
-
-		obs.WaitForCondition(t, "peer offers exit node", 10*time.Second,
-			peerOffersExit("reloadexit-node"))
-		exit.WaitForCondition(t, "self offers exit node", 10*time.Second,
-			selfOffersExit)
-	})
-
 	// An auto-approver approves the route inside the advertising map
-	// request, a fourth producer of the same change.
+	// request, a third producer of the same change.
 	t.Run("exit_route_auto_approved_on_advertise_reaches_self", func(t *testing.T) {
 		t.Parallel()
 

hscontrol/servertest/lifecycle_compat_test.go

@@ -36,17 +36,6 @@ import (
 // the step of what killed nodes leave behind. A listed check that matches
 // fails, so a fix must delete its entry.
 var lifecycleSkipReasons = map[string]string{
-	"route-approval/b6/enabled":     approvalOnHostinfo,
-	"route-approval/b6/served":      approvalOnHostinfo,
-	"route-approval/b6/self-routes": approvalOnHostinfo,
-
-	"route-approval/b7/enabled":     approvalOnPolicy,
-	"route-approval/b7/served":      approvalOnPolicy,
-	"route-approval/b7/self-routes": approvalOnPolicy,
-	"route-approval/b8/enabled":     approvalOnPolicy,
-	"route-approval/b8/served":      approvalOnPolicy,
-	"route-approval/b8/self-routes": approvalOnPolicy,
-
 	"route-approval/b9/enabled":     approvalOnSameKeyReauth,
 	"route-approval/b9/served":      approvalOnSameKeyReauth,
 	"route-approval/b9/self-routes": approvalOnSameKeyReauth,
@@ -76,8 +65,6 @@ var lifecycleSkipReasons = map[string]string{
 // SaaS runs auto-approval when a router's announced routes change and on a
 // re-auth that rotates its node key, over every announced route.
 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"
 )
 

hscontrol/state/autoapprove_test.go

@@ -10,33 +10,106 @@ import (
 	"tailscale.com/tailcfg"
 )
 
-// TestAutoApproveBatchApprovesRoutes verifies the batched autoApproveNodes still
-// approves a node's advertised route when policy auto-approvers permit it. The
-// batching collapses the per-node SetApprovedRoutes calls into one NodeStore
-// update and one policy rebuild; this guards that correctness is preserved.
-func TestAutoApproveBatchApprovesRoutes(t *testing.T) {
+// TestRouteApprovalWaitsForAnnouncement: approvers run when the announced
+// set changes, over every announced route, and neither on a policy reload
+// nor on a report that keeps the routes.
+func TestRouteApprovalWaitsForAnnouncement(t *testing.T) {
 	_, s, nodeID := persistTestSetup(t)
 	t.Cleanup(func() { _ = s.Close() })
 
 	route := netip.MustParsePrefix("10.0.0.0/24")
+	added := netip.MustParsePrefix("10.0.1.0/24")
 
-	_, ok := s.nodeStore.UpdateNode(nodeID, func(n *types.Node) {
-		n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{route}}
-	})
+	nv, ok := s.GetNodeByID(nodeID)
 	require.True(t, ok)
 
-	pol := `{
-		"autoApprovers": {"routes": {"10.0.0.0/24": ["persist-user@"]}},
+	report := func(hostname string, routes ...netip.Prefix) {
+		t.Helper()
+
+		_, err := s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{
+			NodeKey:  nv.NodeKey(),
+			DiscoKey: nv.DiscoKey(),
+			Hostinfo: &tailcfg.Hostinfo{Hostname: hostname, RoutableIPs: routes},
+		})
+		require.NoError(t, err)
+	}
+	approved := func() []netip.Prefix {
+		t.Helper()
+
+		current, ok := s.GetNodeByID(nodeID)
+		require.True(t, ok)
+
+		return current.ApprovedRoutes().AsSlice()
+	}
+
+	report("router", route)
+	require.Empty(t, approved())
+
+	_, err := s.db.SetPolicy(`{
+		"autoApprovers": {"routes": {"10.0.0.0/16": ["persist-user@"]}},
 		"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}]
-	}`
-	_, err := s.SetPolicy([]byte(pol))
+	}`)
 	require.NoError(t, err)
 
 	_, err = s.ReloadPolicy()
 	require.NoError(t, err)
+	assert.Empty(t, approved(), "a reload approves nothing")
 
-	nv, ok := s.GetNodeByID(nodeID)
-	require.True(t, ok)
-	assert.Contains(t, nv.ApprovedRoutes().AsSlice(), route,
-		"auto-approver should have approved the advertised route")
+	report("router-renamed", route)
+	assert.Empty(t, approved(), "a report that keeps the routes approves nothing")
+
+	report("router-renamed", route, added)
+	assert.ElementsMatch(t, []netip.Prefix{route, added}, approved(),
+		"a new announcement runs approvers over every announced route")
+}
+
+// TestAutoApproveKeepsCurrentManualRoutes: registration approval runs on
+// the node the writer replaces, so an admin write that lands after the
+// caller read its view survives.
+func TestAutoApproveKeepsCurrentManualRoutes(t *testing.T) {
+	manual := netip.MustParsePrefix("10.0.0.0/24")
+	automatic := netip.MustParsePrefix("10.1.0.0/24")
+	addition := netip.MustParsePrefix("10.2.0.0/24")
+
+	for _, tt := range []struct {
+		name        string
+		replacement []netip.Prefix
+		want        []netip.Prefix
+	}{
+		{name: "revoke", want: []netip.Prefix{automatic}},
+		{name: "add", replacement: []netip.Prefix{manual, addition}, want: []netip.Prefix{manual, automatic, addition}},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			_, s, nodeID := persistTestSetup(t)
+			t.Cleanup(func() { _ = s.Close() })
+
+			_, err := s.SetPolicy([]byte(`{
+				"autoApprovers": {"routes": {"10.1.0.0/24": ["persist-user@"]}},
+				"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}]
+			}`))
+			require.NoError(t, err)
+
+			_, ok := s.nodeStore.UpdateNode(nodeID, func(n *types.Node) {
+				n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{manual, automatic}}
+			})
+			require.True(t, ok)
+
+			stale, _, err := s.SetApprovedRoutes(nodeID, []netip.Prefix{manual})
+			require.NoError(t, err)
+
+			_, _, err = s.SetApprovedRoutes(nodeID, tt.replacement)
+			require.NoError(t, err)
+
+			_, err = s.AutoApproveRoutes(stale)
+			require.NoError(t, err)
+
+			current, ok := s.GetNodeByID(nodeID)
+			require.True(t, ok)
+			assert.ElementsMatch(t, tt.want, current.ApprovedRoutes().AsSlice())
+
+			stored, err := s.db.GetNodeByID(nodeID)
+			require.NoError(t, err)
+			assert.ElementsMatch(t, tt.want, stored.ApprovedRoutes.List())
+		})
+	}
 }

hscontrol/state/persist_test.go

@@ -1215,47 +1215,6 @@ func TestFailedPersistPolicyChangeSurvivesDroppedChange(t *testing.T) {
 		failedC.Type(), retryC.Type())
 }
 
-// TestReloadPolicyReturnsChangesOnAutoApproveFailure covers a policy reload
-// whose route auto-approval fails to persist. The NodeStore write already
-// fed the approved routes to the policy manager, so no later write sees the
-// policy move again; the reload must return its changes with the error for
-// the caller to publish.
-func TestReloadPolicyReturnsChangesOnAutoApproveFailure(t *testing.T) {
-	_, s, nodeID := persistTestSetup(t)
-	t.Cleanup(func() { _ = s.Close() })
-
-	route := netip.MustParsePrefix("10.9.0.0/24")
-	_, ok := s.nodeStore.UpdateNode(nodeID, func(n *types.Node) {
-		n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{route}}
-	})
-	require.True(t, ok)
-
-	_, err := s.db.SetPolicy(`{
-		"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}],
-		"autoApprovers": {"routes": {"10.9.0.0/24": ["persist-user@"]}}
-	}`)
-	require.NoError(t, err)
-
-	require.NoError(t, s.db.DB.Callback().Update().Before("gorm:update").
-		Register("fail_node_update", func(tx *gorm.DB) {
-			if tx.Statement.Table == "nodes" {
-				_ = tx.AddError(errInjectedNodeUpdate)
-			}
-		}))
-	t.Cleanup(func() { _ = s.db.DB.Callback().Update().Remove("fail_node_update") })
-
-	cs, err := s.ReloadPolicy()
-	require.ErrorIs(t, err, errInjectedNodeUpdate)
-
-	approved, ok := s.GetNodeByID(nodeID)
-	require.True(t, ok)
-	require.Contains(t, approved.ApprovedRoutes().AsSlice(), route,
-		"the NodeStore holds the approval the database write lost")
-
-	assert.True(t, slices.ContainsFunc(cs, func(c change.Change) bool { return c.IncludePolicy }),
-		"the reload must return its policy change with the error: %v", cs)
-}
-
 // TestReloadPolicyOverlappingWrites has a second policy write complete after
 // the reload of the first one loaded its document and before it installs it.
 // The first reload must not put the older document back over the newer one.

hscontrol/state/state.go

@@ -468,9 +468,11 @@ func (s *State) DERPMap() tailcfg.DERPMapView {
 	return s.derpMap.Load().View()
 }
 
-// ReloadPolicy reloads the access control policy and triggers auto-approval if changed.
-// Returns the resulting [change.Change] slice when the policy or routes changed,
-// also alongside an error once the policy is swapped.
+// ReloadPolicy reloads the access control policy.
+//
+// It approves no routes: auto-approval waits for a node's next change to
+// its announced routes or its next node key rotation, as on Tailscale SaaS,
+// so a reload cannot undo an admin's revoke.
 func (s *State) ReloadPolicy() ([]change.Change, error) {
 	s.policyMu.Lock()
 	defer s.policyMu.Unlock()
@@ -514,36 +516,13 @@ func (s *State) reloadPolicyLocked() ([]change.Change, error) {
 		s.nodeStore.RebuildPeerMaps()
 	}
 
+	if policyChanged {
+		log.Info().Msg("Policy reload completed with changes")
+	}
+
 	// Nodes whose CapMap shifted get their self refresh from
 	// [State.DrainSelfRefreshes] when these changes are dispatched.
-	//nolint:prealloc // cs starts with one element and may grow
-	cs := []change.Change{change.PolicyChange()}
-
-	// Always call autoApproveNodes during policy reload, regardless of whether
-	// the policy content has changed. This ensures that routes are re-evaluated
-	// when they might have been manually disabled but could now be auto-approved
-	// with the current policy.
-	rcs, err := s.autoApproveNodes()
-	if err != nil {
-		// The policy is already swapped and the approvals already in the
-		// NodeStore; callers publish these before handling the error.
-		return append(cs, rcs...), fmt.Errorf("auto approving nodes: %w", err)
-	}
-
-	// TODO(kradalby): These changes can probably be safely ignored.
-	// If the PolicyChange is happening, that will lead to a full update
-	// meaning that we do not need to send individual route changes.
-	cs = append(cs, rcs...)
-
-	if len(rcs) > 0 || policyChanged {
-		log.Info().
-			Bool("policy.changed", policyChanged).
-			Int("route.changes", len(rcs)).
-			Int("total.changes", len(cs)).
-			Msg("Policy reload completed with changes")
-	}
-
-	return cs, nil
+	return []change.Change{change.PolicyChange()}, nil
 }
 
 // CreateUser creates a new user and updates the policy manager.
@@ -672,8 +651,6 @@ func (s *State) ListAllUsers() ([]types.User, error) {
 
 // persistNode writes the node's database row, re-reading the
 // authoritative copy from [NodeStore], without touching the policy manager.
-// Batch callers (e.g. autoApproveNodes) use it to write many rows and then
-// trigger a single policy rebuild instead of one per node.
 func (s *State) persistNode(node types.NodeView) (types.NodeView, error) {
 	return s.persistNodeWith(node, nodeUpdateColumns, nil)
 }
@@ -1231,12 +1208,18 @@ func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (t
 	// TODO(kradalby): In principle we should call the AutoApprove logic here
 	// because even if the CLI removes an auto-approved route, it will be added
 	// back automatically.
+	return s.updateApprovedRoutes(nodeID, func(node *types.Node) {
+		node.ApprovedRoutes = routes
+	})
+}
+
+func (s *State) updateApprovedRoutes(nodeID types.NodeID, set UpdateNodeFunc) (types.NodeView, change.Change, error) {
 	prevRoutes := s.nodeStore.PrimaryRoutes()
 	prevPeers := s.nodeStore.ListPeerIDs(nodeID)
 	genBefore := s.polMan.NodesGeneration()
 
 	n, ok := s.nodeStore.UpdateNode(nodeID, func(node *types.Node) {
-		node.ApprovedRoutes = routes
+		set(node)
 		// A node with no approved routes is no longer an HA
 		// candidate; drop any stale Unhealthy bit (mirrors the
 		// legacy routes.SetRoutes(empty) auto-clear).
@@ -1532,10 +1515,8 @@ func (s *State) AutoApproveRoutes(nv types.NodeView) (change.Change, error) {
 			Strs("routes.approved.new", util.PrefixesToString(approved)).
 			Msg("Single node auto-approval detected route changes")
 
-		// Persist the auto-approved routes to database and [NodeStore] via
-		// [State.SetApprovedRoutes]. This ensures consistency between database
-		// and [NodeStore].
-		_, c, err := s.SetApprovedRoutes(nv.ID(), approved)
+		// Recompute on the current node so an older view cannot undo an admin write.
+		_, c, err := s.updateApprovedRoutes(nv.ID(), s.approveAnnouncedRoutes)
 		if err != nil {
 			log.Error().
 				EmbedObject(nv).
@@ -1551,6 +1532,10 @@ func (s *State) AutoApproveRoutes(nv types.NodeView) (change.Change, error) {
 	return change.Change{}, nil
 }
 
+func (s *State) approveAnnouncedRoutes(n *types.Node) {
+	n.ApprovedRoutes, _ = policy.ApproveRoutesWithPolicy(s.polMan, n.View(), n.ApprovedRoutes, n.AnnouncedRoutes())
+}
+
 // GetPolicy retrieves the current policy from the database.
 func (s *State) GetPolicy() (*types.Policy, error) {
 	return s.db.GetPolicy()
@@ -3634,83 +3619,6 @@ func (s *State) PingDB(ctx context.Context) error {
 	return s.db.PingDB(ctx)
 }
 
-// autoApproveNodes mass approves routes on all nodes. It is _only_ intended for
-// use when the policy is replaced. It is not sending or reporting any changes
-// or updates as we send full updates after replacing the policy.
-// TODO(kradalby): This is kind of messy, maybe this is another +1
-// for an event bus. See example comments here.
-// autoApproveNodes automatically approves nodes based on policy rules.
-func (s *State) autoApproveNodes() ([]change.Change, error) {
-	nodes := s.ListNodes()
-
-	// Compute every node's approval first, then apply them all in a single
-	// NodeStore batch and a single policy/peer-map rebuild. One
-	// SetApprovedRoutes per node would otherwise drive an O(n) policy SetNodes
-	// and O(n^2) peer-map rebuild for each changed node, i.e. O(m*n^2) per
-	// policy reload.
-	approvedByID := make(map[types.NodeID][]netip.Prefix)
-
-	for _, nv := range nodes.All() {
-		approved, changed := policy.ApproveRoutesWithPolicy(s.polMan, nv, nv.ApprovedRoutes().AsSlice(), nv.AnnouncedRoutes())
-		if !changed {
-			continue
-		}
-
-		log.Debug().
-			Uint64(zf.NodeID, nv.ID().Uint64()).
-			Str(zf.NodeName, nv.Hostname()).
-			Strs(zf.RoutesApprovedOld, util.PrefixesToString(nv.ApprovedRoutes().AsSlice())).
-			Strs(zf.RoutesApprovedNew, util.PrefixesToString(approved)).
-			Msg("Routes auto-approved by policy")
-
-		approvedByID[nv.ID()] = approved
-	}
-
-	if len(approvedByID) == 0 {
-		return nil, nil
-	}
-
-	genBefore := s.polMan.NodesGeneration()
-
-	updates := make(map[types.NodeID]UpdateNodeFunc, len(approvedByID))
-	for id, approved := range approvedByID {
-		updates[id] = func(n *types.Node) {
-			n.ApprovedRoutes = approved
-
-			// A node with no approved routes is no longer an HA candidate;
-			// drop any stale Unhealthy bit (mirrors SetApprovedRoutes).
-			if len(n.AllApprovedRoutes()) == 0 {
-				n.Unhealthy = false
-			}
-		}
-	}
-
-	s.nodeStore.UpdateNodes(updates)
-
-	for id := range approvedByID {
-		fresh, ok := s.nodeStore.GetNode(id)
-		if !ok {
-			continue
-		}
-
-		_, err := s.persistNode(fresh)
-		if err != nil {
-			return []change.Change{s.policyChangeSince(genBefore)}, err
-		}
-	}
-
-	c, err := s.updatePolicyManagerNodes(genBefore)
-	if err != nil {
-		return []change.Change{c}, err
-	}
-
-	if c.IsEmpty() {
-		c = change.PolicyChange()
-	}
-
-	return []change.Change{c}, nil
-}
-
 // isAutoDerivedGivenName reports whether given matches what
 // dnsname.SanitizeHostname(hostname) would produce, optionally with a
 // [NodeStore] collision-bump "-N" suffix. It is used to detect whether a
@@ -3754,7 +3662,6 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 	var (
 		delta              mapRequestDelta
 		routeChange        bool
-		needsRouteApproval bool
 		autoApprovedRoutes []netip.Prefix
 		staleNodeKey       bool
 	)
@@ -3843,16 +3750,10 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest
 
 		hostinfoToStore := delta.hostinfoChanged || delta.derpChanged
 
-		// Calculate route approval before [NodeStore] update to avoid
-		// calling View() inside callback
-		var hasNewRoutes bool
-		if hi := req.Hostinfo; hi != nil {
-			hasNewRoutes = len(hi.RoutableIPs) > 0
-		}
-
-		needsRouteApproval = delta.hostinfoChanged &&
-			(delta.routesChanged || (hasNewRoutes && len(currentNode.ApprovedRoutes) == 0))
-		if needsRouteApproval {
+		// Approvers run over every announced route, but only when the
+		// announced set changes: any other report would re-grant a route
+		// an admin revoked.
+		if delta.routesChanged {
 			// Extract announced routes from request
 			var announcedRoutes []netip.Prefix
 			if req.Hostinfo != nil {

integration/route_test.go

@@ -2857,8 +2857,9 @@ func TestAutoApproveMultiNetwork(t *testing.T) {
 						}
 					}, assertTimeout, 200*time.Millisecond, "Verifying routes disabled after route removal")
 
-					// Add the route back to the auto approver in the policy, the route should
-					// now become available again.
+					// Add the route back to the auto approver in the policy. The
+					// policy alone approves nothing; the route comes back once the
+					// router announces it again.
 					var newApprovers policyv2.AutoApprovers
 
 					switch {
@@ -2875,6 +2876,25 @@ func TestAutoApproveMultiNetwork(t *testing.T) {
 					err = headscale.SetPolicy(pol)
 					require.NoError(t, err)
 
+					assert.EventuallyWithT(t, func(c *assert.CollectT) {
+						nodes, err = headscale.ListNodes()
+						assert.NoError(c, err)
+						requireNodeRouteCountWithCollect(c, MustFindNode(routerUsernet1.Hostname(), nodes), 1, 0, 0)
+					}, assertTimeout, 500*time.Millisecond, "a policy change alone approves no route")
+
+					// Withdraw and wait, so the new announcement is a change.
+					_, _, err = routerUsernet1.Execute([]string{"tailscale", "set", "--advertise-routes="})
+					require.NoErrorf(t, err, "failed to withdraw route: %s", err)
+
+					assert.EventuallyWithT(t, func(c *assert.CollectT) {
+						nodes, err = headscale.ListNodes()
+						assert.NoError(c, err)
+						requireNodeRouteCountWithCollect(c, MustFindNode(routerUsernet1.Hostname(), nodes), 0, 0, 0)
+					}, assertTimeout, 500*time.Millisecond, "route withdrawn")
+
+					_, _, err = routerUsernet1.Execute([]string{"tailscale", "set", "--advertise-routes=" + route.String()})
+					require.NoErrorf(t, err, "failed to advertise route: %s", err)
+
 					// Wait for route state changes to propagate
 					assert.EventuallyWithT(t, func(c *assert.CollectT) {
 						// These route should auto approve, so the node is expected to have a route