Merge pull request #3872 from vyos/mergify/bp/sagitta/pr-3857

interface: T6592: remove interface from conntrack ct_iface_map on deletion (backport #3857)

5 files changed, 80 insertions, 33 deletions

diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py
index fd4f5b269..f29615a44 100644
--- a/python/vyos/ifconfig/interface.py
+++ b/python/vyos/ifconfig/interface.py
@@ -371,6 +371,9 @@ class Interface(Control):
         # can not delete ALL interfaces, see below
         self.flush_addrs()
 
+        # remove interface from conntrack VRF interface map
+        self._del_interface_from_ct_iface_map()
+
         # ---------------------------------------------------------------------
         # Any class can define an eternal regex in its definition
         # interface matching the regex will not be deleted
@@ -388,33 +391,20 @@ class Interface(Control):
         cmd = 'ip link del dev {ifname}'.format(**self.config)
         return self._cmd(cmd)
 
-    def _set_vrf_ct_zone(self, vrf, old_vrf_tableid=None):
-        """
-        Add/Remove rules in nftables to associate traffic in VRF to an
-        individual conntack zone
-        """
+    def _nft_check_and_run(self, nft_command):
+        # Check if deleting is possible first to avoid raising errors
+        _, err = self._popen(f'nft --check {nft_command}')
+        if not err:
+            # Remove map element
+            self._cmd(f'nft {nft_command}')
 
-        def nft_check_and_run(nft_command):
-            # Check if deleting is possible first to avoid raising errors
-            _, err = self._popen(f'nft --check {nft_command}')
-            if not err:
-                # Remove map element
-                self._cmd(f'nft {nft_command}')
+    def _del_interface_from_ct_iface_map(self):
+        nft_command = f'delete element inet vrf_zones ct_iface_map {{ "{self.ifname}" }}'
+        self._nft_check_and_run(nft_command)
 
-        if vrf:
-            # Get routing table ID for VRF
-            vrf_table_id = get_vrf_tableid(vrf)
-            # Add map element with interface and zone ID
-            if vrf_table_id:
-                # delete old table ID from nftables if it has changed, e.g. interface moved to a different VRF
-                if old_vrf_tableid and old_vrf_tableid != int(vrf_table_id):
-                    nft_del_element = f'delete element inet vrf_zones ct_iface_map {{ "{self.ifname}" }}'
-                    nft_check_and_run(nft_del_element)
-
-                self._cmd(f'nft add element inet vrf_zones ct_iface_map {{ "{self.ifname}" : {vrf_table_id} }}')
-        else:
-            nft_del_element = f'delete element inet vrf_zones ct_iface_map {{ "{self.ifname}" }}'
-            nft_check_and_run(nft_del_element)
+    def _add_interface_to_ct_iface_map(self, vrf_table_id: int):
+        nft_command = f'add element inet vrf_zones ct_iface_map {{ "{self.ifname}" : {vrf_table_id} }}'
+        self._nft_check_and_run(nft_command)
 
     def get_min_mtu(self):
         """
@@ -564,6 +554,10 @@ class Interface(Control):
         >>> Interface('eth0').set_vrf()
         """
 
+        # Don't allow for netns yet
+        if 'netns' in self.config:
+            return False
+
         tmp = self.get_interface('vrf')
         if tmp == vrf:
             return None
@@ -571,7 +565,19 @@ class Interface(Control):
         # Get current VRF table ID
         old_vrf_tableid = get_vrf_tableid(self.ifname)
         self.set_interface('vrf', vrf)
-        self._set_vrf_ct_zone(vrf, old_vrf_tableid)
+
+        if vrf:
+            # Get routing table ID number for VRF
+            vrf_table_id = get_vrf_tableid(vrf)
+            # Add map element with interface and zone ID
+            if vrf_table_id:
+                # delete old table ID from nftables if it has changed, e.g. interface moved to a different VRF
+                if old_vrf_tableid and old_vrf_tableid != int(vrf_table_id):
+                    self._del_interface_from_ct_iface_map()
+                self._add_interface_to_ct_iface_map(vrf_table_id)
+        else:
+            self._del_interface_from_ct_iface_map()
+
         return True
 
     def set_arp_cache_tmo(self, tmo):
diff --git a/python/vyos/ifconfig/l2tpv3.py b/python/vyos/ifconfig/l2tpv3.py
index 85a89ef8b..c1f2803ee 100644
--- a/python/vyos/ifconfig/l2tpv3.py
+++ b/python/vyos/ifconfig/l2tpv3.py
@@ -90,9 +90,17 @@ class L2TPv3If(Interface):
         """
 
         if self.exists(self.ifname):
-            # interface is always A/D down. It needs to be enabled explicitly
             self.set_admin_state('down')
 
+            # remove all assigned IP addresses from interface - this is a bit redundant
+            # as the kernel will remove all addresses on interface deletion
+            self.flush_addrs()
+
+            # remove interface from conntrack VRF interface map, here explicitly and do not
+            # rely on the base class implementation as the interface will
+            # vanish as soon as the l2tp session is deleted
+            self._del_interface_from_ct_iface_map()
+
             if {'tunnel_id', 'session_id'} <= set(self.config):
                 cmd = 'ip l2tp del session tunnel_id {tunnel_id}'
                 cmd += ' session_id {session_id}'
@@ -101,3 +109,5 @@ class L2TPv3If(Interface):
             if 'tunnel_id' in self.config:
                 cmd = 'ip l2tp del tunnel tunnel_id {tunnel_id}'
                 self._cmd(cmd.format(**self.config))
+
+            # No need to call the baseclass as the interface is now already gone
diff --git a/python/vyos/utils/network.py b/python/vyos/utils/network.py
index 8befe370f..55798651f 100644
--- a/python/vyos/utils/network.py
+++ b/python/vyos/utils/network.py
@@ -530,3 +530,31 @@ def get_vxlan_vni_filter(interface: str) -> list:
             os_configured_vnis.append(str(vniStart))
 
     return os_configured_vnis
+
+def get_nft_vrf_zone_mapping() -> dict:
+    """
+    Retrieve current nftables conntrack mapping list from Kernel
+
+    returns: [{'interface': 'red', 'vrf_tableid': 1000},
+              {'interface': 'eth2', 'vrf_tableid': 1000},
+              {'interface': 'blue', 'vrf_tableid': 2000}]
+    """
+    from json import loads
+    from jmespath import search
+    from vyos.utils.process import cmd
+    output = []
+    tmp = loads(cmd('sudo nft -j list table inet vrf_zones'))
+    # {'nftables': [{'metainfo': {'json_schema_version': 1,
+    #                     'release_name': 'Old Doc Yak #3',
+    #                     'version': '1.0.9'}},
+    #       {'table': {'family': 'inet', 'handle': 6, 'name': 'vrf_zones'}},
+    #       {'map': {'elem': [['eth0', 666],
+    #                         ['dum0', 666],
+    #                         ['wg500', 666],
+    #                         ['bond10.666', 666]],
+    vrf_list = search('nftables[].map.elem | [0]', tmp)
+    if not vrf_list:
+        return output
+    for (vrf_name, vrf_id) in vrf_list:
+        output.append({'interface' : vrf_name, 'vrf_tableid' : vrf_id})
+    return output
diff --git a/smoketest/scripts/cli/base_interfaces_test.py b/smoketest/scripts/cli/base_interfaces_test.py
index 66b789e94..e7e29387f 100644
--- a/smoketest/scripts/cli/base_interfaces_test.py
+++ b/smoketest/scripts/cli/base_interfaces_test.py
@@ -15,7 +15,6 @@
 from netifaces import AF_INET
 from netifaces import AF_INET6
 from netifaces import ifaddresses
-from netifaces import interfaces
 
 from base_vyostest_shim import VyOSUnitTestSHIM
 
@@ -25,13 +24,15 @@ from vyos.ifconfig import Interface
 from vyos.ifconfig import Section
 from vyos.utils.file import read_file
 from vyos.utils.dict import dict_search
+from vyos.utils.process import cmd
 from vyos.utils.process import process_named_running
 from vyos.utils.network import get_interface_config
 from vyos.utils.network import get_interface_vrf
 from vyos.utils.network import get_vrf_tableid
-from vyos.utils.process import cmd
+from vyos.utils.network import interface_exists
 from vyos.utils.network import is_intf_addr_assigned
 from vyos.utils.network import is_ipv6_link_local
+from vyos.utils.network import get_nft_vrf_zone_mapping
 from vyos.xml_ref import cli_defined
 
 dhclient_base_dir = directories['isc_dhclient_dir']
@@ -117,8 +118,11 @@ class BasicInterfaceTest:
             self.cli_commit()
 
             # Verify that no previously interface remained on the system
+            ct_map = get_nft_vrf_zone_mapping()
             for intf in self._interfaces:
-                self.assertNotIn(intf, interfaces())
+                self.assertFalse(interface_exists(intf))
+                for map_entry in ct_map:
+                     self.assertNotEqual(intf, map_entry['interface'])
 
             # No daemon started during tests should remain running
             for daemon in ['dhcp6c', 'dhclient']:
diff --git a/smoketest/scripts/cli/test_interfaces_l2tpv3.py b/smoketest/scripts/cli/test_interfaces_l2tpv3.py
index af3d49f75..abc55e6d2 100755
--- a/smoketest/scripts/cli/test_interfaces_l2tpv3.py
+++ b/smoketest/scripts/cli/test_interfaces_l2tpv3.py
@@ -20,7 +20,7 @@ import unittest
 
 from base_interfaces_test import BasicInterfaceTest
 from vyos.utils.process import cmd
-
+from vyos.utils.kernel import unload_kmod
 class L2TPv3InterfaceTest(BasicInterfaceTest.TestCase):
     @classmethod
     def setUpClass(cls):
@@ -62,7 +62,6 @@ if __name__ == '__main__':
     # reloaded on demand - not needed but test more and more features
     for module in ['l2tp_ip6', 'l2tp_ip', 'l2tp_eth', 'l2tp_eth',
                    'l2tp_netlink', 'l2tp_core']:
-        if os.path.exists(f'/sys/module/{module}'):
-            cmd(f'sudo rmmod {module}')
+        unload_kmod(module)
 
     unittest.main(verbosity=2)