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

@@ -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)

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
+    IPv4 address of the default router. A default value is only provided
-    in most cases and should be changed.
+    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:
 

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 = []
+        network_metadata = get_network_data(module.params)
-        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
-        }
     facts = {'network_metadata': network_metadata}
 
     module.exit_json(changed=False, ansible_facts=facts)

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_gateway: "{{ '192.168.122.1' if testing | bool else '' }}"
-ipv4_nameserver: 8.8.8.8
-network_mtu: 1500
 
 # Default ISO generation utility
 iso_gen_utility: "mkisofs"

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_gateway: "{{ ipv4_gateway | default(omit) }}"
-    ipv4_interface_mac: "{{ ipv4_interface_mac | default('') }}"
+    ipv4_interface_mac: "{{ ipv4_interface_mac | default(omit) }}"
-    ipv4_nameserver: "{% if ipv4_nameserver is string %}['{{ ipv4_nameserver | default('') }}']{% else %}{{ ipv4_nameserver }}{% endif %}"
+    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('') }}"
+    vlan_id: "{{ vlan_id | default(omit) }}"
-    network_mtu: "{{ network_mtu | default('1500') }}"
+    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

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 %}

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.