From 80aa5761105b0927ee98f3dfa2b112bf23fabbb6 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 28 Mar 2024 16:06:45 -0600 Subject: [PATCH 1/2] refactor(iproute2): Make expressions multi-line for legibility --- cloudinit/net/netops/iproute2.py | 71 +++++++++++++++++++++++--------- 1 file changed, 52 insertions(+), 19 deletions(-) diff --git a/cloudinit/net/netops/iproute2.py b/cloudinit/net/netops/iproute2.py index 08d79b187e1..8898f44b179 100644 --- a/cloudinit/net/netops/iproute2.py +++ b/cloudinit/net/netops/iproute2.py @@ -7,18 +7,18 @@ class Iproute2(netops.NetOps): @staticmethod def link_up(interface: str, family: Optional[str] = None): - subp.subp( - ["ip"] - + (["-family", family] if family else []) - + ["link", "set", "dev", interface, "up"] - ) + family_args = [] + if family: + family_args = ["-family", family] + subp.subp(["ip", *family_args, "link", "set", "dev", interface, "up"]) @staticmethod def link_down(interface: str, family: Optional[str] = None): + family_args = [] + if family: + family_args = ["-family", family] subp.subp( - ["ip"] - + (["-family", family] if family else []) - + ["link", "set", "dev", interface, "down"] + ["ip", *family_args, "link", "set", "dev", interface, "down"] ) @staticmethod @@ -29,22 +29,42 @@ def add_route( gateway: Optional[str] = None, source_address: Optional[str] = None, ): + gateway_args = [] + source_args = [] + if gateway and gateway != "0.0.0.0": + gateway_args = ["via", gateway] + if source_address: + source_args = ["src", source_address] subp.subp( - ["ip", "-4", "route", "add", route] - + (["via", gateway] if gateway and gateway != "0.0.0.0" else []) - + [ + [ + "ip", + "-4", + "route", + "add", + route, + *gateway_args, "dev", interface, + *source_args, ] - + (["src", source_address] if source_address else []), ) @staticmethod def append_route(interface: str, address: str, gateway: str): + gateway_args = [] + if gateway and gateway != "0.0.0.0": + gateway_args = ["via", gateway] subp.subp( - ["ip", "-4", "route", "append", address] - + (["via", gateway] if gateway and gateway != "0.0.0.0" else []) - + ["dev", interface] + [ + "ip", + "-4", + "route", + "append", + address, + *gateway_args, + "dev", + interface, + ] ) @staticmethod @@ -55,11 +75,24 @@ def del_route( gateway: Optional[str] = None, source_address: Optional[str] = None, ): + gateway_args = [] + source_args = [] + if gateway and gateway != "0.0.0.0": + gateway_args = ["via", gateway] + if source_address: + source_args = ["src", source_address] subp.subp( - ["ip", "-4", "route", "del", address] - + (["via", gateway] if gateway and gateway != "0.0.0.0" else []) - + ["dev", interface] - + (["src", source_address] if source_address else []) + [ + "ip", + "-4", + "route", + "del", + address, + *gateway_args, + "dev", + interface, + *source_args, + ] ) @staticmethod From 4d91563590f87c3c4201232c149c0fe4527878b4 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 28 Mar 2024 17:41:32 -0600 Subject: [PATCH 2/2] refactor(net): Reuse netops code Various different activators, datasources, and networking code implementations make use of manual iproute2 calls, which has led to much code duplication in the codebase. This is a small step towards replacing distro assumptions at call sites with common interfaces, which will simplify future refactors for more distro-agnostic code. These same abstractions will also enable simpler testing. --- cloudinit/distros/networking.py | 3 +- cloudinit/net/__init__.py | 16 +++----- cloudinit/net/activators.py | 35 ++++++++++++------ cloudinit/net/netops/__init__.py | 18 +++++++-- cloudinit/net/netops/bsd_netops.py | 18 +++++---- cloudinit/net/netops/iproute2.py | 36 +++++++++++++----- cloudinit/sources/DataSourceDigitalOcean.py | 2 +- cloudinit/sources/helpers/digitalocean.py | 23 +++--------- tests/unittests/distros/test_networking.py | 4 +- tests/unittests/test_net.py | 41 ++++----------------- tests/unittests/test_net_activators.py | 8 ++-- 11 files changed, 106 insertions(+), 98 deletions(-) diff --git a/cloudinit/distros/networking.py b/cloudinit/distros/networking.py index e63d2177d3e..af9584bdfca 100644 --- a/cloudinit/distros/networking.py +++ b/cloudinit/distros/networking.py @@ -4,6 +4,7 @@ from cloudinit import net, subp, util from cloudinit.distros.parsers import ifconfig +from cloudinit.net.netops.iproute2 import Iproute2 LOG = logging.getLogger(__name__) @@ -299,5 +300,5 @@ def try_set_link_up(self, devname: DeviceName) -> bool: """Try setting the link to up explicitly and return if it is up. Not guaranteed to bring the interface up. The caller is expected to add wait times before retrying.""" - subp.subp(["ip", "link", "set", devname, "up"]) + Iproute2.link_up(devname) return self.is_up(devname) diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py index 855009869ea..4add9838ab0 100644 --- a/cloudinit/net/__init__.py +++ b/cloudinit/net/__init__.py @@ -15,6 +15,7 @@ from urllib.parse import urlparse from cloudinit import subp, util +from cloudinit.net.netops.iproute2 import Iproute2 from cloudinit.url_helper import UrlError, readurl LOG = logging.getLogger(__name__) @@ -723,15 +724,6 @@ def _rename_interfaces( def update_byname(bymac): return dict((data["name"], data) for data in cur_info.values()) - def rename(cur, new): - subp.subp(["ip", "link", "set", cur, "name", new], capture=True) - - def down(name): - subp.subp(["ip", "link", "set", name, "down"], capture=True) - - def up(name): - subp.subp(["ip", "link", "set", name, "up"], capture=True) - ops = [] errors = [] ups = [] @@ -835,7 +827,11 @@ def find_entry(mac, driver, device_id): cur_byname = update_byname(cur_info) ops += cur_ops - opmap = {"rename": rename, "down": down, "up": up} + opmap = { + "rename": Iproute2.link_rename, + "down": Iproute2.link_down, + "up": Iproute2.link_up, + } if len(ops) + len(ups) == 0: if len(errors): diff --git a/cloudinit/net/activators.py b/cloudinit/net/activators.py index e544eae106b..ddc449b3491 100644 --- a/cloudinit/net/activators.py +++ b/cloudinit/net/activators.py @@ -1,10 +1,12 @@ # This file is part of cloud-init. See LICENSE file for license information. import logging from abc import ABC, abstractmethod -from typing import Dict, Iterable, List, Optional, Type, Union +from functools import partial +from typing import Callable, Dict, Iterable, List, Optional, Type, Union from cloudinit import subp, util from cloudinit.net.eni import available as eni_available +from cloudinit.net.netops.iproute2 import Iproute2 from cloudinit.net.netplan import available as netplan_available from cloudinit.net.network_manager import available as nm_available from cloudinit.net.network_state import NetworkState @@ -17,15 +19,24 @@ class NoActivatorException(Exception): pass -def _alter_interface(cmd, device_name) -> bool: - LOG.debug("Attempting command %s for device %s", cmd, device_name) +def _alter_interface(cmd: list, device_name: str) -> bool: + """Attempt to alter an interface using a command list""" + return _alter_interface_callable(partial(subp.subp, cmd)) + + +def _alter_interface_callable(callable: Callable) -> bool: + """Attempt to alter an interface using a callable + + this function standardizes logging and response to failure for + various activators + """ try: - (_out, err) = subp.subp(cmd) + _out, err = callable() if len(err): - LOG.warning("Running %s resulted in stderr output: %s", cmd, err) + LOG.warning("Received stderr output: %s", err) return True - except subp.ProcessExecutionError: - util.logexc(LOG, "Running interface command %s failed", cmd) + except subp.ProcessExecutionError as e: + util.logexc(LOG, "Running interface command %s failed", e.cmd) return False @@ -231,8 +242,9 @@ def available(target=None) -> bool: @staticmethod def bring_up_interface(device_name: str) -> bool: """Return True is successful, otherwise return False""" - cmd = ["ip", "link", "set", "up", device_name] - return _alter_interface(cmd, device_name) + return _alter_interface_callable( + partial(Iproute2.link_up, device_name) + ) @staticmethod def bring_up_all_interfaces(network_state: NetworkState) -> bool: @@ -243,8 +255,9 @@ def bring_up_all_interfaces(network_state: NetworkState) -> bool: @staticmethod def bring_down_interface(device_name: str) -> bool: """Return True is successful, otherwise return False""" - cmd = ["ip", "link", "set", "down", device_name] - return _alter_interface(cmd, device_name) + return _alter_interface_callable( + partial(Iproute2.link_down, device_name) + ) # This section is mostly copied and pasted from renderers.py. An abstract diff --git a/cloudinit/net/netops/__init__.py b/cloudinit/net/netops/__init__.py index 0a862918ef7..7b95917874b 100644 --- a/cloudinit/net/netops/__init__.py +++ b/cloudinit/net/netops/__init__.py @@ -1,13 +1,19 @@ from typing import Optional +from cloudinit.subp import SubpResult + class NetOps: @staticmethod - def link_up(interface: str): + def link_up(interface: str) -> SubpResult: + pass + + @staticmethod + def link_down(interface: str) -> SubpResult: pass @staticmethod - def link_down(interface: str): + def link_rename(current_name: str, new_name: str): pass @staticmethod @@ -39,9 +45,15 @@ def get_default_route() -> str: pass @staticmethod - def add_addr(interface: str, address: str, broadcast: str): + def add_addr( + interface: str, address: str, broadcast: Optional[str] = None + ): pass @staticmethod def del_addr(interface: str, address: str): pass + + @staticmethod + def flush_addr(interface: str): + pass diff --git a/cloudinit/net/netops/bsd_netops.py b/cloudinit/net/netops/bsd_netops.py index fd9ea8ca573..55f6fae5275 100644 --- a/cloudinit/net/netops/bsd_netops.py +++ b/cloudinit/net/netops/bsd_netops.py @@ -6,12 +6,12 @@ class BsdNetOps(netops.NetOps): @staticmethod - def link_up(interface: str): - subp.subp(["ifconfig", interface, "up"]) + def link_up(interface: str) -> subp.SubpResult: + return subp.subp(["ifconfig", interface, "up"]) @staticmethod - def link_down(interface: str): - subp.subp(["ifconfig", interface, "down"]) + def link_down(interface: str) -> subp.SubpResult: + return subp.subp(["ifconfig", interface, "down"]) @staticmethod def add_route( @@ -50,14 +50,18 @@ def get_default_route() -> str: return std.splitlines()[-1].strip() @staticmethod - def add_addr(interface: str, address: str, broadcast: str): + def add_addr( + interface: str, address: str, broadcast: Optional[str] = None + ): + broadcast_args = [] + if broadcast: + broadcast_args = ["broadcast", broadcast] subp.subp( [ "ifconfig", interface, address, - "broadcast", - broadcast, + *broadcast_args, "alias", ], ) diff --git a/cloudinit/net/netops/iproute2.py b/cloudinit/net/netops/iproute2.py index 8898f44b179..70c311da918 100644 --- a/cloudinit/net/netops/iproute2.py +++ b/cloudinit/net/netops/iproute2.py @@ -1,26 +1,36 @@ from typing import Optional -import cloudinit.net.netops as netops from cloudinit import subp +from cloudinit.net.netops import NetOps -class Iproute2(netops.NetOps): +class Iproute2(NetOps): @staticmethod - def link_up(interface: str, family: Optional[str] = None): + def link_up( + interface: str, family: Optional[str] = None + ) -> subp.SubpResult: family_args = [] if family: family_args = ["-family", family] - subp.subp(["ip", *family_args, "link", "set", "dev", interface, "up"]) + return subp.subp( + ["ip", *family_args, "link", "set", "dev", interface, "up"] + ) @staticmethod - def link_down(interface: str, family: Optional[str] = None): + def link_down( + interface: str, family: Optional[str] = None + ) -> subp.SubpResult: family_args = [] if family: family_args = ["-family", family] - subp.subp( + return subp.subp( ["ip", *family_args, "link", "set", "dev", interface, "down"] ) + @staticmethod + def link_rename(current_name: str, new_name: str): + subp.subp(["ip", "link", "set", current_name, "name", new_name]) + @staticmethod def add_route( interface: str, @@ -102,7 +112,12 @@ def get_default_route() -> str: ).stdout @staticmethod - def add_addr(interface: str, address: str, broadcast: str): + def add_addr( + interface: str, address: str, broadcast: Optional[str] = None + ): + broadcast_args = [] + if broadcast: + broadcast_args = ["broadcast", broadcast] subp.subp( [ "ip", @@ -111,8 +126,7 @@ def add_addr(interface: str, address: str, broadcast: str): "addr", "add", address, - "broadcast", - broadcast, + *broadcast_args, "dev", interface, ], @@ -124,3 +138,7 @@ def del_addr(interface: str, address: str): subp.subp( ["ip", "-family", "inet", "addr", "del", address, "dev", interface] ) + + @staticmethod + def flush_addr(interface: str): + subp.subp(["ip", "flush", "dev", interface]) diff --git a/cloudinit/sources/DataSourceDigitalOcean.py b/cloudinit/sources/DataSourceDigitalOcean.py index f445aefc6af..951006ed815 100644 --- a/cloudinit/sources/DataSourceDigitalOcean.py +++ b/cloudinit/sources/DataSourceDigitalOcean.py @@ -94,7 +94,7 @@ def _get_data(self): self.userdata_raw = md.get("user_data", None) if ipv4LL_nic: - do_helper.del_ipv4_link_local(ipv4LL_nic) + do_helper.del_ipv4_link_local(self.distro, ipv4LL_nic) return True diff --git a/cloudinit/sources/helpers/digitalocean.py b/cloudinit/sources/helpers/digitalocean.py index f9fd683c7da..4d0dd363bbe 100644 --- a/cloudinit/sources/helpers/digitalocean.py +++ b/cloudinit/sources/helpers/digitalocean.py @@ -8,7 +8,7 @@ from cloudinit import dmi from cloudinit import net as cloudnet -from cloudinit import subp, url_helper, util +from cloudinit import url_helper, util NIC_MAP = {"public": "eth0", "private": "eth1"} @@ -36,19 +36,10 @@ def assign_ipv4_link_local(distro, nic=None): random.randint(1, 168), random.randint(0, 255) ) - ip_addr_cmd = ["ip", "addr", "add", addr, "dev", nic] - ip_link_cmd = ["ip", "link", "set", "dev", nic, "up"] - - if not subp.which("ip"): - raise RuntimeError( - "No 'ip' command available to configure ip4LL address" - ) - try: - subp.subp(ip_addr_cmd) - LOG.debug("assigned ip4LL address '%s' to '%s'", addr, nic) - subp.subp(ip_link_cmd) - LOG.debug("brought device '%s' up", nic) + distro.net_ops.add_addr(nic, addr) + distro.net_ops.link_up(nic) + LOG.debug("brought device '%s' up with address %s", nic, addr) except Exception: util.logexc( LOG, @@ -73,7 +64,7 @@ def get_link_local_nic(distro): return min(nics, key=lambda d: cloudnet.read_sys_net_int(d, "ifindex")) -def del_ipv4_link_local(nic=None): +def del_ipv4_link_local(distro, nic=None): """Remove the ip4LL address. While this is not necessary, the ip4LL address is extraneous and confusing to users. """ @@ -86,10 +77,8 @@ def del_ipv4_link_local(nic=None): LOG.debug("cleaning up ipv4LL address") - ip_addr_cmd = ["ip", "addr", "flush", "dev", nic] - try: - subp.subp(ip_addr_cmd) + distro.net_ops.flush_addr(nic) LOG.debug("removed ip4LL addresses from %s", nic) except Exception as e: diff --git a/tests/unittests/distros/test_networking.py b/tests/unittests/distros/test_networking.py index 9a8cf507cf5..8b65f0a278b 100644 --- a/tests/unittests/distros/test_networking.py +++ b/tests/unittests/distros/test_networking.py @@ -143,7 +143,7 @@ def test_calls_subp_return_true(self, m_subp, m_is_up): is_success = LinuxNetworking().try_set_link_up(devname) assert ( - mock.call(["ip", "link", "set", devname, "up"]) + mock.call(["ip", "link", "set", "dev", devname, "up"]) == m_subp.call_args_list[-1] ) assert is_success @@ -154,7 +154,7 @@ def test_calls_subp_return_false(self, m_subp, m_is_up): is_success = LinuxNetworking().try_set_link_up(devname) assert ( - mock.call(["ip", "link", "set", devname, "up"]) + mock.call(["ip", "link", "set", "dev", devname, "up"]) == m_subp.call_args_list[-1] ) assert not is_success diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py index c71a3d50214..724b0cb5134 100644 --- a/tests/unittests/test_net.py +++ b/tests/unittests/test_net.py @@ -5072,16 +5072,13 @@ def test_rename_all(self, mock_subp): }, } net._rename_interfaces(renames, current_info=current_info) - print(mock_subp.call_args_list) mock_subp.assert_has_calls( [ mock.call( ["ip", "link", "set", "ens3", "name", "interface0"], - capture=True, ), mock.call( ["ip", "link", "set", "ens5", "name", "interface2"], - capture=True, ), ] ) @@ -5111,16 +5108,13 @@ def test_rename_no_driver_no_device_id(self, mock_subp): }, } net._rename_interfaces(renames, current_info=current_info) - print(mock_subp.call_args_list) mock_subp.assert_has_calls( [ mock.call( ["ip", "link", "set", "eth0", "name", "interface0"], - capture=True, ), mock.call( ["ip", "link", "set", "eth1", "name", "interface1"], - capture=True, ), ] ) @@ -5150,25 +5144,18 @@ def test_rename_all_bounce(self, mock_subp): }, } net._rename_interfaces(renames, current_info=current_info) - print(mock_subp.call_args_list) mock_subp.assert_has_calls( [ - mock.call(["ip", "link", "set", "ens3", "down"], capture=True), + mock.call(["ip", "link", "set", "dev", "ens3", "down"]), mock.call( ["ip", "link", "set", "ens3", "name", "interface0"], - capture=True, ), - mock.call(["ip", "link", "set", "ens5", "down"], capture=True), + mock.call(["ip", "link", "set", "dev", "ens5", "down"]), mock.call( ["ip", "link", "set", "ens5", "name", "interface2"], - capture=True, - ), - mock.call( - ["ip", "link", "set", "interface0", "up"], capture=True - ), - mock.call( - ["ip", "link", "set", "interface2", "up"], capture=True ), + mock.call(["ip", "link", "set", "dev", "interface0", "up"]), + mock.call(["ip", "link", "set", "dev", "interface2", "up"]), ] ) @@ -5197,12 +5184,9 @@ def test_rename_duplicate_macs(self, mock_subp): }, } net._rename_interfaces(renames, current_info=current_info) - print(mock_subp.call_args_list) mock_subp.assert_has_calls( [ - mock.call( - ["ip", "link", "set", "eth1", "name", "vf1"], capture=True - ), + mock.call(["ip", "link", "set", "eth1", "name", "vf1"]), ] ) @@ -5231,12 +5215,9 @@ def test_rename_duplicate_macs_driver_no_devid(self, mock_subp): }, } net._rename_interfaces(renames, current_info=current_info) - print(mock_subp.call_args_list) mock_subp.assert_has_calls( [ - mock.call( - ["ip", "link", "set", "eth1", "name", "vf1"], capture=True - ), + mock.call(["ip", "link", "set", "eth1", "name", "vf1"]), ] ) @@ -5274,15 +5255,10 @@ def test_rename_multi_mac_dups(self, mock_subp): }, } net._rename_interfaces(renames, current_info=current_info) - print(mock_subp.call_args_list) mock_subp.assert_has_calls( [ - mock.call( - ["ip", "link", "set", "eth1", "name", "vf1"], capture=True - ), - mock.call( - ["ip", "link", "set", "eth2", "name", "vf2"], capture=True - ), + mock.call(["ip", "link", "set", "eth1", "name", "vf1"]), + mock.call(["ip", "link", "set", "eth2", "name", "vf2"]), ] ) @@ -5326,7 +5302,6 @@ def test_rename_macs_case_insensitive(self, mock_subp): expected = [ mock.call( ["ip", "link", "set", "eth%d" % i, "name", "en%d" % i], - capture=True, ) for i in range(len(renames)) ] diff --git a/tests/unittests/test_net_activators.py b/tests/unittests/test_net_activators.py index d53701efafb..6a75506a905 100644 --- a/tests/unittests/test_net_activators.py +++ b/tests/unittests/test_net_activators.py @@ -234,8 +234,8 @@ def test_available(self, activator, available_calls, available_mocks): ] NETWORKD_BRING_UP_CALL_LIST: list = [ - ((["ip", "link", "set", "up", "eth0"],), {}), - ((["ip", "link", "set", "up", "eth1"],), {}), + ((["ip", "link", "set", "dev", "eth0", "up"],), {}), + ((["ip", "link", "set", "dev", "eth1", "up"],), {}), ((["systemctl", "restart", "systemd-networkd", "systemd-resolved"],), {}), ] @@ -300,8 +300,8 @@ def test_bring_up_all_interfaces_v2( ] NETWORKD_BRING_DOWN_CALL_LIST: list = [ - ((["ip", "link", "set", "down", "eth0"],), {}), - ((["ip", "link", "set", "down", "eth1"],), {}), + ((["ip", "link", "set", "dev", "eth0", "down"],), {}), + ((["ip", "link", "set", "dev", "eth1", "down"],), {}), ]