Skip to content

fix: dedupe nameservers to stop duplicate-NS 422 on new zones#54

Closed
ecv wants to merge 2 commits into
mainfrom
fix/pdns-duplicate-ns-422
Closed

fix: dedupe nameservers to stop duplicate-NS 422 on new zones#54
ecv wants to merge 2 commits into
mainfrom
fix/pdns-duplicate-ns-422

Conversation

@ecv

@ecv ecv commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

What

Newly-created DNSZones in prod failed to program: PowerDNS rejected the default NS RRset PATCH with 422 "duplicate record with content", so zones never reached Accepted/Programmed. This blocked every new zone and the infra Integration Chainsaw dns-setup test (#51, Mode 1 — the fleet-wide blocker).

Root cause

The DNSZoneClass NameServerPolicy.Static.Servers list carried each nameserver more than once, and nothing on the path deduped it:

  • normalizeStringSlice sorted but kept duplicates → doubled Status.Nameservers (downstream, then mirrored upstream).
  • ensureNSRecordSet emitted one NS RecordEntry per (duplicate) entry.
  • buildRRSets NS case appended one record per entry → the PATCH carried duplicate NS records → 422.

Trailing-dot variants (ns1.datumdns.net vs ns1.datumdns.net.) made it worse: they only collapse to identical content after qualifyIfNeeded, so a raw-string dedupe alone isn't sufficient.

Fix (three NS-scoped dedupe points)

  • normalizeStringSlice — drop duplicate entries. Cleans both downstream and upstream Status.Nameservers at the source (also makes the CR status correct).
  • CreateZone — dedupe the apex nameserver list sent at zone creation.
  • buildRRSets NS case — dedupe by qualified content, collapsing exact dups and trailing-dot variants right before the PATCH is built. This is the guaranteed 422 fix regardless of what the source list contains.

Scope

Mode 1 (duplicate NS) only, per triage. The other two modes from #51 are tracked as sub-issues and a separate PR:

  • Mode 2 — CNAME multi-valued/duplicate content (single-value enforcement + dedupe).
  • Mode 3 — ALIAS written where another RRset already exists (conflict handling).

Complementary: #52 surfaces the specific pdns 422 detail into the DNSRecordSet status.

Test

  • internal/pdnsTestBuildRRSets_NSDedup: exact dup + trailing-dot variant collapse to one record.
  • internal/controllerTestNormalizeStringSlice: duplicates removed.
  • go build ./... clean; affected unit tests pass. (The pdns package's Docker/testcontainers integration test is unrelated and not run here.)

Refs #51

🤖 Generated with Claude Code

New DNSZones failed to program in prod: PowerDNS rejected the default
NS RRset PATCH with 422 "duplicate record with content", so zones never
reached Accepted/Programmed. Root cause: the DNSZoneClass Static
nameserver list carried each nameserver more than once, and nothing on
the path deduped it — normalizeStringSlice sorted but kept duplicates,
and buildRRSets appended one NS record per entry.

Dedupe at three NS-scoped points:
- normalizeStringSlice: drop duplicate entries (cleans downstream and
  upstream Status.Nameservers).
- CreateZone: dedupe the apex nameserver list sent at zone creation.
- buildRRSets NS case: dedupe by qualified content, which also collapses
  trailing-dot variants (ns1.example.net vs ns1.example.net.) that only
  become identical after qualification.

Fixes the fleet-wide new-zone failure and unblocks the Integration
Chainsaw dns-setup test.

Refs: #51

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ecv

ecv commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #53, which is the broader fix: its dedupeRecords helper dedupes every record type at the buildRRSets emit boundary plus ReplaceRRSet, and dedupes both normalizeStringSlice and normalizeDomainNameservers. This PR only covered the NS path (CreateZone + the NS case in buildRRSets + normalizeStringSlice), leaving ReplaceRRSet and normalizeDomainNameservers un-deduped. #53 also collapses trailing-dot NS variants (content is already qualified before dedup) and is green on CI. Closing as duplicate; tracking the fix in #53.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant