Automated cherry pick of #6898: Ensure that promote_secondaries is set on IPAssigner #6900: Ensure correct sysctl values if VLAN iface exists in

pkg/agent/ipassigner/ip_assigner_linux.go

@@ -299,7 +299,6 @@ func getARPIgnoreForInterface(iface string) (int, error) {
 		return arpIgnore, nil
 	}
 	return arpIgnoreAll, nil
-
 }
 
 // ensureDummyDevice creates the dummy device if it doesn't exist.
@@ -311,7 +310,14 @@ func ensureDummyDevice(deviceName string) (netlink.Link, error) {
 	dummy := &netlink.Dummy{
 		LinkAttrs: netlink.LinkAttrs{Name: deviceName},
 	}
-	if err = netlink.LinkAdd(dummy); err != nil {
+	if err := netlink.LinkAdd(dummy); err != nil {
+		return nil, err
+	}
+	// When the primary IP address is removed from the interface, promote a corresponding secondary IP address
+	// instead of removing all the corresponding secondary IP addresses. Otherwise, the deletion of one IP address
+	// can trigger the automatic removal of all other IP addresses in the same subnet, if the deleted IP happens to
+	// be the primary (first one assigned chronologically).
+	if err := util.EnsurePromoteSecondariesOnInterface(deviceName); err != nil {
 		return nil, err
 	}
 	return dummy, nil
@@ -502,12 +508,6 @@ func (a *ipAssigner) getAssignee(subnetInfo *crdv1b1.SubnetInfo, createIfNotExis
 			return nil, fmt.Errorf("error creating VLAN sub-interface for VLAN %d", subnetInfo.VLAN)
 		}
 	}
-	// Loose mode is needed because incoming traffic received on the interface is expected to be received on the parent
-	// external interface when looking up the main table. To make it look up the custom table, we will need to restore
-	// the mark on the reply traffic and turn on src_valid_mark on this interface, which is more complicated.
-	if err := util.EnsureRPFilterOnInterface(name, 2); err != nil {
-		return nil, err
-	}
 	as, err := a.addVLANAssignee(vlan, subnetInfo.VLAN)
 	if err != nil {
 		return nil, err
@@ -516,10 +516,24 @@ func (a *ipAssigner) getAssignee(subnetInfo *crdv1b1.SubnetInfo, createIfNotExis
 }
 
 func (a *ipAssigner) addVLANAssignee(link netlink.Link, vlan int32) (*assignee, error) {
+	name := link.Attrs().Name
+	// Loose mode is needed because incoming traffic received on the interface is expected to be received on the parent
+	// external interface when looking up the main table. To make it look up the custom table, we will need to restore
+	// the mark on the reply traffic and turn on src_valid_mark on this interface, which is more complicated.
+	if err := util.EnsureRPFilterOnInterface(name, 2); err != nil {
+		return nil, err
+	}
+	// When the primary IP address is removed from the interface, promote a corresponding secondary IP address
+	// instead of removing all the corresponding secondary IP addresses. Otherwise, the deletion of one IP address
+	// can trigger the automatic removal of all other IP addresses in the same subnet, if the deleted IP happens to
+	// be the primary (first one assigned chronologically).
+	if err := util.EnsurePromoteSecondariesOnInterface(name); err != nil {
+		return nil, err
+	}
 	if err := netlink.LinkSetUp(link); err != nil {
 		return nil, fmt.Errorf("error setting up interface %v", link)
 	}
-	iface, err := net.InterfaceByName(link.Attrs().Name)
+	iface, err := net.InterfaceByName(name)
 	if err != nil {
 		return nil, err
 	}

pkg/agent/util/net_linux.go

@@ -354,6 +354,11 @@ func EnsureRPFilterOnInterface(ifaceName string, value int) error {
 	return sysctl.EnsureSysctlNetValue(path, value)
 }
 
+func EnsurePromoteSecondariesOnInterface(ifaceName string) error {
+	path := fmt.Sprintf("ipv4/conf/%s/promote_secondaries", ifaceName)
+	return sysctl.EnsureSysctlNetValue(path, 1)
+}
+
 func getRoutesOnInterface(linkIndex int) ([]interface{}, error) {
 	link, err := netlinkUtil.LinkByIndex(linkIndex)
 	if err != nil {

test/integration/agent/ip_assigner_linux_test.go

@@ -25,11 +25,29 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	"antrea.io/antrea/pkg/agent/ipassigner"
+	"antrea.io/antrea/pkg/agent/util/sysctl"
 	crdv1b1 "antrea.io/antrea/pkg/apis/crd/v1beta1"
 )
 
 const dummyDeviceName = "antrea-dummy0"
 
+func checkSysctl(t *testing.T, path string, expected int) {
+	t.Helper()
+	v, err := sysctl.GetSysctlNet(path)
+	require.NoError(t, err)
+	assert.Equalf(t, expected, v, "Wrong value for %s", path)
+}
+
+func checkRPFilterOnInterface(t *testing.T, ifaceName string, expected int) {
+	t.Helper()
+	checkSysctl(t, fmt.Sprintf("ipv4/conf/%s/rp_filter", ifaceName), expected)
+}
+
+func checkPromoteSecondariesOnInterface(t *testing.T, ifaceName string, expected int) {
+	t.Helper()
+	checkSysctl(t, fmt.Sprintf("ipv4/conf/%s/promote_secondaries", ifaceName), expected)
+}
+
 func TestIPAssigner(t *testing.T) {
 	nodeLinkName := nodeIntf.Name
 	require.NotNil(t, nodeLinkName, "Get Node link failed")
@@ -40,6 +58,7 @@ func TestIPAssigner(t *testing.T) {
 	dummyDevice, err := netlink.LinkByName(dummyDeviceName)
 	require.NoError(t, err, "Failed to find the dummy device")
 	defer netlink.LinkDel(dummyDevice)
+	checkPromoteSecondariesOnInterface(t, dummyDeviceName, 1)
 
 	_, err = ipAssigner.AssignIP("x", nil, false)
 	assert.Error(t, err, "Assigning an invalid IP should fail")
@@ -52,9 +71,43 @@ func TestIPAssigner(t *testing.T) {
 	ip1VLAN30 := "10.10.30.10"
 	subnet20 := &crdv1b1.SubnetInfo{PrefixLength: 24, VLAN: 20}
 	subnet30 := &crdv1b1.SubnetInfo{PrefixLength: 24, VLAN: 30}
-	desiredIPs := map[string]*crdv1b1.SubnetInfo{ip1: nil, ip2: nil, ip3: nil, ip1VLAN20: subnet20, ip2VLAN20: subnet20, ip1VLAN30: subnet30}
+	// These IPs will be assigned to the correct interface, in the specified order.
+	ipsToAssign := []struct {
+		ip         string
+		subnetInfo *crdv1b1.SubnetInfo
+	}{
+		{
+			ip: ip1,
+		},
+		{
+			ip: ip2,
+		},
+		{
+			ip: ip3,
+		},
+		// ip1VLAN20 and ip2VLAN20 are in the same subnet and will be assigned to the same
+		// interface (antrea-ext.20).
+		// ip1VLAN20 will be assigned first, which means ip1VLAN20 will be the "primary" IP,
+		// while ip2VLAN20 will be the "secondary" IP.
+		{
+			ip:         ip1VLAN20,
+			subnetInfo: subnet20,
+		},
+		{
+			ip:         ip2VLAN20,
+			subnetInfo: subnet20,
+		},
+		{
+			ip:         ip1VLAN30,
+			subnetInfo: subnet30,
+		},
+	}
+
+	desiredIPs := make(map[string]*crdv1b1.SubnetInfo)
+	for _, assignment := range ipsToAssign {
+		ip, subnetInfo := assignment.ip, assignment.subnetInfo
+		desiredIPs[ip] = subnetInfo
 
-	for ip, subnetInfo := range desiredIPs {
 		_, errAssign := ipAssigner.AssignIP(ip, subnetInfo, false)
 		cmd := exec.Command("ip", "addr")
 		out, err := cmd.CombinedOutput()
@@ -69,9 +122,13 @@ func TestIPAssigner(t *testing.T) {
 	vlan20Device, err := netlink.LinkByName("antrea-ext.20")
 	require.NoError(t, err, "Failed to find the VLAN 20 device")
 	defer netlink.LinkDel(vlan20Device)
+	checkRPFilterOnInterface(t, "antrea-ext.20", 2)
+	checkPromoteSecondariesOnInterface(t, "antrea-ext.20", 1)
 	vlan30Device, err := netlink.LinkByName("antrea-ext.30")
 	require.NoError(t, err, "Failed to find the VLAN 30 device")
 	defer netlink.LinkDel(vlan30Device)
+	checkRPFilterOnInterface(t, "antrea-ext.30", 2)
+	checkPromoteSecondariesOnInterface(t, "antrea-ext.30", 1)
 
 	actualIPs, err := listIPAddresses(dummyDevice)
 	require.NoError(t, err, "Failed to list IP addresses")
@@ -88,7 +145,14 @@ func TestIPAssigner(t *testing.T) {
 	assert.Equal(t, map[string]*crdv1b1.SubnetInfo{}, newIPAssigner.AssignedIPs(), "Assigned IPs don't match")
 
 	ip4 := "2021:124:6020:1006:250:56ff:fea7:36c4"
-	newDesiredIPs := map[string]*crdv1b1.SubnetInfo{ip1: nil, ip2: nil, ip4: nil, ip1VLAN20: subnet20}
+	// ip1VLAN20 is omitted, so it will be removed from the antrea-ext.20 interface. Because it
+	// is the primary IP address, secondary IPs (in this case ip2VLAN20) in the same subnet will
+	// be automatically removed when the primary is removed, unless the promote_secondaries
+	// sysctl variable has been set to 1 on the interface, which should be the case.
+	// By removing ip1VLAN20 (primary), we can therefore validate that IPAssigner is setting
+	// promote_secondaries correctly on the interface, as otherwise ip2VLAN20 will be removed
+	// automatically.
+	newDesiredIPs := map[string]*crdv1b1.SubnetInfo{ip1: nil, ip2: nil, ip4: nil, ip2VLAN20: subnet20}
 	err = newIPAssigner.InitIPs(newDesiredIPs)
 	require.NoError(t, err, "InitIPs failed")
 	assert.Equal(t, newDesiredIPs, newIPAssigner.AssignedIPs(), "Assigned IPs don't match")
@@ -98,7 +162,7 @@ func TestIPAssigner(t *testing.T) {
 	assert.Equal(t, sets.New[string](fmt.Sprintf("%s/32", ip1), fmt.Sprintf("%s/32", ip2), fmt.Sprintf("%s/128", ip4)), actualIPs, "Actual IPs don't match")
 	actualIPs, err = listIPAddresses(vlan20Device)
 	require.NoError(t, err, "Failed to list IP addresses")
-	assert.Equal(t, sets.New[string](fmt.Sprintf("%s/%d", ip1VLAN20, subnet20.PrefixLength)), actualIPs, "Actual IPs don't match")
+	assert.Equal(t, sets.New[string](fmt.Sprintf("%s/%d", ip2VLAN20, subnet20.PrefixLength)), actualIPs, "Actual IPs don't match")
 	_, err = netlink.LinkByName("antrea-ext.30")
 	require.Error(t, err, "VLAN 30 device should be deleted but was not")