From cc714dc2b4710b1c9aff3c836be91513c628cdbb Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 26 Sep 2026 19:41:53 +1000 Subject: [PATCH] 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 | 19 ++++++---- internal/bind/nsupdate_test.go | 24 ++++++++++++- internal/controller/bindzone_controller.go | 41 +++++++++++----------- internal/controller/zone_helpers.go | 7 +++- 4 files changed, 63 insertions(+), 28 deletions(-) diff --git a/internal/bind/nsupdate.go b/internal/bind/nsupdate.go index 8611696..eccb6a2 100644 --- a/internal/bind/nsupdate.go +++ b/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. diff --git a/internal/bind/nsupdate_test.go b/internal/bind/nsupdate_test.go index e9ba75a..26d6344 100644 --- a/internal/bind/nsupdate_test.go +++ b/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) + } + } +} diff --git a/internal/controller/bindzone_controller.go b/internal/controller/bindzone_controller.go index bf26eac..9d02f10 100644 --- a/internal/controller/bindzone_controller.go +++ b/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. diff --git a/internal/controller/zone_helpers.go b/internal/controller/zone_helpers.go index 8b77934..683e180 100644 --- a/internal/controller/zone_helpers.go +++ b/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})