openstack/python-keystoneclient
Code Issues Proposed changes

Properly handle Regions in keystoneclient

Browse Source 
Region name is taken as a parameter but is ignored in all communication
with the service catalog. Currently region can be stored in the token
data and then requests to url functions will return the appropriate
region. This is the wrong approach because there is nothing specific to
the token (or auth_data) that is region specific. Instead region
information should be held by the client.

Closes-Bug: 1147530
Closes-Bug: 1255992
Change-Id: I812aa89c8b4af28e294e63926a7f88e8246fffc5
This commit is contained in:
Jamie Lennox
2013-10-22 09:41:00 +10:00
committed by Dolph Mathews
parent e9b26fc61f
commit d4c06d3035
8 changed files with 325 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files

46
keystoneclient/access.py
View File

@@ -33,21 +33,28 @@ class AccessInfo(dict):
"""
@classmethod
def factory(cls, resp=None, body=None, **kwargs):
def factory(cls, resp=None, body=None, region_name=None, **kwargs):
"""Create AccessInfo object given a successful auth response & body
or a user-provided dict.
"""
# FIXME(jamielennox): Passing region_name is deprecated. Provide an
# appropriate warning.
if body is not None or len(kwargs):
if AccessInfoV3.is_valid(body, **kwargs):
token = None
if resp:
token = resp.headers['X-Subject-Token']
if body:
if region_name:
body['token']['region_name'] = region_name
return AccessInfoV3(token, **body['token'])
else:
return AccessInfoV3(token, **kwargs)
elif AccessInfoV2.is_valid(body, **kwargs):
if body:
if region_name:
body['access']['region_name'] = region_name
return AccessInfoV2(**body['access'])
else:
return AccessInfoV2(**kwargs)
@@ -59,7 +66,11 @@ class AccessInfo(dict):
def __init__(self, *args, **kwargs):
super(AccessInfo, self).__init__(*args, **kwargs)
self.service_catalog = service_catalog.ServiceCatalog.factory(
resource_dict=self, region_name=self.get('region_name'))
resource_dict=self, region_name=self._region_name)
@property
def _region_name(self):
return self.get('region_name')
def will_expire_soon(self, stale_duration=None):
"""Determines if expiration is about to occur.
@@ -272,6 +283,9 @@ class AccessInfo(dict):
request. If the authentication request wasn't scoped to a tenant
(project), this property will return None.
DEPRECATED: this doesn't correctly handle region name. You should fetch
it from the service catalog yourself.
:returns: tuple of urls
"""
raise NotImplementedError()
@@ -282,6 +296,9 @@ class AccessInfo(dict):
associated with the authorization request, or None if the
authentication request wasn't scoped to a tenant (project).
DEPRECATED: this doesn't correctly handle region name. You should fetch
it from the service catalog yourself.
:returns: tuple of urls
"""
raise NotImplementedError()
@@ -306,7 +323,7 @@ class AccessInfoV2(AccessInfo):
self.service_catalog = service_catalog.ServiceCatalog.factory(
resource_dict=self,
token=self['token']['id'],
region_name=self.get('region_name'))
region_name=self._region_name)
@classmethod
def is_valid(cls, body, **kwargs):
@@ -430,17 +447,23 @@ class AccessInfoV2(AccessInfo):
@property
def auth_url(self):
# FIXME(jamielennox): this is deprecated in favour of retrieving it
# from the service catalog. Provide a warning.
if self.service_catalog:
return self.service_catalog.get_urls(service_type='identity',
endpoint_type='publicURL')
endpoint_type='publicURL',
region_name=self._region_name)
else:
return None
@property
def management_url(self):
# FIXME(jamielennox): this is deprecated in favour of retrieving it
# from the service catalog. Provide a warning.
if self.service_catalog:
return self.service_catalog.get_urls(service_type='identity',
endpoint_type='adminURL')
endpoint_type='adminURL',
region_name=self._region_name)
else:
return None
@@ -456,7 +479,7 @@ class AccessInfoV3(AccessInfo):
self.service_catalog = service_catalog.ServiceCatalog.factory(
resource_dict=self,
token=token,
region_name=self.get('region_name'))
region_name=self._region_name)
if token:
self.update(auth_token=token)
@@ -554,16 +577,23 @@ class AccessInfoV3(AccessInfo):
@property
def auth_url(self):
# FIXME(jamielennox): this is deprecated in favour of retrieving it
# from the service catalog. Provide a warning.
if self.service_catalog:
return self.service_catalog.get_urls(service_type='identity',
endpoint_type='public')
endpoint_type='public',
region_name=self._region_name)
else:
return None
@property
def management_url(self):
# FIXME(jamielennox): this is deprecated in favour of retrieving it
# from the service catalog. Provide a warning.
if self.service_catalog:
return self.service_catalog.get_urls(service_type='identity',
endpoint_type='admin')
endpoint_type='admin',
region_name=self._region_name)
else:
return None

38
keystoneclient/httpclient.py
View File

@@ -227,6 +227,7 @@ class HTTPClient(object):
self.project_domain_id = None
self.project_domain_name = None
self.region_name = None
self.auth_url = None
self._endpoint = None
self._management_url = None
@@ -251,6 +252,8 @@ class HTTPClient(object):
self._management_url = self.auth_ref.management_url[0]
self.auth_token = self.auth_ref.auth_token
self.trust_id = self.auth_ref.trust_id
if self.auth_ref.has_service_catalog():
self.region_name = self.auth_ref.service_catalog.region_name
else:
self.auth_ref = None
@@ -307,7 +310,8 @@ class HTTPClient(object):
self.auth_token_from_user = None
if endpoint:
self._endpoint = endpoint.rstrip('/')
self.region_name = region_name
if region_name:
self.region_name = region_name
self.original_ip = original_ip
if cacert:
@@ -383,7 +387,8 @@ class HTTPClient(object):
user_id=None, domain_name=None, domain_id=None,
project_name=None, project_id=None, user_domain_id=None,
user_domain_name=None, project_domain_id=None,
project_domain_name=None, trust_id=None):
project_domain_name=None, trust_id=None,
region_name=None):
"""Authenticate user.
Uses the data provided at instantiation to authenticate against
@@ -442,6 +447,7 @@ class HTTPClient(object):
project_domain_name = project_domain_name or self.project_domain_name
trust_id = trust_id or self.trust_id
region_name = region_name or self.region_name
if not token:
token = self.auth_token_from_user
@@ -470,10 +476,14 @@ class HTTPClient(object):
new_token_needed = True
kwargs['password'] = password
resp, body = self.get_raw_token_from_identity_service(**kwargs)
self.auth_ref = access.AccessInfo.factory(resp, body)
# TODO(jamielennox): passing region_name here is wrong but required
# for backwards compatibility. Deprecate and provide warning.
self.auth_ref = access.AccessInfo.factory(resp, body,
region_name=region_name)
else:
self.auth_ref = auth_ref
self.process_token()
self.process_token(region_name=region_name)
if new_token_needed:
self.store_auth_ref_into_keyring(keyring_key)
return True
@@ -528,7 +538,7 @@ class HTTPClient(object):
except Exception as e:
_logger.warning("Failed to store token into keyring %s" % (e))
def process_token(self):
def process_token(self, region_name=None):
"""Extract and process information from the new auth_ref.
And set the relevant authentication information.
@@ -540,8 +550,14 @@ class HTTPClient(object):
if not self.auth_ref.tenant_id:
raise exceptions.AuthorizationFailure(
"Token didn't provide tenant_id")
if self.auth_ref.management_url:
self._management_url = self.auth_ref.management_url[0]
try:
self._management_url = self.auth_ref.service_catalog.url_for(
service_type='identity',
endpoint_type='admin',
region_name=region_name or self.region_name)
except exceptions.EndpointNotFound:
_logger.warning("Failed to retrieve management_url from token")
self.project_name = self.auth_ref.tenant_name
self.project_id = self.auth_ref.tenant_id
@@ -590,14 +606,6 @@ class HTTPClient(object):
"""
raise NotImplementedError
def _extract_service_catalog(self, url, body):
"""Set the client's service catalog from the response data.
Not implemented here because data returned may be API
version-specific.
"""
raise NotImplementedError
def serialize(self, entity):
return jsonutils.dumps(entity)

57
keystoneclient/service_catalog.py
View File

@@ -37,6 +37,16 @@ class ServiceCatalog(object):
else:
raise NotImplementedError('Unrecognized auth response')
def __init__(self, region_name=None):
self._region_name = region_name
@property
def region_name(self):
# FIXME(jamielennox): Having region_name set on the service catalog
# directly is deprecated. It should instead be provided as a parameter
# to calls made to the service_catalog. Provide appropriate warning.
return self._region_name
@abc.abstractmethod
def get_token(self):
"""Fetch token details from service catalog.
@@ -72,13 +82,15 @@ class ServiceCatalog(object):
service catalog.
"""
def get_endpoints(self, service_type=None, endpoint_type=None):
def get_endpoints(self, service_type=None, endpoint_type=None,
region_name=None):
"""Fetch and filter endpoints for the specified service(s).
Returns endpoints for the specified service (or all) and
that contain the specified type (or all).
Returns endpoints for the specified service (or all) containing
the specified type (or all) and region (or all).
"""
endpoint_type = self._normalize_endpoint_type(endpoint_type)
region_name = region_name or self._region_name
sc = {}
@@ -97,20 +109,20 @@ class ServiceCatalog(object):
if (endpoint_type and not
self._is_endpoint_type_match(endpoint, endpoint_type)):
continue
if (self.region_name and
self.region_name != endpoint.get('region')):
if region_name and region_name != endpoint.get('region'):
continue
sc[st].append(endpoint)
return sc
def _get_service_endpoints(self, attr, filter_value, service_type,
endpoint_type):
endpoint_type, region_name):
"""Fetch the endpoints of a particular service_type and handle
the filtering.
"""
sc_endpoints = self.get_endpoints(service_type=service_type,
endpoint_type=endpoint_type)
endpoint_type=endpoint_type,
region_name=region_name)
try:
endpoints = sc_endpoints[service_type]
@@ -129,7 +141,8 @@ class ServiceCatalog(object):
@abc.abstractmethod
def get_urls(self, attr=None, filter_value=None,
service_type='identity', endpoint_type='publicURL'):
service_type='identity', endpoint_type='publicURL',
region_name=None):
"""Fetch endpoint urls from the service catalog.
Fetch the endpoints from the service catalog for a particular
@@ -150,7 +163,8 @@ class ServiceCatalog(object):
raise NotImplementedError()
def url_for(self, attr=None, filter_value=None,
service_type='identity', endpoint_type='publicURL'):
service_type='identity', endpoint_type='publicURL',
region_name=None):
"""Fetch an endpoint from the service catalog.
Fetch the specified endpoint from the service catalog for
@@ -167,16 +181,19 @@ class ServiceCatalog(object):
urls = self.get_urls(attr=attr,
filter_value=filter_value,
service_type=service_type,
endpoint_type=endpoint_type)
endpoint_type=endpoint_type,
region_name=region_name)
try:
return urls[0]
except Exception:
pass
msg = '%(endpoint)s endpoint for %(service)s not found'
msg = '%(endpoint)s endpoint for %(service)s%(region)s not found'
region = ' in %s region' % region_name if region_name else ''
msg = msg % {'endpoint': endpoint_type,
'service': service_type}
'service': service_type,
'region': region}
raise exceptions.EndpointNotFound(msg)
@@ -199,7 +216,7 @@ class ServiceCatalogV2(ServiceCatalog):
def __init__(self, resource_dict, region_name=None):
self.catalog = resource_dict
self.region_name = region_name
super(ServiceCatalogV2, self).__init__(region_name=region_name)
@classmethod
def is_valid(cls, resource_dict):
@@ -232,12 +249,14 @@ class ServiceCatalogV2(ServiceCatalog):
return token
def get_urls(self, attr=None, filter_value=None,
service_type='identity', endpoint_type='publicURL'):
service_type='identity', endpoint_type='publicURL',
region_name=None):
endpoint_type = self._normalize_endpoint_type(endpoint_type)
endpoints = self._get_service_endpoints(attr=attr,
filter_value=filter_value,
service_type=service_type,
endpoint_type=endpoint_type)
endpoint_type=endpoint_type,
region_name=region_name)
if endpoints:
return tuple([endpoint[endpoint_type] for endpoint in endpoints])
@@ -251,9 +270,9 @@ class ServiceCatalogV3(ServiceCatalog):
"""
def __init__(self, token, resource_dict, region_name=None):
super(ServiceCatalogV3, self).__init__(region_name=region_name)
self._auth_token = token
self.catalog = resource_dict
self.region_name = region_name
@classmethod
def is_valid(cls, resource_dict):
@@ -294,11 +313,13 @@ class ServiceCatalogV3(ServiceCatalog):
return token
def get_urls(self, attr=None, filter_value=None,
service_type='identity', endpoint_type='public'):
service_type='identity', endpoint_type='public',
region_name=None):
endpoints = self._get_service_endpoints(attr=attr,
filter_value=filter_value,
service_type=service_type,
endpoint_type=endpoint_type)
endpoint_type=endpoint_type,
region_name=region_name)
if endpoints:
return tuple([endpoint['url'] for endpoint in endpoints])

22
keystoneclient/tests/v2_0/test_client.py
View File

@@ -130,3 +130,25 @@ class KeystoneClientTest(utils.TestCase):
self.stub_auth(json=second)
cl.authenticate()
self.assertEqual(cl.management_url, second_url % 35357)
@httpretty.activate
def test_client_with_region_name_passes_to_service_catalog(self):
# NOTE(jamielennox): this is deprecated behaviour that should be
# removed ASAP, however must remain compatible.
self.stub_auth(json=client_fixtures.AUTH_RESPONSE_BODY)
cl = client.Client(username='exampleuser',
password='password',
tenant_name='exampleproject',
auth_url=self.TEST_URL,
region_name='North')
self.assertEqual(cl.service_catalog.url_for(service_type='image'),
'https://image.north.host/v1/')
cl = client.Client(username='exampleuser',
password='password',
tenant_name='exampleproject',
auth_url=self.TEST_URL,
region_name='South')
self.assertEqual(cl.service_catalog.url_for(service_type='image'),
'https://image.south.host/v1/')

83
keystoneclient/tests/v2_0/test_service_catalog.py
View File

@@ -77,3 +77,86 @@ class ServiceCatalogTest(utils.TestCase):
auth_ref.service_catalog.url_for,
service_type='image',
endpoint_type='internalURL')
def test_service_catalog_get_endpoints_region_names(self):
auth_ref = access.AccessInfo.factory(None, self.AUTH_RESPONSE_BODY)
sc = auth_ref.service_catalog
endpoints = sc.get_endpoints(service_type='image', region_name='North')
self.assertEqual(len(endpoints), 1)
self.assertEqual(endpoints['image'][0]['publicURL'],
'https://image.north.host/v1/')
endpoints = sc.get_endpoints(service_type='image', region_name='South')
self.assertEqual(len(endpoints), 1)
self.assertEqual(endpoints['image'][0]['publicURL'],
'https://image.south.host/v1/')
endpoints = sc.get_endpoints(service_type='compute')
self.assertEqual(len(endpoints['compute']), 2)
endpoints = sc.get_endpoints(service_type='compute',
region_name='North')
self.assertEqual(len(endpoints['compute']), 2)
endpoints = sc.get_endpoints(service_type='compute',
region_name='West')
self.assertEqual(len(endpoints['compute']), 0)
def test_service_catalog_url_for_region_names(self):
auth_ref = access.AccessInfo.factory(None, self.AUTH_RESPONSE_BODY)
sc = auth_ref.service_catalog
url = sc.url_for(service_type='image', region_name='North')
self.assertEqual(url, 'https://image.north.host/v1/')
url = sc.url_for(service_type='image', region_name='South')
self.assertEqual(url, 'https://image.south.host/v1/')
url = sc.url_for(service_type='compute',
region_name='North',
attr='versionId',
filter_value='1.1')
self.assertEqual(url, 'https://compute.north.host/v1.1/3456')
self.assertRaises(exceptions.EndpointNotFound, sc.url_for,
service_type='image', region_name='West')
def test_servcie_catalog_get_url_region_names(self):
auth_ref = access.AccessInfo.factory(None, self.AUTH_RESPONSE_BODY)
sc = auth_ref.service_catalog
urls = sc.get_urls(service_type='image')
self.assertEqual(len(urls), 2)
urls = sc.get_urls(service_type='image', region_name='North')
self.assertEqual(len(urls), 1)
self.assertEqual(urls[0], 'https://image.north.host/v1/')
urls = sc.get_urls(service_type='image', region_name='South')
self.assertEqual(len(urls), 1)
self.assertEqual(urls[0], 'https://image.south.host/v1/')
urls = sc.get_urls(service_type='image', region_name='West')
self.assertEqual(urls, None)
def test_service_catalog_param_overrides_body_region(self):
self.AUTH_RESPONSE_BODY['access']['region_name'] = "North"
auth_ref = access.AccessInfo.factory(None, self.AUTH_RESPONSE_BODY)
sc = auth_ref.service_catalog
url = sc.url_for(service_type='image')
self.assertEqual(url, 'https://image.north.host/v1/')
url = sc.url_for(service_type='image', region_name='South')
self.assertEqual(url, 'https://image.south.host/v1/')
endpoints = sc.get_endpoints(service_type='image')
self.assertEqual(len(endpoints['image']), 1)
self.assertEqual(endpoints['image'][0]['publicURL'],
'https://image.north.host/v1/')
endpoints = sc.get_endpoints(service_type='image', region_name='South')
self.assertEqual(len(endpoints['image']), 1)
self.assertEqual(endpoints['image'][0]['publicURL'],
'https://image.south.host/v1/')

23
keystoneclient/tests/v3/test_client.py
View File

@@ -169,3 +169,26 @@ class KeystoneClientTest(utils.TestCase):
self.stub_auth(json=second)
cl.authenticate()
self.assertEqual(cl.management_url, second_url % 35357)
@httpretty.activate
def test_client_with_region_name_passes_to_service_catalog(self):
# NOTE(jamielennox): this is deprecated behaviour that should be
# removed ASAP, however must remain compatible.
self.stub_auth(json=client_fixtures.AUTH_RESPONSE_BODY)
cl = client.Client(username='exampleuser',
password='password',
tenant_name='exampleproject',
auth_url=self.TEST_URL,
region_name='North')
self.assertEqual(cl.service_catalog.url_for(service_type='image'),
'http://glance.north.host/glanceapi/public')
cl = client.Client(username='exampleuser',
password='password',
tenant_name='exampleproject',
auth_url=self.TEST_URL,
region_name='South')
self.assertEqual(cl.service_catalog.url_for(service_type='image'),
'http://glance.south.host/glanceapi/public')

95
keystoneclient/tests/v3/test_service_catalog.py
View File

@@ -28,6 +28,20 @@ class ServiceCatalogTest(utils.TestCase):
"headers": client_fixtures.AUTH_RESPONSE_HEADERS
})
self.north_endpoints = {'public':
'http://glance.north.host/glanceapi/public',
'internal':
'http://glance.north.host/glanceapi/internal',
'admin':
'http://glance.north.host/glanceapi/admin'}
self.south_endpoints = {'public':
'http://glance.south.host/glanceapi/public',
'internal':
'http://glance.south.host/glanceapi/internal',
'admin':
'http://glance.south.host/glanceapi/admin'}
def test_building_a_service_catalog(self):
auth_ref = access.AccessInfo.factory(self.RESPONSE,
self.AUTH_RESPONSE_BODY)
@@ -82,3 +96,84 @@ class ServiceCatalogTest(utils.TestCase):
auth_ref.service_catalog.url_for,
service_type='image',
endpoint_type='internalURL')
def test_service_catalog_get_endpoints_region_names(self):
auth_ref = access.AccessInfo.factory(None, self.AUTH_RESPONSE_BODY)
sc = auth_ref.service_catalog
endpoints = sc.get_endpoints(service_type='image', region_name='North')
self.assertEqual(len(endpoints), 1)
for endpoint in endpoints['image']:
self.assertEqual(endpoint['url'],
self.north_endpoints[endpoint['interface']])
endpoints = sc.get_endpoints(service_type='image', region_name='South')
self.assertEqual(len(endpoints), 1)
for endpoint in endpoints['image']:
self.assertEqual(endpoint['url'],
self.south_endpoints[endpoint['interface']])
endpoints = sc.get_endpoints(service_type='compute')
self.assertEqual(len(endpoints['compute']), 3)
endpoints = sc.get_endpoints(service_type='compute',
region_name='North')
self.assertEqual(len(endpoints['compute']), 3)
endpoints = sc.get_endpoints(service_type='compute',
region_name='West')
self.assertEqual(len(endpoints['compute']), 0)
def test_service_catalog_url_for_region_names(self):
auth_ref = access.AccessInfo.factory(None, self.AUTH_RESPONSE_BODY)
sc = auth_ref.service_catalog
url = sc.url_for(service_type='image', region_name='North')
self.assertEqual(url, self.north_endpoints['public'])
url = sc.url_for(service_type='image', region_name='South')
self.assertEqual(url, self.south_endpoints['public'])
self.assertRaises(exceptions.EndpointNotFound, sc.url_for,
service_type='image', region_name='West')
def test_servcie_catalog_get_url_region_names(self):
auth_ref = access.AccessInfo.factory(None, self.AUTH_RESPONSE_BODY)
sc = auth_ref.service_catalog
urls = sc.get_urls(service_type='image')
self.assertEqual(len(urls), 2)
urls = sc.get_urls(service_type='image', region_name='North')
self.assertEqual(len(urls), 1)
self.assertEqual(urls[0], self.north_endpoints['public'])
urls = sc.get_urls(service_type='image', region_name='South')
self.assertEqual(len(urls), 1)
self.assertEqual(urls[0], self.south_endpoints['public'])
urls = sc.get_urls(service_type='image', region_name='West')
self.assertEqual(urls, None)
def test_service_catalog_param_overrides_body_region(self):
self.AUTH_RESPONSE_BODY['token']['region_name'] = "North"
auth_ref = access.AccessInfo.factory(None, self.AUTH_RESPONSE_BODY)
sc = auth_ref.service_catalog
url = sc.url_for(service_type='image')
self.assertEqual(url, self.north_endpoints['public'])
url = sc.url_for(service_type='image', region_name='South')
self.assertEqual(url, self.south_endpoints['public'])
endpoints = sc.get_endpoints(service_type='image')
self.assertEqual(len(endpoints['image']), 3)
for endpoint in endpoints['image']:
self.assertEqual(endpoint['url'],
self.north_endpoints[endpoint['interface']])
endpoints = sc.get_endpoints(service_type='image', region_name='South')
self.assertEqual(len(endpoints['image']), 3)
for endpoint in endpoints['image']:
self.assertEqual(endpoint['url'],
self.south_endpoints[endpoint['interface']])

4
keystoneclient/v3/client.py
View File

@@ -106,12 +106,12 @@ class Client(httpclient.HTTPClient):
def serialize(self, entity):
return jsonutils.dumps(entity, sort_keys=True)
def process_token(self):
def process_token(self, **kwargs):
"""Extract and process information from the new auth_ref.
And set the relevant authentication information.
"""
super(Client, self).process_token()
super(Client, self).process_token(**kwargs)
if self.auth_ref.domain_scoped:
if not self.auth_ref.domain_id:
raise exceptions.AuthorizationFailure(