issuer: acme: clouddns: Fix race condition

[cypres]

Pull Request Motivation

If you are running multiple cert managers, like in different clusters, they can race against each other when using Google CloudDNS dns01 issuer.

This is because cert-manager removes any previous txt records under _acme-challenge for the FQDN it's trying to issue. This causes a state where cert-manager will have successfully done the Present() call, but then never complete the challenge.

Log lines associated with this would be like:

cert-manager/challenges "msg"="propagation check failed" "error"="DNS record for "foo.example.com" not yet propagated" "dnsName"="foo.example.com" "resource_kind"="Challenge" "resource_name"="foo.example.com-755w2-153119475-769035721" "resource_namespace"="istio-system" "resource_version"="v1" "type"="DNS-01"

To solve this, I have changed the behavior so instead of the issuer removing all existing _acme-challenge it does an atomic update to add the wanted rrdata (txt value) to the record, and during CleanUp() remove it again unless it's the last one, at which points the removes the record.

There can still be races, but instead of getting into a state where it's stuck in propagation check failed, the back-off allows the cert-manager's to quickly reach a state where all the records are added and then removed as needed, and certificate requests are fulfilled.

When you are doing GitOps, where a configuration change is automatically pushed to several clusters at the same time, it's quite common to hit this race condition. Without this PR, cluster admins had to manually intervene and resolve the race condition by removing the certificate and reapplying the kubernetes manifests or similar.

I tested by applying a certificate to three clusters simultaniously with

kubectl apply -f c.yaml --context cluster0 & \
kubectl apply -f c.yaml --context cluster1 & \
kubectl apply -f c.yaml --context cluster2

Kind

bug

Release Note

Fix CloudDNS issuers stuck in propagation check, when multiple instances are issuing for the same FQDN

[jetstack-bot]

Hi @cypres. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[irbekrm]

/ok-to-test

[irbekrm]

Thank you for working on this @cypres .

I agree that this is an issue that we should fix.

I see that the change in this PR makes it so that adding a TXT record, deletes and re-applies any other found records with the same FQDN via the changes API and the cleanup does the same.
I don't know how the changes API is implemented internally, but I am concerned that this could in practice cause a 'downtime' for the records that are re-added (in between them being deleted and added again).
Do you think we could instead use ResourceRecordSets:patch to patch RRSets that contain records that we don't want to delete?

Looking at the docs we already request permissions for dns.resourceRecordSets.* so this should not be a breaking change https://cert-manager.io/docs/configuration/acme/dns01/google/#set-up-a-service-account

[cypres]

Thank you for the feedback, I replied below.

I see that the change in this PR makes it so that adding a TXT record, deletes and re-applies any other found records with the same FQDN via the changes API and the cleanup does the same. I don't know how the changes API is implemented internally, but I am concerned that this could in practice cause a 'downtime' for the records that are re-added (in between them being deleted and added again).

I understand your concern, but that is actually how the changes API is intended to be used. The changes are applied atomically, from the Google documentation;
A Change represents a set of ResourceRecordSet additions and deletions applied atomically to a ManagedZone. ResourceRecordSets within a ManagedZone are modified by creating a new Change element in the Changes collection. In turn the Changes collection also records the past modifications to the ResourceRecordSets in a ManagedZone. The current state of the ManagedZone is the sum effect of applying all Change elements in the Changes collection in sequence.

Do you think we could instead use ResourceRecordSets:patch to patch RRSets that contain records that we don't want to delete?

Sadly, google has no way to patch the rrdata for a TXT record to add or remove a single value.
You can try this yourself with just using the gcloud cli.

# Create a dummy record with value "foo"
$ gcloud dns record-sets create foo.myzone.tld --rrdatas="foo" --ttl=60 --type=TXT --zone=test
NAME                        TYPE  TTL  DATA
foo.myzone.tld.  TXT   60   "foo"
# Patch a partial update
$ gcloud dns record-sets update foo.myzone.tld --rrdatas="bar" --ttl=60 --type=TXT --zone=test
NAME                        TYPE  TTL  DATA
foo.myzone.tld.  TXT   60   "bar"
# Inspect the record
$ gcloud dns record-sets describe foo.myzone.tld --type=TXT --zone=test
NAME                        TYPE  TTL  DATA
foo.myzone.tld.  TXT   60   "bar"

[irbekrm]

I understand your concern, but that is actually how the changes API is intended to be used. The changes are applied atomically, from the Google documentation;
A Change represents a set of ResourceRecordSet additions and deletions applied atomically to a ManagedZone. ResourceRecordSets within a ManagedZone are modified by creating a new Change element in the Changes collection. In turn the Changes collection also records the past modifications to the ResourceRecordSets in a ManagedZone. The current state of the ManagedZone is the sum effect of applying all Change elements in the Changes collection in sequence.

Thanks for the detailed explanation. Could we please add a comment along the lines of this explanation to the relevant bits of code to prevent confusion in the future?

[cypres]

Thanks for the detailed explanation. Could we please add a comment along the lines of this explanation to the relevant bits of code to prevent confusion in the future?

Thank you. I expanded the comments a bit, let me know if you want something more long-form.

[irbekrm]

Thank you for working on this @cypres

I tested it with two clusters with a CloudDNS issuer and a cert with the same DNS name for both clusters.
I could reproduce the error (in my case, both Challenges ended up in an invalid state as one of them overwrote the value of another after the self-check had already validated it, the first one then failed ACME validation and deleted the text record which caused the second one to fail.
I saw both issuances succeed with this fix, observed TXT records in the acme challenge record, both issuances succeeded and the record got cleaned up afterwards as expected.

I think that we will not backport this- it is arguably almost like a new feature (we didn't really promise multicluster concurrency before), but it would be good to cut a v1.13 alpha release soon-ish so interested folks could try it out.

(/cc @aww-aww as I imagine that you folks might have seen this issue too and might be interested in the fix)

/lgtm

[irbekrm]

Thanks for adding that 👍🏼

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cypres, irbekrm, tobiasbrodersen

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:

• OWNERS [irbekrm]

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