Check for two IP addresses assigned to same host

When parsing the openstack_user_config.yml file, the code would not
account for user mistakes when multiple IP addresses were specified for
the same hostname. When multiple IP addresses were specified, the last
one parsed would be written to the inventory.

This change instead throws a runtime error when this situation arises,
so that the invalid config cannot be written.

The tox.ini configuration is modified to make sure that the insert order
on the configuration dictionary is the same on every run of the tests.
Were this not set, the insertion order may well change dependon on the
hash seed, which would cause test failures because the assertions would
not match.

An OrderedDict is also used to ensure platform differences don't affect
testing order. The behavior of this class shouldn't differ from normal
dictionaries in a way that invalidates the test cases.

Change-Id: I7c724b1dd668a8372bf2dafaf3461e0a3cb1a557

playbooks/inventory/dynamic_inventory.py

@@ -68,6 +68,24 @@ class MultipleHostsWithOneIPError(Exception):
         return self.message
 
 
+class MultipleIpForHostError(Exception):
+    def __init__(self, hostname, current_ip, new_ip):
+        self.hostname = hostname
+        self.current_ip = current_ip
+        self.new_ip = new_ip
+
+        # Sort the IPs for our error message so we're always consistent.
+        ips = [current_ip, new_ip]
+        ips.sort()
+
+        error_msg = "Host {hostname} has both {ips[0]} and {ips[1]} assigned"
+
+        self.message = error_msg.format(hostname=hostname, ips=ips)
+
+    def __str__(self):
+        return self.message
+
+
 def args():
     """Setup argument Parsing."""
     parser = argparse.ArgumentParser(
@@ -389,8 +407,8 @@ def user_defined_setup(config, inventory):
                 return
 
             for _key, _value in value.iteritems():
-                if _key not in inventory['_meta']['hostvars']:
+                if _key not in hvs:
-                    inventory['_meta']['hostvars'][_key] = {}
+                    hvs[_key] = {}
 
                 hvs[_key].update({
                     'ansible_ssh_host': _value['ip'],
@@ -894,6 +912,28 @@ def _check_same_ip_to_multiple_host(config):
                         raise MultipleHostsWithOneIPError(*info)
 
 
+def _check_multiple_ips_to_host(config):
+    """Check for multiple IPs assigned to a single hostname
+
+    :param: config: ``dict`` User provided configuration
+    """
+
+    # Extract only the dictionaries in the host groups.
+    host_ip_map = {}
+    for groupnames, group in config.items():
+        if '_hosts' in groupnames:
+            for hostname, entries in group.items():
+                if hostname not in host_ip_map:
+                    host_ip_map[hostname] = entries['ip']
+                else:
+                    current_ip = host_ip_map[hostname]
+                    new_ip = entries['ip']
+                    if not current_ip == new_ip:
+                        raise MultipleIpForHostError(hostname, current_ip,
+                                                     new_ip)
+    return True
+
+
 def _check_config_settings(cidr_networks, config, container_skel):
     """check preciseness of config settings
 
@@ -938,6 +978,8 @@ def _check_config_settings(cidr_networks, config, container_skel):
     # look for same ip address assigned to different hosts
     _check_same_ip_to_multiple_host(config)
 
+    _check_multiple_ips_to_host(config)
+
 
 def load_environment(config_path):
     """Create an environment dictionary from config files

releasenotes/notes/multiple-ips-for-host-f27cb1f1e878640d.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - Assigning multiple IP addresses to the same host name will now result in
+    an inventory error before running any playbooks.

tests/test_inventory.py

@@ -390,6 +390,11 @@ class TestConfigChecks(unittest.TestCase):
         user_defined_config[group][new_hostname] = old_hostname_settings
         self.write_config()
 
+    def set_new_ip(self, user_defined_config, group, hostname, ip):
+        # Sets an IP address for a specified host.
+        user_defined_config[group][hostname]['ip'] = ip
+        self.write_config()
+
     def test_provider_networks_check(self):
         # create config file without provider networks
         self.delete_config_key(self.user_defined_config, 'provider_networks')
@@ -440,6 +445,55 @@ class TestConfigChecks(unittest.TestCase):
                        "assign same ip to both hosts")
         self.assertEqual(context.exception.message, expectedLog)
 
+    def test_one_host_two_ips_externally(self):
+        # haproxy chosen because it was last in the config file as of
+        # writing
+        self.set_new_ip(self.user_defined_config, 'haproxy_hosts', 'aio1',
+                        '172.29.236.101')
+        with self.assertRaises(di.MultipleIpForHostError) as context:
+            get_inventory()
+        expectedLog = ("Host aio1 has both 172.29.236.100 and 172.29.236.101 "
+                       "assigned")
+        self.assertEqual(context.exception.message, expectedLog)
+
+    def test_two_ips(self):
+        # Use an OrderedDict to be certain our testing order is preserved
+        # Even with the same hash seed, different OSes get different results,
+        # eg. local OS X vs gate's Linux
+        config = collections.OrderedDict()
+        config['infra_hosts'] = {
+            'host1': {
+                'ip': '192.168.1.1'
+            }
+        }
+        config['compute_hosts'] = {
+            'host1': {
+                'ip': '192.168.1.2'
+            }
+        }
+
+        with self.assertRaises(di.MultipleIpForHostError) as context:
+            di._check_multiple_ips_to_host(config)
+        self.assertEqual(context.exception.current_ip, '192.168.1.1')
+        self.assertEqual(context.exception.new_ip, '192.168.1.2')
+        self.assertEqual(context.exception.hostname, 'host1')
+
+    def test_correct_hostname_ip_map(self):
+        config = {
+            'infra_hosts': {
+                'host1': {
+                    'ip': '192.168.1.1'
+                }
+            },
+            'compute_hosts': {
+                'host2': {
+                    'ip': '192.168.1.2'
+                }
+            },
+        }
+        ret = di._check_multiple_ips_to_host(config)
+        self.assertTrue(ret)
+
     def tearDown(self):
         if self.config_changed:
             # get back our initial user config file

tox.ini

@@ -90,6 +90,10 @@ commands =
       {toxinidir}/playbooks/*.yml"
 
 [testenv:inventory]
+# Use a fixed seed since some inventory tests rely on specific ordering
+setenv =
+    {[testenv]setenv}
+    PYTHONHASHSEED = 100
 commands =
     coverage erase
     coverage run {toxinidir}/tests/test_inventory.py