From 70ce64428119be5b21b188994b72f2ebb65a0c57 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 30 Aug 2022 14:26:03 -0500 Subject: [PATCH] Fix v2 interface matching when no MAC Commit af40478 added a regression. When parsing v2 config, matching can fail when no MAC is available. This commit fixes that behavior. LP: #1986551 --- cloudinit/cmd/devel/net_convert.py | 2 +- cloudinit/distros/__init__.py | 4 +- cloudinit/net/network_state.py | 13 ++- cloudinit/net/networkd.py | 6 +- tests/unittests/conftest.py | 10 ++ .../cloud-init-encc000.2653.nmconnection | 21 ++++ .../cloud-init-encc000.nmconnection | 12 +++ .../cloud-init-zz-all-en.nmconnection | 16 +++ .../cloud-init-zz-all-eth.nmconnection | 16 +++ .../net/artifacts/no_matching_mac_v2.yaml | 22 ++++ tests/unittests/net/test_net_rendering.py | 101 ++++++++++++++++++ 11 files changed, 212 insertions(+), 11 deletions(-) create mode 100644 tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-encc000.2653.nmconnection create mode 100644 tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-encc000.nmconnection create mode 100644 tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-zz-all-en.nmconnection create mode 100644 tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-zz-all-eth.nmconnection create mode 100644 tests/unittests/net/artifacts/no_matching_mac_v2.yaml create mode 100644 tests/unittests/net/test_net_rendering.py diff --git a/cloudinit/cmd/devel/net_convert.py b/cloudinit/cmd/devel/net_convert.py index 50e268a293..269d72cd5a 100755 --- a/cloudinit/cmd/devel/net_convert.py +++ b/cloudinit/cmd/devel/net_convert.py @@ -140,7 +140,7 @@ def handle_args(name, args): config = distro.renderer_configs.get("eni") elif args.output_kind == "netplan": r_cls = netplan.Renderer - config = distro.renderer_configs.get("netplan") + config = distro.renderer_configs.get("netplan", {}) # don't run netplan generate/apply config["postcmds"] = False # trim leading slash diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index 8de3955e34..4a468cf8ca 100644 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -16,7 +16,7 @@ import string import urllib.parse from io import StringIO -from typing import Any, Mapping, Optional, Type +from typing import Any, Mapping, MutableMapping, Optional, Type from cloudinit import importer from cloudinit import log as logging @@ -79,7 +79,7 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta): tz_zone_dir = "/usr/share/zoneinfo" default_owner = "root:root" init_cmd = ["service"] # systemctl, service etc - renderer_configs: Mapping[str, Mapping[str, Any]] = {} + renderer_configs: Mapping[str, MutableMapping[str, Any]] = {} _preferred_ntp_clients = None networking_cls: Type[Networking] = LinuxNetworking # This is used by self.shutdown_command(), and can be overridden in diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py index fd92bf0eb7..e4f7a7fdcb 100644 --- a/cloudinit/net/network_state.py +++ b/cloudinit/net/network_state.py @@ -796,7 +796,7 @@ def handle_wifis(self, command): " netplan rendering support." ) - def _v2_common(self, cfg): + def _v2_common(self, cfg) -> None: LOG.debug("v2_common: handling config:\n%s", cfg) for iface, dev_cfg in cfg.items(): if "set-name" in dev_cfg: @@ -813,10 +813,13 @@ def _v2_common(self, cfg): name_cmd.update({"address": dns}) self.handle_nameserver(name_cmd) - mac_address = dev_cfg.get("match", {}).get("macaddress") - real_if_name = find_interface_name_from_mac(mac_address) - if real_if_name: - iface = real_if_name + mac_address: Optional[str] = dev_cfg.get("match", {}).get( + "macaddress" + ) + if mac_address: + real_if_name = find_interface_name_from_mac(mac_address) + if real_if_name: + iface = real_if_name self._handle_individual_nameserver(name_cmd, iface) diff --git a/cloudinit/net/networkd.py b/cloudinit/net/networkd.py index abfc1037ee..e0a5d84814 100644 --- a/cloudinit/net/networkd.py +++ b/cloudinit/net/networkd.py @@ -255,7 +255,7 @@ def _render_content(self, ns): self.parse_routes(route, cfg) if ns.version == 2: - name = iface["name"] + name: Optional[str] = iface["name"] # network state doesn't give dhcp domain info # using ns.config as a workaround here @@ -270,8 +270,8 @@ def _render_content(self, ns): if dev_cfg.get("set-name") == name: name = dev_name break - - self.dhcp_domain(ns.config["ethernets"][name], cfg) + if name in ns.config["ethernets"]: + self.dhcp_domain(ns.config["ethernets"][name], cfg) ret_dict.update({link: cfg.get_final_conf()}) diff --git a/tests/unittests/conftest.py b/tests/unittests/conftest.py index e265a285ca..1ab17e8b10 100644 --- a/tests/unittests/conftest.py +++ b/tests/unittests/conftest.py @@ -1,6 +1,7 @@ import builtins import glob import os +from pathlib import Path import pytest @@ -55,3 +56,12 @@ def fake_filesystem(mocker, tmpdir): func = getattr(mod, f) trap_func = retarget_many_wrapper(str(tmpdir), nargs, func) mocker.patch.object(mod, f, trap_func) + + +PYTEST_VERSION_TUPLE = tuple(map(int, pytest.__version__.split("."))) + +if PYTEST_VERSION_TUPLE < (3, 9, 0): + + @pytest.fixture + def tmp_path(tmpdir): + return Path(tmpdir) diff --git a/tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-encc000.2653.nmconnection b/tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-encc000.2653.nmconnection new file mode 100644 index 0000000000..80483d4f00 --- /dev/null +++ b/tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-encc000.2653.nmconnection @@ -0,0 +1,21 @@ +# Generated by cloud-init. Changes will be lost. + +[connection] +id=cloud-init encc000.2653 +uuid=116aaf19-aabc-50ea-b480-e9aee18bda59 +type=vlan +interface-name=encc000.2653 + +[user] +org.freedesktop.NetworkManager.origin=cloud-init + +[vlan] +id=2653 +parent=f869ebd3-f175-5747-bf02-d0d44d687248 + +[ipv4] +method=manual +may-fail=false +address1=10.245.236.14/24 +gateway=10.245.236.1 +dns=10.245.236.1; diff --git a/tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-encc000.nmconnection b/tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-encc000.nmconnection new file mode 100644 index 0000000000..3368388d4a --- /dev/null +++ b/tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-encc000.nmconnection @@ -0,0 +1,12 @@ +# Generated by cloud-init. Changes will be lost. + +[connection] +id=cloud-init encc000 +uuid=f869ebd3-f175-5747-bf02-d0d44d687248 +type=ethernet +interface-name=encc000 + +[user] +org.freedesktop.NetworkManager.origin=cloud-init + +[ethernet] diff --git a/tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-zz-all-en.nmconnection b/tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-zz-all-en.nmconnection new file mode 100644 index 0000000000..16120bc179 --- /dev/null +++ b/tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-zz-all-en.nmconnection @@ -0,0 +1,16 @@ +# Generated by cloud-init. Changes will be lost. + +[connection] +id=cloud-init zz-all-en +uuid=159daec9-cba3-5101-85e7-46d831857f43 +type=ethernet +interface-name=zz-all-en + +[user] +org.freedesktop.NetworkManager.origin=cloud-init + +[ethernet] + +[ipv4] +method=auto +may-fail=false diff --git a/tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-zz-all-eth.nmconnection b/tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-zz-all-eth.nmconnection new file mode 100644 index 0000000000..df44d546c8 --- /dev/null +++ b/tests/unittests/net/artifacts/no_matching_mac/etc/NetworkManager/system-connections/cloud-init-zz-all-eth.nmconnection @@ -0,0 +1,16 @@ +# Generated by cloud-init. Changes will be lost. + +[connection] +id=cloud-init zz-all-eth +uuid=23a83d8a-d7db-5133-a77b-e68a6ac61ec9 +type=ethernet +interface-name=zz-all-eth + +[user] +org.freedesktop.NetworkManager.origin=cloud-init + +[ethernet] + +[ipv4] +method=auto +may-fail=false diff --git a/tests/unittests/net/artifacts/no_matching_mac_v2.yaml b/tests/unittests/net/artifacts/no_matching_mac_v2.yaml new file mode 100644 index 0000000000..f5fc5ef10a --- /dev/null +++ b/tests/unittests/net/artifacts/no_matching_mac_v2.yaml @@ -0,0 +1,22 @@ +network: + version: 2 + ethernets: + encc000: {} + zz-all-en: + match: + name: "en*" + dhcp4: true + zz-all-eth: + match: + name: "eth*" + dhcp4: true + vlans: + encc000.2653: + id: 2653 + link: "encc000" + addresses: + - "10.245.236.14/24" + gateway4: "10.245.236.1" + nameservers: + addresses: + - "10.245.236.1" diff --git a/tests/unittests/net/test_net_rendering.py b/tests/unittests/net/test_net_rendering.py new file mode 100644 index 0000000000..06feab8914 --- /dev/null +++ b/tests/unittests/net/test_net_rendering.py @@ -0,0 +1,101 @@ +"""Home of the tests for end-to-end net rendering + +Tests defined here should take a v1 or v2 yaml config as input, and verify +that the rendered network config is as expected. Input files are defined +under `tests/unittests/net/artifacts` with the format of + +.yaml + +For example, if my test name is "test_all_the_things" and I'm testing a +v2 format, I should have a file named test_all_the_things_v2.yaml. + +If a renderer outputs multiple files, the expected files should live in +the artifacts directory under the given test name. For example, if I'm +expecting NetworkManager to output a file named eth0.nmconnection as +part of my "test_all_the_things" test, then in the artifacts directory +there should be a +`test_all_the_things/etc/NetworkManager/system-connections/eth0.nmconnection` +file. + +To add a new nominal test, create the input and output files, then add the test +name to the `test_convert` test along with it's supported renderers. + +Before adding a test here, check that it is not already represented +in `unittests/test_net.py`. While that file contains similar tests, it has +become too large to be maintainable. +""" +import glob +from enum import Flag, auto +from pathlib import Path + +import pytest + +from cloudinit import safeyaml +from cloudinit.net.netplan import Renderer as NetplanRenderer +from cloudinit.net.network_manager import Renderer as NetworkManagerRenderer +from cloudinit.net.network_state import NetworkState, parse_net_config_data + +ARTIFACT_DIR = Path(__file__).parent.absolute() / "artifacts" + + +class Renderer(Flag): + Netplan = auto() + NetworkManager = auto() + Networkd = auto() + + +@pytest.fixture(autouse=True) +def setup(mocker): + mocker.patch("cloudinit.net.network_state.get_interfaces_by_mac") + + +def _check_netplan( + network_state: NetworkState, netplan_path: Path, expected_config +): + if network_state.version == 2: + renderer = NetplanRenderer(config={"netplan_path": netplan_path}) + renderer.render_network_state(network_state) + assert safeyaml.load(netplan_path.read_text()) == expected_config, ( + f"Netplan config generated at {netplan_path} does not match v2 " + "config defined for this test." + ) + else: + raise NotImplementedError + + +def _check_network_manager(network_state: NetworkState, tmp_path: Path): + renderer = NetworkManagerRenderer() + renderer.render_network_state( + network_state, target=str(tmp_path / "no_matching_mac") + ) + expected_paths = glob.glob( + str(ARTIFACT_DIR / "no_matching_mac" / "**/*.nmconnection"), + recursive=True, + ) + for expected_path in expected_paths: + expected_contents = Path(expected_path).read_text() + actual_path = tmp_path / expected_path.split( + str(ARTIFACT_DIR), maxsplit=1 + )[1].lstrip("/") + assert ( + actual_path.exists() + ), f"Expected {actual_path} to exist, but it does not" + actual_contents = actual_path.read_text() + assert expected_contents.strip() == actual_contents.strip() + + +@pytest.mark.parametrize( + "test_name, renderers", + [("no_matching_mac_v2", Renderer.Netplan | Renderer.NetworkManager)], +) +def test_convert(test_name, renderers, tmp_path): + network_config = safeyaml.load( + Path(ARTIFACT_DIR, f"{test_name}.yaml").read_text() + ) + network_state = parse_net_config_data(network_config["network"]) + if Renderer.Netplan in renderers: + _check_netplan( + network_state, tmp_path / "netplan.yaml", network_config + ) + if Renderer.NetworkManager in renderers: + _check_network_manager(network_state, tmp_path)