From dff646c5c1e7e22057c24ebd2354ab6eb5927ce0 Mon Sep 17 00:00:00 2001 From: Travis Truman Date: Mon, 27 Jun 2016 15:53:08 -0400 Subject: [PATCH] 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 | 53 +++++++------------ ...network_config_check-66778387f38b9e0c.yaml | 8 +++ tests/test_inventory.py | 48 ++++++++--------- 3 files changed, 49 insertions(+), 60 deletions(-) create mode 100644 releasenotes/notes/management_network_config_check-66778387f38b9e0c.yaml diff --git a/playbooks/inventory/dynamic_inventory.py b/playbooks/inventory/dynamic_inventory.py index 8ec4cc2db6..7dfeaa4286 100755 --- a/playbooks/inventory/dynamic_inventory.py +++ b/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) diff --git a/releasenotes/notes/management_network_config_check-66778387f38b9e0c.yaml b/releasenotes/notes/management_network_config_check-66778387f38b9e0c.yaml new file mode 100644 index 0000000000..637462c0f2 --- /dev/null +++ b/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. diff --git a/tests/test_inventory.py b/tests/test_inventory.py index 409145a450..9d8811b052 100644 --- a/tests/test_inventory.py +++ b/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()