From aa0cd62c81866d632522bbc54dc03eb4fa7fd913 Mon Sep 17 00:00:00 2001 From: Alberto Contreras Date: Thu, 27 Apr 2023 21:11:07 +0200 Subject: gce: activate network discovery on every boot (#2128) Google wants to allow users to make changes on nics while the instance is stopped. Activate network discovery on every boot. Additionally, skip the call to `netplan generate` if the rendered config is the same on subsequent boots. --- cloudinit/net/netplan.py | 28 +++++-- cloudinit/net/renderer.py | 2 +- cloudinit/sources/DataSourceGCE.py | 7 ++ cloudinit/util.py | 14 +++- .../integration_tests/modules/test_user_events.py | 5 +- tests/integration_tests/test_networking.py | 93 ++++++++++++++++++++++ tests/unittests/net/test_netplan.py | 41 ++++++++++ tests/unittests/test_net.py | 4 +- tests/unittests/test_util.py | 21 +++++ 9 files changed, 203 insertions(+), 12 deletions(-) create mode 100644 tests/integration_tests/test_networking.py create mode 100644 tests/unittests/net/test_netplan.py diff --git a/cloudinit/net/netplan.py b/cloudinit/net/netplan.py index 1c28e16e..8b52a641 100644 --- a/cloudinit/net/netplan.py +++ b/cloudinit/net/netplan.py @@ -1,6 +1,7 @@ # This file is part of cloud-init. See LICENSE file ... import copy +import io import ipaddress import os import textwrap @@ -277,31 +278,48 @@ class Renderer(renderer.Renderer): fpnplan = os.path.join(subp.target_path(target), self.netplan_path) util.ensure_dir(os.path.dirname(fpnplan)) - header = self.netplan_header if self.netplan_header else "" # render from state content = self._render_content(network_state) + # normalize header + header = self.netplan_header if self.netplan_header else "" if not header.endswith("\n"): header += "\n" + content = header + content - mode = 0o600 if features.NETPLAN_CONFIG_ROOT_READ_ONLY else 0o644 + # determine if existing config files have the same content + same_content = False if os.path.exists(fpnplan): + hashed_content = util.hash_buffer(io.BytesIO(content.encode())) + with open(fpnplan, "rb") as f: + hashed_original_content = util.hash_buffer(f) + if hashed_content == hashed_original_content: + same_content = True + + mode = 0o600 if features.NETPLAN_CONFIG_ROOT_READ_ONLY else 0o644 + if not same_content and os.path.exists(fpnplan): current_mode = util.get_permissions(fpnplan) if current_mode & mode == current_mode: # preserve mode if existing perms are more strict than default mode = current_mode - util.write_file(fpnplan, header + content, mode=mode) + util.write_file(fpnplan, content, mode=mode) if self.clean_default: _clean_default(target=target) - self._netplan_generate(run=self._postcmds) + self._netplan_generate(run=self._postcmds, same_content=same_content) self._net_setup_link(run=self._postcmds) - def _netplan_generate(self, run=False): + def _netplan_generate(self, run: bool = False, same_content: bool = False): if not run: LOG.debug("netplan generate postcmd disabled") return + if same_content: + LOG.debug( + "skipping call to `netplan generate`." + " reason: identical netplan config" + ) + return subp.subp(self.NETPLAN_GENERATE, capture=True) def _net_setup_link(self, run=False): diff --git a/cloudinit/net/renderer.py b/cloudinit/net/renderer.py index 72813e32..c429d068 100644 --- a/cloudinit/net/renderer.py +++ b/cloudinit/net/renderer.py @@ -24,7 +24,7 @@ def filter_by_attr(match_name): filter_by_physical = filter_by_type("physical") -class Renderer: +class Renderer(abc.ABC): def __init__(self, config=None): pass diff --git a/cloudinit/sources/DataSourceGCE.py b/cloudinit/sources/DataSourceGCE.py index 041c8914..27d6089a 100644 --- a/cloudinit/sources/DataSourceGCE.py +++ b/cloudinit/sources/DataSourceGCE.py @@ -11,6 +11,7 @@ from cloudinit import dmi from cloudinit import log as logging from cloudinit import sources, url_helper, util from cloudinit.distros import ug_util +from cloudinit.event import EventScope, EventType from cloudinit.net.ephemeral import EphemeralDHCPv4 from cloudinit.sources import DataSourceHostname @@ -63,6 +64,12 @@ class DataSourceGCE(sources.DataSource): dsname = "GCE" perform_dhcp_setup = False + default_update_events = { + EventScope.NETWORK: { + EventType.BOOT_NEW_INSTANCE, + EventType.BOOT, + } + } def __init__(self, sys_cfg, distro, paths): sources.DataSource.__init__(self, sys_cfg, distro, paths) diff --git a/cloudinit/util.py b/cloudinit/util.py index fc777b82..2eb79d33 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -1725,7 +1725,7 @@ def logexc(log, msg, *args): log.debug(msg, exc_info=exc_info, *args) -def hash_blob(blob, routine, mlen=None): +def hash_blob(blob, routine: str, mlen=None) -> str: hasher = hashlib.new(routine) hasher.update(encode_text(blob)) digest = hasher.hexdigest() @@ -1736,6 +1736,18 @@ def hash_blob(blob, routine, mlen=None): return digest +def hash_buffer(f: io.BufferedIOBase) -> bytes: + """Hash the content of a binary buffer using SHA1. + + @param f: buffered binary stream to hash. + @return: digested data as bytes. + """ + hasher = hashlib.sha1() + for chunk in iter(lambda: f.read(io.DEFAULT_BUFFER_SIZE), b""): + hasher.update(chunk) + return hasher.digest() + + def is_user(name): try: if pwd.getpwnam(name): diff --git a/tests/integration_tests/modules/test_user_events.py b/tests/integration_tests/modules/test_user_events.py index 79d88022..810f8727 100644 --- a/tests/integration_tests/modules/test_user_events.py +++ b/tests/integration_tests/modules/test_user_events.py @@ -28,8 +28,7 @@ def _add_dummy_bridge_to_netplan(client: IntegrationInstance): @pytest.mark.skipif( - PLATFORM - not in ["lxd_container", "lxd_vm", "ec2", "gce", "oci", "openstack"], + PLATFORM not in ["lxd_container", "lxd_vm", "ec2", "oci", "openstack"], reason="Default boot events testing is datasource specific", ) def test_boot_event_disabled_by_default(client: IntegrationInstance): @@ -93,7 +92,7 @@ def _test_network_config_applied_on_reboot(client: IntegrationInstance): @pytest.mark.skipif( - PLATFORM != "azure", + PLATFORM not in ("azure", "gce"), reason=( f"{PLATFORM} doesn't support updates every boot event by default " "(or hasn't been testing for it)." diff --git a/tests/integration_tests/test_networking.py b/tests/integration_tests/test_networking.py new file mode 100644 index 00000000..cde69afc --- /dev/null +++ b/tests/integration_tests/test_networking.py @@ -0,0 +1,93 @@ +"""Networking-related tests.""" +import pytest +import yaml + +from tests.integration_tests.instances import IntegrationInstance +from tests.integration_tests.integration_settings import PLATFORM + + +def _add_dummy_bridge_to_netplan(client: IntegrationInstance): + # Update netplan configuration to ensure it doesn't change on reboot + netplan = yaml.safe_load( + client.execute("cat /etc/netplan/50-cloud-init.yaml") + ) + # Just a dummy bridge to do nothing + try: + netplan["network"]["bridges"]["dummy0"] = {"dhcp4": False} + except KeyError: + netplan["network"]["bridges"] = {"dummy0": {"dhcp4": False}} + + dumped_netplan = yaml.dump(netplan) + client.write_to_file("/etc/netplan/50-cloud-init.yaml", dumped_netplan) + + +USER_DATA = """\ +#cloud-config +updates: + network: + when: [boot] +""" + + +@pytest.mark.skipif( + PLATFORM not in ("lxd_container", "lxd_vm"), + reason=( + f"{PLATFORM} could make nic changes in a reboot event invalidating" + f" these tests." + ), +) +@pytest.mark.user_data(USER_DATA) +class TestNetplanGenerateBehaviorOnReboot: + def test_skip(self, client: IntegrationInstance): + log = client.read_from_file("/var/log/cloud-init.log") + assert "Applying network configuration" in log + assert "Selected renderer 'netplan'" in log + client.execute( + "mv /var/log/cloud-init.log /var/log/cloud-init.log.bak" + ) + netplan = yaml.safe_load( + client.execute("cat /etc/netplan/50-cloud-init.yaml") + ) + + client.restart() + + log = client.read_from_file("/var/log/cloud-init.log") + assert "Event Allowed: scope=network EventType=boot" in log + assert "Applying network configuration" in log + assert "Running command ['netplan', 'generate']" not in log + assert ( + "skipping call to `netplan generate`." + " reason: identical netplan config" + ) in log + netplan_new = yaml.safe_load( + client.execute("cat /etc/netplan/50-cloud-init.yaml") + ) + assert netplan == netplan_new, "no changes expected in netplan config" + + def test_applied(self, client: IntegrationInstance): + log = client.read_from_file("/var/log/cloud-init.log") + assert "Applying network configuration" in log + assert "Selected renderer 'netplan'" in log + client.execute( + "mv /var/log/cloud-init.log /var/log/cloud-init.log.bak" + ) + # fake a change in the rendered network config file + _add_dummy_bridge_to_netplan(client) + netplan = yaml.safe_load( + client.execute("cat /etc/netplan/50-cloud-init.yaml") + ) + + client.restart() + + log = client.read_from_file("/var/log/cloud-init.log") + assert "Event Allowed: scope=network EventType=boot" in log + assert "Applying network configuration" in log + assert ( + "skipping call to `netplan generate`." + " reason: identical netplan config" + ) not in log + assert "Running command ['netplan', 'generate']" in log + netplan_new = yaml.safe_load( + client.execute("cat /etc/netplan/50-cloud-init.yaml") + ) + assert netplan != netplan_new, "changes expected in netplan config" diff --git a/tests/unittests/net/test_netplan.py b/tests/unittests/net/test_netplan.py new file mode 100644 index 00000000..28b0891d --- /dev/null +++ b/tests/unittests/net/test_netplan.py @@ -0,0 +1,41 @@ +import os +from unittest import mock + +import pytest + +from cloudinit import util +from cloudinit.net import netplan + + +@pytest.fixture +def renderer(tmp_path): + config = { + "netplan_path": str(tmp_path / "netplan/50-cloud-init.yaml"), + "postcmds": True, + } + yield netplan.Renderer(config) + + +class TestNetplanRenderer: + @pytest.mark.parametrize("write_config", [True, False]) + def test_skip_netplan_generate(self, renderer, write_config, mocker): + """Check `netplan generate` is called if netplan config has changed.""" + header = "\n" + content = "foo" + renderer_mocks = mocker.patch.multiple( + renderer, + _render_content=mocker.Mock(return_value=content), + _netplan_generate=mocker.DEFAULT, + _net_setup_link=mocker.DEFAULT, + ) + if write_config: + util.ensure_dir(os.path.dirname(renderer.netplan_path)) + with open(renderer.netplan_path, "w") as f: + f.write(header) + f.write(content) + + renderer.render_network_state(mocker.Mock()) + + assert renderer_mocks["_netplan_generate"].call_args_list == [ + mock.call(run=True, same_content=write_config) + ] diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py index d7640d70..a8b18ee2 100644 --- a/tests/unittests/test_net.py +++ b/tests/unittests/test_net.py @@ -6470,7 +6470,7 @@ class TestNetplanPostcommands(CiTestCase): @mock.patch.object(netplan.Renderer, "_net_setup_link") @mock.patch("cloudinit.subp.subp") def test_netplan_render_calls_postcmds( - self, mock_subp, mock_netplan_generate, mock_net_setup_link + self, mock_subp, mock_net_setup_link, mock_netplan_generate ): tmp_dir = self.tmp_dir() ns = network_state.parse_net_config_data(self.mycfg, skip_broken=False) @@ -6485,7 +6485,7 @@ class TestNetplanPostcommands(CiTestCase): mock_subp.side_effect = iter([subp.ProcessExecutionError]) renderer.render_network_state(ns, target=render_dir) - mock_netplan_generate.assert_called_with(run=True) + mock_netplan_generate.assert_called_with(run=True, same_content=False) mock_net_setup_link.assert_called_with(run=True) @mock.patch("cloudinit.util.SeLinuxGuard") diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py index e5243ef3..c23f6399 100644 --- a/tests/unittests/test_util.py +++ b/tests/unittests/test_util.py @@ -3043,3 +3043,24 @@ class TestResolvable: assert util.is_resolvable("http://169.254.169.254/") is True assert util.is_resolvable("http://[fd00:ec2::254]/") is True assert not m_getaddr.called + + +class TestHashBuffer: + def test_in_memory(self): + buf = io.BytesIO(b"hola") + assert ( + util.hash_buffer(buf) + == b"\x99\x80\x0b\x85\xd38>:/\xb4^\xb7\xd0\x06jHy\xa9\xda\xd0" + ) + + def test_file(self, tmp_path): + content = b"hola" + file = tmp_path / "file.txt" + with file.open("wb") as f: + f.write(content) + + with file.open("rb") as f: + assert ( + util.hash_buffer(f) + == b"\x99\x80\x0b\x85\xd38>:/\xb4^\xb7\xd0\x06jHy\xa9\xda\xd0" + ) -- cgit v1.2.1