johannes-engler-mw opened a new pull request, #2834:
URL: https://github.com/apache/apisix-ingress-controller/pull/2834
<!-- Please answer these questions before submitting a pull request -->
### Type of change:
- [x] Bugfix
### What this PR does / why we need it:
Fixes #2833.
When a Gateway API route's `parentRefs` are repointed at a Gateway belonging
to a different
`GatewayClass.spec.controllerName`, the new controller picks the route up,
but the previous
controller never removes the configuration it had already pushed. Its data
plane keeps serving
the route indefinitely — until the controller process happens to restart.
`ParseRouteParentRefs` skips parentRefs whose GatewayClass belongs to
another controller
(`utils.go:357-359`), so the reconciler ends up with an empty gateway list
and returns early:
```go
if len(gateways) == 0 {
return ctrl.Result{}, nil
}
```
`Provider.Delete` was only reachable from the `IgnoreNotFound` branch, i.e.
when the object is
deleted from the cluster. There was no path for "this object used to be mine
and no longer is".
Because the periodic sync replays the in-memory store
(`ConfigManager.List()` →
`GetResources()`) rather than re-deriving state from Kubernetes, the
orphaned service is
re-pushed to the data plane on every sync period. Restarting the controller
is the only thing
that clears it, since startup rebuilds the store from scratch.
This PR calls `Provider.Delete` before that early return, in all five route
reconcilers
(HTTPRoute, GRPCRoute, TCPRoute, UDPRoute, TLSRoute).
This is the same fix #2543 applied to Ingress for the equivalent
`ingressClassName` case, whose
description states the general problem:
> the controller simply ignores the Ingress resource. However, this behavior
is problematic: the
> corresponding configuration is not removed and remains in the data plane,
which may result in
> incorrect routing or stale configurations.
`ingress_controller.go:159-166` has carried that cleanup since; the Gateway
API reconcilers never
received the equivalent.
#### Notes for reviewers
- **`TypeMeta` is set before the call.** `Provider.Delete` derives both the
store key
(`utils.NamespacedNameKind`) and the resource labels (`label.GenLabel`)
from
`obj.GetObjectKind()`, which is empty on objects read through the client.
The existing
`IgnoreNotFound` branches already set it for the same reason.
- **No-op when there is nothing to remove.** `DeleteConfig` →
`applyStoreChanges(args, true)`
looks the key up in `ConfigManager` first; for a route this controller
never translated, the
delta is empty and neither the store nor ADC is touched. Routes belonging
to other controllers
therefore cost one map lookup.
- **One observation left alone deliberately.** `apisixProvider.Delete` calls
`syncNotify()`
unconditionally, so a reconcile of a route owned by another controller now
nudges the sync
loop even when the delta was empty. `syncNotify` is a non-blocking send to
a buffered channel,
so these coalesce, but making it conditional on a non-empty delta would
mean changing the
`Provider.Delete` signature to return the delta. That felt out of scope
here — happy to follow
up separately if you would prefer it in this PR.
### Pre-submission checklist:
- [x] Did you explain what problem does this PR solve? Or what new features
have been added?
- [x] Have you added corresponding test cases?
- [ ] Have you modified the corresponding document?
- [x] Is this PR backward compatible?
The test is in `test/e2e/gatewayapi/httproute.go`, in the existing
"HTTPRoute with Multiple
Gateway" context: it attaches an HTTPRoute to the additional gateway, then
repoints `parentRefs`
at a Gateway whose GatewayClass names `apisix.apache.org/not-exist`. The
route object itself is
never deleted, so it exercises the empty-gateway-list path rather than the
deletion path.
Verified against a kind cluster by rebuilding the controller image from each
revision and
re-running the same test binary:
```console
# controller built from master, test present
$ make e2e-test TEST_FOCUS="stop being served after moving"
[FAIL] ... HTTPRoute should stop being served after moving to another
controller's Gateway
test/e2e/gatewayapi/httproute.go:461
FAIL! -- 0 Passed | 1 Failed | 1 Pending | 232 Skipped
# controller built with this fix, same test
$ make e2e-test TEST_FOCUS="stop being served after moving"
SUCCESS! -- 1 Passed | 0 Failed | 1 Pending | 232 Skipped
```
Line 461 is the closing assertion: every earlier step passes in both runs,
including the one
that confirms the route is served through the additional gateway before the
move. Without the
fix the gateway keeps answering `200` for the full 30s retry window after
`parentRefs` move away.
--
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]
