From ddbcf1b07d9c71555fc013a8543a954128d95dbe Mon Sep 17 00:00:00 2001 From: Ian Wienand Date: Fri, 4 Oct 2019 10:38:46 +1000 Subject: [PATCH] Validate openstack provider pool labels have top-level labels We broke nodepool configuration with I3795fee1530045363e3f629f0793cbe6e95c23ca by not having the labels defined in the OpenStack provider in the top-level label list. The added check here would have found such a case. The validate() function is reworked slightly; previously it would return various exceptions from the tools it was calling (YAML, voluptuous, etc.). Now we have more testing (and I'd imagine we could do even more, similar vaildations too) we'd have to keep adding exception types. Just make the function return a value; this also makes sure the regular exit paths are taken from the caller in nodepoolcmd.py, rather than dying with an exception at whatever point. A unit test is added. Co-Authored-By: Mohammed Naser Change-Id: I5455f5d7eb07abea34c11a3026d630dee62f2185 --- nodepool/cmd/config_validator.py | 50 ++++++++++++++++--- nodepool/cmd/nodepoolcmd.py | 3 +- nodepool/tests/__init__.py | 4 +- .../config_validate/missing_top_label.yaml | 14 ++++++ nodepool/tests/unit/test_config_validator.py | 20 ++++++-- nodepool/tests/unit/test_driver_static.py | 4 +- nodepool/tests/unit/test_sdk_integration.py | 3 +- 7 files changed, 79 insertions(+), 19 deletions(-) create mode 100644 nodepool/tests/fixtures/config_validate/missing_top_label.yaml diff --git a/nodepool/cmd/config_validator.py b/nodepool/cmd/config_validator.py index 8a52a5a82..a2c828f85 100644 --- a/nodepool/cmd/config_validator.py +++ b/nodepool/cmd/config_validator.py @@ -27,6 +27,11 @@ class ConfigValidator: self.config_file = config_file def validate(self): + ''' + Validate a configuration file + + :return: 0 for success, non-zero for failure + ''' provider = ProviderConfig.getCommonSchemaDict() label = { @@ -72,11 +77,42 @@ class ConfigValidator: } log.info("validating %s" % self.config_file) - config = yaml.safe_load(open(self.config_file)) - # validate the overall schema - schema = v.Schema(top_level) - schema(config) - for provider_dict in config.get('providers', []): - provider_schema = get_provider_config(provider_dict).getSchema() - provider_schema.extend(provider)(provider_dict) + try: + config = yaml.safe_load(open(self.config_file)) + except Exception: + log.exception('YAML parsing failed') + return 1 + + try: + # validate the overall schema + schema = v.Schema(top_level) + schema(config) + for provider_dict in config.get('providers', []): + provider_schema = \ + get_provider_config(provider_dict).getSchema() + provider_schema.extend(provider)(provider_dict) + except Exception: + log.exception('Schema validation failed') + return 1 + + errors = False + + # Ensure in openstack provider sections, diskimages have + # top-level labels + labels = [x['name'] for x in config.get('labels', [])] + for provider in config.get('providers', []): + if provider.get('driver', 'openstack') != 'openstack': + continue + for pool in provider.get('pools', []): + for label in pool.get('labels', []): + if label['name'] not in labels: + errors = True + log.error("diskimage %s in provider %s " + "not in top-level labels" % + (label['name'], provider['name'])) + + if errors is True: + return 1 + else: + return 0 diff --git a/nodepool/cmd/nodepoolcmd.py b/nodepool/cmd/nodepoolcmd.py index fab987b53..fdfeab455 100755 --- a/nodepool/cmd/nodepoolcmd.py +++ b/nodepool/cmd/nodepoolcmd.py @@ -340,8 +340,7 @@ class NodePoolCmd(NodepoolApp): def config_validate(self): validator = ConfigValidator(self.args.config) - validator.validate() - log.info("Configuration validation complete") + return validator.validate() # TODO(asselin,yolanda): add validation of secure.conf def request_list(self): diff --git a/nodepool/tests/__init__.py b/nodepool/tests/__init__.py index a452af935..11536f820 100644 --- a/nodepool/tests/__init__.py +++ b/nodepool/tests/__init__.py @@ -366,7 +366,9 @@ class DBTestCase(BaseTestCase): self._config_images_dir = images_dir self._config_build_log_dir = build_log_dir validator = ConfigValidator(path) - validator.validate() + ret = validator.validate() + if ret != 0: + raise ValueError("Config file %s could not be validated" % path) return path def replace_config(self, configfile, filename): diff --git a/nodepool/tests/fixtures/config_validate/missing_top_label.yaml b/nodepool/tests/fixtures/config_validate/missing_top_label.yaml new file mode 100644 index 000000000..ef02f8e62 --- /dev/null +++ b/nodepool/tests/fixtures/config_validate/missing_top_label.yaml @@ -0,0 +1,14 @@ +--- +# NOTE(mnaser): The trusty label missing here will fail during runtime. +labels: [] + +providers: + - name: static + driver: openstack + cloud: potato + pools: + - name: main + labels: + - name: trusty + diskimage: trusty + min-ram: 8192 \ No newline at end of file diff --git a/nodepool/tests/unit/test_config_validator.py b/nodepool/tests/unit/test_config_validator.py index ba1789df5..289369ae4 100644 --- a/nodepool/tests/unit/test_config_validator.py +++ b/nodepool/tests/unit/test_config_validator.py @@ -16,8 +16,6 @@ import os from nodepool.cmd.config_validator import ConfigValidator from nodepool import tests -from yaml.parser import ParserError -from voluptuous import MultipleInvalid class TestConfigValidation(tests.BaseTestCase): @@ -30,14 +28,25 @@ class TestConfigValidation(tests.BaseTestCase): 'fixtures', 'config_validate', 'good.yaml') validator = ConfigValidator(config) - validator.validate() + ret = validator.validate() + self.assertEqual(ret, 0) def test_yaml_error(self): config = os.path.join(os.path.dirname(tests.__file__), 'fixtures', 'config_validate', 'yaml_error.yaml') validator = ConfigValidator(config) - self.assertRaises(ParserError, validator.validate) + ret = validator.validate() + self.assertEqual(ret, 1) + + def test_missing_top_level_label(self): + config = os.path.join(os.path.dirname(tests.__file__), + 'fixtures', 'config_validate', + 'missing_top_label.yaml') + + validator = ConfigValidator(config) + ret = validator.validate() + self.assertEqual(ret, 1) def test_schema(self): config = os.path.join(os.path.dirname(tests.__file__), @@ -45,4 +54,5 @@ class TestConfigValidation(tests.BaseTestCase): 'schema_error.yaml') validator = ConfigValidator(config) - self.assertRaises(MultipleInvalid, validator.validate) + ret = validator.validate() + self.assertEqual(ret, 1) diff --git a/nodepool/tests/unit/test_driver_static.py b/nodepool/tests/unit/test_driver_static.py index 219a89758..3e5c8d653 100644 --- a/nodepool/tests/unit/test_driver_static.py +++ b/nodepool/tests/unit/test_driver_static.py @@ -21,7 +21,6 @@ from nodepool import config as nodepool_config from nodepool import tests from nodepool import zk from nodepool.cmd.config_validator import ConfigValidator -from voluptuous import MultipleInvalid class TestDriverStatic(tests.DBTestCase): @@ -32,7 +31,8 @@ class TestDriverStatic(tests.DBTestCase): 'fixtures', 'config_validate', 'static_error.yaml') validator = ConfigValidator(config) - self.assertRaises(MultipleInvalid, validator.validate) + ret = validator.validate() + self.assertEqual(ret, 1) def test_static_config(self): configfile = self.setup_config('static.yaml') diff --git a/nodepool/tests/unit/test_sdk_integration.py b/nodepool/tests/unit/test_sdk_integration.py index 3382f9793..5bb64a869 100644 --- a/nodepool/tests/unit/test_sdk_integration.py +++ b/nodepool/tests/unit/test_sdk_integration.py @@ -15,7 +15,6 @@ import os import fixtures -import voluptuous import yaml from nodepool import config as nodepool_config @@ -45,7 +44,7 @@ class TestShadeIntegration(tests.IntegrationTestCase): # Assert that we get a nodepool error and not an openstacksdk # error. self.assertRaises( - voluptuous.MultipleInvalid, + Exception, self.setup_config, 'integration_noocc.yaml') def test_nodepool_occ_config(self):