fix(txt/endpoint/crd/coredns) Arbitrary TXT Record Bug Fixes and Support for CoreDNS

[onelapahead]

What does it do ?

Allow for managing TXT records via DNSEndpoints when using the coredns provider. Specifically, achieves the properties I expected when using the CRD source with TXT records:

1. If I deleted the DNSEndpoint I made, all the TXT records would be cleaned up
2. TXT records should be able to end with / contain arbitrary text like . and =
3. TXT records should be able to have multiple targets and all them be resolved
4. Updating DNSEndpoints should (eventually) result in changes in the managed DNS records
5. It is nice-to-have because of the declarative nature of DNSEndpoints, if the order of the targets defined for TXT records can be preserved when resolving the TXT records, as their format might mean something in the context of a broader protocol (though I understand single strings with ; separators are preferred due to the fact that ordering is not guaranteed by providers).

To do so, this PR fixes several bugs observed:

1. TXT records ending with . were considered "illegal" wrongfully, and if they weren't considered illegal, they had their trailing . truncated
2. TXT endpoints containing = were incorrectly parsed when building the {target}={hash} label value pairs within endpoints
3. For CRD sources, the order of targets was being lost due to sortings for comparisons within Same(). While order of records is not guaranteed depending on the record type and the provider, it felt unnecessary for ExternalDNS to randomly order the declaratively defined targets from a user in the CRD case, and helped make sure the order of TXT targets was preserved.
4. For coredns specifically:
   a. multiple targets for a TXT record were not supported. Only a single etcd Service with Text was being synced. Ordered, hash prefixes are now used to create unique keys for each Service containing the bit of Text expected.
   b. updating records (whether A or TXT) with --policy=sync did not work because the owner label was never being set. causing the provider to think the records were up-to-date
   c. cleaning up TXT records was not working (might address Delete orphaned owned TXT record #1503)

Motivation

I went to try our the CRD source for managing a mixture of A and TXT records, using the CoreDNS provider for my local development environment on KinD (with plans to begin testing main cloud providers like AWS shortly after).

I had not realized the docs stated only digitalocean was currently supported, and after some trial and error noticed that for CoreDNS was partially working. Attached is the YAML manifests I used to create a local CoreDNS + etcd + ExternalDNS environment (extdns.yaml). But the following are the ExternalDNS args I used:

          - --source=crd
          - --crd-source-apiversion=externaldns.k8s.io/v1alpha1
          - --crd-source-kind=DNSEndpoint
          - --provider=coredns
          - --domain-filter=example.dev
          - --log-level=debug
          - --policy=sync                                   # noting I initially used upsert-only but found dynamic updates did not work
          - --txt-prefix=extdns                          # this seems required for managed TXT records to work so that registry records don't clash
          - --managed-record-types=A
          - --managed-record-types=TXT

I tested with a set of DNSEndpoints like these:

apiVersion: externaldns.k8s.io/v1alpha1
kind: DNSEndpoint
metadata:
  name: peer2-registry
  namespace: default
spec:
  endpoints:
    - dnsName: peer2.example.dev
      recordTTL: 5
      recordType: TXT
      targets:
        - v=11
---
apiVersion: externaldns.k8s.io/v1alpha1
kind: DNSEndpoint
metadata:
  name: peer1-registry
  namespace: default
spec:
  endpoints:
    - dnsName: peer1.example.dev
      recordTTL: 5
      recordType: TXT
      targets:
        - v=1;id=enode1;addr=/ip4/10.0.0.10/tcp/30303/p2p/Qm..
        - additional-txt-value
    - dnsName: peer1.example.dev
      recordTTL: 5
      recordType: A
      targets:
        - 10.0.0.14
        - 10.0.0.15
---
apiVersion: externaldns.k8s.io/v1alpha1
kind: DNSEndpoint
metadata:
  name: peer3-registry
  namespace: default
spec:
  endpoints:
    - dnsName: peer3.example.dev
      recordTTL: 5
      recordType: TXT
      targets:
        - other.text.woo=!
        - additional-txt-value
        - please-delete all this

I found the bugs above as I created, updated, and deleted variations of these endpoints, and made the various fixes in the PR in order to get all of those edge cases working. Disclaimer: I used claud-4-sonnet model of Cursor to assist me in my changes as I got more familiar with the code base and shared logs/results from my manual testing.

More

• Yes, this PR title follows Conventional Commits
• Yes, I added unit tests
• Yes, I updated end user documentation accordingly

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: onelapahead / name: hayden (a6bb7da, 6414434, 1807809, 9afd329, 5e6ff16, eac20cd, f03cee2, 52cd321, a14c2d8, c0554b5, 78fa0b4, 6dcb48e, 30bef6f)

[k8s-ci-robot]

Welcome @onelapahead!

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 @onelapahead. 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.

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-sigs/prow repository.

[onelapahead]

Recognize because of the fixes in both registry / source / endpoint and provider/coredns this PR has been marked XXL.

Should be fine to pull out the former into smaller PRs, and make this a more specific PR to just coredns.

But could use the maintainers overall feedback on my motivations and to see how the fixes all relate - and if there were any flaws in my expectations around non-registry TXT records and the CRD source - before I go that additional effort.

Thanks !

[ivankatliarchuk]

This PR is too large; fixes, refactoring, new fucntionality and etc. How would you recommend we break it down? Just a heads-up that changes to the TXT logic can be extremely slow to approve, as you can see from this past example, as it most likely will require owners approval: #5459.

[ivankatliarchuk]

/ok-to-test

[onelapahead]

Thanks @ivankatliarchuk - I agree. I have also started testing TXT records via DNSEndpoints for AWS, and found smaller bugs - some for AWS specifically and one in the registry which I expect needs a lot of thought/feedback from the core maintainers.

I think a lot of the fixes outside of provider/coredns can be opened as very small contained PRs. And then I'll open what is ideally a L or XL PR for multi-TXT record support in coredns so that can just require the provider's input.

Additionally, I will open two feat issues which track support for TXT records via DNSEndpoints for CoreDNS and AWS specifically - so that the multiple smaller PRs can be tracked with the large context of their intent.