-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support CNAME for dns-01 challenge #670
Conversation
Domain for which certificate is asked for can have a CNAME, so we should check it. If domain has a CNAME, create the challange TXT record in the alias domain. This is useful in the scenario where a company like us is using some DNS provider which is not supported dynamically. We can then create a CNAME for records like _acme-challenge.example.com -> example.aws.hosted.com So this will allow us getting cert for *.example.com with creating txt record in route53 for above exxample.
Hi @gurvindersingh. Thanks for your PR. I'm waiting for a cert-manager or jetstack member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Thanks very much for this PR, there's been a few requests for this made in the past! 😄 /ok-to-test |
@munnerz any update on this PR |
/assign @kragniz |
Thanks @gurvindersingh! I've sucessfully run this using digitalocean, delegating to google clouddns for challenges. With the addition of #750, it's working fine. /lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kragniz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments that can be addressed in a follow up.
Thanks very much for the addition @gurvindersingh!
/cc @kragniz
|
||
// Check if the domain has CNAME then return that | ||
r, err := dnsQuery(fqdn, dns.TypeCNAME, RecursiveNameservers, true) | ||
if err == nil && r.Rcode == dns.RcodeSuccess { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the err != nil due to some failure other than record does not exist?
for _, rr := range r.Answer { | ||
if cn, ok := rr.(*dns.CNAME); ok { | ||
if cn.Hdr.Name == fqdn { | ||
glog.Infof("Updating FQDN: %s with it's CNAME: %s", fqdn, cn.Target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it's -> its
fqdn := fmt.Sprintf("_acme-challenge.%s.", domain) | ||
|
||
// Check if the domain has CNAME then return that | ||
r, err := dnsQuery(fqdn, dns.TypeCNAME, RecursiveNameservers, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RecursiveNameservers does not respect the recently added --dns01-self-check-nameservers
flag. We should plumb this into this point too imo.
/retest |
Even with #750 it's still not working for me. This seems to not entirely solved the issue? textPayload: "Updating FQDN: _acme-challenge.app.xxx.xxx.xxx with it's CNAME: app-acme.yyy.yyy.yyy" textPayload: "Error preparing issuer for certificate prod/ingress-public-svcs-1-prod-lseprod-tls: dns-01 self check failed for domain "app.xxx.xxx.xxx"" So it's looking for the domain "app.xxx.xxx.xxx" instead of following the CNAME record pointing to app-acme.yyy.yyy.yyy The current fix "zone, err := c.getHostedZone(fqdn)" does not seem to follow the configured CNAME, if the CNAME belongs to a different zone than the original requested FQDN. Please help. |
@gurvindersingh Could you please post your config's in order to obtain wildcard cert for
providers:
- name: route53-example-cloud
cnameStrategy: "Follow"
route53:
region: us-east-1
hostedZoneID: blabla
ingress:
annotations:
kubernetes.io/tls-acme: "true"
certmanager.k8s.io/acme-challenge-type: "dns01"
certmanager.k8s.io/acme-dns01-provider: "route53-example-cloud"
tls:
- hosts:
- '*.example.com'
secretName: wildcard-example-com-tls
rules:
- host: echo.example.cloud in cert-manager logs I get
nothing happens and that line does not look right to me. |
What this PR does / why we need it:
Domain for which certificate is asked for can have a
CNAME
, so we should check it.If domain has a
CNAME
, create the challengeTXT
record in the alias domain.This is useful in the scenario where a company like us is using some DNS provider
which is not supported dynamically. We can then create a CNAME for records like
_acme-challenge.example.com -> example.aws.hosted.com
So this will allow us getting cert for
*.example.com
with creating aTXT
record in route53 hosted zone for above example.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
Release note: