Merge lp:~jshepher/glance/lp813291 into lp:~hudson-openstack/glance/trunk

[Justin Shepherd]

Added fix for Bug #813291: POST to /images setting x-image-meta-id to an already existing image id causes a 500 error.

I came up with about 3 or 4 different ways of resolving this bug, and all tests pass. Let me know if you think this should be resolved in a different way.

This branch also includes a test in tests/functional/test_httplib2_api.py that shows the 500 error.

[Justin Shepherd (jshepher)]

Here are the other solutions i tried:

#2

=== modified file 'glance/registry/db/api.py'
--- glance/registry/db/api.py 2011-07-20 04:11:54 +0000
+++ glance/registry/db/api.py 2011-07-20 04:14:00 +0000
@@ -92,10 +92,10 @@

 def image_create(context, values):
     """Create an image from the values dictionary."""
+ if 'id' in values:
+ return _image_update(context, values, values['id'], False)
+ else:
+ return _image_update(context, values, None, False)

 def image_update(context, image_id, values, purge_props=False):

====================================================================================================

#3

=== modified file 'glance/registry/db/api.py'
--- glance/registry/db/api.py 2011-07-20 04:11:54 +0000
+++ glance/registry/db/api.py 2011-07-20 04:15:01 +0000
@@ -258,6 +255,9 @@
         # not a dict.
         properties = values.pop('properties', {})

+ if 'id' in values:
+ image_id = values['id']
+
         if image_id:
             image_ref = image_get(context, image_id, session=session)
         else:

[lp:~jshepher/glance/lp813291]

157. By Justin Shepherd on 2011-07-20

left in 2 fixes.. removing redundant fix

[Brian Waldon (bcwaldon)]

Thanks for addressing this bug, Justin. I'm not so sure I agree with this behavior. If you try to POST with an image id that is already taken, the client should get a 400 Bad Request. It seems counterintuitive to me to effectively do a PUT and update an existing image when the creation of a new image was expected. What do you think?

review: Needs Information

[Justin Shepherd (jshepher)]

There was already existing code meant to address this bug.. Where it would check for an already existing image id and update the existing image instead of responding with a error.

The issue is that it was a triple level method chain where image id gets lost in translation between method calls.

Image create gets the values as a dict, and then them on to _image_update (which expects image_id as its 3rd argument).. however image_create was not explicitly passing image_id.. so the existing logic was getting bypassed.

Now i am not sure this is the right code path.. i initially thought this should throw a 409 (http conflict).. and throw an error about a duplicate image id.

[Jay Pipes (jaypipes)]

> Now i am not sure this is the right code path.. i initially thought this
> should throw a 409 (http conflict).. and throw an error about a duplicate
> image id.

This is what *should* happen, AFAIK. For POST. For PUT, should update of course...

Shep, are you OK with modifying the code to ensure the above behaviour?

-jay

review: Needs Fixing

[lp:~jshepher/glance/lp813291]

158. By Justin Shepherd on 2011-07-20

API is now returning a 409 error on duplicate POST. I also modified the testcase to expect a 409 response.

[Jay Pipes (jaypipes)]

good work, Shep!

review: Approve

[Brian Waldon (bcwaldon)]

Awesome, Justin. I functionally tested at the api and registry level and got a 409 both times.

review: Approve

=== modified file 'glance/registry/db/api.py'
--- glance/registry/db/api.py	2011-07-18 18:22:41 +0000
+++ glance/registry/db/api.py	2011-07-20 20:53:36 +0000
@@ -24,6 +24,7 @@
 import logging
 
 from sqlalchemy import asc, create_engine, desc
+from sqlalchemy.exc import IntegrityError
 from sqlalchemy.ext.declarative import declarative_base
 from sqlalchemy.orm import exc
 from sqlalchemy.orm import joinedload
@@ -273,7 +274,11 @@
         # idiotic.
         validate_image(image_ref.to_dict())
 
-        image_ref.save(session=session)
+        try:
+            image_ref.save(session=session)
+        except IntegrityError, e:
+            raise exception.Duplicate("Image ID %s already exists!"
+                                      % values['id'])
 
         _set_properties_for_image(context, image_ref, properties, purge_props,
                                   session)

=== modified file 'tests/functional/test_httplib2_api.py'
--- tests/functional/test_httplib2_api.py	2011-07-20 03:05:28 +0000
+++ tests/functional/test_httplib2_api.py	2011-07-20 20:53:36 +0000
@@ -1029,3 +1029,50 @@
         self.assertEqual(data['images'][2]['id'], 2)
 
         self.stop_servers()
+
+    def test_duplicate_image_upload(self):
+        """
+        Upload initial image, then attempt to upload duplicate image
+        """
+        self.cleanup()
+        self.start_servers()
+
+        # 0. GET /images
+        # Verify no public images
+        path = "http://%s:%d/v1/images" % ("0.0.0.0", self.api_port)
+        http = httplib2.Http()
+        response, content = http.request(path, 'GET')
+        self.assertEqual(response.status, 200)
+        self.assertEqual(content, '{"images": []}')
+
+        # 1. POST /images with public image named Image1
+        headers = {'Content-Type': 'application/octet-stream',
+                   'X-Image-Meta-Name': 'Image1',
+                   'X-Image-Meta-Status': 'active',
+                   'X-Image-Meta-Container-Format': 'ovf',
+                   'X-Image-Meta-Disk-Format': 'vdi',
+                   'X-Image-Meta-Size': '19',
+                   'X-Image-Meta-Is-Public': 'True'}
+        path = "http://%s:%d/v1/images" % ("0.0.0.0", self.api_port)
+        http = httplib2.Http()
+        response, content = http.request(path, 'POST', headers=headers)
+        self.assertEqual(response.status, 201)
+
+        # 2. POST /images with public image named Image1, and ID: 1
+        headers = {'Content-Type': 'application/octet-stream',
+                   'X-Image-Meta-Name': 'Image1 Update',
+                   'X-Image-Meta-Status': 'active',
+                   'X-Image-Meta-Container-Format': 'ovf',
+                   'X-Image-Meta-Disk-Format': 'vdi',
+                   'X-Image-Meta-Size': '19',
+                   'X-Image-Meta-Id': '1',
+                   'X-Image-Meta-Is-Public': 'True'}
+        path = "http://%s:%d/v1/images" % ("0.0.0.0", self.api_port)
+        http = httplib2.Http()
+        response, content = http.request(path, 'POST', headers=headers)
+        self.assertEqual(response.status, 409)
+        expected = "An image with identifier 1 already exists"
+        self.assertTrue(expected in content,
+                        "Could not find '%s' in '%s'" % (expected, content))
+
+        self.stop_servers()