From 6e60538095c98854f45dc8fbef244d18f4ad479f Mon Sep 17 00:00:00 2001 From: "jaypipes@gmail.com" <> Date: Wed, 27 Jul 2011 14:15:47 -0400 Subject: [PATCH 1/7] Add tests for bad URI parsing and get_backend_class() --- glance/store/__init__.py | 23 +++++++++++++---------- glance/store/filesystem.py | 5 +++-- glance/store/s3.py | 12 ++++++++++++ glance/store/swift.py | 17 ++++++++++++++++- tests/unit/test_store_location.py | 28 +++++++++++++++++++++++++++- 5 files changed, 71 insertions(+), 14 deletions(-) diff --git a/glance/store/__init__.py b/glance/store/__init__.py index 23e74b7e97..fb263c56d6 100644 --- a/glance/store/__init__.py +++ b/glance/store/__init__.py @@ -59,22 +59,25 @@ def get_backend_class(backend): :param backend: Name of backend to create """ # NOTE(sirp): avoiding circular import - from glance.store.http import HTTPBackend - from glance.store.s3 import S3Backend - from glance.store.swift import SwiftBackend - from glance.store.filesystem import FilesystemBackend + import glance.store.http + import glance.store.s3 + import glance.store.swift + import glance.store.filesystem BACKENDS = { - "file": FilesystemBackend, - "http": HTTPBackend, - "https": HTTPBackend, - "swift": SwiftBackend, - "s3": S3Backend} + "filesystem": glance.store.filesystem.FilesystemBackend, + "http": glance.store.http.HTTPBackend, + "swift": glance.store.swift.SwiftBackend, + "s3": glance.store.s3.S3Backend} try: return BACKENDS[backend] except KeyError: - raise UnsupportedBackend("No backend found for '%s'" % backend) + # Total hack... this will go away with refactor-stores + try: + return BACKENDS[location.SCHEME_TO_STORE_MAP[backend]] + except KeyError: + raise UnsupportedBackend("No backend found for '%s'" % backend) def get_from_backend(uri, **kwargs): diff --git a/glance/store/filesystem.py b/glance/store/filesystem.py index 8041049377..0dcbb4ce64 100644 --- a/glance/store/filesystem.py +++ b/glance/store/filesystem.py @@ -30,7 +30,8 @@ import glance.store.location logger = logging.getLogger('glance.store.filesystem') -glance.store.location.add_scheme_map({'file': 'filesystem'}) +glance.store.location.add_scheme_map({'file': 'filesystem', + 'filesystem': 'filesystem'}) class StoreLocation(glance.store.location.StoreLocation): @@ -51,7 +52,7 @@ class StoreLocation(glance.store.location.StoreLocation): versions of Python. """ pieces = urlparse.urlparse(uri) - assert pieces.scheme == 'file' + assert pieces.scheme in ('file', 'filesystem') self.scheme = pieces.scheme path = (pieces.netloc + pieces.path).strip() if path == '': diff --git a/glance/store/s3.py b/glance/store/s3.py index a25a2e46ae..d6abe469c8 100644 --- a/glance/store/s3.py +++ b/glance/store/s3.py @@ -72,6 +72,18 @@ class StoreLocation(glance.store.location.StoreLocation): which is entirely retarded, and breaks urlparse miserably. This function works around that issue. """ + # Make sure that URIs that contain multiple schemes, such as: + # swift://user:pass@http://authurl.com/v1/container/obj + # are immediately rejected. + if uri.count('://') != 1: + reason = ("URI Cannot contain more than one occurrence of a " + "scheme. If you have specified a " + "URI like s3://user:pass@https://s3.amazonaws.com/" + "bucket/key, you need to change it to use the " + "s3+https:// scheme, like so: " + "s3+https://user:pass@s3.amazonaws.com/bucket/key") + raise exception.BadStoreUri(uri, reason) + pieces = urlparse.urlparse(uri) assert pieces.scheme in ('s3', 's3+http', 's3+https') self.scheme = pieces.scheme diff --git a/glance/store/swift.py b/glance/store/swift.py index 5be18c5db7..ef9258123b 100644 --- a/glance/store/swift.py +++ b/glance/store/swift.py @@ -44,10 +44,13 @@ class StoreLocation(glance.store.location.StoreLocation): the following: swift://user:pass@authurl.com/container/obj-id + swift://account:user:pass@authurl.com/container/obj-id swift+http://user:pass@authurl.com/container/obj-id swift+https://user:pass@authurl.com/container/obj-id - The swift+https:// URIs indicate there is an HTTPS authentication URL + The swift+http:// URIs indicate there is an HTTP authentication URL. + The default for Swift is an HTTPS authentication URL, so swift:// and + swift+https:// are the same... """ def process_specs(self): @@ -80,6 +83,18 @@ class StoreLocation(glance.store.location.StoreLocation): swift://account:user:pass@authurl.com/container/obj """ + # Make sure that URIs that contain multiple schemes, such as: + # swift://user:pass@http://authurl.com/v1/container/obj + # are immediately rejected. + if uri.count('://') != 1: + reason = ("URI Cannot contain more than one occurrence of a " + "scheme. If you have specified a " + "URI like swift://user:pass@http://authurl.com/v1/" + "container/obj, you need to change it to use the " + "swift+http:// scheme, like so: " + "swift+http://user:pass@authurl.com/v1/container/obj") + raise exception.BadStoreUri(uri, reason) + pieces = urlparse.urlparse(uri) assert pieces.scheme in ('swift', 'swift+http', 'swift+https') self.scheme = pieces.scheme diff --git a/tests/unit/test_store_location.py b/tests/unit/test_store_location.py index a08f4905d0..e0087933e1 100644 --- a/tests/unit/test_store_location.py +++ b/tests/unit/test_store_location.py @@ -185,6 +185,9 @@ class TestStoreLocation(unittest.TestCase): bad_uri = 'swift://user@example.com:8080/images/1' self.assertRaises(exception.BadStoreUri, loc.parse_uri, bad_uri) + bad_uri = 'swift://user:pass@http://example.com:8080/images/1' + self.assertRaises(exception.BadStoreUri, loc.parse_uri, bad_uri) + def test_s3_store_location(self): """ Test the specific StoreLocation for the S3 store @@ -233,7 +236,7 @@ class TestStoreLocation(unittest.TestCase): self.assertEqual("pass/withslash", loc.secretkey) self.assertEqual(uri, loc.get_uri()) - bad_uri = 'swif://' + bad_uri = 's://' self.assertRaises(Exception, loc.parse_uri, bad_uri) bad_uri = 's3://' @@ -241,3 +244,26 @@ class TestStoreLocation(unittest.TestCase): bad_uri = 's3://accesskey@example.com:8080/images/1' self.assertRaises(exception.BadStoreUri, loc.parse_uri, bad_uri) + + bad_uri = 's3://user:pass@http://example.com:8080/images/1' + self.assertRaises(exception.BadStoreUri, loc.parse_uri, bad_uri) + + def test_get_backend_class(self): + """ + Test that the backend returned by glance.store.get_backend_class + is correct or raises an appropriate error. + """ + good_results = { + 'swift': glance.store.swift.SwiftBackend, + 'swift+http': glance.store.swift.SwiftBackend, + 'swift+https': glance.store.swift.SwiftBackend, + 's3': glance.store.s3.S3Backend, + 's3+http': glance.store.s3.S3Backend, + 's3+https': glance.store.s3.S3Backend, + 'file': glance.store.filesystem.FilesystemBackend, + 'filesystem': glance.store.filesystem.FilesystemBackend, + 'http': glance.store.http.HTTPBackend, + 'https': glance.store.http.HTTPBackend} + + for scheme, store in good_results.items(): + self.assertEqual(glance.store.get_backend_class(scheme), store) From 225347561ad0086f7faa2a6a3381501df1c75d6c Mon Sep 17 00:00:00 2001 From: "jaypipes@gmail.com" <> Date: Wed, 27 Jul 2011 14:17:59 -0400 Subject: [PATCH 2/7] Add tests for bad schemes passed to get_backend_class() --- tests/unit/test_store_location.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/unit/test_store_location.py b/tests/unit/test_store_location.py index e0087933e1..537472ac0e 100644 --- a/tests/unit/test_store_location.py +++ b/tests/unit/test_store_location.py @@ -267,3 +267,10 @@ class TestStoreLocation(unittest.TestCase): for scheme, store in good_results.items(): self.assertEqual(glance.store.get_backend_class(scheme), store) + + bad_results = ['fil', 'swift+h', 'unknown'] + + for store in bad_results: + self.assertRaises(glance.store.UnsupportedBackend, + glance.store.get_backend_class, + store) From 73a5d8e7a4f3f74d00dde5a6f05f8f12164ea402 Mon Sep 17 00:00:00 2001 From: Rick Harris Date: Wed, 27 Jul 2011 19:44:29 -0500 Subject: [PATCH 3/7] Don't tee same image into cache multiple times --- glance/api/v1/images.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/glance/api/v1/images.py b/glance/api/v1/images.py index 54ee7a86be..81c5a5a7a0 100644 --- a/glance/api/v1/images.py +++ b/glance/api/v1/images.py @@ -214,26 +214,30 @@ class Controller(api.BaseController): if cache.enabled: if cache.hit(id): # hit - logger.debug("image cache HIT, retrieving image '%s'" - " from cache", id) + logger.debug("image '%s' is a cache HIT", id) image_iterator = get_from_cache(image, cache) else: # miss - logger.debug("image cache MISS, retrieving image '%s'" - " from store and tee'ing into cache", id) + logger.debug("image '%s' is a cache MISS", id) - # We only want to tee-into the cache if we're not currently - # prefetching an image - image_id = image['id'] - if cache.is_image_currently_prefetching(image_id): + # Make sure we're not already prefetching or caching the image + # that just generated the miss + if cache.is_image_currently_prefetching(id): + logger.debug("image '%s' is already being prefetched," + " not tee'ing into the cache", id) + image_iterator = get_from_store(image) + elif cache.is_image_currently_being_written(id): + logger.debug("image '%s' is already being cached," + " not tee'ing into the cache", id) image_iterator = get_from_store(image) else: # NOTE(sirp): If we're about to download and cache an # image which is currently in the prefetch queue, just # delete the queue items since we're caching it anyway - if cache.is_image_queued_for_prefetch(image_id): - cache.delete_queued_prefetch_image(image_id) + if cache.is_image_queued_for_prefetch(id): + cache.delete_queued_prefetch_image(id) + logger.debug("tee'ing image '%s' into cache", id) image_iterator = get_from_store_tee_into_cache( image, cache) else: From a4f4d6157c4ce1e273d4dc0eafdbe8fb027e1bda Mon Sep 17 00:00:00 2001 From: Rick Harris Date: Wed, 27 Jul 2011 19:50:04 -0500 Subject: [PATCH 4/7] Adding back image_cache_enabled config option for glance-api --- etc/glance-api.conf | 2 ++ 1 file changed, 2 insertions(+) diff --git a/etc/glance-api.conf b/etc/glance-api.conf index 0a0420df4d..db7b7e76f3 100644 --- a/etc/glance-api.conf +++ b/etc/glance-api.conf @@ -53,6 +53,8 @@ swift_store_create_container_on_put = False # ============ Image Cache Options ======================== +image_cache_enabled = False + # Directory that the Image Cache writes data to # Make sure this is also set in glance-pruner.conf image_cache_datadir = /var/lib/glance/image-cache/ From a889225b527482fda639cb04a3c1ac2c0b5c5fb5 Mon Sep 17 00:00:00 2001 From: "jaypipes@gmail.com" <> Date: Thu, 28 Jul 2011 10:02:57 -0400 Subject: [PATCH 5/7] Final fixes merging Rick's swift_auth_url @property with previous URI parsing fixes that were in the S3 bug branch... --- glance/store/swift.py | 20 ++++++++++++++------ tests/unit/test_swift_store.py | 7 ++++++- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/glance/store/swift.py b/glance/store/swift.py index 8527c35dbe..8a7c6ca196 100644 --- a/glance/store/swift.py +++ b/glance/store/swift.py @@ -67,10 +67,15 @@ class StoreLocation(glance.store.location.StoreLocation): return '' def get_uri(self): + authurl = self.authurl + if authurl.startswith('http://'): + authurl = authurl[7:] + elif authurl.startswith('https://'): + authurl = authurl[8:] return "%s://%s%s/%s/%s" % ( self.scheme, self._get_credstring(), - self.authurl, + authurl, self.container, self.obj) @@ -137,9 +142,14 @@ class StoreLocation(glance.store.location.StoreLocation): try: self.obj = path_parts.pop() self.container = path_parts.pop() - self.authurl = netloc - if len(path_parts) > 0: - self.authurl = netloc + '/' + '/'.join(path_parts).strip('/') + if self.scheme == 'swift+http': + self.authurl = 'http://' + else: + self.authurl = '' # default is https anyway + if not netloc.startswith('http'): + # push hostname back into the remaining to build full authurl + path_parts.insert(0, netloc) + self.authurl += '/'.join(path_parts) except IndexError: reason = "Badly formed Swift URI" raise exception.BadStoreUri(uri, reason) @@ -203,8 +213,6 @@ class SwiftBackend(glance.store.Backend): uri = location.get_store_uri() raise exception.NotFound("Swift could not find image at " "uri %(uri)s" % locals()) - else: - raise if expected_size: obj_size = int(resp_headers['content-length']) diff --git a/tests/unit/test_swift_store.py b/tests/unit/test_swift_store.py index 044fc14a82..64425ff01a 100644 --- a/tests/unit/test_swift_store.py +++ b/tests/unit/test_swift_store.py @@ -124,7 +124,9 @@ def stub_out_swift_common_client(stubs): def fake_http_connection(*args, **kwargs): return None - def fake_get_auth(*args, **kwargs): + def fake_get_auth(url, *args, **kwargs): + if 'http' in url and '://' not in url: + raise ValueError('Invalid url %s' % url) return None, None stubs.Set(swift.common.client, @@ -153,6 +155,9 @@ def format_swift_location(user, key, authurl, container, obj): scheme = 'swift+https' if authurl.startswith('http://'): scheme = 'swift+http' + authurl = authurl[7:] + if authurl.startswith('https://'): + authurl = authurl[8:] return "%s://%s:%s@%s/%s/%s" % (scheme, user, key, authurl, container, obj) From 70b938937712f97017356f350ec14349ec4e8bb7 Mon Sep 17 00:00:00 2001 From: "jaypipes@gmail.com" <> Date: Thu, 28 Jul 2011 11:40:04 -0400 Subject: [PATCH 6/7] Re-add else: raise --- glance/store/swift.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/glance/store/swift.py b/glance/store/swift.py index 8a7c6ca196..57ff79022c 100644 --- a/glance/store/swift.py +++ b/glance/store/swift.py @@ -213,6 +213,8 @@ class SwiftBackend(glance.store.Backend): uri = location.get_store_uri() raise exception.NotFound("Swift could not find image at " "uri %(uri)s" % locals()) + else: + raise if expected_size: obj_size = int(resp_headers['content-length']) From a4fe58efec5c5f4da2ad59d7923ff3343c645d46 Mon Sep 17 00:00:00 2001 From: "jaypipes@gmail.com" <> Date: Thu, 28 Jul 2011 13:04:20 -0400 Subject: [PATCH 7/7] Added unit tests for swift_auth_url @property. It was broken. startwith('swift+http') matches swift+https first --- glance/store/swift.py | 16 +++------------- tests/unit/test_store_location.py | 8 ++++++-- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/glance/store/swift.py b/glance/store/swift.py index 57ff79022c..a3cd9af081 100644 --- a/glance/store/swift.py +++ b/glance/store/swift.py @@ -142,14 +142,10 @@ class StoreLocation(glance.store.location.StoreLocation): try: self.obj = path_parts.pop() self.container = path_parts.pop() - if self.scheme == 'swift+http': - self.authurl = 'http://' - else: - self.authurl = '' # default is https anyway if not netloc.startswith('http'): # push hostname back into the remaining to build full authurl path_parts.insert(0, netloc) - self.authurl += '/'.join(path_parts) + self.authurl = '/'.join(path_parts) except IndexError: reason = "Badly formed Swift URI" raise exception.BadStoreUri(uri, reason) @@ -163,16 +159,10 @@ class StoreLocation(glance.store.location.StoreLocation): HTTPS is assumed, unless 'swift+http' is specified. """ - if self.scheme.startswith('swift+http'): - auth_scheme = 'http://' - elif self.scheme.startswith('swift+https'): - auth_scheme = 'https://' - elif self.scheme.startswith('swift'): + if self.scheme in ('swift+https', 'swift'): auth_scheme = 'https://' else: - logger.warn("Unrecognized scheme '%s', defaulting auth url" - " to https", self.scheme) - auth_scheme = 'https://' + auth_scheme = 'http://' full_url = ''.join([auth_scheme, self.authurl]) return full_url diff --git a/tests/unit/test_store_location.py b/tests/unit/test_store_location.py index 537472ac0e..1de6ad3f87 100644 --- a/tests/unit/test_store_location.py +++ b/tests/unit/test_store_location.py @@ -138,6 +138,7 @@ class TestStoreLocation(unittest.TestCase): self.assertEqual("swift", loc.scheme) self.assertEqual("example.com", loc.authurl) + self.assertEqual("https://example.com", loc.swift_auth_url) self.assertEqual("images", loc.container) self.assertEqual("1", loc.obj) self.assertEqual(None, loc.user) @@ -148,6 +149,7 @@ class TestStoreLocation(unittest.TestCase): self.assertEqual("swift+https", loc.scheme) self.assertEqual("authurl.com", loc.authurl) + self.assertEqual("https://authurl.com", loc.swift_auth_url) self.assertEqual("images", loc.container) self.assertEqual("1", loc.obj) self.assertEqual("user", loc.user) @@ -159,17 +161,19 @@ class TestStoreLocation(unittest.TestCase): self.assertEqual("swift+https", loc.scheme) self.assertEqual("authurl.com/v1", loc.authurl) + self.assertEqual("https://authurl.com/v1", loc.swift_auth_url) self.assertEqual("container", loc.container) self.assertEqual("12345", loc.obj) self.assertEqual("user", loc.user) self.assertEqual("pass", loc.key) self.assertEqual(uri, loc.get_uri()) - uri = 'swift://account:user:pass@authurl.com/v1/container/12345' + uri = 'swift+http://account:user:pass@authurl.com/v1/container/12345' loc.parse_uri(uri) - self.assertEqual("swift", loc.scheme) + self.assertEqual("swift+http", loc.scheme) self.assertEqual("authurl.com/v1", loc.authurl) + self.assertEqual("http://authurl.com/v1", loc.swift_auth_url) self.assertEqual("container", loc.container) self.assertEqual("12345", loc.obj) self.assertEqual("account:user", loc.user)