fix: Stop respelled owner names deleting live PowerDNS RRsets

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,13 +167,54 @@ 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
 	}
 
 	logger.Info("powerdns reconcile complete")
 	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,

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

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

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