fix(crd): txt record deletion

[arontsang]

Description

Adds support for deleting TXT records after the CRD is deleted.

Fixes #5254
Fixes #5279

Checklist

• Unit tests updated
• End user documentation updated

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: arontsang (cb58622)

[k8s-ci-robot]

[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 szuecs for approval. For more information see the Code Review Process.

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

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

[k8s-ci-robot]

Welcome @arontsang!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @arontsang. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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-sigs/prow repository.

[mloiseleur]

Thanks for this @arontsang 👍
Do you think you can add some tests on this ?

[arontsang]

I can try. But this PR is me popping my GoLang cherry. I took an hour to setup the environment (after a few false starts).

…

On Sun, 13 Apr 2025, 22:45 Michel Loiseleur, ***@***.***> wrote: Thanks for this @arontsang <https://github.com/arontsang> 👍 Do you think you can add some tests on this ? — Reply to this email directly, view it on GitHub <#5280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA46MLIVMQ2B7BXFMMIDNCT2ZJZ63AVCNFSM6AAAAAB3AEW2UGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJZHE4DCMJRGI> . You are receiving this because you were mentioned.Message ID: ***@***.***> *mloiseleur* left a comment (kubernetes-sigs/external-dns#5280) <#5280 (comment)> Thanks for this @arontsang <https://github.com/arontsang> 👍 Do you think you can add some tests on this ? — Reply to this email directly, view it on GitHub <#5280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA46MLIVMQ2B7BXFMMIDNCT2ZJZ63AVCNFSM6AAAAAB3AEW2UGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJZHE4DCMJRGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[ivankatliarchuk]

Could you explain a little more what this code suppose to be doing? I do not understand it

[arontsang]

Sure @ivankatliarchuk

The bug.

https://github.com/kubernetes-sigs/external-dns/blob/master/registry/txt.go#L198 is failing to apply the labels for TXT records to the TXT endpoints.

The result.

https://github.com/kubernetes-sigs/external-dns/blob/master/controller/controller.go#L255 plan.Calculate() regards any TXT records created by external-dns as not being created by external-dns (missing owner etc etc).

The cause.

a) Old style TXT records come in on the same record as the target

foo.acme.com TXT value [ 'FOOBAR', 'heritage=external-dns,....blah' ]

Depending on which value is returned first, we end up with two different but equally broken behaviors.

• We detect the record is an endpoint, but are missing the label
• We detect the label, but have no endpoint to apply it to....:(

Therefore we need to disable the creation of old style TXT records for TXT endpoints.
https://github.com/kubernetes-sigs/external-dns/blob/master/registry/txt.go#L240

Also disable fallback to old style when annotating TXT endpoints.
https://github.com/kubernetes-sigs/external-dns/blob/master/registry/txt.go#L199

b) New style TXT records are not recognized as being the heritage for the endpoint, because getSupportedTypes() is hard coded to NOT allow support for new style TXT records.
https://github.com/kubernetes-sigs/external-dns/blob/master/registry/txt.go#L111

Ideally, someone updates this code to use the --managed-record-types options to drive this code. But that is well beyond my abilities with golang.

[ivankatliarchuk]

I will try to give it another shot. But from my integration tests it does not work. The whole issue is, we support legacy and new formats, as a result the TXT handling is not properly supported.

Have you tried to do end-2-end testing and it works on your side?

[ivankatliarchuk]

I've added comment in the issue #5254 (comment)

[arontsang]

I was debugging with a local cluster/docker with my own cloudflare account.

Seems to work well with deleting TXT records.

[arontsang]

I also know that if the user has A/AAAA/CNAME records with the same name as the TXT records you are trying to create, my PR will not work.

It ONLY works with the case where TXT names are different from the other records, OR "only new TXT" is enabled.

Again, ideally, we inject the --managed-record-types values into getSupportedTypes.

I suggested both that, and a change where --managed-record-types=TXT requires that --txt-new-format-only is also switch on.

PS, did you test the change with --managed-record-types=TXT?

[ivankatliarchuk]

Indeed. It does not seems like CRD is care at all about this config. Seems like you have some other manifest that generates TXT record. As CRD simply does not support TXT from the code itself.

[ivankatliarchuk]

I suggested both that, and a change where --managed-record-types=TXT requires that --txt-new-format-only is also switch on.

This is it. The problem with TXT formats. There is a PR to simplify logic, #5172 but most likely is not going into master anytime soon (in fact I'm planning to close it, as there are pushbucks and folks happy to have this TXT mess). CRD and TXT is a problem for long time.

Share all manifests. I could try once again. I'm testing on staging accounts where I have thousands of TXT recrods, not on fresh/empty

[arontsang]

hey @ivankatliarchuk
I've updated my PR to fix the logic in the TXT record handling.

Previously it was assumed that a TXT records is either a heritage/label entry, OR a TXT entry.

We would only process the FIRST txt target for a TXT record into a label.

I've now changed the logic to process ALL targets as labels, and then process the rest as a TXT entry.

This means that we can use old style txt format with A/AAAA records together.
It should also mean that TXT should work correctly now.

[mloiseleur]

/ok-to-test

[mloiseleur]

/retitle fix(crd): txt record deletion

[mloiseleur]

@ivankatliarchuk anything left on your side ?
@mcharriere @mrozentsvayg Do you think one of you can proceed with a final review ?

[ivankatliarchuk]

I could not approve. Both issues do not click to me. Overall I understand the direction, to have a better integration most likely with cert-manager or smth.

From the code perspective it's seems like a breaking change. We use to process first Target, now we process all record targets.

No integration tests before and after, unit tests not updated to cover a case.

[ivankatliarchuk]

Let's keep it open. I'll asked one of the team working with to try on some close to real clusters. May takes few weeks