Merge lp:~jaypipes/glance/checksum into lp:~glance-coresec/glance/cactus-trunk

[Jay Pipes]

Adds checksumming to Glance.

When adding an image (or uploading an image during PUT operations),
you may now supply an optional X-Image-Meta-Checksum header. When
storing the uploaded image, the backend image stores now are required
to return a checksum of the data they just stored. The optional
X-Image-Meta-Checksum header is compared against this generated checksum
and returns a 409 Bad Request if there is a mismatch.

The ETag header is now properly set to the image's checksum now
for all GET /images/<ID>, HEAD /images/<ID>, POST /images and
PUT /images/<ID> operations.

Adds unit tests verifying the checksumming behaviour in the API, and
in the Swift and Filesystem backend stores.

Includes migration script.

NOTE: This does not include the DB migration script. Separate bug will be filed for that.

[Chuck Thier (cthier)]

The swift parts look reasonable to me. The only other thing that I might add is to clarify in the docs that the checksum is an md5 checksum, otherwise people may try to use CRC.

review: Approve

[Jay Pipes (jaypipes)]

thx Chuck. Made a note in the docs about the checksum being an MD5 checksum.

[Rick Harris (rconradharris)]

Nice patch.

Unfortunately with the state of Nova <=> Glance trunks at the moment, I wasn't able to test functionally. That said, this seems pretty safe to go ahead and merge, and if we have issues, we can fix them along with all the other fixes that are in progress at the moment :)

> 110 + checksum = Column(String(32))

I assume the answer is YAGNI, but I'll ask it anyway: Will we ever allow SHA1 or SHA256?

Would it make sense to make this a String(64) or, heck, a String(255) for flexibility?

review: Approve

[Jay Pipes (jaypipes)]

> > 110 + checksum = Column(String(32))
>
> I assume the answer is YAGNI, but I'll ask it anyway: Will we ever allow SHA1
> or SHA256?
>
> Would it make sense to make this a String(64) or, heck, a String(255) for
> flexibility?

Hmm, good question. Not sure, really. I suppose we could expand it in the future if we decide its a needed feature.

[Cory Wright (corywright)]

I noticed that there is no migration script to add the checksum column. Trunk is already missing a migration for disk_format/container_format, but we probably shouldn't get in the habit of merging to trunk without migrations, since that effectively leaves trunk broken. However, the tests seem to run fine without the migration.

I tried merging trunk into this branch to test these changes with the glance cli tool, but there were conflicts so you may want to merge trunk once more.

Otherwise, this looks good to me. I'm going to mark it as Needs Fixing because of the missing migration. If you think it's ok to merge this to trunk without the migration then let me know and I'll mark this approved.

review: Needs Fixing

[Jay Pipes (jaypipes)]

> I noticed that there is no migration script to add the checksum column. Trunk
> is already missing a migration for disk_format/container_format, but we
> probably shouldn't get in the habit of merging to trunk without migrations,
> since that effectively leaves trunk broken. However, the tests seem to run
> fine without the migration.
>
> I tried merging trunk into this branch to test these changes with the glance
> cli tool, but there were conflicts so you may want to merge trunk once more.
>
> Otherwise, this looks good to me. I'm going to mark it as Needs Fixing
> because of the missing migration. If you think it's ok to merge this to trunk
> without the migration then let me know and I'll mark this approved.

Agreed, though I did note this in the merge proposal commit message:

"NOTE: This does not include the DB migration script. Separate bug will be filed for that."

But, I'll work on a migrate script after merging trunk again. Thanks, Cory!

[Cory Wright (corywright)]

You sure did mention it, sorry about that. I read the commit message yesterday when I first looked at this and forgot.

[Sandy Walsh (sandy-walsh)]

Hmm, I got bitten by this migration bug yesterday ... can't this be put in this branch to keep trunk happy?

[Jay Pipes (jaypipes)]

> Hmm, I got bitten by this migration bug yesterday ... can't this be put in
> this branch to keep trunk happy?

? This hasn't been merged yet, so I'm unclear how this could have bitten you :)

FYI, I've now figured out the migrations stuff. Full steam ahead today on getting the image format migration bug done and getting this branch merged with a migration file.

[Sandy Walsh (sandy-walsh)]

sorry, it was a nova issue. _get_kernel_ramdisk_from_image checks 'disk_format' != 'ami'

I guess that just stresses the importance of having the migrations in place with the branch :)

Minor tweak ... ping me and I'll approve immediately after

278 missing _() ?

review: Needs Fixing

[Jay Pipes (jaypipes)]

On Wed, Mar 23, 2011 at 10:26 AM, Sandy Walsh <email address hidden> wrote:
> Review: Needs Fixing
> sorry, it was a nova issue. _get_kernel_ramdisk_from_image checks 'disk_format' != 'ami'
>
> I guess that just stresses the importance of having the migrations in place with the branch :)

Ya, I'm currently working on the migration script. should be done shortly...

> Minor tweak ... ping me and I'll approve immediately after
>
> 278 missing _() ?

Glance doesn't have i18n. Yet :)

[Sandy Walsh (sandy-walsh)]

ah! haha

review: Approve

[Sandy Walsh (sandy-walsh)]

second time's a charm

review: Approve

[Thierry Carrez (ttx)]

Essential feature. FFE preapproved for late merging until Tuesday EOD

review: Approve (ffe)

[Rick Harris (rconradharris)]

Really nice job, Jay. Very clean implementation, nice test coverage, and great docs. Bravo.

review: Approve

[Jay Pipes (jaypipes)]

approving the stop_servers() fix.. looks like rick approved the MP right before that fix changed the diff...

review: Approve

[Cory Wright (corywright)]

Looks great and is working for me. Nice work, Jay!

review: Approve

=== modified file 'doc/source/glanceapi.rst'
--- doc/source/glanceapi.rst	2011-03-05 17:04:43 +0000
+++ doc/source/glanceapi.rst	2011-03-25 19:01:10 +0000
@@ -67,6 +67,7 @@
      'disk_format': 'vhd',
      'container_format': 'ovf',
      'size': '5368709120',
+     'checksum': 'c2e5db72bd7fd153f53ede5da5a06de3',
      'location': 'swift://account:key/container/image.tar.gz.0',
      'created_at': '2010-02-03 09:34:01',
      'updated_at': '2010-02-03 09:34:01',
@@ -89,6 +90,8 @@
   The `properties` field is a mapping of free-form key/value pairs that
   have been saved with the image metadata
 
+  The `checksum` field is an MD5 checksum of the image file data
+
 
 Requesting Detailed Metadata on a Specific Image
 ------------------------------------------------
@@ -116,6 +119,7 @@
   x-image-meta-disk-format      vhd
   x-image-meta-container-format ovf
   x-image-meta-size             5368709120
+  x-image-meta-checksum         c2e5db72bd7fd153f53ede5da5a06de3
   x-image-meta-location         swift://account:key/container/image.tar.gz.0
   x-image-meta-created_at       2010-02-03 09:34:01
   x-image-meta-updated_at       2010-02-03 09:34:01
@@ -137,6 +141,9 @@
   that have been saved with the image metadata. The key is the string
   after `x-image-meta-property-` and the value is the value of the header
 
+  The response's `ETag` header will always be equal to the
+  `x-image-meta-checksum` value
+
 
 Retrieving a Virtual Machine Image
 ----------------------------------
@@ -166,6 +173,7 @@
   x-image-meta-disk-format      vhd
   x-image-meta-container-format ovf
   x-image-meta-size             5368709120
+  x-image-meta-checksum         c2e5db72bd7fd153f53ede5da5a06de3
   x-image-meta-location         swift://account:key/container/image.tar.gz.0
   x-image-meta-created_at       2010-02-03 09:34:01
   x-image-meta-updated_at       2010-02-03 09:34:01
@@ -190,6 +198,9 @@
   The response's `Content-Length` header shall be equal to the value of
   the `x-image-meta-size` header
 
+  The response's `ETag` header will always be equal to the
+  `x-image-meta-checksum` value
+
   The image data itself will be the body of the HTTP response returned
   from the request, which will have content-type of
   `application/octet-stream`.
@@ -284,6 +295,15 @@
   When not present, Glance will calculate the image's size based on the size
   of the request body.
 
+* ``x-image-meta-checksum``
+
+  This header is optional. When present it shall be the expected **MD5**
+  checksum of the image file data.
+
+  When present, Glance will verify the checksum generated from the backend
+  store when storing your image against this value and return a 
+  **400 Bad Request** if the values do not match.
+
 * ``x-image-meta-is-public``
 
   This header is optional.

=== modified file 'glance/client.py'
--- glance/client.py	2011-03-06 17:02:44 +0000
+++ glance/client.py	2011-03-25 19:01:10 +0000
@@ -142,7 +142,10 @@
                 c.request(method, action, body, headers)
             res = c.getresponse()
             status_code = self.get_status_code(res)
-            if status_code == httplib.OK:
+            if status_code in (httplib.OK,
+                               httplib.CREATED,
+                               httplib.ACCEPTED,
+                               httplib.NO_CONTENT):
                 return res
             elif status_code == httplib.UNAUTHORIZED:
                 raise exception.NotAuthorized

=== modified file 'glance/registry/db/api.py'
--- glance/registry/db/api.py	2011-03-14 19:10:24 +0000
+++ glance/registry/db/api.py	2011-03-25 19:01:10 +0000
@@ -43,7 +43,7 @@
 
 IMAGE_ATTRS = BASE_MODEL_ATTRS | set(['name', 'status', 'size',
                                       'disk_format', 'container_format',
-                                      'is_public', 'location'])
+                                      'is_public', 'location', 'checksum'])
 
 CONTAINER_FORMATS = ['ami', 'ari', 'aki', 'bare', 'ovf']
 DISK_FORMATS = ['ami', 'ari', 'aki', 'vhd', 'vmdk', 'raw', 'qcow2', 'vdi']

=== added file 'glance/registry/db/migrate_repo/versions/004_add_checksum.py'
--- glance/registry/db/migrate_repo/versions/004_add_checksum.py	1970-01-01 00:00:00 +0000
+++ glance/registry/db/migrate_repo/versions/004_add_checksum.py	2011-03-25 19:01:10 +0000
@@ -0,0 +1,80 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 OpenStack LLC.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from migrate.changeset import *
+from sqlalchemy import *
+from sqlalchemy.sql import and_, not_
+
+from glance.registry.db.migrate_repo.schema import (
+    Boolean, DateTime, Integer, String, Text, from_migration_import)
+
+
+def get_images_table(meta):
+    """
+    Returns the Table object for the images table that
+    corresponds to the images table definition of this version.
+    """
+    images = Table('images', meta,
+        Column('id', Integer(), primary_key=True, nullable=False),
+        Column('name', String(255)),
+        Column('disk_format', String(20)),
+        Column('container_format', String(20)),
+        Column('size', Integer()),
+        Column('status', String(30), nullable=False),
+        Column('is_public', Boolean(), nullable=False, default=False,
+               index=True),
+        Column('location', Text()),
+        Column('created_at', DateTime(), nullable=False),
+        Column('updated_at', DateTime()),
+        Column('deleted_at', DateTime()),
+        Column('deleted', Boolean(), nullable=False, default=False,
+               index=True),
+        Column('checksum', String(32)),
+        mysql_engine='InnoDB',
+        useexisting=True)
+
+    return images
+
+
+def get_image_properties_table(meta):
+    """
+    No changes to the image properties table from 002...
+    """
+    (define_image_properties_table,) = from_migration_import(
+        '002_add_image_properties_table', ['define_image_properties_table'])
+
+    image_properties = define_image_properties_table(meta)
+    return image_properties
+
+
+def upgrade(migrate_engine):
+    meta = MetaData()
+    meta.bind = migrate_engine
+
+    images = get_images_table(meta)
+
+    checksum = Column('checksum', String(32))
+    checksum.create(images)
+
+
+def downgrade(migrate_engine):
+    meta = MetaData()
+    meta.bind = migrate_engine
+
+    images = get_images_table(meta)
+
+    images.columns['checksum'].drop()

=== modified file 'glance/registry/db/models.py'
--- glance/registry/db/models.py	2011-03-05 17:04:43 +0000
+++ glance/registry/db/models.py	2011-03-25 19:01:10 +0000
@@ -104,6 +104,7 @@
     status = Column(String(30), nullable=False)
     is_public = Column(Boolean, nullable=False, default=False)
     location = Column(Text)
+    checksum = Column(String(32))
 
 
 class ImageProperty(BASE, ModelBase):

=== modified file 'glance/registry/server.py'
--- glance/registry/server.py	2011-02-25 16:52:12 +0000
+++ glance/registry/server.py	2011-03-25 19:01:10 +0000
@@ -14,8 +14,9 @@
 #    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 #    License for the specific language governing permissions and limitations
 #    under the License.
+
 """
-Parllax Image controller
+Reference implementation registry server WSGI controller
 """
 
 import json
@@ -31,6 +32,10 @@
 
 logger = logging.getLogger('glance.registry.server')
 
+DISPLAY_FIELDS_IN_INDEX = ['id', 'name', 'size',
+                           'disk_format', 'container_format',
+                           'checksum']
+
 
 class Controller(wsgi.Controller):
     """Controller for the reference implementation registry server"""
@@ -49,16 +54,24 @@
 
         Where image_list is a sequence of mappings::
 
-            {'id': image_id, 'name': image_name}
+            {
+            'id': <ID>,
+            'name': <NAME>,
+            'size': <SIZE>,
+            'disk_format': <DISK_FORMAT>,
+            'container_format': <CONTAINER_FORMAT>,
+            'checksum': <CHECKSUM>
+            }
 
         """
         images = db_api.image_get_all_public(None)
-        image_dicts = [dict(id=i['id'],
-                            name=i['name'],
-                            disk_format=i['disk_format'],
-                            container_format=i['container_format'],
-                            size=i['size']) for i in images]
-        return dict(images=image_dicts)
+        results = []
+        for image in images:
+            result = {}
+            for field in DISPLAY_FIELDS_IN_INDEX:
+                result[field] = image[field]
+            results.append(result)
+        return dict(images=results)
 
     def detail(self, req):
         """Return detailed information for all public, non-deleted images

=== modified file 'glance/server.py'
--- glance/server.py	2011-03-24 23:03:17 +0000
+++ glance/server.py	2011-03-25 19:01:10 +0000
@@ -29,6 +29,7 @@
 
 """
 
+import httplib
 import json
 import logging
 import sys
@@ -68,8 +69,8 @@
         GET /images/<ID> -- Return image data for image with id <ID>
         POST /images -- Store image data and return metadata about the
                         newly-stored image
-        PUT /images/<ID> -- Update image metadata (not image data, since
-                            image data is immutable once stored)
+        PUT /images/<ID> -- Update image metadata and/or upload image
+                            data for a previously-reserved image
         DELETE /images/<ID> -- Delete the image with id <ID>
     """
 
@@ -82,6 +83,9 @@
 
             * id -- The opaque image identifier
             * name -- The name of the image
+            * disk_format -- The disk image format
+            * container_format -- The "container" format of the image
+            * checksum -- MD5 checksum of the image data
             * size -- Size of image data in bytes
 
         :param request: The WSGI/Webob Request object
@@ -92,6 +96,7 @@
                  'name': <NAME>,
                  'disk_format': <DISK_FORMAT>,
                  'container_format': <DISK_FORMAT>,
+                 'checksum': <CHECKSUM>
                  'size': <SIZE>}, ...
             ]}
         """
@@ -111,6 +116,7 @@
                  'size': <SIZE>,
                  'disk_format': <DISK_FORMAT>,
                  'container_format': <CONTAINER_FORMAT>,
+                 'checksum': <CHECKSUM>,
                  'store': <STORE>,
                  'status': <STATUS>,
                  'created_at': <TIMESTAMP>,
@@ -136,6 +142,8 @@
 
         res = Response(request=req)
         utils.inject_image_meta_into_headers(res, image)
+        res.headers.add('Location', "/images/%s" % id)
+        res.headers.add('ETag', image['checksum'])
 
         return req.get_response(res)
 
@@ -167,6 +175,8 @@
         # Using app_iter blanks content-length, so we set it here...
         res.headers.add('Content-Length', image['size'])
         utils.inject_image_meta_into_headers(res, image)
+        res.headers.add('Location', "/images/%s" % id)
+        res.headers.add('ETag', image['checksum'])
         return req.get_response(res)
 
     def _reserve(self, req):
@@ -227,36 +237,45 @@
 
         store = self.get_store_or_400(req, store_name)
 
-        image_meta['status'] = 'saving'
         image_id = image_meta['id']
-        logger.debug("Updating image metadata for image %s"
+        logger.debug("Setting image %s to status 'saving'"
                      % image_id)
-        registry.update_image_metadata(self.options,
-                                       image_meta['id'],
-                                       image_meta)
-
+        registry.update_image_metadata(self.options, image_id,
+                                       {'status': 'saving'})
         try:
             logger.debug("Uploading image data for image %(image_id)s "
                          "to %(store_name)s store" % locals())
-            location, size = store.add(image_meta['id'],
-                                       req.body_file,
-                                       self.options)
-            # If size returned from store is different from size
-            # already stored in registry, update the registry with
-            # the new size of the image
-            if image_meta.get('size', 0) != size:
-                image_meta['size'] = size
-                logger.debug("Updating image metadata for image %s"
-                             % image_id)
-                registry.update_image_metadata(self.options,
-                                               image_meta['id'],
-                                               image_meta)
+            location, size, checksum = store.add(image_meta['id'],
+                                                 req.body_file,
+                                                 self.options)
+
+            # Verify any supplied checksum value matches checksum
+            # returned from store when adding image
+            supplied_checksum = image_meta.get('checksum')
+            if supplied_checksum and supplied_checksum != checksum:
+                msg = ("Supplied checksum (%(supplied_checksum)s) and "
+                       "checksum generated from uploaded image "
+                       "(%(checksum)s) did not match. Setting image "
+                       "status to 'killed'.") % locals()
+                self._safe_kill(req, image_meta)
+                raise HTTPBadRequest(msg, content_type="text/plain",
+                                     request=req)
+
+            # Update the database with the checksum returned
+            # from the backend store
+            logger.debug("Updating image %(image_id)s data. "
+                         "Checksum set to %(checksum)s, size set "
+                         "to %(size)d" % locals())
+            registry.update_image_metadata(self.options, image_id,
+                                           {'checksum': checksum,
+                                            'size': size})
+
             return location
         except exception.Duplicate, e:
             logger.error("Error adding image to store: %s", str(e))
             raise HTTPConflict(str(e), request=req)
 
-    def _activate(self, req, image_meta, location):
+    def _activate(self, req, image_id, location):
         """
         Sets the image status to `active` and the image's location
         attribute.
@@ -265,25 +284,25 @@
         :param image_meta: Mapping of metadata about image
         :param location: Location of where Glance stored this image
         """
+        image_meta = {}
         image_meta['location'] = location
         image_meta['status'] = 'active'
-        registry.update_image_metadata(self.options,
-                                       image_meta['id'],
+        return registry.update_image_metadata(self.options,
+                                       image_id,
                                        image_meta)
 
-    def _kill(self, req, image_meta):
+    def _kill(self, req, image_id):
         """
         Marks the image status to `killed`
 
         :param request: The WSGI/Webob Request object
-        :param image_meta: Mapping of metadata about image
+        :param image_id: Opaque image identifier
         """
-        image_meta['status'] = 'killed'
         registry.update_image_metadata(self.options,
-                                       image_meta['id'],
-                                       image_meta)
+                                       image_id,
+                                       {'status': 'killed'})
 
-    def _safe_kill(self, req, image_meta):
+    def _safe_kill(self, req, image_id):
         """
         Mark image killed without raising exceptions if it fails.
 
@@ -291,12 +310,13 @@
         not raise itself, rather it should just log its error.
 
         :param request: The WSGI/Webob Request object
+        :param image_id: Opaque image identifier
         """
         try:
-            self._kill(req, image_meta)
+            self._kill(req, image_id)
         except Exception, e:
             logger.error("Unable to kill image %s: %s",
-                          image_meta['id'], repr(e))
+                          image_id, repr(e))
 
     def _upload_and_activate(self, req, image_meta):
         """
@@ -306,13 +326,16 @@
 
         :param request: The WSGI/Webob Request object
         :param image_meta: Mapping of metadata about image
+
+        :retval Mapping of updated image data
         """
         try:
+            image_id = image_meta['id']
             location = self._upload(req, image_meta)
-            self._activate(req, image_meta, location)
+            return self._activate(req, image_id, location)
         except:  # unqualified b/c we're re-raising it
             exc_type, exc_value, exc_traceback = sys.exc_info()
-            self._safe_kill(req, image_meta)
+            self._safe_kill(req, image_id)
             # NOTE(sirp): _safe_kill uses httplib which, in turn, uses
             # Eventlet's GreenSocket. Eventlet subsequently clears exceptions
             # by calling `sys.exc_clear()`.
@@ -356,19 +379,21 @@
                 image data.
         """
         image_meta = self._reserve(req)
+        image_id = image_meta['id']
 
         if utils.has_body(req):
-            self._upload_and_activate(req, image_meta)
+            image_meta = self._upload_and_activate(req, image_meta)
         else:
             if 'x-image-meta-location' in req.headers:
                 location = req.headers['x-image-meta-location']
-                self._activate(req, image_meta, location)
+                image_meta = self._activate(req, image_id, location)
 
         # APP states we should return a Location: header with the edit
         # URI of the resource newly-created.
         res = Response(request=req, body=json.dumps(dict(image=image_meta)),
-                       content_type="text/plain")
-        res.headers.add('Location', "/images/%s" % image_meta['id'])
+                       status=httplib.CREATED, content_type="text/plain")
+        res.headers.add('Location', "/images/%s" % image_id)
+        res.headers.add('ETag', image_meta['checksum'])
 
         return req.get_response(res)
 
@@ -395,9 +420,14 @@
                                                         id,
                                                         new_image_meta)
             if has_body:
-                self._upload_and_activate(req, image_meta)
+                image_meta = self._upload_and_activate(req, image_meta)
 
-            return dict(image=image_meta)
+            res = Response(request=req,
+                           body=json.dumps(dict(image=image_meta)),
+                           content_type="text/plain")
+            res.headers.add('Location', "/images/%s" % id)
+            res.headers.add('ETag', image_meta['checksum'])
+            return res
         except exception.Invalid, e:
             msg = ("Failed to update image metadata. Got error: %(e)s"
                    % locals())

=== modified file 'glance/store/__init__.py'
--- glance/store/__init__.py	2011-02-24 02:50:24 +0000
+++ glance/store/__init__.py	2011-03-25 19:01:10 +0000
@@ -141,40 +141,3 @@
     authurl = "https://%s" % '/'.join(path_parts)
 
     return user, key, authurl, container, obj
-
-
-def add_options(parser):
-    """
-    Adds any configuration options that each store might
-    have.
-
-    :param parser: An optparse.OptionParser object
-    :retval None
-    """
-    # TODO(jaypipes): Remove these imports...
-    from glance.store.http import HTTPBackend
-    from glance.store.s3 import S3Backend
-    from glance.store.swift import SwiftBackend
-    from glance.store.filesystem import FilesystemBackend
-
-    help_text = "The following configuration options are specific to the "\
-                "Glance image store."
-
-    DEFAULT_STORE_CHOICES = ['file', 'swift', 's3']
-    group = optparse.OptionGroup(parser, "Image Store Options", help_text)
-    group.add_option('--default-store', metavar="STORE",
-                     default="file",
-                     choices=DEFAULT_STORE_CHOICES,
-                     help="The backend store that Glance will use to store "
-                     "virtual machine images to. Choices: ('%s') "
-                     "Default: %%default" % "','".join(DEFAULT_STORE_CHOICES))
-
-    backend_classes = [FilesystemBackend,
-                       HTTPBackend,
-                       SwiftBackend,
-                       S3Backend]
-    for backend_class in backend_classes:
-        if hasattr(backend_class, 'add_options'):
-            backend_class.add_options(group)
-
-    parser.add_option_group(group)

=== modified file 'glance/store/filesystem.py'
--- glance/store/filesystem.py	2011-02-27 20:54:29 +0000
+++ glance/store/filesystem.py	2011-03-25 19:01:10 +0000
@@ -19,6 +19,7 @@
 A simple filesystem-backed store
 """
 
+import hashlib
 import logging
 import os
 import urlparse
@@ -110,9 +111,10 @@
         :param data: The image data to write, as a file-like object
         :param options: Conf mapping
 
-        :retval Tuple with (location, size)
-                The location that was written, with file:// scheme prepended
-                and the size in bytes of the data written
+        :retval Tuple with (location, size, checksum)
+                The location that was written, with file:// scheme prepended,
+                the size in bytes of the data written, and the checksum of
+                the image added.
         """
         datadir = options['filesystem_store_datadir']
 
@@ -127,6 +129,7 @@
             raise exception.Duplicate("Image file %s already exists!"
                                       % filepath)
 
+        checksum = hashlib.md5()
         bytes_written = 0
         with open(filepath, 'wb') as f:
             while True:
@@ -134,23 +137,11 @@
                 if not buf:
                     break
                 bytes_written += len(buf)
+                checksum.update(buf)
                 f.write(buf)
 
-        logger.debug("Wrote %(bytes_written)d bytes to %(filepath)s"
-                     % locals())
-        return ('file://%s' % filepath, bytes_written)
-
-    @classmethod
-    def add_options(cls, parser):
-        """
-        Adds specific configuration options for this store
-
-        :param parser: An optparse.OptionParser object
-        :retval None
-        """
-
-        parser.add_option('--filesystem-store-datadir', metavar="DIR",
-                          default="/var/lib/glance/images/",
-                          help="Location to write image data. This directory "
-                          "should be writeable by the user that runs the "
-                          "glance-api program. Default: %default")
+        checksum_hex = checksum.hexdigest()
+
+        logger.debug("Wrote %(bytes_written)d bytes to %(filepath)s with "
+                     "checksum %(checksum_hex)s" % locals())
+        return ('file://%s' % filepath, bytes_written, checksum_hex)

=== modified file 'glance/store/swift.py'
--- glance/store/swift.py	2011-03-16 16:45:01 +0000
+++ glance/store/swift.py	2011-03-25 19:01:10 +0000
@@ -161,7 +161,7 @@
             # header keys are lowercased by Swift
             if 'content-length' in resp_headers:
                 size = int(resp_headers['content-length'])
-            return (location, size)
+            return (location, size, obj_etag)
         except swift_client.ClientException, e:
             if e.http_status == httplib.CONFLICT:
                 raise exception.Duplicate("Swift already has an image at "

=== modified file 'tests/functional/__init__.py'
--- tests/functional/__init__.py	2011-03-17 22:15:41 +0000
+++ tests/functional/__init__.py	2011-03-25 19:01:10 +0000
@@ -203,14 +203,14 @@
         """
 
         # Spin down the API and default registry server
-        cmd = ("./bin/glance-control api start "
+        cmd = ("./bin/glance-control api stop "
                "%(conf_file_name)s --pid-file=%(api_pid_file)s"
                % self.__dict__)
         exitcode, out, err = execute(cmd)
         self.assertEqual(0, exitcode,
                          "Failed to spin down the API server. "
                          "Got: %s" % err)
-        cmd = ("./bin/glance-control registry start "
+        cmd = ("./bin/glance-control registry stop "
                "%(conf_file_name)s --pid-file=%(registry_pid_file)s"
                % self.__dict__)
         exitcode, out, err = execute(cmd)

=== modified file 'tests/functional/test_curl_api.py'
--- tests/functional/test_curl_api.py	2011-03-17 21:43:26 +0000
+++ tests/functional/test_curl_api.py	2011-03-25 19:01:10 +0000
@@ -110,7 +110,7 @@
         lines = out.split("\r\n")
         status_line = lines[0]
 
-        self.assertEqual("HTTP/1.1 200 OK", status_line)
+        self.assertEqual("HTTP/1.1 201 Created", status_line)
 
         # 4. HEAD /images
         # Verify image found now
@@ -214,6 +214,7 @@
              "disk_format": None,
              "id": 1,
              "name": "Image1",
+             "checksum": "c2e5db72bd7fd153f53ede5da5a06de3",
              "size": 5120}]}
         self.assertEqual(expected_result, json.loads(out.strip()))
 

=== modified file 'tests/stubs.py'
--- tests/stubs.py	2011-03-14 19:10:24 +0000
+++ tests/stubs.py	2011-03-25 19:01:10 +0000
@@ -282,6 +282,7 @@
                 'updated_at': datetime.datetime.utcnow(),
                 'deleted_at': None,
                 'deleted': False,
+                'checksum': None,
                 'size': 13,
                 'location': "swift://user:passwd@acct/container/obj.tar.0",
              'properties': [{'key': 'type',
@@ -297,6 +298,7 @@
                 'updated_at': datetime.datetime.utcnow(),
                 'deleted_at': None,
                 'deleted': False,
+                'checksum': None,
                 'size': 19,
                 'location': "file:///tmp/glance-tests/2",
                 'properties': []}]
@@ -316,6 +318,7 @@
             glance.registry.db.api.validate_image(values)
 
             values['size'] = values.get('size', 0)
+            values['checksum'] = values.get('checksum')
             values['deleted'] = False
             values['properties'] = values.get('properties', {})
             values['created_at'] = datetime.datetime.utcnow()
@@ -348,6 +351,7 @@
             copy_image.update(values)
             glance.registry.db.api.validate_image(copy_image)
             props = []
+            orig_properties = image['properties']
 
             if 'properties' in values.keys():
                 for k, v in values['properties'].items():
@@ -360,7 +364,8 @@
                     p['deleted_at'] = None
                     props.append(p)
 
-            values['properties'] = props
+            orig_properties = orig_properties + props
+            values['properties'] = orig_properties
 
             image.update(values)
             return image

=== modified file 'tests/unit/test_api.py'
--- tests/unit/test_api.py	2011-03-14 19:10:24 +0000
+++ tests/unit/test_api.py	2011-03-25 19:01:10 +0000
@@ -15,8 +15,9 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import hashlib
+import httplib
 import os
-import httplib
 import json
 import unittest
 
@@ -52,7 +53,9 @@
 
         """
         fixture = {'id': 2,
-                   'name': 'fake image #2'}
+                   'name': 'fake image #2',
+                   'size': 19,
+                   'checksum': None}
         req = webob.Request.blank('/')
         res = req.get_response(self.api)
         res_dict = json.loads(res.body)
@@ -70,7 +73,9 @@
 
         """
         fixture = {'id': 2,
-                   'name': 'fake image #2'}
+                   'name': 'fake image #2',
+                   'size': 19,
+                   'checksum': None}
         req = webob.Request.blank('/images')
         res = req.get_response(self.api)
         res_dict = json.loads(res.body)
@@ -90,6 +95,8 @@
         fixture = {'id': 2,
                    'name': 'fake image #2',
                    'is_public': True,
+                   'size': 19,
+                   'checksum': None,
                    'disk_format': 'vhd',
                    'container_format': 'ovf',
                    'status': 'active'}
@@ -396,7 +403,7 @@
         for k, v in fixture_headers.iteritems():
             req.headers[k] = v
         res = req.get_response(self.api)
-        self.assertEquals(res.status_int, httplib.OK)
+        self.assertEquals(res.status_int, httplib.CREATED)
 
         res_body = json.loads(res.body)['image']
         self.assertEquals('queued', res_body['status'])
@@ -431,7 +438,7 @@
         req.headers['Content-Type'] = 'application/octet-stream'
         req.body = "chunk00000remainder"
         res = req.get_response(self.api)
-        self.assertEquals(res.status_int, 200)
+        self.assertEquals(res.status_int, httplib.CREATED)
 
         res_body = json.loads(res.body)['image']
         self.assertEquals(res_body['location'],
@@ -445,6 +452,97 @@
                         "res.headerlist = %r" % res.headerlist)
         self.assertTrue('/images/3' in res.headers['location'])
 
+    def test_image_is_checksummed(self):
+        """Test that the image contents are checksummed properly"""
+        fixture_headers = {'x-image-meta-store': 'file',
+                           'x-image-meta-disk-format': 'vhd',
+                           'x-image-meta-container-format': 'ovf',
+                           'x-image-meta-name': 'fake image #3'}
+        image_contents = "chunk00000remainder"
+        image_checksum = hashlib.md5(image_contents).hexdigest()
+
+        req = webob.Request.blank("/images")
+        req.method = 'POST'
+        for k, v in fixture_headers.iteritems():
+            req.headers[k] = v
+
+        req.headers['Content-Type'] = 'application/octet-stream'
+        req.body = image_contents
+        res = req.get_response(self.api)
+        self.assertEquals(res.status_int, httplib.CREATED)
+
+        res_body = json.loads(res.body)['image']
+        self.assertEquals(res_body['location'],
+                          'file:///tmp/glance-tests/3')
+        self.assertEquals(image_checksum, res_body['checksum'],
+                          "Mismatched checksum. Expected %s, got %s" %
+                          (image_checksum, res_body['checksum']))
+
+    def test_etag_equals_checksum_header(self):
+        """Test that the ETag header matches the x-image-meta-checksum"""
+        fixture_headers = {'x-image-meta-store': 'file',
+                           'x-image-meta-disk-format': 'vhd',
+                           'x-image-meta-container-format': 'ovf',
+                           'x-image-meta-name': 'fake image #3'}
+        image_contents = "chunk00000remainder"
+        image_checksum = hashlib.md5(image_contents).hexdigest()
+
+        req = webob.Request.blank("/images")
+        req.method = 'POST'
+        for k, v in fixture_headers.iteritems():
+            req.headers[k] = v
+
+        req.headers['Content-Type'] = 'application/octet-stream'
+        req.body = image_contents
+        res = req.get_response(self.api)
+        self.assertEquals(res.status_int, httplib.CREATED)
+
+        # HEAD the image and check the ETag equals the checksum header...
+        expected_headers = {'x-image-meta-checksum': image_checksum,
+                            'etag': image_checksum}
+        req = webob.Request.blank("/images/3")
+        req.method = 'HEAD'
+        res = req.get_response(self.api)
+        self.assertEquals(res.status_int, 200)
+
+        for key in expected_headers.keys():
+            self.assertTrue(key in res.headers,
+                            "required header '%s' missing from "
+                            "returned headers" % key)
+        for key, value in expected_headers.iteritems():
+            self.assertEquals(value, res.headers[key])
+
+    def test_bad_checksum_kills_image(self):
+        """Test that the image contents are checksummed properly"""
+        image_contents = "chunk00000remainder"
+        bad_checksum = hashlib.md5("invalid").hexdigest()
+        fixture_headers = {'x-image-meta-store': 'file',
+                           'x-image-meta-disk-format': 'vhd',
+                           'x-image-meta-container-format': 'ovf',
+                           'x-image-meta-name': 'fake image #3',
+                           'x-image-meta-checksum': bad_checksum}
+
+        req = webob.Request.blank("/images")
+        req.method = 'POST'
+        for k, v in fixture_headers.iteritems():
+            req.headers[k] = v
+
+        req.headers['Content-Type'] = 'application/octet-stream'
+        req.body = image_contents
+        res = req.get_response(self.api)
+        self.assertEquals(res.status_int, webob.exc.HTTPBadRequest.code)
+
+        # Test the image was killed...
+        expected_headers = {'x-image-meta-id': '3',
+                            'x-image-meta-status': 'killed'}
+        req = webob.Request.blank("/images/3")
+        req.method = 'HEAD'
+        res = req.get_response(self.api)
+        self.assertEquals(res.status_int, 200)
+
+        for key, value in expected_headers.iteritems():
+            self.assertEquals(value, res.headers[key])
+
     def test_image_meta(self):
         """Test for HEAD /images/<ID>"""
         expected_headers = {'x-image-meta-id': '2',

=== modified file 'tests/unit/test_filesystem_store.py'
--- tests/unit/test_filesystem_store.py	2011-02-27 20:54:29 +0000
+++ tests/unit/test_filesystem_store.py	2011-03-25 19:01:10 +0000
@@ -18,6 +18,7 @@
 """Tests the filesystem backend store"""
 
 import StringIO
+import hashlib
 import unittest
 import urlparse
 
@@ -27,6 +28,11 @@
 from glance.store.filesystem import FilesystemBackend, ChunkedFile
 from tests import stubs
 
+FILESYSTEM_OPTIONS = {
+    'verbose': True,
+    'debug': True,
+    'filesystem_store_datadir': stubs.FAKE_FILESYSTEM_ROOTDIR}
+
 
 class TestFilesystemBackend(unittest.TestCase):
 
@@ -75,17 +81,17 @@
         expected_image_id = 42
         expected_file_size = 1024 * 5  # 5K
         expected_file_contents = "*" * expected_file_size
+        expected_checksum = hashlib.md5(expected_file_contents).hexdigest()
         expected_location = "file://%s/%s" % (stubs.FAKE_FILESYSTEM_ROOTDIR,
                                               expected_image_id)
         image_file = StringIO.StringIO(expected_file_contents)
-        options = {'verbose': True,
-                   'debug': True,
-                   'filesystem_store_datadir': stubs.FAKE_FILESYSTEM_ROOTDIR}
 
-        location, size = FilesystemBackend.add(42, image_file, options)
+        location, size, checksum = FilesystemBackend.add(42, image_file,
+                                                         FILESYSTEM_OPTIONS)
 
         self.assertEquals(expected_location, location)
         self.assertEquals(expected_file_size, size)
+        self.assertEquals(expected_checksum, checksum)
 
         url_pieces = urlparse.urlparse("file:///tmp/glance-tests/42")
         new_image_file = FilesystemBackend.get(url_pieces)
@@ -110,7 +116,7 @@
                    'filesystem_store_datadir': stubs.FAKE_FILESYSTEM_ROOTDIR}
         self.assertRaises(exception.Duplicate,
                           FilesystemBackend.add,
-                          2, image_file, options)
+                          2, image_file, FILESYSTEM_OPTIONS)
 
     def test_delete(self):
         """

=== modified file 'tests/unit/test_swift_store.py'
--- tests/unit/test_swift_store.py	2011-03-16 16:45:01 +0000
+++ tests/unit/test_swift_store.py	2011-03-25 19:01:10 +0000
@@ -70,15 +70,20 @@
             if hasattr(contents, 'read'):
                 fixture_object = StringIO.StringIO()
                 chunk = contents.read(SwiftBackend.CHUNKSIZE)
+                checksum = hashlib.md5()
                 while chunk:
                     fixture_object.write(chunk)
+                    checksum.update(chunk)
                     chunk = contents.read(SwiftBackend.CHUNKSIZE)
+                etag = checksum.hexdigest()
             else:
                 fixture_object = StringIO.StringIO(contents)
+                etag = hashlib.md5(fixture_object.getvalue()).hexdigest()
+            read_len = fixture_object.len
             fixture_objects[fixture_key] = fixture_object
             fixture_headers[fixture_key] = {
-                'content-length': fixture_object.len,
-                'etag': hashlib.md5(fixture_object.read()).hexdigest()}
+                'content-length': read_len,
+                'etag': etag}
             return fixture_headers[fixture_key]['etag']
         else:
             msg = ("Object PUT failed - Object with key %s already exists"
@@ -226,8 +231,9 @@
     def test_add(self):
         """Test that we can add an image via the swift backend"""
         expected_image_id = 42
-        expected_swift_size = 1024 * 5  # 5K
+        expected_swift_size = FIVE_KB
         expected_swift_contents = "*" * expected_swift_size
+        expected_checksum = hashlib.md5(expected_swift_contents).hexdigest()
         expected_location = format_swift_location(
             SWIFT_OPTIONS['swift_store_user'],
             SWIFT_OPTIONS['swift_store_key'],
@@ -236,10 +242,12 @@
             expected_image_id)
         image_swift = StringIO.StringIO(expected_swift_contents)
 
-        location, size = SwiftBackend.add(42, image_swift, SWIFT_OPTIONS)
+        location, size, checksum = SwiftBackend.add(42, image_swift,
+                                                    SWIFT_OPTIONS)
 
         self.assertEquals(expected_location, location)
         self.assertEquals(expected_swift_size, size)
+        self.assertEquals(expected_checksum, checksum)
 
         url_pieces = urlparse.urlparse(expected_location)
         new_image_swift = SwiftBackend.get(url_pieces)
@@ -280,8 +288,9 @@
         options['swift_store_create_container_on_put'] = 'True'
         options['swift_store_container'] = 'noexist'
         expected_image_id = 42
-        expected_swift_size = 1024 * 5  # 5K
+        expected_swift_size = FIVE_KB
         expected_swift_contents = "*" * expected_swift_size
+        expected_checksum = hashlib.md5(expected_swift_contents).hexdigest()
         expected_location = format_swift_location(
             options['swift_store_user'],
             options['swift_store_key'],
@@ -290,10 +299,12 @@
             expected_image_id)
         image_swift = StringIO.StringIO(expected_swift_contents)
 
-        location, size = SwiftBackend.add(42, image_swift, options)
+        location, size, checksum = SwiftBackend.add(42, image_swift,
+                                                    options)
 
         self.assertEquals(expected_location, location)
         self.assertEquals(expected_swift_size, size)
+        self.assertEquals(expected_checksum, checksum)
 
         url_pieces = urlparse.urlparse(expected_location)
         new_image_swift = SwiftBackend.get(url_pieces)