From 37d0f99d752e6a5b8b75b5f82b906301a495d37c Mon Sep 17 00:00:00 2001 From: Ryan Fredette Date: Mon, 9 Sep 2024 17:17:15 -0400 Subject: [PATCH] Don't allow gateway DNS records to be created if they clash with existing ingress controllers. --- .../gateway-service-dns/controller.go | 25 ++++++++ .../gateway-service-dns/controller_test.go | 62 +++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/pkg/operator/controller/gateway-service-dns/controller.go b/pkg/operator/controller/gateway-service-dns/controller.go index 3beeca93f1..ca230d4743 100644 --- a/pkg/operator/controller/gateway-service-dns/controller.go +++ b/pkg/operator/controller/gateway-service-dns/controller.go @@ -17,6 +17,7 @@ import ( corev1 "k8s.io/api/core/v1" configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" iov1 "github.com/openshift/api/operatoringress/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -241,8 +242,18 @@ func (r *reconciler) ensureDNSRecordsForGateway(ctx context.Context, gateway *ga Name: service.Name, UID: service.UID, } + ics := operatorv1.IngressControllerList{} + if err := r.cache.List(ctx, &ics); err != nil { + log.Error(err, "failed to list Ingress Controllers") + return []error{err} + } var errs []error for _, domain := range domains { + // Check if the domain matches one from an existing ingress controller. If so, log an error and ignore the invalid domain. + if err := domainClashes(domain, ics); err != nil { + log.Error(fmt.Errorf("error creating DNS record for gateway %s: %w", gateway.Name, err), "ignoring invalid gateway domain") + continue + } name := operatorcontroller.GatewayDNSRecordName(gateway, domain) dnsPolicy := iov1.UnmanagedDNS if dnsrecord.ManageDNSForDomain(domain, infraConfig.Status.PlatformStatus, dnsConfig) { @@ -254,6 +265,20 @@ func (r *reconciler) ensureDNSRecordsForGateway(ctx context.Context, gateway *ga return errs } +// domainClashes checks if domain clashes with any of the wildcard records of the ingress controllers in icList. If +// there is a clash, an error is returned describing which ingress controller already uses the specified domain name. +func domainClashes(domain string, icList operatorv1.IngressControllerList) error { + for _, ic := range icList.Items { + if len(ic.Status.Domain) == 0 { + continue + } + if strings.Compare(domain, "*."+ic.Status.Domain+".") == 0 { + return fmt.Errorf("listener hostname %q conflicts with the domain from ingress controller %s.", domain, ic.Name) + } + } + return nil +} + // deleteStaleDNSRecordsForGateway deletes any DNSRecord CRs that are associated // with the given gateway but specify a DNS name that is not in the given set of // domains. Such DNSRecord CRs may exist if a hostname was modified or deleted diff --git a/pkg/operator/controller/gateway-service-dns/controller_test.go b/pkg/operator/controller/gateway-service-dns/controller_test.go index bac5794b19..a46fe0b5ea 100644 --- a/pkg/operator/controller/gateway-service-dns/controller_test.go +++ b/pkg/operator/controller/gateway-service-dns/controller_test.go @@ -11,6 +11,7 @@ import ( gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" iov1 "github.com/openshift/api/operatoringress/v1" corev1 "k8s.io/api/core/v1" @@ -41,6 +42,17 @@ func Test_Reconcile(t *testing.T) { }, }, } + ic := func(name, domain string) *operatorv1.IngressController { + return &operatorv1.IngressController{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "openshift-ingress-operator", + Name: name, + }, + Status: operatorv1.IngressControllerStatus{ + Domain: domain, + }, + } + } gw := func(name string, listeners ...gatewayapiv1beta1.Listener) *gatewayapiv1beta1.Gateway { return &gatewayapiv1beta1.Gateway{ ObjectMeta: metav1.ObjectMeta{ @@ -127,6 +139,7 @@ func Test_Reconcile(t *testing.T) { infraConfig, gw("example-gateway", l("stage-http", "*.stage.example.com", 80)), svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")), + ic("default", "apps.example.com"), }, reconcileRequest: req("openshift-ingress", "example-gateway"), expectCreate: []client.Object{}, @@ -140,6 +153,7 @@ func Test_Reconcile(t *testing.T) { dnsConfig, gw("example-gateway", l("stage-http", "*.stage.example.com", 80)), svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")), + ic("default", "apps.example.com"), }, reconcileRequest: req("openshift-ingress", "example-gateway"), expectCreate: []client.Object{}, @@ -153,6 +167,7 @@ func Test_Reconcile(t *testing.T) { dnsConfig, infraConfig, gw("example-gateway"), svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")), + ic("default", "apps.example.com"), }, reconcileRequest: req("openshift-ingress", "example-gateway"), expectCreate: []client.Object{}, @@ -170,6 +185,7 @@ func Test_Reconcile(t *testing.T) { l("prod-https", "*.prod.example.com", 443), ), svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")), + ic("default", "apps.example.com"), }, reconcileRequest: req("openshift-ingress", "example-gateway"), expectCreate: []client.Object{ @@ -190,6 +206,7 @@ func Test_Reconcile(t *testing.T) { ), svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("newlb.example.com")), dnsrecord("example-gateway-7bdcfc8f68-wildcard", "*.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "oldlb.example.com"), + ic("default", "apps.example.com"), }, reconcileRequest: req("openshift-ingress", "example-gateway"), expectCreate: []client.Object{}, @@ -208,6 +225,7 @@ func Test_Reconcile(t *testing.T) { ), svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")), dnsrecord("example-gateway-64754456b8-wildcard", "*.old.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "lb.example.com"), + ic("default", "apps.example.com"), }, reconcileRequest: req("openshift-ingress", "example-gateway"), expectCreate: []client.Object{ @@ -224,6 +242,7 @@ func Test_Reconcile(t *testing.T) { dnsConfig, infraConfig, gw("example-gateway", l("stage-http", "*.stage.example.com", 80), l("stage-https", "*.stage.example.com", 443)), svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")), + ic("default", "apps.example.com"), }, reconcileRequest: req("openshift-ingress", "example-gateway"), expectCreate: []client.Object{ @@ -238,6 +257,7 @@ func Test_Reconcile(t *testing.T) { dnsConfig, infraConfig, gw("example-gateway", l("http", "*.foo.com", 80)), svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")), + ic("default", "apps.example.com"), }, reconcileRequest: req("openshift-ingress", "example-gateway"), expectCreate: []client.Object{ @@ -246,12 +266,54 @@ func Test_Reconcile(t *testing.T) { expectUpdate: []client.Object{}, expectDelete: []client.Object{}, }, + { + name: "gateway with two unique host names, one of which clashes with an existing ingress controller", + existingObjects: []runtime.Object{ + dnsConfig, infraConfig, + gw( + "example-gateway", + l("stage-https", "*.stage.apps.example.com", 443), + l("apps-https", "*.apps.example.com", 443), + ), + svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")), + ic("default", "apps.example.com"), + }, + reconcileRequest: req("openshift-ingress", "example-gateway"), + expectCreate: []client.Object{ + dnsrecord("example-gateway-644bf77744-wildcard", "*.stage.apps.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "lb.example.com"), + }, + expectUpdate: []client.Object{}, + expectDelete: []client.Object{}, + }, + { + name: "gateway with two unique host names, neither of which clashes with an existing ingress controller", + existingObjects: []runtime.Object{ + dnsConfig, infraConfig, + gw( + "example-gateway", + l("stage-https", "*.stage.apps.example.com", 443), + // apps.example.com looks like it'll clash with the default ic below, but the ic creates a record + // for *.apps.example.com, which doesn't actually clash with apps.example.com + l("apps-https", "apps.example.com", 443), + ), + svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")), + ic("default", "apps.example.com"), + }, + reconcileRequest: req("openshift-ingress", "example-gateway"), + expectCreate: []client.Object{ + dnsrecord("example-gateway-644bf77744-wildcard", "*.stage.apps.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "lb.example.com"), + dnsrecord("example-gateway-54b5446744-wildcard", "apps.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "lb.example.com"), + }, + expectUpdate: []client.Object{}, + expectDelete: []client.Object{}, + }, } scheme := runtime.NewScheme() iov1.AddToScheme(scheme) corev1.AddToScheme(scheme) gatewayapiv1beta1.AddToScheme(scheme) + operatorv1.AddToScheme(scheme) for _, tc := range tests { t.Run(tc.name, func(t *testing.T) {