Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCPBUGS-31521: Don't allow DNS records for gateway to be created if they clash with any existing ingress controller. #1145

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OCPBUGS-31521: Don't allow DNS records for gateway to be created if they clash with any existing ingress controller. #1145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions pkg/operator/controller/gateway-service-dns/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
Copy link
Contributor

@candita candita Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use %v instead of %w for errors.

continue
}
name := operatorcontroller.GatewayDNSRecordName(gateway, domain)
dnsPolicy := iov1.UnmanagedDNS
if dnsrecord.ManageDNSForDomain(domain, infraConfig.Status.PlatformStatus, dnsConfig) {
Expand All @@ -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
Expand Down
62 changes: 62 additions & 0 deletions pkg/operator/controller/gateway-service-dns/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{},
Expand All @@ -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{},
Expand All @@ -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{},
Expand All @@ -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{
Expand All @@ -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{},
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{},
},
rfredette marked this conversation as resolved.
Show resolved Hide resolved
{
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),
Copy link
Contributor

@candita candita Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should clash though, because the DNSRecord for the listener is created as a wildcard and thus will be prefixed with *. too. This has to be prevented because if the listener's DNSRecord gets deleted then the IC's DNSRecord is too and the component routes begin to fail.

),
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", "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) {
Expand Down
7 changes: 6 additions & 1 deletion pkg/operator/controller/names.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package controller

import (
"fmt"
"strings"

operatorv1 "github.com/openshift/api/operator/v1"

Expand Down Expand Up @@ -287,8 +288,12 @@ func ServiceMeshSubscriptionName() types.NamespacedName {
// is named using the Gateway's name, listener's hashed host name, and the
// suffix "-wildcard".
func GatewayDNSRecordName(gateway *gatewayapiv1beta1.Gateway, host string) types.NamespacedName {
name := fmt.Sprintf("%s-%s", gateway.Name, util.Hash(host))
if strings.HasPrefix(host, "*.") {
name = name + "-wildcard"
}
return types.NamespacedName{
Namespace: gateway.Namespace,
Name: fmt.Sprintf("%s-%s-wildcard", gateway.Name, util.Hash(host)),
Name: name,
Comment on lines +291 to +297
Copy link
Contributor

@candita candita Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't change this, the Listener's DNSRecord will always be a wildcard.

}
}