From 83dd6a1c0ea0b2ec8059b90a3f7af5f2d766fef2 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 7 Dec 2017 16:23:18 +0100 Subject: [PATCH] Auto-detect the defaults for [glance]swift_{account,temp_url_key,endpoint_url} In many cases, including devstack and TripleO, the swift account to use is based on the project_id of the 'service' project, so it makes sense to use it as a default. Similarly, the temporary URL key can be fetches from Swift, using HEAD on the account used to access it. In both cases it is assumed that the same project is used by Ironic and Glance to access Swift. Explicit values have to be provided otherwise. Finally, the endpoint URL can be fetched from the service catalog. Care is taken to strip the /v1/AUTH_ suffix. Closes-Bug: #1737714 Change-Id: I701899928f0f780939f17aabc59eb90593ba95f0 --- devstack/lib/ironic | 10 +- doc/source/install/configure-glance-swift.rst | 24 +-- etc/ironic/ironic.conf.sample | 17 ++- .../common/glance_service/v2/image_service.py | 53 +++++-- ironic/common/swift.py | 7 +- ironic/conf/glance.py | 12 +- .../tests/unit/common/test_glance_service.py | 144 ++++++++++++++++-- ironic/tests/unit/common/test_swift.py | 2 +- ...efault-swift_account-b008d08e85bdf154.yaml | 15 ++ 9 files changed, 228 insertions(+), 56 deletions(-) create mode 100644 releasenotes/notes/default-swift_account-b008d08e85bdf154.yaml diff --git a/devstack/lib/ironic b/devstack/lib/ironic index 0600ab3dc3..b1a4dba0e8 100644 --- a/devstack/lib/ironic +++ b/devstack/lib/ironic @@ -1198,21 +1198,13 @@ function configure_ironic_conductor { # directly from glance, and don't set them where the image is pushed # over iSCSI. if is_glance_configuration_required; then - if [[ "$SWIFT_ENABLE_TEMPURLS" == "True" ]] ; then - iniset $IRONIC_CONF_FILE glance swift_temp_url_key $SWIFT_TEMPURL_KEY - else + if [[ "$SWIFT_ENABLE_TEMPURLS" == "False" ]] ; then die $LINENO "SWIFT_ENABLE_TEMPURLS must be True. This is " \ "required either because IRONIC_DEPLOY_DRIVER was " \ "set to some agent_* driver OR configuration of " \ "Glance with Swift was explicitly requested with " \ "IRONIC_CONFIGURE_GLANCE_WITH_SWIFT=True" fi - iniset $IRONIC_CONF_FILE glance swift_endpoint_url ${SWIFT_SERVICE_PROTOCOL}://${SERVICE_HOST}:${SWIFT_DEFAULT_BIND_PORT:-8080} - iniset $IRONIC_CONF_FILE glance swift_api_version v1 - local tenant_id - tenant_id=$(get_or_create_project $SERVICE_PROJECT_NAME default) - 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 fi diff --git a/doc/source/install/configure-glance-swift.rst b/doc/source/install/configure-glance-swift.rst index 13e67f435d..d707475343 100644 --- a/doc/source/install/configure-glance-swift.rst +++ b/doc/source/install/configure-glance-swift.rst @@ -50,18 +50,18 @@ and Object Storage service as described below. $ openstack object store account set --property Temp-Url-Key=secret +#. Optionally, configure the ironic-conductor service. The default + configuration assumes that: -#. Configure the ironic-conductor service. - The configuration file is typically located at - ``/etc/ironic/ironic.conf``. - Some of the required values are available in the response of an - ``openstack object store account show`` command; - others have to match those configured in Image and Object Store services - configuration files. Below is a example of a minimal set of configuration - options to specify when Object Storage service is provided by swift - (check configuration file sample included within ironic - code ``etc/ironic/ironic.conf.sample`` for full list of available options - and their detailed descriptions): + #. the Object Storage service is implemented by swift_, + #. the Object Storage service URL is available from the service catalog, + #. the project, used by the Image service to access the Object Storage, is + the same as the project, used by the Bare Metal service to access it, + #. the container, used by the Image service, is called ``glance``. + + If any of these assumptions do not hold, you may want to change your + configuration file (typically located at ``/etc/ironic/ironic.conf``), + for example: .. code-block:: ini @@ -74,3 +74,5 @@ and Object Storage service as described below. swift_temp_url_key = secret #. (Re)start the ironic-conductor service. + +.. _swift: https://docs.openstack.org/swift/latest/ diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index e821dcbf6d..8490ff7d7d 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -1756,10 +1756,11 @@ # The account that Glance uses to communicate with Swift. The # format is "AUTH_uuid". "uuid" is the UUID for the account -# configured in the glance-api.conf. Required for temporary -# URLs when Glance backend is Swift. For example: -# "AUTH_a422b2-91f3-2f46-74b7-d7c9e8958f5d30". Swift temporary -# URL format: +# configured in the glance-api.conf. For example: +# "AUTH_a422b2-91f3-2f46-74b7-d7c9e8958f5d30". If not set, the +# default value is calculated based on the ID of the project +# used to access Swift (as set in the [swift] section). Swift +# temporary URL format: # "endpoint_url/api_version/[account/]container/object_id" # (string value) #swift_account = @@ -1783,8 +1784,8 @@ # not include trailing "/". For example, use # "https://swift.example.com". If using RADOS Gateway, # endpoint may also contain /swift path; if it does not, it -# will be appended. Required for temporary URLs. (string -# value) +# will be appended. Used for temporary URLs, will be fetched +# from the service catalog, if not provided. (string value) #swift_endpoint_url = # This should match a config by the same name in the Glance @@ -1823,7 +1824,9 @@ #swift_temp_url_expected_download_start_delay = 0 # The secret token given to Swift to allow temporary URL -# downloads. Required for temporary URLs. (string value) +# downloads. Required for temporary URLs. For the Swift +# backend, the key on the service project (as set in the +# [swift] section) is used by default. (string value) #swift_temp_url_key = # Tenant ID (string value) diff --git a/ironic/common/glance_service/v2/image_service.py b/ironic/common/glance_service/v2/image_service.py index 0e0d9a8b0d..78c9165a60 100644 --- a/ironic/common/glance_service/v2/image_service.py +++ b/ironic/common/glance_service/v2/image_service.py @@ -14,6 +14,7 @@ # under the License. import collections +import re import time from oslo_utils import uuidutils @@ -25,6 +26,8 @@ from ironic.common.glance_service import base_image_service from ironic.common.glance_service import service from ironic.common.glance_service import service_utils from ironic.common.i18n import _ +from ironic.common import keystone +from ironic.common import swift from ironic.conf import CONF TempUrlCacheElement = collections.namedtuple('TempUrlCacheElement', @@ -126,12 +129,26 @@ class GlanceImageService(base_image_service.BaseImageService, url_fragments = { 'api_version': CONF.glance.swift_api_version, - 'account': CONF.glance.swift_account, 'container': self._get_swift_container(image_id), 'object_id': image_id } endpoint_url = CONF.glance.swift_endpoint_url + if not endpoint_url: + swift_session = swift.get_swift_session() + adapter = keystone.get_adapter('swift', session=swift_session) + endpoint_url = adapter.get_endpoint() + + if not endpoint_url: + raise exc.MissingParameterValue(_( + 'Swift temporary URLs require a Swift endpoint URL, but it ' + 'was not found in the service catalog. ' + 'You must provide "swift_endpoint_url" as a config option.')) + + # Strip /v1/AUTH_%(tenant_id)s, if present + endpoint_url = re.sub('/v1/AUTH_[^/]+/?$', '', endpoint_url) + + key = CONF.glance.swift_temp_url_key if CONF.deploy.object_store_endpoint_type == 'radosgw': chunks = urlparse.urlsplit(CONF.glance.swift_endpoint_url) if not chunks.path: @@ -143,8 +160,28 @@ class GlanceImageService(base_image_service.BaseImageService, 'hostname, optional port and optional /swift path ' 'without trailing slash; provided value is: %s') % endpoint_url) + template = '/{api_version}/{container}/{object_id}' else: + account = CONF.glance.swift_account + if not account: + swift_session = swift.get_swift_session() + auth_ref = swift_session.auth.get_auth_ref(swift_session) + account = 'AUTH_%s' % auth_ref.project_id + + if not key: + swift_api = swift.SwiftAPI() + key_header = 'x-account-meta-temp-url-key' + key = swift_api.connection.head_account().get(key_header) + + if not key: + raise exc.MissingParameterValue(_( + 'Swift temporary URLs require a shared secret to be ' + 'created. You must provide "swift_temp_url_key" as a ' + 'config option or pre-generate the key on the project ' + 'used to access Swift.')) + + url_fragments['account'] = account template = '/{api_version}/{account}/{container}/{object_id}' url_path = template.format(**url_fragments) @@ -152,7 +189,7 @@ class GlanceImageService(base_image_service.BaseImageService, return self._generate_temp_url( path=url_path, seconds=CONF.glance.swift_temp_url_duration, - key=CONF.glance.swift_temp_url_key, + key=key, method='GET', endpoint=endpoint_url, image_id=image_id @@ -160,19 +197,11 @@ class GlanceImageService(base_image_service.BaseImageService, def _validate_temp_url_config(self): """Validate the required settings for a temporary URL.""" - if not CONF.glance.swift_temp_url_key: + if (not CONF.glance.swift_temp_url_key and + CONF.deploy.object_store_endpoint_type != 'swift'): raise exc.MissingParameterValue(_( 'Swift temporary URLs require a shared secret to be created. ' 'You must provide "swift_temp_url_key" as a config option.')) - if not CONF.glance.swift_endpoint_url: - raise exc.MissingParameterValue(_( - 'Swift temporary URLs require a Swift endpoint URL. ' - 'You must provide "swift_endpoint_url" as a config option.')) - if (not CONF.glance.swift_account and - CONF.deploy.object_store_endpoint_type == 'swift'): - raise exc.MissingParameterValue(_( - 'Swift temporary URLs require a Swift account string. ' - 'You must provide "swift_account" as a config option.')) if (CONF.glance.swift_temp_url_duration < CONF.glance.swift_temp_url_expected_download_start_delay): raise exc.InvalidParameterValue(_( diff --git a/ironic/common/swift.py b/ironic/common/swift.py index 6d07ed5386..b67df2a40b 100644 --- a/ironic/common/swift.py +++ b/ironic/common/swift.py @@ -29,7 +29,7 @@ from ironic.conf import CONF _SWIFT_SESSION = None -def _get_swift_session(): +def get_swift_session(): global _SWIFT_SESSION if not _SWIFT_SESSION: auth = keystone.get_auth('swift') @@ -40,6 +40,9 @@ def _get_swift_session(): class SwiftAPI(object): """API for communicating with Swift.""" + connection = None + """Underlying Swift connection object.""" + def __init__(self): """Initialize the connection with swift or radosgw @@ -64,7 +67,7 @@ class SwiftAPI(object): # support. # TODO(pas-ha) pass the context here and use token from context # with service auth - params['session'] = session = _get_swift_session() + params['session'] = session = get_swift_session() adapter = keystone.get_adapter('swift', session=session) params['os_options'] = { 'object_storage_url': adapter.get_endpoint()} diff --git a/ironic/conf/glance.py b/ironic/conf/glance.py index 089bf839d2..8ba3878f2e 100644 --- a/ironic/conf/glance.py +++ b/ironic/conf/glance.py @@ -33,7 +33,9 @@ opts = [ # radosgw-admin user modify --uid=user --temp-url-key=secretkey cfg.StrOpt('swift_temp_url_key', help=_('The secret token given to Swift to allow temporary URL ' - 'downloads. Required for temporary URLs.'), + 'downloads. Required for temporary URLs. For the ' + 'Swift backend, the key on the service project (as set ' + 'in the [swift] section) is used by default.'), secret=True), cfg.IntOpt('swift_temp_url_duration', default=1200, @@ -70,7 +72,8 @@ opts = [ 'Do not include trailing "/". ' 'For example, use "https://swift.example.com". If using RADOS ' 'Gateway, endpoint may also contain /swift path; if it does ' - 'not, it will be appended. Required for temporary URLs.')), + 'not, it will be appended. Used for temporary URLs, will ' + 'be fetched from the service catalog, if not provided.')), cfg.StrOpt( 'swift_api_version', default='v1', @@ -82,9 +85,10 @@ opts = [ help=_('The account that Glance uses to communicate with ' 'Swift. The format is "AUTH_uuid". "uuid" is the ' 'UUID for the account configured in the glance-api.conf. ' - 'Required for temporary URLs when Glance backend is Swift. ' 'For example: "AUTH_a422b2-91f3-2f46-74b7-d7c9e8958f5d30". ' - 'Swift temporary URL format: ' + 'If not set, the default value is calculated based on the ID ' + 'of the project used to access Swift (as set in the [swift] ' + 'section). Swift temporary URL format: ' '"endpoint_url/api_version/[account/]container/object_id"')), cfg.StrOpt( 'swift_container', diff --git a/ironic/tests/unit/common/test_glance_service.py b/ironic/tests/unit/common/test_glance_service.py index b449ee7592..7e135fb403 100644 --- a/ironic/tests/unit/common/test_glance_service.py +++ b/ironic/tests/unit/common/test_glance_service.py @@ -527,6 +527,134 @@ class TestGlanceSwiftTempURL(base.TestCase): key=CONF.glance.swift_temp_url_key, method='GET') + @mock.patch('ironic.common.keystone.get_adapter', autospec=True) + @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) + def test_swift_temp_url_endpoint_detected(self, tempurl_mock, + adapter_mock): + self.config(swift_endpoint_url=None, group='glance') + + path = ('/v1/AUTH_a422b2-91f3-2f46-74b7-d7c9e8958f5d30' + '/glance' + '/757274c4-2856-4bd2-bb20-9a4a231e187b') + tempurl_mock.return_value = ( + path + '?temp_url_sig=hmacsig&temp_url_expires=1400001200') + endpoint = 'http://another.example.com:8080' + adapter_mock.return_value.get_endpoint.return_value = endpoint + + self.service._validate_temp_url_config = mock.Mock() + + temp_url = self.service.swift_temp_url(image_info=self.fake_image) + + self.assertEqual(endpoint + tempurl_mock.return_value, + temp_url) + tempurl_mock.assert_called_with( + path=path, + seconds=CONF.glance.swift_temp_url_duration, + key=CONF.glance.swift_temp_url_key, + method='GET') + + @mock.patch('ironic.common.keystone.get_adapter', autospec=True) + @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) + def test_swift_temp_url_endpoint_with_suffix(self, tempurl_mock, + adapter_mock): + self.config(swift_endpoint_url=None, group='glance') + + path = ('/v1/AUTH_a422b2-91f3-2f46-74b7-d7c9e8958f5d30' + '/glance' + '/757274c4-2856-4bd2-bb20-9a4a231e187b') + tempurl_mock.return_value = ( + path + '?temp_url_sig=hmacsig&temp_url_expires=1400001200') + endpoint = 'http://another.example.com:8080' + adapter_mock.return_value.get_endpoint.return_value = ( + endpoint + '/v1/AUTH_foobar') + + self.service._validate_temp_url_config = mock.Mock() + + temp_url = self.service.swift_temp_url(image_info=self.fake_image) + + self.assertEqual(endpoint + tempurl_mock.return_value, + temp_url) + tempurl_mock.assert_called_with( + path=path, + seconds=CONF.glance.swift_temp_url_duration, + key=CONF.glance.swift_temp_url_key, + method='GET') + + @mock.patch('ironic.common.swift.get_swift_session', autospec=True) + @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) + def test_swift_temp_url_account_detected(self, tempurl_mock, swift_mock): + self.config(swift_account=None, group='glance') + + path = ('/v1/AUTH_42/glance' + '/757274c4-2856-4bd2-bb20-9a4a231e187b') + tempurl_mock.return_value = ( + path + '?temp_url_sig=hmacsig&temp_url_expires=1400001200') + auth_ref = swift_mock.return_value.auth.get_auth_ref.return_value + auth_ref.project_id = '42' + + self.service._validate_temp_url_config = mock.Mock() + + temp_url = self.service.swift_temp_url(image_info=self.fake_image) + + self.assertEqual(CONF.glance.swift_endpoint_url + + tempurl_mock.return_value, + temp_url) + tempurl_mock.assert_called_with( + path=path, + seconds=CONF.glance.swift_temp_url_duration, + key=CONF.glance.swift_temp_url_key, + method='GET') + swift_mock.assert_called_once_with() + + @mock.patch('ironic.common.swift.SwiftAPI', autospec=True) + @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) + def test_swift_temp_url_key_detected(self, tempurl_mock, swift_mock): + self.config(swift_temp_url_key=None, group='glance') + + path = ('/v1/AUTH_a422b2-91f3-2f46-74b7-d7c9e8958f5d30' + '/glance' + '/757274c4-2856-4bd2-bb20-9a4a231e187b') + tempurl_mock.return_value = ( + path + '?temp_url_sig=hmacsig&temp_url_expires=1400001200') + conn = swift_mock.return_value.connection + conn.head_account.return_value = { + 'x-account-meta-temp-url-key': 'secret' + } + + self.service._validate_temp_url_config = mock.Mock() + + temp_url = self.service.swift_temp_url(image_info=self.fake_image) + + self.assertEqual(CONF.glance.swift_endpoint_url + + tempurl_mock.return_value, + temp_url) + tempurl_mock.assert_called_with( + path=path, + seconds=CONF.glance.swift_temp_url_duration, + key='secret', + method='GET') + conn.head_account.assert_called_once_with() + + @mock.patch('ironic.common.swift.SwiftAPI', autospec=True) + @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) + def test_swift_temp_url_no_key_detected(self, tempurl_mock, swift_mock): + self.config(swift_temp_url_key=None, group='glance') + + path = ('/v1/AUTH_a422b2-91f3-2f46-74b7-d7c9e8958f5d30' + '/glance' + '/757274c4-2856-4bd2-bb20-9a4a231e187b') + tempurl_mock.return_value = ( + path + '?temp_url_sig=hmacsig&temp_url_expires=1400001200') + conn = swift_mock.return_value.connection + conn.head_account.return_value = {} + + self.service._validate_temp_url_config = mock.Mock() + + self.assertRaises(exception.InvalidParameterValue, + self.service.swift_temp_url, + image_info=self.fake_image) + conn.head_account.assert_called_once_with() + @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) def test_swift_temp_url_invalid_image_info(self, tempurl_mock): self.service._validate_temp_url_config = mock.Mock() @@ -630,18 +758,14 @@ class TestGlanceSwiftTempURL(base.TestCase): def test__validate_temp_url_config(self): self.service._validate_temp_url_config() + def test__validate_temp_url_key_no_exception(self): + self.config(swift_temp_url_key=None, group='glance') + self.config(object_store_endpoint_type='swift', group='deploy') + self.service._validate_temp_url_config() + def test__validate_temp_url_key_exception(self): self.config(swift_temp_url_key=None, group='glance') - self.assertRaises(exception.MissingParameterValue, - self.service._validate_temp_url_config) - - def test__validate_temp_url_endpoint_config_exception(self): - self.config(swift_endpoint_url=None, group='glance') - self.assertRaises(exception.MissingParameterValue, - self.service._validate_temp_url_config) - - def test__validate_temp_url_account_exception(self): - self.config(swift_account=None, group='glance') + self.config(object_store_endpoint_type='radosgw', group='deploy') self.assertRaises(exception.MissingParameterValue, self.service._validate_temp_url_config) diff --git a/ironic/tests/unit/common/test_swift.py b/ironic/tests/unit/common/test_swift.py index a9dd404743..af340aab1d 100644 --- a/ironic/tests/unit/common/test_swift.py +++ b/ironic/tests/unit/common/test_swift.py @@ -32,7 +32,7 @@ if six.PY3: file = io.BytesIO -@mock.patch.object(swift, '_get_swift_session', autospec=True, +@mock.patch.object(swift, 'get_swift_session', autospec=True, return_value=mock.Mock(verify=False, cert=('spam', 'ham'), timeout=42)) @mock.patch.object(swift_client, 'Connection', autospec=True) diff --git a/releasenotes/notes/default-swift_account-b008d08e85bdf154.yaml b/releasenotes/notes/default-swift_account-b008d08e85bdf154.yaml new file mode 100644 index 0000000000..c09ec78a0e --- /dev/null +++ b/releasenotes/notes/default-swift_account-b008d08e85bdf154.yaml @@ -0,0 +1,15 @@ +features: + - | + If the ``[glance]swift_account`` option is not set, the default value is + now calculated based on the ID of the project used to access the object + store. Previously this option was required. This change does not affect + using RadosGW as an object store backend. + - | + If the ``[glance]swift_temp_url_key`` option is not set, ironic now tries + to fetch the key from the project used to access swift (often + called ``service``). This change does not affect using RadosGW as an + object store backend. + - | + If the ``[glance]swift_endpoint_url`` option is not set, ironic now tries + to fetch the Object Store service URL from the service catalog. The + ``/v1/AUTH_*`` suffix is stripped, if present.