From 9ee28252a4ecb9b79676b2a00ea7b5c6c7e4c936 Mon Sep 17 00:00:00 2001 From: Pavlo Shchelokovskyy Date: Thu, 29 Jun 2017 09:03:18 +0000 Subject: [PATCH] Fix swiftclient creation in the change I52f1386df45ebe0a43b11fe1583e012dfa3af532 we lost most of swiftclient options in a belief that those are handled by the keystoneauth session passed to the swiftclient. In fact though, swiftclient only uses this session to get itself an endpoint and a token, but it has no SessionClient, and does not use that passed in session to make further requests to swift itself. This patch restores all the logic that we had to decompose the session object loaded from config to options that are passed to swiftclient explicitly. Change-Id: I08f382aa9d2ad22f7dbd65f7b54a8dd0a765ba44 Partial-Bug: #1699547 Closes-Bug: #1736158 --- devstack/lib/ironic | 2 +- etc/ironic/ironic.conf.sample | 37 ++++++++++++++++ ironic/common/swift.py | 42 +++++++++++++++---- ironic/conf/swift.py | 4 +- ironic/tests/unit/common/test_swift.py | 19 +++++++-- ...ix-swift-ssl-options-d93d653dcd404960.yaml | 6 +++ .../keystoneauth-config-1baa45a0a2dd93b4.yaml | 4 ++ 7 files changed, 99 insertions(+), 15 deletions(-) create mode 100644 releasenotes/notes/fix-swift-ssl-options-d93d653dcd404960.yaml diff --git a/devstack/lib/ironic b/devstack/lib/ironic index ee8119624b..b90636a41c 100644 --- a/devstack/lib/ironic +++ b/devstack/lib/ironic @@ -1113,7 +1113,7 @@ function configure_ironic_conductor { # TODO(pas-ha) this block is for transition period only, # after all clients are moved to use keystoneauth adapters, # it will be deleted - local sections_with_adapter="service_catalog glance cinder inspector" + local sections_with_adapter="service_catalog glance cinder inspector swift" for conf_section in $sections_with_adapter; do configure_adapter_for $conf_section done diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index a384d7ec90..b8e00cfee6 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -3894,12 +3894,28 @@ # Domain name to scope to (string value) #domain_name = +# Always use this endpoint URL for requests for this client. +# (string value) +#endpoint_override = + # Verify HTTPS connections. (boolean value) #insecure = false # PEM encoded client certificate key file (string value) #keyfile = +# The maximum major version of a given API, intended to be +# used as the upper bound of a range with min_version. +# Mutually exclusive with version. (string value) +#max_version = + +# The minimum major version of a given API, intended to be +# used as the lower bound of a range with max_version. +# Mutually exclusive with version. If min_version is given +# with no max_version it is as if max version is "latest". +# (string value) +#min_version = + # User's password (string value) #password = @@ -3917,6 +3933,18 @@ # Deprecated group/name - [swift]/tenant_name #project_name = +# The default region_name for endpoint URL discovery. (string +# value) +#region_name = + +# The default service_name for endpoint URL discovery. (string +# value) +#service_name = + +# The default service_type for endpoint URL discovery. (string +# value) +#service_type = object-store + # Maximum number of times to retry a Swift request, before # failing. (integer value) #swift_max_retries = 2 @@ -3945,3 +3973,12 @@ # Username (string value) # Deprecated group/name - [swift]/user_name #username = + +# List of interfaces, in order of preference, for endpoint +# URL. (list value) +#valid_interfaces = internal,public + +# Minimum Major API version within a given Major API version +# for endpoint URL discovery. Mutually exclusive with +# min_version and max_version (string value) +#version = diff --git a/ironic/common/swift.py b/ironic/common/swift.py index 8168870eef..6d07ed5386 100644 --- a/ironic/common/swift.py +++ b/ironic/common/swift.py @@ -14,6 +14,7 @@ # License for the specific language governing permissions and limitations # under the License. +import six from six.moves import http_client from six.moves.urllib import parse from swiftclient import client as swift_client @@ -45,16 +46,41 @@ class SwiftAPI(object): :raises: ConfigInvalid if required keystone authorization credentials with swift are missing. """ - params = {} + params = {'retries': CONF.swift.swift_max_retries} if CONF.deploy.object_store_endpoint_type == 'radosgw': - params = {'authurl': CONF.swift.auth_url, - 'user': CONF.swift.username, - 'key': CONF.swift.password} + params.update({'authurl': CONF.swift.auth_url, + 'user': CONF.swift.username, + 'key': CONF.swift.password}) else: - # NOTE(aNuposic): Session will be initiated only when connection - # with swift is initialized. Since v3.2.0 swiftclient supports - # instantiating the API client from keystoneauth session. - params = {'session': _get_swift_session()} + # 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() + adapter = keystone.get_adapter('swift', session=session) + params['os_options'] = { + 'object_storage_url': adapter.get_endpoint()} + # deconstruct back session-related options + params['timeout'] = session.timeout + if session.verify is False: + params['insecure'] = True + elif isinstance(session.verify, six.string_types): + 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) def create_object(self, container, obj, filename, diff --git a/ironic/conf/swift.py b/ironic/conf/swift.py index 66a0b1f5c9..802238b45b 100644 --- a/ironic/conf/swift.py +++ b/ironic/conf/swift.py @@ -29,8 +29,8 @@ opts = [ def register_opts(conf): conf.register_opts(opts, group='swift') - auth.register_auth_opts(conf, 'swift') + auth.register_auth_opts(conf, 'swift', service_type='object-store') def list_opts(): - return auth.add_auth_opts(opts) + return auth.add_auth_opts(opts, service_type='object-store') diff --git a/ironic/tests/unit/common/test_swift.py b/ironic/tests/unit/common/test_swift.py index 0468810b6a..a9dd404743 100644 --- a/ironic/tests/unit/common/test_swift.py +++ b/ironic/tests/unit/common/test_swift.py @@ -32,7 +32,9 @@ 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) class SwiftTestCase(base.TestCase): @@ -42,10 +44,18 @@ class SwiftTestCase(base.TestCase): def test___init__(self, connection_mock, keystone_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( - session=keystone_mock.return_value) + 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'} + ) def test___init___radosgw(self, connection_mock, swift_session_mock): """Check if client is properly initialized with radosgw""" @@ -66,7 +76,8 @@ class SwiftTestCase(base.TestCase): swift.SwiftAPI() params = {'authurl': auth_url, 'user': username, - 'key': password} + 'key': password, + 'retries': 2} connection_mock.assert_called_once_with(**params) self.assertFalse(swift_session_mock.called) diff --git a/releasenotes/notes/fix-swift-ssl-options-d93d653dcd404960.yaml b/releasenotes/notes/fix-swift-ssl-options-d93d653dcd404960.yaml new file mode 100644 index 0000000000..896400d5ec --- /dev/null +++ b/releasenotes/notes/fix-swift-ssl-options-d93d653dcd404960.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixes a bug when SSL-related options in ``[swift]`` section of ironic + configuration file were ignored when performing API requests to Swift. + See https://launchpad.net/bugs/1736158 for more information. diff --git a/releasenotes/notes/keystoneauth-config-1baa45a0a2dd93b4.yaml b/releasenotes/notes/keystoneauth-config-1baa45a0a2dd93b4.yaml index 98f687c8a0..81a3f0753c 100644 --- a/releasenotes/notes/keystoneauth-config-1baa45a0a2dd93b4.yaml +++ b/releasenotes/notes/keystoneauth-config-1baa45a0a2dd93b4.yaml @@ -12,3 +12,7 @@ features: Adds the ability to set keystoneauth settings in the ``[inspector]`` configuration section for service automatic discovery. + - | + Adds the ability to set keystoneauth settings in the + ``[swift]`` configuration section for service automatic + discovery.