Remove _net_address_search from dynamic inventory

This function was mutating user-specified configuration
for the provider_networks list. Rather than change the config
the user provides, we should instead error when what is provided
does not meet our preconditions.

Now, if the provider network configuration for the management
network is missing the keys 'is_container_address' or 'is_ssh_address'
or the value of those keys is false, the dynamic inventory will exit
with a failure and a helpful message indicating the configuration issue.

Test cases associated with the function have also been removed.

Change-Id: I7d3a6229bef35e8e057a3b95ed5bfc422c1a14a3

playbooks/inventory/dynamic_inventory.py

@@ -70,6 +70,20 @@ class MultipleHostsWithOneIPError(Exception):
         return self.message
 
 
+class ProviderNetworkMisconfiguration(Exception):
+    def __init__(self, queue_name):
+        self.queue_name = queue_name
+
+        error_msg = ("Provider network with queue '{queue}' "
+                     "requires 'is_container_address' and "
+                     "'is_ssh_address' to be set to True.")
+
+        self.message = error_msg.format(queue=self.queue_name)
+
+    def __str__(self):
+        return self.message
+
+
 class MultipleIpForHostError(Exception):
     def __init__(self, hostname, current_ip, new_ip):
         self.hostname = hostname
@@ -665,27 +679,6 @@ def _add_additional_networks(key, inventory, ip_q, q_name, netmask, interface,
                 networks[old_address]['static_routes'].append(route)
 
 
-def _net_address_search(provider_networks, main_network, key):
-    """Set the key network type to the main network if not specified.
-
-    :param provider_networks: ``list`` Network list of ``dict``s
-    :param main_network: ``str`` The name of the main network bridge.
-    :param key: ``str`` The name of the key to set true.
-    """
-    for pn in provider_networks:
-        # p_net are the provider_network values
-        p_net = pn.get('network')
-        if p_net:
-            # Check for the key
-            if p_net.get(key):
-                break
-            else:
-                if p_net.get('container_bridge') == main_network:
-                    p_net[key] = True
-
-    return provider_networks
-
-
 def container_skel_load(container_skel, inventory, config):
     """Build out all containers as defined in the environment file.
 
@@ -719,18 +712,6 @@ def container_skel_load(container_skel, inventory, config):
         overrides = config['global_overrides']
         # iterate over a list of provider_networks, var=pn
         pns = overrides.get('provider_networks', list())
-        pns = _net_address_search(
-            provider_networks=pns,
-            main_network=config['global_overrides']['management_bridge'],
-            key='is_ssh_address'
-        )
-
-        pns = _net_address_search(
-            provider_networks=pns,
-            main_network=config['global_overrides']['management_bridge'],
-            key='is_container_address'
-        )
-
         for pn in pns:
             # p_net are the provider_network values
             p_net = pn.get('network')
@@ -1004,6 +985,12 @@ def _check_config_settings(cidr_networks, config, container_skel):
                     raise SystemExit(
                         "can't find " + q_name + " in cidr_networks"
                     )
+                if (p_net.get('container_bridge') ==
+                        overrides.get('management_bridge')):
+                    if (not p_net.get('is_ssh_address') or
+                            not p_net.get('is_container_address')):
+                        raise ProviderNetworkMisconfiguration(q_name)
+
     # look for same ip address assigned to different hosts
     _check_same_ip_to_multiple_host(config)
 

releasenotes/notes/management_network_config_check-66778387f38b9e0c.yaml

@@ -0,0 +1,8 @@
+---
+upgrade:
+  - The dynamic_inventory script previously set the provider network
+    attributes ``is_container_address`` and ``is_ssh_address`` to True
+    for the management network regardless of whether a deployer had them
+    configured this way or not. Now, these attributes must be configured
+    by deployers and the dynamic_inventory script will fail if they are
+    missing or not True.

tests/test_inventory.py

@@ -375,6 +375,14 @@ class TestConfigChecks(unittest.TestCase):
         del self.user_defined_config['cidr_networks'][net_name]
         self.write_config()
 
+    def delete_provider_network_key(self, net_name, key):
+        pns = self.user_defined_config['global_overrides']['provider_networks']
+        for net in pns:
+            if 'ip_from_q' in net['network']:
+                if net['network']['ip_from_q'] == net_name:
+                    if key in net['network']:
+                        del net['network'][key]
+
     def write_config(self):
         self.config_changed = True
         # rename temporarily our user_config_file so we can use the new one
@@ -397,6 +405,19 @@ class TestConfigChecks(unittest.TestCase):
                        "user config.")
         self.assertEqual(context.exception.message, expectedLog)
 
+    def test_management_network_malformed(self):
+        self.delete_provider_network_key('container', 'is_container_address')
+        self.delete_provider_network_key('container', 'is_ssh_address')
+        self.write_config()
+
+        with self.assertRaises(di.ProviderNetworkMisconfiguration) as context:
+            get_inventory()
+        expectedLog = ("Provider network with queue 'container' "
+                       "requires 'is_container_address' and "
+                       "'is_ssh_address' to be set to True.")
+        self.assertEqual(context.exception.message, expectedLog)
+        self.restore_config()
+
     def test_missing_cidr_network_present_in_provider(self):
         self.delete_provider_network('storage')
         with self.assertRaises(SystemExit) as context:
@@ -621,33 +642,6 @@ class TestStaticRouteConfig(TestConfigChecks):
         self.assertEqual(exception.message, self.expectedMsg)
 
 
-class TestNetAddressSearch(unittest.TestCase):
-    def test_net_address_search_key_not_found(self):
-        pns = [
-            {'network': {'container_bridge': 'br-mgmt'}}
-        ]
-        new_pns = di._net_address_search(pns, 'br-mgmt', 'is_ssh_address')
-
-        self.assertTrue(new_pns[0]['network']['is_ssh_address'])
-
-    def test_net_address_search_key_not_found_bridge_doesnt_match(self):
-        pns = [
-            {'network': {'container_bridge': 'lxcbr0'}}
-        ]
-        new_pns = di._net_address_search(pns, 'br-mgmt', 'is_ssh_address')
-
-        self.assertNotIn('is_ssh_address', new_pns[0]['network'])
-
-    def test_net_address_search_key_found(self):
-        pns = [
-            {'network': {'container_bridge': 'br-mgmt',
-                         'is_ssh_address': True}}
-        ]
-        new_pns = di._net_address_search(pns, 'br-mgmt', 'is_ssh_address')
-
-        self.assertEqual(pns, new_pns)
-
-
 class TestGlobalOverridesConfigDeletion(TestConfigChecks):
     def setUp(self):
         super(TestGlobalOverridesConfigDeletion, self).setUp()