Skip to content

feat: let providers declare a record identity for duplicate checks - #4883

Merged
TomOnTime merged 5 commits into
DNSControl:mainfrom
Alice39s:feat/record-identity
Sep 22, 2026
Merged

TomOnTime merged 5 commits into
DNSControl:mainfrom
Alice39s:feat/record-identity

Conversation

@Alice39s

Copy link
Copy Markdown
Contributor

I ran into this while moving a geo-routed zone to DNSControl.

DNSPod, Gcore, Huawei Cloud and ClouDNS keep one record per line, so several
records share a name and type. That is how these providers express per-region
answers.

Two checks block those configs before the provider ever sees them.

checkCNAMEs rejects a second CNAME under the same name, so a geo-routed CNAME
set cannot be declared at all.

checkDuplicates compares name, type and RDATA and never looks at provider
metadata, so two records that differ only by line count as duplicates even
though the provider stores them separately.

The R53_WEIGHT example in the docs fails the same check, so this is not
specific to any one provider.

The change adds an optional identity function to the provider interface. A
provider that declares one gets its identity text appended to the duplicate
key. A provider that declares nothing keeps today's rules.

Route 53 weighted records and Cloudflare flattened CNAMEs already rely on
hard-coded exceptions to the same checks. A generic hook gives other providers
a supported way in.

@github-actions github-actions Bot added the provider-TENCENTDNS Tencent Cloud DNS provider label Sep 15, 2026
@TomOnTime

Copy link
Copy Markdown
Collaborator

Hi there!

Thanks for submitting this! I didn't even realize it was a problem. Thank you for reporting it.

Let me suggest an easier way to fix this. It might not be better or equivalent. I'm interested in your feedback.

What if we added a metadata flag similar to DISABLE_REPEATED_DOMAIN_CHECK, perhaps called DISABLE_DUPLICATE_RECORD_CHECK? Records with that metadata would be exempt from the checkDuplicates and checkCNAMEs checks.

The benefit would be that it would not require provider changes. The downside is that it could lead to some false negatives which will later be detected/rejected at push time.

Tom

@TomOnTime
TomOnTime requested review from KlettIT and removed request for KlettIT September 15, 2026 19:29
@Alice39s

Alice39s commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for the suggestion, and for spelling out the trade-off.

I would like to keep the provider-declared identity. The main argument for it is already in this repo.

The two testgroups already in integrationTest/integration_test.go on main describe exactly this shape. TENCENTDNS_LINE_WEIGHT declares one name, type and value twice with different tencentdns_line. R53_WEIGHT_CNAME declares one name twice as a CNAME.

Both pass today because the integration path goes through zonerecs.CorrectZoneRecords and never runs pkg/normalize. Put the same config through check or preview and both fail.

The provider side already declares this identity. recordMetadataComparable in providers/tencentdns carries the comment "makes DNSPod's record line and weight part of a record's identity", route53 has r53ComparableFunc, and pkg/normalize reads provider metadata when it checks R53 weighted groups. Duplicate detection is the one place that computes its own key.

For DNSPod the key is measurable. The account exposes a read-only conflict check, DescribeRecordImpactInfo, and it returns the same InvalidParameter.DomainRecordExist that CreateRecord returns. Unknown parameters are rejected, so the input set can be enumerated: it takes SubDomain/RecordLine/RecordType/Value and rejects TTL, Weight and GroupId.

Probing existing records against it returns the key as (name, line, type, value), with the line matched by RecordLineId. #4775 states the same rule in English from the project's own test logs: "duplicate records on the same default line are forbidden".

A flag stops at validation. With the same per-line config, the provider's comparable produces the expected CREATEs; replace it with nil and the diff reports zero changes, because the engine sees one record and finds everything in place. Validation can allow or reject that config either way, and the engine still decides on its own what identity means.

checkCNAMEs already carries two provider-driven exceptions: AKAMAICDN, and the Cloudflare proxied CNAME from #4189.

Cost is close either way. The hook is one optional field on DspFuncs, and providers that do not declare it keep today's behavior. A per-record flag needs a new metadata key, its entry in the generated others.d.ts, three conditionals with their messages in validate.go, and a documentation page. v4.0.0 moved the other way by collapsing per-record escape hatches into domain-level ones, which is the level such a switch belongs at.

Validation runs before the provider has fetched the zone, so the identity
function cannot depend on a record set. It takes a single record now, and the
tencentdns implementation reads only that record: the line ID, falling back to
the line name. Weight is no longer part of it, because the service keys records
on name, line, type and value and rejects two records that differ only by
weight.
Providers that store per-line records now have a documented hook: what it does,
the rule that it must depend only on the record it is given, and what happens
when a domain has several providers. The tencentdns page states that duplicate
detection uses the key the service uses, and that mixing a line name with a line
ID describes the same line twice.

Tests cover weight, line names on their own, and the record-only signature.
@Alice39s
Alice39s force-pushed the feat/record-identity branch from 5469f95 to 9240964 Compare September 16, 2026 13:40
@Alice39s
Alice39s marked this pull request as draft September 16, 2026 13:52
@Alice39s
Alice39s force-pushed the feat/record-identity branch 4 times, most recently from 2a638e6 to 0a31d9f Compare September 16, 2026 14:24
Validation now treats a record without line metadata as the default line, so it
matches an explicit line ID of 0. The default line also has one name and one ID.

The tests describe a generic zone with RFC 5737 addresses, and the provider
without an identity function is registered instead of unknown. The provider docs
note where validation cannot see the zone.
@Alice39s
Alice39s force-pushed the feat/record-identity branch from 0a31d9f to 039e2fc Compare September 16, 2026 14:24
@Alice39s
Alice39s marked this pull request as ready for review September 16, 2026 14:50
@TomOnTime

Copy link
Copy Markdown
Collaborator

Ok!

@TomOnTime
TomOnTime merged commit e6e1fb7 into DNSControl:main Sep 22, 2026
3 checks passed
TomOnTime pushed a commit that referenced this pull request Sep 24, 2026
As discussed in #4897, this keeps the table of contents that #4818 added
and makes its links work on https://docs.dnscontrol.org next to
GitBook's "On this page" navigation. All pages are done in this one pull
request, as requested.

The TOC no longer starts with an entry for the page title, because
GitBook renders the H1 as the page title without an anchor. The links
now use the anchors GitBook generates. Besides the two differences
mentioned in the issue (periods are kept, and a heading that starts with
a number gets an `id-` prefix), GitBook also turns a `/` into a dash and
a `&` into `and`: "Don't conditionally add/remove trailing dots" becomes
`#dont-conditionally-add-remove-trailing-dots`. I compared every heading
of the 35 published pages with the `id` attributes on
docs.dnscontrol.org, and all TOC links now match an existing anchor.

A few other changes came along:

- Step headings use a colon everywhere (`Step 1: Pick a unique id`)
instead of a mix of `Step 1.` and `Step 1:`, matching
`writing-providers.md` and the "Step 1: Foo" example in
`styleguide-doc.md`. A colon is dropped by GitHub and GitBook alike, so
these anchors are the same on both sites.
- Headings that used ` - ` or `&` now use a colon or "and"
(`ci-cd-gitlab.md`, `github-actions.md`, `goreleaser.md`), so their
anchors are the same on GitHub and GitBook as well.
- The TOC stops at H4. GitBook renders an H5 as bold text without an
anchor, so the six numbered steps under "Steps to activate" in
`goreleaser.md` couldn't be linked on GitBook anyway.
- The TOCs of `debugging-with-dlv.md` (still linking to the old
"Debugger" title and missing "Debug `helpers.js`") and
`writing-providers.md` (missing "Record identity for providers with
per-line records" from #4883) were out of date and are regenerated from
the headings.
- `modernizingproviders.md` isn't in `SUMMARY.md`, so it isn't published
on GitBook. It only loses the entry for the page title and keeps the
GitHub anchors.
- `provider/index.md` is left alone, because its "Jump to a table" list
is generated and its anchors already match.

The trade-off is GitHub: 33 of the 288 links on the published pages use
a GitBook anchor that GitHub generates differently, so they don't jump
to the heading when you read the Markdown on github.com. The VS Code
plug-in would also write GitHub anchors and the H1 entry back when it
regenerates a TOC, so it's worth limiting it to levels 2 to 4 and
checking the anchors of headings with a period, a leading number, a `/`
or a `&` after regenerating.

Fixes #4897

## AI-attributie

Assisted-by: Claude Code:claude-opus-5-5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

provider-ROUTE53 provider-TENCENTDNS Tencent Cloud DNS provider

Development

Successfully merging this pull request may close these issues.

2 participants