From 3ce5cb4bf6edd755fbba885666700bd4592a0032 Mon Sep 17 00:00:00 2001 From: Jamie Lennox Date: Tue, 28 Feb 2017 11:51:10 +1100 Subject: [PATCH] Add an allow_version_hack flag to session and identity plugins. Whilst historically we always wanted keystoneauth to do the most permissive thing and allow a versioned or unversioned entry in a service catalog there are now cases where we would prefer to fail when the catalog is misconfigured. This will allow a client to opt out of versioned catalog endpoints to insist that the deployment is correctly configured. Closes-Bug: #1668484 Change-Id: Ided0e0c7409994f703175fe61bd4043b840bcf1e --- keystoneauth1/adapter.py | 8 +- keystoneauth1/identity/base.py | 42 ++++++--- .../unit/identity/test_identity_common.py | 88 +++++++++++++++++++ keystoneauth1/tests/unit/utils.py | 2 +- ...ow_version_hack-flag-9b53b72d9b084c04.yaml | 9 ++ 5 files changed, 137 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/allow_version_hack-flag-9b53b72d9b084c04.yaml diff --git a/keystoneauth1/adapter.py b/keystoneauth1/adapter.py index 6fb59355..57216961 100644 --- a/keystoneauth1/adapter.py +++ b/keystoneauth1/adapter.py @@ -57,6 +57,9 @@ class Adapter(object): :param str client_version: The version of the client that created the adapter. This will be used to create the user_agent. + :param bool allow_version_hack: Allow keystoneauth to hack up catalog + URLS to support older schemes. + (optional, default True) """ client_name = None @@ -68,7 +71,7 @@ class Adapter(object): version=None, auth=None, user_agent=None, connect_retries=None, logger=None, allow={}, additional_headers=None, client_name=None, - client_version=None): + client_version=None, allow_version_hack=None): # NOTE(jamielennox): when adding new parameters to adapter please also # add them to the adapter call in httpclient.HTTPClient.__init__ as # well as to load_adapter_from_argparse below if the argument is @@ -87,6 +90,7 @@ class Adapter(object): self.logger = logger self.allow = allow self.additional_headers = additional_headers or {} + self.allow_version_hack = allow_version_hack if client_name: self.client_name = client_name @@ -104,6 +108,8 @@ class Adapter(object): kwargs.setdefault('region_name', self.region_name) if self.version: kwargs.setdefault('version', self.version) + if self.allow_version_hack is not None: + kwargs.setdefault('allow_version_hack', self.allow_version_hack) def request(self, url, method, **kwargs): endpoint_filter = kwargs.setdefault('endpoint_filter', {}) diff --git a/keystoneauth1/identity/base.py b/keystoneauth1/identity/base.py index 11bdff2b..2c9b4b54 100644 --- a/keystoneauth1/identity/base.py +++ b/keystoneauth1/identity/base.py @@ -159,7 +159,7 @@ class BaseIdentityPlugin(plugin.BaseAuthPlugin): def get_endpoint(self, session, service_type=None, interface=None, region_name=None, service_name=None, version=None, - allow={}, **kwargs): + allow={}, allow_version_hack=True, **kwargs): """Return a valid endpoint for a service. If a valid token is not present then a new one will be fetched using @@ -183,6 +183,9 @@ class BaseIdentityPlugin(plugin.BaseAuthPlugin): endpoint. (optional) :param dict allow: Extra filters to pass when discovering API versions. (optional) + :param bool allow_version_hack: Allow keystoneauth to hack up catalog + URLS to support older schemes. + (optional, default True) :raises keystoneauth1.exceptions.http.HttpError: An error from an invalid HTTP response. @@ -226,19 +229,38 @@ class BaseIdentityPlugin(plugin.BaseAuthPlugin): # other endpoint versions. So we support a list of client defined # situations where we can strip the version component from a URL before # doing discovery. - hacked_url = discover._get_catalog_discover_hack(service_type, url) + if allow_version_hack: + vers_url = discover._get_catalog_discover_hack(service_type, url) + else: + vers_url = url try: - disc = self.get_discovery(session, hacked_url, authenticated=False) + disc = self.get_discovery(session, vers_url, authenticated=False) except (exceptions.DiscoveryFailure, exceptions.HttpError, exceptions.ConnectionError): - # NOTE(jamielennox): Again if we can't contact the server we fall - # back to just returning the URL from the catalog. This may not be - # the best default but we need it for now. - LOG.warning('Failed to contact the endpoint at %s for discovery. ' - 'Fallback to using that endpoint as the base url.', - url) + # NOTE(jamielennox): The logic here is required for backwards + # compatibility. By itself it is not ideal. + + if allow_version_hack: + # NOTE(jamielennox): Again if we can't contact the server we + # fall back to just returning the URL from the catalog. This + # is backwards compatible behaviour and used when there is no + # other choice. Realistically if you have provided a version + # you should be able to rely on that version being returned or + # the request failing. + LOG.warning('Failed to contact the endpoint at %s for ' + 'discovery. Fallback to using that endpoint as ' + 'the base url.', url) + + else: + # NOTE(jamielennox): If you've said no to allow_version_hack + # and you can't determine the actual URL this is a failure + # because we are specifying that the deployment must be up to + # date enough to properly specify a version and keystoneauth + # can't deliver. + return None + else: # NOTE(jamielennox): urljoin allows the url to be relative or even # protocol-less. The additional trailing '/' make urljoin respect @@ -249,7 +271,7 @@ class BaseIdentityPlugin(plugin.BaseAuthPlugin): url = disc.url_for(version, **allow) if url: - url = urllib.parse.urljoin(hacked_url.rstrip('/') + '/', url) + url = urllib.parse.urljoin(vers_url.rstrip('/') + '/', url) return url diff --git a/keystoneauth1/tests/unit/identity/test_identity_common.py b/keystoneauth1/tests/unit/identity/test_identity_common.py index 42741cd6..4e70c36b 100644 --- a/keystoneauth1/tests/unit/identity/test_identity_common.py +++ b/keystoneauth1/tests/unit/identity/test_identity_common.py @@ -562,6 +562,94 @@ class CatalogHackTests(utils.TestCase): self.assertEqual(self.V3_URL, endpoint) + def test_setting_no_discover_hack(self): + v2_disc = fixture.V2Discovery(self.V2_URL) + common_disc = fixture.DiscoveryList(href=self.BASE_URL) + + v2_m = self.stub_url('GET', + ['v2.0'], + base_url=self.BASE_URL, + status_code=200, + json=v2_disc) + + common_m = self.stub_url('GET', + [], + base_url=self.BASE_URL, + status_code=300, + json=common_disc) + + resp_text = uuid.uuid4().hex + + resp_m = self.stub_url('GET', + ['v3', 'path'], + base_url=self.BASE_URL, + status_code=200, + text=resp_text) + + # it doesn't matter that we auth with v2 here, discovery hack is in + # base. All identity endpoints point to v2 urls. + token = fixture.V2Token() + service = token.add_service(self.IDENTITY) + service.add_endpoint(public=self.V2_URL, + admin=self.V2_URL, + internal=self.V2_URL) + + self.stub_url('POST', + ['tokens'], + base_url=self.V2_URL, + json=token) + + v2_auth = identity.V2Password(self.V2_URL, + username=uuid.uuid4().hex, + password=uuid.uuid4().hex) + + sess = session.Session(auth=v2_auth) + + # v2 auth with v2 url doesn't make any discovery calls. + self.assertFalse(v2_m.called) + self.assertFalse(common_m.called) + + # v3 endpoint with hack will strip v2 suffix and call root discovery + endpoint = sess.get_endpoint(service_type=self.IDENTITY, + version=(3, 0), + allow_version_hack=True) + + # got v3 url + self.assertEqual(self.V3_URL, endpoint) + + # only called root discovery. + self.assertFalse(v2_m.called) + self.assertTrue(common_m.called_once) + + # with hack turned off it calls v2 discovery and finds nothing + endpoint = sess.get_endpoint(service_type=self.IDENTITY, + version=(3, 0), + allow_version_hack=False) + self.assertIsNone(endpoint) + + # this one called v2 + self.assertTrue(v2_m.called_once) + self.assertTrue(common_m.called_once) + + # get_endpoint returning None raises EndpointNotFound when requesting + self.assertRaises(exceptions.EndpointNotFound, + sess.get, + '/path', + endpoint_filter={'service_type': 'identity', + 'version': (3, 0), + 'allow_version_hack': False}) + + self.assertFalse(resp_m.called) + + # works when allow_version_hack is set + resp = sess.get('/path', + endpoint_filter={'service_type': 'identity', + 'version': (3, 0), + 'allow_version_hack': True}) + + self.assertTrue(resp_m.called_once) + self.assertEqual(resp_text, resp.text) + class GenericPlugin(plugin.BaseAuthPlugin): diff --git a/keystoneauth1/tests/unit/utils.py b/keystoneauth1/tests/unit/utils.py index a47430ab..d58aed8c 100644 --- a/keystoneauth1/tests/unit/utils.py +++ b/keystoneauth1/tests/unit/utils.py @@ -60,7 +60,7 @@ class TestCase(testtools.TestCase): url = base_url url = url.replace("/?", "?") - self.requests_mock.register_uri(method, url, **kwargs) + return self.requests_mock.register_uri(method, url, **kwargs) def assertRequestBodyIs(self, body=None, json=None): last_request_body = self.requests_mock.last_request.body diff --git a/releasenotes/notes/allow_version_hack-flag-9b53b72d9b084c04.yaml b/releasenotes/notes/allow_version_hack-flag-9b53b72d9b084c04.yaml new file mode 100644 index 00000000..6a5f73dc --- /dev/null +++ b/releasenotes/notes/allow_version_hack-flag-9b53b72d9b084c04.yaml @@ -0,0 +1,9 @@ +--- +features: + - A new flag `allow_version_hack` was added to identity plugins and the + adapter which will allow a client to opt out of making guesses at the + version url page of a service. This means that if a deployment is + misconfigured and the service catalog contains a versioned endpoint that + does not match the requested version the request will fail. This will be + useful in beginning to require correctly deployed catalogs rather than + continue to hide the problem.