Containeracl #422

kpfaulkner · 2016-10-21T11:22:36Z

This covers setting/getting ACLs for containers and also AccessPolicy creation.
This is referenced in Issue 14.

Hope it is of some use.

…actoring

kpfaulkner · 2016-10-24T00:56:49Z

Was expecting the CLA bot to ping me. Nothing? /cc @ahmetalpbalkan ?

msftclas · 2016-10-26T17:39:29Z

Hi @kpfaulkner, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

mcardosos

Just some comments :3

mcardosos · 2016-10-26T18:50:51Z

storage/blob_test.go

+	c.Assert(returnedPerms.ContainerAccess, chk.Equals, perms.AccessOptions.ContainerAccess)
+
+	// now check policy set.
+	c.Assert(1, chk.Equals, len(returnedPerms.AccessPolicy.SignedIdentifiers))


c.Assert(returnedPerms.AccessPolicy.SignedIdentifiers, chk.HasLen, 1) :3

mcardosos · 2016-10-26T18:50:58Z

storage/blob_test.go

+	if ID != "" {
+		perms.AccessPolicy.ID = ID
+		perms.AccessPolicy.StartTime = time.Now()
+		perms.AccessPolicy.ExpiryTime = time.Date(2017, 1, 1, 0, 0, 0, 0, time.UTC)


I think it would be better if Add is used

mcardosos · 2016-10-26T18:51:07Z

storage/blob.go

+			permissions += "d"
+		}
+
+		s += fmt.Sprintf("<Start>%s</Start>", accessPolicy.StartTime.Format("2006-01-02T15:04:05Z"))


Would one of this consts work instead of "2006-01-02T15:04:05Z" ?

I don't believe any of those Consts are valid. Tried a few that I thought might be ok but all were rejected. Have kept strings for now.

mcardosos · 2016-10-26T18:51:28Z

storage/blob.go

+}
+
+// generateAccessPolicy generates the XML access policy used as the payload for SetContainerPermissions.
+// TODO(kpfaulkner) find better way to generate the XML. This is clunky.


What about xml.Marshal :D ?
xmlMarshal in storage/util.go could be useful

mcardosos · 2016-10-26T18:51:43Z

storage/blob.go

+	var resp *storageResponse
+	var err error
+	if accessPolicyXML != "" {
+		headers["Content-Length"] = fmt.Sprintf("%v", len(accessPolicyXML))


What about strconv.Itoa(accessPolicyXML) ?

mcardosos · 2016-10-26T18:52:10Z

storage/blob.go

@@ -438,6 +496,124 @@ func (b BlobStorageClient) ContainerExists(name string) (bool, error) {
 	return false, err
 }

+// SetContainerPermissions sets up container permissions as per https://msdn.microsoft.com/en-us/library/azure/dd179391(d=printer).aspx
+func (b BlobStorageClient) SetContainerPermissions(container string, containerPermissions ContainerPermissions) error {
+	params := url.Values{"restype": {"container"},


Mini detail

params := url.Values{ "restype": {"container"}, "comp": {"acl"}, }

mcardosos · 2016-10-26T18:52:22Z

storage/blob.go

@@ -438,6 +496,124 @@ func (b BlobStorageClient) ContainerExists(name string) (bool, error) {
 	return false, err
 }

+// SetContainerPermissions sets up container permissions as per https://msdn.microsoft.com/en-us/library/azure/dd179391(d=printer).aspx


d=printer LOL!

mcardosos · 2016-10-26T18:54:14Z

storage/blob_test.go

@@ -678,6 +678,115 @@ func (s *StorageBlobSuite) TestSetBlobProperties(c *chk.C) {
 	c.Check(mPut.ContentLanguage, chk.Equals, props.ContentLanguage)
 }

+func (s *StorageBlobSuite) createContainerPermissions(accessType ContainerAccessType,
+	timeout int, leaseID string, ID string, canRead bool,
+	canWrite bool, canDelete bool) ContainerPermissions {


Add a new line for readability

kpfaulkner · 2016-10-26T19:35:41Z

Thanks, will do.

On 27 Oct. 2016, at 05:57, mcardosos [email protected] wrote:

@mcardosos requested changes on this pull request.

Just some comments :3

In storage/blob_test.go:

c.Assert(cli.CreateContainer(cnt, ContainerAccessTypePrivate), chk.IsNil)

defer cli.deleteContainer(cnt)
+

perms := s.createContainerPermissions(ContainerAccessTypeBlob, 0, "", "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTa=", true, true, true)
+

err := cli.SetContainerPermissions(cnt, perms)

c.Assert(err, chk.IsNil)
+

returnedPerms, err := cli.GetContainerPermissions(cnt, 0, "")

c.Assert(err, chk.IsNil)
+

// check container permissions itself.

c.Assert(returnedPerms.ContainerAccess, chk.Equals, perms.AccessOptions.ContainerAccess)
+

// now check policy set.

c.Assert(1, chk.Equals, len(returnedPerms.AccessPolicy.SignedIdentifiers))
c.Assert(returnedPerms.AccessPolicy.SignedIdentifiers, chk.HasLen, 1) :3

In storage/blob_test.go:

@@ -678,6 +678,115 @@ func (s *StorageBlobSuite) TestSetBlobProperties(c *chk.C) {
c.Check(mPut.ContentLanguage, chk.Equals, props.ContentLanguage)
}

+func (s *StorageBlobSuite) createContainerPermissions(accessType ContainerAccessType,
timeout int, leaseID string, ID string, canRead bool,

canWrite bool, canDelete bool) ContainerPermissions {

perms := ContainerPermissions{}

perms.AccessOptions.ContainerAccess = accessType

perms.AccessOptions.Timeout = timeout

perms.AccessOptions.LeaseID = leaseID
+

if ID != "" {
perms.AccessPolicy.ID = ID
perms.AccessPolicy.StartTime = time.Now()
perms.AccessPolicy.ExpiryTime = time.Date(2017, 1, 1, 0, 0, 0, 0, time.UTC)
I think it would be better if Add is used
In storage/blob.go:
  s += "<AccessPolicy>"
permissions := ""
if accessPolicy.CanRead {
    permissions += "r"
}
if accessPolicy.CanWrite {
    permissions += "w"
}
if accessPolicy.CanDelete {
    permissions += "d"
}
s += fmt.Sprintf("<Start>%s</Start>", accessPolicy.StartTime.Format("2006-01-02T15:04:05Z"))
Would one of this consts work instead of "2006-01-02T15:04:05Z" ?
In storage/blob.go:
defer resp.body.Close()

var out AccessPolicyResponse

err = xmlUnmarshal(resp.body, &out.SignedIdentifiersList)

if err != nil {
return nil, err
}
+

permissionResponse = &ContainerAccessResponse{}

permissionResponse.AccessPolicy = out.SignedIdentifiersList

permissionResponse.ContainerAccess = ContainerAccessType(containerAccess)
+

return permissionResponse, nil
+}
+
+// generateAccessPolicy generates the XML access policy used as the payload for SetContainerPermissions.
+// TODO(kpfaulkner) find better way to generate the XML. This is clunky.
What about xml.Marshal :D ?
xmlMarshal in storage/util.go could be useful
In storage/blob.go:
headers := b.client.getStandardHeaders()

if containerPermissions.AccessOptions.ContainerAccess != "" {
headers[ContainerAccessHeader] = string(containerPermissions.AccessOptions.ContainerAccess)
}
+

if containerPermissions.AccessOptions.LeaseID != "" {
headers[leaseID] = containerPermissions.AccessOptions.LeaseID
}
+

// generate the XML for the SharedAccessSignature if required.

accessPolicyXML := b.generateAccessPolicy(containerPermissions.AccessPolicy)
+

var resp *storageResponse

var err error

if accessPolicyXML != "" {
headers["Content-Length"] = fmt.Sprintf("%v", len(accessPolicyXML))
What about strconv.Itoa(accessPolicyXML) ?
In storage/blob.go:

@@ -438,6 +496,124 @@ func (b BlobStorageClient) ContainerExists(name string) (bool, error) {
return false, err
}

+// SetContainerPermissions sets up container permissions as per https://msdn.microsoft.com/en-us/library/azure/dd179391(d=printer).aspx
+func (b BlobStorageClient) SetContainerPermissions(container string, containerPermissions ContainerPermissions) error {

params := url.Values{"restype": {"container"},
Mini detail

params := url.Values{
"restype": {"container"},
"comp": {"acl"},
}
In storage/blob.go:

@@ -438,6 +496,124 @@ func (b BlobStorageClient) ContainerExists(name string) (bool, error) {
return false, err
}

+// SetContainerPermissions sets up container permissions as per https://msdn.microsoft.com/en-us/library/azure/dd179391(d=printer).aspx
d=printer LOL!

In storage/blob_test.go:

@@ -678,6 +678,115 @@ func (s *StorageBlobSuite) TestSetBlobProperties(c *chk.C) {
c.Check(mPut.ContentLanguage, chk.Equals, props.ContentLanguage)
}

+func (s *StorageBlobSuite) createContainerPermissions(accessType ContainerAccessType,

timeout int, leaseID string, ID string, canRead bool,

canWrite bool, canDelete bool) ContainerPermissions {
Add a new line for readability

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

mcardosos · 2016-10-26T20:51:44Z

storage/blob_test.go

+	c.Assert(cli.CreateContainer(cnt, ContainerAccessTypePrivate), chk.IsNil)
+	defer cli.deleteContainer(cnt)
+
+	perms := s.createContainerPermissions(ContainerAccessTypeBlob, 30, "", "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTa=", true, true, true)


Just a question, where does the ID come from?

Just made it up (well stole from a previous test). Just needed to be a unique string... and that's pretty unique :)

kpfaulkner · 2016-10-26T21:32:25Z

Have committed all changes except XML generation. Will get that done soon

mcardosos · 2016-10-28T01:48:48Z

storage/blob.go

+
+	if accessPolicy.ID != "" {
+		ap := b.convertAccessPolicyToXMLStructs(accessPolicy)
+		body, _, err := xmlMarshal(ap.SignedIdentifiersList)


I think you need just to include <?xml version="1.0" encoding="utf-8"?

Nah, already looked at that. Isn't required. The most bizarre thing is locally the test is running fine (against a real Azure acct) but on travis is failing.

If I take the XML generated in the travis run and try it locally it fails (so at least I can work on that) but check this out:

This is the generated version that works fine:

MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTa=

2016-10-28T01:43:41.4689754Z
2016-10-28T11:43:41.4689754Z
rwd

This is generated on travis and fails.

MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTa=

2016-10-28T01:39:15.324363803Z
2016-10-28T11:39:15.324364101Z
rwd

Pretty similar :)

One thing I think I've just discovered is that the travis one has more decimal places for the time and I think that's screwing it up.

Just confirmed, 7 decimal places work fine.... anything more than that blows up. In travis its generating more. :/

I was exploring the option of having create my own time struct (so I can control the serialisation of the time). Beyond generating my own struct, any Go suggestions?

ok, have done 2 things to "resolve" it.

make sure the tests round their timestamps down to the second.

make sure the XML generation code rounds it as well (yes, strictly dont need Tidy up with gofmt #1 then, but it's there already so thought I'd leave it).

Am wondering if the number of decimal places is based off platform? I'm coding and testing on Windows where I assume travis is on Linux?

It looks like Azure just works with 7 decimals (Request body section). I would format the time in the XML gen with a custom string... "2006-01-02T15:04:05.0000000Z07:00"

kpfaulkner · 2016-10-29T04:29:04Z

all changes done.

mcardosos

After this changes, it LGTM. Something more to add, @vishrutshah , @jhendrixMSFT ?

mcardosos · 2016-10-31T18:42:44Z

storage/blob.go

+
+	if accessPolicy.ID != "" {
+		ap := b.convertAccessPolicyToXMLStructs(accessPolicy)
+		body, _, err := xmlMarshal(ap.SignedIdentifiersList)


If you just need ap.SignedIdentifiersList, don't gen the complete ap struct :D

Changed. Was still trying to figure out XML Marshaling in Go. Still a strange strange beast to me :)

mcardosos · 2016-10-31T18:52:20Z

storage/blob.go

+// convertAccessPolicyToXMLStructs converts between AccessPolicyDetails which is a struct better for API usage to the
+// AccessPolicy struct which will get converted to XML.
+func (b BlobStorageClient) convertAccessPolicyToXMLStructs(accessPolicy AccessPolicyDetails) (accessPolicyToMarshal AccessPolicy) {
+	accessPolicyToMarshal = AccessPolicy{}


I think something like this is a bit more readable...

return AccessPolicy{ SignedIdentifiersList: SignedIdentifiers{ SignedIdentifiers: []SignedIdentifier{ { ID: accessPolicy.ID, AccessPolicy: AccessPolicyDetailsXML{ StartTime: accessPolicy.StartTime.Round(time.Second), ExpiryTime: accessPolicy.ExpiryTime.Round(time.Second), Permission: generatePermissions(accessPolicy), }, }, }, }, }

mcardosos · 2016-10-31T18:54:44Z

storage/blob.go

+	return permissionResponse, nil
+}
+
+func (b BlobStorageClient) generatePermissions(accessPolicy AccessPolicyDetails) (permissions string) {


I don't think you need b BlobStorageClient here. The same goes for convertAccessPolicyToXMLStructs and generateAccessPolicy.

jhendrixMSFT · 2016-11-01T18:38:06Z

storage/blob.go

+	}
+
+	if resp != nil {
+		defer resp.body.Close()


I think we need to capture and return the error object returned from Close() if it's not nil. See PR #431 for an example (util.go).

jhendrixMSFT · 2016-11-01T18:38:27Z

storage/blob.go

+	// containerAccess. Blob, Container, empty
+	containerAccess := resp.headers.Get(http.CanonicalHeaderKey(ContainerAccessHeader))
+
+	defer resp.body.Close()


Same as my above comment.

mcardosos

Yes, last comments (I noticed this later)

mcardosos · 2016-11-01T21:58:28Z

storage/blob.go

+			{
+				ID: accessPolicy.ID,
+				AccessPolicy: AccessPolicyDetailsXML{
+					StartTime:  accessPolicy.StartTime.Round(time.Second),


Last comment. In think it would be better if .UTC() was included here, instead of it being used when defining StartTime and ExpiryTime. createContainerPermissions woundn't need .UTC(). Also, do not round to the second, but to time.Nanosecond * 100 so all 7 decimals Azure expects can be used :D

kpfaulkner · 2016-11-04T05:36:23Z

Reverted to rounding to nearest second. 100 * Nano was not working.
Found out (the hard way) that rounding by nano * 100 can generate 6 decimal places sometimes (but most of the times 7). 6 Decimal places breaks the Azure service.

We could either pad to 7 decimal places, or (as I've done) round to second.

Given on https://msdn.microsoft.com/en-us/library/azure/dd179391.aspx it says that rounding to second (even minute) is perfectly valid, my vote is for second rounding.

Objections?

example output showing 6 vs 7 decimal places:

C:\Users\kenfa\projects\gopath\src\helloworld> go run .\helloworld.go
2016/11/04 16:29:02 start time is 2016-11-04 16:29:02.7634219 +1100 AEDT
2016/11/04 16:29:02 exp time is 2016-11-05 02:29:02.7634219 +1100 AEDT
2016/11/04 16:29:02 xml MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTa=2016-11-04T05:29:02.7634219Z2016-11-04T15:29:02.7634219Zrwd</Permiss
ion>
2016/11/04 16:29:03 GOOD

C:\Users\kenfa\projects\gopath\src\helloworld> go run .\helloworld.go
2016/11/04 16:29:05 start time is 2016-11-04 16:29:05.268237 +1100 AEDT
2016/11/04 16:29:05 exp time is 2016-11-05 02:29:05.268237 +1100 AEDT
2016/11/04 16:29:05 xml MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTa=2016-11-04T05:29:05.268237Z2016-11-04T15:29:05.268237Zrwd</Permissio
n>
2016/11/04 16:29:06 storage: service returned error: StatusCode=400, ErrorCode=InvalidXmlDocument, ErrorMessage=XML spec
ified is not syntactically valid.
RequestId:b35db99a-0001-000c-335c-365e3f000000
Time:2016-11-04T05:29:07.7640920Z, RequestId=b35db99a-0001-000c-335c-365e3f000000, QueryParameterName=, QueryParameterVa
lue=
exit status 1

mcardosos · 2016-11-04T17:58:20Z

No objections on the time stuff, everything else looks good to me. Anything more to add @vishrutshah
@jhendrixMSFT ?

mcardosos · 2016-11-08T17:55:09Z

Hey @kpfaulkner , thanks for the awesome contribution!

kpfaulkner added 21 commits October 6, 2016 22:20

beginning of blob lease functionality

0bfe6be

breaklease with and without break period working

daf1095

change lease working

5161cd3

renew lease working

15e14b1

refactor common code into leaseCommonPut

4a35137

all unit tests passing

ab94ed9

tweak of godoc

080d2ef

Set container ACL initial code

db88e32

change so param is struct and can handle optional properties

473d57d

adding in SAS part of permissions

5b3af80

Generate XML payload for setting permissions

63cb499

permissions working unsure about access policy:

309d477

working on XML deserialization, WIP

f9a0fd1

GetContainerPermission returning permissions correctly. Check for ref…

4184e8d

…actoring

merged master

cce5bd0

containerACL tests in progress

907bd6c

upstream merge

6083a83

Merge branch 'master' into containeracl

ac366db

SetContainerPermission and GetContainerPermission tests

362ef1d

removing some debugging

5bf4861

formatting fix causing lint error

831ca62

msftclas added the cla-already-signed label Oct 26, 2016

mcardosos suggested changes Oct 26, 2016

View reviewed changes

mcardosos reviewed Oct 26, 2016

View reviewed changes

changes based on Mcardosos' feedback.

522dfdc

marshalling working

f22ad3d

kpfaulkner added 3 commits October 28, 2016 12:00

fixing test

bb22d80

test tweak

21f854e

debugging tests working fine locally

7287de8

mcardosos reviewed Oct 28, 2016

View reviewed changes

kpfaulkner added 2 commits October 28, 2016 13:15

rounding of access policy times

84c0f6c

force rounding of dates for policy

0df936c

mcardosos suggested changes Oct 31, 2016

View reviewed changes

changes based on review

24913ed

mcardosos approved these changes Oct 31, 2016

View reviewed changes

jhendrixMSFT reviewed Nov 1, 2016

View reviewed changes

capturing potential error from closing body

6a9edd1

mcardosos suggested changes Nov 3, 2016

View reviewed changes

kpfaulkner added 2 commits November 4, 2016 12:47

change accesspolicy to convert to UTC

3218bf1

reverting to rounding to second.

5b8d080

kpfaulkner added 2 commits November 4, 2016 16:42

rounding times for tests so test reflects reality

ded4c38

typo

c47bff4

mcardosos approved these changes Nov 4, 2016

View reviewed changes

mcardosos merged commit 1f47b81 into Azure:master Nov 8, 2016

mcardosos mentioned this pull request Jun 22, 2017

Queuesas #657

Merged

Containeracl #422

Containeracl #422

Conversation

kpfaulkner commented Oct 21, 2016

kpfaulkner commented Oct 24, 2016

msftclas commented Oct 26, 2016

mcardosos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpfaulkner commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpfaulkner commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpfaulkner commented Oct 29, 2016

mcardosos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcardosos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpfaulkner commented Nov 4, 2016

mcardosos commented Nov 4, 2016

mcardosos commented Nov 8, 2016