From 1265ac9f4d7b384b28c574ce6af72f4127fa9a65 Mon Sep 17 00:00:00 2001 From: Evan Vetere Date: Sun, 5 Jul 2026 14:23:11 -0400 Subject: [PATCH] fix: enforce single-value CNAME/ALIAS and flag coexistence conflicts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two PowerDNS 422 modes from #51 beyond the duplicate-NS blocker: Mode 2 — the operator could send a CNAME or ALIAS RRset with the same content repeated (multi-valued), which PowerDNS rejects ("only one such record allowed" / "duplicate record"). buildRRSets now keeps a single record for these single-valued types (first non-empty entry wins), so a malformed multi-valued input can no longer produce an invalid payload. Mode 3 — an ALIAS written at a name that already holds a conflicting RRset returns 422 "Conflicts with pre-existing RRset". This is a well-formed record that cannot coexist, not a transient provider error. Classify it (pdns.IsConflict) and surface a distinct Programmed=False reason (Conflict) so it is greppable in the DNSRecordSet status and not mistaken for a retryable PDNS error. Pre-flight conflict prevention (predicting coexistence before the PATCH) is deferred: it needs validated PowerDNS coexistence rules — ALIAS at apex legitimately coexists with SOA/NS — plus integration testing. Closes #55 Closes #56 Refs #51 Co-Authored-By: Claude Opus 4.8 --- internal/controller/conditions.go | 1 + .../dnsrecordset_powerdns_controller.go | 9 ++++- internal/pdns/client.go | 10 +++-- internal/pdns/errors.go | 19 ++++++++++ internal/pdns/errors_test.go | 37 ++++++++++++++++++ internal/pdns/pdns_test.go | 38 +++++++++++++++++++ 6 files changed, 110 insertions(+), 4 deletions(-) diff --git a/internal/controller/conditions.go b/internal/controller/conditions.go index 637a57e..13ec148 100644 --- a/internal/controller/conditions.go +++ b/internal/controller/conditions.go @@ -13,4 +13,5 @@ const ( ReasonDNSZoneInUse = "DNSZoneInUse" ReasonNotOwner = "NotOwner" ReasonPDNSError = "PDNSError" + ReasonConflict = "Conflict" ) diff --git a/internal/controller/dnsrecordset_powerdns_controller.go b/internal/controller/dnsrecordset_powerdns_controller.go index 8e9cf10..17c3dfe 100644 --- a/internal/controller/dnsrecordset_powerdns_controller.go +++ b/internal/controller/dnsrecordset_powerdns_controller.go @@ -199,7 +199,14 @@ func (r *DNSRecordSetPowerDNSReconciler) setRecordProgrammedCondition( newCond.Message = "Another DNSRecordSet owns this record" case pdnsErr != nil: newCond.Status = metav1.ConditionFalse - newCond.Reason = ReasonPDNSError + if pdnsclient.IsConflict(pdnsErr) { + // CNAME/ALIAS coexistence conflict: well-formed record, but it + // cannot share this name with existing data. Distinct reason so it + // is greppable and not mistaken for a transient provider error. + newCond.Reason = ReasonConflict + } else { + newCond.Reason = ReasonPDNSError + } newCond.Message = pdnsclient.FriendlyMessage(pdnsErr) default: newCond.Status = metav1.ConditionTrue diff --git a/internal/pdns/client.go b/internal/pdns/client.go index 4c0faaa..d84f014 100644 --- a/internal/pdns/client.go +++ b/internal/pdns/client.go @@ -456,8 +456,10 @@ func buildRRSets(zone string, rs dnsv1alpha1.DNSRecordSet) []rrset { } target := strings.TrimSpace(rec.CNAME.Content) target = qualifyIfNeeded(target) - if target != "" { - // TODO: Technically this is a violation of the RFC, but we'll allow it for now. + if target != "" && len(r.Records) == 0 { + // CNAME is single-valued (RFC 1034): keep exactly one record. + // First non-empty entry wins; extras/duplicates are dropped, as + // PowerDNS rejects a multi-valued or duplicate CNAME RRset (422). r.Records = append(r.Records, rrsetRecord{Content: target, Disabled: false}) } @@ -604,7 +606,9 @@ func buildRRSets(zone string, rs dnsv1alpha1.DNSRecordSet) []rrset { } target := strings.TrimSpace(rec.ALIAS.Content) target = qualifyIfNeeded(target) - if target != "" { + if target != "" && len(r.Records) == 0 { + // ALIAS is single-valued at an owner name: keep one record. + // First non-empty entry wins; extras/duplicates are dropped. r.Records = append(r.Records, rrsetRecord{Content: target, Disabled: false}) } diff --git a/internal/pdns/errors.go b/internal/pdns/errors.go index 2ee6deb..c365d90 100644 --- a/internal/pdns/errors.go +++ b/internal/pdns/errors.go @@ -57,3 +57,22 @@ func FriendlyMessage(err error) string { return "Failed to apply DNS record. It will be retried automatically." } } + +// IsConflict reports whether err is a PowerDNS rejection caused by a record +// coexistence conflict — a CNAME or ALIAS RRset that cannot share an owner name +// with a pre-existing RRset. This is distinct from a malformed payload: the +// record is well-formed but conflicts with existing data at the same name. +func IsConflict(err error) bool { + if err == nil { + return false + } + var apiErr *pdnsAPIError + if !errors.As(err, &apiErr) || apiErr.Status != 422 { + return false + } + var body pdnsErrorBody + if apiErr.Body != "" { + _ = json.Unmarshal([]byte(apiErr.Body), &body) + } + return strings.Contains(body.Error, "Conflicts with pre-existing RRset") +} diff --git a/internal/pdns/errors_test.go b/internal/pdns/errors_test.go index f820d1c..5a43189 100644 --- a/internal/pdns/errors_test.go +++ b/internal/pdns/errors_test.go @@ -87,3 +87,40 @@ func TestFriendlyMessage(t *testing.T) { }) } } + +func TestIsConflict(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"non-pdns error", errors.New("boom"), false}, + { + "ALIAS coexistence conflict", + &pdnsAPIError{Status: 422, Body: `{"error": "RRset www.ab.dk. IN ALIAS: Conflicts with pre-existing RRset"}`}, + true, + }, + { + "non-conflict 422 (duplicate record)", + &pdnsAPIError{Status: 422, Body: `{"error": "RRset x. IN NS: duplicate record with content \"ns1.\""}`}, + false, + }, + { + "non-422 status", + &pdnsAPIError{Status: 500, Body: `{"error": "Conflicts with pre-existing RRset"}`}, + false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := IsConflict(tt.err); got != tt.want { + t.Errorf("IsConflict() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/internal/pdns/pdns_test.go b/internal/pdns/pdns_test.go index e0ce57e..cee969d 100644 --- a/internal/pdns/pdns_test.go +++ b/internal/pdns/pdns_test.go @@ -677,6 +677,44 @@ func TestApplyRecordSetAuthoritative_PathAndHeaders(t *testing.T) { } } +// CNAME and ALIAS are single-valued at an owner name: multiple/duplicate +// entries must collapse to exactly one record (PowerDNS rejects otherwise, 422). +func TestBuildRRSets_CNAMEAliasSingleValue(t *testing.T) { + t.Parallel() + + rsCNAME := dnsv1alpha1.DNSRecordSet{ + Spec: dnsv1alpha1.DNSRecordSetSpec{ + RecordType: dnsv1alpha1.RRTypeCNAME, + Records: []dnsv1alpha1.RecordEntry{ + {Name: "www", CNAME: &dnsv1alpha1.CNAMERecordSpec{Content: "target.example.net."}}, + {Name: "www", CNAME: &dnsv1alpha1.CNAMERecordSpec{Content: "target.example.net."}}, + {Name: "www", CNAME: &dnsv1alpha1.CNAMERecordSpec{Content: "other.example.net."}}, + }, + }, + } + rr := buildRRSets("example.com", rsCNAME) + if len(rr) != 1 || len(rr[0].Records) != 1 { + t.Fatalf("CNAME single-value: expected 1 rrset/1 record, got %#v", rr) + } + if rr[0].Records[0].Content != "target.example.net." { + t.Fatalf("CNAME single-value: first-wins expected target.example.net., got %q", rr[0].Records[0].Content) + } + + rsALIAS := dnsv1alpha1.DNSRecordSet{ + Spec: dnsv1alpha1.DNSRecordSetSpec{ + RecordType: dnsv1alpha1.RRTypeALIAS, + Records: []dnsv1alpha1.RecordEntry{ + {Name: "@", ALIAS: &dnsv1alpha1.ALIASRecordSpec{Content: "lb.example.net."}}, + {Name: "@", ALIAS: &dnsv1alpha1.ALIASRecordSpec{Content: "lb.example.net."}}, + }, + }, + } + rr = buildRRSets("example.com", rsALIAS) + if len(rr) != 1 || len(rr[0].Records) != 1 { + t.Fatalf("ALIAS single-value: expected 1 rrset/1 record, got %#v", rr) + } +} + // sanity: makeSimpleRRSet keeps values verbatim (used after we normalize) func TestMakeSimpleRRSet(t *testing.T) { t.Parallel()