From cb7cdd35347c76e1ca66644532be1b6c898491c1 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 22 Jun 2017 14:33:32 +0200 Subject: [PATCH] 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 --- devstack/lib/ironic | 3 - etc/ironic/ironic.conf.sample | 24 ++++++-- ironic/common/exception.py | 2 +- .../glance_service/base_image_service.py | 29 +++------ ironic/common/glance_service/service_utils.py | 59 +++++++++--------- ironic/conf/glance.py | 19 ++++-- .../tests/unit/common/test_glance_service.py | 60 ++++++++----------- .../unit/drivers/modules/test_image_cache.py | 2 + .../glance-keystone-dd30b884f07f83fb.yaml | 11 ++++ 9 files changed, 106 insertions(+), 103 deletions(-) create mode 100644 releasenotes/notes/glance-keystone-dd30b884f07f83fb.yaml diff --git a/devstack/lib/ironic b/devstack/lib/ironic index 7c3072a0e1..be02c56bc2 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 6109895ca4..f692758514 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 = # 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 = -# 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 = # 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 a64e490004..283a7207be 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 72cbc47bef..7cd47eafb7 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 56bfdcf95f..1320177f3b 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 b29500e321..0bbbfdbacc 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 d65b2df798..cd6793fc7e 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 b50d3fe40d..eb12cfbd32 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 0000000000..f9db5fa69f --- /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.