diff --git a/internal/controller/dnsrecordset_powerdns_controller.go b/internal/controller/dnsrecordset_powerdns_controller.go index 17c3dfe..52d230f 100644 --- a/internal/controller/dnsrecordset_powerdns_controller.go +++ b/internal/controller/dnsrecordset_powerdns_controller.go @@ -6,6 +6,7 @@ import ( "context" "fmt" "sort" + "time" "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -28,6 +29,13 @@ import ( pdnsclient "go.miloapis.com/dns-operator/internal/pdns" ) +// conflictRequeueInterval is how long to wait before re-attempting a record +// PowerDNS rejected because it cannot coexist with existing data at the same +// owner name. Long, because clearing the conflict needs a change we may never +// be woken for: the conflicting RRset can be written directly to PowerDNS by +// another writer, producing no Kubernetes watch event. +const conflictRequeueInterval = 10 * time.Minute + // PowerDNSRecordSetReconcileRequest scopes reconciliation to a single (zone, type, owner name) tuple. type PowerDNSRecordSetReconcileRequest struct { ctrl.Request @@ -123,17 +131,32 @@ func (r *DNSRecordSetPowerDNSReconciler) Reconcile( } var pdnsErr error - if owner == nil { - pdnsErr = r.PDNS.DeleteRRSet(ctx, zone.Spec.DomainName, req.RecordSetType, req.RecordSetName) - } else { + // Default to deleting: unless this request resolves to records worth + // writing, the RRset goes away. The delete itself is guarded below, so both + // paths that reach it — no owning DNSRecordSet, and an owner that yields no + // usable records — get the same protection. + wantDelete := true + if owner != nil { entries := filterRecordEntries(owner, req.RecordSetName) payload, ok := pdnsclient.BuildOwnerRRSet(zone.Spec.DomainName, dnsv1alpha1.RRType(req.RecordSetType), req.RecordSetName, entries) - if !ok || len(payload.Records) == 0 { - pdnsErr = r.PDNS.DeleteRRSet(ctx, zone.Spec.DomainName, req.RecordSetType, req.RecordSetName) - } else { + if ok && len(payload.Records) > 0 { + wantDelete = false pdnsErr = r.PDNS.ReplaceRRSet(ctx, zone.Spec.DomainName, req.RecordSetType, req.RecordSetName, payload.TTL, payload.Records) } } + if wantDelete { + // Only delete once no *other spelling* of this owner name still claims + // the RRset. "api", "api.example.com." and "@" at the apex all qualify + // to a single PowerDNS RRset, so rewriting an owner name enqueues both + // the old and the new spelling; the request holding the retired + // spelling must not delete what the live one just wrote. + rrsetName := pdnsclient.QualifyOwner(req.RecordSetName, zone.Spec.DomainName) + if aliasedOwnerExists(&rsList, req.RecordSetType, zone.Spec.DomainName, rrsetName, req.RecordSetName) { + logger.Info("owner name aliases a live RRset; skipping delete", "rrsetName", rrsetName) + } else { + pdnsErr = r.PDNS.DeleteRRSet(ctx, zone.Spec.DomainName, req.RecordSetType, req.RecordSetName) + } + } if pdnsErr != nil { logger.Error(pdnsErr, "pdns apply failed") } @@ -144,6 +167,17 @@ func (r *DNSRecordSetPowerDNSReconciler) Reconcile( } if pdnsErr != nil { + if pdnsclient.IsConflict(pdnsErr) { + // A coexistence conflict is not a transient provider failure: the + // record is well-formed but cannot share this owner name with data + // already there. Retrying on the error path cannot clear it, and + // hot-looping on it inflates the controller's reconcile error ratio + // until the alert fires. Poll slowly instead; the status already + // carries Programmed=False with reason Conflict. + logger.Info("pdns coexistence conflict; requeueing without error", + "requeueAfter", conflictRequeueInterval) + return reconcile.Result{RequeueAfter: conflictRequeueInterval}, nil + } return reconcile.Result{}, pdnsErr } @@ -151,6 +185,36 @@ func (r *DNSRecordSetPowerDNSReconciler) Reconcile( return reconcile.Result{}, nil } +// aliasedOwnerExists reports whether any DNSRecordSet in list holds a record of +// recordType, under an owner name other than excludeOwnerName, that qualifies to +// rrsetName within zoneDomain. It lets the delete paths distinguish an owner name +// that was genuinely removed from one that was merely respelled: the latter still +// owns the RRset under a different spelling and must not be deleted. +// +// excludeOwnerName is the spelling being reconciled. Skipping it keeps the +// genuine cleanup case working — a request whose own records have gone away +// must still be able to delete the RRset it alone owned. +func aliasedOwnerExists( + list *dnsv1alpha1.DNSRecordSetList, + recordType, zoneDomain, rrsetName, excludeOwnerName string, +) bool { + for i := range list.Items { + rs := &list.Items[i] + if string(rs.Spec.RecordType) != recordType { + continue + } + for _, rec := range rs.Spec.Records { + if rec.Name == "" || rec.Name == excludeOwnerName { + continue + } + if pdnsclient.QualifyOwner(rec.Name, zoneDomain) == rrsetName { + return true + } + } + } + return false +} + func (r *DNSRecordSetPowerDNSReconciler) updateStatuses( ctx context.Context, recordName string, diff --git a/internal/controller/dnsrecordset_powerdns_controller_test.go b/internal/controller/dnsrecordset_powerdns_controller_test.go index 67fa9e4..d471513 100644 --- a/internal/controller/dnsrecordset_powerdns_controller_test.go +++ b/internal/controller/dnsrecordset_powerdns_controller_test.go @@ -4,6 +4,7 @@ package controller_test import ( "context" "errors" + "net/http" "testing" "time" @@ -589,3 +590,196 @@ func TestReconcile_OwnerWithNoRecords_DeletesRRSet(t *testing.T) { t.Fatalf("unexpected per-record condition on delete-only owner: %+v", cond) } } + +// --------------------------------------------------------------------------- +// Owner-name spelling aliases +// +// "api", "api.example.com." and "@" at the apex all qualify to a single +// PowerDNS RRset. Rewriting an owner name therefore enqueues two requests for +// one RRset, and the request carrying the retired spelling finds no owning +// DNSRecordSet — it must not delete what the live spelling wrote. +// --------------------------------------------------------------------------- + +const ( + ownerRelative = "api" + ownerAbsolute = "api.example.com." + aliasValue = "9.9.9.9" +) + +func newPDNSRequest(zoneName, ownerName string) controller.PowerDNSRecordSetReconcileRequest { + return controller.PowerDNSRecordSetReconcileRequest{ + Request: ctrl.Request{ + NamespacedName: client.ObjectKey{ + Namespace: ns, + Name: zoneName, + }, + }, + RecordSetType: string(dnsv1alpha1.RRTypeA), + RecordSetName: ownerName, + } +} + +func TestReconcile_RespelledOwnerName_DoesNotDeleteLiveRRSet(t *testing.T) { + t.Parallel() + + // Both queue orderings must converge on the record being present: the fix + // removes the race, so neither order may produce a delete. + orders := map[string][]string{ + "live spelling first": {ownerRelative, ownerAbsolute}, + "retired spelling first": {ownerAbsolute, ownerRelative}, + } + + for name, order := range orders { + t.Run(name, func(t *testing.T) { + t.Parallel() + + scheme := newScheme(t) + zoneName := "zone-respelled" + zone, zc := newZoneAndClass(zoneName) + // The spec has already converged on the relative spelling; the + // absolute one survives only as a queued request. + rs := newARecordSet("rs-respelled", zoneName, ownerRelative, aliasValue) + + pdns := &fakePDNSClient{} + cl := newFakeClient(t, scheme, zone, zc, rs) + r := &controller.DNSRecordSetPowerDNSReconciler{ + Client: cl, + Scheme: scheme, + PDNS: pdns, + } + + for _, ownerName := range order { + if _, err := r.Reconcile(context.Background(), newPDNSRequest(zoneName, ownerName)); err != nil { + t.Fatalf("reconcile %q: %v", ownerName, err) + } + } + + if len(pdns.deleteCalls) != 0 { + t.Fatalf("retired spelling deleted a live RRset: %+v", pdns.deleteCalls) + } + if len(pdns.replaceCalls) != 1 { + t.Fatalf("expected 1 ReplaceRRSet, got %+v", pdns.replaceCalls) + } + if got := pdns.replaceCalls[0].OwnerName; got != ownerRelative { + t.Fatalf("expected RRset written under %q, got %q", ownerRelative, got) + } + }) + } +} + +func TestReconcile_ApexSpellingAlias_DoesNotDeleteLiveRRSet(t *testing.T) { + t.Parallel() + + scheme := newScheme(t) + zoneName := "zone-apex-alias" + zone, zc := newZoneAndClass(zoneName) + rs := newARecordSet("rs-apex-alias", zoneName, "@", aliasValue) + + pdns := &fakePDNSClient{} + cl := newFakeClient(t, scheme, zone, zc, rs) + r := &controller.DNSRecordSetPowerDNSReconciler{ + Client: cl, + Scheme: scheme, + PDNS: pdns, + } + + // The absolute apex spelling is the same RRset as "@". + req := newPDNSRequest(zoneName, zone.Spec.DomainName+".") + if _, err := r.Reconcile(context.Background(), req); err != nil { + t.Fatalf("reconcile apex alias: %v", err) + } + + if len(pdns.deleteCalls) != 0 { + t.Fatalf("apex alias deleted the live RRset: %+v", pdns.deleteCalls) + } +} + +func TestReconcile_RemovedOwnerName_StillDeletesRRSet(t *testing.T) { + t.Parallel() + + scheme := newScheme(t) + zoneName := "zone-removed-owner" + zone, zc := newZoneAndClass(zoneName) + rs := newARecordSet("rs-unrelated", zoneName, ownerRelative, aliasValue) + + pdns := &fakePDNSClient{} + cl := newFakeClient(t, scheme, zone, zc, rs) + r := &controller.DNSRecordSetPowerDNSReconciler{ + Client: cl, + Scheme: scheme, + PDNS: pdns, + } + + // Nothing claims "retired" under any spelling, so cleanup must proceed. + const removed = "retired" + if _, err := r.Reconcile(context.Background(), newPDNSRequest(zoneName, removed)); err != nil { + t.Fatalf("reconcile removed owner: %v", err) + } + + if len(pdns.deleteCalls) != 1 { + t.Fatalf("expected 1 DeleteRRSet for a genuinely removed owner, got %+v", pdns.deleteCalls) + } + if got := pdns.deleteCalls[0].OwnerName; got != removed { + t.Fatalf("expected delete of %q, got %q", removed, got) + } +} + +// --------------------------------------------------------------------------- +// Coexistence conflicts +// --------------------------------------------------------------------------- + +// A PowerDNS coexistence conflict cannot be cleared by retrying, and retrying +// on the error path pins the controller's reconcile error ratio high. It must +// requeue slowly instead, without reporting a reconcile error. +func TestReconcile_CoexistenceConflict_RequeuesWithoutError(t *testing.T) { + t.Parallel() + + scheme := newScheme(t) + zoneName := "zone-conflict" + zone, zc := newZoneAndClass(zoneName) + rs := newARecordSet("rs-conflict", zoneName, ownerRelative, aliasValue) + + pdns := &fakePDNSClient{ + replaceErr: pdnsclient.NewAPIError( + http.StatusUnprocessableEntity, + `{"error":"RRset api.example.com. IN A: Conflicts with pre-existing RRset"}`, + ), + } + cl := newFakeClient(t, scheme, zone, zc, rs) + r := &controller.DNSRecordSetPowerDNSReconciler{ + Client: cl, + Scheme: scheme, + PDNS: pdns, + } + + ctx := context.Background() + res, err := r.Reconcile(ctx, newPDNSRequest(zoneName, ownerRelative)) + if err != nil { + t.Fatalf("conflict must not surface as a reconcile error, got %v", err) + } + if res.RequeueAfter <= 0 { + t.Fatalf("expected a requeue so the record recovers when the conflict clears, got %+v", res) + } + + var updated dnsv1alpha1.DNSRecordSet + if err := cl.Get(ctx, client.ObjectKey{Namespace: ns, Name: rs.Name}, &updated); err != nil { + t.Fatalf("get updated: %v", err) + } + var st *dnsv1alpha1.RecordSetStatus + for i := range updated.Status.RecordSets { + if updated.Status.RecordSets[i].Name == ownerRelative { + st = &updated.Status.RecordSets[i] + break + } + } + if st == nil { + t.Fatalf("expected RecordSetStatus for %q", ownerRelative) + } + cond := apimeta.FindStatusCondition(st.Conditions, controller.CondProgrammed) + if cond == nil { + t.Fatalf("expected per-record CondProgrammed condition") + } + if cond.Status != metav1.ConditionFalse || cond.Reason != controller.ReasonConflict { + t.Fatalf("expected Programmed=False/Conflict, got %+v", cond) + } +} diff --git a/internal/pdns/client.go b/internal/pdns/client.go index 00bed82..762ef04 100644 --- a/internal/pdns/client.go +++ b/internal/pdns/client.go @@ -206,7 +206,7 @@ func BuildOwnerRRSet( }, } rrsets := buildRRSets(zone, rs) - target := qualifyOwner(ownerName, zone) + target := QualifyOwner(ownerName, zone) for _, rr := range rrsets { if rr.Name != target { continue @@ -348,7 +348,7 @@ func (c *Client) ReplaceRRSet( records = append(records, rrsetRecord{Content: v, Disabled: false}) } patch := []rrset{{ - Name: qualifyOwner(ownerName, zone), + Name: QualifyOwner(ownerName, zone), Type: recordType, TTL: ttl, ChangeType: "REPLACE", @@ -360,7 +360,7 @@ func (c *Client) ReplaceRRSet( // DeleteRRSet removes the referenced (type, owner) RRset from PDNS. func (c *Client) DeleteRRSet(ctx context.Context, zone, recordType, ownerName string) error { patch := []rrset{{ - Name: qualifyOwner(ownerName, zone), + Name: QualifyOwner(ownerName, zone), Type: recordType, ChangeType: "DELETE", Records: []rrsetRecord{}, @@ -428,7 +428,7 @@ func buildRRSets(zone string, rs dnsv1alpha1.DNSRecordSet) []rrset { if rec.TTL != nil { ttl = int(*rec.TTL) } - name := qualifyOwner(rec.Name, zone) + name := QualifyOwner(rec.Name, zone) r := getOrInit(name, ttl) switch rs.Spec.RecordType { @@ -756,7 +756,14 @@ func makeSimpleRRSet(name, typ string, ttl int, values []string) rrset { } } -func qualifyOwner(owner, zone string) string { +// QualifyOwner returns the absolute RRset name PowerDNS keys an owner on within +// zone. It accepts every spelling the API allows: "@" or the empty string for +// the apex, a relative label such as "api", or an already-absolute name ending +// in a dot. Several spellings therefore collapse to one RRset — "api" and +// "api.example.com." both qualify to "api.example.com." in zone example.com — +// so callers comparing two owner names for RRset identity must compare their +// qualified forms rather than the raw values. +func QualifyOwner(owner, zone string) string { if owner == "@" || owner == "" { return zone + "." } diff --git a/internal/pdns/errors.go b/internal/pdns/errors.go index c365d90..0d05603 100644 --- a/internal/pdns/errors.go +++ b/internal/pdns/errors.go @@ -13,6 +13,14 @@ type pdnsErrorBody struct { Error string `json:"error"` } +// NewAPIError builds a PowerDNS API error carrying an HTTP status and the raw +// response body. The client produces these itself; this constructor exists so +// code outside this package — controllers and their tests — can build the error +// shapes that IsConflict and FriendlyMessage classify. +func NewAPIError(status int, body string) error { + return &pdnsAPIError{Status: status, Body: body} +} + // FriendlyMessage returns a user-readable message for a PowerDNS API error. // The raw technical error is preserved in operator logs; only the translated // message is written to the DNSRecordSet status condition so end users see diff --git a/internal/pdns/pdns_integration_test.go b/internal/pdns/pdns_integration_test.go index 91eb340..0d5af13 100644 --- a/internal/pdns/pdns_integration_test.go +++ b/internal/pdns/pdns_integration_test.go @@ -233,7 +233,7 @@ func TestPDNS_EndToEnd_AllTypes(t *testing.T) { // helper for asserts with normalization get := func(typ, owner string) []string { - return index[key{typ, qualifyOwner(owner, zone)}] + return index[key{typ, QualifyOwner(owner, zone)}] } stripq := func(s string) string { if len(s) >= 2 && s[0] == '"' && s[len(s)-1] == '"' { @@ -362,7 +362,7 @@ func TestPDNS_ApplyRecordSetAuthoritative_CleansRemovedOwners(t *testing.T) { sort.Strings(index[k]) } get := func(typ, owner string) []string { - return index[[2]string{typ, qualifyOwner(owner, zone)}] + return index[[2]string{typ, QualifyOwner(owner, zone)}] } return index, get } @@ -397,8 +397,8 @@ func TestPDNS_ApplyRecordSetAuthoritative_CleansRemovedOwners(t *testing.T) { // Capture NS/SOA count before we mutate A records, to verify we don't touch other types. indexBefore, _ := buildIndex(t) - nsBefore := len(indexBefore[[2]string{"NS", qualifyOwner("@", zone)}]) - soaBefore := len(indexBefore[[2]string{"SOA", qualifyOwner("@", zone)}]) + nsBefore := len(indexBefore[[2]string{"NS", QualifyOwner("@", zone)}]) + soaBefore := len(indexBefore[[2]string{"SOA", QualifyOwner("@", zone)}]) // Updated: drop "www", change @ and api. updated := dnsv1alpha1.DNSRecordSet{ @@ -430,10 +430,10 @@ func TestPDNS_ApplyRecordSetAuthoritative_CleansRemovedOwners(t *testing.T) { } // Verify we did not touch NS/SOA rrsets (ApplyRecordSetAuthoritative is per-type). - if got := len(indexAfter[[2]string{"NS", qualifyOwner("@", zone)}]); got != nsBefore { + if got := len(indexAfter[[2]string{"NS", QualifyOwner("@", zone)}]); got != nsBefore { t.Fatalf("NS rrset count changed: before=%d after=%d", nsBefore, got) } - if got := len(indexAfter[[2]string{"SOA", qualifyOwner("@", zone)}]); got != soaBefore { + if got := len(indexAfter[[2]string{"SOA", QualifyOwner("@", zone)}]); got != soaBefore { t.Fatalf("SOA rrset count changed: before=%d after=%d", soaBefore, got) } } diff --git a/internal/pdns/pdns_test.go b/internal/pdns/pdns_test.go index baab21c..2d8905b 100644 --- a/internal/pdns/pdns_test.go +++ b/internal/pdns/pdns_test.go @@ -576,14 +576,14 @@ func TestHelpers(t *testing.T) { if got := quoteIfNeeded(`"x"`); got != `"x"` { t.Fatalf("quoteIfNeeded pass-through: %q", got) } - if got := qualifyOwner("@", "example.com"); got != exampleCom { - t.Fatalf("qualifyOwner @: %q", got) + if got := QualifyOwner("@", "example.com"); got != exampleCom { + t.Fatalf("QualifyOwner @: %q", got) } - if got := qualifyOwner("www", "example.com"); got != "www.example.com." { - t.Fatalf("qualifyOwner rel: %q", got) + if got := QualifyOwner("www", "example.com"); got != "www.example.com." { + t.Fatalf("QualifyOwner rel: %q", got) } - if got := qualifyOwner("abs.example.", "example.com"); got != "abs.example." { - t.Fatalf("qualifyOwner abs: %q", got) + if got := QualifyOwner("abs.example.", "example.com"); got != "abs.example." { + t.Fatalf("QualifyOwner abs: %q", got) } }