From 3593c2ab0dc5b9fc84716fa9837443a46932ef56 Mon Sep 17 00:00:00 2001 From: Antonin Bas Date: Mon, 6 Jan 2025 12:41:23 -0800 Subject: [PATCH 1/2] 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 6256bae61b2..6850cbe38f4 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 @@ -508,6 +514,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 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..803cd4acfe5 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") From dc1fd9ae8e5a01059b20f060e420588c89088cae Mon Sep 17 00:00:00 2001 From: Antonin Bas Date: Wed, 8 Jan 2025 11:22:37 -0800 Subject: [PATCH 2/2] Ensure correct sysctl values if VLAN iface exists in IPAssigner (#6900) If the VLAN interface already exixts (e.g., in case of an Agent restart), we should still ensure that the necessary systcl variables are set correctly. Signed-off-by: Antonin Bas --- pkg/agent/ipassigner/ip_assigner_linux.go | 19 +++++++-------- .../agent/ip_assigner_linux_test.go | 23 +++++++++++++++++++ 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/pkg/agent/ipassigner/ip_assigner_linux.go b/pkg/agent/ipassigner/ip_assigner_linux.go index 6850cbe38f4..d09dbfcdef4 100644 --- a/pkg/agent/ipassigner/ip_assigner_linux.go +++ b/pkg/agent/ipassigner/ip_assigner_linux.go @@ -508,6 +508,15 @@ func (a *ipAssigner) getAssignee(subnetInfo *crdv1b1.SubnetInfo, createIfNotExis return nil, fmt.Errorf("error creating VLAN sub-interface for VLAN %d", subnetInfo.VLAN) } } + as, err := a.addVLANAssignee(vlan, subnetInfo.VLAN) + if err != nil { + return nil, err + } + return as, nil +} + +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. @@ -521,18 +530,10 @@ func (a *ipAssigner) getAssignee(subnetInfo *crdv1b1.SubnetInfo, createIfNotExis if err := util.EnsurePromoteSecondariesOnInterface(name); err != nil { return nil, err } - as, err := a.addVLANAssignee(vlan, subnetInfo.VLAN) - if err != nil { - return nil, err - } - return as, nil -} - -func (a *ipAssigner) addVLANAssignee(link netlink.Link, vlan int32) (*assignee, error) { 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/test/integration/agent/ip_assigner_linux_test.go b/test/integration/agent/ip_assigner_linux_test.go index 803cd4acfe5..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") @@ -103,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")