From c0e1d5bd2944a011c0cbaba61d284aedec086b60 Mon Sep 17 00:00:00 2001 From: Venkatesh Sampath Date: Fri, 21 Feb 2014 19:33:40 +0530 Subject: [PATCH] Log 'image_id' with all BadStoreURI error messages - handle all BadStoreUri exceptions raised by 'legacy_parse_uri' function in one location so that it becomes easier to log the error message with 'image_id' - add tests for 'legacy_parse_uri' Closes-Bug: #1243704 Change-Id: Ifc5de11832860ed51c1eb359d6f5cf78de8c0ba4 --- .../versions/015_quote_swift_credentials.py | 29 +++---- .../017_quote_encrypted_swift_credentials.py | 27 +++--- glance/tests/unit/test_migrations.py | 84 +++++++++++++++++++ 3 files changed, 112 insertions(+), 28 deletions(-) diff --git a/glance/db/sqlalchemy/migrate_repo/versions/015_quote_swift_credentials.py b/glance/db/sqlalchemy/migrate_repo/versions/015_quote_swift_credentials.py index d617361fa1..1afb3bbd70 100644 --- a/glance/db/sqlalchemy/migrate_repo/versions/015_quote_swift_credentials.py +++ b/glance/db/sqlalchemy/migrate_repo/versions/015_quote_swift_credentials.py @@ -51,14 +51,20 @@ def migrate_location_credentials(migrate_engine, to_quoted): 'swift')).execute()) for image in images: - fixed_uri = legacy_parse_uri(image['location'], to_quoted, - image['id']) - images_table.update()\ - .where(images_table.c.id == image['id'])\ - .values(location=fixed_uri).execute() + try: + fixed_uri = legacy_parse_uri(image['location'], to_quoted) + images_table.update()\ + .where(images_table.c.id == image['id'])\ + .values(location=fixed_uri).execute() + except exception.BadStoreUri as e: + err_msg = _("Invalid store uri for image: %(image_id)s. " + "Details: %(reason)s") % {'image_id': image.id, + 'reason': unicode(e)} + LOG.exception(err_msg) + raise -def legacy_parse_uri(uri, to_quote, image_id): +def legacy_parse_uri(uri, to_quote): """ Parse URLs. This method fixes an issue where credentials specified in the URL are interpreted differently in Python 2.6.1+ than prior @@ -86,8 +92,6 @@ def legacy_parse_uri(uri, to_quote, image_id): "like so: " "swift+http://user:pass@authurl.com/v1/container/obj") - LOG.error(_("Invalid store uri for image %(image_id)s: %(reason)s") % - {'image_id': image_id, 'reason': reason}) raise exception.BadStoreUri(message=reason) pieces = urlparse.urlparse(uri) @@ -120,8 +124,7 @@ def legacy_parse_uri(uri, to_quote, image_id): if len(cred_parts) == 1: reason = (_("Badly formed credentials '%(creds)s' in Swift " "URI") % {'creds': creds}) - LOG.error(reason) - raise exception.BadStoreUri() + raise exception.BadStoreUri(message=reason) elif len(cred_parts) == 3: user = ':'.join(cred_parts[0:2]) else: @@ -132,8 +135,7 @@ def legacy_parse_uri(uri, to_quote, image_id): else: if len(cred_parts) != 2: reason = (_("Badly formed credentials in Swift URI.")) - LOG.debug(reason) - raise exception.BadStoreUri() + raise exception.BadStoreUri(message=reason) user, key = cred_parts user = urllib.unquote(user) key = urllib.unquote(key) @@ -150,8 +152,7 @@ def legacy_parse_uri(uri, to_quote, image_id): auth_or_store_url = '/'.join(path_parts) except IndexError: reason = _("Badly formed S3 URI: %(uri)s") % {'uri': uri} - LOG.error(message=reason) - raise exception.BadStoreUri() + raise exception.BadStoreUri(message=reason) if auth_or_store_url.startswith('http://'): auth_or_store_url = auth_or_store_url[len('http://'):] diff --git a/glance/db/sqlalchemy/migrate_repo/versions/017_quote_encrypted_swift_credentials.py b/glance/db/sqlalchemy/migrate_repo/versions/017_quote_encrypted_swift_credentials.py index 0406ce9250..670bc076ff 100644 --- a/glance/db/sqlalchemy/migrate_repo/versions/017_quote_encrypted_swift_credentials.py +++ b/glance/db/sqlalchemy/migrate_repo/versions/017_quote_encrypted_swift_credentials.py @@ -78,14 +78,19 @@ def migrate_location_credentials(migrate_engine, to_quoted): for image in images: try: - fixed_uri = fix_uri_credentials(image['location'], to_quoted, - image['id']) + fixed_uri = fix_uri_credentials(image['location'], to_quoted) images_table.update()\ .where(images_table.c.id == image['id'])\ .values(location=fixed_uri).execute() except exception.Invalid: msg = _("Failed to decrypt location value for image %(image_id)s") LOG.warn(msg % {'image_id': image['id']}) + except exception.BadStoreUri as e: + err_msg = _("Invalid store uri for image: %(image_id)s. " + "Details: %(reason)s") % {'image_id': image.id, + 'reason': unicode(e)} + LOG.exception(err_msg) + raise def decrypt_location(uri): @@ -96,7 +101,7 @@ def encrypt_location(uri): return crypt.urlsafe_encrypt(CONF.metadata_encryption_key, uri, 64) -def fix_uri_credentials(uri, to_quoted, image_id): +def fix_uri_credentials(uri, to_quoted): """ Fix the given uri's embedded credentials by round-tripping with StoreLocation. @@ -118,10 +123,10 @@ def fix_uri_credentials(uri, to_quoted, image_id): except (TypeError, ValueError) as e: raise exception.Invalid(str(e)) - return legacy_parse_uri(decrypted_uri, to_quoted, image_id) + return legacy_parse_uri(decrypted_uri, to_quoted) -def legacy_parse_uri(uri, to_quote, image_id): +def legacy_parse_uri(uri, to_quote): """ Parse URLs. This method fixes an issue where credentials specified in the URL are interpreted differently in Python 2.6.1+ than prior @@ -148,9 +153,6 @@ def legacy_parse_uri(uri, to_quote, image_id): ", you need to change it to use the swift+http:// scheme, " "like so: " "swift+http://user:pass@authurl.com/v1/container/obj") - - LOG.error(_("Invalid store uri for image %(image_id)s: %(reason)s") % - {'image_id': image_id, 'reason': reason}) raise exception.BadStoreUri(message=reason) pieces = urlparse.urlparse(uri) @@ -183,8 +185,7 @@ def legacy_parse_uri(uri, to_quote, image_id): if len(cred_parts) == 1: reason = (_("Badly formed credentials '%(creds)s' in Swift " "URI") % {'creds': creds}) - LOG.error(reason) - raise exception.BadStoreUri() + raise exception.BadStoreUri(message=reason) elif len(cred_parts) == 3: user = ':'.join(cred_parts[0:2]) else: @@ -195,8 +196,7 @@ def legacy_parse_uri(uri, to_quote, image_id): else: if len(cred_parts) != 2: reason = (_("Badly formed credentials in Swift URI.")) - LOG.debug(reason) - raise exception.BadStoreUri() + raise exception.BadStoreUri(message=reason) user, key = cred_parts user = urllib.unquote(user) key = urllib.unquote(key) @@ -213,8 +213,7 @@ def legacy_parse_uri(uri, to_quote, image_id): auth_or_store_url = '/'.join(path_parts) except IndexError: reason = _("Badly formed S3 URI: %(uri)s") % {'uri': uri} - LOG.error(message=reason) - raise exception.BadStoreUri() + raise exception.BadStoreUri(message=reason) if auth_or_store_url.startswith('http://'): auth_or_store_url = auth_or_store_url[len('http://'):] diff --git a/glance/tests/unit/test_migrations.py b/glance/tests/unit/test_migrations.py index 043d8fcd9a..b9af6def8b 100644 --- a/glance/tests/unit/test_migrations.py +++ b/glance/tests/unit/test_migrations.py @@ -27,6 +27,7 @@ from __future__ import print_function import ConfigParser import datetime +import exceptions import os import pickle import subprocess @@ -40,9 +41,11 @@ from six.moves import xrange import sqlalchemy from glance.common import crypt +from glance.common import exception from glance.common import utils import glance.db.migration as migration import glance.db.sqlalchemy.migrate_repo +from glance.db.sqlalchemy.migrate_repo.schema import from_migration_import from glance.db.sqlalchemy import models from glance.openstack.common import jsonutils from glance.openstack.common import log as logging @@ -712,6 +715,72 @@ class TestMigrations(test_utils.BaseTestCase): if row['name'] == 'ramdisk_id': self.assertEqual(row['value'], str(ids['ramdisk'])) + def _assert_invalid_swift_uri_raises_bad_store_uri(self, + legacy_parse_uri_fn): + invalid_uri = ('swift://http://acct:usr:pass@example.com' + '/container/obj-id') + # URI cannot contain more than one occurrence of a scheme. + self.assertRaises(exception.BadStoreUri, + legacy_parse_uri_fn, + invalid_uri, + True) + + invalid_scheme_uri = ('http://acct:usr:pass@example.com' + '/container/obj-id') + self.assertRaises(exceptions.AssertionError, + legacy_parse_uri_fn, + invalid_scheme_uri, + True) + + invalid_account_missing_uri = 'swift+http://container/obj-id' + # Badly formed S3 URI: swift+http://container/obj-id + self.assertRaises(exception.BadStoreUri, + legacy_parse_uri_fn, + invalid_account_missing_uri, + True) + + invalid_container_missing_uri = ('swift+http://' + 'acct:usr:pass@example.com/obj-id') + # Badly formed S3 URI: swift+http://acct:usr:pass@example.com/obj-id + self.assertRaises(exception.BadStoreUri, + legacy_parse_uri_fn, + invalid_container_missing_uri, + True) + + invalid_object_missing_uri = ('swift+http://' + 'acct:usr:pass@example.com/container') + # Badly formed S3 URI: swift+http://acct:usr:pass@example.com/container + self.assertRaises(exception.BadStoreUri, + legacy_parse_uri_fn, + invalid_object_missing_uri, + True) + + invalid_user_without_pass_uri = ('swift://acctusr@example.com' + '/container/obj-id') + # Badly formed credentials '%(creds)s' in Swift URI + self.assertRaises(exception.BadStoreUri, + legacy_parse_uri_fn, + invalid_user_without_pass_uri, + True) + + # Badly formed credentials in Swift URI. + self.assertRaises(exception.BadStoreUri, + legacy_parse_uri_fn, + invalid_user_without_pass_uri, + False) + + def test_legacy_parse_swift_uri_015(self): + (legacy_parse_uri,) = from_migration_import( + '015_quote_swift_credentials', ['legacy_parse_uri']) + + uri = legacy_parse_uri( + 'swift://acct:usr:pass@example.com/container/obj-id', + True) + self.assertTrue(uri, 'swift://acct%3Ausr:pass@example.com' + '/container/obj-id') + + self._assert_invalid_swift_uri_raises_bad_store_uri(legacy_parse_uri) + def _pre_upgrade_015(self, engine): images = get_table(engine, 'images') unquoted_locations = [ @@ -774,6 +843,21 @@ class TestMigrations(test_utils.BaseTestCase): "columns! image_members table columns: %s" % image_members.c.keys()) + def test_legacy_parse_swift_uri_017(self): + metadata_encryption_key = 'a' * 16 + self.config(metadata_encryption_key=metadata_encryption_key) + + (legacy_parse_uri, encrypt_location) = from_migration_import( + '017_quote_encrypted_swift_credentials', ['legacy_parse_uri', + 'encrypt_location']) + + uri = legacy_parse_uri('swift://acct:usr:pass@example.com' + '/container/obj-id', True) + self.assertTrue(uri, encrypt_location( + 'swift://acct%3Ausr:pass@example.com/container/obj-id')) + + self._assert_invalid_swift_uri_raises_bad_store_uri(legacy_parse_uri) + def _pre_upgrade_017(self, engine): metadata_encryption_key = 'a' * 16 self.config(metadata_encryption_key=metadata_encryption_key)