Merge lp:~nick-craig-wood/goamz/list-buckets into lp:~gophers/goamz/trunk

[Nick Craig-Wood]

s3: Implement ListBuckets method

https://codereview.appspot.com/7305051/

[Gustavo Niemeyer (niemeyer)]

Looking good, thanks for this. Just a few trivial details to sort out.

https://codereview.appspot.com/7305051/diff/1/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/7305051/diff/1/s3/s3.go#newcode43
s3/s3.go:43: // The following fields are only set on Buckets returned
from the ListBuckets call
s/Buckets/buckets/

https://codereview.appspot.com/7305051/diff/1/s3/s3.go#newcode45
s3/s3.go:45: CreationDate string // Date the bucket was created, e.g.,
2009-02-03T16:45:09.000Z
Seems like this should be a native time.Time.

https://codereview.appspot.com/7305051/diff/1/s3/s3.go#newcode74
s3/s3.go:74: type BucketInfo struct {
s/BucketInfo/bucketInfo/

This is used only internally, so doesn't have to be exposed in the
public API.

https://codereview.appspot.com/7305051/diff/1/s3/s3.go#newcode80
s3/s3.go:80: type Buckets struct {
s/Buckets/bucketInfoResp/, for the same reason.

The doc also needs fixing.

https://codereview.appspot.com/7305051/diff/1/s3/s3.go#newcode444
s3/s3.go:444: if req.baseurl == "" {
That logic is changed in trunk, both due to retries and to fix a bug.
Trunk needs to be merged back so we can see what the actual change
needed is here.

https://codereview.appspot.com/7305051/

[Nick Craig-Wood (nick-craig-wood)]

Arg, I've messed something up - probably this branch!

bzr diff -c38 | diffstat
 responses_test.go | 19 +++++++++++++++
 s3.go | 68 ++++++++++++++++++++++++++++++++++++++++++------------
 s3_test.go | 29 +++++++++++++++++++++++
 3 files changed, 102 insertions(+), 14 deletions(-)

Will try a new branch

Unmerged revisions

38. By Nick Craig-Wood on 2013-06-27

Implement S3.List to list all Buckets

37. By Ian Booth on 2013-05-23

Support EC2_ env variables

Allow the user to specify their auth credentials using EC2_ style
environment variables. I ran go fmt and it also made some changes.

The tests failed to run due to a test http server not being closed when
each test suite finished. I added a fix for this also, so now all tests
pass when go test ./... is run from the project's root directory.

R=jameinel, Danilo
CC=
https://codereview.appspot.com/9545045

36. By Gustavo Niemeyer on 2013-05-22

s3: close http response Body properly

The interface of the run method was error prone.

R=dfc, anacrolix
CC=
https://codereview.appspot.com/9611047

=== modified file 'aws/aws.go'
--- aws/aws.go	2012-11-13 23:59:04 +0000
+++ aws/aws.go	2013-06-27 11:56:25 +0000
@@ -162,10 +162,16 @@
 
 // EnvAuth creates an Auth based on environment information.
 // The AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment
-// variables are used.
+// variables are used as the first preference, but EC2_ACCESS_KEY
+// and EC2_SECRET_KEY environment variables are also supported.
 func EnvAuth() (auth Auth, err error) {
 	auth.AccessKey = os.Getenv("AWS_ACCESS_KEY_ID")
 	auth.SecretKey = os.Getenv("AWS_SECRET_ACCESS_KEY")
+	// We fallback to EC2_ env variables if the AWS_ variants are not used.
+	if auth.AccessKey == "" && auth.SecretKey == "" {
+		auth.AccessKey = os.Getenv("EC2_ACCESS_KEY")
+		auth.SecretKey = os.Getenv("EC2_SECRET_KEY")
+	}
 	if auth.AccessKey == "" {
 		err = errors.New("AWS_ACCESS_KEY_ID not found in environment")
 	}

=== modified file 'aws/aws_test.go'
--- aws/aws_test.go	2013-01-31 14:52:05 +0000
+++ aws/aws_test.go	2013-06-27 11:56:25 +0000
@@ -52,6 +52,15 @@
 	c.Assert(auth, Equals, aws.Auth{SecretKey: "secret", AccessKey: "access"})
 }
 
+func (s *S) TestEnvAuthLegacy(c *C) {
+	os.Clearenv()
+	os.Setenv("EC2_SECRET_KEY", "secret")
+	os.Setenv("EC2_ACCESS_KEY", "access")
+	auth, err := aws.EnvAuth()
+	c.Assert(err, IsNil)
+	c.Assert(auth, Equals, aws.Auth{SecretKey: "secret", AccessKey: "access"})
+}
+
 func (s *S) TestEncode(c *C) {
 	c.Assert(aws.Encode("foo"), Equals, "foo")
 	c.Assert(aws.Encode("/"), Equals, "%2F")

=== modified file 'ec2/ec2_test.go'
--- ec2/ec2_test.go	2013-01-31 14:52:05 +0000
+++ ec2/ec2_test.go	2013-06-27 11:56:25 +0000
@@ -26,6 +26,10 @@
 	s.ec2 = ec2.New(auth, aws.Region{EC2Endpoint: testServer.URL})
 }
 
+func (s *S) TearDownSuite(c *C) {
+	testServer.Stop()
+}
+
 func (s *S) TearDownTest(c *C) {
 	testServer.Flush()
 }

=== modified file 'exp/mturk/mturk_test.go'
--- exp/mturk/mturk_test.go	2013-01-31 14:52:05 +0000
+++ exp/mturk/mturk_test.go	2013-06-27 11:56:25 +0000
@@ -35,6 +35,10 @@
 	}
 }
 
+func (s *S) TearDownSuite(c *C) {
+	testServer.Stop()
+}
+
 func (s *S) TearDownTest(c *C) {
 	testServer.Flush()
 }

=== modified file 'exp/sdb/sdb_test.go'
--- exp/sdb/sdb_test.go	2013-01-31 14:52:05 +0000
+++ exp/sdb/sdb_test.go	2013-06-27 11:56:25 +0000
@@ -26,6 +26,10 @@
 	s.sdb = sdb.New(auth, aws.Region{SDBEndpoint: testServer.URL})
 }
 
+func (s *S) TearDownSuite(c *C) {
+	testServer.Stop()
+}
+
 func (s *S) TearDownTest(c *C) {
 	testServer.Flush()
 }

=== modified file 'exp/sns/sns_test.go'
--- exp/sns/sns_test.go	2013-01-31 14:52:05 +0000
+++ exp/sns/sns_test.go	2013-06-27 11:56:25 +0000
@@ -26,6 +26,10 @@
 	s.sns = sns.New(auth, aws.Region{SNSEndpoint: testServer.URL})
 }
 
+func (s *S) TearDownSuite(c *C) {
+	testServer.Stop()
+}
+
 func (s *S) TearDownTest(c *C) {
 	testServer.Flush()
 }

=== modified file 'iam/iam_test.go'
--- iam/iam_test.go	2013-03-02 02:11:38 +0000
+++ iam/iam_test.go	2013-06-27 11:56:25 +0000
@@ -27,6 +27,10 @@
 	s.iam = iam.New(auth, aws.Region{IAMEndpoint: testServer.URL})
 }
 
+func (s *S) TearDownSuite(c *C) {
+	testServer.Stop()
+}
+
 func (s *S) TearDownTest(c *C) {
 	testServer.Flush()
 }
@@ -146,8 +150,8 @@
 		{
 			Path: "/division_abc/subdivision_xyz/product_1234/",
 			Name: "Managers",
-			Id: "AGPIODR4TAW7CSEXAMPLE",
-			Arn: "arn:aws:iam::123456789012:group/division_abc/subdivision_xyz/product_1234/Managers",
+			Id:   "AGPIODR4TAW7CSEXAMPLE",
+			Arn:  "arn:aws:iam::123456789012:group/division_abc/subdivision_xyz/product_1234/Managers",
 		},
 	}
 	c.Assert(resp.Groups, DeepEquals, expected)

=== modified file 's3/multi.go'
--- s3/multi.go	2013-02-13 12:20:38 +0000
+++ s3/multi.go	2013-06-27 11:56:25 +0000
@@ -175,14 +175,15 @@
 		if err != nil {
 			return Part{}, err
 		}
-		resp, err := m.Bucket.S3.run(req, nil)
+		hresp, err := m.Bucket.S3.run(req)
 		if shouldRetry(err) && attempt.HasNext() {
 			continue
 		}
 		if err != nil {
 			return Part{}, err
 		}
-		etag := resp.Header.Get("ETag")
+		hresp.Body.Close()
+		etag := hresp.Header.Get("ETag")
 		if etag == "" {
 			return Part{}, errors.New("part upload succeeded with no ETag")
 		}

=== modified file 's3/responses_test.go'
--- s3/responses_test.go	2013-02-12 04:50:11 +0000
+++ s3/responses_test.go	2013-06-27 11:56:25 +0000
@@ -196,3 +196,22 @@
   <HostId>kjhwqk</HostId>
 </Error>
 `
+
+var GetBucketsListResultDump = `
+<ListAllMyBucketsResult xmlns="http://doc.s3.amazonaws.com/2006-03-01">
+  <Owner>
+    <ID>bcaf1ffd86f461ca5fb16fd081034f</ID>
+    <DisplayName>webfile</DisplayName>
+  </Owner>
+  <Buckets>
+    <Bucket>
+      <Name>quotes</Name>
+      <CreationDate>2006-02-03T16:45:09.000Z</CreationDate>
+    </Bucket>
+    <Bucket>
+      <Name>samples</Name>
+      <CreationDate>2006-02-03T16:41:58.000Z</CreationDate>
+    </Bucket>
+  </Buckets>
+</ListAllMyBucketsResult>
+`

=== modified file 's3/s3.go'
--- s3/s3.go	2013-02-11 17:15:59 +0000
+++ s3/s3.go	2013-06-27 11:56:25 +0000
@@ -40,6 +40,9 @@
 type Bucket struct {
 	*S3
 	Name string
+	// The following fields are only set on buckets returned from the ListBuckets call
+	Owner        Owner     // Owner of the bucket
+	CreationDate time.Time // When the bucket was created
 }
 
 // The Owner type represents the owner of the object in an S3 bucket.
@@ -64,7 +67,44 @@
 	if s3.Region.S3BucketEndpoint != "" || s3.Region.S3LowercaseBucket {
 		name = strings.ToLower(name)
 	}
-	return &Bucket{s3, name}
+	return &Bucket{S3: s3, Name: name}
+}
+
+// bucketsResp is returned by S3.Buckets
+type bucketsResp struct {
+	Owner   Owner
+	Buckets []struct {
+		Name         string
+		CreationDate string // Date the bucket was created, e.g., 2009-02-03T16:45:09.000Z
+	} `xml:"Buckets>Bucket"`
+}
+
+// List returns a list of all buckets owned by the sender
+//
+// See http://docs.amazonwebservices.com/AmazonS3/latest/API/RESTServiceGET.html
+func (s3 *S3) ListBuckets() (buckets []*Bucket, err error) {
+	req := &request{
+		method: "GET",
+		bucket: "",
+		path:   "",
+	}
+	result := &bucketsResp{}
+	err = s3.query(req, result)
+	if err != nil {
+		return nil, err
+	}
+	buckets = make([]*Bucket, len(result.Buckets))
+	for i := range buckets {
+		info := result.Buckets[i]
+		b := s3.Bucket(info.Name)
+		b.CreationDate, err = time.Parse(time.RFC3339Nano, info.CreationDate)
+		if err != nil {
+			return nil, err
+		}
+		b.Owner = result.Owner
+		buckets[i] = b
+	}
+	return buckets, nil
 }
 
 var createBucketConfiguration = `<CreateBucketConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"> 
@@ -156,14 +196,14 @@
 		return nil, err
 	}
 	for attempt := attempts.Start(); attempt.Next(); {
-		resp, err := b.S3.run(req, nil)
+		hresp, err := b.S3.run(req)
 		if shouldRetry(err) && attempt.HasNext() {
 			continue
 		}
 		if err != nil {
 			return nil, err
 		}
-		return resp.Body, nil
+		return hresp.Body, nil
 	}
 	panic("unreachable")
 }
@@ -382,10 +422,18 @@
 // body will be unmarshalled on it.
 func (s3 *S3) query(req *request, resp interface{}) error {
 	err := s3.prepare(req)
-	if err == nil {
-		_, err = s3.run(req, resp)
-	}
-	return err
+	if err != nil {
+		return err
+	}
+	hresp, err := s3.run(req)
+	if err != nil {
+		return err
+	}
+	if resp != nil {
+		err = xml.NewDecoder(hresp.Body).Decode(resp)
+	}
+	hresp.Body.Close()
+	return nil
 }
 
 // prepare sets up req to be delivered to S3.
@@ -410,21 +458,21 @@
 			req.path = "/" + req.path
 		}
 		req.signpath = req.path
-		if req.bucket != "" {
-			req.baseurl = s3.Region.S3BucketEndpoint
-			if req.baseurl == "" {
-				// Use the path method to address the bucket.
-				req.baseurl = s3.Region.S3Endpoint
+		req.baseurl = s3.Region.S3BucketEndpoint
+		if req.baseurl == "" {
+			// Use the path method to address the bucket.
+			req.baseurl = s3.Region.S3Endpoint
+			if req.bucket != "" {
 				req.path = "/" + req.bucket + req.path
-			} else {
-				// Just in case, prevent injection.
-				if strings.IndexAny(req.bucket, "/:@") >= 0 {
-					return fmt.Errorf("bad S3 bucket: %q", req.bucket)
-				}
-				req.baseurl = strings.Replace(req.baseurl, "${bucket}", req.bucket, -1)
-			}
-			req.signpath = "/" + req.bucket + req.signpath
+			}
+		} else {
+			// Just in case, prevent injection.
+			if strings.IndexAny(req.bucket, "/:@") >= 0 {
+				return fmt.Errorf("bad S3 bucket: %q", req.bucket)
+			}
+			req.baseurl = strings.Replace(req.baseurl, "${bucket}", req.bucket, -1)
 		}
+		req.signpath = "/" + req.bucket + req.signpath
 	}
 
 	// Always sign again as it's not clear how far the
@@ -440,9 +488,7 @@
 }
 
 // run sends req and returns the http response from the server.
-// If resp is not nil, the XML data contained in the response
-// body will be unmarshalled on it.
-func (s3 *S3) run(req *request, resp interface{}) (*http.Response, error) {
+func (s3 *S3) run(req *request) (*http.Response, error) {
 	if debug {
 		log.Printf("Running S3 request: %#v", req)
 	}
@@ -480,10 +526,6 @@
 	if hresp.StatusCode != 200 && hresp.StatusCode != 204 {
 		return nil, buildError(hresp)
 	}
-	if resp != nil {
-		err = xml.NewDecoder(hresp.Body).Decode(resp)
-		hresp.Body.Close()
-	}
 	return hresp, err
 }
 

=== modified file 's3/s3_test.go'
--- s3/s3_test.go	2013-02-12 04:50:11 +0000
+++ s3/s3_test.go	2013-06-27 11:56:25 +0000
@@ -4,6 +4,7 @@
 	"bytes"
 	"io/ioutil"
 	"net/http"
+	"net/url"
 	"testing"
 
 	"launchpad.net/goamz/aws"
@@ -33,6 +34,7 @@
 
 func (s *S) TearDownSuite(c *C) {
 	s3.SetAttemptStrategy(nil)
+	testServer.Stop()
 }
 
 func (s *S) SetUpTest(c *C) {
@@ -275,3 +277,31 @@
 	c.Assert(len(data.Contents), Equals, 0)
 	c.Assert(data.CommonPrefixes, DeepEquals, []string{"photos/2006/feb/", "photos/2006/jan/"})
 }
+
+// List buckets docs: See http://docs.amazonwebservices.com/AmazonS3/latest/API/RESTServiceGET.html
+
+func (s *S) TestListBuckets(c *C) {
+	testServer.Response(200, nil, GetBucketsListResultDump)
+
+	buckets, err := s.s3.ListBuckets()
+	c.Assert(err, IsNil)
+
+	req := testServer.WaitRequest()
+	c.Assert(req.Method, Equals, "GET")
+	c.Assert(req.URL.Path, Equals, "/")
+	c.Assert(req.Header["Date"], Not(Equals), "")
+	c.Assert(req.Form, DeepEquals, url.Values{})
+
+	c.Assert(len(buckets), Equals, 2)
+
+	c.Assert(buckets[0].Name, Equals, "quotes")
+	c.Assert(buckets[0].CreationDate, Equals, time.Date(2006, 2, 3, 16, 45, 9, 0, time.UTC))
+	c.Assert(buckets[0].Owner.ID, Equals, "bcaf1ffd86f461ca5fb16fd081034f")
+	c.Assert(buckets[0].Owner.DisplayName, Equals, "webfile")
+
+	c.Assert(buckets[1].Name, Equals, "samples")
+	c.Assert(buckets[1].CreationDate, Equals, time.Date(2006, 2, 3, 16, 41, 58, 0, time.UTC))
+	c.Assert(buckets[1].Owner.ID, Equals, "bcaf1ffd86f461ca5fb16fd081034f")
+	c.Assert(buckets[1].Owner.DisplayName, Equals, "webfile")
+
+}

=== modified file 'testutil/http.go'
--- testutil/http.go	2013-02-01 05:55:19 +0000
+++ testutil/http.go	2013-06-27 11:56:25 +0000
@@ -15,6 +15,7 @@
 	URL      string
 	Timeout  time.Duration
 	started  bool
+	listener net.Listener
 	request  chan *http.Request
 	response chan ResponseFunc
 }
@@ -42,11 +43,11 @@
 	if err != nil {
 		panic(err)
 	}
-	l, err := net.Listen("tcp", u.Host)
+	s.listener, err = net.Listen("tcp", u.Host)
 	if err != nil {
 		panic(err)
 	}
-	go http.Serve(l, s)
+	go http.Serve(s.listener, s)
 
 	s.Response(203, nil, "")
 	for {
@@ -60,6 +61,17 @@
 	s.WaitRequest() // Consume dummy request.
 }
 
+func (s *HTTPServer) Stop() {
+	if s.listener == nil {
+		return
+	}
+	err := s.listener.Close()
+	if err != nil {
+		panic(err)
+	}
+	s.listener = nil
+}
+
 // Flush discards all pending requests and responses.
 func (s *HTTPServer) Flush() {
 	for {