diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index c2099e3d2..351f30f7f 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -38,10 +38,20 @@ import ( ) const ( - notReconciledReason = "ReconcileIngressFailed" - notReconciledMessage = "Ingress reconciliation failed" + notReconciledReason = "ReconcileIngressFailed" + notReconciledMessage = "Ingress reconciliation failed" + gatewayNotFoundMessage = "could not find Gateway" ) +type GatewayNotFoundError struct { + NamespacedGatewayName string + Err error +} + +func (gnfe *GatewayNotFoundError) Error() string { + return fmt.Sprintf("%s %s: %s", gatewayNotFoundMessage, gnfe.NamespacedGatewayName, gnfe.Err.Error()) +} + // Reconciler implements controller.Reconciler for Route resources. type Reconciler struct { statusManager status.Manager @@ -148,46 +158,19 @@ 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 - - externalGateway := pluginConfig.ExternalGateway() - localGateway := pluginConfig.LocalGateway() - - publicLbs, err = c.determineLoadBalancerIngressStatus(externalGateway) + externalLBs, internalLBs, err := c.lookUpLoadBalancers(ing, pluginConfig) 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 + // 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 } ing.Status.MarkLoadBalancerNotReady() return err } - ing.Status.MarkLoadBalancerReady(publicLbs, privateLbs) + ing.Status.MarkLoadBalancerReady(externalLBs, internalLBs) } else { ing.Status.MarkLoadBalancerNotReady() } @@ -195,36 +178,70 @@ 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. -// 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) ([]v1alpha1.LoadBalancerIngressStatus, []v1alpha1.LoadBalancerIngressStatus, error) { + externalStatuses, err := c.collectLBIngressStatus(ing, gpc.ExternalGateway()) + if err != nil { + return nil, 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 + return nil, nil, err } - var lbis v1alpha1.LoadBalancerIngressStatus + 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 +// address in the Gateway status. +func (c *Reconciler) collectLBIngressStatus(ing *v1alpha1.Ingress, gwc config.Gateway) ([]v1alpha1.LoadBalancerIngressStatus, error) { + statuses := []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} + // 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, &GatewayNotFoundError{ + NamespacedGatewayName: gwc.Namespace + "/" + gwc.Name, + Err: err, + } + } + return nil, fmt.Errorf("failed to get 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 diff --git a/pkg/reconciler/ingress/ingress_test.go b/pkg/reconciler/ingress/ingress_test.go index 6c4c00550..643257d32 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", @@ -2258,18 +2279,45 @@ 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() - 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`), + 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...), + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{Object: ing( + withBasicSpec, + withGatewayAPIClass, + withFinalizer, + func(i *v1alpha1.Ingress) { + i.Status.InitializeConditions() + i.Status.MarkLoadBalancerFailed("GatewayDoesNotExist", "could not find Gateway istio-system/istio-gateway") + i.Status.MarkNetworkConfigured() + })}}, }} table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler { @@ -2302,6 +2350,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 +2476,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.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..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, @@ -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 { 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" diff --git a/test/e2e/gateway_config_test.go b/test/e2e/gateway_config_test.go index e215809ee..e5f926fe1 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 }