AlinsRan commented on code in PR #2846:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2846#discussion_r3780474283
##########
internal/controller/gateway_controller.go:
##########
@@ -194,10 +194,14 @@ func (r *GatewayReconciler) Reconcile(ctx
context.Context, req ctrl.Request) (ct
msg: "gateway proxy not found",
}
} else {
- for _, addr := range gatewayProxy.Spec.StatusAddress {
- if addr == "" {
- continue
- }
+ statusAddresses, err := r.resolveStatusAddresses(ctx,
&gatewayProxy)
+ if err != nil {
+ // fail the reconcile so a missing or invalid publish
Service retries
+ // with backoff, mirroring the Ingress status path
+ r.Log.Error(err, "failed to resolve gateway status
addresses", "gateway", req.NamespacedName)
+ return ctrl.Result{}, err
Review Comment:
This returns before `getListenerStatus` (L228), `r.Provider.Update` (L234)
and `r.Updater.Update` (L265), so an unresolvable `publishService` now blocks
the data plane, not just the status address. A GatewayProxy with
`publishService: apisix/not-exists` (typo, or GitOps applying the Gateway
before the Service exists) means the Gateway is never translated and pushed to
APISIX, and gets no Accepted/Programmed/listener conditions at all. For a
malformed value like `a/b/c` the error is permanent, so this retries with
backoff forever and the Gateway never becomes Programmed.
The comment says this mirrors the Ingress status path, but it doesn't:
`IngressReconciler.updateStatus` is called *after* `r.Provider.Update`
(ingress_controller.go:207 vs 216), so the config is already pushed by the time
the status lookup can fail. It also diverges from the local convention in this
function — both the `gateway proxy not found` branch above and a failing
`r.Provider.Update` below record the problem in a condition and keep going.
Suggest logging the error and publishing no address (optionally surfacing it
in a condition), or at minimum moving this check after `r.Provider.Update`.
##########
internal/controller/gateway_controller.go:
##########
@@ -259,6 +263,38 @@ func (r *GatewayReconciler) Reconcile(ctx context.Context,
req ctrl.Request) (ct
return ctrl.Result{}, nil
}
+// resolveStatusAddresses returns the addresses to publish in
+// Gateway.status.addresses: the statically configured statusAddress if set,
+// otherwise the external addresses of the Service named by publishService.
+// This mirrors the Ingress status path, so the same GatewayProxy yields the
+// same addresses for both APIs.
+func (r *GatewayReconciler) resolveStatusAddresses(
+ ctx context.Context,
+ gatewayProxy *v1alpha1.GatewayProxy,
+) ([]string, error) {
+ if len(gatewayProxy.Spec.StatusAddress) > 0 {
+ return utils.Filter(gatewayProxy.Spec.StatusAddress, func(addr
string) bool {
+ return addr != ""
+ }), nil
+ }
+
+ if gatewayProxy.Spec.PublishService == "" {
+ return nil, nil
+ }
+
+ // a bare name is resolved against the GatewayProxy's namespace
+ svc, err := resolvePublishService(ctx, r.Client,
gatewayProxy.Spec.PublishService, gatewayProxy.GetNamespace())
Review Comment:
A bare `publishService` name resolves differently in the two paths: here
against the GatewayProxy's namespace, but in `IngressReconciler.updateStatus`
against `ingress.Namespace` (ingress_controller.go:711-718, unchanged by this
PR). Those are not the same namespace in general — a Gateway's GatewayProxy is
always fetched from the Gateway's own namespace (utils.go:1311), while an
Ingress's comes from the IngressClass parameters namespace
(`GetIngressClassParametersNamespace`, defaulting to `default`).
Concretely: GatewayProxy in `ingress-apisix` with `publishService:
apisix-gateway`, Ingress in `app` — the Ingress path looks up
`app/apisix-gateway` (NotFound, `updateStatus` returns an error), the Gateway
path looks up `ingress-apisix/apisix-gateway` (found). So "the same
GatewayProxy yields the same addresses for both APIs" does not hold. It also
doesn't hold when the Service is ClusterIP: the Ingress path propagates
addresses from other Ingresses, this one returns nothing.
Resolving against the GatewayProxy's namespace looks like the more
defensible rule of the two; the fix is probably to make the Ingress path do the
same. Either way the comment should be adjusted.
--
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]
