From 5c79260971a87a5af002b3eeab82866928e88ac9 Mon Sep 17 00:00:00 2001 From: Monty Taylor Date: Wed, 25 Apr 2018 19:10:40 +0200 Subject: [PATCH] Infer version from old versioned service type aliases The last piece of service type alias support is to handle volumev2, volumev3, workflowv2, workflowv3 and friends. Although it's an annoying scenario, luckily legacy code that uses them has a clear meaning. volumev2, version='3' is just legit not a thing. Needed-By: https://review.openstack.org/564494 Change-Id: Iec09bcb16d8e9b09e09bf12d03c2a55e679ad70c --- keystoneauth1/discover.py | 50 +++++++++++++++- keystoneauth1/exceptions/discovery.py | 32 ++++++++++ keystoneauth1/identity/base.py | 6 +- .../unit/identity/test_identity_common.py | 26 ++++++-- keystoneauth1/tests/unit/test_discovery.py | 59 +++++++++++-------- 5 files changed, 141 insertions(+), 32 deletions(-) diff --git a/keystoneauth1/discover.py b/keystoneauth1/discover.py index 2e79a239..a3be0f94 100644 --- a/keystoneauth1/discover.py +++ b/keystoneauth1/discover.py @@ -220,7 +220,28 @@ def normalize_version_number(version): raise TypeError('Invalid version specified: %s' % version) -def _normalize_version_args(version, min_version, max_version): +def _normalize_version_args( + version, min_version, max_version, service_type=None): + # The sins of our fathers become the blood on our hands. + # If a user requests an old-style service type such as volumev2, then they + # are inherently requesting the major API version 2. It's not a good + # interface, but it's the one that was imposed on the world years ago + # because the client libraries all hid the version discovery document. + # In order to be able to ensure that a user who requests volumev2 does not + # get a block-storage endpoint that only provides v3 of the block-storage + # service, we need to pull the version out of the service_type. The + # service-types-authority will prevent the growth of new monstrosities such + # as this, but in order to move forward without breaking people, we have + # to just cry in the corner while striking ourselves with thorned branches. + # That said, for sure only do this hack for officially known service_types. + if (service_type and + _SERVICE_TYPES.is_known(service_type) and + service_type[-1].isdigit() and + service_type[-2] == 'v'): + implied_version = normalize_version_number(service_type[-1]) + else: + implied_version = None + if version and (min_version or max_version): raise ValueError( "version is mutually exclusive with min_version and max_version") @@ -229,6 +250,12 @@ def _normalize_version_args(version, min_version, max_version): # Explode this into min_version and max_version min_version = normalize_version_number(version) max_version = (min_version[0], LATEST) + if implied_version: + if min_version[0] != implied_version[0]: + raise exceptions.ImpliedVersionMismatch( + service_type=service_type, + implied=implied_version, + given=version_to_string(version)) return min_version, max_version if min_version == 'latest': @@ -257,6 +284,27 @@ def _normalize_version_args(version, min_version, max_version): if None not in (min_version, max_version) and max_version < min_version: raise ValueError("min_version cannot be greater than max_version") + if implied_version: + if min_version: + if min_version[0] != implied_version[0]: + raise exceptions.ImpliedMinVersionMismatch( + service_type=service_type, + implied=implied_version, + given=version_to_string(min_version)) + else: + min_version = implied_version + + # If 'latest' is provided with a versioned service-type like + # volumev2 - the user wants the latest of volumev2, not the latest + # of block-storage. + if max_version and max_version[0] != LATEST: + if max_version[0] != implied_version[0]: + raise exceptions.ImpliedMaxVersionMismatch( + service_type=service_type, + implied=implied_version, + given=version_to_string(max_version)) + else: + max_version = (implied_version[0], LATEST) return min_version, max_version diff --git a/keystoneauth1/exceptions/discovery.py b/keystoneauth1/exceptions/discovery.py index 209009b4..5bc6b06a 100644 --- a/keystoneauth1/exceptions/discovery.py +++ b/keystoneauth1/exceptions/discovery.py @@ -10,10 +10,17 @@ # License for the specific language governing permissions and limitations # under the License. +import os_service_types + from keystoneauth1.exceptions import base +_SERVICE_TYPES = os_service_types.ServiceTypes() + __all__ = ('DiscoveryFailure', + 'ImpliedVersionMismatch', + 'ImpliedMinVersionMismatch', + 'ImpliedMaxVersionMismatch', 'VersionNotAvailable') @@ -23,3 +30,28 @@ class DiscoveryFailure(base.ClientException): class VersionNotAvailable(DiscoveryFailure): message = "Discovery failed. Requested version is not available." + + +class ImpliedVersionMismatch(ValueError): + label = 'version' + + def __init__(self, service_type, implied, given): + super(ImpliedVersionMismatch, self).__init__( + "service_type {service_type} was given which implies" + " major API version {implied} but {label} of" + " {given} was also given. Please update your code" + " to use the official service_type {official_type}.".format( + service_type=service_type, + implied=str(implied[0]), + given=given, + label=self.label, + official_type=_SERVICE_TYPES.get_service_type(service_type), + )) + + +class ImpliedMinVersionMismatch(ImpliedVersionMismatch): + label = 'min_version' + + +class ImpliedMaxVersionMismatch(ImpliedVersionMismatch): + label = 'max_version' diff --git a/keystoneauth1/identity/base.py b/keystoneauth1/identity/base.py index 29124500..d73ef524 100644 --- a/keystoneauth1/identity/base.py +++ b/keystoneauth1/identity/base.py @@ -226,7 +226,7 @@ class BaseIdentityPlugin(plugin.BaseAuthPlugin): allow = allow or {} min_version, max_version = discover._normalize_version_args( - None, min_version, max_version) + None, min_version, max_version, service_type=service_type) # NOTE(jamielennox): if you specifically ask for requests to be sent to # the auth url then we can ignore many of the checks. Typically if you @@ -365,7 +365,7 @@ class BaseIdentityPlugin(plugin.BaseAuthPlugin): # Explode `version` into min_version and max_version - everything below # here uses the latter rather than the former. min_version, max_version = discover._normalize_version_args( - version, min_version, max_version) + version, min_version, max_version, service_type=service_type) # Set discover_versions to False since we're only going to return # a URL. Fetching the microversion data would be needlessly # expensive in the common case. However, discover_versions=False @@ -487,7 +487,7 @@ class BaseIdentityPlugin(plugin.BaseAuthPlugin): # Explode `version` into min_version and max_version - everything below # here uses the latter rather than the former. min_version, max_version = discover._normalize_version_args( - version, min_version, max_version) + version, min_version, max_version, service_type=service_type) # Using functools.partial here just to reduce copy-pasta of params get_endpoint_data = functools.partial( self.get_endpoint_data, diff --git a/keystoneauth1/tests/unit/identity/test_identity_common.py b/keystoneauth1/tests/unit/identity/test_identity_common.py index 0e4ba6a4..b208e546 100644 --- a/keystoneauth1/tests/unit/identity/test_identity_common.py +++ b/keystoneauth1/tests/unit/identity/test_identity_common.py @@ -1053,7 +1053,7 @@ class CommonIdentityTests(object): # We should only try to fetch the versioned discovery url once self.requests_mock.get( - self.TEST_VOLUME.versions['v3'].discovery.public, resps) + self.TEST_VOLUME.versions['v3'].discovery.public + '/', resps) data = a.get_endpoint_data(session=s, service_type='volumev3', @@ -1128,7 +1128,7 @@ class CommonIdentityTests(object): # Fetch v2.0 first - since that doesn't match endpoint optimization, # it should fetch the unversioned endpoint - v2_data = s.get_endpoint_data(service_type='volumev3', + v2_data = s.get_endpoint_data(service_type='block-storage', interface='public', min_version='2.0', max_version='2.latest', @@ -1179,10 +1179,10 @@ class CommonIdentityTests(object): self.requests_mock.get( self.TEST_VOLUME.unversioned.public + '/', json=disc) - # We're requesting version 2 of volumev3 to make sure we + # We're requesting version 2 of block-storage to make sure we # trigger the logic constructing the unversioned endpoint from the # versioned endpoint in the catalog - s.get_endpoint_data(service_type='volumev3', + s.get_endpoint_data(service_type='block-storage', interface='public', min_version='2.0', max_version='2.latest', @@ -1413,6 +1413,15 @@ class V3(CommonIdentityTests, utils.TestCase): internal=self.TEST_VOLUME.versions['v3'].service.internal, region=region) + # Add block-storage as a versioned endpoint so that we can test + # versioned to unversioned inference. + svc = token.add_service('block-storage') + svc.add_standard_endpoints( + admin=self.TEST_VOLUME.versions['v3'].service.admin, + public=self.TEST_VOLUME.versions['v3'].service.public, + internal=self.TEST_VOLUME.versions['v3'].service.internal, + region=region) + svc = token.add_service('baremetal') svc.add_standard_endpoints( internal=self.TEST_BAREMETAL_INTERNAL, @@ -1490,6 +1499,15 @@ class V2(CommonIdentityTests, utils.TestCase): internal=self.TEST_VOLUME.versions['v3'].service.internal, region=region) + # Add block-storage as a versioned endpoint so that we can test + # versioned to unversioned inferance. + svc = token.add_service('block-storage') + svc.add_endpoint( + admin=self.TEST_VOLUME.versions['v3'].service.admin, + public=self.TEST_VOLUME.versions['v3'].service.public, + internal=self.TEST_VOLUME.versions['v3'].service.internal, + region=region) + svc = token.add_service('baremetal') svc.add_endpoint( public=None, admin=None, diff --git a/keystoneauth1/tests/unit/test_discovery.py b/keystoneauth1/tests/unit/test_discovery.py index ffc95c3a..42c88cd8 100644 --- a/keystoneauth1/tests/unit/test_discovery.py +++ b/keystoneauth1/tests/unit/test_discovery.py @@ -323,52 +323,63 @@ class DiscoverUtils(utils.TestCase): def test_version_args(self): """Validate _normalize_version_args.""" - def assert_min_max(in_ver, in_min, in_max, out_min, out_max): + def assert_min_max(in_ver, in_min, in_max, in_type, out_min, out_max): self.assertEqual( (out_min, out_max), - discover._normalize_version_args(in_ver, in_min, in_max)) + discover._normalize_version_args( + in_ver, in_min, in_max, service_type=in_type)) - def normalize_raises(ver, min, max): - self.assertRaises(ValueError, - discover._normalize_version_args, ver, min, max) + def normalize_raises(ver, min, max, in_type): + self.assertRaises( + ValueError, + discover._normalize_version_args, + ver, min, max, service_type=in_type) - assert_min_max(None, None, None, + assert_min_max(None, None, None, None, None, None) - assert_min_max(None, None, 'v1.2', + assert_min_max(None, None, 'v1.2', None, None, (1, 2)) - assert_min_max(None, 'v1.2', 'latest', + assert_min_max(None, 'v1.2', 'latest', None, (1, 2), (discover.LATEST, discover.LATEST)) - assert_min_max(None, 'v1.2', '1.6', + assert_min_max(None, 'v1.2', '1.6', None, (1, 2), (1, 6)) - assert_min_max(None, 'v1.2', '1.latest', + assert_min_max(None, 'v1.2', '1.latest', None, (1, 2), (1, discover.LATEST)) - assert_min_max(None, 'latest', 'latest', + assert_min_max(None, 'latest', 'latest', None, (discover.LATEST, discover.LATEST), (discover.LATEST, discover.LATEST)) - assert_min_max(None, 'latest', None, + assert_min_max(None, 'latest', None, None, (discover.LATEST, discover.LATEST), (discover.LATEST, discover.LATEST)) - assert_min_max(None, (1, 2), None, + assert_min_max(None, (1, 2), None, None, (1, 2), (discover.LATEST, discover.LATEST)) - assert_min_max('', ('1', '2'), (1, 6), + assert_min_max('', ('1', '2'), (1, 6), None, (1, 2), (1, 6)) - assert_min_max(None, ('1', '2'), (1, discover.LATEST), + assert_min_max(None, ('1', '2'), (1, discover.LATEST), None, (1, 2), (1, discover.LATEST)) - assert_min_max('v1.2', '', None, + assert_min_max('v1.2', '', None, None, (1, 2), (1, discover.LATEST)) - assert_min_max('1.latest', None, '', + assert_min_max('1.latest', None, '', None, (1, discover.LATEST), (1, discover.LATEST)) - assert_min_max('v1', None, None, + assert_min_max('v1', None, None, None, (1, 0), (1, discover.LATEST)) - assert_min_max('latest', None, None, + assert_min_max('latest', None, None, None, (discover.LATEST, discover.LATEST), (discover.LATEST, discover.LATEST)) + assert_min_max(None, None, 'latest', 'volumev2', + (2, 0), (2, discover.LATEST)) + assert_min_max(None, None, None, 'volumev2', + (2, 0), (2, discover.LATEST)) - normalize_raises('v1', 'v2', None) - normalize_raises('v1', None, 'v2') - normalize_raises(None, 'latest', 'v1') - normalize_raises(None, 'v1.2', 'v1.1') - normalize_raises(None, 'v1.2', 1) + normalize_raises('v1', 'v2', None, None) + normalize_raises('v1', None, 'v2', None) + normalize_raises(None, 'latest', 'v1', None) + normalize_raises(None, 'v1.2', 'v1.1', None) + normalize_raises(None, 'v1.2', 1, None) + normalize_raises('v2', None, None, 'volumev3') + normalize_raises('v3', None, None, 'volumev2') + normalize_raises(None, 'v2', None, 'volumev3') + normalize_raises(None, None, 'v3', 'volumev2') def test_version_to_string(self): def assert_string(out, inp):