-
Notifications
You must be signed in to change notification settings - Fork 359
WIP: Add support for dual stack load balancers #1313
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
base: master
Are you sure you want to change the base?
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
This issue is currently awaiting triage. If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the The DetailsInstructions 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. |
|
/test pull-cloud-provider-aws-test |
|
Hey @kmala do we know what's happening with the Netlify checks? It looks like they are broken across many open PRs. TY |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
|
/test pull-cloud-provider-aws-test One more run to see if this is a rate limiting issue or some sort of genuine issue with my code. |
tthvo
left a comment
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 tested with a simple service below:
apiVersion: v1
kind: Service
metadata:
name: wordpress-mysql
labels:
app: wordpress
annotations:
service.beta.kubernetes.io/aws-load-balancer-type: nlb
service.beta.kubernetes.io/aws-load-balancer-ip-address-type: dualstack
spec:
ipFamilyPolicy: PreferDualStack
ports:
- port: 3306
selector:
app: wordpress
type: LoadBalancerThe CCM does create the dualstack NLB as expected 🚀
$ kubectl -n myns get svc/wordpress-mysql -o yaml | yq .status.loadBalancer.ingress
- hostname: <id>.elb.us-east-1.amazonaws.com
$ nslookup <id>.elb.us-east-1.amazonaws.com
# nslookup shows both public IPv4 EIP and IPv6 GUA addresses
$ aws elbv2 describe-load-balancers --names=<nlb-id> --query='LoadBalancers[0].[Type,IpAddressType,Scheme]'
[
"network",
"dualstack",
"internet-facing"
]Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
|
/test pull-cloud-provider-aws-test |
Add support for annotating a Service with a desired IP address type.
As an example:
apiVersion: v1
kind: Service
metadata:
annotations:
service.beta.kubernetes.io/aws-load-balancer-type: "nlb"
service.beta.kubernetes.io/aws-load-balancer-ip-address-type: "dualstack"
service.beta.kubernetes.io/aws-load-balancer-target-group-ip-address-type: "ipv6"
spec:
type: LoadBalancer
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
The Kubernetes Service API allows load balancer IP families to be configured via the Service.spec.ipFamilies and Service.spec.ipFamilyPolicy fields. These should be considered along with the annotations that we are adding, especially since this is how other cloud providers do it. Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
|
/test pull-cloud-provider-aws-test |
|
/test pull-cloud-provider-aws-check |
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
|
/test pull-cloud-provider-aws-test |
|
@kmala I'm going to do some manual testing with this PR starting today before I mark it as ready for review, but would you be able to give it a look by any chance? |
|
/assign @kmala |
|
Tagging you all because we've talked at various points. @tthvo (also working on dual stack enablement in Cluster API Provider AWS) Summing up some of the various discussions. Please correct me if any of these things are wrong.
I can see us proceeding in a few ways here.
In any of the 3 approaches here, I'd like to keep the number of total fields to the absolute minimum for supporting dual stack on load balancers. If users need something beyond that, they should opt for the ALBC. That minimum would be:
I'd appreciate your thoughts here - which of these options seems more workable to you? |
mtulio
left a comment
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.
Great work! A few nits, and questions related to the restrictions on target ipv6.
| // ServiceAnnotationLoadBalancerIPAddressType is the annotation used on the service | ||
| // to specify the IP address type for the load balancer. Supported values are "ipv4" and "dualstack". | ||
| // Defaults to "ipv4". Only supported on NLB. | ||
| const ServiceAnnotationLoadBalancerIPAddressType = "service.beta.kubernetes.io/aws-load-balancer-ip-address-type" | ||
|
|
||
| // ServiceAnnotationLoadBalancerTargetGroupIPAddressType is the annotation used on the service | ||
| // to specify the IP address type for the target groups. Supported values are "ipv4" and "ipv6". | ||
| // Defaults to "ipv4". Only supported on NLB. | ||
| const ServiceAnnotationLoadBalancerTargetGroupIPAddressType = "service.beta.kubernetes.io/aws-load-balancer-target-group-ip-address-type" | ||
|
|
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.
Please also update the service controller documentation: https://github.com/kubernetes/cloud-provider-aws/blob/master/docs/service_controller.md
| if len(sourceRangeCidrs) == 0 { | ||
| sourceRangeCidrs = append(sourceRangeCidrs, "0.0.0.0/0") | ||
|
|
||
| // For dual-stack or IPv6 load balancers, also add IPv6 default route |
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.
route or rules?
| // For dual-stack or IPv6 load balancers, also add IPv6 default route | |
| // For dual-stack or IPv6 load balancers, also add IPv6 default rules |
| // Validate ipFamilyPolicy and ipFamilies | ||
| if v.apiService.Spec.IPFamilyPolicy != nil { |
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.
nit: what about return early when IPFamilyPolicy==nil?
| if _, present := v.annotations[ServiceAnnotationLoadBalancerIPAddressType]; present { | ||
| if !isNLB { | ||
| return fmt.Errorf("ip address type annotation is only supported for NLB") | ||
| } | ||
| } | ||
|
|
||
| if _, present := v.annotations[ServiceAnnotationLoadBalancerTargetGroupIPAddressType]; present { | ||
| if !isNLB { | ||
| return fmt.Errorf("target group ip address type annotation is only supported for NLB") | ||
| } | ||
| } |
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.
do we need to validate valid values were set here?
| } | ||
|
|
||
| // Fall back to spec.ipFamilies and spec.ipFamilyPolicy | ||
| if service.Spec.IPFamilyPolicy != nil { |
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.
what about return early here with elbv2types.IpAddressTypeIpv4?
| if _, present := v.annotations[ServiceAnnotationLoadBalancerIPAddressType]; present { | ||
| if !isNLB { | ||
| return fmt.Errorf("ip address type annotation is only supported for NLB") | ||
| } |
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.
Considering this statement from AWS docs:
Also, with dualstack Network Load Balancers, client IP address preservation does not work when translating IPv4 addresses to IPv6, or IPv6 to IPv4 addresses. Client IP address preservation only works when client and target IP addresses are both IPv4 or both IPv6.
Do we need to add some validation when the preserve ip is set?
| } | ||
| if len(networkInterface.Ipv6Addresses) > 0 { | ||
| // Return the first IPv6 address | ||
| return aws.ToString(networkInterface.Ipv6Addresses[0].Ipv6Address) |
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.
In order to satisfy the requirement of the target ip family ipv6, I believe you need to verify if the Ipv6Address is primary (IsPrimaryIpv6: true).
https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_NetworkInterfaceIpv6Address.html
When registering targets by instance ID for a IPv6 target group, the targets must have an assigned primary IPv6 address. To learn more, see IPv6 addresses in the Amazon EC2 User Guide
When registering targets by IP address for an IPv6 target group, the IP addresses that you register must be within the VPC IPv6 CIDR block or within the IPv6 CIDR block of a peered VPC.
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.
Hey @mtulio That's right, but the primary IPv6 requirement actually depends on the target type of the Target Group. In our case, the following type is applicable:
-
instance: The instance must be in the same VPC as NLB and is assigned a "stable" IPv6 address. Unlike IPv4, IPv6 address, by default, can be re-assigned elsewhere. Thus, we need to setIsPrimaryIpv6: trueto make sure the IPv6 address is tied to the ENI of the instance for its entire lifecycle. I guess "primary" on AWS means "stable/owned".When registering targets by instance ID for a IPv6 target group, the targets must have an assigned primary IPv6 address. To learn more, see IPv6 addresses in the Amazon EC2 User Guide
-
IP: There is no requirement for a "stable" IPv6 address (or setting flagIsPrimaryIpv6). But the IPv6 address must be within the VPC IPv6 CIDR of the cluster.When registering targets by IP address for an IPv6 target group, the IP addresses that you register must be within the VPC IPv6 CIDR block or within the IPv6 CIDR block of a peered VPC.
My understanding is that we should prefer instanceID target type in order to ensure traffic will always reach the intended target (i.e. cluster nodes). Registering with IP address will pin the traffic to the IP, which may not be the cluster node at all as it can be re-assigned to other services, for example, another non-cluster EC2 instance.
@nrb IIUC, we should make the CCM prefer registering by instance when, let's say, primary IPv6 is available. Otherwise, fall back to register by the IP address?
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds basic support for configuring the IP address family on Services.
This functionality overlaps with the ALBC somewhat, but the goal here is to provide much more basic functionality, not the full scope of ALBC.
Which issue(s) this PR fixes:
Fixes #1219
Special notes for your reviewer:
Does this PR introduce a user-facing change?: