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