From 98ad5a05e4fb342ba4deed92754da51ca98973b3 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 6 Nov 2015 16:38:38 -0500 Subject: [PATCH 1/3] Validate additional files before merging them. Consolidates all the top level config handling into `process_config_file` which is now used for both files and merge sources. Signed-off-by: Daniel Nephin --- compose/cli/main.py | 2 +- compose/config/__init__.py | 1 - compose/config/config.py | 56 +++++++++++++++++--------------- compose/config/validation.py | 10 +----- tests/unit/config/config_test.py | 13 ++++++++ 5 files changed, 45 insertions(+), 37 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index 08c1aee07..806926d84 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -13,12 +13,12 @@ from requests.exceptions import ReadTimeout from .. import __version__ from .. import legacy +from ..config import ConfigurationError from ..config import parse_environment from ..const import DEFAULT_TIMEOUT from ..const import HTTP_TIMEOUT from ..const import IS_WINDOWS_PLATFORM from ..progress_stream import StreamOutputError -from ..project import ConfigurationError from ..project import NoSuchService from ..service import BuildError from ..service import ConvergenceStrategy diff --git a/compose/config/__init__.py b/compose/config/__init__.py index de6f10c94..ec607e087 100644 --- a/compose/config/__init__.py +++ b/compose/config/__init__.py @@ -1,5 +1,4 @@ # flake8: noqa -from .config import ConfigDetails from .config import ConfigurationError from .config import DOCKER_CONFIG_KEYS from .config import find diff --git a/compose/config/config.py b/compose/config/config.py index 7931608d2..feef03877 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -13,7 +13,6 @@ from .errors import ConfigurationError from .interpolation import interpolate_environment_variables from .validation import validate_against_fields_schema from .validation import validate_against_service_schema -from .validation import validate_extended_service_exists from .validation import validate_extends_file_path from .validation import validate_top_level_object @@ -99,6 +98,10 @@ class ConfigFile(namedtuple('_ConfigFile', 'filename config')): :type config: :class:`dict` """ + @classmethod + def from_filename(cls, filename): + return cls(filename, load_yaml(filename)) + def find(base_dir, filenames): if filenames == ['-']: @@ -114,7 +117,7 @@ def find(base_dir, filenames): log.debug("Using configuration files: {}".format(",".join(filenames))) return ConfigDetails( os.path.dirname(filenames[0]), - [ConfigFile(f, load_yaml(f)) for f in filenames]) + [ConfigFile.from_filename(f) for f in filenames]) def get_default_config_files(base_dir): @@ -183,12 +186,10 @@ def load(config_details): validate_paths(service_dict) return service_dict - def load_file(filename, config): - processed_config = interpolate_environment_variables(config) - validate_against_fields_schema(processed_config) + def build_services(filename, config): return [ build_service(filename, name, service_config) - for name, service_config in processed_config.items() + for name, service_config in config.items() ] def merge_services(base, override): @@ -200,16 +201,27 @@ def load(config_details): for name in all_service_names } - config_file = config_details.config_files[0] - validate_top_level_object(config_file.config) + config_file = process_config_file(config_details.config_files[0]) for next_file in config_details.config_files[1:]: - validate_top_level_object(next_file.config) + next_file = process_config_file(next_file) - config_file = ConfigFile( - config_file.filename, - merge_services(config_file.config, next_file.config)) + config = merge_services(config_file.config, next_file.config) + config_file = config_file._replace(config=config) - return load_file(config_file.filename, config_file.config) + return build_services(config_file.filename, config_file.config) + + +def process_config_file(config_file, service_name=None): + validate_top_level_object(config_file.config) + processed_config = interpolate_environment_variables(config_file.config) + validate_against_fields_schema(processed_config) + + if service_name and service_name not in processed_config: + raise ConfigurationError( + "Cannot extend service '{}' in {}: Service not found".format( + service_name, config_file.filename)) + + return config_file._replace(config=processed_config) class ServiceLoader(object): @@ -259,22 +271,13 @@ class ServiceLoader(object): if not isinstance(extends, dict): extends = {'service': extends} - validate_extends_file_path(self.service_name, extends, self.filename) config_path = self.get_extended_config_path(extends) service_name = extends['service'] - config = load_yaml(config_path) - validate_top_level_object(config) - full_extended_config = interpolate_environment_variables(config) - - validate_extended_service_exists( - service_name, - full_extended_config, - config_path - ) - validate_against_fields_schema(full_extended_config) - - service_config = full_extended_config[service_name] + extended_file = process_config_file( + ConfigFile.from_filename(config_path), + service_name=service_name) + service_config = extended_file.config[service_name] return config_path, service_config, service_name def resolve_extends(self, extended_config_path, service_config, service_name): @@ -304,6 +307,7 @@ class ServiceLoader(object): need to obtain a full path too or we are extending from a service defined in our own file. """ + validate_extends_file_path(self.service_name, extends_options, self.filename) if 'file' in extends_options: return expand_path(self.working_dir, extends_options['file']) return self.filename diff --git a/compose/config/validation.py b/compose/config/validation.py index 542081d52..3bd404109 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -96,14 +96,6 @@ def validate_extends_file_path(service_name, extends_options, filename): ) -def validate_extended_service_exists(extended_service_name, full_extended_config, extended_config_path): - if extended_service_name not in full_extended_config: - msg = ( - "Cannot extend service '%s' in %s: Service not found" - ) % (extended_service_name, extended_config_path) - raise ConfigurationError(msg) - - def get_unsupported_config_msg(service_name, error_key): msg = "Unsupported config option for '{}' service: '{}'".format(service_name, error_key) if error_key in DOCKER_CONFIG_HINTS: @@ -264,7 +256,7 @@ def process_errors(errors, service_name=None): msg)) else: root_msgs.append( - "Service '{}' doesn\'t have any configuration options. " + "Service \"{}\" doesn't have any configuration options. " "All top level keys in your docker-compose.yml must map " "to a dictionary of configuration options.'".format(service_name)) elif error.validator == 'required': diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index f27329ba0..ab34f4dcb 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -195,6 +195,19 @@ class ConfigTest(unittest.TestCase): ] self.assertEqual(service_sort(service_dicts), service_sort(expected)) + def test_load_with_multiple_files_and_invalid_override(self): + base_file = config.ConfigFile( + 'base.yaml', + {'web': {'image': 'example/web'}}) + override_file = config.ConfigFile( + 'override.yaml', + {'bogus': 'thing'}) + details = config.ConfigDetails('.', [base_file, override_file]) + + with pytest.raises(ConfigurationError) as exc: + config.load(details) + assert 'Service "bogus" doesn\'t have any configuration' in exc.exconly() + def test_config_valid_service_names(self): for valid_name in ['_', '-', '.__.', '_what-up.', 'what_.up----', 'whatup']: config.load( From a92d86308f7bc3e571f06df4990e609ec370bfc5 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 6 Nov 2015 17:18:47 -0500 Subject: [PATCH 2/3] Rename ServiceLoader to ServiceExtendsResolver ServiceLoader has evolved to be not really all that related to "loading" a service. It's responsibility is more to do with handling the `extends` field, which is only part of loading. The class and its primary method (make_service_dict()) were renamed to better reflect their responsibility. As part of that change process_container_options() was removed from make_service_dict() and renamed to process_service(). It contains logic for handling the non-extends options. This change allows us to remove the hacks from testcase.py and only call the functions we need to format a service dict correctly for integration tests. Signed-off-by: Daniel Nephin --- compose/config/config.py | 27 ++++++++++++--------------- tests/integration/service_test.py | 25 ++++++++++++++++++++++--- tests/integration/testcases.py | 21 +++++++-------------- tests/unit/config/config_test.py | 7 ++++--- 4 files changed, 45 insertions(+), 35 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index feef03877..7846ea7b4 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -177,12 +177,12 @@ def load(config_details): """ def build_service(filename, service_name, service_dict): - loader = ServiceLoader( + resolver = ServiceExtendsResolver( config_details.working_dir, filename, service_name, service_dict) - service_dict = loader.make_service_dict() + service_dict = process_service(config_details.working_dir, resolver.run()) validate_paths(service_dict) return service_dict @@ -224,7 +224,7 @@ def process_config_file(config_file, service_name=None): return config_file._replace(config=processed_config) -class ServiceLoader(object): +class ServiceExtendsResolver(object): def __init__( self, working_dir, @@ -234,7 +234,7 @@ class ServiceLoader(object): already_seen=None ): if working_dir is None: - raise ValueError("No working_dir passed to ServiceLoader()") + raise ValueError("No working_dir passed to ServiceExtendsResolver()") self.working_dir = os.path.abspath(working_dir) @@ -251,7 +251,7 @@ class ServiceLoader(object): if self.signature(name) in self.already_seen: raise CircularReference(self.already_seen + [self.signature(name)]) - def make_service_dict(self): + def run(self): service_dict = dict(self.service_dict) env = resolve_environment(self.working_dir, self.service_dict) if env: @@ -264,7 +264,7 @@ class ServiceLoader(object): if not self.already_seen: validate_against_service_schema(service_dict, self.service_name) - return process_container_options(self.working_dir, service_dict) + return service_dict def validate_and_construct_extends(self): extends = self.service_dict['extends'] @@ -281,19 +281,16 @@ class ServiceLoader(object): return config_path, service_config, service_name def resolve_extends(self, extended_config_path, service_config, service_name): - other_working_dir = os.path.dirname(extended_config_path) - other_already_seen = self.already_seen + [self.signature(self.service_name)] - - other_loader = ServiceLoader( - other_working_dir, + resolver = ServiceExtendsResolver( + os.path.dirname(extended_config_path), extended_config_path, self.service_name, service_config, - already_seen=other_already_seen, + already_seen=self.already_seen + [self.signature(self.service_name)], ) - other_loader.detect_cycle(service_name) - other_service_dict = other_loader.make_service_dict() + resolver.detect_cycle(service_name) + other_service_dict = process_service(resolver.working_dir, resolver.run()) validate_extended_service_dict( other_service_dict, extended_config_path, @@ -358,7 +355,7 @@ def validate_ulimits(ulimit_config): "than 'hard' value".format(ulimit_config)) -def process_container_options(working_dir, service_dict): +def process_service(working_dir, service_dict): service_dict = dict(service_dict) if 'volumes' in service_dict and service_dict.get('volume_driver') is None: diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 804f5219a..d4474dcca 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -815,7 +815,13 @@ class ServiceTest(DockerClientTestCase): environment=['ONE=1', 'TWO=2', 'THREE=3'], env_file=['tests/fixtures/env/one.env', 'tests/fixtures/env/two.env']) env = create_and_start_container(service).environment - for k, v in {'ONE': '1', 'TWO': '2', 'THREE': '3', 'FOO': 'baz', 'DOO': 'dah'}.items(): + for k, v in { + 'ONE': '1', + 'TWO': '2', + 'THREE': '3', + 'FOO': 'baz', + 'DOO': 'dah' + }.items(): self.assertEqual(env[k], v) @mock.patch.dict(os.environ) @@ -823,9 +829,22 @@ class ServiceTest(DockerClientTestCase): os.environ['FILE_DEF'] = 'E1' os.environ['FILE_DEF_EMPTY'] = 'E2' os.environ['ENV_DEF'] = 'E3' - service = self.create_service('web', environment={'FILE_DEF': 'F1', 'FILE_DEF_EMPTY': '', 'ENV_DEF': None, 'NO_DEF': None}) + service = self.create_service( + 'web', + environment={ + 'FILE_DEF': 'F1', + 'FILE_DEF_EMPTY': '', + 'ENV_DEF': None, + 'NO_DEF': None + } + ) env = create_and_start_container(service).environment - for k, v in {'FILE_DEF': 'F1', 'FILE_DEF_EMPTY': '', 'ENV_DEF': 'E3', 'NO_DEF': ''}.items(): + for k, v in { + 'FILE_DEF': 'F1', + 'FILE_DEF_EMPTY': '', + 'ENV_DEF': 'E3', + 'NO_DEF': '' + }.items(): self.assertEqual(env[k], v) def test_with_high_enough_api_version_we_get_default_network_mode(self): diff --git a/tests/integration/testcases.py b/tests/integration/testcases.py index 60e67b5bf..5ee6a4212 100644 --- a/tests/integration/testcases.py +++ b/tests/integration/testcases.py @@ -7,7 +7,8 @@ from pytest import skip from .. import unittest from compose.cli.docker_client import docker_client -from compose.config.config import ServiceLoader +from compose.config.config import process_service +from compose.config.config import resolve_environment from compose.const import LABEL_PROJECT from compose.progress_stream import stream_output from compose.service import Service @@ -42,23 +43,15 @@ class DockerClientTestCase(unittest.TestCase): if 'command' not in kwargs: kwargs['command'] = ["top"] - workaround_options = {} - for option in ['links', 'volumes_from', 'net']: - if option in kwargs: - workaround_options[option] = kwargs.pop(option, None) - - options = ServiceLoader( - working_dir='.', - filename=None, - service_name=name, - service_dict=kwargs - ).make_service_dict() - options.update(workaround_options) + # TODO: remove this once #2299 is fixed + kwargs['name'] = name + options = process_service('.', kwargs) + options['environment'] = resolve_environment('.', kwargs) labels = options.setdefault('labels', {}) labels['com.docker.compose.test-name'] = self.id() - return Service(project='composetest', client=self.client, **options) + return Service(client=self.client, project='composetest', **options) def check_build(self, *args, **kwargs): kwargs.setdefault('rm', True) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index ab34f4dcb..2835a4317 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -18,13 +18,14 @@ from tests import unittest def make_service_dict(name, service_dict, working_dir, filename=None): """ - Test helper function to construct a ServiceLoader + Test helper function to construct a ServiceExtendsResolver """ - return config.ServiceLoader( + resolver = config.ServiceExtendsResolver( working_dir=working_dir, filename=filename, service_name=name, - service_dict=service_dict).make_service_dict() + service_dict=service_dict) + return config.process_service(working_dir, resolver.run()) def service_sort(services): From 5e97b806d51e72f282046231af417b4d647cf64f Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 9 Nov 2015 14:24:44 -0500 Subject: [PATCH 3/3] Fix a bug in ExtendsResolver where the service name of the extended service was wrong. This bug can be seen by the change to the test case. When the extended service uses a different name, the error was reported incorrectly. By fixing this bug we can simplify self.signature and self.detect_cycles to always use self.service_name. Signed-off-by: Daniel Nephin --- compose/config/config.py | 20 +++++++++++--------- tests/fixtures/extends/circle-1.yml | 2 +- tests/fixtures/extends/circle-2.yml | 2 +- tests/unit/config/config_test.py | 23 ++++++++++++----------- 4 files changed, 25 insertions(+), 22 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 7846ea7b4..1ddb2abe0 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -247,11 +247,17 @@ class ServiceExtendsResolver(object): self.service_name = service_name self.service_dict['name'] = service_name - def detect_cycle(self, name): - if self.signature(name) in self.already_seen: - raise CircularReference(self.already_seen + [self.signature(name)]) + @property + def signature(self): + return self.filename, self.service_name + + def detect_cycle(self): + if self.signature in self.already_seen: + raise CircularReference(self.already_seen + [self.signature]) def run(self): + self.detect_cycle() + service_dict = dict(self.service_dict) env = resolve_environment(self.working_dir, self.service_dict) if env: @@ -284,12 +290,11 @@ class ServiceExtendsResolver(object): resolver = ServiceExtendsResolver( os.path.dirname(extended_config_path), extended_config_path, - self.service_name, + service_name, service_config, - already_seen=self.already_seen + [self.signature(self.service_name)], + already_seen=self.already_seen + [self.signature], ) - resolver.detect_cycle(service_name) other_service_dict = process_service(resolver.working_dir, resolver.run()) validate_extended_service_dict( other_service_dict, @@ -309,9 +314,6 @@ class ServiceExtendsResolver(object): return expand_path(self.working_dir, extends_options['file']) return self.filename - def signature(self, name): - return self.filename, name - def resolve_environment(working_dir, service_dict): """Unpack any environment variables from an env_file, if set. diff --git a/tests/fixtures/extends/circle-1.yml b/tests/fixtures/extends/circle-1.yml index a034e9619..d88ea61d0 100644 --- a/tests/fixtures/extends/circle-1.yml +++ b/tests/fixtures/extends/circle-1.yml @@ -5,7 +5,7 @@ bar: web: extends: file: circle-2.yml - service: web + service: other baz: image: busybox quux: diff --git a/tests/fixtures/extends/circle-2.yml b/tests/fixtures/extends/circle-2.yml index fa6ddefcc..de05bc8da 100644 --- a/tests/fixtures/extends/circle-2.yml +++ b/tests/fixtures/extends/circle-2.yml @@ -2,7 +2,7 @@ foo: image: busybox bar: image: busybox -web: +other: extends: file: circle-1.yml service: web diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 2835a4317..717831681 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -1080,18 +1080,19 @@ class ExtendsTest(unittest.TestCase): ])) def test_circular(self): - try: + with pytest.raises(config.CircularReference) as exc: load_from_filename('tests/fixtures/extends/circle-1.yml') - raise Exception("Expected config.CircularReference to be raised") - except config.CircularReference as e: - self.assertEqual( - [(os.path.basename(filename), service_name) for (filename, service_name) in e.trail], - [ - ('circle-1.yml', 'web'), - ('circle-2.yml', 'web'), - ('circle-1.yml', 'web'), - ], - ) + + path = [ + (os.path.basename(filename), service_name) + for (filename, service_name) in exc.value.trail + ] + expected = [ + ('circle-1.yml', 'web'), + ('circle-2.yml', 'other'), + ('circle-1.yml', 'web'), + ] + self.assertEqual(path, expected) def test_extends_validation_empty_dictionary(self): with self.assertRaisesRegexp(ConfigurationError, 'service'):