From 2db444bce1caedd6d58aec58288ea9ca3f63c45c Mon Sep 17 00:00:00 2001 From: Steve Baker Date: Mon, 6 Nov 2023 10:01:24 +1300 Subject: [PATCH] Replace swiftclient usage with openstacksdk Object create/delete operations translate clearly from swiftclient to the SDK. Switching the temp URL handling is a little more disruptive but the result is slightly more centralized and enables key rotation. Change-Id: I8df2f032224bd5e540139a798a7ab76a1aeebb06 Closes-Bug: #2042493 --- ironic/common/glance_service/image_service.py | 9 +- ironic/common/swift.py | 151 ++++-------- ironic/conf/swift.py | 13 +- ironic/drivers/modules/inspect_utils.py | 9 +- .../drivers/modules/redfish/firmware_utils.py | 9 +- .../tests/unit/common/test_glance_service.py | 147 ++++++------ ironic/tests/unit/common/test_swift.py | 214 +++++++++--------- .../modules/redfish/test_firmware_utils.py | 16 +- .../drivers/modules/test_inspect_utils.py | 19 +- .../temp_url_key_rot-1e7cb004df8c788f.yaml | 25 ++ requirements.txt | 1 - 11 files changed, 288 insertions(+), 325 deletions(-) create mode 100644 releasenotes/notes/temp_url_key_rot-1e7cb004df8c788f.yaml diff --git a/ironic/common/glance_service/image_service.py b/ironic/common/glance_service/image_service.py index 1d9d6d4bc4..7ef032d778 100644 --- a/ironic/common/glance_service/image_service.py +++ b/ironic/common/glance_service/image_service.py @@ -25,7 +25,6 @@ from glanceclient import client from glanceclient import exc as glance_exc from oslo_log import log from oslo_utils import uuidutils -from swiftclient import utils as swift_utils import tenacity from ironic.common import exception @@ -236,8 +235,9 @@ class GlanceImageService(object): if image_id in self._cache: return self._cache[image_id].url - path = swift_utils.generate_temp_url( - path=path, seconds=seconds, key=key, method=method) + swiftapi = swift.SwiftAPI() + path = swiftapi.generate_temp_url( + path=path, timeout=seconds, temp_url_key=key, method=method) temp_url = '{endpoint_url}{url_path}'.format( endpoint_url=endpoint, url_path=path) @@ -321,8 +321,7 @@ class GlanceImageService(object): if not key: swift_api = swift.SwiftAPI() - key_header = 'x-account-meta-temp-url-key' - key = swift_api.connection.head_account().get(key_header) + key = swift_api.get_temp_url_key() if not key: raise exception.MissingParameterValue(_( diff --git a/ironic/common/swift.py b/ironic/common/swift.py index 87cda4fade..4f1d45cccb 100644 --- a/ironic/common/swift.py +++ b/ironic/common/swift.py @@ -14,18 +14,19 @@ # License for the specific language governing permissions and limitations # under the License. -from http import client as http_client from urllib import parse as urlparse -from swiftclient import client as swift_client -from swiftclient import exceptions as swift_exceptions -from swiftclient import utils as swift_utils +import openstack +from openstack.connection import exceptions as openstack_exc +from oslo_log import log from ironic.common import exception from ironic.common.i18n import _ from ironic.common import keystone from ironic.conf import CONF +LOG = log.getLogger(__name__) + _SWIFT_SESSION = None @@ -40,45 +41,11 @@ 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 - - :raises: ConfigInvalid if required keystone authorization credentials - with swift are missing. - """ - params = {'retries': CONF.swift.swift_max_retries} - # NOTE(pas-ha) swiftclient still (as of 3.3.0) does not use - # (adapter-based) SessionClient, and uses the passed in session - # only to resolve endpoint and get a token, - # but not to make further requests to Swift itself (LP 1736135). - # Thus we need to deconstruct back all the adapter- and - # session-related args as loaded by keystoneauth from config - # to pass them to the client explicitly. - # TODO(pas-ha) re-write this when swiftclient is brought on par - # with other OS clients re auth plugins, sessions and adapters - # support. - # TODO(pas-ha) pass the context here and use token from context - # with service auth - params['session'] = session = get_swift_session() - endpoint = keystone.get_endpoint('swift', session=session) - params['os_options'] = {'object_storage_url': endpoint} - # deconstruct back session-related options - params['timeout'] = session.timeout - if session.verify is False: - params['insecure'] = True - elif isinstance(session.verify, str): - params['cacert'] = session.verify - if session.cert: - # NOTE(pas-ha) although setting cert as path to single file - # with both client cert and key is supported by Session, - # keystoneauth loading always sets the session.cert - # as tuple of cert and key. - params['cert'], params['cert_key'] = session.cert - - self.connection = swift_client.Connection(**params) + """Initialize the connection with swift""" + self.connection = openstack.connection.Connection( + session=get_swift_session(), + oslo_conf=CONF) def create_object(self, container, obj, filename, object_headers=None): @@ -92,29 +59,28 @@ class SwiftAPI(object): :raises: SwiftOperationError, if any operation with Swift fails. """ try: - self.connection.put_container(container) - except swift_exceptions.ClientException as e: + self.connection.create_container(container) + except openstack_exc.OpenStackCloudException as e: operation = _("put container") raise exception.SwiftOperationError(operation=operation, error=e) - with open(filename, "rb") as fileobj: + if object_headers is None: + object_headers = {} - try: - obj_uuid = self.connection.put_object(container, - obj, - fileobj, - headers=object_headers) - except swift_exceptions.ClientException as e: - operation = _("put object") - raise exception.SwiftOperationError(operation=operation, - error=e) + try: + obj_uuid = self.connection.create_object( + container, obj, filename=filename, **object_headers) + except openstack_exc.OpenStackCloudException as e: + operation = _("put object") + raise exception.SwiftOperationError(operation=operation, + error=e) return obj_uuid - def create_object_from_data(self, object, data, container): + def create_object_from_data(self, obj, data, container): """Uploads a given string to Swift. - :param object: The name of the object in Swift + :param obj: The name of the object in Swift :param data: string data to put in the object :param container: The name of the container for the object. Defaults to the value set in the configuration options. @@ -122,20 +88,24 @@ class SwiftAPI(object): :raises: utils.Error, if any operation with Swift fails. """ try: - self.connection.put_container(container) - except swift_exceptions.ClientException as e: + self.connection.create_container(container) + except openstack_exc.OpenStackCloudException as e: operation = _("put container") raise exception.SwiftOperationError(operation=operation, error=e) try: obj_uuid = self.connection.create_object( - container, object, data=data) - except swift_exceptions.ClientException as e: + container, obj, data=data) + except openstack_exc.OpenStackCloudException as e: operation = _("put object") raise exception.SwiftOperationError(operation=operation, error=e) return obj_uuid + def get_temp_url_key(self): + """Get the best temporary url key from the account headers.""" + return self.connection.object_store.get_temp_url_key() + def get_temp_url(self, container, obj, timeout): """Returns the temp url for the given Swift object. @@ -147,27 +117,26 @@ class SwiftAPI(object): :returns: The temp url for the object. :raises: SwiftOperationError, if any operation with Swift fails. """ - try: - account_info = self.connection.head_account() - except swift_exceptions.ClientException as e: - operation = _("head account") - raise exception.SwiftOperationError(operation=operation, - error=e) - - parse_result = urlparse.urlparse(self.connection.url) + endpoint = keystone.get_endpoint('swift', session=get_swift_session()) + parse_result = urlparse.urlparse(endpoint) swift_object_path = '/'.join((parse_result.path, container, obj)) - temp_url_key = account_info.get('x-account-meta-temp-url-key') + temp_url_key = self.get_temp_url_key() if not temp_url_key: raise exception.MissingParameterValue(_( 'Swift temporary URLs require a shared secret to be ' 'created. You must provide pre-generate the key on ' 'the project used to access Swift.')) - url_path = swift_utils.generate_temp_url(swift_object_path, timeout, - temp_url_key, 'GET') + url_path = self.generate_temp_url( + swift_object_path, timeout, 'GET', temp_url_key=temp_url_key) return urlparse.urlunparse( (parse_result.scheme, parse_result.netloc, url_path, None, None, None)) + def generate_temp_url(self, path, timeout, method, temp_url_key): + """Returns the temp url for a given path""" + return self.connection.object_store.generate_temp_url( + path, timeout, method, temp_url_key=temp_url_key) + def get_object(self, object, container): """Downloads a given object from Swift. @@ -178,8 +147,9 @@ class SwiftAPI(object): :raises: utils.Error, if the Swift operation fails. """ try: - obj = self.connection.download_object(object, container=container) - except swift_exceptions.ClientException as e: + headers, obj = self.connection.get_object( + object, container=container) + except openstack_exc.OpenStackCloudException as e: operation = _("get object") raise exception.SwiftOperationError(operation=operation, error=e) @@ -196,42 +166,11 @@ class SwiftAPI(object): """ try: self.connection.delete_object(container, obj) - except swift_exceptions.ClientException as e: + except openstack_exc.OpenStackCloudException as e: operation = _("delete object") - if e.http_status == http_client.NOT_FOUND: + if isinstance(e, openstack_exc.ResourceNotFound): raise exception.SwiftObjectNotFoundError(obj=obj, container=container, operation=operation) raise exception.SwiftOperationError(operation=operation, error=e) - - def head_object(self, container, obj): - """Retrieves the information about the given Swift object. - - :param container: The name of the container in which Swift object - is placed. - :param obj: The name of the object in Swift - :returns: The information about the object as returned by - Swift client's head_object call. - :raises: SwiftOperationError, if operation with Swift fails. - """ - try: - return self.connection.head_object(container, obj) - except swift_exceptions.ClientException as e: - operation = _("head object") - raise exception.SwiftOperationError(operation=operation, error=e) - - def update_object_meta(self, container, obj, object_headers): - """Update the metadata of a given Swift object. - - :param container: The name of the container in which Swift object - is placed. - :param obj: The name of the object in Swift - :param object_headers: the headers for the object to pass to Swift - :raises: SwiftOperationError, if operation with Swift fails. - """ - try: - self.connection.post_object(container, obj, object_headers) - except swift_exceptions.ClientException as e: - operation = _("post object") - raise exception.SwiftOperationError(operation=operation, error=e) diff --git a/ironic/conf/swift.py b/ironic/conf/swift.py index 802238b45b..e1739fa930 100644 --- a/ironic/conf/swift.py +++ b/ironic/conf/swift.py @@ -14,23 +14,12 @@ # License for the specific language governing permissions and limitations # under the License. -from oslo_config import cfg - -from ironic.common.i18n import _ from ironic.conf import auth -opts = [ - cfg.IntOpt('swift_max_retries', - default=2, - help=_('Maximum number of times to retry a Swift request, ' - 'before failing.')) -] - def register_opts(conf): - conf.register_opts(opts, group='swift') auth.register_auth_opts(conf, 'swift', service_type='object-store') def list_opts(): - return auth.add_auth_opts(opts, service_type='object-store') + return auth.add_auth_opts([], service_type='object-store') diff --git a/ironic/drivers/modules/inspect_utils.py b/ironic/drivers/modules/inspect_utils.py index 14caa05491..adea73c1c0 100644 --- a/ironic/drivers/modules/inspect_utils.py +++ b/ironic/drivers/modules/inspect_utils.py @@ -18,7 +18,6 @@ import urllib from oslo_log import log as logging from oslo_utils import netutils -import swiftclient.exceptions from ironic.common import exception from ironic.common import states @@ -91,8 +90,8 @@ def clean_up_swift_entries(task): plugin_obj_name = f'{_OBJECT_NAME_PREFIX}-{task.node.uuid}-plugin' try: swift_api.delete_object(inventory_obj_name, container) - except swiftclient.exceptions.ClientException as e: - if e.http_status != 404: + except exception.SwiftOperationError as e: + if not isinstance(e, exception.SwiftObjectNotFoundError): LOG.error("Object %(obj)s in container %(cont)s with inventory " "for node %(node)s failed to be deleted: %(e)s", {'obj': inventory_obj_name, 'node': task.node.uuid, @@ -101,8 +100,8 @@ def clean_up_swift_entries(task): node=task.node.uuid) try: swift_api.delete_object(plugin_obj_name, container) - except swiftclient.exceptions.ClientException as e: - if e.http_status != 404: + except exception.SwiftOperationError as e: + if not isinstance(e, exception.SwiftObjectNotFoundError): LOG.error("Object %(obj)s in container %(cont)s with plugin data " "for node %(node)s failed to be deleted: %(e)s", {'obj': plugin_obj_name, 'node': task.node.uuid, diff --git a/ironic/drivers/modules/redfish/firmware_utils.py b/ironic/drivers/modules/redfish/firmware_utils.py index 843597d8f7..fb7ba4f7c8 100644 --- a/ironic/drivers/modules/redfish/firmware_utils.py +++ b/ironic/drivers/modules/redfish/firmware_utils.py @@ -302,14 +302,13 @@ def cleanup(node): LOG.debug('For node %(node)s cleaning up files from Swift container ' '%(container)s.', {'node': node.uuid, 'container': container}) - _, objects = swift_api.connection.get_container(container) + objects = swift_api.connection.list_objects(container) for o in objects: - name = o.get('name') - if name and name.startswith(node.uuid): + if o.name and o.name.startswith(node.uuid): try: - swift_api.delete_object(container, name) + swift_api.delete_object(container, o.name) except exception.SwiftOperationError as error: LOG.warning('For node %(node)s failed to clean up ' '%(object)s. Error: %(error)s', - {'node': node.uuid, 'object': name, + {'node': node.uuid, 'object': o.name, 'error': error}) diff --git a/ironic/tests/unit/common/test_glance_service.py b/ironic/tests/unit/common/test_glance_service.py index f9e713e911..ab55aacb42 100644 --- a/ironic/tests/unit/common/test_glance_service.py +++ b/ironic/tests/unit/common/test_glance_service.py @@ -30,6 +30,7 @@ from ironic.common import context from ironic.common import exception from ironic.common.glance_service import image_service from ironic.common.glance_service import service_utils +from ironic.common import swift from ironic.tests import base from ironic.tests.unit import stubs @@ -499,12 +500,13 @@ class TestGlanceSwiftTempURL(base.TestCase): 'id': '757274c4-2856-4bd2-bb20-9a4a231e187b' } - @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) - def test_swift_temp_url(self, tempurl_mock): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_swift_temp_url(self, swift_mock): path = ('/v1/AUTH_a422b2-91f3-2f46-74b7-d7c9e8958f5d30' '/glance' '/757274c4-2856-4bd2-bb20-9a4a231e187b') + tempurl_mock = swift_mock.return_value.generate_temp_url tempurl_mock.return_value = ( path + '?temp_url_sig=hmacsig&temp_url_expires=1400001200') @@ -517,19 +519,21 @@ class TestGlanceSwiftTempURL(base.TestCase): temp_url) tempurl_mock.assert_called_with( path=path, - seconds=CONF.glance.swift_temp_url_duration, - key=CONF.glance.swift_temp_url_key, + timeout=CONF.glance.swift_temp_url_duration, + temp_url_key=CONF.glance.swift_temp_url_key, method='GET') + @mock.patch('ironic.common.swift.get_swift_session', autospec=True) @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): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_swift_temp_url_endpoint_detected(self, swift_mock, + adapter_mock, session_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 = swift_mock.return_value.generate_temp_url tempurl_mock.return_value = ( path + '?temp_url_sig=hmacsig&temp_url_expires=1400001200') endpoint = 'http://another.example.com:8080' @@ -543,19 +547,21 @@ class TestGlanceSwiftTempURL(base.TestCase): temp_url) tempurl_mock.assert_called_with( path=path, - seconds=CONF.glance.swift_temp_url_duration, - key=CONF.glance.swift_temp_url_key, + timeout=CONF.glance.swift_temp_url_duration, + temp_url_key=CONF.glance.swift_temp_url_key, method='GET') + @mock.patch('ironic.common.swift.get_swift_session', autospec=True) @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): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_swift_temp_url_endpoint_with_suffix(self, swift_mock, + adapter_mock, session_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 = swift_mock.return_value.generate_temp_url tempurl_mock.return_value = ( path + '?temp_url_sig=hmacsig&temp_url_expires=1400001200') endpoint = 'http://another.example.com:8080' @@ -570,20 +576,21 @@ class TestGlanceSwiftTempURL(base.TestCase): temp_url) tempurl_mock.assert_called_with( path=path, - seconds=CONF.glance.swift_temp_url_duration, - key=CONF.glance.swift_temp_url_key, + timeout=CONF.glance.swift_temp_url_duration, + temp_url_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): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_swift_temp_url_account_detected(self, swift_mock, session_mock): self.config(swift_account=None, group='glance') path = ('/v1/AUTH_42/glance' '/757274c4-2856-4bd2-bb20-9a4a231e187b') + tempurl_mock = swift_mock.return_value.generate_temp_url 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 = session_mock.return_value.auth.get_auth_ref.return_value auth_ref.project_id = '42' self.service._validate_temp_url_config = mock.Mock() @@ -595,23 +602,24 @@ class TestGlanceSwiftTempURL(base.TestCase): temp_url) tempurl_mock.assert_called_with( path=path, - seconds=CONF.glance.swift_temp_url_duration, - key=CONF.glance.swift_temp_url_key, + timeout=CONF.glance.swift_temp_url_duration, + temp_url_key=CONF.glance.swift_temp_url_key, method='GET') - swift_mock.assert_called_once_with() + session_mock.assert_called_once_with() @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_with_prefix(self, tempurl_mock, - swift_mock): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_swift_temp_url_account_detected_with_prefix(self, swift_mock, + session_mock): self.config(swift_account=None, group='glance') self.config(swift_account_prefix='SWIFTPREFIX', group='glance') path = ('/v1/SWIFTPREFIX_42/glance' '/757274c4-2856-4bd2-bb20-9a4a231e187b') + tempurl_mock = swift_mock.return_value.generate_temp_url 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 = session_mock.return_value.auth.get_auth_ref.return_value auth_ref.project_id = '42' self.service._validate_temp_url_config = mock.Mock() @@ -623,23 +631,24 @@ class TestGlanceSwiftTempURL(base.TestCase): temp_url) tempurl_mock.assert_called_with( path=path, - seconds=CONF.glance.swift_temp_url_duration, - key=CONF.glance.swift_temp_url_key, + timeout=CONF.glance.swift_temp_url_duration, + temp_url_key=CONF.glance.swift_temp_url_key, method='GET') - swift_mock.assert_called_once_with() + session_mock.assert_called_once_with() @mock.patch('ironic.common.swift.get_swift_session', autospec=True) - @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) + @mock.patch.object(swift, 'SwiftAPI', autospec=True) def test_swift_temp_url_account_detected_with_prefix_underscore( - self, tempurl_mock, swift_mock): + self, swift_mock, session_mock): self.config(swift_account=None, group='glance') self.config(swift_account_prefix='SWIFTPREFIX_', group='glance') path = ('/v1/SWIFTPREFIX_42/glance' '/757274c4-2856-4bd2-bb20-9a4a231e187b') + tempurl_mock = swift_mock.return_value.generate_temp_url 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 = session_mock.return_value.auth.get_auth_ref.return_value auth_ref.project_id = '42' self.service._validate_temp_url_config = mock.Mock() @@ -651,25 +660,22 @@ class TestGlanceSwiftTempURL(base.TestCase): temp_url) tempurl_mock.assert_called_with( path=path, - seconds=CONF.glance.swift_temp_url_duration, - key=CONF.glance.swift_temp_url_key, + timeout=CONF.glance.swift_temp_url_duration, + temp_url_key=CONF.glance.swift_temp_url_key, method='GET') - swift_mock.assert_called_once_with() + session_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): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_swift_temp_url_key_detected(self, 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 = swift_mock.return_value.generate_temp_url 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' - } + swift_mock.return_value.get_temp_url_key.return_value = 'secret' self.service._validate_temp_url_config = mock.Mock() @@ -680,35 +686,33 @@ class TestGlanceSwiftTempURL(base.TestCase): temp_url) tempurl_mock.assert_called_with( path=path, - seconds=CONF.glance.swift_temp_url_duration, - key='secret', + timeout=CONF.glance.swift_temp_url_duration, + temp_url_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): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_swift_temp_url_no_key_detected(self, 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 = swift_mock.return_value.generate_temp_url tempurl_mock.return_value = ( path + '?temp_url_sig=hmacsig&temp_url_expires=1400001200') - conn = swift_mock.return_value.connection - conn.head_account.return_value = {} + swift_mock.return_value.get_temp_url_key.return_value = None 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): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_swift_temp_url_invalid_image_info(self, swift_mock): self.service._validate_temp_url_config = mock.Mock() image_info = {} + tempurl_mock = swift_mock.return_value.generate_temp_url self.assertRaises(exception.ImageUnacceptable, self.service.swift_temp_url, image_info) image_info = {'id': 'not an id'} @@ -716,8 +720,8 @@ class TestGlanceSwiftTempURL(base.TestCase): self.service.swift_temp_url, image_info) self.assertFalse(tempurl_mock.called) - @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) - def test_swift_temp_url_multiple_containers(self, tempurl_mock): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_swift_temp_url_multiple_containers(self, swift_mock): self.config(swift_store_multiple_containers_seed=8, group='glance') @@ -725,6 +729,7 @@ class TestGlanceSwiftTempURL(base.TestCase): path = ('/v1/AUTH_a422b2-91f3-2f46-74b7-d7c9e8958f5d30' '/glance_757274c4' '/757274c4-2856-4bd2-bb20-9a4a231e187b') + tempurl_mock = swift_mock.return_value.generate_temp_url tempurl_mock.return_value = ( path + '?temp_url_sig=hmacsig&temp_url_expires=1400001200') @@ -737,8 +742,8 @@ class TestGlanceSwiftTempURL(base.TestCase): temp_url) tempurl_mock.assert_called_with( path=path, - seconds=CONF.glance.swift_temp_url_duration, - key=CONF.glance.swift_temp_url_key, + timeout=CONF.glance.swift_temp_url_duration, + temp_url_key=CONF.glance.swift_temp_url_key, method='GET') def test_swift_temp_url_url_bad_no_info(self): @@ -804,8 +809,8 @@ class TestSwiftTempUrlCache(base.TestCase): self.glance_service = image_service.GlanceImageService( client, context=self.context) - @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) - def test_add_items_to_cache(self, tempurl_mock): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_add_items_to_cache(self, swift_mock): fake_image = { 'id': uuidutils.generate_uuid() } @@ -814,6 +819,7 @@ class TestSwiftTempUrlCache(base.TestCase): '/glance' '/%s' % fake_image['id']) exp_time = int(time.time()) + 1200 + tempurl_mock = swift_mock.return_value.generate_temp_url tempurl_mock.return_value = ( path + '?temp_url_sig=hmacsig&temp_url_expires=%s' % exp_time) @@ -830,14 +836,14 @@ class TestSwiftTempUrlCache(base.TestCase): cleanup_mock.assert_called_once_with() tempurl_mock.assert_called_with( path=path, - seconds=CONF.glance.swift_temp_url_duration, - key=CONF.glance.swift_temp_url_key, + timeout=CONF.glance.swift_temp_url_duration, + temp_url_key=CONF.glance.swift_temp_url_key, method='GET') self.assertEqual((temp_url, exp_time), self.glance_service._cache[fake_image['id']]) - @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) - def test_return_cached_tempurl(self, tempurl_mock): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_return_cached_tempurl(self, swift_mock): fake_image = { 'id': uuidutils.generate_uuid() } @@ -850,6 +856,7 @@ class TestSwiftTempUrlCache(base.TestCase): '?temp_url_sig=hmacsig&temp_url_expires=%(exp_time)s' % {'uuid': fake_image['id'], 'exp_time': exp_time} ) + tempurl_mock = swift_mock.return_value.generate_temp_url self.glance_service._cache[fake_image['id']] = ( image_service.TempUrlCacheElement(url=temp_url, url_expires_at=exp_time) @@ -866,8 +873,8 @@ class TestSwiftTempUrlCache(base.TestCase): cleanup_mock.assert_called_once_with() self.assertFalse(tempurl_mock.called) - @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) - def test_do_not_return_expired_tempurls(self, tempurl_mock): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def test_do_not_return_expired_tempurls(self, swift_mock): fake_image = { 'id': uuidutils.generate_uuid() } @@ -886,6 +893,7 @@ class TestSwiftTempUrlCache(base.TestCase): ) new_exp_time = int(time.time()) + 1200 + tempurl_mock = swift_mock.return_value.generate_temp_url tempurl_mock.return_value = ( path + query % new_exp_time) @@ -899,8 +907,8 @@ class TestSwiftTempUrlCache(base.TestCase): fresh_temp_url) tempurl_mock.assert_called_with( path=path, - seconds=CONF.glance.swift_temp_url_duration, - key=CONF.glance.swift_temp_url_key, + timeout=CONF.glance.swift_temp_url_duration, + temp_url_key=CONF.glance.swift_temp_url_key, method='GET') self.assertEqual( (fresh_temp_url, new_exp_time), @@ -936,11 +944,12 @@ class TestSwiftTempUrlCache(base.TestCase): for uuid in expired_items: self.assertNotIn(uuid, self.glance_service._cache) - @mock.patch('swiftclient.utils.generate_temp_url', autospec=True) - def _test__generate_temp_url(self, fake_image, tempurl_mock): + @mock.patch.object(swift, 'SwiftAPI', autospec=True) + def _test__generate_temp_url(self, fake_image, swift_mock): path = ('/v1/AUTH_a422b2-91f3-2f46-74b7-d7c9e8958f5d30' '/glance' '/%s' % fake_image['id']) + tempurl_mock = swift_mock.return_value.generate_temp_url tempurl_mock.return_value = ( path + '?temp_url_sig=hmacsig&temp_url_expires=1400001200') @@ -958,8 +967,8 @@ class TestSwiftTempUrlCache(base.TestCase): temp_url) tempurl_mock.assert_called_with( path=path, - seconds=CONF.glance.swift_temp_url_duration, - key=CONF.glance.swift_temp_url_key, + timeout=CONF.glance.swift_temp_url_duration, + temp_url_key=CONF.glance.swift_temp_url_key, method='GET') def test_swift_temp_url_cache_enabled(self): diff --git a/ironic/tests/unit/common/test_swift.py b/ironic/tests/unit/common/test_swift.py index 44103ea699..544a749216 100644 --- a/ironic/tests/unit/common/test_swift.py +++ b/ironic/tests/unit/common/test_swift.py @@ -12,155 +12,159 @@ # License for the specific language governing permissions and limitations # under the License. -import builtins -from http import client as http_client -import io from unittest import mock -from oslo_config import cfg -from swiftclient import client as swift_client -from swiftclient import exceptions as swift_exception -from swiftclient import utils as swift_utils +import openstack +from openstack.connection import exceptions as openstack_exc from ironic.common import exception +from ironic.common import keystone from ironic.common import swift +from ironic.conf import CONF from ironic.tests import base -CONF = cfg.CONF - @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) + return_value=mock.MagicMock(verify=False, + cert=('spam', 'ham'), + timeout=42)) +@mock.patch.object(openstack.connection, 'Connection', autospec=True) class SwiftTestCase(base.TestCase): def setUp(self): super(SwiftTestCase, self).setUp() - self.swift_exception = swift_exception.ClientException('', '') + self.swift_exception = openstack_exc.OpenStackCloudException() - def test___init__(self, connection_mock, keystone_mock): + @mock.patch.object(keystone, 'get_auth', autospec=True, + return_value=mock.Mock()) + @mock.patch.object(keystone, 'get_endpoint', autospec=True, + return_value='http://example.com/v1') + def test___init__(self, get_endpoint_mock, get_auth_mock, connection_mock, + session_mock): """Check if client is properly initialized with swift""" - self.config(group='swift', - endpoint_override='http://example.com/objects') - swift.SwiftAPI() - connection_mock.assert_called_once_with( - retries=2, - session=keystone_mock.return_value, - timeout=42, - insecure=True, - cert='spam', - cert_key='ham', - os_options={'object_storage_url': 'http://example.com/objects'} - ) - - @mock.patch.object(builtins, 'open', autospec=True) - def test_create_object(self, open_mock, connection_mock, keystone_mock): swiftapi = swift.SwiftAPI() - connection_obj_mock = connection_mock.return_value - mock_file_handle = mock.MagicMock(spec=io.BytesIO) - mock_file_handle.__enter__.return_value = 'file-object' - open_mock.return_value = mock_file_handle + connection_mock.assert_called_once_with( + session=session_mock.return_value, + oslo_conf=CONF + ) + self.assertEqual(connection_mock.return_value, swiftapi.connection) - connection_obj_mock.put_object.return_value = 'object-uuid' + def test_create_object(self, connection_mock, session_mock): + swiftapi = swift.SwiftAPI() + connection = connection_mock.return_value + connection.create_object.return_value = 'object-uuid' object_uuid = swiftapi.create_object('container', 'object', - 'some-file-location') + 'some-file-location', + object_headers={'foo': 'bar'}) - connection_obj_mock.put_container.assert_called_once_with('container') - connection_obj_mock.put_object.assert_called_once_with( - 'container', 'object', 'file-object', headers=None) + connection.create_container.assert_called_once_with('container') + connection.create_object.assert_called_once_with( + 'container', 'object', filename='some-file-location', foo='bar') self.assertEqual('object-uuid', object_uuid) - @mock.patch.object(builtins, 'open', autospec=True) - def test_create_object_create_container_fails(self, open_mock, - connection_mock, - keystone_mock): + def test_create_object_create_container_fails(self, connection_mock, + session_mock): swiftapi = swift.SwiftAPI() - connection_obj_mock = connection_mock.return_value - connection_obj_mock.put_container.side_effect = self.swift_exception + connection = connection_mock.return_value + connection.create_container.side_effect = self.swift_exception self.assertRaises(exception.SwiftOperationError, swiftapi.create_object, 'container', 'object', 'some-file-location') - connection_obj_mock.put_container.assert_called_once_with('container') - self.assertFalse(connection_obj_mock.put_object.called) + connection.create_container.assert_called_once_with('container') + self.assertFalse(connection.create_object.called) - @mock.patch.object(builtins, 'open', autospec=True) - def test_create_object_put_object_fails(self, open_mock, connection_mock, - keystone_mock): + def test_create_object_create_object_fails(self, connection_mock, + session_mock): swiftapi = swift.SwiftAPI() - mock_file_handle = mock.MagicMock(spec=io.BytesIO) - mock_file_handle.__enter__.return_value = 'file-object' - open_mock.return_value = mock_file_handle - connection_obj_mock = connection_mock.return_value - connection_obj_mock.head_account.side_effect = None - connection_obj_mock.put_object.side_effect = self.swift_exception + connection = connection_mock.return_value + connection.create_object.side_effect = self.swift_exception self.assertRaises(exception.SwiftOperationError, swiftapi.create_object, 'container', 'object', 'some-file-location') - connection_obj_mock.put_container.assert_called_once_with('container') - connection_obj_mock.put_object.assert_called_once_with( - 'container', 'object', 'file-object', headers=None) + connection.create_container.assert_called_once_with('container') + connection.create_object.assert_called_once_with( + 'container', 'object', filename='some-file-location') - @mock.patch.object(swift_utils, 'generate_temp_url', autospec=True) - def test_get_temp_url(self, gen_temp_url_mock, connection_mock, - keystone_mock): + def test_create_object_from_data(self, connection_mock, session_mock): swiftapi = swift.SwiftAPI() - connection_obj_mock = connection_mock.return_value - connection_obj_mock.url = 'http://host/v1/AUTH_tenant_id' - head_ret_val = {'x-account-meta-temp-url-key': 'secretkey'} - connection_obj_mock.head_account.return_value = head_ret_val - gen_temp_url_mock.return_value = 'temp-url-path' + connection = connection_mock.return_value + connection.create_object.return_value = 'object-uuid' + + object_uuid = swiftapi.create_object_from_data( + 'object', 'some-data', 'container') + + connection.create_container.assert_called_once_with('container') + connection.create_object.assert_called_once_with( + 'container', 'object', data='some-data') + self.assertEqual('object-uuid', object_uuid) + + def test_create_object_from_data_create_container_fails( + self, connection_mock, session_mock): + swiftapi = swift.SwiftAPI() + connection = connection_mock.return_value + connection.create_container.side_effect = self.swift_exception + self.assertRaises(exception.SwiftOperationError, + swiftapi.create_object_from_data, + 'object', 'some-data', 'container') + connection.create_container.assert_called_once_with('container') + self.assertFalse(connection.create_object.called) + + def test_create_object_from_data_create_object_fails(self, connection_mock, + session_mock): + swiftapi = swift.SwiftAPI() + connection = connection_mock.return_value + connection.create_object.side_effect = self.swift_exception + self.assertRaises(exception.SwiftOperationError, + swiftapi.create_object_from_data, + 'object', 'some-data', 'container') + connection.create_container.assert_called_once_with('container') + connection.create_object.assert_called_once_with( + 'container', 'object', data='some-data') + + @mock.patch.object(keystone, 'get_auth', autospec=True) + @mock.patch.object(keystone, 'get_endpoint', autospec=True) + def test_get_temp_url(self, get_endpoint_mock, get_auth_mock, + connection_mock, session_mock): + swiftapi = swift.SwiftAPI() + connection = connection_mock.return_value + get_endpoint_mock.return_value = 'http://example.com/v1/AUTH_tenant_id' + object_store = connection.object_store + object_store.get_temp_url_key.return_value = 'secretkey' + + object_store.generate_temp_url.return_value = \ + '/v1/AUTH_tenant_id/temp-url-path' temp_url_returned = swiftapi.get_temp_url('container', 'object', 10) - connection_obj_mock.head_account.assert_called_once_with() object_path_expected = '/v1/AUTH_tenant_id/container/object' - gen_temp_url_mock.assert_called_once_with(object_path_expected, 10, - 'secretkey', 'GET') - self.assertEqual('http://host/temp-url-path', temp_url_returned) + object_store.generate_temp_url.assert_called_once_with( + object_path_expected, 10, 'GET', temp_url_key='secretkey') + self.assertEqual('http://example.com/v1/AUTH_tenant_id/temp-url-path', + temp_url_returned) - def test_delete_object(self, connection_mock, keystone_mock): + def test_delete_object(self, connection_mock, session_mock): swiftapi = swift.SwiftAPI() - connection_obj_mock = connection_mock.return_value + connection = connection_mock.return_value swiftapi.delete_object('container', 'object') - connection_obj_mock.delete_object.assert_called_once_with('container', - 'object') + connection.delete_object.assert_called_once_with('container', + 'object') def test_delete_object_exc_resource_not_found(self, connection_mock, - keystone_mock): + session_mock): swiftapi = swift.SwiftAPI() - exc = swift_exception.ClientException( - "Resource not found", http_status=http_client.NOT_FOUND) - connection_obj_mock = connection_mock.return_value - connection_obj_mock.delete_object.side_effect = exc + exc = openstack_exc.ResourceNotFound(message="Resource not found") + connection = connection_mock.return_value + connection.delete_object.side_effect = exc self.assertRaises(exception.SwiftObjectNotFoundError, swiftapi.delete_object, 'container', 'object') - connection_obj_mock.delete_object.assert_called_once_with('container', - 'object') + connection.delete_object.assert_called_once_with('container', + 'object') - def test_delete_object_exc(self, connection_mock, keystone_mock): + def test_delete_object_exc(self, connection_mock, session_mock): swiftapi = swift.SwiftAPI() - exc = swift_exception.ClientException("Operation error") - connection_obj_mock = connection_mock.return_value - connection_obj_mock.delete_object.side_effect = exc + exc = self.swift_exception + connection = connection_mock.return_value + connection.delete_object.side_effect = exc self.assertRaises(exception.SwiftOperationError, swiftapi.delete_object, 'container', 'object') - connection_obj_mock.delete_object.assert_called_once_with('container', - 'object') - - def test_head_object(self, connection_mock, keystone_mock): - swiftapi = swift.SwiftAPI() - connection_obj_mock = connection_mock.return_value - expected_head_result = {'a': 'b'} - connection_obj_mock.head_object.return_value = expected_head_result - actual_head_result = swiftapi.head_object('container', 'object') - connection_obj_mock.head_object.assert_called_once_with('container', - 'object') - self.assertEqual(expected_head_result, actual_head_result) - - def test_update_object_meta(self, connection_mock, keystone_mock): - swiftapi = swift.SwiftAPI() - connection_obj_mock = connection_mock.return_value - headers = {'a': 'b'} - swiftapi.update_object_meta('container', 'object', headers) - connection_obj_mock.post_object.assert_called_once_with( - 'container', 'object', headers) + connection.delete_object.assert_called_once_with('container', + 'object') diff --git a/ironic/tests/unit/drivers/modules/redfish/test_firmware_utils.py b/ironic/tests/unit/drivers/modules/redfish/test_firmware_utils.py index 0bbc38eba4..2f64b15197 100644 --- a/ironic/tests/unit/drivers/modules/redfish/test_firmware_utils.py +++ b/ironic/tests/unit/drivers/modules/redfish/test_firmware_utils.py @@ -431,8 +431,12 @@ class FirmwareUtilsTestCase(base.TestCase): uuid='55cdaba0-1123-4622-8b37-bb52dd6285d3', driver_internal_info={'firmware_cleanup': ['http', 'swift']}) object_name = '55cdaba0-1123-4622-8b37-bb52dd6285d3/file.exe' - get_container = mock_swift_api.return_value.connection.get_container - get_container.return_value = (mock.Mock(), [{'name': object_name}]) + obj = mock.Mock() + obj.name = object_name + + connection = mock.MagicMock() + mock_swift_api.return_value.connection = connection + connection.list_objects.return_value = [obj] firmware_utils.cleanup(node) @@ -470,8 +474,12 @@ class FirmwareUtilsTestCase(base.TestCase): uuid='55cdaba0-1123-4622-8b37-bb52dd6285d3', driver_internal_info={'firmware_cleanup': ['swift']}) object_name = '55cdaba0-1123-4622-8b37-bb52dd6285d3/file.exe' - get_container = mock_swift_api.return_value.connection.get_container - get_container.return_value = (mock.Mock(), [{'name': object_name}]) + obj = mock.Mock() + obj.name = object_name + + connection = mock.MagicMock() + mock_swift_api.return_value.connection = connection + connection.list_objects.return_value = [obj] mock_swift_api.return_value.delete_object.side_effect =\ exception.SwiftOperationError diff --git a/ironic/tests/unit/drivers/modules/test_inspect_utils.py b/ironic/tests/unit/drivers/modules/test_inspect_utils.py index a7b2ac2fde..52cfd9bad8 100644 --- a/ironic/tests/unit/drivers/modules/test_inspect_utils.py +++ b/ironic/tests/unit/drivers/modules/test_inspect_utils.py @@ -18,7 +18,6 @@ from unittest import mock from oslo_utils import importutils from oslo_utils import uuidutils -import swiftclient.exceptions from ironic.common import context as ironic_context from ironic.common import exception @@ -125,10 +124,8 @@ class SwiftCleanUp(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: swift_obj_mock.delete_object.side_effect = [ - swiftclient.exceptions.ClientException("not found", - http_status=404), - swiftclient.exceptions.ClientException("not found", - http_status=404)] + exception.SwiftObjectNotFoundError("not found"), + exception.SwiftObjectNotFoundError("not found")] utils.clean_up_swift_entries(task) @mock.patch.object(swift, 'SwiftAPI', autospec=True) @@ -140,10 +137,8 @@ class SwiftCleanUp(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: swift_obj_mock.delete_object.side_effect = [ - swiftclient.exceptions.ClientException("failed", - http_status=417), - swiftclient.exceptions.ClientException("not found", - http_status=404)] + exception.SwiftOperationError("failed"), + exception.SwiftObjectNotFoundError("not found")] self.assertRaises(exception.SwiftObjectStillExists, utils.clean_up_swift_entries, task) @@ -156,10 +151,8 @@ class SwiftCleanUp(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: swift_obj_mock.delete_object.side_effect = [ - swiftclient.exceptions.ClientException("failed", - http_status=417), - swiftclient.exceptions.ClientException("failed", - http_status=417)] + exception.SwiftOperationError("failed"), + exception.SwiftOperationError("failed")] self.assertRaises((exception.SwiftObjectStillExists, exception.SwiftObjectStillExists), utils.clean_up_swift_entries, task) diff --git a/releasenotes/notes/temp_url_key_rot-1e7cb004df8c788f.yaml b/releasenotes/notes/temp_url_key_rot-1e7cb004df8c788f.yaml new file mode 100644 index 0000000000..1c185b715d --- /dev/null +++ b/releasenotes/notes/temp_url_key_rot-1e7cb004df8c788f.yaml @@ -0,0 +1,25 @@ +--- +features: + - | + Previously the key for building temporary URLs from Swift was taken from the + `x-account-meta-temp-url-key` header in the object store account. Now the + header `x-account-meta-temp-url-key-2` is also checked, which allows + password rotation to occur without breaking old URLs. + + This applies to the following temporary URL scenarios: + + * Temp URL image transfer from Glance + (when `[glance]swift_temp_url_key` is not set) + * Publishing an image with the Swift publisher + (`[redfish]use_swift=True` or `[ilo]use_web_server_for_images=False`) + * Storing the config drive in Swift + (`[deploy]configdrive_use_object_store=True`) + * Fetching Swift stored firmware update payloads. +upgrade: + - | + `python-swiftclient` is no longer a dependency, all OpenStack Swift + operations are now down using `openstacksdk`. + + Configuration option `[swift]swift_max_retries` has been removed and any + custom value will no longer have any effect on failed object-store + operations. \ No newline at end of file diff --git a/requirements.txt b/requirements.txt index aa0e212b30..7df883dc07 100644 --- a/requirements.txt +++ b/requirements.txt @@ -15,7 +15,6 @@ python-cinderclient!=4.0.0,>=3.3.0 # Apache-2.0 python-glanceclient>=2.8.0 # Apache-2.0 keystoneauth1>=4.2.0 # Apache-2.0 ironic-lib>=5.5.0 # Apache-2.0 -python-swiftclient>=3.2.0 # Apache-2.0 pytz>=2013.6 # MIT stevedore>=1.29.0 # Apache-2.0 oslo.concurrency>=4.2.0 # Apache-2.0