From dc57ac1b2d380d34862899f8c13f45f9cce6e54e Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 26 Sep 2026 19:06:47 +1000 Subject: [PATCH] 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 | 4 + api/v1alpha1/zz_generated.deepcopy.go | 5 + .../crd/bases/bind.unkin.net_bindzones.yaml | 7 ++ config/crd/install.yaml | 7 ++ internal/bind/consts.go | 1 - internal/bind/rndc.go | 18 --- internal/controller/apex_ns_test.go | 52 +++++++-- internal/controller/bindzone_controller.go | 9 +- internal/controller/zone_helpers.go | 107 +++++++++++------- 9 files changed, 134 insertions(+), 76 deletions(-) diff --git a/api/v1alpha1/bindzone_types.go b/api/v1alpha1/bindzone_types.go index 87d99c4..cd76bed 100644 --- a/api/v1alpha1/bindzone_types.go +++ b/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"` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index c9cb4cc..e8669f8 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/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)) diff --git a/config/crd/bases/bind.unkin.net_bindzones.yaml b/config/crd/bases/bind.unkin.net_bindzones.yaml index 4f2dfb7..e07785a 100644 --- a/config/crd/bases/bind.unkin.net_bindzones.yaml +++ b/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 diff --git a/config/crd/install.yaml b/config/crd/install.yaml index dae3e7b..05618d3 100644 --- a/config/crd/install.yaml +++ b/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 diff --git a/internal/bind/consts.go b/internal/bind/consts.go index b574fc0..7f2e383 100644 --- a/internal/bind/consts.go +++ b/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. diff --git a/internal/bind/rndc.go b/internal/bind/rndc.go index 41149e5..ba05f04 100644 --- a/internal/bind/rndc.go +++ b/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 -} diff --git a/internal/controller/apex_ns_test.go b/internal/controller/apex_ns_test.go index 4d6f426..9fbdc01 100644 --- a/internal/controller/apex_ns_test.go +++ b/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."}}) diff --git a/internal/controller/bindzone_controller.go b/internal/controller/bindzone_controller.go index 17c76dc..c41c65f 100644 --- a/internal/controller/bindzone_controller.go +++ b/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 { diff --git a/internal/controller/zone_helpers.go b/internal/controller/zone_helpers.go index 11a7c50..cf5f31b 100644 --- a/internal/controller/zone_helpers.go +++ b/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) + "."} -}