utils: fix IPv6 address w/ port parsing (#3006)

This was using a deprecated function (`urllib.splitnport`),
ostensibly to work around issues with brackets on IPv6 addresses.

Ironically, its usage was broken, and would result in mangled IPv6
addresses if they had a port specified in some instances.

Usage of the deprecated function has been eliminated and extra test
cases added where missing. All existing cases pass as-is. (The only
other change to the test was to improve assertion messages.)

Signed-off-by: Milas Bowman <milas.bowman@docker.com>

docker/utils/utils.py

@@ -1,4 +1,5 @@
 import base64
+import collections
 import json
 import os
 import os.path
@@ -8,15 +9,20 @@ from datetime import datetime
 from distutils.version import StrictVersion
 
 from .. import errors
-from .. import tls
 from ..constants import DEFAULT_HTTP_HOST
 from ..constants import DEFAULT_UNIX_SOCKET
 from ..constants import DEFAULT_NPIPE
 from ..constants import BYTE_UNITS
+from ..tls import TLSConfig
 
-from urllib.parse import splitnport, urlparse
+from urllib.parse import urlparse, urlunparse
 
 
+URLComponents = collections.namedtuple(
+    'URLComponents',
+    'scheme netloc url params query fragment',
+)
+
 def create_ipam_pool(*args, **kwargs):
     raise errors.DeprecatedMethod(
         'utils.create_ipam_pool has been removed. Please use a '
@@ -201,10 +207,6 @@ def parse_repository_tag(repo_name):
 
 
 def parse_host(addr, is_win32=False, tls=False):
-    path = ''
-    port = None
-    host = None
-
     # Sensible defaults
     if not addr and is_win32:
         return DEFAULT_NPIPE
@@ -263,20 +265,20 @@ def parse_host(addr, is_win32=False, tls=False):
             # to be valid and equivalent to unix:///path
             path = '/'.join((parsed_url.hostname, path))
 
+    netloc = parsed_url.netloc
     if proto in ('tcp', 'ssh'):
-        # parsed_url.hostname strips brackets from IPv6 addresses,
-        # which can be problematic hence our use of splitnport() instead.
-        host, port = splitnport(parsed_url.netloc)
-        if port is None or port < 0:
+        port = parsed_url.port or 0
+        if port <= 0:
             if proto != 'ssh':
                 raise errors.DockerException(
                     'Invalid bind address format: port is required:'
                     ' {}'.format(addr)
                 )
             port = 22
+            netloc = f'{parsed_url.netloc}:{port}'
 
-        if not host:
-            host = DEFAULT_HTTP_HOST
+        if not parsed_url.hostname:
+            netloc = f'{DEFAULT_HTTP_HOST}:{port}'
 
     # Rewrite schemes to fit library internals (requests adapters)
     if proto == 'tcp':
@@ -286,7 +288,15 @@ def parse_host(addr, is_win32=False, tls=False):
 
     if proto in ('http+unix', 'npipe'):
         return f"{proto}://{path}".rstrip('/')
-    return f'{proto}://{host}:{port}{path}'.rstrip('/')
+
+    return urlunparse(URLComponents(
+        scheme=proto,
+        netloc=netloc,
+        url=path,
+        params='',
+        query='',
+        fragment='',
+    )).rstrip('/')
 
 
 def parse_devices(devices):
@@ -351,7 +361,7 @@ def kwargs_from_env(ssl_version=None, assert_hostname=None, environment=None):
         # so if it's not set already then set it to false.
         assert_hostname = False
 
-    params['tls'] = tls.TLSConfig(
+    params['tls'] = TLSConfig(
         client_cert=(os.path.join(cert_path, 'cert.pem'),
                      os.path.join(cert_path, 'key.pem')),
         ca_cert=os.path.join(cert_path, 'ca.pem'),

tests/unit/utils_test.py

@@ -296,17 +296,24 @@ class ParseHostTest(unittest.TestCase):
             '[fd12::82d1]:2375/docker/engine': (
                 'http://[fd12::82d1]:2375/docker/engine'
             ),
+            'ssh://[fd12::82d1]': 'ssh://[fd12::82d1]:22',
+            'ssh://user@[fd12::82d1]:8765': 'ssh://user@[fd12::82d1]:8765',
             'ssh://': 'ssh://127.0.0.1:22',
             'ssh://user@localhost:22': 'ssh://user@localhost:22',
             'ssh://user@remote': 'ssh://user@remote:22',
         }
 
         for host in invalid_hosts:
-            with pytest.raises(DockerException):
+            msg = f'Should have failed to parse invalid host: {host}'
+            with self.assertRaises(DockerException, msg=msg):
                 parse_host(host, None)
 
         for host, expected in valid_hosts.items():
-            assert parse_host(host, None) == expected
+            self.assertEqual(
+                parse_host(host, None),
+                expected,
+                msg=f'Failed to parse valid host: {host}',
+            )
 
     def test_parse_host_empty_value(self):
         unix_socket = 'http+unix:///var/run/docker.sock'