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) + "."} +}