You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "AlinsRan (via GitHub)" <gi...@apache.org> on 2026/07/21 11:19:36 UTC
[PR] fix: retry discovery on transient errors so API outages don't skip field indexes [apisix-ingress-controller]
AlinsRan opened a new pull request, #2817:
URL: https://github.com/apache/apisix-ingress-controller/pull/2817
### Type of change:
- [x] Bugfix
### What this PR does / why we need it:
Fixes #2734
After a control-plane failover (kube-apiserver transiently unreachable at controller startup), the controller logged `Index with name field:serviceRefs/secretRefs/ingressClassParametersRef does not exist` forever and every reconcile failed until manual restart — and even restart didn't help if the API was still unstable at boot.
Root cause: `HasAPIResource` created a discovery client and, on **any** error, returned `false` — indistinguishable from "CRD genuinely absent". Field-index registration is gated on `HasAPIResource` and runs once at startup with no retry, so a transient discovery failure permanently skipped the indexes; informers later reconnected and reconcile ran, but every field-selector `List` then failed forever.
Fix: classify the discovery error. A definitive answer (GroupVersion `NotFound`, or discovery `Forbidden`) returns immediately; a transient transport/availability error (connection refused, timeout, 5xx, throttling) is retried with exponential backoff (1s→30s cap) until a definitive result. A reachable API server always yields a definitive result, so the loop only waits while the API server is actually unreachable.
Why not register the indexes unconditionally: controller-runtime's `IndexField` eagerly resolves the GVK through the RESTMapper, which errors for a genuinely-absent CRD — so unconditional registration would break setup for optional CRDs that aren't installed.
### Discussion point
On a *persistently* unreachable API server, `HasAPIResource` now blocks setup (before `mgr.Start`) until the API is reachable, rather than proceeding into a permanently-broken state. This is intentional (the controller can't function without the API server), but there is no overall deadline — a hard bound via context could be added if preferred, at the cost of threading a context through the ~20 call sites. Feedback welcome.
### Pre-submission checklist:
- [x] Did you explain what problem does this PR solve?
- [x] Have you added corresponding test cases? (transient-then-success regression + error classification)
- [ ] Have you modified the corresponding document? (n/a)
- [x] Is this PR backward compatible?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: never treat an unreachable API server as a missing API resource [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #2817:
URL: https://github.com/apache/apisix-ingress-controller/pull/2817#discussion_r3656099539
##########
pkg/utils/k8s.go:
##########
@@ -52,29 +121,124 @@ func HasAPIResourceWithLogger(mgr ctrl.Manager, obj client.Object, logger logr.L
"groupVersion", groupVersion,
)
- // Create discovery client
- discoveryClient, err := discovery.NewDiscoveryClientForConfig(mgr.GetConfig())
- if err != nil {
- logger.Info("failed to create discovery client", "error", err)
- return false
- }
-
// Query server resources for the specific group/version
- apiResources, err := discoveryClient.ServerResourcesForGroupVersion(groupVersion)
- if err != nil {
+ var apiResources *metav1.APIResourceList
+ err := retryUntilDefinitive(backoff, logger, func() error {
+ var err error
+ apiResources, err = discoveryClient.ServerResourcesForGroupVersion(groupVersion)
+ return err
+ })
+ switch {
+ case err == nil:
+ case apierrors.IsNotFound(err):
logger.Info("group/version not available in cluster", "error", err)
- return false
+ return false, nil
+ case apierrors.IsForbidden(err):
Review Comment:
Agreed — you and the EE reviewer landed on the same conclusion independently, and my earlier justification was wrong. The `nonResourceURLs`-denies-one-group case is real but rare, and it is not worth the failure mode: the result is cached for the manager's lifetime, so any RBAC slip permanently omits a controller and its indexes behind a ready manager.
Fixed in 9490f6d6: `isResourceAbsent` is now `isDefinitive` (it only decides whether retrying can change the answer), and `hasAPIResource` maps the two definitive cases apart — 404 resolves to absent, Forbidden returns an error naming the service account's discovery permissions. `TestHasAPIResource/discovery_forbidden` now asserts the error.
Same change in api7/api7-ingress-controller#444.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: never treat an unreachable API server as a missing API resource [apisix-ingress-controller]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan merged PR #2817:
URL: https://github.com/apache/apisix-ingress-controller/pull/2817
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: never treat an unreachable API server as a missing API resource [apisix-ingress-controller]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #2817:
URL: https://github.com/apache/apisix-ingress-controller/pull/2817#discussion_r3656003303
##########
pkg/utils/k8s.go:
##########
@@ -52,29 +121,124 @@ func HasAPIResourceWithLogger(mgr ctrl.Manager, obj client.Object, logger logr.L
"groupVersion", groupVersion,
)
- // Create discovery client
- discoveryClient, err := discovery.NewDiscoveryClientForConfig(mgr.GetConfig())
- if err != nil {
- logger.Info("failed to create discovery client", "error", err)
- return false
- }
-
// Query server resources for the specific group/version
- apiResources, err := discoveryClient.ServerResourcesForGroupVersion(groupVersion)
- if err != nil {
+ var apiResources *metav1.APIResourceList
+ err := retryUntilDefinitive(backoff, logger, func() error {
+ var err error
+ apiResources, err = discoveryClient.ServerResourcesForGroupVersion(groupVersion)
+ return err
+ })
+ switch {
+ case err == nil:
+ case apierrors.IsNotFound(err):
logger.Info("group/version not available in cluster", "error", err)
- return false
+ return false, nil
+ case apierrors.IsForbidden(err):
Review Comment:
[P1] Forbidden discovery cannot be treated as “resource not installed”. This result is decided once for the lifetime of the manager, so an RBAC or discovery misconfiguration starts a ready controller with the corresponding controllers and indexes permanently omitted, recreating the half-disabled startup mode this PR is meant to prevent. Return the Forbidden error and fail startup; only a real NotFound is evidence that the API is absent.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org