Fix leak managed/owned security group on Service update with BYO SG

[mtulio]

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR propose fix on leaked security group (SG) when a Service type-loadBalancer (CLB) is updated adding the BYO SG annotation (service.beta.kubernetes.io/aws-load-balancer-security-groups), which replaces all SG added to the Load Balancer without removing linked rules, as well not deleting managed SG (created by controller).

Which issue(s) this PR fixes:

Fixes #1208

Special notes for your reviewer:

We decided of creating isolated dedicated methods to discover and remove linked rule's SG targeting to:

• enhance code maintenance
• enhance unit tests
• allow to reuse the logic when NLB with SG is supported (future)

The unit tests and documentation(function) comments have been assisted by Cursor AI(model claude-4-sonet): AIA HAb SeCeNc Hin R v1.0

Does this PR introduce a user-facing change?:

Fixed security group leak when updating Classic Load Balancer (CLB) services with `service.beta.kubernetes.io/aws-load-balancer-security-groups` annotation. Controller-managed security groups are now properly cleaned up when switching CLB from managed security groups to user-specified security groups.

[k8s-ci-robot]

This issue is currently awaiting triage.

If cloud-provider-aws contributors determine 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.

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

[k8s-ci-robot]

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

[elmiko]

/ok-to-test

[mtulio]

/test all

[mtulio]

Fixing doc strings and failed unit tests from previous unexpected behavior:

/test all

[k8s-ci-robot]

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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

[mtulio]

/test pull-cloud-provider-aws-e2e-kubetest2

[mtulio]

/test all

[mtulio]

I can't find connection between failures in pull-cloud-provider-aws-e2e-kubetest2 and existing changes.

I am going to convert to regular PR to ask for reviewers while we observe if this isnt a CI flake.

PTAL?
/assign @kmala @elmiko @JoelSpeed

[mtulio]

FWIW interim update, this PR is still alive and need to be fixed, and proposal could be used in the logic of BYOSG in NLBs. I am planning to return on it next week to rebase and ask for final review with recent updates in the Service NLB and e2e.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from elmiko. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[mtulio]

PR rebased including/merging managed NLB SG, ensuring readiness with new e2e tests validating the fix:

/test pull-cloud-provider-aws-e2e

[mtulio]

Fixed conflicts
/test pull-cloud-provider-aws-e2e

[mtulio]

small fixes in the e2e for PSP and logging:

/test pull-cloud-provider-aws-e2e

[mtulio]

e2e is now passing, I am going to polish the e2e. Let’s see other overall:

/test all

[mtulio]

Code Review - Claude Assistant (claude-sonnet-4.5@20250929)

---

Summary

This PR addresses security group leakage (#1208) when updating Classic Load Balancer services from controller-managed to user-specified security groups via the service.beta.kubernetes.io/aws-load-balancer-security-groups annotation.

Core Change: Introduces lifecycle management for controller-owned security groups during load balancer updates, ensuring proper cleanup when security groups are replaced.

---

Problem Analysis

Current Behavior:
When the BYO security group annotation is added to an existing Service, the controller applies new security groups but leaves orphaned managed security groups in AWS.

Impact:

• Resource leaks consuming AWS quotas
• Potential security exposure from unmanaged ingress rules
• Manual cleanup burden on operators

Root Cause:
The ensureLoadBalancer function in aws_loadbalancer.go replaces security groups via ApplySecurityGroupsToLoadBalancer but lacks cleanup logic for the replaced security groups.

---

Solution Design

New Functions

1. buildSecurityGroupRuleReferences()

Purpose: Identifies security groups with ingress rules referencing a target security group.

Returns:

• Map of all referencing security groups → cluster tag status
• Map of cluster-tagged security groups → their relevant IP permissions

Usage: Called before security group deletion to identify and revoke dependencies.

Code Reuse: Replaces duplicated logic in updateInstanceSecurityGroupsForLoadBalancer (aws.go:2940).

2. removeOwnedSecurityGroups()

Purpose: Safely removes controller-owned security groups after validation and dependency cleanup.

Algorithm:

1. Validate ownership via cluster tags (kubernetes.io/cluster/<name>: owned)
2. Query dependent security groups
3. Revoke ingress rules from cluster-tagged dependents
4. Delete owned security groups via deleteSecurityGroupsWithBackoff

Error Handling: Collects all errors during processing, continues with remaining security groups.

Integration Point

Modified ensureLoadBalancer() to call removeOwnedSecurityGroups() after ApplySecurityGroupsToLoadBalancer successfully updates security groups.

---

Code Quality

Strengths

1. Separation of Concerns: Clean functional decomposition with single-responsibility functions
2. Error Handling: Proper error wrapping using %w for error chain preservation
3. Defensive Programming: Validates ownership and cluster tags before destructive operations
4. Test Coverage: 428 lines of unit tests covering owned/non-owned scenarios, errors, and edge cases
5. E2E Testing: Comprehensive scenario testing with resource cleanup validation
6. Documentation: Clear GoDoc with parameter and return value descriptions

Code Review Findings

Minor Improvements Implemented

1. Error Wrapping Consistency (aws_loadbalancer.go:1906)

   • Changed %q to %w for proper error unwrapping support
   • Enables use of errors.Is() and errors.As() for error inspection

2. Logging Enhancement (aws_loadbalancer.go:1992)

   • Added deletion count to log message: "deleted %d loadbalancer owned security groups"
   • Improves debugging and operational visibility

Retry Logic Analysis

Question: Should retry logic be added for RevokeSecurityGroupIngress?

Answer: No. The AWS SDK v2 for Go includes automatic retry logic with exponential backoff for:

• Throttling errors (429)
• Transient network errors
• 5xx server errors
• Timeout errors

Evidence: EC2 client initialized via ec2.NewFromConfig(cfg) (aws_sdk.go) inherits default retry configuration from the SDK, which implements the standard retry strategy.

Current Behavior: If RevokeSecurityGroupIngress fails due to transient errors, the SDK retries automatically. If it permanently fails, the error is collected but doesn't block security group deletion. The deleteSecurityGroupsWithBackoff function handles dependency violations during deletion with its own retry mechanism (10-minute timeout, 5-second intervals).

Conclusion: Adding manual retry logic would be redundant and potentially interfere with SDK behavior.

---

Testing Strategy

Unit Tests (428 lines)

TestCloud_removeOwnedSecurityGroups:

• Successfully remove owned security groups
• Skip non-owned security groups
• Handle ownership validation errors
• Handle empty security group lists

TestCloud_buildSecurityGroupRuleReferences:

• Successfully build references with cluster-tagged groups
• Handle non-cluster-tagged groups
• Handle API errors
• Handle no results

E2E Tests (860 net lines)

New Test Scenario: "CLB with managed Security Group must update to BYO Security Group"

Test Flow:

1. Create Service with default (managed) security groups
2. Update Service annotation with BYO security group
3. Verify BYO security group is applied to load balancer
4. Verify managed security groups are deleted from AWS
5. Cleanup test resources

Test Infrastructure: New aws_helper.go provides utilities for:

• Cluster tag discovery
• Security group management
• Load balancer inspection
• Rule verification

---

Logging Strategy

Production Code:

• klog.V(2).Infof() for internal operations (revoke rules, deletion steps)
• klog.Warningf() for operational decisions (skipping non-owned SGs)

Rationale: Follows Kubernetes logging conventions where V(2) is the recommended minimum for troubleshooting. Operators run with --v=1 by default for clean logs, increasing to --v=2 when debugging security group issues.

E2E Tests:

• Reduced from ~100 to ~71 log statements (29% reduction)
• Retained: phase transitions, errors, important state verification
• Removed: routine operation confirmations, iteration details

---

Security Considerations

1. Ownership Validation: Checks cluster tag before deletion (kubernetes.io/cluster/<name>: owned)
2. Dependency Resolution: Revokes ingress rules before deletion to prevent dependency violations
3. Selective Deletion: Only deletes controller-owned resources, warns about user-managed resources
4. Error Boundaries: Failures in one security group don't block processing of others

---

Potential Concerns

1. Race Conditions

Scenario: Security group deleted between buildSecurityGroupRuleReferences and RevokeSecurityGroupIngress calls.

Mitigation: Errors are collected but don't fail the operation. The deleteSecurityGroupsWithBackoff retry logic handles timing issues.

2. Partial Failures

Scenario: Some security groups fail to delete due to permissions or AWS limits.

Behavior: Errors are returned to caller, logged, but Service update succeeds. Security groups remain for manual investigation.

Alternative Consideration: Could return error to retry entire operation, but current design prioritizes Service availability over cleanup completeness.

---

Recommendations

Approve with Observations

Recommendation: Approve and merge

Confidence: High - code quality is strong, testing is comprehensive, and the approach is sound.

Observations for Future Work:

1. Metrics: Consider adding Prometheus metrics for security group cleanup operations (success/failure counts, duration)

2. Documentation: Add troubleshooting section to docs for security group cleanup failures

3. Async Cleanup: For production systems with many security groups, consider whether cleanup should be asynchronous to avoid blocking Service updates

---

Technical Verification

• Code compiles successfully
• Unit tests added with good coverage
• E2E tests validate full scenario
• No breaking API changes
• Error handling follows Go best practices
• Logging follows Kubernetes conventions
• AWS SDK best practices followed (automatic retries, error inspection)

---

Reviewed by: Claude Code Assistant (claude-sonnet-4.5@20250929)
Date: 2025-12-23
Recommendation: APPROVE

[mtulio]

initial investigation is pointing to flake test, checking:

/test pull-cloud-provider-aws-test

[mtulio]

Introduced test is passing:

[cloud-provider-aws-e2e] loadbalancer CLB with managed Security Group must update to BYO Security Group

Failed tests, both hairpinning traffic, are failing to resolve DNS:

DNS resolution failure - check if target hostname is resolvable

Checking if this would be transient ('cloudability' issues) or related to e2e updates (which is mostly debug and new e2e).

[mtulio]

/test pull-cloud-provider-aws-e2e

[mtulio]

The last attempt only the NLB test has failed for same reason as before (timeout):

[It] [cloud-provider-aws-e2e] loadbalancer NLB internal should be reachable with hairpinning traffic

I wonder if I need to consider increasing the timeout, although I think it is already high.

/test pull-cloud-provider-aws-e2e

[mtulio]

@mtulio: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cloud-provider-aws-e2e 63e7396 link true /test pull-cloud-provider-aws-e2e
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

The hairpinning trafic test(s) keeps flaking due timeout (to have a LB/resolve it's name). I had a good sync with @elmiko today, one approach would be increasing timeout of hairpinning traffic tests. I am also considering isolating the e2e improvements added on this PR to a dedicated one, so we can focus here in the fix part of things, while we investigate /isolate the e2e improvements. Open for thoughts.

[mtulio]

e2e timeout increased on loadbalancer curller/pooler to validate if CI issues are related. Expected to decrease the flake between hairpin traffic tests (CLB and NLB)

[mtulio]

/test pull-cloud-provider-aws-e2e

[k8s-ci-robot]

@mtulio: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cloud-provider-aws-test	3e497b2	link	true	/test pull-cloud-provider-aws-test
pull-cloud-provider-aws-e2e	3e497b2	link	true	/test pull-cloud-provider-aws-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.