WIP Always create AAAA alias records in route53

[johngmyers]

Description

Always creates both A and AAAA alias records in Route53.

When a Route53 AAAA alias record points to an IPv4-only resource, AAAA DNS queries return zero records. This is the same behavior as when there is no AAAA alias record but is an A alias record. For this reason, there is no rational reason to not also create the AAAA alias record.

Adds logic for reconciling discrepancies between the A and AAAA alias records, including when only one of the two is present. In some situations, this reconciliation will take two iterations of the reconciliation algorithm to converge, as the Route53 provider does not store all information about the AAAA record with the discrepancy in the Endpoint. Encoding this information in the provider-specific annotations would result in a lot of complicated code for a rare edge case. An alternative would be to add an opaque provider-specific interface{} to Endpoint so that the Route53 provider could store the ResourceRecordSet directly.

This PR is an alternative to #2050.

Fixes #3707

Checklist

• Unit tests updated
• End user documentation updated

[szuecs]

@johngmyers this will 1) change default to use dualstack and therefore 2) roughly double requests to AWS route53 with its "nice" API rate limits (see also #3402).
I would rather like to opt-in this feature at least via a new flag.

[johngmyers]

One should not have to opt-in to expected, correct behavior. #3402 should address rate limiting, especially if it defaults to enabled.

At worst, this should be opt-out, though that will further increase complexity and the number of needed test cases.

[johngmyers]

Also, why does a mere factor of 1.5 deserve such complexity?

[szuecs]

@johngmyers in general I agree with you!
I just wanted to raise awareness. Maybe you can also review the cache PR, I see that you did, but if you think it's ready please add your /lgtm, too.

[szuecs]

/ok-to-test

[johngmyers]

I'm going to experiment with implementing this using AdjustEndpoints. I'm concerned with the complexity of reconciling discrepancies between A and AAAA in the provider.

[johngmyers]

The AdjustEndpoints approach is much simpler, though with the disadvantage that the TXT registry creates ownership records for the additional AAAA alias records.

I don't think registries should be creating ownership records for endpoints that were added by AdjustEndpoints, though that's arguably an issue for a separate PR. I haven't yet figured out how to avoid creating such records without giving the txt registry knowledge of route53.

I'm also thinking of writing a dynamodb registry.

[johngmyers]

To avoid creating ownership records for the AAAA alias records, we could:

1. Define a label and have Provider.AdjustEndpoints() add said label to each additional Endpoint it creates.
2. The presence of this label causes TXTRegistry.ApplyChanges() to not create any ownership records for such Endpoints.
3. Define a new interface method on Provider or a new interface implemented by AWSProvider. Registry.Records() implementations call this method after adding all their Labels to the endpoints. The AWSProvider implementation of this method copies the Labels from the CNAME alias Endpoints to the corresponding AAAA alias Endpoints.

This wouldn't handle the case where the A record is deleted but the AAAA and TXT were not. To handle that case, the new interface method would have to take and endpointName, type, and possibly labels and return a set of additional endpointNames and types that the labels would apply to.

[szuecs]

/ok-to-test

[johngmyers]

@szuecs what is the next step to get this merged? I'm working on a DynamoDB registry in #3648, but I doubt that should be a prerequisite.

Would scheduling a meeting be helpful?

[mloiseleur]

It is open source.
One can either wait for @johngmyers to finish this PR or create a new PR (based on this one), rebase and fix the remaining issues.

[schmitz-chris]

It is open source. One can either wait for @johngmyers to finish this PR or create a new PR (based on this one), rebase and fix the remaining issues.

not everyone can do this and fix the problem but still need the fix for their systems. So +1 from my site, I also need it.

[k8s-ci-robot]

@johngmyers: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-external-dns-licensecheck	4dcce15	link	true	/test pull-external-dns-licensecheck

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[paalkr]

/remove-lifecycle rotten

[ruben-boada-klarrio]

Hi there,

I would like to push forward this fix, so I'm trying to raise a PR based on this one but it's not clear to me what's still pending and how to resolve the conflicts that this one has once you rebase from original/main, so I would need some heklp to go through them. For instance, changes like the ones here #3910 gets into conflict whit what had been changed in this PR.

Thanks in advance! 🙂

[soostdijck]

Is this change scheduled? I still have to go around manually creating AAAA records pointing to my NLBs, and IPv4 has run out quite a while ago.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[luis02lopez]

/remove-lifecycle stale

[ypaq]

Any progress on this?

[project0]

@mloiseleur @Raffo sorry to bother you guys, but i saw that you have been implicitly part of the discussion in #4511.

I am eager to adopt/reopen the PR so we can move forward, wdyt about this approach? It actually resolves your past concern with vendor/provider specific labels in the core of the project and resolves the outstanding problem of missing dualstack capabilities for NLBs.

[mloiseleur]

@project0 long time no see :).

Sure, as long as you address concerns expressed in #4511 (comment), it would be great to move forward on this subject.

[rlees85]

@project0 just wondered if you are still able to have a go at working on this? In the absence of anyone else trying to get this PR over the line, I might have a go myself - I'll be coming in from 0 background of working on external DNS but really would like this feature working

[rlees85]

OK so I've made a start. Just to let you know in case you don't want to work side-by-side unknowingly. I am feeling a bit more confident now I've actually made a start as well. I hope to raise a PR within a couple of days BUT (biggest BUT ever) it probably wont be merge-able for a while. I'll need to run tests, run it in our cluster to see if it behaves, try and understand why this PR never made it over the line, etc. I hope others will be able to help out once the PR is raised.

[project0]

@rlees85 the change itself is pretty simple, but it turned out updating the tests is the painful part (they way there are built, it is super hard to debug and update them) :-(.
I had unfortunately no time to dig deeper into.

[rlees85]

#5111

Right, here goes!

@project0 yes, I had lots of fun with the tests! Had to do some nasty things to make them dump out why actually they were failing sometimes.

[mloiseleur]

superseded by #5111
/close

[k8s-ci-robot]

@mloiseleur: Closed this PR.

In response to this:

superseded by #5111
/close

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.