Fix behaivour of aws-load-balancer-security-groups annotation by Elias481 · Pull Request #83446 · kubernetes/kubernetes

Elias481 · 2019-10-03T12:45:50Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
The intention of users utilizing the specified by aws-load-balancer-security-groups annotation is typically, that they want to assign an externally managed security to the load balancer and do not want Kubernetes to modify the group.
(Also see summary/fixed issues)

Which issue(s) this PR fixes:
Ensures that Security Group specified by aws-load-balancer-security-groups is not modified on setup of Load Balancer anymore.
Fixes #79723, kubernetes/cloud-provider-aws/issues/65
Redesign implementation from PR #62774
Also addresses #49445, #79279

Special notes for your reviewer:
If changing return signature of buildELBSecurityGroupList is an issue I would suggest to rename the function and add a wrapper with the old name.

Release Note

Fix handling of aws-load-balancer-security-groups annotation. Security-Groups assigned with this annotation are no longer modified by kubernetes which is the expected behaviour of most users. Also no unnecessary Security-Groups are created anymore if this annotation is used.

k8s-ci-robot · 2019-10-03T12:45:51Z

Welcome @Elias481!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-10-03T12:45:58Z

Hi @Elias481. 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.

Details

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.

Elias481 · 2019-10-03T12:58:39Z

/assign @micahhausler

nckturner · 2019-10-04T07:32:38Z

/ok-to-test

Elias481 · 2019-10-05T05:33:05Z

/test pull-kubernetes-e2e-gce-100-performance

redradrat · 2019-11-06T09:49:23Z

This actually addresses a quite heavy impact issue, as I understand it? It fixes the override of a loadbalancer's sg to allow incoming traffic from 0.0.0.0.

Is there anything that can be done to help move this along from the outside?

rmt · 2019-12-09T17:00:38Z

Ping? We are also affected by this issue. Who can approve this?

/sig aws

k8s-ci-robot · 2019-12-11T08:49:39Z

@rmt: The label(s) sig/aws cannot be applied, because the repository doesn't have them

Details

In response to this:

Ping? We are also affected by this issue. Who can approve this?

/sig aws

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.

M00nF1sh · 2020-02-27T02:43:29Z

/approve

k8s-ci-robot · 2020-02-27T02:43:53Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

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

M00nF1sh · 2020-02-27T05:48:08Z

/test pull-kubernetes-e2e-gce

Elias481 · 2020-02-27T07:07:15Z

OK fine, I updated the description a bit. So hopefully we can get the fixed version soon in our live enviroment.

bhks · 2020-02-27T07:27:56Z

@Elias481

We should update the release notes so that it can be added in next release.

Elias481 · 2020-02-27T08:50:05Z

@bhagwat070919 shall I do another PR for that or how? I think it's just a short note that the behaviour that was probably expected/desired is now in place.

ivan-sukhomlyn · 2020-02-27T10:15:38Z

@Elias481 @bhagwat070919 Hi guys,
After this PR merging, will Kubernetes continue to create its own Security Group in case if aws-load-balancer-security-groups defined in the annotations list with custom SG?
More details are available by the link kubernetes/cloud-provider-aws#65
Thanks

Elias481 · 2020-02-27T10:48:30Z

@ivan-sukhomlyn You are right, I forgot to mention and possibly worth to adding to description of PR und Release notes explicitly. This is of course also fixed in this turn.

ivan-sukhomlyn · 2020-02-27T11:17:56Z

Thanks for clarifying and your work 👍
@bhagwat070919 Maybe you have some ETA of Kubernetes 1.18 at the AWS EKS service?

seh · 2020-02-27T14:56:39Z

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

 	var err error
 	var securityGroupID string
+	// We do not want to make changes to a Global defined SG
+	var setupSg = false


s/setupSg/setUpSG/

bhks · 2020-02-27T19:25:40Z

@bhagwat070919 shall I do another PR for that or how? I think it's just a short note that the behaviour that was probably expected/desired is now in place.

@Elias481 you can just update this PR Release note and it would be taken when release note is being made.

bhks · 2020-02-27T19:26:29Z

@Elias481 ,

Do you mind creating Cherry-pick of this PR for k8s 1.17, 1.16, 1.15 and 1.14 ?

Elias481 · 2020-02-28T07:18:00Z

@bhagwat070919 I can do so, I think I will find some time till monday morning..

ivan-sukhomlyn · 2020-02-28T12:03:08Z

@Elias481 @bhagwat070919 It would be nice to have this fix in other K8S versions.

…e.md Annotation `service.beta.kubernetes.io/aws-load-balancer-security-groups` is missing from the list of annotations that can be applied to an ELB. This annotation was introduced in kubernetes/kubernetes#62774 and refactored kubernetes/kubernetes#83446 to allow users to specify a set of existing security groups to attach to the ELB instead of using any precreated security groups.

vampirismtrueblood · 2023-07-06T10:44:25Z

I think this issue needs to be re-opened, issue still exists with EKS v1.25,
loadBalancerSourceRanges works when creating the NLB, but updating still does not work.
Can we have this re-opened please

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 3, 2019

k8s-ci-robot requested review from micahhausler and nckturner October 3, 2019 12:47

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. 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 3, 2019

Elias481 force-pushed the fix-pr-49445 branch from abda64b to 654aed2 Compare October 3, 2019 12:57

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 3, 2019

k8s-ci-robot assigned micahhausler Oct 3, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 4, 2019

Elias481 force-pushed the fix-pr-49445 branch from 41a1153 to 5f6e5ce Compare October 4, 2019 20:19

perdasilva mentioned this pull request Dec 2, 2019

[Security] BFF/Grafana visible to the outside world awslabs/benchmark-ai#984

Closed

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

k8s-ci-robot merged commit 6b13bef into kubernetes:master Feb 27, 2020

k8s-ci-robot added this to the v1.18 milestone Feb 27, 2020

Elias481 deleted the fix-pr-49445 branch February 27, 2020 07:07

bhks mentioned this pull request Feb 27, 2020

Kubernetes Service Overwrite SecurityGroups with AWS Extra SG Annotations #85669

Closed

ivan-sukhomlyn mentioned this pull request Feb 27, 2020

Don't create an additional SecurityGroups if it's already defined by aws-load-balancer-security-groups annotation kubernetes/cloud-provider-aws#65

Closed

seh reviewed Feb 27, 2020

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 29, 2020

bhks mentioned this pull request Mar 11, 2020

AWS not updating security group for loadBalancerSourceRanges #77557

Closed

bnutt mentioned this pull request Oct 1, 2020

Add missing annotation in en/docs/concepts/services-networking/service.md kubernetes/website#24344

Merged

Conversation

Elias481 commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 3, 2019

Uh oh!

k8s-ci-robot commented Oct 3, 2019

Uh oh!

Elias481 commented Oct 3, 2019

Uh oh!

nckturner commented Oct 4, 2019

Uh oh!

Elias481 commented Oct 5, 2019

Uh oh!

redradrat commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmt commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Dec 11, 2019

Uh oh!

M00nF1sh commented Feb 27, 2020

Uh oh!

k8s-ci-robot commented Feb 27, 2020

Uh oh!

M00nF1sh commented Feb 27, 2020

Uh oh!

Elias481 commented Feb 27, 2020

Uh oh!

bhks commented Feb 27, 2020

Uh oh!

Elias481 commented Feb 27, 2020

Uh oh!

ivan-sukhomlyn commented Feb 27, 2020

Uh oh!

Elias481 commented Feb 27, 2020

Uh oh!

ivan-sukhomlyn commented Feb 27, 2020

Uh oh!

seh Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

bhks commented Feb 27, 2020

Uh oh!

bhks commented Feb 27, 2020

Uh oh!

Elias481 commented Feb 28, 2020

Uh oh!

ivan-sukhomlyn commented Feb 28, 2020

Uh oh!

vampirismtrueblood commented Jul 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Elias481 commented Oct 3, 2019 •

edited

Loading

redradrat commented Nov 6, 2019 •

edited

Loading

rmt commented Dec 9, 2019 •

edited

Loading