From 6159a9f7c0304cf632c6d067c59bf83d3ff59c68 Mon Sep 17 00:00:00 2001 From: Viktor Voytovych Date: Tue, 20 Oct 2026 17:02:01 +0300 Subject: [PATCH] fix: Remove a record set's records of other types A record set that changed type kept its old records in PowerDNS. The agent pruned only records of the set's current type, so an ALIAS that became a CNAME conflicted with its own old ALIAS, and deleting the set left the old record behind. The agent now deletes every record that the set's ownership note names and that the set no longer wants, whatever its type. A delete at a name the set rewrites travels in the same PATCH, ahead of the rewrite, so a refused rewrite rolls back and the old record keeps serving. A conflict message no longer names the set's own old record. The agent logs and counts each record of another type that it deletes. The record set controller's comment said a resync corrects drift. A resync stops at the same programmed-at-generation check, so it does not, and the comment now says so. Fixes #225 --- docs/architecture/backends/powerdns.md | 11 +- .../dnsrecordset_downstream_controller.go | 5 +- internal/dns/pdns/client.go | 151 ++++++++---- internal/dns/pdns/pdns_integration_test.go | 233 ++++++++++++++++++ internal/dns/pdns/prune_test.go | 211 ++++++++++++++++ internal/dns/pdns/refused_test.go | 48 ++-- 6 files changed, 593 insertions(+), 66 deletions(-) create mode 100644 internal/dns/pdns/prune_test.go diff --git a/docs/architecture/backends/powerdns.md b/docs/architecture/backends/powerdns.md index 29eb5c7b..85e0ab52 100644 --- a/docs/architecture/backends/powerdns.md +++ b/docs/architecture/backends/powerdns.md @@ -72,9 +72,14 @@ record types require: The client drives PowerDNS through its HTTP API, authenticating with an API key in the `X-API-Key` header. For a zone, the agent ensures the zone exists and applies the nameserver policy from its `DNSZoneClass`. For a record set, the -agent replaces the desired owners and deletes extraneous owners of the same -type. When PowerDNS rejects a change, the agent reports the failure on the -resource with reason `PDNSError` and a human-readable message. +agent replaces the desired owners and deletes every other RRset the record set +wrote, whatever its type. So a record set that changes type removes its old +records, such as the ALIAS it held before it became a CNAME. The delete and the +rewrite at one name travel in one request, so when PowerDNS refuses the +rewrite, the old record keeps serving. The agent counts each record it deletes +of another type in `dns_operator_other_type_records_pruned_total`. When +PowerDNS rejects a change, the agent reports the failure on the resource with +reason `PDNSError` and a human-readable message. ## Storage and Serving diff --git a/internal/controller/dnsrecordset_downstream_controller.go b/internal/controller/dnsrecordset_downstream_controller.go index 7adf327b..50a1be2a 100644 --- a/internal/controller/dnsrecordset_downstream_controller.go +++ b/internal/controller/dnsrecordset_downstream_controller.go @@ -189,8 +189,9 @@ func (r *DNSRecordSetReconciler) Reconcile(ctx context.Context, req ctrl.Request // provider and the status says so. Every status write the controller makes // comes back as an update event, so without this the object reprograms // itself once per write — for a record set holding thousands of names that - // is the difference between a reconcile and a reconcile loop. Drift written - // outside the operator is still corrected on the manager's resync. + // is the difference between a reconcile and a reconcile loop. A resync + // returns here too, so drift written outside the operator stays until the + // record set's generation changes. if recordSetProgrammedAtGeneration(&rs) { logger.V(1).Info("RecordSet already programmed at this generation", "generation", rs.Generation) return r.checkALIASTargets(ctx, &rs) diff --git a/internal/dns/pdns/client.go b/internal/dns/pdns/client.go index 63673091..b065b40e 100644 --- a/internal/dns/pdns/client.go +++ b/internal/dns/pdns/client.go @@ -15,6 +15,7 @@ import ( "time" "github.com/go-logr/logr" + "github.com/prometheus/client_golang/prometheus" dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" dnserrors "go.miloapis.com/dns-operator/internal/dns/errors" "go.miloapis.com/dns-operator/internal/dns/ownername" @@ -23,8 +24,18 @@ import ( apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" logf "sigs.k8s.io/controller-runtime/pkg/log" + ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" ) +var otherTypeRecordsPruned = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "dns_operator_other_type_records_pruned_total", + Help: "Records a DNSRecordSet wrote under a type other than its current one, deleted from PowerDNS.", +}, []string{"record_type"}) + +func init() { + ctrlmetrics.Registry.MustRegister(otherTypeRecordsPruned) +} + type Client struct { BaseURL string APIKey string @@ -323,33 +334,23 @@ func (c *Client) EnsureRecordSet(ctx context.Context, zone dnsv1alpha1.DNSZone, replacedStatus = append(replacedStatus, len(statusList)-1) } - // Deletion phase. The zone read already says which owner names of this type - // this record set owns, so the surplus ones are a local diff — no global - // comment search, which is unscoped and therefore couples every zone in the - // database to this reconcile. - deletes := make([]rrset, 0) - for key, current := range existing { - if key.typ != recordType { - continue - } - if _, wanted := desired[key.name]; wanted { - continue - } - // A record set only prunes the owner names it wrote itself. Names owned - // by another DNSRecordSet of the same type, or by nothing at all, belong - // to whoever holds them. - if rrsetOwnerRef(current) != ownerRef { - continue - } + // Deletion phase. The zone read already says which RRsets this record set + // wrote, so the surplus ones are a local diff — no global comment search, + // which is unscoped and therefore couples every zone in the database to this + // reconcile. + surplus := make([]rrset, 0) + for _, key := range ownedSurplus(existing, ownerRef, recordType, desired) { c.logger.Info("Deleting RecordSet from PowerDNS", "owner", key.name, "recordType", key.typ, "recordSet", recordSet.Name) - deletes = append(deletes, newDeleteRRSet(key.name, key.typ)) + surplus = append(surplus, newDeleteRRSet(key.name, key.typ)) } + changes, deletes := pairDeletes(replaces, surplus) sortRRSets(deletes) transient := make([]error, 0) for _, chunk := range chunkRRSets(replaces) { - for i, err := range c.applyIsolatingRefusals(ctx, zoneName, chunk.rrsets) { + for i, err := range c.applyIsolatingRefusals(ctx, zoneName, changes[chunk.start:chunk.end]) { if err == nil { + c.notePrunes(zoneName, recordSet, changes[chunk.start+i]) continue } idx := replacedStatus[chunk.start+i] @@ -359,7 +360,7 @@ func (c *Client) EnsureRecordSet(ctx context.Context, zone dnsv1alpha1.DNSZone, } if refuses(err, patchIdentity(chunk.rrsets[i])) { c.logger.Info("PowerDNS refused rrset because another record holds its name", "zone", zoneName, "owner", owner, "recordType", recordType) - statusList[idx] = recordSetRefusedStatus(owner, ownername.Qualify(owner, zoneName), recordType, existing) + statusList[idx] = recordSetRefusedStatus(owner, ownername.Qualify(owner, zoneName), recordType, ownerRef, existing) continue } c.logger.Error(err, "Failed to apply rrsets to PowerDNS", "zone", zoneName, "owner", owner) @@ -370,22 +371,74 @@ func (c *Client) EnsureRecordSet(ctx context.Context, zone dnsv1alpha1.DNSZone, for _, chunk := range chunkRRSets(deletes) { if err := c.applyRRSetPatch(ctx, zoneName, chunk.rrsets); err != nil { c.logger.Error(err, "failed to delete recordsets from PowerDNS", "zone", zoneName, "count", len(chunk.rrsets)) + continue } + c.notePrunes(zoneName, recordSet, chunk.rrsets) } return keepTransitionTimes(statusList, recordSet.Status.RecordSets), errors.Join(transient...) } -func (c *Client) applyIsolatingRefusals(ctx context.Context, zone string, rrsets []rrset) []error { - errs := make([]error, len(rrsets)) - pending := make([]int, len(rrsets)) +// ownedSurplus returns the RRsets this record set wrote and no longer wants: +// every one of another type, and those of its own type at a name outside +// wanted. Records another record set wrote, or nothing did, are not its own. +func ownedSurplus(existing map[rrsetKey]zoneRRset, ownerRef, recordType string, wanted map[string]struct{}) []rrsetKey { + out := make([]rrsetKey, 0) + for key, current := range existing { + if rrsetOwnerRef(current) != ownerRef { + continue + } + if _, ok := wanted[key.name]; ok && key.typ == recordType { + continue + } + out = append(out, key) + } + return out +} + +// pairDeletes groups each replace with the deletes at its name, and returns the +// deletes at no replaced name apart. PowerDNS refuses a CNAME beside another +// type and rolls back the whole PATCH, so a delete that travels with the replace +// at its name keeps the old record serving until the new one can be written. +func pairDeletes(replaces, deletes []rrset) ([][]rrset, []rrset) { + changes := make([][]rrset, len(replaces)) + at := make(map[string]int, len(replaces)) + for i, rr := range replaces { + changes[i] = []rrset{rr} + at[rr.Name] = i + } + rest := make([]rrset, 0, len(deletes)) + for _, rr := range deletes { + i, replaced := at[rr.Name] + if !replaced { + rest = append(rest, rr) + continue + } + changes[i] = append(changes[i], rr) + } + return changes, rest +} + +func (c *Client) notePrunes(zone string, recordSet dnsv1alpha1.DNSRecordSet, applied []rrset) { + for _, rr := range applied { + if rr.ChangeType != changeTypeDelete || rr.Type == string(recordSet.Spec.RecordType) { + continue + } + c.logger.Info("Deleted a record its record set wrote under another type", "zone", zone, "owner", rr.Name, "recordType", rr.Type, "recordSet", recordSet.Name, "recordSetType", recordSet.Spec.RecordType) + otherTypeRecordsPruned.WithLabelValues(rr.Type).Inc() + } +} + +func (c *Client) applyIsolatingRefusals(ctx context.Context, zone string, changes [][]rrset) []error { + errs := make([]error, len(changes)) + pending := make([]int, len(changes)) for i := range pending { pending[i] = i } for len(pending) > 0 { batch := make([]rrset, 0, len(pending)) for _, i := range pending { - batch = append(batch, rrsets[i]) + batch = append(batch, changes[i]...) } err := c.applyRRSetPatch(ctx, zone, batch) if err == nil { @@ -397,7 +450,7 @@ func (c *Client) applyIsolatingRefusals(ctx context.Context, zone string, rrsets } kept := pending[:0] for _, i := range pending { - if _, ok := refused[patchIdentity(rrsets[i])]; ok { + if refusedIn(changes[i], refused) { errs[i] = err continue } @@ -414,15 +467,28 @@ func (c *Client) applyIsolatingRefusals(ctx context.Context, zone string, rrsets return errs } -func recordSetRefusedStatus(owner, qualified, recordType string, existing map[rrsetKey]zoneRRset) dnsv1alpha1.RecordSetStatus { +func refusedIn(change []rrset, refused map[rrsetKey]struct{}) bool { + for _, rr := range change { + if _, ok := refused[patchIdentity(rr)]; ok { + return true + } + } + return false +} + +func recordSetRefusedStatus(owner, qualified, recordType, ownerRef string, existing map[rrsetKey]zoneRRset) dnsv1alpha1.RecordSetStatus { name := stripTrailingDot(qualified) holders := make([]string, 0) for key, rr := range existing { if !strings.EqualFold(key.name, qualified) || strings.EqualFold(key.typ, recordType) { continue } + ref := rrsetOwnerRef(rr) + if ref == ownerRef { + continue + } holder := key.typ + " record" - if ref := rrsetOwnerRef(rr); ref != "" { + if ref != "" { holder = fmt.Sprintf("%s record of DNSRecordSet %s", key.typ, ref[strings.LastIndex(ref, ":")+1:]) } holders = append(holders, holder) @@ -596,10 +662,10 @@ func (c *Client) DeleteRecordSet(ctx context.Context, zone dnsv1alpha1.DNSZone, return err } - targets := make(map[string]struct{}, len(recordSet.Spec.Records)) + targets := make(map[rrsetKey]struct{}, len(recordSet.Spec.Records)) for i := range recordSet.Spec.Records { - qualified := ownername.Qualify(recordSet.Spec.Records[i].Name, zoneName) - current, found := existing[rrsetKey{name: qualified, typ: recordType}] + key := rrsetKey{name: ownername.Qualify(recordSet.Spec.Records[i].Name, zoneName), typ: recordType} + current, found := existing[key] if !found { continue } @@ -608,22 +674,16 @@ func (c *Client) DeleteRecordSet(ctx context.Context, zone dnsv1alpha1.DNSZone, if owner := rrsetOwnerRef(current); owner != "" && owner != ownerRef { continue } - targets[qualified] = struct{}{} + targets[key] = struct{}{} } - // Owner names this record set wrote that its spec no longer mentions. - for key, current := range existing { - if key.typ != recordType { - continue - } - if rrsetOwnerRef(current) == ownerRef { - targets[key.name] = struct{}{} - } + for _, key := range ownedSurplus(existing, ownerRef, recordType, nil) { + targets[key] = struct{}{} } deletes := make([]rrset, 0, len(targets)) - for name := range targets { - deletes = append(deletes, newDeleteRRSet(name, recordType)) + for key := range targets { + deletes = append(deletes, newDeleteRRSet(key.name, key.typ)) } sortRRSets(deletes) @@ -632,6 +692,7 @@ func (c *Client) DeleteRecordSet(ctx context.Context, zone dnsv1alpha1.DNSZone, c.logger.Error(err, "Failed to delete record set from PowerDNS", "zone", zoneName, "recordType", recordType) return err } + c.notePrunes(zoneName, recordSet, chunk.rrsets) } return nil @@ -784,8 +845,14 @@ func chunkRRSets(rrsets []rrset) []rrsetChunk { return out } +// sortRRSets puts every DELETE ahead of every REPLACE. PowerDNS applies a patch +// in order and checks each REPLACE against what the name holds by then, so a +// record that changes type must be gone before the new one is written. func sortRRSets(patch []rrset) { sort.Slice(patch, func(i, j int) bool { + if deleteI, deleteJ := patch[i].ChangeType == changeTypeDelete, patch[j].ChangeType == changeTypeDelete; deleteI != deleteJ { + return deleteI + } if patch[i].Type != patch[j].Type { return patch[i].Type < patch[j].Type } diff --git a/internal/dns/pdns/pdns_integration_test.go b/internal/dns/pdns/pdns_integration_test.go index bfc9bfbb..c73ed6b1 100644 --- a/internal/dns/pdns/pdns_integration_test.go +++ b/internal/dns/pdns/pdns_integration_test.go @@ -937,3 +937,236 @@ func TestPDNS_LMDB_AZoneDeleteThatFailsKeepsTheRecords(t *testing.T) { t.Fatalf("the zone is still there after the retry: GetZone returned %v", err) } } + +func typedRecordSet(name string, generation int64, recordType dnsv1alpha1.RRType, owners ...string) dnsv1alpha1.DNSRecordSet { + records := make([]dnsv1alpha1.RecordEntry, 0, len(owners)) + for _, owner := range owners { + entry := dnsv1alpha1.RecordEntry{Name: owner} + switch recordType { + case dnsv1alpha1.RRTypeA: + entry.A = &dnsv1alpha1.ARecordSpec{Content: "192.0.2.10"} + case dnsv1alpha1.RRTypeAAAA: + entry.AAAA = &dnsv1alpha1.AAAARecordSpec{Content: "2001:db8::10"} + case dnsv1alpha1.RRTypeALIAS: + entry.ALIAS = &dnsv1alpha1.ALIASRecordSpec{Content: "target.example.net."} + case dnsv1alpha1.RRTypeCNAME: + entry.CNAME = &dnsv1alpha1.CNAMERecordSpec{Content: "target.example.net."} + case dnsv1alpha1.RRTypeTXT: + entry.TXT = &dnsv1alpha1.TXTRecordSpec{Content: "held by " + name} + case dnsv1alpha1.RRTypeNS: + entry.NS = &dnsv1alpha1.NSRecordSpec{Content: "ns1.example.net."} + } + records = append(records, entry) + } + return dnsv1alpha1.DNSRecordSet{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default", UID: types.UID("uid-" + name), Generation: generation}, + Spec: dnsv1alpha1.DNSRecordSetSpec{RecordType: recordType, Records: records}, + } +} + +func ensureProgrammed(ctx context.Context, t *testing.T, client *Client, zone dnsv1alpha1.DNSZone, rs dnsv1alpha1.DNSRecordSet) { + t.Helper() + statuses, err := client.EnsureRecordSet(ctx, zone, rs) + if err != nil { + t.Fatalf("EnsureRecordSet(%s %s): %v", rs.Name, rs.Spec.RecordType, err) + } + for _, st := range statuses { + if cond := st.Conditions[0]; cond.Status != metav1.ConditionTrue { + t.Fatalf("%s %s at %s is not programmed: %s: %s", rs.Name, rs.Spec.RecordType, st.Name, cond.Reason, cond.Message) + } + } +} + +func patchCountingClient(t *testing.T, baseURL, apiKey string) (*Client, *atomic.Int32) { + t.Helper() + backend, err := url.Parse(baseURL) + if err != nil { + t.Fatalf("parse %s: %v", baseURL, err) + } + forward := httputil.NewSingleHostReverseProxy(backend) + var patches atomic.Int32 + proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPatch { + patches.Add(1) + } + forward.ServeHTTP(w, r) + })) + t.Cleanup(proxy.Close) + return NewClient(proxy.URL, apiKey), &patches +} + +type heldRRSet struct{ owner, typ string } + +func requireHeld(ctx context.Context, t *testing.T, client *Client, zoneName string, want ...heldRRSet) { + t.Helper() + for _, rr := range want { + if records, _, _ := rrsetCounts(ctx, t, client, zoneName, rr.owner, rr.typ); records != 1 { + t.Fatalf("%s holds %d %s records, want 1", rr.owner, records, rr.typ) + } + } +} + +func requireGone(ctx context.Context, t *testing.T, client *Client, zoneName string, gone ...heldRRSet) { + t.Helper() + for _, rr := range gone { + if _, _, listed := rrsetCounts(ctx, t, client, zoneName, rr.owner, rr.typ); listed { + t.Fatalf("%s still lists its %s", rr.owner, rr.typ) + } + } +} + +func TestPDNS_LMDB_ARecordSetChangesTypeAtOneName(t *testing.T) { + // No t.Parallel(): container + real PDNS. + baseURL, apiKey, stop := startPDNSLMDB(t) + defer stop() + + client, patches := patchCountingClient(t, baseURL, apiKey) + ctx := context.Background() + const zoneName = "retype.test" + zone := dnsv1alpha1.DNSZone{Spec: dnsv1alpha1.DNSZoneSpec{DomainName: zoneName}} + + if err := client.CreateZone(ctx, zoneName, []string{"ns1.example.net", "ns2.example.net"}); err != nil { + t.Fatalf("CreateZone: %v", err) + } + before := zoneCommentTotal(ctx, t, client, zoneName) + + alias, cname := heldRRSet{"www", "ALIAS"}, heldRRSet{"www", "CNAME"} + steps := []struct { + recordType dnsv1alpha1.RRType + held, other heldRRSet + }{ + {dnsv1alpha1.RRTypeALIAS, alias, cname}, + {dnsv1alpha1.RRTypeCNAME, cname, alias}, + {dnsv1alpha1.RRTypeALIAS, alias, cname}, + } + var web dnsv1alpha1.DNSRecordSet + for i, step := range steps { + web = typedRecordSet("web", int64(i+1), step.recordType, "www") + patches.Store(0) + ensureProgrammed(ctx, t, client, zone, web) + if got := patches.Load(); got != 1 { + t.Fatalf("step %d (%s) took %d PATCHes, want the delete and the replace in one", i+1, step.recordType, got) + } + requireHeld(ctx, t, client, zoneName, step.held) + requireGone(ctx, t, client, zoneName, step.other) + } + + patches.Store(0) + ensureProgrammed(ctx, t, client, zone, web) + if got := patches.Load(); got != 0 { + t.Fatalf("a second reconcile at the same generation sent %d PATCHes, want none", got) + } + + if err := client.DeleteRecordSet(ctx, zone, web); err != nil { + t.Fatalf("DeleteRecordSet: %v", err) + } + requireGone(ctx, t, client, zoneName, alias, cname) + if after := zoneCommentTotal(ctx, t, client, zoneName); after != before { + t.Fatalf("the zone holds %d comments after the record set was deleted, and held %d before it", after, before) + } +} + +func TestPDNS_LMDB_ATypeChangeLeavesWhatItDidNotWrite(t *testing.T) { + // No t.Parallel(): container + real PDNS. + baseURL, apiKey, stop := startPDNSLMDB(t) + defer stop() + + client := NewClient(baseURL, apiKey) + ctx := context.Background() + const zoneName = "neighbours.test" + zone := dnsv1alpha1.DNSZone{Spec: dnsv1alpha1.DNSZoneSpec{DomainName: zoneName}} + + if err := client.CreateZone(ctx, zoneName, []string{"ns1.example.net", "ns2.example.net"}); err != nil { + t.Fatalf("CreateZone: %v", err) + } + ensureProgrammed(ctx, t, client, zone, typedRecordSet("web", 1, dnsv1alpha1.RRTypeA, "www", "api", "old")) + ensureProgrammed(ctx, t, client, zone, typedRecordSet("txt", 1, dnsv1alpha1.RRTypeTXT, "www")) + ensureProgrammed(ctx, t, client, zone, typedRecordSet("mail", 1, dnsv1alpha1.RRTypeA, "mail")) + if err := sendRaw(ctx, t, client, http.MethodPatch, zoneName, + `{"rrsets":[{"name":"`+ownername.Qualify("www", zoneName)+`","type":"MX","ttl":300,"changetype":"REPLACE","records":[{"content":"10 mx.example.net.","disabled":false}]}]}`); err != nil { + t.Fatalf("write the unnoted MX: %v", err) + } + notWritten := []heldRRSet{{"www", "TXT"}, {"www", "MX"}, {"mail", "A"}} + written := []heldRRSet{{"www", "AAAA"}, {"api", "AAAA"}} + + web := typedRecordSet("web", 2, dnsv1alpha1.RRTypeAAAA, "www", "api") + ensureProgrammed(ctx, t, client, zone, web) + requireGone(ctx, t, client, zoneName, heldRRSet{"www", "A"}, heldRRSet{"api", "A"}, heldRRSet{"old", "A"}) + requireHeld(ctx, t, client, zoneName, append(written, notWritten...)...) + + if err := client.DeleteRecordSet(ctx, zone, web); err != nil { + t.Fatalf("DeleteRecordSet: %v", err) + } + requireGone(ctx, t, client, zoneName, written...) + requireHeld(ctx, t, client, zoneName, notWritten...) +} + +func TestPDNS_LMDB_AnApexRecordChangesTypeBesideSOAAndNS(t *testing.T) { + // No t.Parallel(): container + real PDNS. + baseURL, apiKey, stop := startPDNSLMDB(t) + defer stop() + + client := NewClient(baseURL, apiKey) + ctx := context.Background() + const zoneName = "apex.test" + zone := dnsv1alpha1.DNSZone{Spec: dnsv1alpha1.DNSZoneSpec{DomainName: zoneName}} + + if err := client.CreateZone(ctx, zoneName, []string{"ns1.example.net", "ns2.example.net"}); err != nil { + t.Fatalf("CreateZone: %v", err) + } + ensureProgrammed(ctx, t, client, zone, typedRecordSet("ns", 1, dnsv1alpha1.RRTypeNS, "@")) + ensureProgrammed(ctx, t, client, zone, typedRecordSet("apex", 1, dnsv1alpha1.RRTypeALIAS, "@")) + ensureProgrammed(ctx, t, client, zone, typedRecordSet("apex", 2, dnsv1alpha1.RRTypeA, "@")) + + if _, _, listed := rrsetCounts(ctx, t, client, zoneName, "@", "ALIAS"); listed { + t.Fatal("the apex became A and its old ALIAS is still listed") + } + for _, rr := range []struct { + typ string + comments int + }{{"A", 3}, {"NS", 3}, {"SOA", 0}} { + if records, comments, _ := rrsetCounts(ctx, t, client, zoneName, "@", rr.typ); records != 1 || comments != rr.comments { + t.Fatalf("the apex %s holds records=%d comments=%d, want 1 and %d", rr.typ, records, comments, rr.comments) + } + } +} + +func TestPDNS_LMDB_ARefusedTypeChangeKeepsTheOldRecord(t *testing.T) { + // No t.Parallel(): container + real PDNS. + baseURL, apiKey, stop := startPDNSLMDB(t) + defer stop() + + client := NewClient(baseURL, apiKey) + ctx := context.Background() + const zoneName = "refusal.test" + zone := dnsv1alpha1.DNSZone{Spec: dnsv1alpha1.DNSZoneSpec{DomainName: zoneName}} + + if err := client.CreateZone(ctx, zoneName, []string{"ns1.example.net", "ns2.example.net"}); err != nil { + t.Fatalf("CreateZone: %v", err) + } + ensureProgrammed(ctx, t, client, zone, typedRecordSet("web", 1, dnsv1alpha1.RRTypeALIAS, "www", "api")) + txt := typedRecordSet("txt", 1, dnsv1alpha1.RRTypeTXT, "www") + ensureProgrammed(ctx, t, client, zone, txt) + + web := typedRecordSet("web", 2, dnsv1alpha1.RRTypeCNAME, "www", "api") + statuses, err := client.EnsureRecordSet(ctx, zone, web) + if err != nil { + t.Fatalf("EnsureRecordSet: %v", err) + } + refused := programmedCondition(t, statuses, "www") + if refused.Status != metav1.ConditionFalse || refused.Reason != "Conflict" { + t.Fatalf("a CNAME beside another record set's TXT was not refused: %+v", refused) + } + if !strings.Contains(refused.Message, "TXT record of DNSRecordSet txt") || strings.Contains(refused.Message, "DNSRecordSet web") { + t.Fatalf("the message must name the TXT that blocks and not the record set's own old record: %q", refused.Message) + } + requireHeld(ctx, t, client, zoneName, heldRRSet{"www", "ALIAS"}, heldRRSet{"api", "CNAME"}) + requireGone(ctx, t, client, zoneName, heldRRSet{"api", "ALIAS"}) + + if err := client.DeleteRecordSet(ctx, zone, txt); err != nil { + t.Fatalf("DeleteRecordSet(txt): %v", err) + } + ensureProgrammed(ctx, t, client, zone, web) + requireHeld(ctx, t, client, zoneName, heldRRSet{"www", "CNAME"}) + requireGone(ctx, t, client, zoneName, heldRRSet{"www", "ALIAS"}) +} diff --git a/internal/dns/pdns/prune_test.go b/internal/dns/pdns/prune_test.go new file mode 100644 index 00000000..2d0623af --- /dev/null +++ b/internal/dns/pdns/prune_test.go @@ -0,0 +1,211 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package pdns + +import ( + "context" + "reflect" + "sort" + "strings" + "testing" + + dto "github.com/prometheus/client_model/go" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func notedRRSet(name, typ, owner string) zoneRRset { + rr := zoneRRset{Name: name, Type: typ, TTL: 300, Records: []zoneRRsetRecord{{Content: "kept"}}} + if owner != "" { + rr.Comments = []zoneRRsetComment{{Account: ACCOUNT_OWNER, Content: owner}} + } + return rr +} + +func deletedKeys(stub *pdnsStub) []rrsetKey { + out := make([]rrsetKey, 0) + for _, patch := range stub.patches { + for _, rr := range patch.RRSets { + if rr.ChangeType == changeTypeDelete { + out = append(out, rrsetKey{name: rr.Name, typ: rr.Type}) + } + } + } + sort.Slice(out, func(i, j int) bool { + if out[i].name != out[j].name { + return out[i].name < out[j].name + } + return out[i].typ < out[j].typ + }) + return out +} + +func changeIndex(patch patchZoneRequest, name, typ, changeType string) int { + for i, rr := range patch.RRSets { + if rr.Name == name && rr.Type == typ && rr.ChangeType == changeType { + return i + } + } + return -1 +} + +func prunedCount(t *testing.T, recordType string) float64 { + t.Helper() + var m dto.Metric + if err := otherTypeRecordsPruned.WithLabelValues(recordType).Write(&m); err != nil { + t.Fatalf("read the prune counter: %v", err) + } + return m.GetCounter().GetValue() +} + +func TestEnsureRecordSet_PrunesEveryRecordItWroteAndNoOther(t *testing.T) { + t.Parallel() + + stub, c := newPDNSStub(t, zoneResponse{Name: exampleCom, RRSets: []zoneRRset{ + notedRRSet("www.example.com.", "ALIAS", "default:rs"), + notedRRSet("gone.example.com.", "TXT", "default:rs"), + notedRRSet("stale.example.com.", "A", "default:rs"), + notedRRSet("www.example.com.", "TXT", "default:other"), + notedRRSet("www.example.com.", "MX", ""), + notedRRSet("other.example.com.", "A", "default:other"), + }}) + + if _, err := c.EnsureRecordSet(context.Background(), testZone, aRecordSet(2, "www", "api")); err != nil { + t.Fatalf("EnsureRecordSet error: %v", err) + } + + want := []rrsetKey{ + {name: "gone.example.com.", typ: "TXT"}, + {name: "stale.example.com.", typ: "A"}, + {name: "www.example.com.", typ: "ALIAS"}, + } + if got := deletedKeys(stub); !reflect.DeepEqual(got, want) { + t.Fatalf("deleted %v, want every record this record set wrote and no longer wants, and nothing else", got) + } + + for _, patch := range stub.patches { + rewrite := changeIndex(patch, "www.example.com.", "A", changeTypeReplace) + if rewrite < 0 { + continue + } + if prune := changeIndex(patch, "www.example.com.", "ALIAS", changeTypeDelete); prune < 0 || prune > rewrite { + t.Fatalf("the old ALIAS at www must be deleted ahead of the A that replaces it, in the same PATCH: %+v", patch.RRSets) + } + return + } + t.Fatal("no PATCH wrote the A at www") +} + +func TestDeleteRecordSet_RemovesEveryRecordItWroteAndNoOther(t *testing.T) { + t.Parallel() + + stub, c := newPDNSStub(t, zoneResponse{Name: exampleCom, RRSets: []zoneRRset{ + notedRRSet("www.example.com.", "A", "default:rs"), + notedRRSet("www.example.com.", "ALIAS", "default:rs"), + notedRRSet("old.example.com.", "TXT", "default:rs"), + notedRRSet("api.example.com.", "A", ""), + notedRRSet("www.example.com.", "TXT", "default:other"), + notedRRSet("www.example.com.", "MX", ""), + }}) + + if err := c.DeleteRecordSet(context.Background(), testZone, aRecordSet(1, "www", "api")); err != nil { + t.Fatalf("DeleteRecordSet error: %v", err) + } + + want := []rrsetKey{ + {name: "api.example.com.", typ: "A"}, + {name: "old.example.com.", typ: "TXT"}, + {name: "www.example.com.", typ: "A"}, + {name: "www.example.com.", typ: "ALIAS"}, + } + if got := deletedKeys(stub); !reflect.DeepEqual(got, want) { + t.Fatalf("deleted %v, want every record this record set wrote, its own type at its names, and nothing else", got) + } +} + +func TestEnsureRecordSet_ReplacesItsOwnRecordOfAnotherType(t *testing.T) { + t.Parallel() + + fake, c := newCoexistencePDNS(t, notedRRSet("www.example.com.", "ALIAS", "default:aliases")) + + statuses, err := c.EnsureRecordSet(context.Background(), testZone, cnameRecordSet("www")) + if err != nil { + t.Fatalf("EnsureRecordSet error: %v", err) + } + if cond := programmedCondition(t, statuses, "www"); cond.Status != metav1.ConditionTrue { + t.Fatalf("a record set that changed from ALIAS to CNAME must publish, got %+v", cond) + } + if !fake.has("www.example.com.", "CNAME") || fake.has("www.example.com.", "ALIAS") { + t.Fatal("www must hold the new CNAME and not the old ALIAS") + } + if fake.patches != 1 { + t.Fatalf("the type change took %d PATCHes, want the delete and the replace in one", fake.patches) + } +} + +func TestEnsureRecordSet_KeepsItsOldRecordWhileTheNewOneIsRefused(t *testing.T) { + t.Parallel() + + fake, c := newCoexistencePDNS(t, + notedRRSet("www.example.com.", "ALIAS", "default:aliases"), + blockingARecord("www.example.com."), + ) + + statuses, err := c.EnsureRecordSet(context.Background(), testZone, cnameRecordSet("api", "www")) + if err != nil { + t.Fatalf("EnsureRecordSet error: %v", err) + } + cond := programmedCondition(t, statuses, "www") + if cond.Status != metav1.ConditionFalse || cond.Reason != "Conflict" { + t.Fatalf("expected the CNAME beside another record set's A to be refused, got %+v", cond) + } + if !strings.Contains(cond.Message, "A record of DNSRecordSet web") || strings.Contains(cond.Message, "DNSRecordSet aliases") { + t.Fatalf("the message must name the record that blocks, not the record set's own old record: %q", cond.Message) + } + if !fake.has("www.example.com.", "ALIAS") { + t.Fatal("the refused change removed the old ALIAS, so www stopped serving it") + } + if cond := programmedCondition(t, statuses, "api"); cond.Status != metav1.ConditionTrue { + t.Fatalf("api must publish beside the refused name, got %+v", cond) + } +} + +func TestPrunesOfAnotherTypeAreCountedOnceApplied(t *testing.T) { + ctx := context.Background() + aliases, records := prunedCount(t, "ALIAS"), prunedCount(t, "A") + + fake, c := newCoexistencePDNS(t, + notedRRSet("api.example.com.", "ALIAS", "default:aliases"), + blockingARecord("api.example.com."), + ) + if _, err := c.EnsureRecordSet(ctx, testZone, cnameRecordSet("api")); err != nil { + t.Fatalf("EnsureRecordSet error: %v", err) + } + if got := prunedCount(t, "ALIAS") - aliases; got != 0 { + t.Fatalf("counted %v ALIAS prunes while the change that carries it was refused, want 0", got) + } + + fake.remove("api.example.com.", "A") + if _, err := c.EnsureRecordSet(ctx, testZone, cnameRecordSet("api")); err != nil { + t.Fatalf("EnsureRecordSet error: %v", err) + } + if got := prunedCount(t, "ALIAS") - aliases; got != 1 { + t.Fatalf("counted %v ALIAS prunes once the change applied, want 1", got) + } + + stub, d := newPDNSStub(t, zoneResponse{Name: exampleCom, RRSets: []zoneRRset{ + notedRRSet("www.example.com.", "A", "default:rs"), + notedRRSet("www.example.com.", "ALIAS", "default:rs"), + }}) + if err := d.DeleteRecordSet(ctx, testZone, aRecordSet(1, "www")); err != nil { + t.Fatalf("DeleteRecordSet error: %v", err) + } + if len(stub.patches) != 1 { + t.Fatalf("expected 1 PATCH, got %d", len(stub.patches)) + } + if got := prunedCount(t, "ALIAS") - aliases; got != 2 { + t.Fatalf("counted %v ALIAS prunes after the delete, want 2", got) + } + if got := prunedCount(t, "A") - records; got != 0 { + t.Fatalf("counted %v A prunes: a record of the record set's own type is not another type's prune", got) + } +} diff --git a/internal/dns/pdns/refused_test.go b/internal/dns/pdns/refused_test.go index 31422105..02511be2 100644 --- a/internal/dns/pdns/refused_test.go +++ b/internal/dns/pdns/refused_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "io" + "maps" "net/http" "net/http/httptest" "strings" @@ -61,30 +62,30 @@ func newCoexistencePDNS(t *testing.T, seed ...zoneRRset) (*coexistencePDNS, *Cli body, _ := io.ReadAll(r.Body) var req patchZoneRequest _ = json.Unmarshal(body, &req) + staged := maps.Clone(fake.rrsets) for _, rr := range req.RRSets { - if rr.ChangeType != changeTypeReplace { - continue - } - for key := range fake.rrsets { - if key.name == rr.Name && key.typ != rr.Type && (key.typ == "CNAME" || rr.Type == "CNAME") { - w.WriteHeader(http.StatusUnprocessableEntity) - if fake.multi { - _, _ = fmt.Fprintf(w, `{"error": "Multiple errors found in RRset", "errors": ["RRset %s IN %s: Conflicts with pre-existing RRset"]}`, rr.Name, rr.Type) - return - } - _, _ = fmt.Fprintf(w, `{"error": "RRset %s IN %s: Conflicts with pre-existing RRset"}`, rr.Name, rr.Type) + key := rrsetKey{name: rr.Name, typ: rr.Type} + switch { + case rr.ChangeType == changeTypeDelete: + delete(staged, key) + case rr.Records == nil: + if current, ok := staged[key]; ok { + current.Comments = rr.Comments + staged[key] = current + } + case conflictsAt(staged, rr): + w.WriteHeader(http.StatusUnprocessableEntity) + if fake.multi { + _, _ = fmt.Fprintf(w, `{"error": "Multiple errors found in RRset", "errors": ["RRset %s IN %s: Conflicts with pre-existing RRset"]}`, rr.Name, rr.Type) return } + _, _ = fmt.Fprintf(w, `{"error": "RRset %s IN %s: Conflicts with pre-existing RRset"}`, rr.Name, rr.Type) + return + default: + staged[key] = zoneRRset{Name: rr.Name, Type: rr.Type, Comments: rr.Comments} } } - for _, rr := range req.RRSets { - key := rrsetKey{name: rr.Name, typ: rr.Type} - if rr.ChangeType == changeTypeDelete { - delete(fake.rrsets, key) - continue - } - fake.rrsets[key] = zoneRRset{Name: rr.Name, Type: rr.Type, Comments: rr.Comments} - } + fake.rrsets = staged w.WriteHeader(http.StatusNoContent) default: w.WriteHeader(http.StatusMethodNotAllowed) @@ -95,6 +96,15 @@ func newCoexistencePDNS(t *testing.T, seed ...zoneRRset) (*coexistencePDNS, *Cli return fake, NewClient(s.URL, "k") } +func conflictsAt(rrsets map[rrsetKey]zoneRRset, rr rrset) bool { + for key := range rrsets { + if key.name == rr.Name && key.typ != rr.Type && (key.typ == "CNAME" || rr.Type == "CNAME") { + return true + } + } + return false +} + func (f *coexistencePDNS) has(name, typ string) bool { f.mu.Lock() defer f.mu.Unlock()