diff --git a/doc/source/install-guide/configure-networking.rst b/doc/source/install-guide/configure-networking.rst index 2b9573f693..fc51e5c999 100644 --- a/doc/source/install-guide/configure-networking.rst +++ b/doc/source/install-guide/configure-networking.rst @@ -236,6 +236,11 @@ Adding static routes to network interfaces post-up ip route add 10.176.0.0/12 via 172.29.248.1 || true + The ``cidr`` and ``gateway`` values must *both* be specified, or the + inventory script will raise an error. Accuracy of the network information + is not checked within the inventory script, just that the keys and values + are present. + Setting an MTU on a network interface ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/playbooks/inventory/dynamic_inventory.py b/playbooks/inventory/dynamic_inventory.py index 914160d085..96aa32472c 100755 --- a/playbooks/inventory/dynamic_inventory.py +++ b/playbooks/inventory/dynamic_inventory.py @@ -87,6 +87,19 @@ class MultipleIpForHostError(Exception): return self.message +class MissingStaticRouteInfo(Exception): + def __init__(self, queue_name): + self.queue_name = queue_name + + error_msg = ("Static route provider network with queue '{queue}' " + "needs both 'cidr' and 'gateway' values.") + + self.message = error_msg.format(queue=self.queue_name) + + def __str__(self): + return self.message + + def args(arg_list): """Setup argument Parsing.""" parser = argparse.ArgumentParser( @@ -638,10 +651,19 @@ def _add_additional_networks(key, inventory, ip_q, q_name, netmask, interface, # NOTE: networks[old_address]['static_routes'] will get # regenerated on each run networks[old_address]['static_routes'] = [] - for s in static_routes: - # only add static routes if they are specified correctly - if 'cidr' in s and 'gateway' in s: - networks[old_address]['static_routes'].append(s) + for route in static_routes: + # only add static routes if they are specified correctly; + # that is, the key and a value must be present. This doesn't + # ensure that the values provided are routeable, just that + # they are not empty. + cidr_present = route.get('cidr', False) + gateway_present = route.get('gateway', False) + + if cidr_present and gateway_present: + networks[old_address]['static_routes'].append(route) + + elif not cidr_present or not gateway_present: + raise MissingStaticRouteInfo(q_name) def _net_address_search(provider_networks, main_network, key): diff --git a/releasenotes/notes/static_route_error_check-5e7ed6ddf9eb1d1f.yaml b/releasenotes/notes/static_route_error_check-5e7ed6ddf9eb1d1f.yaml new file mode 100644 index 0000000000..ea653f73af --- /dev/null +++ b/releasenotes/notes/static_route_error_check-5e7ed6ddf9eb1d1f.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - Static route information for provider networks now *must* include the + `cidr` and `gateway` information. If either key is missing, an error will + be raised and the dynamic_inventory.py script will halt before any Ansible + action is taken. Previously, if either key was missing, the inventory + script would continue silently without adding the static route + information to the networks. + + Note that this check does not validate the CIDR or gateway values, just + just that the values are present. diff --git a/tests/test_inventory.py b/tests/test_inventory.py index 64b3763a44..4e15c3941b 100644 --- a/tests/test_inventory.py +++ b/tests/test_inventory.py @@ -518,6 +518,98 @@ class TestConfigChecks(unittest.TestCase): os.rename(USER_CONFIG_FILE + ".tmp", USER_CONFIG_FILE) +class TestStaticRouteConfig(TestConfigChecks): + def setUp(self): + super(TestStaticRouteConfig, self).setUp() + self.expectedMsg = ("Static route provider network with queue " + "'container' needs both 'cidr' and 'gateway' " + "values.") + + def add_static_route(self, q_name, route_dict): + """Adds a static route to a provider network.""" + pn = self.user_defined_config['global_overrides']['provider_networks'] + for net in pn: + net_dict = net['network'] + q = net_dict.get('ip_from_q', None) + if q == q_name: + net_dict['static_routes'] = [route_dict] + self.write_config() + + def test_setting_static_route(self): + route_dict = {'cidr': '10.176.0.0/12', + 'gateway': '172.29.248.1'} + self.add_static_route('container', route_dict) + inventory = get_inventory() + + # Use aio1 and 'container_address' since they're known keys. + hostvars = inventory['_meta']['hostvars']['aio1'] + cont_add = hostvars['container_networks']['container_address'] + + self.assertIn('static_routes', cont_add) + + first_route = cont_add['static_routes'][0] + self.assertIn('cidr', first_route) + self.assertIn('gateway', first_route) + + def test_setting_bad_static_route_only_cidr(self): + route_dict = {'cidr': '10.176.0.0/12'} + self.add_static_route('container', route_dict) + + with self.assertRaises(di.MissingStaticRouteInfo) as context: + get_inventory() + + exception = context.exception + + self.assertEqual(str(exception), self.expectedMsg) + + def test_setting_bad_static_route_only_gateway(self): + route_dict = {'gateway': '172.29.248.1'} + self.add_static_route('container', route_dict) + + with self.assertRaises(di.MissingStaticRouteInfo) as context: + get_inventory() + + exception = context.exception + + self.assertEqual(exception.message, self.expectedMsg) + + def test_setting_bad_gateway_value(self): + route_dict = {'cidr': '10.176.0.0/12', + 'gateway': None} + self.add_static_route('container', route_dict) + + with self.assertRaises(di.MissingStaticRouteInfo) as context: + get_inventory() + + exception = context.exception + + self.assertEqual(exception.message, self.expectedMsg) + + def test_setting_bad_cidr_value(self): + route_dict = {'cidr': None, + 'gateway': '172.29.248.1'} + self.add_static_route('container', route_dict) + + with self.assertRaises(di.MissingStaticRouteInfo) as context: + get_inventory() + + exception = context.exception + + self.assertEqual(exception.message, self.expectedMsg) + + def test_setting_bad_cidr_gateway_value(self): + route_dict = {'cidr': None, + 'gateway': None} + self.add_static_route('container', route_dict) + + with self.assertRaises(di.MissingStaticRouteInfo) as context: + get_inventory() + + exception = context.exception + + self.assertEqual(exception.message, self.expectedMsg) + + class TestMultipleRuns(unittest.TestCase): def test_creating_backup_file(self): # Generate the initial inventory files