Move handling keys for hieradata template completely to instack_undercloud module

Currently it's easy to create a new instack_env variable and forget to white-list it
in 02-puppet-stack-config. This change makes instack_undercloud.undercloud module
the primary source of truth on what instack_env can and cannot contain.

Closes-Bug: #1660671
Change-Id: I82eef3ba4bb172b0260f72ce50f52bfb6bfc8e21

elements/puppet-stack-config/install.d/02-puppet-stack-config

@@ -27,28 +27,8 @@ template = os.path.join(os.path.dirname(__file__),
                         '..',
                         'puppet-stack-config.yaml.template')
 
-# Only variables that are not oslo.config opts need to be added here
-context = {
-    'INSPECTION_COLLECTORS': os.environ['INSPECTION_COLLECTORS'],
-    'INSPECTION_KERNEL_ARGS': os.environ['INSPECTION_KERNEL_ARGS'],
-    'TRIPLEO_INSTALL_USER': os.environ['TRIPLEO_INSTALL_USER'],
-    'TRIPLEO_UNDERCLOUD_CONF_FILE':
-        os.environ['TRIPLEO_UNDERCLOUD_CONF_FILE'],
-    'TRIPLEO_UNDERCLOUD_PASSWORD_FILE':
-        os.environ['TRIPLEO_UNDERCLOUD_PASSWORD_FILE'],
-    'MEMBER_ROLE_EXISTS': os.environ.get('MEMBER_ROLE_EXISTS', 'false'),
-}
-
-# Include all config opts in the context
-for _, group in undercloud.list_opts():
-    for opt in group:
-        upper_name = opt.name.upper()
-        context[upper_name] = os.environ[upper_name]
-
-# Mustache conditional logic requires ENABLE_NOVAJOIN to be undefined
-# when novajoin is not enabled.
-if context['ENABLE_NOVAJOIN'].lower() == 'false':
-    del context['ENABLE_NOVAJOIN']
+context = {item: os.environ.get(item)
+           for item in undercloud.InstackEnvironment.PUPPET_KEYS}
 
 endpoint_context = {}
 for k, v in os.environ.items():

instack_undercloud/tests/test_undercloud.py

@@ -856,3 +856,23 @@ class TestUpgradeFact(base.BaseTestCase):
         ]
         mock_run.assert_has_calls(run_calls)
         self.assertEqual(mock_run.call_count, 2)
+
+
+class TestInstackEnvironment(BaseTestCase):
+    def test_set_allowed_keys(self):
+        env = undercloud.InstackEnvironment()
+        env['HOSTNAME'] = 'localhost1'
+        env['INSPECTION_COLLECTORS'] = 'a,b,c'
+
+    def test_set_unknown_keys(self):
+        env = undercloud.InstackEnvironment()
+
+        def _set():
+            env['CATS_AND_DOGS_PATH'] = '/home'
+
+        self.assertRaisesRegexp(KeyError, 'CATS_AND_DOGS_PATH', _set)
+
+    def test_get_always_allowed(self):
+        env = undercloud.InstackEnvironment()
+        env.get('HOSTNAME')
+        env.get('CATS_AND_DOGS_PATH')

instack_undercloud/undercloud.py

@@ -860,6 +860,40 @@ def _member_role_exists(instack_env):
     instack_env['MEMBER_ROLE_EXISTS'] = six.text_type(role_exists)
 
 
+class InstackEnvironment(dict):
+    """An environment to pass to Puppet with some safety checks.
+
+    Keeps lists of variables we add to the operating system environment,
+    and ensures that we don't anything not defined there.
+    """
+
+    INSTACK_KEYS = {'HOSTNAME', 'ELEMENTS_PATH', 'NODE_DIST', 'JSONFILE',
+                    'REG_METHOD', 'REG_HALT_UNREGISTER', 'PUBLIC_INTERFACE_IP'}
+    """The variables instack and/or used elements can read."""
+
+    DYNAMIC_KEYS = {'INSPECTION_COLLECTORS', 'INSPECTION_KERNEL_ARGS',
+                    'INSPECTION_NODE_NOT_FOUND_HOOK',
+                    'TRIPLEO_INSTALL_USER', 'TRIPLEO_UNDERCLOUD_CONF_FILE',
+                    'TRIPLEO_UNDERCLOUD_PASSWORD_FILE', 'MEMBER_ROLE_EXISTS'}
+    """The variables we calculate in _generate_environment call."""
+
+    PUPPET_KEYS = DYNAMIC_KEYS | {opt.name.upper() for _, group in list_opts()
+                                  for opt in group}
+    """Keys we pass for formatting the resulting hieradata."""
+
+    SET_ALLOWED_KEYS = DYNAMIC_KEYS | INSTACK_KEYS | PUPPET_KEYS
+    """Keys which we allow to add/change in this environment."""
+
+    def __init__(self):
+        super(InstackEnvironment, self).__init__(os.environ)
+
+    def __setitem__(self, key, value):
+        if key not in self.SET_ALLOWED_KEYS:
+            raise KeyError('Key %s is not allowed for an InstackEnvironment' %
+                           key)
+        return super(InstackEnvironment, self).__setitem__(key, value)
+
+
 def _generate_environment(instack_root):
     """Generate an environment dict for instack
 
@@ -869,7 +903,7 @@ def _generate_environment(instack_root):
     :param instack_root: The path containing the instack-undercloud elements
         and json files.
     """
-    instack_env = dict(os.environ)
+    instack_env = InstackEnvironment()
     # Rabbit uses HOSTNAME, so we need to make sure it's right
     instack_env['HOSTNAME'] = CONF.undercloud_hostname or socket.gethostname()
 
@@ -965,6 +999,11 @@ def _generate_environment(instack_root):
     instack_env['TRIPLEO_UNDERCLOUD_CONF_FILE'] = PATHS.CONF_PATH
     instack_env['TRIPLEO_UNDERCLOUD_PASSWORD_FILE'] = PATHS.PASSWORD_PATH
 
+    # Mustache conditional logic requires ENABLE_NOVAJOIN to be undefined
+    # when novajoin is not enabled.
+    if instack_env['ENABLE_NOVAJOIN'].lower() == 'false':
+        del instack_env['ENABLE_NOVAJOIN']
+
     _generate_endpoints(instack_env)
 
     _write_password_file(instack_env)