Reject unrecognized options in config objects (but still allow it at …

…the top-level) (#623)

* config: re-enable strict mode by manually removing excess top level keys

* tests: fix typo in stack_policy_path in functional tests

* config: ensure Schematics errors are converted during parsing

Validation already caught them, but parsing needs to do it too.

* Update changelog with strict parsing changes

* tests: fix stack policy path (should be relative to the tests dir)

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## Upcoming/Master
 
 - Add JSON and YAML codecs to file lookup
+- Improve config. validation by only allowing unrecognized keys at the top level
 
 ## 1.3.0 (2018-05-03)
 

stacker/config/__init__.py

@@ -13,7 +13,11 @@
 
 from schematics import Model
 from schematics.exceptions import ValidationError
-from schematics.exceptions import BaseError as SchematicsError
+from schematics.exceptions import (
+    BaseError as SchematicsError,
+    UndefinedValueError
+)
+
 from schematics.types import (
     ModelType,
     ListType,
@@ -137,19 +141,12 @@ def parse(raw_config):
                     tmp_list.append(tmp_dict)
                 config_dict[top_level_key] = tmp_list
 
-    # We have to enable non-strict mode, because people may be including top
-    # level keys for re-use with stacks (e.g. including something like
-    # `common_variables: &common_variables`).
-    #
-    # The unfortunate side effect of this is that it propagates down to every
-    # schematics model, and there doesn't seem to be a good way to only disable
-    # strict mode on a single model.
-    #
-    # If we enabled strict mode, it would break backwards compatibility, so we
-    # should consider enabling this in the future.
-    strict = False
-
-    return Config(config_dict, strict=strict)
+    # Top-level excess keys are removed by Config._convert, so enabling strict
+    # mode is fine here.
+    try:
+        return Config(config_dict, strict=True)
+    except SchematicsError as e:
+        raise exceptions.InvalidConfig(e.errors)
 
 
 def load(config):
@@ -407,9 +404,36 @@ class Config(Model):
     stacks = ListType(
         ModelType(Stack), default=[], validators=[not_empty_list])
 
-    def validate(self):
+    def _remove_excess_keys(self, data):
+        excess_keys = set(data.keys())
+        excess_keys -= self._schema.valid_input_keys
+        if not excess_keys:
+            return data
+
+        logger.debug('Removing excess keys from config input: %s',
+                     excess_keys)
+        clean_data = data.copy()
+        for key in excess_keys:
+            del clean_data[key]
+
+        return clean_data
+
+    def _convert(self, raw_data=None, context=None, **kwargs):
+        if raw_data is not None:
+            # Remove excess top-level keys, since we want to allow them to be
+            # used for custom user variables to be reference later. This is
+            # preferable to just disabling strict mode, as we can still
+            # disallow excess keys in the inner models.
+            raw_data = self._remove_excess_keys(raw_data)
+
+        return super(Config, self)._convert(raw_data=raw_data, context=context,
+                                            **kwargs)
+
+    def validate(self, *args, **kwargs):
         try:
-            super(Config, self).validate()
+            return super(Config, self).validate(*args, **kwargs)
+        except UndefinedValueError as e:
+            raise exceptions.InvalidConfig([e.message])
         except SchematicsError as e:
             raise exceptions.InvalidConfig(e.errors)
 

stacker/tests/test_config.py

@@ -537,6 +537,20 @@ def test_raise_construct_error_on_duplicate_stack_name_dict(self):
         with self.assertRaises(ConstructorError):
             parse(yaml_config)
 
+    def test_parse_invalid_inner_keys(self):
+        yaml_config = """
+        namespace: prod
+        stacks:
+        - name: vpc
+          class_path: blueprints.VPC
+          garbage: yes
+          variables:
+            Foo: bar
+        """
+
+        with self.assertRaises(exceptions.InvalidConfig):
+            parse(yaml_config)
+
 
 if __name__ == '__main__':
     unittest.main()

tests/test_suite/10_stacker_build-simple_build.bats

@@ -11,7 +11,7 @@ namespace: ${STACKER_NAMESPACE}
 stacks:
   - name: vpc
     class_path: stacker.tests.fixtures.mock_blueprints.VPC
-    stack_policy: tests/fixtures/stack_policies/default.json
+    stack_policy_path: ${PWD}/fixtures/stack_policies/default.json
     variables:
       PublicSubnets: 10.128.0.0/24,10.128.1.0/24,10.128.2.0/24,10.128.3.0/24
       PrivateSubnets: 10.128.8.0/22,10.128.12.0/22,10.128.16.0/22,10.128.20.0/22