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

[Jay Pipes]

Completes the S3 storage backend.

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)]

40: You don't appear to use "config"

99: Feels a little weird to call it "parsed_uri" when the first thing you do is give it to parse_s3_tokens. How about resource_uri, or connection_uri? There are a bunch of places across the diff, I was just too lazy to point them all out. Actually, how would you feel about replacing parse_s3_tokens and format_s3_location with an S3ConnectionString class (that name sucks) that handles the same functionality?

162/187: Can we make these match?

190: Can we handle full_s3_host and s3_host a little differently? I would prefer to split it up into separate tokens rather than have s3_host encapsulated by full_s3_host.

review: Needs Fixing

[Jay Pipes (jaypipes)]

> 40: You don't appear to use "config"

Hmm, yes. Removed.

> 99: Feels a little weird to call it "parsed_uri" when the first thing you do
> is give it to parse_s3_tokens. How about resource_uri, or connection_uri?
> There are a bunch of places across the diff, I was just too lazy to point them
> all out. Actually, how would you feel about replacing parse_s3_tokens and
> format_s3_location with an S3ConnectionString class (that name sucks) that
> handles the same functionality?

This is mostly due to copy/paste fail from the Swift driver. I think I'd prefer to keep the code as-is and then refactor the URI parsing in both the Swift and S3 drivers into a common lib (possibly into glance.store.__init__ or glance.utils) in a separate patch. I agree it's messy. Would a separate patch be OK with you?

> 162/187: Can we make these match?

Done.

> 190: Can we handle full_s3_host and s3_host a little differently? I would
> prefer to split it up into separate tokens rather than have s3_host
> encapsulated by full_s3_host.

Hmm, I think I'd prefer to do this in a separate patch as well because the same code exists in the Swift driver. I agree it might be useful to separate the scheme from the host, but since the code also exists in the Swift driver, I'd prefer to put in a separate patch. Thoughts?

-jay

[Brian Waldon (bcwaldon)]

> > 40: You don't appear to use "config"
>
> Hmm, yes. Removed.
>
> > 99: Feels a little weird to call it "parsed_uri" when the first thing you do
> > is give it to parse_s3_tokens. How about resource_uri, or connection_uri?
> > There are a bunch of places across the diff, I was just too lazy to point
> them
> > all out. Actually, how would you feel about replacing parse_s3_tokens and
> > format_s3_location with an S3ConnectionString class (that name sucks) that
> > handles the same functionality?
>
> This is mostly due to copy/paste fail from the Swift driver. I think I'd
> prefer to keep the code as-is and then refactor the URI parsing in both the
> Swift and S3 drivers into a common lib (possibly into glance.store.__init__ or
> glance.utils) in a separate patch. I agree it's messy. Would a separate patch
> be OK with you?
>
> > 162/187: Can we make these match?
>
> Done.
>
> > 190: Can we handle full_s3_host and s3_host a little differently? I would
> > prefer to split it up into separate tokens rather than have s3_host
> > encapsulated by full_s3_host.
>
> Hmm, I think I'd prefer to do this in a separate patch as well because the
> same code exists in the Swift driver. I agree it might be useful to separate
> the scheme from the host, but since the code also exists in the Swift driver,
> I'd prefer to put in a separate patch. Thoughts?
>
> -jay

Sounds good.

review: Approve

[Christopher MacGown (0x44)]

looks good

review: Approve

[Diego Lalo (diego-lalo)]

Hi Jay,

First thanks for your answer, second: how do I pull the branch and use it?

Cheers
Diego

[Dan Prince (dan-prince)]

Hey Jay,

First time I've messed w/ S3 and glance. Couple of things. These may or may not all be related to this patch:

1) Should we add example default S3 configs to the glance.conf file. (glance-api.conf in the versioned branch).

2) Why do we use the get_key function in s3.py's 'get' and then call bucket.get_key directly in 'add'? Should both of these make use of get_key.

Dan

review: Needs Information

[Jay Pipes (jaypipes)]

> Hey Jay,
>
> First time I've messed w/ S3 and glance. Couple of things. These may or may
> not all be related to this patch:
>
> 1) Should we add example default S3 configs to the glance.conf file. (glance-
> api.conf in the versioned branch).

I did... :)

> 2) Why do we use the get_key function in s3.py's 'get' and then call
> bucket.get_key directly in 'add'? Should both of these make use of get_key.

The reason is the way the boto/s3 API works and because in the the case of get, we want to check for existence and raise different exceptions (NotFound vs. Duplicate) in the case of get vs. add..

-jay

[Jay Pipes (jaypipes)]

On Wed, May 11, 2011 at 2:35 PM, Diego Lalo <email address hidden> wrote:
> First thanks for your answer, second: how do I pull the branch and use it?

on a test machine/environment, you would do:

bzr branch lp:~jaypipes/glance/bug713154
cd bug713154
sudo python setup.py install

# Edit your glance.conf and add appropriate values for the new S3 stuff:

# ============ S3 Store Options ================================

# Address where the S3 authentication service lives
s3_store_host = 127.0.0.1:3333/

# User to authenticate against the S3 authentication service
s3_store_access_key = jdoe

# Auth key for the user authenticating against the
# S3 authentication service
s3_store_secret_key = a86850deb2742ec3cb41518e26aa2d89

# Bucket to store images on the S3 host.
s3_store_bucket = glance

Then repeat your calls to register images in Glance with the S3 backend :)

-jay

[Dan Prince (dan-prince)]

Just tried this with my S3 account. I'm doing a glance add to upload a ramdisk into S3 using Glance.

Doing this it appears line 201:

if key.exists():

needs to be something like:

if key and key.exists():

After making this change I still get the following error:

Error uploading image: 'Input' object has no attribute 'seek

review: Needs Fixing

[Jay Pipes (jaypipes)]

OK, wow. So I added a functional test case that uses Amazon S3 settings. As Dan found, the functional test case uncovered some issues with the store code:

* As Dan mentioned, I needed if key and key.exists() in one place
* "S3-style" URLs cause urlparse.urlparse() to utterly barf because AWS secret keys stupidly allow a forward slash. Had to rewrite the parse_s3_tokens() and add another unit test for this.
* The md5 kwarg to boto.s3.key.set_contents_from_file() doesn't work as advertised. Had to work around this by calculating the MD5 with compute_md5() and just not passing it to that method
* The key.get_file() method call wasn't necessary. Just needed to pass the Key object (which is an iterator itself) to the ChunkedFile constructor.

Fun! :)

Please re-review.

[Jay Pipes (jaypipes)]

Also, I fixed the issue with Input having no attribute 'seek' that Dan mentioned above. That was pretty easy fix: just change webob.Request.body_file to body_file_seekable... yeah for docs \o/

[Christopher MacGown (0x44)]

Hi Jay,

The functional tests work, but the S3 unit test is failing:

======================================================================
ERROR: test_add (tests.unit.test_s3_store.TestS3Backend)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/chris/Projects/review_day/bug713154/tests/unit/test_s3_store.py", line 244, in test_add
    for chunk in new_image_s3:
  File "/Users/chris/Projects/review_day/bug713154/glance/store/s3.py", line 53, in __iter__
    self.close()
  File "/Users/chris/Projects/review_day/bug713154/glance/store/s3.py", line 58, in close
    self.fp.close()
AttributeError: FakeKey instance has no attribute 'close'

======================================================================
ERROR: test_get (tests.unit.test_s3_store.TestS3Backend)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/chris/Projects/review_day/bug713154/tests/unit/test_s3_store.py", line 188, in test_get
    for chunk in image_s3:
  File "/Users/chris/Projects/review_day/bug713154/glance/store/s3.py", line 53, in __iter__
    self.close()
  File "/Users/chris/Projects/review_day/bug713154/glance/store/s3.py", line 58, in close
    self.fp.close()
AttributeError: FakeKey instance has no attribute 'close'

review: Needs Fixing

[Jay Pipes (jaypipes)]

thx Chris, I'll get right on that.

[Jay Pipes (jaypipes)]

ok, all fixed. we still have to deal with skipping tests better, but the bug mentioned is now fixed.

[Christopher MacGown (0x44)]

Looks good

review: Approve

[Dan Prince (dan-prince)]

Hey Jay,

I just tried this and I'm still getting:

2011-05-31 13:57:41 DEBUG [glance.registry] 1 custom properties...
2011-05-31 13:57:41 DEBUG [glance.registry] type: ramdisk
2011-05-31 13:57:41 DEBUG [glance.api.v1.images] Uploading image data for image 4 to s3 store
2011-05-31 13:57:41 ERROR [glance.api.v1.images] Error uploading image: body_file_seekable

What I'm doing for this test case is putting my S3 credentials into /etc/glance/glance-api.conf, starting glance and then running the following command:

root@glance1:~# glance add name="ari-tty" type="ramdisk" disk_format="ari" container_format="ari" is_public=true < /tmp/tty_linux/ramdisk
Failed to add image. Got error:
400 Bad Request

The server could not comply with the request since it is either malformed or otherwise incorrect.

 Error uploading image: body_file_seekable
Note: Your image metadata may still be in the registry, but the image's status will likely be 'killed'.

---

Am I doing something wrong here?

review: Needs Fixing

[Jay Pipes (jaypipes)]

> Hey Jay,
>
> I just tried this and I'm still getting:
>
> 2011-05-31 13:57:41 DEBUG [glance.registry] 1 custom properties...
> 2011-05-31 13:57:41 DEBUG [glance.registry] type: ramdisk
> 2011-05-31 13:57:41 DEBUG [glance.api.v1.images] Uploading image data for
> image 4 to s3 store
> 2011-05-31 13:57:41 ERROR [glance.api.v1.images] Error uploading image:
> body_file_seekable
>
> What I'm doing for this test case is putting my S3 credentials into
> /etc/glance/glance-api.conf, starting glance and then running the following
> command:
>
> root@glance1:~# glance add name="ari-tty" type="ramdisk" disk_format="ari"
> container_format="ari" is_public=true < /tmp/tty_linux/ramdisk
> Failed to add image. Got error:
> 400 Bad Request
>
> The server could not comply with the request since it is either malformed or
> otherwise incorrect.
>
> Error uploading image: body_file_seekable
> Note: Your image metadata may still be in the registry, but the image's status
> will likely be 'killed'.
>
> ---
>
> Am I doing something wrong here?

Heya Dan,

Hmm, so I tried doing that and see the same thing *only* when not operating in a virtualenv. I'm going to check whether the version of webob.Request that has body_file_seekable may be the issue.

Check for an update in a short while. Cheers,
jay

[Jay Pipes (jaypipes)]

OK, latest commit passes all tests in -N or -V mode. Please re-test/review. Thanks much.

[Jay Pipes (jaypipes)]

Add workaround for Webob bug issue #12 and fix DELETE operation in S3 where URL parsing was broken. Please re-check/review. Thanks again.

[Rick Harris (rconradharris)]

Great stuff Jay.

One small suggestion:

> 79 +class ChunkedFile(object):

I think we could DRY up the code a bit by moving the generally useful
ChunkedFile class out to glance/store/__init__.py or perhaps
glance/store/utils.py.

The one small modification we'd need to make is changing the implementation in
filesystem backend to accept a file-obj rather than a filename. That's
probably a good idea anyway.

review: Approve

[Jay Pipes (jaypipes)]

@dprince:

Are you still seeing any issues with this branch?

Thanks,
jay

[Jay Pipes (jaypipes)]

> Great stuff Jay.
>
> One small suggestion:
>
> > 79 +class ChunkedFile(object):
>
> I think we could DRY up the code a bit by moving the generally useful
> ChunkedFile class out to glance/store/__init__.py or perhaps
> glance/store/utils.py.
>
> The one small modification we'd need to make is changing the implementation in
> filesystem backend to accept a file-obj rather than a filename. That's
> probably a good idea anyway.

Agreed. I'll be working on refactoring some of that stuff here:

https://blueprints.launchpad.net/glance/+spec/refactor-stores

Cheers!
jay

[Dan Prince (dan-prince)]

Yes. I'm still seeing issues.

Using bzr134. I'm able to successfully upload images to glance with the S3 backend. When I look in S3 however the images all have a size of 0.

Dan

> @dprince:
>
> Are you still seeing any issues with this branch?
>
> Thanks,
> jay

[Jay Pipes (jaypipes)]

OK, so bug 794718 is the real culprit for the issues seen here. I'm proposing to depend on webob 1.0.7+. Swift has already done this (actually, they depend on *either* 0.9.8 OR 1.0.7+. Unfortunately, we cannot use 0.9.8 and still have the S3 driver, because 0.9.8's handling of make_body_seekable() is buggy for chunked-transfer requests (which our client uses).

If we merge this, we need to coordinate packaging dependency changes as well. I've spoke with Andrey about the RPM packaging. Dan, can you handle the Deb stuff?

-jay

[Dan Prince (dan-prince)]

> Dan, can you handle the Deb stuff?
>
> -jay

Sure. I'll work w/ Soren on that.

[Brian Waldon (bcwaldon)]

So are we good on the webob pre-reqs now? Can this fix go in?

[Dan Prince (dan-prince)]

> So are we good on the webob pre-reqs now? Can this fix go in?

Hitting the following conflict w/ Glance trunk:

Text conflict in tools/pip-requires
1 conflicts encountered.

[Jay Pipes (jaypipes)]

OK, conflict fixed and merged up with trunk. Dan, could you please smoke this?

[Dan Prince (dan-prince)]

Looks good now.

review: Approve

[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
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
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
Test that the image contents are checksummed properly ... ok
Test for HEAD /images/<ID> ... ok
test_show_image_basic (tests.unit.test_api.TestGlanceAPI) ... ok
test_show_non_exists_image (tests.unit.test_api.TestGlanceAPI) ... ok
Tests that the /images POST registry API creates the image ... ok
Tests proper exception is raised if a bad disk_format is set ... ok
Tests proper exception is raised if a bad disk_format is set ... ok
Tests proper exception is raised if a bad status is set ... ok
Tests that exception raised for bad matching disk and container ... ok
Tests that the /images DELETE registry API deletes the image ... ok
Tests proper exception is raised if attempt to delete non-existing ... ok
Tests that the /images/detail registry API returns ... ok
Tests that the /images/detail registry API returns list of ... ok
Tests that the /images/detail registry API returns list of ... ok
Tests that the /images/detail registry API returns list of ... ok
Tests that the /images/detail registry API returns list of ... ok
Tests that the /images/detail registry API returns lis...

[Brian Waldon (bcwaldon)]

I'm getting a test failure, Jay. I ran the tests with a clean venv:

======================================================================
FAIL: test_parse_s3_tokens (tests.unit.test_s3_store.TestS3Backend)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/brian.waldon/valhalla/glance/bug713154/tests/unit/test_s3_store.py", line 157, in test_parse_s3_tokens
    self.assertEqual("user", user)
AssertionError: 'user' != '//user'

[Jay Pipes (jaypipes)]

I'll look into it. In SFO in meetings all day today... I'll try to
look at it tomorrow morning early.

On Fri, Jun 24, 2011 at 12:37 PM, Brian Waldon
<email address hidden> wrote:
> I'm getting a test failure, Jay. I ran the tests with a clean venv:
>
> ======================================================================
> FAIL: test_parse_s3_tokens (tests.unit.test_s3_store.TestS3Backend)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>  File "/Users/brian.waldon/valhalla/glance/bug713154/tests/unit/test_s3_store.py", line 157, in test_parse_s3_tokens
>    self.assertEqual("user", user)
> AssertionError: 'user' != '//user'
>
> --
> https://code.launchpad.net/~jaypipes/glance/bug713154/+merge/59110
> You are the owner of lp:~jaypipes/glance/bug713154.
>

[Christopher MacGown (0x44)]

I just ran this with a clean venv and all tests pass for me.

On Fri, Jun 24, 2011 at 2:42 PM, Jay Pipes <email address hidden> wrote:

> I'll look into it. In SFO in meetings all day today... I'll try to
> look at it tomorrow morning early.
>
> On Fri, Jun 24, 2011 at 12:37 PM, Brian Waldon
> <email address hidden> wrote:
> > I'm getting a test failure, Jay. I ran the tests with a clean venv:
> >
> > ======================================================================
> > FAIL: test_parse_s3_tokens (tests.unit.test_s3_store.TestS3Backend)
> > ----------------------------------------------------------------------
> > Traceback (most recent call last):
> > File
> "/Users/brian.waldon/valhalla/glance/bug713154/tests/unit/test_s3_store.py",
> line 157, in test_parse_s3_tokens
> > self.assertEqual("user", user)
> > AssertionError: 'user' != '//user'
> >
> > --
> > https://code.launchpad.net/~jaypipes/glance/bug713154/+merge/59110
> > You are the owner of lp:~jaypipes/glance/bug713154.
> >
>
> --
> https://code.launchpad.net/~jaypipes/glance/bug713154/+merge/59110
> You are reviewing the proposed merge of lp:~jaypipes/glance/bug713154 into
> lp:glance.
>

--
Christopher MacGown
Piston Cloud Computing, Inc.
[phone]
<email address hidden>

[Brian Waldon (bcwaldon)]

The problem is the call to urlparse.urlparse() in tests/unit/test_s3_store.py. The netloc and path attributes are parsed differently depending on your python version. Here's the sample script I used:

import urlparse
uri = "s3://user:key@localhost/v1.0/bucket/objname"
urlparse.urlparse(uri)

On python 2.6.1 (my version), it returns the following:

ParseResult(scheme='s3', netloc='', path='//user:key@localhost/v1.0/bucket/objname', params='', query='', fragment='')

On python 2.6.6 and 2.7.1, it returns this:

ParseResult(scheme='s3', netloc='user:key@localhost', path='/v1.0/bucket/objname', params='', query='', fragment='')

[Christopher MacGown (0x44)]

I thought we already had a fix for the 2.6/2.7 urlparse?

On Fri, Jun 24, 2011 at 3:56 PM, Brian Waldon <email address hidden>wrote:

> The problem is the call to urlparse.urlparse() in
> tests/unit/test_s3_store.py. The netloc and path attributes are parsed
> differently depending on your python version. Here's the sample script I
> used:
>
> import urlparse
> uri = "s3://user:key@localhost/v1.0/bucket/objname"
> urlparse.urlparse(uri)
>
>
> On python 2.6.1 (my version), it returns the following:
>
> ParseResult(scheme='s3', netloc='', path='//user:key@localhost/v1.0/bucket/objname',
> params='', query='', fragment='')
>
>
> On python 2.6.6 and 2.7.1, it returns this:
>
> ParseResult(scheme='s3', netloc='user:key@localhost',
> path='/v1.0/bucket/objname', params='', query='', fragment='')
>
>
> --
> https://code.launchpad.net/~jaypipes/glance/bug713154/+merge/59110
> You are reviewing the proposed merge of lp:~jaypipes/glance/bug713154 into
> lp:glance.
>

--
Christopher MacGown
Piston Cloud Computing, Inc.
[phone]
<email address hidden>

[Brian Waldon (bcwaldon)]

Not that I know of. I don't see anything in the code about it.

[Jay Pipes (jaypipes)]

On Fri, Jun 24, 2011 at 2:01 PM, Christopher MacGown <email address hidden> wrote:
> I thought we already had a fix for the 2.6/2.7 urlparse?

We do. I just forgot to copy the fix into the S3 parse_tokens function. :)

I'll fix it..
-jay

[Christopher MacGown (0x44)]

In an old branch, I'd merged the parse_tokens functions… think it's worth
rewriting for inclusion?

On Sat, Jun 25, 2011 at 11:01 AM, Jay Pipes <email address hidden> wrote:

> On Fri, Jun 24, 2011 at 2:01 PM, Christopher MacGown <email address hidden>
> wrote:
> > I thought we already had a fix for the 2.6/2.7 urlparse?
>
> We do. I just forgot to copy the fix into the S3 parse_tokens function. :)
>
> I'll fix it..
> -jay
>
> --
> https://code.launchpad.net/~jaypipes/glance/bug713154/+merge/59110
> You are reviewing the proposed merge of lp:~jaypipes/glance/bug713154 into
> lp:glance.
>

--
Christopher MacGown
Piston Cloud Computing, Inc.
[phone]
<email address hidden>

[Jay Pipes (jaypipes)]

I had to rewrite it for this particular driver, unfortunately, because
of S3's silly insistence on allowing forward slashes in passwords, so
http://user:<email address hidden>/bucket/ blows up urlparse :(

-jay

[Brian Waldon (bcwaldon)]

Looks like there is a little left to do here. Setting to WIP for now.

review: Needs Fixing

[Jay Pipes (jaypipes)]

FYI, this is the issue this is now blocked on: https://lists.launchpad.net/openstack-testing/msg00002.html

[Chuck Short (zulcss)]

> FYI, this is the issue this is now blocked on: https://lists.launchpad.net
> /openstack-testing/msg00002.html

Are we any closer to getting this merged?

Thanks
chuck

[Jay Pipes (jaypipes)]

Hey Chuck,

AFAIK, this actually can be merged fine. The S3 functional tests won't run in the Jenkins job of course, but we can deal with that later.

If people are OK with that, feel free to approve and merge it.

-jay

[Chuck Short (zulcss)]

lgtm although documentation will need to be updated.

[Brian Waldon (bcwaldon)]

Still seeing that test failure. Are you going to be able to address that?

[Jay Pipes (jaypipes)]

Uh, sorry, not sure why I set it back to Needs Review. I need to get
ahold of a python 2.6 dev env to do these fixes. On vacation until
Monday, so if someone can pick that up, would help get this through...

-jay

On Fri, Jul 15, 2011 at 11:54 AM, Brian Waldon
<email address hidden> wrote:
> Still seeing that test failure. Are you going to be able to address that?
> --
> https://code.launchpad.net/~jaypipes/glance/bug713154/+merge/59110
> You are the owner of lp:~jaypipes/glance/bug713154.
>

[lp:~jaypipes/glance/bug713154]

138. By Jay Pipes on 2011-07-22

Merge trunk and resolve conflicts. All tests passing for me...

139. By Jay Pipes on 2011-07-25

Fixed review stuff from Brian

140. By Jay Pipes on 2011-07-25

Missing import of common.config in S3 driver

141. By Jay Pipes on 2011-07-25

Add test case for S3 s3_store_host variations and fixes for URL bug

142. By Jay Pipes on 2011-07-25

Forgot to put back fix for the get_backend_class problem...

143. By Jay Pipes on 2011-07-26

Add functional test case for checking delete and get of non-existing image

144. By Jay Pipes on 2011-07-26

Typo in error condition for create_bucket_on_put, make body seekable in req object, and remove +glance from docs and configs

145. By Jay Pipes on 2011-07-28

Merge trunk

146. By Jay Pipes on 2011-07-28

Merge latest trunk

147. By Jay Pipes on 2011-07-29

Merge trunk

148. By Jay Pipes on 2011-07-29

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

Unmerged revisions

=== modified file 'etc/glance-api.conf'
--- etc/glance-api.conf	2011-05-11 23:03:51 +0000
+++ etc/glance-api.conf	2011-07-06 21:36:36 +0000
@@ -51,6 +51,27 @@
 # 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_store_bucket = <20-char AWS access key>+glance
+
+# Do we create the bucket if it does not exist?
+s3_store_create_bucket_on_put = False
+
 [pipeline:glance-api]
 pipeline = versionnegotiation apiv1app
 

=== modified file 'glance/api/v1/images.py'
--- glance/api/v1/images.py	2011-06-29 18:10:39 +0000
+++ glance/api/v1/images.py	2011-07-06 21:36:36 +0000
@@ -272,6 +272,10 @@
         try:
             logger.debug("Uploading image data for image %(image_id)s "
                          "to %(store_name)s store" % locals())
+            # NOTE(jaypipes): webob 1.0.3 has body_file_seekable attr
+            # but webob 0.9.8 does not; it has the make_body_seekable() method
+            # which newer versions have also, so we use the old method
+            req.make_body_seekable()
             location, size, checksum = store.add(image_meta['id'],
                                                  req.body_file,
                                                  self.options)
@@ -367,6 +371,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-01-27 04:19:13 +0000
+++ glance/store/s3.py	2011-07-06 21:36:36 +0000
@@ -15,95 +15,321 @@
 #    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 urlparse
+
+from glance.common import exception
 import glance.store
 
+logger = logging.getLogger('glance.store.s3')
+
+
+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 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, parsed_uri, 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, parsed_uri, expected_size=None, options=None):
         """
         Takes a parsed_uri in the format of:
         s3://access_key:[e-mail]/bucket/file.gz.0, connects
         to s3 and downloads the file. Returns the generator resp_body provided
         by get_object.
         """
-
-        if conn_class:
-            pass
-        else:
-            import boto.s3.connection
-            conn_class = boto.s3.connection.S3Connection
-
-        (access_key, secret_key, host, bucket, obj) = \
-            cls._parse_s3_tokens(parsed_uri)
-
-        # Close the connection when we're through.
-        with conn_class(access_key, secret_key, host=host) as s3_conn:
-            bucket = cls._get_bucket(s3_conn, bucket)
-
-            # Close the key when we're through.
-            with cls._get_key(bucket, 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
-
-    @classmethod
-    def delete(cls, parsed_uri, conn_class=None):
+        from boto.s3.connection import S3Connection
+
+        (access_key, secret_key, s3_host, bucket, obj_name) = \
+            parse_s3_tokens(parsed_uri)
+
+        # This is annoying. If I pass http://s3.amazonaws.com to Boto, it
+        # dies. If I pass s3.amazonaws.com it works fine. :(
+        s3_host_only = urlparse.urlparse(s3_host).netloc
+
+        s3_conn = S3Connection(access_key, secret_key, host=s3_host_only)
+        bucket_obj = get_bucket(s3_conn, bucket)
+
+        key = get_key(bucket_obj, obj_name)
+
+        logger.debug("Retrieved image object from S3 using "
+                     "(s3_host=%(s3_host)s, access_key=%(access_key)s, "
+                     "bucket=%(bucket)s, key=%(obj_name)s)" % locals())
+
+        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')
+
+        full_s3_host = s3_host
+        if not full_s3_host.startswith('http'):
+            full_s3_host = 'https://' + full_s3_host
+
+        s3_conn = S3Connection(access_key, secret_key, host=s3_host)
+
+        create_bucket_if_missing(bucket, s3_conn, options)
+
+        bucket_obj = get_bucket(s3_conn, bucket)
+        obj_name = str(id)
+        location = format_s3_location(access_key, secret_key, s3_host,
+                                      bucket, obj_name)
+
+        key = bucket_obj.get_key(obj_name)
+        if key and key.exists():
+            raise exception.Duplicate("S3 already has an image at "
+                                      "location %(location)s" % locals())
+
+        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 (location, size, obj_md5)
+
+    @classmethod
+    def delete(cls, parsed_uri, options=None):
         """
         Takes a parsed_uri in the format of:
         s3://access_key:[e-mail]/bucket/file.gz.0, connects
         to s3 and deletes the file. Returns whatever boto.s3.key.Key.delete()
         returns.
         """
-
-        if conn_class:
-            pass
+        from boto.s3.connection import S3Connection
+
+        (access_key, secret_key, s3_host, bucket, obj_name) = \
+            parse_s3_tokens(parsed_uri)
+
+        # This is annoying. If I pass http://s3.amazonaws.com to Boto, it
+        # dies. If I pass s3.amazonaws.com it works fine. :(
+        s3_host_only = urlparse.urlparse(s3_host).netloc
+
+        # Close the connection when we're through.
+        s3_conn = S3Connection(access_key, secret_key, host=s3_host_only)
+        bucket_obj = get_bucket(s3_conn, bucket)
+
+        # Close the key when we're through.
+        key = get_key(bucket_obj, obj_name)
+
+        logger.debug("Deleting image object from S3 using "
+                     "(s3_host=%(s3_host)s, user=%(access_key)s, "
+                     "bucket=%(bucket)s, key=%(obj_name)s)" % locals())
+
+        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)
         else:
-            conn_class = boto.s3.connection.S3Connection
-
-        (access_key, secret_key, host, bucket, obj) = \
-            cls._parse_s3_tokens(parsed_uri)
-
-        # Close the connection when we're through.
-        with conn_class(access_key, secret_key, host=host) as s3_conn:
-            bucket = cls._get_bucket(s3_conn, bucket)
-
-            # Close the key when we're through.
-            with cls._get_key(bucket, 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
-
-    @classmethod
-    def _parse_s3_tokens(cls, parsed_uri):
-        """Parse tokens from the parsed_uri"""
-        return glance.store.parse_uri_tokens(parsed_uri, cls.EXAMPLE_URL)
+            raise
+
+
+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.exists():
+        msg = ("Could not find key %(obj)s in bucket %(bucket)s") % locals()
+        logger.error(msg)
+        raise exception.NotFound(msg)
+    return key
+
+
+def format_s3_location(user, key, s3_host, bucket, obj_name):
+    """
+    Returns the s3 URI in the format:
+        s3://<USER_KEY>:<SECRET_KEY>@<S3_HOST>/<BUCKET>/<OBJNAME>
+
+    :param user: The s3 user key to authenticate with
+    :param key: The s3 secret key for the authenticating user
+    :param s3_host: The base URL for the s3 service
+    :param bucket: The name of the bucket
+    :param obj_name: The name of the object
+    """
+    return "s3://%(user)s:%(key)s@%(s3_host)s/"\
+           "%(bucket)s/%(obj_name)s" % locals()
+
+
+def parse_s3_tokens(parsed_uri):
+    """
+    Return the various tokens used by S3.
+
+    Note that an Amazon AWS secret key can contain the forward slash,
+    which is entirely retarded, and breaks urlparse miserably.
+    This function works around that issue.
+
+    :param parsed_uri: The pieces of a URI returned by urlparse
+    :retval A tuple of (access_key, secret_key, s3_host, bucket, obj_name)
+    """
+
+    # TODO(jaypipes): Do parsing in the stores. Don't call urlparse in the
+    #                 base get_backend_class routine...
+    entire_path = "%s%s" % (parsed_uri.netloc, parsed_uri.path)
+
+    try:
+        creds, path = entire_path.split('@')
+        cred_parts = creds.split(':')
+
+        access_key = cred_parts[0]
+        secret_key = cred_parts[1]
+        path_parts = path.split('/')
+        obj = path_parts.pop()
+        bucket = path_parts.pop()
+    except (ValueError, IndexError):
+        raise glance.store.BackendException(
+             "Expected four values to unpack in: s3:%s. "
+             "Should have received something like: %s."
+             % (parsed_uri.path, S3Backend.EXAMPLE_URL))
+
+    # 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')
+    s3_host = "https://%s" % '/'.join(path_parts)
+
+    return access_key, secret_key, s3_host, bucket, obj

=== modified file 'tests/functional/__init__.py'
--- tests/functional/__init__.py	2011-05-27 02:24:55 +0000
+++ tests/functional/__init__.py	2011-07-06 21:36:36 +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.conf_base = """[DEFAULT]
 verbose = %(verbose)s
 debug = %(debug)s
@@ -147,6 +151,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
 
 [pipeline:glance-api]
 pipeline = versionnegotiation apiv1app

=== 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-06 21:36:36 +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-06 21:36:36 +0000
@@ -0,0 +1,482 @@
+# 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
+
+        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)
+
+        # 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()

=== 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-06 21:36:36 +0000
@@ -0,0 +1,315 @@
+# 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
+from glance.store.s3 import (S3Backend,
+                             format_s3_location,
+                             parse_s3_tokens)
+
+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):
+        pass
+
+    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)
+
+
+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_parse_s3_tokens(self):
+        """
+        Test that the parse_s3_tokens function returns
+        user, key, authurl, bucket, and objname properly
+        """
+        uri = "s3://user:key@localhost/v1.0/bucket/objname"
+        url_pieces = urlparse.urlparse(uri)
+        user, key, authurl, bucket, objname =\
+                parse_s3_tokens(url_pieces)
+        self.assertEqual("user", user)
+        self.assertEqual("key", key)
+        self.assertEqual("https://localhost/v1.0", authurl)
+        self.assertEqual("bucket", bucket)
+        self.assertEqual("objname", objname)
+
+        uri = "s3://user:key@localhost:9090/v1.0/bucket/objname"
+        url_pieces = urlparse.urlparse(uri)
+        user, key, authurl, bucket, objname =\
+                parse_s3_tokens(url_pieces)
+        self.assertEqual("user", user)
+        self.assertEqual("key", key)
+        self.assertEqual("https://localhost:9090/v1.0", authurl)
+        self.assertEqual("bucket", bucket)
+        self.assertEqual("objname", objname)
+
+        uri = "s3://user:key/[e-mail]/bucket/objname"
+        url_pieces = urlparse.urlparse(uri)
+        user, key, authurl, bucket, objname =\
+                parse_s3_tokens(url_pieces)
+        self.assertEqual("user", user)
+        self.assertEqual("key/part", key)
+        self.assertEqual("https://s3.amazonaws.com", authurl)
+        self.assertEqual("bucket", bucket)
+        self.assertEqual("objname", objname)
+
+    def test_get(self):
+        """Test a "normal" retrieval of an image in chunks"""
+        url_pieces = urlparse.urlparse(
+            "s3://user:key@auth_address/glance/2")
+        image_s3 = S3Backend.get(url_pieces)
+
+        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
+        """
+        url_pieces = urlparse.urlparse(
+            "s3://user:key@auth_address/glance/2")
+        self.assertRaises(BackendException,
+                          S3Backend.get,
+                          url_pieces,
+                          {'expected_size': 42})
+
+    def test_get_non_existing(self):
+        """
+        Test that trying to retrieve a s3 that doesn't exist
+        raises an error
+        """
+        url_pieces = urlparse.urlparse(
+            "s3://user:key@auth_address/badbucket")
+        self.assertRaises(exception.NotFound,
+                          S3Backend.get,
+                          url_pieces)
+
+        url_pieces = urlparse.urlparse(
+            "s3://user:key@auth_address/glance/noexist")
+        self.assertRaises(exception.NotFound,
+                          S3Backend.get,
+                          url_pieces)
+
+    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)
+
+        url_pieces = urlparse.urlparse(expected_location)
+        new_image_s3 = S3Backend.get(url_pieces)
+        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_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
+        """
+        url_pieces = urlparse.urlparse(
+            "s3://user:key@auth_address/glance/2")
+
+        S3Backend.delete(url_pieces)
+
+        self.assertRaises(exception.NotFound,
+                          S3Backend.get,
+                          url_pieces)
+
+    def test_delete_non_existing(self):
+        """
+        Test that trying to delete a s3 that doesn't exist
+        raises an error
+        """
+        url_pieces = urlparse.urlparse(
+            "s3://user:key@auth_address/noexist")
+        self.assertRaises(exception.NotFound,
+                          S3Backend.delete,
+                          url_pieces)

=== modified file 'tests/unit/test_stores.py'
--- tests/unit/test_stores.py	2011-03-05 00:02:26 +0000
+++ tests/unit/test_stores.py	2011-07-06 21:36:36 +0000
@@ -64,21 +64,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-06-22 14:19:33 +0000
+++ tools/pip-requires	2011-07-06 21:36:36 +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