Skip to content

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

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.

Sign up for GitHub

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

Merged
oilbeater merged 2 commits into from
Feb 26, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
053eafb
fix(e2e): fix SLR e2e flaky failure caused by VIP not being cleaned up
oilbeater Feb 26, 2026
b1ba42a
fix(slr): handle IsNotFound when deleting VIP CRD in handleDelSwitchL…
oilbeater Feb 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion pkg/controller/switch_lb_rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,13 @@ func (c *Controller) handleDelSwitchLBRule(info *SlrInfo) error {
)

name = generateSvcName(info.Name)
// Read the subnet annotation before deleting the service, so we can use it as a
// fallback to clean up the health-check VIP when no LBHC is found (e.g. the LBHC
// was already removed because the service was deleted before the SLR).
subnetForVip := ""
if svc, e := c.servicesLister.Services(info.Namespace).Get(name); e == nil {
subnetForVip = svc.Annotations[util.LogicalSwitchAnnotation]
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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.

}
if err = c.config.KubeClient.CoreV1().Services(info.Namespace).Delete(context.Background(), name, metav1.DeleteOptions{}); err != nil {
if !k8serrors.IsNotFound(err) {
klog.Errorf("failed to delete service %s, err: %v", name, err)
Expand Down Expand Up @@ -261,6 +268,14 @@ func (c *Controller) handleDelSwitchLBRule(info *SlrInfo) error {
}
}

// Fallback: if no VIP was discovered via LBHC (e.g. LBHC was already deleted
// because the backing service was removed before the SLR), use the subnet that
// was read from the service annotation before deletion.
if len(vips) == 0 && subnetForVip != "" {
klog.Infof("handleDelSwitchLBRule %s: no LBHC found for vips %v, falling back to subnet %s from service annotation", info.Name, info.Vips, subnetForVip)
vips[subnetForVip] = struct{}{}
}

if err = c.OVNNbClient.DeleteLoadBalancerHealthChecks(
func(lbhc *ovnnb.LoadBalancerHealthCheck) bool {
return slices.Contains(info.Vips, lbhc.Vip)
Expand All @@ -282,7 +297,7 @@ func (c *Controller) handleDelSwitchLBRule(info *SlrInfo) error {

if len(lbhcs) == 0 {
err = c.config.KubeOvnClient.KubeovnV1().Vips().Delete(context.Background(), vip, metav1.DeleteOptions{})
if err != nil {
if err != nil && !k8serrors.IsNotFound(err) {
klog.Errorf("failed to delete vip %s for load balancer health check, err: %v", vip, err)
return err
}
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/kube-ovn/switch_lb_rule/switch_lb_rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,12 @@ var _ = framework.Describe("[group:slr]", func() {
podClient.DeleteSync(clientPodName)
ginkgo.By("Deleting statefulset " + stsName)
stsClient.DeleteSync(stsName)
ginkgo.By("Deleting service " + stsSvcName)
serviceClient.DeleteSync(stsSvcName)
ginkgo.By("Deleting switch-lb-rule " + selSlrName)
switchLBRuleClient.DeleteSync(selSlrName)
ginkgo.By("Deleting switch-lb-rule " + epSlrName)
switchLBRuleClient.DeleteSync(epSlrName)
ginkgo.By("Deleting service " + stsSvcName)
serviceClient.DeleteSync(stsSvcName)
ginkgo.By("Deleting subnet " + subnetName)
subnetClient.DeleteSync(subnetName)
ginkgo.By("Deleting vpc " + vpcName)
Expand Down
Loading