Handle IPv6 specifid glance servers gracefully

IPv6 netlocs can for valid reasons contain ':', so
splitting by ':' is dangerous. Instead of splitting
by host and port in order to only reassemble it into
a netloc again later, simply always pass netloc, which
avoids the bug.

Add extra textcases to verify the new behavior.
Fix Flake8 warnings.

Fixes LP Bug #1182830

Change-Id: I130528ae946e8888d35c25e468b4ea6ac29db0cf

diff --git a/cinder/image/glance.py b/cinder/image/glance.py
index bfec5288e70106092ef0be9e57b497cbe6d8fe11..ca1dd0964886ba139a6dd6005aed975ca5cbf2b2 100644 (file)
--- a/cinder/image/glance.py
+++ b/cinder/image/glance.py
@@ -44,21 +44,20 @@ def _parse_image_ref(image_href):
     """Parse an image href into composite parts.
 
     :param image_href: href of an image
-    :returns: a tuple of the form (image_id, host, port)
+    :returns: a tuple of the form (image_id, netloc, use_ssl)
     :raises ValueError
 
     """
     url = urlparse.urlparse(image_href)
-    port = url.port or 80
-    host = url.netloc.split(':', 1)[0]
+    netloc = url.netloc
     image_id = url.path.split('/')[-1]
     use_ssl = (url.scheme == 'https')
-    return (image_id, host, port, use_ssl)
+    return (image_id, netloc, use_ssl)
 
 
-def _create_glance_client(context, host, port, use_ssl,
+def _create_glance_client(context, netloc, use_ssl,
                           version=FLAGS.glance_api_version):
-    """Instantiate a new glanceclient.Client object"""
+    """Instantiate a new glanceclient.Client object."""
     if version is None:
         version = FLAGS.glance_api_version
     if use_ssl:
@@ -69,12 +68,13 @@ def _create_glance_client(context, host, port, use_ssl,
     params['insecure'] = FLAGS.glance_api_insecure
     if FLAGS.auth_strategy == 'keystone':
         params['token'] = context.auth_token
-    endpoint = '%s://%s:%s' % (scheme, host, port)
+    endpoint = '%s://%s' % (scheme, netloc)
     return glanceclient.Client(str(version), endpoint, **params)
 
 
 def get_api_servers():
-    """
+    """Return Iterable over shuffled api servers.
+
     Shuffle a list of FLAGS.glance_api_servers and return an iterator
     that will cycle through the list, looping around to the beginning
     if necessary.
@@ -84,10 +84,9 @@ def get_api_servers():
         if '//' not in api_server:
             api_server = 'http://' + api_server
         url = urlparse.urlparse(api_server)
-        port = url.port or 80
-        host = url.netloc.split(':', 1)[0]
+        netloc = url.netloc
         use_ssl = (url.scheme == 'https')
-        api_servers.append((host, port, use_ssl))
+        api_servers.append((netloc, use_ssl))
     random.shuffle(api_servers)
     return itertools.cycle(api_servers)
 
@@ -95,39 +94,39 @@ def get_api_servers():
 class GlanceClientWrapper(object):
     """Glance client wrapper class that implements retries."""
 
-    def __init__(self, context=None, host=None, port=None, use_ssl=False,
+    def __init__(self, context=None, netloc=None, use_ssl=False,
                  version=None):
-        if host is not None:
+        if netloc is not None:
             self.client = self._create_static_client(context,
-                                                     host, port,
+                                                     netloc,
                                                      use_ssl, version)
         else:
             self.client = None
         self.api_servers = None
         self.version = version
 
-    def _create_static_client(self, context, host, port, use_ssl, version):
+    def _create_static_client(self, context, netloc, use_ssl, version):
         """Create a client that we'll use for every call."""
-        self.host = host
-        self.port = port
+        self.netloc = netloc
         self.use_ssl = use_ssl
         self.version = version
         return _create_glance_client(context,
-                                     self.host, self.port,
+                                     self.netloc,
                                      self.use_ssl, self.version)
 
     def _create_onetime_client(self, context, version):
         """Create a client that will be used for one call."""
         if self.api_servers is None:
             self.api_servers = get_api_servers()
-        self.host, self.port, self.use_ssl = self.api_servers.next()
+        self.netloc, self.use_ssl = self.api_servers.next()
         return _create_glance_client(context,
-                                     self.host, self.port,
+                                     self.netloc,
                                      self.use_ssl, version)
 
     def call(self, context, method, *args, **kwargs):
-        """
-        Call a glance client method.  If we get a connection error,
+        """Call a glance client method.
+
+        If we get a connection error,
         retry the request according to FLAGS.glance_num_retries.
         """
         version = self.version
@@ -145,17 +144,15 @@ class GlanceClientWrapper(object):
             try:
                 return getattr(client.images, method)(*args, **kwargs)
             except retry_excs as e:
-                host = self.host
-                port = self.port
+                netloc = self.netloc
                 extra = "retrying"
                 error_msg = _("Error contacting glance server "
-                              "'%(host)s:%(port)s' for '%(method)s', "
+                              "'%(netloc)s' for '%(method)s', "
                               "%(extra)s.")
                 if attempt == num_attempts:
                     extra = 'done trying'
                     LOG.exception(error_msg, locals())
-                    raise exception.GlanceConnectionFailed(host=host,
-                                                           port=port,
+                    raise exception.GlanceConnectionFailed(netloc=netloc,
                                                            reason=str(e))
                 LOG.exception(error_msg, locals())
                 time.sleep(1)
@@ -212,7 +209,8 @@ class GlanceImageService(object):
 
     def get_location(self, context, image_id):
         """Returns the direct url representing the backend storage location,
-        or None if this attribute is not shown by Glance."""
+        or None if this attribute is not shown by Glance.
+        """
         try:
             client = GlanceClientWrapper()
             image_meta = client.call(context, 'get', image_id)
@@ -443,11 +441,9 @@ def get_remote_image_service(context, image_href):
         return image_service, image_href
 
     try:
-        (image_id, glance_host, glance_port, use_ssl) = \
-            _parse_image_ref(image_href)
+        (image_id, glance_netloc, use_ssl) = _parse_image_ref(image_href)
         glance_client = GlanceClientWrapper(context=context,
-                                            host=glance_host,
-                                            port=glance_port,
+                                            netloc=glance_netloc,
                                             use_ssl=use_ssl)
     except ValueError:
         raise exception.InvalidImageRef(image_href=image_href)

diff --git a/cinder/tests/image/test_glance.py b/cinder/tests/image/test_glance.py
index 46bbafa9c5dbb78b52b1f84faea571a3428326bc..2a80e2dfe5a4ad145a44931d6ea9bc0f83258357 100644 (file)
--- a/cinder/tests/image/test_glance.py
+++ b/cinder/tests/image/test_glance.py
@@ -17,8 +17,6 @@
 
 
 import datetime
-import random
-import time
 
 import glanceclient.exc
 
@@ -78,8 +76,7 @@ class TestGlanceSerializer(test.TestCase):
 
 
 class TestGlanceImageService(test.TestCase):
-    """
-    Tests the Glance image service.
+    """Tests the Glance image service.
 
     At a high level, the translations involved are:
 
@@ -110,7 +107,7 @@ class TestGlanceImageService(test.TestCase):
         self.stubs.Set(glance.time, 'sleep', lambda s: None)
 
     def _create_image_service(self, client):
-        def _fake_create_glance_client(context, host, port, use_ssl, version):
+        def _fake_create_glance_client(context, netloc, use_ssl, version):
             return client
 
         self.stubs.Set(glance,
@@ -166,7 +163,8 @@ class TestGlanceImageService(test.TestCase):
         self.assertDictMatch(image_metas[0], expected)
 
     def test_create_without_instance_id(self):
-        """
+        """Test Creating images without instance_id.
+
         Ensure we can create an image without having to specify an
         instance_id. Public images are an example of an image not tied to an
         instance.
@@ -538,12 +536,18 @@ class TestGlanceImageService(test.TestCase):
         (service, same_id) = glance.get_remote_image_service(self.context,
                                                              image_url)
         self.assertEquals(same_id, image_id)
-        self.assertEquals(service._client.host,
+        self.assertEquals(service._client.netloc,
                           'something-less-likely')
+        for ipv6_url in ('[::1]', '::1', '[::1]:444'):
+            image_url = 'http://%s/%s' % (ipv6_url, image_id)
+            (service, same_id) = glance.get_remote_image_service(self.context,
+                                                                 image_url)
+            self.assertEquals(same_id, image_id)
+            self.assertEquals(service._client.netloc, ipv6_url)
 
 
 class TestGlanceClientVersion(test.TestCase):
-    """Tests the version of the glance client generated"""
+    """Tests the version of the glance client generated."""
     def setUp(self):
         super(TestGlanceClientVersion, self).setUp()
 
@@ -554,7 +558,7 @@ class TestGlanceClientVersion(test.TestCase):
                        fake_get_image_model)
 
     def test_glance_version_by_flag(self):
-        """Test glance version set by flag is honoured"""
+        """Test glance version set by flag is honoured."""
         client_wrapper_v1 = glance.GlanceClientWrapper('fake', 'fake_host',
                                                        9292)
         self.assertEquals(client_wrapper_v1.client.__module__,