-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(aws): always create AAAA alias records in route53 #5111
base: master
Are you sure you want to change the base?
Conversation
Welcome @rlees85! |
Hi @rlees85. 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 Once the patch is verified, the new status will be reflected by the 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. |
I just let it rip in our development cluster. At first look, seems to work well. AAAA records created and nothing has broken, yet. One observation, is that the TXT records that are created seem a bit odd: We get: We also get: If anyone has any good example test cases I can carry out I will do so. |
I found the concerns @mloiseleur raised:
I think I've addressed this now.
This works, we were using it wrong:
I can confirm the last one is now working. It cleans up AAAA records when both changing the name of an ingress as well as when deleting it. |
When you ready, please share all relevant manifest, kubectl and AWS commands. I would like to test it as well. One concern here, not sure if covered, why would I need AAAA records alongside A records? This seems more like a flag to me, as why would I need AAAA records if I never asked for it, plus it's easy to remove a flag and make it default behaviour at some point? |
/ok-to-test |
@ivankatliarchuk AAAA records are the same as A records, but for IPv6. So for example, if a load balancer in AWS has dualstack networking enabled (both IPv4 and IPv6) we should add both an A record with the IPv4 address and AAAA record for IPv6. The previous pull request found that even with IPv4 only load balancers, adding AAAA records causes no harm, since simply no IPv6 address is returned. This makes the code much simpler. There is not really much point in turning this off beyond not liking AAAA records being in the console when having a look there. I think there were concerns about the extra requests contributing to rate limiting, but a caching feature has gone in recently to mitigate that (I believe?). If a way to turn this off is a deal breaker, I will try to implement something. With manifests sadly its all a bit rough right now.
I will have a look to see if I can host it in a public repository somewhere for testing. |
Just share manifests here. Normally, you don't need to create a container. You could do something like this https://github.com/kubernetes-sigs/external-dns/blob/master/docs/contributing/dev-guide.md#execute-code-without-building-binary, as long as external-dns is aware about the cluster context. I got your point, sounds reasonable. What about Probaly worth to validate cases when ALB switch is happening from |
Am I understood correctly Before
With this change, the proposed behaviour is not to rely on This statement is correct
But now, on top of that, for example we have dozens of nodes with IPv6 that we do not want to expose over route53, suddenly will be exposed? |
I can't think of any case where you'd have a load balancer with dualstack enabled where you'd want to expose IPv4 but not IPv6. The simple solution here would be to make it not a dualstack load balancer, then it won't be exposed on IPv6. We create an AAAA record for it (as long as ALIAS is enabled), but it just resolves to blank/empty, so nothing is exposed. |
You can exclude this by using the following flags:
Tested working just now. I think that covers everything required to get this moving |
Could you share e2e test results and manifests you were using? Example test cases #5085 (comment) I believe in this case, we are looking for something like create NLB and validate different things, like change it from ipv4 to dualstack and back. |
This most likely should go to documentation somewhere. |
Any chance to rebase with master? I have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is quite big. I'll try to go over changes in sources over a weekend.
Could you review docs, there are some references to an annotation external-dns.alpha.kubernetes.io/dualstack
. Just reference to annotations need to be removed, the actual manifest we better keep for , as it's still applies
Example
external-dns/docs/sources/gateway.md
Line 87 in 58ac76e
External DNS Controller uses the `external-dns.alpha.kubernetes.io/dualstack` annotation to determine this. If this annotation is |
@@ -1142,7 +1142,7 @@ func useAlias(ep *endpoint.Endpoint, preferCNAME bool) bool { | |||
// and (if so) returns the target hosted zone ID | |||
func isAWSAlias(ep *endpoint.Endpoint) string { | |||
isAlias, exists := ep.GetProviderSpecificProperty(providerSpecificAlias) | |||
if exists && isAlias == "true" && ep.RecordType == endpoint.RecordTypeA && len(ep.Targets) > 0 { | |||
if exists && isAlias == "true" && slices.Contains([]string{endpoint.RecordTypeA, endpoint.RecordTypeAAAA}, ep.RecordType) && len(ep.Targets) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the same logic aka slices.Contains
shell we consider to create a method in endpoints go?
something like
func (ep *Endpoint) IsSupportedRecordTypes(r ...string) bool
Just an idea
Thanks for the review (so far). There is a lot to go at here, I will try and come back with some changes. |
We most likely need to be think more about our users as well. As in this pull request we are removing the support for @mloiseleur wdyt? |
I wasn't aware of an Following this change, ExternalDNS does not "care" if this third party annotation is set or not, hence why I don't really want to get bogged down with documenting it in ExternalDNS. It looks like edit: yes it looks like gateway source used it. The label was only ever picked up by the AWS provider, which still now take the "with dualstack annotation" behaviour by default. To users, who are using this annotation, will see no change in behaviour. Users who don't like seeing AAAA records in the Route53 console will need the exclusion flag, as per push docs. Sorry I hope this helps! |
Got it. I've tried to understand why this annotations was added, but not too sure. Some relevant pull requests https://github.com/kubernetes-sigs/external-dns/pulls?q=is%3Apr+dualstack+annotation+is%3Aclosed. Seems like is safe to remove references to this annotation |
I've been over every comment on the PR so far (thank you) and below is a list I will work from to address feedback. This is for my reference really and will hopefully get worked on tomorrow or over the next few days at least.
|
/retitle feat(aws): always create AAAA alias records in route53 |
Starting with a Kubernetes cluster (EKS, if it matters) and the AWS load balancer controller installed, so that I can create load balancers for services and ingresses. The a compiled version of the current head of this branch, as of this comment, is in a public Docker image located here: Steps:
Currently the AWS load balancer controller seems not to support: For this test I hand-cranked an application load balancer with this mode and again hand-cranked in
Route53 seems to handle the fact that the backend can only support IPv4, IPv6 or both for you as long as you have both A and AAAA records created. Returning blank just means the other protocol gets used, which does return records. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
cc @mloiseleur
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ivankatliarchuk The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@project0 : You were interested by this feature, IIRC. Anything missing on this PR ? Do you want to review this PR before merge ? |
New changes are detected. LGTM label has been removed. |
Definitely worth |
/label tide/merge-method-squash |
Description
This change creates AAAA and A records for AWS Route53. It is heavily based on this pull request that seems to have sadly been abandoned: #3605
This is now based on the current master branch. I've tried to add some more thorough test cases so that when I let this loose in a Kubernetes cluster hopefully not too many bad things will happen.
Fixes: #3707
Fixes: #4509
All credits to @johngmyers for the vast majority of the leg work.
Checklist
Checklist to drop WIP