Test static route settings

Provider networks can be given static route information that is
populated on all containers. This commit only tests the common cases
around that code - adding a static route as expected, and attempting to
add one with missing information.

Currently, the code fails silently if the cidr or gateway for a static
route are missing. This change adds an exception informing the user that
there is missing static route information. The information is
purposefully general (no specific cases for missing cidr/gateway) in
order to keep the conditionals simple in the code.

Also, the change now checks to make sure that a value is present in
addition to the key, and the variable names are clarified. The values
provided are not check to be valid CIDRs or IP addresses at this time.

A release note and documentation were added given the new errors.

The test cases were split into their own class so as to reduce the
sprawl in the TestConfigChecks class.

Change-Id: I7e36736c1dbd86b7131365b6c27ff2c221814059

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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

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:
+            for route in static_routes:
-                # only add static routes if they are specified correctly
+                # only add static routes if they are specified correctly;
-                if 'cidr' in s and 'gateway' in s:
+                # that is, the key and a value must be present. This doesn't
-                    networks[old_address]['static_routes'].append(s)
+                # 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):

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.

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