Merge lp:~jaypipes/glance/bug713154 into lp:~hudson-openstack/glance/trunk

[Jay Pipes]

Completes the S3 storage backend. The original code did not actually fit
the API from boto it turned out, and the stubs that were in the unit test
were hiding this fact.

Adds a unit test that gets S3 testing up to snuff with the Swift backend.

[Brian Waldon (bcwaldon)]

I could successfully upload and download images through glance-cli and the api with this code over http. There are a few things I would like to bring up:

1) The suggested s3_store_bucket value did not work for me. Amazon rejected it complaining about uppercase characters.

2) When I tried https (s3_store_host=http://s3.amazonaws.com), upload worked but download failed with the following exception: "UnsupportedBackend: No backend found for 's3+https'"

3) Are the packages going to be updated with the boto dependency?

4) Can you update this doc page w/ s3 configuration? http://glance.openstack.org/configuring.html

Overall, great work, Jay. I'd love to get this in for D3.

review: Needs Fixing

[Brian Waldon (bcwaldon)]

If I try to upload to s3 with s3_store_create_bucket_on_put set to False, I get the following output: "Error uploading image: global name 'httplib' is not defined". It correctly prevents the creation of the bucket, it just looks like bad error handling:

review: Needs Fixing

[Jay Pipes (jaypipes)]

> I could successfully upload and download images through glance-cli and the api
> with this code over http. There are a few things I would like to bring up:
>
> 1) The suggested s3_store_bucket value did not work for me. Amazon rejected it
> complaining about uppercase characters.

Fixed. Added a note in the docs and in the configuration file.

> 2) When I tried https (s3_store_host=http://s3.amazonaws.com), upload worked
> but download failed with the following exception: "UnsupportedBackend: No
> backend found for 's3+https'"

Fixed.

> 3) Are the packages going to be updated with the boto dependency?

Yes, though the larger question is how best to keep these kinds of things in-sync...

> 4) Can you update this doc page w/ s3 configuration?
> http://glance.openstack.org/configuring.html

Fixed.

Cheers,
jay

[Jay Pipes (jaypipes)]

> If I try to upload to s3 with s3_store_create_bucket_on_put set to False, I
> get the following output: "Error uploading image: global name 'httplib' is not
> defined". It correctly prevents the creation of the bucket, it just looks like
> bad error handling:

Fixed that, too. Just missing an import.

-jay

[Brian Waldon (bcwaldon)]

> > If I try to upload to s3 with s3_store_create_bucket_on_put set to False, I
> > get the following output: "Error uploading image: global name 'httplib' is
> not
> > defined". It correctly prevents the creation of the bucket, it just looks
> like
> > bad error handling:
>
> Fixed that, too. Just missing an import.

Now I'm getting "Error uploading image: global name 'config' is not defined" with that flag set to True or False with a non-existant bucket. Looks like this line in glance/store/s3.py is the culprit:

add_bucket = config.get_option(options,
                                's3_store_create_bucket_on_put',
                                type='bool', default=False)

[Brian Waldon (bcwaldon)]

Thanks for the documentation. It's should be much clearer now. However, I'm still having the same issue with #2 above (uploading with https). Any idea what it might be?

[Jay Pipes (jaypipes)]

> Now I'm getting "Error uploading image: global name 'config' is not defined"
> with that flag set to True or False with a non-existant bucket. Looks like
> this line in glance/store/s3.py is the culprit:
>
> add_bucket = config.get_option(options,
> 's3_store_create_bucket_on_put',
> type='bool', default=False)

Strange. This doesn't come up for me...

I think this might be a MacOSX thingie. I'll log into our MacOSX server testing box to see what I can reproduce.

jay

[Jay Pipes (jaypipes)]

> Now I'm getting "Error uploading image: global name 'config' is not defined"
> with that flag set to True or False with a non-existant bucket. Looks like
> this line in glance/store/s3.py is the culprit:
>
> add_bucket = config.get_option(options,
> 's3_store_create_bucket_on_put',
> type='bool', default=False)

Hmm, so I wasn't hitting this because the bucket I was using in testing was already created. I added the import for common.config and this should be fixed. I'll still look into the MacOSX testing.

-jay

[Brian Waldon (bcwaldon)]

> > Now I'm getting "Error uploading image: global name 'config' is not defined"
> > with that flag set to True or False with a non-existant bucket. Looks like
> > this line in glance/store/s3.py is the culprit:
> >
> > add_bucket = config.get_option(options,
> > 's3_store_create_bucket_on_put',
> > type='bool', default=False)
>
> Strange. This doesn't come up for me...
>
> I think this might be a MacOSX thingie. I'll log into our MacOSX server
> testing box to see what I can reproduce.
>
> jay

I was testing on Maverick, btw.

[Jay Pipes (jaypipes)]

Could you try it again, Brian?

[Brian Waldon (bcwaldon)]

> Could you try it again, Brian?

The s3_store_create_bucket_on_put bug is now fixed.

[Brian Waldon (bcwaldon)]

Lamar and I came up with this diff to address the other outstanding problem:

=== modified file 'glance/store/location.py'
--- glance/store/location.py 2011-07-13 20:15:46 +0000
+++ glance/store/location.py 2011-07-25 19:23:09 +0000
@@ -78,7 +78,8 @@
     pieces = urlparse.urlparse(uri)
     if pieces.scheme not in SCHEME_TO_STORE_MAP.keys():
         raise exception.UnknownScheme(pieces.scheme)
- loc = Location(pieces.scheme, uri=uri)
+ store_name = SCHEME_TO_STORE_MAP[pieces.scheme]
+ loc = Location(store_name, uri=uri)
     return loc

[Brian Waldon (bcwaldon)]

Adding an image with a non-existant object works, but subsequent requests for that image fail with this traceback:

2011-07-25 19:36:11 DEBUG [eventlet.wsgi.server] Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/eventlet/wsgi.py", line 347, in handle_one_response
    for data in result:
  File "/usr/lib/pymodules/python2.6/glance/api/v1/images.py", line 194, in image_iterator
    options=self.options)
  File "/usr/lib/pymodules/python2.6/glance/store/__init__.py", line 86, in get_from_backend
    return backend_class.get(loc, **kwargs)
  File "/usr/lib/pymodules/python2.6/glance/store/s3.py", line 192, in get
    key = get_key(bucket_obj, loc.key)
  File "/usr/lib/pymodules/python2.6/glance/store/s3.py", line 378, in get_key
    if not key.exists():
AttributeError: 'NoneType' object has no attribute 'exists'

[Brian Waldon (bcwaldon)]

> Adding an image with a non-existant object works, but subsequent requests for
> that image fail with this traceback:
>
>
> 2011-07-25 19:36:11 DEBUG [eventlet.wsgi.server] Traceback (most recent
> call last):
> File "/usr/lib/python2.6/dist-packages/eventlet/wsgi.py", line 347, in
> handle_one_response
> for data in result:
> File "/usr/lib/pymodules/python2.6/glance/api/v1/images.py", line 194, in
> image_iterator
> options=self.options)
> File "/usr/lib/pymodules/python2.6/glance/store/__init__.py", line 86, in
> get_from_backend
> return backend_class.get(loc, **kwargs)
> File "/usr/lib/pymodules/python2.6/glance/store/s3.py", line 192, in get
> key = get_key(bucket_obj, loc.key)
> File "/usr/lib/pymodules/python2.6/glance/store/s3.py", line 378, in get_key
> if not key.exists():
> AttributeError: 'NoneType' object has no attribute 'exists'

To be clear, I was using glance add and a location option to specifiy a valid s3 connection string to an invalid object identifier.

[Brian Waldon (bcwaldon)]

Trying to delete an image with a non-existant s3 object also fails with the same traceback. This raises the following question: Should we be trying to delete s3 objects when we delete an image from the glance api? I do understand we could just remove the image from the registry, but our command-line tool doesn't support that.

[Jay Pipes (jaypipes)]

> Lamar and I came up with this diff to address the other outstanding problem:
>
> === modified file 'glance/store/location.py'
> --- glance/store/location.py 2011-07-13 20:15:46 +0000
> +++ glance/store/location.py 2011-07-25 19:23:09 +0000
> @@ -78,7 +78,8 @@
> pieces = urlparse.urlparse(uri)
> if pieces.scheme not in SCHEME_TO_STORE_MAP.keys():
> raise exception.UnknownScheme(pieces.scheme)
> - loc = Location(pieces.scheme, uri=uri)
> + store_name = SCHEME_TO_STORE_MAP[pieces.scheme]
> + loc = Location(store_name, uri=uri)
> return loc

Hmm, actually, the code in Location._get_store_location() should handle that. The root of the issue is the glance.store.get_backend_class() method. This is now called with the URI scheme or the store name, so a quick hack fixed that issue. This mess all gets cleaned up substantially in the refactor-stores branch, btw...

Pushed a fix and a test case testing variations of URLs in s3_store_host. Please re-verify. Thanks for your patience, Brian^2.

-jay

[Jay Pipes (jaypipes)]

Hold on, still working through some issues...

[Brian Waldon (bcwaldon)]

I can now fetch images over s3+https. Only outstanding issue is deletion of images with s3 store where object is non-existent.

[Jay Pipes (jaypipes)]

Added a functional test case for the non-existing image. I had already fixed that in a prior patch, but this should verify the behaviour is fixed.

Please do a final review :)
-jay

[Brian Waldon (bcwaldon)]

Deletion is now handled properly! I discovered that you can't successfully add a remote image (using a location attribute) with glance-cli. You can't set the size property, so you get zero data back! I could successfully add a remote image through the glance api. Maybe this is a bug to address in the future?

I have two minor comments, then this branch is good to go in:

1) You can't have a plus sign in your bucket name. The suggestion in the config file is very misleading :)

2) I know you didn't add this in your patch, but can you add a space between 'option' and 'to' in this error message: "Please set the s3_store_create_bucket_on_put optionto add bucket to S3 automatically"

[Brian Waldon (bcwaldon)]

And you need to merge trunk :)

[Jay Pipes (jaypipes)]

On Tue, Jul 26, 2011 at 10:02 AM, Brian Waldon
<email address hidden> wrote:
> And you need to merge trunk :)

OK, fixed and merged. Please try again. fingers crossed...

-jay

[Brian Waldon (bcwaldon)]

Excellent work, Jay! All of my tests pass.

review: Approve

[Brian Lamar (blamar)]

These are notes really, rather than a review, because the code looks good to me and the tests look awesome:

309 + from boto.s3.connection import S3Connection
354 + from boto.s3.connection import S3Connection

Any reason for these imports to be where it is? Waldon says this is probably a dependency management thing (you don't want to require boto if they're not using S3, right?), but I think there should be a blueprint for better optional importing in both Nova and Glance if this is the case because it's done several different ways.

252 + try:
253 + while True:
254 + chunk = self.fp.read(ChunkedFile.CHUNKSIZE)
255 + if chunk:
256 + yield chunk
257 + else:
258 + break
259 + finally:
260 + self.close()

Might consider shorter "with" syntax here, I think it would just be:

with self.fp:
    yield self.fp.read(ChunkedFile.CHUNKSIZE)

review: Approve

[Jay Pipes (jaypipes)]

On Tue, Jul 26, 2011 at 11:34 AM, Brian Lamar <email address hidden> wrote:
> Review: Approve
> These are notes really, rather than a review, because the code looks good to me and the tests look awesome:
>
> 309     +        from boto.s3.connection import S3Connection
> 354     +        from boto.s3.connection import S3Connection
>
> Any reason for these imports to be where it is? Waldon says this is probably a dependency management thing (you don't want to require boto if they're not using S3, right?), but I think there should be a blueprint for better optional importing in both Nova and Glance if this is the case because it's done several different ways.

Yup, and they get cleaned up in my refactor-stores branch...

> 252     +        try:
> 253     +            while True:
> 254     +                chunk = self.fp.read(ChunkedFile.CHUNKSIZE)
> 255     +                if chunk:
> 256     +                    yield chunk
> 257     +                else:
> 258     +                    break
> 259     +        finally:
> 260     +            self.close()
>
> Might consider shorter "with" syntax here, I think it would just be:
>
> with self.fp:
>    yield self.fp.read(ChunkedFile.CHUNKSIZE)

Noted. I'll clean that up in the refactor-stores branch, too :)

Cheers!
jay

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~jaypipes/glance/bug713154 into lp:glance failed. Below is the output from the failed tests.

running test
running egg_info
creating glance.egg-info
writing glance.egg-info/PKG-INFO
writing top-level names to glance.egg-info/top_level.txt
writing dependency_links to glance.egg-info/dependency_links.txt
writing manifest file 'glance.egg-info/SOURCES.txt'
reading manifest file 'glance.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
warning: no files found matching 'ChangeLog'
writing manifest file 'glance.egg-info/SOURCES.txt'
running build_ext

We test the following: ... ok
We test the following: ... ok
Test for LP Bugs #736295, #767203 ... ok
We test conditions that produced LP Bug #768969, where an image ... ok
Set up three test images and ensure each query param filter works ... ok
We test the following sequential series of actions: ... ok
Ensure marker and limit query params work ... ok
Set up three test images and ensure each query param filter works ... ok
We test the process flow where a user registers an image ... ok
A test against the actual datastore backend for the registry ... ok
A test that errors coming from the POST API do not ... ok
We test that various calls to the images and root endpoints are ... ok
Upload initial image, then attempt to upload duplicate image ... ok
Set up four test images and ensure each query param filter works ... ok
We test the following sequential series of actions: ... ok
Ensure marker and limit query params work ... ok
Set up three test images and ensure each query param filter works ... ok
We test the process flow where a user registers an image ... ok
A test against the actual datastore backend for the registry ... ok
A test that errors coming from the POST API do not ... ok
We test that various calls to the images and root endpoints are ... ok
Test logging output proper when verbose and debug ... ok
Test logging output proper when verbose and debug ... ok
A test for LP bug #704854 -- Exception thrown by registry ... ok
We test the following: ... ERROR
We test the following: ... ERROR
test that images don't get deleted immediatly and that the scrubber ... ok
test that images get deleted immediately by default ... ok
Tests raises BadRequest for invalid store header ... ok
Tests to add a basic image in the file store ... ok
Tests creates a queued image for no body and no loc header ... ok
Tests creates a queued image for no body and no loc header ... ok
Test that the image contents are checksummed properly ... ok
test_bad_container_format (tests.unit.test_api.TestGlanceAPI) ... ok
test_bad_disk_format (tests.unit.test_api.TestGlanceAPI) ... ok
test_delete_image (tests.unit.test_api.TestGlanceAPI) ... ok
test_delete_non_exists_image (tests.unit.test_api.TestGlanceAPI) ... ok
Here, we try to delete an image that is in the queued state. ... ok
Test that the ETag header matches the x-image-meta-checksum ... ok
Tests that the /images/detail registry API returns a 400 ... ok
Tests that the /images registry API returns list of ... ok
Test that the image contents are checksummed properly ... ok
Test for HEAD /images/<ID> ... ok
test_show_image_basic (tests.unit.test_api.T...

[Brian Waldon (bcwaldon)]

Works for me.

review: Approve

[lp:~jaypipes/glance/bug713154]

148. By Jay Pipes on 2011-07-29

Fix for boto1.9b issue 540 (http://code.google.com/p/boto/issues/detail?id=540)

[Jay Pipes (jaypipes)]

I pushed a fix for boto bug 540: http://code.google.com/p/boto/issues/detail?id=540

Please re-review.

-jay

[Brian Waldon (bcwaldon)]

This definitely handles the boto issue. Tested at r147 w/ boto==1.9b, tests fail. Tested at r148 w/ boto==1.9b, tests pass.

review: Approve

=== modified file 'doc/source/configuring.rst'
--- doc/source/configuring.rst	2011-05-11 23:52:06 +0000
+++ doc/source/configuring.rst	2011-07-29 18:46:57 +0000
@@ -150,6 +150,9 @@
 Sets the storage backend to use by default when storing images in Glance.
 Available options for this option are (``file``, ``swift``, or ``s3``).
 
+Configuring the Filesystem Storage Backend
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
 * ``filesystem_store_datadir=PATH``
 
 Optional. Default: ``/var/lib/glance/images/``
@@ -163,6 +166,9 @@
 not exist. Ensure that the user that ``glance-api`` runs under has write
 permissions to this directory.
 
+Configuring the Swift Storage Backend
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
 * ``swift_store_auth_address=URL``
 
 Required when using the Swift storage backend.
@@ -219,6 +225,82 @@
 If true, Glance will attempt to create the container ``swift_store_container``
 if it does not exist.
 
+Configuring the S3 Storage Backend
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+* ``s3_store_host=URL``
+
+Required when using the S3 storage backend.
+
+Can only be specified in configuration files.
+
+`This option is specific to the S3 storage backend.`
+
+Default: s3.amazonaws.com
+
+Sets the main service URL supplied to S3 when making calls to its storage
+system. For more information about the S3 authentication system, please
+see the `S3 documentation <http://aws.amazon.com/documentation/s3/>`_ 
+
+* ``s3_store_access_key=ACCESS_KEY``
+
+Required when using the S3 storage backend.
+
+Can only be specified in configuration files.
+
+`This option is specific to the S3 storage backend.`
+
+Sets the access key to authenticate against the ``s3_store_host`` with.
+
+You should set this to your 20-character Amazon AWS access key.
+
+* ``s3_store_secret_key=SECRET_KEY``
+
+Required when using the S3 storage backend.
+
+Can only be specified in configuration files.
+
+`This option is specific to the S3 storage backend.`
+
+Sets the secret key to authenticate against the
+``s3_store_host`` with for the access key ``s3_store_access_key``.
+
+You should set this to your 40-character Amazon AWS secret key.
+
+* ``s3_store_bucket=BUCKET``
+
+Required when using the S3 storage backend.
+
+Can only be specified in configuration files.
+
+`This option is specific to the S3 storage backend.`
+
+Sets the name of the bucket to use for Glance images in S3.
+
+Note that the namespace for S3 buckets is **global**, and
+therefore you must use a name for the bucket that is unique. It
+is recommended that you use a combination of your AWS access key,
+**lowercased** with "glance".
+
+For instance if your Amazon AWS access key is:
+
+``ABCDEFGHIJKLMNOPQRST``
+
+then make your bucket value be:
+
+``abcdefghijklmnopqrstglance``
+
+* ``s3_store_create_bucket_on_put``
+
+Optional. Default: ``False``
+
+Can only be specified in configuration files.
+
+`This option is specific to the S3 storage backend.`
+
+If true, Glance will attempt to create the bucket ``s3_store_bucket``
+if it does not exist.
+
 Configuring the Glance Registry
 -------------------------------
 

=== modified file 'etc/glance-api.conf'
--- etc/glance-api.conf	2011-07-28 00:50:04 +0000
+++ etc/glance-api.conf	2011-07-29 18:46:57 +0000
@@ -51,6 +51,29 @@
 # Do we create the container if it does not exist?
 swift_store_create_container_on_put = False
 
+# ============ S3 Store Options =============================
+
+# Address where the S3 authentication service lives
+s3_store_host = 127.0.0.1:8080/v1.0/
+
+# User to authenticate against the S3 authentication service
+s3_store_access_key = <20-char AWS access key>
+
+# Auth key for the user authenticating against the
+# S3 authentication service
+s3_store_secret_key = <40-char AWS secret key>
+
+# Container within the account that the account should use
+# for storing images in S3. Note that S3 has a flat namespace,
+# so you need a unique bucket name for your glance images. An
+# easy way to do this is append your AWS access key to "glance".
+# S3 buckets in AWS *must* be lowercased, so remember to lowercase
+# your AWS access key if you use it in your bucket name below!
+s3_store_bucket = <lowercased 20-char aws access key>glance
+
+# Do we create the bucket if it does not exist?
+s3_store_create_bucket_on_put = False
+
 # ============ Image Cache Options ========================
 
 image_cache_enabled = False

=== modified file 'glance/api/v1/images.py'
--- glance/api/v1/images.py	2011-07-28 00:44:29 +0000
+++ glance/api/v1/images.py	2011-07-29 18:46:57 +0000
@@ -331,6 +331,7 @@
         try:
             logger.debug("Uploading image data for image %(image_id)s "
                          "to %(store_name)s store", locals())
+            req.make_body_seekable()
             location, size, checksum = store.add(image_meta['id'],
                                                  req.body_file,
                                                  self.options)
@@ -436,6 +437,10 @@
         :retval Mapping of updated image data
         """
         image_id = image_meta['id']
+        # This is necessary because of a bug in Webob 1.0.2 - 1.0.7
+        # See: https://bitbucket.org/ianb/webob/
+        # issue/12/fix-for-issue-6-broke-chunked-transfer
+        req.is_body_readable = True
         location = self._upload(req, image_meta)
         return self._activate(req, image_id, location)
 

=== modified file 'glance/store/s3.py'
--- glance/store/s3.py	2011-07-27 18:15:47 +0000
+++ glance/store/s3.py	2011-07-29 18:46:57 +0000
@@ -15,14 +15,19 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-"""The s3 backend adapter"""
+"""Storage backend for S3 or Storage Servers that follow the S3 Protocol"""
 
+import logging
+import httplib
 import urlparse
 
+from glance.common import config
 from glance.common import exception
 import glance.store
 import glance.store.location
 
+logger = logging.getLogger('glance.store.s3')
+
 glance.store.location.add_scheme_map({'s3': 's3',
                                       's3+http': 's3',
                                       's3+https': 's3'})
@@ -45,10 +50,17 @@
         self.scheme = self.specs.get('scheme', 's3')
         self.accesskey = self.specs.get('accesskey')
         self.secretkey = self.specs.get('secretkey')
-        self.s3serviceurl = self.specs.get('s3serviceurl')
+        s3_host = self.specs.get('s3serviceurl')
         self.bucket = self.specs.get('bucket')
         self.key = self.specs.get('key')
 
+        if s3_host.startswith('https://'):
+            self.scheme = 's3+https'
+            s3_host = s3_host[8:].strip('/')
+        elif s3_host.startswith('http://'):
+            s3_host = s3_host[7:].strip('/')
+        self.s3serviceurl = s3_host.strip('/')
+
     def _get_credstring(self):
         if self.accesskey:
             return '%s:%s@' % (self.accesskey, self.secretkey)
@@ -117,7 +129,7 @@
             self.key = path_parts.pop()
             self.bucket = path_parts.pop()
             if len(path_parts) > 0:
-                self.s3serviceurl = '/'.join(path_parts)
+                self.s3serviceurl = '/'.join(path_parts).strip('/')
             else:
                 reason = "Badly formed S3 URI. Missing s3 service URL."
                 raise exception.BadStoreUri(uri, reason)
@@ -126,13 +138,63 @@
             raise exception.BadStoreUri(uri, reason)
 
 
+class ChunkedFile(object):
+
+    """
+    We send this back to the Glance API server as
+    something that can iterate over a ``boto.s3.key.Key``
+    """
+
+    CHUNKSIZE = 65536
+
+    def __init__(self, fp):
+        self.fp = fp
+
+    def __iter__(self):
+        """Return an iterator over the image file"""
+        try:
+            while True:
+                chunk = self.fp.read(ChunkedFile.CHUNKSIZE)
+                if chunk:
+                    yield chunk
+                else:
+                    break
+        finally:
+            self.close()
+
+    def getvalue(self):
+        """Return entire string value... used in testing"""
+        data = ""
+        self.len = 0
+        for chunk in self:
+            read_bytes = len(chunk)
+            data = data + chunk
+            self.len = self.len + read_bytes
+        return data
+
+    def close(self):
+        """Close the internal file pointer"""
+        if self.fp:
+            self.fp.close()
+            self.fp = None
+
+
 class S3Backend(glance.store.Backend):
     """An implementation of the s3 adapter."""
 
-    EXAMPLE_URL = "s3://ACCESS_KEY:SECRET_KEY@s3_url/bucket/file.gz.0"
-
-    @classmethod
-    def get(cls, location, expected_size, conn_class=None):
+    EXAMPLE_URL = "s3://<ACCESS_KEY>:<SECRET_KEY>@<S3_URL>/<BUCKET>/<OBJ>"
+
+    @classmethod
+    def _option_get(cls, options, param):
+        result = options.get(param)
+        if not result:
+            msg = ("Could not find %s in configuration options." % param)
+            logger.error(msg)
+            raise glance.store.BackendException(msg)
+        return result
+
+    @classmethod
+    def get(cls, location, expected_size=None, options=None):
         """
         Takes a `glance.store.location.Location` object that indicates
         where to find the image file, and returns a generator from S3
@@ -141,71 +203,202 @@
         :location `glance.store.location.Location` object, supplied
                   from glance.store.location.get_location_from_uri()
         """
-        if conn_class:
-            pass
+        loc = location.store_location
+        from boto.s3.connection import S3Connection
+
+        s3_conn = S3Connection(loc.accesskey, loc.secretkey,
+                               host=loc.s3serviceurl,
+                               is_secure=(loc.scheme == 's3+https'))
+        bucket_obj = get_bucket(s3_conn, loc.bucket)
+
+        key = get_key(bucket_obj, loc.key)
+
+        logger.debug("Retrieved image object from S3 using "
+                     "(s3_host=%s, access_key=%s, "
+                     "bucket=%s, key=%s)" % (loc.s3serviceurl, loc.accesskey,
+                                             loc.bucket, loc.key))
+
+        if expected_size and (key.size != expected_size):
+            msg = "Expected %s bytes, got %s" % (expected_size, key.size)
+            logger.error(msg)
+            raise glance.store.BackendException(msg)
+
+        key.BufferSize = cls.CHUNKSIZE
+        return ChunkedFile(key)
+
+    @classmethod
+    def add(cls, id, data, options):
+        """
+        Stores image data to S3 and returns a location that the image was
+        written to.
+
+        S3 writes the image data using the scheme:
+            s3://<ACCESS_KEY>:<SECRET_KEY>@<S3_URL>/<BUCKET>/<OBJ>
+        where:
+            <USER> = ``s3_store_user``
+            <KEY> = ``s3_store_key``
+            <S3_HOST> = ``s3_store_host``
+            <BUCKET> = ``s3_store_bucket``
+            <ID> = The id of the image being added
+
+        :param id: The opaque image identifier
+        :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,
+                and the size in bytes of the data written
+        """
+        from boto.s3.connection import S3Connection
+
+        # TODO(jaypipes): This needs to be checked every time
+        # because of the decision to make glance.store.Backend's
+        # interface all @classmethods. This is inefficient. Backend
+        # should be a stateful object with options parsed once in
+        # a constructor.
+        s3_host = cls._option_get(options, 's3_store_host')
+        access_key = cls._option_get(options, 's3_store_access_key')
+        secret_key = cls._option_get(options, 's3_store_secret_key')
+        # NOTE(jaypipes): Need to encode to UTF-8 here because of a
+        # bug in the HMAC library that boto uses.
+        # See: http://bugs.python.org/issue5285
+        # See: http://trac.edgewall.org/ticket/8083
+        access_key = access_key.encode('utf-8')
+        secret_key = secret_key.encode('utf-8')
+        bucket = cls._option_get(options, 's3_store_bucket')
+
+        scheme = 's3'
+        if s3_host.startswith('https://'):
+            scheme = 'swift+https'
+            full_s3_host = s3_host
+        elif s3_host.startswith('http://'):
+            full_s3_host = s3_host
         else:
-            import boto.s3.connection
-            conn_class = boto.s3.connection.S3Connection
-
-        loc = location.store_location
-
-        # Close the connection when we're through.
-        with conn_class(loc.accesskey, loc.secretkey,
-                        host=loc.s3serviceurl) as s3_conn:
-            bucket = cls._get_bucket(s3_conn, loc.bucket)
-
-            # Close the key when we're through.
-            with cls._get_key(bucket, loc.obj) as key:
-                if not key.size == expected_size:
-                    raise glance.store.BackendException(
-                        "Expected %s bytes, got %s" %
-                        (expected_size, key.size))
-
-                key.BufferSize = cls.CHUNKSIZE
-                for chunk in key:
-                    yield chunk
+            full_s3_host = 'http://' + s3_host  # Defaults http
+
+        loc = StoreLocation({'scheme': scheme,
+                             'bucket': bucket,
+                             'key': id,
+                             's3serviceurl': full_s3_host,
+                             'accesskey': access_key,
+                             'secretkey': secret_key})
+
+        s3_conn = S3Connection(loc.accesskey, loc.secretkey,
+                               host=loc.s3serviceurl,
+                               is_secure=(loc.scheme == 's3+https'))
+
+        create_bucket_if_missing(bucket, s3_conn, options)
+
+        bucket_obj = get_bucket(s3_conn, bucket)
+        obj_name = str(id)
+
+        key = bucket_obj.get_key(obj_name)
+        if key and key.exists():
+            raise exception.Duplicate("S3 already has an image at "
+                                      "location %s" % loc.get_uri())
+
+        logger.debug("Adding image object to S3 using "
+                     "(s3_host=%(s3_host)s, access_key=%(access_key)s, "
+                     "bucket=%(bucket)s, key=%(obj_name)s)" % locals())
+
+        key = bucket_obj.new_key(obj_name)
+
+        # OK, now upload the data into the key
+        obj_md5, _base64_digest = key.compute_md5(data)
+        key.set_contents_from_file(data, replace=False)
+        size = key.size
+
+        return (loc.get_uri(), size, obj_md5)
 
     @classmethod
-    def delete(cls, location, conn_class=None):
+    def delete(cls, location, options=None):
         """
-        Takes a `glance.store.location.Location` object that indicates
-        where to find the image file to delete
+        Delete an object in a specific location
 
         :location `glance.store.location.Location` object, supplied
                   from glance.store.location.get_location_from_uri()
         """
-        if conn_class:
-            pass
-        else:
-            conn_class = boto.s3.connection.S3Connection
-
         loc = location.store_location
-
-        # Close the connection when we're through.
-        with conn_class(loc.accesskey, loc.secretkey,
-                        host=loc.s3serviceurl) as s3_conn:
-            bucket = cls._get_bucket(s3_conn, loc.bucket)
-
-            # Close the key when we're through.
-            with cls._get_key(bucket, loc.obj) as key:
-                return key.delete()
-
-    @classmethod
-    def _get_bucket(cls, conn, bucket_id):
-        """Get a bucket from an s3 connection"""
-
-        bucket = conn.get_bucket(bucket_id)
-        if not bucket:
-            raise glance.store.BackendException("Could not find bucket: %s" %
-                bucket_id)
-
-        return bucket
-
-    @classmethod
-    def _get_key(cls, bucket, obj):
-        """Get a key from a bucket"""
-
-        key = bucket.get_key(obj)
-        if not key:
-            raise glance.store.BackendException("Could not get key: %s" % key)
-        return key
+        from boto.s3.connection import S3Connection
+        s3_conn = S3Connection(loc.accesskey, loc.secretkey,
+                               host=loc.s3serviceurl,
+                               is_secure=(loc.scheme == 's3+https'))
+        bucket_obj = get_bucket(s3_conn, loc.bucket)
+
+        # Close the key when we're through.
+        key = get_key(bucket_obj, loc.key)
+
+        logger.debug("Deleting image object from S3 using "
+                     "(s3_host=%s, access_key=%s, "
+                     "bucket=%s, key=%s)" % (loc.s3serviceurl, loc.accesskey,
+                                             loc.bucket, loc.key))
+
+        return key.delete()
+
+
+def get_bucket(conn, bucket_id):
+    """
+    Get a bucket from an s3 connection
+
+    :param conn: The ``boto.s3.connection.S3Connection``
+    :param bucket_id: ID of the bucket to fetch
+    :raises ``glance.exception.NotFound`` if bucket is not found.
+    """
+
+    bucket = conn.get_bucket(bucket_id)
+    if not bucket:
+        msg = ("Could not find bucket with ID %(bucket_id)s") % locals()
+        logger.error(msg)
+        raise exception.NotFound(msg)
+
+    return bucket
+
+
+def create_bucket_if_missing(bucket, s3_conn, options):
+    """
+    Creates a missing bucket in S3 if the
+    ``s3_store_create_bucket_on_put`` option is set.
+
+    :param bucket: Name of bucket to create
+    :param s3_conn: Connection to S3
+    :param options: Option mapping
+    """
+    from boto.exception import S3ResponseError
+    try:
+        s3_conn.get_bucket(bucket)
+    except S3ResponseError, e:
+        if e.status == httplib.NOT_FOUND:
+            add_bucket = config.get_option(options,
+                                's3_store_create_bucket_on_put',
+                                type='bool', default=False)
+            if add_bucket:
+                try:
+                    s3_conn.create_bucket(bucket)
+                except S3ResponseError, e:
+                    msg = ("Failed to add bucket to S3.\n"
+                           "Got error from S3: %(e)s" % locals())
+                    raise glance.store.BackendException(msg)
+            else:
+                msg = ("The bucket %(bucket)s does not exist in "
+                       "S3. Please set the "
+                       "s3_store_create_bucket_on_put option "
+                       "to add bucket to S3 automatically."
+                       % locals())
+                raise glance.store.BackendException(msg)
+
+
+def get_key(bucket, obj):
+    """
+    Get a key from a bucket
+
+    :param bucket: The ``boto.s3.Bucket``
+    :param obj: Object to get the key for
+    :raises ``glance.exception.NotFound`` if key is not found.
+    """
+
+    key = bucket.get_key(obj)
+    if not key or not key.exists():
+        msg = ("Could not find key %(obj)s in bucket %(bucket)s") % locals()
+        logger.error(msg)
+        raise exception.NotFound(msg)
+    return key

=== modified file 'tests/functional/__init__.py'
--- tests/functional/__init__.py	2011-07-23 03:38:20 +0000
+++ tests/functional/__init__.py	2011-07-29 18:46:57 +0000
@@ -137,6 +137,10 @@
                                          "api.pid")
         self.log_file = os.path.join(self.test_dir, "api.log")
         self.registry_port = registry_port
+        self.s3_store_host = "s3.amazonaws.com"
+        self.s3_store_access_key = ""
+        self.s3_store_secret_key = ""
+        self.s3_store_bucket = ""
         self.delayed_delete = delayed_delete
         self.conf_base = """[DEFAULT]
 verbose = %(verbose)s
@@ -148,6 +152,10 @@
 registry_host = 0.0.0.0
 registry_port = %(registry_port)s
 log_file = %(log_file)s
+s3_store_host = %(s3_store_host)s
+s3_store_access_key = %(s3_store_access_key)s
+s3_store_secret_key = %(s3_store_secret_key)s
+s3_store_bucket = %(s3_store_bucket)s
 delayed_delete = %(delayed_delete)s
 
 [pipeline:glance-api]

=== added file 'tests/functional/test_s3.conf'
--- tests/functional/test_s3.conf	1970-01-01 00:00:00 +0000
+++ tests/functional/test_s3.conf	2011-07-29 18:46:57 +0000
@@ -0,0 +1,21 @@
+[DEFAULT]
+
+# Set the following to Amazon S3 credentials that you want
+# to use while functional testing the S3 backend.
+
+# Address where the S3 authentication service lives
+s3_store_host = s3.amazonaws.com
+
+# User to authenticate against the S3 authentication service
+s3_store_access_key = <20-char AWS access key>
+
+# Auth key for the user authenticating against the
+# S3 authentication service
+s3_store_secret_key = <40-char AWS secret key>
+
+# Container within the account that the account should use
+# for storing images in S3. Note that S3 has a flat namespace,
+# so you need a unique bucket name for your glance images. An
+# easy way to do this is append your lower-cased AWS access key
+# to "glance"
+s3_store_bucket = <20-char AWS access key - lowercased>glance

=== added file 'tests/functional/test_s3.py'
--- tests/functional/test_s3.py	1970-01-01 00:00:00 +0000
+++ tests/functional/test_s3.py	2011-07-29 18:46:57 +0000
@@ -0,0 +1,532 @@
+# 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.
+
+"""
+Tests a Glance API server which uses an S3 backend by default
+
+This test requires that a real S3 account is available. It looks
+in a file /tests/functional/test_s3.conf for the credentials to
+use.
+
+Note that this test clears the entire bucket from the S3 account
+for use by the test case, so make sure you supply credentials for
+test accounts only.
+
+If a connection cannot be established, all the test cases are
+skipped.
+"""
+
+import ConfigParser
+import json
+import os
+import tempfile
+import unittest
+
+from tests import functional
+from tests.utils import execute
+
+FIVE_KB = 5 * 1024
+FIVE_GB = 5 * 1024 * 1024 * 1024
+
+
+class TestS3(functional.FunctionalTest):
+
+    """Functional tests for the S3 backend"""
+
+    # Test machines can set the GLANCE_TEST_MIGRATIONS_CONF variable
+    # to override the location of the config file for migration testing
+    CONFIG_FILE_PATH = os.environ.get('GLANCE_TEST_S3_CONF',
+                                      os.path.join('tests', 'functional',
+                                                   'test_s3.conf'))
+
+    def setUp(self):
+        """
+        Test a connection to an S3 store using the credentials
+        found in the environs or /tests/functional/test_s3.conf, if found.
+        If the connection fails, mark all tests to skip.
+        """
+        self.inited = False
+        self.skip_tests = True
+
+        if self.inited:
+            return
+
+        if os.path.exists(TestS3.CONFIG_FILE_PATH):
+            cp = ConfigParser.RawConfigParser()
+            try:
+                cp.read(TestS3.CONFIG_FILE_PATH)
+                defaults = cp.defaults()
+                for key, value in defaults.items():
+                    self.__dict__[key] = value
+            except ConfigParser.ParsingError, e:
+                print ("Failed to read test_s3.conf config file. "
+                       "Got error: %s" % e)
+                super(TestS3, self).setUp()
+                self.inited = True
+                return
+
+        from boto.s3.connection import S3Connection
+        from boto.exception import S3ResponseError
+
+        try:
+            s3_host = self.s3_store_host
+            access_key = self.s3_store_access_key
+            secret_key = self.s3_store_secret_key
+            bucket_name = self.s3_store_bucket
+        except AttributeError, e:
+            print ("Failed to find required configuration options for "
+                   "S3 store. Got error: %s" % e)
+            self.inited = True
+            super(TestS3, self).setUp()
+            return
+
+        s3_conn = S3Connection(access_key, secret_key, host=s3_host)
+
+        self.bucket = None
+        try:
+            buckets = s3_conn.get_all_buckets()
+            for bucket in buckets:
+                if bucket.name == bucket_name:
+                    self.bucket = bucket
+        except S3ResponseError, e:
+            print ("Failed to connect to S3 with credentials,"
+                   "to find bucket. Got error: %s" % e)
+            self.inited = True
+            super(TestS3, self).setUp()
+            return
+        except TypeError, e:
+            # This hack is necessary because of a bug in boto 1.9b:
+            # http://code.google.com/p/boto/issues/detail?id=540
+            print ("Failed to connect to S3 with credentials. "
+                   "Got error: %s" % e)
+            self.inited = True
+            super(TestS3, self).setUp()
+            return
+
+        self.s3_conn = s3_conn
+
+        if not self.bucket:
+            try:
+                self.bucket = s3_conn.create_bucket(bucket_name)
+            except boto.exception.S3ResponseError, e:
+                print ("Failed to create bucket. Got error: %s" % e)
+                self.inited = True
+                super(TestS3, self).setUp()
+                return
+        else:
+            self.clear_bucket()
+
+        self.skip_tests = False
+        self.inited = True
+        self.default_store = 's3'
+
+        super(TestS3, self).setUp()
+
+    def tearDown(self):
+        if not self.skip_tests:
+            self.clear_bucket()
+        super(TestS3, self).tearDown()
+
+    def clear_bucket(self):
+        # It's not possible to simply clear a bucket. You
+        # need to loop over all the keys and delete them
+        # all first...
+        keys = self.bucket.list()
+        for key in keys:
+            key.delete()
+
+    def test_add_list_delete_list(self):
+        """
+        We test the following:
+
+        0. GET /images
+        - Verify no public images
+        1. GET /images/detail
+        - Verify no public images
+        2. HEAD /images/1
+        - Verify 404 returned
+        3. POST /images with public image named Image1 with a location
+        attribute and no custom properties
+        - Verify 201 returned
+        4. HEAD /images/1
+        - Verify HTTP headers have correct information we just added
+        5. GET /images/1
+        - Verify all information on image we just added is correct
+        6. GET /images
+        - Verify the image we just added is returned
+        7. GET /images/detail
+        - Verify the image we just added is returned
+        8. PUT /images/1 with custom properties of "distro" and "arch"
+        - Verify 200 returned
+        9. GET /images/1
+        - Verify updated information about image was stored
+        10. PUT /images/1
+        - Remove a previously existing property.
+        11. PUT /images/1
+        - Add a previously deleted property.
+        """
+        if self.skip_tests:
+            return True
+
+        self.cleanup()
+        self.start_servers(**self.__dict__.copy())
+
+        api_port = self.api_port
+        registry_port = self.registry_port
+
+        # 0. GET /images
+        # Verify no public images
+        cmd = "curl http://0.0.0.0:%d/v1/images" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+        self.assertEqual('{"images": []}', out.strip())
+
+        # 1. GET /images/detail
+        # Verify no public images
+        cmd = "curl http://0.0.0.0:%d/v1/images/detail" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+        self.assertEqual('{"images": []}', out.strip())
+
+        # 2. HEAD /images/1
+        # Verify 404 returned
+        cmd = "curl -i -X HEAD http://0.0.0.0:%d/v1/images/1" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 404 Not Found", status_line)
+
+        # 3. POST /images with public image named Image1
+        # attribute and no custom properties. Verify a 200 OK is returned
+        image_data = "*" * FIVE_KB
+
+        cmd = ("curl -i -X POST "
+               "-H 'Expect: ' "  # Necessary otherwise sends 100 Continue
+               "-H 'Content-Type: application/octet-stream' "
+               "-H 'X-Image-Meta-Name: Image1' "
+               "-H 'X-Image-Meta-Is-Public: True' "
+               "--data-binary \"%s\" "
+               "http://0.0.0.0:%d/v1/images") % (image_data, api_port)
+
+        exitcode, out, err = execute(cmd)
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 201 Created", status_line, out)
+
+        # 4. HEAD /images
+        # Verify image found now
+        cmd = "curl -i -X HEAD http://0.0.0.0:%d/v1/images/1" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 200 OK", status_line)
+        self.assertTrue("X-Image-Meta-Name: Image1" in out)
+
+        # 5. GET /images/1
+        # Verify all information on image we just added is correct
+
+        cmd = "curl -i http://0.0.0.0:%d/v1/images/1" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+
+        self.assertEqual("HTTP/1.1 200 OK", lines.pop(0))
+
+        # Handle the headers
+        image_headers = {}
+        std_headers = {}
+        other_lines = []
+        for line in lines:
+            if line.strip() == '':
+                continue
+            if line.startswith("X-Image"):
+                pieces = line.split(":")
+                key = pieces[0].strip()
+                value = ":".join(pieces[1:]).strip()
+                image_headers[key] = value
+            elif ':' in line:
+                pieces = line.split(":")
+                key = pieces[0].strip()
+                value = ":".join(pieces[1:]).strip()
+                std_headers[key] = value
+            else:
+                other_lines.append(line)
+
+        expected_image_headers = {
+            'X-Image-Meta-Id': '1',
+            'X-Image-Meta-Name': 'Image1',
+            'X-Image-Meta-Is_public': 'True',
+            'X-Image-Meta-Status': 'active',
+            'X-Image-Meta-Disk_format': '',
+            'X-Image-Meta-Container_format': '',
+            'X-Image-Meta-Size': str(FIVE_KB),
+            'X-Image-Meta-Location': 's3://%s:%s@%s/%s/1' % (
+                self.s3_store_access_key,
+                self.s3_store_secret_key,
+                self.s3_store_host,
+                self.s3_store_bucket)}
+
+        expected_std_headers = {
+            'Content-Length': str(FIVE_KB),
+            'Content-Type': 'application/octet-stream'}
+
+        for expected_key, expected_value in expected_image_headers.items():
+            self.assertTrue(expected_key in image_headers,
+                            "Failed to find key %s in image_headers"
+                            % expected_key)
+            self.assertEqual(expected_value, image_headers[expected_key],
+                            "For key '%s' expected header value '%s'. Got '%s'"
+                            % (expected_key,
+                               expected_value,
+                               image_headers[expected_key]))
+
+        for expected_key, expected_value in expected_std_headers.items():
+            self.assertTrue(expected_key in std_headers,
+                            "Failed to find key %s in std_headers"
+                            % expected_key)
+            self.assertEqual(expected_value, std_headers[expected_key],
+                            "For key '%s' expected header value '%s'. Got '%s'"
+                            % (expected_key,
+                               expected_value,
+                               std_headers[expected_key]))
+
+        # Now the image data...
+        expected_image_data = "*" * FIVE_KB
+
+        # Should only be a single "line" left, and
+        # that's the image data
+        self.assertEqual(1, len(other_lines))
+        self.assertEqual(expected_image_data, other_lines[0])
+
+        # 6. GET /images
+        # Verify no public images
+        cmd = "curl http://0.0.0.0:%d/v1/images" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+
+        expected_result = {"images": [
+            {"container_format": None,
+             "disk_format": None,
+             "id": 1,
+             "name": "Image1",
+             "checksum": "c2e5db72bd7fd153f53ede5da5a06de3",
+             "size": 5120}]}
+        self.assertEqual(expected_result, json.loads(out.strip()))
+
+        # 7. GET /images/detail
+        # Verify image and all its metadata
+        cmd = "curl http://0.0.0.0:%d/v1/images/detail" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+
+        expected_image = {
+            "status": "active",
+            "name": "Image1",
+            "deleted": False,
+            "container_format": None,
+            "disk_format": None,
+            "id": 1,
+            'location': 's3://%s:%s@%s/%s/1' % (
+                self.s3_store_access_key,
+                self.s3_store_secret_key,
+                self.s3_store_host,
+                self.s3_store_bucket),
+            "is_public": True,
+            "deleted_at": None,
+            "properties": {},
+            "size": 5120}
+
+        image = json.loads(out.strip())['images'][0]
+
+        for expected_key, expected_value in expected_image.items():
+            self.assertTrue(expected_key in image,
+                            "Failed to find key %s in image"
+                            % expected_key)
+            self.assertEqual(expected_value, expected_image[expected_key],
+                            "For key '%s' expected header value '%s'. Got '%s'"
+                            % (expected_key,
+                               expected_value,
+                               image[expected_key]))
+
+        # 8. PUT /images/1 with custom properties of "distro" and "arch"
+        # Verify 200 returned
+
+        cmd = ("curl -i -X PUT "
+               "-H 'X-Image-Meta-Property-Distro: Ubuntu' "
+               "-H 'X-Image-Meta-Property-Arch: x86_64' "
+               "http://0.0.0.0:%d/v1/images/1") % api_port
+
+        exitcode, out, err = execute(cmd)
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 200 OK", status_line)
+
+        # 9. GET /images/detail
+        # Verify image and all its metadata
+        cmd = "curl http://0.0.0.0:%d/v1/images/detail" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+
+        expected_image = {
+            "status": "active",
+            "name": "Image1",
+            "deleted": False,
+            "container_format": None,
+            "disk_format": None,
+            "id": 1,
+            'location': 's3://%s:%s@%s/%s/1' % (
+                self.s3_store_access_key,
+                self.s3_store_secret_key,
+                self.s3_store_host,
+                self.s3_store_bucket),
+            "is_public": True,
+            "deleted_at": None,
+            "properties": {'distro': 'Ubuntu', 'arch': 'x86_64'},
+            "size": 5120}
+
+        image = json.loads(out.strip())['images'][0]
+
+        for expected_key, expected_value in expected_image.items():
+            self.assertTrue(expected_key in image,
+                            "Failed to find key %s in image"
+                            % expected_key)
+            self.assertEqual(expected_value, image[expected_key],
+                            "For key '%s' expected header value '%s'. Got '%s'"
+                            % (expected_key,
+                               expected_value,
+                               image[expected_key]))
+
+        # 10. PUT /images/1 and remove a previously existing property.
+        cmd = ("curl -i -X PUT "
+               "-H 'X-Image-Meta-Property-Arch: x86_64' "
+               "http://0.0.0.0:%d/v1/images/1") % api_port
+
+        exitcode, out, err = execute(cmd)
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 200 OK", status_line)
+
+        cmd = "curl http://0.0.0.0:%d/v1/images/detail" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+
+        image = json.loads(out.strip())['images'][0]
+        self.assertEqual(1, len(image['properties']))
+        self.assertEqual('x86_64', image['properties']['arch'])
+
+        # 11. PUT /images/1 and add a previously deleted property.
+        cmd = ("curl -i -X PUT "
+               "-H 'X-Image-Meta-Property-Distro: Ubuntu' "
+               "-H 'X-Image-Meta-Property-Arch: x86_64' "
+               "http://0.0.0.0:%d/v1/images/1") % api_port
+
+        exitcode, out, err = execute(cmd)
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 200 OK", status_line)
+
+        cmd = "curl http://0.0.0.0:%d/v1/images/detail" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+
+        image = json.loads(out.strip())['images'][0]
+        self.assertEqual(2, len(image['properties']))
+        self.assertEqual('x86_64', image['properties']['arch'])
+        self.assertEqual('Ubuntu', image['properties']['distro'])
+
+        self.stop_servers()
+
+    def test_delete_not_existing(self):
+        """
+        We test the following:
+
+        0. GET /images/1
+        - Verify 404
+        1. DELETE /images/1
+        - Verify 404
+        """
+        if self.skip_tests:
+            return True
+
+        self.cleanup()
+        self.start_servers(**self.__dict__.copy())
+
+        api_port = self.api_port
+        registry_port = self.registry_port
+
+        # 0. GET /images/1
+        # Verify 404 returned
+        cmd = "curl -i http://0.0.0.0:%d/v1/images/1" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 404 Not Found", status_line)
+
+        # 1. DELETE /images/1
+        # Verify 404 returned
+        cmd = "curl -i -X DELETE http://0.0.0.0:%d/v1/images/1" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 404 Not Found", status_line)
+
+        self.stop_servers()

=== added file 'tests/unit/test_s3_store.py'
--- tests/unit/test_s3_store.py	1970-01-01 00:00:00 +0000
+++ tests/unit/test_s3_store.py	2011-07-29 18:46:57 +0000
@@ -0,0 +1,345 @@
+# 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 s3 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.
+
+"""Tests the S3 backend store"""
+
+import StringIO
+import hashlib
+import httplib
+import sys
+import unittest
+import urlparse
+
+import stubout
+import boto.s3.connection
+
+from glance.common import exception
+from glance.store import BackendException, UnsupportedBackend
+from glance.store.location import get_location_from_uri
+from glance.store.s3 import S3Backend
+
+FIVE_KB = (5 * 1024)
+S3_OPTIONS = {'verbose': True,
+              'debug': True,
+              's3_store_access_key': 'user',
+              's3_store_secret_key': 'key',
+              's3_store_host': 'localhost:8080',
+              's3_store_bucket': 'glance'}
+
+
+# We stub out as little as possible to ensure that the code paths
+# between glance.store.s3 and boto.s3.connection are tested
+# thoroughly
+def stub_out_s3(stubs):
+
+    class FakeKey:
+        """
+        Acts like a ``boto.s3.key.Key``
+        """
+        def __init__(self, bucket, name):
+            self.bucket = bucket
+            self.name = name
+            self.data = None
+            self.size = 0
+            self.BufferSize = 1024
+
+        def close(self):
+            pass
+
+        def exists(self):
+            return self.bucket.exists(self.name)
+
+        def delete(self):
+            self.bucket.delete(self.name)
+
+        def compute_md5(self, data):
+            chunk = data.read(self.BufferSize)
+            checksum = hashlib.md5()
+            while chunk:
+                checksum.update(chunk)
+                chunk = data.read(self.BufferSize)
+            return checksum.hexdigest(), None
+
+        def set_contents_from_file(self, fp, replace=False, **kwargs):
+            self.data = StringIO.StringIO()
+            self.data.write(fp.getvalue())
+            self.size = self.data.len
+            # Reset the buffer to start
+            self.data.seek(0)
+            self.read = self.data.read
+
+        def get_file(self):
+            return self.data
+
+    class FakeBucket:
+        """
+        Acts like a ``boto.s3.bucket.Bucket``
+        """
+        def __init__(self, name, keys=None):
+            self.name = name
+            self.keys = keys or {}
+
+        def __str__(self):
+            return self.name
+
+        def exists(self, key):
+            return key in self.keys
+
+        def delete(self, key):
+            del self.keys[key]
+
+        def get_key(self, key_name, **kwargs):
+            key = self.keys.get(key_name)
+            if not key:
+                return FakeKey(self, key_name)
+            return key
+
+        def new_key(self, key_name):
+            new_key = FakeKey(self, key_name)
+            self.keys[key_name] = new_key
+            return new_key
+
+    fixture_buckets = {'glance': FakeBucket('glance')}
+    b = fixture_buckets['glance']
+    k = b.new_key('2')
+    k.set_contents_from_file(StringIO.StringIO("*" * FIVE_KB))
+
+    def fake_connection_constructor(self, *args, **kwargs):
+        host = kwargs.get('host')
+        if host.startswith('http://') or host.startswith('https://'):
+            raise UnsupportedBackend(host)
+
+    def fake_get_bucket(conn, bucket_id):
+        bucket = fixture_buckets.get(bucket_id)
+        if not bucket:
+            bucket = FakeBucket(bucket_id)
+        return bucket
+
+    stubs.Set(boto.s3.connection.S3Connection,
+              '__init__', fake_connection_constructor)
+    stubs.Set(boto.s3.connection.S3Connection,
+              'get_bucket', fake_get_bucket)
+
+
+def format_s3_location(user, key, authurl, bucket, obj):
+    """
+    Helper method that returns a S3 store URI given
+    the component pieces.
+    """
+    scheme = 's3'
+    if authurl.startswith('https://'):
+        scheme = 's3+https'
+        authurl = authurl[8:]
+    elif authurl.startswith('http://'):
+        authurl = authurl[7:]
+    authurl = authurl.strip('/')
+    return "%s://%s:%s@%s/%s/%s" % (scheme, user, key, authurl,
+                                    bucket, obj)
+
+
+class TestS3Backend(unittest.TestCase):
+
+    def setUp(self):
+        """Establish a clean test environment"""
+        self.stubs = stubout.StubOutForTesting()
+        stub_out_s3(self.stubs)
+
+    def tearDown(self):
+        """Clear the test environment"""
+        self.stubs.UnsetAll()
+
+    def test_get(self):
+        """Test a "normal" retrieval of an image in chunks"""
+        loc = get_location_from_uri(
+            "s3://user:key@auth_address/glance/2")
+        image_s3 = S3Backend.get(loc)
+
+        expected_data = "*" * FIVE_KB
+        data = ""
+
+        for chunk in image_s3:
+            data += chunk
+        self.assertEqual(expected_data, data)
+
+    def test_get_mismatched_expected_size(self):
+        """
+        Test retrieval of an image with wrong expected_size param
+        raises an exception
+        """
+        loc = get_location_from_uri(
+            "s3://user:key@auth_address/glance/2")
+        self.assertRaises(BackendException,
+                          S3Backend.get,
+                          loc,
+                          {'expected_size': 42})
+
+    def test_get_non_existing(self):
+        """
+        Test that trying to retrieve a s3 that doesn't exist
+        raises an error
+        """
+        loc = get_location_from_uri(
+            "s3://user:key@auth_address/badbucket/2")
+        self.assertRaises(exception.NotFound,
+                          S3Backend.get,
+                          loc)
+
+        loc = get_location_from_uri(
+            "s3://user:key@auth_address/glance/noexist")
+        self.assertRaises(exception.NotFound,
+                          S3Backend.get,
+                          loc)
+
+    def test_add(self):
+        """Test that we can add an image via the s3 backend"""
+        expected_image_id = 42
+        expected_s3_size = FIVE_KB
+        expected_s3_contents = "*" * expected_s3_size
+        expected_checksum = hashlib.md5(expected_s3_contents).hexdigest()
+        expected_location = format_s3_location(
+            S3_OPTIONS['s3_store_access_key'],
+            S3_OPTIONS['s3_store_secret_key'],
+            S3_OPTIONS['s3_store_host'],
+            S3_OPTIONS['s3_store_bucket'],
+            expected_image_id)
+        image_s3 = StringIO.StringIO(expected_s3_contents)
+
+        location, size, checksum = S3Backend.add(42, image_s3, S3_OPTIONS)
+
+        self.assertEquals(expected_location, location)
+        self.assertEquals(expected_s3_size, size)
+        self.assertEquals(expected_checksum, checksum)
+
+        loc = get_location_from_uri(expected_location)
+        new_image_s3 = S3Backend.get(loc)
+        new_image_contents = StringIO.StringIO()
+        for chunk in new_image_s3:
+            new_image_contents.write(chunk)
+        new_image_s3_size = new_image_contents.len
+
+        self.assertEquals(expected_s3_contents, new_image_contents.getvalue())
+        self.assertEquals(expected_s3_size, new_image_s3_size)
+
+    def test_add_host_variations(self):
+        """
+        Test that having http(s):// in the s3serviceurl in config
+        options works as expected.
+        """
+        variations = ['http://localhost:80',
+                      'http://localhost',
+                      'http://localhost/v1',
+                      'http://localhost/v1/',
+                      'https://localhost',
+                      'https://localhost:8080',
+                      'https://localhost/v1',
+                      'https://localhost/v1/',
+                      'localhost',
+                      'localhost:8080/v1']
+        i = 42
+        for variation in variations:
+            expected_image_id = i
+            expected_s3_size = FIVE_KB
+            expected_s3_contents = "*" * expected_s3_size
+            expected_checksum = \
+                    hashlib.md5(expected_s3_contents).hexdigest()
+            new_options = S3_OPTIONS.copy()
+            new_options['s3_store_host'] = variation
+            expected_location = format_s3_location(
+                new_options['s3_store_access_key'],
+                new_options['s3_store_secret_key'],
+                new_options['s3_store_host'],
+                new_options['s3_store_bucket'],
+                expected_image_id)
+            image_s3 = StringIO.StringIO(expected_s3_contents)
+
+            location, size, checksum = S3Backend.add(i, image_s3,
+                                                        new_options)
+
+            self.assertEquals(expected_location, location)
+            self.assertEquals(expected_s3_size, size)
+            self.assertEquals(expected_checksum, checksum)
+
+            loc = get_location_from_uri(expected_location)
+            new_image_s3 = S3Backend.get(loc)
+            new_image_contents = new_image_s3.getvalue()
+            new_image_s3_size = new_image_s3.len
+
+            self.assertEquals(expected_s3_contents, new_image_contents)
+            self.assertEquals(expected_s3_size, new_image_s3_size)
+            i = i + 1
+
+    def test_add_already_existing(self):
+        """
+        Tests that adding an image with an existing identifier
+        raises an appropriate exception
+        """
+        image_s3 = StringIO.StringIO("nevergonnamakeit")
+        self.assertRaises(exception.Duplicate,
+                          S3Backend.add,
+                          2, image_s3, S3_OPTIONS)
+
+    def _assertOptionRequiredForS3(self, key):
+        image_s3 = StringIO.StringIO("nevergonnamakeit")
+        options = S3_OPTIONS.copy()
+        del options[key]
+        self.assertRaises(BackendException, S3Backend.add,
+                          2, image_s3, options)
+
+    def test_add_no_user(self):
+        """
+        Tests that adding options without user raises
+        an appropriate exception
+        """
+        self._assertOptionRequiredForS3('s3_store_access_key')
+
+    def test_no_key(self):
+        """
+        Tests that adding options without key raises
+        an appropriate exception
+        """
+        self._assertOptionRequiredForS3('s3_store_secret_key')
+
+    def test_add_no_host(self):
+        """
+        Tests that adding options without host raises
+        an appropriate exception
+        """
+        self._assertOptionRequiredForS3('s3_store_host')
+
+    def test_delete(self):
+        """
+        Test we can delete an existing image in the s3 store
+        """
+        loc = get_location_from_uri(
+            "s3://user:key@auth_address/glance/2")
+
+        S3Backend.delete(loc)
+
+        self.assertRaises(exception.NotFound,
+                          S3Backend.get,
+                          loc)
+
+    def test_delete_non_existing(self):
+        """
+        Test that trying to delete a s3 that doesn't exist
+        raises an error
+        """
+        loc = get_location_from_uri(
+            "s3://user:key@auth_address/glance/noexist")
+        self.assertRaises(exception.NotFound,
+                          S3Backend.delete,
+                          loc)

=== modified file 'tests/unit/test_stores.py'
--- tests/unit/test_stores.py	2011-07-13 20:15:46 +0000
+++ tests/unit/test_stores.py	2011-07-29 18:46:57 +0000
@@ -63,21 +63,3 @@
 
         chunks = [c for c in fetcher]
         self.assertEqual(chunks, expected_returns)
-
-
-class TestS3Backend(TestBackend):
-    def setUp(self):
-        super(TestS3Backend, self).setUp()
-        stubs.stub_out_s3_backend(self.stubs)
-
-    def test_get(self):
-        s3_uri = "s3://user:password@localhost/bucket1/file.tar.gz"
-
-        expected_returns = ['I ', 'am', ' a', ' t', 'ea', 'po', 't,', ' s',
-                            'ho', 'rt', ' a', 'nd', ' s', 'to', 'ut', '\n']
-        fetcher = get_from_backend(s3_uri,
-                                   expected_size=8,
-                                   conn_class=S3Backend)
-
-        chunks = [c for c in fetcher]
-        self.assertEqual(chunks, expected_returns)

=== modified file 'tools/pip-requires'
--- tools/pip-requires	2011-07-29 15:31:43 +0000
+++ tools/pip-requires	2011-07-29 18:46:57 +0000
@@ -12,6 +12,7 @@
 sphinx
 argparse
 mox==0.5.0
+boto
 swift
 -f http://pymox.googlecode.com/files/mox-0.5.0.tar.gz
 sqlalchemy-migrate>=0.6,<0.7