Fix IP Port Mapping overwritten by new LBs in the same VPC

pkg/controller/endpoint_slice.go

@@ -249,11 +249,11 @@ func (c *Controller) handleUpdateEndpointSlice(key string) error {
 				}
 
 				if c.config.EnableOVNLBPreferLocal {
-					if err := c.OVNNbClient.LoadBalancerDeleteIPPortMapping(lb, vip); err != nil {
+					if err := c.OVNNbClient.LoadBalancerDeleteVipIPPortMapping(lb, vip); err != nil {
 						klog.Errorf("failed to delete ip port mapping for vip %s from LB %s: %v", vip, lb, err)
 						return err
 					}
-					if err := c.OVNNbClient.LoadBalancerDeleteIPPortMapping(oldLb, vip); err != nil {
+					if err := c.OVNNbClient.LoadBalancerDeleteVipIPPortMapping(oldLb, vip); err != nil {
 						klog.Errorf("failed to delete ip port mapping for vip %s from LB %s: %v", vip, lb, err)
 						return err
 					}

pkg/controller/gc.go

@@ -3,6 +3,7 @@
 import (
 	"context"
 	"fmt"
+	"net"
 	"slices"
 	"strings"
 	"unicode"
@@ -50,6 +51,7 @@
 		c.gcVip,
 		c.gcLbSvcPods,
 		c.gcVPCDNS,
+		c.gcIpPortMapping,
 	}
 	for _, gcFunc := range gcFunctions {
 		if err := gcFunc(); err != nil {
@@ -1189,6 +1191,46 @@
 	return nil
 }
 
+// gcIpPortMapping handles cleaning up ip_port_mappings created by SwitchLBRules/EndpointSlices
+// For every LB present in OVN, we make sure the ip_port_mappings are not stale (they're used by a VIP)
+func (c *Controller) gcIpPortMapping() error {
+	klog.Infof("start to gc ovn load balancers ip_port_mappings")
+	lbs, err := c.OVNNbClient.ListLoadBalancers(func(lb *ovnnb.LoadBalancer) bool {
+		return lb.ExternalIDs["vendor"] == util.CniTypeName
+	})
+	if err != nil {
+		klog.Errorf("failed to list load balancers: %v", err)
+		return err
+	}
+
+	// Run GC on every loadbalancer within OVN
+	for _, lb := range lbs {
+		backendIPs := set.New[string]()
+
+		// Collect every backend IP associated with every VIP in that loadbalancer
+		for _, backends := range lb.Vips {
+			for backend := range strings.SplitSeq(backends, ",") {
+				if ip, _, err := net.SplitHostPort(backend); err == nil {
+					backendIPs.Insert(ip)
+				}
+			}
+		}
+
+		// If a backend is present in the ip_port_mapping but not associated with any VIP, delete the entry
+		for ip := range lb.IPPortMappings {
+			if !backendIPs.Has(ip) {
+				klog.Infof("gc stale ip_port_mapping entry %s in load balancer %s", ip, lb.Name)
+				if err := c.OVNNbClient.LoadBalancerDeleteIPPortMapping(lb.Name, ip); err != nil {
+					klog.Errorf("failed to delete stale ip_port_mapping entry %s from load balancer %s: %v", ip, lb.Name, err)
+					return err
+				}
+			}
+		}
+	}
+
+	return nil
+}
+
 func (c *Controller) gcVPCDNS() error {
 	if !c.config.EnableLb {
 		return nil

pkg/controller/gc_test.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/scylladb/go-set/strset"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 
 	"github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb"
 	"github.com/kubeovn/kube-ovn/pkg/util"
@@ -49,3 +50,97 @@ func Test_logicalRouterPortFilter(t *testing.T) {
 		}
 	}
 }
+
+func Test_gcOvnLb(t *testing.T) {
+	t.Parallel()
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	fakeCtrl, err := newFakeControllerWithOptions(t, nil)
+	if err != nil {
+		t.Fatalf("failed to create fake controller: %v", err)
+	}
+
+	t.Run("cleanup stale ip_port_mappings", func(t *testing.T) {
+		lb1 := &ovnnb.LoadBalancer{
+			Name: "lb1",
+			ExternalIDs: map[string]string{
+				"vendor": util.CniTypeName,
+			},
+			Vips: map[string]string{
+				"10.96.0.1:80": "192.168.1.1:80,192.168.1.2:80",
+			},
+			IPPortMappings: map[string]string{
+				"192.168.1.1": "node1", // active
+				"192.168.1.2": "node2", // active
+				"192.168.1.3": "node3", // stale
+			},
+		}
+
+		lb2 := &ovnnb.LoadBalancer{
+			Name: "lb2",
+			ExternalIDs: map[string]string{
+				"vendor": util.CniTypeName,
+			},
+			Vips: map[string]string{
+				"10.96.0.2:443": "192.168.2.10:443",
+			},
+			IPPortMappings: map[string]string{
+				"192.168.2.1": "node1", // stale
+				"192.168.2.2": "node2", // stale
+			},
+		}
+
+		// IPv6 test case
+		lb3 := &ovnnb.LoadBalancer{
+			Name: "lb3",
+			ExternalIDs: map[string]string{
+				"vendor": util.CniTypeName,
+			},
+			Vips: map[string]string{
+				"[fd00::1]:80": "[fd00::101]:80",
+			},
+			IPPortMappings: map[string]string{
+				"fd00::101": "node1", // active
+				"fd00::102": "node2", // stale
+			},
+		}
+
+		fakeCtrl.mockOvnClient.EXPECT().ListLoadBalancers(gomock.Any()).Return([]ovnnb.LoadBalancer{*lb1, *lb2, *lb3}, nil)
+
+		// Expect deletions for stale entries
+		fakeCtrl.mockOvnClient.EXPECT().LoadBalancerDeleteIPPortMapping("lb1", "192.168.1.3").Return(nil)
+		fakeCtrl.mockOvnClient.EXPECT().LoadBalancerDeleteIPPortMapping("lb2", "192.168.2.1").Return(nil)
+		fakeCtrl.mockOvnClient.EXPECT().LoadBalancerDeleteIPPortMapping("lb2", "192.168.2.2").Return(nil)
+		fakeCtrl.mockOvnClient.EXPECT().LoadBalancerDeleteIPPortMapping("lb3", "fd00::102").Return(nil)
+
+		err := fakeCtrl.fakeController.gcIpPortMapping()
+		if err != nil {
+			t.Errorf("gcIpPortMapping() error = %v", err)
+		}
+	})
+
+	t.Run("no stale mappings", func(t *testing.T) {
+		lb := &ovnnb.LoadBalancer{
+			Name: "lb-clean",
+			ExternalIDs: map[string]string{
+				"vendor": util.CniTypeName,
+			},
+			Vips: map[string]string{
+				"10.96.0.1:80": "192.168.1.1:80",
+			},
+			IPPortMappings: map[string]string{
+				"192.168.1.1": "node1",
+			},
+		}
+
+		fakeCtrl.mockOvnClient.EXPECT().ListLoadBalancers(gomock.Any()).Return([]ovnnb.LoadBalancer{*lb}, nil)
+		// No LoadBalancerDeleteIPPortMapping expected
+
+		err := fakeCtrl.fakeController.gcIpPortMapping()
+		if err != nil {
+			t.Errorf("gcIpPortMapping() error = %v", err)
+		}
+	})
+}

pkg/ovs/interface.go

@@ -133,7 +133,8 @@ type LoadBalancer interface {
 	LoadBalancerDeleteVip(lbName, vip string, ignoreHealthCheck bool) error
 	LoadBalancerAddIPPortMapping(lbName, vip string, ipPortMappings map[string]string) error
 	LoadBalancerUpdateIPPortMapping(lbName, vip string, ipPortMappings map[string]string) error
-	LoadBalancerDeleteIPPortMapping(lbName, vip string) error
+	LoadBalancerDeleteIPPortMapping(lbName, backendIP string) error
+	LoadBalancerDeleteVipIPPortMapping(lbName, vip string) error
 	LoadBalancerAddHealthCheck(lbName, vip string, ignoreHealthCheck bool, ipPortMapping, externals map[string]string) error
 	LoadBalancerDeleteHealthCheck(lbName, uuid string) error
 	SetLoadBalancerAffinityTimeout(lbName string, timeout int) error

pkg/ovs/ovn-nb-load_balancer.go

@@ -160,7 +160,7 @@ func (c *OVNNbClient) LoadBalancerDeleteVip(lbName, vipEndpoint string, ignoreHe
 	if !ignoreHealthCheck && lbhc != nil {
 		klog.Infof("clean health check for lb %s with vip %s", lbName, vipEndpoint)
 		// delete ip port mapping
-		if err = c.LoadBalancerDeleteIPPortMapping(lbName, vipEndpoint); err != nil {
+		if err = c.LoadBalancerDeleteVipIPPortMapping(lbName, vipEndpoint); err != nil {
 			klog.Errorf("failed to delete lb ip port mapping: %v", err)
 			return err
 		}
@@ -473,9 +473,43 @@ func (c *OVNNbClient) LoadBalancerAddIPPortMapping(lbName, vipEndpoint string, m
 	return nil
 }
 
-// LoadBalancerDeleteIPPortMapping deletes IP port mappings for a specific VIP from a load balancer.
+// LoadBalancerDeleteIPPortMapping deletes IP port mappings for a specific backend IP from a load balancer.
+func (c *OVNNbClient) LoadBalancerDeleteIPPortMapping(lbName, backendIP string) error {
+	ops, err := c.LoadBalancerOp(
+		lbName,
+		func(lb *ovnnb.LoadBalancer) []model.Mutation {
+			if _, ok := lb.IPPortMappings[backendIP]; !ok {
+				return nil
+			}
+			return []model.Mutation{
+				{
+					Field:   &lb.IPPortMappings,
+					Value:   map[string]string{backendIP: lb.IPPortMappings[backendIP]},
+					Mutator: ovsdb.MutateOperationDelete,
+				},
+			}
+		},
+	)
+	if err != nil {
+		klog.Error(err)
+		return fmt.Errorf("failed to generate operations when deleting IP port mapping for backend %s from load balancer %s: %w", backendIP, lbName, err)
+	}
+
+	if len(ops) == 0 {
+		return nil
+	}
+
+	if err = c.Transact("lb-del", ops); err != nil {
+		return fmt.Errorf("failed to delete IP port mapping for backend %s from load balancer %s: %w", backendIP, lbName, err)
+	}
+
+	klog.Infof("successfully deleted ip port mapping for backend %s from load balancer %s", backendIP, lbName)
+	return nil
+}
+
+// LoadBalancerDeleteVipIPPortMapping deletes IP port mappings for a specific VIP from a load balancer.
 // This function ensures that only backend IPs that are no longer referenced by any VIP are removed.
-func (c *OVNNbClient) LoadBalancerDeleteIPPortMapping(lbName, vipEndpoint string) error {
+func (c *OVNNbClient) LoadBalancerDeleteVipIPPortMapping(lbName, vipEndpoint string) error {
 	lb, err := c.getLoadBalancerForDeletion(lbName)
 	if err != nil {
 		klog.Errorf("failed to get load balancer for deletion: %v", err)
@@ -625,24 +659,7 @@ func (c *OVNNbClient) LoadBalancerUpdateIPPortMapping(lbName, vipEndpoint string
 	ops, err := c.LoadBalancerOp(
 		lbName,
 		func(lb *ovnnb.LoadBalancer) []model.Mutation {
-			// Delete from the IPPortMappings any outdated mapping
-			mappingToDelete := make(map[string]string)
-			for portIP, portMapVip := range lb.IPPortMappings {
-				if _, ok := ipPortMappings[portIP]; !ok {
-					mappingToDelete[portIP] = portMapVip
-				}
-			}
-
-			if len(mappingToDelete) > 0 {
-				klog.Infof("deleting outdated entry from ipportmapping %v", mappingToDelete)
-			}
-
 			return []model.Mutation{
-				{
-					Field:   &lb.IPPortMappings,
-					Value:   mappingToDelete,
-					Mutator: ovsdb.MutateOperationDelete,
-				},
 				{
 					Field:   &lb.IPPortMappings,
 					Value:   ipPortMappings,