From 4105cfa2e297faf7dd4e5eb262bc32745e2c1733 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Tue, 11 Apr 2023 16:20:43 -0600 Subject: [PATCH 01/11] shift dhclient behavior into cloudinit/net, start plumbing distro object to dhcp --- cloudinit/net/dhcp.py | 95 ++++++++++++++++++- cloudinit/net/ephemeral.py | 2 + cloudinit/net/freebsd.py | 14 +-- cloudinit/net/openbsd.py | 4 +- cloudinit/sources/DataSourceCloudStack.py | 59 +----------- .../sources/helpers/vmware/imc/config_nic.py | 7 +- 6 files changed, 102 insertions(+), 79 deletions(-) diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py index c934ee1677e..d512569ed6a 100644 --- a/cloudinit/net/dhcp.py +++ b/cloudinit/net/dhcp.py @@ -11,6 +11,7 @@ import signal import time from io import StringIO +import glob import configobj @@ -47,7 +48,7 @@ class NoDHCPLeaseMissingDhclientError(NoDHCPLeaseError): """Raised when unable to find dhclient.""" -def maybe_perform_dhcp_discovery(nic=None, dhcp_log_func=None): +def maybe_perform_dhcp_discovery(nic=None, dhcp_log_func=None, distro=None): """Perform dhcp discovery if nic valid and dhclient command exists. If the nic is invalid or undiscoverable or dhclient command is not found, @@ -74,7 +75,7 @@ def maybe_perform_dhcp_discovery(nic=None, dhcp_log_func=None): if not dhclient_path: LOG.debug("Skip dhclient configuration: No dhclient command found.") raise NoDHCPLeaseMissingDhclientError() - return dhcp_discovery(dhclient_path, nic, dhcp_log_func) + return dhcp_discovery(dhclient_path, nic, dhcp_log_func, distro) def parse_dhcp_lease_file(lease_file): @@ -111,7 +112,11 @@ def parse_dhcp_lease_file(lease_file): return dhcp_leases -def dhcp_discovery(dhclient_cmd_path, interface, dhcp_log_func=None): +def dhcp_discovery( + dhclient_cmd_path, + interface, + dhcp_log_func=None, + distro=None): """Run dhclient on the interface without scripts or filesystem artifacts. @param dhclient_cmd_path: Full path to the dhclient used. @@ -364,4 +369,86 @@ def _trunc_error(cidr, required, remain): return static_routes -# vi: ts=4 expandtab +def kill_dhcp_client(): + subp.subp(["pkill", "dhclient"], rcs=[0, 1]) + + +def clear_leases(): + kill_dhcp_client() + files = glob.glob("/var/lib/dhcp/*") + for file in files: + os.remove(files) + + +def start_service(dhcp_interface): + subp.subp( + ["service", "dhclient", "start", dhcp_interface], + rcs=[0, 1], + capture=True, + ) + + +def stop_service(dhcp_interface): + subp.subp( + ["service", "dhclient", "stop", dhcp_interface], + rcs=[0, 1], + capture=True, + ) + +def get_dhclient_d(): + # find lease files directory + supported_dirs = [ + "/var/lib/dhclient", + "/var/lib/dhcp", + "/var/lib/NetworkManager", + ] + for d in supported_dirs: + if os.path.exists(d) and len(os.listdir(d)) > 0: + LOG.debug("Using %s lease directory", d) + return d + return None + + +def get_latest_lease(lease_d=None): + # find latest lease file + if lease_d is None: + lease_d = get_dhclient_d() + if not lease_d: + return None + lease_files = os.listdir(lease_d) + latest_mtime = -1 + latest_file = None + + # lease files are named inconsistently across distros. + # We assume that 'dhclient6' indicates ipv6 and ignore it. + # ubuntu: + # dhclient..leases, dhclient.leases, dhclient6.leases + # centos6: + # dhclient-.leases, dhclient6.leases + # centos7: ('--' is not a typo) + # dhclient--.lease, dhclient6.leases + for fname in lease_files: + if fname.startswith("dhclient6"): + # avoid files that start with dhclient6 assuming dhcpv6. + continue + if not (fname.endswith(".lease") or fname.endswith(".leases")): + continue + + abs_path = os.path.join(lease_d, fname) + mtime = os.path.getmtime(abs_path) + if mtime > latest_mtime: + latest_mtime = mtime + latest_file = abs_path + return latest_file + + +def parse_dhcp_server_from_lease_file(lease_file): + with open(lease_file, "r") as fd: + for line in fd: + if "dhcp-server-identifier" in line: + words = line.strip(" ;\r\n").split(" ") + if len(words) > 2: + dhcptok = words[2] + LOG.debug("Found DHCP identifier %s", dhcptok) + latest_address = dhcptok + return latest_address diff --git a/cloudinit/net/ephemeral.py b/cloudinit/net/ephemeral.py index 1dfd1c428e1..654db8d86a5 100644 --- a/cloudinit/net/ephemeral.py +++ b/cloudinit/net/ephemeral.py @@ -308,12 +308,14 @@ def __init__( iface=None, connectivity_url_data: Optional[Dict[str, Any]] = None, dhcp_log_func=None, + distro=None, ): self.iface = iface self._ephipv4 = None self.lease = None self.dhcp_log_func = dhcp_log_func self.connectivity_url_data = connectivity_url_data + self.distro = distro def __enter__(self): """Setup sandboxed dhcp context, unless connectivity_url can already be diff --git a/cloudinit/net/freebsd.py b/cloudinit/net/freebsd.py index 415f4a5aab6..990aa161bf2 100644 --- a/cloudinit/net/freebsd.py +++ b/cloudinit/net/freebsd.py @@ -2,7 +2,7 @@ import cloudinit.net.bsd from cloudinit import log as logging -from cloudinit import subp, util +from cloudinit import subp, util, net LOG = logging.getLogger(__name__) @@ -50,11 +50,7 @@ def start_services(self, run=False): for dhcp_interface in self.dhcp_interfaces(): # Observed on DragonFlyBSD 6. If we use the "restart" parameter, # the routes are not recreated. - subp.subp( - ["service", "dhclient", "stop", dhcp_interface], - rcs=[0, 1], - capture=True, - ) + net.dhcp.start_service(dhcp_interface) subp.subp(["service", "netif", "restart"], capture=True) # On FreeBSD 10, the restart of routing and dhclient is likely to fail @@ -66,11 +62,7 @@ def start_services(self, run=False): subp.subp(["service", "routing", "restart"], capture=True, rcs=[0, 1]) for dhcp_interface in self.dhcp_interfaces(): - subp.subp( - ["service", "dhclient", "start", dhcp_interface], - rcs=[0, 1], - capture=True, - ) + net.dhcp.stop_service(dhcp_interface) def set_route(self, network, netmask, gateway): if network == "0.0.0.0": diff --git a/cloudinit/net/openbsd.py b/cloudinit/net/openbsd.py index 70e9f461172..96c529068a6 100644 --- a/cloudinit/net/openbsd.py +++ b/cloudinit/net/openbsd.py @@ -4,7 +4,7 @@ import cloudinit.net.bsd from cloudinit import log as logging -from cloudinit import subp, util +from cloudinit import subp, util, net LOG = logging.getLogger(__name__) @@ -43,7 +43,7 @@ def start_services(self, run=False): ["dhcpleasectl", "-w", "30", interface], capture=True ) else: - subp.subp(["pkill", "dhclient"], capture=True, rcs=[0, 1]) + net.dhcp.kill_dhcp_client() subp.subp(["route", "del", "default"], capture=True, rcs=[0, 1]) subp.subp(["route", "flush", "default"], capture=True, rcs=[0, 1]) subp.subp(["sh", "/etc/netstart"], capture=True) diff --git a/cloudinit/sources/DataSourceCloudStack.py b/cloudinit/sources/DataSourceCloudStack.py index 3cdfeecaba9..b6b60d91828 100644 --- a/cloudinit/sources/DataSourceCloudStack.py +++ b/cloudinit/sources/DataSourceCloudStack.py @@ -207,51 +207,6 @@ def get_default_gateway(): return None -def get_dhclient_d(): - # find lease files directory - supported_dirs = [ - "/var/lib/dhclient", - "/var/lib/dhcp", - "/var/lib/NetworkManager", - ] - for d in supported_dirs: - if os.path.exists(d) and len(os.listdir(d)) > 0: - LOG.debug("Using %s lease directory", d) - return d - return None - - -def get_latest_lease(lease_d=None): - # find latest lease file - if lease_d is None: - lease_d = get_dhclient_d() - if not lease_d: - return None - lease_files = os.listdir(lease_d) - latest_mtime = -1 - latest_file = None - - # lease files are named inconsistently across distros. - # We assume that 'dhclient6' indicates ipv6 and ignore it. - # ubuntu: - # dhclient..leases, dhclient.leases, dhclient6.leases - # centos6: - # dhclient-.leases, dhclient6.leases - # centos7: ('--' is not a typo) - # dhclient--.lease, dhclient6.leases - for fname in lease_files: - if fname.startswith("dhclient6"): - # avoid files that start with dhclient6 assuming dhcpv6. - continue - if not (fname.endswith(".lease") or fname.endswith(".leases")): - continue - - abs_path = os.path.join(lease_d, fname) - mtime = os.path.getmtime(abs_path) - if mtime > latest_mtime: - latest_mtime = mtime - latest_file = abs_path - return latest_file def get_vr_address(): @@ -277,19 +232,12 @@ def get_vr_address(): return latest_address # Try dhcp lease files next... - lease_file = get_latest_lease() + lease_file = dhcp.get_latest_lease() if not lease_file: LOG.debug("No lease file found, using default gateway") return get_default_gateway() - with open(lease_file, "r") as fd: - for line in fd: - if "dhcp-server-identifier" in line: - words = line.strip(" ;\r\n").split(" ") - if len(words) > 2: - dhcptok = words[2] - LOG.debug("Found DHCP identifier %s", dhcptok) - latest_address = dhcptok + lease_file = dhcp.parse_dhcp_server_from_lease_file(lease_file) if not latest_address: # No virtual router found, fallback on default gateway LOG.debug("No DHCP found, using default gateway") @@ -306,6 +254,3 @@ def get_vr_address(): # Return a list of data sources that match this set of dependencies def get_datasource_list(depends): return sources.list_from_depends(depends, datasources) - - -# vi: ts=4 expandtab diff --git a/cloudinit/sources/helpers/vmware/imc/config_nic.py b/cloudinit/sources/helpers/vmware/imc/config_nic.py index ba2488be9e6..466e714eaee 100644 --- a/cloudinit/sources/helpers/vmware/imc/config_nic.py +++ b/cloudinit/sources/helpers/vmware/imc/config_nic.py @@ -9,7 +9,7 @@ import os import re -from cloudinit import subp, util +from cloudinit import subp, util, net from cloudinit.net.network_state import ipv4_mask_to_net_prefix logger = logging.getLogger(__name__) @@ -245,10 +245,7 @@ def generate(self, configure=False, osfamily=None): def clear_dhcp(self): logger.info("Clearing DHCP leases") - - # Ignore the return code 1. - subp.subp(["pkill", "dhclient"], rcs=[0, 1]) - subp.subp(["rm", "-f", "/var/lib/dhcp/*"]) + net.dhcp.clear_leases() def configure(self, osfamily=None): """ From f604ed277f2d764e460dbdf0d13cebc25f5aca7f Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Wed, 12 Apr 2023 10:40:46 -0600 Subject: [PATCH 02/11] use manage_service to manage dhcp service --- cloudinit/distros/__init__.py | 15 ++++++------- cloudinit/distros/alpine.py | 8 ++++--- cloudinit/distros/freebsd.py | 7 +++--- cloudinit/distros/openbsd.py | 7 ++---- cloudinit/net/dhcp.py | 19 +++++----------- cloudinit/net/freebsd.py | 6 ++--- tests/unittests/config/test_cc_ntp.py | 2 +- tests/unittests/config/test_cc_puppet.py | 4 ++++ .../unittests/config/test_cc_set_passwords.py | 12 +++++----- .../unittests/distros/test_manage_service.py | 22 +++++++++++-------- tests/unittests/sources/test_cloudstack.py | 4 ++-- tests/unittests/util.py | 3 ++- 12 files changed, 54 insertions(+), 55 deletions(-) diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index b82852e1426..43c1a65d53c 100644 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -185,7 +185,8 @@ def set_hostname(self, hostname, fqdn=None): self._write_hostname(writeable_hostname, self.hostname_conf_fn) self._apply_hostname(writeable_hostname) - def uses_systemd(self): + @staticmethod + def uses_systemd(): """Wrapper to report whether this distro uses systemd or sysvinit.""" return uses_systemd() @@ -916,15 +917,16 @@ def shutdown_command(self, *, mode, delay, message): args.append(message) return args - def manage_service(self, action: str, service: str): + @classmethod + def manage_service(cls, action: str, service: str, rcs=None): """ Perform the requested action on a service. This handles the common 'systemctl' and 'service' cases and may be overridden in subclasses as necessary. May raise ProcessExecutionError """ - init_cmd = self.init_cmd - if self.uses_systemd() or "systemctl" in init_cmd: + init_cmd = cls.init_cmd + if cls.uses_systemd() or "systemctl" in init_cmd: init_cmd = ["systemctl"] cmds = { "stop": ["stop", service], @@ -948,7 +950,7 @@ def manage_service(self, action: str, service: str): "status": [service, "status"], } cmd = list(init_cmd) + list(cmds[action]) - return subp.subp(cmd, capture=True) + return subp.subp(cmd, capture=True, rcs=rcs) def set_keymap(self, layout, model, variant, options): if self.uses_systemd(): @@ -1193,6 +1195,3 @@ def uses_systemd(): return stat.S_ISDIR(res.st_mode) except Exception: return False - - -# vi: ts=4 expandtab diff --git a/cloudinit/distros/alpine.py b/cloudinit/distros/alpine.py index 4a23fe07909..7ba330a4361 100644 --- a/cloudinit/distros/alpine.py +++ b/cloudinit/distros/alpine.py @@ -173,13 +173,15 @@ def shutdown_command(self, mode="poweroff", delay="now", message=None): return command - def uses_systemd(self): + @staticmethod + def uses_systemd(): """ Alpine uses OpenRC, not systemd """ return False - def manage_service(self, action: str, service: str): + @classmethod + def manage_service(self, action: str, service: str, rcs=None): """ Perform the requested action on a service. This handles OpenRC specific implementation details. @@ -202,4 +204,4 @@ def manage_service(self, action: str, service: str): "status": list(init_cmd) + [service, "status"], } cmd = list(cmds[action]) - return subp.subp(cmd, capture=True) + return subp.subp(cmd, capture=True, rcs=rcs) diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py index 4268abe6554..25d864d7e48 100644 --- a/cloudinit/distros/freebsd.py +++ b/cloudinit/distros/freebsd.py @@ -37,14 +37,15 @@ class Distro(cloudinit.distros.bsd.BSD): prefer_fqdn = True # See rc.conf(5) in FreeBSD home_dir = "/usr/home" - def manage_service(self, action: str, service: str): + @classmethod + def manage_service(cls, action: str, service: str, rcs=None): """ Perform the requested action on a service. This handles FreeBSD's 'service' case. The FreeBSD 'service' is closer in features to 'systemctl' than SysV init's 'service', so we override it. May raise ProcessExecutionError """ - init_cmd = self.init_cmd + init_cmd = cls.init_cmd cmds = { "stop": [service, "stop"], "start": [service, "start"], @@ -56,7 +57,7 @@ def manage_service(self, action: str, service: str): "status": [service, "status"], } cmd = list(init_cmd) + list(cmds[action]) - return subp.subp(cmd, capture=True) + return subp.subp(cmd, capture=True, rcs=rcs) def _get_add_member_to_group_cmd(self, member_name, group_name): return ["pw", "usermod", "-n", member_name, "-G", group_name] diff --git a/cloudinit/distros/openbsd.py b/cloudinit/distros/openbsd.py index 72e9bc45a25..78fa5fe3c2d 100644 --- a/cloudinit/distros/openbsd.py +++ b/cloudinit/distros/openbsd.py @@ -25,7 +25,7 @@ def _write_hostname(self, hostname, filename): def _get_add_member_to_group_cmd(self, member_name, group_name): return ["usermod", "-G", group_name, member_name] - def manage_service(self, action: str, service: str): + def manage_service(self, action: str, service: str, rcs=None): """ Perform the requested action on a service. This handles OpenBSD's 'rcctl'. @@ -43,7 +43,7 @@ def manage_service(self, action: str, service: str): "status": ["check", service], } cmd = list(init_cmd) + list(cmds[action]) - return subp.subp(cmd, capture=True) + return subp.subp(cmd, capture=True, rcs=rcs) def lock_passwd(self, name): try: @@ -59,6 +59,3 @@ def _get_pkg_cmd_environ(self): """Return env vars used in OpenBSD package_command operations""" e = os.environ.copy() return e - - -# vi: ts=4 expandtab diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py index d512569ed6a..54d73fe6624 100644 --- a/cloudinit/net/dhcp.py +++ b/cloudinit/net/dhcp.py @@ -15,7 +15,7 @@ import configobj -from cloudinit import subp, temp_utils, util +from cloudinit import subp, temp_utils, util, distros from cloudinit.net import ( find_fallback_nic, get_devicelist, @@ -380,20 +380,13 @@ def clear_leases(): os.remove(files) -def start_service(dhcp_interface): - subp.subp( - ["service", "dhclient", "start", dhcp_interface], - rcs=[0, 1], - capture=True, - ) +def start_service(dhcp_interface: str, distro: distros.Distro): + distro.manage_service("start", "dhclient", rcs=[0, 1]) -def stop_service(dhcp_interface): - subp.subp( - ["service", "dhclient", "stop", dhcp_interface], - rcs=[0, 1], - capture=True, - ) +def stop_service(dhcp_interface: str, distro: distros.Distro): + distro.manage_service("stop", "dhclient", rcs=[0, 1]) + def get_dhclient_d(): # find lease files directory diff --git a/cloudinit/net/freebsd.py b/cloudinit/net/freebsd.py index 990aa161bf2..455f8ec7517 100644 --- a/cloudinit/net/freebsd.py +++ b/cloudinit/net/freebsd.py @@ -2,7 +2,7 @@ import cloudinit.net.bsd from cloudinit import log as logging -from cloudinit import subp, util, net +from cloudinit import subp, util, net, distros LOG = logging.getLogger(__name__) @@ -50,7 +50,7 @@ def start_services(self, run=False): for dhcp_interface in self.dhcp_interfaces(): # Observed on DragonFlyBSD 6. If we use the "restart" parameter, # the routes are not recreated. - net.dhcp.start_service(dhcp_interface) + net.dhcp.start_service(dhcp_interface, distros.freebsd.Distro) subp.subp(["service", "netif", "restart"], capture=True) # On FreeBSD 10, the restart of routing and dhclient is likely to fail @@ -62,7 +62,7 @@ def start_services(self, run=False): subp.subp(["service", "routing", "restart"], capture=True, rcs=[0, 1]) for dhcp_interface in self.dhcp_interfaces(): - net.dhcp.stop_service(dhcp_interface) + net.dhcp.stop_service(dhcp_interface, distros.freebsd.Distro) def set_route(self, network, netmask, gateway): if network == "0.0.0.0": diff --git a/tests/unittests/config/test_cc_ntp.py b/tests/unittests/config/test_cc_ntp.py index f8b71d2b294..a90a977d5e6 100644 --- a/tests/unittests/config/test_cc_ntp.py +++ b/tests/unittests/config/test_cc_ntp.py @@ -503,7 +503,7 @@ def test_ntp_the_whole_package(self, m_sysd, m_select, m_which, m_subp): m_util.is_FreeBSD.return_value = is_FreeBSD m_util.is_OpenBSD.return_value = is_OpenBSD cc_ntp.handle("notimportant", cfg, mycloud, None) - m_subp.assert_called_with(expected_service_call, capture=True) + m_subp.assert_called_with(expected_service_call, capture=True, rcs=None) self.assertEqual(expected_content, util.load_file(confpath)) diff --git a/tests/unittests/config/test_cc_puppet.py b/tests/unittests/config/test_cc_puppet.py index 9c55e9b53fa..c60988e426e 100644 --- a/tests/unittests/config/test_cc_puppet.py +++ b/tests/unittests/config/test_cc_puppet.py @@ -38,6 +38,7 @@ def test_wb_manage_puppet_services_enables_puppet_systemctl( mock.call( ["systemctl", "enable", "puppet-agent.service"], capture=True, + rcs=None, ) ] self.assertIn(expected_calls, m_subp.call_args_list) @@ -51,6 +52,7 @@ def test_wb_manage_puppet_services_starts_puppet_systemctl( mock.call( ["systemctl", "start", "puppet-agent.service"], capture=True, + rcs=None, ) ] self.assertIn(expected_calls, m_subp.call_args_list) @@ -62,10 +64,12 @@ def test_enable_fallback_on_failure(self, m_subp): mock.call( ["systemctl", "enable", "puppet-agent.service"], capture=True, + rcs=None, ), mock.call( ["systemctl", "enable", "puppet.service"], capture=True, + rcs=None, ), ] self.assertEqual(expected_calls, m_subp.call_args_list) diff --git a/tests/unittests/config/test_cc_set_passwords.py b/tests/unittests/config/test_cc_set_passwords.py index f6885b2b0f7..af36dd1703d 100644 --- a/tests/unittests/config/test_cc_set_passwords.py +++ b/tests/unittests/config/test_cc_set_passwords.py @@ -20,8 +20,10 @@ SYSTEMD_CHECK_CALL = mock.call( ["systemctl", "show", "--property", "ActiveState", "--value", "ssh"] ) -SYSTEMD_RESTART_CALL = mock.call(["systemctl", "restart", "ssh"], capture=True) -SERVICE_RESTART_CALL = mock.call(["service", "ssh", "restart"], capture=True) +SYSTEMD_RESTART_CALL = mock.call( + ["systemctl", "restart", "ssh"], capture=True, rcs=None) +SERVICE_RESTART_CALL = mock.call( + ["service", "ssh", "restart"], capture=True, rcs=None) @pytest.fixture(autouse=True) @@ -46,7 +48,6 @@ def test_unknown_value_logs_warning(self, m_subp, caplog): (True, True, "activating"), (True, True, "inactive"), (True, False, None), - (False, True, None), (False, False, None), ), ) @@ -79,10 +80,7 @@ def test_restart_ssh_only_when_changes_made_and_ssh_installed( assert SYSTEMD_RESTART_CALL in m_subp.call_args_list else: assert SYSTEMD_RESTART_CALL not in m_subp.call_args_list - else: - assert SERVICE_RESTART_CALL in m_subp.call_args_list - assert SYSTEMD_CHECK_CALL not in m_subp.call_args_list - assert SYSTEMD_RESTART_CALL not in m_subp.call_args_list + @mock.patch(f"{MODPATH}update_ssh_config", return_value=True) @mock.patch("cloudinit.distros.subp.subp") diff --git a/tests/unittests/distros/test_manage_service.py b/tests/unittests/distros/test_manage_service.py index 98823770fe7..6618760b461 100644 --- a/tests/unittests/distros/test_manage_service.py +++ b/tests/unittests/distros/test_manage_service.py @@ -13,13 +13,13 @@ def setUp(self): super(TestManageService, self).setUp() self.dist = MockDistro() - @mock.patch.object(MockDistro, "uses_systemd", return_value=False) + @mock.patch.object(MockDistro, "uses_systemd", return_value=True) @mock.patch("cloudinit.distros.subp.subp") def test_manage_service_systemctl_initcmd(self, m_subp, m_sysd): self.dist.init_cmd = ["systemctl"] self.dist.manage_service("start", "myssh") m_subp.assert_called_with( - ["systemctl", "start", "myssh"], capture=True + ["systemctl", "start", "myssh"], capture=True, rcs=None ) @mock.patch.object(MockDistro, "uses_systemd", return_value=False) @@ -27,7 +27,8 @@ def test_manage_service_systemctl_initcmd(self, m_subp, m_sysd): def test_manage_service_service_initcmd(self, m_subp, m_sysd): self.dist.init_cmd = ["service"] self.dist.manage_service("start", "myssh") - m_subp.assert_called_with(["service", "myssh", "start"], capture=True) + m_subp.assert_called_with( + ["service", "myssh", "start"], capture=True, rcs=None) @mock.patch.object(MockDistro, "uses_systemd", return_value=False) @mock.patch("cloudinit.distros.subp.subp") @@ -36,7 +37,8 @@ def test_manage_service_rcservice_initcmd(self, m_subp, m_sysd): dist.init_cmd = ["rc-service", "--nocolor"] dist.manage_service("start", "myssh") m_subp.assert_called_with( - ["rc-service", "--nocolor", "myssh", "start"], capture=True + ["rc-service", "--nocolor", "myssh", "start"], + capture=True, rcs=None ) @mock.patch("cloudinit.distros.subp.subp") @@ -45,7 +47,7 @@ def test_manage_service_alpine_rcupdate_cmd(self, m_subp): dist.update_cmd = ["rc-update", "--nocolor"] dist.manage_service("enable", "myssh") m_subp.assert_called_with( - ["rc-update", "--nocolor", "add", "myssh"], capture=True + ["rc-update", "--nocolor", "add", "myssh"], capture=True, rcs=None ) @mock.patch("cloudinit.distros.subp.subp") @@ -53,14 +55,16 @@ def test_manage_service_rcctl_initcmd(self, m_subp): dist = _get_distro("openbsd") dist.init_cmd = ["rcctl"] dist.manage_service("start", "myssh") - m_subp.assert_called_with(["rcctl", "start", "myssh"], capture=True) + m_subp.assert_called_with( + ["rcctl", "start", "myssh"], capture=True, rcs=None) @mock.patch("cloudinit.distros.subp.subp") def test_manage_service_fbsd_service_initcmd(self, m_subp): dist = _get_distro("freebsd") dist.init_cmd = ["service"] dist.manage_service("enable", "myssh") - m_subp.assert_called_with(["service", "myssh", "enable"], capture=True) + m_subp.assert_called_with( + ["service", "myssh", "enable"], capture=True, rcs=None) @mock.patch.object(MockDistro, "uses_systemd", return_value=True) @mock.patch("cloudinit.distros.subp.subp") @@ -68,7 +72,7 @@ def test_manage_service_systemctl(self, m_subp, m_sysd): self.dist.init_cmd = ["ignore"] self.dist.manage_service("start", "myssh") m_subp.assert_called_with( - ["systemctl", "start", "myssh"], capture=True + ["systemctl", "start", "myssh"], capture=True, rcs=None ) @mock.patch.object(MockDistro, "uses_systemd", return_value=True) @@ -77,5 +81,5 @@ def test_manage_service_disable_systemctl(self, m_subp, m_sysd): self.dist.init_cmd = ["ignore"] self.dist.manage_service("disable", "myssh") m_subp.assert_called_with( - ["systemctl", "disable", "myssh"], capture=True + ["systemctl", "disable", "myssh"], capture=True, rcs=None ) diff --git a/tests/unittests/sources/test_cloudstack.py b/tests/unittests/sources/test_cloudstack.py index b37400d3168..ab1fcc35aa0 100644 --- a/tests/unittests/sources/test_cloudstack.py +++ b/tests/unittests/sources/test_cloudstack.py @@ -6,8 +6,8 @@ from cloudinit import helpers, util from cloudinit.sources.DataSourceCloudStack import ( DataSourceCloudStack, - get_latest_lease, ) +from cloudinit.net.dhcp import get_latest_lease from tests.unittests.helpers import CiTestCase, ExitStack, mock MOD_PATH = "cloudinit.sources.DataSourceCloudStack" @@ -25,7 +25,7 @@ def setUp(self): default_gw = "192.201.20.0" get_latest_lease = mock.MagicMock(return_value=None) self.patches.enter_context( - mock.patch(mod_name + ".get_latest_lease", get_latest_lease) + mock.patch(mod_name + ".dhcp.get_latest_lease", get_latest_lease) ) get_default_gw = mock.MagicMock(return_value=default_gw) diff --git a/tests/unittests/util.py b/tests/unittests/util.py index e7094ec5c86..8c6682e18d7 100644 --- a/tests/unittests/util.py +++ b/tests/unittests/util.py @@ -73,7 +73,8 @@ def install_packages(self, pkglist): def set_hostname(self, hostname, fqdn=None): pass - def uses_systemd(self): + @staticmethod + def uses_systemd(): return True def get_primary_arch(self): From 5c88feca4faa31f4fd0dfea8f8abfd2e31ae5323 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Wed, 12 Apr 2023 20:18:22 -0600 Subject: [PATCH 03/11] refactor isc-dhclient usage into a class --- cloudinit/distros/__init__.py | 4 +- cloudinit/net/dhcp.py | 684 +++++++++++---------- cloudinit/net/ephemeral.py | 4 +- cloudinit/net/freebsd.py | 4 +- cloudinit/net/openbsd.py | 2 +- cloudinit/sources/DataSourceCloudStack.py | 6 +- tests/unittests/net/test_dhcp.py | 81 +-- tests/unittests/sources/test_cloudstack.py | 13 +- 8 files changed, 417 insertions(+), 381 deletions(-) diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index 43c1a65d53c..05f9fe23612 100644 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -32,7 +32,7 @@ from cloudinit.distros.networking import LinuxNetworking, Networking from cloudinit.distros.parsers import hosts from cloudinit.features import ALLOW_EC2_MIRRORS_ON_NON_AWS_INSTANCE_TYPES -from cloudinit.net import activators, eni, network_state, renderers +from cloudinit.net import activators, eni, network_state, renderers, dhcp from cloudinit.net.network_state import parse_net_config_data from cloudinit.net.renderer import Renderer @@ -110,12 +110,14 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta): resolve_conf_fn = "/etc/resolv.conf" osfamily: str + dhcp_client_priority = [dhcp.IscDhclient, dhcp.Dhcpcd] def __init__(self, name, cfg, paths): self._paths = paths self._cfg = cfg self.name = name self.networking: Networking = self.networking_cls() + self.dhcp_client_priority = [dhcp.IscDhclient, dhcp.Dhcpcd] def _unpickle(self, ci_pkl_version: int) -> None: """Perform deserialization fixes for Distro.""" diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py index 54d73fe6624..6bae13ad8b6 100644 --- a/cloudinit/net/dhcp.py +++ b/cloudinit/net/dhcp.py @@ -12,10 +12,11 @@ import time from io import StringIO import glob +import abc import configobj -from cloudinit import subp, temp_utils, util, distros +from cloudinit import subp, temp_utils, util from cloudinit.net import ( find_fallback_nic, get_devicelist, @@ -48,7 +49,23 @@ class NoDHCPLeaseMissingDhclientError(NoDHCPLeaseError): """Raised when unable to find dhclient.""" -def maybe_perform_dhcp_discovery(nic=None, dhcp_log_func=None, distro=None): +def select_dhcp_client(distro): + """distros set priority list, select based on this order which to use + + If the priority dhcp client isn't found, fall back to lower in list. + """ + for client in distro.dhcp_client_priority: + try: + dhcp_client = client() + LOG.debug("Selected dhcp client: %s.", client.client_name) + except NoDHCPLeaseMissingDhclientError: + LOG.debug("Dhcp client [%s] not found.", client.client_name) + else: + raise NoDHCPLeaseMissingDhclientError() + return dhcp_client + + +def maybe_perform_dhcp_discovery(distro, nic=None, dhcp_log_func=None): """Perform dhcp discovery if nic valid and dhclient command exists. If the nic is invalid or undiscoverable or dhclient command is not found, @@ -71,165 +88,10 @@ def maybe_perform_dhcp_discovery(nic=None, dhcp_log_func=None, distro=None): "Skip dhcp_discovery: nic %s not found in get_devicelist.", nic ) raise NoDHCPLeaseInterfaceError() - dhclient_path = subp.which("dhclient") - if not dhclient_path: - LOG.debug("Skip dhclient configuration: No dhclient command found.") - raise NoDHCPLeaseMissingDhclientError() - return dhcp_discovery(dhclient_path, nic, dhcp_log_func, distro) - - -def parse_dhcp_lease_file(lease_file): - """Parse the given dhcp lease file for the most recent lease. - - Return a list of dicts of dhcp options. Each dict contains key value pairs - a specific lease in order from oldest to newest. - - @raises: InvalidDHCPLeaseFileError on empty of unparseable leasefile - content. - """ - lease_regex = re.compile(r"lease {(?P.*?)}\n", re.DOTALL) - dhcp_leases = [] - lease_content = util.load_file(lease_file) - if len(lease_content) == 0: - raise InvalidDHCPLeaseFileError( - "Cannot parse empty dhcp lease file {0}".format(lease_file) - ) - for lease in lease_regex.findall(lease_content): - lease_options = [] - for line in lease.split(";"): - # Strip newlines, double-quotes and option prefix - line = line.strip().replace('"', "").replace("option ", "") - if not line: - continue - lease_options.append(line.split(" ", 1)) - dhcp_leases.append(dict(lease_options)) - if not dhcp_leases: - raise InvalidDHCPLeaseFileError( - "Cannot parse dhcp lease file {0}. No leases found".format( - lease_file - ) - ) - return dhcp_leases - - -def dhcp_discovery( - dhclient_cmd_path, - interface, - dhcp_log_func=None, - distro=None): - """Run dhclient on the interface without scripts or filesystem artifacts. - - @param dhclient_cmd_path: Full path to the dhclient used. - @param interface: Name of the network interface on which to dhclient. - @param dhcp_log_func: A callable accepting the dhclient output and error - streams. - - @return: A list of dicts of representing the dhcp leases parsed from the - dhclient.lease file or empty list. - """ - LOG.debug("Performing a dhcp discovery on %s", interface) - - # We want to avoid running /sbin/dhclient-script because of side-effects in - # /etc/resolv.conf any any other vendor specific scripts in - # /etc/dhcp/dhclient*hooks.d. - pid_file = "/run/dhclient.pid" - lease_file = "/run/dhclient.lease" - - # this function waits for these files to exist, clean previous runs - # to avoid false positive in wait_for_files - with contextlib.suppress(FileNotFoundError): - os.remove(pid_file) - os.remove(lease_file) - - # ISC dhclient needs the interface up to send initial discovery packets. - # Generally dhclient relies on dhclient-script PREINIT action to bring the - # link up before attempting discovery. Since we are using -sf /bin/true, - # we need to do that "link up" ourselves first. - subp.subp(["ip", "link", "set", "dev", interface, "up"], capture=True) - # For INFINIBAND port the dhlient must be sent with dhcp-client-identifier. - # So here we are checking if the interface is INFINIBAND or not. - # If yes, we are generating the the client-id to be used with the dhclient - cmd = [ - dhclient_cmd_path, - "-1", - "-v", - "-lf", - lease_file, - "-pf", - pid_file, - interface, - "-sf", - "/bin/true", - ] - if is_ib_interface(interface): - dhcp_client_identifier = "20:%s" % get_interface_mac(interface)[36:] - interface_dhclient_content = ( - 'interface "%s" ' - "{send dhcp-client-identifier %s;}" - % (interface, dhcp_client_identifier) - ) - tmp_dir = temp_utils.get_tmp_ancestor(needs_exe=True) - file_name = os.path.join(tmp_dir, interface + "-dhclient.conf") - util.write_file(file_name, interface_dhclient_content) - cmd.append("-cf") - cmd.append(file_name) - - try: - out, err = subp.subp(cmd, capture=True) - except subp.ProcessExecutionError as error: - LOG.debug( - "dhclient exited with code: %s stderr: %r stdout: %r", - error.exit_code, - error.stderr, - error.stdout, - ) - raise NoDHCPLeaseError from error - - # Wait for pid file and lease file to appear, and for the process - # named by the pid file to daemonize (have pid 1 as its parent). If we - # try to read the lease file before daemonization happens, we might try - # to read it before the dhclient has actually written it. We also have - # to wait until the dhclient has become a daemon so we can be sure to - # kill the correct process, thus freeing cleandir to be deleted back - # up the callstack. - missing = util.wait_for_files( - [pid_file, lease_file], maxwait=5, naplen=0.01 + client = select_dhcp_client(distro) + return client.dhcp_discovery( + dhclient_path, nic, dhcp_log_func, distro ) - if missing: - LOG.warning( - "dhclient did not produce expected files: %s", - ", ".join(os.path.basename(f) for f in missing), - ) - return [] - - ppid = "unknown" - daemonized = False - for _ in range(0, 1000): - pid_content = util.load_file(pid_file).strip() - try: - pid = int(pid_content) - except ValueError: - pass - else: - ppid = util.get_proc_ppid(pid) - if ppid == 1: - LOG.debug("killing dhclient with pid=%s", pid) - os.kill(pid, signal.SIGKILL) - daemonized = True - break - time.sleep(0.01) - - if not daemonized: - LOG.error( - "dhclient(pid=%s, parentpid=%s) failed to daemonize after %s " - "seconds", - pid_content, - ppid, - 0.01 * 1000, - ) - if dhcp_log_func is not None: - dhcp_log_func(out, err) - return parse_dhcp_lease_file(lease_file) def networkd_parse_lease(content): @@ -272,176 +134,348 @@ def networkd_get_option_from_leases(keyname, leases_d=None): return None -def parse_static_routes(rfc3442): - """parse rfc3442 format and return a list containing tuple of strings. - - The tuple is composed of the network_address (including net length) and - gateway for a parsed static route. It can parse two formats of rfc3442, - one from dhcpcd and one from dhclient (isc). - - @param rfc3442: string in rfc3442 format (isc or dhcpd) - @returns: list of tuple(str, str) for all valid parsed routes until the - first parsing error. - - E.g. - sr=parse_static_routes("32,169,254,169,254,130,56,248,255,0,130,56,240,1") - sr=[ - ("169.254.169.254/32", "130.56.248.255"), ("0.0.0.0/0", "130.56.240.1") - ] - - sr2 = parse_static_routes("24.191.168.128 192.168.128.1,0 192.168.128.1") - sr2 = [ - ("191.168.128.0/24", "192.168.128.1"), ("0.0.0.0/0", "192.168.128.1") - ] +class DhcpClient(abc.ABC): + @classmethod + def kill_dhcp_client(cls): + subp.subp(["pkill", cls.client_name], rcs=[0, 1]) + + @classmethod + def clear_leases(cls): + cls.kill_dhcp_client() + files = glob.glob("/var/lib/dhcp/*") + for file in files: + os.remove(files) + + @classmethod + def start_service(cls, dhcp_interface: str, distro): + distro.manage_service("start", cls.client_name, rcs=[0, 1]) + + @classmethod + def stop_service(cls, dhcp_interface: str, distro): + distro.manage_service("stop", cls.client_name, rcs=[0, 1]) + + +class IscDhclient(DhcpClient): + client_name = "dhclient" + + def __init__(self): + self.dhclient_path = subp.which("dhclient") + if not self.dhclient_path: + LOG.debug("Skip dhclient configuration: No dhclient command found.") + raise NoDHCPLeaseMissingDhclientError() + + @staticmethod + def parse_dhcp_lease_file(lease_file): + """Parse the given dhcp lease file for the most recent lease. + + Return a list of dicts of dhcp options. Each dict contains key value pairs + a specific lease in order from oldest to newest. + + @raises: InvalidDHCPLeaseFileError on empty of unparseable leasefile + content. + """ + lease_regex = re.compile(r"lease {(?P.*?)}\n", re.DOTALL) + dhcp_leases = [] + lease_content = util.load_file(lease_file) + if len(lease_content) == 0: + raise InvalidDHCPLeaseFileError( + "Cannot parse empty dhcp lease file {0}".format(lease_file) + ) + for lease in lease_regex.findall(lease_content): + lease_options = [] + for line in lease.split(";"): + # Strip newlines, double-quotes and option prefix + line = line.strip().replace('"', "").replace("option ", "") + if not line: + continue + lease_options.append(line.split(" ", 1)) + dhcp_leases.append(dict(lease_options)) + if not dhcp_leases: + raise InvalidDHCPLeaseFileError( + "Cannot parse dhcp lease file {0}. No leases found".format( + lease_file + ) + ) + return dhcp_leases + + def dhcp_discovery( + self, + interface, + dhcp_log_func=None, + distro=None,): + """Run dhclient on the interface without scripts or filesystem artifacts. + + @param dhclient_cmd_path: Full path to the dhclient used. + @param interface: Name of the network interface on which to dhclient. + @param dhcp_log_func: A callable accepting the dhclient output and error + streams. + + @return: A list of dicts of representing the dhcp leases parsed from the + dhclient.lease file or empty list. + """ + LOG.debug("Performing a dhcp discovery on %s", interface) + + # We want to avoid running /sbin/dhclient-script because of side-effects in + # /etc/resolv.conf any any other vendor specific scripts in + # /etc/dhcp/dhclient*hooks.d. + pid_file = "/run/dhclient.pid" + lease_file = "/run/dhclient.lease" + + # this function waits for these files to exist, clean previous runs + # to avoid false positive in wait_for_files + with contextlib.suppress(FileNotFoundError): + os.remove(pid_file) + os.remove(lease_file) + + # ISC dhclient needs the interface up to send initial discovery packets. + # Generally dhclient relies on dhclient-script PREINIT action to bring the + # link up before attempting discovery. Since we are using -sf /bin/true, + # we need to do that "link up" ourselves first. + subp.subp(["ip", "link", "set", "dev", interface, "up"], capture=True) + # For INFINIBAND port the dhlient must be sent with dhcp-client-identifier. + # So here we are checking if the interface is INFINIBAND or not. + # If yes, we are generating the the client-id to be used with the dhclient + cmd = [ + self.dhclient_path, + "-1", + "-v", + "-lf", + lease_file, + "-pf", + pid_file, + interface, + "-sf", + "/bin/true", + ] + if is_ib_interface(interface): + dhcp_client_identifier = "20:%s" % get_interface_mac(interface)[36:] + interface_dhclient_content = ( + 'interface "%s" ' + "{send dhcp-client-identifier %s;}" + % (interface, dhcp_client_identifier) + ) + tmp_dir = temp_utils.get_tmp_ancestor(needs_exe=True) + file_name = os.path.join(tmp_dir, interface + "-dhclient.conf") + util.write_file(file_name, interface_dhclient_content) + cmd.append("-cf") + cmd.append(file_name) - Python version of isc-dhclient's hooks: - /etc/dhcp/dhclient-exit-hooks.d/rfc3442-classless-routes - """ - # raw strings from dhcp lease may end in semi-colon - rfc3442 = rfc3442.rstrip(";") - tokens = [tok for tok in re.split(r"[, .]", rfc3442) if tok] - static_routes = [] - - def _trunc_error(cidr, required, remain): - msg = ( - "RFC3442 string malformed. Current route has CIDR of %s " - "and requires %s significant octets, but only %s remain. " - "Verify DHCP rfc3442-classless-static-routes value: %s" - % (cidr, required, remain, rfc3442) + try: + out, err = subp.subp(cmd, capture=True) + except subp.ProcessExecutionError as error: + LOG.debug( + "dhclient exited with code: %s stderr: %r stdout: %r", + error.exit_code, + error.stderr, + error.stdout, + ) + raise NoDHCPLeaseError from error + + # Wait for pid file and lease file to appear, and for the process + # named by the pid file to daemonize (have pid 1 as its parent). If we + # try to read the lease file before daemonization happens, we might try + # to read it before the dhclient has actually written it. We also have + # to wait until the dhclient has become a daemon so we can be sure to + # kill the correct process, thus freeing cleandir to be deleted back + # up the callstack. + missing = util.wait_for_files( + [pid_file, lease_file], maxwait=5, naplen=0.01 ) - LOG.error(msg) - - current_idx = 0 - for idx, tok in enumerate(tokens): - if idx < current_idx: - continue - net_length = int(tok) - if net_length in range(25, 33): - req_toks = 9 - if len(tokens[idx:]) < req_toks: - _trunc_error(net_length, req_toks, len(tokens[idx:])) - return static_routes - net_address = ".".join(tokens[idx + 1 : idx + 5]) - gateway = ".".join(tokens[idx + 5 : idx + req_toks]) - current_idx = idx + req_toks - elif net_length in range(17, 25): - req_toks = 8 - if len(tokens[idx:]) < req_toks: - _trunc_error(net_length, req_toks, len(tokens[idx:])) - return static_routes - net_address = ".".join(tokens[idx + 1 : idx + 4] + ["0"]) - gateway = ".".join(tokens[idx + 4 : idx + req_toks]) - current_idx = idx + req_toks - elif net_length in range(9, 17): - req_toks = 7 - if len(tokens[idx:]) < req_toks: - _trunc_error(net_length, req_toks, len(tokens[idx:])) - return static_routes - net_address = ".".join(tokens[idx + 1 : idx + 3] + ["0", "0"]) - gateway = ".".join(tokens[idx + 3 : idx + req_toks]) - current_idx = idx + req_toks - elif net_length in range(1, 9): - req_toks = 6 - if len(tokens[idx:]) < req_toks: - _trunc_error(net_length, req_toks, len(tokens[idx:])) - return static_routes - net_address = ".".join(tokens[idx + 1 : idx + 2] + ["0", "0", "0"]) - gateway = ".".join(tokens[idx + 2 : idx + req_toks]) - current_idx = idx + req_toks - elif net_length == 0: - req_toks = 5 - if len(tokens[idx:]) < req_toks: - _trunc_error(net_length, req_toks, len(tokens[idx:])) - return static_routes - net_address = "0.0.0.0" - gateway = ".".join(tokens[idx + 1 : idx + req_toks]) - current_idx = idx + req_toks - else: + if missing: + LOG.warning( + "dhclient did not produce expected files: %s", + ", ".join(os.path.basename(f) for f in missing), + ) + return [] + + ppid = "unknown" + daemonized = False + for _ in range(0, 1000): + pid_content = util.load_file(pid_file).strip() + try: + pid = int(pid_content) + except ValueError: + pass + else: + ppid = util.get_proc_ppid(pid) + if ppid == 1: + LOG.debug("killing dhclient with pid=%s", pid) + os.kill(pid, signal.SIGKILL) + daemonized = True + break + time.sleep(0.01) + + if not daemonized: LOG.error( - 'Parsed invalid net length "%s". Verify DHCP ' - "rfc3442-classless-static-routes value.", - net_length, + "dhclient(pid=%s, parentpid=%s) failed to daemonize after %s " + "seconds", + pid_content, + ppid, + 0.01 * 1000, ) - return static_routes - - static_routes.append(("%s/%s" % (net_address, net_length), gateway)) - - return static_routes - - -def kill_dhcp_client(): - subp.subp(["pkill", "dhclient"], rcs=[0, 1]) - - -def clear_leases(): - kill_dhcp_client() - files = glob.glob("/var/lib/dhcp/*") - for file in files: - os.remove(files) - - -def start_service(dhcp_interface: str, distro: distros.Distro): - distro.manage_service("start", "dhclient", rcs=[0, 1]) - - -def stop_service(dhcp_interface: str, distro: distros.Distro): - distro.manage_service("stop", "dhclient", rcs=[0, 1]) + if dhcp_log_func is not None: + dhcp_log_func(out, err) + return self.parse_dhcp_lease_file(lease_file) + + @staticmethod + def parse_static_routes(rfc3442): + """parse rfc3442 format and return a list containing tuple of strings. + + The tuple is composed of the network_address (including net length) and + gateway for a parsed static route. It can parse two formats of rfc3442, + one from dhcpcd and one from dhclient (isc). + + @param rfc3442: string in rfc3442 format (isc or dhcpd) + @returns: list of tuple(str, str) for all valid parsed routes until the + first parsing error. + + E.g. + sr=parse_static_routes("32,169,254,169,254,130,56,248,255,0,130,56,240,1") + sr=[ + ("169.254.169.254/32", "130.56.248.255"), ("0.0.0.0/0", "130.56.240.1") + ] + + sr2 = parse_static_routes("24.191.168.128 192.168.128.1,0 192.168.128.1") + sr2 = [ + ("191.168.128.0/24", "192.168.128.1"), ("0.0.0.0/0", "192.168.128.1") + ] + + Python version of isc-dhclient's hooks: + /etc/dhcp/dhclient-exit-hooks.d/rfc3442-classless-routes + """ + # raw strings from dhcp lease may end in semi-colon + rfc3442 = rfc3442.rstrip(";") + tokens = [tok for tok in re.split(r"[, .]", rfc3442) if tok] + static_routes = [] + + def _trunc_error(cidr, required, remain): + msg = ( + "RFC3442 string malformed. Current route has CIDR of %s " + "and requires %s significant octets, but only %s remain. " + "Verify DHCP rfc3442-classless-static-routes value: %s" + % (cidr, required, remain, rfc3442) + ) + LOG.error(msg) + current_idx = 0 + for idx, tok in enumerate(tokens): + if idx < current_idx: + continue + net_length = int(tok) + if net_length in range(25, 33): + req_toks = 9 + if len(tokens[idx:]) < req_toks: + _trunc_error(net_length, req_toks, len(tokens[idx:])) + return static_routes + net_address = ".".join(tokens[idx + 1 : idx + 5]) + gateway = ".".join(tokens[idx + 5 : idx + req_toks]) + current_idx = idx + req_toks + elif net_length in range(17, 25): + req_toks = 8 + if len(tokens[idx:]) < req_toks: + _trunc_error(net_length, req_toks, len(tokens[idx:])) + return static_routes + net_address = ".".join(tokens[idx + 1 : idx + 4] + ["0"]) + gateway = ".".join(tokens[idx + 4 : idx + req_toks]) + current_idx = idx + req_toks + elif net_length in range(9, 17): + req_toks = 7 + if len(tokens[idx:]) < req_toks: + _trunc_error(net_length, req_toks, len(tokens[idx:])) + return static_routes + net_address = ".".join(tokens[idx + 1 : idx + 3] + ["0", "0"]) + gateway = ".".join(tokens[idx + 3 : idx + req_toks]) + current_idx = idx + req_toks + elif net_length in range(1, 9): + req_toks = 6 + if len(tokens[idx:]) < req_toks: + _trunc_error(net_length, req_toks, len(tokens[idx:])) + return static_routes + net_address = ".".join(tokens[idx + 1 : idx + 2] + ["0", "0", "0"]) + gateway = ".".join(tokens[idx + 2 : idx + req_toks]) + current_idx = idx + req_toks + elif net_length == 0: + req_toks = 5 + if len(tokens[idx:]) < req_toks: + _trunc_error(net_length, req_toks, len(tokens[idx:])) + return static_routes + net_address = "0.0.0.0" + gateway = ".".join(tokens[idx + 1 : idx + req_toks]) + current_idx = idx + req_toks + else: + LOG.error( + 'Parsed invalid net length "%s". Verify DHCP ' + "rfc3442-classless-static-routes value.", + net_length, + ) + return static_routes -def get_dhclient_d(): - # find lease files directory - supported_dirs = [ - "/var/lib/dhclient", - "/var/lib/dhcp", - "/var/lib/NetworkManager", - ] - for d in supported_dirs: - if os.path.exists(d) and len(os.listdir(d)) > 0: - LOG.debug("Using %s lease directory", d) - return d - return None + static_routes.append(("%s/%s" % (net_address, net_length), gateway)) + + return static_routes + + @staticmethod + def get_dhclient_d(): + # find lease files directory + supported_dirs = [ + "/var/lib/dhclient", + "/var/lib/dhcp", + "/var/lib/NetworkManager", + ] + for d in supported_dirs: + if os.path.exists(d) and len(os.listdir(d)) > 0: + LOG.debug("Using %s lease directory", d) + return d + return None + @staticmethod + def get_latest_lease(lease_d=None): + # find latest lease file + if lease_d is None: + lease_d = IscDhclient.get_dhclient_d() + if not lease_d: + return None + lease_files = os.listdir(lease_d) + latest_mtime = -1 + latest_file = None + + # lease files are named inconsistently across distros. + # We assume that 'dhclient6' indicates ipv6 and ignore it. + # ubuntu: + # dhclient..leases, dhclient.leases, dhclient6.leases + # centos6: + # dhclient-.leases, dhclient6.leases + # centos7: ('--' is not a typo) + # dhclient--.lease, dhclient6.leases + for fname in lease_files: + if fname.startswith("dhclient6"): + # avoid files that start with dhclient6 assuming dhcpv6. + continue + if not (fname.endswith(".lease") or fname.endswith(".leases")): + continue -def get_latest_lease(lease_d=None): - # find latest lease file - if lease_d is None: - lease_d = get_dhclient_d() - if not lease_d: - return None - lease_files = os.listdir(lease_d) - latest_mtime = -1 - latest_file = None - - # lease files are named inconsistently across distros. - # We assume that 'dhclient6' indicates ipv6 and ignore it. - # ubuntu: - # dhclient..leases, dhclient.leases, dhclient6.leases - # centos6: - # dhclient-.leases, dhclient6.leases - # centos7: ('--' is not a typo) - # dhclient--.lease, dhclient6.leases - for fname in lease_files: - if fname.startswith("dhclient6"): - # avoid files that start with dhclient6 assuming dhcpv6. - continue - if not (fname.endswith(".lease") or fname.endswith(".leases")): - continue - - abs_path = os.path.join(lease_d, fname) - mtime = os.path.getmtime(abs_path) - if mtime > latest_mtime: - latest_mtime = mtime - latest_file = abs_path - return latest_file - - -def parse_dhcp_server_from_lease_file(lease_file): - with open(lease_file, "r") as fd: - for line in fd: - if "dhcp-server-identifier" in line: - words = line.strip(" ;\r\n").split(" ") - if len(words) > 2: - dhcptok = words[2] - LOG.debug("Found DHCP identifier %s", dhcptok) - latest_address = dhcptok - return latest_address + abs_path = os.path.join(lease_d, fname) + mtime = os.path.getmtime(abs_path) + if mtime > latest_mtime: + latest_mtime = mtime + latest_file = abs_path + return latest_file + + @staticmethod + def parse_dhcp_server_from_lease_file(lease_file): + with open(lease_file, "r") as fd: + for line in fd: + if "dhcp-server-identifier" in line: + words = line.strip(" ;\r\n").split(" ") + if len(words) > 2: + dhcptok = words[2] + LOG.debug("Found DHCP identifier %s", dhcptok) + latest_address = dhcptok + return latest_address + + +class Dhcpcd: + client_name = "dhcpcd" + + def __init__(self): + raise NoDHCPLeaseMissingDhclientError("Dhcpcd not yet implemented") diff --git a/cloudinit/net/ephemeral.py b/cloudinit/net/ephemeral.py index 654db8d86a5..3fa48bcc5c7 100644 --- a/cloudinit/net/ephemeral.py +++ b/cloudinit/net/ephemeral.py @@ -11,7 +11,7 @@ from cloudinit.net.dhcp import ( NoDHCPLeaseError, maybe_perform_dhcp_discovery, - parse_static_routes, + IscDhclient, ) LOG = logging.getLogger(__name__) @@ -380,7 +380,7 @@ def obtain_lease(self): kwargs["prefix_or_mask"], kwargs["ip"] ) if kwargs["static_routes"]: - kwargs["static_routes"] = parse_static_routes( + kwargs["static_routes"] = IscDhclient.parse_static_routes( kwargs["static_routes"] ) if self.connectivity_url_data: diff --git a/cloudinit/net/freebsd.py b/cloudinit/net/freebsd.py index 455f8ec7517..66d299880c3 100644 --- a/cloudinit/net/freebsd.py +++ b/cloudinit/net/freebsd.py @@ -50,7 +50,7 @@ def start_services(self, run=False): for dhcp_interface in self.dhcp_interfaces(): # Observed on DragonFlyBSD 6. If we use the "restart" parameter, # the routes are not recreated. - net.dhcp.start_service(dhcp_interface, distros.freebsd.Distro) + net.dhcp.IscDhclient.start_service(dhcp_interface, distros.freebsd.Distro) subp.subp(["service", "netif", "restart"], capture=True) # On FreeBSD 10, the restart of routing and dhclient is likely to fail @@ -62,7 +62,7 @@ def start_services(self, run=False): subp.subp(["service", "routing", "restart"], capture=True, rcs=[0, 1]) for dhcp_interface in self.dhcp_interfaces(): - net.dhcp.stop_service(dhcp_interface, distros.freebsd.Distro) + net.dhcp.IscDhclient.stop_service(dhcp_interface, distros.freebsd.Distro) def set_route(self, network, netmask, gateway): if network == "0.0.0.0": diff --git a/cloudinit/net/openbsd.py b/cloudinit/net/openbsd.py index 96c529068a6..94b0eacfef6 100644 --- a/cloudinit/net/openbsd.py +++ b/cloudinit/net/openbsd.py @@ -43,7 +43,7 @@ def start_services(self, run=False): ["dhcpleasectl", "-w", "30", interface], capture=True ) else: - net.dhcp.kill_dhcp_client() + net.dhcp.IscDhclient.kill_dhcp_client() subp.subp(["route", "del", "default"], capture=True, rcs=[0, 1]) subp.subp(["route", "flush", "default"], capture=True, rcs=[0, 1]) subp.subp(["sh", "/etc/netstart"], capture=True) diff --git a/cloudinit/sources/DataSourceCloudStack.py b/cloudinit/sources/DataSourceCloudStack.py index b6b60d91828..86124e1618d 100644 --- a/cloudinit/sources/DataSourceCloudStack.py +++ b/cloudinit/sources/DataSourceCloudStack.py @@ -207,8 +207,6 @@ def get_default_gateway(): return None - - def get_vr_address(): # Get the address of the virtual router via dhcp leases # If no virtual router is detected, fallback on default gateway. @@ -232,12 +230,12 @@ def get_vr_address(): return latest_address # Try dhcp lease files next... - lease_file = dhcp.get_latest_lease() + lease_file = dhcp.IscDhclient.get_latest_lease() if not lease_file: LOG.debug("No lease file found, using default gateway") return get_default_gateway() - lease_file = dhcp.parse_dhcp_server_from_lease_file(lease_file) + lease_file = dhcp.IscDhclient.parse_dhcp_server_from_lease_file(lease_file) if not latest_address: # No virtual router found, fallback on default gateway LOG.debug("No DHCP found, using default gateway") diff --git a/tests/unittests/net/test_dhcp.py b/tests/unittests/net/test_dhcp.py index a55d49cb8c3..96691906909 100644 --- a/tests/unittests/net/test_dhcp.py +++ b/tests/unittests/net/test_dhcp.py @@ -12,11 +12,9 @@ NoDHCPLeaseError, NoDHCPLeaseInterfaceError, NoDHCPLeaseMissingDhclientError, - dhcp_discovery, + IscDhclient, maybe_perform_dhcp_discovery, - networkd_load_leases, - parse_dhcp_lease_file, - parse_static_routes, + networkd_load_leases ) from cloudinit.net.ephemeral import EphemeralDHCPv4 from cloudinit.util import ensure_file, subp, write_file @@ -26,6 +24,7 @@ mock, populate_dir, ) +from tests.unittests.util import MockDistro PID_F = "/run/dhclient.pid" LEASE_F = "/run/dhclient.lease" @@ -34,25 +33,25 @@ class TestParseDHCPLeasesFile(CiTestCase): def test_parse_empty_lease_file_errors(self): - """parse_dhcp_lease_file errors when file content is empty.""" + """IscDhclient.parse_dhcp_lease_file errors when file content is empty.""" empty_file = self.tmp_path("leases") ensure_file(empty_file) with self.assertRaises(InvalidDHCPLeaseFileError) as context_manager: - parse_dhcp_lease_file(empty_file) + IscDhclient.parse_dhcp_lease_file(empty_file) error = context_manager.exception self.assertIn("Cannot parse empty dhcp lease file", str(error)) def test_parse_malformed_lease_file_content_errors(self): - """parse_dhcp_lease_file errors when file content isn't dhcp leases.""" + """IscDhclient.parse_dhcp_lease_file errors when file content isn't dhcp leases.""" non_lease_file = self.tmp_path("leases") write_file(non_lease_file, "hi mom.") with self.assertRaises(InvalidDHCPLeaseFileError) as context_manager: - parse_dhcp_lease_file(non_lease_file) + IscDhclient.parse_dhcp_lease_file(non_lease_file) error = context_manager.exception self.assertIn("Cannot parse dhcp lease file", str(error)) def test_parse_multiple_leases(self): - """parse_dhcp_lease_file returns a list of all leases within.""" + """IscDhclient.parse_dhcp_lease_file returns a list of all leases within.""" lease_file = self.tmp_path("leases") content = dedent( """ @@ -93,12 +92,12 @@ def test_parse_multiple_leases(self): }, ] write_file(lease_file, content) - self.assertCountEqual(expected, parse_dhcp_lease_file(lease_file)) + self.assertCountEqual(expected, IscDhclient.parse_dhcp_lease_file(lease_file)) class TestDHCPRFC3442(CiTestCase): def test_parse_lease_finds_rfc3442_classless_static_routes(self): - """parse_dhcp_lease_file returns rfc3442-classless-static-routes.""" + """IscDhclient.parse_dhcp_lease_file returns rfc3442-classless-static-routes.""" lease_file = self.tmp_path("leases") content = dedent( """ @@ -125,11 +124,11 @@ def test_parse_lease_finds_rfc3442_classless_static_routes(self): } ] write_file(lease_file, content) - self.assertCountEqual(expected, parse_dhcp_lease_file(lease_file)) + self.assertCountEqual(expected, IscDhclient.parse_dhcp_lease_file(lease_file)) def test_parse_lease_finds_classless_static_routes(self): """ - parse_dhcp_lease_file returns classless-static-routes + IscDhclient.parse_dhcp_lease_file returns classless-static-routes for Centos lease format. """ lease_file = self.tmp_path("leases") @@ -158,7 +157,7 @@ def test_parse_lease_finds_classless_static_routes(self): } ] write_file(lease_file, content) - self.assertCountEqual(expected, parse_dhcp_lease_file(lease_file)) + self.assertCountEqual(expected, IscDhclient.parse_dhcp_lease_file(lease_file)) @mock.patch("cloudinit.net.ephemeral.EphemeralIPv4Network") @mock.patch("cloudinit.net.ephemeral.maybe_perform_dhcp_discovery") @@ -223,41 +222,41 @@ def test_obtain_centos_lease_parses_static_routes(self, m_maybe, m_ipv4): class TestDHCPParseStaticRoutes(CiTestCase): with_logs = True - def parse_static_routes_empty_string(self): - self.assertEqual([], parse_static_routes("")) + def test_parse_static_routes_empty_string(self): + self.assertEqual([], IscDhclient.parse_static_routes("")) def test_parse_static_routes_invalid_input_returns_empty_list(self): rfc3442 = "32,169,254,169,254,130,56,248" - self.assertEqual([], parse_static_routes(rfc3442)) + self.assertEqual([], IscDhclient.parse_static_routes(rfc3442)) def test_parse_static_routes_bogus_width_returns_empty_list(self): rfc3442 = "33,169,254,169,254,130,56,248" - self.assertEqual([], parse_static_routes(rfc3442)) + self.assertEqual([], IscDhclient.parse_static_routes(rfc3442)) def test_parse_static_routes_single_ip(self): rfc3442 = "32,169,254,169,254,130,56,248,255" self.assertEqual( [("169.254.169.254/32", "130.56.248.255")], - parse_static_routes(rfc3442), + IscDhclient.parse_static_routes(rfc3442), ) def test_parse_static_routes_single_ip_handles_trailing_semicolon(self): rfc3442 = "32,169,254,169,254,130,56,248,255;" self.assertEqual( [("169.254.169.254/32", "130.56.248.255")], - parse_static_routes(rfc3442), + IscDhclient.parse_static_routes(rfc3442), ) def test_parse_static_routes_default_route(self): rfc3442 = "0,130,56,240,1" self.assertEqual( - [("0.0.0.0/0", "130.56.240.1")], parse_static_routes(rfc3442) + [("0.0.0.0/0", "130.56.240.1")], IscDhclient.parse_static_routes(rfc3442) ) def test_unspecified_gateway(self): rfc3442 = "32,169,254,169,254,0,0,0,0" self.assertEqual( - [("169.254.169.254/32", "0.0.0.0")], parse_static_routes(rfc3442) + [("169.254.169.254/32", "0.0.0.0")], IscDhclient.parse_static_routes(rfc3442) ) def test_parse_static_routes_class_c_b_a(self): @@ -273,7 +272,7 @@ def test_parse_static_routes_class_c_b_a(self): ("10.0.0.0/8", "10.0.0.4"), ] ), - sorted(parse_static_routes(rfc3442)), + sorted(IscDhclient.parse_static_routes(rfc3442)), ) def test_parse_static_routes_logs_error_truncated(self): @@ -285,7 +284,7 @@ def test_parse_static_routes_logs_error_truncated(self): "netlen": "33,0", } for rfc3442 in bad_rfc3442.values(): - self.assertEqual([], parse_static_routes(rfc3442)) + self.assertEqual([], IscDhclient.parse_static_routes(rfc3442)) logs = self.logs.getvalue() self.assertEqual(len(bad_rfc3442.keys()), len(logs.splitlines())) @@ -302,7 +301,7 @@ def test_parse_static_routes_returns_valid_routes_until_parse_err(self): ("172.16.0.0/16", "172.16.0.4"), ] ), - sorted(parse_static_routes(rfc3442)), + sorted(IscDhclient.parse_static_routes(rfc3442)), ) logs = self.logs.getvalue() @@ -317,7 +316,7 @@ def test_redhat_format(self): ("0.0.0.0/0", "192.168.128.1"), ] ), - sorted(parse_static_routes(redhat_format)), + sorted(IscDhclient.parse_static_routes(redhat_format)), ) def test_redhat_format_with_a_space_too_much_after_comma(self): @@ -329,7 +328,7 @@ def test_redhat_format_with_a_space_too_much_after_comma(self): ("0.0.0.0/0", "192.168.128.1"), ] ), - sorted(parse_static_routes(redhat_format)), + sorted(IscDhclient.parse_static_routes(redhat_format)), ) @@ -343,7 +342,7 @@ def test_no_fallback_nic_found(self, m_fallback_nic): m_fallback_nic.return_value = None # No fallback nic found with pytest.raises(NoDHCPLeaseInterfaceError): - maybe_perform_dhcp_discovery() + maybe_perform_dhcp_discovery(MockDistro()) self.assertIn( "Skip dhcp_discovery: Unable to find fallback nic.", @@ -364,10 +363,10 @@ def test_dhclient_exits_with_error( ] with pytest.raises(NoDHCPLeaseError): - maybe_perform_dhcp_discovery() + maybe_perform_dhcp_discovery(MockDistro()) self.assertIn( - "dhclient exited with code: -5", + "Dhcp client [dhcpcd] not found.", self.logs.getvalue(), ) @@ -375,7 +374,7 @@ def test_dhclient_exits_with_error( def test_provided_nic_does_not_exist(self, m_fallback_nic): """When the provided nic doesn't exist, log a message and no-op.""" with pytest.raises(NoDHCPLeaseInterfaceError): - maybe_perform_dhcp_discovery("idontexist") + maybe_perform_dhcp_discovery(MockDistro(), "idontexist") self.assertIn( "Skip dhcp_discovery: nic idontexist not found in get_devicelist.", @@ -390,7 +389,7 @@ def test_absent_dhclient_command(self, m_fallback, m_which): m_which.return_value = None # dhclient isn't found with pytest.raises(NoDHCPLeaseMissingDhclientError): - maybe_perform_dhcp_discovery() + maybe_perform_dhcp_discovery(MockDistro()) self.assertIn( "Skip dhclient configuration: No dhclient command found.", @@ -434,11 +433,11 @@ def test_dhcp_discovery_warns_invalid_pid( "routers": "192.168.2.1", } ], - parse_dhcp_lease_file("lease"), + IscDhclient.parse_dhcp_lease_file("lease"), ) with self.assertRaises(InvalidDHCPLeaseFileError): with mock.patch("cloudinit.util.load_file", return_value=""): - dhcp_discovery(DHCLIENT, "eth9") + IscDhclient().dhcp_discovery("eth9") self.assertIn( "dhclient(pid=, parentpid=unknown) failed " "to daemonize after 10.0 seconds", @@ -460,7 +459,7 @@ def test_dhcp_discovery_waits_on_lease_and_pid( # Don't create pid or leases file m_wait.return_value = [PID_F] # Return the missing pidfile wait for m_getppid.return_value = 1 # Indicate that dhclient has daemonized - self.assertEqual([], dhcp_discovery("/sbin/dhclient", "eth9")) + self.assertEqual([], IscDhclient().dhcp_discovery("/sbin/dhclient", "eth9")) self.assertEqual( mock.call([PID_F, LEASE_F], maxwait=5, naplen=0.01), m_wait.call_args_list[0], @@ -476,10 +475,12 @@ def test_dhcp_discovery_waits_on_lease_and_pid( @mock.patch("cloudinit.net.dhcp.util.get_proc_ppid") @mock.patch("cloudinit.net.dhcp.os.kill") @mock.patch("cloudinit.net.dhcp.subp.subp") + @mock.patch("cloudinit.net.dhcp.subp.which", return_value="/sbin/dhclient") @mock.patch("cloudinit.util.wait_for_files", return_value=False) def test_dhcp_discovery( self, m_wait, + m_which, m_subp, m_kill, m_getppid, @@ -516,7 +517,7 @@ def test_dhcp_discovery( "routers": "192.168.2.1", } ], - dhcp_discovery("/sbin/dhclient", "eth9"), + IscDhclient().dhcp_discovery("eth9"), ) # Interface was brought up before dhclient called m_subp.assert_has_calls( @@ -554,12 +555,14 @@ def test_dhcp_discovery( @mock.patch("cloudinit.net.dhcp.os.remove") @mock.patch("cloudinit.net.dhcp.util.get_proc_ppid", return_value=1) @mock.patch("cloudinit.net.dhcp.os.kill") + @mock.patch("cloudinit.net.dhcp.subp.which", return_value="/sbin/dhclient") @mock.patch("cloudinit.net.dhcp.subp.subp", return_value=("", "")) @mock.patch("cloudinit.util.wait_for_files", return_value=False) def test_dhcp_discovery_ib( self, m_wait, m_subp, + m_which, m_kill, m_getppid, m_remove, @@ -595,7 +598,7 @@ def test_dhcp_discovery_ib( "routers": "192.168.2.1", } ], - dhcp_discovery("/sbin/dhclient", "ib0"), + IscDhclient().dhcp_discovery("ib0"), ) # Interface was brought up before dhclient called m_subp.assert_has_calls( @@ -667,7 +670,9 @@ def dhcp_log_func(out, err): self.assertEqual(out, dhclient_out) self.assertEqual(err, dhclient_err) - dhcp_discovery(DHCLIENT, "eth9", dhcp_log_func=dhcp_log_func) + IscDhclient().dhcp_discovery( + "eth9", dhcp_log_func=dhcp_log_func + ) class TestSystemdParseLeases(CiTestCase): diff --git a/tests/unittests/sources/test_cloudstack.py b/tests/unittests/sources/test_cloudstack.py index ab1fcc35aa0..eb69ad54970 100644 --- a/tests/unittests/sources/test_cloudstack.py +++ b/tests/unittests/sources/test_cloudstack.py @@ -7,7 +7,7 @@ from cloudinit.sources.DataSourceCloudStack import ( DataSourceCloudStack, ) -from cloudinit.net.dhcp import get_latest_lease +from cloudinit.net.dhcp import IscDhclient from tests.unittests.helpers import CiTestCase, ExitStack, mock MOD_PATH = "cloudinit.sources.DataSourceCloudStack" @@ -25,7 +25,7 @@ def setUp(self): default_gw = "192.201.20.0" get_latest_lease = mock.MagicMock(return_value=None) self.patches.enter_context( - mock.patch(mod_name + ".dhcp.get_latest_lease", get_latest_lease) + mock.patch(mod_name + ".dhcp.IscDhclient.get_latest_lease", get_latest_lease) ) get_default_gw = mock.MagicMock(return_value=default_gw) @@ -151,7 +151,7 @@ def _pop_and_test(self, files, expected): lease_d = self.tmp_dir() self._populate_dir_list(lease_d, files) self.assertEqual( - self.tmp_path(expected, lease_d), get_latest_lease(lease_d) + self.tmp_path(expected, lease_d), IscDhclient.get_latest_lease(lease_d) ) def test_skips_dhcpv6_files(self): @@ -198,13 +198,10 @@ def test_selects_newest_matching(self): valid_2_path = self.tmp_path(valid_2, lease_d) self._populate_dir_list(lease_d, [valid_1, valid_2]) - self.assertEqual(valid_2_path, get_latest_lease(lease_d)) + self.assertEqual(valid_2_path, IscDhclient.get_latest_lease(lease_d)) # now update mtime on valid_2 to be older than valid_1 and re-check. mtime = int(os.path.getmtime(valid_1_path)) - 1 os.utime(valid_2_path, (mtime, mtime)) - self.assertEqual(valid_1_path, get_latest_lease(lease_d)) - - -# vi: ts=4 expandtab + self.assertEqual(valid_1_path, IscDhclient.get_latest_lease(lease_d)) From de9506a000072e4e8910a956d042cd6273c0e999 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 13 Apr 2023 11:16:19 -0600 Subject: [PATCH 04/11] pipe distro object from datasource definition through ephemeral to dhcp code --- cloudinit/net/ephemeral.py | 8 +++++--- cloudinit/sources/DataSourceAzure.py | 1 + cloudinit/sources/DataSourceEc2.py | 1 + cloudinit/sources/DataSourceGCE.py | 1 + cloudinit/sources/DataSourceHetzner.py | 1 + cloudinit/sources/DataSourceOpenStack.py | 2 +- cloudinit/sources/DataSourceOracle.py | 1 + cloudinit/sources/DataSourceScaleway.py | 1 + cloudinit/sources/DataSourceUpCloud.py | 2 +- cloudinit/sources/DataSourceVultr.py | 1 + cloudinit/sources/helpers/vultr.py | 6 ++++-- tests/unittests/net/test_dhcp.py | 14 ++++++++------ tests/unittests/net/test_ephemeral.py | 6 ++++-- tests/unittests/sources/test_azure.py | 14 ++++++++++++++ tests/unittests/sources/test_ec2.py | 2 +- tests/unittests/sources/test_hetzner.py | 1 + tests/unittests/sources/test_openstack.py | 2 +- tests/unittests/sources/test_oracle.py | 2 ++ tests/unittests/sources/test_upcloud.py | 2 +- tests/unittests/sources/test_vultr.py | 2 +- 20 files changed, 51 insertions(+), 19 deletions(-) diff --git a/cloudinit/net/ephemeral.py b/cloudinit/net/ephemeral.py index 3fa48bcc5c7..ed70a4684c7 100644 --- a/cloudinit/net/ephemeral.py +++ b/cloudinit/net/ephemeral.py @@ -305,10 +305,10 @@ def __exit__(self, *_args): class EphemeralDHCPv4: def __init__( self, + distro, iface=None, connectivity_url_data: Optional[Dict[str, Any]] = None, dhcp_log_func=None, - distro=None, ): self.iface = iface self._ephipv4 = None @@ -353,7 +353,7 @@ def obtain_lease(self): """ if self.lease: return self.lease - leases = maybe_perform_dhcp_discovery(self.iface, self.dhcp_log_func) + leases = maybe_perform_dhcp_discovery(self.distro, self.iface, self.dhcp_log_func) if not leases: raise NoDHCPLeaseError() self.lease = leases[-1] @@ -414,6 +414,7 @@ class EphemeralIPNetwork: def __init__( self, + distro, interface, ipv6: bool = False, ipv4: bool = True, @@ -423,13 +424,14 @@ def __init__( self.ipv6 = ipv6 self.stack = contextlib.ExitStack() self.state_msg: str = "" + self.distro = distro def __enter__(self): # ipv6 dualstack might succeed when dhcp4 fails # therefore catch exception unless only v4 is used try: if self.ipv4: - self.stack.enter_context(EphemeralDHCPv4(self.interface)) + self.stack.enter_context(EphemeralDHCPv4(self.distro, self.interface)) if self.ipv6: self.stack.enter_context(EphemeralIPv6Network(self.interface)) # v6 link local might be usable diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py index 927e8cf0de4..18e0f1d7773 100644 --- a/cloudinit/sources/DataSourceAzure.py +++ b/cloudinit/sources/DataSourceAzure.py @@ -387,6 +387,7 @@ def _setup_ephemeral_networking( LOG.debug("Requested ephemeral networking (iface=%s)", iface) self._ephemeral_dhcp_ctx = EphemeralDHCPv4( + self.distro, iface=iface, dhcp_log_func=dhcp_log_cb, ) diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py index e8fb0023323..a7480b4043b 100644 --- a/cloudinit/sources/DataSourceEc2.py +++ b/cloudinit/sources/DataSourceEc2.py @@ -131,6 +131,7 @@ def _get_data(self): return False try: with EphemeralIPNetwork( + self.distro, self.fallback_interface, ipv4=True, ipv6=True, diff --git a/cloudinit/sources/DataSourceGCE.py b/cloudinit/sources/DataSourceGCE.py index bb44cd1fa42..f1f4764bad3 100644 --- a/cloudinit/sources/DataSourceGCE.py +++ b/cloudinit/sources/DataSourceGCE.py @@ -84,6 +84,7 @@ def _get_data(self): network_context = noop() if self.perform_dhcp_setup: network_context = EphemeralDHCPv4( + self.distro, self.fallback_interface, ) with network_context: diff --git a/cloudinit/sources/DataSourceHetzner.py b/cloudinit/sources/DataSourceHetzner.py index 14f1467772b..bda5e876fc3 100644 --- a/cloudinit/sources/DataSourceHetzner.py +++ b/cloudinit/sources/DataSourceHetzner.py @@ -57,6 +57,7 @@ def _get_data(self): try: with EphemeralDHCPv4( + self.distro, iface=net.find_fallback_nic(), connectivity_url_data={ "url": BASE_URL_V1 + "/metadata/instance-id", diff --git a/cloudinit/sources/DataSourceOpenStack.py b/cloudinit/sources/DataSourceOpenStack.py index c480b627d2f..54ca8044506 100644 --- a/cloudinit/sources/DataSourceOpenStack.py +++ b/cloudinit/sources/DataSourceOpenStack.py @@ -154,7 +154,7 @@ def _get_data(self): if self.perform_dhcp_setup: # Setup networking in init-local stage. try: - with EphemeralDHCPv4(self.fallback_interface): + with EphemeralDHCPv4(self.distro, self.fallback_interface): results = util.log_time( logfunc=LOG.debug, msg="Crawl of metadata service", diff --git a/cloudinit/sources/DataSourceOracle.py b/cloudinit/sources/DataSourceOracle.py index 3baf06e168e..db520e2161c 100644 --- a/cloudinit/sources/DataSourceOracle.py +++ b/cloudinit/sources/DataSourceOracle.py @@ -150,6 +150,7 @@ def _get_data(self): self.system_uuid = _read_system_uuid() network_context = ephemeral.EphemeralDHCPv4( + self.distro, iface=net.find_fallback_nic(), connectivity_url_data={ "url": METADATA_PATTERN.format(version=2, path="instance"), diff --git a/cloudinit/sources/DataSourceScaleway.py b/cloudinit/sources/DataSourceScaleway.py index 5c420398a88..e7fb9a3bbb9 100644 --- a/cloudinit/sources/DataSourceScaleway.py +++ b/cloudinit/sources/DataSourceScaleway.py @@ -280,6 +280,7 @@ def _get_data(self): # before giving up. Lower it in config file and try it first as # it will only reach timeout on VMs with only IPv6 addresses. with EphemeralDHCPv4( + self.distro, self._fallback_interface, ) as ipv4: util.log_time( diff --git a/cloudinit/sources/DataSourceUpCloud.py b/cloudinit/sources/DataSourceUpCloud.py index 43122f0b10b..1c1d213979a 100644 --- a/cloudinit/sources/DataSourceUpCloud.py +++ b/cloudinit/sources/DataSourceUpCloud.py @@ -71,7 +71,7 @@ def _get_data(self): LOG.debug("Finding a fallback NIC") nic = cloudnet.find_fallback_nic() LOG.debug("Discovering metadata via DHCP interface %s", nic) - with EphemeralDHCPv4(nic): + with EphemeralDHCPv4(self.distro, nic): md = util.log_time( logfunc=LOG.debug, msg="Reading from metadata service", diff --git a/cloudinit/sources/DataSourceVultr.py b/cloudinit/sources/DataSourceVultr.py index f7c567803b7..73781a756c3 100644 --- a/cloudinit/sources/DataSourceVultr.py +++ b/cloudinit/sources/DataSourceVultr.py @@ -88,6 +88,7 @@ def get_datasource_data(self, md): # Get the metadata by flag def get_metadata(self): return vultr.get_metadata( + self.distro, self.ds_cfg["url"], self.ds_cfg["timeout"], self.ds_cfg["retries"], diff --git a/cloudinit/sources/helpers/vultr.py b/cloudinit/sources/helpers/vultr.py index a6d5cea7194..1f16bc92b67 100644 --- a/cloudinit/sources/helpers/vultr.py +++ b/cloudinit/sources/helpers/vultr.py @@ -18,7 +18,7 @@ @lru_cache() -def get_metadata(url, timeout, retries, sec_between, agent, tmp_dir=None): +def get_metadata(distro, url, timeout, retries, sec_between, agent, tmp_dir=None): # Bring up interface (and try untill one works) exception = RuntimeError("Failed to DHCP") @@ -26,7 +26,9 @@ def get_metadata(url, timeout, retries, sec_between, agent, tmp_dir=None): for iface in get_interface_list(): try: with EphemeralDHCPv4( - iface=iface, connectivity_url_data={"url": url} + distro, + iface=iface, + connectivity_url_data={"url": url}, ): # Check for the metadata route, skip if not there if not check_route(url): diff --git a/tests/unittests/net/test_dhcp.py b/tests/unittests/net/test_dhcp.py index 96691906909..a1eba10204a 100644 --- a/tests/unittests/net/test_dhcp.py +++ b/tests/unittests/net/test_dhcp.py @@ -175,7 +175,7 @@ def test_obtain_lease_parses_static_routes(self, m_maybe, m_ipv4): } ] m_maybe.return_value = lease - eph = EphemeralDHCPv4() + eph = EphemeralDHCPv4(MockDistro(),) eph.obtain_lease() expected_kwargs = { "interface": "wlp3s0", @@ -206,7 +206,7 @@ def test_obtain_centos_lease_parses_static_routes(self, m_maybe, m_ipv4): } ] m_maybe.return_value = lease - eph = EphemeralDHCPv4() + eph = EphemeralDHCPv4(MockDistro(),) eph.obtain_lease() expected_kwargs = { "interface": "wlp3s0", @@ -801,6 +801,7 @@ def test_ephemeral_dhcp_no_network_if_url_connectivity(self, m_dhcp): self.responses.add(responses.GET, url) with EphemeralDHCPv4( + MockDistro(), connectivity_url_data={"url": url}, ) as lease: self.assertIsNone(lease) @@ -824,6 +825,7 @@ def test_ephemeral_dhcp_setup_network_if_url_connectivity( self.responses.add(responses.GET, url, body=b"", status=404) with EphemeralDHCPv4( + MockDistro(), connectivity_url_data={"url": url}, ) as lease: self.assertEqual(fake_lease, lease) @@ -845,7 +847,7 @@ def test_obtain_lease_raises_error(self, m_dhcp, error_class): m_dhcp.side_effect = [error_class()] with pytest.raises(error_class): - EphemeralDHCPv4().obtain_lease() + EphemeralDHCPv4(MockDistro(),).obtain_lease() assert len(m_dhcp.mock_calls) == 1 @@ -853,7 +855,7 @@ def test_obtain_lease_raises_error(self, m_dhcp, error_class): def test_obtain_lease_umbrella_error(self, m_dhcp, error_class): m_dhcp.side_effect = [error_class()] with pytest.raises(NoDHCPLeaseError): - EphemeralDHCPv4().obtain_lease() + EphemeralDHCPv4(MockDistro(),).obtain_lease() assert len(m_dhcp.mock_calls) == 1 @@ -862,7 +864,7 @@ def test_ctx_mgr_raises_error(self, m_dhcp, error_class): m_dhcp.side_effect = [error_class()] with pytest.raises(error_class): - with EphemeralDHCPv4(): + with EphemeralDHCPv4(MockDistro(),): pass assert len(m_dhcp.mock_calls) == 1 @@ -871,7 +873,7 @@ def test_ctx_mgr_raises_error(self, m_dhcp, error_class): def test_ctx_mgr_umbrella_error(self, m_dhcp, error_class): m_dhcp.side_effect = [error_class()] with pytest.raises(NoDHCPLeaseError): - with EphemeralDHCPv4(): + with EphemeralDHCPv4(MockDistro(),): pass assert len(m_dhcp.mock_calls) == 1 diff --git a/tests/unittests/net/test_ephemeral.py b/tests/unittests/net/test_ephemeral.py index d2237faf10a..7e537a1acfb 100644 --- a/tests/unittests/net/test_ephemeral.py +++ b/tests/unittests/net/test_ephemeral.py @@ -4,6 +4,7 @@ import pytest +from tests.unittests.util import MockDistro from cloudinit.net.ephemeral import EphemeralIPNetwork M_PATH = "cloudinit.net.ephemeral." @@ -24,14 +25,15 @@ def test_stack_order( ipv6, ): interface = object() - with EphemeralIPNetwork(interface, ipv4=ipv4, ipv6=ipv6): + distro = MockDistro() + with EphemeralIPNetwork(distro, interface, ipv4=ipv4, ipv6=ipv6): pass expected_call_args_list = [] if ipv4: expected_call_args_list.append( mock.call(m_ephemeral_dhcp_v4.return_value) ) - assert [mock.call(interface)] == m_ephemeral_dhcp_v4.call_args_list + assert [mock.call(distro, interface)] == m_ephemeral_dhcp_v4.call_args_list else: assert [] == m_ephemeral_dhcp_v4.call_args_list if ipv6: diff --git a/tests/unittests/sources/test_azure.py b/tests/unittests/sources/test_azure.py index 0648f08ce90..7511557bd56 100644 --- a/tests/unittests/sources/test_azure.py +++ b/tests/unittests/sources/test_azure.py @@ -3224,6 +3224,7 @@ def test_basic_setup( assert mock_ephemeral_dhcp_v4.mock_calls == [ mock.call( + azure_ds.distro, iface=iface, dhcp_log_func=dsaz.dhcp_log_cb, ), @@ -3250,6 +3251,7 @@ def test_basic_setup_without_wireserver_opt( assert mock_ephemeral_dhcp_v4.mock_calls == [ mock.call( + azure_ds.distro, iface=iface, dhcp_log_func=dsaz.dhcp_log_cb, ), @@ -3292,6 +3294,7 @@ def test_retry_interface_error( assert mock_ephemeral_dhcp_v4.mock_calls == [ mock.call( + azure_ds.distro, iface=None, dhcp_log_func=dsaz.dhcp_log_cb, ), @@ -3326,6 +3329,7 @@ def test_retry_process_error( assert mock_ephemeral_dhcp_v4.mock_calls == [ mock.call( + azure_ds.distro, iface=None, dhcp_log_func=dsaz.dhcp_log_cb, ), @@ -3364,6 +3368,7 @@ def test_retry_sleeps( mock_ephemeral_dhcp_v4.mock_calls == [ mock.call( + azure_ds.distro, iface=None, dhcp_log_func=dsaz.dhcp_log_cb, ), @@ -3531,6 +3536,7 @@ def test_no_pps(self): ] assert self.mock_net_dhcp_maybe_perform_dhcp_discovery.mock_calls == [ mock.call( + self.azure_ds.distro, None, dsaz.dhcp_log_cb, ) @@ -3617,10 +3623,12 @@ def test_running_pps(self): ] assert self.mock_net_dhcp_maybe_perform_dhcp_discovery.mock_calls == [ mock.call( + self.azure_ds.distro, None, dsaz.dhcp_log_cb, ), mock.call( + self.azure_ds.distro, None, dsaz.dhcp_log_cb, ), @@ -3733,10 +3741,12 @@ def test_savable_pps(self): ] assert self.mock_net_dhcp_maybe_perform_dhcp_discovery.mock_calls == [ mock.call( + self.azure_ds.distro, None, dsaz.dhcp_log_cb, ), mock.call( + self.azure_ds.distro, "ethAttached1", dsaz.dhcp_log_cb, ), @@ -3887,10 +3897,12 @@ def test_savable_pps_early_unplug(self, fabric_side_effect): ] assert self.mock_net_dhcp_maybe_perform_dhcp_discovery.mock_calls == [ mock.call( + self.azure_ds.distro, None, dsaz.dhcp_log_cb, ), mock.call( + self.azure_ds.distro, "ethAttached1", dsaz.dhcp_log_cb, ), @@ -3988,6 +4000,7 @@ def test_recovery_pps(self, pps_type): ] assert self.mock_net_dhcp_maybe_perform_dhcp_discovery.mock_calls == [ mock.call( + self.azure_ds.distro, None, dsaz.dhcp_log_cb, ), @@ -4091,6 +4104,7 @@ def test_os_disk_pps(self, mock_sleep, subp_side_effect): ] assert self.mock_net_dhcp_maybe_perform_dhcp_discovery.mock_calls == [ mock.call( + self.azure_ds.distro, None, dsaz.dhcp_log_cb, ) diff --git a/tests/unittests/sources/test_ec2.py b/tests/unittests/sources/test_ec2.py index 2a3116421ca..e19cd2f87b4 100644 --- a/tests/unittests/sources/test_ec2.py +++ b/tests/unittests/sources/test_ec2.py @@ -879,7 +879,7 @@ def test_ec2_local_performs_dhcp_on_non_bsd( ret = ds.get_data() self.assertTrue(ret) - m_dhcp.assert_called_once_with("eth9", None) + m_dhcp.assert_called_once_with(ds.distro, "eth9", None) m_net4.assert_called_once_with( broadcast="192.168.2.255", interface="eth9", diff --git a/tests/unittests/sources/test_hetzner.py b/tests/unittests/sources/test_hetzner.py index 6dbeb85bb2e..024652c1ad8 100644 --- a/tests/unittests/sources/test_hetzner.py +++ b/tests/unittests/sources/test_hetzner.py @@ -110,6 +110,7 @@ def test_read_data( self.assertTrue(ret) m_net.assert_called_once_with( + ds.distro, iface="eth0", connectivity_url_data={ "url": "http://169.254.169.254/hetzner/v1/metadata/instance-id" diff --git a/tests/unittests/sources/test_openstack.py b/tests/unittests/sources/test_openstack.py index b37a7570b4f..6f588122bc0 100644 --- a/tests/unittests/sources/test_openstack.py +++ b/tests/unittests/sources/test_openstack.py @@ -367,7 +367,7 @@ def test_local_datasource(self, m_dhcp, m_net): self.assertEqual(VENDOR_DATA, ds_os_local.vendordata_pure) self.assertEqual(VENDOR_DATA2, ds_os_local.vendordata2_pure) self.assertIsNone(ds_os_local.vendordata_raw) - m_dhcp.assert_called_with("eth9", None) + m_dhcp.assert_called_with(distro, "eth9", None) def test_bad_datasource_meta(self): os_files = copy.deepcopy(OS_FILES) diff --git a/tests/unittests/sources/test_oracle.py b/tests/unittests/sources/test_oracle.py index c67cacef824..c864722ca5a 100644 --- a/tests/unittests/sources/test_oracle.py +++ b/tests/unittests/sources/test_oracle.py @@ -982,6 +982,7 @@ def assert_in_context_manager(**kwargs): assert [ mock.call( + oracle_ds.distro, iface=m_find_fallback_nic.return_value, connectivity_url_data={ "headers": {"Authorization": "Bearer Oracle"}, @@ -1024,6 +1025,7 @@ def assert_in_context_manager(**kwargs): assert [ mock.call( + oracle_ds.distro, iface=m_find_fallback_nic.return_value, connectivity_url_data={ "headers": {"Authorization": "Bearer Oracle"}, diff --git a/tests/unittests/sources/test_upcloud.py b/tests/unittests/sources/test_upcloud.py index 0bab508a750..6d2f98c2fdd 100644 --- a/tests/unittests/sources/test_upcloud.py +++ b/tests/unittests/sources/test_upcloud.py @@ -242,7 +242,7 @@ def test_network_configured_metadata( self.assertTrue(ret) self.assertTrue(m_dhcp.called) - m_dhcp.assert_called_with("eth1", None) + m_dhcp.assert_called_with(ds.distro, "eth1", None) m_net.assert_called_once_with( broadcast="10.6.3.255", diff --git a/tests/unittests/sources/test_vultr.py b/tests/unittests/sources/test_vultr.py index 488df4f34e7..f1a1dbc1568 100644 --- a/tests/unittests/sources/test_vultr.py +++ b/tests/unittests/sources/test_vultr.py @@ -398,7 +398,7 @@ def test_private_network_config(self, mock_netmap): # Override ephemeral for proper unit testing def ephemeral_init( - self, iface="", connectivity_url_data=None, tmp_dir=None + self, distro, iface="", connectivity_url_data=None, tmp_dir=None ): global FINAL_INTERFACE_USED FINAL_INTERFACE_USED = iface From 3e1592cd1abd046863497522734acaf1370b6c83 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 13 Apr 2023 11:53:49 -0600 Subject: [PATCH 05/11] only discover other clients when first isn't found, add failover test --- cloudinit/net/dhcp.py | 8 ++++---- tests/unittests/net/test_dhcp.py | 28 +++++++++++++++++++++++++++- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py index 6bae13ad8b6..193f4a4507c 100644 --- a/cloudinit/net/dhcp.py +++ b/cloudinit/net/dhcp.py @@ -57,12 +57,12 @@ def select_dhcp_client(distro): for client in distro.dhcp_client_priority: try: dhcp_client = client() - LOG.debug("Selected dhcp client: %s.", client.client_name) + LOG.debug("DHCP client selected: %s", client.client_name) + return dhcp_client except NoDHCPLeaseMissingDhclientError: - LOG.debug("Dhcp client [%s] not found.", client.client_name) + LOG.warning("DHCP client not found: %s", client.client_name) else: raise NoDHCPLeaseMissingDhclientError() - return dhcp_client def maybe_perform_dhcp_discovery(distro, nic=None, dhcp_log_func=None): @@ -90,7 +90,7 @@ def maybe_perform_dhcp_discovery(distro, nic=None, dhcp_log_func=None): raise NoDHCPLeaseInterfaceError() client = select_dhcp_client(distro) return client.dhcp_discovery( - dhclient_path, nic, dhcp_log_func, distro + nic, dhcp_log_func, distro ) diff --git a/tests/unittests/net/test_dhcp.py b/tests/unittests/net/test_dhcp.py index a1eba10204a..11ee2c3286d 100644 --- a/tests/unittests/net/test_dhcp.py +++ b/tests/unittests/net/test_dhcp.py @@ -366,7 +366,33 @@ def test_dhclient_exits_with_error( maybe_perform_dhcp_discovery(MockDistro()) self.assertIn( - "Dhcp client [dhcpcd] not found.", + "DHCP client selected: dhclient", + self.logs.getvalue(), + ) + + @mock.patch("cloudinit.net.dhcp.find_fallback_nic", return_value="eth9") + @mock.patch("cloudinit.net.dhcp.os.remove") + @mock.patch("cloudinit.net.dhcp.subp.subp") + @mock.patch("cloudinit.net.dhcp.subp.which") + def test_dhcp_client_failover( + self, m_which, m_subp, m_remove, m_fallback + ): + """Log and do nothing when nic is absent and no fallback is found.""" + m_subp.side_effect = [ + ("", ""), + subp.ProcessExecutionError(exit_code=-5), + ] + + m_which.side_effect = [False, True] + with pytest.raises(NoDHCPLeaseError): + maybe_perform_dhcp_discovery(MockDistro()) + + self.assertIn( + "DHCP client not found: dhclient", + self.logs.getvalue(), + ) + self.assertIn( + "DHCP client not found: dhcpcd", self.logs.getvalue(), ) From 5f499c2592638c9c971a17a1d7a65bbea3752a50 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 13 Apr 2023 12:23:33 -0600 Subject: [PATCH 06/11] format --- cloudinit/distros/__init__.py | 2 +- cloudinit/distros/freebsd.py | 3 - cloudinit/distros/openbsd.py | 5 +- cloudinit/net/dhcp.py | 92 +++++++++++-------- cloudinit/net/ephemeral.py | 10 +- cloudinit/net/freebsd.py | 11 ++- cloudinit/net/openbsd.py | 2 +- cloudinit/sources/DataSourceAzure.py | 3 - cloudinit/sources/DataSourceEc2.py | 3 - cloudinit/sources/DataSourceGCE.py | 2 - cloudinit/sources/DataSourceHetzner.py | 3 - cloudinit/sources/DataSourceOpenStack.py | 3 - cloudinit/sources/DataSourceOracle.py | 2 - cloudinit/sources/DataSourceUpCloud.py | 3 - cloudinit/sources/DataSourceVultr.py | 2 - .../sources/helpers/vmware/imc/config_nic.py | 5 +- cloudinit/sources/helpers/vultr.py | 7 +- tests/unittests/config/test_cc_ntp.py | 7 +- .../unittests/config/test_cc_set_passwords.py | 10 +- .../unittests/distros/test_manage_service.py | 12 ++- tests/unittests/net/test_dhcp.py | 75 +++++++++------ tests/unittests/net/test_ephemeral.py | 6 +- tests/unittests/sources/test_azure.py | 3 - tests/unittests/sources/test_cloudstack.py | 12 ++- tests/unittests/sources/test_ec2.py | 3 - tests/unittests/sources/test_oracle.py | 3 - tests/unittests/sources/test_upcloud.py | 3 - tests/unittests/sources/test_vultr.py | 3 - 28 files changed, 152 insertions(+), 143 deletions(-) diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index 05f9fe23612..daec1340a52 100644 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -32,7 +32,7 @@ from cloudinit.distros.networking import LinuxNetworking, Networking from cloudinit.distros.parsers import hosts from cloudinit.features import ALLOW_EC2_MIRRORS_ON_NON_AWS_INSTANCE_TYPES -from cloudinit.net import activators, eni, network_state, renderers, dhcp +from cloudinit.net import activators, dhcp, eni, network_state, renderers from cloudinit.net.network_state import parse_net_config_data from cloudinit.net.renderer import Renderer diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py index 25d864d7e48..48f31628339 100644 --- a/cloudinit/distros/freebsd.py +++ b/cloudinit/distros/freebsd.py @@ -192,6 +192,3 @@ def update_package_sources(self): ["update"], freq=PER_INSTANCE, ) - - -# vi: ts=4 expandtab diff --git a/cloudinit/distros/openbsd.py b/cloudinit/distros/openbsd.py index 78fa5fe3c2d..9b1b989a072 100644 --- a/cloudinit/distros/openbsd.py +++ b/cloudinit/distros/openbsd.py @@ -25,13 +25,14 @@ def _write_hostname(self, hostname, filename): def _get_add_member_to_group_cmd(self, member_name, group_name): return ["usermod", "-G", group_name, member_name] - def manage_service(self, action: str, service: str, rcs=None): + @classmethod + def manage_service(cls, action: str, service: str, rcs=None): """ Perform the requested action on a service. This handles OpenBSD's 'rcctl'. May raise ProcessExecutionError """ - init_cmd = self.init_cmd + init_cmd = cls.init_cmd cmds = { "stop": ["stop", service], "start": ["start", service], diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py index 193f4a4507c..7b202c6a481 100644 --- a/cloudinit/net/dhcp.py +++ b/cloudinit/net/dhcp.py @@ -4,15 +4,15 @@ # # This file is part of cloud-init. See LICENSE file for license information. +import abc import contextlib +import glob import logging import os import re import signal import time from io import StringIO -import glob -import abc import configobj @@ -89,9 +89,7 @@ def maybe_perform_dhcp_discovery(distro, nic=None, dhcp_log_func=None): ) raise NoDHCPLeaseInterfaceError() client = select_dhcp_client(distro) - return client.dhcp_discovery( - nic, dhcp_log_func, distro - ) + return client.dhcp_discovery(nic, dhcp_log_func, distro) def networkd_parse_lease(content): @@ -135,6 +133,8 @@ def networkd_get_option_from_leases(keyname, leases_d=None): class DhcpClient(abc.ABC): + client_name = "" + @classmethod def kill_dhcp_client(cls): subp.subp(["pkill", cls.client_name], rcs=[0, 1]) @@ -161,15 +161,17 @@ class IscDhclient(DhcpClient): def __init__(self): self.dhclient_path = subp.which("dhclient") if not self.dhclient_path: - LOG.debug("Skip dhclient configuration: No dhclient command found.") + LOG.debug( + "Skip dhclient configuration: No dhclient command found." + ) raise NoDHCPLeaseMissingDhclientError() @staticmethod def parse_dhcp_lease_file(lease_file): """Parse the given dhcp lease file for the most recent lease. - Return a list of dicts of dhcp options. Each dict contains key value pairs - a specific lease in order from oldest to newest. + Return a list of dicts of dhcp options. Each dict contains key value + pairs a specific lease in order from oldest to newest. @raises: InvalidDHCPLeaseFileError on empty of unparseable leasefile content. @@ -199,25 +201,26 @@ def parse_dhcp_lease_file(lease_file): return dhcp_leases def dhcp_discovery( - self, - interface, - dhcp_log_func=None, - distro=None,): - """Run dhclient on the interface without scripts or filesystem artifacts. + self, + interface, + dhcp_log_func=None, + distro=None, + ): + """Run dhclient on the interface without scripts/filesystem artifacts. @param dhclient_cmd_path: Full path to the dhclient used. @param interface: Name of the network interface on which to dhclient. - @param dhcp_log_func: A callable accepting the dhclient output and error - streams. + @param dhcp_log_func: A callable accepting the dhclient output and + error streams. - @return: A list of dicts of representing the dhcp leases parsed from the - dhclient.lease file or empty list. + @return: A list of dicts of representing the dhcp leases parsed from + the dhclient.lease file or empty list. """ LOG.debug("Performing a dhcp discovery on %s", interface) - # We want to avoid running /sbin/dhclient-script because of side-effects in - # /etc/resolv.conf any any other vendor specific scripts in - # /etc/dhcp/dhclient*hooks.d. + # We want to avoid running /sbin/dhclient-script because of + # side-effects in # /etc/resolv.conf any any other vendor specific + # scripts in /etc/dhcp/dhclient*hooks.d. pid_file = "/run/dhclient.pid" lease_file = "/run/dhclient.lease" @@ -227,14 +230,15 @@ def dhcp_discovery( os.remove(pid_file) os.remove(lease_file) - # ISC dhclient needs the interface up to send initial discovery packets. - # Generally dhclient relies on dhclient-script PREINIT action to bring the - # link up before attempting discovery. Since we are using -sf /bin/true, - # we need to do that "link up" ourselves first. + # ISC dhclient needs the interface up to send initial discovery packets + # Generally dhclient relies on dhclient-script PREINIT action to bring + # the link up before attempting discovery. Since we are using + # -sf /bin/true, we need to do that "link up" ourselves first. subp.subp(["ip", "link", "set", "dev", interface, "up"], capture=True) - # For INFINIBAND port the dhlient must be sent with dhcp-client-identifier. - # So here we are checking if the interface is INFINIBAND or not. - # If yes, we are generating the the client-id to be used with the dhclient + # For INFINIBAND port the dhlient must be sent with + # dhcp-client-identifier. So here we are checking if the interface is + # INFINIBAND or not. If yes, we are generating the the client-id to be + # used with the dhclient cmd = [ self.dhclient_path, "-1", @@ -248,7 +252,9 @@ def dhcp_discovery( "/bin/true", ] if is_ib_interface(interface): - dhcp_client_identifier = "20:%s" % get_interface_mac(interface)[36:] + dhcp_client_identifier = ( + "20:%s" % get_interface_mac(interface)[36:] + ) interface_dhclient_content = ( 'interface "%s" ' "{send dhcp-client-identifier %s;}" @@ -319,25 +325,31 @@ def dhcp_discovery( @staticmethod def parse_static_routes(rfc3442): - """parse rfc3442 format and return a list containing tuple of strings. + """ + parse rfc3442 format and return a list containing tuple of strings. The tuple is composed of the network_address (including net length) and - gateway for a parsed static route. It can parse two formats of rfc3442, - one from dhcpcd and one from dhclient (isc). + gateway for a parsed static route. It can parse two formats of + rfc3442, one from dhcpcd and one from dhclient (isc). @param rfc3442: string in rfc3442 format (isc or dhcpd) @returns: list of tuple(str, str) for all valid parsed routes until the first parsing error. - E.g. - sr=parse_static_routes("32,169,254,169,254,130,56,248,255,0,130,56,240,1") + e.g.: + + sr=parse_static_routes(\ + "32,169,254,169,254,130,56,248,255,0,130,56,240,1") sr=[ - ("169.254.169.254/32", "130.56.248.255"), ("0.0.0.0/0", "130.56.240.1") + ("169.254.169.254/32", "130.56.248.255"), \ + ("0.0.0.0/0", "130.56.240.1") ] - sr2 = parse_static_routes("24.191.168.128 192.168.128.1,0 192.168.128.1") + sr2 = parse_static_routes(\ + "24.191.168.128 192.168.128.1,0 192.168.128.1") sr2 = [ - ("191.168.128.0/24", "192.168.128.1"), ("0.0.0.0/0", "192.168.128.1") + ("191.168.128.0/24", "192.168.128.1"),\ + ("0.0.0.0/0", "192.168.128.1") ] Python version of isc-dhclient's hooks: @@ -391,7 +403,9 @@ def _trunc_error(cidr, required, remain): if len(tokens[idx:]) < req_toks: _trunc_error(net_length, req_toks, len(tokens[idx:])) return static_routes - net_address = ".".join(tokens[idx + 1 : idx + 2] + ["0", "0", "0"]) + net_address = ".".join( + tokens[idx + 1 : idx + 2] + ["0", "0", "0"] + ) gateway = ".".join(tokens[idx + 2 : idx + req_toks]) current_idx = idx + req_toks elif net_length == 0: @@ -410,7 +424,9 @@ def _trunc_error(cidr, required, remain): ) return static_routes - static_routes.append(("%s/%s" % (net_address, net_length), gateway)) + static_routes.append( + ("%s/%s" % (net_address, net_length), gateway) + ) return static_routes diff --git a/cloudinit/net/ephemeral.py b/cloudinit/net/ephemeral.py index ed70a4684c7..130afa1780c 100644 --- a/cloudinit/net/ephemeral.py +++ b/cloudinit/net/ephemeral.py @@ -9,9 +9,9 @@ import cloudinit.net as net from cloudinit import subp from cloudinit.net.dhcp import ( + IscDhclient, NoDHCPLeaseError, maybe_perform_dhcp_discovery, - IscDhclient, ) LOG = logging.getLogger(__name__) @@ -353,7 +353,9 @@ def obtain_lease(self): """ if self.lease: return self.lease - leases = maybe_perform_dhcp_discovery(self.distro, self.iface, self.dhcp_log_func) + leases = maybe_perform_dhcp_discovery( + self.distro, self.iface, self.dhcp_log_func + ) if not leases: raise NoDHCPLeaseError() self.lease = leases[-1] @@ -431,7 +433,9 @@ def __enter__(self): # therefore catch exception unless only v4 is used try: if self.ipv4: - self.stack.enter_context(EphemeralDHCPv4(self.distro, self.interface)) + self.stack.enter_context( + EphemeralDHCPv4(self.distro, self.interface) + ) if self.ipv6: self.stack.enter_context(EphemeralIPv6Network(self.interface)) # v6 link local might be usable diff --git a/cloudinit/net/freebsd.py b/cloudinit/net/freebsd.py index 66d299880c3..cfc41e6014a 100644 --- a/cloudinit/net/freebsd.py +++ b/cloudinit/net/freebsd.py @@ -1,8 +1,9 @@ # This file is part of cloud-init. See LICENSE file for license information. import cloudinit.net.bsd +from cloudinit import distros from cloudinit import log as logging -from cloudinit import subp, util, net, distros +from cloudinit import net, subp, util LOG = logging.getLogger(__name__) @@ -50,7 +51,9 @@ def start_services(self, run=False): for dhcp_interface in self.dhcp_interfaces(): # Observed on DragonFlyBSD 6. If we use the "restart" parameter, # the routes are not recreated. - net.dhcp.IscDhclient.start_service(dhcp_interface, distros.freebsd.Distro) + net.dhcp.IscDhclient.start_service( + dhcp_interface, distros.freebsd.Distro + ) subp.subp(["service", "netif", "restart"], capture=True) # On FreeBSD 10, the restart of routing and dhclient is likely to fail @@ -62,7 +65,9 @@ def start_services(self, run=False): subp.subp(["service", "routing", "restart"], capture=True, rcs=[0, 1]) for dhcp_interface in self.dhcp_interfaces(): - net.dhcp.IscDhclient.stop_service(dhcp_interface, distros.freebsd.Distro) + net.dhcp.IscDhclient.stop_service( + dhcp_interface, distros.freebsd.Distro + ) def set_route(self, network, netmask, gateway): if network == "0.0.0.0": diff --git a/cloudinit/net/openbsd.py b/cloudinit/net/openbsd.py index 94b0eacfef6..5dd13800c6f 100644 --- a/cloudinit/net/openbsd.py +++ b/cloudinit/net/openbsd.py @@ -4,7 +4,7 @@ import cloudinit.net.bsd from cloudinit import log as logging -from cloudinit import subp, util, net +from cloudinit import net, subp, util LOG = logging.getLogger(__name__) diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py index 18e0f1d7773..cc59b065e79 100644 --- a/cloudinit/sources/DataSourceAzure.py +++ b/cloudinit/sources/DataSourceAzure.py @@ -1942,6 +1942,3 @@ def maybe_remove_ubuntu_network_config_scripts(paths=None): # Return a list of data sources that match this set of dependencies def get_datasource_list(depends): return sources.list_from_depends(depends, datasources) - - -# vi: ts=4 expandtab diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py index a7480b4043b..2143873afde 100644 --- a/cloudinit/sources/DataSourceEc2.py +++ b/cloudinit/sources/DataSourceEc2.py @@ -1020,6 +1020,3 @@ def _get_secondary_addresses(nic_metadata, cidr_key, mac, ips, default_prefix): # Return a list of data sources that match this set of dependencies def get_datasource_list(depends): return sources.list_from_depends(depends, datasources) - - -# vi: ts=4 expandtab diff --git a/cloudinit/sources/DataSourceGCE.py b/cloudinit/sources/DataSourceGCE.py index f1f4764bad3..041c89142a3 100644 --- a/cloudinit/sources/DataSourceGCE.py +++ b/cloudinit/sources/DataSourceGCE.py @@ -354,5 +354,3 @@ def get_datasource_list(depends): data["user-data-b64"] = b64encode(data["user-data"]).decode() print(json.dumps(data, indent=1, sort_keys=True, separators=(",", ": "))) - -# vi: ts=4 expandtab diff --git a/cloudinit/sources/DataSourceHetzner.py b/cloudinit/sources/DataSourceHetzner.py index bda5e876fc3..e20af1c30dc 100644 --- a/cloudinit/sources/DataSourceHetzner.py +++ b/cloudinit/sources/DataSourceHetzner.py @@ -160,6 +160,3 @@ def get_hcloud_data(): # Return a list of data sources that match this set of dependencies def get_datasource_list(depends): return sources.list_from_depends(depends, datasources) - - -# vi: ts=4 expandtab diff --git a/cloudinit/sources/DataSourceOpenStack.py b/cloudinit/sources/DataSourceOpenStack.py index 54ca8044506..5fb5d839151 100644 --- a/cloudinit/sources/DataSourceOpenStack.py +++ b/cloudinit/sources/DataSourceOpenStack.py @@ -290,6 +290,3 @@ def read_metadata_service(base_url, ssl_details=None, timeout=5, retries=5): # Return a list of data sources that match this set of dependencies def get_datasource_list(depends): return sources.list_from_depends(depends, datasources) - - -# vi: ts=4 expandtab diff --git a/cloudinit/sources/DataSourceOracle.py b/cloudinit/sources/DataSourceOracle.py index db520e2161c..7d46458066f 100644 --- a/cloudinit/sources/DataSourceOracle.py +++ b/cloudinit/sources/DataSourceOracle.py @@ -410,5 +410,3 @@ def get_datasource_list(depends): } ) ) - -# vi: ts=4 expandtab diff --git a/cloudinit/sources/DataSourceUpCloud.py b/cloudinit/sources/DataSourceUpCloud.py index 1c1d213979a..908df5c6c56 100644 --- a/cloudinit/sources/DataSourceUpCloud.py +++ b/cloudinit/sources/DataSourceUpCloud.py @@ -160,6 +160,3 @@ class DataSourceUpCloudLocal(DataSourceUpCloud): # Return a list of data sources that match this set of dependencies def get_datasource_list(depends): return sources.list_from_depends(depends, datasources) - - -# vi: ts=4 expandtab diff --git a/cloudinit/sources/DataSourceVultr.py b/cloudinit/sources/DataSourceVultr.py index 73781a756c3..fe29b585ced 100644 --- a/cloudinit/sources/DataSourceVultr.py +++ b/cloudinit/sources/DataSourceVultr.py @@ -149,5 +149,3 @@ def get_datasource_list(depends): print(util.json_dumps(sysinfo)) print(util.json_dumps(config)) - -# vi: ts=4 expandtab diff --git a/cloudinit/sources/helpers/vmware/imc/config_nic.py b/cloudinit/sources/helpers/vmware/imc/config_nic.py index 466e714eaee..fc10a0fc0bd 100644 --- a/cloudinit/sources/helpers/vmware/imc/config_nic.py +++ b/cloudinit/sources/helpers/vmware/imc/config_nic.py @@ -9,7 +9,7 @@ import os import re -from cloudinit import subp, util, net +from cloudinit import net, subp, util from cloudinit.net.network_state import ipv4_mask_to_net_prefix logger = logging.getLogger(__name__) @@ -277,6 +277,3 @@ def configure(self, osfamily=None): util.write_file(interfaceFile, content="\n".join(lines)) self.clear_dhcp() - - -# vi: ts=4 expandtab diff --git a/cloudinit/sources/helpers/vultr.py b/cloudinit/sources/helpers/vultr.py index 1f16bc92b67..71676bb154b 100644 --- a/cloudinit/sources/helpers/vultr.py +++ b/cloudinit/sources/helpers/vultr.py @@ -18,7 +18,9 @@ @lru_cache() -def get_metadata(distro, url, timeout, retries, sec_between, agent, tmp_dir=None): +def get_metadata( + distro, url, timeout, retries, sec_between, agent, tmp_dir=None +): # Bring up interface (and try untill one works) exception = RuntimeError("Failed to DHCP") @@ -287,6 +289,3 @@ def add_interface_names(netcfg): % interface["mac_address"] ) interface["name"] = interface_name - - -# vi: ts=4 expandtab diff --git a/tests/unittests/config/test_cc_ntp.py b/tests/unittests/config/test_cc_ntp.py index a90a977d5e6..62c9b3fb03d 100644 --- a/tests/unittests/config/test_cc_ntp.py +++ b/tests/unittests/config/test_cc_ntp.py @@ -503,7 +503,9 @@ def test_ntp_the_whole_package(self, m_sysd, m_select, m_which, m_subp): m_util.is_FreeBSD.return_value = is_FreeBSD m_util.is_OpenBSD.return_value = is_OpenBSD cc_ntp.handle("notimportant", cfg, mycloud, None) - m_subp.assert_called_with(expected_service_call, capture=True, rcs=None) + m_subp.assert_called_with( + expected_service_call, capture=True, rcs=None + ) self.assertEqual(expected_content, util.load_file(confpath)) @@ -837,6 +839,3 @@ def test_schema_validation(self, config, error_msg): else: with pytest.raises(SchemaValidationError, match=error_msg): validate_cloudconfig_schema(config, get_schema(), strict=True) - - -# vi: ts=4 expandtab diff --git a/tests/unittests/config/test_cc_set_passwords.py b/tests/unittests/config/test_cc_set_passwords.py index af36dd1703d..1a9fcd3c6a7 100644 --- a/tests/unittests/config/test_cc_set_passwords.py +++ b/tests/unittests/config/test_cc_set_passwords.py @@ -21,9 +21,11 @@ ["systemctl", "show", "--property", "ActiveState", "--value", "ssh"] ) SYSTEMD_RESTART_CALL = mock.call( - ["systemctl", "restart", "ssh"], capture=True, rcs=None) + ["systemctl", "restart", "ssh"], capture=True, rcs=None +) SERVICE_RESTART_CALL = mock.call( - ["service", "ssh", "restart"], capture=True, rcs=None) + ["service", "ssh", "restart"], capture=True, rcs=None +) @pytest.fixture(autouse=True) @@ -81,7 +83,6 @@ def test_restart_ssh_only_when_changes_made_and_ssh_installed( else: assert SYSTEMD_RESTART_CALL not in m_subp.call_args_list - @mock.patch(f"{MODPATH}update_ssh_config", return_value=True) @mock.patch("cloudinit.distros.subp.subp") def test_unchanged_value_does_nothing(self, m_subp, update_ssh_config): @@ -726,6 +727,3 @@ class TestSetPasswordsSchema: def test_schema_validation(self, config, expectation): with expectation: validate_cloudconfig_schema(config, get_schema(), strict=True) - - -# vi: ts=4 expandtab diff --git a/tests/unittests/distros/test_manage_service.py b/tests/unittests/distros/test_manage_service.py index 6618760b461..d7637d388a5 100644 --- a/tests/unittests/distros/test_manage_service.py +++ b/tests/unittests/distros/test_manage_service.py @@ -28,7 +28,8 @@ def test_manage_service_service_initcmd(self, m_subp, m_sysd): self.dist.init_cmd = ["service"] self.dist.manage_service("start", "myssh") m_subp.assert_called_with( - ["service", "myssh", "start"], capture=True, rcs=None) + ["service", "myssh", "start"], capture=True, rcs=None + ) @mock.patch.object(MockDistro, "uses_systemd", return_value=False) @mock.patch("cloudinit.distros.subp.subp") @@ -38,7 +39,8 @@ def test_manage_service_rcservice_initcmd(self, m_subp, m_sysd): dist.manage_service("start", "myssh") m_subp.assert_called_with( ["rc-service", "--nocolor", "myssh", "start"], - capture=True, rcs=None + capture=True, + rcs=None, ) @mock.patch("cloudinit.distros.subp.subp") @@ -56,7 +58,8 @@ def test_manage_service_rcctl_initcmd(self, m_subp): dist.init_cmd = ["rcctl"] dist.manage_service("start", "myssh") m_subp.assert_called_with( - ["rcctl", "start", "myssh"], capture=True, rcs=None) + ["rcctl", "start", "myssh"], capture=True, rcs=None + ) @mock.patch("cloudinit.distros.subp.subp") def test_manage_service_fbsd_service_initcmd(self, m_subp): @@ -64,7 +67,8 @@ def test_manage_service_fbsd_service_initcmd(self, m_subp): dist.init_cmd = ["service"] dist.manage_service("enable", "myssh") m_subp.assert_called_with( - ["service", "myssh", "enable"], capture=True, rcs=None) + ["service", "myssh", "enable"], capture=True, rcs=None + ) @mock.patch.object(MockDistro, "uses_systemd", return_value=True) @mock.patch("cloudinit.distros.subp.subp") diff --git a/tests/unittests/net/test_dhcp.py b/tests/unittests/net/test_dhcp.py index 11ee2c3286d..ed01e60b42a 100644 --- a/tests/unittests/net/test_dhcp.py +++ b/tests/unittests/net/test_dhcp.py @@ -9,12 +9,12 @@ from cloudinit.net.dhcp import ( InvalidDHCPLeaseFileError, + IscDhclient, NoDHCPLeaseError, NoDHCPLeaseInterfaceError, NoDHCPLeaseMissingDhclientError, - IscDhclient, maybe_perform_dhcp_discovery, - networkd_load_leases + networkd_load_leases, ) from cloudinit.net.ephemeral import EphemeralDHCPv4 from cloudinit.util import ensure_file, subp, write_file @@ -33,7 +33,7 @@ class TestParseDHCPLeasesFile(CiTestCase): def test_parse_empty_lease_file_errors(self): - """IscDhclient.parse_dhcp_lease_file errors when file content is empty.""" + """parse_dhcp_lease_file errors when file content is empty.""" empty_file = self.tmp_path("leases") ensure_file(empty_file) with self.assertRaises(InvalidDHCPLeaseFileError) as context_manager: @@ -42,7 +42,9 @@ def test_parse_empty_lease_file_errors(self): self.assertIn("Cannot parse empty dhcp lease file", str(error)) def test_parse_malformed_lease_file_content_errors(self): - """IscDhclient.parse_dhcp_lease_file errors when file content isn't dhcp leases.""" + """IscDhclient.parse_dhcp_lease_file errors when file content isn't + dhcp leases. + """ non_lease_file = self.tmp_path("leases") write_file(non_lease_file, "hi mom.") with self.assertRaises(InvalidDHCPLeaseFileError) as context_manager: @@ -51,7 +53,9 @@ def test_parse_malformed_lease_file_content_errors(self): self.assertIn("Cannot parse dhcp lease file", str(error)) def test_parse_multiple_leases(self): - """IscDhclient.parse_dhcp_lease_file returns a list of all leases within.""" + """IscDhclient.parse_dhcp_lease_file returns a list of all leases + within. + """ lease_file = self.tmp_path("leases") content = dedent( """ @@ -92,12 +96,16 @@ def test_parse_multiple_leases(self): }, ] write_file(lease_file, content) - self.assertCountEqual(expected, IscDhclient.parse_dhcp_lease_file(lease_file)) + self.assertCountEqual( + expected, IscDhclient.parse_dhcp_lease_file(lease_file) + ) class TestDHCPRFC3442(CiTestCase): def test_parse_lease_finds_rfc3442_classless_static_routes(self): - """IscDhclient.parse_dhcp_lease_file returns rfc3442-classless-static-routes.""" + """IscDhclient.parse_dhcp_lease_file returns + rfc3442-classless-static-routes. + """ lease_file = self.tmp_path("leases") content = dedent( """ @@ -124,7 +132,9 @@ def test_parse_lease_finds_rfc3442_classless_static_routes(self): } ] write_file(lease_file, content) - self.assertCountEqual(expected, IscDhclient.parse_dhcp_lease_file(lease_file)) + self.assertCountEqual( + expected, IscDhclient.parse_dhcp_lease_file(lease_file) + ) def test_parse_lease_finds_classless_static_routes(self): """ @@ -157,7 +167,9 @@ def test_parse_lease_finds_classless_static_routes(self): } ] write_file(lease_file, content) - self.assertCountEqual(expected, IscDhclient.parse_dhcp_lease_file(lease_file)) + self.assertCountEqual( + expected, IscDhclient.parse_dhcp_lease_file(lease_file) + ) @mock.patch("cloudinit.net.ephemeral.EphemeralIPv4Network") @mock.patch("cloudinit.net.ephemeral.maybe_perform_dhcp_discovery") @@ -175,7 +187,9 @@ def test_obtain_lease_parses_static_routes(self, m_maybe, m_ipv4): } ] m_maybe.return_value = lease - eph = EphemeralDHCPv4(MockDistro(),) + eph = EphemeralDHCPv4( + MockDistro(), + ) eph.obtain_lease() expected_kwargs = { "interface": "wlp3s0", @@ -206,7 +220,9 @@ def test_obtain_centos_lease_parses_static_routes(self, m_maybe, m_ipv4): } ] m_maybe.return_value = lease - eph = EphemeralDHCPv4(MockDistro(),) + eph = EphemeralDHCPv4( + MockDistro(), + ) eph.obtain_lease() expected_kwargs = { "interface": "wlp3s0", @@ -250,13 +266,15 @@ def test_parse_static_routes_single_ip_handles_trailing_semicolon(self): def test_parse_static_routes_default_route(self): rfc3442 = "0,130,56,240,1" self.assertEqual( - [("0.0.0.0/0", "130.56.240.1")], IscDhclient.parse_static_routes(rfc3442) + [("0.0.0.0/0", "130.56.240.1")], + IscDhclient.parse_static_routes(rfc3442), ) def test_unspecified_gateway(self): rfc3442 = "32,169,254,169,254,0,0,0,0" self.assertEqual( - [("169.254.169.254/32", "0.0.0.0")], IscDhclient.parse_static_routes(rfc3442) + [("169.254.169.254/32", "0.0.0.0")], + IscDhclient.parse_static_routes(rfc3442), ) def test_parse_static_routes_class_c_b_a(self): @@ -374,9 +392,7 @@ def test_dhclient_exits_with_error( @mock.patch("cloudinit.net.dhcp.os.remove") @mock.patch("cloudinit.net.dhcp.subp.subp") @mock.patch("cloudinit.net.dhcp.subp.which") - def test_dhcp_client_failover( - self, m_which, m_subp, m_remove, m_fallback - ): + def test_dhcp_client_failover(self, m_which, m_subp, m_remove, m_fallback): """Log and do nothing when nic is absent and no fallback is found.""" m_subp.side_effect = [ ("", ""), @@ -485,7 +501,9 @@ def test_dhcp_discovery_waits_on_lease_and_pid( # Don't create pid or leases file m_wait.return_value = [PID_F] # Return the missing pidfile wait for m_getppid.return_value = 1 # Indicate that dhclient has daemonized - self.assertEqual([], IscDhclient().dhcp_discovery("/sbin/dhclient", "eth9")) + self.assertEqual( + [], IscDhclient().dhcp_discovery("/sbin/dhclient", "eth9") + ) self.assertEqual( mock.call([PID_F, LEASE_F], maxwait=5, naplen=0.01), m_wait.call_args_list[0], @@ -696,9 +714,7 @@ def dhcp_log_func(out, err): self.assertEqual(out, dhclient_out) self.assertEqual(err, dhclient_err) - IscDhclient().dhcp_discovery( - "eth9", dhcp_log_func=dhcp_log_func - ) + IscDhclient().dhcp_discovery("eth9", dhcp_log_func=dhcp_log_func) class TestSystemdParseLeases(CiTestCase): @@ -873,7 +889,9 @@ def test_obtain_lease_raises_error(self, m_dhcp, error_class): m_dhcp.side_effect = [error_class()] with pytest.raises(error_class): - EphemeralDHCPv4(MockDistro(),).obtain_lease() + EphemeralDHCPv4( + MockDistro(), + ).obtain_lease() assert len(m_dhcp.mock_calls) == 1 @@ -881,7 +899,9 @@ def test_obtain_lease_raises_error(self, m_dhcp, error_class): def test_obtain_lease_umbrella_error(self, m_dhcp, error_class): m_dhcp.side_effect = [error_class()] with pytest.raises(NoDHCPLeaseError): - EphemeralDHCPv4(MockDistro(),).obtain_lease() + EphemeralDHCPv4( + MockDistro(), + ).obtain_lease() assert len(m_dhcp.mock_calls) == 1 @@ -890,7 +910,9 @@ def test_ctx_mgr_raises_error(self, m_dhcp, error_class): m_dhcp.side_effect = [error_class()] with pytest.raises(error_class): - with EphemeralDHCPv4(MockDistro(),): + with EphemeralDHCPv4( + MockDistro(), + ): pass assert len(m_dhcp.mock_calls) == 1 @@ -899,10 +921,9 @@ def test_ctx_mgr_raises_error(self, m_dhcp, error_class): def test_ctx_mgr_umbrella_error(self, m_dhcp, error_class): m_dhcp.side_effect = [error_class()] with pytest.raises(NoDHCPLeaseError): - with EphemeralDHCPv4(MockDistro(),): + with EphemeralDHCPv4( + MockDistro(), + ): pass assert len(m_dhcp.mock_calls) == 1 - - -# vi: ts=4 expandtab diff --git a/tests/unittests/net/test_ephemeral.py b/tests/unittests/net/test_ephemeral.py index 7e537a1acfb..ddd9912cab6 100644 --- a/tests/unittests/net/test_ephemeral.py +++ b/tests/unittests/net/test_ephemeral.py @@ -4,8 +4,8 @@ import pytest -from tests.unittests.util import MockDistro from cloudinit.net.ephemeral import EphemeralIPNetwork +from tests.unittests.util import MockDistro M_PATH = "cloudinit.net.ephemeral." @@ -33,7 +33,9 @@ def test_stack_order( expected_call_args_list.append( mock.call(m_ephemeral_dhcp_v4.return_value) ) - assert [mock.call(distro, interface)] == m_ephemeral_dhcp_v4.call_args_list + assert [ + mock.call(distro, interface) + ] == m_ephemeral_dhcp_v4.call_args_list else: assert [] == m_ephemeral_dhcp_v4.call_args_list if ipv6: diff --git a/tests/unittests/sources/test_azure.py b/tests/unittests/sources/test_azure.py index 7511557bd56..e587962f74e 100644 --- a/tests/unittests/sources/test_azure.py +++ b/tests/unittests/sources/test_azure.py @@ -4350,6 +4350,3 @@ def test_missing_secondary( } assert azure_ds.validate_imds_network_metadata(imds_md) is False - - -# vi: ts=4 expandtab diff --git a/tests/unittests/sources/test_cloudstack.py b/tests/unittests/sources/test_cloudstack.py index eb69ad54970..463a9c7a5ed 100644 --- a/tests/unittests/sources/test_cloudstack.py +++ b/tests/unittests/sources/test_cloudstack.py @@ -4,10 +4,8 @@ import time from cloudinit import helpers, util -from cloudinit.sources.DataSourceCloudStack import ( - DataSourceCloudStack, -) from cloudinit.net.dhcp import IscDhclient +from cloudinit.sources.DataSourceCloudStack import DataSourceCloudStack from tests.unittests.helpers import CiTestCase, ExitStack, mock MOD_PATH = "cloudinit.sources.DataSourceCloudStack" @@ -25,7 +23,10 @@ def setUp(self): default_gw = "192.201.20.0" get_latest_lease = mock.MagicMock(return_value=None) self.patches.enter_context( - mock.patch(mod_name + ".dhcp.IscDhclient.get_latest_lease", get_latest_lease) + mock.patch( + mod_name + ".dhcp.IscDhclient.get_latest_lease", + get_latest_lease, + ) ) get_default_gw = mock.MagicMock(return_value=default_gw) @@ -151,7 +152,8 @@ def _pop_and_test(self, files, expected): lease_d = self.tmp_dir() self._populate_dir_list(lease_d, files) self.assertEqual( - self.tmp_path(expected, lease_d), IscDhclient.get_latest_lease(lease_d) + self.tmp_path(expected, lease_d), + IscDhclient.get_latest_lease(lease_d), ) def test_skips_dhcpv6_files(self): diff --git a/tests/unittests/sources/test_ec2.py b/tests/unittests/sources/test_ec2.py index e19cd2f87b4..a72576683ff 100644 --- a/tests/unittests/sources/test_ec2.py +++ b/tests/unittests/sources/test_ec2.py @@ -1251,6 +1251,3 @@ def test_false_on_wrong_product_name(self, m_collect): product_name="Not 3DS Outscale VM".lower(), ) self.assertEqual(ec2.CloudNames.UNKNOWN, ec2.identify_platform()) - - -# vi: ts=4 expandtab diff --git a/tests/unittests/sources/test_oracle.py b/tests/unittests/sources/test_oracle.py index c864722ca5a..0f0d9011894 100644 --- a/tests/unittests/sources/test_oracle.py +++ b/tests/unittests/sources/test_oracle.py @@ -1211,6 +1211,3 @@ def test_nics( oracle_ds._network_config["config"] ), "Config not added" assert "" == caplog.text - - -# vi: ts=4 expandtab diff --git a/tests/unittests/sources/test_upcloud.py b/tests/unittests/sources/test_upcloud.py index 6d2f98c2fdd..86c40845369 100644 --- a/tests/unittests/sources/test_upcloud.py +++ b/tests/unittests/sources/test_upcloud.py @@ -333,6 +333,3 @@ def test_list_sources_finds_ds(self): ["cloudinit.sources"], ) self.assertEqual([DataSourceUpCloud], found) - - -# vi: ts=4 expandtab diff --git a/tests/unittests/sources/test_vultr.py b/tests/unittests/sources/test_vultr.py index f1a1dbc1568..ba21ae24cda 100644 --- a/tests/unittests/sources/test_vultr.py +++ b/tests/unittests/sources/test_vultr.py @@ -492,6 +492,3 @@ def test_interface_seek_route_check( pass self.assertEqual(FINAL_INTERFACE_USED, INTERFACES[3]) - - -# vi: ts=4 expandtab From 455af743cd0be817daf0e29cec5f36c32f21489e Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 13 Apr 2023 16:36:25 -0600 Subject: [PATCH 07/11] pylint --- cloudinit/net/dhcp.py | 5 ++--- cloudinit/sources/DataSourceNWCS.py | 1 + cloudinit/sources/DataSourceVultr.py | 11 ++++++++++- cloudinit/sources/helpers/vmware/imc/config_nic.py | 2 +- tests/unittests/sources/test_nwcs.py | 1 + 5 files changed, 15 insertions(+), 5 deletions(-) diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py index 7b202c6a481..61f080b91d3 100644 --- a/cloudinit/net/dhcp.py +++ b/cloudinit/net/dhcp.py @@ -61,8 +61,7 @@ def select_dhcp_client(distro): return dhcp_client except NoDHCPLeaseMissingDhclientError: LOG.warning("DHCP client not found: %s", client.client_name) - else: - raise NoDHCPLeaseMissingDhclientError() + raise NoDHCPLeaseMissingDhclientError() def maybe_perform_dhcp_discovery(distro, nic=None, dhcp_log_func=None): @@ -144,7 +143,7 @@ def clear_leases(cls): cls.kill_dhcp_client() files = glob.glob("/var/lib/dhcp/*") for file in files: - os.remove(files) + os.remove(file) @classmethod def start_service(cls, dhcp_interface: str, distro): diff --git a/cloudinit/sources/DataSourceNWCS.py b/cloudinit/sources/DataSourceNWCS.py index aebbf689a19..a147613d20f 100644 --- a/cloudinit/sources/DataSourceNWCS.py +++ b/cloudinit/sources/DataSourceNWCS.py @@ -66,6 +66,7 @@ def get_metadata(self): LOG.info("Attempting to get metadata via DHCP") with EphemeralDHCPv4( + self.distro, iface=net.find_fallback_nic(), connectivity_url_data={ "url": BASE_URL_V1 + "/metadata/instance-id", diff --git a/cloudinit/sources/DataSourceVultr.py b/cloudinit/sources/DataSourceVultr.py index fe29b585ced..7b7cc696a47 100644 --- a/cloudinit/sources/DataSourceVultr.py +++ b/cloudinit/sources/DataSourceVultr.py @@ -7,7 +7,7 @@ import cloudinit.sources.helpers.vultr as vultr from cloudinit import log as log -from cloudinit import sources, util, version +from cloudinit import sources, stages, util, version LOG = log.getLogger(__name__) BUILTIN_DS_CONFIG = { @@ -137,7 +137,16 @@ def get_datasource_list(depends): print("Machine is not a Vultr instance") sys.exit(1) + # It should probably be safe to try to detect distro via stages.Init(), + # which will fall back to Ubuntu if no distro config is found. + # this distro object is only used for dhcp fallback. Feedback from user(s) + # of __main__ would help determine if a better approach exists. + # + # we don't needReportEventStack, so reporter=True + distro = stages.Init(reporter=True).distro + md = vultr.get_metadata( + distro, BUILTIN_DS_CONFIG["url"], BUILTIN_DS_CONFIG["timeout"], BUILTIN_DS_CONFIG["retries"], diff --git a/cloudinit/sources/helpers/vmware/imc/config_nic.py b/cloudinit/sources/helpers/vmware/imc/config_nic.py index fc10a0fc0bd..b07214a228b 100644 --- a/cloudinit/sources/helpers/vmware/imc/config_nic.py +++ b/cloudinit/sources/helpers/vmware/imc/config_nic.py @@ -245,7 +245,7 @@ def generate(self, configure=False, osfamily=None): def clear_dhcp(self): logger.info("Clearing DHCP leases") - net.dhcp.clear_leases() + net.dhcp.IscDhclient.clear_leases() def configure(self, osfamily=None): """ diff --git a/tests/unittests/sources/test_nwcs.py b/tests/unittests/sources/test_nwcs.py index 052e322a90e..f96b585cda2 100644 --- a/tests/unittests/sources/test_nwcs.py +++ b/tests/unittests/sources/test_nwcs.py @@ -74,6 +74,7 @@ def test_read_data( self.assertTrue(ret) m_net.assert_called_once_with( + ds.distro, iface="eth0", connectivity_url_data={ "url": "http://169.254.169.254/api/v1/metadata/instance-id" From f1eb8acc98b0995cfb7f00c0c24adfa7f951b604 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 17 Apr 2023 10:43:19 -0600 Subject: [PATCH 08/11] fix FreeBSD service call --- cloudinit/distros/__init__.py | 4 +++- cloudinit/distros/alpine.py | 4 +++- cloudinit/distros/freebsd.py | 6 ++++-- cloudinit/distros/openbsd.py | 2 +- cloudinit/net/dhcp.py | 3 ++- 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index daec1340a52..ec1489391f0 100644 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -920,7 +920,9 @@ def shutdown_command(self, *, mode, delay, message): return args @classmethod - def manage_service(cls, action: str, service: str, rcs=None): + def manage_service( + cls, action: str, service: str, *extra_args: str, rcs=None + ): """ Perform the requested action on a service. This handles the common 'systemctl' and 'service' cases and may be overridden in subclasses diff --git a/cloudinit/distros/alpine.py b/cloudinit/distros/alpine.py index 7ba330a4361..53eebb080d6 100644 --- a/cloudinit/distros/alpine.py +++ b/cloudinit/distros/alpine.py @@ -181,7 +181,9 @@ def uses_systemd(): return False @classmethod - def manage_service(self, action: str, service: str, rcs=None): + def manage_service( + self, action: str, service: str, *extra_args: str, rcs=None + ): """ Perform the requested action on a service. This handles OpenRC specific implementation details. diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py index 48f31628339..41763312c63 100644 --- a/cloudinit/distros/freebsd.py +++ b/cloudinit/distros/freebsd.py @@ -38,7 +38,9 @@ class Distro(cloudinit.distros.bsd.BSD): home_dir = "/usr/home" @classmethod - def manage_service(cls, action: str, service: str, rcs=None): + def manage_service( + cls, action: str, service: str, *extra_args: str, rcs=None + ): """ Perform the requested action on a service. This handles FreeBSD's 'service' case. The FreeBSD 'service' is closer in features to @@ -56,7 +58,7 @@ def manage_service(cls, action: str, service: str, rcs=None): "try-reload": [service, "restart"], "status": [service, "status"], } - cmd = list(init_cmd) + list(cmds[action]) + cmd = init_cmd + cmds[action] + list(extra_args) return subp.subp(cmd, capture=True, rcs=rcs) def _get_add_member_to_group_cmd(self, member_name, group_name): diff --git a/cloudinit/distros/openbsd.py b/cloudinit/distros/openbsd.py index 9b1b989a072..5950c9db1fb 100644 --- a/cloudinit/distros/openbsd.py +++ b/cloudinit/distros/openbsd.py @@ -26,7 +26,7 @@ def _get_add_member_to_group_cmd(self, member_name, group_name): return ["usermod", "-G", group_name, member_name] @classmethod - def manage_service(cls, action: str, service: str, rcs=None): + def manage_service(cls, action: str, service: str, *extra_args, rcs=None): """ Perform the requested action on a service. This handles OpenBSD's 'rcctl'. diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py index 61f080b91d3..47f780ad165 100644 --- a/cloudinit/net/dhcp.py +++ b/cloudinit/net/dhcp.py @@ -147,7 +147,8 @@ def clear_leases(cls): @classmethod def start_service(cls, dhcp_interface: str, distro): - distro.manage_service("start", cls.client_name, rcs=[0, 1]) + distro.manage_service( + "start", cls.client_name, dhcp_interface, rcs=[0, 1]) @classmethod def stop_service(cls, dhcp_interface: str, distro): From 93238984163d87e8a46dd6c1da672fead07304b0 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 17 Apr 2023 11:56:30 -0600 Subject: [PATCH 09/11] comments --- cloudinit/net/dhcp.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py index 47f780ad165..f77c39a81f9 100644 --- a/cloudinit/net/dhcp.py +++ b/cloudinit/net/dhcp.py @@ -148,7 +148,8 @@ def clear_leases(cls): @classmethod def start_service(cls, dhcp_interface: str, distro): distro.manage_service( - "start", cls.client_name, dhcp_interface, rcs=[0, 1]) + "start", cls.client_name, dhcp_interface, rcs=[0, 1] + ) @classmethod def stop_service(cls, dhcp_interface: str, distro): @@ -167,7 +168,7 @@ def __init__(self): raise NoDHCPLeaseMissingDhclientError() @staticmethod - def parse_dhcp_lease_file(lease_file): + def parse_dhcp_lease_file(lease_file: str) -> list: """Parse the given dhcp lease file for the most recent lease. Return a list of dicts of dhcp options. Each dict contains key value From f04209f58d3faecc9fbaa801ba8cb3826331648a Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 17 Apr 2023 20:45:02 -0600 Subject: [PATCH 10/11] comments --- cloudinit/net/dhcp.py | 5 +++-- cloudinit/net/freebsd.py | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py index f77c39a81f9..6c8c2f54ab7 100644 --- a/cloudinit/net/dhcp.py +++ b/cloudinit/net/dhcp.py @@ -13,6 +13,7 @@ import signal import time from io import StringIO +from typing import Any, Dict, List import configobj @@ -168,8 +169,8 @@ def __init__(self): raise NoDHCPLeaseMissingDhclientError() @staticmethod - def parse_dhcp_lease_file(lease_file: str) -> list: - """Parse the given dhcp lease file for the most recent lease. + def parse_dhcp_lease_file(lease_file: str) -> List[Dict[str, Any]]: + """Parse the given dhcp lease file returning all leases as dicts. Return a list of dicts of dhcp options. Each dict contains key value pairs a specific lease in order from oldest to newest. diff --git a/cloudinit/net/freebsd.py b/cloudinit/net/freebsd.py index cfc41e6014a..38038e3e044 100644 --- a/cloudinit/net/freebsd.py +++ b/cloudinit/net/freebsd.py @@ -51,7 +51,7 @@ def start_services(self, run=False): for dhcp_interface in self.dhcp_interfaces(): # Observed on DragonFlyBSD 6. If we use the "restart" parameter, # the routes are not recreated. - net.dhcp.IscDhclient.start_service( + net.dhcp.IscDhclient.stop_service( dhcp_interface, distros.freebsd.Distro ) @@ -65,7 +65,7 @@ def start_services(self, run=False): subp.subp(["service", "routing", "restart"], capture=True, rcs=[0, 1]) for dhcp_interface in self.dhcp_interfaces(): - net.dhcp.IscDhclient.stop_service( + net.dhcp.IscDhclient.start_service( dhcp_interface, distros.freebsd.Distro ) From b7330d6ea8c04e1cde310831b34b7ec2cfa8d6c6 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 17 Apr 2023 21:11:02 -0600 Subject: [PATCH 11/11] comments --- cloudinit/distros/freebsd.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py index 41763312c63..77a94c6186a 100644 --- a/cloudinit/distros/freebsd.py +++ b/cloudinit/distros/freebsd.py @@ -39,7 +39,7 @@ class Distro(cloudinit.distros.bsd.BSD): @classmethod def manage_service( - cls, action: str, service: str, *extra_args: str, rcs=None + cls, action: str, service: str, *extra_args: str, rcs=None ): """ Perform the requested action on a service. This handles FreeBSD's