Merge pull request #3837 from vyos/mergify/bp/sagitta/pr-3834

interfaces: T6592: moving an interface between VRF instances failed (backport #3834)

6 files changed, 89 insertions, 18 deletions

diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py
index fa79395ff..fd4f5b269 100644
--- a/python/vyos/ifconfig/interface.py
+++ b/python/vyos/ifconfig/interface.py
@@ -36,6 +36,7 @@ from vyos.template import render
 from vyos.utils.network import mac2eui64
 from vyos.utils.dict import dict_search
 from vyos.utils.network import get_interface_config
+from vyos.utils.network import get_vrf_tableid
 from vyos.utils.process import is_systemd_service_active
 from vyos.template import is_ipv4
 from vyos.template import is_ipv6
@@ -387,25 +388,33 @@ class Interface(Control):
         cmd = 'ip link del dev {ifname}'.format(**self.config)
         return self._cmd(cmd)
 
-    def _set_vrf_ct_zone(self, vrf):
+    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(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}')
+
         if vrf:
             # Get routing table ID for VRF
-            vrf_table_id = get_interface_config(vrf).get('linkinfo', {}).get(
-                'info_data', {}).get('table')
+            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}" }}'
-            # Check if deleting is possible first to avoid raising errors
-            _, err = self._popen(f'nft --check {nft_del_element}')
-            if not err:
-                # Remove map element
-                self._cmd(f'nft {nft_del_element}')
+            nft_check_and_run(nft_del_element)
 
     def get_min_mtu(self):
         """
@@ -559,8 +568,11 @@ class Interface(Control):
         if tmp == vrf:
             return None
 
+        # Get current VRF table ID
+        old_vrf_tableid = get_vrf_tableid(self.ifname)
         self.set_interface('vrf', vrf)
-        self._set_vrf_ct_zone(vrf)
+        self._set_vrf_ct_zone(vrf, old_vrf_tableid)
+        return True
 
     def set_arp_cache_tmo(self, tmo):
         """
diff --git a/python/vyos/utils/network.py b/python/vyos/utils/network.py
index a3bd5c58f..8befe370f 100644
--- a/python/vyos/utils/network.py
+++ b/python/vyos/utils/network.py
@@ -70,6 +70,19 @@ def get_interface_vrf(interface):
         return tmp['master']
     return 'default'
 
+def get_vrf_tableid(interface: str):
+    """ Return VRF table ID for given interface name or None """
+    from vyos.utils.dict import dict_search
+    table = None
+    tmp = get_interface_config(interface)
+    # Check if we are "the" VRF interface
+    if dict_search('linkinfo.info_kind', tmp) == 'vrf':
+        table = tmp['linkinfo']['info_data']['table']
+    # or an interface bound to a VRF
+    elif dict_search('linkinfo.info_slave_kind', tmp) == 'vrf':
+        table = tmp['linkinfo']['info_slave_data']['table']
+    return table
+
 def get_interface_config(interface):
     """ Returns the used encapsulation protocol for given interface.
         If interface does not exist, None is returned.
diff --git a/smoketest/scripts/cli/base_interfaces_test.py b/smoketest/scripts/cli/base_interfaces_test.py
index 9be2c2f1a..4072fd5c2 100644
--- a/smoketest/scripts/cli/base_interfaces_test.py
+++ b/smoketest/scripts/cli/base_interfaces_test.py
@@ -28,6 +28,7 @@ from vyos.utils.dict import dict_search
 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 is_intf_addr_assigned
 from vyos.utils.network import is_ipv6_link_local
@@ -257,6 +258,51 @@ class BasicInterfaceTest:
 
             self.cli_delete(['vrf', 'name', vrf_name])
 
+        def test_move_interface_between_vrf_instances(self):
+            if not self._test_vrf:
+                self.skipTest('not supported')
+
+            vrf1_name = 'smoketest_mgmt1'
+            vrf1_table = '5424'
+            vrf2_name = 'smoketest_mgmt2'
+            vrf2_table = '7412'
+
+            self.cli_set(['vrf', 'name', vrf1_name, 'table', vrf1_table])
+            self.cli_set(['vrf', 'name', vrf2_name, 'table', vrf2_table])
+
+            # move interface into first VRF
+            for interface in self._interfaces:
+                for option in self._options.get(interface, []):
+                    self.cli_set(self._base_path + [interface] + option.split())
+                self.cli_set(self._base_path + [interface, 'vrf', vrf1_name])
+
+            self.cli_commit()
+
+            # check that interface belongs to proper VRF
+            for interface in self._interfaces:
+                tmp = get_interface_vrf(interface)
+                self.assertEqual(tmp, vrf1_name)
+
+                tmp = get_interface_config(vrf1_name)
+                self.assertEqual(int(vrf1_table), get_vrf_tableid(interface))
+
+            # move interface into second VRF
+            for interface in self._interfaces:
+                self.cli_set(self._base_path + [interface, 'vrf', vrf2_name])
+
+            self.cli_commit()
+
+            # check that interface belongs to proper VRF
+            for interface in self._interfaces:
+                tmp = get_interface_vrf(interface)
+                self.assertEqual(tmp, vrf2_name)
+
+                tmp = get_interface_config(vrf2_name)
+                self.assertEqual(int(vrf2_table), get_vrf_tableid(interface))
+
+            self.cli_delete(['vrf', 'name', vrf1_name])
+            self.cli_delete(['vrf', 'name', vrf2_name])
+
         def test_span_mirror(self):
             if not self._mirror_interfaces:
                 self.skipTest('not supported')
diff --git a/smoketest/scripts/cli/test_protocols_static.py b/smoketest/scripts/cli/test_protocols_static.py
index c5cf2aab6..f676e2a52 100755
--- a/smoketest/scripts/cli/test_protocols_static.py
+++ b/smoketest/scripts/cli/test_protocols_static.py
@@ -21,6 +21,7 @@ from base_vyostest_shim import VyOSUnitTestSHIM
 from vyos.configsession import ConfigSessionError
 from vyos.template import is_ipv6
 from vyos.utils.network import get_interface_config
+from vyos.utils.network import get_vrf_tableid
 
 base_path = ['protocols', 'static']
 vrf_path =  ['protocols', 'vrf']
@@ -421,7 +422,7 @@ class TestProtocolsStatic(VyOSUnitTestSHIM.TestCase):
             tmp = get_interface_config(vrf)
 
             # Compare VRF table ID
-            self.assertEqual(tmp['linkinfo']['info_data']['table'], int(vrf_config['table']))
+            self.assertEqual(get_vrf_tableid(vrf), int(vrf_config['table']))
             self.assertEqual(tmp['linkinfo']['info_kind'],          'vrf')
 
             # Verify FRR bgpd configuration
@@ -478,4 +479,4 @@ class TestProtocolsStatic(VyOSUnitTestSHIM.TestCase):
                     self.assertIn(tmp, frrconfig)
 
 if __name__ == '__main__':
-    unittest.main(verbosity=2)
+    unittest.main(verbosity=2, failfast=True)
diff --git a/smoketest/scripts/cli/test_vrf.py b/smoketest/scripts/cli/test_vrf.py
index 243397dc2..176882ca5 100755
--- a/smoketest/scripts/cli/test_vrf.py
+++ b/smoketest/scripts/cli/test_vrf.py
@@ -25,6 +25,7 @@ from vyos.ifconfig import Interface
 from vyos.ifconfig import Section
 from vyos.utils.file import read_file
 from vyos.utils.network import get_interface_config
+from vyos.utils.network import get_vrf_tableid
 from vyos.utils.network import is_intf_addr_assigned
 from vyos.utils.network import interface_exists
 from vyos.utils.system import sysctl_read
@@ -111,8 +112,7 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
             frrconfig = self.getFRRconfig(f'vrf {vrf}')
             self.assertIn(f' vni {table}', frrconfig)
 
-            tmp = get_interface_config(vrf)
-            self.assertEqual(int(table), tmp['linkinfo']['info_data']['table'])
+            self.assertEqual(int(table), get_vrf_tableid(vrf))
 
             # Increment table ID for the next run
             table = str(int(table) + 1)
@@ -266,8 +266,7 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
         for address in addresses:
             self.assertTrue(is_intf_addr_assigned(interface, address))
         # Verify VRF table ID
-        tmp = get_interface_config(vrf)
-        self.assertEqual(int(table), tmp['linkinfo']['info_data']['table'])
+        self.assertEqual(int(table), get_vrf_tableid(vrf))
 
         # Verify interface is assigned to VRF
         tmp = get_interface_config(interface)
diff --git a/src/conf_mode/vrf.py b/src/conf_mode/vrf.py
index 8d8c234c0..184725573 100755
--- a/src/conf_mode/vrf.py
+++ b/src/conf_mode/vrf.py
@@ -26,7 +26,7 @@ from vyos.ifconfig import Interface
 from vyos.template import render
 from vyos.template import render_to_string
 from vyos.utils.dict import dict_search
-from vyos.utils.network import get_interface_config
+from vyos.utils.network import get_vrf_tableid
 from vyos.utils.network import get_vrf_members
 from vyos.utils.network import interface_exists
 from vyos.utils.process import call
@@ -160,8 +160,8 @@ def verify(vrf):
 
             # routing table id can't be changed - OS restriction
             if interface_exists(name):
-                tmp = str(dict_search('linkinfo.info_data.table', get_interface_config(name)))
-                if tmp and tmp != vrf_config['table']:
+                tmp = get_vrf_tableid(name)
+                if tmp and tmp != int(vrf_config['table']):
                     raise ConfigError(f'VRF "{name}" table id modification not possible!')
 
             # VRF routing table ID must be unique on the system