From fbd5e7513984dc3e4b8b973b3017c6b9e3ffa8ad Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Wed, 5 Feb 2025 22:11:22 +0100 Subject: [PATCH] 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. --- python/vyos/ifconfig/interface.py | 32 +++++++++++-------- smoketest/scripts/cli/base_interfaces_test.py | 27 +++++++++++++++- 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py index cb73e2597d..8c248c7087 100644 --- a/python/vyos/ifconfig/interface.py +++ b/python/vyos/ifconfig/interface.py @@ -595,12 +595,16 @@ def set_vrf(self, vrf: str) -> bool: """ 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 @@ -619,7 +623,7 @@ def set_vrf(self, vrf: str) -> bool: # 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): + if old_vrf_tableid != vrf_table_id: self._del_interface_from_ct_iface_map() self._add_interface_to_ct_iface_map(vrf_table_id) else: @@ -1181,7 +1185,7 @@ def get_addr(self): """ 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 +1218,7 @@ def add_addr(self, addr): # 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 +1226,6 @@ def add_addr(self, addr): 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 +1235,7 @@ def add_addr(self, addr): 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 @@ -1264,7 +1267,7 @@ def del_addr(self, addr): # remove from interface if addr == 'dhcp': - self.set_dhcp(False) + self.set_dhcp(False, vrf_changed=False) elif addr == 'dhcpv6': self.set_dhcpv6(False) elif is_intf_addr_assigned(self.ifname, addr, netns=netns): @@ -1356,7 +1359,7 @@ def add_to_bridge(self, bridge_dict): 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, vrf_changed: bool=False): """ Enable/Disable DHCP client on a given interface. """ @@ -1396,7 +1399,9 @@ def set_dhcp(self, enable): # 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 +1681,24 @@ def update(self, config): # 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 c19bfcfe26..85888a448d 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_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 @@ def test_dhcp_vrf(self): 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 @@ def test_dhcp_vrf(self): 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])