Better glance exception handling

- Adds a conversion step that turns glance exceptions into nova
  exceptions
- Converts Invalid to HTTPBadRequest in /images and /images/detail,
  fixing bug 944846
- Makes stub glance client return glance exceptions instead of nova
  exceptions
- Rebased off of http://review.openstack.org/4788 to pick up
  MissingCredentialError handling as well, and added a test
- A few small, miscellaneous testing fixes for issues I noticed

Change-Id: I88eebfe7a7ac21cc5cd84ad84d64b311ddccf91e

5 files changed, 124 insertions, 21 deletions

diff --git a/nova/api/openstack/compute/images.py b/nova/api/openstack/compute/images.py
index 1a957bc56..eceee6651 100644
--- a/nova/api/openstack/compute/images.py
+++ b/nova/api/openstack/compute/images.py
@@ -171,8 +171,11 @@ class Controller(wsgi.Controller):
         for key, val in page_params.iteritems():
             params[key] = val
 
-        images = self._image_service.index(context, filters=filters,
-                                           **page_params)
+        try:
+            images = self._image_service.index(context, filters=filters,
+                                               **page_params)
+        except exception.Invalid as e:
+            raise webob.exc.HTTPBadRequest(explanation=str(e))
         return self._view_builder.index(req, images)
 
     @wsgi.serializers(xml=ImagesTemplate)
@@ -188,8 +191,11 @@ class Controller(wsgi.Controller):
         page_params = common.get_pagination_params(req)
         for key, val in page_params.iteritems():
             params[key] = val
-        images = self._image_service.detail(context, filters=filters,
-                                            **page_params)
+        try:
+            images = self._image_service.detail(context, filters=filters,
+                                                **page_params)
+        except exception.Invalid as e:
+            raise webob.exc.HTTPBadRequest(explanation=str(e))
 
         return self._view_builder.detail(req, images)
 
diff --git a/nova/image/glance.py b/nova/image/glance.py
index 5edab2655..d924b2c41 100644
--- a/nova/image/glance.py
+++ b/nova/image/glance.py
@@ -23,6 +23,7 @@ import copy
 import datetime
 import json
 import random
+import sys
 import time
 import urlparse
 
@@ -202,7 +203,10 @@ class GlanceImageService(object):
 
     def _fetch_images(self, fetch_func, **kwargs):
         """Paginate through results from glance server"""
-        images = fetch_func(**kwargs)
+        try:
+            images = fetch_func(**kwargs)
+        except Exception:
+            _reraise_translated_exception()
 
         if not images:
             # break out of recursive loop to end pagination
@@ -234,8 +238,8 @@ class GlanceImageService(object):
         try:
             image_meta = self._call_retry(context, 'get_image_meta',
                                           image_id)
-        except glance_exception.NotFound:
-            raise exception.ImageNotFound(image_id=image_id)
+        except Exception:
+            _reraise_translated_image_exception(image_id)
 
         if not self._is_image_available(context, image_meta):
             raise exception.ImageNotFound(image_id=image_id)
@@ -256,8 +260,8 @@ class GlanceImageService(object):
         try:
             image_meta, image_chunks = self._call_retry(context, 'get_image',
                                                         image_id)
-        except glance_exception.NotFound:
-            raise exception.ImageNotFound(image_id=image_id)
+        except Exception:
+            _reraise_translated_image_exception(image_id)
 
         for chunk in image_chunks:
             data.write(chunk)
@@ -296,14 +300,11 @@ class GlanceImageService(object):
         # NOTE(vish): show is to check if image is available
         self.show(context, image_id)
         image_meta = self._translate_to_glance(image_meta)
+        client = self._get_client(context)
         try:
-            client = self._get_client(context)
             image_meta = client.update_image(image_id, image_meta, data)
-        except glance_exception.NotFound:
-            raise exception.ImageNotFound(image_id=image_id)
-        # NOTE(vish): this gets raised for public images
-        except glance_exception.MissingCredentialError:
-            raise exception.ImageNotAuthorized(image_id=image_id)
+        except Exception:
+            _reraise_translated_image_exception(image_id)
 
         base_image_meta = self._translate_from_glance(image_meta)
         return base_image_meta
@@ -468,3 +469,39 @@ def _remove_read_only(image_meta):
         if attr in output:
             del output[attr]
     return output
+
+
+def _reraise_translated_image_exception(image_id):
+    """Transform the exception for the image but keep its traceback intact."""
+    exc_type, exc_value, exc_trace = sys.exc_info()
+    new_exc = _translate_image_exception(image_id, exc_type, exc_value)
+    raise new_exc, None, exc_trace
+
+
+def _reraise_translated_exception():
+    """Transform the exception but keep its traceback intact."""
+    exc_type, exc_value, exc_trace = sys.exc_info()
+    new_exc = _translate_plain_exception(exc_type, exc_value)
+    raise new_exc, None, exc_trace
+
+
+def _translate_image_exception(image_id, exc_type, exc_value):
+    if exc_type in (glance_exception.NotAuthorized,
+                    glance_exception.MissingCredentialError):
+        return exception.ImageNotAuthorized(image_id=image_id)
+    if exc_type is glance_exception.NotFound:
+        return exception.ImageNotFound(image_id=image_id)
+    if exc_type is glance_exception.Invalid:
+        return exception.Invalid(exc_value)
+    return exc_value
+
+
+def _translate_plain_exception(exc_type, exc_value):
+    if exc_type in (glance_exception.NotAuthorized,
+                    glance_exception.MissingCredentialError):
+        return exception.NotAuthorized(exc_value)
+    if exc_type is glance_exception.NotFound:
+        return exception.NotFound(exc_value)
+    if exc_type is glance_exception.Invalid:
+        return exception.Invalid(exc_value)
+    return exc_value
diff --git a/nova/tests/api/openstack/compute/test_images.py b/nova/tests/api/openstack/compute/test_images.py
index 092485a10..0bcae01ce 100644
--- a/nova/tests/api/openstack/compute/test_images.py
+++ b/nova/tests/api/openstack/compute/test_images.py
@@ -25,6 +25,7 @@ import urlparse
 from lxml import etree
 import webob
 
+from nova import exception
 from nova import flags
 from nova.api.openstack.compute import images
 from nova.api.openstack.compute.views import images as images_view
@@ -903,10 +904,10 @@ class ImagesControllerTest(test.TestCase):
         request = fakes.HTTPRequest.blank('/v2/images?status=ACTIVE&'
                                           'UNSUPPORTEDFILTER=testname')
         context = request.environ['nova.context']
-        image_service.detail(context, filters=filters).AndReturn([])
+        image_service.index(context, filters=filters).AndReturn([])
         self.mox.ReplayAll()
         controller = images.Controller(image_service=image_service)
-        controller.detail(request)
+        controller.index(request)
 
     def test_image_no_filters(self):
         image_service = self.mox.CreateMockAnything()
@@ -918,6 +919,16 @@ class ImagesControllerTest(test.TestCase):
         controller = images.Controller(image_service=image_service)
         controller.index(request)
 
+    def test_image_invalid_marker(self):
+        class InvalidImageService(object):
+
+            def index(self, *args, **kwargs):
+                raise exception.Invalid('meow')
+
+        request = fakes.HTTPRequest.blank('/v2/images?marker=invalid')
+        controller = images.Controller(image_service=InvalidImageService())
+        self.assertRaises(webob.exc.HTTPBadRequest, controller.index, request)
+
     def test_image_detail_filter_with_name(self):
         image_service = self.mox.CreateMockAnything()
         filters = {'name': 'testname'}
@@ -1018,6 +1029,16 @@ class ImagesControllerTest(test.TestCase):
         controller = images.Controller(image_service=image_service)
         controller.detail(request)
 
+    def test_image_detail_invalid_marker(self):
+        class InvalidImageService(object):
+
+            def detail(self, *args, **kwargs):
+                raise exception.Invalid('meow')
+
+        request = fakes.HTTPRequest.blank('/v2/images?marker=invalid')
+        controller = images.Controller(image_service=InvalidImageService())
+        self.assertRaises(webob.exc.HTTPBadRequest, controller.detail, request)
+
     def test_generate_alternate_link(self):
         view = images_view.ViewBuilder()
         request = fakes.HTTPRequest.blank('/v2/fake/images/1')
diff --git a/nova/tests/glance/stubs.py b/nova/tests/glance/stubs.py
index 5c0f4f05e..ccf431b69 100644
--- a/nova/tests/glance/stubs.py
+++ b/nova/tests/glance/stubs.py
@@ -17,6 +17,8 @@
 import copy
 import StringIO
 
+from glance.common import exception as glance_exception
+
 from nova import exception
 from nova.image import glance
 
@@ -103,7 +105,7 @@ class StubGlanceClient(object):
         for image in self.images:
             if image['id'] == str(image_id):
                 return image
-        raise exception.ImageNotFound(image_id=image_id)
+        raise glance_exception.NotFound()
 
     #TODO(bcwaldon): implement filters
     def get_images_detailed(self, filters=None, marker=None, limit=3):
@@ -114,6 +116,8 @@ class StubGlanceClient(object):
                 if image['id'] == str(marker):
                     index += 1
                     break
+            else:
+                raise glance_exception.Invalid()
 
         return self.images[index:index + limit]
 
@@ -143,11 +147,11 @@ class StubGlanceClient(object):
                     metadata['id'] = str(metadata['id'])
                 self.images[i].update(metadata)
                 return self.images[i]
-        raise exception.ImageNotFound(image_id=image_id)
+        raise glance_exception.NotFound()
 
     def delete_image(self, image_id):
         for i, image in enumerate(self.images):
             if image['id'] == image_id:
                 del self.images[i]
                 return
-        raise exception.ImageNotFound(image_id=image_id)
+        raise glance_exception.NotFound()
diff --git a/nova/tests/image/test_glance.py b/nova/tests/image/test_glance.py
index 6d965756a..6ed4c501f 100644
--- a/nova/tests/image/test_glance.py
+++ b/nova/tests/image/test_glance.py
@@ -18,6 +18,8 @@
 
 import datetime
 
+import glance.common.exception as glance_exception
+
 from nova.tests.api.openstack import fakes
 from nova import context
 from nova import exception
@@ -181,7 +183,7 @@ class TestGlanceImageService(test.TestCase):
         image_id = self.service.create(self.context, fixture)['id']
 
         self.assertNotEquals(None, image_id)
-        self.assertRaises(exception.NotFound,
+        self.assertRaises(exception.ImageNotFound,
                           self.service.show,
                           self.context,
                           'bad image id')
@@ -260,6 +262,17 @@ class TestGlanceImageService(test.TestCase):
             self.assertDictMatch(meta, expected)
             i = i + 1
 
+    def test_index_invalid_marker(self):
+        fixtures = []
+        ids = []
+        for i in range(10):
+            fixture = self._make_fixture(name='TestImage %d' % (i))
+            fixtures.append(fixture)
+            ids.append(self.service.create(self.context, fixture)['id'])
+
+        self.assertRaises(exception.Invalid, self.service.index,
+                          self.context, marker='invalidmarker')
+
     def test_index_private_image(self):
         fixture = self._make_fixture(name='test image')
         fixture['is_public'] = False
@@ -354,6 +367,17 @@ class TestGlanceImageService(test.TestCase):
             self.assertDictMatch(meta, expected)
             i = i + 1
 
+    def test_detail_invalid_marker(self):
+        fixtures = []
+        ids = []
+        for i in range(10):
+            fixture = self._make_fixture(name='TestImage %d' % (i))
+            fixtures.append(fixture)
+            ids.append(self.service.create(self.context, fixture)['id'])
+
+        self.assertRaises(exception.Invalid, self.service.detail,
+                          self.context, marker='invalidmarker')
+
     def test_update(self):
         fixture = self._make_fixture(name='test image')
         image_id = self.service.create(self.context, fixture)['id']
@@ -445,6 +469,17 @@ class TestGlanceImageService(test.TestCase):
                           self.context,
                           image_id)
 
+    def test_show_raises_on_missing_credential(self):
+        def raise_missing_credentials(*args, **kwargs):
+            raise glance_exception.MissingCredentialError()
+
+        self.stubs.Set(glance_stubs.StubGlanceClient, 'get_image_meta',
+                       raise_missing_credentials)
+        self.assertRaises(exception.ImageNotAuthorized,
+                          self.service.show,
+                          self.context,
+                          'test-image-id')
+
     def test_detail_passes_through_to_client(self):
         fixture = self._make_fixture(name='image10', is_public=True)
         image_id = self.service.create(self.context, fixture)['id']