From da36ee7cbcaf2051fc0829f273c01517bd7d9bc2 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 27 Jul 2015 15:15:07 +0100 Subject: [PATCH] Perform schema validation Define a schema that we can pass to jsonschema to validate against the config a user has supplied. This will help catch a wide variety of common errors that occur. If the config does not pass schema validation then it raises an exception and prints out human readable reasons. Signed-off-by: Mazz Mosley --- compose/config/config.py | 43 ++++--- compose/schema.json | 79 ++++++++++++ compose/service.py | 6 - requirements.txt | 1 + setup.py | 1 + .../fixtures/extends/specify-file-as-self.yml | 1 + tests/unit/config_test.py | 118 +++++++++++------- tests/unit/service_test.py | 1 - 8 files changed, 175 insertions(+), 75 deletions(-) create mode 100644 compose/schema.json diff --git a/compose/config/config.py b/compose/config/config.py index 4d3f5faef..1e793d9f6 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -3,6 +3,8 @@ import os import sys import yaml from collections import namedtuple +import json +import jsonschema import six @@ -131,13 +133,31 @@ def get_config_path(base_dir): return os.path.join(path, winner) +def validate_against_schema(config): + config_source_dir = os.path.dirname(os.path.abspath(__file__)) + schema_file = os.path.join(config_source_dir, "schema.json") + + with open(schema_file, "r") as schema_fh: + schema = json.load(schema_fh) + + validation_output = jsonschema.Draft4Validator(schema) + + errors = [error.message for error in sorted(validation_output.iter_errors(config), key=str)] + if errors: + raise ConfigurationError("Validation failed, reason(s): {}".format("\n".join(errors))) + + def load(config_details): - dictionary, working_dir, filename = config_details - dictionary = interpolate_environment_variables(dictionary) + config, working_dir, filename = config_details + config = interpolate_environment_variables(config) service_dicts = [] - for service_name, service_dict in list(dictionary.items()): + validate_against_schema(config) + + for service_name, service_dict in list(config.items()): + if not isinstance(service_dict, dict): + raise ConfigurationError('Service "%s" doesn\'t have any configuration options. All top level keys in your docker-compose.yml must map to a dictionary of configuration options.' % service_name) loader = ServiceLoader(working_dir=working_dir, filename=filename) service_dict = loader.make_service_dict(service_name, service_dict) validate_paths(service_dict) @@ -210,25 +230,11 @@ class ServiceLoader(object): def validate_extends_options(self, service_name, extends_options): error_prefix = "Invalid 'extends' configuration for %s:" % service_name - if not isinstance(extends_options, dict): - raise ConfigurationError("%s must be a dictionary" % error_prefix) - - if 'service' not in extends_options: - raise ConfigurationError( - "%s you need to specify a service, e.g. 'service: web'" % error_prefix - ) - if 'file' not in extends_options and self.filename is None: raise ConfigurationError( "%s you need to specify a 'file', e.g. 'file: something.yml'" % error_prefix ) - for k, _ in extends_options.items(): - if k not in ['file', 'service']: - raise ConfigurationError( - "%s unsupported configuration option '%s'" % (error_prefix, k) - ) - return extends_options @@ -256,9 +262,6 @@ def process_container_options(service_dict, working_dir=None): service_dict = service_dict.copy() - if 'memswap_limit' in service_dict and 'mem_limit' not in service_dict: - raise ConfigurationError("Invalid 'memswap_limit' configuration for %s service: when defining 'memswap_limit' you must set 'mem_limit' as well" % service_dict['name']) - if 'volumes' in service_dict and service_dict.get('volume_driver') is None: service_dict['volumes'] = resolve_volume_paths(service_dict['volumes'], working_dir=working_dir) diff --git a/compose/schema.json b/compose/schema.json new file mode 100644 index 000000000..7c7e2d096 --- /dev/null +++ b/compose/schema.json @@ -0,0 +1,79 @@ +{ + "$schema": "http://json-schema.org/draft-04/schema#", + + "type": "object", + + "patternProperties": { + "^[a-zA-Z0-9._-]+$": { + "$ref": "#/definitions/service" + } + }, + + "definitions": { + "service": { + "type": "object", + + "properties": { + "build": {"type": "string"}, + "env_file": {"$ref": "#/definitions/string_or_list"}, + "environment": { + "oneOf": [ + {"type": "object"}, + {"type": "array", "items": {"type": "string"}, "uniqueItems": true} + ] + }, + "image": {"type": "string"}, + "mem_limit": {"type": "number"}, + "memswap_limit": {"type": "number"}, + + "extends": { + "type": "object", + + "properties": { + "service": {"type": "string"}, + "file": {"type": "string"} + }, + "required": ["service"], + "additionalProperties": false + } + + }, + + "anyOf": [ + { + "required": ["build"], + "not": {"required": ["image"]} + }, + { + "required": ["image"], + "not": {"required": ["build"]} + }, + { + "required": ["extends"], + "not": {"required": ["build", "image"]} + } + ], + + "dependencies": { + "memswap_limit": ["mem_limit"] + } + + }, + + "string_or_list": { + "oneOf": [ + {"type": "string"}, + {"$ref": "#/definitions/list_of_strings"} + ] + }, + + "list_of_strings": { + "type": "array", + "items": {"type": "string"}, + "uniqueItems": true + } + + }, + + "additionalProperties": false +} diff --git a/compose/service.py b/compose/service.py index 2e0490a50..c72365cf9 100644 --- a/compose/service.py +++ b/compose/service.py @@ -82,14 +82,8 @@ ConvergencePlan = namedtuple('ConvergencePlan', 'action containers') class Service(object): def __init__(self, name, client=None, project='default', links=None, external_links=None, volumes_from=None, net=None, **options): - if not re.match('^%s+$' % VALID_NAME_CHARS, name): - raise ConfigError('Invalid service name "%s" - only %s are allowed' % (name, VALID_NAME_CHARS)) if not re.match('^%s+$' % VALID_NAME_CHARS, project): raise ConfigError('Invalid project name "%s" - only %s are allowed' % (project, VALID_NAME_CHARS)) - if 'image' in options and 'build' in options: - raise ConfigError('Service %s has both an image and build path specified. A service can either be built to image or use an existing image, not both.' % name) - if 'image' not in options and 'build' not in options: - raise ConfigError('Service %s has neither an image nor a build path specified. Exactly one must be provided.' % name) self.name = name self.client = client diff --git a/requirements.txt b/requirements.txt index f9cec8372..641687686 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,5 @@ PyYAML==3.10 +jsonschema==2.5.1 docker-py==1.3.1 dockerpty==0.3.4 docopt==0.6.1 diff --git a/setup.py b/setup.py index 9bca4752d..1f9c981d1 100644 --- a/setup.py +++ b/setup.py @@ -33,6 +33,7 @@ install_requires = [ 'docker-py >= 1.3.1, < 1.4', 'dockerpty >= 0.3.4, < 0.4', 'six >= 1.3.0, < 2', + 'jsonschema >= 2.5.1, < 3', ] diff --git a/tests/fixtures/extends/specify-file-as-self.yml b/tests/fixtures/extends/specify-file-as-self.yml index 7e2499762..c24f10bc9 100644 --- a/tests/fixtures/extends/specify-file-as-self.yml +++ b/tests/fixtures/extends/specify-file-as-self.yml @@ -12,5 +12,6 @@ web: environment: - "BAZ=3" otherweb: + image: busybox environment: - "YEP=1" diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index 004620203..3ed394a92 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -20,10 +20,10 @@ class ConfigTest(unittest.TestCase): config.ConfigDetails( { 'foo': {'image': 'busybox'}, - 'bar': {'environment': ['FOO=1']}, + 'bar': {'image': 'busybox', 'environment': ['FOO=1']}, }, - 'working_dir', - 'filename.yml' + 'tests/fixtures/extends', + 'common.yml' ) ) @@ -32,13 +32,14 @@ class ConfigTest(unittest.TestCase): sorted([ { 'name': 'bar', + 'image': 'busybox', 'environment': {'FOO': '1'}, }, { 'name': 'foo', 'image': 'busybox', } - ]) + ], key=lambda d: d['name']) ) def test_load_throws_error_when_not_dict(self): @@ -327,23 +328,26 @@ class MemoryOptionsTest(unittest.TestCase): When you set a 'memswap_limit' it is invalid config unless you also set a mem_limit """ - with self.assertRaises(config.ConfigurationError): - make_service_dict( - 'foo', { - 'memswap_limit': 2000000, - }, - 'tests/' + with self.assertRaisesRegexp(config.ConfigurationError, "u'mem_limit' is a dependency of u'memswap_limit'"): + config.load( + config.ConfigDetails( + { + 'foo': {'image': 'busybox', 'memswap_limit': 2000000}, + }, + 'tests/fixtures/extends', + 'filename.yml' + ) ) def test_validation_with_correct_memswap_values(self): - service_dict = make_service_dict( - 'foo', { - 'mem_limit': 1000000, - 'memswap_limit': 2000000, - }, - 'tests/' + service_dict = config.load( + config.ConfigDetails( + {'foo': {'image': 'busybox', 'mem_limit': 1000000, 'memswap_limit': 2000000}}, + 'tests/fixtures/extends', + 'common.yml' + ) ) - self.assertEqual(service_dict['memswap_limit'], 2000000) + self.assertEqual(service_dict[0]['memswap_limit'], 2000000) class EnvTest(unittest.TestCase): @@ -528,6 +532,7 @@ class ExtendsTest(unittest.TestCase): { 'environment': {'YEP': '1'}, + 'image': 'busybox', 'name': 'otherweb' }, { @@ -553,36 +558,47 @@ class ExtendsTest(unittest.TestCase): ) def test_extends_validation_empty_dictionary(self): - dictionary = {'extends': None} - - def load_config(): - return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends') - - self.assertRaisesRegexp(config.ConfigurationError, 'dictionary', load_config) - - dictionary['extends'] = {} - self.assertRaises(config.ConfigurationError, load_config) + with self.assertRaisesRegexp(config.ConfigurationError, 'service'): + config.load( + config.ConfigDetails( + { + 'web': {'image': 'busybox', 'extends': {}}, + }, + 'tests/fixtures/extends', + 'filename.yml' + ) + ) def test_extends_validation_missing_service_key(self): - dictionary = {'extends': {'file': 'common.yml'}} - - def load_config(): - return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends') - - self.assertRaisesRegexp(config.ConfigurationError, 'service', load_config) + with self.assertRaisesRegexp(config.ConfigurationError, "u'service' is a required property"): + config.load( + config.ConfigDetails( + { + 'web': {'image': 'busybox', 'extends': {'file': 'common.yml'}}, + }, + 'tests/fixtures/extends', + 'filename.yml' + ) + ) def test_extends_validation_invalid_key(self): - dictionary = { - 'extends': - { - 'service': 'web', 'file': 'common.yml', 'what': 'is this' - } - } - - def load_config(): - return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends') - - self.assertRaisesRegexp(config.ConfigurationError, 'what', load_config) + with self.assertRaisesRegexp(config.ConfigurationError, "'rogue_key' was unexpected"): + config.load( + config.ConfigDetails( + { + 'web': { + 'image': 'busybox', + 'extends': { + 'file': 'common.yml', + 'service': 'web', + 'rogue_key': 'is not allowed' + } + }, + }, + 'tests/fixtures/extends', + 'filename.yml' + ) + ) def test_extends_validation_no_file_key_no_filename_set(self): dictionary = {'extends': {'service': 'web'}} @@ -593,12 +609,18 @@ class ExtendsTest(unittest.TestCase): self.assertRaisesRegexp(config.ConfigurationError, 'file', load_config) def test_extends_validation_valid_config(self): - dictionary = {'extends': {'service': 'web', 'file': 'common.yml'}} + service = config.load( + config.ConfigDetails( + { + 'web': {'image': 'busybox', 'extends': {'service': 'web', 'file': 'common.yml'}}, + }, + 'tests/fixtures/extends', + 'common.yml' + ) + ) - def load_config(): - return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends') - - self.assertIsInstance(load_config(), dict) + self.assertEquals(len(service), 1) + self.assertIsInstance(service[0], dict) def test_extends_file_defaults_to_self(self): """ diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index bc6b9e485..aa348466a 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -49,7 +49,6 @@ class ServiceTest(unittest.TestCase): Service('.__.', image='foo') def test_project_validation(self): - self.assertRaises(ConfigError, lambda: Service('bar')) self.assertRaises(ConfigError, lambda: Service(name='foo', project='>', image='foo')) Service(name='foo', project='bar.bar__', image='foo')