vyos.ifconfig: T5103: force dhclient restart on VRF change

Moving an interface in, out or between VRFs will not re-install the received
default route. This is because the dhclient binary is not restarted in the new
VRF. Dhclient itself will report an error like: "receive_packet failed on
eth0.10: Network is down".

Take the return value of vyos.ifconfig.Interface().set_vrf() into account to
forcefully restart the DHCP client process and optain a proper lease.

3 files changed, 50 insertions, 21 deletions

diff --git a/python/vyos/configdict.py b/python/vyos/configdict.py
index 5a353b110..a6594871e 100644
--- a/python/vyos/configdict.py
+++ b/python/vyos/configdict.py
@@ -492,10 +492,6 @@ def get_interface_dict(config, base, ifname='', recursive_defaults=True, with_pk
     dhcp = is_node_changed(config, base + [ifname, 'dhcp-options'])
     if dhcp: dict.update({'dhcp_options_changed' : {}})
 
-    # Changine interface VRF assignemnts require a DHCP restart, too
-    dhcp = is_node_changed(config, base + [ifname, 'vrf'])
-    if dhcp: dict.update({'dhcp_options_changed' : {}})
-
     # Some interfaces come with a source_interface which must also not be part
     # of any other bond or bridge interface as it is exclusivly assigned as the
     # Kernels "lower" interface to this new "virtual/upper" interface.
diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py
index cb73e2597..91ee09d90 100644
--- a/python/vyos/ifconfig/interface.py
+++ b/python/vyos/ifconfig/interface.py
@@ -595,12 +595,16 @@ class Interface(Control):
         """
         Add/Remove interface from given VRF instance.
 
+        Keyword arguments:
+        vrf: VRF instance name or empty string (default VRF)
+
+        Return True if VRF was changed, False otherwise
+
         Example:
         >>> from vyos.ifconfig import Interface
         >>> Interface('eth0').set_vrf('foo')
         >>> Interface('eth0').set_vrf()
         """
-
         # Don't allow for netns yet
         if 'netns' in self.config:
             return False
@@ -617,15 +621,17 @@ class Interface(Control):
             # 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:
+            if vrf_table_id and old_vrf_tableid != 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._del_interface_from_ct_iface_map()
                 self._add_interface_to_ct_iface_map(vrf_table_id)
+                return True
         else:
-            self._del_interface_from_ct_iface_map()
+            if old_vrf_tableid != get_vrf_tableid(self.ifname):
+                self._del_interface_from_ct_iface_map()
+                return True
 
-        return True
+        return False
 
     def set_arp_cache_tmo(self, tmo):
         """
@@ -1181,7 +1187,7 @@ class Interface(Control):
         """
         return self.get_addr_v4() + self.get_addr_v6()
 
-    def add_addr(self, addr):
+    def add_addr(self, addr: str, vrf_changed: bool=False) -> bool:
         """
         Add IP(v6) address to interface. Address is only added if it is not
         already assigned to that interface. Address format must be validated
@@ -1214,7 +1220,7 @@ class Interface(Control):
 
         # add to interface
         if addr == 'dhcp':
-            self.set_dhcp(True)
+            self.set_dhcp(True, vrf_changed=vrf_changed)
         elif addr == 'dhcpv6':
             self.set_dhcpv6(True)
         elif not is_intf_addr_assigned(self.ifname, addr, netns=netns):
@@ -1222,7 +1228,6 @@ class Interface(Control):
             tmp = f'{netns_cmd} ip addr add {addr} dev {self.ifname}'
             # Add broadcast address for IPv4
             if is_ipv4(addr): tmp += ' brd +'
-
             self._cmd(tmp)
         else:
             return False
@@ -1232,7 +1237,7 @@ class Interface(Control):
 
         return True
 
-    def del_addr(self, addr):
+    def del_addr(self, addr: str, vrf_changed: bool=False) -> bool:
         """
         Delete IP(v6) address from interface. Address is only deleted if it is
         assigned to that interface. Address format must be exactly the same as
@@ -1356,7 +1361,7 @@ class Interface(Control):
                     cmd = f'bridge vlan add dev {ifname} vid {native_vlan_id} pvid untagged master'
                     self._cmd(cmd)
 
-    def set_dhcp(self, enable):
+    def set_dhcp(self, enable: bool, vrf_changed: bool=False):
         """
         Enable/Disable DHCP client on a given interface.
         """
@@ -1396,7 +1401,9 @@ class Interface(Control):
             # the old lease is released a new one is acquired (T4203). We will
             # only restart DHCP client if it's option changed, or if it's not
             # running, but it should be running (e.g. on system startup)
-            if 'dhcp_options_changed' in self.config or not is_systemd_service_active(systemd_service):
+            if (vrf_changed or
+                ('dhcp_options_changed' in self.config) or
+                (not is_systemd_service_active(systemd_service))):
                 return self._cmd(f'systemctl restart {systemd_service}')
         else:
             if is_systemd_service_active(systemd_service):
@@ -1676,23 +1683,24 @@ class Interface(Control):
         # XXX: Bind interface to given VRF or unbind it if vrf is not set. Unbinding
         # will call 'ip link set dev eth0 nomaster' which will also drop the
         # interface out of any bridge or bond - thus this is checked before.
+        vrf_changed = False
         if 'is_bond_member' in config:
             bond_if = next(iter(config['is_bond_member']))
             tmp = get_interface_config(config['ifname'])
             if 'master' in tmp and tmp['master'] != bond_if:
-                self.set_vrf('')
+                vrf_changed = self.set_vrf('')
 
         elif 'is_bridge_member' in config:
             bridge_if = next(iter(config['is_bridge_member']))
             tmp = get_interface_config(config['ifname'])
             if 'master' in tmp and tmp['master'] != bridge_if:
-                self.set_vrf('')
+                vrf_changed = self.set_vrf('')
         else:
-            self.set_vrf(config.get('vrf', ''))
+            vrf_changed = self.set_vrf(config.get('vrf', ''))
 
         # Add this section after vrf T4331
         for addr in new_addr:
-            self.add_addr(addr)
+            self.add_addr(addr, vrf_changed=vrf_changed)
 
         # Configure MSS value for IPv4 TCP connections
         tmp = dict_search('ip.adjust_mss', config)
diff --git a/smoketest/scripts/cli/base_interfaces_test.py b/smoketest/scripts/cli/base_interfaces_test.py
index c19bfcfe2..85888a448 100644
--- a/smoketest/scripts/cli/base_interfaces_test.py
+++ b/smoketest/scripts/cli/base_interfaces_test.py
@@ -38,6 +38,7 @@ 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
+from vyos.xml_ref import default_value
 
 dhclient_base_dir = directories['isc_dhclient_dir']
 dhclient_process_name = 'dhclient'
@@ -282,6 +283,9 @@ class BasicInterfaceTest:
             if not self._test_dhcp or not self._test_vrf:
                 self.skipTest('not supported')
 
+            cli_default_metric = default_value(self._base_path + [self._interfaces[0],
+                                               'dhcp-options', 'default-route-distance'])
+
             vrf_name = 'purple4'
             self.cli_set(['vrf', 'name', vrf_name, 'table', '65000'])
 
@@ -307,7 +311,28 @@ class BasicInterfaceTest:
                 self.assertIn(str(dhclient_pid), vrf_pids)
                 # and the commandline has the appropriate options
                 cmdline = read_file(f'/proc/{dhclient_pid}/cmdline')
-                self.assertIn('-e\x00IF_METRIC=210', cmdline) # 210 is the default value
+                self.assertIn(f'-e\x00IF_METRIC={cli_default_metric}', cmdline)
+
+            # T5103: remove interface from VRF instance and move DHCP client
+            # back to default VRF. This must restart the DHCP client process
+            for interface in self._interfaces:
+                self.cli_delete(self._base_path + [interface, 'vrf'])
+
+            self.cli_commit()
+
+            # Validate interface state
+            for interface in self._interfaces:
+                tmp = get_interface_vrf(interface)
+                self.assertEqual(tmp, 'default')
+                # Check if dhclient process runs
+                dhclient_pid = process_named_running(dhclient_process_name, cmdline=interface, timeout=10)
+                self.assertTrue(dhclient_pid)
+                # .. inside the appropriate VRF instance
+                vrf_pids = cmd(f'ip vrf pids {vrf_name}')
+                self.assertNotIn(str(dhclient_pid), vrf_pids)
+                # and the commandline has the appropriate options
+                cmdline = read_file(f'/proc/{dhclient_pid}/cmdline')
+                self.assertIn(f'-e\x00IF_METRIC={cli_default_metric}', cmdline)
 
             self.cli_delete(['vrf', 'name', vrf_name])