Fixes #2008 - re-use list_or_dict schema for all the types

At the same time, moves extra_hosts validation to the config module.

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Author: dnephin

compose/config/config.py

@@ -14,6 +14,7 @@
 from .errors import ComposeFileNotFound
 from .errors import ConfigurationError
 from .interpolation import interpolate_environment_variables
+from .types import parse_extra_hosts
 from .types import parse_restart_spec
 from .types import VolumeFromSpec
 from .validation import validate_against_fields_schema
@@ -379,6 +380,9 @@ def process_service(service_config):
     if 'labels' in service_dict:
         service_dict['labels'] = parse_labels(service_dict['labels'])
 
+    if 'extra_hosts' in service_dict:
+        service_dict['extra_hosts'] = parse_extra_hosts(service_dict['extra_hosts'])
+
     # TODO: move to a validate_service()
     if 'ulimits' in service_dict:
         validate_ulimits(service_dict['ulimits'])

compose/config/fields_schema.json

@@ -37,22 +37,7 @@
         "domainname": {"type": "string"},
         "entrypoint": {"$ref": "#/definitions/string_or_list"},
         "env_file": {"$ref": "#/definitions/string_or_list"},
-
-        "environment": {
-          "oneOf": [
-            {
-              "type": "object",
-              "patternProperties": {
-                ".+": {
-                  "type": ["string", "number", "boolean", "null"],
-                  "format": "environment"
-                }
-              },
-              "additionalProperties": false
-            },
-            {"type": "array", "items": {"type": "string"}, "uniqueItems": true}
-          ]
-        },
+        "environment": {"$ref": "#/definitions/list_or_dict"},
 
         "expose": {
           "type": "array",
@@ -165,10 +150,18 @@
 
     "list_or_dict": {
       "oneOf": [
-        {"type": "array", "items": {"type": "string"}, "uniqueItems": true},
-        {"type": "object"}
+        {
+          "type": "object",
+          "patternProperties": {
+            ".+": {
+              "type": ["string", "number", "boolean", "null"],
+              "format": "bool-value-in-mapping"
+            }
+          },
+          "additionalProperties": false
+        },
+        {"type": "array", "items": {"type": "string"}, "uniqueItems": true}
       ]
     }
-
   }
 }

compose/config/types.py

@@ -43,3 +43,19 @@ def parse_restart_spec(restart_config):
         max_retry_count = 0
 
     return {'Name': name, 'MaximumRetryCount': int(max_retry_count)}
+
+
+def parse_extra_hosts(extra_hosts_config):
+    if not extra_hosts_config:
+        return {}
+
+    if isinstance(extra_hosts_config, dict):
+        return dict(extra_hosts_config)
+
+    if isinstance(extra_hosts_config, list):
+        extra_hosts_dict = {}
+        for extra_hosts_line in extra_hosts_config:
+            # TODO: validate string contains ':' ?
+            host, ip = extra_hosts_line.split(':')
+            extra_hosts_dict[host.strip()] = ip.strip()
+        return extra_hosts_dict

compose/config/validation.py

@@ -49,7 +49,7 @@ def format_ports(instance):
     return True
 
 
-@FormatChecker.cls_checks(format="environment")
+@FormatChecker.cls_checks(format="bool-value-in-mapping")
 def format_boolean_in_environment(instance):
     """
     Check if there is a boolean in the environment and display a warning.
@@ -273,7 +273,7 @@ def validate_against_fields_schema(config, filename):
     _validate_against_schema(
         config,
         "fields_schema.json",
-        format_checker=["ports", "environment"],
+        format_checker=["ports", "bool-value-in-mapping"],
         filename=filename)
 
 

compose/service.py

@@ -640,6 +640,7 @@ def _get_container_host_config(self, override_options, one_off=False):
         pid = options.get('pid', None)
         security_opt = options.get('security_opt', None)
 
+        # TODO: these options are already normalized by config
         dns = options.get('dns', None)
         if isinstance(dns, six.string_types):
             dns = [dns]
@@ -648,9 +649,6 @@ def _get_container_host_config(self, override_options, one_off=False):
         if isinstance(dns_search, six.string_types):
             dns_search = [dns_search]
 
-        extra_hosts = build_extra_hosts(options.get('extra_hosts', None))
-        read_only = options.get('read_only', None)
-
         devices = options.get('devices', None)
         cgroup_parent = options.get('cgroup_parent', None)
         ulimits = build_ulimits(options.get('ulimits', None))
@@ -672,8 +670,8 @@ def _get_container_host_config(self, override_options, one_off=False):
             memswap_limit=options.get('memswap_limit'),
             ulimits=ulimits,
             log_config=log_config,
-            extra_hosts=extra_hosts,
-            read_only=read_only,
+            extra_hosts=options.get('extra_hosts'),
+            read_only=options.get('read_only'),
             pid_mode=pid,
             security_opt=security_opt,
             ipc_mode=options.get('ipc'),
@@ -1057,31 +1055,3 @@ def build_ulimits(ulimit_config):
             ulimits.append(ulimit_dict)
 
     return ulimits
-
-
-# Extra hosts
-
-
-def build_extra_hosts(extra_hosts_config):
-    if not extra_hosts_config:
-        return {}
-
-    if isinstance(extra_hosts_config, list):
-        extra_hosts_dict = {}
-        for extra_hosts_line in extra_hosts_config:
-            if not isinstance(extra_hosts_line, six.string_types):
-                raise ConfigError(
-                    "extra_hosts_config \"%s\" must be either a list of strings or a string->string mapping," %
-                    extra_hosts_config
-                )
-            host, ip = extra_hosts_line.split(':')
-            extra_hosts_dict.update({host.strip(): ip.strip()})
-        extra_hosts_config = extra_hosts_dict
-
-    if isinstance(extra_hosts_config, dict):
-        return extra_hosts_config
-
-    raise ConfigError(
-        "extra_hosts_config \"%s\" must be either a list of strings or a string->string mapping," %
-        extra_hosts_config
-    )

tests/integration/service_test.py

@@ -22,8 +22,6 @@
 from compose.const import LABEL_SERVICE
 from compose.const import LABEL_VERSION
 from compose.container import Container
-from compose.service import build_extra_hosts
-from compose.service import ConfigError
 from compose.service import ConvergencePlan
 from compose.service import ConvergenceStrategy
 from compose.service import Net
@@ -139,37 +137,6 @@ def test_create_container_with_cpu_shares(self):
         container.start()
         self.assertEqual(container.get('HostConfig.CpuShares'), 73)
 
-    def test_build_extra_hosts(self):
-        # string
-        self.assertRaises(ConfigError, lambda: build_extra_hosts("www.example.com: 192.168.0.17"))
-
-        # list of strings
-        self.assertEqual(build_extra_hosts(
-            ["www.example.com:192.168.0.17"]),
-            {'www.example.com': '192.168.0.17'})
-        self.assertEqual(build_extra_hosts(
-            ["www.example.com: 192.168.0.17"]),
-            {'www.example.com': '192.168.0.17'})
-        self.assertEqual(build_extra_hosts(
-            ["www.example.com: 192.168.0.17",
-             "static.example.com:192.168.0.19",
-             "api.example.com: 192.168.0.18"]),
-            {'www.example.com': '192.168.0.17',
-             'static.example.com': '192.168.0.19',
-             'api.example.com': '192.168.0.18'})
-
-        # list of dictionaries
-        self.assertRaises(ConfigError, lambda: build_extra_hosts(
-            [{'www.example.com': '192.168.0.17'},
-             {'api.example.com': '192.168.0.18'}]))
-
-        # dictionaries
-        self.assertEqual(build_extra_hosts(
-            {'www.example.com': '192.168.0.17',
-             'api.example.com': '192.168.0.18'}),
-            {'www.example.com': '192.168.0.17',
-             'api.example.com': '192.168.0.18'})
-
     def test_create_container_with_extra_hosts_list(self):
         extra_hosts = ['somehost:162.242.195.82', 'otherhost:50.31.209.229']
         service = self.create_service('db', extra_hosts=extra_hosts)

tests/unit/config/config_test.py

@@ -32,7 +32,7 @@ def service_sort(services):
     return sorted(services, key=itemgetter('name'))
 
 
-def build_config_details(contents, working_dir, filename):
+def build_config_details(contents, working_dir='working_dir', filename='filename.yml'):
     return config.ConfigDetails(
         working_dir,
         [config.ConfigFile(filename, contents)])
@@ -512,6 +512,29 @@ def test_load_yaml_with_yaml_error(self):
 
         assert 'line 3, column 32' in exc.exconly()
 
+    def test_validate_extra_hosts_invalid(self):
+        with pytest.raises(ConfigurationError) as exc:
+            config.load(build_config_details({
+                'web': {
+                    'image': 'alpine',
+                    'extra_hosts': "www.example.com: 192.168.0.17",
+                }
+            }))
+        assert "'extra_hosts' contains an invalid type" in exc.exconly()
+
+    def test_validate_extra_hosts_invalid_list(self):
+        with pytest.raises(ConfigurationError) as exc:
+            config.load(build_config_details({
+                'web': {
+                    'image': 'alpine',
+                    'extra_hosts': [
+                        {'www.example.com': '192.168.0.17'},
+                        {'api.example.com': '192.168.0.18'}
+                    ],
+                }
+            }))
+        assert "which is an invalid type" in exc.exconly()
+
 
 class InterpolationTest(unittest.TestCase):
     @mock.patch.dict(os.environ)

tests/unit/config/types_test.py

@@ -0,0 +1,29 @@
+from compose.config.types import parse_extra_hosts
+
+
+def test_parse_extra_hosts_list():
+    expected = {'www.example.com': '192.168.0.17'}
+    assert parse_extra_hosts(["www.example.com:192.168.0.17"]) == expected
+
+    expected = {'www.example.com': '192.168.0.17'}
+    assert parse_extra_hosts(["www.example.com: 192.168.0.17"]) == expected
+
+    assert parse_extra_hosts([
+        "www.example.com: 192.168.0.17",
+        "static.example.com:192.168.0.19",
+        "api.example.com: 192.168.0.18"
+    ]) == {
+        'www.example.com': '192.168.0.17',
+        'static.example.com': '192.168.0.19',
+        'api.example.com': '192.168.0.18'
+    }
+
+
+def test_parse_extra_hosts_dict():
+    assert parse_extra_hosts({
+        'www.example.com': '192.168.0.17',
+        'api.example.com': '192.168.0.18'
+    }) == {
+        'www.example.com': '192.168.0.17',
+        'api.example.com': '192.168.0.18'
+    }