converge apex NS per record, not by RRset replace

BIND ignores an RRset-wide delete of apex NS, so the previous replace only
appended to the seed placeholder.

internal/controller/zone_helpers.go

@@ -42,6 +42,11 @@ func fqdn(name, zone string) string {
 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) {
+			continue
+		}
 		ttl := defaultTTL
 		if rec.TTL != nil {
 			ttl = *rec.TTL
@@ -120,52 +125,94 @@ func alsoNotifyList(addrs []string, key string) string {
 // zone: an in-zone nameserver is spelled out in full.
 func absolute(name string) string { return strings.TrimSuffix(name, ".") + "." }
 
-// zoneNameservers resolves the names to publish in a zone's apex NS RRset: the
-// declared nameservers, else the primary's stable in-cluster DNS name. The
-// fallback is deliberately out-of-zone, so no pod IP is needed as glue.
-func zoneNameservers(declared []string, cluster *bindv1alpha1.BindCluster) []string {
-	if len(declared) > 0 {
-		return declared
+// zoneNameservers resolves the names to publish in a zone's apex NS RRset and
+// reports whether the zone declared them. An apex NS in spec.records counts as a
+// declaration: BIND ignores an RRset-wide delete at the apex, so records alone
+// can only append to what the zone was seeded with, never replace it. Undeclared
+// zones fall back to the primary's stable in-cluster name, which is deliberately
+// out-of-zone so no pod IP is needed as glue.
+func zoneNameservers(zone *bindv1alpha1.BindZone, cluster *bindv1alpha1.BindCluster) (names []string, declared bool) {
+	for _, ns := range zone.Spec.Nameservers {
+		names = append(names, absolute(ns))
 	}
-	return []string{primaryAddress(cluster.Name, cluster.Namespace) + "."}
-}
-
-// apexNSUpdates returns the dynamic-update ops that keep a zone's apex NS RRset
-// equal to nameservers, plus removal of the seed's ns1 glue once the zone
-// declares its own nameservers. Ops colliding with a spec.records entry are
-// dropped: records are applied afterwards and would re-add them, and the churn
-// would bump the serial on every reconcile.
-func apexNSUpdates(zone *bindv1alpha1.BindZone, nameservers []string) []bind.RecordUpdate {
-	owns := func(name, typ string) bool {
-		for _, rec := range zone.Spec.Records {
-			if strings.EqualFold(rec.Type, typ) && fqdn(rec.Name, zone.Spec.ZoneName) == fqdn(name, zone.Spec.ZoneName) {
-				return true
+	if len(names) > 0 {
+		return names, true
+	}
+	for _, rec := range zone.Spec.Records {
+		if strings.EqualFold(rec.Type, "NS") && fqdn(rec.Name, zone.Spec.ZoneName) == fqdn("@", zone.Spec.ZoneName) {
+			for _, v := range rec.Values {
+				names = append(names, absolute(v))
 			}
 		}
-		return false
 	}
-	ttl := zone.Spec.DefaultTTL
+	if len(names) > 0 {
+		return names, true
+	}
+	return []string{primaryAddress(cluster.Name, cluster.Namespace) + "."}, false
+}
+
+// apexNSUpdates converges a zone's live apex NS RRset onto desired, and retires
+// the seed's ns1 glue once no published nameserver needs it. Adds come first:
+// BIND refuses to leave an apex with no NS record, so the replacement must exist
+// before the old name goes.
+func apexNSUpdates(zone *bindv1alpha1.BindZone, desired, live []string, ttl int32) []bind.RecordUpdate {
 	if ttl <= 0 {
 		ttl = 3600
 	}
+	apex := fqdn("@", zone.Spec.ZoneName)
+	add := missing(desired, live)
+	del := missing(live, desired)
+
 	var updates []bind.RecordUpdate
-	if !owns("@", "NS") {
-		values := make([]string, 0, len(nameservers))
-		for _, ns := range nameservers {
-			values = append(values, absolute(ns))
-		}
-		updates = append(updates, bind.RecordUpdate{FQDN: fqdn("@", zone.Spec.ZoneName), Type: "NS", TTL: ttl, Values: values})
+	if len(add) > 0 {
+		updates = append(updates, bind.RecordUpdate{FQDN: apex, Type: "NS", TTL: ttl, Values: add, PerValue: true})
 	}
-	// The seed's placeholder glue pins a pod IP that goes stale on the first
-	// reschedule; drop it once the zone names its real nameservers.
-	if glue := fqdn("ns1", zone.Spec.ZoneName); len(zone.Spec.Nameservers) > 0 && !owns("ns1", "A") {
-		published := false
-		for _, ns := range nameservers {
-			published = published || absolute(ns) == glue
-		}
-		if !published {
-			updates = append(updates, bind.RecordUpdate{FQDN: glue, Type: "A", Delete: true})
-		}
+	if len(del) > 0 {
+		updates = append(updates, bind.RecordUpdate{FQDN: apex, Type: "NS", Values: del, PerValue: true, Delete: true})
+	}
+	// The seed glues an in-zone nameserver to the primary pod's IP, which goes
+	// stale on the first reschedule. Drop it once no published nameserver is that
+	// name, unless spec.records owns the address itself. Deleting it while an
+	// in-zone NS still points at it would fail named's post-update sanity check.
+	glue := fqdn("ns1", zone.Spec.ZoneName)
+	if containsName(del, glue) && !containsName(desired, glue) && !recordsOwn(zone, "ns1", "A") {
+		updates = append(updates, bind.RecordUpdate{FQDN: glue, Type: "A", Delete: true})
 	}
 	return updates
 }
+
+// missing returns the names in want that have no case-insensitive match in have.
+func missing(want, have []string) (out []string) {
+	for _, w := range want {
+		if !containsName(have, w) {
+			out = append(out, w)
+		}
+	}
+	return out
+}
+
+func containsName(names []string, name string) bool {
+	for _, n := range names {
+		if strings.EqualFold(absolute(n), absolute(name)) {
+			return true
+		}
+	}
+	return false
+}
+
+// recordsOwn reports whether spec.records already manages an owner/type pair, in
+// which case the apex sync must leave it alone.
+func recordsOwn(zone *bindv1alpha1.BindZone, name, typ string) bool {
+	for _, rec := range zone.Spec.Records {
+		if strings.EqualFold(rec.Type, typ) && strings.EqualFold(fqdn(rec.Name, zone.Spec.ZoneName), fqdn(name, zone.Spec.ZoneName)) {
+			return true
+		}
+	}
+	return false
+}
+
+// clusterNameservers is the apex NS for the operator's own internal zones
+// (catalog, policy): the primary's stable in-cluster name, never a pod IP.
+func clusterNameservers(cluster *bindv1alpha1.BindCluster) []string {
+	return []string{primaryAddress(cluster.Name, cluster.Namespace) + "."}
+}