From 1d16f64fbf376a956cafed1b3edd8e51ccc16f2c Mon Sep 17 00:00:00 2001 From: Monty Taylor Date: Fri, 13 Oct 2017 16:19:44 -0500 Subject: [PATCH] Handle glance image pagination links better Glance returns pagination links like "/v2/images?marker=". keystoneauth Adapter treats absolute links like that as if they are rooted on the service url. Since the catalog has https://image.example.com/v2 in it, the Adapter will be mounted on that, which means /v2/images will be treated as "https://image.example.com/v2/v2/iamges - which is not correct. Doing a urljoin would also be incorrect in an OpenStack context, because the service could be on a sub-url, such as https://example.com/image, meaning the v2 API endpoint would be https://example.com/image/v2 - which means a straight urljoin of https://example.com/image/v2 and /v2/images?marker= would result in https://example.com/v2/images?marker= which is wrong since it strips the /image path prefix. The most correct things for glance to do would be to return a full absolute url - or a full relative URL. Constructing the absolute url shade-side is hard for the above reasons - we don't have the right context. Strip /v1/ or /v2/ from the front of the next links (yay!) to turn it into a relative url - thereby letting adapter.get(endpoint) do the correct thing. While doing this, update the catalog fixture in the test suite to have a trailing /v2 so that we can be sure we're doing the correct thing in our unit tests. Also add suburl fixtures to ensure that we work with suburls. Change-Id: I6fc8484ed62a029a8ba8ff1d31a37ba69e3000cf --- shade/openstackcloud.py | 10 +- shade/tests/unit/base.py | 13 +- shade/tests/unit/fixtures/catalog-v2.json | 6 +- .../unit/fixtures/catalog-v3-suburl.json | 185 ++++++++++++++++++ .../unit/fixtures/image-version-suburl.json | 64 ++++++ shade/tests/unit/test_image.py | 38 ++++ 6 files changed, 306 insertions(+), 10 deletions(-) create mode 100644 shade/tests/unit/fixtures/catalog-v3-suburl.json create mode 100644 shade/tests/unit/fixtures/image-version-suburl.json diff --git a/shade/openstackcloud.py b/shade/openstackcloud.py index 531a66ba3..227d85474 100644 --- a/shade/openstackcloud.py +++ b/shade/openstackcloud.py @@ -2146,9 +2146,13 @@ class OpenStackCloud( while 'next' in response: image_list.extend(meta.obj_list_to_munch(response['images'])) endpoint = response['next'] - # Use the raw endpoint from the catalog not the one from - # version discovery so that the next links will work right - response = self._raw_image_client.get(endpoint) + # next links from glance have the version prefix. If the catalog + # has a versioned endpoint, then we can't append the next link to + # it. Strip the absolute prefix (/v1/ or /v2/ to turn it into + # a proper relative link. + if endpoint.startswith('/v'): + endpoint = endpoint[4:] + response = self._image_client.get(endpoint) if 'images' in response: image_list.extend(meta.obj_list_to_munch(response['images'])) else: diff --git a/shade/tests/unit/base.py b/shade/tests/unit/base.py index 95f731e36..381bbca70 100644 --- a/shade/tests/unit/base.py +++ b/shade/tests/unit/base.py @@ -454,10 +454,12 @@ class RequestsMockTestCase(BaseTestCase): log_inner_exceptions=True) def get_glance_discovery_mock_dict( - self, image_version_json='image-version.json'): + self, + image_version_json='image-version.json', + image_discovery_url='https://image.example.com/'): discovery_fixture = os.path.join( self.fixtures_directory, image_version_json) - return dict(method='GET', uri='https://image.example.com/', + return dict(method='GET', uri=image_discovery_url, status_code=300, text=open(discovery_fixture, 'r').read()) @@ -473,14 +475,17 @@ class RequestsMockTestCase(BaseTestCase): return dict(method='GET', uri="https://bare-metal.example.com/", text=open(discovery_fixture, 'r').read()) - def use_glance(self, image_version_json='image-version.json'): + def use_glance( + self, image_version_json='image-version.json', + image_discovery_url='https://image.example.com/'): # NOTE(notmorgan): This method is only meant to be used in "setUp" # where the ordering of the url being registered is tightly controlled # if the functionality of .use_glance is meant to be used during an # actual test case, use .get_glance_discovery_mock and apply to the # right location in the mock_uris when calling .register_uris self.__do_register_uris([ - self.get_glance_discovery_mock_dict(image_version_json)]) + self.get_glance_discovery_mock_dict( + image_version_json, image_discovery_url)]) def use_designate(self): # NOTE(slaweq): This method is only meant to be used in "setUp" diff --git a/shade/tests/unit/fixtures/catalog-v2.json b/shade/tests/unit/fixtures/catalog-v2.json index aa8744355..b63669bc9 100644 --- a/shade/tests/unit/fixtures/catalog-v2.json +++ b/shade/tests/unit/fixtures/catalog-v2.json @@ -47,10 +47,10 @@ "endpoints_links": [], "endpoints": [ { - "adminURL": "https://image.example.com", + "adminURL": "https://image.example.com/v2", "region": "RegionOne", - "publicURL": "https://image.example.com", - "internalURL": "https://image.example.com", + "publicURL": "https://image.example.com/v2", + "internalURL": "https://image.example.com/v2", "id": "5a64de3c4a614d8d8f8d1ba3dee5f45f" } ], diff --git a/shade/tests/unit/fixtures/catalog-v3-suburl.json b/shade/tests/unit/fixtures/catalog-v3-suburl.json new file mode 100644 index 000000000..710815d06 --- /dev/null +++ b/shade/tests/unit/fixtures/catalog-v3-suburl.json @@ -0,0 +1,185 @@ +{ + "token": { + "audit_ids": [ + "Rvn7eHkiSeOwucBIPaKdYA" + ], + "catalog": [ + { + "endpoints": [ + { + "id": "32466f357f3545248c47471ca51b0d3a", + "interface": "public", + "region": "RegionOne", + "url": "https://example.com/compute/v2.1/" + } + ], + "name": "nova", + "type": "compute" + }, + { + "endpoints": [ + { + "id": "1e875ca2225b408bbf3520a1b8e1a537", + "interface": "public", + "region": "RegionOne", + "url": "https://example.com/volumev2/v2/1c36b64c840a42cd9e9b931a369337f0" + } + ], + "name": "cinderv2", + "type": "volumev2" + }, + { + "endpoints": [ + { + "id": "5a64de3c4a614d8d8f8d1ba3dee5f45f", + "interface": "public", + "region": "RegionOne", + "url": "https://example.com/image" + } + ], + "name": "glance", + "type": "image" + }, + { + "endpoints": [ + { + "id": "3d15fdfc7d424f3c8923324417e1a3d1", + "interface": "public", + "region": "RegionOne", + "url": "https://example.com/volume/v1/1c36b64c840a42cd9e9b931a369337f0" + } + ], + "name": "cinder", + "type": "volume" + }, + { + "endpoints": [ + { + "id": "4deb4d0504a044a395d4480741ba628c", + "interface": "public", + "region": "RegionOne", + "url": "https://identity.example.com" + }, + { + "id": "012322eeedcd459edabb4933021112bc", + "interface": "admin", + "region": "RegionOne", + "url": "https://example.com/identity" + } + ], + "endpoints_links": [], + "name": "keystone", + "type": "identity" + }, + { + "endpoints": [ + { + "id": "4deb4d0504a044a395d4480741ba628d", + "interface": "public", + "region": "RegionOne", + "url": "https://example.com/example" + } + ], + "endpoints_links": [], + "name": "neutron", + "type": "network" + }, + { + "endpoints": [ + { + "id": "4deb4d0504a044a395d4480741ba628e", + "interface": "public", + "region": "RegionOne", + "url": "https://example.com/container-infra/v1" + } + ], + "endpoints_links": [], + "name": "magnum", + "type": "container-infra" + }, + { + "endpoints": [ + { + "id": "4deb4d0504a044a395d4480741ba628c", + "interface": "public", + "region": "RegionOne", + "url": "https://example.com/object-store/v1/1c36b64c840a42cd9e9b931a369337f0" + } + ], + "endpoints_links": [], + "name": "swift", + "type": "object-store" + }, + { + "endpoints": [ + { + "id": "652f0612744042bfbb8a8bb2c777a16d", + "interface": "public", + "region": "RegionOne", + "url": "https://example.com/bare-metal" + } + ], + "endpoints_links": [], + "name": "ironic", + "type": "baremetal" + }, + { + "endpoints": [ + { + "id": "4deb4d0504a044a395d4480741ba628c", + "interface": "public", + "region": "RegionOne", + "url": "https://example.com/orchestration/v1/1c36b64c840a42cd9e9b931a369337f0" + } + ], + "endpoints_links": [], + "name": "heat", + "type": "orchestration" + }, + { + "endpoints": [ + { + "id": "10c76ffd2b744a67950ed1365190d352", + "interface": "public", + "region": "RegionOne", + "url": "https://example.com/dns" + } + ], + "endpoints_links": [], + "name": "designate", + "type": "dns" + } + ], + "expires_at": "9999-12-31T23:59:59Z", + "issued_at": "2016-12-17T14:25:05.000000Z", + "methods": [ + "password" + ], + "project": { + "domain": { + "id": "default", + "name": "default" + }, + "id": "1c36b64c840a42cd9e9b931a369337f0", + "name": "Default Project" + }, + "roles": [ + { + "id": "9fe2ff9ee4384b1894a90878d3e92bab", + "name": "_member_" + }, + { + "id": "37071fc082e14c2284c32a2761f71c63", + "name": "swiftoperator" + } + ], + "user": { + "domain": { + "id": "default", + "name": "default" + }, + "id": "c17534835f8f42bf98fc367e0bf35e09", + "name": "mordred" + } + } +} diff --git a/shade/tests/unit/fixtures/image-version-suburl.json b/shade/tests/unit/fixtures/image-version-suburl.json new file mode 100644 index 000000000..5ec1a0793 --- /dev/null +++ b/shade/tests/unit/fixtures/image-version-suburl.json @@ -0,0 +1,64 @@ +{ + "versions": [ + { + "status": "CURRENT", + "id": "v2.3", + "links": [ + { + "href": "http://example.com/image/v2/", + "rel": "self" + } + ] + }, + { + "status": "SUPPORTED", + "id": "v2.2", + "links": [ + { + "href": "http://example.com/image/v2/", + "rel": "self" + } + ] + }, + { + "status": "SUPPORTED", + "id": "v2.1", + "links": [ + { + "href": "http://example.com/image/v2/", + "rel": "self" + } + ] + }, + { + "status": "SUPPORTED", + "id": "v2.0", + "links": [ + { + "href": "http://example.com/image/v2/", + "rel": "self" + } + ] + }, + { + "status": "SUPPORTED", + "id": "v1.1", + "links": [ + { + "href": "http://example.com/image/v1/", + "rel": "self" + } + ] + }, + { + "status": "SUPPORTED", + "id": "v1.0", + "links": [ + { + "href": "http://example.com/image/v1/", + "rel": "self" + } + ] + } + ] +} diff --git a/shade/tests/unit/test_image.py b/shade/tests/unit/test_image.py index 2ee6a85b9..97cab614e 100644 --- a/shade/tests/unit/test_image.py +++ b/shade/tests/unit/test_image.py @@ -687,6 +687,44 @@ class TestImage(BaseTestImage): self.assert_calls() +class TestImageSuburl(BaseTestImage): + + def setUp(self): + super(TestImageSuburl, self).setUp() + self.use_keystone_v3(catalog='catalog-v3-suburl.json') + self.use_glance( + image_version_json='image-version-suburl.json', + image_discovery_url='https://example.com/image') + + def test_list_images(self): + self.register_uris([ + dict(method='GET', uri='https://example.com/image/v2/images', + json=self.fake_search_return) + ]) + self.assertEqual( + self.cloud._normalize_images([self.fake_image_dict]), + self.cloud.list_images()) + self.assert_calls() + + def test_list_images_paginated(self): + marker = str(uuid.uuid4()) + self.register_uris([ + dict(method='GET', uri='https://example.com/image/v2/images', + json={'images': [self.fake_image_dict], + 'next': '/v2/images?marker={marker}'.format( + marker=marker)}), + dict(method='GET', + uri=('https://example.com/image/v2/images?' + 'marker={marker}'.format(marker=marker)), + json=self.fake_search_return) + ]) + self.assertEqual( + self.cloud._normalize_images([ + self.fake_image_dict, self.fake_image_dict]), + self.cloud.list_images()) + self.assert_calls() + + class TestImageV1Only(base.RequestsMockTestCase): def setUp(self):