From f65b52ed8d0b2644657589cf8f22ac12efd98c25 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 17 Feb 2022 19:04:45 +0100 Subject: [PATCH] Remove questionable defaults from the network configuration We definitely should not default to using Google DNS or assuming a specific MTU. The default gateway won't work for many deployments. Remove them, refactor network_metadata and add some unit tests. Change-Id: Icb5d077a5f68b5affc1ed545e04c96dcc01d9f3e --- bifrost/tests/unit/test_network_metadata.py | 93 ++++++++ doc/source/user/howto.rst | 7 +- playbooks/library/network_metadata.py | 206 ++++++++++-------- .../defaults/main.yml | 16 +- .../tasks/main.yml | 10 +- .../templates/interfaces.j2 | 4 + .../network-metadata-0fe34ec559ec3b44.yaml | 5 + 7 files changed, 229 insertions(+), 112 deletions(-) create mode 100644 bifrost/tests/unit/test_network_metadata.py create mode 100644 releasenotes/notes/network-metadata-0fe34ec559ec3b44.yaml diff --git a/bifrost/tests/unit/test_network_metadata.py b/bifrost/tests/unit/test_network_metadata.py new file mode 100644 index 000000000..000e9ff34 --- /dev/null +++ b/bifrost/tests/unit/test_network_metadata.py @@ -0,0 +1,93 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import os.path + +from bifrost.tests import base + + +# NOTE(dtantsur): the module is not importable, hence all the troubles +get_network_data = None # make pep8 happy +FILE = os.path.join(os.path.dirname(os.path.abspath(__file__)), + '..', '..', '..', 'playbooks', 'library', + 'network_metadata.py') +with open(FILE) as fp: + # Not going to test the ansible bits, strip the imports away to avoid + # depending on it. + script = [line for line in fp if not line.startswith('from ansible.')] + exec(''.join(script)) + + +TEST_MAC = '12:34:56:78:9a:bc' + + +class TestBifrostNetworkMetadata(base.TestCase): + + def test_simple(self): + params = { + 'nics': [{'mac': TEST_MAC}], + 'ipv4_address': '1.2.3.4', + 'ipv4_subnet_mask': '255.255.0.0', + } + result = get_network_data(params) + self.assertEqual({ + 'links': [ + {'id': TEST_MAC, + 'ethernet_mac_address': TEST_MAC, + 'type': 'phy'}, + ], + 'networks': [ + {'id': f'ipv4-{TEST_MAC}', + 'link': TEST_MAC, + 'type': 'ipv4', + 'ip_address': '1.2.3.4', + 'netmask': '255.255.0.0', + 'routes': []} + ], + 'services': [], + }, result) + + def test_everything(self): + another_mac = 'aa:aa:aa:bb:cc:dd' + params = { + 'nics': [{'mac': another_mac}, {'mac': TEST_MAC}], + 'ipv4_address': '1.2.3.4', + 'ipv4_subnet_mask': '255.255.0.0', + 'ipv4_interface_mac': TEST_MAC, + 'ipv4_gateway': '1.2.1.1', + 'ipv4_nameserver': ['1.1.1.1'], + } + result = get_network_data(params) + self.assertEqual({ + 'links': [ + {'id': TEST_MAC, + 'ethernet_mac_address': TEST_MAC, + 'type': 'phy'}, + ], + 'networks': [ + {'id': f'ipv4-{TEST_MAC}', + 'link': TEST_MAC, + 'type': 'ipv4', + 'ip_address': '1.2.3.4', + 'netmask': '255.255.0.0', + 'dns_nameservers': ['1.1.1.1'], + 'routes': [{ + 'network': '0.0.0.0', + 'netmask': '0.0.0.0', + 'gateway': '1.2.1.1', + }]} + ], + 'services': [{ + 'type': 'dns', + 'address': '1.1.1.1', + }], + }, result) diff --git a/doc/source/user/howto.rst b/doc/source/user/howto.rst index 029ac803d..d80512682 100644 --- a/doc/source/user/howto.rst +++ b/doc/source/user/howto.rst @@ -182,13 +182,12 @@ as cloud-init_ or glean_. The following fields can be set: MAC address of the interface to configure. If missing, the MAC address of the first NIC defined in the inventory is used. ``ipv4_gateway`` - IPv4 address of the default router. The Bifrost default will be incorrect - in most cases and should be changed. + IPv4 address of the default router. A default value is only provided + for testing case. ``ipv4_nameserver`` The server to use for name resolution (a string or a list). - Defaults to `8.8.8.8` (Google public DNS). ``network_mtu`` - MTU to use for the link. Defaults to 1500. + MTU to use for the link. For example: diff --git a/playbooks/library/network_metadata.py b/playbooks/library/network_metadata.py index 70ad0b665..6ab9b7459 100644 --- a/playbooks/library/network_metadata.py +++ b/playbooks/library/network_metadata.py @@ -24,6 +24,117 @@ extends_documentation_fragment: openstack ''' +def get_network_data(params): + links = [] + networks = [] + + if params.get('ipv4_interface_mac'): + nic_id = params['ipv4_interface_mac'] + + if params.get('vlan_id'): + nic_id = 'vlan-%s' % params['ipv4_interface_mac'] + + links.append({ + 'id': nic_id, + 'type': 'vlan', + 'vlan_id': params['vlan_id'], + 'vlan_link': params['ipv4_interface_mac'], + 'vlan_mac_address': params['ipv4_interface_mac'], + }) + + link = { + 'id': params['ipv4_interface_mac'], + 'type': 'phy', + 'ethernet_mac_address': params['ipv4_interface_mac'], + } + if params.get('network_mtu'): + link['mtu'] = params['network_mtu'] + links.append(link) + + for nic in params['nics']: + if nic['mac'] == params['ipv4_interface_mac']: + network = { + 'id': 'ipv4-%s' % nic_id, + 'link': nic_id, + 'type': 'ipv4', + 'ip_address': params['ipv4_address'], + 'netmask': params['ipv4_subnet_mask'], + 'routes': [] + } + if params.get('ipv4_nameserver'): + network['dns_nameservers'] = \ + params['ipv4_nameserver'] + if params.get('ipv4_gateway'): + network['routes'].append({ + 'network': '0.0.0.0', + 'netmask': '0.0.0.0', + 'gateway': params['ipv4_gateway'] + }) + networks.append(network) + else: + for i, nic in enumerate(params['nics']): + nic_id = nic['mac'] + if params.get('vlan_id'): + nic_id = 'vlan-%s' % nic['mac'] + + links.append({ + 'id': nic_id, + 'type': 'vlan', + 'vlan_id': params['vlan_id'], + 'vlan_link': nic['mac'], + 'vlan_mac_address': nic['mac'] + }) + + link = { + 'id': nic['mac'], + 'type': 'phy', + 'ethernet_mac_address': nic['mac'], + } + if params.get('network_mtu'): + link['mtu'] = params['network_mtu'] + links.append(link) + + if i == 0: + network = { + 'id': 'ipv4-%s' % nic_id, + 'link': nic_id, + 'type': 'ipv4', + 'ip_address': params['ipv4_address'], + 'netmask': params['ipv4_subnet_mask'], + 'routes': [] + } + if params.get('ipv4_nameserver'): + network['dns_nameservers'] = \ + params['ipv4_nameserver'] + if params.get('ipv4_gateway'): + network['routes'].append({ + 'network': '0.0.0.0', + 'netmask': '0.0.0.0', + 'gateway': params['ipv4_gateway'] + }) + networks.append(network) + else: + networks.append({ + 'id': 'ipv4-dhcp-%s' % nic_id, + 'link': nic_id, + 'type': 'ipv4_dhcp', + }) + + services = [] + if params.get('ipv4_nameserver'): + for item in params['ipv4_nameserver']: + services.append({ + 'type': 'dns', + 'address': item + }) + + return { + 'links': links, + 'networks': networks, + 'services': services + } + + def main(): argument_spec = dict( ipv4_address=dict(required=False), @@ -41,100 +152,7 @@ def main(): network_metadata = module.params['node_network_data'] if not network_metadata: - links = [] - networks = [] - - if module.params['ipv4_interface_mac']: - nic_id = module.params['ipv4_interface_mac'] - - if module.params['vlan_id']: - nic_id = 'vlan-%s' % module.params['ipv4_interface_mac'] - - links.append({ - 'id': nic_id, - 'type': 'vlan', - 'vlan_id': module.params['vlan_id'], - 'vlan_link': module.params['ipv4_interface_mac'], - 'vlan_mac_address': module.params['ipv4_interface_mac'], - }) - - links.append({ - 'id': module.params['ipv4_interface_mac'], - 'type': 'phy', - 'ethernet_mac_address': module.params['ipv4_interface_mac'], - 'mtu': module.params['network_mtu'] - }) - - for nic in module.params['nics']: - if nic['mac'] == module.params['ipv4_interface_mac']: - networks.append({ - 'id': 'ipv4-%s' % nic_id, - 'link': nic_id, - 'type': 'ipv4', - 'ip_address': module.params['ipv4_address'], - 'netmask': module.params['ipv4_subnet_mask'], - 'dns_nameservers': module.params['ipv4_nameserver'], - 'routes': [{ - 'network': '0.0.0.0', - 'netmask': '0.0.0.0', - 'gateway': module.params['ipv4_gateway'] - }] - }) - else: - for i, nic in enumerate(module.params['nics']): - nic_id = nic['mac'] - if module.params['vlan_id']: - nic_id = 'vlan-%s' % nic['mac'] - - links.append({ - 'id': nic_id, - 'type': 'vlan', - 'vlan_id': module.params['vlan_id'], - 'vlan_link': nic['mac'], - 'vlan_mac_address': nic['mac'] - }) - - links.append({ - 'id': nic['mac'], - 'type': 'phy', - 'ethernet_mac_address': nic['mac'], - 'mtu': module.params['network_mtu'] - }) - - if i == 0: - networks.append({ - 'id': 'ipv4-%s' % nic_id, - 'link': nic_id, - 'type': 'ipv4', - 'ip_address': module.params['ipv4_address'], - 'netmask': module.params['ipv4_subnet_mask'], - 'dns_nameservers': module.params['ipv4_nameserver'], - 'routes': [{ - 'network': '0.0.0.0', - 'netmask': '0.0.0.0', - 'gateway': module.params['ipv4_gateway'] - }] - }) - else: - networks.append({ - 'id': 'ipv4-dhcp-%s' % nic_id, - 'link': nic_id, - 'type': 'ipv4_dhcp', - }) - - services = [] - if module.params['ipv4_nameserver']: - for item in module.params['ipv4_nameserver']: - services.append({ - 'type': 'dns', - 'address': item - }) - - network_metadata = { - 'links': links, - 'networks': networks, - 'services': services - } + network_metadata = get_network_data(module.params) facts = {'network_metadata': network_metadata} module.exit_json(changed=False, ansible_facts=facts) diff --git a/playbooks/roles/bifrost-configdrives-dynamic/defaults/main.yml b/playbooks/roles/bifrost-configdrives-dynamic/defaults/main.yml index 57e04566a..a77b3c966 100644 --- a/playbooks/roles/bifrost-configdrives-dynamic/defaults/main.yml +++ b/playbooks/roles/bifrost-configdrives-dynamic/defaults/main.yml @@ -1,9 +1,4 @@ --- -# write_interfaces_file is intended for utilizing base logic to write -# a debian style interfaces file into the configuration drive file -# such that cirros will receive basic network configuration when -# performing basic testing. -write_interfaces_file: false http_boot_folder: /var/lib/ironic/httpboot # Default location to the ssh public key for the user operating Bifrost. #ssh_public_key_path: "/path/to/id_rsa.pub" @@ -20,13 +15,16 @@ internal_ip: "{{ hostvars[ans_hostname]['ansible_' + ans_network_interface]['ipv node_default_network_interface: eth0 testing: false +use_cirros: false +# write_interfaces_file is intended for utilizing base logic to write +# a debian style interfaces file into the configuration drive file +# such that cirros will receive basic network configuration when +# performing basic testing. +write_interfaces_file: "{{ use_cirros }}" # Basic networking defaults -# TODO(TheJulia): Require these to be supplied some other way. ipv4_subnet_mask: 255.255.255.0 -ipv4_gateway: "{{ '192.168.122.1' if testing | bool else '192.168.1.1' }}" -ipv4_nameserver: 8.8.8.8 -network_mtu: 1500 +ipv4_gateway: "{{ '192.168.122.1' if testing | bool else '' }}" # Default ISO generation utility iso_gen_utility: "mkisofs" diff --git a/playbooks/roles/bifrost-configdrives-dynamic/tasks/main.yml b/playbooks/roles/bifrost-configdrives-dynamic/tasks/main.yml index b9f511026..5ae5749b0 100644 --- a/playbooks/roles/bifrost-configdrives-dynamic/tasks/main.yml +++ b/playbooks/roles/bifrost-configdrives-dynamic/tasks/main.yml @@ -60,12 +60,12 @@ - name: "Generate network_data" network_metadata: ipv4_address: "{{ ipv4_address | default('') }}" - ipv4_gateway: "{{ ipv4_gateway | default('') }}" - ipv4_interface_mac: "{{ ipv4_interface_mac | default('') }}" - ipv4_nameserver: "{% if ipv4_nameserver is string %}['{{ ipv4_nameserver | default('') }}']{% else %}{{ ipv4_nameserver }}{% endif %}" + ipv4_gateway: "{{ ipv4_gateway | default(omit) }}" + ipv4_interface_mac: "{{ ipv4_interface_mac | default(omit) }}" + ipv4_nameserver: "{% if ipv4_nameserver is string %}['{{ ipv4_nameserver }}']{% else %}{{ ipv4_nameserver | default(omit) }}{% endif %}" ipv4_subnet_mask: "{{ ipv4_subnet_mask | default('') }}" - vlan_id: "{{ vlan_id | default('') }}" - network_mtu: "{{ network_mtu | default('1500') }}" + vlan_id: "{{ vlan_id | default(omit) }}" + network_mtu: "{{ network_mtu | default(omit) }}" nics: "{{ node_info.node.nics | default(omit) }}" node_network_data: "{{ node_network_data | default(omit) }}" when: addressing_mode is undefined or "dhcp" not in addressing_mode diff --git a/playbooks/roles/bifrost-configdrives-dynamic/templates/interfaces.j2 b/playbooks/roles/bifrost-configdrives-dynamic/templates/interfaces.j2 index 2a9d03195..b1868c17c 100644 --- a/playbooks/roles/bifrost-configdrives-dynamic/templates/interfaces.j2 +++ b/playbooks/roles/bifrost-configdrives-dynamic/templates/interfaces.j2 @@ -9,6 +9,10 @@ iface {{ node_default_network_interface }} inet dhcp iface {{ node_default_network_interface }} inet static address {{ ipv4_address }} netmask {{ ipv4_subnet_mask }} +{% if ipv4_gateway | default('') != '' %} gateway {{ ipv4_gateway }} +{% endif %} +{% if ipv4_nameserver | default('') != '' %} dns-nameservers {{ ipv4_nameserver }} {% endif %} +{% endif %} diff --git a/releasenotes/notes/network-metadata-0fe34ec559ec3b44.yaml b/releasenotes/notes/network-metadata-0fe34ec559ec3b44.yaml new file mode 100644 index 000000000..150729b98 --- /dev/null +++ b/releasenotes/notes/network-metadata-0fe34ec559ec3b44.yaml @@ -0,0 +1,5 @@ +--- +upgrade: + - | + The parameters ``network_mtu``, ``ipv4_nameserver`` and ``ipv4_gateway`` + no longer have default values. If needed, specify them explicitly.