Skip to content

fix(mx): keep null MX and strip trailing dot - #6661

Open
mloiseleur wants to merge 5 commits into
kubernetes-sigs:masterfrom
mloiseleur:fix/mx-trailing-dot
Open

fix(mx): keep null MX and strip trailing dot#6661
mloiseleur wants to merge 5 commits into
kubernetes-sigs:masterfrom
mloiseleur:fix/mx-trailing-dot

Conversation

@mloiseleur

@mloiseleur mloiseleur commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

What does it do ?

NewEndpointWithTTL trimmed the trailing dot off the whole MX target, turning the null MX
0 . (RFC 7505) into 0 , which no longer parses as an MX record.

source/crd.go passed MX targets through unchecked while every other source normalizes them
via NewEndpointWithTTL, so a DNSEndpoint with 10 mail.example.com. diffed on every
reconcile — on any provider.

It now normalizes instead, leaving unparseable targets and the null MX alone.

Motivation

Follow-up to #6660, split out per review.

More

  • Yes, this PR title follows Conventional Commits
  • Yes, I added unit tests
  • Yes, I updated end user documentation accordingly

@kubernetes-prow

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kubernetes-prow kubernetes-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 24, 2026
@kubernetes-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign szuecs for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubernetes-prow
kubernetes-prow Bot requested review from Raffo and szuecs August 24, 2026 07:45
@kubernetes-prow kubernetes-prow Bot added provider Issues or PRs related to a provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 24, 2026
@coveralls

coveralls commented Aug 24, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 33159371836

Coverage increased (+0.01%) to 82.083%

Details

  • Coverage increased (+0.01%) from the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • 14 coverage regressions across 3 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

14 previously-covered lines in 3 files lost coverage.

File Lines Losing Coverage Coverage
cloudflare/cloudflare_batch.go 8 96.28%
crd.go 4 89.61%
cloudflare/cloudflare.go 2 93.31%

Coverage Stats

Coverage Status
Relevant Lines: 21349
Covered Lines: 17524
Line Coverage: 82.08%
Coverage Strength: 1465.38 hits per line

💛 - Coveralls

@kubernetes-prow kubernetes-prow Bot added source needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 26, 2026
@mloiseleur

Copy link
Copy Markdown
Collaborator Author

Verified end to end against a real PowerDNS in Docker with a kind cluster and the CRD source, comparing master, the previous head of this branch, and the current one.

DNSEndpoint used:

spec:
  endpoints:
    - dnsName: example.com
      recordType: MX
      recordTTL: 300
      targets: ["10 mail.example.com.", "010  backup.example.com"]
    - dnsName: nomail.example.com
      recordType: MX
      recordTTL: 300
      targets: ["0 ."]

master:

CREATE: example.com 300 IN MX  010  backup.example.com;10 mail.example.com. []
fatal: unable to patch zone: Record example.com./MX '010  backup.example.com.':
       Not in expected format (parsed as '10 backup.example.com.')

Previous head of this branch — trailing dot fixed, still fatal:

CREATE: example.com 300 IN MX  010  backup.example.com;10 mail.example.com []
fatal: unable to patch zone: Record example.com./MX '010  backup.example.com.': Not in expected format

Current head:

run 1: CREATE: example.com 300 IN MX  10 backup.example.com;10 mail.example.com []
       CREATE: nomail.example.com 300 IN MX  0 . []
run 2: All records are already up to date
run 3: All records are already up to date
run 4: All records are already up to date
dig +short @127.0.0.1 -p 1053 example.com MX        -> 10 backup.example.com. / 10 mail.example.com.
dig +short @127.0.0.1 -p 1053 nomail.example.com MX -> 0 .

PowerDNS rejects a non-canonical MX target server-side, so the whole zone patch fails and the ownership TXT records and the null MX never get written either — not only reconcile churn.

Setup follows docs/tutorials/pdns.md, with --source=crd and --managed-record-types repeated once per type (it is a kingpin Strings(); a single comma-joined value parses as one element and silently filters everything out).

@mloiseleur
mloiseleur marked this pull request as ready for review August 28, 2026 08:13
@kubernetes-prow kubernetes-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 28, 2026
@kubernetes-prow
kubernetes-prow Bot requested a review from vflaux August 28, 2026 08:13
NewEndpointWithTTL trimmed the trailing dot off the whole MX target, turning
the null MX "0 ." (RFC 7505) into "0 ", which no longer parses as an MX record.

On cloudflare, the CRD source exempts MX from its target format check while the
API never returns a trailing dot, so "10 mail.example.com." diffed on every
reconcile and getRecordID could not resolve the record to delete.
Trailing dots reached every provider, not just cloudflare. Unparseable
targets and the null MX are left as-is.
Render MX targets as "<preference> <host>" in NewEndpointWithTTL and the
CRD source, replacing the Cloudflare-only normalization: a single space,
no leading zeros on the preference, no trailing dot on the host, with
unparseable targets and the null MX "0 ." left alone.

PowerDNS rejects a non-canonical target server-side, so the whole zone
patch fails rather than merely diffing on every reconcile. Doing this in
endpoint covers pdns, google and both Azure providers as well.
@mloiseleur
mloiseleur force-pushed the fix/mx-trailing-dot branch from a87dc1f to 89e9a3e Compare August 28, 2026 08:17
@kubernetes-prow kubernetes-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2026
NormalizeMXTarget already renders MX targets undotted before they reach
newCloudFlareChange, so the extra trim there did nothing. Revert the
helper to its original name and scope: only SRV keeps a trailing dot
this far down the write path.
@mloiseleur
mloiseleur force-pushed the fix/mx-trailing-dot branch from e5bd5e3 to 3bfd8f0 Compare August 28, 2026 09:27
@mloiseleur mloiseleur changed the title fix(mx): keep null MX and strip trailing dot on cloudflare fix(mx): keep null MX and strip trailing dot Aug 28, 2026
@ivankatliarchuk

Copy link
Copy Markdown
Member

/lgtm

@kubernetes-prow kubernetes-prow Bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. provider Issues or PRs related to a provider size/L Denotes a PR that changes 100-499 lines, ignoring generated files. source

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants