Skip to content
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

Ensure ownership when deleting a load balancer security group #74311

Merged
merged 3 commits into from Mar 15, 2019

Conversation

@hpedrorodrigues
Copy link
Contributor

commented Feb 20, 2019

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR fixes the behavior of a load balancer deletion.
When deleting a load balancer, there is no check for ownership.
Given this, when attaching an extra security group to a load balancer with the annotation service.beta.kubernetes.io/aws-load-balancer-extra-security-groups, the security group is deleted along with it. Even if there are no cluster tags in the security group.

With this fix, when there is a security group attached to a load balancer that has no cluster tags, after the load balancer deletion the security group should be kept alive.

Which issue(s) this PR fixes:

Fixes #62204
Fixes #62489

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Ensure ownership when deleting a load balancer security group
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

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

@skull0801

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

/assign @jsafrane

@yagonobre
Copy link
Member

left a comment

Thanks @hpedrorodrigues
/ok-to-test

@christopherhein

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

/assign @micahhausler
/cc @M00nF1sh

@M00nF1sh

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

/lgtm

@micahhausler
Copy link
Member

left a comment

LGTM, do you mind fixing the few nits?

Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated

@k8s-ci-robot k8s-ci-robot added size/M and removed lgtm size/S labels Mar 7, 2019

Check for ownership when deleting a load balancer security group
Co-authored-by: Marcus Fonseca <marcus.080196@gmail.com>

hpedrorodrigues added some commits Mar 7, 2019

@skull0801

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

LGTM, do you mind fixing the few nits?

Nits fixed!

@skull0801

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

@micahhausler, fixed! Also, is it possible to add this to the milestone v1.14?

@dims

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

/sig aws
/priority important-soon

/assign @mcrute

@dims

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

/milestone v1.14

@k8s-ci-robot k8s-ci-robot added this to the v1.14 milestone Mar 13, 2019

@vllry

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I don't have the permission, but LGTM.

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 14, 2019

@micahhausler
Copy link
Member

left a comment

/LGTM
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-ci-robot k8s-ci-robot merged commit d5a3db0 into kubernetes:master Mar 15, 2019

17 checks passed

cla/linuxfoundation hpedrorodrigues authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@hpedrorodrigues hpedrorodrigues deleted the hpedrorodrigues:fix-lb-sg-deletion branch Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.