delete deleted image 500 bug

Fixes bug 1138666

When you delete an image that you just deleted, you get a 500 Internal Server Error.
In this case, the problem isn't an internal server error but the "unauthorized" response
from the image service is not expected.

This can be recreated with devstack:
 1) delete an image
 2) delete the same image

Here's an example:

$ nova image-list
+--------------------------------------+---------------------------------+--------+--------+
| ID                                   | Name                            | Status | Server |
+--------------------------------------+---------------------------------+--------+--------+
| caa6c969-0f32-466b-8e8e-a7e0ac835470 | cirros-0.3.1-x86_64-uec         | ACTIVE |        |
| 8af603da-b69f-49c6-8e52-25632eccb31c | cirros-0.3.1-x86_64-uec-kernel  | ACTIVE |        |
| fe67d4e1-baca-4968-8638-2314373b620b | cirros-0.3.1-x86_64-uec-ramdisk | ACTIVE |        |
+--------------------------------------+---------------------------------+--------+--------+

$ nova image-delete caa6c969-0f32-466b-8e8e-a7e0ac835470

$ nova image-delete caa6c969-0f32-466b-8e8e-a7e0ac835470
ERROR: The server has either erred or is incapable of performing the requested operation. (HTTP 500) (Request-ID: req-1254f8a5-8dda-444a-a077-2072ab3baa6a)

If you delete a deleted image, the server should respond with '403 Forbidden'
because the image still exists and no user is allowed to delete a deleted image.

The fix is to catch the exception from the image service and
convert it to a wsgi exception for '403 Forbidden'.

Change-Id: I2f14687d5468b67389f5dd6ab338ceb54e8a29bb

5 files changed, 35 insertions, 4 deletions

diff --git a/nova/api/openstack/compute/images.py b/nova/api/openstack/compute/images.py
index 82db21c9e..3d54da0b5 100644
--- a/nova/api/openstack/compute/images.py
+++ b/nova/api/openstack/compute/images.py
@@ -159,6 +159,11 @@ class Controller(wsgi.Controller):
         except exception.ImageNotFound:
             explanation = _("Image not found.")
             raise webob.exc.HTTPNotFound(explanation=explanation)
+        except exception.ImageNotAuthorized:
+            # The image service raises this exception on delete if glanceclient
+            # raises HTTPForbidden.
+            explanation = _("You are not allowed to delete the image.")
+            raise webob.exc.HTTPForbidden(explanation=explanation)
         return webob.exc.HTTPNoContent()
 
     @wsgi.serializers(xml=MinimalImagesTemplate)
diff --git a/nova/tests/api/openstack/compute/test_images.py b/nova/tests/api/openstack/compute/test_images.py
index db0ad51f7..a35dc6e51 100644
--- a/nova/tests/api/openstack/compute/test_images.py
+++ b/nova/tests/api/openstack/compute/test_images.py
@@ -665,6 +665,18 @@ class ImagesControllerTest(test.TestCase):
         response = self.controller.delete(request, '124')
         self.assertEqual(response.status_int, 204)
 
+    def test_delete_deleted_image(self):
+        """If you try to delete a deleted image, you get back 403 Forbidden."""
+
+        deleted_image_id = 128
+        # see nova.tests.api.openstack.fakes:_make_image_fixtures
+
+        request = fakes.HTTPRequest.blank(
+              '/v2/fake/images/%s' % deleted_image_id)
+        request.method = 'DELETE'
+        self.assertRaises(webob.exc.HTTPForbidden, self.controller.delete,
+             request, '%s' % deleted_image_id)
+
     def test_delete_image_not_found(self):
         request = fakes.HTTPRequest.blank('/v2/fake/images/300')
         request.method = 'DELETE'
diff --git a/nova/tests/api/openstack/fakes.py b/nova/tests/api/openstack/fakes.py
index 1bc7b313e..1299c8bb9 100644
--- a/nova/tests/api/openstack/fakes.py
+++ b/nova/tests/api/openstack/fakes.py
@@ -216,12 +216,10 @@ def _make_image_fixtures():
     NOW_GLANCE_FORMAT = "2010-10-11T10:30:22"
 
     image_id = 123
-    base_attrs = {'deleted': False}
 
     fixtures = []
 
     def add_fixture(**kwargs):
-        kwargs.update(base_attrs)
         fixtures.append(kwargs)
 
     # Public image
@@ -236,9 +234,11 @@ def _make_image_fixtures():
     snapshot_properties = {'instance_uuid': uuid, 'user_id': 'fake'}
     for status in ('queued', 'saving', 'active', 'killed',
                    'deleted', 'pending_delete'):
+        deleted = False if status != 'deleted' else True
         add_fixture(id=image_id, name='%s snapshot' % status,
                     is_public=False, status=status,
-                    properties=snapshot_properties, size='25165824')
+                    properties=snapshot_properties, size='25165824',
+                    deleted=deleted)
         image_id += 1
 
     # Image without a name
diff --git a/nova/tests/glance/stubs.py b/nova/tests/glance/stubs.py
index 076afeffc..56dae7ffc 100644
--- a/nova/tests/glance/stubs.py
+++ b/nova/tests/glance/stubs.py
@@ -83,7 +83,13 @@ class StubGlanceClient(object):
     def delete(self, image_id):
         for i, image in enumerate(self._images):
             if image.id == image_id:
-                del self._images[i]
+                # When you delete an image from glance, it sets the status to
+                # DELETED. If you try to delete a DELETED image, it raises
+                # HTTPForbidden.
+                image_data = self._images[i]
+                if image_data.deleted:
+                    raise glanceclient.exc.HTTPForbidden()
+                image_data.deleted = True
                 return
         raise glanceclient.exc.NotFound(image_id)
 
diff --git a/nova/tests/image/test_glance.py b/nova/tests/image/test_glance.py
index 2eca4b969..433a25e61 100644
--- a/nova/tests/image/test_glance.py
+++ b/nova/tests/image/test_glance.py
@@ -363,8 +363,16 @@ class TestGlanceImageService(test.TestCase):
         self.assertEquals(2, num_images)
 
         self.service.delete(self.context, ids[0])
+        # When you delete an image from glance, it sets the status to DELETED
+        # and doesn't actually remove the image.
 
+        # Check the image is still there.
         num_images = len(self.service.detail(self.context))
+        self.assertEquals(2, num_images)
+
+        # Check the image is marked as deleted.
+        num_images = reduce(lambda x, y: x + (0 if y['deleted'] else 1),
+                            self.service.detail(self.context), 0)
         self.assertEquals(1, num_images)
 
     def test_show_passes_through_to_client(self):