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})