WIP Add support for AAAA records

[johngmyers]

Adds support for managing AAAA records.

Fixes #2300 Fixes #1812 Fixes #2051

Checklist

• [x] Unit tests updated
• [x] End user documentation updated (it doesn't state external-dns doesn't support AAAA)

Bringing work started in #2309 to completion, as that PR's author has expressed that they will not be moving the work forward at this time.

Tested against the Route53 provider and the TXT registry. The external-dns AWS SD registry code likely does not support AAAA records.

This touches on multi-target, since dual-stack targets need both an A and an AAAA record. This PR solves the dual-stack problem by creating separate Endpoint objects for the A and AAAA record and extending the TXT registry to store ownership records for AAAA Endpoints under a different domain.

[samip5]

It's sad that you twisted my words, because that's not what I said.

[MTRNord]

Hm I know this is fairly early, but it seems the current code does something weird for the txt records.

It fails with No matching zones were found for the following endpoints: [aaaadocs.nordgedanken.dev-tls 0 IN TXT (shortened the log). It seems very odd that it tries to use the tls secret instead of the host of the ingress. And obviously that's not going to work.

[johngmyers]

@MTRNord could you provide reproduction steps?

[MTRNord]

Oh I see what happens here. A helm chart I am using is setting the tls hosts with that postfix. Sorry. I only just realized why I wanted to send you an ingress that fail. So the issue is in the helm chart used, not in external dns.

[anthr76]

Is there anything left on this besides review? @johngmyers do you have an image availble to test?

[adamcharnock]

@anthr76 (any anyone else in need) – I've built this image and pushed it to docker hub. It is:

adamcharnock/external-dns-ipv6:v0.10.2-8-g23bf88d9

Looking forward to this being in an official release :-)

[johngmyers]

I'm not aware of anything other than review being needed. I tested on a frankenimage including commits from #2419 because I had difficulty creating IPv6-capable Ingresses due to AWS LBC not supporting k8s > 1.21.

[johngmyers]

There are going to be followup PRs extending AAAA support to other resources and possibly the AWS SD registry.

[olemarkus]

What's the reason for the extra registry TXT record? is that in case there may be different owners for A and AAAA?

[johngmyers]

@olemarkus The record type is a field of Endpoint. To be able to have both A and AAAA DNS records for the same DNSName, there are two possible approaches:

1. Have one Endpoint of RecordTypeA and another of RecordTypeAAAA. Since each Endpoint needs an ownership entry in the registry, that means the TXTRegistry needs to store a separate TXT record under a different DNSName for the AAAA Endpoint.

2. Push the RecordType field of Endpoint down into the Targets. This would be a much more substantial change, requiring a change to every Provider implementation. It seemed to me that this approach would have a much lower probability of success.

This PR takes approach (1).

[NobodysNightmare]

Just for cross-reference, this will most likely also help fix: https://github.com/kubernetes-sigs/external-dns/issues/1877

[johngmyers]

My understanding is that this PR only adds picking up AAAA from Ingress. Picking up AAAA from other resource types is for followup PRs. I plan on submitting said followup PRs.

I'm not sure what to add to the documentation about how to use this. One doesn't do anything different than what the existing documentation describes; it's just that AAAA records now get registered in DNS whereas before they were omitted. And I think it's outside the scope of this package's documentation to crib RFC 3596.

How it works: the plan separates the two types of addresses into two different Endpoints for the same domain. The registry then has to support simultaneously registering A and AAAA Endpoints for the same domain, so the TXT registry treats AAAA records as a special case when transforming the domain.

So could you give a little more detail on what/where you want additional documentation? An addition to the "Ownership" sections of docs/initial-design.md? Something about the planner, which doesn't appear to be currently documented?

[Raffo]

@johngmyers the change looks good overall to me, thank you for contributing this. I have a question: did you try to figure out what happens when we change the TXT ownership records to running records? Is the case handled correctly or we need something to convert the existing records?

[johngmyers]

The TXT ownership records for A records are the same before and after. The TXT ownership records for AAAA records are new—neither those ownership records nor the AAAA records being owned would have existed before, so there's no need to convert.

If a domain starts with "aaaa" and has an A record, then that TXT ownership record would be misinterpreted as an ownership record of an AAAA record for that domain stripped of the "aaaa" prefix. This is an area where we might want a different design than is in this PR.

[NobodysNightmare]

neither those ownership records nor the AAAA records being owned would have existed before, so there's no need to convert

What about the AAAA records currently created by the AWS provider?

These are created in case of a dualstack ingress when planning to add an A record. From a users perspective external-dns "owns" these, i.e. they have been created by external-dns, but there is no AAAA ownership record created for them, right?

Would the current implementation mean that they would not be recognized as being created through external-dns, because they lack the ownership record for AAAA records?

[johngmyers]

That code is for alias targets. It should be mapping back to look for an ownership record for the alias (or ignoring, to rely on the corresponding A alias record). It would be worth testing to verify.

[Raffo]

It would be worth testing to verify.

@johngmyers can you take care of this testing?

Also, I was taking a look at https://github.com/kubernetes-sigs/external-dns/pull/2157 yesterday and @k0da mentioned that this is a more generic change for the TXT registry. Would you be okay with me reviewing/merging https://github.com/kubernetes-sigs/external-dns/pull/2157 and then rebase this PR?

[johngmyers]

It might take me a couple of days to block off some time to do the testing. I've got some urgent projects going on right now.

I think #2157 should go in before this. I think this will have to then modify the TXT registry code to suppress old-format records for the AAAA record type.

[Raffo]

@johngmyers thank you for you answer and absolutely take the time you need. I'll prioritize #2157 and feel free to let me know when you have more news on this one.

[muhlba91]

any news on that one? we heavily use ipv6 addresses and, thus, we always run into issues.

[ghost]

LGTM

Thank you to everyone involved in this work!

[Raffo]

The news is that we are still targeting https://github.com/kubernetes-sigs/external-dns/pull/2157 before this one and the review on that one is taking a bit longer (real life, covid and stuff 😅 ).

[Raffo]

https://github.com/kubernetes-sigs/external-dns/pull/2157 has been merged, this PR needs to be rebased to incorporate the changes implemented there.

[lou-lan]

empty

[axkng]

Are there any news on this PR?

[batistein]

Any updates? I am really looking forward to this feature

[Raffo]

@johngmyers Is this still WIP? Can we review it?

[johngmyers]

I'm having difficulty blocking out time to bring up a test environment and do the manual testing. If anyone else could take that on, I would be happy to hand over the PR.

[olemarkus]

I am happy to do manual testing, but I don't have the time to get into all this code, so if something doesn't work, I would struggle. Give me a few days and I will report back how it goes.

[olemarkus]

@Raffo I think you can start doing a code review in parallel.