AWS NLB: Support cross-zone load balancing annotation #61064

johanneswuerbach · 2018-03-12T21:12:46Z

AWS Network Load Balancer recently got support for cross-zone load balancing.

Use the existing service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled
annotation to configure it.

AWS release announcement:
https://aws.amazon.com/about-aws/whats-new/2018/02/network-load-balancer-now-supports-cross-zone-load-balancing/

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Special notes for your reviewer:

Release note:

AWS NLB: Support cross-zone load balancing annotation

johanneswuerbach · 2018-03-12T21:17:01Z

@chrislovecnm / @jsafrane I'm not entirely sure how to test this as the entire NLB feature seems to be barely test covered, any hints?

johanneswuerbach · 2018-04-04T12:33:12Z

/sig aws

k8s-ci-robot · 2018-04-15T09:14:24Z

@johanneswuerbach: GitHub didn't allow me to assign the following users: micahhausler.

Note that only kubernetes members and repo collaborators can be assigned.

In response to this:

/assign micahhausler

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.

johanneswuerbach · 2018-05-17T07:55:12Z

/assign @justinsb

seh · 2018-05-30T00:05:54Z

@justinb, have you taken a look at this PR? I ran into the lack of cross-AZ support for NLBs today, and only discovered the root of the problem by studying the implementation. We have an implementation available here. What's the next step?

idealhack · 2018-07-03T11:06:09Z

/ok-to-test

micahhausler

Great work! Just a few changes requested

micahhausler · 2018-07-12T18:38:03Z

pkg/cloudprovider/providers/aws/aws.go

+
+		// Determine if cross zone load balancing enabled/disabled has been specified
+		crossZoneLoadBalancingEnabledAnnotation := annotations[ServiceAnnotationLoadBalancerCrossZoneLoadBalancingEnabled]
+		if crossZoneLoadBalancingEnabledAnnotation != "" {


Why add a parameter to ensureLoadBalancerv2() since annotations are already getting passed?

Could you move this logic into ensureLoadBalancerv2()

micahhausler · 2018-07-12T18:44:07Z

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

+			}
+			describeAttributesOutput, err := c.elbv2.DescribeLoadBalancerAttributes(describeAttributesRequest)
+			if err != nil {
+				glog.Warning("Unable to retrieve load balancer attributes during attribute sync")


Since you're returning the error, you don't need to log it here. Can you wrap it instead?

fmt.Errorf("My error message: %q", err)

micahhausler · 2018-07-12T18:58:30Z

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

+				return nil, err
+			}
+
+			foundAttributes := &describeAttributesOutput.Attributes


Why dereference this here and add a *foundAttributes below? Why not just

for _, foundAttribute := range describeAttributesOutput.Attributes { ... }

micahhausler · 2018-07-12T19:09:39Z

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

+		// Whether the ELB was new or existing, sync attributes regardless. This accounts for things
+		// that cannot be specified at the time of creation and can only be modified after the fact,
+		// e.g. idle connection timeout.
+		if len(loadBalancerAttributes) > 0 {


It might help this read better if loadBalancerAttributes was renamed to something like desiredLoadBalancerAttributes. Reading below,

if targetValue, ok := loadBalancerAttributes[*foundAttribute.Key]

would be easier to understand if it was

if targetValue, ok := desiredLoadBalancerAttributes[*foundAttribute.Key]

micahhausler · 2018-07-12T19:11:53Z

pkg/cloudprovider/providers/aws/aws.go

+		// Determine if cross zone load balancing enabled/disabled has been specified
+		crossZoneLoadBalancingEnabledAnnotation := annotations[ServiceAnnotationLoadBalancerCrossZoneLoadBalancingEnabled]
+		if crossZoneLoadBalancingEnabledAnnotation != "" {
+			loadBalancerAttributes["load_balancing.cross_zone.enabled"] = crossZoneLoadBalancingEnabledAnnotation


This value should be "true" not whatever string value the user entered.

The reasoning behind using the value here, was to make setting it explicitly to false work. I know that you could also delete the annotation to revert it to false, but that seemed not really obvious. wdyt?

Yea, you're right true or false could be valid values. Can you make sure you call strconv.ParseBool() like the check for access logs does to make sure the input is valid? https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/aws/aws.go#L3415-L3425

micahhausler · 2018-07-12T19:12:45Z

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

+
+			// Identify to be changed attributes
+			for _, foundAttribute := range *foundAttributes {
+				if targetValue, ok := loadBalancerAttributes[*foundAttribute.Key]; ok {


You should add an else case for if !ok, meaning the annotation for the attribute was deleted.

From the docs:

Any existing attributes that you do not modify retain their current values.

If the annotation gets removed, we would want to set the value to "false" and add it to changedAttributes.

@micahhausler Wouldn't that mean that we set any value to false, also the attributes k8s doesn't know about and which might have a different default (e.g. routing.http2.enabled, while only being a valid attribute for ALBs defaults to true).

Instead I set now load_balancing.cross_zone.enabled to false by default, so a set annotation will change it, but an eventually removed annotation changes it back to false.

micahhausler · 2018-07-23T17:10:31Z

👍
/lgtm

johanneswuerbach · 2018-07-23T18:18:45Z

/retest

johanneswuerbach · 2018-07-23T18:49:40Z

/test pull-kubernetes-e2e-gce

idealhack · 2018-07-29T15:31:49Z

@kubernetes/sig-aws-misc
ping for approval

k8s-github-robot · 2018-08-07T08:12:32Z

/test all

Tests are more than 96 hours old. Re-running tests.

nikhita · 2018-09-29T22:16:04Z

/retest

itsmepetrov · 2018-11-20T21:40:24Z

Any updates?

nukepuppy · 2018-12-11T17:16:40Z

would be great to get this going.. kind of wasteful otherwise when using NLB

micahhausler · 2018-12-19T23:41:51Z

/test pull-kubernetes-godeps

micahhausler · 2018-12-19T23:43:53Z

/milestone v1.14

k8s-ci-robot · 2018-12-19T23:43:53Z

@micahhausler: You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

/milestone v1.14

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.

micahhausler · 2018-12-19T23:48:23Z

/approve

idealhack · 2018-12-20T03:18:49Z

/assign @zmerlynn @jsafrane

jsafrane · 2019-01-02T14:23:34Z

/approve

k8s-ci-robot · 2019-01-02T14:24:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johanneswuerbach, jsafrane, micahhausler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/cloudprovider/providers/aws/OWNERS~~ [jsafrane]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2019-01-02T17:56:21Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

AWS Network Load Balancer recently got support for cross-zone load balancing. Use the existing `service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled` annotation to configure it.

johanneswuerbach · 2019-01-02T19:39:53Z

@jsafrane rebased and replaced glog usage with klog. CI should pass now 🤞

jsafrane · 2019-01-03T08:41:12Z

/lgtm
/retest

majidma-ag · 2020-10-14T06:30:32Z

@johanneswuerbach, can you please let me know if this feature (NLB-cross zone load balancing) is available in kubernetes v1.15 ?

defyjoy · 2021-02-04T05:32:22Z

AWS docs are not provided for this one . I understand this is a kubernetes only annotation but is it not supposed to be included by AWS docs ? Is this the official doc for the annotation ? Also if I understand correctly this is for the controller annotation for service not for the ingress resource itself .

https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.1/guide/service/annotations/

k8s-ci-robot requested review from chrislovecnm and jsafrane March 12, 2018 21:12

k8s-ci-robot added the sig/aws label Apr 4, 2018

k8s-ci-robot assigned justinsb May 17, 2018

johanneswuerbach force-pushed the nlb-cross-zone branch from 259ad3b to decabbd Compare May 17, 2018 07:59

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 3, 2018

micahhausler suggested changes Jul 12, 2018

View reviewed changes

johanneswuerbach force-pushed the nlb-cross-zone branch 2 times, most recently from f478ff6 to ff13751 Compare July 17, 2018 17:39

k8s-ci-robot assigned micahhausler Jul 23, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2018

micahhausler mentioned this pull request Jul 24, 2018

Support AWS Network Load Balancer kubernetes/enhancements#423

Closed

aledbf mentioned this pull request Oct 16, 2018

AWS Cross Zone Load Balancing not working on NLB kubernetes/ingress-nginx#3254

Closed

k8s-ci-robot assigned jsafrane and zmerlynn Dec 20, 2018

idealhack added this to the v1.14 milestone Dec 20, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 2, 2019

AWS NLB: Support cross-zone load balancing annotation

5e6d865

AWS Network Load Balancer recently got support for cross-zone load balancing. Use the existing `service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled` annotation to configure it.

johanneswuerbach force-pushed the nlb-cross-zone branch from ff13751 to 5e6d865 Compare January 2, 2019 19:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 2, 2019

k8s-ci-robot added the sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. label Jan 2, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 3, 2019

k8s-ci-robot merged commit d7e0d9b into kubernetes:master Jan 3, 2019

johanneswuerbach deleted the nlb-cross-zone branch January 4, 2019 19:56

aylei mentioned this pull request Jul 24, 2019

Enable nlb cross zone load balancing by default pingcap/tidb-operator#686

Merged

AWS NLB: Support cross-zone load balancing annotation #61064

AWS NLB: Support cross-zone load balancing annotation #61064

Conversation

johanneswuerbach commented Mar 12, 2018

johanneswuerbach commented Mar 12, 2018

johanneswuerbach commented Apr 4, 2018

k8s-ci-robot commented Apr 15, 2018

johanneswuerbach commented May 17, 2018

seh commented May 30, 2018

idealhack commented Jul 3, 2018

micahhausler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micahhausler Jul 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanneswuerbach Jul 17, 2018 • edited

Choose a reason for hiding this comment

micahhausler commented Jul 23, 2018

johanneswuerbach commented Jul 23, 2018

johanneswuerbach commented Jul 23, 2018

idealhack commented Jul 29, 2018

k8s-github-robot commented Aug 7, 2018

nikhita commented Sep 29, 2018

itsmepetrov commented Nov 20, 2018

nukepuppy commented Dec 11, 2018

micahhausler commented Dec 19, 2018

micahhausler commented Dec 19, 2018

k8s-ci-robot commented Dec 19, 2018

micahhausler commented Dec 19, 2018

idealhack commented Dec 20, 2018

jsafrane commented Jan 2, 2019

k8s-ci-robot commented Jan 2, 2019

fejta-bot commented Jan 2, 2019

johanneswuerbach commented Jan 2, 2019

jsafrane commented Jan 3, 2019

majidma-ag commented Oct 14, 2020 • edited

defyjoy commented Feb 4, 2021

micahhausler Jul 17, 2018 •

edited

johanneswuerbach Jul 17, 2018 •

edited

majidma-ag commented Oct 14, 2020 •

edited