From c5e949c02a1d5226ae9b1cb39846db19d223c6c2 Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Wed, 25 Mar 2020 09:44:16 -0400 Subject: distros/tests/test_init: add tests for _get_package_mirror_info (#272) --- cloudinit/distros/tests/__init__.py | 0 cloudinit/distros/tests/test_init.py | 83 ++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 cloudinit/distros/tests/__init__.py create mode 100644 cloudinit/distros/tests/test_init.py (limited to 'cloudinit/distros/tests') diff --git a/cloudinit/distros/tests/__init__.py b/cloudinit/distros/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/cloudinit/distros/tests/test_init.py b/cloudinit/distros/tests/test_init.py new file mode 100644 index 00000000..707a0a49 --- /dev/null +++ b/cloudinit/distros/tests/test_init.py @@ -0,0 +1,83 @@ +# Copyright (C) 2020 Canonical Ltd. +# +# Author: Daniel Watkins +# +# This file is part of cloud-init. See LICENSE file for license information. +"""Tests for cloudinit/distros/__init__.py""" + +from unittest import mock + +import pytest + +from cloudinit.distros import _get_package_mirror_info + + +class TestGetPackageMirrorInfo: + """ + Tests for cloudinit.distros._get_package_mirror_info. + + These supplement the tests in tests/unittests/test_distros/test_generic.py + which are more focused on testing a single production-like configuration. + These tests are more focused on specific aspects of the unit under test. + """ + + @pytest.mark.parametrize('mirror_info,expected', [ + # Empty info gives empty return + ({}, {}), + # failsafe values used if present + ({'failsafe': {'primary': 'value', 'security': 'other'}}, + {'primary': 'value', 'security': 'other'}), + # search values used if present + ({'search': {'primary': ['value'], 'security': ['other']}}, + {'primary': ['value'], 'security': ['other']}), + # failsafe values used if search value not present + ({'search': {'primary': ['value']}, 'failsafe': {'security': 'other'}}, + {'primary': ['value'], 'security': 'other'}) + ]) + def test_get_package_mirror_info_failsafe(self, mirror_info, expected): + """ + Test the interaction between search and failsafe inputs + + (This doesn't test the case where the mirror_filter removes all search + options; test_failsafe_used_if_all_search_results_filtered_out covers + that.) + """ + assert expected == _get_package_mirror_info(mirror_info, + mirror_filter=lambda x: x) + + def test_failsafe_used_if_all_search_results_filtered_out(self): + """Test the failsafe option used if all search options eliminated.""" + mirror_info = { + 'search': {'primary': ['value']}, 'failsafe': {'primary': 'other'} + } + assert {'primary': 'other'} == _get_package_mirror_info( + mirror_info, mirror_filter=lambda x: False) + + @pytest.mark.parametrize('availability_zone,region,patterns,expected', ( + # Test ec2_region alone + ('fk-fake-1f', None, ['EC2-%(ec2_region)s'], ['EC2-fk-fake-1']), + # Test availability_zone alone + ('fk-fake-1f', None, ['AZ-%(availability_zone)s'], ['AZ-fk-fake-1f']), + # Test region alone + (None, 'fk-fake-1', ['RG-%(region)s'], ['RG-fk-fake-1']), + # Test that ec2_region is not available for non-matching AZs + ('fake-fake-1f', None, + ['EC2-%(ec2_region)s', 'AZ-%(availability_zone)s'], + ['AZ-fake-fake-1f']), + # Test that template order maintained + (None, 'fake-region', ['RG-%(region)s-2', 'RG-%(region)s-1'], + ['RG-fake-region-2', 'RG-fake-region-1']), + )) + def test_substitution(self, availability_zone, region, patterns, expected): + """Test substitution works as expected.""" + m_data_source = mock.Mock( + availability_zone=availability_zone, region=region + ) + mirror_info = {'search': {'primary': patterns}} + + ret = _get_package_mirror_info( + mirror_info, + data_source=m_data_source, + mirror_filter=lambda x: x + ) + assert {'primary': expected} == ret -- cgit v1.2.3 From c478d0bff412c67280dfe8f08568de733f9425a1 Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Tue, 31 Mar 2020 13:52:21 -0400 Subject: distros: replace invalid characters in mirror URLs with hyphens (#291) This modifies _get_package_mirror_info to convert the hostnames of generated mirror URLs to their IDNA form, and then iterate through them replacing any invalid characters (i.e. anything other than letters, digits or a hyphen) with a hyphen. This commit introduces the following changes in behaviour: * generated mirror URLs with Unicode characters in their hostnames will have their hostnames converted to their all-ASCII IDNA form * generated mirror URLs with invalid-for-hostname characters in their hostname will have those characters converted to hyphens * generated mirror URLs which cannot be parsed by `urllib.parse.urlsplit` will not be considered for use * other configured patterns will still be considered * if all configured patterns fail to produce a URL that parses then the fallback mirror URL will be used LP: #1868232 --- cloudinit/distros/__init__.py | 109 ++++++++++++++++++++++++++++++++++- cloudinit/distros/tests/test_init.py | 84 +++++++++++++++++++++------ 2 files changed, 174 insertions(+), 19 deletions(-) (limited to 'cloudinit/distros/tests') diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index 92598a2d..86a22c3e 100755 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -13,6 +13,8 @@ import abc import os import re import stat +import string +import urllib.parse from io import StringIO from cloudinit import importer @@ -50,6 +52,9 @@ _EC2_AZ_RE = re.compile('^[a-z][a-z]-(?:[a-z]+-)+[0-9][a-z]$') # Default NTP Client Configurations PREFERRED_NTP_CLIENTS = ['chrony', 'systemd-timesyncd', 'ntp', 'ntpdate'] +# Letters/Digits/Hyphen characters, for use in domain name validation +LDH_ASCII_CHARS = string.ascii_letters + string.digits + "-" + class Distro(metaclass=abc.ABCMeta): @@ -720,6 +725,102 @@ class Distro(metaclass=abc.ABCMeta): LOG.info("Added user '%s' to group '%s'", member, name) +def _apply_hostname_transformations_to_url(url: str, transformations: list): + """ + Apply transformations to a URL's hostname, return transformed URL. + + This is a separate function because unwrapping and rewrapping only the + hostname portion of a URL is complex. + + :param url: + The URL to operate on. + :param transformations: + A list of ``(str) -> Optional[str]`` functions, which will be applied + in order to the hostname portion of the URL. If any function + (regardless of ordering) returns None, ``url`` will be returned without + any modification. + + :return: + A string whose value is ``url`` with the hostname ``transformations`` + applied, or ``None`` if ``url`` is unparseable. + """ + try: + parts = urllib.parse.urlsplit(url) + except ValueError: + # If we can't even parse the URL, we shouldn't use it for anything + return None + new_hostname = parts.hostname + + for transformation in transformations: + new_hostname = transformation(new_hostname) + if new_hostname is None: + # If a transformation returns None, that indicates we should abort + # processing and return `url` unmodified + return url + + new_netloc = new_hostname + if parts.port is not None: + new_netloc = "{}:{}".format(new_netloc, parts.port) + return urllib.parse.urlunsplit(parts._replace(netloc=new_netloc)) + + +def _sanitize_mirror_url(url: str): + """ + Given a mirror URL, replace or remove any invalid URI characters. + + This performs the following actions on the URL's hostname: + * Checks if it is an IP address, returning the URL immediately if it is + * Converts it to its IDN form (see below for details) + * Replaces any non-Letters/Digits/Hyphen (LDH) characters in it with + hyphens + * TODO: Remove any leading/trailing hyphens from each domain name label + + Before we replace any invalid domain name characters, we first need to + ensure that any valid non-ASCII characters in the hostname will not be + replaced, by ensuring the hostname is in its Internationalized domain name + (IDN) representation (see RFC 5890). This conversion has to be applied to + the whole hostname (rather than just the substitution variables), because + the Punycode algorithm used by IDNA transcodes each part of the hostname as + a whole string (rather than encoding individual characters). It cannot be + applied to the whole URL, because (a) the Punycode algorithm expects to + operate on domain names so doesn't output a valid URL, and (b) non-ASCII + characters in non-hostname parts of the URL aren't encoded via Punycode. + + To put this in RFC 5890's terminology: before we remove or replace any + characters from our domain name (which we do to ensure that each label is a + valid LDH Label), we first ensure each label is in its A-label form. + + (Note that Python's builtin idna encoding is actually IDNA2003, not + IDNA2008. This changes the specifics of how some characters are encoded to + ASCII, but doesn't affect the logic here.) + + :param url: + The URL to operate on. + + :return: + A sanitized version of the URL, which will have been IDNA encoded if + necessary, or ``None`` if the generated string is not a parseable URL. + """ + # Acceptable characters are LDH characters, plus "." to separate each label + acceptable_chars = LDH_ASCII_CHARS + "." + transformations = [ + # This is an IP address, not a hostname, so no need to apply the + # transformations + lambda hostname: None if net.is_ip_address(hostname) else hostname, + + # Encode with IDNA to get the correct characters (as `bytes`), then + # decode with ASCII so we return a `str` + lambda hostname: hostname.encode('idna').decode('ascii'), + + # Replace any unacceptable characters with "-" + lambda hostname: ''.join( + c if c in acceptable_chars else "-" for c in hostname + ), + ] + + return _apply_hostname_transformations_to_url(url, transformations) + + def _get_package_mirror_info(mirror_info, data_source=None, mirror_filter=util.search_for_mirror): # given a arch specific 'mirror_info' entry (from package_mirrors) @@ -748,9 +849,13 @@ def _get_package_mirror_info(mirror_info, data_source=None, mirrors = [] for tmpl in searchlist: try: - mirrors.append(tmpl % subst) + mirror = tmpl % subst except KeyError: - pass + continue + + mirror = _sanitize_mirror_url(mirror) + if mirror is not None: + mirrors.append(mirror) found = mirror_filter(mirrors) if found: diff --git a/cloudinit/distros/tests/test_init.py b/cloudinit/distros/tests/test_init.py index 707a0a49..daa81ab8 100644 --- a/cloudinit/distros/tests/test_init.py +++ b/cloudinit/distros/tests/test_init.py @@ -9,7 +9,18 @@ from unittest import mock import pytest -from cloudinit.distros import _get_package_mirror_info +from cloudinit.distros import _get_package_mirror_info, LDH_ASCII_CHARS + + +# Define a set of characters we would expect to be replaced +INVALID_URL_CHARS = [ + chr(x) for x in range(127) if chr(x) not in LDH_ASCII_CHARS +] +for separator in [":", ".", "/", "#", "?", "@", "[", "]"]: + # Remove from the set characters that either separate hostname parts (":", + # "."), terminate hostnames ("/", "#", "?", "@"), or cause Python to be + # unable to parse URLs ("[", "]"). + INVALID_URL_CHARS.remove(separator) class TestGetPackageMirrorInfo: @@ -25,14 +36,16 @@ class TestGetPackageMirrorInfo: # Empty info gives empty return ({}, {}), # failsafe values used if present - ({'failsafe': {'primary': 'value', 'security': 'other'}}, - {'primary': 'value', 'security': 'other'}), + ({'failsafe': {'primary': 'http://value', 'security': 'http://other'}}, + {'primary': 'http://value', 'security': 'http://other'}), # search values used if present - ({'search': {'primary': ['value'], 'security': ['other']}}, - {'primary': ['value'], 'security': ['other']}), + ({'search': {'primary': ['http://value'], + 'security': ['http://other']}}, + {'primary': ['http://value'], 'security': ['http://other']}), # failsafe values used if search value not present - ({'search': {'primary': ['value']}, 'failsafe': {'security': 'other'}}, - {'primary': ['value'], 'security': 'other'}) + ({'search': {'primary': ['http://value']}, + 'failsafe': {'security': 'http://other'}}, + {'primary': ['http://value'], 'security': 'http://other'}) ]) def test_get_package_mirror_info_failsafe(self, mirror_info, expected): """ @@ -48,26 +61,63 @@ class TestGetPackageMirrorInfo: def test_failsafe_used_if_all_search_results_filtered_out(self): """Test the failsafe option used if all search options eliminated.""" mirror_info = { - 'search': {'primary': ['value']}, 'failsafe': {'primary': 'other'} + 'search': {'primary': ['http://value']}, + 'failsafe': {'primary': 'http://other'} } - assert {'primary': 'other'} == _get_package_mirror_info( + assert {'primary': 'http://other'} == _get_package_mirror_info( mirror_info, mirror_filter=lambda x: False) @pytest.mark.parametrize('availability_zone,region,patterns,expected', ( # Test ec2_region alone - ('fk-fake-1f', None, ['EC2-%(ec2_region)s'], ['EC2-fk-fake-1']), + ('fk-fake-1f', None, ['http://EC2-%(ec2_region)s/ubuntu'], + ['http://ec2-fk-fake-1/ubuntu']), # Test availability_zone alone - ('fk-fake-1f', None, ['AZ-%(availability_zone)s'], ['AZ-fk-fake-1f']), + ('fk-fake-1f', None, ['http://AZ-%(availability_zone)s/ubuntu'], + ['http://az-fk-fake-1f/ubuntu']), # Test region alone - (None, 'fk-fake-1', ['RG-%(region)s'], ['RG-fk-fake-1']), + (None, 'fk-fake-1', ['http://RG-%(region)s/ubuntu'], + ['http://rg-fk-fake-1/ubuntu']), # Test that ec2_region is not available for non-matching AZs ('fake-fake-1f', None, - ['EC2-%(ec2_region)s', 'AZ-%(availability_zone)s'], - ['AZ-fake-fake-1f']), + ['http://EC2-%(ec2_region)s/ubuntu', + 'http://AZ-%(availability_zone)s/ubuntu'], + ['http://az-fake-fake-1f/ubuntu']), # Test that template order maintained - (None, 'fake-region', ['RG-%(region)s-2', 'RG-%(region)s-1'], - ['RG-fake-region-2', 'RG-fake-region-1']), - )) + (None, 'fake-region', + ['http://RG-%(region)s-2/ubuntu', 'http://RG-%(region)s-1/ubuntu'], + ['http://rg-fake-region-2/ubuntu', 'http://rg-fake-region-1/ubuntu']), + # Test that non-ASCII hostnames are IDNA encoded; + # "IDNA-ТεЅТ̣".encode('idna') == b"xn--idna--4kd53hh6aba3q" + (None, 'ТεЅТ̣', ['http://www.IDNA-%(region)s.com/ubuntu'], + ['http://www.xn--idna--4kd53hh6aba3q.com/ubuntu']), + # Test that non-ASCII hostnames with a port are IDNA encoded; + # "IDNA-ТεЅТ̣".encode('idna') == b"xn--idna--4kd53hh6aba3q" + (None, 'ТεЅТ̣', ['http://www.IDNA-%(region)s.com:8080/ubuntu'], + ['http://www.xn--idna--4kd53hh6aba3q.com:8080/ubuntu']), + # Test that non-ASCII non-hostname parts of URLs are unchanged + (None, 'ТεЅТ̣', ['http://www.example.com/%(region)s/ubuntu'], + ['http://www.example.com/ТεЅТ̣/ubuntu']), + # Test that IPv4 addresses are unchanged + (None, 'fk-fake-1', ['http://192.168.1.1:8080/%(region)s/ubuntu'], + ['http://192.168.1.1:8080/fk-fake-1/ubuntu']), + # Test that IPv6 addresses are unchanged + (None, 'fk-fake-1', + ['http://[2001:67c:1360:8001::23]/%(region)s/ubuntu'], + ['http://[2001:67c:1360:8001::23]/fk-fake-1/ubuntu']), + # Test that unparseable URLs are filtered out of the mirror list + (None, 'inv[lid', + ['http://%(region)s.in.hostname/should/be/filtered', + 'http://but.not.in.the.path/%(region)s'], + ['http://but.not.in.the.path/inv[lid']), + ) + ( + # Dynamically generate a test case for each non-LDH + # (Letters/Digits/Hyphen) ASCII character, testing that it is + # substituted with a hyphen + tuple( + (None, 'fk{0}fake{0}1'.format(invalid_char), + ['http://%(region)s/ubuntu'], ['http://fk-fake-1/ubuntu']) + for invalid_char in INVALID_URL_CHARS)) + ) def test_substitution(self, availability_zone, region, patterns, expected): """Test substitution works as expected.""" m_data_source = mock.Mock( -- cgit v1.2.3 From 1bbc4908ff7a2be19483811b3b6fee6ebc916235 Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Tue, 31 Mar 2020 17:04:17 -0400 Subject: distros: drop leading/trailing hyphens from mirror URL labels (#296) * distros/tests/test_init: drop needless brackets/indentation * distros: drop leading/trailing hyphens from mirror URL labels --- cloudinit/distros/__init__.py | 5 +++++ cloudinit/distros/tests/test_init.py | 14 ++++++++------ 2 files changed, 13 insertions(+), 6 deletions(-) (limited to 'cloudinit/distros/tests') diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index 86a22c3e..e63b8c7a 100755 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -816,6 +816,11 @@ def _sanitize_mirror_url(url: str): lambda hostname: ''.join( c if c in acceptable_chars else "-" for c in hostname ), + + # Drop leading/trailing hyphens from each part of the hostname + lambda hostname: '.'.join( + part.strip('-') for part in hostname.split('.') + ), ] return _apply_hostname_transformations_to_url(url, transformations) diff --git a/cloudinit/distros/tests/test_init.py b/cloudinit/distros/tests/test_init.py index daa81ab8..40939133 100644 --- a/cloudinit/distros/tests/test_init.py +++ b/cloudinit/distros/tests/test_init.py @@ -109,15 +109,17 @@ class TestGetPackageMirrorInfo: ['http://%(region)s.in.hostname/should/be/filtered', 'http://but.not.in.the.path/%(region)s'], ['http://but.not.in.the.path/inv[lid']), - ) + ( + (None, '-some-region-', + ['http://-lead-ing.%(region)s.trail-ing-.example.com/ubuntu'], + ['http://lead-ing.some-region.trail-ing.example.com/ubuntu']), + ) + tuple( # Dynamically generate a test case for each non-LDH # (Letters/Digits/Hyphen) ASCII character, testing that it is # substituted with a hyphen - tuple( - (None, 'fk{0}fake{0}1'.format(invalid_char), - ['http://%(region)s/ubuntu'], ['http://fk-fake-1/ubuntu']) - for invalid_char in INVALID_URL_CHARS)) - ) + (None, 'fk{0}fake{0}1'.format(invalid_char), + ['http://%(region)s/ubuntu'], ['http://fk-fake-1/ubuntu']) + for invalid_char in INVALID_URL_CHARS + )) def test_substitution(self, availability_zone, region, patterns, expected): """Test substitution works as expected.""" m_data_source = mock.Mock( -- cgit v1.2.3 From 882f1a5f2d5bafd08e6900a2782c3affa67c9d86 Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Tue, 30 Jun 2020 14:19:38 -0400 Subject: networking: refactor is_physical from cloudinit.net (#457) As the first refactor PR, this also includes the initial structure for tests. LP: #1884619 --- cloudinit/distros/networking.py | 16 ++- cloudinit/distros/tests/test_networking.py | 42 ++++++ cloudinit/net/__init__.py | 4 - cloudinit/net/tests/test_init.py | 6 - cloudinit/sources/DataSourceDigitalOcean.py | 2 +- cloudinit/sources/DataSourceOpenNebula.py | 30 +++-- cloudinit/sources/helpers/digitalocean.py | 12 +- tests/unittests/test_datasource/test_opennebula.py | 149 +++++++++++++-------- 8 files changed, 181 insertions(+), 80 deletions(-) create mode 100644 cloudinit/distros/tests/test_networking.py (limited to 'cloudinit/distros/tests') diff --git a/cloudinit/distros/networking.py b/cloudinit/distros/networking.py index eecdccc6..e421a2ce 100644 --- a/cloudinit/distros/networking.py +++ b/cloudinit/distros/networking.py @@ -1,4 +1,5 @@ import abc +import os from cloudinit import net @@ -79,8 +80,15 @@ class Networking(metaclass=abc.ABCMeta): def is_bridge(self, devname: DeviceName) -> bool: return net.is_bridge(devname) + @abc.abstractmethod def is_physical(self, devname: DeviceName) -> bool: - return net.is_physical(devname) + """ + Is ``devname`` a physical network device? + + Examples of non-physical network devices: bonds, bridges, tunnels, + loopback devices. + """ + pass def is_renamed(self, devname: DeviceName) -> bool: return net.is_renamed(devname) @@ -103,7 +111,8 @@ class Networking(metaclass=abc.ABCMeta): class BSDNetworking(Networking): """Implementation of networking functionality shared across BSDs.""" - pass + def is_physical(self, devname: DeviceName) -> bool: + raise NotImplementedError() class LinuxNetworking(Networking): @@ -126,3 +135,6 @@ class LinuxNetworking(Networking): def is_netfail_standby(self, devname: DeviceName) -> bool: return net.is_netfail_standby(devname) + + def is_physical(self, devname: DeviceName) -> bool: + return os.path.exists(net.sys_dev_path(devname, "device")) diff --git a/cloudinit/distros/tests/test_networking.py b/cloudinit/distros/tests/test_networking.py new file mode 100644 index 00000000..2acb12f4 --- /dev/null +++ b/cloudinit/distros/tests/test_networking.py @@ -0,0 +1,42 @@ +from unittest import mock + +import pytest + +from cloudinit.distros.networking import BSDNetworking, LinuxNetworking + + +@pytest.yield_fixture +def sys_class_net(tmpdir): + sys_class_net_path = tmpdir.join("sys/class/net") + sys_class_net_path.ensure_dir() + with mock.patch( + "cloudinit.net.get_sys_class_path", + return_value=sys_class_net_path.strpath + "/", + ): + yield sys_class_net_path + + +class TestBSDNetworkingIsPhysical: + def test_raises_notimplementederror(self): + with pytest.raises(NotImplementedError): + BSDNetworking().is_physical("eth0") + + +class TestLinuxNetworkingIsPhysical: + def test_returns_false_by_default(self, sys_class_net): + assert not LinuxNetworking().is_physical("eth0") + + def test_returns_false_if_devname_exists_but_not_physical( + self, sys_class_net + ): + devname = "eth0" + sys_class_net.join(devname).mkdir() + assert not LinuxNetworking().is_physical(devname) + + def test_returns_true_if_device_is_physical(self, sys_class_net): + devname = "eth0" + device_dir = sys_class_net.join(devname) + device_dir.mkdir() + device_dir.join("device").write("") + + assert LinuxNetworking().is_physical(devname) diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py index fd5a1b3e..9d8c7ba9 100644 --- a/cloudinit/net/__init__.py +++ b/cloudinit/net/__init__.py @@ -262,10 +262,6 @@ def is_vlan(devname): return 'DEVTYPE=vlan' in uevent.splitlines() -def is_physical(devname): - return os.path.exists(sys_dev_path(devname, "device")) - - def device_driver(devname): """Return the device driver for net device named 'devname'.""" driver = None diff --git a/cloudinit/net/tests/test_init.py b/cloudinit/net/tests/test_init.py index e36d4387..eb458c39 100644 --- a/cloudinit/net/tests/test_init.py +++ b/cloudinit/net/tests/test_init.py @@ -198,12 +198,6 @@ class TestReadSysNet(CiTestCase): write_file(os.path.join(self.sysdir, 'eth0', 'uevent'), content) self.assertTrue(net.is_vlan('eth0')) - def test_is_physical(self): - """is_physical is True when /sys/net/devname/device exists.""" - self.assertFalse(net.is_physical('eth0')) - ensure_file(os.path.join(self.sysdir, 'eth0', 'device')) - self.assertTrue(net.is_physical('eth0')) - class TestGenerateFallbackConfig(CiTestCase): diff --git a/cloudinit/sources/DataSourceDigitalOcean.py b/cloudinit/sources/DataSourceDigitalOcean.py index e0ef665e..5040ce5b 100644 --- a/cloudinit/sources/DataSourceDigitalOcean.py +++ b/cloudinit/sources/DataSourceDigitalOcean.py @@ -58,7 +58,7 @@ class DataSourceDigitalOcean(sources.DataSource): ipv4LL_nic = None if self.use_ip4LL: - ipv4LL_nic = do_helper.assign_ipv4_link_local() + ipv4LL_nic = do_helper.assign_ipv4_link_local(self.distro) md = do_helper.read_metadata( self.metadata_address, timeout=self.timeout, diff --git a/cloudinit/sources/DataSourceOpenNebula.py b/cloudinit/sources/DataSourceOpenNebula.py index c7fdc2a5..12b1f94f 100644 --- a/cloudinit/sources/DataSourceOpenNebula.py +++ b/cloudinit/sources/DataSourceOpenNebula.py @@ -13,6 +13,7 @@ # This file is part of cloud-init. See LICENSE file for license information. import collections +import functools import os import pwd import re @@ -60,10 +61,19 @@ class DataSourceOpenNebula(sources.DataSource): for cdev in candidates: try: if os.path.isdir(self.seed_dir): - results = read_context_disk_dir(cdev, asuser=parseuser) + results = read_context_disk_dir( + cdev, self.distro, asuser=parseuser + ) elif cdev.startswith("/dev"): - results = util.mount_cb(cdev, read_context_disk_dir, - data=parseuser) + # util.mount_cb only handles passing a single argument + # through to the wrapped function, so we have to partially + # apply the function to pass in `distro`. See LP: #1884979 + partially_applied_func = functools.partial( + read_context_disk_dir, + asuser=parseuser, + distro=self.distro, + ) + results = util.mount_cb(cdev, partially_applied_func) except NonContextDiskDir: continue except BrokenContextDiskDir as exc: @@ -129,10 +139,10 @@ class BrokenContextDiskDir(Exception): class OpenNebulaNetwork(object): - def __init__(self, context, system_nics_by_mac=None): + def __init__(self, context, distro, system_nics_by_mac=None): self.context = context if system_nics_by_mac is None: - system_nics_by_mac = get_physical_nics_by_mac() + system_nics_by_mac = get_physical_nics_by_mac(distro) self.ifaces = collections.OrderedDict( [k for k in sorted(system_nics_by_mac.items(), key=lambda k: net.natural_sort_key(k[1]))]) @@ -367,7 +377,7 @@ def parse_shell_config(content, keylist=None, bash=None, asuser=None, return ret -def read_context_disk_dir(source_dir, asuser=None): +def read_context_disk_dir(source_dir, distro, asuser=None): """ read_context_disk_dir(source_dir): read source_dir and return a tuple with metadata dict and user-data @@ -450,15 +460,17 @@ def read_context_disk_dir(source_dir, asuser=None): # http://docs.opennebula.org/5.4/operation/references/template.html#context-section ipaddr_keys = [k for k in context if re.match(r'^ETH\d+_IP.*$', k)] if ipaddr_keys: - onet = OpenNebulaNetwork(context) + onet = OpenNebulaNetwork(context, distro) results['network-interfaces'] = onet.gen_conf() return results -def get_physical_nics_by_mac(): +def get_physical_nics_by_mac(distro): devs = net.get_interfaces_by_mac() - return dict([(m, n) for m, n in devs.items() if net.is_physical(n)]) + return dict( + [(m, n) for m, n in devs.items() if distro.networking.is_physical(n)] + ) # Legacy: Must be present in case we load an old pkl object diff --git a/cloudinit/sources/helpers/digitalocean.py b/cloudinit/sources/helpers/digitalocean.py index f5bbe46a..b545c4d6 100644 --- a/cloudinit/sources/helpers/digitalocean.py +++ b/cloudinit/sources/helpers/digitalocean.py @@ -16,7 +16,7 @@ NIC_MAP = {'public': 'eth0', 'private': 'eth1'} LOG = logging.getLogger(__name__) -def assign_ipv4_link_local(nic=None): +def assign_ipv4_link_local(distro, nic=None): """Bring up NIC using an address using link-local (ip4LL) IPs. On DigitalOcean, the link-local domain is per-droplet routed, so there is no risk of collisions. However, to be more safe, the ip4LL @@ -24,7 +24,7 @@ def assign_ipv4_link_local(nic=None): """ if not nic: - nic = get_link_local_nic() + nic = get_link_local_nic(distro) LOG.debug("selected interface '%s' for reading metadata", nic) if not nic: @@ -54,8 +54,12 @@ def assign_ipv4_link_local(nic=None): return nic -def get_link_local_nic(): - nics = [f for f in cloudnet.get_devicelist() if cloudnet.is_physical(f)] +def get_link_local_nic(distro): + nics = [ + f + for f in cloudnet.get_devicelist() + if distro.networking.is_physical(f) + ] if not nics: return None return min(nics, key=lambda d: cloudnet.read_sys_net_int(d, 'ifindex')) diff --git a/tests/unittests/test_datasource/test_opennebula.py b/tests/unittests/test_datasource/test_opennebula.py index 7c859c8a..80841182 100644 --- a/tests/unittests/test_datasource/test_opennebula.py +++ b/tests/unittests/test_datasource/test_opennebula.py @@ -132,18 +132,18 @@ class TestOpenNebulaDataSource(CiTestCase): def test_seed_dir_non_contextdisk(self): self.assertRaises(ds.NonContextDiskDir, ds.read_context_disk_dir, - self.seed_dir) + self.seed_dir, mock.Mock()) def test_seed_dir_empty1_context(self): populate_dir(self.seed_dir, {'context.sh': ''}) - results = ds.read_context_disk_dir(self.seed_dir) + results = ds.read_context_disk_dir(self.seed_dir, mock.Mock()) self.assertIsNone(results['userdata']) self.assertEqual(results['metadata'], {}) def test_seed_dir_empty2_context(self): populate_context_dir(self.seed_dir, {}) - results = ds.read_context_disk_dir(self.seed_dir) + results = ds.read_context_disk_dir(self.seed_dir, mock.Mock()) self.assertIsNone(results['userdata']) self.assertEqual(results['metadata'], {}) @@ -153,11 +153,11 @@ class TestOpenNebulaDataSource(CiTestCase): self.assertRaises(ds.BrokenContextDiskDir, ds.read_context_disk_dir, - self.seed_dir) + self.seed_dir, mock.Mock()) def test_context_parser(self): populate_context_dir(self.seed_dir, TEST_VARS) - results = ds.read_context_disk_dir(self.seed_dir) + results = ds.read_context_disk_dir(self.seed_dir, mock.Mock()) self.assertTrue('metadata' in results) self.assertEqual(TEST_VARS, results['metadata']) @@ -168,7 +168,7 @@ class TestOpenNebulaDataSource(CiTestCase): for k in ('SSH_KEY', 'SSH_PUBLIC_KEY'): my_d = os.path.join(self.tmp, "%s-%i" % (k, c)) populate_context_dir(my_d, {k: '\n'.join(public_keys)}) - results = ds.read_context_disk_dir(my_d) + results = ds.read_context_disk_dir(my_d, mock.Mock()) self.assertTrue('metadata' in results) self.assertTrue('public-keys' in results['metadata']) @@ -182,7 +182,7 @@ class TestOpenNebulaDataSource(CiTestCase): my_d = os.path.join(self.tmp, k) populate_context_dir(my_d, {k: USER_DATA, 'USERDATA_ENCODING': ''}) - results = ds.read_context_disk_dir(my_d) + results = ds.read_context_disk_dir(my_d, mock.Mock()) self.assertTrue('userdata' in results) self.assertEqual(USER_DATA, results['userdata']) @@ -192,7 +192,7 @@ class TestOpenNebulaDataSource(CiTestCase): for k in ('USER_DATA', 'USERDATA'): my_d = os.path.join(self.tmp, k) populate_context_dir(my_d, {k: b64userdata}) - results = ds.read_context_disk_dir(my_d) + results = ds.read_context_disk_dir(my_d, mock.Mock()) self.assertTrue('userdata' in results) self.assertEqual(b64userdata, results['userdata']) @@ -202,7 +202,7 @@ class TestOpenNebulaDataSource(CiTestCase): my_d = os.path.join(self.tmp, k) populate_context_dir(my_d, {k: util.b64e(USER_DATA), 'USERDATA_ENCODING': 'base64'}) - results = ds.read_context_disk_dir(my_d) + results = ds.read_context_disk_dir(my_d, mock.Mock()) self.assertTrue('userdata' in results) self.assertEqual(USER_DATA, results['userdata']) @@ -214,7 +214,7 @@ class TestOpenNebulaDataSource(CiTestCase): for k in ('HOSTNAME', 'PUBLIC_IP', 'IP_PUBLIC', 'ETH0_IP'): my_d = os.path.join(self.tmp, k) populate_context_dir(my_d, {k: PUBLIC_IP}) - results = ds.read_context_disk_dir(my_d) + results = ds.read_context_disk_dir(my_d, mock.Mock()) self.assertTrue('metadata' in results) self.assertTrue('local-hostname' in results['metadata']) @@ -229,7 +229,7 @@ class TestOpenNebulaDataSource(CiTestCase): # without ETH0_MAC # for Older OpenNebula? populate_context_dir(self.seed_dir, {'ETH0_IP': IP_BY_MACADDR}) - results = ds.read_context_disk_dir(self.seed_dir) + results = ds.read_context_disk_dir(self.seed_dir, mock.Mock()) self.assertTrue('network-interfaces' in results) self.assertTrue( @@ -239,7 +239,7 @@ class TestOpenNebulaDataSource(CiTestCase): # ETH0_IP and ETH0_MAC populate_context_dir( self.seed_dir, {'ETH0_IP': IP_BY_MACADDR, 'ETH0_MAC': MACADDR}) - results = ds.read_context_disk_dir(self.seed_dir) + results = ds.read_context_disk_dir(self.seed_dir, mock.Mock()) self.assertTrue('network-interfaces' in results) self.assertTrue( @@ -251,7 +251,7 @@ class TestOpenNebulaDataSource(CiTestCase): # "AR = [ TYPE = ETHER ]" populate_context_dir( self.seed_dir, {'ETH0_IP': '', 'ETH0_MAC': MACADDR}) - results = ds.read_context_disk_dir(self.seed_dir) + results = ds.read_context_disk_dir(self.seed_dir, mock.Mock()) self.assertTrue('network-interfaces' in results) self.assertTrue( @@ -265,7 +265,7 @@ class TestOpenNebulaDataSource(CiTestCase): 'ETH0_MAC': MACADDR, 'ETH0_MASK': '255.255.0.0' }) - results = ds.read_context_disk_dir(self.seed_dir) + results = ds.read_context_disk_dir(self.seed_dir, mock.Mock()) self.assertTrue('network-interfaces' in results) self.assertTrue( @@ -279,7 +279,7 @@ class TestOpenNebulaDataSource(CiTestCase): 'ETH0_MAC': MACADDR, 'ETH0_MASK': '' }) - results = ds.read_context_disk_dir(self.seed_dir) + results = ds.read_context_disk_dir(self.seed_dir, mock.Mock()) self.assertTrue('network-interfaces' in results) self.assertTrue( @@ -292,7 +292,7 @@ class TestOpenNebulaDataSource(CiTestCase): 'ETH0_IP6': IP6_GLOBAL, 'ETH0_MAC': MACADDR, }) - results = ds.read_context_disk_dir(self.seed_dir) + results = ds.read_context_disk_dir(self.seed_dir, mock.Mock()) self.assertTrue('network-interfaces' in results) self.assertTrue( @@ -305,7 +305,7 @@ class TestOpenNebulaDataSource(CiTestCase): 'ETH0_IP6_ULA': IP6_ULA, 'ETH0_MAC': MACADDR, }) - results = ds.read_context_disk_dir(self.seed_dir) + results = ds.read_context_disk_dir(self.seed_dir, mock.Mock()) self.assertTrue('network-interfaces' in results) self.assertTrue( @@ -319,7 +319,7 @@ class TestOpenNebulaDataSource(CiTestCase): 'ETH0_IP6_PREFIX_LENGTH': IP6_PREFIX, 'ETH0_MAC': MACADDR, }) - results = ds.read_context_disk_dir(self.seed_dir) + results = ds.read_context_disk_dir(self.seed_dir, mock.Mock()) self.assertTrue('network-interfaces' in results) self.assertTrue( @@ -333,7 +333,7 @@ class TestOpenNebulaDataSource(CiTestCase): 'ETH0_IP6_PREFIX_LENGTH': '', 'ETH0_MAC': MACADDR, }) - results = ds.read_context_disk_dir(self.seed_dir) + results = ds.read_context_disk_dir(self.seed_dir, mock.Mock()) self.assertTrue('network-interfaces' in results) self.assertTrue( @@ -370,7 +370,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): expected = { '02:00:0a:12:01:01': 'ETH0', '02:00:0a:12:0f:0f': 'ETH1', } - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) self.assertEqual(expected, net.context_devname) def test_get_nameservers(self): @@ -385,21 +385,21 @@ class TestOpenNebulaNetwork(unittest.TestCase): expected = { 'addresses': ['1.2.3.6', '1.2.3.7', '1.2.3.8'], 'search': ['example.com', 'example.org']} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_nameservers('eth0') self.assertEqual(expected, val) def test_get_mtu(self): """Verify get_mtu('device') correctly returns MTU size.""" context = {'ETH0_MTU': '1280'} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_mtu('eth0') self.assertEqual('1280', val) def test_get_ip(self): """Verify get_ip('device') correctly returns IPv4 address.""" context = {'ETH0_IP': PUBLIC_IP} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_ip('eth0', MACADDR) self.assertEqual(PUBLIC_IP, val) @@ -410,7 +410,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): string. """ context = {'ETH0_IP': ''} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_ip('eth0', MACADDR) self.assertEqual(IP_BY_MACADDR, val) @@ -423,7 +423,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'ETH0_IP6': IP6_GLOBAL, 'ETH0_IP6_ULA': '', } expected = [IP6_GLOBAL] - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_ip6('eth0') self.assertEqual(expected, val) @@ -436,7 +436,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'ETH0_IP6': '', 'ETH0_IP6_ULA': IP6_ULA, } expected = [IP6_ULA] - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_ip6('eth0') self.assertEqual(expected, val) @@ -449,7 +449,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'ETH0_IP6': IP6_GLOBAL, 'ETH0_IP6_ULA': IP6_ULA, } expected = [IP6_GLOBAL, IP6_ULA] - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_ip6('eth0') self.assertEqual(expected, val) @@ -458,7 +458,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): Verify get_ip6_prefix('device') correctly returns IPv6 prefix. """ context = {'ETH0_IP6_PREFIX_LENGTH': IP6_PREFIX} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_ip6_prefix('eth0') self.assertEqual(IP6_PREFIX, val) @@ -469,7 +469,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): string. """ context = {'ETH0_IP6_PREFIX_LENGTH': ''} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_ip6_prefix('eth0') self.assertEqual('64', val) @@ -479,7 +479,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): address. """ context = {'ETH0_GATEWAY': '1.2.3.5'} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_gateway('eth0') self.assertEqual('1.2.3.5', val) @@ -489,7 +489,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): address. """ context = {'ETH0_GATEWAY6': IP6_GW} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_gateway6('eth0') self.assertEqual(IP6_GW, val) @@ -498,7 +498,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): Verify get_mask('device') correctly returns IPv4 subnet mask. """ context = {'ETH0_MASK': '255.255.0.0'} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_mask('eth0') self.assertEqual('255.255.0.0', val) @@ -508,7 +508,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): It returns default value '255.255.255.0' if ETH0_MASK has empty string. """ context = {'ETH0_MASK': ''} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_mask('eth0') self.assertEqual('255.255.255.0', val) @@ -517,7 +517,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): Verify get_network('device') correctly returns IPv4 network address. """ context = {'ETH0_NETWORK': '1.2.3.0'} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_network('eth0', MACADDR) self.assertEqual('1.2.3.0', val) @@ -528,7 +528,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): empty string. """ context = {'ETH0_NETWORK': ''} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_network('eth0', MACADDR) self.assertEqual('10.18.1.0', val) @@ -537,7 +537,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): Verify get_field('device', 'name') returns *context* value. """ context = {'ETH9_DUMMY': 'DUMMY_VALUE'} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_field('eth9', 'dummy') self.assertEqual('DUMMY_VALUE', val) @@ -547,7 +547,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): value. """ context = {'ETH9_DUMMY': 'DUMMY_VALUE'} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_field('eth9', 'dummy', 'DEFAULT_VALUE') self.assertEqual('DUMMY_VALUE', val) @@ -557,7 +557,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): value if context value is empty string. """ context = {'ETH9_DUMMY': ''} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_field('eth9', 'dummy', 'DEFAULT_VALUE') self.assertEqual('DEFAULT_VALUE', val) @@ -567,7 +567,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): empty string. """ context = {'ETH9_DUMMY': ''} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_field('eth9', 'dummy') self.assertEqual(None, val) @@ -577,7 +577,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): None. """ context = {'ETH9_DUMMY': None} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) val = net.get_field('eth9', 'dummy') self.assertEqual(None, val) @@ -597,7 +597,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'match': {'macaddress': MACADDR}, 'addresses': [IP_BY_MACADDR + '/' + IP4_PREFIX]}}} m_get_phys_by_mac.return_value = {MACADDR: nic} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) self.assertEqual(net.gen_conf(), expected) # set ETH0_GATEWAY @@ -613,7 +613,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'match': {'macaddress': MACADDR}, 'addresses': [IP_BY_MACADDR + '/' + IP4_PREFIX]}}} m_get_phys_by_mac.return_value = {MACADDR: nic} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) self.assertEqual(net.gen_conf(), expected) @mock.patch(DS_PATH + ".get_physical_nics_by_mac") @@ -632,7 +632,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'match': {'macaddress': MACADDR}, 'addresses': [IP_BY_MACADDR + '/' + IP4_PREFIX]}}} m_get_phys_by_mac.return_value = {MACADDR: nic} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) self.assertEqual(net.gen_conf(), expected) # set ETH0_GATEWAY6 @@ -648,7 +648,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'match': {'macaddress': MACADDR}, 'addresses': [IP_BY_MACADDR + '/' + IP4_PREFIX]}}} m_get_phys_by_mac.return_value = {MACADDR: nic} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) self.assertEqual(net.gen_conf(), expected) @mock.patch(DS_PATH + ".get_physical_nics_by_mac") @@ -669,7 +669,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'match': {'macaddress': MACADDR}, 'addresses': [IP_BY_MACADDR + '/' + IP4_PREFIX]}}} m_get_phys_by_mac.return_value = {MACADDR: nic} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) self.assertEqual(net.gen_conf(), expected) # set ETH0_IP6, ETH0_IP6_ULA, ETH0_IP6_PREFIX_LENGTH @@ -689,7 +689,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): IP6_GLOBAL + '/' + IP6_PREFIX, IP6_ULA + '/' + IP6_PREFIX]}}} m_get_phys_by_mac.return_value = {MACADDR: nic} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) self.assertEqual(net.gen_conf(), expected) @mock.patch(DS_PATH + ".get_physical_nics_by_mac") @@ -710,7 +710,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'match': {'macaddress': MACADDR}, 'addresses': [IP_BY_MACADDR + '/' + IP4_PREFIX]}}} m_get_phys_by_mac.return_value = {MACADDR: nic} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) self.assertEqual(net.gen_conf(), expected) # set DNS, ETH0_DNS, ETH0_SEARCH_DOMAIN @@ -730,7 +730,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'match': {'macaddress': MACADDR}, 'addresses': [IP_BY_MACADDR + '/' + IP4_PREFIX]}}} m_get_phys_by_mac.return_value = {MACADDR: nic} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) self.assertEqual(net.gen_conf(), expected) @mock.patch(DS_PATH + ".get_physical_nics_by_mac") @@ -749,7 +749,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'match': {'macaddress': MACADDR}, 'addresses': [IP_BY_MACADDR + '/' + IP4_PREFIX]}}} m_get_phys_by_mac.return_value = {MACADDR: nic} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) self.assertEqual(net.gen_conf(), expected) # set ETH0_MTU @@ -765,14 +765,14 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'match': {'macaddress': MACADDR}, 'addresses': [IP_BY_MACADDR + '/' + IP4_PREFIX]}}} m_get_phys_by_mac.return_value = {MACADDR: nic} - net = ds.OpenNebulaNetwork(context) + net = ds.OpenNebulaNetwork(context, mock.Mock()) self.assertEqual(net.gen_conf(), expected) @mock.patch(DS_PATH + ".get_physical_nics_by_mac") def test_eth0(self, m_get_phys_by_mac): for nic in self.system_nics: m_get_phys_by_mac.return_value = {MACADDR: nic} - net = ds.OpenNebulaNetwork({}) + net = ds.OpenNebulaNetwork({}, mock.Mock()) expected = { 'version': 2, 'ethernets': { @@ -782,6 +782,14 @@ class TestOpenNebulaNetwork(unittest.TestCase): self.assertEqual(net.gen_conf(), expected) + @mock.patch(DS_PATH + ".get_physical_nics_by_mac") + def test_distro_passed_through(self, m_get_physical_nics_by_mac): + ds.OpenNebulaNetwork({}, mock.sentinel.distro) + self.assertEqual( + [mock.call(mock.sentinel.distro)], + m_get_physical_nics_by_mac.call_args_list, + ) + def test_eth0_override(self): self.maxDiff = None context = { @@ -800,7 +808,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'ETH0_SEARCH_DOMAIN': '', } for nic in self.system_nics: - net = ds.OpenNebulaNetwork(context, + net = ds.OpenNebulaNetwork(context, mock.Mock(), system_nics_by_mac={MACADDR: nic}) expected = { 'version': 2, @@ -832,7 +840,7 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'ETH0_SEARCH_DOMAIN': 'example.com example.org', } for nic in self.system_nics: - net = ds.OpenNebulaNetwork(context, + net = ds.OpenNebulaNetwork(context, mock.Mock(), system_nics_by_mac={MACADDR: nic}) expected = { @@ -886,7 +894,10 @@ class TestOpenNebulaNetwork(unittest.TestCase): 'ETH3_SEARCH_DOMAIN': 'third.example.com third.example.org', } net = ds.OpenNebulaNetwork( - context, system_nics_by_mac={MAC_1: 'enp0s25', MAC_2: 'enp1s2'}) + context, + mock.Mock(), + system_nics_by_mac={MAC_1: 'enp0s25', MAC_2: 'enp1s2'} + ) expected = { 'version': 2, @@ -926,6 +937,36 @@ class TestParseShellConfig: assert ret == {"foo": "bar", "xx": "foo"} +class TestGetPhysicalNicsByMac: + @pytest.mark.parametrize( + "interfaces_by_mac,physical_devs,expected_return", + [ + # No interfaces => empty return + ({}, [], {}), + # Only virtual interface => empty return + ({"mac1": "virtual0"}, [], {}), + # Only physical interface => it is returned + ({"mac2": "physical0"}, ["physical0"], {"mac2": "physical0"}), + # Combination of physical and virtual => only physical returned + ( + {"mac3": "physical1", "mac4": "virtual1"}, + ["physical1"], + {"mac3": "physical1"}, + ), + ], + ) + def test(self, interfaces_by_mac, physical_devs, expected_return): + distro = mock.Mock() + distro.networking.is_physical.side_effect = ( + lambda devname: devname in physical_devs + ) + with mock.patch( + DS_PATH + ".net.get_interfaces_by_mac", + return_value=interfaces_by_mac, + ): + assert expected_return == ds.get_physical_nics_by_mac(distro) + + def populate_context_dir(path, variables): data = "# Context variables generated by OpenNebula\n" for k, v in variables.items(): -- cgit v1.2.3 From 3fcdacc8995d6908858aceaf1da7ee5ff090fc04 Mon Sep 17 00:00:00 2001 From: lucasmoura Date: Tue, 30 Jun 2020 19:25:26 -0300 Subject: Disable ec2 mirror for non aws instances (#390) For versions before 20.2, we allowed the use of ec2 mirrors if the datasource availability_zone matches one of the ec2 regions. We are now updating that behavior to allow allow the use of ec2 mirrors on ec2 instances or if the user directly passes an an ec2 mirror url through #cloud-config apt directives. LP: #1456277 --- cloudinit/distros/__init__.py | 10 +- cloudinit/distros/tests/test_init.py | 35 +++- cloudinit/features.py | 11 ++ tests/unittests/test_distros/test_generic.py | 186 +++++++++++++-------- .../test_handler_apt_configure_sources_list_v1.py | 6 +- .../test_handler/test_handler_apt_source_v1.py | 7 + .../test_handler/test_handler_apt_source_v3.py | 27 ++- 7 files changed, 193 insertions(+), 89 deletions(-) (limited to 'cloudinit/distros/tests') diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index 89940cf0..2fc91bbc 100755 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -28,6 +28,9 @@ from cloudinit import type_utils from cloudinit import subp from cloudinit import util +from cloudinit.features import \ + ALLOW_EC2_MIRRORS_ON_NON_AWS_INSTANCE_TYPES + from cloudinit.distros.parsers import hosts from .networking import LinuxNetworking @@ -849,7 +852,12 @@ def _get_package_mirror_info(mirror_info, data_source=None, # ec2 availability zones are named cc-direction-[0-9][a-d] (us-east-1b) # the region is us-east-1. so region = az[0:-1] if _EC2_AZ_RE.match(data_source.availability_zone): - subst['ec2_region'] = "%s" % data_source.availability_zone[0:-1] + ec2_region = data_source.availability_zone[0:-1] + + if ALLOW_EC2_MIRRORS_ON_NON_AWS_INSTANCE_TYPES: + subst['ec2_region'] = "%s" % ec2_region + elif data_source.platform_type == "ec2": + subst['ec2_region'] = "%s" % ec2_region if data_source and data_source.region: subst['region'] = data_source.region diff --git a/cloudinit/distros/tests/test_init.py b/cloudinit/distros/tests/test_init.py index 40939133..db534654 100644 --- a/cloudinit/distros/tests/test_init.py +++ b/cloudinit/distros/tests/test_init.py @@ -67,6 +67,9 @@ class TestGetPackageMirrorInfo: assert {'primary': 'http://other'} == _get_package_mirror_info( mirror_info, mirror_filter=lambda x: False) + @pytest.mark.parametrize('allow_ec2_mirror, platform_type', [ + (True, 'ec2') + ]) @pytest.mark.parametrize('availability_zone,region,patterns,expected', ( # Test ec2_region alone ('fk-fake-1f', None, ['http://EC2-%(ec2_region)s/ubuntu'], @@ -120,16 +123,34 @@ class TestGetPackageMirrorInfo: ['http://%(region)s/ubuntu'], ['http://fk-fake-1/ubuntu']) for invalid_char in INVALID_URL_CHARS )) - def test_substitution(self, availability_zone, region, patterns, expected): + def test_valid_substitution(self, + allow_ec2_mirror, + platform_type, + availability_zone, + region, + patterns, + expected): """Test substitution works as expected.""" + flag_path = "cloudinit.distros." \ + "ALLOW_EC2_MIRRORS_ON_NON_AWS_INSTANCE_TYPES" + m_data_source = mock.Mock( - availability_zone=availability_zone, region=region + availability_zone=availability_zone, + region=region, + platform_type=platform_type ) mirror_info = {'search': {'primary': patterns}} - ret = _get_package_mirror_info( - mirror_info, - data_source=m_data_source, - mirror_filter=lambda x: x - ) + with mock.patch(flag_path, allow_ec2_mirror): + ret = _get_package_mirror_info( + mirror_info, + data_source=m_data_source, + mirror_filter=lambda x: x + ) + print(allow_ec2_mirror) + print(platform_type) + print(availability_zone) + print(region) + print(patterns) + print(expected) assert {'primary': expected} == ret diff --git a/cloudinit/features.py b/cloudinit/features.py index e455213d..c44fa29e 100644 --- a/cloudinit/features.py +++ b/cloudinit/features.py @@ -26,6 +26,17 @@ After the 20.2 release, we instead raise an exception. This flag can be removed after Focal is no longer supported """ + +ALLOW_EC2_MIRRORS_ON_NON_AWS_INSTANCE_TYPES = False +""" +When configuring apt mirrors, old behavior is to allow +the use of ec2 mirrors if the datasource availability_zone format +matches one of the possible aws ec2 regions. After the 20.2 release, we +no longer publish ec2 region mirror urls on non-AWS cloud platforms. +Besides feature_overrides.py, users can override this by providing +#cloud-config apt directives. +""" + try: # pylint: disable=wildcard-import from cloudinit.feature_overrides import * # noqa diff --git a/tests/unittests/test_distros/test_generic.py b/tests/unittests/test_distros/test_generic.py index 6b6c1566..44607489 100644 --- a/tests/unittests/test_distros/test_generic.py +++ b/tests/unittests/test_distros/test_generic.py @@ -6,6 +6,7 @@ from cloudinit import util from cloudinit.tests import helpers import os +import pytest import shutil import tempfile from unittest import mock @@ -37,24 +38,6 @@ gapmi = distros._get_arch_package_mirror_info class TestGenericDistro(helpers.FilesystemMockingTestCase): - def return_first(self, mlist): - if not mlist: - return None - return mlist[0] - - def return_second(self, mlist): - if not mlist: - return None - return mlist[1] - - def return_none(self, _mlist): - return None - - def return_last(self, mlist): - if not mlist: - return None - return(mlist[-1]) - def setUp(self): super(TestGenericDistro, self).setUp() # Make a temp directoy for tests to use. @@ -145,61 +128,6 @@ class TestGenericDistro(helpers.FilesystemMockingTestCase): arch_mirrors = gapmi(package_mirrors, arch="amd64") self.assertEqual(package_mirrors[0], arch_mirrors) - def test_get_package_mirror_info_az_ec2(self): - arch_mirrors = gapmi(package_mirrors, arch="amd64") - data_source_mock = mock.Mock(availability_zone="us-east-1a") - - results = gpmi(arch_mirrors, data_source=data_source_mock, - mirror_filter=self.return_first) - self.assertEqual(results, - {'primary': 'http://us-east-1.ec2/', - 'security': 'http://security-mirror1-intel'}) - - results = gpmi(arch_mirrors, data_source=data_source_mock, - mirror_filter=self.return_second) - self.assertEqual(results, - {'primary': 'http://us-east-1a.clouds/', - 'security': 'http://security-mirror2-intel'}) - - results = gpmi(arch_mirrors, data_source=data_source_mock, - mirror_filter=self.return_none) - self.assertEqual(results, package_mirrors[0]['failsafe']) - - def test_get_package_mirror_info_az_non_ec2(self): - arch_mirrors = gapmi(package_mirrors, arch="amd64") - data_source_mock = mock.Mock(availability_zone="nova.cloudvendor") - - results = gpmi(arch_mirrors, data_source=data_source_mock, - mirror_filter=self.return_first) - self.assertEqual(results, - {'primary': 'http://nova.cloudvendor.clouds/', - 'security': 'http://security-mirror1-intel'}) - - results = gpmi(arch_mirrors, data_source=data_source_mock, - mirror_filter=self.return_last) - self.assertEqual(results, - {'primary': 'http://nova.cloudvendor.clouds/', - 'security': 'http://security-mirror2-intel'}) - - def test_get_package_mirror_info_none(self): - arch_mirrors = gapmi(package_mirrors, arch="amd64") - data_source_mock = mock.Mock(availability_zone=None) - - # because both search entries here replacement based on - # availability-zone, the filter will be called with an empty list and - # failsafe should be taken. - results = gpmi(arch_mirrors, data_source=data_source_mock, - mirror_filter=self.return_first) - self.assertEqual(results, - {'primary': 'http://fs-primary-intel', - 'security': 'http://security-mirror1-intel'}) - - results = gpmi(arch_mirrors, data_source=data_source_mock, - mirror_filter=self.return_last) - self.assertEqual(results, - {'primary': 'http://fs-primary-intel', - 'security': 'http://security-mirror2-intel'}) - def test_systemd_in_use(self): cls = distros.fetch("ubuntu") d = cls("ubuntu", {}, None) @@ -259,4 +187,116 @@ class TestGenericDistro(helpers.FilesystemMockingTestCase): ["pw", "usermod", "myuser", "-p", "01-Jan-1970"]) +class TestGetPackageMirrors: + + def return_first(self, mlist): + if not mlist: + return None + return mlist[0] + + def return_second(self, mlist): + if not mlist: + return None + + return mlist[1] if len(mlist) > 1 else None + + def return_none(self, _mlist): + return None + + def return_last(self, mlist): + if not mlist: + return None + return(mlist[-1]) + + @pytest.mark.parametrize( + "allow_ec2_mirror, platform_type, mirrors", + [ + (True, "ec2", [ + {'primary': 'http://us-east-1.ec2/', + 'security': 'http://security-mirror1-intel'}, + {'primary': 'http://us-east-1a.clouds/', + 'security': 'http://security-mirror2-intel'} + ]), + (True, "other", [ + {'primary': 'http://us-east-1.ec2/', + 'security': 'http://security-mirror1-intel'}, + {'primary': 'http://us-east-1a.clouds/', + 'security': 'http://security-mirror2-intel'} + ]), + (False, "ec2", [ + {'primary': 'http://us-east-1.ec2/', + 'security': 'http://security-mirror1-intel'}, + {'primary': 'http://us-east-1a.clouds/', + 'security': 'http://security-mirror2-intel'} + ]), + (False, "other", [ + {'primary': 'http://us-east-1a.clouds/', + 'security': 'http://security-mirror1-intel'}, + {'primary': 'http://fs-primary-intel', + 'security': 'http://security-mirror2-intel'} + ]) + ]) + def test_get_package_mirror_info_az_ec2(self, + allow_ec2_mirror, + platform_type, + mirrors): + flag_path = "cloudinit.distros." \ + "ALLOW_EC2_MIRRORS_ON_NON_AWS_INSTANCE_TYPES" + with mock.patch(flag_path, allow_ec2_mirror): + arch_mirrors = gapmi(package_mirrors, arch="amd64") + data_source_mock = mock.Mock( + availability_zone="us-east-1a", + platform_type=platform_type) + + results = gpmi(arch_mirrors, data_source=data_source_mock, + mirror_filter=self.return_first) + assert(results == mirrors[0]) + + results = gpmi(arch_mirrors, data_source=data_source_mock, + mirror_filter=self.return_second) + assert(results == mirrors[1]) + + results = gpmi(arch_mirrors, data_source=data_source_mock, + mirror_filter=self.return_none) + assert(results == package_mirrors[0]['failsafe']) + + def test_get_package_mirror_info_az_non_ec2(self): + arch_mirrors = gapmi(package_mirrors, arch="amd64") + data_source_mock = mock.Mock(availability_zone="nova.cloudvendor") + + results = gpmi(arch_mirrors, data_source=data_source_mock, + mirror_filter=self.return_first) + assert(results == { + 'primary': 'http://nova.cloudvendor.clouds/', + 'security': 'http://security-mirror1-intel'} + ) + + results = gpmi(arch_mirrors, data_source=data_source_mock, + mirror_filter=self.return_last) + assert(results == { + 'primary': 'http://nova.cloudvendor.clouds/', + 'security': 'http://security-mirror2-intel'} + ) + + def test_get_package_mirror_info_none(self): + arch_mirrors = gapmi(package_mirrors, arch="amd64") + data_source_mock = mock.Mock(availability_zone=None) + + # because both search entries here replacement based on + # availability-zone, the filter will be called with an empty list and + # failsafe should be taken. + results = gpmi(arch_mirrors, data_source=data_source_mock, + mirror_filter=self.return_first) + assert(results == { + 'primary': 'http://fs-primary-intel', + 'security': 'http://security-mirror1-intel'} + ) + + results = gpmi(arch_mirrors, data_source=data_source_mock, + mirror_filter=self.return_last) + assert(results == { + 'primary': 'http://fs-primary-intel', + 'security': 'http://security-mirror2-intel'} + ) + # vi: ts=4 expandtab diff --git a/tests/unittests/test_handler/test_handler_apt_configure_sources_list_v1.py b/tests/unittests/test_handler/test_handler_apt_configure_sources_list_v1.py index e5382544..369480be 100644 --- a/tests/unittests/test_handler/test_handler_apt_configure_sources_list_v1.py +++ b/tests/unittests/test_handler/test_handler_apt_configure_sources_list_v1.py @@ -101,6 +101,7 @@ class TestAptSourceConfigSourceList(t_help.FilesystemMockingTestCase): cfg = {'apt_mirror_search': mirror} else: cfg = {'apt_mirror': mirror} + mycloud = self._get_cloud(distro) with mock.patch.object(util, 'write_file') as mockwf: @@ -108,8 +109,9 @@ class TestAptSourceConfigSourceList(t_help.FilesystemMockingTestCase): return_value="faketmpl") as mocklf: with mock.patch.object(os.path, 'isfile', return_value=True) as mockisfile: - with mock.patch.object(templater, 'render_string', - return_value="fake") as mockrnd: + with mock.patch.object( + templater, 'render_string', + return_value='fake') as mockrnd: with mock.patch.object(util, 'rename'): cc_apt_configure.handle("test", cfg, mycloud, LOG, None) diff --git a/tests/unittests/test_handler/test_handler_apt_source_v1.py b/tests/unittests/test_handler/test_handler_apt_source_v1.py index f2349157..367971cb 100644 --- a/tests/unittests/test_handler/test_handler_apt_source_v1.py +++ b/tests/unittests/test_handler/test_handler_apt_source_v1.py @@ -43,10 +43,17 @@ class FakeDistro(object): return +class FakeDatasource: + """Fake Datasource helper object""" + def __init__(self): + self.region = 'region' + + class FakeCloud(object): """Fake Cloud helper object""" def __init__(self): self.distro = FakeDistro() + self.datasource = FakeDatasource() class TestAptSourceConfig(TestCase): diff --git a/tests/unittests/test_handler/test_handler_apt_source_v3.py b/tests/unittests/test_handler/test_handler_apt_source_v3.py index 220100e2..ac847238 100644 --- a/tests/unittests/test_handler/test_handler_apt_source_v3.py +++ b/tests/unittests/test_handler/test_handler_apt_source_v3.py @@ -49,6 +49,18 @@ MOCK_LSB_RELEASE_DATA = { 'release': '18.04', 'codename': 'bionic'} +class FakeDatasource: + """Fake Datasource helper object""" + def __init__(self): + self.region = 'region' + + +class FakeCloud: + """Fake Cloud helper object""" + def __init__(self): + self.datasource = FakeDatasource() + + class TestAptSourceConfig(t_help.FilesystemMockingTestCase): """TestAptSourceConfig Main Class to test apt configs @@ -471,7 +483,7 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): fromfn = ("%s/%s_%s" % (pre, archive, post)) tofn = ("%s/test.ubuntu.com_%s" % (pre, post)) - mirrors = cc_apt_configure.find_apt_mirror_info(cfg, None, arch) + mirrors = cc_apt_configure.find_apt_mirror_info(cfg, FakeCloud(), arch) self.assertEqual(mirrors['MIRROR'], "http://test.ubuntu.com/%s/" % component) @@ -559,7 +571,8 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): "security": [{'arches': ["default"], "uri": smir}]} - mirrors = cc_apt_configure.find_apt_mirror_info(cfg, None, 'amd64') + mirrors = cc_apt_configure.find_apt_mirror_info( + cfg, FakeCloud(), 'amd64') self.assertEqual(mirrors['MIRROR'], pmir) @@ -594,7 +607,7 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): "security": [{'arches': ["default"], "uri": "nothis-security"}, {'arches': [arch], "uri": smir}]} - mirrors = cc_apt_configure.find_apt_mirror_info(cfg, None, arch) + mirrors = cc_apt_configure.find_apt_mirror_info(cfg, FakeCloud(), arch) self.assertEqual(mirrors['PRIMARY'], pmir) self.assertEqual(mirrors['MIRROR'], pmir) @@ -613,7 +626,8 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): {'arches': ["default"], "uri": smir}]} - mirrors = cc_apt_configure.find_apt_mirror_info(cfg, None, 'amd64') + mirrors = cc_apt_configure.find_apt_mirror_info( + cfg, FakeCloud(), 'amd64') self.assertEqual(mirrors['MIRROR'], pmir) @@ -673,7 +687,7 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): with mock.patch.object(cc_apt_configure.util, 'search_for_mirror', side_effect=[pmir, smir]) as mocksearch: - mirrors = cc_apt_configure.find_apt_mirror_info(cfg, None, + mirrors = cc_apt_configure.find_apt_mirror_info(cfg, FakeCloud(), 'amd64') calls = [call(["pfailme", pmir]), @@ -712,7 +726,8 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase): # should not be called, since primary is specified with mock.patch.object(cc_apt_configure.util, 'search_for_mirror') as mockse: - mirrors = cc_apt_configure.find_apt_mirror_info(cfg, None, arch) + mirrors = cc_apt_configure.find_apt_mirror_info( + cfg, FakeCloud(), arch) mockse.assert_not_called() self.assertEqual(mirrors['MIRROR'], -- cgit v1.2.3 From 25289087e44c9c74543248519e37ca1f11b8a711 Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Tue, 14 Jul 2020 13:31:13 -0400 Subject: networking: refactor wait_for_physdevs from cloudinit.net (#466) * Refactor `cloudinit.net.wait_for_physdevs` to `cloudinit.distros.networking.Networking.wait_for_physdevs` * Split the Linux-specific `udevadm_settle` call out to a separate abstract `Networking.settle` method; implement it on `LinuxNetworking` and add a `NotImplementedError` implementation to `BSDNetworking` * Modify `wait_for_physdevs`s one callsite to use the new location LP: #1884626 --- cloudinit/distros/networking.py | 79 ++++++++++++++- cloudinit/distros/tests/test_networking.py | 152 ++++++++++++++++++++++++++++- cloudinit/net/__init__.py | 38 -------- cloudinit/net/tests/test_init.py | 74 -------------- cloudinit/stages.py | 2 +- 5 files changed, 229 insertions(+), 116 deletions(-) (limited to 'cloudinit/distros/tests') diff --git a/cloudinit/distros/networking.py b/cloudinit/distros/networking.py index e421a2ce..75e69df5 100644 --- a/cloudinit/distros/networking.py +++ b/cloudinit/distros/networking.py @@ -1,7 +1,11 @@ import abc +import logging import os -from cloudinit import net +from cloudinit import net, util + + +LOG = logging.getLogger(__name__) # Type aliases (https://docs.python.org/3/library/typing.html#type-aliases), @@ -102,10 +106,72 @@ class Networking(metaclass=abc.ABCMeta): def master_is_bridge_or_bond(self, devname: DeviceName) -> bool: return net.master_is_bridge_or_bond(devname) + @abc.abstractmethod + def settle(self, *, exists=None) -> None: + """Wait for device population in the system to complete. + + :param exists: + An optional optimisation. If given, only perform as much of the + settle process as is required for the given DeviceName to be + present in the system. (This may include skipping the settle + process entirely, if the device already exists.) + :type exists: Optional[DeviceName] + """ + pass + def wait_for_physdevs( self, netcfg: NetworkConfig, *, strict: bool = True ) -> None: - return net.wait_for_physdevs(netcfg, strict=strict) + """Wait for all the physical devices in `netcfg` to exist on the system + + Specifically, this will call `self.settle` 5 times, and check after + each one if the physical devices are now present in the system. + + :param netcfg: + The NetworkConfig from which to extract physical devices to wait + for. + :param strict: + Raise a `RuntimeError` if any physical devices are not present + after waiting. + """ + physdevs = self.extract_physdevs(netcfg) + + # set of expected iface names and mac addrs + expected_ifaces = dict([(iface[0], iface[1]) for iface in physdevs]) + expected_macs = set(expected_ifaces.keys()) + + # set of current macs + present_macs = self.get_interfaces_by_mac().keys() + + # compare the set of expected mac address values to + # the current macs present; we only check MAC as cloud-init + # has not yet renamed interfaces and the netcfg may include + # such renames. + for _ in range(0, 5): + if expected_macs.issubset(present_macs): + LOG.debug("net: all expected physical devices present") + return + + missing = expected_macs.difference(present_macs) + LOG.debug("net: waiting for expected net devices: %s", missing) + for mac in missing: + # trigger a settle, unless this interface exists + devname = expected_ifaces[mac] + msg = "Waiting for settle or {} exists".format(devname) + util.log_time( + LOG.debug, + msg, + func=self.settle, + kwargs={"exists": devname}, + ) + + # update present_macs after settles + present_macs = self.get_interfaces_by_mac().keys() + + msg = "Not all expected physical devices present: %s" % missing + LOG.warning(msg) + if strict: + raise RuntimeError(msg) class BSDNetworking(Networking): @@ -114,6 +180,10 @@ class BSDNetworking(Networking): def is_physical(self, devname: DeviceName) -> bool: raise NotImplementedError() + def settle(self, *, exists=None) -> None: + """BSD has no equivalent to `udevadm settle`; noop.""" + pass + class LinuxNetworking(Networking): """Implementation of networking functionality common to Linux distros.""" @@ -138,3 +208,8 @@ class LinuxNetworking(Networking): def is_physical(self, devname: DeviceName) -> bool: return os.path.exists(net.sys_dev_path(devname, "device")) + + def settle(self, *, exists=None) -> None: + if exists is not None: + exists = net.sys_dev_path(exists) + util.udevadm_settle(exists=exists) diff --git a/cloudinit/distros/tests/test_networking.py b/cloudinit/distros/tests/test_networking.py index 2acb12f4..b9a63842 100644 --- a/cloudinit/distros/tests/test_networking.py +++ b/cloudinit/distros/tests/test_networking.py @@ -2,7 +2,39 @@ from unittest import mock import pytest -from cloudinit.distros.networking import BSDNetworking, LinuxNetworking +from cloudinit import net +from cloudinit.distros.networking import ( + BSDNetworking, + LinuxNetworking, + Networking, +) + +# See https://docs.pytest.org/en/stable/example +# /parametrize.html#parametrizing-conditional-raising +from contextlib import ExitStack as does_not_raise + + +@pytest.yield_fixture +def generic_networking_cls(): + """Returns a direct Networking subclass which errors on /sys usage. + + This enables the direct testing of functionality only present on the + ``Networking`` super-class, and provides a check on accidentally using /sys + in that context. + """ + + class TestNetworking(Networking): + def is_physical(self, *args, **kwargs): + raise NotImplementedError + + def settle(self, *args, **kwargs): + raise NotImplementedError + + error = AssertionError("Unexpectedly used /sys in generic networking code") + with mock.patch( + "cloudinit.net.get_sys_class_path", side_effect=error, + ): + yield TestNetworking @pytest.yield_fixture @@ -40,3 +72,121 @@ class TestLinuxNetworkingIsPhysical: device_dir.join("device").write("") assert LinuxNetworking().is_physical(devname) + + +class TestBSDNetworkingSettle: + def test_settle_doesnt_error(self): + # This also implicitly tests that it doesn't use subp.subp + BSDNetworking().settle() + + +@pytest.mark.usefixtures("sys_class_net") +@mock.patch("cloudinit.distros.networking.util.udevadm_settle", autospec=True) +class TestLinuxNetworkingSettle: + def test_no_arguments(self, m_udevadm_settle): + LinuxNetworking().settle() + + assert [mock.call(exists=None)] == m_udevadm_settle.call_args_list + + def test_exists_argument(self, m_udevadm_settle): + LinuxNetworking().settle(exists="ens3") + + expected_path = net.sys_dev_path("ens3") + assert [ + mock.call(exists=expected_path) + ] == m_udevadm_settle.call_args_list + + +class TestNetworkingWaitForPhysDevs: + @pytest.fixture + def wait_for_physdevs_netcfg(self): + """This config is shared across all the tests in this class.""" + + def ethernet(mac, name, driver=None, device_id=None): + v2_cfg = {"set-name": name, "match": {"macaddress": mac}} + if driver: + v2_cfg["match"].update({"driver": driver}) + if device_id: + v2_cfg["match"].update({"device_id": device_id}) + + return v2_cfg + + physdevs = [ + ["aa:bb:cc:dd:ee:ff", "eth0", "virtio", "0x1000"], + ["00:11:22:33:44:55", "ens3", "e1000", "0x1643"], + ] + netcfg = { + "version": 2, + "ethernets": {args[1]: ethernet(*args) for args in physdevs}, + } + return netcfg + + def test_skips_settle_if_all_present( + self, generic_networking_cls, wait_for_physdevs_netcfg, + ): + networking = generic_networking_cls() + with mock.patch.object( + networking, "get_interfaces_by_mac" + ) as m_get_interfaces_by_mac: + m_get_interfaces_by_mac.side_effect = iter( + [{"aa:bb:cc:dd:ee:ff": "eth0", "00:11:22:33:44:55": "ens3"}] + ) + with mock.patch.object( + networking, "settle", autospec=True + ) as m_settle: + networking.wait_for_physdevs(wait_for_physdevs_netcfg) + assert 0 == m_settle.call_count + + def test_calls_udev_settle_on_missing( + self, generic_networking_cls, wait_for_physdevs_netcfg, + ): + networking = generic_networking_cls() + with mock.patch.object( + networking, "get_interfaces_by_mac" + ) as m_get_interfaces_by_mac: + m_get_interfaces_by_mac.side_effect = iter( + [ + { + "aa:bb:cc:dd:ee:ff": "eth0" + }, # first call ens3 is missing + { + "aa:bb:cc:dd:ee:ff": "eth0", + "00:11:22:33:44:55": "ens3", + }, # second call has both + ] + ) + with mock.patch.object( + networking, "settle", autospec=True + ) as m_settle: + networking.wait_for_physdevs(wait_for_physdevs_netcfg) + m_settle.assert_called_with(exists="ens3") + + @pytest.mark.parametrize( + "strict,expectation", + [(True, pytest.raises(RuntimeError)), (False, does_not_raise())], + ) + def test_retrying_and_strict_behaviour( + self, + strict, + expectation, + generic_networking_cls, + wait_for_physdevs_netcfg, + ): + networking = generic_networking_cls() + with mock.patch.object( + networking, "get_interfaces_by_mac" + ) as m_get_interfaces_by_mac: + m_get_interfaces_by_mac.return_value = {} + + with mock.patch.object( + networking, "settle", autospec=True + ) as m_settle: + with expectation: + networking.wait_for_physdevs( + wait_for_physdevs_netcfg, strict=strict + ) + + assert ( + 5 * len(wait_for_physdevs_netcfg["ethernets"]) + == m_settle.call_count + ) diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py index 9d8c7ba9..322af77b 100644 --- a/cloudinit/net/__init__.py +++ b/cloudinit/net/__init__.py @@ -10,7 +10,6 @@ import ipaddress import logging import os import re -from functools import partial from cloudinit import subp from cloudinit import util @@ -494,43 +493,6 @@ def extract_physdevs(netcfg): raise RuntimeError('Unknown network config version: %s' % version) -def wait_for_physdevs(netcfg, strict=True): - physdevs = extract_physdevs(netcfg) - - # set of expected iface names and mac addrs - expected_ifaces = dict([(iface[0], iface[1]) for iface in physdevs]) - expected_macs = set(expected_ifaces.keys()) - - # set of current macs - present_macs = get_interfaces_by_mac().keys() - - # compare the set of expected mac address values to - # the current macs present; we only check MAC as cloud-init - # has not yet renamed interfaces and the netcfg may include - # such renames. - for _ in range(0, 5): - if expected_macs.issubset(present_macs): - LOG.debug('net: all expected physical devices present') - return - - missing = expected_macs.difference(present_macs) - LOG.debug('net: waiting for expected net devices: %s', missing) - for mac in missing: - # trigger a settle, unless this interface exists - syspath = sys_dev_path(expected_ifaces[mac]) - settle = partial(util.udevadm_settle, exists=syspath) - msg = 'Waiting for udev events to settle or %s exists' % syspath - util.log_time(LOG.debug, msg, func=settle) - - # update present_macs after settles - present_macs = get_interfaces_by_mac().keys() - - msg = 'Not all expected physical devices present: %s' % missing - LOG.warning(msg) - if strict: - raise RuntimeError(msg) - - def apply_network_config_names(netcfg, strict_present=True, strict_busy=True): """read the network config and rename devices accordingly. if strict_present is false, then do not raise exception if no devices diff --git a/cloudinit/net/tests/test_init.py b/cloudinit/net/tests/test_init.py index eb458c39..311ab6f8 100644 --- a/cloudinit/net/tests/test_init.py +++ b/cloudinit/net/tests/test_init.py @@ -963,80 +963,6 @@ class TestExtractPhysdevs(CiTestCase): net.extract_physdevs({'version': 3, 'awesome_config': []}) -class TestWaitForPhysdevs(CiTestCase): - - def setUp(self): - super(TestWaitForPhysdevs, self).setUp() - self.add_patch('cloudinit.net.get_interfaces_by_mac', - 'm_get_iface_mac') - self.add_patch('cloudinit.util.udevadm_settle', 'm_udev_settle') - - def test_wait_for_physdevs_skips_settle_if_all_present(self): - physdevs = [ - ['aa:bb:cc:dd:ee:ff', 'eth0', 'virtio', '0x1000'], - ['00:11:22:33:44:55', 'ens3', 'e1000', '0x1643'], - ] - netcfg = { - 'version': 2, - 'ethernets': {args[1]: _mk_v2_phys(*args) - for args in physdevs}, - } - self.m_get_iface_mac.side_effect = iter([ - {'aa:bb:cc:dd:ee:ff': 'eth0', - '00:11:22:33:44:55': 'ens3'}, - ]) - net.wait_for_physdevs(netcfg) - self.assertEqual(0, self.m_udev_settle.call_count) - - def test_wait_for_physdevs_calls_udev_settle_on_missing(self): - physdevs = [ - ['aa:bb:cc:dd:ee:ff', 'eth0', 'virtio', '0x1000'], - ['00:11:22:33:44:55', 'ens3', 'e1000', '0x1643'], - ] - netcfg = { - 'version': 2, - 'ethernets': {args[1]: _mk_v2_phys(*args) - for args in physdevs}, - } - self.m_get_iface_mac.side_effect = iter([ - {'aa:bb:cc:dd:ee:ff': 'eth0'}, # first call ens3 is missing - {'aa:bb:cc:dd:ee:ff': 'eth0', - '00:11:22:33:44:55': 'ens3'}, # second call has both - ]) - net.wait_for_physdevs(netcfg) - self.m_udev_settle.assert_called_with(exists=net.sys_dev_path('ens3')) - - def test_wait_for_physdevs_raise_runtime_error_if_missing_and_strict(self): - physdevs = [ - ['aa:bb:cc:dd:ee:ff', 'eth0', 'virtio', '0x1000'], - ['00:11:22:33:44:55', 'ens3', 'e1000', '0x1643'], - ] - netcfg = { - 'version': 2, - 'ethernets': {args[1]: _mk_v2_phys(*args) - for args in physdevs}, - } - self.m_get_iface_mac.return_value = {} - with self.assertRaises(RuntimeError): - net.wait_for_physdevs(netcfg) - - self.assertEqual(5 * len(physdevs), self.m_udev_settle.call_count) - - def test_wait_for_physdevs_no_raise_if_not_strict(self): - physdevs = [ - ['aa:bb:cc:dd:ee:ff', 'eth0', 'virtio', '0x1000'], - ['00:11:22:33:44:55', 'ens3', 'e1000', '0x1643'], - ] - netcfg = { - 'version': 2, - 'ethernets': {args[1]: _mk_v2_phys(*args) - for args in physdevs}, - } - self.m_get_iface_mac.return_value = {} - net.wait_for_physdevs(netcfg, strict=False) - self.assertEqual(5 * len(physdevs), self.m_udev_settle.call_count) - - class TestNetFailOver(CiTestCase): def setUp(self): diff --git a/cloudinit/stages.py b/cloudinit/stages.py index db8ba64c..69e6b7e1 100644 --- a/cloudinit/stages.py +++ b/cloudinit/stages.py @@ -696,7 +696,7 @@ class Init(object): netcfg, src = self._find_networking_config() # ensure all physical devices in config are present - net.wait_for_physdevs(netcfg) + self.distro.networking.wait_for_physdevs(netcfg) # apply renames from config self._apply_netcfg_names(netcfg) -- cgit v1.2.3