Ensure removal of security group rules on deleting load balancers

[JoelSpeed]

What type of PR is this?
/kind bug

What this PR does / why we need it:

This PR updates the load balancer security group update logic for ELBs so that we can actually delete a security group when there is an untagged group present.

In the current flow, we can add rules to the security group if it is untagged, but we cannot remove them since the logic excludes untagged groups from the actualGroups list. When we are deleting the load balancer and deleting the security group, we need to make sure that we remove all rules that refer to the security group else the security group deletion will reach a dependency violation deadlock.

To ensure compatibility with BYO security groups, the code ensures that we only pass the isDeleting parameter as true when the existing logic determines that the load balancer should be removing the security group already. This should mean that we only do the full removal of all references when we are about to delete the security group, and if the security group is being left over, we won't remove any references - I don't think in the BYO security group case we have any way to track what we have added so I can't fix that bug here.

Which issue(s) this PR fixes:

Fixes #566

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

When removing a load balancer, the service controller will now remove all security group rules referencing the load balancer's security group, even when the security group containing the rule is unmanaged.

[cartermckinnon]

We're recommending that folks run the separate load balancer controller: https://github.com/kubernetes-sigs/aws-load-balancer-controller

Instead of using the legacy service controller here. Do you see this issue with that controller as well?

[JoelSpeed]

As far as I know, this bug only exists in the classic load balancer code path, which has no parallel in the load balancer controller. Until recently, NLBs didn't even support security groups right?

While I appreciate that there's a new way to do things and you can recommend moving people across, the CLB has no equivalent support and as such, still needs to be supported here, and this is a fairly major bug that requires users to manually enter the AWS console/CLI to remove resources to be able to clean up their environment. Else they are leaked.

Note as well that the controllers logic today will happily remove the load balancer and service object, leaking the resources and not even giving the user any indication that resources were leaked, I think this is pretty bad behaviour for a controller like this.

[cartermckinnon]

/assign @M00nF1sh @kishorj

can y'all take a look at this?

[cartermckinnon]

/retest

CI should be fixed now

[JoelSpeed]

Any of the maintainers able to give this bug a review please?

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[JoelSpeed]

Any indication that this might be an acceptable patch? The bug is still present

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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-sigs/prow repository.

[JoelSpeed]

/reopen

Any AWS maintainers able to review this bug?

[k8s-ci-robot]

@JoelSpeed: Reopened this PR.

In response to this:

/reopen

Any AWS maintainers able to review this bug?

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-sigs/prow repository.

[kmala]

/triage accepted

[kmala]

/remove-lifecycle rotten

[kmala]

changes mostly looks good, can we do a rebase?

[JoelSpeed]

The E2E failures here don't look to be related as far as I can tell, @kmala do you agree or do I need to look deeper?

[kmala]

The E2E failures here don't look to be related as far as I can tell, @kmala do you agree or do I need to look deeper?

they are happening for other PR's also #1016 (comment) . Looking at the reason for the issue.

[kmala]

@JoelSpeed can you rebase such that the e2e works as its been fixed

[JoelSpeed]

I think Prow is supposed to handle that for you, but I've rebased anyway

[kmala]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kmala

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:

• OWNERS [kmala]

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

[jimmidyson]

@JoelSpeed Thanks for this fix! Can we get this backported to release-1.31 and release-1.30 release branches please? 🙏