From 94cc1e8a4eea1eb7ee8329caa4b25a61cd98c648 Mon Sep 17 00:00:00 2001 From: Daniel Miranda Date: Sun, 1 Jul 2018 18:08:31 -0300 Subject: [PATCH 1/5] config: re-enable strict mode by manually removing excess top level keys --- stacker/config/__init__.py | 54 +++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/stacker/config/__init__.py b/stacker/config/__init__.py index ae514c4ca..ca97750cb 100644 --- a/stacker/config/__init__.py +++ b/stacker/config/__init__.py @@ -13,7 +13,12 @@ from schematics import Model from schematics.exceptions import ValidationError -from schematics.exceptions import BaseError as SchematicsError +from schematics.exceptions import ( + BaseError as SchematicsError, + UndefinedValueError, + UnknownFieldError +) + from schematics.types import ( ModelType, ListType, @@ -137,19 +142,9 @@ 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. + return Config(config_dict, strict=True) def load(config): @@ -407,9 +402,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, UnknownFieldError) as e: + raise exceptions.InvalidConfig([e.message]) except SchematicsError as e: raise exceptions.InvalidConfig(e.errors) From 877998b2a015e69b388baee4e54a98d708b6e032 Mon Sep 17 00:00:00 2001 From: Daniel Miranda Date: Sun, 1 Jul 2018 20:00:02 -0300 Subject: [PATCH 2/5] tests: fix typo in stack_policy_path in functional tests --- tests/test_suite/10_stacker_build-simple_build.bats | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_suite/10_stacker_build-simple_build.bats b/tests/test_suite/10_stacker_build-simple_build.bats index 2c92ca84d..c6145a5a6 100644 --- a/tests/test_suite/10_stacker_build-simple_build.bats +++ b/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: tests/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 From 7051bca41aa911eb107218d1dd03443919aa87ad Mon Sep 17 00:00:00 2001 From: Daniel Miranda Date: Sat, 7 Jul 2018 21:13:40 -0300 Subject: [PATCH 3/5] config: ensure Schematics errors are converted during parsing Validation already caught them, but parsing needs to do it too. --- stacker/config/__init__.py | 10 ++++++---- stacker/tests/test_config.py | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/stacker/config/__init__.py b/stacker/config/__init__.py index ca97750cb..d1d10124d 100644 --- a/stacker/config/__init__.py +++ b/stacker/config/__init__.py @@ -15,8 +15,7 @@ from schematics.exceptions import ValidationError from schematics.exceptions import ( BaseError as SchematicsError, - UndefinedValueError, - UnknownFieldError + UndefinedValueError ) from schematics.types import ( @@ -144,7 +143,10 @@ def parse(raw_config): # Top-level excess keys are removed by Config._convert, so enabling strict # mode is fine here. - return Config(config_dict, strict=True) + try: + return Config(config_dict, strict=True) + except SchematicsError as e: + raise exceptions.InvalidConfig(e.errors) def load(config): @@ -430,7 +432,7 @@ def _convert(self, raw_data=None, context=None, **kwargs): def validate(self, *args, **kwargs): try: return super(Config, self).validate(*args, **kwargs) - except (UndefinedValueError, UnknownFieldError) as e: + except UndefinedValueError as e: raise exceptions.InvalidConfig([e.message]) except SchematicsError as e: raise exceptions.InvalidConfig(e.errors) diff --git a/stacker/tests/test_config.py b/stacker/tests/test_config.py index 3c6de86b8..f1e7b25dd 100644 --- a/stacker/tests/test_config.py +++ b/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() From 063593f39aa9099a0fb0a2cf58d80ca1aac7dc07 Mon Sep 17 00:00:00 2001 From: Daniel Miranda Date: Sat, 7 Jul 2018 21:15:14 -0300 Subject: [PATCH 4/5] Update changelog with strict parsing changes --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b1fba2f4..47365e90c 100644 --- a/CHANGELOG.md +++ b/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) From 6e682df69e9b65b6d65c334eb6f6b261369dbf0b Mon Sep 17 00:00:00 2001 From: Daniel Miranda Date: Sat, 7 Jul 2018 21:54:56 -0300 Subject: [PATCH 5/5] tests: fix stack policy path (should be relative to the tests dir) --- tests/test_suite/10_stacker_build-simple_build.bats | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_suite/10_stacker_build-simple_build.bats b/tests/test_suite/10_stacker_build-simple_build.bats index c6145a5a6..57435d7d3 100644 --- a/tests/test_suite/10_stacker_build-simple_build.bats +++ b/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_path: 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