read the live apex NS with a signed query, retract nothing when unreadable

An unsigned localhost query is REFUSED for a zone behind a key-matched view and
answers empty with exit 0; signing it with the update creds reaches the view.

api/v1alpha1/bindzone_types.go

@@ -68,8 +68,10 @@ type BindZoneSpec struct {
 	// sync on every reconcile. Each entry is a full domain name, never relative
 	// to the zone. Prefer out-of-zone names glued by the parent: an
 	// in-zone name needs an address record in the zone, and the seed can only
-	// supply the primary pod's (unstable) IP for it. When empty the apex NS is
-	// the primary's stable in-cluster DNS name.
+	// supply the primary pod's (unstable) IP for it. When empty the operator
+	// leaves the apex NS alone and a newly seeded zone gets the primary's stable
+	// in-cluster DNS name; clearing the field later does not retract what it
+	// published.
 	// +optional
 	Nameservers []string `json:"nameservers,omitempty"`
 
@@ -126,10 +128,6 @@ 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,11 +1017,6 @@ 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

@@ -100,8 +100,10 @@ spec:
                   sync on every reconcile. Each entry is a full domain name, never relative
                   to the zone. Prefer out-of-zone names glued by the parent: an
                   in-zone name needs an address record in the zone, and the seed can only
-                  supply the primary pod's (unstable) IP for it. When empty the apex NS is
-                  the primary's stable in-cluster DNS name.
+                  supply the primary pod's (unstable) IP for it. When empty the operator
+                  leaves the apex NS alone and a newly seeded zone gets the primary's stable
+                  in-cluster DNS name; clearing the field later does not retract what it
+                  published.
                 items:
                   type: string
                 type: array
@@ -243,13 +245,6 @@ 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

@@ -2766,8 +2766,10 @@ spec:
                   sync on every reconcile. Each entry is a full domain name, never relative
                   to the zone. Prefer out-of-zone names glued by the parent: an
                   in-zone name needs an address record in the zone, and the seed can only
-                  supply the primary pod's (unstable) IP for it. When empty the apex NS is
-                  the primary's stable in-cluster DNS name.
+                  supply the primary pod's (unstable) IP for it. When empty the operator
+                  leaves the apex NS alone and a newly seeded zone gets the primary's stable
+                  in-cluster DNS name; clearing the field later does not retract what it
+                  published.
                 items:
                   type: string
                 type: array
@@ -2909,13 +2911,6 @@ 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,6 +23,7 @@ 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/nsupdate.go

@@ -38,6 +38,31 @@ func (e *Executor) NSUpdate(ctx context.Context, namespace, pod, zone string, cr
 	return nil
 }
 
+// ApexNS returns the zone's currently published apex NS names, so the operator
+// can converge the RRset rather than append to it. The query is TSIG-signed with
+// the same creds as an update: a zone behind a view whose match-clients is a key
+// is unreachable to an unsigned query, which named answers REFUSED (with an empty
+// body and a zero exit status), and the caller must not read that as "no NS".
+func (e *Executor) ApexNS(ctx context.Context, namespace, pod, zone string, creds TSIGCreds) ([]string, error) {
+	cmd := []string{
+		DigBin, "-y", fmt.Sprintf("%s:%s:%s", creds.Algorithm, creds.Name, creds.Secret),
+		"+short", "+time=5", "+tries=1", "@127.0.0.1", dot(zone), "NS",
+	}
+	out, err := e.Exec(ctx, namespace, pod, cmd, "")
+	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
+}
+
 // nsupdateScript renders the nsupdate input for a set of changes.
 func nsupdateScript(zone string, updates []RecordUpdate) string {
 	var b strings.Builder

internal/controller/apex_ns_test.go

@@ -56,19 +56,15 @@ func TestZoneNameservers(t *testing.T) {
 	}
 }
 
-// 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)
-	}
+// A query that cannot see the zone returns nothing, which must not be read as an
+// empty apex: retracting blind means deleting the last NS record, which named
+// rejects, leaving the zone stuck.
+func TestApexNSUpdatesUnreadableLiveSetIsAdditive(t *testing.T) {
+	zone := zoneWith(bindv1alpha1.BindZoneSpec{Nameservers: []string{"ns1.unkin.net."}})
+	got := apexNSUpdates(zone, []string{"ns1.unkin.net."}, nil, 3600)
+	assertUpdates(t, got, []bind.RecordUpdate{
+		{FQDN: "acme.unkin.net.", Type: "NS", TTL: 3600, Values: []string{"ns1.unkin.net."}, PerValue: true},
+	})
 }
 
 // The apex NS RRset must be converged per record: an RRset-wide delete at the

internal/controller/bindzone_controller.go

@@ -129,13 +129,19 @@ func (r *BindZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 			if credErr != nil {
 				return r.setPhase(ctx, &zone, "Error", "NoUpdateKey", credErr.Error())
 			}
-			if apex := apexNSUpdates(&zone, nameservers, publishedNameservers(&zone, cluster), nsTTL); len(apex) > 0 {
+			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())
+			}
+			if len(live) == 0 {
+				logger.Info("apex NS not readable, publishing without retracting", "zone", zone.Spec.ZoneName)
+			}
+			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())
 				}
 				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

@@ -159,35 +159,26 @@ func zoneNameservers(zone *bindv1alpha1.BindZone, cluster *bindv1alpha1.BindClus
 	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
+// apexNSUpdates moves a zone's apex NS RRset from live onto 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 {
+//
+// An empty live set means the query could not see the zone, not that the apex has
+// 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.
+func apexNSUpdates(zone *bindv1alpha1.BindZone, desired, live []string, ttl int32) []bind.RecordUpdate {
 	apex := fqdn("@", zone.Spec.ZoneName)
-	add := missing(desired, published)
-	del := missing(published, desired)
+	add := missing(desired, live)
 
 	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 {
+	del := missing(live, desired)
+	if len(live) == 0 || len(del) == 0 {
 		return updates
 	}
 	updates = append(updates, bind.RecordUpdate{FQDN: apex, Type: "NS", Values: del, PerValue: true, Delete: true})