Hemanth1205 commented on PR #2732:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2732#issuecomment-4096839937
> ## Review Comments
> ### 1. `ServiceIndexRef` semantic mismatch in ClusterIP branch
> In `ingress_controller.go`, the new ClusterIP branch uses:
>
> ```go
> ingressList := &networkingv1.IngressList{}
> if err := r.List(ctx, ingressList, client.MatchingFields{
> indexer.ServiceIndexRef: indexer.GenIndexKey(namespace, name),
> }); err != nil {
> ```
>
> Here `namespace` and `name` come from the `publishService` config (i.e.,
the APISIX gateway's ClusterIP Service). However, `ServiceIndexRef` is built by
`IngressServiceIndexFunc`, which indexes Ingress objects by their **backend
services** in `spec.rules[].http.paths[].backend.service.name` — not by any
arbitrary service reference.
>
> This works **only** when some Ingress in the cluster has the APISIX
ClusterIP Service as a backend in its routing rules (which is the scenario in
#2730 — a cloud ALB Ingress fronting the APISIX ClusterIP Service). But this is
an **implicit assumption** that isn't documented anywhere.
>
> **Suggestions:**
>
> * Add a code comment explaining this assumption explicitly (e.g., "This
relies on another Ingress existing in the cluster whose `spec.rules` backends
point at this ClusterIP service").
> * Log at info/debug level when `ingressList.Items` is empty after the
query, so users can diagnose why ClusterIP `publishService` isn't propagating
status.
>
> ### 2. `len()` short-circuit condition prevents status updates when values
change
> This is a **pre-existing issue** in `gateway_controller.go`, but the PR
builds on top of it and now makes it worse by adding the `Type` field:
>
> ```go
> if len(gateway.Status.Addresses) != len(gatewayProxy.Spec.StatusAddress) {
> for _, addr := range gatewayProxy.Spec.StatusAddress {
> ```
>
> This condition only triggers an update when the **count** of addresses
changes. If a user modifies a `statusAddress` value without changing the count
(e.g., `1.2.3.4` → `example.com`), the status won't be updated — the Gateway
will keep the old address and the old (or missing) `Type`.
>
> Since this PR introduces the `Type` field on `GatewayStatusAddress`, this
short-circuit becomes more impactful: even if the existing addresses lack a
`Type` (pre-upgrade state), they won't be backfilled as long as the count
matches.
>
> **Suggestion:** Replace the `len()` comparison with a proper equality
check, e.g.:
>
> ```go
> if !reflect.DeepEqual(gateway.Status.Addresses, addrs) {
> ```
>
> or build `addrs` unconditionally and let the comparison guard the actual
update call (similar to how `ingress_controller.go` already does it with
`reflect.DeepEqual` at line 739).
@Baoyuantop , thanks for bringing this up. I've updated both the files with
the suggested changes.
Also added test cases to cover the new changes
--
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]
