Add NLB IP dualstack mode support (ipv6 support for NLB) #2050
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @logingood! |
k8s-ci-robot
added
the
size/M
|
/kind feature |
k8s-ci-robot
added
the
kind/feature
There was a problem hiding this comment.
Added a few comments. Did you test this @logingood? I don't think this is going to be enough to create AAAA records 🤔
docs/tutorials/alb-ingress.md
Outdated
|
|
||
| ## Dualstack NLBs | ||
|
|
||
| The ALB ingress controller satisifies service of "LoadBalancer" type with |
There was a problem hiding this comment.
well actually it is aws loadbalancer ingress controller. It satisfies ingress resource and service resource of type loadbalancer in presence of annotation. I will change just in case to avoid confusion.
I'll test it today, I was relying on this piece of code, which expects the annotation to be present external-dns/provider/aws/aws.go Lines 558 to 568 in a5baad2 external-dns/provider/aws/aws_test.go Line 932 in a5baad2 |
@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:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
Any movement on this? |
|
hey is there any update on that ? @Raffo @seanmalloy is there any alternatives to that ? |
|
/remove-lifecycle stale |
k8s-ci-robot
removed
the
lifecycle/stale
|
/assign @njuettner |
|
Bump please? |
|
How does this one compare to #2309? |
|
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:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
| @@ -40,6 +40,10 @@ import ( | |||
|
|
|||
| const ( | |||
| defaultTargetsCapacity = 10 | |||
| // NLBDualstackAnnotationKey is the annotation used for determining if an NLB LoadBalancer is dualstack | |||
| NLBDualstackAnnotationKey = "service.beta.kubernetes.io/aws-load-balancer-ip-address-type" | |||
There was a problem hiding this comment.
This doesn't look right, there shouldn't be any provider specific setting inside generic source like ingress or service
There was a problem hiding this comment.
More specifically, what records to create should be determined by spec.ipFamilies. If you strictly test for IPv4 and IPv6 you'll also support LoadBalancer implementations that are ipv6-only.
There was a problem hiding this comment.
This PR is for AWS NLB and that is how AWS does it. AWS currently does not use spec.ipFamilies.
For what I see it replicates what is being done for ingresses:
func (sc *ingressSource) setDualstackLabel(ingress *networkv1.Ingress, endpoints []*endpoint.Endpoint) {
val, ok := ingress.Annotations[ALBDualstackAnnotationKey]
if ok && val == ALBDualstackAnnotationValue {
log.Debugf("Adding dualstack label to ingress %s/%s.", ingress.Namespace, ingress.Name)
for _, ep := range endpoints {
ep.Labels[endpoint.DualstackLabelKey] = "true"
}
}
}
I'd suggest to move forward with this.
There was a problem hiding this comment.
There was a problem hiding this comment.
From kubectl explain service.spec.ipFamilies:
These families must correspond to the values of the clusterIPs field, if specified. Both clusterIPs and ipFamilies are governed by the ipFamilyPolicy field.
To me this means that the load balancer controller shouldn't set the ipFamilies to contain IPv6 unless the Service also has an IPv6 clusterIPs entry. Considering that the load balancer controller is distinct from the CNI, this doesn't seem like it can be used to indicate if the load balancer has a public IPv6 address. And hence they needed to use the annotation instead.
There was a problem hiding this comment.
@szuecs I'm asking for clarification because I don't know what this means in the context of external-dns with type: LoadBalancer Service behind an AWS NLB:
you have to follow the ClusterIPs entries (i.e. the Services fields), otherwise, if the load balancer has ipv6 and ipv4 but the service only ipv4, or they implement some nat64 or it will not work, at least I can't see how
The external-dns entries do not have anything to do with ClusterIPs on the Service. I don't know what the task associated with that sentence would be. I'm asking for clarification on what you think the solution looks like.
Once again, for clarification, the use case is: there is a single stack Kubernetes cluster where everything is IPv4. All clusterIPs for all services within the cluster will always be IPv4 addresses. The load balancer controller is capable of provisioning a dual-stack load balancer. The dual-stack load balancer has a routable IPv6 address. Incoming IPv6 traffic arriving at the load balancer is NAT'd and sent to the Kubernetes Service's IPv4 clusterIP(s). Nothing in the Kubernetes API entities knows the external IPv6 address. The load balancer controller provisions this type of load balancer based on an annotation on the Service entity. The exact same behavior happens for Ingress entities as well. That use case is handled in external-dns with provider specific code in the source/ingress.go file here:
external-dns/source/ingress.go
Line 219 in 4f41229
An example of a type: LoadBalancer Service which is configured for NLB DualStack looks like this:
apiVersion: v1
items:
- apiVersion: v1
kind: Service
metadata:
annotations:
external-dns.alpha.kubernetes.io/hostname: anexternalname.example.com.,anotherexternalname.example.com.
kubectl.kubernetes.io/last-applied-configuration: |
{"apiVersion":"v1","kind":"Service","metadata":{"annotations":{"external-dns.alpha.kubernetes.io/hostname":"anexternalname.example.com.,anotherexternalname.example.com.","service.beta.kubernetes.io/aws-load-balancer-ip-address-type":"dualstack","service.beta.kubernetes.io/aws-load-balancer-scheme":"internet-facing","service.beta.kubernetes.io/aws-load-balancer-target-group-attributes":"proxy_protocol_v2.enabled=true"},"labels":{"skaffold.dev/run-id":"edec5eb5-9454-4d5f-afb4-7134aad56c3b"},"name":"edgeproxy-external","namespace":"edgeproxy"},"spec":{"externalTrafficPolicy":"Cluster","loadBalancerClass":"service.k8s.aws/nlb","ports":[{"name":"http","port":80,"protocol":"TCP","targetPort":8080},{"name":"https","port":443,"protocol":"TCP","targetPort":8443}],"selector":{"app":"edgeproxy"},"type":"LoadBalancer"}}
service.beta.kubernetes.io/aws-load-balancer-ip-address-type: dualstack
service.beta.kubernetes.io/aws-load-balancer-scheme: internet-facing
service.beta.kubernetes.io/aws-load-balancer-target-group-attributes: proxy_protocol_v2.enabled=true
creationTimestamp: "2023-04-28T23:17:29Z"
finalizers:
- service.k8s.aws/resources
labels:
skaffold.dev/run-id: edec5eb5-9454-4d5f-afb4-7134aad56c3b
name: edgeproxy-external
namespace: edgeproxy
resourceVersion: "125018992"
uid: 404573f8-4163-48c2-a2b6-2c42ccaf0e76
spec:
allocateLoadBalancerNodePorts: true
clusterIP: 172.20.7.113
clusterIPs:
- 172.20.7.113
externalTrafficPolicy: Cluster
internalTrafficPolicy: Cluster
ipFamilies:
- IPv4
ipFamilyPolicy: SingleStack
loadBalancerClass: service.k8s.aws/nlb
ports:
- name: http
nodePort: 30027
port: 80
protocol: TCP
targetPort: 8080
- name: https
nodePort: 31165
port: 443
protocol: TCP
targetPort: 8443
selector:
app: edgeproxy
sessionAffinity: None
type: LoadBalancer
status:
loadBalancer:
ingress:
- hostname: k8s-edgeproxy-edgeproxyext-fd8d0357e7ab-e7068f6d4543.elb.us-west-2.amazonaws.com
kind: List
metadata:
resourceVersion: ""
selfLink: ""We do not want to create non-ALIAS A/AAAA records – we already have a solution which works for non-ALIAS records, which is CNAMEs. Presumably #3554 is another solution which works for non-ALIAS records.
We want to create ALIAS records in Route53. A simple reason to prefer ALIAS records is that they are cheaper to serve to clients in Route53's billing model. They are also the default behavior of external-dns and ideally exeternal-dns would support dual-stack NLBs out of the box without further configuration. It does support dual-stack ALBs targeting K8s Ingresses today.
A potential solution is to have the provider attempt to resolve the A/AAAA records and then to create the corresponding ALIAS records based on which resolutions return valid addresses. There are many valid solutions. We could change the domain model to pass all the service annotations up and let the provider pick dual stack instead of making source choose it when the creating the CNAME endpoint. We could change the provider to always create AAAA records if its making ALIASes. We could use an external-dns-specific annotation to demand source put dual-stack = true in the endpoint labels.
I'm asking for guidance on what is acceptable here. Obviously I think the current PR is perfectly reasonable: it follows existing practices in the code base; it works and is documented; it solves a surprising potential for brokeness which someone can encounter when they deploy external-dns. As a solution, it has the marked benefit of actually existing – the external-dns project is one click away from supporting DualStack NLBs on AWS! The external-dns maintainers seem to feel otherwise. That's fine. But for two years there has been very little help in actually driving towards a solution which lets this use case work out of the box with external-dns. People want this to work with their clusters. Given the information in this PR, if you tell us what is acceptable, someone will probably make it happen.
There was a problem hiding this comment.
Yep, exactly. I don't think it's technically NAT64, but there's only IPv4 involved on the service / target group / Kubernetes cluster side. The load balancer has its own externally routable IPv6 addresses. The load balancer has explicit targets which are eligible to receive traffic based on configured target groups and health check status and the likes. It translates incoming IPv6 connection traffic into corresponding IPv4 connection traffic when it forwards the traffic to the backends.
Press release here: https://aws.amazon.com/about-aws/whats-new/2020/11/network-load-balancer-supports-ipv6/. The language they use there is: Your Network Load Balancer seamlessly converts IPv6 traffic to IPv4 before routing it to back-end targets..
Documentation for the feature is partially available here, for example: https://docs.aws.amazon.com/elasticloadbalancing/latest/network/load-balancer-target-groups.html#target-group-ip-address-type
AFAIK, the IP protocol family used by clients communicating with the load balancer and the IP protocol family used by the load balancer communicating with the target groups is completely decoupled. You could also, for example, have an IPv4 NLB in front of a purely IPv6 target group.
GCP load balancers can similarly terminate IPv6 traffic and translate it to IPv4 going to the backends: https://cloud.google.com/load-balancing/docs/ipv6. The language there is more explicit about the IPv6 connection being terminated at the GCP load balancer.
For TLS traffic to an NLB, the AWS documentation is explicit about connection termination, but for IPv6 -> IPv4 TCP translated traffic, I'm not 100% certain they claim full termination – it might be more reframing + fragmentation/MTU handling + NAT or something like that. Seems hairy to get right without connection termination, so who knows...all speculation on my part.
There was a problem hiding this comment.
@reltuk I am very happy to be that close to having dual stack support.
I have to think about and test in a cluster how it can be done best.
I think if we can ALIAS an ALIAS it would be the best. In the past afaik it wasn't possible, that's why we CNAME the lb ALIAS. Resolve to IPs A/AAAA is a poor solution as I tried to tell. It would work low scale cloud loadbalancers, because at scale you will get more and more IPs in the pool of a cloud loadbalancer and DNS clients don't get all IPs resolved and that means we depend on which kind of IPs we resolved that can easily lead to flapping and even worse loadbalancer can not scale to all handlers they have, if we don't find all IPs.
The correct wording, I think what nlb does is forwarding. It's not proxying in case of ip preservation. I didn't check if they can do ip preservation for dualstack but I don't see the problem.
|
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:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
New custom build with this patch source: https://github.com/sylr/external-dns/commits/v0.12%2Bsylr |
k8s-ci-robot
added
the
needs-rebase
|
@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. Would love to see this working and then merged into mainline. |
AWS Loadbalancer Ingress controller supports [service resource of LoadBalancer type](https://github.com/kubernetes-sigs/aws-load-balancer-controller/blob/main/docs/guide/service/nlb_ip_mode.md). Currently dualstack annotation for `service` is ignored by external dns and corresponding `AAAA` record is not beging created. This PR adds support of ipv6 similar to ingress resource/ALB.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: crawforde, logingood, mesge, sylr 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 |
k8s-ci-robot
removed
the
needs-rebase
|
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 Some alternative options:
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 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. |
We need this due a particular quirk of 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 |
|
My proposal would be to remove 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. |
|
My competing PR is #3605. |
|
While I thought CNAMEs were a reasonable workaround, I did come across one situation where it does not work. If you have a delegated zone like |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
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. |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
Description
AWS Loadbalancer Ingress controller supports service resource of
LoadBalancer type.
Currently dualstack annotation for
serviceis ignored by external dnsand corresponding
AAAArecord is not being created.This PR adds support of ipv6 similar to ingress resource/ALB.
Fixes #ISSUE
Checklist