From 24e49c24ee32c20cb0b6bcb94892f088b521fae8 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Wed, 16 Jun 2021 16:57:18 -0500 Subject: [PATCH 1/2] Fix DNS in NetworkState v1 network config currently has no concept of interface-specific DNS, which is required for certain renderers. To fix this, added an optional 'interface' key on the v1 nameserver definition. If specified, it makes the DNS settings specific to the interface. Otherwise, it will be defined as global DNS as it always has. Additionally, DNS for v2 wasn't being recognized correctly. For DNS defined on a particular interface, these settings now also go into the global DNS settings as they were intended. --- cloudinit/net/network_state.py | 66 ++++++++++--- cloudinit/net/tests/test_network_state.py | 103 ++++++++++++++++++++ doc/rtd/topics/network-config-format-v1.rst | 4 + 3 files changed, 158 insertions(+), 15 deletions(-) diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py index e8bf9e39a6a..62e4be30e8f 100644 --- a/cloudinit/net/network_state.py +++ b/cloudinit/net/network_state.py @@ -237,6 +237,7 @@ def __init__(self, version=NETWORK_STATE_VERSION, config=None): self._network_state = copy.deepcopy(self.initial_network_state) self._network_state['config'] = config self._parsed = False + self._interface_dns_map = {} @property def network_state(self): @@ -310,6 +311,21 @@ def parse_config_v1(self, skip_broken=True): LOG.warning("Skipping invalid command: %s", command, exc_info=True) LOG.debug(self.dump_network_state()) + for interface, dns in self._interface_dns_map.items(): + iface = None + try: + iface = self._network_state['interfaces'][interface] + except KeyError as e: + raise ValueError( + 'Nameserver specified for interface {0}, ' + 'but interface {0} does not exist!'.format(interface) + ) from e + if iface: + nameservers, search = dns + iface['dns'] = { + 'addresses': nameservers, + 'search': search, + } def parse_config_v2(self, skip_broken=True): for command_type, command in self._config.items(): @@ -526,21 +542,40 @@ def handle_bridge(self, command): def handle_infiniband(self, command): self.handle_physical(command) - @ensure_command_keys(['address']) - def handle_nameserver(self, command): - dns = self._network_state.get('dns') + def _parse_dns(self, command): + nameservers = [] + search = [] if 'address' in command: addrs = command['address'] if not type(addrs) == list: addrs = [addrs] for addr in addrs: - dns['nameservers'].append(addr) + nameservers.append(addr) if 'search' in command: paths = command['search'] if not isinstance(paths, list): paths = [paths] for path in paths: - dns['search'].append(path) + search.append(path) + return nameservers, search + + @ensure_command_keys(['address']) + def handle_nameserver(self, command): + dns = self._network_state.get('dns') + nameservers, search = self._parse_dns(command) + if 'interface' in command: + self._interface_dns_map[command['interface']] = ( + nameservers, search + ) + else: + dns['nameservers'].extend(nameservers) + dns['search'].extend(search) + + @ensure_command_keys(['address']) + def handle_individual_nameserver(self, command, iface): + _iface = self._network_state.get('interfaces') + nameservers, search = self._parse_dns(command) + _iface[iface]['dns'] = {'nameservers': nameservers, 'search': search} @ensure_command_keys(['destination']) def handle_route(self, command): @@ -706,16 +741,17 @@ def handle_wifis(self, command): def _v2_common(self, cfg): LOG.debug('v2_common: handling config:\n%s', cfg) - if 'nameservers' in cfg: - search = cfg.get('nameservers').get('search', []) - dns = cfg.get('nameservers').get('addresses', []) - name_cmd = {'type': 'nameserver'} - if len(search) > 0: - name_cmd.update({'search': search}) - if len(dns) > 0: - name_cmd.update({'addresses': dns}) - LOG.debug('v2(nameserver) -> v1(nameserver):\n%s', name_cmd) - self.handle_nameserver(name_cmd) + for iface, dev_cfg in cfg.items(): + if 'nameservers' in dev_cfg: + search = dev_cfg.get('nameservers').get('search', []) + dns = dev_cfg.get('nameservers').get('addresses', []) + name_cmd = {'type': 'nameserver'} + if len(search) > 0: + name_cmd.update({'search': search}) + if len(dns) > 0: + name_cmd.update({'address': dns}) + self.handle_nameserver(name_cmd) + self.handle_individual_nameserver(name_cmd, iface) def _handle_bond_bridge(self, command, cmd_type=None): """Common handler for bond and bridge types""" diff --git a/cloudinit/net/tests/test_network_state.py b/cloudinit/net/tests/test_network_state.py index 07d726e2638..c3d8e33fe9a 100644 --- a/cloudinit/net/tests/test_network_state.py +++ b/cloudinit/net/tests/test_network_state.py @@ -2,12 +2,62 @@ from unittest import mock +import pytest + +from cloudinit import safeyaml from cloudinit.net import network_state from cloudinit.tests.helpers import CiTestCase netstate_path = 'cloudinit.net.network_state' +_V1_CONFIG_NAMESERVERS = """\ +network: + version: 1 + config: + - type: nameserver + interface: {} + address: + - 192.168.1.1 + - 8.8.8.8 + search: + - spam.local + - type: nameserver + address: + - 192.168.1.0 + - 4.4.4.4 + search: + - eggs.local + - type: physical + name: eth0 + mac_address: '00:11:22:33:44:55' + - type: physical + name: eth1 + mac_address: '66:77:88:99:00:11' +""" + +V1_CONFIG_NAMESERVERS_VALID = _V1_CONFIG_NAMESERVERS.format('eth1') +V1_CONFIG_NAMESERVERS_INVALID = _V1_CONFIG_NAMESERVERS.format('eth90') + +V2_CONFIG_NAMESERVERS = """\ +network: + version: 2 + ethernets: + eth0: + match: + macaddress: '00:11:22:33:44:55' + nameservers: + search: [spam.local, eggs.local] + addresses: [8.8.8.8] + eth1: + match: + macaddress: '66:77:88:99:00:11' + nameservers: + search: [foo.local, bar.local] + addresses: [4.4.4.4] +""" + + class TestNetworkStateParseConfig(CiTestCase): def setUp(self): @@ -55,4 +105,57 @@ def test_version_2_ignores_renderer_key(self): self.assertEqual(ncfg, nsi.as_dict()['config']) +class TestNetworkStateParseNameservers: + def _parse_network_state_from_config(self, config): + yaml = safeyaml.load(config) + return network_state.parse_net_config_data(yaml['network']) + + def test_v1_nameservers_valid(self): + config = self._parse_network_state_from_config( + V1_CONFIG_NAMESERVERS_VALID) + + # If an interface was specified, DNS shouldn't be in the global list + assert ['192.168.1.0', '4.4.4.4'] == sorted( + config.dns_nameservers) + assert ['eggs.local'] == config.dns_searchdomains + + # If an interface was specified, DNS should be part of the interface + for iface in config.iter_interfaces(): + if iface['name'] == 'eth1': + assert iface['dns']['addresses'] == ['192.168.1.1', '8.8.8.8'] + assert iface['dns']['search'] == ['spam.local'] + else: + assert 'dns' not in iface + + def test_v1_nameservers_invalid(self): + with pytest.raises(ValueError): + self._parse_network_state_from_config( + V1_CONFIG_NAMESERVERS_INVALID) + + def test_v2_nameservers(self): + config = self._parse_network_state_from_config(V2_CONFIG_NAMESERVERS) + + # Ensure DNS defined on interface exists on interface + for iface in config.iter_interfaces(): + if iface['name'] == 'eth0': + assert iface['dns'] == { + 'nameservers': ['8.8.8.8'], + 'search': ['spam.local', 'eggs.local'], + } + else: + assert iface['dns'] == { + 'nameservers': ['4.4.4.4'], + 'search': ['foo.local', 'bar.local'] + } + + # Ensure DNS defined on interface also exists globally (since there + # is no global DNS definitions in v2) + assert ['4.4.4.4', '8.8.8.8'] == sorted(config.dns_nameservers) + assert [ + 'bar.local', + 'eggs.local', + 'foo.local', + 'spam.local', + ] == sorted(config.dns_searchdomains) + # vi: ts=4 expandtab diff --git a/doc/rtd/topics/network-config-format-v1.rst b/doc/rtd/topics/network-config-format-v1.rst index 17732c2abf3..d379b437c00 100644 --- a/doc/rtd/topics/network-config-format-v1.rst +++ b/doc/rtd/topics/network-config-format-v1.rst @@ -335,6 +335,10 @@ the following keys: - ``address``: List of IPv4 or IPv6 address of nameservers. - ``search``: List of of hostnames to include in the resolv.conf search path. +- ``interface``: Optional. Ties the nameserver definition to the specified + interface. The value specified here must match the `name` of an interface + defined in this config. If unspecified, this nameserver will be considered + a global nameserver. **Nameserver Example**:: From dc4e57f0d804a7eb99c502f3b6f863a359306325 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Thu, 17 Jun 2021 16:14:01 -0500 Subject: [PATCH 2/2] review_comments --- cloudinit/net/network_state.py | 4 ++-- cloudinit/net/tests/test_network_state.py | 6 +++--- doc/rtd/topics/network-config-format-v1.rst | 1 + 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py index 62e4be30e8f..8018cfb9ff8 100644 --- a/cloudinit/net/network_state.py +++ b/cloudinit/net/network_state.py @@ -572,7 +572,7 @@ def handle_nameserver(self, command): dns['search'].extend(search) @ensure_command_keys(['address']) - def handle_individual_nameserver(self, command, iface): + def _handle_individual_nameserver(self, command, iface): _iface = self._network_state.get('interfaces') nameservers, search = self._parse_dns(command) _iface[iface]['dns'] = {'nameservers': nameservers, 'search': search} @@ -751,7 +751,7 @@ def _v2_common(self, cfg): if len(dns) > 0: name_cmd.update({'address': dns}) self.handle_nameserver(name_cmd) - self.handle_individual_nameserver(name_cmd, iface) + self._handle_individual_nameserver(name_cmd, iface) def _handle_bond_bridge(self, command, cmd_type=None): """Common handler for bond and bridge types""" diff --git a/cloudinit/net/tests/test_network_state.py b/cloudinit/net/tests/test_network_state.py index c3d8e33fe9a..fc4724a1962 100644 --- a/cloudinit/net/tests/test_network_state.py +++ b/cloudinit/net/tests/test_network_state.py @@ -16,7 +16,7 @@ version: 1 config: - type: nameserver - interface: {} + interface: {iface} address: - 192.168.1.1 - 8.8.8.8 @@ -36,8 +36,8 @@ mac_address: '66:77:88:99:00:11' """ -V1_CONFIG_NAMESERVERS_VALID = _V1_CONFIG_NAMESERVERS.format('eth1') -V1_CONFIG_NAMESERVERS_INVALID = _V1_CONFIG_NAMESERVERS.format('eth90') +V1_CONFIG_NAMESERVERS_VALID = _V1_CONFIG_NAMESERVERS.format(iface='eth1') +V1_CONFIG_NAMESERVERS_INVALID = _V1_CONFIG_NAMESERVERS.format(iface='eth90') V2_CONFIG_NAMESERVERS = """\ network: diff --git a/doc/rtd/topics/network-config-format-v1.rst b/doc/rtd/topics/network-config-format-v1.rst index d379b437c00..3202163b7a8 100644 --- a/doc/rtd/topics/network-config-format-v1.rst +++ b/doc/rtd/topics/network-config-format-v1.rst @@ -353,6 +353,7 @@ the following keys: address: 192.168.23.14/27 gateway: 192.168.23.1 - type: nameserver + interface: interface0 # Ties nameserver to interface0 only address: - 192.168.23.2 - 8.8.8.8