Copilot commented on code in PR #2732:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2732#discussion_r2944195597
##########
internal/controller/ingress_controller.go:
##########
@@ -694,9 +695,13 @@ func (r *IngressReconciler) updateStatus(ctx
context.Context, tctx *provider.Tra
if addr == "" {
continue
}
- loadBalancerStatus.Ingress =
append(loadBalancerStatus.Ingress, networkingv1.IngressLoadBalancerIngress{
- IP: addr,
- })
+ ingress := networkingv1.IngressLoadBalancerIngress{}
+ if net.ParseIP(addr) != nil {
+ ingress.IP = addr
+ } else {
+ ingress.Hostname = addr
+ }
+ loadBalancerStatus.Ingress =
append(loadBalancerStatus.Ingress, ingress)
Review Comment:
The local variable `ingress` shadows the function parameter `ingress
*networkingv1.Ingress` (line 680). While it doesn't cause a bug in the current
code because the parameter isn't accessed within this `for` loop body, it's
fragile — a future edit inside this loop that intends to reference the outer
`*networkingv1.Ingress` would silently use the wrong variable. Consider
renaming this to something like `lbIngress` or `lbEntry` to avoid the shadowing.
##########
test/e2e/ingress/ingress.go:
##########
@@ -1294,4 +1296,226 @@ spec:
Should(Equal(http.StatusOK))
})
})
+
+ Context("Ingress Status Address", func() {
+ var gatewayProxyWithStatusAddressYaml = `
+apiVersion: apisix.apache.org/v1alpha1
+kind: GatewayProxy
+metadata:
+ name: apisix-proxy-config
+ namespace: %s
+spec:
+ statusAddress:
+ - %s
+ provider:
+ type: ControlPlane
+ controlPlane:
+ endpoints:
+ - %s
+ auth:
+ type: AdminKey
+ adminKey:
+ value: "%s"
+`
+ var gatewayProxyWithPublishServiceYaml = `
+apiVersion: apisix.apache.org/v1alpha1
+kind: GatewayProxy
+metadata:
+ name: apisix-proxy-config
+ namespace: %s
+spec:
+ publishService: %s/%s
+ provider:
+ type: ControlPlane
+ controlPlane:
+ endpoints:
+ - %s
+ auth:
+ type: AdminKey
+ adminKey:
+ value: "%s"
+`
+ var ingressClassYaml = `
+apiVersion: networking.k8s.io/v1
+kind: IngressClass
+metadata:
+ name: %s
+spec:
+ controller: "%s"
+ parameters:
+ apiGroup: "apisix.apache.org"
+ kind: "GatewayProxy"
+ name: "apisix-proxy-config"
+ namespace: "%s"
+ scope: "Namespace"
+`
+ var ingressYaml = `
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+ name: %s
+spec:
+ ingressClassName: %s
+ rules:
+ - host: status.example.com
+ http:
+ paths:
+ - path: /
+ pathType: Prefix
+ backend:
+ service:
+ name: %s
+ port:
+ number: 80
+`
+ getIngressLBStatus := func(ingressName string)
([]networkingv1.IngressLoadBalancerIngress, error) {
+ ing := &networkingv1.Ingress{}
+ if err := s.K8sClient.Get(context.Background(),
types.NamespacedName{
+ Name: ingressName,
+ Namespace: s.Namespace(),
+ }, ing); err != nil {
+ return nil, err
+ }
+ return ing.Status.LoadBalancer.Ingress, nil
+ }
+
+ checkIngressStatusAddress := func(addrValue, ingressSuffix,
expectedIP, expectedHostname string) {
+ ingressClassName := s.Namespace()
+
+ By("create GatewayProxy with statusAddress")
+ gatewayProxy :=
fmt.Sprintf(gatewayProxyWithStatusAddressYaml,
+ s.Namespace(), addrValue,
s.Deployer.GetAdminEndpoint(), s.AdminKey())
+
Expect(s.CreateResourceFromStringWithNamespace(gatewayProxy,
s.Namespace())).NotTo(HaveOccurred(), "creating GatewayProxy")
+
+ By("create IngressClass")
+ Expect(s.CreateResourceFromStringWithNamespace(
+ fmt.Sprintf(ingressClassYaml, ingressClassName,
s.GetControllerName(), s.Namespace()), ""),
+ ).NotTo(HaveOccurred(), "creating IngressClass")
Review Comment:
The `checkIngressStatusAddress` helper creates a cluster-scoped
`IngressClass` (line 1391-1393) but does not clean it up with `DeferCleanup`.
In contrast, the ClusterIP test below (line 1514-1518) properly cleans up its
IngressClass. This could leak cluster-scoped resources across test runs.
Consider adding `DeferCleanup` inside `checkIngressStatusAddress` to delete the
IngressClass after each test, consistent with the pattern used in the ClusterIP
test case.
##########
internal/controller/ingress_controller.go:
##########
@@ -731,6 +737,33 @@ func (r *IngressReconciler) updateStatus(ctx
context.Context, tctx *provider.Tra
})
}
}
+ case corev1.ServiceTypeClusterIP:
+ // for ClusterIP services, find Ingresses that
reference this service
+ // and collect hostnames from their load
balancer status
+ // this is when we run the apisix in ClusterIP
mode and enable Ingress
+ // when deploying in Cloud environments.
+ ingressList := &networkingv1.IngressList{}
+ if err := r.List(ctx, ingressList,
client.MatchingFields{
+ indexer.ServiceIndexRef:
indexer.GenIndexKey(namespace, name),
+ }); err != nil {
+ return fmt.Errorf("failed to list
ingresses for ClusterIP service %s/%s: %w", namespace, name, err)
+ }
+ for _, ing := range ingressList.Items {
+ // Skip the current Ingress being
reconciled to avoid a
+ // self-referential loop: updating its
own status would trigger
+ // a new reconcile, which would collect
its own (just-written)
+ // hostname again and potentially
repeat indefinitely.
+ if ing.Namespace == ingress.Namespace
&& ing.Name == ingress.Name {
+ continue
+ }
+ for _, lb := range
ing.Status.LoadBalancer.Ingress {
+ if lb.Hostname != "" {
+
loadBalancerStatus.Ingress = append(loadBalancerStatus.Ingress,
networkingv1.IngressLoadBalancerIngress{
+ Hostname:
lb.Hostname,
+ })
+ }
+ }
Review Comment:
In the ClusterIP case, only `Hostname` values are propagated from referenced
Ingresses, but `IP` values from their load balancer status are silently
ignored. If an Ingress fronting the ClusterIP service has an IP-based load
balancer status (e.g., `lb.IP` is set but `lb.Hostname` is empty), that address
won't be propagated. Consider also propagating `lb.IP` for completeness,
similar to the LoadBalancer case above which handles both IP and Hostname.
--
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]
