Fixes for ec2 images

 * Fixes s3 image service to convert back to uuids on update
 * Adds exception for attempt to update an unowned image
 * Adds error messages to ec2 for failure cases
 * Adds tests to verify changes
 * Fixes bug 942865

Change-Id: I35331c635756f10c02b30dd43ab3fe0ad98bc28c

nova/api/ec2/cloud.py

@@ -1427,6 +1427,9 @@ def _register_image(self, context, metadata):
     def register_image(self, context, image_location=None, **kwargs):
         if image_location is None and 'name' in kwargs:
             image_location = kwargs['name']
+        if image_location is None:
+            raise exception.EC2APIError(_('imageLocation is required'))
+
         metadata = {'properties': {'image_location': image_location}}
 
         if 'root_device_name' in kwargs:
@@ -1501,7 +1504,11 @@ def modify_image_attribute(self, context, image_id, attribute,
         del(image['id'])
 
         image['is_public'] = (operation_type == 'add')
-        return self.image_service.update(context, internal_id, image)
+        try:
+            return self.image_service.update(context, internal_id, image)
+        except exception.ImageNotAuthorized:
+            msg = _('Not allowed to modify attributes for image %s')
+            raise exception.EC2APIError(msg % image_id)
 
     def update_image(self, context, image_id, **kwargs):
         internal_id = ec2utils.ec2_id_to_id(image_id)

nova/exception.py

@@ -201,7 +201,7 @@ class MelangeConnectionFailed(NovaException):
 
 class NotAuthorized(NovaException):
     message = _("Not authorized.")
-    code = 401
+    code = 403
 
 
 class AdminRequired(NotAuthorized):
@@ -212,6 +212,10 @@ class PolicyNotAuthorized(NotAuthorized):
     message = _("Policy doesn't allow %(action)s to be performed.")
 
 
+class ImageNotAuthorized(NovaException):
+    message = _("Not authorized for image %(image_id)s.")
+
+
 class Invalid(NovaException):
     message = _("Unacceptable parameters.")
     code = 400

nova/image/glance.py

@@ -301,6 +301,9 @@ def update(self, context, image_id, image_meta, data=None):
             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)
 
         base_image_meta = self._translate_from_glance(image_meta)
         return base_image_meta

nova/image/s3.py

@@ -105,6 +105,27 @@ def _find_or_create(image_uuid):
 
         return image_copy
 
+    def _translate_id_to_uuid(self, context, image):
+        image_copy = image.copy()
+
+        try:
+            image_id = image_copy['id']
+        except KeyError:
+            pass
+        else:
+            image_copy['id'] = self.get_image_uuid(context, image_id)
+
+        for prop in ['kernel_id', 'ramdisk_id']:
+            try:
+                image_id = image_copy['properties'][prop]
+            except (KeyError, ValueError):
+                pass
+            else:
+                image_uuid = self.get_image_uuid(context, image_id)
+                image_copy['properties'][prop] = image_uuid
+
+        return image_copy
+
     def create(self, context, metadata, data=None):
         """Create an image.
 
@@ -120,6 +141,7 @@ def delete(self, context, image_id):
 
     def update(self, context, image_id, metadata, data=None):
         image_uuid = self.get_image_uuid(context, image_id)
+        metadata = self._translate_id_to_uuid(context, metadata)
         image = self.service.update(context, image_uuid, metadata, data)
         return self._translate_uuid_to_id(context, image)
 

nova/tests/api/ec2/test_cloud.py

@@ -33,6 +33,7 @@
 from nova import exception
 from nova import flags
 from nova.image import fake
+from nova.image import s3
 from nova import log as logging
 from nova import rpc
 from nova import test
@@ -1153,7 +1154,7 @@ def test_modify_image_attribute(self):
         modify_image_attribute = self.cloud.modify_image_attribute
 
         fake_metadata = {
-            'id': 1,
+            'id': 'cedef40a-ed67-4d10-800e-17455edce175',
             'container_format': 'ami',
             'properties': {
                 'kernel_id': 'cedef40a-ed67-4d10-800e-17455edce175',
@@ -1162,19 +1163,49 @@ def test_modify_image_attribute(self):
             'is_public': False}
 
         def fake_show(meh, context, id):
-            return fake_metadata
+            return copy.deepcopy(fake_metadata)
 
         def fake_update(meh, context, image_id, metadata, data=None):
-            fake_metadata.update(metadata)
-            return fake_metadata
+            self.assertEqual(metadata['properties']['kernel_id'],
+                             fake_metadata['properties']['kernel_id'])
+            self.assertEqual(metadata['properties']['ramdisk_id'],
+                             fake_metadata['properties']['ramdisk_id'])
+            self.assertTrue(metadata['is_public'])
+            image = copy.deepcopy(fake_metadata)
+            image.update(metadata)
+            return image
 
         self.stubs.Set(fake._FakeImageService, 'show', fake_show)
         self.stubs.Set(fake._FakeImageService, 'show_by_name', fake_show)
         self.stubs.Set(fake._FakeImageService, 'update', fake_update)
         result = modify_image_attribute(self.context, 'ami-00000001',
                                           'launchPermission', 'add',
                                            user_group=['all'])
-        self.assertEqual(True, result['is_public'])
+        self.assertTrue(result['is_public'])
+
+    def test_register_image(self):
+        register_image = self.cloud.register_image
+
+        def fake_create(*args, **kwargs):
+            # NOTE(vish): We are mocking s3 so make sure we have converted
+            #             to ids instead of uuids.
+            return {'id': 1,
+            'container_format': 'ami',
+            'properties': {
+                'kernel_id': 1,
+                'ramdisk_id': 1,
+                'type': 'machine'},
+            'is_public': False}
+
+        self.stubs.Set(s3.S3ImageService, 'create', fake_create)
+        image_location = 'fake_bucket/fake.img.manifest.xml'
+        result = register_image(self.context, image_location)
+        self.assertEqual(result['imageId'], 'ami-00000001')
+
+    def test_register_image_empty(self):
+        register_image = self.cloud.register_image
+        self.assertRaises(exception.EC2APIError, register_image, self.context,
+                          image_location=None)
 
     def test_deregister_image(self):
         deregister_image = self.cloud.deregister_image