Merge lp:~julian-edwards/gwacl/container-acl into lp:gwacl

[Julian Edwards]

Add API call for SetContainerACL.

This adds the SetContainerACL call where you can set a container's ACL. This ACL has three options (unless you dive into shared access keys, which we don't do here):

 1. private
 2. container - allows full read-only access to all aspects of the container
 3. blob - allows people to retrieve known blobs from a container, nothing else including no container listing

I buggered up my new branch and branched off my previous one instead of trunk, so I stuck it as a prerequisite to make the diff correct. Ignore it here.

[Jeroen T. Vermeulen (jtv)]

I see we have "*Request" and "*Params" types now. Doesn't it get a bit verbose, what with all the long camel-cased names?

.

In example/storage/run.go:

    case "containeracl":
        fmt.Printf("***:%s\n", acl)

—did you intend to remove this before submitting? Or if not, it could be clearer.

.

        if container == "" {
            return "", fmt.Errorf("Must supply container with containeracl")
        }

        if acl != "container" && acl != "blob" && acl != "private" {
            return "", fmt.Errorf("Usage: containeracl -container=<container> <container|blob|private>")
        }

—this would be so easy in Python: you'd just use the same single list of possibilities for the check and the error message. Go adds more overhead, pushing you towards a "switch" as the clearest expression.

But given that the "<container|blob|private>" choice keeps coming back, isn't it worth folding all this into a single data structure?

.

In SetContainerACL:

    switch req.Access {
    case "container", "blob":
        extraHeaders.Add("x-ms-blob-public-access", req.Access)
    case "private":
        // Don't add a header, Azure resets to private if it's omitted.
    default:
        panic("Access must be one of 'container', 'blob' or 'private'")
    }

Is the special treatment of "private" really pulling its weight here? If it's at all reasonable to extraHeaders.Add("x-ms-blob-public-access", "private") then I'd just do that unconditionally, and reduce this switch to just one line. It'd also save you some testing.

.

In TestInvalidAccessTypePanics, for testing the contents of the error, you probably want the ErrorMatches checker rather than Equals — since the panic is probably an error implementation, not a string as such. Assuming that it compares to a string may be a bit brittle. ErrorMatches takes a regex for the error text.

.

ISTR seeing some non-aligned initializer elements in there. Fine by me, but it may mean that you ought to re-run the formatter. Save somebody else the spurious diff.

.

And that's it! My compliments on the thorough testing.

review: Approve

[Julian Edwards (julian-edwards)]

On 24/06/13 20:05, Jeroen T. Vermeulen wrote:
> Review: Approve
>
> I see we have "*Request" and "*Params" types now. Doesn't it get a bit verbose, what with all the long camel-cased names?

Yeah, we need to make all of this consistent :/ I have a card on the
board already to review this before it gets out of hand.

>
> .
>
> In example/storage/run.go:
>
> case "containeracl":
> fmt.Printf("***:%s\n", acl)
>
> —did you intend to remove this before submitting? Or if not, it could be clearer.

I did mean to remove it, it was to debug the thing I complained about in
IRC yesterday where flags coming after positional args are treated like
positional args :/

>
> .
>
> if container == "" {
> return "", fmt.Errorf("Must supply container with containeracl")
> }
>
> if acl != "container" && acl != "blob" && acl != "private" {
> return "", fmt.Errorf("Usage: containeracl -container=<container> <container|blob|private>")
> }
>
> —this would be so easy in Python: you'd just use the same single list of possibilities for the check and the error message. Go adds more overhead, pushing you towards a "switch" as the clearest expression.
>
> But given that the "<container|blob|private>" choice keeps coming back, isn't it worth folding all this into a single data structure?

Maybe. My Go-fu is not strong enough to see this stuff as clearly and
intuitively as with Python.

We've tended to treat the example code as a bit of a hackfest but
perhaps it's time to clean it up and make it more of a good example than
a bad one.

>
> .
>
> In SetContainerACL:
>
> switch req.Access {
> case "container", "blob":
> extraHeaders.Add("x-ms-blob-public-access", req.Access)
> case "private":
> // Don't add a header, Azure resets to private if it's omitted.
> default:
> panic("Access must be one of 'container', 'blob' or 'private'")
> }
>
> Is the special treatment of "private" really pulling its weight here? If it's at all reasonable to extraHeaders.Add("x-ms-blob-public-access", "private") then I'd just do that unconditionally, and reduce this switch to just one line. It'd also save you some testing.

Yes. The header *has* to be omitted for private access. This does
require explicit testing. I added the empty case to make it obvious
what it's doing with that case and also so there's a sensible default case.

>
> .
>
> In TestInvalidAccessTypePanics, for testing the contents of the error, you probably want the ErrorMatches checker rather than Equals — since the panic is probably an error implementation, not a string as such. Assuming that it compares to a string may be a bit brittle. ErrorMatches takes a regex for the error text.

Good point, thanks.

>
> .
>
> ISTR seeing some non-aligned initializer elements in there. Fine by me, but it may mean that you ought to re-run the formatter. Save somebody else the spurious diff.

Yeah, will do.

>
> .
>
> And that's it! My compliments on the thorough testing.
>

Why thank you sir!

[Julian Edwards (julian-edwards)]

Well here's why I used Equals instead of ErrorMatches:

storage_base_test.go:1177:
    context.SetContainerACL(&SetContainerACLRequest{
        Container: "mycontainer", Access: "thisisnotvalid"})
storage_base_test.go:1173:
    c.Assert(err, ErrorMatches, "Access must be one of 'container',
'blob' or 'private'")
... value string = "Access must be one of 'container', 'blob' or 'private'"
... regex string = "Access must be one of 'container', 'blob' or 'private'"
... Value is not an error

So recover() here returns a string not an error. Notice the declaration
of recover() in the Go source:

func recover() interface{}

The original panic panicked with a string, not an error.

[lp:~julian-edwards/gwacl/container-acl]

138. By Julian Edwards on 2013-06-25

remove spurious printf

139. By Julian Edwards on 2013-06-25

format

=== modified file 'example/storage/run.go'
--- example/storage/run.go	2013-06-24 11:25:25 +0000
+++ example/storage/run.go	2013-06-25 00:41:29 +0000
@@ -26,6 +26,7 @@
 var blobtype string
 var size int
 var pagerange string
+var acl string
 
 func checkContainerAndFilename() error {
     if container == "" || filename == "" {
@@ -34,7 +35,7 @@
     return nil
 }
 
-var VALID_CMDS string = "<listcontainers|list|get|add|delete|putblob|putpage>"
+var VALID_CMDS string = "<listcontainers|containeracl|list|get|add|delete|putblob|putpage>"
 
 func badOperationError() error {
     return fmt.Errorf("Must specify one of %s", VALID_CMDS)
@@ -50,6 +51,7 @@
     flag.StringVar(&blobtype, "blobtype", "", "Type of blob, 'page' or 'block'")
     flag.IntVar(&size, "size", 0, "Size of blob to create for a page 'putblob'")
     flag.StringVar(&pagerange, "pagerange", "", "When uploading to a page blob, this specifies what range in the blob. Use the format 'start-end', e.g. -pagerange 1024-2048")
+    flag.StringVar(&acl, "acl", "", "When using 'containeracl', specify an ACL type")
     flag.Parse()
 
     if account == "" {
@@ -63,6 +65,14 @@
     switch flag.Arg(0) {
     case "listcontainers":
         return "listcontainers", nil
+    case "containeracl":
+        if container == "" {
+            return "", fmt.Errorf("Must supply container with containeracl")
+        }
+        if acl != "container" && acl != "blob" && acl != "private" {
+            return "", fmt.Errorf("Usage: containeracl -container=<container> <container|blob|private>")
+        }
+        return "containeracl", nil
     case "list":
         if container == "" {
             return "", fmt.Errorf("Must supply container with 'list'")
@@ -119,6 +129,9 @@
     List files in a container:
         list -container=<container>
 
+    Set access on a container:
+        containeracl -container=<container> -acl <container|blob|private>
+
     Get a file from a container (it's returned on stdout):
         get -container=<container> -filename=<filename>
     
@@ -158,6 +171,14 @@
     }
 }
 
+func containeracl(storage gwacl.StorageContext) {
+    err := storage.SetContainerACL(&gwacl.SetContainerACLRequest{
+        Container: container,
+        Access:    acl,
+    })
+    dumpError(err)
+}
+
 func list(storage gwacl.StorageContext) {
     request := &gwacl.ListBlobsRequest{
         Container: container, Prefix: prefix}
@@ -260,6 +281,8 @@
     switch op {
     case "listcontainers":
         listcontainers(storage)
+    case "containeracl":
+        containeracl(storage)
     case "list":
         list(storage)
     case "add":

=== modified file 'storage_base.go'
--- storage_base.go	2013-06-25 00:31:29 +0000
+++ storage_base.go	2013-06-25 00:41:29 +0000
@@ -615,3 +615,41 @@
     }
     return response.Body, nil
 }
+
+type SetContainerACLRequest struct {
+    Container string // Container name in the storage account
+    Access    string // "container", "blob", or "private"
+}
+
+// SetContainerACL sets the specified container's access rights.
+// See http://msdn.microsoft.com/en-us/library/windowsazure/dd179391.aspx
+func (context *StorageContext) SetContainerACL(req *SetContainerACLRequest) error {
+    uri := addURLQueryParams(
+        context.getContainerURL(req.Container),
+        "restype", "container",
+        "comp", "acl")
+
+    extraHeaders := http.Header{}
+    switch req.Access {
+    case "container", "blob":
+        extraHeaders.Add("x-ms-blob-public-access", req.Access)
+    case "private":
+        // Don't add a header, Azure resets to private if it's omitted.
+    default:
+        panic("Access must be one of 'container', 'blob' or 'private'")
+    }
+
+    _, err := context.performRequest(requestParams{
+        Method:         "PUT",
+        URL:            uri,
+        APIVersion:     "2009-09-19",
+        ExtraHeaders:   extraHeaders,
+        ExpectedStatus: http.StatusOK,
+    })
+
+    if err != nil {
+        msg := fmt.Sprintf("failed to set ACL for container %s: ", req.Container)
+        return extendError(err, msg)
+    }
+    return nil
+}

=== modified file 'storage_base_test.go'
--- storage_base_test.go	2013-06-25 00:31:29 +0000
+++ storage_base_test.go	2013-06-25 00:41:29 +0000
@@ -1132,3 +1132,82 @@
     c.Check(err, ErrorMatches, ".*Frotzed.*")
     c.Check(err, ErrorMatches, ".*failed to get blob.*")
 }
+
+type TestSetContainerACL struct{}
+
+var _ = Suite(&TestSetContainerACL{})
+
+func (suite *TestSetContainerACL) TestHappyPath(c *C) {
+    response := makeHttpResponse(http.StatusOK, "")
+    transport := &TestTransport{Response: response}
+    context := makeStorageContext(transport)
+    err := context.SetContainerACL(&SetContainerACLRequest{
+        Container: "mycontainer", Access: "container"})
+
+    c.Assert(err, IsNil)
+    c.Check(transport.Request.Method, Equals, "PUT")
+    c.Check(transport.Request.URL.String(), Matches,
+        fmt.Sprintf(
+            "http://%s.blob.core.windows.net/mycontainer?.*", context.Account))
+    c.Check(transport.Request.URL.Query(), DeepEquals, url.Values{
+        "comp":    {"acl"},
+        "restype": {"container"},
+    })
+
+    c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
+    c.Check(transport.Request.Header.Get("x-ms-blob-public-access"), Equals, "container")
+}
+
+func (suite *TestSetContainerACL) TestAcceptsBlobAccess(c *C) {
+    response := makeHttpResponse(http.StatusOK, "")
+    transport := &TestTransport{Response: response}
+    context := makeStorageContext(transport)
+    err := context.SetContainerACL(&SetContainerACLRequest{
+        Container: "mycontainer", Access: "blob"})
+    c.Assert(err, IsNil)
+    c.Check(transport.Request.Header.Get("x-ms-blob-public-access"), Equals, "blob")
+}
+
+func (suite *TestSetContainerACL) TestAccessHeaderOmittedWhenPrivate(c *C) {
+    response := makeHttpResponse(http.StatusOK, "")
+    transport := &TestTransport{Response: response}
+    context := makeStorageContext(transport)
+    err := context.SetContainerACL(&SetContainerACLRequest{
+        Container: "mycontainer", Access: "private"})
+
+    c.Assert(err, IsNil)
+    c.Check(transport.Request.Header.Get("x-ms-blob-public-access"), Equals, "")
+}
+
+func (suite *TestSetContainerACL) TestInvalidAccessTypePanics(c *C) {
+    defer func() {
+        err := recover()
+        c.Assert(err, Equals, "Access must be one of 'container', 'blob' or 'private'")
+    }()
+    context := makeStorageContext(&TestTransport{})
+    context.SetContainerACL(&SetContainerACLRequest{
+        Container: "mycontainer", Access: "thisisnotvalid"})
+    c.Assert("This test failed", Equals, "because there was no panic")
+}
+
+func (suite *TestSetContainerACL) TestClientSideError(c *C) {
+    error := fmt.Errorf("canned-error")
+    context := makeStorageContext(&TestTransport{Error: error})
+    err := context.SetContainerACL(&SetContainerACLRequest{
+        Container: "mycontainer", Access: "private"})
+    c.Assert(err, NotNil)
+}
+
+// Azure HTTP errors (for instance 404 responses) are propagated back to
+// the caller as ServerError objects.
+func (suite *TestSetContainerACL) TestServerError(c *C) {
+    response := makeHttpResponse(http.StatusNotFound, "not found")
+    context := makeStorageContext(&TestTransport{Response: response})
+    err := context.SetContainerACL(&SetContainerACLRequest{
+        Container: "mycontainer", Access: "private"})
+    c.Assert(err, NotNil)
+    serverError, ok := err.(*ServerError)
+    c.Check(ok, Equals, true)
+    c.Check(serverError.HTTPStatus.StatusCode(), Equals, http.StatusNotFound)
+    c.Check(IsNotFoundError(err), Equals, true)
+}