Merge pull request #2464 from aanand/validate-expose

Validate `expose`

compose/config/fields_schema.json

@@ -41,7 +41,10 @@
 
         "expose": {
           "type": "array",
-          "items": {"type": ["string", "number"]},
+          "items": {
+            "type": ["string", "number"],
+            "format": "expose"
+          },
           "uniqueItems": true
         },
 
@@ -83,16 +86,8 @@
         "ports": {
           "type": "array",
           "items": {
-            "oneOf": [
-              {
-                "type": "string",
-                "format": "ports"
-              },
-              {
-                "type": "number",
-                "format": "ports"
-              }
-            ]
+            "type": ["string", "number"],
+            "format": "ports"
           },
           "uniqueItems": true
         },

compose/config/validation.py

@@ -1,6 +1,7 @@
 import json
 import logging
 import os
+import re
 import sys
 
 import six
@@ -34,18 +35,25 @@ DOCKER_CONFIG_HINTS = {
 
 
 VALID_NAME_CHARS = '[a-zA-Z0-9\._\-]'
+VALID_EXPOSE_FORMAT = r'^\d+(\/[a-zA-Z]+)?$'
 
 
[email protected]_checks(
-    format="ports",
-    raises=ValidationError(
-        "Invalid port formatting, it should be "
-        "'[[remote_ip:]remote_port:]port[/protocol]'"))
[email protected]_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
+
+
[email protected]_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
 
 
@@ -184,6 +192,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 {}"
 
@@ -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)
 
 

tests/unit/config/config_test.py

@@ -263,29 +263,6 @@ class ConfigTest(unittest.TestCase):
                     'common.yml'))
             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"]]:
-                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):
@@ -559,6 +536,97 @@ 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 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(
+                {'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):