Skip to content

Commit

Permalink
gateway-service-dns: Set DNS policy appropriately
Browse files Browse the repository at this point in the history 
When creating a DNSRecord CR for a gateway listener, set the DNS management
policy to "Unmanaged" if the DNSRecord's domain is outside the cluster's
base domain.

Before this commit, the operator would attempt to create DNS records for
gateway listeners even if they had domains outside of the cluster's base
domain zone, resulting in "failed to publish DNS record to zone" errors.

This commit fixes OCPBUGS-10875.

https://issues.redhat.com/browse/OCPBUGS-10875

* pkg/operator/controller/gateway-service-dns/controller.go (Reconcile):
Get the dns and infrastructure config objects, and pass them to
ensureDNSRecordsForGateway.
(ensureDNSRecordsForGateway): Add infraConfig and dnsConfig parameters.
Use them and the ManageDNSForDomain function to determine the appropriate
policy (managed or unmanaged) for each DNS record, and pass that policy to
EnsureDNSRecord.
* pkg/resources/dnsrecord/dns.go (EnsureDNSRecord): Add a dnsPolicy
parameter, and pass it to desiredDNSRecord.
(ManageDNSForDomain): Ignore trailing dots when comparing the given domain
and the cluster's base domain.
* pkg/operator/controller/gateway-service-dns/controller_test.go
(Test_Reconcile): Add expectError to the test inputs, and assert that the
expected error is returned if expectError is not empty.  Add dnsConfig and
infraConfig to existingObjects in test cases.  Add test cases to verify
that the reconciler returns the expected error if the cluster DNS config or
cluster infrastructure config is not found.  Add a test case to verify that
the DNS management policy is set to "Unmanaged" if the DNS name doesn't
belong to the cluster's base domain.
  • Loading branch information
@Miciah
Miciah committed Jun 29, 2023
1 parent bcfe0f6 commit 26492db
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 18 deletions.
26 changes: 23 additions & 3 deletions pkg/operator/controller/gateway-service-dns/controller.go
View file
Expand Up @@ -2,6 +2,7 @@ package gateway_service_dns

import (
"context"
"fmt"
"reflect"
"strings"

Expand All @@ -15,6 +16,7 @@ import (

corev1 "k8s.io/api/core/v1"

configv1 "github.com/openshift/api/config/v1"
iov1 "github.com/openshift/api/operatoringress/v1"

apierrors "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -184,9 +186,23 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
return reconcile.Result{}, err
}

dnsConfig := &configv1.DNS{}
if err := r.client.Get(ctx, types.NamespacedName{Name: "cluster"}, dnsConfig); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to get dns 'cluster': %v", err)
}

infraConfig := &configv1.Infrastructure{}
if err := r.client.Get(ctx, types.NamespacedName{Name: "cluster"}, infraConfig); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to get infrastructure 'cluster': %v", err)
}
if infraConfig.Status.PlatformStatus == nil {
log.Info("infrastructure 'cluster' has nil status.platformStatus; reconciliation will be skipped")
return reconcile.Result{}, nil
}

domains := getGatewayHostnames(&gateway)
var errs []error
errs = append(errs, r.ensureDNSRecordsForGateway(ctx, &gateway, &service, domains.List())...)
errs = append(errs, r.ensureDNSRecordsForGateway(ctx, &gateway, &service, domains.List(), infraConfig, dnsConfig)...)
errs = append(errs, r.deleteStaleDNSRecordsForGateway(ctx, &gateway, &service, domains)...)
return reconcile.Result{}, utilerrors.NewAggregate(errs)
}
Expand All @@ -212,7 +228,7 @@ func getGatewayHostnames(gateway *gatewayapiv1beta1.Gateway) sets.String {
// ensureDNSRecordsForGateway ensures that a DNSRecord CR exists, associated
// with the given gateway and service, for each of the given domains. It
// returns a list of any errors that result from ensuring those DNSRecord CRs.
func (r *reconciler) ensureDNSRecordsForGateway(ctx context.Context, gateway *gatewayapiv1beta1.Gateway, service *corev1.Service, domains []string) []error {
func (r *reconciler) ensureDNSRecordsForGateway(ctx context.Context, gateway *gatewayapiv1beta1.Gateway, service *corev1.Service, domains []string, infraConfig *configv1.Infrastructure, dnsConfig *configv1.DNS) []error {
labels := map[string]string{
gatewayNameLabelKey: gateway.Name,
}
Expand All @@ -228,7 +244,11 @@ func (r *reconciler) ensureDNSRecordsForGateway(ctx context.Context, gateway *ga
var errs []error
for _, domain := range domains {
name := operatorcontroller.GatewayDNSRecordName(gateway, domain)
_, _, err := dnsrecord.EnsureDNSRecord(r.client, name, labels, ownerRef, domain, service)
dnsPolicy := iov1.UnmanagedDNS
if dnsrecord.ManageDNSForDomain(domain, infraConfig.Status.PlatformStatus, dnsConfig) {
dnsPolicy = iov1.ManagedDNS
}
_, _, err := dnsrecord.EnsureDNSRecord(r.client, name, labels, ownerRef, domain, dnsPolicy, service)
errs = append(errs, err)
}
return errs
Expand Down
91 changes: 79 additions & 12 deletions pkg/operator/controller/gateway-service-dns/controller_test.go
View file
Expand Up @@ -10,6 +10,7 @@ import (

gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"

configv1 "github.com/openshift/api/config/v1"
iov1 "github.com/openshift/api/operatoringress/v1"

corev1 "k8s.io/api/core/v1"
Expand All @@ -26,6 +27,20 @@ import (
)

func Test_Reconcile(t *testing.T) {
dnsConfig := &configv1.DNS{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: configv1.DNSSpec{
BaseDomain: "example.com",
},
}
infraConfig := &configv1.Infrastructure{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Status: configv1.InfrastructureStatus{
PlatformStatus: &configv1.PlatformStatus{
Type: configv1.AWSPlatformType,
},
},
}
gw := func(name string, listeners ...gatewayapiv1beta1.Listener) *gatewayapiv1beta1.Gateway {
return &gatewayapiv1beta1.Gateway{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -73,7 +88,7 @@ func Test_Reconcile(t *testing.T) {
Hostname: hostname,
}
}
dnsrecord := func(name, dnsName string, labels map[string]string, targets ...string) *iov1.DNSRecord {
dnsrecord := func(name, dnsName string, policy iov1.DNSManagementPolicy, labels map[string]string, targets ...string) *iov1.DNSRecord {
return &iov1.DNSRecord{
ObjectMeta: metav1.ObjectMeta{
Labels: labels,
Expand All @@ -85,7 +100,7 @@ func Test_Reconcile(t *testing.T) {
RecordType: iov1.CNAMERecordType,
Targets: targets,
RecordTTL: 30,
DNSManagementPolicy: iov1.ManagedDNS,
DNSManagementPolicy: policy,
},
}
}
Expand All @@ -104,10 +119,38 @@ func Test_Reconcile(t *testing.T) {
expectCreate []client.Object
expectUpdate []client.Object
expectDelete []client.Object
expectError string
}{
{
name: "missing dns config",
existingObjects: []runtime.Object{
infraConfig,
gw("example-gateway", l("stage-http", "*.stage.example.com", 80)),
svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")),
},
reconcileRequest: req("openshift-ingress", "example-gateway"),
expectCreate: []client.Object{},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
expectError: `dnses.config.openshift.io "cluster" not found`,
},
{
name: "missing infrastructure config",
existingObjects: []runtime.Object{
dnsConfig,
gw("example-gateway", l("stage-http", "*.stage.example.com", 80)),
svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")),
},
reconcileRequest: req("openshift-ingress", "example-gateway"),
expectCreate: []client.Object{},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
expectError: `infrastructures.config.openshift.io "cluster" not found`,
},
{
name: "gateway with no listeners",
existingObjects: []runtime.Object{
dnsConfig, infraConfig,
gw("example-gateway"),
svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")),
},
Expand All @@ -119,6 +162,7 @@ func Test_Reconcile(t *testing.T) {
{
name: "gateway with three listeners and two unique host names, no dnsrecords",
existingObjects: []runtime.Object{
dnsConfig, infraConfig,
gw(
"example-gateway",
l("stage-http", "*.stage.example.com", 80),
Expand All @@ -129,58 +173,75 @@ func Test_Reconcile(t *testing.T) {
},
reconcileRequest: req("openshift-ingress", "example-gateway"),
expectCreate: []client.Object{
dnsrecord("example-gateway-76456f8647-wildcard", "*.prod.example.com.", exampleGatewayLabel, "lb.example.com"),
dnsrecord("example-gateway-64754456b8-wildcard", "*.stage.example.com.", exampleGatewayLabel, "lb.example.com"),
dnsrecord("example-gateway-76456f8647-wildcard", "*.prod.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "lb.example.com"),
dnsrecord("example-gateway-64754456b8-wildcard", "*.stage.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "lb.example.com"),
},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
},
{
name: "gateway with two listeners and one dnsrecord with a stale target, hostname already has trailing dot",
existingObjects: []runtime.Object{
dnsConfig, infraConfig,
gw(
"example-gateway",
l("http", "*.example.com", 80),
l("https", "*.example.com", 443),
),
svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("newlb.example.com")),
dnsrecord("example-gateway-7bdcfc8f68-wildcard", "*.example.com.", exampleGatewayLabel, "oldlb.example.com"),
dnsrecord("example-gateway-7bdcfc8f68-wildcard", "*.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "oldlb.example.com"),
},
reconcileRequest: req("openshift-ingress", "example-gateway"),
expectCreate: []client.Object{},
expectUpdate: []client.Object{
dnsrecord("example-gateway-7bdcfc8f68-wildcard", "*.example.com.", exampleGatewayLabel, "newlb.example.com"),
dnsrecord("example-gateway-7bdcfc8f68-wildcard", "*.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "newlb.example.com"),
},
expectDelete: []client.Object{},
},
{
name: "gateway with a stale dnsrecord",
existingObjects: []runtime.Object{
dnsConfig, infraConfig,
gw(
"example-gateway",
l("http", "*.new.example.com", 80),
),
svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")),
dnsrecord("example-gateway-64754456b8-wildcard", "*.old.example.com.", exampleGatewayLabel, "lb.example.com"),
dnsrecord("example-gateway-64754456b8-wildcard", "*.old.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "lb.example.com"),
},
reconcileRequest: req("openshift-ingress", "example-gateway"),
expectCreate: []client.Object{
dnsrecord("example-gateway-68ffc6d64-wildcard", "*.new.example.com.", exampleGatewayLabel, "lb.example.com"),
dnsrecord("example-gateway-68ffc6d64-wildcard", "*.new.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "lb.example.com"),
},
expectUpdate: []client.Object{},
expectDelete: []client.Object{
dnsrecord("example-gateway-64754456b8-wildcard", "*.old.example.com.", exampleGatewayLabel, "lb.example.com"),
dnsrecord("example-gateway-64754456b8-wildcard", "*.old.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "lb.example.com"),
},
},
{
name: "gateway with two listeners and one host name, no dnsrecords, name ends up with trailing dot",
existingObjects: []runtime.Object{
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")),
},
reconcileRequest: req("openshift-ingress", "example-gateway"),
expectCreate: []client.Object{
dnsrecord("example-gateway-64754456b8-wildcard", "*.stage.example.com.", exampleGatewayLabel, "lb.example.com"),
dnsrecord("example-gateway-64754456b8-wildcard", "*.stage.example.com.", iov1.ManagedDNS, exampleGatewayLabel, "lb.example.com"),
},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
},
{
name: "gateway with a listener with an unmanaged domain, no dnsrecords",
existingObjects: []runtime.Object{
dnsConfig, infraConfig,
gw("example-gateway", l("http", "*.foo.com", 80)),
svc("example-gateway", gatewayManagedLabel, exampleGatewayLabel, ingHost("lb.example.com")),
},
reconcileRequest: req("openshift-ingress", "example-gateway"),
expectCreate: []client.Object{
dnsrecord("example-gateway-795d4b47fd-wildcard", "*.foo.com.", iov1.UnmanagedDNS, exampleGatewayLabel, "lb.example.com"),
},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
Expand Down Expand Up @@ -209,8 +270,14 @@ func Test_Reconcile(t *testing.T) {
client: cl,
}
res, err := reconciler.Reconcile(context.Background(), tc.reconcileRequest)
if assert.NoError(t, err) {
assert.Equal(t, reconcile.Result{}, res)
if tc.expectError == "" {
if assert.NoError(t, err) {
assert.Equal(t, reconcile.Result{}, res)
}
} else {
if assert.Error(t, err) {
assert.Contains(t, err.Error(), tc.expectError)
}
}
cmpOpts := []cmp.Option{
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "Finalizers", "Labels", "OwnerReferences", "ResourceVersion"),
Expand Down
15 changes: 12 additions & 3 deletions pkg/resources/dnsrecord/dns.go
View file
Expand Up @@ -65,8 +65,8 @@ func EnsureWildcardDNSRecord(client client.Client, name types.NamespacedName, dn

// EnsureDNSRecord will create DNS records for the given LB service. If service
// is nil (haveLBS is false), nothing is done.
func EnsureDNSRecord(client client.Client, name types.NamespacedName, dnsRecordLabels map[string]string, ownerRef metav1.OwnerReference, domain string, service *corev1.Service) (bool, *iov1.DNSRecord, error) {
wantWC, desired := desiredDNSRecord(name, dnsRecordLabels, ownerRef, domain, iov1.ManagedDNS, service)
func EnsureDNSRecord(client client.Client, name types.NamespacedName, dnsRecordLabels map[string]string, ownerRef metav1.OwnerReference, domain string, dnsPolicy iov1.DNSManagementPolicy, service *corev1.Service) (bool, *iov1.DNSRecord, error) {
wantWC, desired := desiredDNSRecord(name, dnsRecordLabels, ownerRef, domain, dnsPolicy, service)
haveWC, current, err := CurrentDNSRecord(client, name)
if err != nil {
return false, nil, err
Expand Down Expand Up @@ -229,11 +229,20 @@ func dnsRecordChanged(current, expected *iov1.DNSRecord) (bool, *iov1.DNSRecord)
// once we know there are no users depending on this.
// See https://bugzilla.redhat.com/show_bug.cgi?id=2041616
func ManageDNSForDomain(domain string, status *configv1.PlatformStatus, dnsConfig *configv1.DNS) bool {
if len(domain) == 0 {
if len(domain) == 0 || len(dnsConfig.Spec.BaseDomain) == 0 {
return false
}

mustContain := "." + dnsConfig.Spec.BaseDomain

// Ignore any trailing dot for comparison.
if strings.HasSuffix(mustContain, ".") {
mustContain = mustContain[:len(mustContain)-1]
}
if strings.HasSuffix(domain, ".") {
domain = domain[:len(domain)-1]
}

switch status.Type {
case configv1.AWSPlatformType, configv1.GCPPlatformType:
return strings.HasSuffix(domain, mustContain)
Expand Down

0 comments on commit 26492db

Please sign in to comment.