Merge pull request 'Reject an apex NS DNSRecord instead of silently appending' (#24) from benvin/apex-ns-dnsrecord-reject into main

Reviewed-on: #24

api/v1alpha1/dnsrecord_types.go

@@ -6,6 +6,7 @@ import (
 
 // DNSRecordSpec defines a single record set applied to a zone via TSIG dynamic
 // update (nsupdate) — the external-dns write path expressed as a CRD.
+// +kubebuilder:validation:XValidation:rule="!(self.type.lowerAscii() == 'ns' && (!has(self.name) || self.name == '@' || self.name.size() == 0))",message="an apex NS RRset cannot be managed by a DNSRecord: BIND ignores an RRset-wide delete at a zone apex, so this would only append. Use BindZone.spec.nameservers"
 type DNSRecordSpec struct {
 	// ZoneRef names the BindZone this record belongs to. The cluster, view and
 	// update key are derived from the referenced zone.

config/crd/bases/bind.unkin.net_dnsrecords.yaml

@@ -86,6 +86,12 @@ spec:
             - values
             - zoneRef
             type: object
+            x-kubernetes-validations:
+            - message: 'an apex NS RRset cannot be managed by a DNSRecord: BIND ignores
+                an RRset-wide delete at a zone apex, so this would only append. Use
+                BindZone.spec.nameservers'
+              rule: '!(self.type.lowerAscii() == ''ns'' && (!has(self.name) || self.name
+                == ''@'' || self.name.size() == 0))'
           status:
             description: DNSRecordStatus reports observed record state.
             properties:

config/crd/install.yaml

@@ -3022,6 +3022,12 @@ spec:
             - values
             - zoneRef
             type: object
+            x-kubernetes-validations:
+            - message: 'an apex NS RRset cannot be managed by a DNSRecord: BIND ignores
+                an RRset-wide delete at a zone apex, so this would only append. Use
+                BindZone.spec.nameservers'
+              rule: '!(self.type.lowerAscii() == ''ns'' && (!has(self.name) || self.name
+                == ''@'' || self.name.size() == 0))'
           status:
             description: DNSRecordStatus reports observed record state.
             properties:

internal/controller/dnsrecord_controller.go

@@ -44,6 +44,8 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	}
 	primaryPod := primaryPodName(cluster.Name)
 	name := fqdn(record.Spec.Name, zone.Spec.ZoneName)
+	// CEL on the spec catches "@" and ""; a zone-qualified apex name reaches here.
+	apexNS := isApexNS(record.Spec.Name, record.Spec.Type, zone.Spec.ZoneName)
 
 	creds, err := resolveTSIG(ctx, r.Client, record.Namespace, zone.Spec.UpdateKeyRef)
 	if err != nil {
@@ -53,7 +55,7 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	// Deletion via finalizer: remove the RRset.
 	if !record.DeletionTimestamp.IsZero() {
 		if controllerutil.ContainsFinalizer(&record, finalizer) {
-			if primaryReady(ctx, r.Client, cluster) && r.Exec != nil {
+			if primaryReady(ctx, r.Client, cluster) && r.Exec != nil && !apexNS {
 				_ = r.Exec.NSUpdate(ctx, record.Namespace, primaryPod, zone.Spec.ZoneName, creds,
 					[]bind.RecordUpdate{{FQDN: name, Type: record.Spec.Type, Delete: true}})
 			}
@@ -65,6 +67,11 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		return ctrl.Result{}, nil
 	}
 
+	if apexNS {
+		return r.setPhase(ctx, &record, "Error", "ApexNSUnsupported",
+			"BIND ignores an RRset-wide delete at a zone apex, so this record can only append; publish the apex NS via BindZone.spec.nameservers")
+	}
+
 	if !controllerutil.ContainsFinalizer(&record, finalizer) {
 		controllerutil.AddFinalizer(&record, finalizer)
 		if err := r.Update(ctx, &record); err != nil {

internal/controller/zone_helpers.go

@@ -39,12 +39,19 @@ func fqdn(name, zone string) string {
 	return name + "." + zone
 }
 
+// isApexNS reports whether an owner/type pair addresses a zone's apex NS RRset.
+// BIND ignores an RRset-wide delete there, so such a record can only append:
+// the apex NS is converged per rdata from BindZone.spec.nameservers.
+func isApexNS(name, typ, zone string) bool {
+	return strings.EqualFold(typ, "NS") && strings.EqualFold(fqdn(name, zone), fqdn("@", zone))
+}
+
 func recordsToUpdates(zone string, records []bindv1alpha1.Record, defaultTTL int32) []bind.RecordUpdate {
 	updates := make([]bind.RecordUpdate, 0, len(records))
 	for _, rec := range records {
 		// The apex NS RRset is converged by apexNSUpdates: an RRset-wide delete
 		// here is ignored by BIND and would only append to the live set.
-		if strings.EqualFold(rec.Type, "NS") && fqdn(rec.Name, zone) == fqdn("@", zone) {
+		if isApexNS(rec.Name, rec.Type, zone) {
 			continue
 		}
 		ttl := defaultTTL
@@ -140,7 +147,7 @@ func zoneNameservers(zone *bindv1alpha1.BindZone, cluster *bindv1alpha1.BindClus
 		if len(names) > 0 {
 			break
 		}
-		if !strings.EqualFold(rec.Type, "NS") || fqdn(rec.Name, zone.Spec.ZoneName) != fqdn("@", zone.Spec.ZoneName) {
+		if !isApexNS(rec.Name, rec.Type, zone.Spec.ZoneName) {
 			continue
 		}
 		for _, v := range rec.Values {

internal/controller/zone_helpers_test.go

@@ -77,3 +77,29 @@ func TestCatalogEnabledDefault(t *testing.T) {
 		t.Error("secondary zone should never be a catalog member")
 	}
 }
+
+func TestIsApexNS(t *testing.T) {
+	const zone = "acme.unkin.net"
+	cases := []struct {
+		name, typ string
+		want      bool
+	}{
+		{"@", "NS", true},
+		{"", "NS", true},
+		{"acme.unkin.net.", "NS", true},
+		{"ACME.UNKIN.NET.", "ns", true},
+		{"@", "ns", true},
+		// No trailing dot means relative: acme.unkin.net.acme.unkin.net.
+		{"acme.unkin.net", "NS", false},
+		{"sub", "NS", false},
+		{"ns1", "NS", false},
+		{"@", "TXT", false},
+		{"@", "MX", false},
+		{"@", "SOA", false},
+	}
+	for _, c := range cases {
+		if got := isApexNS(c.name, c.typ, zone); got != c.want {
+			t.Errorf("isApexNS(%q,%q)=%v want %v", c.name, c.typ, got, c.want)
+		}
+	}
+}