From 1ca6f6ca826c49b2ac5f8e9c40ae6ae765cdea9a Mon Sep 17 00:00:00 2001 From: Antonin Bas Date: Mon, 6 Jan 2025 12:41:23 -0800 Subject: [PATCH] Ensure that promote_secondaries is set on IPAssigner interfaces (#6898) The IPAssigner should ensure that the promote_secondaries sysctl variable is always set when creating an interface. This ensures that When the primary IP address is removed from the interface, a secondary IP address is promoted, 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). For example, this can affect the Egress feature (when EgressSeparateSubnet is used). Fixes #6890 Signed-off-by: Antonin Bas --- pkg/agent/ipassigner/ip_assigner_linux.go | 17 ++++++- pkg/agent/util/net_linux.go | 5 ++ .../agent/ip_assigner_linux_test.go | 49 +++++++++++++++++-- 3 files changed, 65 insertions(+), 6 deletions(-) diff --git a/pkg/agent/ipassigner/ip_assigner_linux.go b/pkg/agent/ipassigner/ip_assigner_linux.go index defa0199f5e..07ec11b033a 100644 --- a/pkg/agent/ipassigner/ip_assigner_linux.go +++ b/pkg/agent/ipassigner/ip_assigner_linux.go @@ -294,7 +294,6 @@ func getARPIgnoreForInterface(iface string) (int, error) { return arpIgnore, nil } return arpIgnoreAll, nil - } // ensureDummyDevice creates the dummy device if it doesn't exist. @@ -306,7 +305,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 @@ -503,6 +509,13 @@ func (a *ipAssigner) getAssignee(subnetInfo *crdv1b1.SubnetInfo, createIfNotExis 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 + } as, err := a.addVLANAssignee(vlan, subnetInfo.VLAN) if err != nil { return nil, err diff --git a/pkg/agent/util/net_linux.go b/pkg/agent/util/net_linux.go index 7d70c747ad0..eb8195b45ec 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 ae4571bc0fc..03774a0eabb 100644 --- a/test/integration/agent/ip_assigner_linux_test.go +++ b/test/integration/agent/ip_assigner_linux_test.go @@ -52,9 +52,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() @@ -88,7 +122,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 +139,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")