From 745d2ad280e1f6064c8a1477822559382669cdb5 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Mon, 22 Apr 2024 11:18:47 -0400 Subject: [PATCH 01/15] test cleanup * addressing PR comments from #685 --- test/e2e/gateway_config_test.go | 89 ++++++++------------------------- 1 file changed, 22 insertions(+), 67 deletions(-) diff --git a/test/e2e/gateway_config_test.go b/test/e2e/gateway_config_test.go index e215809ee..351e954b4 100644 --- a/test/e2e/gateway_config_test.go +++ b/test/e2e/gateway_config_test.go @@ -22,56 +22,39 @@ package e2e import ( "context" "os" - "strings" "testing" - "unicode" "gopkg.in/yaml.v2" corev1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/apimachinery/pkg/util/sets" "knative.dev/networking/pkg/apis/networking" "knative.dev/networking/pkg/apis/networking/v1alpha1" "knative.dev/networking/test" "knative.dev/networking/test/conformance/ingress" + . "knative.dev/networking/test/defaultsystem" "knative.dev/pkg/apis" - "knative.dev/pkg/configmap" + "knative.dev/pkg/system" ) -const ( - controlNamespace = "knative-serving" - domain = ".example.com" -) - -func TestNetGatewayAPIConfigNoService(t *testing.T) { - if !(strings.Contains(test.ServingFlags.IngressClass, "gateway-api")) { - t.Skip("Skip this test for non-gateway-api ingress.") - } - +func TestGatewayWithNoService(t *testing.T) { clients := test.Setup(t) ctx := context.Background() var configGateway, original *corev1.ConfigMap - original, err := clients.KubeClient.CoreV1().ConfigMaps(controlNamespace).Get(ctx, "config-gateway", v1.GetOptions{}) + original, err := clients.KubeClient.CoreV1().ConfigMaps(system.Namespace()).Get(ctx, "config-gateway", v1.GetOptions{}) if err != nil { t.Fatalf("failed to get original config-gateway ConfigMap: %v", err) } - pwd, err := os.Getwd() - if err != nil { - t.Fatalf("failed to get working dir") - } - testdata := pwd + "/testdata/" - // set up configmap for ingress if ingress, ok := os.LookupEnv("INGRESS"); ok { switch ingress { case "contour": - configGateway = ConfigMapFromTestFile(t, testdata+"contour-no-service-vis.yaml", "external-gateways", "local-gateways") + configGateway = ConfigMapFromTestFile(t, "testdata/contour-no-service-vis.yaml") case "istio": - configGateway = ConfigMapFromTestFile(t, testdata+"istio-no-service-vis.yaml", "external-gateways", "local-gateways") + configGateway = ConfigMapFromTestFile(t, "testdata/istio-no-service-vis.yaml") case "default": t.Fatalf("value for INGRESS (%s) not supported", ingress) } @@ -79,17 +62,16 @@ func TestNetGatewayAPIConfigNoService(t *testing.T) { configGateway.Name = "config-gateway" } - updated, err := clients.KubeClient.CoreV1().ConfigMaps(controlNamespace).Update(ctx, configGateway, v1.UpdateOptions{}) + updated, err := clients.KubeClient.CoreV1().ConfigMaps(system.Namespace()).Update(ctx, configGateway, v1.UpdateOptions{}) if err != nil { t.Fatalf("failed to update config-gateway ConfigMap: %v", err) } svcName, svcPort, svcCancel := ingress.CreateRuntimeService(ctx, t, clients, networking.ServicePortNameHTTP1) - defer svcCancel() _, client, ingressCancel := ingress.CreateIngressReady(ctx, t, clients, v1alpha1.IngressSpec{ Rules: []v1alpha1.IngressRule{{ - Hosts: []string{svcName + domain}, + Hosts: []string{svcName + test.NetworkingFlags.ServiceDomain}, HTTP: &v1alpha1.HTTPIngressRuleValue{ Paths: []v1alpha1.HTTPIngressPath{{ Splits: []v1alpha1.IngressBackendSplit{{ @@ -104,24 +86,29 @@ func TestNetGatewayAPIConfigNoService(t *testing.T) { Visibility: v1alpha1.IngressVisibilityExternalIP, }}, }) - defer ingressCancel() - url := apis.HTTP(svcName + domain) + test.EnsureCleanup(t, func() { + // restore the old configmap + updated.Data = original.Data + _, err = clients.KubeClient.CoreV1().ConfigMaps(system.Namespace()).Update(ctx, updated, v1.UpdateOptions{}) + if err != nil { + t.Fatalf("failed to restore config-gateway ConfigMap: %v", err) + } + + svcCancel() + ingressCancel() + }) + + url := apis.HTTP(svcName + test.NetworkingFlags.ServiceDomain) // Verify the new service is accessible via the ingress. ingress.RuntimeRequest(ctx, t, client, url.URL().String()) - // restore the old configmap - updated.Data = original.Data - _, err = clients.KubeClient.CoreV1().ConfigMaps(controlNamespace).Update(ctx, updated, v1.UpdateOptions{}) - if err != nil { - t.Fatalf("failed to restore config-gateway ConfigMap: %v", err) - } } // ConfigMapFromTestFile creates a corev1.ConfigMap resources from the config // file read from the filepath -func ConfigMapFromTestFile(t testing.TB, name string, allowed ...string) *corev1.ConfigMap { +func ConfigMapFromTestFile(t testing.TB, name string) *corev1.ConfigMap { t.Helper() b, err := os.ReadFile(name) @@ -131,41 +118,9 @@ func ConfigMapFromTestFile(t testing.TB, name string, allowed ...string) *corev1 var orig corev1.ConfigMap - // Use sigs.k8s.io/yaml since it reads json struct - // tags so things unmarshal properly. if err := yaml.Unmarshal(b, &orig); err != nil { t.Fatal("yaml.Unmarshal() =", err) } - if len(orig.Data) != len(allowed) { - // See here for why we only check in empty ConfigMaps: - // https://github.com/knative/serving/issues/2668 - t.Errorf("Data = %v, wanted %v", orig.Data, allowed) - } - allow := sets.NewString(allowed...) - for key := range orig.Data { - if !allow.Has(key) { - t.Errorf("Encountered key %q in %q that wasn't on the allowed list", key, name) - } - } - // With the length and membership checks, we know that the keyspace matches. - - exampleBody, hasExampleBody := orig.Data[configmap.ExampleKey] - // Check that exampleBody does not have lines that end in a trailing space. - for i, line := range strings.Split(exampleBody, "\n") { - if strings.TrimRightFunc(line, unicode.IsSpace) != line { - t.Errorf("line %d of %q example contains trailing spaces", i, name) - } - } - - // Check that the hashed exampleBody matches the assigned annotation, if present. - gotChecksum, hasExampleChecksumAnnotation := orig.Annotations[configmap.ExampleChecksumAnnotation] - if hasExampleBody && hasExampleChecksumAnnotation { - wantChecksum := configmap.Checksum(exampleBody) - if gotChecksum != wantChecksum { - t.Errorf("example checksum annotation = %s, want %s (you may need to re-run ./hack/update-codegen.sh)", gotChecksum, wantChecksum) - } - } - return &orig } From 35d046a328827cd8e8900e96053ea2acdaa36791 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Mon, 22 Apr 2024 11:23:44 -0400 Subject: [PATCH 02/15] fix error message style --- pkg/reconciler/ingress/lister.go | 2 +- pkg/reconciler/ingress/lister_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/ingress/lister.go b/pkg/reconciler/ingress/lister.go index 8e81bf889..019ad8066 100644 --- a/pkg/reconciler/ingress/lister.go +++ b/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{ diff --git a/pkg/reconciler/ingress/lister_test.go b/pkg/reconciler/ingress/lister_test.go index 5412fa879..652be7724 100644 --- a/pkg/reconciler/ingress/lister_test.go +++ b/pkg/reconciler/ingress/lister_test.go @@ -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 { From 54eeab5d77023058050ab2b91997496636df87a6 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Mon, 22 Apr 2024 11:24:36 -0400 Subject: [PATCH 03/15] cleanup old comment --- pkg/reconciler/ingress/ingress_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/reconciler/ingress/ingress_test.go b/pkg/reconciler/ingress/ingress_test.go index 6c4c00550..f0800b584 100644 --- a/pkg/reconciler/ingress/ingress_test.go +++ b/pkg/reconciler/ingress/ingress_test.go @@ -2258,7 +2258,6 @@ 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) { i.Status.InitializeConditions() i.Status.MarkLoadBalancerNotReady() From 898b7dbb81317997ed28609e4e90246ede8e72c1 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Mon, 22 Apr 2024 11:32:12 -0400 Subject: [PATCH 04/15] import defaultsystem correctly --- test/e2e/gateway_config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/gateway_config_test.go b/test/e2e/gateway_config_test.go index 351e954b4..e5f926fe1 100644 --- a/test/e2e/gateway_config_test.go +++ b/test/e2e/gateway_config_test.go @@ -32,7 +32,7 @@ import ( "knative.dev/networking/pkg/apis/networking/v1alpha1" "knative.dev/networking/test" "knative.dev/networking/test/conformance/ingress" - . "knative.dev/networking/test/defaultsystem" + _ "knative.dev/networking/test/defaultsystem" "knative.dev/pkg/apis" "knative.dev/pkg/system" ) From cdc08e10fabe8fea2a3dd2add516e6ed5f0535f9 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Mon, 22 Apr 2024 11:55:06 -0400 Subject: [PATCH 05/15] refactor: condense lb lookup code --- pkg/reconciler/ingress/ingress.go | 106 +++++++++++++----------------- 1 file changed, 46 insertions(+), 60 deletions(-) diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index c2099e3d2..40927614f 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -148,46 +148,12 @@ 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() } @@ -195,36 +161,56 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress return nil } -// determineLoadBalancerIngressStatus will return the address for the Gateway. -// If a service is provided, it will return the address of the service. +// lookUpLoadBalancers will return a map of visibilites to +// LoadBalancerIngressStatuses for the provided 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) determineLoadBalancerIngressStatus(gwc config.Gateway) ([]v1alpha1.LoadBalancerIngressStatus, error) { - if gwc.Service != nil { - return []v1alpha1.LoadBalancerIngressStatus{ - {DomainInternal: network.GetServiceHostname(gwc.Service.Name, gwc.Service.Namespace)}, - }, nil - } - gw, err := c.gatewayLister.Gateways(gwc.Namespace).Get(gwc.Name) - if err != nil { - return nil, err +func (c *Reconciler) lookUpLoadBalancers(ing *v1alpha1.Ingress, gpc *config.GatewayPlugin) (map[v1alpha1.IngressVisibility][]v1alpha1.LoadBalancerIngressStatus, error) { + gateways := map[v1alpha1.IngressVisibility][]config.Gateway{ + v1alpha1.IngressVisibilityExternalIP: gpc.ExternalGateways, + v1alpha1.IngressVisibilityClusterLocal: gpc.LocalGateways, } + ips := map[v1alpha1.IngressVisibility][]v1alpha1.LoadBalancerIngressStatus{} + for vis, gatewayConfigs := range gateways { + // TODO: when the config is updated to support label selectors, this + // code must change to find out which Gateway is appropriate for the + // given Ingress + gwc := gatewayConfigs[0] + if gwc.Service != nil { + ips[vis] = []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", + gw.Namespace, + gw.Name, + ), + ) + return nil, fmt.Errorf("Gateway %s does not exist: %w", gwc.Name, err) //nolint:stylecheck + } + ing.Status.MarkLoadBalancerNotReady() + return nil, err + } - var lbis v1alpha1.LoadBalancerIngressStatus - - 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} + if len(gw.Status.Addresses) > 0 { + switch *gw.Status.Addresses[0].Type { + case gatewayapi.IPAddressType: + ips[vis] = []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... + ips[vis] = []v1alpha1.LoadBalancerIngressStatus{{DomainInternal: gw.Status.Addresses[0].Value}} + } + } } - - return []v1alpha1.LoadBalancerIngressStatus{lbis}, nil } - return nil, fmt.Errorf("Gateway %q does not have an address in status", gwc.NamespacedName) //nolint:stylecheck - + return ips, nil } // isHTTPRouteReady will check the status conditions of the ingress and return true if From 1567e3cf5d76db08ad91ba030e064d4fe20a8ad5 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Mon, 22 Apr 2024 12:29:21 -0400 Subject: [PATCH 06/15] fix logic * earlier refactor messed up how things errored --- pkg/reconciler/ingress/ingress.go | 5 +++-- pkg/reconciler/ingress/ingress_test.go | 18 +++++++++++------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 40927614f..27e5e6cc5 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -151,6 +151,7 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress lbs, err := c.lookUpLoadBalancers(ing, pluginConfig) if err != nil { + ing.Status.MarkLoadBalancerNotReady() return err } ing.Status.MarkLoadBalancerReady(lbs[v1alpha1.IngressVisibilityExternalIP], lbs[v1alpha1.IngressVisibilityClusterLocal]) @@ -192,9 +193,7 @@ func (c *Reconciler) lookUpLoadBalancers(ing *v1alpha1.Ingress, gpc *config.Gate gw.Name, ), ) - return nil, fmt.Errorf("Gateway %s does not exist: %w", gwc.Name, err) //nolint:stylecheck } - ing.Status.MarkLoadBalancerNotReady() return nil, err } @@ -206,6 +205,8 @@ func (c *Reconciler) lookUpLoadBalancers(ing *v1alpha1.Ingress, gpc *config.Gate // Should this actually be under Domain? It seems like the rest of the code expects DomainInternal though... ips[vis] = []v1alpha1.LoadBalancerIngressStatus{{DomainInternal: gw.Status.Addresses[0].Value}} } + } else { + return nil, fmt.Errorf("Gateway %s/%s does not have an address in status", gwc.Namespace, gwc.Name) //nolint:stylecheck } } } diff --git a/pkg/reconciler/ingress/ingress_test.go b/pkg/reconciler/ingress/ingress_test.go index f0800b584..e03117590 100644 --- a/pkg/reconciler/ingress/ingress_test.go +++ b/pkg/reconciler/ingress/ingress_test.go @@ -2258,13 +2258,17 @@ func TestReconcileProbingOffClusterGateway(t *testing.T) { }, 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") - }), - }, + {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`), From 2f9819b724299f5af51ccbabe901d64d69cb32f7 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Mon, 22 Apr 2024 12:50:16 -0400 Subject: [PATCH 07/15] improve test coverage --- pkg/reconciler/ingress/ingress.go | 2 +- pkg/reconciler/ingress/ingress_test.go | 78 +++++++++++++++++++++++++- pkg/reconciler/ingress/lister_test.go | 6 +- 3 files changed, 79 insertions(+), 7 deletions(-) diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 27e5e6cc5..1ad571ea9 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -206,7 +206,7 @@ func (c *Reconciler) lookUpLoadBalancers(ing *v1alpha1.Ingress, gpc *config.Gate ips[vis] = []v1alpha1.LoadBalancerIngressStatus{{DomainInternal: gw.Status.Addresses[0].Value}} } } else { - return nil, fmt.Errorf("Gateway %s/%s does not have an address in status", gwc.Namespace, gwc.Name) //nolint:stylecheck + return nil, fmt.Errorf("no address found in status of Gateway %s/%s", gwc.Namespace, gwc.Name) } } } diff --git a/pkg/reconciler/ingress/ingress_test.go b/pkg/reconciler/ingress/ingress_test.go index e03117590..74d47fd73 100644 --- a/pkg/reconciler/ingress/ingress_test.go +++ b/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", @@ -2271,7 +2292,39 @@ func TestReconcileProbingOffClusterGateway(t *testing.T) { )}, }, WantEvents: []string{ - Eventf(corev1.EventTypeWarning, "InternalError", `Gateway "istio-system/istio-gateway" does not have an address in status`), + 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.gateway.networking.k8s.io "istio-gateway" not found`), }, }} @@ -2305,6 +2358,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() @@ -2419,13 +2484,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{ diff --git a/pkg/reconciler/ingress/lister_test.go b/pkg/reconciler/ingress/lister_test.go index 652be7724..e44e0e69a 100644 --- a/pkg/reconciler/ingress/lister_test.go +++ b/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, From 3d387b5f1aa872b7cef5d7c22f1dab79a054ff3e Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Mon, 22 Apr 2024 13:48:26 -0400 Subject: [PATCH 08/15] explain e2e test running serially --- test/e2e-common.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/e2e-common.sh b/test/e2e-common.sh index 6c443c0d9..b6f783003 100755 --- a/test/e2e-common.sh +++ b/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" From 593e64f3fc9d95515759970885f8b936c24ea7e4 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Mon, 22 Apr 2024 16:21:37 -0400 Subject: [PATCH 09/15] refactor: split lb status code up --- pkg/reconciler/ingress/ingress.go | 43 +++++++++++++++++++------------ 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 1ad571ea9..1ebf3c6c7 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -167,20 +167,32 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress // 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) lookUpLoadBalancers(ing *v1alpha1.Ingress, gpc *config.GatewayPlugin) (map[v1alpha1.IngressVisibility][]v1alpha1.LoadBalancerIngressStatus, error) { - gateways := map[v1alpha1.IngressVisibility][]config.Gateway{ - v1alpha1.IngressVisibilityExternalIP: gpc.ExternalGateways, - v1alpha1.IngressVisibilityClusterLocal: gpc.LocalGateways, + externalStatuses, err := c.collectLBIngressStatus(ing, gpc.ExternalGateways) + if err != nil { + return nil, err } - ips := map[v1alpha1.IngressVisibility][]v1alpha1.LoadBalancerIngressStatus{} - for vis, gatewayConfigs := range gateways { - // TODO: when the config is updated to support label selectors, this - // code must change to find out which Gateway is appropriate for the - // given Ingress - gwc := gatewayConfigs[0] + + internalStatuses, err := c.collectLBIngressStatus(ing, gpc.LocalGateways) + if err != nil { + return nil, err + } + + return map[v1alpha1.IngressVisibility][]v1alpha1.LoadBalancerIngressStatus{ + v1alpha1.IngressVisibilityExternalIP: externalStatuses, + v1alpha1.IngressVisibilityClusterLocal: internalStatuses, + }, nil +} + +// TODO: when the config is updated to support label selectors, this +// code must change to find out which Gateway is appropriate for the +// given Ingress +func (c *Reconciler) collectLBIngressStatus(ing *v1alpha1.Ingress, gatewayConfigs []config.Gateway) ([]v1alpha1.LoadBalancerIngressStatus, error) { + statuses := []v1alpha1.LoadBalancerIngressStatus{} + for _, gwc := range gatewayConfigs { if gwc.Service != nil { - ips[vis] = []v1alpha1.LoadBalancerIngressStatus{ - {DomainInternal: network.GetServiceHostname(gwc.Service.Name, gwc.Service.Namespace)}, - } + 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 { @@ -200,18 +212,17 @@ func (c *Reconciler) lookUpLoadBalancers(ing *v1alpha1.Ingress, gpc *config.Gate if len(gw.Status.Addresses) > 0 { switch *gw.Status.Addresses[0].Type { case gatewayapi.IPAddressType: - ips[vis] = []v1alpha1.LoadBalancerIngressStatus{{IP: gw.Status.Addresses[0].Value}} + 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... - ips[vis] = []v1alpha1.LoadBalancerIngressStatus{{DomainInternal: gw.Status.Addresses[0].Value}} + 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 ips, nil + return statuses, nil } // isHTTPRouteReady will check the status conditions of the ingress and return true if From e65e7739bfe41d028dcfa4717c75163085fa0788 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Mon, 22 Apr 2024 16:29:06 -0400 Subject: [PATCH 10/15] fix seg violation in test --- pkg/reconciler/ingress/ingress.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 1ebf3c6c7..99ce896fe 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -188,6 +188,9 @@ func (c *Reconciler) lookUpLoadBalancers(ing *v1alpha1.Ingress, gpc *config.Gate // given Ingress func (c *Reconciler) collectLBIngressStatus(ing *v1alpha1.Ingress, gatewayConfigs []config.Gateway) ([]v1alpha1.LoadBalancerIngressStatus, error) { statuses := []v1alpha1.LoadBalancerIngressStatus{} + if len(gatewayConfigs) == 0 { + return nil, fmt.Errorf("no Gateways provided") + } for _, gwc := range gatewayConfigs { if gwc.Service != nil { statuses = append(statuses, v1alpha1.LoadBalancerIngressStatus{ @@ -201,8 +204,8 @@ func (c *Reconciler) collectLBIngressStatus(ing *v1alpha1.Ingress, gatewayConfig "GatewayDoesNotExist", fmt.Sprintf( "could not find Gateway %s/%s", - gw.Namespace, - gw.Name, + gwc.Namespace, + gwc.Name, ), ) } From f0b4ff5e3e257bfb6acf363ef3cac911246f392e Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Mon, 22 Apr 2024 16:36:05 -0400 Subject: [PATCH 11/15] refactor: assume single gateway * code in config only allows a single Gateway, and if none are provided, it will use the defaults. --- pkg/reconciler/ingress/ingress.go | 76 +++++++++++++++---------------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 99ce896fe..df418b016 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -163,9 +163,7 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress } // lookUpLoadBalancers will return a map of visibilites to -// LoadBalancerIngressStatuses for the provided 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. +// 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.ExternalGateways) if err != nil { @@ -183,48 +181,50 @@ func (c *Reconciler) lookUpLoadBalancers(ing *v1alpha1.Ingress, gpc *config.Gate }, nil } -// TODO: when the config is updated to support label selectors, this -// code must change to find out which Gateway is appropriate for the -// given Ingress +// 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, gatewayConfigs []config.Gateway) ([]v1alpha1.LoadBalancerIngressStatus, error) { statuses := []v1alpha1.LoadBalancerIngressStatus{} - if len(gatewayConfigs) == 0 { - return nil, fmt.Errorf("no Gateways provided") - } - for _, gwc := range gatewayConfigs { - 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, err + + // 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 + gwc := gatewayConfigs[0] + 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, err + } - 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) + 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 statuses, nil } From aea6526a2ea42144a67c891bebdb95476079a154 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Mon, 22 Apr 2024 17:34:51 -0400 Subject: [PATCH 12/15] address nits --- pkg/reconciler/ingress/ingress.go | 9 ++++----- pkg/reconciler/ingress/ingress_test.go | 25 +++++++++++-------------- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index df418b016..aeaab80a9 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -165,12 +165,12 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress // 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.ExternalGateways) + externalStatuses, err := c.collectLBIngressStatus(ing, gpc.ExternalGateway()) if err != nil { return nil, err } - internalStatuses, err := c.collectLBIngressStatus(ing, gpc.LocalGateways) + internalStatuses, err := c.collectLBIngressStatus(ing, gpc.LocalGateway()) if err != nil { return nil, err } @@ -185,13 +185,12 @@ func (c *Reconciler) lookUpLoadBalancers(ing *v1alpha1.Ingress, gpc *config.Gate // 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, gatewayConfigs []config.Gateway) ([]v1alpha1.LoadBalancerIngressStatus, error) { +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 - gwc := gatewayConfigs[0] if gwc.Service != nil { statuses = append(statuses, v1alpha1.LoadBalancerIngressStatus{ DomainInternal: network.GetServiceHostname(gwc.Service.Name, gwc.Service.Namespace), @@ -209,7 +208,7 @@ func (c *Reconciler) collectLBIngressStatus(ing *v1alpha1.Ingress, gatewayConfig ), ) } - return nil, err + return nil, fmt.Errorf("could not find Gateway \"%s/%s\": %w", gwc.Namespace, gwc.Name, err) } if len(gw.Status.Addresses) > 0 { diff --git a/pkg/reconciler/ingress/ingress_test.go b/pkg/reconciler/ingress/ingress_test.go index 74d47fd73..217a702eb 100644 --- a/pkg/reconciler/ingress/ingress_test.go +++ b/pkg/reconciler/ingress/ingress_test.go @@ -2310,21 +2310,18 @@ func TestReconcileProbingOffClusterGateway(t *testing.T) { 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") - }, - )}, - }, + 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.gateway.networking.k8s.io "istio-gateway" not found`), + Eventf(corev1.EventTypeWarning, "InternalError", `could not find Gateway "istio-system/istio-gateway": gateway.gateway.networking.k8s.io "istio-gateway" not found`), }, }} From b5228644e775dcf4e6db46aa746009d4e020f452 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Tue, 23 Apr 2024 11:33:36 -0400 Subject: [PATCH 13/15] refactor: don't error if gateway not found * don't want to trigger re-reconciling if it doesn't exist --- pkg/reconciler/ingress/ingress.go | 38 +++++++++++++++++--------- pkg/reconciler/ingress/ingress_test.go | 5 ---- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index aeaab80a9..87f3b7e8f 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -42,6 +42,14 @@ const ( notReconciledMessage = "Ingress reconciliation failed" ) +type GatewayNotFoundError struct { + Err error +} + +func (gnfe *GatewayNotFoundError) Error() string { + return gnfe.Err.Error() +} + // Reconciler implements controller.Reconciler for Route resources. type Reconciler struct { statusManager status.Manager @@ -148,13 +156,19 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress // TODO: check Gateway readiness before reporting Ingress ready if routesReady { - lbs, err := c.lookUpLoadBalancers(ing, pluginConfig) - + externalLBs, internalLBs, err := c.lookUpLoadBalancers(ing, pluginConfig) if err != nil { ing.Status.MarkLoadBalancerNotReady() - return err + if _, ok := err.(*GatewayNotFoundError); ok { + // if we can't find a Gateway, we mark it as failed, and + // return no error, since there is no point in retrying + return nil + } else { + return err + } } - ing.Status.MarkLoadBalancerReady(lbs[v1alpha1.IngressVisibilityExternalIP], lbs[v1alpha1.IngressVisibilityClusterLocal]) + + ing.Status.MarkLoadBalancerReady(externalLBs, internalLBs) } else { ing.Status.MarkLoadBalancerNotReady() } @@ -164,26 +178,23 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress // 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) { +func (c *Reconciler) lookUpLoadBalancers(ing *v1alpha1.Ingress, gpc *config.GatewayPlugin) ([]v1alpha1.LoadBalancerIngressStatus, []v1alpha1.LoadBalancerIngressStatus, error) { externalStatuses, err := c.collectLBIngressStatus(ing, gpc.ExternalGateway()) if err != nil { - return nil, err + return nil, nil, err } internalStatuses, err := c.collectLBIngressStatus(ing, gpc.LocalGateway()) if err != nil { - return nil, err + return nil, nil, err } - return map[v1alpha1.IngressVisibility][]v1alpha1.LoadBalancerIngressStatus{ - v1alpha1.IngressVisibilityExternalIP: externalStatuses, - v1alpha1.IngressVisibilityClusterLocal: internalStatuses, - }, nil + return externalStatuses, internalStatuses, nil } // 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 +// 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{} @@ -207,8 +218,9 @@ func (c *Reconciler) collectLBIngressStatus(ing *v1alpha1.Ingress, gwc config.Ga gwc.Name, ), ) + return nil, &GatewayNotFoundError{Err: err} } - return nil, fmt.Errorf("could not find Gateway \"%s/%s\": %w", gwc.Namespace, gwc.Name, err) + return nil, fmt.Errorf("failed to get Gateway \"%s/%s\": %w", gwc.Namespace, gwc.Name, err) } if len(gw.Status.Addresses) > 0 { diff --git a/pkg/reconciler/ingress/ingress_test.go b/pkg/reconciler/ingress/ingress_test.go index 217a702eb..a9a2ec9e1 100644 --- a/pkg/reconciler/ingress/ingress_test.go +++ b/pkg/reconciler/ingress/ingress_test.go @@ -2309,7 +2309,6 @@ func TestReconcileProbingOffClusterGateway(t *testing.T) { ing(withBasicSpec, withGatewayAPIclass, withFinalizer, withInitialConditions), httpRoute(t, ing(withBasicSpec, withGatewayAPIclass), httpRouteReady), }, servicesAndEndpoints...), - WantErr: true, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{Object: ing( withBasicSpec, withGatewayAPIClass, @@ -2318,11 +2317,7 @@ func TestReconcileProbingOffClusterGateway(t *testing.T) { i.Status.InitializeConditions() i.Status.MarkLoadBalancerNotReady() i.Status.MarkNetworkConfigured() - i.Status.MarkIngressNotReady("ReconcileIngressFailed", "Ingress reconciliation failed") })}}, - WantEvents: []string{ - Eventf(corev1.EventTypeWarning, "InternalError", `could not find Gateway "istio-system/istio-gateway": gateway.gateway.networking.k8s.io "istio-gateway" not found`), - }, }} table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler { From f690d35b58a41092b7f785d5c600089d1eef88fb Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Tue, 23 Apr 2024 12:05:58 -0400 Subject: [PATCH 14/15] mark ingress as failed when cant find gateway * marking as not ready was overwriting the failed status --- pkg/reconciler/ingress/ingress.go | 2 +- pkg/reconciler/ingress/ingress_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 87f3b7e8f..35dc8fdd0 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -158,12 +158,12 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress if routesReady { externalLBs, internalLBs, err := c.lookUpLoadBalancers(ing, pluginConfig) if err != nil { - ing.Status.MarkLoadBalancerNotReady() if _, ok := err.(*GatewayNotFoundError); ok { // if we can't find a Gateway, we mark it as failed, and // return no error, since there is no point in retrying return nil } else { + ing.Status.MarkLoadBalancerNotReady() return err } } diff --git a/pkg/reconciler/ingress/ingress_test.go b/pkg/reconciler/ingress/ingress_test.go index a9a2ec9e1..643257d32 100644 --- a/pkg/reconciler/ingress/ingress_test.go +++ b/pkg/reconciler/ingress/ingress_test.go @@ -2315,7 +2315,7 @@ func TestReconcileProbingOffClusterGateway(t *testing.T) { withFinalizer, func(i *v1alpha1.Ingress) { i.Status.InitializeConditions() - i.Status.MarkLoadBalancerNotReady() + i.Status.MarkLoadBalancerFailed("GatewayDoesNotExist", "could not find Gateway istio-system/istio-gateway") i.Status.MarkNetworkConfigured() })}}, }} From 63ddc0e7ac7728efdc70e50dad992bd46b32f635 Mon Sep 17 00:00:00 2001 From: Clay Kauzlaric Date: Tue, 23 Apr 2024 12:18:08 -0400 Subject: [PATCH 15/15] fix lint errors --- pkg/reconciler/ingress/ingress.go | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 35dc8fdd0..351f30f7f 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -38,16 +38,18 @@ import ( ) const ( - notReconciledReason = "ReconcileIngressFailed" - notReconciledMessage = "Ingress reconciliation failed" + notReconciledReason = "ReconcileIngressFailed" + notReconciledMessage = "Ingress reconciliation failed" + gatewayNotFoundMessage = "could not find Gateway" ) type GatewayNotFoundError struct { - Err error + NamespacedGatewayName string + Err error } func (gnfe *GatewayNotFoundError) Error() string { - return gnfe.Err.Error() + return fmt.Sprintf("%s %s: %s", gatewayNotFoundMessage, gnfe.NamespacedGatewayName, gnfe.Err.Error()) } // Reconciler implements controller.Reconciler for Route resources. @@ -158,14 +160,14 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress if routesReady { externalLBs, internalLBs, err := c.lookUpLoadBalancers(ing, pluginConfig) if err != nil { - if _, ok := err.(*GatewayNotFoundError); ok { + // TODO: use errors.Is instead + if _, ok := err.(*GatewayNotFoundError); ok { //nolint // if we can't find a Gateway, we mark it as failed, and // return no error, since there is no point in retrying return nil - } else { - ing.Status.MarkLoadBalancerNotReady() - return err } + ing.Status.MarkLoadBalancerNotReady() + return err } ing.Status.MarkLoadBalancerReady(externalLBs, internalLBs) @@ -218,7 +220,10 @@ func (c *Reconciler) collectLBIngressStatus(ing *v1alpha1.Ingress, gwc config.Ga gwc.Name, ), ) - return nil, &GatewayNotFoundError{Err: err} + return nil, &GatewayNotFoundError{ + NamespacedGatewayName: gwc.Namespace + "/" + gwc.Name, + Err: err, + } } return nil, fmt.Errorf("failed to get Gateway \"%s/%s\": %w", gwc.Namespace, gwc.Name, err) }