external-dns
external-dns copied to clipboard
kubernetes-sigs
Add NLB IP dualstack mode support (ipv6 support for NLB)
Description
AWS Loadbalancer Ingress controller supports service resource of
LoadBalancer type.
Currently dualstack annotation for service is ignored by external dns
and corresponding AAAA record is not being created.
This PR adds support of ipv6 similar to ingress resource/ALB.
Fixes #ISSUE
Checklist
- [x] Unit tests updated
- [x] End user documentation updated
Welcome @logingood!
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. :smiley:
Added a few comments. Did you test this @logingood? I don't think this is going to be enough to create AAAA records 🤔
I'll test it today, I was relying on this piece of code, which expects the annotation to be present https://github.com/kubernetes-sigs/external-dns/blob/a5baad26d7f4a7e1ff9ff2fefa6d566fe726a1cd/provider/aws/aws.go#L558-L568 and here's the test that validates this condition https://github.com/kubernetes-sigs/external-dns/blob/a5baad26d7f4a7e1ff9ff2fefa6d566fe726a1cd/provider/aws/aws_test.go#L932
Added a few comments. Did you test this @logingood? I don't think this is going to be enough to create AAAA records 🤔
I'll test it today, I was relying on this piece of code, which expects the annotation to be present
https://github.com/kubernetes-sigs/external-dns/blob/a5baad26d7f4a7e1ff9ff2fefa6d566fe726a1cd/provider/aws/aws.go#L558-L568 and here's the test that validates this condition
https://github.com/kubernetes-sigs/external-dns/blob/a5baad26d7f4a7e1ff9ff2fefa6d566fe726a1cd/provider/aws/aws_test.go#L932
@Raffo I've just tested, it worked fine
@Raffo @seanmalloy any feedback on that ? We are actively using forked version and it perfectly creates AAAA for nlb-ip load balancers. It would be very useful to fix this behavior.
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale - Mark this issue or PR as rotten with
/lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
hey is there any update on that ? @Raffo @seanmalloy is there any alternatives to that ?
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale - Mark this issue or PR as rotten with
/lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
How does this one compare to #2309?
this PR covers IPv6 for NLB #2309 might be more generic ?
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale - Mark this issue or PR as rotten with
/lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: crawforde, logingood, mesge, sylr To complete the pull request process, please ask for approval from njuettner after the PR has been reviewed.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
For those interested I've built a docker image with this PR:
source: https://github.com/sylr/external-dns/commits/v0.12.0%2Bsylr image: ghcr.io/sylr/external-dns:v0.12.0-2-gca826a18
In my case I had to remove the TXT record and A record of the services which use dualstack for external-dns to create both A and AAAA records at the next run.
Anyone know the status on this @Raffo @seanmalloy @njuettner?
Thanks @sylr.
New custom build with this patch
source: https://github.com/sylr/external-dns/commits/v0.12%2Bsylr image: ghcr.io/sylr/external-dns:v0.12.2-3-g264cde1a
@Raffo @seanmalloy @njuettner I've been running this patch for 6 months now, can we merge it please ?
Not sure why but adding this to our service while using your latest image didn't work @sylr. The aws load balancer controller did update it appropriately to be dualstack to the outside world.
service.beta.kubernetes.io/aws-load-balancer-ip-address-type: "dualstack"
Would love to see this working and then merged into mainline.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: crawforde, logingood, mesge, sylr Once this PR has been reviewed and has the lgtm label, please ask for approval from njuettner. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
I've rebased just in case if it remains relevant, this code have been running without issues more than a year. If someone has a better way/more generic to implement this that'll be great!
I just independently implemented this after needing the missing functionality and realizing external-dns did not support it. I searched issues before I implemented it but I only searched PRs as I went to open my own.
This is seemingly a perfectly reasonable PR, following the same approach as is used to handle the ALB dual stack annotation, and it's been open for over 2 years now. Multiple people have commented that it works and is useful for them. I don't understand the reticence to merge. If there is a problem with this approach, can you please provide guidance on a different approach which would be acceptable?
The issue at hand is caused by a specific quirk in Route53 ALIAS records, where both A and AAAA alias records need to be created for certain load balancers. The Endpoint domain model is currently modeling these as CNAMEs. If you run external-dns with --aws-prefer-cname things general work as expected, since A and AAAA queries will resolve through the CNAME. But when the AWS provider is converting these entries into ALIAS A records it doesn't create the required AAAA record.
Some alternative options:
-
Change the AWS provider to always create AAAA alias records as well. AFAICT, this does not change query response behavior. In particular, a AAAA query of a AAAA ALIAS record pointing to an NLB that does not itself have an AAAA record will return a 0-answer NOERROR response. You get the same response if there is no AAAA ALIAS record at all. I'm not sure if this would have scale or backwards compatibility concerns...
-
Create a non-provider specific annotation which helps external-dns know to create the AAAA record. It will look pretty similar to the above, but maybe it will meet your requirements. It should only apply to ALIAS-type records, and I don't know if any other DNS providers have such things. Something like placing the following annotation on the relevant
Service:external-dns.alpha.kubernetes.io/create-dualstack-alias-records: "true"orexternal-dns.alpha.kubernetes.io/alias-record-ip-address-type: "ipv6"|"ipv4"|"dualstack"orexternal-dns.alpha.kubernetes.io/alias-record-types: "a"|"aaaa"|"a, aaaa"or something...
I would really love to see some solution here. It's a useful feature and it's surprising missing behavior when someone runs across it.
A concern I have with this approach is that it is making assumptions about the behavior of the AWS Load Balancer Controller that aren't always going to be true. It's already possible to configure LBC to create dualstack ALBs by specifying that in the IngressClassParams instead of in an annotation. It's possible LBC could in the future be made configurable to create dual-stack NLBs by default, particularly since I plan to submit such a feature.
Is there a problem with always creating both A and AAAA alias records?
@johngmyers I wonder why we need this at all, because if ALIAS would return A/AAAA, then CNAME to the ALIAS would just work. Of course it would depend on the client that resolves to ask for AAAA instead of A.
@johngmyers I wonder why we need this at all, because if ALIAS would return A/AAAA, then CNAME to the ALIAS would just work. Of course it would depend on the client that resolves to ask for AAAA instead of A.
We need this due a particular quirk of ALIAS records in Route53. In particular. ALIAS is not a record type, it's a type of record value, which allows a record entry to resolve to an existing other record in Route53. Whereas a CNAME is a record type which has specific resolution semantics that resolvers follow. So, the way to setup an AAAA record in Route53 using an ALIAS record value is to create the AAAA record. And if you want an A record, you have to create the A record.
As mentioned above, creating both records in all cases, regardless of whether the actual alias'd name has an AAAA record or not, is potentially fine. Questions remain around backward compatibility, any potential scale concerns arising from blindly doubling the number of records, etc.
AFAICT, the concerns around IngressClassParams do not apply to this PR. They apply to existing code in external-dns. It's true that any provider-specific code will always need to be updated to support new behavior and functionality in providers. It's true that the existing modeling of ALIAS's as CNAMEs in source and the use of an endpoint label to model dual-stack CNAMEs in source makes it somewhat awkward to keep provider specific logic out of source. Those don't seem like compelling reasons to leave external-dns broken for this use case. Certainly further development work in the future could improve the situation if it's something the external-dns developers feel warrants further investment.
My proposal would be to remove endpoint.DualstackLabelKey, changing the AWS provider to act as if it was always present. That would probably keep all of the logic inside the AWS provider.
There might need to be some logic so that the provider would, possibly with the help of core, be able to reconcile discrepancies between the two alias records, such as the AAAA record not existing.
I can take a look at putting together such a PR if @logingood doesn't want dibs.