From 1ae12dc6f8e5375ca7ee7023af68d8004b2b0410 Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Mon, 26 Apr 2021 11:43:41 +0100 Subject: [PATCH] Ubuntu: define implied VLAN parent interfaces in networkd On CentOS, when using the MichaelRigart.interfaces role, defining a VLAN interface implies the existence of its parent interface. This is handled via the CentOS network interface scripts. With systemd-networkd, the parent interface is not implicit, and must be configured. The parent interface network configuration file needs to contain this: [Network] VLAN = eth0.42 This change adds support for these implied parent interfaces. It also removes the Broadcast = true option, which was added in an attempt to fix an issue but is not supported in the [Network] section, and generates a warning: Unknown key name 'Broadcast' in section 'Network', ignoring. Story: 2004960 Task: 42368 Task: 42367 Change-Id: I5d105006fad6e7e80473b9d9fa693de644c35a6d --- kayobe/plugins/filter/networkd.py | 96 ++++++-- .../unit/plugins/filter/test_networkd.py | 217 +++++++++++++++++- tox.ini | 3 +- 3 files changed, 294 insertions(+), 22 deletions(-) diff --git a/kayobe/plugins/filter/networkd.py b/kayobe/plugins/filter/networkd.py index 552722472..dac05b303 100644 --- a/kayobe/plugins/filter/networkd.py +++ b/kayobe/plugins/filter/networkd.py @@ -284,7 +284,6 @@ def _network(context, name, inventory_hostname, bridge, bond, vlan_interfaces): { 'Network': [ {'Address': ip}, - {'Broadcast': 'true' if ip else None}, {'Gateway': gateway}, {'DHCP': ('yes' if bootproto and bootproto.lower() == 'dhcp' else None)}, @@ -317,6 +316,34 @@ def _network(context, name, inventory_hostname, bridge, bond, vlan_interfaces): return _filter_options(config) +def _vlan_parent_network(device, mtu, vlan_interfaces): + """Return a networkd network configuration for a VLAN parent interface. + + :param device: name of the interface. + :param mtu: Interface MTU. + :param vlan_interfaces: List of VLAN subinterfaces of the interface. + """ + config = [ + { + 'Match': [ + {'Name': device}, + ] + }, + { + 'Network': [ + {'VLAN': vlan_interface} + for vlan_interface in vlan_interfaces + ] + }, + { + 'Link': [ + {'MTUBytes': mtu}, + ] + } + ] + return _filter_options(config) + + def _bridge_port_network(context, name, port, inventory_hostname, vlan_interfaces): """Return a networkd network configuration for a bridge port. @@ -437,6 +464,25 @@ def _veth_peer_network(context, veth, inventory_hostname): return _filter_options(config) +def _add_to_result(result, prefix, device, config): + """Add configuration for an interface to a filter result. + + :param result: the result dict. + :param prefix: the systemd-networkd configuration file prefix. + :param device: the interface being configured. + :param config: the configuration to add to the result. + :raises: AnsibleFilterError if the interface already exists in the result. + """ + key = "%s%s" % (prefix, device) + # Catch the case where interface configuration is added multiple times. + # This should not happen. + if key in result: + raise errors.AnsibleFilterError( + "Programming error: duplicate interface configuration for %s" + % device) + result[key] = config + + @jinja2.contextfilter def networkd_netdevs(context, names, inventory_hostname=None): """Return a dict representation of networkd NetDev configuration. @@ -459,7 +505,7 @@ def networkd_netdevs(context, names, inventory_hostname=None): device = networks.get_and_validate_interface(context, name, inventory_hostname) netdev = _vlan_netdev(context, name, inventory_hostname) - result["%s%s" % (prefix, device)] = netdev + _add_to_result(result, prefix, device, netdev) # Bridges. for name in networks.net_select_bridges(context, names, @@ -467,21 +513,21 @@ def networkd_netdevs(context, names, inventory_hostname=None): device = networks.get_and_validate_interface(context, name, inventory_hostname) netdev = _bridge_netdev(context, name, inventory_hostname) - result["%s%s" % (prefix, device)] = netdev + _add_to_result(result, prefix, device, netdev) # Bonds. for name in networks.net_select_bonds(context, names, inventory_hostname): device = networks.get_and_validate_interface(context, name, inventory_hostname) netdev = _bond_netdev(context, name, inventory_hostname) - result["%s%s" % (prefix, device)] = netdev + _add_to_result(result, prefix, device, netdev) # Virtual Ethernet pairs. veths = networks.get_ovs_veths(context, names, inventory_hostname) for veth in veths: netdev = _veth_netdev(context, veth, inventory_hostname) device = veth['name'] - result["%s%s" % (prefix, device)] = netdev + _add_to_result(result, prefix, device, netdev) return result @@ -551,9 +597,10 @@ def networkd_networks(context, names, inventory_hostname=None): device = networks.get_and_validate_interface(context, name, inventory_hostname) vlan = networks.net_vlan(context, name, inventory_hostname) + mtu = networks.net_mtu(context, name, inventory_hostname) parent = networks.get_vlan_parent(device, vlan) vlan_interfaces = interface_to_vlans.setdefault(parent, []) - vlan_interfaces.append(device) + vlan_interfaces.append({"device": device, "mtu": mtu}) # Prefix for configuration file names. prefix = utils.get_hostvar(context, "networkd_prefix", inventory_hostname) @@ -568,8 +615,22 @@ def networkd_networks(context, names, inventory_hostname=None): bond = bond_member_to_bond.get(device) vlan_interfaces = interface_to_vlans.get(device, []) net = _network(context, name, inventory_hostname, bridge, bond, - vlan_interfaces) - result["%s%s" % (prefix, device)] = net + [vlan["device"] for vlan in vlan_interfaces]) + _add_to_result(result, prefix, device, net) + + # VLAN parent interfaces that are not in configured networks, bridge ports + # or bond members. + implied_vlan_parents = (set(interface_to_vlans) - + set(interfaces) - + set(bridge_port_to_bridge) - + set(bond_member_to_bond)) + for device in implied_vlan_parents: + vlan_interfaces = interface_to_vlans[device] + mtu = max([vlan["mtu"] for vlan in vlan_interfaces]) + net = _vlan_parent_network(device, mtu, + [vlan["device"] + for vlan in vlan_interfaces]) + _add_to_result(result, prefix, device, net) # Bridge ports that are not in configured networks. for name in networks.net_select_bridges(context, names, @@ -580,9 +641,10 @@ def networkd_networks(context, names, inventory_hostname=None): inventory_hostname) for port in set(bridge_ports) - set(interfaces): vlan_interfaces = interface_to_vlans.get(port, []) - netdev = _bridge_port_network(context, name, port, - inventory_hostname, vlan_interfaces) - result["%s%s" % (prefix, port)] = netdev + net = _bridge_port_network(context, name, port, inventory_hostname, + [vlan["device"] + for vlan in vlan_interfaces]) + _add_to_result(result, prefix, port, net) # Bond members that are not in configured networks. for name in networks.net_select_bonds(context, names, inventory_hostname): @@ -592,20 +654,22 @@ def networkd_networks(context, names, inventory_hostname=None): inventory_hostname) for member in set(bond_members) - set(interfaces): vlan_interfaces = interface_to_vlans.get(member, []) - netdev = _bond_member_network(context, name, member, - inventory_hostname, vlan_interfaces) - result["%s%s" % (prefix, member)] = netdev + net = _bond_member_network(context, name, member, + inventory_hostname, + [vlan["device"] + for vlan in vlan_interfaces]) + _add_to_result(result, prefix, member, net) # Virtual Ethernet pairs for Open vSwitch. veths = networks.get_ovs_veths(context, names, inventory_hostname) for veth in veths: net = _veth_network(context, veth, inventory_hostname) device = veth['name'] - result["%s%s" % (prefix, device)] = net + _add_to_result(result, prefix, device, net) net = _veth_peer_network(context, veth, inventory_hostname) device = veth['peer'] - result["%s%s" % (prefix, device)] = net + _add_to_result(result, prefix, device, net) return result diff --git a/kayobe/tests/unit/plugins/filter/test_networkd.py b/kayobe/tests/unit/plugins/filter/test_networkd.py index 5d17f1cb1..12d2c3d61 100644 --- a/kayobe/tests/unit/plugins/filter/test_networkd.py +++ b/kayobe/tests/unit/plugins/filter/test_networkd.py @@ -298,7 +298,6 @@ class TestNetworkdNetworks(BaseNetworkdTest): { "Network": [ {"Address": "1.2.3.4/24"}, - {"Broadcast": "true"}, ] }, ] @@ -347,7 +346,6 @@ class TestNetworkdNetworks(BaseNetworkdTest): { "Network": [ {"Address": "1.2.3.4/24"}, - {"Broadcast": "true"}, {"Gateway": "1.2.3.1"}, {"DHCP": "yes"}, {'UseGateway': "false"}, @@ -400,6 +398,18 @@ class TestNetworkdNetworks(BaseNetworkdTest): def test_vlan(self): nets = networkd.networkd_networks(self.context, ["net2"]) expected = { + "50-kayobe-eth0": [ + { + "Match": [ + {"Name": "eth0"} + ], + }, + { + "Network": [ + {"VLAN": "eth0.2"}, + ] + }, + ], "50-kayobe-eth0.2": [ { "Match": [ @@ -422,7 +432,6 @@ class TestNetworkdNetworks(BaseNetworkdTest): { "Network": [ {"Address": "1.2.3.4/24"}, - {"Broadcast": "true"}, {"VLAN": "eth0.2"}, ] }, @@ -527,6 +536,106 @@ class TestNetworkdNetworks(BaseNetworkdTest): } self.assertEqual(expected, nets) + def test_bridge_with_bridge_port_vlan(self): + # Test the case where one of the bridge ports has a VLAN subinterface. + self._update_context({ + "net2_interface": "eth1.2", + }) + nets = networkd.networkd_networks(self.context, ["net2", "net3"]) + expected = { + "50-kayobe-br0": [ + { + "Match": [ + {"Name": "br0"} + ] + }, + ], + "50-kayobe-eth0": [ + { + "Match": [ + {"Name": "eth0"} + ] + }, + { + "Network": [ + {"Bridge": "br0"}, + ] + }, + ], + "50-kayobe-eth1": [ + { + "Match": [ + {"Name": "eth1"} + ] + }, + { + "Network": [ + {"Bridge": "br0"}, + {"VLAN": "eth1.2"} + ] + }, + ], + "50-kayobe-eth1.2": [ + { + "Match": [ + {"Name": "eth1.2"} + ] + }, + ], + } + self.assertEqual(expected, nets) + + def test_bridge_with_bridge_port_vlan_net(self): + # Test the case where one of the bridge ports has a VLAN subinterface, + # and is also a Kayobe network (here eth0 is net1). + self._update_context({ + "net1_ips": None, + }) + nets = networkd.networkd_networks(self.context, + ["net1", "net2", "net3"]) + expected = { + "50-kayobe-br0": [ + { + "Match": [ + {"Name": "br0"} + ] + }, + ], + "50-kayobe-eth0": [ + { + "Match": [ + {"Name": "eth0"} + ] + }, + { + "Network": [ + {"Bridge": "br0"}, + {"VLAN": "eth0.2"} + ] + }, + ], + "50-kayobe-eth0.2": [ + { + "Match": [ + {"Name": "eth0.2"} + ] + }, + ], + "50-kayobe-eth1": [ + { + "Match": [ + {"Name": "eth1"} + ] + }, + { + "Network": [ + {"Bridge": "br0"}, + ] + }, + ] + } + self.assertEqual(expected, nets) + def test_bridge_no_interface(self): self._update_context({"net3_interface": None}) self.assertRaises(errors.AnsibleFilterError, @@ -617,6 +726,106 @@ class TestNetworkdNetworks(BaseNetworkdTest): } self.assertEqual(expected, nets) + def test_bond_with_bond_member_vlan(self): + # Test the case where one of the bond members has a VLAN subinterface. + self._update_context({ + "net2_interface": "eth1.2", + }) + nets = networkd.networkd_networks(self.context, ["net2", "net4"]) + expected = { + "50-kayobe-bond0": [ + { + "Match": [ + {"Name": "bond0"} + ] + }, + ], + "50-kayobe-eth0": [ + { + "Match": [ + {"Name": "eth0"} + ] + }, + { + "Network": [ + {"Bond": "bond0"}, + ] + }, + ], + "50-kayobe-eth1": [ + { + "Match": [ + {"Name": "eth1"} + ] + }, + { + "Network": [ + {"Bond": "bond0"}, + {"VLAN": "eth1.2"}, + ] + }, + ], + "50-kayobe-eth1.2": [ + { + "Match": [ + {"Name": "eth1.2"} + ] + }, + ], + } + self.assertEqual(expected, nets) + + def test_bond_with_bond_member_vlan_net(self): + # Test the case where one of the bond members has a VLAN subinterface, + # and is also a Kayobe network (here eth0 is net1). + self._update_context({ + "net1_ips": None, + }) + nets = networkd.networkd_networks(self.context, + ["net1", "net2", "net4"]) + expected = { + "50-kayobe-bond0": [ + { + "Match": [ + {"Name": "bond0"} + ] + }, + ], + "50-kayobe-eth0": [ + { + "Match": [ + {"Name": "eth0"} + ] + }, + { + "Network": [ + {"Bond": "bond0"}, + {"VLAN": "eth0.2"}, + ] + }, + ], + "50-kayobe-eth0.2": [ + { + "Match": [ + {"Name": "eth0.2"} + ] + }, + ], + "50-kayobe-eth1": [ + { + "Match": [ + {"Name": "eth1"} + ] + }, + { + "Network": [ + {"Bond": "bond0"}, + ] + }, + ] + } + self.assertEqual(expected, nets) + def test_bond_no_interface(self): self._update_context({"net4_interface": None}) self.assertRaises(errors.AnsibleFilterError, @@ -737,7 +946,6 @@ class TestNetworkdNetworks(BaseNetworkdTest): { "Network": [ {"Address": "1.2.3.4/24"}, - {"Broadcast": "true"}, ] }, ] @@ -760,7 +968,6 @@ class TestNetworkdNetworks(BaseNetworkdTest): { "Network": [ {"Address": "1.2.3.4/24"}, - {"Broadcast": "true"}, {"VLAN": "eth0.2"}, ] }, diff --git a/tox.ini b/tox.ini index 43e829a20..bb75ac49e 100644 --- a/tox.ini +++ b/tox.ini @@ -135,8 +135,9 @@ commands = [flake8] # E123, E125 skipped as they are invalid PEP-8. +# W504 line break after binary operator show-source = True -ignore = E123,E125 +ignore = E123,E125,W504 builtins = _ exclude=.venv,.git,.tox,dist,doc,*lib/python*,*egg,build