fix: fall back to publishService for Gateway status addresses - #461
fix: fall back to publishService for Gateway status addresses#461janiussyafiq wants to merge 2 commits into
Conversation
|
|
📝 WalkthroughWalkthroughThe controllers now resolve Gateway and Ingress status addresses from configured values or publish Services. They classify IPs and hostnames, deduplicate entries, clear stale status, and support LoadBalancer and ClusterIP publish Services. Unit and end-to-end tests cover these flows. ChangesStatus address resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟠 High · up to The PR adds Gateway status-address fallback through publishService, but lookup failures currently abort Gateway reconciliation before dataplane configuration and status conditions are updated. A missing, invalid, or temporarily unavailable Service can therefore leave affected Gateways unconfigured, so merge should wait until status resolution is decoupled from configuration sync or the behavior is explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant IngressController
participant PublishService
participant BackendIngress
participant IngressStatus
IngressController->>PublishService: Resolve publishService
PublishService-->>IngressController: Return Service type and status
IngressController->>BackendIngress: Read backend Ingress status
BackendIngress-->>IngressController: Return IP and hostname entries
IngressController->>IngressStatus: Deduplicate and update status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
docs/en/latest/reference/example.md (1)
1259-1260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new
publishServicefallback in the Gateway tab.This PR adds
publishServicesupport to the Gateway status path. The Gateway tab still documentsstatusAddressonly. Add thepublishServicebehavior for Gateway, including these two rules that the code implements:
statusAddresstakes precedence when both fields are set.- Only a
LoadBalancerpublish Service produces Gateway status addresses. AClusterIPpublish Service produces none for Gateway.I can draft the Gateway tab section if you want.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/en/latest/reference/example.md` around lines 1259 - 1260, Update the Gateway documentation section near the statusAddress description to document publishService fallback behavior: statusAddress takes precedence when both are configured, and only a LoadBalancer publish Service generates Gateway status addresses; a ClusterIP publish Service generates none.internal/controller/utils_publishservice_test.go (1)
39-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the dedup helpers.
deduplicateLoadBalancerIngressanddeduplicateGatewayStatusAddressesare pure functions in the same package. Unit tests pin the duplicate-removal result and the output order, so a later change to the ordering strategy is caught without an end-to-end run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/utils_publishservice_test.go` around lines 39 - 41, Add focused unit tests in the existing utility test suite for deduplicateLoadBalancerIngress and deduplicateGatewayStatusAddresses. Cover repeated addresses and assert both duplicate removal and preservation of the helpers’ current first-seen output order.test/e2e/gatewayapi/gateway.go (2)
855-914: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the fixtures from the previous context instead of redefining them.
defaultGatewayClass,defaultGateway, andcreateGatewayClassAndGatewayhere are identical to the definitions on lines 670-694 and 729-739. Both copies are new in this PR, so they will drift. Move them to theDescribescope and let both contexts use them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/gatewayapi/gateway.go` around lines 855 - 914, Move the duplicate defaultGatewayClass, defaultGateway, and createGatewayClassAndGateway definitions into the enclosing Describe scope, reusing the existing fixtures from the earlier context. Remove these local redefinitions so both contexts reference the shared fixtures and remain consistent.
944-963: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a publish Service that cannot be resolved.
The suite covers a
LoadBalancerpublish Service and thestatusAddressprecedence. It does not cover the two paths where resolution yields nothing:
publishServicenames a Service that does not exist.publishServicenames aClusterIPService, which the Gateway path skips.The first case matters most.
resolveStatusAddressesreturns an error there, andReconcilecurrently returns beforer.Provider.Update, so route configuration stops for that Gateway. A test that creates a Gateway with a danglingpublishServiceand then asserts that traffic still routes would pin that behavior. See my comment oninternal/controller/gateway_controller.golines 197-203.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/gatewayapi/gateway.go` around lines 944 - 963, The gateway end-to-end tests need a case for an unresolved publishService that verifies reconciliation still updates the provider and traffic routes. Add a test alongside the existing “falls back to publishService” case that creates a GatewayProxy referencing a nonexistent Service, creates the Gateway resources, and asserts successful routing; cover the ClusterIP publishService resolution path as well if consistent with the existing helpers.internal/controller/ingress_controller.go (1)
760-770: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMap the Service load-balancer entries directly instead of round-tripping through strings.
serviceLoadBalancerAddressesdiscards theIPandHostnamedistinction thatcorev1.LoadBalancerIngressalready carries. The code then rebuilds that distinction withnet.ParseIP. A Service whoseHostnamefield holds a literal IP string is reclassified asIP.Read the Service status fields directly in this branch. The
net.ParseIPclassification is still needed for thestatusAddressbranch, where the input is a plain string.♻️ Direct mapping
case corev1.ServiceTypeLoadBalancer: - for _, addr := range serviceLoadBalancerAddresses(svc) { - lbIngress := networkingv1.IngressLoadBalancerIngress{} - if net.ParseIP(addr) != nil { - lbIngress.IP = addr - } else { - lbIngress.Hostname = addr - } - loadBalancerStatus.Ingress = append(loadBalancerStatus.Ingress, lbIngress) - } + for _, lb := range svc.Status.LoadBalancer.Ingress { + if lb.IP != "" { + loadBalancerStatus.Ingress = append(loadBalancerStatus.Ingress, + networkingv1.IngressLoadBalancerIngress{IP: lb.IP}) + } + if lb.Hostname != "" { + loadBalancerStatus.Ingress = append(loadBalancerStatus.Ingress, + networkingv1.IngressLoadBalancerIngress{Hostname: lb.Hostname}) + } + }This also matches the shape of the
ClusterIPbranch below.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/ingress_controller.go` around lines 760 - 770, Update the ServiceTypeLoadBalancer branch to iterate over svc.Status.LoadBalancer.Ingress directly and map each corev1.LoadBalancerIngress IP to the networking ingress IP field and Hostname to the hostname field. Remove use of serviceLoadBalancerAddresses and net.ParseIP in this branch; retain net.ParseIP for the statusAddress branch.internal/controller/utils.go (1)
2125-2148: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe dedup helpers sort the input, so they change the published address order.
Both helpers sort before
slices.CompactFunc. The published order therefore becomes lexicographic instead of the configured or Service order. Two effects follow:
- For
statusAddress, the order inGatewayProxy.spec.statusAddressis not preserved.- For
deduplicateLoadBalancerIngress, a hostname-only entry has an emptyIP, so it sorts before any IP entry. A Service that reports[{IP: a}, {Hostname: b}]produces status[{Hostname: b}, {IP: a}].An order-preserving dedup keeps the status stable and matches the Service and config order. The result is still deterministic, so the change-detection comparison stays correct.
♻️ Order-preserving dedup
-func deduplicateLoadBalancerIngress(entries []networkingv1.IngressLoadBalancerIngress) []networkingv1.IngressLoadBalancerIngress { - slices.SortFunc(entries, func(a, b networkingv1.IngressLoadBalancerIngress) int { - if c := strings.Compare(a.IP, b.IP); c != 0 { - return c - } - return strings.Compare(a.Hostname, b.Hostname) - }) - return slices.CompactFunc(entries, func(a, b networkingv1.IngressLoadBalancerIngress) bool { - return a.IP == b.IP && a.Hostname == b.Hostname - }) -} +func deduplicateLoadBalancerIngress(entries []networkingv1.IngressLoadBalancerIngress) []networkingv1.IngressLoadBalancerIngress { + if len(entries) == 0 { + return entries + } + type key struct{ ip, hostname string } + seen := make(map[key]struct{}, len(entries)) + out := entries[:0] + for _, e := range entries { + k := key{e.IP, e.Hostname} + if _, ok := seen[k]; ok { + continue + } + seen[k] = struct{}{} + out = append(out, e) + } + return out +}Apply the same pattern to
deduplicateGatewayStatusAddresses, keyed onValue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/utils.go` around lines 2125 - 2148, Update deduplicateLoadBalancerIngress and deduplicateGatewayStatusAddresses to remove duplicates without sorting their input slices, preserving the first-seen Service or configured order while keeping the existing deduplication keys (IP plus Hostname, and Value respectively). Use an order-preserving seen-key approach and return the entries in their original order so downstream status comparison remains deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/en/latest/reference/example.md`:
- Around line 1312-1315: Update the publishService documentation to state that a
bare service name is resolved in the reconciled resource’s namespace—Ingress
namespace for the Ingress path and GatewayProxy namespace for the Gateway
path—rather than implying the Kubernetes default namespace. Also revise the
ClusterIP behavior to say the controller propagates both IP and hostname entries
from referencing Ingress resources.
In `@internal/controller/gateway_controller.go`:
- Around line 197-203: Update Reconcile around resolveStatusAddresses to capture
the error in an addressResolveErr variable, log it, and continue with an empty
status address list instead of returning early. Preserve dataplane
configuration, listener status, and condition updates, then return
addressResolveErr from the end of Reconcile after the status update has been
queued so lookup failures retry.
In `@test/e2e/ingress/ingress.go`:
- Around line 1503-1526: Make the source Ingress in the test explicitly target a
foreign IngressClass by adding an ingressClassName referencing a class managed
by another controller, and update the nearby comment to describe this isolation.
Preserve the existing sourceIngressYaml creation and ensure the selected class
cannot be claimed by this controller or any default class.
---
Nitpick comments:
In `@docs/en/latest/reference/example.md`:
- Around line 1259-1260: Update the Gateway documentation section near the
statusAddress description to document publishService fallback behavior:
statusAddress takes precedence when both are configured, and only a LoadBalancer
publish Service generates Gateway status addresses; a ClusterIP publish Service
generates none.
In `@internal/controller/ingress_controller.go`:
- Around line 760-770: Update the ServiceTypeLoadBalancer branch to iterate over
svc.Status.LoadBalancer.Ingress directly and map each corev1.LoadBalancerIngress
IP to the networking ingress IP field and Hostname to the hostname field. Remove
use of serviceLoadBalancerAddresses and net.ParseIP in this branch; retain
net.ParseIP for the statusAddress branch.
In `@internal/controller/utils_publishservice_test.go`:
- Around line 39-41: Add focused unit tests in the existing utility test suite
for deduplicateLoadBalancerIngress and deduplicateGatewayStatusAddresses. Cover
repeated addresses and assert both duplicate removal and preservation of the
helpers’ current first-seen output order.
In `@internal/controller/utils.go`:
- Around line 2125-2148: Update deduplicateLoadBalancerIngress and
deduplicateGatewayStatusAddresses to remove duplicates without sorting their
input slices, preserving the first-seen Service or configured order while
keeping the existing deduplication keys (IP plus Hostname, and Value
respectively). Use an order-preserving seen-key approach and return the entries
in their original order so downstream status comparison remains deterministic.
In `@test/e2e/gatewayapi/gateway.go`:
- Around line 855-914: Move the duplicate defaultGatewayClass, defaultGateway,
and createGatewayClassAndGateway definitions into the enclosing Describe scope,
reusing the existing fixtures from the earlier context. Remove these local
redefinitions so both contexts reference the shared fixtures and remain
consistent.
- Around line 944-963: The gateway end-to-end tests need a case for an
unresolved publishService that verifies reconciliation still updates the
provider and traffic routes. Add a test alongside the existing “falls back to
publishService” case that creates a GatewayProxy referencing a nonexistent
Service, creates the Gateway resources, and asserts successful routing; cover
the ClusterIP publishService resolution path as well if consistent with the
existing helpers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a1691bf6-68e5-4839-bed8-b6b1c99397cb
📒 Files selected for processing (8)
docs/en/latest/reference/example.mdinternal/controller/gateway_controller.gointernal/controller/ingress_controller.gointernal/controller/utils.gointernal/controller/utils_publishservice_test.gointernal/utils/k8s.gotest/e2e/gatewayapi/gateway.gotest/e2e/ingress/ingress.go
| When using `publishService`, the controller will use the endpoint of this Service to update the status information of the Ingress resource. The format can be either `namespace/svc-name` or simply `svc-name` if the default namespace is correctly set. | ||
|
|
||
| - If the Service is of `LoadBalancer` type, the controller uses its external IP or hostname. | ||
| - If the Service is of `ClusterIP` type, the controller propagates the hostname from any Ingress resources that reference that Service. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct two statements about publishService.
- Line 1312 says a bare name works "if the default namespace is correctly set". The controller resolves a bare name against the namespace of the resource being reconciled: the Ingress namespace for the Ingress path and the GatewayProxy namespace for the Gateway path. The current wording can be read as the Kubernetes
defaultnamespace. - Line 1315 says the controller propagates "the hostname" for a
ClusterIPService. The controller propagates both IP and hostname entries from the referencing Ingress resources.
📝 Suggested wording
-When using `publishService`, the controller will use the endpoint of this Service to update the status information of the Ingress resource. The format can be either `namespace/svc-name` or simply `svc-name` if the default namespace is correctly set.
+When using `publishService`, the controller will use the endpoint of this Service to update the status information of the Ingress resource. The format can be either `namespace/svc-name` or `svc-name`. If you omit the namespace, the controller resolves the name in the namespace of the Ingress resource.
- If the Service is of `LoadBalancer` type, the controller uses its external IP or hostname.
-- If the Service is of `ClusterIP` type, the controller propagates the hostname from any Ingress resources that reference that Service.
+- If the Service is of `ClusterIP` type, the controller propagates the IP and hostname entries from the status of any other Ingress resource that references that Service.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| When using `publishService`, the controller will use the endpoint of this Service to update the status information of the Ingress resource. The format can be either `namespace/svc-name` or simply `svc-name` if the default namespace is correctly set. | |
| - If the Service is of `LoadBalancer` type, the controller uses its external IP or hostname. | |
| - If the Service is of `ClusterIP` type, the controller propagates the hostname from any Ingress resources that reference that Service. | |
| When using `publishService`, the controller will use the endpoint of this Service to update the status information of the Ingress resource. The format can be either `namespace/svc-name` or `svc-name`. If you omit the namespace, the controller resolves the name in the namespace of the Ingress resource. | |
| - If the Service is of `LoadBalancer` type, the controller uses its external IP or hostname. | |
| - If the Service is of `ClusterIP` type, the controller propagates the IP and hostname entries from the status of any other Ingress resource that references that Service. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/en/latest/reference/example.md` around lines 1312 - 1315, Update the
publishService documentation to state that a bare service name is resolved in
the reconciled resource’s namespace—Ingress namespace for the Ingress path and
GatewayProxy namespace for the Gateway path—rather than implying the Kubernetes
default namespace. Also revise the ClusterIP behavior to say the controller
propagates both IP and hostname entries from referencing Ingress resources.
| 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 | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A publish Service lookup failure now blocks dataplane configuration for the Gateway.
The early return ctrl.Result{}, err happens before r.Provider.Update(ctx, tctx, gateway) on line 227, before getListenerStatus, and before the Accepted and Programmed conditions are written. A misspelled publishService, a Service that does not exist yet, or a transient API error therefore stops route and listener configuration for this Gateway, and the Gateway keeps no condition update. The failure mode is much wider than the status field the value feeds.
The Ingress path does not have this coupling: updateStatus in internal/controller/ingress_controller.go runs after translation, so a publish Service error there does not block config sync.
Resolve the addresses without aborting the reconcile. Record the failure, continue with no addresses, and requeue at the end so the lookup retries.
🛠️ Decouple status resolution from config sync
- 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
- }
- for _, addr := range statusAddresses {
+ statusAddresses, resolveErr := r.resolveStatusAddresses(ctx, &gatewayProxy)
+ if resolveErr != nil {
+ // Do not abort the reconcile: the provider update and the listener
+ // statuses below must not depend on the publish Service lookup.
+ r.Log.Error(resolveErr, "failed to resolve gateway status addresses", "gateway", req.NamespacedName)
+ addressResolveErr = resolveErr
+ }
+ for _, addr := range statusAddresses {Declare var addressResolveErr error next to var addrs []gatewayv1.GatewayStatusAddress on line 186, and return it from the end of Reconcile so controller-runtime retries with backoff after the status update is queued.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/gateway_controller.go` around lines 197 - 203, Update
Reconcile around resolveStatusAddresses to capture the error in an
addressResolveErr variable, log it, and continue with an empty status address
list instead of returning early. Preserve dataplane configuration, listener
status, and condition updates, then return addressResolveErr from the end of
Reconcile after the status update has been queued so lookup failures retry.
| By("create source Ingress referencing the ClusterIP service") | ||
| sourceIngressName := s.Namespace() + "-source" | ||
| // The source Ingress intentionally has no ingressClassName — it simulates a | ||
| // cloud ALB Ingress managed by a different controller. Without ingressClassName | ||
| // our controller will not reconcile it and will not overwrite its status. | ||
| sourceIngressYaml := fmt.Sprintf(` | ||
| apiVersion: networking.k8s.io/v1 | ||
| kind: Ingress | ||
| metadata: | ||
| name: %s | ||
| spec: | ||
| rules: | ||
| - host: clusterip.example.com | ||
| http: | ||
| paths: | ||
| - path: / | ||
| pathType: Prefix | ||
| backend: | ||
| service: | ||
| name: %s | ||
| port: | ||
| number: 80 | ||
| `, sourceIngressName, clusterIPSvcName) | ||
| Expect(s.CreateResourceFromStringWithNamespace(sourceIngressYaml, s.Namespace())).NotTo(HaveOccurred(), "creating source Ingress") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The source Ingress can be reconciled by this controller, which makes the test flaky.
The comment states that the controller will not reconcile the source Ingress because it has no ingressClassName. That holds only when no default IngressClass for this controller exists. This same file contains Context("IngressClass Selection", Serial), which creates an IngressClass annotated with ingressclass.kubernetes.io/is-default-class: "true" for this controller precisely so that class-less Ingress resources are reconciled.
If a default IngressClass for this controller is present when this test runs, the controller reconciles the source Ingress and overwrites the hostname that the test writes on line 1536. The assertion on line 1577 then fails.
Make the isolation explicit. Two options:
- Set an
ingressClassNameon the source Ingress that points at a foreign controller, so no default class can ever claim it. - Mark this
It(or the context)Serialand assert that no default IngressClass for this controller exists.
The first option is more robust because it does not depend on ordering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/ingress/ingress.go` around lines 1503 - 1526, Make the source
Ingress in the test explicitly target a foreign IngressClass by adding an
ingressClassName referencing a class managed by another controller, and update
the nearby comment to describe this isolation. Preserve the existing
sourceIngressYaml creation and ensure the selected class cannot be claimed by
this controller or any default class.
… controller files (backport apache/apisix-ingress-controller#2732) Cherry-picked unmodified from apache/apisix-ingress-controller#2732 (originally apache/apisix-ingress-controller#2730), authored upstream by Hemanth Chebrolu. This fork was missing it, and apache/apisix-ingress-controller#2846 builds on it.
Gateway.status.addresses was only populated from GatewayProxy.spec.statusAddress. When spec.publishService is set instead, the Gateway published no address, even though the Ingress status path already falls back to the publish Service's LoadBalancer address. Extract shared publish-Service resolution helpers, make Gateway.status.addresses fall back to the LoadBalancer address of spec.publishService when statusAddress is empty (statusAddress still wins when both are set), and refactor the Ingress LoadBalancer status branch onto the same helpers.
0376feb to
8c0c339
Compare
2 participants
Type of change:
What this PR does / why we need it:
Backport of apache/apisix-ingress-controller#2846.
Gateway.status.addresseswas only populated fromGatewayProxy.spec.statusAddress.When
spec.publishServiceis set instead, the Gateway published no address.This makes
Gateway.status.addressesfall back to the LoadBalancer address ofspec.publishServicewhenstatusAddressis empty (statusAddressstill wins when both are set), sharing the same publish-Service resolution helpers with the Ingress status path.This repo was also missing upstream apache/apisix-ingress-controller#2732 (typed/deduplicated status addresses, Ingress ClusterIP status propagation), which the fix builds on, so it is cherry-picked here as the first commit.
Pre-submission checklist:
Summary by CodeRabbit
New Features
Bug Fixes