AlinsRan commented on PR #2834:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2834#issuecomment-5214064216
The root cause analysis is right, and the fix matches what #2543 did for
Ingress. Given the current store model, deleting on the way out is the workable
approach. Two things about the trigger condition though.
### `len(gateways) == 0` conflates "not mine" with "can't tell yet"
`ParseRouteParentRefs` reaches an empty list through three different
`continue` paths:
- `utils.go:359` — controllerName mismatch. This is the case the PR targets;
deleting is correct.
- `utils.go:343` — Gateway `NotFound`
- `utils.go:353` — GatewayClass `NotFound`
The last two mean "the parent could not be resolved", not "this route
belongs to someone else". They currently get the same treatment.
The concrete failure is a GatewayClass that briefly disappears — GitOps
prune-then-apply, a CRD upgrade, an accidental delete. GatewayClass is
cluster-scoped, so while it is gone *every* route under *every* Gateway of that
class resolves to an empty gateway list. On master those routes go stale but
keep serving; with this PR their data plane config is actively deleted. That
turns a stale-config window into an outage.
### That window is easy to hit, because routes are re-reconciled frequently
I assumed `WithEventFilter(predicate.GenerationChangedPredicate{})` would
filter out EndpointSlice churn. It does not — EndpointSlice maintains
`metadata.generation`:
```go
// k8s.io/kubernetes pkg/registry/discovery/endpointslice/strategy.go
endpointSlice.Generation = 1 // :63
ogNewMeta.Generation = ogOldMeta.Generation + 1 // :82
```
`listHTTPRoutesByServiceRef` indexes purely on backend service refs with no
ownership filter, so every pod scale / rollout / readiness flip enqueues *all*
routes referencing that service, including ones owned by other controllers.
Each of those now reaches `Provider.Delete`, and `apisixProvider.Delete` calls
`defer d.syncNotify()` unconditionally — which triggers `Client.Sync`, a full
push of the whole store to every data plane.
### Suggestion: filter at the predicate layer, the way Ingress already does
`MatchesIngressClassPredicate` (`utils.go:1889`) solves exactly this:
```go
predicateFuncs.UpdateFunc = func(e event.UpdateEvent) bool {
return MatchesIngressClass(c, log, e.ObjectOld) ||
MatchesIngressClass(c, log, e.ObjectNew)
}
```
A route equivalent gives three properties at once:
| situation | outcome |
|---|---|
| route owned by another controller | old ✗ / new ✗ → never enqueued, zero
cost |
| GatewayClass temporarily unresolvable | ownership check fails → not
enqueued → data plane untouched |
| route just moved off my Gateway | old ✓ / new ✗ → admitted **exactly
once** → this PR's delete branch runs |
The predicate does not replace the delete branch added here — the single
admitted reconcile is what makes the cleanup happen. They are complementary.
Two implementation notes:
- It has to go on `For(&gatewayv1.HTTPRoute{},
builder.WithPredicates(...))`, not `WithEventFilter`. The latter is global and
would hand EndpointSlice / Gateway / policy objects to a route ownership check.
- Worth reusing the parentRef → Gateway → GatewayClass walk from
`ParseRouteParentRefs` instead of writing a second one, so the two cannot drift.
One known blind spot, probably acceptable: if a GatewayClass's
`controllerName` is edited in place, neither the route nor its Gateway changes
and the route reconcilers do not watch GatewayClass, so nothing enqueues those
routes. Changing `gatewayClassName` on the Gateway is covered by
`listHTTPRoutesForGateway`.
--
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]
