followup changes from #685

pkg/reconciler/ingress/ingress.go

@@ -148,83 +148,83 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
 
 	// TODO: check Gateway readiness before reporting Ingress ready
 	if routesReady {
-		var publicLbs, privateLbs []v1alpha1.LoadBalancerIngressStatus
+		lbs, err := c.lookUpLoadBalancers(ing, pluginConfig)
 
-		externalGateway := pluginConfig.ExternalGateway()
-		localGateway := pluginConfig.LocalGateway()
-
-		publicLbs, err = c.determineLoadBalancerIngressStatus(externalGateway)
-		if err != nil {
-			if apierrs.IsNotFound(err) {
-				ing.Status.MarkLoadBalancerFailed(
-					"GatewayDoesNotExist",
-					fmt.Sprintf(
-						"could not find Gateway %s/%s",
-						externalGateway.Namespace,
-						externalGateway.Name,
-					),
-				)
-				return fmt.Errorf("Gateway %s does not exist: %w", externalGateway.Name, err) //nolint:stylecheck
-			}
-			ing.Status.MarkLoadBalancerNotReady()
-			return err
-		}
-
-		privateLbs, err = c.determineLoadBalancerIngressStatus(localGateway)
 		if err != nil {
-			if apierrs.IsNotFound(err) {
-				ing.Status.MarkLoadBalancerFailed(
-					"GatewayDoesNotExist",
-					fmt.Sprintf(
-						"could not find Gateway %s/%s",
-						localGateway.Namespace,
-						localGateway.Name,
-					),
-				)
-				return fmt.Errorf("Gateway %s does not exist: %w", localGateway.Name, err) //nolint:stylecheck
-			}
 			ing.Status.MarkLoadBalancerNotReady()
 			return err
 		}
-
-		ing.Status.MarkLoadBalancerReady(publicLbs, privateLbs)
+		ing.Status.MarkLoadBalancerReady(lbs[v1alpha1.IngressVisibilityExternalIP], lbs[v1alpha1.IngressVisibilityClusterLocal])
 	} else {
 		ing.Status.MarkLoadBalancerNotReady()
 	}
 
 	return nil
 }
 
-// determineLoadBalancerIngressStatus will return the address for the Gateway.
-// If a service is provided, it will return the address of the service.
-// Otherwise, it will return the first address in the Gateway status.
-func (c *Reconciler) determineLoadBalancerIngressStatus(gwc config.Gateway) ([]v1alpha1.LoadBalancerIngressStatus, error) {
-	if gwc.Service != nil {
-		return []v1alpha1.LoadBalancerIngressStatus{
-			{DomainInternal: network.GetServiceHostname(gwc.Service.Name, gwc.Service.Namespace)},
-		}, nil
+// lookUpLoadBalancers will return a map of visibilites to
+// LoadBalancerIngressStatuses for the current Gateways in use.
+func (c *Reconciler) lookUpLoadBalancers(ing *v1alpha1.Ingress, gpc *config.GatewayPlugin) (map[v1alpha1.IngressVisibility][]v1alpha1.LoadBalancerIngressStatus, error) {
+	externalStatuses, err := c.collectLBIngressStatus(ing, gpc.ExternalGateway())
+	if err != nil {
+		return nil, err
 	}
-	gw, err := c.gatewayLister.Gateways(gwc.Namespace).Get(gwc.Name)
+
+	internalStatuses, err := c.collectLBIngressStatus(ing, gpc.LocalGateway())
 	if err != nil {
 		return nil, err
 	}
 
-	var lbis v1alpha1.LoadBalancerIngressStatus
+	return map[v1alpha1.IngressVisibility][]v1alpha1.LoadBalancerIngressStatus{
+		v1alpha1.IngressVisibilityExternalIP:   externalStatuses,
+		v1alpha1.IngressVisibilityClusterLocal: internalStatuses,
+	}, nil
+}
 
-	if len(gw.Status.Addresses) > 0 {
-		switch *gw.Status.Addresses[0].Type {
-		case gatewayapi.IPAddressType:
-			lbis = v1alpha1.LoadBalancerIngressStatus{IP: gw.Status.Addresses[0].Value}
-		default:
-			// Should this actually be under Domain? It seems like the rest of the code expects DomainInternal though...
-			lbis = v1alpha1.LoadBalancerIngressStatus{DomainInternal: gw.Status.Addresses[0].Value}
+// collectLBIngressStatus will return LoadBalancerIngressStatuses for the
+// provided single Gateway config. If a service is available on a Gateway, it will
+// return the address of the service.  Otherwise, it will return the first
+// address in the Gateway status.
+func (c *Reconciler) collectLBIngressStatus(ing *v1alpha1.Ingress, gwc config.Gateway) ([]v1alpha1.LoadBalancerIngressStatus, error) {
+	statuses := []v1alpha1.LoadBalancerIngressStatus{}
+
+	// TODO: currently only 1 gateway is supported. When the config is updated to
+	// support multiple, this code must change to find out which Gateway is
+	// appropriate for the given Ingress
+	if gwc.Service != nil {
+		statuses = append(statuses, v1alpha1.LoadBalancerIngressStatus{
+			DomainInternal: network.GetServiceHostname(gwc.Service.Name, gwc.Service.Namespace),
+		})
+	} else {
+		gw, err := c.gatewayLister.Gateways(gwc.Namespace).Get(gwc.Name)
+		if err != nil {
+			if apierrs.IsNotFound(err) {
+				ing.Status.MarkLoadBalancerFailed(
+					"GatewayDoesNotExist",
+					fmt.Sprintf(
+						"could not find Gateway %s/%s",
+						gwc.Namespace,
+						gwc.Name,
+					),
+				)
+			}
+			return nil, fmt.Errorf("could not find Gateway \"%s/%s\": %w", gwc.Namespace, gwc.Name, err)
 		}
 
-		return []v1alpha1.LoadBalancerIngressStatus{lbis}, nil
+		if len(gw.Status.Addresses) > 0 {
+			switch *gw.Status.Addresses[0].Type {
+			case gatewayapi.IPAddressType:
+				statuses = append(statuses, v1alpha1.LoadBalancerIngressStatus{IP: gw.Status.Addresses[0].Value})
+			default:
+				// Should this actually be under Domain? It seems like the rest of the code expects DomainInternal though...
+				statuses = append(statuses, v1alpha1.LoadBalancerIngressStatus{DomainInternal: gw.Status.Addresses[0].Value})
+			}
+		} else {
+			return nil, fmt.Errorf("no address found in status of Gateway %s/%s", gwc.Namespace, gwc.Name)
+		}
 	}
 
-	return nil, fmt.Errorf("Gateway %q does not have an address in status", gwc.NamespacedName) //nolint:stylecheck
-
+	return statuses, nil
 }
 
 // isHTTPRouteReady will check the status conditions of the ingress and return true if

pkg/reconciler/ingress/ingress_test.go

@@ -62,6 +62,7 @@ var (
 	fakeStatusKey struct{}
 
 	publicGatewayAddress  = "11.22.33.44"
+	publicGatewayHostname = "off.cluster.gateway"
 	privateGatewayAddress = "55.66.77.88"
 )
 
@@ -2233,12 +2234,32 @@ func TestReconcileProbingOffClusterGateway(t *testing.T) {
 		Objects: append([]runtime.Object{
 			ing(withBasicSpec, withGatewayAPIclass, withFinalizer, withInitialConditions),
 			httpRoute(t, ing(withBasicSpec, withGatewayAPIclass), httpRouteReady),
-			gw(defaultListener, setStatusPublicAddress),
+			gw(defaultListener, setStatusPublicAddressIP),
 			gw(privateGw, defaultListener, setStatusPrivateAddress),
 		}, servicesAndEndpoints...),
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{
 			{Object: ing(withBasicSpec, withGatewayAPIclass, withFinalizer, makeItReadyOffClusterGateway)},
 		},
+	}, {
+		Name: "gateway has hostname in address",
+		Key:  "ns/name",
+		Ctx: withStatusManager(&fakeStatusManager{
+			FakeDoProbes: func(context.Context, status.Backends) (status.ProbeState, error) {
+				return status.ProbeState{Ready: true}, nil
+			},
+			FakeIsProbeActive: func(types.NamespacedName) (status.ProbeState, bool) {
+				return status.ProbeState{Ready: true}, true
+			},
+		}),
+		Objects: append([]runtime.Object{
+			ing(withBasicSpec, withGatewayAPIclass, withFinalizer, withInitialConditions),
+			httpRoute(t, ing(withBasicSpec, withGatewayAPIclass), httpRouteReady),
+			gw(defaultListener, setStatusPublicAddressHostname),
+			gw(privateGw, defaultListener, setStatusPrivateAddress),
+		}, servicesAndEndpoints...),
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{
+			{Object: ing(withBasicSpec, withGatewayAPIclass, withFinalizer, makeItReadyOffClusterGatewayHostname)},
+		},
 	}, {
 		Name: "gateway not ready",
 		Key:  "ns/name",
@@ -2258,17 +2279,49 @@ func TestReconcileProbingOffClusterGateway(t *testing.T) {
 		}, servicesAndEndpoints...),
 		WantErr: true,
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{
-			//{Object: ing(withBasicSpec, withGatewayAPIclass, withFinalizer, makeItReadyOffClusterGateway)},
-			{Object: ing(withBasicSpec, withGatewayAPIClass, withFinalizer, func(i *v1alpha1.Ingress) {
+			{Object: ing(
+				withBasicSpec,
+				withGatewayAPIClass,
+				withFinalizer,
+				func(i *v1alpha1.Ingress) {
+					i.Status.InitializeConditions()
+					i.Status.MarkLoadBalancerNotReady()
+					i.Status.MarkNetworkConfigured()
+					i.Status.MarkIngressNotReady("ReconcileIngressFailed", "Ingress reconciliation failed")
+				},
+			)},
+		},
+		WantEvents: []string{
+			Eventf(corev1.EventTypeWarning, "InternalError", `no address found in status of Gateway istio-system/istio-gateway`),
+		},
+	}, {
+		Name: "gateway doesn't exist",
+		Key:  "ns/name",
+		Ctx: withStatusManager(&fakeStatusManager{
+			FakeDoProbes: func(context.Context, status.Backends) (status.ProbeState, error) {
+				return status.ProbeState{Ready: true}, nil
+			},
+			FakeIsProbeActive: func(types.NamespacedName) (status.ProbeState, bool) {
+				return status.ProbeState{Ready: true}, true
+			},
+		}),
+		Objects: append([]runtime.Object{
+			ing(withBasicSpec, withGatewayAPIclass, withFinalizer, withInitialConditions),
+			httpRoute(t, ing(withBasicSpec, withGatewayAPIclass), httpRouteReady),
+		}, servicesAndEndpoints...),
+		WantErr: true,
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{Object: ing(
+			withBasicSpec,
+			withGatewayAPIClass,
+			withFinalizer,
+			func(i *v1alpha1.Ingress) {
 				i.Status.InitializeConditions()
 				i.Status.MarkLoadBalancerNotReady()
 				i.Status.MarkNetworkConfigured()
 				i.Status.MarkIngressNotReady("ReconcileIngressFailed", "Ingress reconciliation failed")
-			}),
-			},
-		},
+			})}},
 		WantEvents: []string{
-			Eventf(corev1.EventTypeWarning, "InternalError", `Gateway "istio-system/istio-gateway" does not have an address in status`),
+			Eventf(corev1.EventTypeWarning, "InternalError", `could not find Gateway "istio-system/istio-gateway": gateway.gateway.networking.k8s.io "istio-gateway" not found`),
 		},
 	}}
 
@@ -2302,6 +2355,18 @@ func makeItReadyOffClusterGateway(i *v1alpha1.Ingress) {
 		}})
 }
 
+func makeItReadyOffClusterGatewayHostname(i *v1alpha1.Ingress) {
+	i.Status.InitializeConditions()
+	i.Status.MarkNetworkConfigured()
+	i.Status.MarkLoadBalancerReady(
+		[]v1alpha1.LoadBalancerIngressStatus{{
+			DomainInternal: publicGatewayHostname,
+		}},
+		[]v1alpha1.LoadBalancerIngressStatus{{
+			IP: privateGatewayAddress,
+		}})
+}
+
 func makeItReady(i *v1alpha1.Ingress) {
 	i.Status.InitializeConditions()
 	i.Status.MarkNetworkConfigured()
@@ -2416,13 +2481,20 @@ func setStatusPrivateAddress(g *gatewayapi.Gateway) {
 	})
 }
 
-func setStatusPublicAddress(g *gatewayapi.Gateway) {
+func setStatusPublicAddressIP(g *gatewayapi.Gateway) {
 	g.Status.Addresses = append(g.Status.Addresses, gatewayapi.GatewayStatusAddress{
 		Type:  ptr.To[gatewayapi.AddressType](gatewayapi.IPAddressType),
 		Value: publicGatewayAddress,
 	})
 }
 
+func setStatusPublicAddressHostname(g *gatewayapi.Gateway) {
+	g.Status.Addresses = append(g.Status.Addresses, gatewayapi.GatewayStatusAddress{
+		Type:  ptr.To[gatewayapi.AddressType](gatewayapi.HostnameAddressType),
+		Value: publicGatewayHostname,
+	})
+}
+
 func tlsListener(hostname, nsName, secretName string) GatewayOption {
 	return func(g *gatewayapi.Gateway) {
 		g.Spec.Listeners = append(g.Spec.Listeners, gatewayapi.Listener{

pkg/reconciler/ingress/lister.go

@@ -127,7 +127,7 @@ func (l *gatewayPodTargetLister) BackendsToProbeTargets(ctx context.Context, bac
 			}
 
 			if len(gw.Status.Addresses) == 0 {
-				return nil, fmt.Errorf("no Addresses available in Status of Gateway %s/%s", gw.Namespace, gw.Name)
+				return nil, fmt.Errorf("no addresses available in status of Gateway %s/%s", gw.Namespace, gw.Name)
 			}
 
 			pt := status.ProbeTarget{

pkg/reconciler/ingress/lister_test.go

@@ -319,7 +319,7 @@ func TestListProbeTargetsNoService(t *testing.T) {
 			},
 		},
 		objects: []runtime.Object{
-			gw(defaultListener, setStatusPublicAddress),
+			gw(defaultListener, setStatusPublicAddressIP),
 		},
 		ing: ing(withBasicSpec, withGatewayAPIClass),
 		want: []status.ProbeTarget{
@@ -337,7 +337,7 @@ func TestListProbeTargetsNoService(t *testing.T) {
 		name: "gateway has tls listener (http enabled)",
 		objects: []runtime.Object{
 			// objects for secret and referenceGrant not needed in this test
-			gw(defaultListener, tlsListener("example.com", "ns", "secretName"), setStatusPublicAddress),
+			gw(defaultListener, tlsListener("example.com", "ns", "secretName"), setStatusPublicAddressIP),
 		},
 		backends: status.Backends{
 			URLs: map[v1alpha1.IngressVisibility]status.URLSet{
@@ -362,7 +362,7 @@ func TestListProbeTargetsNoService(t *testing.T) {
 		name: "gateway has tls listener (https redirected)",
 		objects: []runtime.Object{
 			// objects for secret and referenceGrant not needed in this test
-			gw(defaultListener, tlsListener("example.com", "ns", "secretName"), setStatusPublicAddress),
+			gw(defaultListener, tlsListener("example.com", "ns", "secretName"), setStatusPublicAddressIP),
 		},
 		backends: status.Backends{
 			HTTPOption: v1alpha1.HTTPOptionRedirected,
@@ -399,7 +399,7 @@ func TestListProbeTargetsNoService(t *testing.T) {
 			},
 		},
 		ing:     ing(withBasicSpec, withGatewayAPIClass, withHTTPOption(v1alpha1.HTTPOptionRedirected)),
-		wantErr: fmt.Errorf("no Addresses available in Status of Gateway istio-system/istio-gateway"),
+		wantErr: fmt.Errorf("no addresses available in status of Gateway istio-system/istio-gateway"),
 	}}
 
 	for _, test := range tests {

test/e2e-common.sh

@@ -163,6 +163,8 @@ function gateway_conformance() {
 }
 
 function test_e2e() {
+  # TestGatewayWithNoService edits the ConfigMap, so running in parallel could cause
+  # unexpected problems when more tests are added.
   local parallel_count="1"