Merge lp:~dave-cheney/goamz/004-send-409-conflict into lp:~gophers/goamz/trunk

[Dave Cheney]

s3: add support for 409 responses

Required to fix LP 1042107

https://codereview.appspot.com/6901061/

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/6901061/

[John A Meinel (jameinel)]

The change LGTM, but there are some small questions.
1) If you're changing the service to return 409, shouldn't there be a
corresponding client-side change that says "oh 409, that means it
already exists and I should just treat it as a duplicate".

2) Should we have something more like:
  s.emulate = "us-east-1"

Rather than a simple boolean s.emulateUSEast1? I'm thinking that
eventually we may find that USEast2 acts differently from USWest, vs
Asia, etc.

I realize that us-east-1 is the most tested at this point, but it does
seem like you want more than 1 bit of info for the parametrization of
the test server.

https://codereview.appspot.com/6901061/

[John A Meinel (jameinel)]

I see that I missed your patch to juju-core to handle the 409 case.
Though I do still wonder if it should be handled inside goamz rather
than being handled in juju-core.

https://codereview.appspot.com/6901061/

[Roger Peppe (rogpeppe)]

On 2012/12/10 08:38:24, jameinel wrote:
> 2) Should we have something more like:
> s.emulate = "us-east-1"

i'd be happier with this too. that means we can add any
number of region-specific tweaks in the future. who
knows what differences there are?

https://codereview.appspot.com/6901061/

[lp:~dave-cheney/goamz/004-send-409-conflict]

27. By Dave Cheney on 2012-12-10

responding to review feedback

[John A Meinel (jameinel)]

On 2012/12/10 23:40:16, dfc wrote:
> On 2012/12/10 23:39:03, dfc wrote:
> > Please take a look.

> Thanks for your comments. I have added a *Config style structure which
can be
> extended when we need more control over the s3test.Server.

LGTM. If you want, we could have a "useast1_config" and a "uswest1" or
whatever. So we don't have to figure out what the various configs
actually mean. But overall it seems fine to me.

https://codereview.appspot.com/6901061/

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2012-12-11 9:15, Dave Cheney wrote:
> We could do that with a set of *Configs, ie
>
> var USEast1 *Config var USWest1 = &Config { Send409Conflict: true,
> }
>
> and so on ...

That was my thought, yes.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Cygwin)
Comment: Using GnuPG with undefined - http://www.enigmail.net/

iEYEARECAAYFAlDGxzkACgkQJdeBCYSNAAME8ACfU6V4r4Yb1pOfotGdi7yyRK/b
DCYAnRYRLboFTvnU2ftHUY3igXQI3gBs
=fCP5
-----END PGP SIGNATURE-----

[Roger Peppe (rogpeppe)]

LGTM
On Dec 11, 2012 5:43 AM, "John A Meinel" <email address hidden> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 2012-12-11 9:15, Dave Cheney wrote:
> > We could do that with a set of *Configs, ie
> >
> > var USEast1 *Config var USWest1 = &Config { Send409Conflict: true,
> > }
> >
> > and so on ...
>
> That was my thought, yes.
>
> John
> =:->
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.12 (Cygwin)
> Comment: Using GnuPG with undefined - http://www.enigmail.net/
>
> iEYEARECAAYFAlDGxzkACgkQJdeBCYSNAAME8ACfU6V4r4Yb1pOfotGdi7yyRK/b
> DCYAnRYRLboFTvnU2ftHUY3igXQI3gBs
> =fCP5
> -----END PGP SIGNATURE-----
>
> --
>
> https://code.launchpad.net/~dave-cheney/goamz/004-send-409-conflict/+merge/138903
> Your team The Go Language Gophers is requested to review the proposed
> merge of lp:~dave-cheney/goamz/004-send-409-conflict into lp:goamz.
>

=== modified file 's3/s3.go'
--- s3/s3.go	2012-07-19 13:51:13 +0000
+++ s3/s3.go	2012-12-10 23:40:24 +0000
@@ -64,7 +64,7 @@
   <LocationConstraint>%s</LocationConstraint> 
 </CreateBucketConfiguration>`
 
-// locationConstraint returns an io.Reader specifying a LocationConstraint if 
+// locationConstraint returns an io.Reader specifying a LocationConstraint if
 // required for the region.
 //
 // See http://goo.gl/bh9Kq for more details.
@@ -269,7 +269,7 @@
 //             "photos/2006/January/",
 //         },
 //     }
-// 
+//
 // See http://goo.gl/YjQTc for more details.
 func (b *Bucket) List(prefix, delim, marker string, max int) (result *ListResp, err error) {
 	params := map[string][]string{

=== modified file 's3/s3i_test.go'
--- s3/s3i_test.go	2012-07-12 01:56:51 +0000
+++ s3/s3i_test.go	2012-12-10 23:40:24 +0000
@@ -299,6 +299,18 @@
 	},
 }
 
+func (s *ClientTests) TestDoublePutBucket(c *C) {
+	b := s.Bucket(testBucket)
+	err := b.PutBucket(s3.PublicRead)
+	c.Assert(err, IsNil)
+
+	err = b.PutBucket(s3.PublicRead)
+	if err != nil {
+		c.Assert(err, FitsTypeOf, new(s3.Error))
+		c.Assert(err.(*s3.Error).Code, Equals, "BucketAlreadyOwnedByYou")
+	}
+}
+
 func (s *ClientTests) TestBucketList(c *C) {
 	b := s.Bucket(testBucket)
 	err := b.PutBucket(s3.Private)

=== modified file 's3/s3t_test.go'
--- s3/s3t_test.go	2012-07-06 03:23:10 +0000
+++ s3/s3t_test.go	2012-12-10 23:40:24 +0000
@@ -11,10 +11,11 @@
 	auth   aws.Auth
 	region aws.Region
 	srv    *s3test.Server
+	config *s3test.Config
 }
 
 func (s *LocalServer) SetUp(c *C) {
-	srv, err := s3test.NewServer()
+	srv, err := s3test.NewServer(s.config)
 	c.Assert(err, IsNil)
 	c.Assert(srv, NotNil)
 
@@ -36,7 +37,17 @@
 	clientTests ClientTests
 }
 
-var _ = Suite(&LocalServerSuite{})
+var (
+	// run tests twice, once in us-east-1 mode, once not.
+	_ = Suite(&LocalServerSuite{})
+	_ = Suite(&LocalServerSuite{
+		srv: LocalServer{
+			config: &s3test.Config{
+				Send409Conflict: true,
+			},
+		},
+	})
+)
 
 func (s *LocalServerSuite) SetUpSuite(c *C) {
 	s.srv.SetUp(c)
@@ -57,3 +68,7 @@
 func (s *LocalServerSuite) TestBucketList(c *C) {
 	s.clientTests.TestBucketList(c)
 }
+
+func (s *LocalServerSuite) TestDoublePutBucket(c *C) {
+	s.clientTests.TestDoublePutBucket(c)
+}

=== modified file 's3/s3test/server.go'
--- s3/s3test/server.go	2012-07-05 05:47:52 +0000
+++ s3/s3test/server.go	2012-12-10 23:40:24 +0000
@@ -40,6 +40,25 @@
 	reqId string
 }
 
+// Config controls the internal behaviour of the Server. A nil config is the default
+// and behaves as if all configurations assume their default behaviour. Once passed
+// to NewServer, the configuration must not be modified.
+type Config struct {
+	// Send409Conflict controls how the Server will respond to calls to PUT on a
+	// previously existing bucket. The default is false, and corresponds to the
+	// us-east-1 s3 enpoint. Setting this value to true emulates the behaviour of
+	// all other regions.
+	// http://docs.amazonwebservices.com/AmazonS3/latest/API/ErrorResponses.html
+	Send409Conflict bool
+}
+
+func (c *Config) send409Conflict() bool {
+	if c != nil {
+		return c.Send409Conflict
+	}
+	return false
+}
+
 // Server is a fake S3 server for testing purposes.
 // All of the data for the server is kept in memory.
 type Server struct {
@@ -48,6 +67,7 @@
 	listener net.Listener
 	mu       sync.Mutex
 	buckets  map[string]*bucket
+	config   *Config
 }
 
 type bucket struct {
@@ -75,7 +95,7 @@
 	delete(a *action) interface{}
 }
 
-func NewServer() (*Server, error) {
+func NewServer(config *Config) (*Server, error) {
 	l, err := net.Listen("tcp", "localhost:0")
 	if err != nil {
 		return nil, fmt.Errorf("cannot listen on localhost: %v", err)
@@ -84,6 +104,7 @@
 		listener: l,
 		url:      "http://" + l.Addr().String(),
 		buckets:  make(map[string]*bucket),
+		config:   config,
 	}
 	go http.Serve(l, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		srv.serveHTTP(w, req)
@@ -386,6 +407,7 @@
 // PUT on a bucket creates the bucket.
 // http://docs.amazonwebservices.com/AmazonS3/latest/API/RESTBucketPUT.html
 func (r bucketResource) put(a *action) interface{} {
+	var created bool
 	if r.bucket == nil {
 		if !validBucketName(r.name) {
 			fatalf(400, "InvalidBucketName", "The specified bucket is not valid")
@@ -400,6 +422,10 @@
 			objects: make(map[string]*object),
 		}
 		a.srv.buckets[r.name] = r.bucket
+		created = true
+	}
+	if !created && a.srv.config.send409Conflict() {
+		fatalf(409, "BucketAlreadyOwnedByYou", "Your previous request to create the named bucket succeeded and you already own it.")
 	}
 	r.bucket.acl = s3.ACL(a.req.Header.Get("x-amz-acl"))
 	return nil
@@ -413,15 +439,15 @@
 // validBucketName returns whether name is a valid bucket name.
 // Here are the rules, from:
 // http://docs.amazonwebservices.com/AmazonS3/2006-03-01/dev/BucketRestrictions.html
-// 
-// Can contain lowercase letters, numbers, periods (.), underscores (_), 
+//
+// Can contain lowercase letters, numbers, periods (.), underscores (_),
 // and dashes (-). You can use uppercase letters for buckets only in the
 // US Standard region.
-// 
+//
 // Must start with a number or letter
-// 
+//
 // Must be between 3 and 255 characters long
-// 
+//
 // There's one extra rule (Must not be formatted as an IP address (e.g., 192.168.5.4)
 // but the real S3 server does not seem to check that rule, so we will not
 // check it either.