reconcile AWS NLB attributes on service creation #95247

kishorj · 2020-10-02T03:44:20Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
The NLB attributes are now reconciled immediately after creating the load balancer. Previously, the LB attributes related annotations were applied only during updates to the service object. This required a two step process to apply the attributes correctly.
Which issue(s) this PR fixes:

Fixes #81584
Fixes #82595

Special notes for your reviewer:
Tested on kops cluster by specifying the following annotations on a service, the load balancer type is 'nlb' -

service.beta.kubernetes.io/aws-load-balancer-type
service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled
service.beta.kubernetes.io/aws-load-balancer-access-log-enabled
service.beta.kubernetes.io/aws-load-balancer-access-log-s3-bucket-name
service.beta.kubernetes.io/aws-load-balancer-access-log-s3-bucket-prefix

Test summary

The annotation values get applied as expected during service creation
If aws-load-balancer-access-log-enabled is set to false, the other bucket-name and bucket-prefix get ignored
If service is created without the LB attributes annotations, default values are configured
Modifying the annotations results in corresponding change to the LB attributes

Does this PR introduce a user-facing change?:

The AWS network load balancer attributes can now be specified during service creation

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2020-10-02T03:44:27Z

@kishorj: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 · 2020-10-02T03:44:29Z

Hi @kishorj. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

kishorj · 2020-10-02T05:27:09Z

/assign m00nf1sh

Chili-Man · 2020-10-20T05:51:37Z

We're running into #82595 (cross zone loadbalancing annotation on NLB ignored on creation); it would be great if this can get reviewed and merged; I'd be more then happy to help with getting movement on this @M00nF1sh

2rs2ts · 2020-10-21T18:10:34Z

staging/src/k8s.io/legacy-cloud-providers/aws/aws_loadbalancer.go

+		if err := c.reconcileLBAttributes(aws.StringValue(loadBalancer.LoadBalancerArn), annotations); err != nil {
+			return nil, err
+		}


This is just my $0.02 as a very infrequent contributor, but this code seems to be copy-pasted from here: https://github.com/kubernetes/kubernetes/pull/95247/files#diff-3847bd7a991eef9e3fc1c058562e9a7eac0dbfbcefb28d60c9ce03080eb4caffR371

I wonder if that code can be taken out of this greater if/else to reduce duplication. The only snag I see is this brief section that comes after where this was copy-pasted from: https://github.com/kubernetes/kubernetes/pull/95247/files#diff-3847bd7a991eef9e3fc1c058562e9a7eac0dbfbcefb28d60c9ce03080eb4caffR375-R388

However, I would think that running it regardless of the great if/else case would not be harmful... not sure what the core maintainers think about this.

Regardless, I think a comment is needed here. It seems unintuitive to have to do this on creation, so future generations will thank you for explaining it and not relying on git history.

Thanks for the review and the comments. Your concerns on the maintainability are valid.

Since the ELBv2 CreateLoadBalancer API doesn't support specifying the load balancer attributes, they need to be reconciled separately. Hence the reconcile call immediately after creation. I feel the if-else block is ripe for refactor.

M00nF1sh

/lgtm
/ok-to-test

M00nF1sh · 2020-11-06T23:18:12Z

/approve

k8s-ci-robot · 2020-11-06T23:18:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kishorj, M00nF1sh

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:

~~staging/src/k8s.io/legacy-cloud-providers/aws/OWNERS~~ [M00nF1sh]

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

aSapien · 2021-02-24T17:19:14Z

Similar ticket in the external provider repo: kubernetes/cloud-provider-aws#180

reconcile NLB attributes on service creation

53fc51f

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 2, 2020

k8s-ci-robot requested review from jsafrane and justinsb October 2, 2020 03:45

k8s-ci-robot added area/cloudprovider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 2, 2020

kishorj changed the title ~~[WIP] reconcile AWS NLB attributes on service creation~~ reconcile AWS NLB attributes on service creation Oct 2, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2020

k8s-ci-robot assigned M00nF1sh Oct 2, 2020

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 2, 2020

TBBle mentioned this pull request Oct 20, 2020

AWS: Network Load Balancer ignores annotations upon creation #82595

Closed

2rs2ts reviewed Oct 21, 2020

View reviewed changes

M00nF1sh reviewed Nov 6, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 6, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2020

k8s-ci-robot merged commit d2a8550 into kubernetes:master Nov 7, 2020

k8s-ci-robot added this to the v1.20 milestone Nov 7, 2020

TBBle mentioned this pull request Nov 7, 2020

AWS NLB: support for access log annotations #81584

Closed

2rs2ts mentioned this pull request Dec 10, 2020

Tag Security Group created for AWS ELB with same additional tags as ELB #58767

Merged

kishorj mentioned this pull request Feb 8, 2021

Annotation service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled not working on NLB kubernetes/cloud-provider-aws#168

Closed

byrneo mentioned this pull request Feb 23, 2021

AWS NLB: cross-zone-load-balancing not taking effect upon deployment kubernetes/ingress-nginx#6898

Closed

aSapien mentioned this pull request Feb 24, 2021

First provisioning of NLB ignores logging and cross-zone annotations kubernetes/cloud-provider-aws#180

Closed

kishorj deleted the nlb_attributes branch February 24, 2021 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reconcile AWS NLB attributes on service creation #95247

reconcile AWS NLB attributes on service creation #95247

kishorj commented Oct 2, 2020 •

edited

Loading

k8s-ci-robot commented Oct 2, 2020

k8s-ci-robot commented Oct 2, 2020

kishorj commented Oct 2, 2020

Chili-Man commented Oct 20, 2020

2rs2ts Oct 21, 2020

kishorj Oct 23, 2020

M00nF1sh left a comment

M00nF1sh commented Nov 6, 2020

k8s-ci-robot commented Nov 6, 2020

aSapien commented Feb 24, 2021

reconcile AWS NLB attributes on service creation #95247

reconcile AWS NLB attributes on service creation #95247

Conversation

kishorj commented Oct 2, 2020 • edited Loading

k8s-ci-robot commented Oct 2, 2020

k8s-ci-robot commented Oct 2, 2020

kishorj commented Oct 2, 2020

Chili-Man commented Oct 20, 2020

2rs2ts Oct 21, 2020

Choose a reason for hiding this comment

kishorj Oct 23, 2020

Choose a reason for hiding this comment

M00nF1sh left a comment

Choose a reason for hiding this comment

M00nF1sh commented Nov 6, 2020

k8s-ci-robot commented Nov 6, 2020

aSapien commented Feb 24, 2021

kishorj commented Oct 2, 2020 •

edited

Loading