fix: fix SLR e2e flaky failure caused by VIP not being cleaned up

[oilbeater]

Root Cause

The SLR e2e test intermittently fails with subnet stuck in Terminating state (v4usingIPs: 1). The root cause is a race condition between the stsSvc deletion and the SLR deletion:

1. In the e2e AfterEach, stsSvc was deleted before SLRs. This triggers the endpoint_slice controller to call LoadBalancerDeleteVip, which removes the LBHC from lb.HealthCheck.
2. OVSDB's garbage collection then deletes the orphaned LBHC object and asynchronously propagates the change to the client cache.
3. When handleDelSwitchLBRule later runs and queries ListLoadBalancerHealthChecks (via in-memory WhereCache), the LBHC is already gone from cache.
4. As a result, the vips map is empty → the health-check VIP CRD is never deleted → the VIP holds an IP in the subnet → v4usingIPs = 1 → the subnet finalizer is never removed → subnet stuck in Terminating.

This race was confirmed by controller logs: the clean health check call (removing LBHC from LB) happens 12ms before the SLR deletion is even enqueued.

Fixes

1. e2e test: fix AfterEach cleanup order (test/e2e/kube-ovn/switch_lb_rule/switch_lb_rule.go)

Delete SLRs before stsSvc to ensure handleDelSwitchLBRule runs while the LBHC is still in the OVN NB DB.

2. Controller: add fallback VIP cleanup (pkg/controller/switch_lb_rule.go)

Before deleting the Service, read its ovn.kubernetes.io/logical_switch annotation. If handleDelSwitchLBRule finds no LBHC in cache (i.e. LBHC was already GC'd), use the saved subnet name as a fallback to identify and delete the health-check VIP CRD. This makes the cleanup robust even if the race occurs.

3. Controller: handle IsNotFound on VIP deletion (pkg/controller/switch_lb_rule.go)

When two SLRs share the same subnet and both fall through to the fallback path, both will attempt to delete the same VIP CRD. The second attempt correctly receives IsNotFound which should be ignored rather than treated as an error.

Test plan

• Existing SLR e2e tests pass without flakiness
• Verify subnet is fully cleaned up after SLR e2e AfterEach

🤖 Generated with Claude Code

[gemini-code-assist]

Summary of Changes

Hello @oilbeater, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a flaky e2e test failure related to Switch Load Balancer Rules (SLR) where subnets would get stuck in a Terminating state. The core issue was a race condition during cleanup, leading to health-check VIPs not being properly deleted. The changes introduce a more robust cleanup order in e2e tests and implement a fallback mechanism in the controller to ensure VIPs are always cleaned up, along with improved error handling for VIP deletion.

Highlights

• e2e Test Cleanup Order: The e2e test cleanup order was adjusted to delete Switch Load Balancer Rules (SLRs) before the associated service, preventing a race condition where the service's health check might be removed prematurely.
• Controller Fallback VIP Cleanup: A fallback mechanism was implemented in the controller to ensure health-check VIP CRDs are cleaned up even if the Load Balancer Health Check (LBHC) is already gone due to a race condition. This is achieved by pre-reading the subnet annotation from the service.
• Robust VIP Deletion: The controller's VIP deletion logic was enhanced to gracefully handle IsNotFound errors, preventing failures when multiple SLRs attempt to delete the same VIP CRD.

Changelog

• pkg/controller/switch_lb_rule.go
  • Added logic to retrieve and store the logical switch annotation from the service before its deletion, to be used as a fallback for VIP cleanup.
  • Implemented a fallback mechanism to identify and delete health-check VIPs using the stored subnet annotation if no Load Balancer Health Checks are found.
  • Modified the VIP deletion process to ignore IsNotFound errors, making it resilient to concurrent deletion attempts.
• test/e2e/kube-ovn/switch_lb_rule/switch_lb_rule.go
  • Reordered the cleanup steps in the AfterEach block to ensure Switch Load Balancer Rules are deleted before the associated service.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses a flaky e2e test failure caused by a race condition during resource cleanup by correcting the deletion order in the e2e test's AfterEach block and making the handleDelSwitchLBRule controller logic more robust with a fallback mechanism. However, a critical vulnerability has been identified: the fallback logic trusts the ovn.kubernetes.io/logical_switch annotation on the backing service, which can be manipulated by users with access to the namespace. This allows an attacker to cause the deletion of arbitrary cluster-scoped Vip resources by modifying the service annotation before the SwitchLBRule is deleted. Additionally, enhancing logging for better observability in case of unexpected errors is suggested.

[gemini-code-assist]

The fallback logic for identifying the VIP to delete relies on the ovn.kubernetes.io/logical_switch annotation of the service. Since this service is located in a user-controlled namespace, an attacker with edit permissions in that namespace could modify this annotation to point to an arbitrary Vip resource name. When the SwitchLBRule is subsequently deleted, the controller will delete the specified Vip resource without verifying its ownership or type, leading to unauthorized deletion of cluster-scoped resources (e.g., VM VIPs). To remediate this, the controller should verify that the Vip resource being deleted is indeed a health-check VIP associated with the current SwitchLBRule, for example by checking its labels or owner references. Additionally, while the current logic correctly handles the case where the service is not found, it silently ignores other potential errors from c.servicesLister.Services().Get(). It would be beneficial for debugging to log a warning if an unexpected error occurs, providing better visibility into potential issues.