Skip to content

Acme dns provider support#218

Open
martcpp wants to merge 5 commits into
mainfrom
ACME-DNS_provider_support
Open

Acme dns provider support#218
martcpp wants to merge 5 commits into
mainfrom
ACME-DNS_provider_support

Conversation

@martcpp

@martcpp martcpp commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

The ACME-DNS provider holds a single account (one server URL / username /password / subdomain). Because acme-dns keeps only the two most recent TXT values per record by design, every domain's _acme challenge CNAME points at the same acme-dns record, and only the last two written digests survive.

Acceptance criteria

  • Per-domain credential selection implemented and config-driven.
  • Backward compatible: existing single-account config works unchanged.
  • wiremock tests cover per-domain selection and the 3-identifier case that currently fails.
  • Docs updated to describe the map and remove the ≤2-identifier caveat once lifted.

@Hermann-Core Hermann-Core left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved The ACME-DNS multi-domain support is correctly implemented: ✓ Per-domain credential selection works as specified ✓ Backward compatible: single-account config unchanged ✓ Two-value limit enforced at startup via check_order_domains() ✓ Comprehensive wiremock tests cover all scenarios ✓ Config validation catches misconfigurations at boot ✓ Domain normalization is consistent and correct ✓ Docs updated with clear examples The implementation handles all edge cases (conflicting accounts, empty fields, apex+wildcard sharing accounts) and surfaces configuration errors at startup rather than at renewal time. Tests pass, code is idiomatic.

@Hermann-Core

Copy link
Copy Markdown
Collaborator

@martcpp fix the conflicts on this branch and let's merge this ticket.

@martcpp

martcpp commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

@martcpp fix the conflicts on this branch and let's merge this ticket.

on it

@martcpp

martcpp commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

@ndefokou check this pr let merge thanks

@ndefokou ndefokou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against issue #207. Implementation is solid and well-tested at the provider/config layer, but I have a few concerns — the main one being that the feature is effectively unreachable end-to-end because the ACME order is always for the single server.domain. Details inline.

Comment thread src/utils/state.rs
Comment thread src/utils/state.rs
Comment thread src/utils/cert_manager/challenge/dns01/acme_dns.rs
Comment thread src/config.rs
Comment thread src/utils/cert_manager/challenge/dns01/acme_dns.rs Outdated
@martcpp

martcpp commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

@ndefokou recheck

@martcpp
martcpp requested a review from ndefokou July 17, 2026 10:33

@ndefokou ndefokou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

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.

ACME-DNS provider: support >2 identifiers per certificate via per-domain credentials

3 participants