juju-core

Merge lp:~axwalk/juju-core/lp1216768-azure-public-storage-key into lp:~go-bot/juju-core/trunk

  1. lp1216768-azure-public-storage-key
  2. Merge into trunk
Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 1710
Proposed branch: lp:~axwalk/juju-core/lp1216768-azure-public-storage-key
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 177 lines (+33/-18)
2 files modified
provider/azure/storage.go (+6/-3)
provider/azure/storage_test.go (+27/-15)
To merge this branch: bzr merge lp:~axwalk/juju-core/lp1216768-azure-public-storage-key
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Juju Engineering Pending
Review via email: mp+182041@code.launchpad.net

Commit message

azure: check storage key when generating URL

The Azure provider currently generates an anonymous
(shared access signature) URL for accessing storage
blobs, regardless of whether we're using public or
private storage. If we're using public storage, we
should not attempt to generate a shared access
signature, which requires the account key.

Fixes bug #1216768

https://codereview.appspot.com/13220043/

Description of the change

azure: check storage key when generating URL

The Azure provider currently generates an anonymous
(shared access signature) URL for accessing storage
blobs, regardless of whether we're using public or
private storage. If we're using public storage, we
should not attempt to generate a shared access
signature, which requires the account key.

Fixes bug #1216768

https://codereview.appspot.com/13220043/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (8.3 KiB)

Reviewers: mp+182041_code.launchpad.net,

Message:
Please take a look.

Description:
azure: check storage key when generating URL

The Azure provider currently generates an anonymous
(shared access signature) URL for accessing storage
blobs, regardless of whether we're using public or
private storage. If we're using public storage, we
should not attempt to generate a shared access
signature, which requires the account key.

Fixes bug #1216768

https://code.launchpad.net/~axwalk/juju-core/lp1216768-azure-public-storage-key/+merge/182041

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/13220043/

Affected files:
   A [revision details]
   M provider/azure/storage.go
   M provider/azure/storage_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130825140921-3o6xvqsc01lf9v7q
+New revision: <email address hidden>

Index: provider/azure/storage.go
=== modified file 'provider/azure/storage.go'
--- provider/azure/storage.go 2013-08-05 04:06:09 +0000
+++ provider/azure/storage.go 2013-08-26 06:44:01 +0000
@@ -90,9 +90,12 @@
   if err != nil {
    return "", err
   }
- // 10 years should be good enough.
- expires := time.Now().AddDate(10, 0, 0)
- return context.GetAnonymousFileURL(storage.getContainer(), name,
expires), nil
+ if context.Key != "" {
+ // 10 years should be good enough.
+ expires := time.Now().AddDate(10, 0, 0)
+ return context.GetAnonymousFileURL(storage.getContainer(), name,
expires), nil
+ }
+ return context.GetFileURL(storage.getContainer(), name), nil
  }

  // ConsistencyStrategy is specified in the StorageReader interface.

Index: provider/azure/storage_test.go
=== modified file 'provider/azure/storage_test.go'
--- provider/azure/storage_test.go 2013-08-13 15:45:39 +0000
+++ provider/azure/storage_test.go 2013-08-26 06:44:01 +0000
@@ -83,11 +83,12 @@
  // fake HTTP server set up to always return preconfigured http.Response
objects.
  // The MockingTransport object can be used to check that the expected
query has
  // been issued to the test server.
-func makeFakeStorage(container, account string) (azureStorage,
*MockingTransport) {
+func makeFakeStorage(container, account, key string) (azureStorage,
*MockingTransport) {
   transport := &MockingTransport{}
   client := &http.Client{Transport: transport}
   storageContext := gwacl.NewTestStorageContext(client)
   storageContext.Account = account
+ storageContext.Key = key
   context := &testStorageContext{container: container, storageContext:
storageContext}
   azStorage := azureStorage{storageContext: context}
   return azStorage, transport
@@ -124,7 +125,7 @@
  func (*storageSuite) TestList(c *gc.C) {
   container := "container"
   response := makeResponse(blobListResponse, http.StatusOK)
- azStorage, transport := makeFakeStorage(container, "account")
+ azStorage, transport := makeFakeStorage(container, "account", "")
   transport.AddExchange(response, nil)

   prefix := "prefix"
@@ -143,7 +144,7 @@
   // case the p...

Read more...

Revision history for this message
John A Meinel (jameinel) wrote :

LGTM though I'd want you to make sure to run it by the Azure experts.
You're all at the sprint, right?

https://codereview.appspot.com/13220043/

Revision history for this message
Dave Cheney (dave-cheney) wrote :

> LGTM though I'd want you to make sure to run it by the Azure experts.
> You're all at the sprint, right?

LGTM.

Yup, the Azure expert himself has blessed this, he helped us debug
this, but as he'd already nixed himself from the gophers group, he
left it too us to handle the mechanics of the commit.

Revision history for this message
Julian Edwards (julian-edwards) wrote :

:-)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'provider/azure/storage.go'
2--- provider/azure/storage.go 2013-08-05 04:06:09 +0000
3+++ provider/azure/storage.go 2013-08-26 06:50:48 +0000
4@@ -90,9 +90,12 @@
5 if err != nil {
6 return "", err
7 }
8- // 10 years should be good enough.
9- expires := time.Now().AddDate(10, 0, 0)
10- return context.GetAnonymousFileURL(storage.getContainer(), name, expires), nil
11+ if context.Key != "" {
12+ // 10 years should be good enough.
13+ expires := time.Now().AddDate(10, 0, 0)
14+ return context.GetAnonymousFileURL(storage.getContainer(), name, expires), nil
15+ }
16+ return context.GetFileURL(storage.getContainer(), name), nil
17 }
18
19 // ConsistencyStrategy is specified in the StorageReader interface.
20
21=== modified file 'provider/azure/storage_test.go'
22--- provider/azure/storage_test.go 2013-08-13 15:45:39 +0000
23+++ provider/azure/storage_test.go 2013-08-26 06:50:48 +0000
24@@ -83,11 +83,12 @@
25 // fake HTTP server set up to always return preconfigured http.Response objects.
26 // The MockingTransport object can be used to check that the expected query has
27 // been issued to the test server.
28-func makeFakeStorage(container, account string) (azureStorage, *MockingTransport) {
29+func makeFakeStorage(container, account, key string) (azureStorage, *MockingTransport) {
30 transport := &MockingTransport{}
31 client := &http.Client{Transport: transport}
32 storageContext := gwacl.NewTestStorageContext(client)
33 storageContext.Account = account
34+ storageContext.Key = key
35 context := &testStorageContext{container: container, storageContext: storageContext}
36 azStorage := azureStorage{storageContext: context}
37 return azStorage, transport
38@@ -124,7 +125,7 @@
39 func (*storageSuite) TestList(c *gc.C) {
40 container := "container"
41 response := makeResponse(blobListResponse, http.StatusOK)
42- azStorage, transport := makeFakeStorage(container, "account")
43+ azStorage, transport := makeFakeStorage(container, "account", "")
44 transport.AddExchange(response, nil)
45
46 prefix := "prefix"
47@@ -143,7 +144,7 @@
48 // case the provider should interpret this as "no files" and return nil.
49 container := "container"
50 response := makeResponse("", http.StatusNotFound)
51- azStorage, transport := makeFakeStorage(container, "account")
52+ azStorage, transport := makeFakeStorage(container, "account", "")
53 transport.AddExchange(response, nil)
54
55 names, err := azStorage.List("prefix")
56@@ -156,7 +157,7 @@
57 container := "container"
58 filename := "blobname"
59 response := makeResponse(blobContent, http.StatusOK)
60- azStorage, transport := makeFakeStorage(container, "account")
61+ azStorage, transport := makeFakeStorage(container, "account", "")
62 transport.AddExchange(response, nil)
63
64 reader, err := azStorage.Get(filename)
65@@ -177,7 +178,7 @@
66 container := "container"
67 filename := "blobname"
68 response := makeResponse("not found", http.StatusNotFound)
69- azStorage, transport := makeFakeStorage(container, "account")
70+ azStorage, transport := makeFakeStorage(container, "account", "")
71 transport.AddExchange(response, nil)
72 _, err := azStorage.Get(filename)
73 c.Assert(err, gc.NotNil)
74@@ -188,7 +189,7 @@
75 blobContent := "test blob"
76 container := "container"
77 filename := "blobname"
78- azStorage, transport := makeFakeStorage(container, "account")
79+ azStorage, transport := makeFakeStorage(container, "account", "")
80 // The create container call makes two exchanges.
81 transport.AddExchange(makeResponse("", http.StatusNotFound), nil)
82 transport.AddExchange(makeResponse("", http.StatusCreated), nil)
83@@ -208,7 +209,7 @@
84 container := "container"
85 filename := "blobname"
86 response := makeResponse("", http.StatusAccepted)
87- azStorage, transport := makeFakeStorage(container, "account")
88+ azStorage, transport := makeFakeStorage(container, "account", "")
89 transport.AddExchange(response, nil)
90 err := azStorage.Remove(filename)
91 c.Assert(err, gc.IsNil)
92@@ -224,7 +225,7 @@
93 container := "container"
94 filename := "blobname"
95 response := makeResponse("", http.StatusForbidden)
96- azStorage, transport := makeFakeStorage(container, "account")
97+ azStorage, transport := makeFakeStorage(container, "account", "")
98 transport.AddExchange(response, nil)
99 err := azStorage.Remove(filename)
100 c.Assert(err, gc.NotNil)
101@@ -233,7 +234,7 @@
102 func (*storageSuite) TestRemoveAll(c *gc.C) {
103 // When we ask gwacl to remove all blobs, it calls DeleteContainer.
104 response := makeResponse("", http.StatusAccepted)
105- storage, transport := makeFakeStorage("cntnr", "account")
106+ storage, transport := makeFakeStorage("cntnr", "account", "")
107 transport.AddExchange(response, nil)
108
109 err := storage.RemoveAll()
110@@ -252,7 +253,7 @@
111 container := "container"
112 filename := "blobname"
113 response := makeResponse("", http.StatusNotFound)
114- azStorage, transport := makeFakeStorage(container, "account")
115+ azStorage, transport := makeFakeStorage(container, "account", "")
116 transport.AddExchange(response, nil)
117 err := azStorage.Remove(filename)
118 c.Assert(err, gc.IsNil)
119@@ -262,7 +263,8 @@
120 container := "container"
121 filename := "blobname"
122 account := "account"
123- azStorage, _ := makeFakeStorage(container, account)
124+ key := "bWFkZXlvdWxvb2sK"
125+ azStorage, _ := makeFakeStorage(container, account, key)
126 // Use a realistic service endpoint for this test, so that we can see
127 // that we're really getting the expected kind of URL.
128 setStorageEndpoint(&azStorage, gwacl.GetEndpoint("West US"))
129@@ -280,10 +282,20 @@
130 // The signature is base64-encoded.
131 _, err = base64.StdEncoding.DecodeString(signature)
132 c.Assert(err, gc.IsNil)
133+ // If Key is empty, query string does not contain a signature.
134+ key = ""
135+ azStorage, _ = makeFakeStorage(container, account, key)
136+ URL, err = azStorage.URL(filename)
137+ c.Assert(err, gc.IsNil)
138+ parsedURL, err = url.Parse(URL)
139+ c.Assert(err, gc.IsNil)
140+ values, err = url.ParseQuery(parsedURL.RawQuery)
141+ c.Assert(err, gc.IsNil)
142+ c.Check(values.Get("sig"), gc.HasLen, 0)
143 }
144
145 func (*storageSuite) TestCreateContainerCreatesContainerIfDoesNotExist(c *gc.C) {
146- azStorage, transport := makeFakeStorage("", "account")
147+ azStorage, transport := makeFakeStorage("", "account", "")
148 transport.AddExchange(makeResponse("", http.StatusNotFound), nil)
149 transport.AddExchange(makeResponse("", http.StatusCreated), nil)
150
151@@ -303,7 +315,7 @@
152
153 func (*storageSuite) TestCreateContainerIsDoneIfContainerAlreadyExists(c *gc.C) {
154 container := ""
155- azStorage, transport := makeFakeStorage(container, "account")
156+ azStorage, transport := makeFakeStorage(container, "account", "")
157 header := make(http.Header)
158 header.Add("Last-Modified", "last-modified")
159 header.Add("ETag", "etag")
160@@ -325,7 +337,7 @@
161 }
162
163 func (*storageSuite) TestCreateContainerFailsIfContainerInaccessible(c *gc.C) {
164- azStorage, transport := makeFakeStorage("", "account")
165+ azStorage, transport := makeFakeStorage("", "account", "")
166 transport.AddExchange(makeResponse("", http.StatusInternalServerError), nil)
167
168 err := azStorage.createContainer("cntnr")
169@@ -339,7 +351,7 @@
170 }
171
172 func (*storageSuite) TestDeleteContainer(c *gc.C) {
173- azStorage, transport := makeFakeStorage("", "account")
174+ azStorage, transport := makeFakeStorage("", "account", "")
175 transport.AddExchange(makeResponse("", http.StatusAccepted), nil)
176
177 err := azStorage.deleteContainer("cntnr")

Subscribers

People subscribed via source and target branches

to status/vote changes: