fix: notify cross-namespace routes when a QuotaPolicy is deleted - #2770
vignesh-chaturvedi wants to merge 3 commits into
Conversation
On deletion the QuotaPolicy controller notified only AIGatewayRoutes in the policy's own namespace, while an update reaches every route that references a targeted backend through the BackendToReferencingAIGatewayRoute index. A route in another namespace that references the backend through a ReferenceGrant was therefore never re-reconciled when the policy was deleted. It kept the deleted policy's rate_limits actions, so Envoy went on calling the rate limit service for descriptors that no longer have a limit, and with quotaRateLimitFailureModeDeny a rate limit service outage would reject requests on a route that no longer has any quota. Deletion now lists the AIServiceBackends in the policy's namespace and notifies through the same index the update path uses. Signed-off-by: Vignesh Chaturvedi <vigneshchaturvedi@gmail.com>
✅ Deploy Preview for theagentrouter canceled.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
aabchoo
added
area/controller
| "route", route.Name, "namespace", route.Namespace) | ||
| c.aiGatewayRouteChan <- event.GenericEvent{Object: route} | ||
| for i := range backends.Items { | ||
| key := fmt.Sprintf("%s.%s", backends.Items[i].Name, namespace) |
There was a problem hiding this comment.
do you mind making a helper function that does this so we don't duplicate this logic all over ie in syncAIServiceBackend?
ie
func namespacedNameIndexKey(name, namespace string) string {
return fmt.Sprintf("%s.%s", name, namespace)
}
There was a problem hiding this comment.
Done. Added namespacedNameIndexKey(name, namespace) in controller.go and used
it on both sides of the two indexes this change relies on: aiGatewayRouteIndexFunc
and quotaPolicyTargetRefsIndexFunc, which produce the keys, and
syncAIServiceBackend, BackendToQuotaPolicy, notifyAIGatewayRoutes and the
deletion path, which look them up.
There are about a dozen more "%s.%s" keys in the gateway, MCP route and secret
indexes, plus the existing backendSecurityPolicyKey. I left those alone to keep
this PR focused on the fix, but I am happy to sweep them in a follow-up if you
would like.
| c.aiGatewayRouteChan <- event.GenericEvent{Object: route} | ||
| for i := range backends.Items { | ||
| key := fmt.Sprintf("%s.%s", backends.Items[i].Name, namespace) | ||
| var aiGatewayRoutes aigv1b1.AIGatewayRouteList |
There was a problem hiding this comment.
could you add a dedupe check to avoid calling the same aigatewayroute multiple times?
There was a problem hiding this comment.
Done. Each route is now notified once per deletion, tracked by namespaced name.
The test gives local-route references to two backends in the policy's
namespace, so it fails if the dedupe is removed.
| // notifyAIGatewayRoutesForNamespace sends events for every AIGatewayRoute that | ||
| // references an AIServiceBackend in the given namespace, wherever the route lives. | ||
| // Used on QuotaPolicy deletion when targetRefs are no longer available. | ||
| func (c *QuotaPolicyController) notifyAIGatewayRoutesForNamespace(ctx context.Context, namespace string) { |
There was a problem hiding this comment.
Thoughts on adding a returning an error? Any premature returns will result in the envoy configs not updating properly and the issue you mentioned will still persist
There was a problem hiding this comment.
Agreed, done. The function now returns the error and Reconcile passes it
through, so controller-runtime requeues. Retrying is safe:
deleteQuotaPolicyConfig is idempotent, and a repeated notification just
re-reconciles the route. TestQuotaPolicyController_Reconcile_DeletionReturnsNotifyError
injects a failing list and checks that the error comes back.
notifyAIGatewayRoutes on the update path still logs and continues on the same
kind of error. Want me to make it return the error too, here or in a follow-up?
Address review on the deletion path: - Return list errors from notifyAIGatewayRoutesForNamespace so a failed lookup requeues the reconcile instead of leaving routes un-notified. - Notify each AIGatewayRoute once, even when it references several AIServiceBackends in the policy's namespace. - Add namespacedNameIndexKey and use it on both sides of the backend-to-route and backend-to-QuotaPolicy indexes, including syncAIServiceBackend, instead of repeating the key format. Signed-off-by: Vignesh Chaturvedi <vigneshchaturvedi@gmail.com>
|
I am seeing that this case is being covered in #2766. |
|
Thanks, you're right that #2766 covers the cross-namespace case: it notifies from One difference first. #2766 also removes the cleanup from the not-found branch |
2 participants
Description
When a QuotaPolicy is deleted, the controller re-reconciles only the
AIGatewayRoutes in the policy's own namespace:
notifyAllAIGatewayRoutesInNamespacelists routes with
client.InNamespace. An update goes throughnotifyAIGatewayRoutesinstead, which finds routes via theBackendToReferencingAIGatewayRouteindex. That index is keyed on the backend'snamespace, so it reaches routes in any namespace that reference a targeted
backend through a ReferenceGrant.
A route in namespace B that references a backend in namespace A is therefore
notified when a QuotaPolicy in A changes, but not when that policy is deleted.
It keeps the deleted policy's
rate_limitsactions until something elsere-translates it. Deleting the policy already removes its limits from the rate
limit service, so nothing is enforced, but Envoy keeps calling the rate limit
service on every request and at stream-done for descriptors that no longer have
a limit. With
--quotaRateLimitFailureModeDenyset, a rate limit service outagewould also reject requests on a route that no longer has any quota.
This changes the deletion path to list the AIServiceBackends in the policy's
namespace and notify through the same index the update path uses. The policy's
targetRefs are gone by the time the deletion is observed, so the backends in its
namespace stand in for them. That is a superset of what the policy could have
targeted, since QuotaPolicy targetRefs are local. Routes in the policy's
namespace that reference no backend there are no longer notified, which is fine
because a QuotaPolicy cannot affect them.
Related Issues/PRs (if applicable)
Related: #2550
Related PR: #2561
Special notes for reviewers (if applicable)
This is the follow-up I offered in my review on #2561. That PR stamps a hash of
the targeting QuotaPolicies onto the derived HTTPRoute so Envoy Gateway
re-translates it, which only helps a route that gets re-reconciled in the first
place. Without this change, a cross-namespace route also keeps the deleted
policy's entry in that annotation. The two PRs touch different files and can
land in either order.
TestQuotaPolicyController_Reconcile_DeletionNotifiesCrossNamespaceRoutescreates a backend and a QuotaPolicy in
ns-a, one route inns-a, and one inns-bthat references the backend across namespaces. It asserts that an updatenotifies both routes, then that the deletion does too. On main it fails with
ns-b/remote-routemissing from the deletion events, and it passes with thischange. The existing deletion test did not check which routes were notified.
If a backend is deleted before its QuotaPolicy, this path no longer finds the
routes that referenced it. Those routes are already re-reconciled by the
AIServiceBackend controller when the backend goes away, through the same index.
make precommitis clean andgo test ./internal/controller/...passes. I havenot run this against a live cluster, so the notification is verified at the
controller level only.
I used an AI assistant while investigating and writing this change, noted per
the generative AI policy in the contributing guide. I have reviewed and
understand the diff and can revise it in review.