diff --git a/pkg/agent/ipassigner/ip_assigner_linux.go b/pkg/agent/ipassigner/ip_assigner_linux.go index 6256bae61b2..d09dbfcdef4 100644 --- a/pkg/agent/ipassigner/ip_assigner_linux.go +++ b/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 } diff --git a/pkg/agent/util/net_linux.go b/pkg/agent/util/net_linux.go index d8f1ef0b901..2bc46a8514c 100644 --- a/pkg/agent/util/net_linux.go +++ b/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 { diff --git a/test/integration/agent/ip_assigner_linux_test.go b/test/integration/agent/ip_assigner_linux_test.go index 1016502753e..39f4a1920b8 100644 --- a/test/integration/agent/ip_assigner_linux_test.go +++ b/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")