johannes-engler-mw commented on PR #2834:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2834#issuecomment-5214206896
Thanks for the careful read. The first point lands and I want to fix it. On
the second I'd like to
push back a little, because I don't think a `For()` predicate actually gives
us the first two rows of
that table.
### The conflation is real
Agreed on `len(gateways) == 0`. Only the `controllerName` mismatch at
`utils.go:369` means "not mine";
the Gateway `NotFound` at `:353` and the GatewayClass `NotFound` at `:363`
mean "could not resolve".
GatewayClass being cluster-scoped is what makes it bite — while it is gone,
*every* route under *every*
Gateway of that class resolves empty at once, and this PR turns that from
stale-but-serving into
deleted. That is a regression I don't want to ship.
Worth noting the same conflation already exists on the Ingress side:
`FindMatchingIngressClass`
returns a bare error for a missing IngressClass and for one belonging to
another controller alike
(`utils.go:1747` vs `:1755`), and `ingress_controller.go:159` deletes on
either. So #2543 shipped this
hazard too — it isn't specific to the Gateway API path, which is part of why
I'd rather fix the
decision than replicate the predicate.
### Why I don't think the predicate can be the guard
`MatchesIngressClassPredicate` is attached to `For()` only
(`ingress_controller.go:75`), so it filters
events on the object itself. Every other enqueue path in these controllers
goes through
`handler.EnqueueRequestsFromMapFunc`, which never consults it:
- `listHTTPRoutesByServiceRef` (`httproute_controller.go:284`) — the
EndpointSlice churn you describe
- `listHTTPRoutesForGateway` (`:388`) — indexes on `indexer.ParentRefs`, no
ownership filter
- `listHTTPRoutesForGatewayProxy` (`:641`), `listHTTPRoutesByExtensionRef`
(`:312`), the policy watches
Walking the table again with a route-equivalent predicate on `For()`:
| situation | actual outcome |
|---|---|
| route owned by another controller | filtered for route events only. Any
EndpointSlice / Gateway / GatewayProxy event still enqueues it, still reaches
`Provider.Delete`, still hits the unconditional `defer d.syncNotify()` at
`provider/apisix/provider.go:232` |
| GatewayClass temporarily unresolvable | same — one endpoint readiness flip
on any referenced backend enqueues the route and the delete branch runs |
| route just moved off my Gateway | admitted exactly once — this row holds |
The third row works, and it's a genuinely nice property. But the first two
are the ones being asked to
prevent an outage and remove a cost, and in both the traffic arrives through
a map func, which is
below the predicate. The Ingress controller has the same gap today via
`listIngressesByService`.
### What I'd propose instead
Let the reconciler tell the cases apart, by having `ParseRouteParentRefs`
report whether any parentRef
was left unresolved alongside the gateway list:
```go
if len(gateways) == 0 {
if unresolved {
// ownership unknown, not disproven — leave the data plane alone
return ctrl.Result{}, nil
}
// every parentRef resolved, none named this controller
... Provider.Delete ...
}
```
One extra signal out of the walk that already runs, threaded through the
five call sites. It holds
regardless of which watch enqueued the reconcile, which is the property the
predicate can't offer, and
it satisfies your "reuse the walk rather than writing a second one" point by
construction — there is
no second walk.
One consequence worth stating: on recovery, nothing re-enqueues those routes
if only the GatewayClass
was deleted and recreated, since the route reconcilers don't watch
GatewayClass — the same blind spot
you flagged for in-place `controllerName` edits. They'd stay stale until the
next event on the route or
its Gateway. That matches master's behaviour, so it's not a regression, but
a GatewayClass watch would
close both and I'm happy to open a follow-up for it.
### On the predicate itself
I'm happy to add it regardless — it removes real work for direct route
events and moves these
controllers toward the Ingress shape. I'd just want it on top of the guard
rather than instead of it.
And if the actual goal is to stop the EndpointSlice-driven churn, the filter
has to go inside the map
funcs, which is a bigger change than a data-plane cleanup PR should carry.
So: guard only, guard + `For()` predicate, or all three? Happy to do any of
them here — just let me
know which you'd rather review in one PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
