Skip to content

Conversation

@rightaditya
Copy link
Contributor

I was getting the following spurious error:

# cat Caddyfile
{
  acme_ca https://acme-staging-v02.api.letsencrypt.org/directory
  acme_dns cloudflare REDACTED_API_KEY
}

*.mydomain.com {
  tls {
    resolvers 1.1.1.1 1.0.0.1
  }
}
# caddy run -c Caddyfile
<snip>
Error: adapting config using caddyfile: parsing caddyfile tokens for 'tls': setting DNS challenge options [resolvers] requires a DNS provider (set with the 'dns' subdirective or 'acme_dns' global option), at Caddyfile:9

This brings the logic in line with the intended behaviour implied by the error message. (If the current behaviour is correct, then the error message should be changed accordingly.)

Assistance Disclosure

No AI was used.

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2025

CLA assistant check
All committers have signed the CLA.

@rightaditya rightaditya changed the title add missing check for acme_dns Add missing DNS challenge check for acme_dns Sep 22, 2025
@francislavoie
Copy link
Member

Please add an adapter test to cover this case (see other .caddyfiletest files). You can use mock as the DNS provider name which is available in tests, instead of cloudflare.

@francislavoie francislavoie changed the title Add missing DNS challenge check for acme_dns httpcaddyfile: Add missing DNS challenge check for acme_dns Sep 22, 2025
@rightaditya rightaditya force-pushed the fix-acme-dns-resolvers branch 2 times, most recently from ac1adb9 to 108ac9d Compare September 27, 2025 02:58
@rightaditya
Copy link
Contributor Author

@francislavoie Good point—after testing (and adding the test case), I found that the resolvers were being blown away by the presence of acme_dns. I've just updated my branch with the test and a fix for the newly-uncovered issue. PTAL.

The new issue doesn't just affect resolvers, BTW. If I have a local dns value set, acme_dns overrides it. My commit fixes that as well. I think it'd be worth adding a test for that too, but I wasn't sure about the best/preferred way to do this; AIUI, properly testing that would require a second mock provider (or allowing the mock provider to take some arguments), since they'd be indistinguishable otherwise.

@francislavoie
Copy link
Member

Yeah makes sense re mock, go ahead and add a simple optional string field to it, useful for testing for sure.

@rightaditya rightaditya force-pushed the fix-acme-dns-resolvers branch from 108ac9d to d36bad4 Compare September 28, 2025 06:37
@rightaditya
Copy link
Contributor Author

@francislavoie Cool, just updated. Also reordered the commits to make a bit more sense. LMK if you'd prefer them squashed etc.

@francislavoie
Copy link
Member

Commit order etc doesn't matter, we squash-merge when the PR is done

Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

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

Looks perfect to me, thank you!

@francislavoie francislavoie added this to the v2.10.3 milestone Sep 28, 2025
@mholt mholt merged commit 3c003de into caddyserver:master Oct 3, 2025
23 checks passed
@francislavoie francislavoie modified the milestones: v2.10.3, v2.11.0 Oct 16, 2025
@mohammed90 mohammed90 mentioned this pull request Oct 25, 2025
45 tasks
@github-actions github-actions bot mentioned this pull request Dec 3, 2025
4 tasks
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.

4 participants