Fetch Glance endpoint from Keystone if it's not provided in the configuration

This is needed to fix the CI broken by glance switching to running
under wsgi, and thus breaking our assumption that glance is accessible
by host:port only.

The options glance_host, glance_port and glance_protocol were
deprecated.  Standalone deployments should use glance_api_servers
instead.

Also removes two unused utility functions.

Change-Id: I54dc04ab084aeb7208c9dd9940c6434c029bf41c
Partial-Bug: #1699542

9 files changed, 106 insertions, 103 deletions

diff --git a/devstack/lib/ironic b/devstack/lib/ironic
index 7c3072a0e..be02c56bc 100644
--- a/devstack/lib/ironic
+++ b/devstack/lib/ironic
@@ -1210,9 +1210,6 @@ function configure_ironic_conductor {
         iniset $IRONIC_CONF_FILE glance swift_account AUTH_${tenant_id}
         iniset $IRONIC_CONF_FILE glance swift_container glance
         iniset $IRONIC_CONF_FILE glance swift_temp_url_duration 3600
-        iniset $IRONIC_CONF_FILE glance glance_protocol $GLANCE_SERVICE_PROTOCOL
-        iniset $IRONIC_CONF_FILE glance glance_host $SERVICE_HOST
-        iniset $IRONIC_CONF_FILE glance glance_port $GLANCE_SERVICE_PORT
     fi
 
     if is_deployed_by_agent; then
diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample
index 6109895ca..f69275851 100644
--- a/etc/ironic/ironic.conf.sample
+++ b/etc/ironic/ironic.conf.sample
@@ -1540,7 +1540,9 @@
 
 # A list of the glance api servers available to ironic. Prefix
 # with https:// for SSL-based glance API servers. Format is
-# [hostname|IP]:port. (list value)
+# [hostname|IP]:port. If neither this option nor glance_host
+# is set, the service catalog is used. It is recommended to
+# rely on the service catalog, if possible. (list value)
 #glance_api_servers = <None>
 
 # DEPRECATED: Glance API version (1 or 2) to use. (integer
@@ -1558,21 +1560,31 @@
 # when glance_api_insecure is set to False. (string value)
 #glance_cafile = <None>
 
-# Default glance hostname or IP address. (string value)
-#glance_host = $my_ip
+# DEPRECATED: Default glance hostname or IP address. The
+# service catalog is used when not defined. Deprecated, use
+# glance_api_servers instead. (string value)
+# This option is deprecated for removal.
+# Its value may be silently ignored in the future.
+#glance_host = <None>
 
 # Number of retries when downloading an image from glance.
 # (integer value)
 #glance_num_retries = 0
 
-# Default glance port. (port value)
+# DEPRECATED: Default glance port. Deprecated, use
+# glance_api_servers instead. (port value)
 # Minimum value: 0
 # Maximum value: 65535
+# This option is deprecated for removal.
+# Its value may be silently ignored in the future.
 #glance_port = 9292
 
-# Default protocol to use when connecting to glance. Set to
-# https for SSL. (string value)
+# DEPRECATED: Default protocol to use when connecting to
+# glance. Set to https for SSL. Deprecated, use
+# glance_api_services instead. (string value)
 # Allowed values: http, https
+# This option is deprecated for removal.
+# Its value may be silently ignored in the future.
 #glance_protocol = http
 
 # Verify HTTPS connections. (boolean value)
diff --git a/ironic/common/exception.py b/ironic/common/exception.py
index a64e49000..283a7207b 100644
--- a/ironic/common/exception.py
+++ b/ironic/common/exception.py
@@ -495,7 +495,7 @@ class UnsupportedDriverExtension(Invalid):
 
 
 class GlanceConnectionFailed(IronicException):
-    _msg_fmt = _("Connection to glance host %(host)s:%(port)s failed: "
+    _msg_fmt = _("Connection to glance host %(endpoint)s failed: "
                  "%(reason)s")
 
 
diff --git a/ironic/common/glance_service/base_image_service.py b/ironic/common/glance_service/base_image_service.py
index 72cbc47be..7cd47eafb 100644
--- a/ironic/common/glance_service/base_image_service.py
+++ b/ironic/common/glance_service/base_image_service.py
@@ -59,13 +59,8 @@ def check_image_service(func):
             return func(self, *args, **kwargs)
 
         image_href = kwargs.get('image_href')
-        (image_id, self.glance_host,
-         self.glance_port, use_ssl) = service_utils.parse_image_ref(image_href)
+        _id, self.endpoint, use_ssl = service_utils.parse_image_ref(image_href)
 
-        if use_ssl:
-            scheme = 'https'
-        else:
-            scheme = 'http'
         params = {}
         params['insecure'] = CONF.glance.glance_api_insecure
         if (not params['insecure'] and CONF.glance.glance_cafile
@@ -73,9 +68,8 @@ def check_image_service(func):
             params['cacert'] = CONF.glance.glance_cafile
         if CONF.glance.auth_strategy == 'keystone':
             params['token'] = self.context.auth_token
-        endpoint = '%s://%s:%s' % (scheme, self.glance_host, self.glance_port)
         self.client = client.Client(self.version,
-                                    endpoint, **params)
+                                    self.endpoint, **params)
         return func(self, *args, **kwargs)
     return wrapper
 
@@ -86,6 +80,7 @@ class BaseImageService(object):
         self.client = client
         self.version = version
         self.context = context
+        self.endpoint = None
 
     def call(self, method, *args, **kwargs):
         """Call a glance client method.
@@ -115,20 +110,16 @@ class BaseImageService(object):
             try:
                 return getattr(self.client.images, method)(*args, **kwargs)
             except retry_excs as e:
-                host = self.glance_host
-                port = self.glance_port
                 error_msg = ("Error contacting glance server "
-                             "'%(host)s:%(port)s' for '%(method)s', attempt"
+                             "'%(endpoint)s' for '%(method)s', attempt"
                              " %(attempt)s of %(num_attempts)s failed.")
-                LOG.exception(error_msg, {'host': host,
-                                          'port': port,
+                LOG.exception(error_msg, {'endpoint': self.endpoint,
                                           'num_attempts': num_attempts,
                                           'attempt': attempt,
                                           'method': method})
                 if attempt == num_attempts:
-                    raise exception.GlanceConnectionFailed(host=host,
-                                                           port=port,
-                                                           reason=str(e))
+                    raise exception.GlanceConnectionFailed(
+                        endpoint=self.endpoint, reason=str(e))
                 time.sleep(1)
             except image_excs as e:
                 exc_type, exc_value, exc_trace = sys.exc_info()
@@ -147,8 +138,7 @@ class BaseImageService(object):
         """
         LOG.debug("Getting image metadata from glance. Image: %s",
                   image_href)
-        (image_id, self.glance_host,
-         self.glance_port, use_ssl) = service_utils.parse_image_ref(image_href)
+        image_id = service_utils.parse_image_ref(image_href)[0]
 
         image = self.call(method, image_id)
 
@@ -165,8 +155,7 @@ class BaseImageService(object):
         :param image_id: The opaque image identifier.
         :param data: (Optional) File object to write data to.
         """
-        (image_id, self.glance_host,
-         self.glance_port, use_ssl) = service_utils.parse_image_ref(image_href)
+        image_id = service_utils.parse_image_ref(image_href)[0]
 
         if (self.version == 2 and
                 'file' in CONF.glance.allowed_direct_url_schemes):
diff --git a/ironic/common/glance_service/service_utils.py b/ironic/common/glance_service/service_utils.py
index 56bfdcf95..1320177f3 100644
--- a/ironic/common/glance_service/service_utils.py
+++ b/ironic/common/glance_service/service_utils.py
@@ -27,6 +27,7 @@ import six.moves.urllib.parse as urlparse
 
 from ironic.common import exception
 from ironic.common import image_service
+from ironic.common import keystone
 
 CONF = cfg.CONF
 
@@ -34,18 +35,6 @@ _GLANCE_API_SERVER = None
 """ iterator that cycles (indefinitely) over glance API servers. """
 
 
-def generate_glance_url():
-    """Generate the URL to glance."""
-    return "%s://%s:%d" % (CONF.glance.glance_protocol,
-                           CONF.glance.glance_host,
-                           CONF.glance.glance_port)
-
-
-def generate_image_url(image_ref):
-    """Generate an image URL from an image_ref."""
-    return "%s/images/%s" % (generate_glance_url(), image_ref)
-
-
 def _extract_attributes(image):
     IMAGE_ATTRIBUTES = ['size', 'disk_format', 'owner',
                         'container_format', 'checksum', 'id',
@@ -110,24 +99,28 @@ def _get_api_server_iterator():
 
     If CONF.glance.glance_api_servers isn't set, we fall back to using this
     as the server: CONF.glance.glance_host:CONF.glance.glance_port.
+    If CONF.glance.glance_host is also not set, fetch the endpoint from the
+    service catalog.
 
     :returns: iterator that cycles (indefinitely) over shuffled glance API
-              servers. The iterator returns tuples of (host, port, use_ssl).
+              servers.
     """
     api_servers = []
 
-    configured_servers = (CONF.glance.glance_api_servers or
-                          ['%s:%s' % (CONF.glance.glance_host,
-                                      CONF.glance.glance_port)])
-    for api_server in configured_servers:
-        if '//' not in api_server:
-            api_server = '%s://%s' % (CONF.glance.glance_protocol, api_server)
-        url = urlparse.urlparse(api_server)
-        port = url.port or 80
-        host = url.netloc.split(':', 1)[0]
-        use_ssl = (url.scheme == 'https')
-        api_servers.append((host, port, use_ssl))
-    random.shuffle(api_servers)
+    if not CONF.glance.glance_api_servers and not CONF.glance.glance_host:
+        session = keystone.get_session('service_catalog')
+        api_servers = [keystone.get_service_url(session, service_type='image',
+                                                endpoint_type='public')]
+    else:
+        configured_servers = (CONF.glance.glance_api_servers or
+                              ['%s:%s' % (CONF.glance.glance_host,
+                                          CONF.glance.glance_port)])
+        for api_server in configured_servers:
+            if '//' not in api_server:
+                api_server = '%s://%s' % (CONF.glance.glance_protocol,
+                                          api_server)
+            api_servers.append(api_server)
+        random.shuffle(api_servers)
     return itertools.cycle(api_servers)
 
 
@@ -145,29 +138,31 @@ def _get_api_server():
 
 
 def parse_image_ref(image_href):
-    """Parse an image href into composite parts.
+    """Parse an image href.
 
     :param image_href: href of an image
-    :returns: a tuple of the form (image_id, host, port, use_ssl)
+    :returns: a tuple (image ID, glance URL, whether to use SSL)
 
     :raises ValueError: when input image href is invalid
     """
     if '/' not in six.text_type(image_href):
-        image_id = image_href
-        (glance_host, glance_port, use_ssl) = _get_api_server()
-        return (image_id, glance_host, glance_port, use_ssl)
+        endpoint = _get_api_server()
+        return (image_href, endpoint, endpoint.startswith('https'))
     else:
         try:
             url = urlparse.urlparse(image_href)
             if url.scheme == 'glance':
-                (glance_host, glance_port, use_ssl) = _get_api_server()
+                endpoint = _get_api_server()
                 image_id = image_href.split('/')[-1]
+                return (image_id, endpoint, endpoint.startswith('https'))
             else:
                 glance_port = url.port or 80
                 glance_host = url.netloc.split(':', 1)[0]
                 image_id = url.path.split('/')[-1]
                 use_ssl = (url.scheme == 'https')
-            return (image_id, glance_host, glance_port, use_ssl)
+                endpoint = '%s://%s:%s' % (url.scheme, glance_host,
+                                           glance_port)
+                return (image_id, endpoint, use_ssl)
         except ValueError:
             raise exception.InvalidImageRef(image_href=image_href)
 
diff --git a/ironic/conf/glance.py b/ironic/conf/glance.py
index b29500e32..0bbbfdbac 100644
--- a/ironic/conf/glance.py
+++ b/ironic/conf/glance.py
@@ -104,20 +104,29 @@ opts = [
                       'multiple containers to store images, and this value '
                       'will determine how many containers are created.')),
     cfg.StrOpt('glance_host',
-               default='$my_ip',
-               help=_('Default glance hostname or IP address.')),
+               help=_('Default glance hostname or IP address. The service '
+                      'catalog is used when not defined. Deprecated, '
+                      'use glance_api_servers instead.'),
+               deprecated_for_removal=True),
     cfg.PortOpt('glance_port',
                 default=9292,
-                help=_('Default glance port.')),
+                help=_('Default glance port. Deprecated, use '
+                       'glance_api_servers instead.'),
+                deprecated_for_removal=True),
     cfg.StrOpt('glance_protocol',
                default='http',
                choices=['http', 'https'],
                help=_('Default protocol to use when connecting to glance. '
-                      'Set to https for SSL.')),
+                      'Set to https for SSL. Deprecated, use '
+                      'glance_api_services instead.'),
+               deprecated_for_removal=True),
     cfg.ListOpt('glance_api_servers',
                 help=_('A list of the glance api servers available to ironic. '
                        'Prefix with https:// for SSL-based glance API '
-                       'servers. Format is [hostname|IP]:port.')),
+                       'servers. Format is [hostname|IP]:port. If neither '
+                       'this option nor glance_host is set, the service '
+                       'catalog is used. It is recommended to rely on the '
+                       'service catalog, if possible.')),
     cfg.BoolOpt('glance_api_insecure',
                 default=False,
                 help=_('Allow to perform insecure SSL (https) requests to '
diff --git a/ironic/tests/unit/common/test_glance_service.py b/ironic/tests/unit/common/test_glance_service.py
index d65b2df79..cd6793fc7 100644
--- a/ironic/tests/unit/common/test_glance_service.py
+++ b/ironic/tests/unit/common/test_glance_service.py
@@ -807,24 +807,6 @@ class TestSwiftTempUrlCache(base.TestCase):
         self.assertNotIn(fake_image['id'], self.glance_service._cache)
 
 
-class TestGlanceUrl(base.TestCase):
-
-    def test_generate_glance_http_url(self):
-        self.config(glance_host="127.0.0.1", group='glance')
-        generated_url = service_utils.generate_glance_url()
-        http_url = "http://%s:%d" % (CONF.glance.glance_host,
-                                     CONF.glance.glance_port)
-        self.assertEqual(http_url, generated_url)
-
-    def test_generate_glance_https_url(self):
-        self.config(glance_protocol="https", group='glance')
-        self.config(glance_host="127.0.0.1", group='glance')
-        generated_url = service_utils.generate_glance_url()
-        https_url = "https://%s:%d" % (CONF.glance.glance_host,
-                                       CONF.glance.glance_port)
-        self.assertEqual(https_url, generated_url)
-
-
 class TestServiceUtils(base.TestCase):
 
     def test_parse_image_ref_no_ssl(self):
@@ -832,24 +814,14 @@ class TestServiceUtils(base.TestCase):
             u'image_\u00F9\u00FA\u00EE\u0111'
         parsed_href = service_utils.parse_image_ref(image_href)
         self.assertEqual((u'image_\u00F9\u00FA\u00EE\u0111',
-                          '127.0.0.1', 9292, False), parsed_href)
+                          'http://127.0.0.1:9292', False), parsed_href)
 
     def test_parse_image_ref_ssl(self):
         image_href = 'https://127.0.0.1:9292/image_path/'\
                      u'image_\u00F9\u00FA\u00EE\u0111'
         parsed_href = service_utils.parse_image_ref(image_href)
         self.assertEqual((u'image_\u00F9\u00FA\u00EE\u0111',
-                          '127.0.0.1', 9292, True), parsed_href)
-
-    def test_generate_image_url(self):
-        image_href = u'image_\u00F9\u00FA\u00EE\u0111'
-        self.config(glance_host='123.123.123.123', group='glance')
-        self.config(glance_port=1234, group='glance')
-        self.config(glance_protocol='https', group='glance')
-        generated_url = service_utils.generate_image_url(image_href)
-        self.assertEqual('https://123.123.123.123:1234/images/'
-                         u'image_\u00F9\u00FA\u00EE\u0111',
-                         generated_url)
+                          'https://127.0.0.1:9292', True), parsed_href)
 
     def test_is_glance_image(self):
         image_href = u'uui\u0111'
@@ -884,18 +856,34 @@ class TestGlanceAPIServers(base.TestCase):
         super(TestGlanceAPIServers, self).setUp()
         service_utils._GLANCE_API_SERVER = None
 
-    def test__get_api_servers_default(self):
-        host, port, use_ssl = service_utils._get_api_server()
-        self.assertEqual(CONF.glance.glance_host, host)
-        self.assertEqual(CONF.glance.glance_port, port)
-        self.assertEqual(CONF.glance.glance_protocol == 'https', use_ssl)
+    @mock.patch.object(service_utils.keystone, 'get_service_url',
+                       autospec=True)
+    @mock.patch.object(service_utils.keystone, 'get_session', autospec=True)
+    def test__get_api_servers_with_keystone(self, mock_session,
+                                            mock_service_url):
+        mock_service_url.return_value = 'https://example.com'
+
+        endpoint = service_utils._get_api_server()
+        self.assertEqual('https://example.com', endpoint)
+
+        mock_session.assert_called_once_with('service_catalog')
+        mock_service_url.assert_called_once_with(mock_session.return_value,
+                                                 service_type='image',
+                                                 endpoint_type='public')
+
+    def test__get_api_servers_with_host_port(self):
+        CONF.set_override('glance_host', 'example.com', 'glance')
+        CONF.set_override('glance_port', 42, 'glance')
+        CONF.set_override('glance_protocol', 'https', 'glance')
+        endpoint = service_utils._get_api_server()
+        self.assertEqual('https://example.com:42', endpoint)
 
     def test__get_api_servers_one(self):
         CONF.set_override('glance_api_servers', ['https://10.0.0.1:9293'],
                           'glance')
         s1 = service_utils._get_api_server()
         s2 = service_utils._get_api_server()
-        self.assertEqual(('10.0.0.1', 9293, True), s1)
+        self.assertEqual('https://10.0.0.1:9293', s1)
 
         # Only one server, should always get the same one
         self.assertEqual(s1, s2)
diff --git a/ironic/tests/unit/drivers/modules/test_image_cache.py b/ironic/tests/unit/drivers/modules/test_image_cache.py
index b50d3fe40..eb12cfbd3 100644
--- a/ironic/tests/unit/drivers/modules/test_image_cache.py
+++ b/ironic/tests/unit/drivers/modules/test_image_cache.py
@@ -38,6 +38,8 @@ def touch(filename):
     open(filename, 'w').close()
 
 
+@mock.patch('ironic.common.glance_service.service_utils.parse_image_ref',
+            lambda image: (image, 'example.com', True))
 class TestImageCacheFetch(base.TestCase):
 
     def setUp(self):
diff --git a/releasenotes/notes/glance-keystone-dd30b884f07f83fb.yaml b/releasenotes/notes/glance-keystone-dd30b884f07f83fb.yaml
new file mode 100644
index 000000000..f9db5fa69
--- /dev/null
+++ b/releasenotes/notes/glance-keystone-dd30b884f07f83fb.yaml
@@ -0,0 +1,11 @@
+---
+upgrade:
+  - |
+    The configuration option ``[glance]glance_host`` is now empty by default.
+    If neither it nor ``[glance]glance_api_servers`` are provided, ironic will
+    now try fetching the Image service endpoint from the service catalog.
+deprecations:
+  - |
+    The configuration options ``[glance]glance_host``, ``[glance]glance_port``
+    and ``[glance]glance_protocol`` are deprecated in favor of either using
+    ``[glance]glance_api_servers`` or using the service catalog.