Enable Support for creating Dual Stack NLB #766
Conversation
ayruslore
force-pushed
the
enable-dualstack-nlb
branch
from
ec88f55 to
d8b5cb4
Compare
kubernetes/ingress.go
Outdated
| const ( | ||
| // ingressALBIPAddressType is used in external-dns, https://github.com/kubernetes-incubator/external-dns/pull/1079 | ||
| ingressALBIPAddressType = "alb.ingress.kubernetes.io/ip-address-type" | ||
| ingressNLBIPAddressType = "nlb.ingress.kubernetes.io/ip-address-type" |
There was a problem hiding this comment.
Does it make sense to split between alb and nlb?
NLB did not implement this feature in the past so
There was a problem hiding this comment.
So if we are to merge the annotations into one say lb.ingress.kubernetes.io/ip-address-type then we need to deprecate the existing one. And promote to only use the new one. Cause the using the existing annotation with alb. for when using an NLB is just confusing for users and seems wrong.
My idea was without much effort / changes, we can keep using the same one for ALB and add a new annotation for NLB. (But maybe its a shortcut or not clean...). Feel free to suggest.
There was a problem hiding this comment.
OR we can also remove the annotation for NLB altogether and not even give the users to control the ipAddressType for NLB. It will be whatever flag the controller is set to. Feels a bit restrictive though. I would like to have the control.
What do you think ?
There was a problem hiding this comment.
I would suggest to use ingress.kubernetes.io/ip-address-type and deprecate "alb.ingress.kubernetes.io/ip-address-type".
If we find "alb.ingress.kubernetes.io/ip-address-type", then log a warning for now and add a documentation in the readme where we document the annotations in a table.
There was a problem hiding this comment.
@szuecs I have updated the code to deprecate the old annotation and use the new one. So we log two warnings one for using the old annotation and then one for using both annotations.Also added tests accordingly.
ayruslore
force-pushed
the
enable-dualstack-nlb
branch
from
5b32efd to
88e89d3
Compare
| @@ -0,0 +1,16 @@ | |||
| apiVersion: networking.k8s.io/v1 | |||
There was a problem hiding this comment.
all the test data is copied from existing data and then updated to use dualstack
This PR Enables Support for creating Dual Stack NLB. This is required for Enabling AWSCNI mode in EKA IPv6 clusters. For AWSCNI mode to be enabled in an IPv6 cluster we need to have IPv6 target groups. And IPv6 target groups can only be attached to a `dualstack` load balancer. There is already a flag `--ip-addr-type` to control the ALB IP address type but for NLB we hard code it to `ipv4`, this PR just removes the hardcoding and passes the flag value. It also adds a new annotation `ingress.kubernetes.io/ip-address-type` so that users are able to control the IP Address Type for the NLB and deprecates the existing annotation `alb.ingress.kubernetes.io/ip-address-type`. Signed-off-by: speruri <[email protected]>
ayruslore
force-pushed
the
enable-dualstack-nlb
branch
from
bb8114d to
2e9c48d
Compare
| albIPType := getAnnotationsString(annotations, ingressALBIPAddressType, "") | ||
| ipType := getAnnotationsString(annotations, ingressIPAddressType, "") | ||
| if albIPType != "" { | ||
| log.Warnf("Deprecated annotation %q in use for %q resource named %q in namespace %q", ingressALBIPAddressType, typ, metadata.Name, metadata.Namespace) | ||
|
|
||
| if ipType != "" { | ||
| log.Warnf("Both annotations are set for %q resource named %q in namespace %q, deprecated annotation %q=%q will be ignored, using %q=%q", | ||
| typ, metadata.Name, metadata.Namespace, ingressALBIPAddressType, albIPType, ingressIPAddressType, ipType) | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure we should bother deprecating or just support both like
ipType := getAnnotationsString(annotations, ingressIPAddressType,
getAnnotationsString(annotations, ingressALBIPAddressType, ""))anyways after extracting the values I think we should end up with a single variable ipType to have a simple switch over it below.
We should better return an error instead of logging in case both values are used (or at least if they are different).
|
|
||
| ipAddressType := aws.IPAddressTypeIPV4 | ||
| if getAnnotationsString(annotations, ingressALBIPAddressType, "") == aws.IPAddressTypeDualstack { | ||
| ipAddressType := a.ingressIpAddressType |
There was a problem hiding this comment.
This is strange - looks like a.ingressIpAddressType was never used before for anything (or broken at some point) although we have a flag for it. Could you please check history to understand that?
With this change we'll use flag value as the default ip address type for all ingresses so if any deployment happened to have -ip-addr-type=dualstack this will update all loadbalancers iiuc.
This PR Enables Support for creating Dual Stack NLB.
This is required for Enabling AWSCNI mode in EKA IPv6 clusters. For AWSCNI mode to be enabled in an IPv6 cluster we need to have IPv6 target groups. And IPv6 target groups can only be attached to a
dualstackload balancer.There is already a flag
--ip-addr-typeto control the ALB IP address type but for NLB we hard code it toipv4, this PR just removes the hardcoding and passes the flag value. It also adds a new annotationingress.kubernetes.io/ip-address-typeso that users are able to control the IP Address Type for the NLB and deprecates the existing annotationalb.ingress.kubernetes.io/ip-address-type.