From 4a41cbc42714c4c0ec4a7207bd131238336f96c2 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 26 Sep 2026 18:27:11 +1000 Subject: [PATCH 1/5] reconcile zone apex NS off the pod IP placeholder Add BindZone.spec.nameservers and sync the apex NS RRset on every reconcile. --- api/v1alpha1/bindzone_types.go | 9 ++ api/v1alpha1/zz_generated.deepcopy.go | 5 + .../crd/bases/bind.unkin.net_bindzones.yaml | 11 +++ config/crd/install.yaml | 11 +++ config/samples/01-authoritative.yaml | 7 +- internal/bind/seed.go | 53 +++++++--- internal/bind/seed_nameservers_test.go | 75 ++++++++++++++ internal/bind/seed_test.go | 14 +-- internal/bind/zonestate_test.go | 10 +- internal/controller/apex_ns_test.go | 98 +++++++++++++++++++ .../controller/bindcatalogzone_controller.go | 2 +- internal/controller/bindpolicy_controller.go | 2 +- internal/controller/bindzone_controller.go | 33 +++++-- internal/controller/zone_helpers.go | 55 +++++++++++ 14 files changed, 346 insertions(+), 39 deletions(-) create mode 100644 internal/bind/seed_nameservers_test.go create mode 100644 internal/controller/apex_ns_test.go diff --git a/api/v1alpha1/bindzone_types.go b/api/v1alpha1/bindzone_types.go index f2b7354..87d99c4 100644 --- a/api/v1alpha1/bindzone_types.go +++ b/api/v1alpha1/bindzone_types.go @@ -64,6 +64,15 @@ type BindZoneSpec struct { // +optional DefaultTTL int32 `json:"defaultTTL,omitempty"` + // Nameservers are the names published in the zone's apex NS RRset, kept in + // 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. + // +optional + Nameservers []string `json:"nameservers,omitempty"` + // Records are static record sets seeded into a primary zone. // +optional Records []Record `json:"records,omitempty"` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index d0eb94e..c9cb4cc 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -970,6 +970,11 @@ func (in *BindZoneList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *BindZoneSpec) DeepCopyInto(out *BindZoneSpec) { *out = *in + if in.Nameservers != nil { + in, out := &in.Nameservers, &out.Nameservers + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.Records != nil { in, out := &in.Records, &out.Records *out = make([]Record, len(*in)) diff --git a/config/crd/bases/bind.unkin.net_bindzones.yaml b/config/crd/bases/bind.unkin.net_bindzones.yaml index 37f7406..4f2dfb7 100644 --- a/config/crd/bases/bind.unkin.net_bindzones.yaml +++ b/config/crd/bases/bind.unkin.net_bindzones.yaml @@ -94,6 +94,17 @@ spec: items: type: string type: array + nameservers: + description: |- + Nameservers are the names published in the zone's apex NS RRset, kept in + 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. + items: + type: string + type: array primaries: description: Primaries lists source servers for a secondary/stub-type zone. diff --git a/config/crd/install.yaml b/config/crd/install.yaml index 407d905..dae3e7b 100644 --- a/config/crd/install.yaml +++ b/config/crd/install.yaml @@ -2760,6 +2760,17 @@ spec: items: type: string type: array + nameservers: + description: |- + Nameservers are the names published in the zone's apex NS RRset, kept in + 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. + items: + type: string + type: array primaries: description: Primaries lists source servers for a secondary/stub-type zone. diff --git a/config/samples/01-authoritative.yaml b/config/samples/01-authoritative.yaml index 1ba73ca..adf6412 100644 --- a/config/samples/01-authoritative.yaml +++ b/config/samples/01-authoritative.yaml @@ -71,10 +71,11 @@ spec: - key transfer-key updateKeyRef: transfer-key dynamicUpdate: true + # Published apex NS, kept in sync on every reconcile. Full names only; an + # in-zone name (as here) needs its address record below. + nameservers: + - ns1.internal.example.com. records: - - name: "@" - type: NS - values: ["ns1.internal.example.com."] - name: ns1 type: A values: ["10.0.0.53"] diff --git a/internal/bind/seed.go b/internal/bind/seed.go index a7a8425..f144982 100644 --- a/internal/bind/seed.go +++ b/internal/bind/seed.go @@ -26,36 +26,65 @@ func (e *Executor) ZoneExists(ctx context.Context, namespace, pod, zone, view st return err == nil } -// renderSeedZone renders a minimal loadable zone (SOA + apex NS + glue). The -// apex NS is the in-zone name ns1, and a glue A record pointing at primaryIP is -// included so BIND's check-integrity accepts the zone (an in-zone NS without an -// address record is a load error). -func renderSeedZone(zone, primaryIP string, serial int64) string { +// renderSeedZone renders a minimal loadable zone (SOA + apex NS). nameservers +// are the names published in the apex NS RRset; when empty the in-zone name ns1 +// is used. A glue A pointing at primaryIP is emitted only for a nameserver that +// falls inside the zone, because BIND refuses to load a zone whose in-zone NS +// has no address record. Out-of-zone nameservers therefore keep pod IPs out of +// the zone file entirely. +func renderSeedZone(zone, primaryIP string, nameservers []string, serial int64) string { origin := dot(zone) - ns := "ns1." + origin + ns := make([]string, 0, len(nameservers)) + for _, n := range nameservers { + ns = append(ns, dot(n)) + } + if len(ns) == 0 { + ns = []string{"ns1." + origin} + } // Short refresh/retry so a secondary that misses a NOTIFY (e.g. its pod IP // changed and the primary's also-notify was briefly stale) still converges // in minutes, not the hour a 3600s refresh would impose. minimum is the // negative-cache TTL: keep it low so a stale-secondary NXDOMAIN does not // stick in downstream resolvers for long. NOTIFY (also-notify on the // primary) remains the fast path; these are the fallback. - return fmt.Sprintf(`$TTL 3600 + var b strings.Builder + fmt.Fprintf(&b, `$TTL 3600 @ IN SOA %s hostmaster.%s ( %d ; serial 300 ; refresh 60 ; retry 1209600 ; expire 60 ) ; minimum -@ IN NS %s -ns1 IN A %s -`, ns, origin, serial, ns, primaryIP) +`, ns[0], origin, serial) + for _, n := range ns { + fmt.Fprintf(&b, "@ IN NS %s\n", n) + } + for _, n := range ns { + if owner, ok := InZoneOwner(n, zone); ok { + fmt.Fprintf(&b, "%s IN A %s\n", owner, primaryIP) + } + } + return b.String() +} + +// InZoneOwner reports whether name sits inside zone, and if so returns its owner +// name relative to the apex ("@" for the apex itself). +func InZoneOwner(name, zone string) (string, bool) { + name, origin := dot(name), dot(zone) + switch { + case name == origin: + return "@", true + case strings.HasSuffix(name, "."+origin): + return strings.TrimSuffix(name, "."+origin), true + } + return "", false } // EnsureSeedZone makes path loadable without discarding live data: it probes // the zone file and journal, moves aside whatever cannot load, and writes a // skeleton only when there is nothing to preserve. Every caller that needs a // zone database file on disk goes through here. -func (e *Executor) EnsureSeedZone(ctx context.Context, namespace, pod, zone, path, primaryIP string) error { +func (e *Executor) EnsureSeedZone(ctx context.Context, namespace, pod, zone, path, primaryIP string, nameservers []string) error { state, err := e.ZoneDiskState(ctx, namespace, pod, path) if err != nil { return err @@ -67,7 +96,7 @@ func (e *Executor) EnsureSeedZone(ctx context.Context, namespace, pod, zone, pat if !plan.WriteSeed { return e.Quarantine(ctx, namespace, pod, path, plan) } - content := renderSeedZone(zone, primaryIP, plan.Serial) + content := renderSeedZone(zone, primaryIP, nameservers, plan.Serial) cmd := []string{"sh", "-c", seedScript(path, plan, len(content))} if out, err := e.Exec(ctx, namespace, pod, cmd, content); err != nil { return fmt.Errorf("seed zone %s: %w (out: %s)", zone, err, out) diff --git a/internal/bind/seed_nameservers_test.go b/internal/bind/seed_nameservers_test.go new file mode 100644 index 0000000..09639ea --- /dev/null +++ b/internal/bind/seed_nameservers_test.go @@ -0,0 +1,75 @@ +package bind + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// checkZone runs named-checkzone with full integrity checking, the same check +// named applies when loading a primary zone. Skips where the tool is absent. +func checkZone(t *testing.T, zone, content string) { + t.Helper() + bin, err := exec.LookPath("named-checkzone") + if err != nil { + t.Skip("named-checkzone not installed") + } + path := filepath.Join(t.TempDir(), "db") + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + if out, err := exec.Command(bin, "-i", "full", zone, path).CombinedOutput(); err != nil { + t.Fatalf("zone not loadable: %v\n%s\n%s", err, out, content) + } +} + +func TestRenderSeedZoneDeclaredNameservers(t *testing.T) { + got := renderSeedZone("acme.unkin.net", "10.42.6.38", []string{"ns1.unkin.net", "ns2.unkin.net."}, 7) + if strings.Contains(got, "10.42.6.38") { + t.Errorf("declared nameservers must not pull a pod IP into the zone:\n%s", got) + } + for _, want := range []string{"@ IN NS ns1.unkin.net.\n", "@ IN NS ns2.unkin.net.\n", "SOA ns1.unkin.net. hostmaster.acme.unkin.net."} { + if !strings.Contains(got, want) { + t.Errorf("missing %q in:\n%s", want, got) + } + } + checkZone(t, "acme.unkin.net", got) +} + +// An in-zone nameserver has no address anywhere else, so the seed must glue it +// or named refuses to load the zone. +func TestRenderSeedZoneInZoneNameserverGetsGlue(t *testing.T) { + got := renderSeedZone("example.com", "10.0.0.1", []string{"ns.example.com"}, 1) + if !strings.Contains(got, "ns IN A 10.0.0.1\n") { + t.Errorf("in-zone nameserver needs glue:\n%s", got) + } + checkZone(t, "example.com", got) +} + +func TestRenderSeedZoneFallbackLoads(t *testing.T) { + got := renderSeedZone("example.com", "10.0.0.1", nil, 1) + if !strings.Contains(got, "@ IN NS ns1.example.com.\n") || !strings.Contains(got, "ns1 IN A 10.0.0.1\n") { + t.Errorf("fallback seed changed shape:\n%s", got) + } + checkZone(t, "example.com", got) +} + +func TestInZoneOwner(t *testing.T) { + cases := []struct { + name, zone, owner string + in bool + }{ + {"ns1.example.com.", "example.com", "ns1", true}, + {"example.com", "example.com.", "@", true}, + {"ns1.unkin.net", "acme.unkin.net", "", false}, + {"notexample.com", "example.com", "", false}, + } + for _, c := range cases { + owner, in := InZoneOwner(c.name, c.zone) + if in != c.in || owner != c.owner { + t.Errorf("InZoneOwner(%q, %q) = %q, %v; want %q, %v", c.name, c.zone, owner, in, c.owner, c.in) + } + } +} diff --git a/internal/bind/seed_test.go b/internal/bind/seed_test.go index 71aaa9e..af77322 100644 --- a/internal/bind/seed_test.go +++ b/internal/bind/seed_test.go @@ -29,7 +29,7 @@ func inFlightZone(t *testing.T) (dir, path string) { t.Helper() dir = t.TempDir() path = filepath.Join(dir, "db.example.com") - if err := os.WriteFile(path, []byte(renderSeedZone("example.com", "10.0.0.1", 1)), 0o600); err != nil { + if err := os.WriteFile(path, []byte(renderSeedZone("example.com", "10.0.0.1", nil, 1)), 0o600); err != nil { t.Fatal(err) } if err := os.WriteFile(JournalPath(path), journalHeader(";BIND LOG V9.2\n", 10, 16), 0o600); err != nil { @@ -134,7 +134,7 @@ func TestSeedScriptInterruptedWriteLeavesDiskUntouched(t *testing.T) { if !plan.WriteSeed || !plan.QuarantineZoneFile || !plan.QuarantineJournal { t.Fatalf("expected a reseed over both files, got %+v", plan) } - content := renderSeedZone("example.com", "10.0.0.1", plan.Serial) + content := renderSeedZone("example.com", "10.0.0.1", nil, plan.Serial) if err := runSeedScript(t, sh, path, plan, content, content[:len(content)/2]); err == nil { t.Fatal("a truncated seed write must fail rather than install a torn zone file") @@ -165,7 +165,7 @@ func TestSeedScriptInstallsOverQuarantinedFiles(t *testing.T) { dir, path := inFlightZone(t) plan := PlanSeed(probeState(t, sh, path)) - content := renderSeedZone("example.com", "10.0.0.1", plan.Serial) + content := renderSeedZone("example.com", "10.0.0.1", nil, plan.Serial) if err := runSeedScript(t, sh, path, plan, content, content); err != nil { t.Fatalf("seed script: %v", err) } @@ -198,7 +198,7 @@ func TestSeedScriptFreshInstall(t *testing.T) { path := filepath.Join(t.TempDir(), "zones", "db.example.com") plan := PlanSeed(ZoneDiskState{}) - content := renderSeedZone("example.com", "10.0.0.1", plan.Serial) + content := renderSeedZone("example.com", "10.0.0.1", nil, plan.Serial) if err := runSeedScript(t, sh, path, plan, content, content); err != nil { t.Fatalf("seed script: %v", err) } @@ -225,7 +225,7 @@ func TestSeedScriptFailedQuarantineAbortsInstall(t *testing.T) { exec `+realTool(t, "mv")+` "$@"`) plan := PlanSeed(probeState(t, sh, path)) - content := renderSeedZone("example.com", "10.0.0.1", plan.Serial) + content := renderSeedZone("example.com", "10.0.0.1", nil, plan.Serial) if err := runSeedScriptWithPath(t, sh, path, plan, content, content, pathEnv); err == nil { t.Fatal("a failed quarantine must fail the seed") } @@ -240,7 +240,7 @@ func TestSeedScriptUnmeasurableStagingAbortsInstall(t *testing.T) { pathEnv := shimPath(t, "wc", "exit 127") plan := PlanSeed(probeState(t, sh, path)) - content := renderSeedZone("example.com", "10.0.0.1", plan.Serial) + content := renderSeedZone("example.com", "10.0.0.1", nil, plan.Serial) if err := runSeedScriptWithPath(t, sh, path, plan, content, content, pathEnv); err == nil { t.Fatal("an unmeasurable staging file must fail the seed") } @@ -256,7 +256,7 @@ func TestSeedScriptFailedMkdirAbortsInstall(t *testing.T) { pathEnv := shimPath(t, "mkdir", "exit 1") plan := PlanSeed(probeState(t, sh, path)) - content := renderSeedZone("example.com", "10.0.0.1", plan.Serial) + content := renderSeedZone("example.com", "10.0.0.1", nil, plan.Serial) if err := runSeedScriptWithPath(t, sh, path, plan, content, content, pathEnv); err == nil { t.Fatal("a failed mkdir must fail the seed") } diff --git a/internal/bind/zonestate_test.go b/internal/bind/zonestate_test.go index 5d78490..2d9de1a 100644 --- a/internal/bind/zonestate_test.go +++ b/internal/bind/zonestate_test.go @@ -48,7 +48,7 @@ k8s.syd1.au.unkin.net IN SOA ns1.k8s.syd1.au.unkin.net. hostmaster.k8s.syd1.au.u ok bool }{ {"bind dump", bindDump, 16, true}, - {"seed", renderSeedZone("example.com", "10.0.0.1", 42), 42, true}, + {"seed", renderSeedZone("example.com", "10.0.0.1", nil, 42), 42, true}, {"single line", "@ IN SOA ns1.example.com. hostmaster.example.com. 7 300 60 1209600 60\n", 7, true}, {"glued paren", "@ IN SOA ns. host. (9 300 60 1209600 60)\n", 9, true}, {"no soa", "$TTL 3600\nwww IN A 192.0.2.1\n", 0, false}, @@ -308,7 +308,7 @@ func TestPlanSeedBlocksOnUnreadableOrphanJournal(t *testing.T) { } func TestSeedZoneRoundTripsThroughParser(t *testing.T) { - content := renderSeedZone("200.18.198.in-addr.arpa", "198.18.200.8", 17) + content := renderSeedZone("200.18.198.in-addr.arpa", "198.18.200.8", nil, 17) got, ok := ParseZoneSerial(content) if !ok || got != 17 { t.Fatalf("seed zone serial = (%d,%v) want (17,true)", got, ok) @@ -418,12 +418,12 @@ func TestZoneStateProbeRoundTrip(t *testing.T) { {name: "fresh install"}, { name: "zone file only", - zone: renderSeedZone("example.com", "10.0.0.1", 42), + zone: renderSeedZone("example.com", "10.0.0.1", nil, 42), want: ZoneDiskState{ZoneFile: true, ZoneSerial: 42, ZoneSerialOK: true}, }, { name: "zone file and journal", - zone: renderSeedZone("example.com", "10.0.0.1", 12), + zone: renderSeedZone("example.com", "10.0.0.1", nil, 12), jnl: journalHeader(";BIND LOG V9.2\n", 10, 16), want: ZoneDiskState{ ZoneFile: true, ZoneSerial: 12, ZoneSerialOK: true, @@ -452,7 +452,7 @@ func TestZoneStateProbeRoundTrip(t *testing.T) { }, { name: "live zone beside old quarantine evidence", - zone: renderSeedZone("example.com", "10.0.0.1", 42), + zone: renderSeedZone("example.com", "10.0.0.1", nil, 42), orphans: []string{".orphaned-16"}, want: ZoneDiskState{ ZoneFile: true, ZoneSerial: 42, ZoneSerialOK: true, diff --git a/internal/controller/apex_ns_test.go b/internal/controller/apex_ns_test.go new file mode 100644 index 0000000..9495453 --- /dev/null +++ b/internal/controller/apex_ns_test.go @@ -0,0 +1,98 @@ +package controller + +import ( + "testing" + + bindv1alpha1 "git.unkin.net/unkin/bind-operator/api/v1alpha1" + "git.unkin.net/unkin/bind-operator/internal/bind" +) + +func zoneWith(spec bindv1alpha1.BindZoneSpec) *bindv1alpha1.BindZone { + spec.ZoneName = "acme.unkin.net" + return &bindv1alpha1.BindZone{Spec: spec} +} + +func TestZoneNameserversFallbackIsOutOfZone(t *testing.T) { + cluster := &bindv1alpha1.BindCluster{} + cluster.Name, cluster.Namespace = "auth", "bind-internal" + got := zoneNameservers(nil, cluster) + want := "auth-0.auth-headless.bind-internal.svc.cluster.local." + if len(got) != 1 || got[0] != want { + t.Fatalf("fallback = %v; want [%s]", got, want) + } + if got := zoneNameservers([]string{"ns1.unkin.net."}, cluster); got[0] != "ns1.unkin.net." { + t.Fatalf("declared nameservers must win, got %v", got) + } +} + +func TestApexNSUpdatesReplacesRRsetAndDropsGlue(t *testing.T) { + zone := zoneWith(bindv1alpha1.BindZoneSpec{ + Nameservers: []string{"ns1.unkin.net", "ns2.unkin.net."}, + DefaultTTL: 300, + }) + got := apexNSUpdates(zone, zone.Spec.Nameservers) + want := []bind.RecordUpdate{ + {FQDN: "acme.unkin.net.", Type: "NS", TTL: 300, Values: []string{"ns1.unkin.net.", "ns2.unkin.net."}}, + {FQDN: "ns1.acme.unkin.net.", Type: "A", Delete: true}, + } + assertUpdates(t, got, want) +} + +// Without declared nameservers the apex NS still converges onto the stable +// primary name, but the ns1 glue is left alone: it may be a real record. +func TestApexNSUpdatesFallbackKeepsGlue(t *testing.T) { + zone := zoneWith(bindv1alpha1.BindZoneSpec{}) + got := apexNSUpdates(zone, []string{"auth-0.auth-headless.bind-internal.svc.cluster.local."}) + want := []bind.RecordUpdate{{ + FQDN: "acme.unkin.net.", + Type: "NS", + TTL: 3600, + Values: []string{"auth-0.auth-headless.bind-internal.svc.cluster.local."}, + }} + assertUpdates(t, got, want) +} + +// spec.records is applied after the apex sync, so anything it owns must not be +// touched here: the ops would be undone and the serial would churn every pass. +func TestApexNSUpdatesYieldsToRecords(t *testing.T) { + zone := zoneWith(bindv1alpha1.BindZoneSpec{ + Nameservers: []string{"ns1.unkin.net."}, + Records: []bindv1alpha1.Record{ + {Name: "@", Type: "ns", Values: []string{"ns.other.net."}}, + {Name: "ns1", Type: "A", Values: []string{"10.0.0.53"}}, + }, + }) + if got := apexNSUpdates(zone, zone.Spec.Nameservers); len(got) != 0 { + t.Fatalf("expected no updates, got %+v", got) + } +} + +// A declared in-zone nameserver owns the ns1 record; it is glue, not a leftover. +func TestApexNSUpdatesKeepsDeclaredNs1Glue(t *testing.T) { + zone := zoneWith(bindv1alpha1.BindZoneSpec{Nameservers: []string{"ns1.acme.unkin.net"}}) + got := apexNSUpdates(zone, zone.Spec.Nameservers) + want := []bind.RecordUpdate{{FQDN: "acme.unkin.net.", Type: "NS", TTL: 3600, Values: []string{"ns1.acme.unkin.net."}}} + assertUpdates(t, got, want) +} + +func assertUpdates(t *testing.T, got, want []bind.RecordUpdate) { + t.Helper() + if len(got) != len(want) { + t.Fatalf("got %d updates %+v; want %d %+v", len(got), got, len(want), want) + } + for i := range want { + if got[i].FQDN != want[i].FQDN || got[i].Type != want[i].Type || got[i].TTL != want[i].TTL || got[i].Delete != want[i].Delete { + t.Errorf("update %d = %+v; want %+v", i, got[i], want[i]) + continue + } + if len(got[i].Values) != len(want[i].Values) { + t.Errorf("update %d values = %v; want %v", i, got[i].Values, want[i].Values) + continue + } + for j := range want[i].Values { + if got[i].Values[j] != want[i].Values[j] { + t.Errorf("update %d value %d = %q; want %q", i, j, got[i].Values[j], want[i].Values[j]) + } + } + } +} diff --git a/internal/controller/bindcatalogzone_controller.go b/internal/controller/bindcatalogzone_controller.go index 28c9d98..64cfdfe 100644 --- a/internal/controller/bindcatalogzone_controller.go +++ b/internal/controller/bindcatalogzone_controller.go @@ -54,7 +54,7 @@ func (r *BindCatalogZoneReconciler) Reconcile(ctx context.Context, req ctrl.Requ if primaryIP == "" { return r.fail(ctx, &catalog, "PrimaryNoIP", "waiting for primary pod IP") } - if err := r.Exec.EnsureSeedZone(ctx, catalog.Namespace, primaryPod, catalog.Spec.ZoneName, bind.CatalogFilePath(catalog.Spec.ZoneName), primaryIP); err != nil { + if err := r.Exec.EnsureSeedZone(ctx, catalog.Namespace, primaryPod, catalog.Spec.ZoneName, bind.CatalogFilePath(catalog.Spec.ZoneName), primaryIP, zoneNameservers(nil, cluster)); err != nil { return r.fail(ctx, &catalog, "SeedFailed", err.Error()) } } diff --git a/internal/controller/bindpolicy_controller.go b/internal/controller/bindpolicy_controller.go index 33c15fc..76b7e8c 100644 --- a/internal/controller/bindpolicy_controller.go +++ b/internal/controller/bindpolicy_controller.go @@ -67,7 +67,7 @@ func (r *BindPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) if primaryIP == "" { return r.fail(ctx, &policy, "PrimaryNoIP", "waiting for primary pod IP") } - if err := r.Exec.EnsureSeedZone(ctx, policy.Namespace, primaryPod, policy.Spec.ZoneName, bind.ZoneFilePath(policy.Spec.ZoneName), primaryIP); err != nil { + if err := r.Exec.EnsureSeedZone(ctx, policy.Namespace, primaryPod, policy.Spec.ZoneName, bind.ZoneFilePath(policy.Spec.ZoneName), primaryIP, zoneNameservers(nil, cluster)); err != nil { return r.fail(ctx, &policy, "SeedFailed", err.Error()) } } diff --git a/internal/controller/bindzone_controller.go b/internal/controller/bindzone_controller.go index f326ff8..d9cb9ad 100644 --- a/internal/controller/bindzone_controller.go +++ b/internal/controller/bindzone_controller.go @@ -99,6 +99,8 @@ func (r *BindZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return r.setPhase(ctx, &zone, "Error", "ConfigError", err.Error()) } + nameservers := zoneNameservers(zone.Spec.Nameservers, 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 == "") { primaryIP := primaryPodIP(ctx, r.Client, cluster) @@ -108,7 +110,7 @@ func (r *BindZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c // The zone is absent from named's memory, but its database file and // journal may still be on the PVC from a previous incarnation. path := bind.ZoneFilePath(zone.Spec.ZoneName) - if err := r.Exec.EnsureSeedZone(ctx, zone.Namespace, primaryPod, zone.Spec.ZoneName, path, primaryIP); err != nil { + if err := r.Exec.EnsureSeedZone(ctx, zone.Namespace, primaryPod, zone.Spec.ZoneName, path, primaryIP, nameservers); err != nil { return r.setPhase(ctx, &zone, "Error", "SeedFailed", err.Error()) } } @@ -116,18 +118,29 @@ func (r *BindZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return r.setPhase(ctx, &zone, "Error", "AddZoneFailed", err.Error()) } - // Seed static records (primary zones only). + // Sync the apex NS on every pass, not only at seed time, so an existing zone + // converges off the seed placeholder. Best-effort: a zone that permits no + // dynamic update keeps what it was seeded with rather than failing to + // reconcile. recordCount := 0 - if isPrimaryType(zone.Spec.Type) && len(zone.Spec.Records) > 0 { - creds, err := r.zoneUpdateCreds(ctx, &zone) - if err != nil { - return r.setPhase(ctx, &zone, "Error", "NoUpdateKey", err.Error()) + if isPrimaryType(zone.Spec.Type) { + creds, credErr := r.zoneUpdateCreds(ctx, &zone) + if apex := apexNSUpdates(&zone, nameservers); len(apex) > 0 && credErr == nil { + if err := r.Exec.NSUpdate(ctx, zone.Namespace, primaryPod, zone.Spec.ZoneName, creds, apex); err != nil { + logger.V(1).Info("apex NS sync failed", "zone", zone.Spec.ZoneName, "err", err.Error()) + } } - updates := recordsToUpdates(zone.Spec.ZoneName, zone.Spec.Records, zone.Spec.DefaultTTL) - 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()) + // Seed static records. + if len(zone.Spec.Records) > 0 { + if credErr != nil { + return r.setPhase(ctx, &zone, "Error", "NoUpdateKey", credErr.Error()) + } + updates := recordsToUpdates(zone.Spec.ZoneName, zone.Spec.Records, zone.Spec.DefaultTTL) + 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) } - 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 7af8c5f..074a469 100644 --- a/internal/controller/zone_helpers.go +++ b/internal/controller/zone_helpers.go @@ -114,3 +114,58 @@ func alsoNotifyList(addrs []string, key string) string { } return strings.Join(parts, " ") } + +// absolute qualifies a nameserver name. Unlike record owner names, a +// spec.nameservers entry is always a full domain name, never relative to the +// 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: the +// declared nameservers, else the primary's stable in-cluster DNS name. The +// fallback is deliberately out-of-zone, so no pod IP is needed as glue. +func zoneNameservers(declared []string, cluster *bindv1alpha1.BindCluster) []string { + if len(declared) > 0 { + return declared + } + return []string{primaryAddress(cluster.Name, cluster.Namespace) + "."} +} + +// apexNSUpdates returns the dynamic-update ops that keep a zone's apex NS RRset +// equal to nameservers, plus removal of the seed's ns1 glue once the zone +// declares its own nameservers. Ops colliding with a spec.records entry are +// dropped: records are applied afterwards and would re-add them, and the churn +// would bump the serial on every reconcile. +func apexNSUpdates(zone *bindv1alpha1.BindZone, nameservers []string) []bind.RecordUpdate { + owns := func(name, typ string) bool { + for _, rec := range zone.Spec.Records { + if strings.EqualFold(rec.Type, typ) && fqdn(rec.Name, zone.Spec.ZoneName) == fqdn(name, zone.Spec.ZoneName) { + return true + } + } + return false + } + ttl := zone.Spec.DefaultTTL + if ttl <= 0 { + ttl = 3600 + } + var updates []bind.RecordUpdate + if !owns("@", "NS") { + values := make([]string, 0, len(nameservers)) + for _, ns := range nameservers { + values = append(values, absolute(ns)) + } + updates = append(updates, bind.RecordUpdate{FQDN: fqdn("@", zone.Spec.ZoneName), Type: "NS", TTL: ttl, Values: values}) + } + // The seed's placeholder glue pins a pod IP that goes stale on the first + // reschedule; drop it once the zone names its real nameservers. + if glue := fqdn("ns1", zone.Spec.ZoneName); len(zone.Spec.Nameservers) > 0 && !owns("ns1", "A") { + published := false + for _, ns := range nameservers { + published = published || absolute(ns) == glue + } + if !published { + updates = append(updates, bind.RecordUpdate{FQDN: glue, Type: "A", Delete: true}) + } + } + return updates +} -- 2.47.3 From e4ed6c8052ca6dfd64e02824a7e8fd2a5f755256 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 26 Sep 2026 18:50:55 +1000 Subject: [PATCH 2/5] converge apex NS per record, not by RRset replace BIND ignores an RRset-wide delete of apex NS, so the previous replace only appended to the seed placeholder. --- internal/bind/consts.go | 1 + internal/bind/nsupdate.go | 54 ++++-- internal/bind/nsupdate_test.go | 42 +++++ internal/bind/rndc.go | 18 ++ internal/controller/apex_ns_test.go | 159 +++++++++++------- .../controller/bindcatalogzone_controller.go | 2 +- internal/controller/bindpolicy_controller.go | 2 +- internal/controller/bindzone_controller.go | 35 ++-- internal/controller/zone_helpers.go | 121 +++++++++---- 9 files changed, 304 insertions(+), 130 deletions(-) create mode 100644 internal/bind/nsupdate_test.go diff --git a/internal/bind/consts.go b/internal/bind/consts.go index 7f2e383..b574fc0 100644 --- a/internal/bind/consts.go +++ b/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. diff --git a/internal/bind/nsupdate.go b/internal/bind/nsupdate.go index 5fd4795..76e1be4 100644 --- a/internal/bind/nsupdate.go +++ b/internal/bind/nsupdate.go @@ -19,35 +19,55 @@ type RecordUpdate struct { Type string // RR type TTL int32 // record TTL Values []string // RDATA entries - Delete bool // when true, delete the RRset instead of replacing it + Delete bool // when true, delete instead of add + // PerValue operates on individual records rather than the whole RRset: adds + // leave existing records in place, deletes remove only the listed Values. + // Required at a zone apex, where BIND silently ignores an RRset-wide delete + // of NS or SOA and would turn a replace into an append. + PerValue bool } // NSUpdate applies a set of record changes to zone by executing nsupdate on the // primary pod, targeting the local server and authenticating with creds. All // changes are sent in a single atomic transaction. func (e *Executor) NSUpdate(ctx context.Context, namespace, pod, zone string, creds TSIGCreds, updates []RecordUpdate) error { - var b strings.Builder - b.WriteString("server 127.0.0.1\n") - b.WriteString(fmt.Sprintf("zone %s\n", dot(zone))) - for _, u := range updates { - // Replace semantics: clear the RRset first, then add the desired values. - b.WriteString(fmt.Sprintf("update delete %s %s\n", dot(u.FQDN), u.Type)) - if u.Delete { - continue - } - for _, v := range u.Values { - b.WriteString(fmt.Sprintf("update add %s %d %s %s\n", dot(u.FQDN), u.TTL, u.Type, v)) - } - } - b.WriteString("send\n") - cmd := []string{NsupdateBin, "-y", fmt.Sprintf("%s:%s:%s", creds.Algorithm, creds.Name, creds.Secret)} - if out, err := e.Exec(ctx, namespace, pod, cmd, b.String()); err != nil { + if out, err := e.Exec(ctx, namespace, pod, cmd, nsupdateScript(zone, updates)); err != nil { return fmt.Errorf("nsupdate zone %s: %w (out: %s)", zone, err, out) } return nil } +// nsupdateScript renders the nsupdate input for a set of changes. +func nsupdateScript(zone string, updates []RecordUpdate) string { + var b strings.Builder + b.WriteString("server 127.0.0.1\n") + fmt.Fprintf(&b, "zone %s\n", dot(zone)) + for _, u := range updates { + switch { + case u.PerValue && u.Delete: + for _, v := range u.Values { + fmt.Fprintf(&b, "update delete %s %s %s\n", dot(u.FQDN), u.Type, v) + } + case u.PerValue: + for _, v := range u.Values { + fmt.Fprintf(&b, "update add %s %d %s %s\n", dot(u.FQDN), u.TTL, u.Type, v) + } + default: + // Replace semantics: clear the RRset first, then add the values. + fmt.Fprintf(&b, "update delete %s %s\n", dot(u.FQDN), u.Type) + if u.Delete { + continue + } + for _, v := range u.Values { + fmt.Fprintf(&b, "update add %s %d %s %s\n", dot(u.FQDN), u.TTL, u.Type, v) + } + } + } + b.WriteString("send\n") + return b.String() +} + // dot ensures a name is fully qualified with a trailing dot. func dot(name string) string { if name == "" || name == "@" { diff --git a/internal/bind/nsupdate_test.go b/internal/bind/nsupdate_test.go new file mode 100644 index 0000000..e9ba75a --- /dev/null +++ b/internal/bind/nsupdate_test.go @@ -0,0 +1,42 @@ +package bind + +import "testing" + +func TestNSUpdateScriptReplaceSemantics(t *testing.T) { + got := nsupdateScript("acme.unkin.net", []RecordUpdate{ + {FQDN: "www", Type: "A", TTL: 60, Values: []string{"10.0.0.1", "10.0.0.2"}}, + {FQDN: "old.acme.unkin.net.", Type: "TXT", Delete: true}, + }) + want := `server 127.0.0.1 +zone acme.unkin.net. +update delete www. A +update add www. 60 A 10.0.0.1 +update add www. 60 A 10.0.0.2 +update delete old.acme.unkin.net. TXT +send +` + if got != want { + t.Errorf("got:\n%s\nwant:\n%s", got, want) + } +} + +// At the apex BIND ignores an RRset-wide delete of NS, so the apex sync must add +// the new names and delete the old ones record by record, adds first: named +// refuses to leave an apex with no NS record. +func TestNSUpdateScriptPerValueApexNS(t *testing.T) { + got := nsupdateScript("acme.unkin.net", []RecordUpdate{ + {FQDN: "acme.unkin.net.", Type: "NS", TTL: 60, Values: []string{"acme-ns1.unkin.net."}, PerValue: true}, + {FQDN: "acme.unkin.net.", Type: "NS", Values: []string{"ns1.acme.unkin.net."}, PerValue: true, Delete: true}, + {FQDN: "ns1.acme.unkin.net.", Type: "A", Delete: true}, + }) + want := `server 127.0.0.1 +zone acme.unkin.net. +update add acme.unkin.net. 60 NS acme-ns1.unkin.net. +update delete acme.unkin.net. NS ns1.acme.unkin.net. +update delete ns1.acme.unkin.net. A +send +` + if got != want { + t.Errorf("got:\n%s\nwant:\n%s", got, want) + } +} diff --git a/internal/bind/rndc.go b/internal/bind/rndc.go index ba05f04..41149e5 100644 --- a/internal/bind/rndc.go +++ b/internal/bind/rndc.go @@ -84,3 +84,21 @@ 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 9495453..4d6f426 100644 --- a/internal/controller/apex_ns_test.go +++ b/internal/controller/apex_ns_test.go @@ -1,6 +1,7 @@ package controller import ( + "strings" "testing" bindv1alpha1 "git.unkin.net/unkin/bind-operator/api/v1alpha1" @@ -12,67 +13,108 @@ func zoneWith(spec bindv1alpha1.BindZoneSpec) *bindv1alpha1.BindZone { return &bindv1alpha1.BindZone{Spec: spec} } -func TestZoneNameserversFallbackIsOutOfZone(t *testing.T) { - cluster := &bindv1alpha1.BindCluster{} - cluster.Name, cluster.Namespace = "auth", "bind-internal" - got := zoneNameservers(nil, cluster) - want := "auth-0.auth-headless.bind-internal.svc.cluster.local." - if len(got) != 1 || got[0] != want { - t.Fatalf("fallback = %v; want [%s]", got, want) +func testCluster() *bindv1alpha1.BindCluster { + c := &bindv1alpha1.BindCluster{} + c.Name, c.Namespace = "auth", "bind-internal" + return c +} + +const stableNS = "auth-0.auth-headless.bind-internal.svc.cluster.local." + +func TestZoneNameservers(t *testing.T) { + cases := []struct { + name string + spec bindv1alpha1.BindZoneSpec + want []string + 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}, + // 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}, + {"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}, } - if got := zoneNameservers([]string{"ns1.unkin.net."}, cluster); got[0] != "ns1.unkin.net." { - t.Fatalf("declared nameservers must win, got %v", got) + 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) + } } } -func TestApexNSUpdatesReplacesRRsetAndDropsGlue(t *testing.T) { - zone := zoneWith(bindv1alpha1.BindZoneSpec{ - Nameservers: []string{"ns1.unkin.net", "ns2.unkin.net."}, - DefaultTTL: 300, - }) - got := apexNSUpdates(zone, zone.Spec.Nameservers) - want := []bind.RecordUpdate{ - {FQDN: "acme.unkin.net.", Type: "NS", TTL: 300, Values: []string{"ns1.unkin.net.", "ns2.unkin.net."}}, +// 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. +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) + assertUpdates(t, got, []bind.RecordUpdate{ + {FQDN: "acme.unkin.net.", Type: "NS", TTL: 60, Values: []string{"ns1.unkin.net."}, PerValue: true}, + {FQDN: "acme.unkin.net.", Type: "NS", Values: []string{"ns1.acme.unkin.net."}, PerValue: true, Delete: true}, {FQDN: "ns1.acme.unkin.net.", Type: "A", Delete: true}, - } - assertUpdates(t, got, want) -} - -// Without declared nameservers the apex NS still converges onto the stable -// primary name, but the ns1 glue is left alone: it may be a real record. -func TestApexNSUpdatesFallbackKeepsGlue(t *testing.T) { - zone := zoneWith(bindv1alpha1.BindZoneSpec{}) - got := apexNSUpdates(zone, []string{"auth-0.auth-headless.bind-internal.svc.cluster.local."}) - want := []bind.RecordUpdate{{ - FQDN: "acme.unkin.net.", - Type: "NS", - TTL: 3600, - Values: []string{"auth-0.auth-headless.bind-internal.svc.cluster.local."}, - }} - assertUpdates(t, got, want) -} - -// spec.records is applied after the apex sync, so anything it owns must not be -// touched here: the ops would be undone and the serial would churn every pass. -func TestApexNSUpdatesYieldsToRecords(t *testing.T) { - zone := zoneWith(bindv1alpha1.BindZoneSpec{ - Nameservers: []string{"ns1.unkin.net."}, - Records: []bindv1alpha1.Record{ - {Name: "@", Type: "ns", Values: []string{"ns.other.net."}}, - {Name: "ns1", Type: "A", Values: []string{"10.0.0.53"}}, - }, }) - if got := apexNSUpdates(zone, zone.Spec.Nameservers); len(got) != 0 { +} + +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. + if got := apexNSUpdates(zone, []string{"ns1.unkin.net."}, []string{"NS1.Unkin.Net."}, 3600); len(got) != 0 { t.Fatalf("expected no updates, got %+v", got) } } -// A declared in-zone nameserver owns the ns1 record; it is glue, not a leftover. -func TestApexNSUpdatesKeepsDeclaredNs1Glue(t *testing.T) { - zone := zoneWith(bindv1alpha1.BindZoneSpec{Nameservers: []string{"ns1.acme.unkin.net"}}) - got := apexNSUpdates(zone, zone.Spec.Nameservers) - want := []bind.RecordUpdate{{FQDN: "acme.unkin.net.", Type: "NS", TTL: 3600, Values: []string{"ns1.acme.unkin.net."}}} - assertUpdates(t, got, want) +// Changing the declared set replaces only what changed, keeping the overlap. +func TestApexNSUpdatesPartialChange(t *testing.T) { + zone := zoneWith(bindv1alpha1.BindZoneSpec{Nameservers: []string{"a.ns.unkin.net.", "c.ns.unkin.net."}}) + got := apexNSUpdates(zone, []string{"a.ns.unkin.net.", "c.ns.unkin.net."}, []string{"a.ns.unkin.net.", "b.ns.unkin.net."}, 3600) + assertUpdates(t, got, []bind.RecordUpdate{ + {FQDN: "acme.unkin.net.", Type: "NS", TTL: 3600, Values: []string{"c.ns.unkin.net."}, PerValue: true}, + {FQDN: "acme.unkin.net.", Type: "NS", Values: []string{"b.ns.unkin.net."}, PerValue: true, Delete: true}, + }) +} + +// A declared in-zone ns1 owns the 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."}}) + if got := apexNSUpdates(zone, []string{"ns1.acme.unkin.net."}, []string{"ns1.acme.unkin.net."}, 3600); len(got) != 0 { + t.Fatalf("expected no updates, got %+v", got) + } +} + +// spec.records owning the ns1 address means the glue is real data, not the seed +// placeholder. +func TestApexNSUpdatesLeavesRecordOwnedGlue(t *testing.T) { + zone := zoneWith(bindv1alpha1.BindZoneSpec{ + Nameservers: []string{"ns1.unkin.net."}, + Records: []bindv1alpha1.Record{{Name: "ns1", Type: "a", Values: []string{"10.0.0.53"}}}, + }) + got := apexNSUpdates(zone, []string{"ns1.unkin.net."}, []string{"ns1.acme.unkin.net."}, 3600) + assertUpdates(t, got, []bind.RecordUpdate{ + {FQDN: "acme.unkin.net.", Type: "NS", TTL: 3600, Values: []string{"ns1.unkin.net."}, PerValue: true}, + {FQDN: "acme.unkin.net.", Type: "NS", Values: []string{"ns1.acme.unkin.net."}, PerValue: true, Delete: true}, + }) +} + +// The apex NS is converged by the apex path, so it must not also be emitted as a +// record: an RRset-wide delete there is ignored and the add would append. +func TestRecordsToUpdatesSkipsApexNS(t *testing.T) { + records := []bindv1alpha1.Record{ + {Name: "@", Type: "NS", Values: []string{"a.ns.unkin.net."}}, + {Name: "sub", Type: "NS", Values: []string{"d.ns.unkin.net."}}, + {Name: "@", Type: "MX", Values: []string{"10 mail.unkin.net."}}, + } + got := recordsToUpdates("acme.unkin.net", records, 3600) + if len(got) != 2 || got[0].FQDN != "sub.acme.unkin.net." || got[1].Type != "MX" { + t.Fatalf("got %+v", got) + } } func assertUpdates(t *testing.T, got, want []bind.RecordUpdate) { @@ -81,18 +123,13 @@ func assertUpdates(t *testing.T, got, want []bind.RecordUpdate) { t.Fatalf("got %d updates %+v; want %d %+v", len(got), got, len(want), want) } for i := range want { - if got[i].FQDN != want[i].FQDN || got[i].Type != want[i].Type || got[i].TTL != want[i].TTL || got[i].Delete != want[i].Delete { - t.Errorf("update %d = %+v; want %+v", i, got[i], want[i]) + g, w := got[i], want[i] + if g.FQDN != w.FQDN || g.Type != w.Type || g.TTL != w.TTL || g.Delete != w.Delete || g.PerValue != w.PerValue { + t.Errorf("update %d = %+v; want %+v", i, g, w) continue } - if len(got[i].Values) != len(want[i].Values) { - t.Errorf("update %d values = %v; want %v", i, got[i].Values, want[i].Values) - continue - } - for j := range want[i].Values { - if got[i].Values[j] != want[i].Values[j] { - t.Errorf("update %d value %d = %q; want %q", i, j, got[i].Values[j], want[i].Values[j]) - } + if strings.Join(g.Values, ",") != strings.Join(w.Values, ",") { + t.Errorf("update %d values = %v; want %v", i, g.Values, w.Values) } } } diff --git a/internal/controller/bindcatalogzone_controller.go b/internal/controller/bindcatalogzone_controller.go index 64cfdfe..5edd904 100644 --- a/internal/controller/bindcatalogzone_controller.go +++ b/internal/controller/bindcatalogzone_controller.go @@ -54,7 +54,7 @@ func (r *BindCatalogZoneReconciler) Reconcile(ctx context.Context, req ctrl.Requ if primaryIP == "" { return r.fail(ctx, &catalog, "PrimaryNoIP", "waiting for primary pod IP") } - if err := r.Exec.EnsureSeedZone(ctx, catalog.Namespace, primaryPod, catalog.Spec.ZoneName, bind.CatalogFilePath(catalog.Spec.ZoneName), primaryIP, zoneNameservers(nil, cluster)); err != nil { + if err := r.Exec.EnsureSeedZone(ctx, catalog.Namespace, primaryPod, catalog.Spec.ZoneName, bind.CatalogFilePath(catalog.Spec.ZoneName), primaryIP, clusterNameservers(cluster)); err != nil { return r.fail(ctx, &catalog, "SeedFailed", err.Error()) } } diff --git a/internal/controller/bindpolicy_controller.go b/internal/controller/bindpolicy_controller.go index 76b7e8c..e0f51b0 100644 --- a/internal/controller/bindpolicy_controller.go +++ b/internal/controller/bindpolicy_controller.go @@ -67,7 +67,7 @@ func (r *BindPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) if primaryIP == "" { return r.fail(ctx, &policy, "PrimaryNoIP", "waiting for primary pod IP") } - if err := r.Exec.EnsureSeedZone(ctx, policy.Namespace, primaryPod, policy.Spec.ZoneName, bind.ZoneFilePath(policy.Spec.ZoneName), primaryIP, zoneNameservers(nil, cluster)); err != nil { + if err := r.Exec.EnsureSeedZone(ctx, policy.Namespace, primaryPod, policy.Spec.ZoneName, bind.ZoneFilePath(policy.Spec.ZoneName), primaryIP, clusterNameservers(cluster)); err != nil { return r.fail(ctx, &policy, "SeedFailed", err.Error()) } } diff --git a/internal/controller/bindzone_controller.go b/internal/controller/bindzone_controller.go index d9cb9ad..17c76dc 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 := zoneNameservers(zone.Spec.Nameservers, cluster) + nameservers, 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 == "") { @@ -118,24 +118,33 @@ func (r *BindZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return r.setPhase(ctx, &zone, "Error", "AddZoneFailed", err.Error()) } - // Sync the apex NS on every pass, not only at seed time, so an existing zone - // converges off the seed placeholder. Best-effort: a zone that permits no - // dynamic update keeps what it was seeded with rather than failing to - // reconcile. + // 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. recordCount := 0 if isPrimaryType(zone.Spec.Type) { creds, credErr := r.zoneUpdateCreds(ctx, &zone) - if apex := apexNSUpdates(&zone, nameservers); len(apex) > 0 && credErr == nil { - if err := r.Exec.NSUpdate(ctx, zone.Namespace, primaryPod, zone.Spec.ZoneName, creds, apex); err != nil { - logger.V(1).Info("apex NS sync failed", "zone", zone.Spec.ZoneName, "err", err.Error()) - } - } - // Seed static records. - if len(zone.Spec.Records) > 0 { + if nsDeclared { + 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 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) + } + } + // 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()) } - updates := recordsToUpdates(zone.Spec.ZoneName, zone.Spec.Records, zone.Spec.DefaultTTL) 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()) } diff --git a/internal/controller/zone_helpers.go b/internal/controller/zone_helpers.go index 074a469..11a7c50 100644 --- a/internal/controller/zone_helpers.go +++ b/internal/controller/zone_helpers.go @@ -42,6 +42,11 @@ func fqdn(name, zone string) string { func recordsToUpdates(zone string, records []bindv1alpha1.Record, defaultTTL int32) []bind.RecordUpdate { updates := make([]bind.RecordUpdate, 0, len(records)) for _, rec := range records { + // The apex NS RRset is converged by apexNSUpdates: an RRset-wide delete + // here is ignored by BIND and would only append to the live set. + if strings.EqualFold(rec.Type, "NS") && fqdn(rec.Name, zone) == fqdn("@", zone) { + continue + } ttl := defaultTTL if rec.TTL != nil { ttl = *rec.TTL @@ -120,52 +125,94 @@ 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: the -// declared nameservers, else the primary's stable in-cluster DNS name. The -// fallback is deliberately out-of-zone, so no pod IP is needed as glue. -func zoneNameservers(declared []string, cluster *bindv1alpha1.BindCluster) []string { - if len(declared) > 0 { - return declared +// 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) { + for _, ns := range zone.Spec.Nameservers { + names = append(names, absolute(ns)) } - return []string{primaryAddress(cluster.Name, cluster.Namespace) + "."} -} - -// apexNSUpdates returns the dynamic-update ops that keep a zone's apex NS RRset -// equal to nameservers, plus removal of the seed's ns1 glue once the zone -// declares its own nameservers. Ops colliding with a spec.records entry are -// dropped: records are applied afterwards and would re-add them, and the churn -// would bump the serial on every reconcile. -func apexNSUpdates(zone *bindv1alpha1.BindZone, nameservers []string) []bind.RecordUpdate { - owns := func(name, typ string) bool { - for _, rec := range zone.Spec.Records { - if strings.EqualFold(rec.Type, typ) && fqdn(rec.Name, zone.Spec.ZoneName) == fqdn(name, zone.Spec.ZoneName) { - return true + 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)) } } - return false } - ttl := zone.Spec.DefaultTTL + 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 } + apex := fqdn("@", zone.Spec.ZoneName) + add := missing(desired, live) + del := missing(live, desired) + var updates []bind.RecordUpdate - if !owns("@", "NS") { - values := make([]string, 0, len(nameservers)) - for _, ns := range nameservers { - values = append(values, absolute(ns)) - } - updates = append(updates, bind.RecordUpdate{FQDN: fqdn("@", zone.Spec.ZoneName), Type: "NS", TTL: ttl, Values: values}) + if len(add) > 0 { + updates = append(updates, bind.RecordUpdate{FQDN: apex, Type: "NS", TTL: ttl, Values: add, PerValue: true}) } - // The seed's placeholder glue pins a pod IP that goes stale on the first - // reschedule; drop it once the zone names its real nameservers. - if glue := fqdn("ns1", zone.Spec.ZoneName); len(zone.Spec.Nameservers) > 0 && !owns("ns1", "A") { - published := false - for _, ns := range nameservers { - published = published || absolute(ns) == glue - } - if !published { - updates = append(updates, bind.RecordUpdate{FQDN: glue, Type: "A", Delete: true}) - } + if len(del) > 0 { + 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}) } return updates } + +// missing returns the names in want that have no case-insensitive match in have. +func missing(want, have []string) (out []string) { + for _, w := range want { + if !containsName(have, w) { + out = append(out, w) + } + } + return out +} + +func containsName(names []string, name string) bool { + for _, n := range names { + if strings.EqualFold(absolute(n), absolute(name)) { + return true + } + } + return false +} + +// recordsOwn reports whether spec.records already manages an owner/type pair, in +// which case the apex sync must leave it alone. +func recordsOwn(zone *bindv1alpha1.BindZone, name, typ string) bool { + for _, rec := range zone.Spec.Records { + if strings.EqualFold(rec.Type, typ) && strings.EqualFold(fqdn(rec.Name, zone.Spec.ZoneName), fqdn(name, zone.Spec.ZoneName)) { + return true + } + } + 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) + "."} +} -- 2.47.3 From dc57ac1b2d380d34862899f8c13f45f9cce6e54e Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 26 Sep 2026 19:06:47 +1000 Subject: [PATCH 3/5] 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) + "."} -} -- 2.47.3 From 08d46ccce097def689153d566df19b5ce39adfd7 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 26 Sep 2026 19:25:06 +1000 Subject: [PATCH 4/5] 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 | 10 +++--- api/v1alpha1/zz_generated.deepcopy.go | 5 --- .../crd/bases/bind.unkin.net_bindzones.yaml | 13 +++----- config/crd/install.yaml | 13 +++----- internal/bind/consts.go | 1 + internal/bind/nsupdate.go | 25 ++++++++++++++ internal/controller/apex_ns_test.go | 22 +++++-------- internal/controller/bindzone_controller.go | 10 ++++-- internal/controller/zone_helpers.go | 33 +++++++------------ 9 files changed, 67 insertions(+), 65 deletions(-) diff --git a/api/v1alpha1/bindzone_types.go b/api/v1alpha1/bindzone_types.go index cd76bed..e7925b3 100644 --- a/api/v1alpha1/bindzone_types.go +++ b/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"` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index e8669f8..c9cb4cc 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/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)) diff --git a/config/crd/bases/bind.unkin.net_bindzones.yaml b/config/crd/bases/bind.unkin.net_bindzones.yaml index e07785a..11e193c 100644 --- a/config/crd/bases/bind.unkin.net_bindzones.yaml +++ b/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 diff --git a/config/crd/install.yaml b/config/crd/install.yaml index 05618d3..74507d8 100644 --- a/config/crd/install.yaml +++ b/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 diff --git a/internal/bind/consts.go b/internal/bind/consts.go index 7f2e383..b574fc0 100644 --- a/internal/bind/consts.go +++ b/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. diff --git a/internal/bind/nsupdate.go b/internal/bind/nsupdate.go index 76e1be4..8611696 100644 --- a/internal/bind/nsupdate.go +++ b/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 diff --git a/internal/controller/apex_ns_test.go b/internal/controller/apex_ns_test.go index 9fbdc01..25aee2b 100644 --- a/internal/controller/apex_ns_test.go +++ b/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 diff --git a/internal/controller/bindzone_controller.go b/internal/controller/bindzone_controller.go index c41c65f..bf26eac 100644 --- a/internal/controller/bindzone_controller.go +++ b/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 { diff --git a/internal/controller/zone_helpers.go b/internal/controller/zone_helpers.go index cf5f31b..8b77934 100644 --- a/internal/controller/zone_helpers.go +++ b/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}) -- 2.47.3 From cc714dc2b4710b1c9aff3c836be91513c628cdbb Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 26 Sep 2026 19:41:53 +1000 Subject: [PATCH 5/5] 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}) -- 2.47.3