apply records before the apex NS, so in-zone glue exists first

named rejects an apex NS pointing at an in-zone name with no address record,
so the glue has to land in an earlier transaction.

internal/bind/nsupdate.go

@@ -52,15 +52,22 @@ func (e *Executor) ApexNS(ctx context.Context, namespace, pod, zone string, cred
 	if err != nil {
 		return nil, fmt.Errorf("query apex NS of %s: %w (out: %s)", zone, err, out)
 	}
-	var ns []string
+	return parseDigNames(out), nil
+}
+
+// parseDigNames picks the answers out of `dig +short` output: one fully-qualified
+// name per line. Anything without a trailing dot is not a name, and dig prefixes
+// its diagnostics (a missing or mismatched TSIG key among them) with ';'.
+func parseDigNames(out string) []string {
+	var names []string
 	for _, line := range strings.Split(out, "\n") {
-		// dig +short prints one fully-qualified name per line; anything without a
-		// trailing dot is not an answer.
-		if line = strings.TrimSpace(line); strings.HasSuffix(line, ".") {
-			ns = append(ns, line)
+		line = strings.TrimSpace(line)
+		if strings.HasPrefix(line, ";") || !strings.HasSuffix(line, ".") {
+			continue
 		}
+		names = append(names, line)
 	}
-	return ns, nil
+	return names
 }
 
 // nsupdateScript renders the nsupdate input for a set of changes.

internal/bind/nsupdate_test.go

@@ -1,6 +1,9 @@
 package bind
 
-import "testing"
+import (
+	"strings"
+	"testing"
+)
 
 func TestNSUpdateScriptReplaceSemantics(t *testing.T) {
 	got := nsupdateScript("acme.unkin.net", []RecordUpdate{
@@ -40,3 +43,22 @@ send
 		t.Errorf("got:\n%s\nwant:\n%s", got, want)
 	}
 }
+
+func TestParseDigNames(t *testing.T) {
+	cases := []struct {
+		name, out, want string
+	}{
+		{"answers", "a.ns.unkin.net.\nb.ns.unkin.net.\n", "a.ns.unkin.net.,b.ns.unkin.net."},
+		{"REFUSED, SERVFAIL and NXDOMAIN all answer empty", "", ""},
+		// A zone behind a key-matched view answers an unsigned query REFUSED, and
+		// dig reports the key problem on a ';' line that happens to end in a dot.
+		{"dig diagnostics are not answers", ";; WARNING -- TSIG key was not used.\n", ""},
+		{"relative or partial lines are not names", "10.0.0.1\nns1\n", ""},
+		{"whitespace is trimmed", "  ns1.unkin.net.  \n\n", "ns1.unkin.net."},
+	}
+	for _, c := range cases {
+		if got := strings.Join(parseDigNames(c.out), ","); got != c.want {
+			t.Errorf("%s: parseDigNames(%q) = %q; want %q", c.name, c.out, got, c.want)
+		}
+	}
+}

internal/controller/bindzone_controller.go

@@ -118,17 +118,27 @@ func (r *BindZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		return r.setPhase(ctx, &zone, "Error", "AddZoneFailed", err.Error())
 	}
 
-	// Converge the apex NS on every pass, not only at seed time, so a zone that
-	// was seeded with the placeholder moves onto its real nameservers. Only for a
-	// zone that declared them: otherwise the operator would fight whoever else
-	// manages the RRset.
+	// Records are applied before the apex NS: an in-zone nameserver's address
+	// record has to exist first, or named rejects the apex transaction with a
+	// post-update nameserver sanity check failure.
 	recordCount := 0
-	if isPrimaryType(zone.Spec.Type) {
-		creds, credErr := r.zoneUpdateCreds(ctx, &zone)
-		if nsDeclared {
-			if credErr != nil {
-				return r.setPhase(ctx, &zone, "Error", "NoUpdateKey", credErr.Error())
+	records := recordsToUpdates(zone.Spec.ZoneName, zone.Spec.Records, zone.Spec.DefaultTTL)
+	if isPrimaryType(zone.Spec.Type) && (len(records) > 0 || nsDeclared) {
+		creds, err := r.zoneUpdateCreds(ctx, &zone)
+		if err != nil {
+			return r.setPhase(ctx, &zone, "Error", "NoUpdateKey", err.Error())
+		}
+		if len(records) > 0 {
+			if err := r.Exec.NSUpdate(ctx, zone.Namespace, primaryPod, zone.Spec.ZoneName, creds, records); err != nil {
+				return r.setPhase(ctx, &zone, "Error", "RecordUpdateFailed", err.Error())
 			}
+			recordCount = len(records)
+		}
+		// Converge the apex NS on every pass, not only at seed time, so a zone
+		// seeded with the placeholder moves onto its real nameservers. Only for a
+		// zone that declared them: otherwise the operator would fight whoever else
+		// manages the RRset.
+		if nsDeclared {
 			live, err := r.Exec.ApexNS(ctx, zone.Namespace, primaryPod, zone.Spec.ZoneName, creds)
 			if err != nil {
 				return r.setPhase(ctx, &zone, "Error", "ApexNSQueryFailed", err.Error())
@@ -138,21 +148,12 @@ func (r *BindZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 			}
 			if apex := apexNSUpdates(&zone, nameservers, live, nsTTL); len(apex) > 0 {
 				if err := r.Exec.NSUpdate(ctx, zone.Namespace, primaryPod, zone.Spec.ZoneName, creds, apex); err != nil {
-					return r.setPhase(ctx, &zone, "Error", "ApexNSSyncFailed", err.Error())
+					return r.setPhase(ctx, &zone, "Error", "ApexNSSyncFailed",
+						fmt.Sprintf("%s (a nameserver inside the zone needs an address record here)", err))
 				}
 				logger.Info("apex NS converged", "zone", zone.Spec.ZoneName, "nameservers", nameservers)
 			}
 		}
-		// Seed static records.
-		if updates := recordsToUpdates(zone.Spec.ZoneName, zone.Spec.Records, zone.Spec.DefaultTTL); len(updates) > 0 {
-			if credErr != nil {
-				return r.setPhase(ctx, &zone, "Error", "NoUpdateKey", credErr.Error())
-			}
-			if err := r.Exec.NSUpdate(ctx, zone.Namespace, primaryPod, zone.Spec.ZoneName, creds, updates); err != nil {
-				return r.setPhase(ctx, &zone, "Error", "RecordUpdateFailed", err.Error())
-			}
-			recordCount = len(updates)
-		}
 	}
 
 	// Register in the catalog so secondaries auto-provision.

internal/controller/zone_helpers.go

@@ -169,6 +169,9 @@ func zoneNameservers(zone *bindv1alpha1.BindZone, cluster *bindv1alpha1.BindClus
 // no NS records — a primary always has one. Nothing is retracted in that case:
 // retracting blind is what turns a delete into "delete the last NS", which named
 // rejects outright.
+// ponytail: names already published are diffed by name only, so an edit to just
+// the TTL never republishes them (dig +short cannot report a TTL). Re-add the
+// whole desired set each pass if TTL edits need to converge.
 func apexNSUpdates(zone *bindv1alpha1.BindZone, desired, live []string, ttl int32) []bind.RecordUpdate {
 	apex := fqdn("@", zone.Spec.ZoneName)
 	add := missing(desired, live)
@@ -177,8 +180,10 @@ func apexNSUpdates(zone *bindv1alpha1.BindZone, desired, live []string, ttl int3
 	if len(add) > 0 {
 		updates = append(updates, bind.RecordUpdate{FQDN: apex, Type: "NS", TTL: ttl, Values: add, PerValue: true})
 	}
+	// missing() yields nothing against an empty live set, so an unreadable RRset
+	// retracts nothing on its own.
 	del := missing(live, desired)
-	if len(live) == 0 || len(del) == 0 {
+	if len(del) == 0 {
 		return updates
 	}
 	updates = append(updates, bind.RecordUpdate{FQDN: apex, Type: "NS", Values: del, PerValue: true, Delete: true})