Handle multiple concurrent Azure DNS01 challenges for the same FQDN

[eplightning]

Pull Request Motivation

This PR fixes a race condition in Azure DNS that happens when issuing certificates concurrently in different clusters as described in #6229.

Similar issues were already fixed for AWS/GCP/Cloudflare in other PRs:

• Handle multiple Cloudflare DNS01 challenges for the same FQDN #6191
• issuer: acme: clouddns: Fix race condition #6088
• Use multivalue records instead of simple records  #4793

To make this work in Azure DNS, this PR makes use of optimistic concurrency control supported by Azure DNS API via standard If-Match, If-None-Match, Etag headers: https://learn.microsoft.com/en-us/rest/api/dns/record-sets/create-or-update?tabs=HTTP

Current version simply unconditionally overwrites any changes. New version introduced in this PR does the following for all updates:

1. Try to fetch an already existing matching record. If found, Azure DNS will return the record set and its version (Etag).

2. Modify record set as necessary: Append if doesn't exist on Present, remove if exists on CleanUp

3. Call appropriate Azure API:

• If after modifications no records remain, call Delete API. Etag will ensure we will only delete record set if no modifications happened concurrently. Call is skipped if step 1 didn't find a matching record set.

• If there are records and a matching record set was not found: call CreateOrUpdate API with If-None-Match: *. This will ensure we only create and not update (otherwise we would overwrite other changes).

• If there are records and we found a matching record set: call CreateOrUpdate API with If-Match: $Etag. Call will only succeed if no other updates happened concurrently.

4. If any precondition fail, Azure DNS will return an error which will be eventually retried by cert-manager.

Kind

/kind bug

Release Note

Fix ACME issuer being stuck waiting for DNS propagation when using Azure DNS with multiple instances issuing for the same FQDN 

[irbekrm]

/ok-to-test

[eplightning]

/retest

[eplightning]

/retest

[jetstack-bot]

[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 jakexks for approval. For more information see the Kubernetes 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:

• OWNERS

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

[eplightning]

Alright rebasing seems to have fixed the bot and tests not passing

[jetstack-bot]

PR needs rebase.

Details

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.

[jetstack-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
/lifecycle stale

[eplightning]

/remove-lifecycle stale

I'll rebase this eventually ...

[oliverplann]

@eplightning @irbekrm @maelvls Sorry for tagging you, but is there any update on this whatsoever? The PR was created quite a long time ago and similar fixes have been merged across other DNS providers.

There has been quite a lot of mentions on this e.g.:

• Race condition when two identical certificate requests are made from different clusters #6229
• Handle multiple Cloudflare DNS01 challenges for the same FQDN #6191
• Multi-domain wildcard certificate validation fails when using DNS delegation #3460

I can confirm this also happens on a single cluster attempting to issue either multiple certificates or multiple dnsNames using the same less-priviliged domain in Azure.

I'm not a GO expert, but I have Azure infrastructure available to help you test, if you instruct me how. Currently I'm installing cert-manager through the official helm chart, but I imagine I'll have to do something else in this case.

Furthermore, is there any known workaround as long as this issue persists?

// Oliver

[eplightning]

@oliverplann I rebased the PR, so whatever happens next is up to this project's maintainers.

I'm not aware of any workarounds, whenever this issue happens either a manual DNS update is required or the certificate issuance needs to be somehow attempted again - probably by removing CertificateRequest? (I don't really remember at this point honestly).

[eplightning]

/retest

[oliverplann]

@oliverplann I rebased the PR, so whatever happens next is up to this project's maintainers.

I'm not aware of any workarounds, whenever this issue happens either a manual DNS update is required or the certificate issuance needs to be somehow attempted again - probably by removing CertificateRequest? (I don't really remember at this point honestly).

Thank you for acting quick. I hope the maintainers will prioritize this...

[oliverplann]

TLDR
I'll test this PR and have also found a suitable workaround:
Using Helm: Set the value maxConcurrentChallenges: 1 (Default: 60)
Using kubectl apply: Edit deployment.yaml and set --max-concurrent-challenges=1

Original
I've reached out to the maintainers on the Kubernetes slack and they've instructed me how to test this PR, which I will get done today or tomorrow.

Similarly to a multi-cluster setup, if you are using a delegation domain (source) to issue multiple certificates or dnsNames e.g. _acme-challenge.less-privileged.example.org cert-manager will reuse the same TXT record for every challenge in the cluster. This currently results in an edge case where each challenge will randomly overwrite the value of the TXT record, before the previous challenge succeeded. This in turn puts most, if not all of the challenges in a pending state.

As long as this bug exists, it's facilitated through the default value of maxConcurrentChallenges.

I've found a suitable workaround until this PR has been adressed. Sharing it here in case anyone else needs a working cert-manager without manually interacting with cert-manager resources or Azure DNS:

Using Helm: Set the value maxConcurrentChallenges: 1 (Default: 60)
Using kubectl apply: Edit deployment.yaml and set --max-concurrent-challenges=1

Keep in mind that It will take significantly longer for cert-manager to validate challenges and issue certificates this way, depending on the amount of certs and dnsNames in your environment, but it works!
Also this shouldn't create issues renewing existing certs, as cert-manager will wait until every challenge for a given certificate is Valid before it changes the Ready state of the certificate.

Here's an example of what challenges will look like with maxConcurrentChallenges set to 1, handling 1 at a time:

example-com-certificate-1-2696367434-4084983421           example.com         4m55s example-org-certificate-1-2696367434-410157576              k8s.example.org   4m55s example-org-certificate-1-2696367434-413199771              tst.k8s.example.org 4m55s example-net-certificate-1-1612508506-2239642643   pending   k8s.example.net  4m56s example-net-certificate-1-1612508506-2283501741   valid     example.net       4m56s example-net-certificate-1-1612508506-4097195913   valid     example.net        4m56s 

[oliverplann]

Hello @eplightning

I've tested your PR using AKS & Azure DNS and it works perfectly! Thank you very much for your contribution.
I reached out to @SgtCoDFish on Slack and he'll look into merging this when he has time.

My findings below.

Challenges are now adding separate entries/values to the _acme-challenge TXT record in Azure DNS, as opposed to overwriting the value each time:

kubectl get challenges shows that multiple challenges are being evaluated concurrently and their state are steadily changing to Valid:

NAME STATE DOMAIN example-com-certificate-1-196084347-1516893050 pending tst.k8s.example.com example-com-certificate-1-196084347-2394518141 valid k8s.example.com example-com-certificate-1-196084347-3341303458 example.com example-org-certificate-1-196084347-3621474682 pending tst.k8s.example.org example-org-certificate-1-196084347-4044510119 pending k8s.example.org example-org-certificate-1-196084347-4127470729 valid example.org example-net-certificate-1-2696367434-2908286604 pending tst.k8s.example.net example-net-certificate-1-2696367434-3121967868 valid k8s.example.net example-net-certificate-1-2696367434-3152871262 valid example.net 

Secrets & certificates are created:

NAME TYPE DATA AGE example-com-tls-secret kubernetes.io/tls 2 93m example-net-tls-secret kubernetes.io/tls 2 93m example-org-tls-secret kubernetes.io/tls 2 93m NAME READY SECRET AGE example-com-certificate True example-com-tls-secret 93m example-net-certificate True example-net-tls-secret 93m example-org-certificate True example-org-tls-secret 93m 

Please let me know if anything is missing before we can merge this PR :-)

// Oliver

[SgtCoDFish]

/lgtm
/approve

We're not really able to easily test Azure changes so big thanks to @oliverplann for testing this! And of course, thank you to everyone else involved for your patience and your efforts here. This should make it into 1.15 for sure.

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [SgtCoDFish]

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