From b85bfce65e26a85150be2073576e3ebe840f3ee1 Mon Sep 17 00:00:00 2001 From: Aanand Prasad Date: Thu, 26 Nov 2015 15:06:30 +0000 Subject: [PATCH 1/5] Fix ports validation test We were essentially only testing that *at least one* of the invalid values fails the validation check, rather than that *all* of them fail. Signed-off-by: Aanand Prasad --- tests/unit/config/config_test.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 2cd26e8f7..7ddec8ab9 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -264,9 +264,8 @@ class ConfigTest(unittest.TestCase): assert services[0]['name'] == valid_name def test_config_invalid_ports_format_validation(self): - expected_error_msg = "Service 'web' configuration key 'ports' contains an invalid type" - with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): - for invalid_ports in [{"1": "8000"}, False, 0, "8000", 8000, ["8000", "8000"]]: + for invalid_ports in [{"1": "8000"}, False, 0, "8000", 8000, ["8000", "8000"]]: + with pytest.raises(ConfigurationError): config.load( build_config_details( {'web': {'image': 'busybox', 'ports': invalid_ports}}, From d52508e2b1b8e59900d492cff69169fe4fed7d1f Mon Sep 17 00:00:00 2001 From: Aanand Prasad Date: Thu, 26 Nov 2015 18:52:14 +0000 Subject: [PATCH 2/5] Refactor ports section of fields schema Signed-off-by: Aanand Prasad --- compose/config/fields_schema.json | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/compose/config/fields_schema.json b/compose/config/fields_schema.json index 9cbcfd1b2..3f1f10fa6 100644 --- a/compose/config/fields_schema.json +++ b/compose/config/fields_schema.json @@ -83,16 +83,8 @@ "ports": { "type": "array", "items": { - "oneOf": [ - { - "type": "string", - "format": "ports" - }, - { - "type": "number", - "format": "ports" - } - ] + "type": ["string", "number"], + "format": "ports" }, "uniqueItems": true }, From 374b16843fedca5908d166226e4d1fc9f455acfc Mon Sep 17 00:00:00 2001 From: Aanand Prasad Date: Thu, 26 Nov 2015 18:54:30 +0000 Subject: [PATCH 3/5] Fix ports validation message - The `raises` kwarg to the `cls_check` decorator was being used incorrectly (it should be an exception class, not an object). - We need to check for `error.cause` and get the message out of the exception object. NB: The particular case where validation fails in the case of `ports` is only when ranges don't match in length - no further validation is currently performed client-side. Signed-off-by: Aanand Prasad --- compose/config/validation.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/compose/config/validation.py b/compose/config/validation.py index 38020366d..24a45e768 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -36,16 +36,12 @@ DOCKER_CONFIG_HINTS = { VALID_NAME_CHARS = '[a-zA-Z0-9\._\-]' -@FormatChecker.cls_checks( - format="ports", - raises=ValidationError( - "Invalid port formatting, it should be " - "'[[remote_ip:]remote_port:]port[/protocol]'")) +@FormatChecker.cls_checks(format="ports", raises=ValidationError) def format_ports(instance): try: split_port(instance) - except ValueError: - return False + except ValueError as e: + raise ValidationError(six.text_type(e)) return True @@ -184,6 +180,10 @@ def handle_generic_service_error(error, service_name): config_key, required_keys) + elif error.cause: + error_msg = six.text_type(error.cause) + msg_format = "Service '{}' configuration key {} is invalid: {}" + elif error.path: msg_format = "Service '{}' configuration key {} value {}" From 042c7048f26713d18da559941bc25f974d60883c Mon Sep 17 00:00:00 2001 From: Aanand Prasad Date: Thu, 26 Nov 2015 19:17:13 +0000 Subject: [PATCH 4/5] Split out ports validation tests into type, uniqueness, format Signed-off-by: Aanand Prasad --- tests/unit/config/config_test.py | 86 ++++++++++++++++++++++++-------- 1 file changed, 64 insertions(+), 22 deletions(-) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 7ddec8ab9..f85c52def 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -263,28 +263,6 @@ class ConfigTest(unittest.TestCase): 'common.yml')) assert services[0]['name'] == valid_name - def test_config_invalid_ports_format_validation(self): - for invalid_ports in [{"1": "8000"}, False, 0, "8000", 8000, ["8000", "8000"]]: - with pytest.raises(ConfigurationError): - config.load( - build_config_details( - {'web': {'image': 'busybox', 'ports': invalid_ports}}, - 'working_dir', - 'filename.yml' - ) - ) - - def test_config_valid_ports_format_validation(self): - valid_ports = [["8000", "9000"], ["8000/8050"], ["8000"], [8000], ["49153-49154:3002-3003"]] - for ports in valid_ports: - config.load( - build_config_details( - {'web': {'image': 'busybox', 'ports': ports}}, - 'working_dir', - 'filename.yml' - ) - ) - def test_config_hint(self): expected_error_msg = "(did you mean 'privileged'?)" with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): @@ -558,6 +536,70 @@ class ConfigTest(unittest.TestCase): assert "which is an invalid type" in exc.exconly() +class PortsTest(unittest.TestCase): + INVALID_PORTS_TYPES = [ + {"1": "8000"}, + False, + "8000", + 8000, + ] + + NON_UNIQUE_SINGLE_PORTS = [ + ["8000", "8000"], + ] + + INVALID_PORT_MAPPINGS = [ + ["8000-8001:8000"], + ] + + VALID_SINGLE_PORTS = [ + ["8000"], + ["8000/tcp"], + ["8000", "9000"], + [8000], + [8000, 9000], + ] + + VALID_PORT_MAPPINGS = [ + ["8000:8050"], + ["49153-49154:3002-3003"], + ] + + def test_config_invalid_ports_type_validation(self): + for invalid_ports in self.INVALID_PORTS_TYPES: + with pytest.raises(ConfigurationError) as exc: + self.check_config({'ports': invalid_ports}) + + assert "contains an invalid type" in exc.value.msg + + def test_config_non_unique_ports_validation(self): + for invalid_ports in self.NON_UNIQUE_SINGLE_PORTS: + with pytest.raises(ConfigurationError) as exc: + self.check_config({'ports': invalid_ports}) + + assert "non-unique" in exc.value.msg + + def test_config_invalid_ports_format_validation(self): + for invalid_ports in self.INVALID_PORT_MAPPINGS: + with pytest.raises(ConfigurationError) as exc: + self.check_config({'ports': invalid_ports}) + + assert "Port ranges don't match in length" in exc.value.msg + + def test_config_valid_ports_format_validation(self): + for valid_ports in self.VALID_SINGLE_PORTS + self.VALID_PORT_MAPPINGS: + self.check_config({'ports': valid_ports}) + + def check_config(self, cfg): + config.load( + build_config_details( + {'web': dict(image='busybox', **cfg)}, + 'working_dir', + 'filename.yml' + ) + ) + + class InterpolationTest(unittest.TestCase): @mock.patch.dict(os.environ) def test_config_file_with_environment_variable(self): From ccf548b98c5eca779b753c14439d83832e1f6b54 Mon Sep 17 00:00:00 2001 From: Aanand Prasad Date: Thu, 26 Nov 2015 19:17:58 +0000 Subject: [PATCH 5/5] Validate the 'expose' option Signed-off-by: Aanand Prasad --- compose/config/fields_schema.json | 5 ++++- compose/config/validation.py | 14 +++++++++++++- tests/unit/config/config_test.py | 27 +++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/compose/config/fields_schema.json b/compose/config/fields_schema.json index 3f1f10fa6..7d5220e3f 100644 --- a/compose/config/fields_schema.json +++ b/compose/config/fields_schema.json @@ -41,7 +41,10 @@ "expose": { "type": "array", - "items": {"type": ["string", "number"]}, + "items": { + "type": ["string", "number"], + "format": "expose" + }, "uniqueItems": true }, diff --git a/compose/config/validation.py b/compose/config/validation.py index 24a45e768..d16bdb9d3 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -1,6 +1,7 @@ import json import logging import os +import re import sys import six @@ -34,6 +35,7 @@ DOCKER_CONFIG_HINTS = { VALID_NAME_CHARS = '[a-zA-Z0-9\._\-]' +VALID_EXPOSE_FORMAT = r'^\d+(\/[a-zA-Z]+)?$' @FormatChecker.cls_checks(format="ports", raises=ValidationError) @@ -45,6 +47,16 @@ def format_ports(instance): return True +@FormatChecker.cls_checks(format="expose", raises=ValidationError) +def format_expose(instance): + if isinstance(instance, six.string_types): + if not re.match(VALID_EXPOSE_FORMAT, instance): + raise ValidationError( + "should be of the format 'PORT[/PROTOCOL]'") + + return True + + @FormatChecker.cls_checks(format="bool-value-in-mapping") def format_boolean_in_environment(instance): """ @@ -273,7 +285,7 @@ def validate_against_fields_schema(config, filename): _validate_against_schema( config, "fields_schema.json", - format_checker=["ports", "bool-value-in-mapping"], + format_checker=["ports", "expose", "bool-value-in-mapping"], filename=filename) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index f85c52def..6c4454323 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -590,6 +590,33 @@ class PortsTest(unittest.TestCase): for valid_ports in self.VALID_SINGLE_PORTS + self.VALID_PORT_MAPPINGS: self.check_config({'ports': valid_ports}) + def test_config_invalid_expose_type_validation(self): + for invalid_expose in self.INVALID_PORTS_TYPES: + with pytest.raises(ConfigurationError) as exc: + self.check_config({'expose': invalid_expose}) + + assert "contains an invalid type" in exc.value.msg + + def test_config_non_unique_expose_validation(self): + for invalid_expose in self.NON_UNIQUE_SINGLE_PORTS: + with pytest.raises(ConfigurationError) as exc: + self.check_config({'expose': invalid_expose}) + + assert "non-unique" in exc.value.msg + + def test_config_invalid_expose_format_validation(self): + # Valid port mappings ARE NOT valid 'expose' entries + for invalid_expose in self.INVALID_PORT_MAPPINGS + self.VALID_PORT_MAPPINGS: + with pytest.raises(ConfigurationError) as exc: + self.check_config({'expose': invalid_expose}) + + assert "should be of the format" in exc.value.msg + + def test_config_valid_expose_format_validation(self): + # Valid single ports ARE valid 'expose' entries + for valid_expose in self.VALID_SINGLE_PORTS: + self.check_config({'expose': valid_expose}) + def check_config(self, cfg): config.load( build_config_details(