Skip to content

fix: update publishService/statusAdress logic in gateway and ingress controller files (#2730)#2732

Open
hemanthjk-c wants to merge 10 commits intoapache:masterfrom
hemanthjk-c:bug-2730
Open

fix: update publishService/statusAdress logic in gateway and ingress controller files (#2730)#2732
hemanthjk-c wants to merge 10 commits intoapache:masterfrom
hemanthjk-c:bug-2730

Conversation

@hemanthjk-c
Copy link
Copy Markdown

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches
  • Documentation
  • Refactor
  • Chore
  • CI/CD or Tests

What this PR does / why we need it:

This PR fixes #2730

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes #2730 where apisix-ingress-controller only supported IP addresses in statusAddress and publishService configurations, rejecting hostnames (common in cloud environments like AWS EKS). The changes add hostname support by detecting whether addresses are IPs or hostnames and setting the appropriate fields in Gateway/Ingress status objects. Additionally, a new ClusterIP service flow is added for publishService, which propagates hostnames from Ingress resources fronting the ClusterIP service.

Changes:

  • Added IP vs hostname detection in gateway_controller.go and ingress_controller.go using net.ParseIP, setting the correct address type (IPAddress/HostnameAddressType for Gateway, IP/Hostname for Ingress).
  • Added ClusterIP service handling in ingress_controller.go that discovers Ingresses referencing the publish service and propagates their load balancer hostnames.
  • Added e2e tests and documentation updates for the new behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
internal/controller/gateway_controller.go Detect IP vs hostname in statusAddress and set Type field on Gateway status addresses
internal/controller/ingress_controller.go Detect IP vs hostname for statusAddress; add ClusterIP publishService support
test/e2e/gatewayapi/gateway.go E2e tests for Gateway status address type (IP and hostname)
test/e2e/ingress/ingress.go E2e tests for Ingress status address (IP, hostname, ClusterIP publishService)
docs/en/latest/reference/example.md Document IP/hostname support in statusAddress and ClusterIP publishService

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Baoyuantop
Copy link
Copy Markdown
Contributor

Review Comments

1. ServiceIndexRef semantic mismatch in ClusterIP branch

In ingress_controller.go, the new ClusterIP branch uses:

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:

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.4example.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.:

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).

@hemanthjk-c
Copy link
Copy Markdown
Author

Review Comments

1. ServiceIndexRef semantic mismatch in ClusterIP branch

In ingress_controller.go, the new ClusterIP branch uses:

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:

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.4example.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.:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: apisix-ingress-controller not supporting Hostnames when specified in gatewayproxy.spec.statusAddress

4 participants