retract apex NS from recorded state, not a live query

An unsigned localhost query silently returns nothing for a zone behind a
BindView, which would strand the placeholder. Record what was published instead.

api/v1alpha1/bindzone_types.go

@@ -126,6 +126,10 @@ type BindZoneStatus struct {
 	// RecordCount is the number of managed record sets applied.
 	// +optional
 	RecordCount int32 `json:"recordCount,omitempty"`
+	// Nameservers records the apex NS names the operator last published, so a
+	// change to spec.nameservers knows which entries to retract.
+	// +optional
+	Nameservers []string `json:"nameservers,omitempty"`
 	// Signed reports whether DNSSEC signing is active.
 	// +optional
 	Signed bool `json:"signed,omitempty"`

api/v1alpha1/zz_generated.deepcopy.go

@@ -1017,6 +1017,11 @@ func (in *BindZoneSpec) DeepCopy() *BindZoneSpec {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *BindZoneStatus) DeepCopyInto(out *BindZoneStatus) {
 	*out = *in
+	if in.Nameservers != nil {
+		in, out := &in.Nameservers, &out.Nameservers
+		*out = make([]string, len(*in))
+		copy(*out, *in)
+	}
 	if in.Conditions != nil {
 		in, out := &in.Conditions, &out.Conditions
 		*out = make([]v1.Condition, len(*in))

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

@@ -243,6 +243,13 @@ spec:
                 x-kubernetes-list-map-keys:
                 - type
                 x-kubernetes-list-type: map
+              nameservers:
+                description: |-
+                  Nameservers records the apex NS names the operator last published, so a
+                  change to spec.nameservers knows which entries to retract.
+                items:
+                  type: string
+                type: array
               observedGeneration:
                 format: int64
                 type: integer

config/crd/install.yaml

@@ -2909,6 +2909,13 @@ spec:
                 x-kubernetes-list-map-keys:
                 - type
                 x-kubernetes-list-type: map
+              nameservers:
+                description: |-
+                  Nameservers records the apex NS names the operator last published, so a
+                  change to spec.nameservers knows which entries to retract.
+                items:
+                  type: string
+                type: array
               observedGeneration:
                 format: int64
                 type: integer

internal/bind/consts.go

@@ -23,7 +23,6 @@ const (
 	NamedBin    = "/usr/sbin/named"
 	RndcBin     = "/usr/sbin/rndc"
 	NsupdateBin = "/usr/bin/nsupdate"
-	DigBin      = "/usr/bin/dig"
 )
 
 // Config file paths derived from ConfigDir.

internal/bind/rndc.go

@@ -84,21 +84,3 @@ func (e *Executor) ZoneSerial(ctx context.Context, namespace, pod, zone, view st
 	}
 	return 0, nil
 }
-
-// ApexNS returns the zone's currently published apex NS names, queried from the
-// local server so the operator can converge the RRset rather than append to it.
-func (e *Executor) ApexNS(ctx context.Context, namespace, pod, zone string) ([]string, error) {
-	out, err := e.Exec(ctx, namespace, pod, []string{DigBin, "+short", "@127.0.0.1", dot(zone), "NS"}, "")
-	if err != nil {
-		return nil, fmt.Errorf("query apex NS of %s: %w (out: %s)", zone, err, out)
-	}
-	var ns []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)
-		}
-	}
-	return ns, nil
-}

internal/controller/apex_ns_test.go

@@ -22,36 +22,58 @@ func testCluster() *bindv1alpha1.BindCluster {
 const stableNS = "auth-0.auth-headless.bind-internal.svc.cluster.local."
 
 func TestZoneNameservers(t *testing.T) {
+	ttl60 := int32(60)
 	cases := []struct {
 		name     string
 		spec     bindv1alpha1.BindZoneSpec
 		want     []string
+		ttl      int32
 		declared bool
 	}{
-		{"fallback is out-of-zone, so it needs no glue", bindv1alpha1.BindZoneSpec{}, []string{stableNS}, false},
-		{"declared wins", bindv1alpha1.BindZoneSpec{Nameservers: []string{"ns1.unkin.net"}}, []string{"ns1.unkin.net."}, true},
+		{"fallback is out-of-zone, so it needs no glue", bindv1alpha1.BindZoneSpec{}, []string{stableNS}, 3600, false},
+		{"declared wins", bindv1alpha1.BindZoneSpec{Nameservers: []string{"ns1.unkin.net"}}, []string{"ns1.unkin.net."}, 3600, true},
+		{"spec.defaultTTL applies", bindv1alpha1.BindZoneSpec{Nameservers: []string{"ns1.unkin.net."}, DefaultTTL: 60}, []string{"ns1.unkin.net."}, 60, true},
 		// An apex NS in spec.records cannot converge on its own: BIND ignores an
 		// RRset-wide delete at the apex, so it has to go through the apex path.
 		{"apex NS in records counts as declared", bindv1alpha1.BindZoneSpec{Records: []bindv1alpha1.Record{
 			{Name: "@", Type: "ns", Values: []string{"a.ns.unkin.net.", "b.ns.unkin.net."}},
 			{Name: "www", Type: "A", Values: []string{"10.0.0.1"}},
-		}}, []string{"a.ns.unkin.net.", "b.ns.unkin.net."}, true},
+		}}, []string{"a.ns.unkin.net.", "b.ns.unkin.net."}, 3600, true},
+		// A TTL on the apex NS record itself must survive the fold.
+		{"record TTL beats the zone default", bindv1alpha1.BindZoneSpec{DefaultTTL: 3600, Records: []bindv1alpha1.Record{
+			{Name: "@", Type: "NS", TTL: &ttl60, Values: []string{"a.ns.unkin.net."}},
+		}}, []string{"a.ns.unkin.net."}, 60, true},
 		{"spec.nameservers beats records", bindv1alpha1.BindZoneSpec{
 			Nameservers: []string{"ns1.unkin.net."},
 			Records:     []bindv1alpha1.Record{{Name: "@", Type: "NS", Values: []string{"other.unkin.net."}}},
-		}, []string{"ns1.unkin.net."}, true},
+		}, []string{"ns1.unkin.net."}, 3600, true},
 	}
 	for _, c := range cases {
-		got, declared := zoneNameservers(zoneWith(c.spec), testCluster())
-		if declared != c.declared || strings.Join(got, ",") != strings.Join(c.want, ",") {
-			t.Errorf("%s: got %v/%v; want %v/%v", c.name, got, declared, c.want, c.declared)
+		got, ttl, declared := zoneNameservers(zoneWith(c.spec), testCluster())
+		if declared != c.declared || ttl != c.ttl || strings.Join(got, ",") != strings.Join(c.want, ",") {
+			t.Errorf("%s: got %v/%d/%v; want %v/%d/%v", c.name, got, ttl, declared, c.want, c.ttl, c.declared)
 		}
 	}
 }
 
+// Before the operator has recorded anything, the only names it can have published
+// are the ones a seed writes; afterwards its record is the authority, so it never
+// retracts a name someone else added.
+func TestPublishedNameservers(t *testing.T) {
+	zone := zoneWith(bindv1alpha1.BindZoneSpec{})
+	want := "ns1.acme.unkin.net.," + stableNS
+	if got := publishedNameservers(zone, testCluster()); strings.Join(got, ",") != want {
+		t.Errorf("unrecorded: got %v; want %s", got, want)
+	}
+	zone.Status.Nameservers = []string{"a.ns.unkin.net."}
+	if got := publishedNameservers(zone, testCluster()); strings.Join(got, ",") != "a.ns.unkin.net." {
+		t.Errorf("recorded: got %v; want [a.ns.unkin.net.]", got)
+	}
+}
+
 // The apex NS RRset must be converged per record: an RRset-wide delete at the
 // apex is ignored by BIND, which would leave the placeholder published alongside
-// the real nameservers.
+// the real nameservers. Retracting an in-zone name takes its glue with it.
 func TestApexNSUpdatesConverges(t *testing.T) {
 	zone := zoneWith(bindv1alpha1.BindZoneSpec{Nameservers: []string{"ns1.unkin.net."}, DefaultTTL: 60})
 	got := apexNSUpdates(zone, []string{"ns1.unkin.net."}, []string{"ns1.acme.unkin.net."}, 60)
@@ -62,6 +84,18 @@ func TestApexNSUpdatesConverges(t *testing.T) {
 	})
 }
 
+// Glue retirement is not special-cased to the name ns1: the seed glues every
+// declared in-zone nameserver.
+func TestApexNSUpdatesRetiresAnyInZoneGlue(t *testing.T) {
+	zone := zoneWith(bindv1alpha1.BindZoneSpec{Nameservers: []string{"a.ns.unkin.net."}})
+	got := apexNSUpdates(zone, []string{"a.ns.unkin.net."}, []string{"dns.acme.unkin.net."}, 3600)
+	assertUpdates(t, got, []bind.RecordUpdate{
+		{FQDN: "acme.unkin.net.", Type: "NS", TTL: 3600, Values: []string{"a.ns.unkin.net."}, PerValue: true},
+		{FQDN: "acme.unkin.net.", Type: "NS", Values: []string{"dns.acme.unkin.net."}, PerValue: true, Delete: true},
+		{FQDN: "dns.acme.unkin.net.", Type: "A", Delete: true},
+	})
+}
+
 func TestApexNSUpdatesNoopWhenConverged(t *testing.T) {
 	zone := zoneWith(bindv1alpha1.BindZoneSpec{Nameservers: []string{"ns1.unkin.net"}})
 	// Case differs: DNS names compare case-insensitively, so this is converged.
@@ -80,7 +114,7 @@ func TestApexNSUpdatesPartialChange(t *testing.T) {
 	})
 }
 
-// A declared in-zone ns1 owns the glue; removing it would fail named's
+// A declared in-zone nameserver owns its glue; removing it would fail named's
 // post-update nameserver sanity check.
 func TestApexNSUpdatesKeepsNeededGlue(t *testing.T) {
 	zone := zoneWith(bindv1alpha1.BindZoneSpec{Nameservers: []string{"ns1.acme.unkin.net."}})

internal/controller/bindzone_controller.go

@@ -99,7 +99,7 @@ func (r *BindZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		return r.setPhase(ctx, &zone, "Error", "ConfigError", err.Error())
 	}
 
-	nameservers, nsDeclared := zoneNameservers(&zone, cluster)
+	nameservers, nsTTL, nsDeclared := zoneNameservers(&zone, cluster)
 
 	created := !r.Exec.ZoneExists(ctx, zone.Namespace, primaryPod, zone.Spec.ZoneName, zone.Spec.ViewRef)
 	if created && (zone.Spec.Type == bindv1alpha1.ZonePrimary || zone.Spec.Type == "") {
@@ -129,16 +129,13 @@ func (r *BindZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 			if credErr != nil {
 				return r.setPhase(ctx, &zone, "Error", "NoUpdateKey", credErr.Error())
 			}
-			live, err := r.Exec.ApexNS(ctx, zone.Namespace, primaryPod, zone.Spec.ZoneName)
-			if err != nil {
-				return r.setPhase(ctx, &zone, "Error", "ApexNSQueryFailed", err.Error())
-			}
-			if apex := apexNSUpdates(&zone, nameservers, live, zone.Spec.DefaultTTL); len(apex) > 0 {
+			if apex := apexNSUpdates(&zone, nameservers, publishedNameservers(&zone, cluster), 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())
 				}
 				logger.Info("apex NS converged", "zone", zone.Spec.ZoneName, "nameservers", nameservers)
 			}
+			zone.Status.Nameservers = nameservers
 		}
 		// Seed static records.
 		if updates := recordsToUpdates(zone.Spec.ZoneName, zone.Spec.Records, zone.Spec.DefaultTTL); len(updates) > 0 {

internal/controller/zone_helpers.go

@@ -125,63 +125,92 @@ 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 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) {
+// zoneNameservers resolves the names to publish in a zone's apex NS RRset, the
+// TTL to publish them with, and 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, ttl int32, declared bool) {
+	ttl = zone.Spec.DefaultTTL
 	for _, ns := range zone.Spec.Nameservers {
 		names = append(names, absolute(ns))
 	}
-	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))
-			}
+		if len(names) > 0 {
+			break
+		}
+		if !strings.EqualFold(rec.Type, "NS") || fqdn(rec.Name, zone.Spec.ZoneName) != fqdn("@", zone.Spec.ZoneName) {
+			continue
+		}
+		for _, v := range rec.Values {
+			names = append(names, absolute(v))
+		}
+		if rec.TTL != nil {
+			ttl = *rec.TTL
 		}
 	}
-	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
 	}
+	if len(names) > 0 {
+		return names, ttl, true
+	}
+	return clusterNameservers(cluster), ttl, false
+}
+
+// publishedNameservers is what the operator has already put in the apex NS RRset.
+// It retracts only these, never a name someone else added, and needs no query
+// against the pod: reading the live RRset back would take a view-scoped lookup,
+// and an unsigned one silently returns nothing for a zone behind a BindView.
+func publishedNameservers(zone *bindv1alpha1.BindZone, cluster *bindv1alpha1.BindCluster) []string {
+	if len(zone.Status.Nameservers) > 0 {
+		return zone.Status.Nameservers
+	}
+	// Nothing recorded yet, so the only names in the RRset are what a seed can
+	// write: an in-zone ns1 glued to the primary pod's IP (older seeds) or the
+	// stable in-cluster name (current ones).
+	return append([]string{fqdn("ns1", zone.Spec.ZoneName)}, clusterNameservers(cluster)...)
+}
+
+// apexNSUpdates moves a zone's apex NS RRset from published to desired, and
+// retires the glue of any in-zone name it retracts. Adds come first: BIND refuses
+// to leave an apex with no NS record, so the replacement must exist before the old
+// name goes, and deleting glue still referenced by an in-zone NS fails named's
+// post-update nameserver sanity check.
+func apexNSUpdates(zone *bindv1alpha1.BindZone, desired, published []string, ttl int32) []bind.RecordUpdate {
 	apex := fqdn("@", zone.Spec.ZoneName)
-	add := missing(desired, live)
-	del := missing(live, desired)
+	add := missing(desired, published)
+	del := missing(published, desired)
 
 	var updates []bind.RecordUpdate
 	if len(add) > 0 {
 		updates = append(updates, bind.RecordUpdate{FQDN: apex, Type: "NS", TTL: ttl, Values: add, PerValue: true})
 	}
-	if len(del) > 0 {
-		updates = append(updates, bind.RecordUpdate{FQDN: apex, Type: "NS", Values: del, PerValue: true, Delete: true})
+	if len(del) == 0 {
+		return updates
 	}
+	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})
+	// stale on the first reschedule. Drop that address with the name, unless
+	// spec.records owns it (then it is real data, not the placeholder).
+	for _, ns := range del {
+		owner, in := bind.InZoneOwner(ns, zone.Spec.ZoneName)
+		if in && owner != "@" && !recordsOwn(zone, owner, "A") {
+			updates = append(updates, bind.RecordUpdate{FQDN: fqdn(owner, zone.Spec.ZoneName), Type: "A", Delete: true})
+		}
 	}
 	return updates
 }
 
-// missing returns the names in want that have no case-insensitive match in have.
+// 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) + "."}
+}
+
+// missing returns the names in want with no match in have. DNS names compare
+// case-insensitively.
 func missing(want, have []string) (out []string) {
 	for _, w := range want {
 		if !containsName(have, w) {
@@ -210,9 +239,3 @@ func recordsOwn(zone *bindv1alpha1.BindZone, name, typ string) bool {
 	}
 	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) + "."}
-}