Fixes unnecessary creation of default SG and trying to delete non-provisioned SG by k8s system when annotation [service.beta.kubernetes.io/aws-load-balancer-security-groups] is present #84265

bhks · 2019-10-24T01:27:01Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR fixes the Issue :

AWS Cloud Provider attempts to delete LoadBalancer security group it didn’t provision
AWS Cloud Provider creates default LoadBalancer security group even if annotation [service.beta.kubernetes.io/aws-load-balancer-security-groups] is present

Which issue(s) this PR fixes:

Fixes ##84256

Special notes for your reviewer:

My first PR in Kubernetes

Does this PR introduce a user-facing change?:


Fixed the following 
-  AWS Cloud Provider attempts to delete LoadBalancer security group it didn’t provision
-  AWS Cloud Provider creates default LoadBalancer security group even if annotation [service.beta.kubernetes.io/aws-load-balancer-security-groups] is present

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


NONE

k8s-ci-robot · 2019-10-24T01:27:03Z

Welcome @bhagwat070919!

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-24T01:27:09Z

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

bhks · 2019-10-24T01:27:50Z

/sig cloud-provider

bhks · 2019-10-24T01:28:01Z

/area platform/aws

k8s-ci-robot · 2019-10-24T01:28:02Z

@bhagwat070919: The label(s) area/platform/aws cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

/area platform/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.

bhks · 2019-10-24T01:29:20Z

/assign @wongma7
/assign @micahhausler

wongma7 · 2019-10-24T01:30:00Z

/ok-to-test

bhks · 2019-10-24T03:53:12Z

/area provider/aws

bhks · 2019-10-24T16:24:09Z

/test pull-kubernetes-kubemark-e2e-gce-big

micahhausler · 2019-10-24T16:29:31Z

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

@@ -4301,6 +4301,10 @@ func (c *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName strin
 		// Delete the security group(s) for the load balancer
 		// Note that this is annoying: the load balancer disappears from the API immediately, but it is still
 		// deleting in the background.  We get a DependencyViolation until the load balancer has deleted itself
+		if securitygroups, ok := service.Annotations[ServiceAnnotationLoadBalancerSecurityGroups]; !ok {


tiny nit: can you put a newline above your new code? Just to clarify that the existing comment is not about your specific if block

Yeah , make sense Thanks.

I am putting this if block above the comments for the block to separate the block comment for deletion and why if block.

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go#L3484 you will leak security groups..

comment "This if code protects" is not english :D

@M00nF1sh Can you explain here how would we leak security group ?

From my understanding we are protecting to call a delete operation on SG which has not been provisioned by k8s.

Thanks for pointing out !!!!!!!!

Looking at the Create Code ,

We are creating a security group even if we have the annotation service.beta.kubernetes.io/aws-load-balancer-security-groups present and have no value.

I am surprised that we are validating the value but insteading of erroring out creating a default security group which dissolves the purpose https://kubernetes.io/docs/concepts/cluster-administration/cloud-providers/#load-balancers

…visioned SG by k8s system when annotation [service.beta.kubernetes.io/aws-load-balancer-security-groups] is present

micahhausler

/lgtm
/approve

bhks · 2020-01-13T21:45:41Z

/retest

micahhausler · 2020-01-13T21:51:33Z

/unhold

bhks · 2020-01-13T21:53:30Z

/retest

bhks · 2020-01-13T22:31:14Z

/test pull-kubernetes-integration

bhks · 2020-01-13T23:29:51Z

/test pull-kubernetes-e2e-gce

bhks · 2020-01-14T00:37:01Z

/test pull-kubernetes-e2e-gce

fejta-bot · 2020-01-14T04:46:02Z

/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 or /hold comment for consistent failures.

bhks · 2020-01-14T06:00:55Z

/test pull-kubernetes-e2e-gce

jsafrane · 2020-01-14T08:55:36Z

/approve

k8s-ci-robot · 2020-01-14T08:56:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bhagwat070919, 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:

~~staging/src/k8s.io/legacy-cloud-providers/aws/OWNERS~~ [jsafrane,micahhausler]

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

fejta-bot · 2020-01-14T11:25:03Z

/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 or /hold comment for consistent failures.

…-#84265-upstream-release-1.15 Automated cherry pick of #84265: Fixes unnecessary creation of default SG and trying to delete

…-#84265-upstream-release-1.17 Automated cherry pick of #84265: Fixes unnecessary creation of default SG and trying to delete

…-#84265-upstream-release-1.16 Automated cherry pick of #84265: Fixes unnecessary creation of default SG and trying to delete

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 24, 2019

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 24, 2019

k8s-ci-robot added 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 24, 2019

k8s-ci-robot requested review from jsafrane and M00nF1sh October 24, 2019 01:28

k8s-ci-robot added the area/cloudprovider label Oct 24, 2019

k8s-ci-robot assigned micahhausler and wongma7 Oct 24, 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 24, 2019

bhks force-pushed the securitygroupfix branch from a75fbfb to 12c3ce6 Compare October 24, 2019 01:43

k8s-ci-robot added the area/provider/aws Issues or PRs related to aws provider label Oct 24, 2019

bhks force-pushed the securitygroupfix branch from 12c3ce6 to 03bbd74 Compare October 24, 2019 16:27

micahhausler reviewed Oct 24, 2019

View reviewed changes

bhks force-pushed the securitygroupfix branch from 03bbd74 to f1e336d Compare October 24, 2019 16:39

Fixes unnecessary creation of default SG and trying to delete non-pro…

1ee8632

…visioned SG by k8s system when annotation [service.beta.kubernetes.io/aws-load-balancer-security-groups] is present

bhks force-pushed the securitygroupfix branch from bd2bd7c to 1ee8632 Compare January 13, 2020 20:56

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

bhks requested a review from micahhausler January 13, 2020 21:28

micahhausler approved these changes Jan 13, 2020

View reviewed changes

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 13, 2020

k8s-ci-robot merged commit c9003a2 into kubernetes:master Jan 14, 2020

k8s-ci-robot added this to the v1.18 milestone Jan 14, 2020

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 Jan 15, 2020

k8s-ci-robot added a commit that referenced this pull request Jan 31, 2020

Merge pull request #87208 from bhagwat070919/automated-cherry-pick-of…

7341843

…-#84265-upstream-release-1.15 Automated cherry pick of #84265: Fixes unnecessary creation of default SG and trying to delete

k8s-ci-robot added a commit that referenced this pull request Jan 31, 2020

Merge pull request #87206 from bhagwat070919/automated-cherry-pick-of…

3fb3c1e

…-#84265-upstream-release-1.17 Automated cherry pick of #84265: Fixes unnecessary creation of default SG and trying to delete

k8s-ci-robot added a commit that referenced this pull request Jan 31, 2020

Merge pull request #87207 from bhagwat070919/automated-cherry-pick-of…

2cd96ff

…-#84265-upstream-release-1.16 Automated cherry pick of #84265: Fixes unnecessary creation of default SG and trying to delete

Elias481 mentioned this pull request Feb 29, 2020

fix behaviour of aws-load-balancer-security-groups annotation #88693

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes unnecessary creation of default SG and trying to delete non-provisioned SG by k8s system when annotation [service.beta.kubernetes.io/aws-load-balancer-security-groups] is present #84265

Fixes unnecessary creation of default SG and trying to delete non-provisioned SG by k8s system when annotation [service.beta.kubernetes.io/aws-load-balancer-security-groups] is present #84265

bhks commented Oct 24, 2019 •

edited

Loading

k8s-ci-robot commented Oct 24, 2019

k8s-ci-robot commented Oct 24, 2019

bhks commented Oct 24, 2019

bhks commented Oct 24, 2019

k8s-ci-robot commented Oct 24, 2019

bhks commented Oct 24, 2019

wongma7 commented Oct 24, 2019

bhks commented Oct 24, 2019

bhks commented Oct 24, 2019

micahhausler Oct 24, 2019

bhks Oct 24, 2019

M00nF1sh Oct 24, 2019

bhks Oct 24, 2019

bhks Oct 24, 2019

micahhausler left a comment

bhks commented Jan 13, 2020

micahhausler commented Jan 13, 2020

bhks commented Jan 13, 2020

bhks commented Jan 13, 2020

bhks commented Jan 13, 2020

bhks commented Jan 14, 2020

fejta-bot commented Jan 14, 2020

bhks commented Jan 14, 2020

jsafrane commented Jan 14, 2020

k8s-ci-robot commented Jan 14, 2020

fejta-bot commented Jan 14, 2020

Fixes unnecessary creation of default SG and trying to delete non-provisioned SG by k8s system when annotation [service.beta.kubernetes.io/aws-load-balancer-security-groups] is present #84265

Fixes unnecessary creation of default SG and trying to delete non-provisioned SG by k8s system when annotation [service.beta.kubernetes.io/aws-load-balancer-security-groups] is present #84265

Conversation

bhks commented Oct 24, 2019 • edited Loading

k8s-ci-robot commented Oct 24, 2019

k8s-ci-robot commented Oct 24, 2019

bhks commented Oct 24, 2019

bhks commented Oct 24, 2019

k8s-ci-robot commented Oct 24, 2019

bhks commented Oct 24, 2019

wongma7 commented Oct 24, 2019

bhks commented Oct 24, 2019

bhks commented Oct 24, 2019

micahhausler Oct 24, 2019

Choose a reason for hiding this comment

bhks Oct 24, 2019

Choose a reason for hiding this comment

M00nF1sh Oct 24, 2019

Choose a reason for hiding this comment

bhks Oct 24, 2019

Choose a reason for hiding this comment

bhks Oct 24, 2019

Choose a reason for hiding this comment

micahhausler left a comment

Choose a reason for hiding this comment

bhks commented Jan 13, 2020

micahhausler commented Jan 13, 2020

bhks commented Jan 13, 2020

bhks commented Jan 13, 2020

bhks commented Jan 13, 2020

bhks commented Jan 14, 2020

fejta-bot commented Jan 14, 2020

bhks commented Jan 14, 2020

jsafrane commented Jan 14, 2020

k8s-ci-robot commented Jan 14, 2020

fejta-bot commented Jan 14, 2020

bhks commented Oct 24, 2019 •

edited

Loading