Merge lp:~jtv/gwacl/service-endpoints into lp:gwacl
- service-endpoints
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Jeroen T. Vermeulen |
Approved revision: | 224 |
Merged at revision: | 213 |
Proposed branch: | lp:~jtv/gwacl/service-endpoints |
Merge into: | lp:gwacl |
Diff against target: |
912 lines (+306/-82) 12 files modified
endpoints.go (+63/-0) endpoints_test.go (+115/-0) example/management/run.go (+3/-1) management_base.go (+2/-2) management_base_test.go (+28/-28) management_test.go (+1/-1) poller_test.go (+3/-3) storage_base.go (+22/-8) storage_base_test.go (+28/-6) x509dispatcher_test.go (+5/-5) x509session.go (+10/-8) x509session_test.go (+26/-20) |
To merge this branch: | bzr merge lp:~jtv/gwacl/service-endpoints |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Julian Edwards (community) | Approve | ||
Review via email: mp+178660@code.launchpad.net |
Commit message
Support Chinese Azure endpoints, as well as the international ones.
Description of the change
This removes the hard-coding of a single pair of base URLs for Azure's Storage and Management APIs. I defined a new type, APIEndpoint, specifying the base URL that these other URLs should be derived from. It's a bit clearer than passing just URL strings around, because those can easily become ambiguous as to what components of the ultimate URL they already include. Methods on the new type will give you URLs for the storage API, the management API, and perhaps in the future, APIs for the other services as well.
You'll note that AZURE_URL is now defaultManagement, a private variable used for tests only. There no longer is a single URL that will service all cases, and actually this was never "the" Azure URL to begin with -- it was for the management API only.
Both the management and the storage API now need to know which location they will act on. Unfortunately the way in which these are added is entirely asymmetrical:
* The management API uses a constructor, which adds a mandatory argument. This means incompatibility in the API.
* The storage API uses a struct, so the field is optional. It chooses an arbitrary (but backwards-
I believe the right thing to do after this would be to remove the default value for the storage API, and require a value. (Not done here because the diff was quite large enough already!) Maybe even provide a constructor like we have for the management API, because if you're going to add a mandatory parameter in a place that just about every client is going to use, breaking client code at compile time is still better than breaking it at run time.
Jeroen
- 223. By Jeroen T. Vermeulen
-
Review change: rename StorageAPI to BlobStorageAPI.
- 224. By Jeroen T. Vermeulen
-
Document usage of the blob-storage API URL.
Jeroen T. Vermeulen (jtv) wrote : | # |
> 27 + if strings.
>
> It's "China East" and "China North" AFAICS, it might be worth being specific
> instead of using Contains in case a new one pops up that's not actually in the
> same domain. Although sod's law dictates whatever we do it'll be wrong ...
> but
> I think explicit is always better as you end up with fewer surprises.
I disagree. When a new region appears with "China" in its name, there's a good chance that it'll be in mainland China. Unless they called it "South China Sea" but it seems unlikely politically -- and "Indo-China" has gone right out of fashion. Conversely, there is a region in China that isn't in mainland China and isn't using the Chinese endpoints, and they took care not to name it after China.
So the name "China" seems to be a good indicator, and "locations with China in the names use the mainland-Chinese endpoints, the rest use the international ones" is a rule that accommodates both the known cases and expected future cases. It's explicit, just not as fragile and verbose as specifying known endpoints. It gives us a chance of supporting a new location without code changes.
> 60 +// StorageAPI returns the base URL for the endpoint's storage API.
> 61 +func (endpoint APIEndpoint) StorageAPI() string {
>
> Why not a pointer receiver? (same for ManagementAPI)
Because it's the language's way of saying that you're not going to get your object modified by calling this. I don't care much about it either way.
> 62 + return prefixHost("blob", string(endpoint))
> 63 +}
>
> Given there's three types of storage, this should to be named
> BlobStorageAPI().
Fixed.
> Also, the storage URL returned here is still not complete, it would need the
> account prefixed.
> It might be worth encapsulating that functionality in this object so that all
> the URL
> manipulation is done in the same place. I'd have StorageAPI take an account
> name parameter.
We do need that, but I felt that was in a different layer of responsibility. We already had a method for it, which I kept in place (although I made it re-use prefixHost so it's very simple).
> 114 +func (*endpointsSuite)
> TestGetEndpoint
> 115 + c.Check(
> 116 + GetEndpoint("South San Marino Highlands"),
> 117 + Equals,
> 118 + GetEndpoint("West US"))
> 119 + c.Check(
> 120 + GetEndpoint(
> 121 + Equals,
> 122 + GetEndpoint("China East"))
> 123 +}
>
> I'm not sure I like this test at all. Its expected output is a function of the
> method being tested
> rather than something either constant or independent of it, and that's kinda
> nasty. It also feels
> fragile.
>
> I'd much rather you were explicit in testing expected output to be the
> required endpoint URLs.
This test verifies a specific relationship between the function's responses to different inputs. Not the actual responses themselves; those are covered by tests at the appropriate level and they're not relevant here. This is just a higher level of abstraction and I don't think that's a bad thi...
Julian Edwards (julian-edwards) wrote : | # |
On Tuesday 06 Aug 2013 08:02:26 you wrote:
> I disagree. When a new region appears with "China" in its name, there's a
[snip]
Ok I just thought I'd bring it up. I think we'll end up getting bitten one way or
another but hey ... SEP.
> > 60 +// StorageAPI returns the base URL for the endpoint's storage
> > API.
> > 61 +func (endpoint APIEndpoint) StorageAPI() string {
> >
> > Why not a pointer receiver? (same for ManagementAPI)
>
> Because it's the language's way of saying that you're not going to get your
> object modified by calling this. I don't care much about it either way.
Ok - it's just that we know that non-pointer receivers don't meet the interface etc
etc. Most of the rest of the code uses pointer receivers.
> > Also, the storage URL returned here is still not complete, it would need
> > the account prefixed.
> > It might be worth encapsulating that functionality in this object so that
> > all the URL
> > manipulation is done in the same place. I'd have StorageAPI take an
> > account name parameter.
>
> We do need that, but I felt that was in a different layer of responsibility.
> We already had a method for it, which I kept in place (although I made it
> re-use prefixHost so it's very simple).
I genuinely don't think it's a different layer of responsibility.
My reasoning is that each storage account is a different API endpoint. This code
says it returns an API endpoint, but it doesn't, it's not usable yet.
> This test verifies a specific relationship between the function's responses
> to different inputs. Not the actual responses themselves; those are
> covered by tests at the appropriate level and they're not relevant here.
> This is just a higher level of abstraction and I don't think that's a bad
> thing.
>
> Making the change you ask for would replace an explicit statement of the
> relationship with one that is implicit and easily lost in the unnecessary
> detail. But it also increases fragility. For example, we might at some
> point remove the trailing slashes after the URLs' hostnames. Completely
> arbitrary change, but simplestreams might force us to make it someday.
> That is completely outside the scope of this test: the test shouldn't care.
> But the way you're asking me to write it, it'd break this test for no
> reason. An engineer with a whole bunch of these failures on their hands
> would have to fix up the URLs in the tests on auto-pilot.
>
> On the other hand, imagine that the *relationship* that this test verifies
> breaks during development. The test would fail, because that's what it's
> for. But if I wrote it your way, the required change would be
> indistinguishable from the change required for the inconsequential change
> from the first example. What is the engineer's reasonable response? Fix
> up the URLs in the tests, on auto-pilot. At that point the test becomes
> meaningless -- because the relationship that's being tested was left
> implicit in the data instead of explicit, and because of the unnecessary
> fragility.
Ok we'll have to agree to disagree.
> > I don't like the "try to set this" stuff at all here. Let's just panic if
> > it's not set.
>
> Neither ...
Preview Diff
1 | === added file 'endpoints.go' |
2 | --- endpoints.go 1970-01-01 00:00:00 +0000 |
3 | +++ endpoints.go 2013-08-06 08:00:44 +0000 |
4 | @@ -0,0 +1,63 @@ |
5 | +// Copyright 2013 Canonical Ltd. This software is licensed under the |
6 | +// GNU Lesser General Public License version 3 (see the file COPYING). |
7 | + |
8 | +package gwacl |
9 | + |
10 | +import ( |
11 | + "fmt" |
12 | + "net/url" |
13 | + "strings" |
14 | +) |
15 | + |
16 | +// APIEndpoint describes the base URL for accesing Windows Azure's APIs. |
17 | +// |
18 | +// Azure will have subdomains on this URL's domain, such as blob.<domain> for |
19 | +// storage, with further sub-domains for storage accounts; management.<domain> |
20 | +// for the management API; and possibly more such as queue.<domain>, |
21 | +// table.<domain>. APIEndpoint defines methods to obtain these URLs. |
22 | +type APIEndpoint string |
23 | + |
24 | +// GetEndpoint returns the API endpoint for the given location. This is |
25 | +// hard-coded, so some guesswork may be involved. |
26 | +func GetEndpoint(location string) APIEndpoint { |
27 | + if strings.Contains(location, "China") { |
28 | + // Mainland China is a special case. It has its own endpoint. |
29 | + return "https://core.chinacloudapi.cn/" |
30 | + } |
31 | + |
32 | + // The rest of the world shares a single endpoint. |
33 | + return "https://core.windows.net/" |
34 | +} |
35 | + |
36 | +// prefixHost prefixes the hostname part of a URL with a subdomain. For |
37 | +// example, prefixHost("foo", "http://example.com") becomes |
38 | +// "http://foo.example.com". |
39 | +// |
40 | +// The URL must be well-formed, and contain a hostname. |
41 | +func prefixHost(host, originalURL string) string { |
42 | + parsedURL, err := url.Parse(originalURL) |
43 | + if err != nil { |
44 | + panic(fmt.Errorf("failed to parse URL %s - %v", originalURL, err)) |
45 | + } |
46 | + if parsedURL.Host == "" { |
47 | + panic(fmt.Errorf("no hostname in URL '%s'", originalURL)) |
48 | + } |
49 | + // Escape manually. Strangely, turning a url.URL into a string does not |
50 | + // do this for you. |
51 | + parsedURL.Host = url.QueryEscape(host) + "." + parsedURL.Host |
52 | + return parsedURL.String() |
53 | +} |
54 | + |
55 | +// ManagementAPI returns the URL for the endpoint's management API. |
56 | +func (endpoint APIEndpoint) ManagementAPI() string { |
57 | + return prefixHost("management", string(endpoint)) |
58 | +} |
59 | + |
60 | +// BlobStorageAPI returns the base URL for the endpoint's blob storage API. |
61 | +// |
62 | +// Actual storage API requests are made to subdomains of this URL. To address |
63 | +// a particular storage account, prefix it as a subdomain to the hostname |
64 | +// portion of this URL. |
65 | +func (endpoint APIEndpoint) BlobStorageAPI() string { |
66 | + return prefixHost("blob", string(endpoint)) |
67 | +} |
68 | |
69 | === added file 'endpoints_test.go' |
70 | --- endpoints_test.go 1970-01-01 00:00:00 +0000 |
71 | +++ endpoints_test.go 2013-08-06 08:00:44 +0000 |
72 | @@ -0,0 +1,115 @@ |
73 | +// Copyright 2013 Canonical Ltd. This software is licensed under the |
74 | +// GNU Lesser General Public License version 3 (see the file COPYING). |
75 | + |
76 | +package gwacl |
77 | + |
78 | +import ( |
79 | + "fmt" |
80 | + . "launchpad.net/gocheck" |
81 | + "net/url" |
82 | +) |
83 | + |
84 | +type endpointsSuite struct{} |
85 | + |
86 | +var _ = Suite(&endpointsSuite{}) |
87 | + |
88 | +func (*endpointsSuite) TestGetEndpointReturnsEndpointsForKnownRegions(c *C) { |
89 | + internationalLocations := []string{ |
90 | + "West Europe", |
91 | + "East Asia", |
92 | + "East US 2", |
93 | + "Southeast Asia", |
94 | + "East US", |
95 | + "Central US", |
96 | + "West US", |
97 | + "North Europe", |
98 | + } |
99 | + internationalEndpoint := APIEndpoint("https://core.windows.net/") |
100 | + |
101 | + for _, location := range internationalLocations { |
102 | + c.Check(GetEndpoint(location), Equals, internationalEndpoint) |
103 | + } |
104 | + |
105 | + // The mainland-China locations have a different endpoint. |
106 | + // (Actually the East Asia data centre is said to be in Hong Kong, but it |
107 | + // acts as international). |
108 | + mainlandChinaLocations := []string{ |
109 | + "China East", |
110 | + "China North", |
111 | + } |
112 | + mainlandChinaEndpoint := APIEndpoint("https://core.chinacloudapi.cn/") |
113 | + for _, location := range mainlandChinaLocations { |
114 | + c.Check(GetEndpoint(location), Equals, mainlandChinaEndpoint) |
115 | + } |
116 | +} |
117 | + |
118 | +func (*endpointsSuite) TestGetEndpointMakesGoodGuessesForUknownRegions(c *C) { |
119 | + c.Check( |
120 | + GetEndpoint("South San Marino Highlands"), |
121 | + Equals, |
122 | + GetEndpoint("West US")) |
123 | + c.Check( |
124 | + GetEndpoint("Central China West"), |
125 | + Equals, |
126 | + GetEndpoint("China East")) |
127 | +} |
128 | + |
129 | +func (*endpointsSuite) TestPrefixHostPrefixesSubdomain(c *C) { |
130 | + c.Check( |
131 | + prefixHost("foo", "http://example.com"), |
132 | + Equals, |
133 | + "http://foo.example.com") |
134 | +} |
135 | + |
136 | +func (*endpointsSuite) TestPrefixHostPreservesOtherURLComponents(c *C) { |
137 | + c.Check( |
138 | + prefixHost("foo", "http://example.com/"), |
139 | + Equals, |
140 | + "http://foo.example.com/") |
141 | + c.Check( |
142 | + prefixHost("foo", "nntp://example.com"), |
143 | + Equals, |
144 | + "nntp://foo.example.com") |
145 | + c.Check( |
146 | + prefixHost("foo", "http://user@example.com"), |
147 | + Equals, |
148 | + "http://user@foo.example.com") |
149 | + c.Check( |
150 | + prefixHost("foo", "http://example.com:999"), |
151 | + Equals, |
152 | + "http://foo.example.com:999") |
153 | + c.Check( |
154 | + prefixHost("foo", "http://example.com/path"), |
155 | + Equals, |
156 | + "http://foo.example.com/path") |
157 | +} |
158 | + |
159 | +func (*endpointsSuite) TestPrefixHostEscapes(c *C) { |
160 | + host := "5%=1/20?" |
161 | + c.Check( |
162 | + prefixHost(host, "http://example.com"), |
163 | + Equals, |
164 | + fmt.Sprintf("http://%s.example.com", url.QueryEscape(host))) |
165 | +} |
166 | + |
167 | +func (*endpointsSuite) TestManagementAPICombinesWithGetEndpoint(c *C) { |
168 | + c.Check( |
169 | + GetEndpoint("West US").ManagementAPI(), |
170 | + Equals, |
171 | + "https://management.core.windows.net/") |
172 | + c.Check( |
173 | + GetEndpoint("China East").ManagementAPI(), |
174 | + Equals, |
175 | + "https://management.core.chinacloudapi.cn/") |
176 | +} |
177 | + |
178 | +func (*endpointsSuite) TestBlobStorageAPICombinesWithGetEndpoint(c *C) { |
179 | + c.Check( |
180 | + GetEndpoint("West US").BlobStorageAPI(), |
181 | + Equals, |
182 | + "https://blob.core.windows.net/") |
183 | + c.Check( |
184 | + GetEndpoint("China East").BlobStorageAPI(), |
185 | + Equals, |
186 | + "https://blob.core.chinacloudapi.cn/") |
187 | +} |
188 | |
189 | === modified file 'example/management/run.go' |
190 | --- example/management/run.go 2013-07-25 22:23:31 +0000 |
191 | +++ example/management/run.go 2013-08-06 08:00:44 +0000 |
192 | @@ -22,11 +22,13 @@ |
193 | var certFile string |
194 | var subscriptionID string |
195 | var pause bool |
196 | +var location string |
197 | |
198 | func getParams() error { |
199 | flag.StringVar(&certFile, "cert", "", "Name of your management certificate file (in PEM format).") |
200 | flag.StringVar(&subscriptionID, "subscriptionid", "", "Your Azure subscription ID.") |
201 | flag.BoolVar(&pause, "pause", false, "Wait for user input after the VM is brought up (useful for further testing)") |
202 | + flag.StringVar(&location, "location", "North Europe", "Azure cloud location, e.g. 'West US' or 'China East'") |
203 | |
204 | flag.Parse() |
205 | |
206 | @@ -73,7 +75,7 @@ |
207 | os.Exit(1) |
208 | } |
209 | |
210 | - api, err := gwacl.NewManagementAPI(subscriptionID, certFile) |
211 | + api, err := gwacl.NewManagementAPI(subscriptionID, certFile, location) |
212 | checkError(err) |
213 | |
214 | ExerciseHostedServicesAPI(api) |
215 | |
216 | === modified file 'management_base.go' |
217 | --- management_base.go 2013-07-25 22:02:41 +0000 |
218 | +++ management_base.go 2013-08-06 08:00:44 +0000 |
219 | @@ -34,8 +34,8 @@ |
220 | |
221 | // NewManagementAPI creates an object used to interact with Windows Azure's API. |
222 | // http://msdn.microsoft.com/en-us/library/windowsazure/ff800682.aspx |
223 | -func NewManagementAPI(subscriptionId string, certFile string) (*ManagementAPI, error) { |
224 | - session, err := newX509Session(subscriptionId, certFile) |
225 | +func NewManagementAPI(subscriptionId, certFile, location string) (*ManagementAPI, error) { |
226 | + session, err := newX509Session(subscriptionId, certFile, location) |
227 | if err != nil { |
228 | return nil, err |
229 | } |
230 | |
231 | === modified file 'management_base_test.go' |
232 | --- management_base_test.go 2013-07-25 22:02:41 +0000 |
233 | +++ management_base_test.go 2013-08-06 08:00:44 +0000 |
234 | @@ -36,7 +36,7 @@ |
235 | |
236 | func makeAPI(c *C) *ManagementAPI { |
237 | subscriptionId := "subscriptionId" |
238 | - api, err := NewManagementAPI(subscriptionId, "") |
239 | + api, err := NewManagementAPI(subscriptionId, "", "West US") |
240 | c.Assert(err, IsNil) |
241 | // Polling is disabled by default. |
242 | api.PollerInterval = 0 |
243 | @@ -233,23 +233,23 @@ |
244 | err := ioutil.WriteFile(certFile, []byte(testCert), 0600) |
245 | c.Assert(err, IsNil) |
246 | |
247 | - api, err := NewManagementAPI(subscriptionId, certFile) |
248 | + api, err := NewManagementAPI(subscriptionId, certFile, "West US") |
249 | |
250 | c.Assert(err, IsNil) |
251 | - session, err := newX509Session(subscriptionId, certFile) |
252 | + session, err := newX509Session(subscriptionId, certFile, "West US") |
253 | c.Assert(api.session.subscriptionId, DeepEquals, session.subscriptionId) |
254 | c.Assert(api.session.certFile, DeepEquals, session.certFile) |
255 | } |
256 | |
257 | func (suite *managementBaseAPISuite) TestNewManagementAPISetsDefaultPollerInterval(c *C) { |
258 | - api, err := NewManagementAPI("subscriptionId", "") |
259 | + api, err := NewManagementAPI("subscriptionId", "", "West US") |
260 | c.Assert(err, IsNil) |
261 | |
262 | c.Assert(api.PollerInterval, Equals, DefaultPollerInterval) |
263 | } |
264 | |
265 | func (suite *managementBaseAPISuite) TestNewManagementAPISetsDefaultPollerTimeout(c *C) { |
266 | - api, err := NewManagementAPI("subscriptionId", "") |
267 | + api, err := NewManagementAPI("subscriptionId", "", "West US") |
268 | c.Assert(err, IsNil) |
269 | |
270 | c.Assert(api.PollerTimeout, Equals, DefaultPollerTimeout) |
271 | @@ -394,7 +394,7 @@ |
272 | descriptors, err := api.ListHostedServices() |
273 | |
274 | c.Assert(err, IsNil) |
275 | - expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices" |
276 | + expectedURL := defaultManagement + api.session.subscriptionId + "/services/hostedservices" |
277 | checkOneRequest(c, &recordedRequests, expectedURL, "2012-03-01", nil, "GET") |
278 | c.Assert(descriptors[0].URL, Equals, url) |
279 | } |
280 | @@ -424,7 +424,7 @@ |
281 | err := api.UpdateHostedService(serviceName, update) |
282 | |
283 | c.Assert(err, IsNil) |
284 | - expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices/" + serviceName |
285 | + expectedURL := defaultManagement + api.session.subscriptionId + "/services/hostedservices/" + serviceName |
286 | checkOneRequest(c, &recordedRequests, expectedURL, "2012-03-01", requestPayload, "PUT") |
287 | } |
288 | |
289 | @@ -435,7 +435,7 @@ |
290 | } else { |
291 | query = "embed-detail=false" |
292 | } |
293 | - expectedURL := fmt.Sprintf("%s%s/services/hostedservices/%s?%s", AZURE_URL, |
294 | + expectedURL := fmt.Sprintf("%s%s/services/hostedservices/%s?%s", defaultManagement, |
295 | api.session.subscriptionId, serviceName, query) |
296 | checkRequest(c, httpRequest, expectedURL, "2012-03-01", nil, "GET") |
297 | } |
298 | @@ -543,7 +543,7 @@ |
299 | createHostedService := NewCreateHostedServiceWithLocation("testName", "testLabel", "East US") |
300 | err := api.AddHostedService(createHostedService) |
301 | c.Assert(err, IsNil) |
302 | - expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices" |
303 | + expectedURL := defaultManagement + api.session.subscriptionId + "/services/hostedservices" |
304 | expectedPayload, err := marshalXML(createHostedService) |
305 | c.Assert(err, IsNil) |
306 | checkOneRequest(c, recordedRequests, expectedURL, "2012-03-01", expectedPayload, "POST") |
307 | @@ -573,7 +573,7 @@ |
308 | err := api.CheckHostedServiceNameAvailability(serviceName) |
309 | |
310 | c.Assert(err, IsNil) |
311 | - expectedURL := (AZURE_URL + api.session.subscriptionId + |
312 | + expectedURL := (defaultManagement + api.session.subscriptionId + |
313 | "/services/hostedservices/operations/isavailable/" + serviceName) |
314 | checkOneRequest(c, &recordedRequests, expectedURL, "2012-03-01", nil, "GET") |
315 | } |
316 | @@ -595,7 +595,7 @@ |
317 | |
318 | c.Assert(err, ErrorMatches, reason) |
319 | c.Check(recordedRequests, HasLen, 1) |
320 | - expectedURL := (AZURE_URL + api.session.subscriptionId + |
321 | + expectedURL := (defaultManagement + api.session.subscriptionId + |
322 | "/services/hostedservices/operations/isavailable/" + serviceName) |
323 | checkOneRequest(c, &recordedRequests, expectedURL, "2012-03-01", nil, "GET") |
324 | } |
325 | @@ -637,7 +637,7 @@ |
326 | } |
327 | |
328 | func assertDeleteHostedServiceRequest(c *C, api *ManagementAPI, serviceName string, httpRequest *X509Request) { |
329 | - expectedURL := fmt.Sprintf("%s%s/services/hostedservices/%s", AZURE_URL, |
330 | + expectedURL := fmt.Sprintf("%s%s/services/hostedservices/%s", defaultManagement, |
331 | api.session.subscriptionId, serviceName) |
332 | checkRequest(c, httpRequest, expectedURL, "2010-10-28", nil, "DELETE") |
333 | } |
334 | @@ -669,7 +669,7 @@ |
335 | err := api.AddDeployment(deployment, serviceName) |
336 | |
337 | c.Assert(err, IsNil) |
338 | - expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices/" + serviceName + "/deployments" |
339 | + expectedURL := defaultManagement + api.session.subscriptionId + "/services/hostedservices/" + serviceName + "/deployments" |
340 | expectedPayload, err := marshalXML(deployment) |
341 | c.Assert(err, IsNil) |
342 | checkOneRequest(c, recordedRequests, expectedURL, "2012-03-01", expectedPayload, "POST") |
343 | @@ -677,7 +677,7 @@ |
344 | |
345 | func assertDeleteDeploymentRequest(c *C, api *ManagementAPI, hostedServiceName, deploymentName string, httpRequest *X509Request) { |
346 | expectedURL := fmt.Sprintf( |
347 | - "%s%s/services/hostedservices/%s/deployments/%s", AZURE_URL, |
348 | + "%s%s/services/hostedservices/%s/deployments/%s", defaultManagement, |
349 | api.session.subscriptionId, hostedServiceName, deploymentName) |
350 | checkRequest(c, httpRequest, expectedURL, "2012-03-01", nil, "DELETE") |
351 | } |
352 | @@ -776,7 +776,7 @@ |
353 | |
354 | func assertGetDeploymentRequest(c *C, api *ManagementAPI, request *GetDeploymentRequest, httpRequest *X509Request) { |
355 | expectedURL := fmt.Sprintf( |
356 | - "%s%s/services/hostedservices/%s/deployments/%s", AZURE_URL, |
357 | + "%s%s/services/hostedservices/%s/deployments/%s", defaultManagement, |
358 | api.session.subscriptionId, request.ServiceName, request.DeploymentName) |
359 | checkRequest(c, httpRequest, expectedURL, "2012-03-01", nil, "GET") |
360 | } |
361 | @@ -818,7 +818,7 @@ |
362 | err := api.AddStorageAccount(cssi) |
363 | c.Assert(err, IsNil) |
364 | |
365 | - expectedURL := AZURE_URL + api.session.subscriptionId + "/services/storageservices" |
366 | + expectedURL := defaultManagement + api.session.subscriptionId + "/services/storageservices" |
367 | expectedPayload, err := marshalXML(cssi) |
368 | c.Assert(err, IsNil) |
369 | checkOneRequest(c, &recordedRequests, expectedURL, "2012-03-01", expectedPayload, "POST") |
370 | @@ -873,7 +873,7 @@ |
371 | } |
372 | |
373 | func assertDeleteDiskRequest(c *C, api *ManagementAPI, diskName string, httpRequest *X509Request) { |
374 | - expectedURL := fmt.Sprintf("%s%s/services/disks/%s", AZURE_URL, |
375 | + expectedURL := fmt.Sprintf("%s%s/services/disks/%s", defaultManagement, |
376 | api.session.subscriptionId, diskName) |
377 | checkRequest(c, httpRequest, expectedURL, "2012-08-01", nil, "DELETE") |
378 | } |
379 | @@ -930,7 +930,7 @@ |
380 | err := api.performRoleOperation(serviceName, deploymentName, roleName, version, operation) |
381 | |
382 | c.Assert(err, IsNil) |
383 | - expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices/" + serviceName + "/deployments/" + deploymentName + "/roleinstances/" + roleName + "/Operations" |
384 | + expectedURL := defaultManagement + api.session.subscriptionId + "/services/hostedservices/" + serviceName + "/deployments/" + deploymentName + "/roleinstances/" + roleName + "/Operations" |
385 | expectedPayload, err := marshalXML(operation) |
386 | c.Assert(err, IsNil) |
387 | checkOneRequest(c, recordedRequests, expectedURL, version, expectedPayload, "POST") |
388 | @@ -942,7 +942,7 @@ |
389 | request := &StartRoleRequest{"serviceName", "deploymentName", "roleName"} |
390 | err := api.StartRole(request) |
391 | c.Assert(err, IsNil) |
392 | - expectedURL := (AZURE_URL + api.session.subscriptionId + "/services/hostedservices/" + |
393 | + expectedURL := (defaultManagement + api.session.subscriptionId + "/services/hostedservices/" + |
394 | request.ServiceName + "/deployments/" + request.DeploymentName + "/roleinstances/" + |
395 | request.RoleName + "/Operations") |
396 | expectedPayload, err := marshalXML(startRoleOperation) |
397 | @@ -956,7 +956,7 @@ |
398 | request := &RestartRoleRequest{"serviceName", "deploymentName", "roleName"} |
399 | err := api.RestartRole(request) |
400 | c.Assert(err, IsNil) |
401 | - expectedURL := (AZURE_URL + api.session.subscriptionId + "/services/hostedservices/" + |
402 | + expectedURL := (defaultManagement + api.session.subscriptionId + "/services/hostedservices/" + |
403 | request.ServiceName + "/deployments/" + request.DeploymentName + "/roleinstances/" + |
404 | request.RoleName + "/Operations") |
405 | expectedPayload, err := marshalXML(restartRoleOperation) |
406 | @@ -967,7 +967,7 @@ |
407 | func assertShutdownRoleRequest(c *C, api *ManagementAPI, request *ShutdownRoleRequest, httpRequest *X509Request) { |
408 | expectedURL := fmt.Sprintf( |
409 | "%s%s/services/hostedservices/%s/deployments/%s/roleinstances/%s/Operations", |
410 | - AZURE_URL, api.session.subscriptionId, request.ServiceName, |
411 | + defaultManagement, api.session.subscriptionId, request.ServiceName, |
412 | request.DeploymentName, request.RoleName) |
413 | expectedPayload, err := marshalXML(shutdownRoleOperation) |
414 | c.Assert(err, IsNil) |
415 | @@ -985,7 +985,7 @@ |
416 | } |
417 | |
418 | func assertGetRoleRequest(c *C, api *ManagementAPI, httpRequest *X509Request, serviceName, deploymentName, roleName string) { |
419 | - expectedURL := (AZURE_URL + api.session.subscriptionId + |
420 | + expectedURL := (defaultManagement + api.session.subscriptionId + |
421 | "/services/hostedservices/" + |
422 | serviceName + "/deployments/" + deploymentName + "/roles/" + roleName) |
423 | checkRequest(c, httpRequest, expectedURL, "2012-03-01", nil, "GET") |
424 | @@ -1013,7 +1013,7 @@ |
425 | } |
426 | |
427 | func assertUpdateRoleRequest(c *C, api *ManagementAPI, httpRequest *X509Request, serviceName, deploymentName, roleName, expectedXML string) { |
428 | - expectedURL := (AZURE_URL + api.session.subscriptionId + |
429 | + expectedURL := (defaultManagement + api.session.subscriptionId + |
430 | "/services/hostedservices/" + |
431 | serviceName + "/deployments/" + deploymentName + "/roles/" + roleName) |
432 | checkRequest( |
433 | @@ -1100,7 +1100,7 @@ |
434 | err := api.CreateAffinityGroup(&request) |
435 | c.Assert(err, IsNil) |
436 | |
437 | - expectedURL := AZURE_URL + api.session.subscriptionId + "/affinitygroups" |
438 | + expectedURL := defaultManagement + api.session.subscriptionId + "/affinitygroups" |
439 | expectedBody, _ := cag.Serialize() |
440 | checkOneRequest(c, &recordedRequests, expectedURL, "2012-03-01", []byte(expectedBody), "POST") |
441 | } |
442 | @@ -1120,7 +1120,7 @@ |
443 | err := api.UpdateAffinityGroup(&request) |
444 | c.Assert(err, IsNil) |
445 | |
446 | - expectedURL := (AZURE_URL + api.session.subscriptionId + |
447 | + expectedURL := (defaultManagement + api.session.subscriptionId + |
448 | "/affinitygroups/" + request.Name) |
449 | expectedBody, _ := uag.Serialize() |
450 | checkOneRequest(c, &recordedRequests, expectedURL, "2011-02-25", []byte(expectedBody), "PUT") |
451 | @@ -1139,7 +1139,7 @@ |
452 | err := api.DeleteAffinityGroup(&request) |
453 | c.Assert(err, IsNil) |
454 | |
455 | - expectedURL := (AZURE_URL + api.session.subscriptionId + |
456 | + expectedURL := (defaultManagement + api.session.subscriptionId + |
457 | "/affinitygroups/" + request.Name) |
458 | checkOneRequest(c, &recordedRequests, expectedURL, "2011-02-25", nil, "DELETE") |
459 | } |
460 | @@ -1206,7 +1206,7 @@ |
461 | |
462 | func assertGetNetworkConfigurationRequest(c *C, api *ManagementAPI, httpRequest *X509Request) { |
463 | expectedURL := fmt.Sprintf( |
464 | - "%s%s/services/networking/media", AZURE_URL, |
465 | + "%s%s/services/networking/media", defaultManagement, |
466 | api.session.subscriptionId) |
467 | checkRequest(c, httpRequest, expectedURL, "2012-03-01", nil, "GET") |
468 | } |
469 | @@ -1245,7 +1245,7 @@ |
470 | |
471 | func assertSetNetworkConfigurationRequest(c *C, api *ManagementAPI, body []byte, httpRequest *X509Request) { |
472 | expectedURL := fmt.Sprintf( |
473 | - "%s%s/services/networking/media", AZURE_URL, |
474 | + "%s%s/services/networking/media", defaultManagement, |
475 | api.session.subscriptionId) |
476 | checkRequest(c, httpRequest, expectedURL, "2012-03-01", body, "PUT") |
477 | // Azure chokes when the content type is text/xml or similar. |
478 | |
479 | === modified file 'management_test.go' |
480 | --- management_test.go 2013-08-05 12:50:48 +0000 |
481 | +++ management_test.go 2013-08-06 08:00:44 +0000 |
482 | @@ -87,7 +87,7 @@ |
483 | c.Assert(record, Not(HasLen), 0) |
484 | expectedURL := fmt.Sprintf( |
485 | "%ssubscriptionId/services/hostedservices/%s?embed-detail=true", |
486 | - AZURE_URL, service.ServiceName) |
487 | + defaultManagement, service.ServiceName) |
488 | c.Check(record[0].URL, Equals, expectedURL) |
489 | c.Check(record[0].Method, Equals, "GET") |
490 | } |
491 | |
492 | === modified file 'poller_test.go' |
493 | --- poller_test.go 2013-07-19 16:11:18 +0000 |
494 | +++ poller_test.go 2013-08-06 08:00:44 +0000 |
495 | @@ -19,7 +19,7 @@ |
496 | func (suite *pollerSuite) makeAPI(c *C) *ManagementAPI { |
497 | subscriptionId := "subscriptionId" |
498 | subscriptionId = subscriptionId |
499 | - api, err := NewManagementAPI(subscriptionId, "") |
500 | + api, err := NewManagementAPI(subscriptionId, "", "West US") |
501 | c.Assert(err, IsNil) |
502 | return api |
503 | } |
504 | @@ -119,7 +119,7 @@ |
505 | _, err := poller.poll() |
506 | |
507 | c.Assert(err, IsNil) |
508 | - expectedURL := AZURE_URL + api.session.subscriptionId + "/operations/" + operationID |
509 | + expectedURL := defaultManagement + api.session.subscriptionId + "/operations/" + operationID |
510 | checkOneRequest(c, recordedRequests, expectedURL, "2009-10-01", nil, "GET") |
511 | } |
512 | |
513 | @@ -210,7 +210,7 @@ |
514 | c.Assert(err, IsNil) |
515 | c.Assert(response, DeepEquals, secondResponse.response) |
516 | operationPollerInstance := poller.(operationPoller) |
517 | - expectedURL := AZURE_URL + operationPollerInstance.api.session.subscriptionId + "/operations/" + operationID |
518 | + expectedURL := defaultManagement + operationPollerInstance.api.session.subscriptionId + "/operations/" + operationID |
519 | c.Assert(len(recordedRequests), Equals, 2) |
520 | checkRequest(c, recordedRequests[0], expectedURL, "2009-10-01", nil, "GET") |
521 | checkRequest(c, recordedRequests[1], expectedURL, "2009-10-01", nil, "GET") |
522 | |
523 | === modified file 'storage_base.go' |
524 | --- storage_base.go 2013-08-05 10:53:20 +0000 |
525 | +++ storage_base.go 2013-08-06 08:00:44 +0000 |
526 | @@ -202,9 +202,22 @@ |
527 | // request to the storage services API. It also has an HTTP Client to allow |
528 | // overriding for custom behaviour, during testing for example. |
529 | type StorageContext struct { |
530 | + // Account is a storage account name. |
531 | Account string |
532 | - // Access key: access will be anonymous if the key is the empty string. |
533 | - Key string |
534 | + |
535 | + // Key authenticates the storage account. Access will be anonymous if this |
536 | + // is left empty. |
537 | + Key string |
538 | + |
539 | + // AzureEndpoint specifies a base service endpoint URL for the Azure APIs. |
540 | + // If this is not set, it will default to the international endpoint which |
541 | + // will not work in mainland China. |
542 | + // |
543 | + // Try to set this if at all possible. Use GetEndpoint() to obtain the |
544 | + // endpoint associated with a given service location, e.g. "West US" or |
545 | + // "North Europe" or "East China". |
546 | + AzureEndpoint APIEndpoint |
547 | + |
548 | client *http.Client |
549 | } |
550 | |
551 | @@ -321,12 +334,13 @@ |
552 | // getAccountURL returns the base URL for the context's storage account. |
553 | // (The result ends in a slash.) |
554 | func (context *StorageContext) getAccountURL() string { |
555 | - escapedAccount := url.QueryEscape(context.Account) |
556 | - // Use https. This does not suffer from the "no renegotiation" bug in |
557 | - // Go's implementation. It's optional, but it gets around spurious |
558 | - // authentication failures when working through a proxy that messes with |
559 | - // our requests instead of passing them on verbatim. |
560 | - return fmt.Sprintf("https://%s.blob.core.windows.net/", escapedAccount) |
561 | + endpoint := context.AzureEndpoint |
562 | + if endpoint == "" { |
563 | + // No API endpoint specified. Default to the international one. |
564 | + // This will not work for mainland China. |
565 | + endpoint = GetEndpoint("West US") |
566 | + } |
567 | + return prefixHost(context.Account, endpoint.BlobStorageAPI()) |
568 | } |
569 | |
570 | // getContainerURL returns the URL for a given storage container. |
571 | |
572 | === modified file 'storage_base_test.go' |
573 | --- storage_base_test.go 2013-08-05 10:53:20 +0000 |
574 | +++ storage_base_test.go 2013-08-06 08:00:44 +0000 |
575 | @@ -286,13 +286,35 @@ |
576 | return MakeRandomString(3) + "?&" + MakeRandomString(3) + "$%" |
577 | } |
578 | |
579 | -func (suite *TestStorageContext) TestGetAccountURL(c *C) { |
580 | +func (suite *TestStorageContext) TestGetAccountURLCombinesAccountAndEndpoint(c *C) { |
581 | + context := StorageContext{ |
582 | + Account: "myaccount", |
583 | + AzureEndpoint: "http://example.com", |
584 | + } |
585 | + c.Check( |
586 | + context.getAccountURL(), |
587 | + Equals, |
588 | + "http://myaccount.blob.example.com") |
589 | +} |
590 | + |
591 | +func (suite *TestStorageContext) TestGetAccountURLEscapesHostname(c *C) { |
592 | account := makeNastyURLUnfriendlyString() |
593 | - context := StorageContext{Account: account} |
594 | - c.Check( |
595 | - context.getAccountURL(), |
596 | - Equals, |
597 | - "https://"+url.QueryEscape(account)+".blob.core.windows.net/") |
598 | + context := StorageContext{ |
599 | + Account: account, |
600 | + AzureEndpoint: "http://example.com", |
601 | + } |
602 | + c.Check( |
603 | + context.getAccountURL(), |
604 | + Equals, |
605 | + "http://"+url.QueryEscape(account)+".blob.example.com") |
606 | +} |
607 | + |
608 | +func (*TestStorageContext) TestGetAccountURLDefaultsToInternationalEndpoint(c *C) { |
609 | + context := StorageContext{Account: "myaccount"} |
610 | + c.Check( |
611 | + context.getAccountURL(), |
612 | + Equals, |
613 | + "https://myaccount.blob.core.windows.net/") |
614 | } |
615 | |
616 | func (suite *TestStorageContext) TestGetContainerURL(c *C) { |
617 | |
618 | === modified file 'x509dispatcher_test.go' |
619 | --- x509dispatcher_test.go 2013-07-30 05:24:42 +0000 |
620 | +++ x509dispatcher_test.go 2013-08-06 08:00:44 +0000 |
621 | @@ -49,7 +49,7 @@ |
622 | server := makeRecordingHTTPServer(httpRequests, http.StatusOK, nil, nil) |
623 | defer server.Close() |
624 | // No real certificate needed since we're testing on http, not https. |
625 | - session, err := newX509Session("subscriptionid", "") |
626 | + session, err := newX509Session("subscriptionid", "", "West US") |
627 | c.Assert(err, IsNil) |
628 | path := "/foo/bar" |
629 | version := "test-version" |
630 | @@ -74,7 +74,7 @@ |
631 | server := makeRecordingHTTPServer(httpRequests, http.StatusOK, responseBody, nil) |
632 | defer server.Close() |
633 | // No real certificate needed since we're testing on http, not https. |
634 | - session, err := newX509Session("subscriptionid", "") |
635 | + session, err := newX509Session("subscriptionid", "", "West US") |
636 | c.Assert(err, IsNil) |
637 | path := "/foo/bar" |
638 | version := "test-version" |
639 | @@ -98,7 +98,7 @@ |
640 | server := makeRecordingHTTPServer(httpRequests, http.StatusOK, nil, nil) |
641 | defer server.Close() |
642 | // No real certificate needed since we're testing on http, not https. |
643 | - session, err := newX509Session("subscriptionid", "") |
644 | + session, err := newX509Session("subscriptionid", "", "West US") |
645 | c.Assert(err, IsNil) |
646 | path := "/foo/bar" |
647 | version := "test-version" |
648 | @@ -122,7 +122,7 @@ |
649 | server := makeRecordingHTTPServer(httpRequests, http.StatusOK, responseBody, nil) |
650 | defer server.Close() |
651 | // No real certificate needed since we're testing on http, not https. |
652 | - session, err := newX509Session("subscriptionid", "") |
653 | + session, err := newX509Session("subscriptionid", "", "West US") |
654 | c.Assert(err, IsNil) |
655 | path := "/foo/bar" |
656 | version := "test-version" |
657 | @@ -151,7 +151,7 @@ |
658 | serveMux.HandleFunc("/", returnRequest) |
659 | server := httptest.NewServer(serveMux) |
660 | defer server.Close() |
661 | - session, err := newX509Session("subscriptionid", "") |
662 | + session, err := newX509Session("subscriptionid", "", "West US") |
663 | c.Assert(err, IsNil) |
664 | path := "/foo/bar" |
665 | request := newX509RequestGET(server.URL+path, "testversion") |
666 | |
667 | === modified file 'x509session.go' |
668 | --- x509session.go 2013-07-22 12:53:27 +0000 |
669 | +++ x509session.go 2013-08-06 08:00:44 +0000 |
670 | @@ -15,13 +15,14 @@ |
671 | subscriptionId string |
672 | certFile string |
673 | client *http.Client |
674 | + baseURL *url.URL |
675 | } |
676 | |
677 | // newX509Session creates and returns a new x509Session based on credentials |
678 | // and X509 certificate files. |
679 | // For testing purposes, certFile can be passed as the empty string and it |
680 | // will be ignored. |
681 | -func newX509Session(subscriptionId string, certFile string) (*x509Session, error) { |
682 | +func newX509Session(subscriptionId, certFile, location string) (*x509Session, error) { |
683 | certs := []tls.Certificate{} |
684 | if certFile != "" { |
685 | // |
686 | @@ -39,16 +40,21 @@ |
687 | }, |
688 | } |
689 | |
690 | + endpointURL := GetEndpoint(location).ManagementAPI() |
691 | + baseURL, err := url.Parse(endpointURL) |
692 | + if err != nil { |
693 | + panic(fmt.Errorf("cannot parse Azure endpoint URL '%s' - %v", endpointURL, err)) |
694 | + } |
695 | + |
696 | session := x509Session{ |
697 | subscriptionId: subscriptionId, |
698 | certFile: certFile, |
699 | client: &client, |
700 | + baseURL: baseURL, |
701 | } |
702 | return &session, nil |
703 | } |
704 | |
705 | -var AZURE_URL = "https://management.core.windows.net/" |
706 | - |
707 | // composeURL puts together a URL for an item on the Azure API based on |
708 | // the starting point used by the session, and a given relative path from |
709 | // there. |
710 | @@ -56,16 +62,12 @@ |
711 | if strings.HasPrefix(path, "/") { |
712 | panic(fmt.Errorf("got absolute API path '%s' instead of relative one", path)) |
713 | } |
714 | - azureURL, err := url.Parse(AZURE_URL) |
715 | - if err != nil { |
716 | - panic(err) |
717 | - } |
718 | escapedID := url.QueryEscape(session.subscriptionId) |
719 | pathURL, err := url.Parse(escapedID + "/" + path) |
720 | if err != nil { |
721 | panic(err) |
722 | } |
723 | - return azureURL.ResolveReference(pathURL).String() |
724 | + return session.baseURL.ResolveReference(pathURL).String() |
725 | } |
726 | |
727 | // _X509Dispatcher is the function used to dispatch requests. We call the |
728 | |
729 | === modified file 'x509session_test.go' |
730 | --- x509session_test.go 2013-07-19 16:11:18 +0000 |
731 | +++ x509session_test.go 2013-08-06 08:00:44 +0000 |
732 | @@ -17,6 +17,10 @@ |
733 | "time" |
734 | ) |
735 | |
736 | +// defaultManagement is the international management API for Azure. |
737 | +// (Mainland China gets a different URL). |
738 | +const defaultManagement = "https://management.core.windows.net/" |
739 | + |
740 | // x509DispatcherFixture records the current x509 dispatcher before a test, |
741 | // and restores it after. This gives your test the freedom to replace the |
742 | // dispatcher with test doubles, using any of the rig*Dispatcher functions. |
743 | @@ -104,20 +108,22 @@ |
744 | return certFile, keyFile |
745 | } |
746 | |
747 | -func (suite *x509SessionSuite) TestNewX509SessionCreation(c *C) { |
748 | - _, err := newX509Session("subscriptionid", "") |
749 | +func (suite *x509SessionSuite) TestNewX509Session(c *C) { |
750 | + session, err := newX509Session("subscriptionid", "", "China East") |
751 | c.Assert(err, IsNil) |
752 | + c.Assert(session.baseURL, NotNil) |
753 | + c.Check(session.baseURL.String(), Equals, GetEndpoint("China East").ManagementAPI()) |
754 | } |
755 | |
756 | func (suite *x509SessionSuite) TestComposeURLComposesURLWithRelativePath(c *C) { |
757 | const subscriptionID = "subscriptionid" |
758 | const path = "foo/bar" |
759 | - session, err := newX509Session(subscriptionID, "") |
760 | + session, err := newX509Session(subscriptionID, "", "West US") |
761 | c.Assert(err, IsNil) |
762 | |
763 | url := session.composeURL(path) |
764 | |
765 | - c.Check(url, Matches, AZURE_URL+subscriptionID+"/"+path) |
766 | + c.Check(url, Matches, defaultManagement+subscriptionID+"/"+path) |
767 | } |
768 | |
769 | func (suite *x509SessionSuite) TestComposeURLRejectsAbsolutePath(c *C) { |
770 | @@ -126,7 +132,7 @@ |
771 | c.Assert(err, NotNil) |
772 | c.Check(err, ErrorMatches, ".*absolute.*path.*") |
773 | }() |
774 | - session, err := newX509Session("subscriptionid", "") |
775 | + session, err := newX509Session("subscriptionid", "", "West US") |
776 | c.Assert(err, IsNil) |
777 | |
778 | // This panics because we're passing an absolute path. |
779 | @@ -136,7 +142,7 @@ |
780 | func (suite *x509SessionSuite) TestGetServerErrorProducesServerError(c *C) { |
781 | msg := "huhwhat" |
782 | status := http.StatusNotFound |
783 | - session, err := newX509Session("subscriptionid", "") |
784 | + session, err := newX509Session("subscriptionid", "", "West US") |
785 | c.Assert(err, IsNil) |
786 | |
787 | err = session.getServerError(status, []byte{}, msg) |
788 | @@ -152,7 +158,7 @@ |
789 | http.StatusOK, |
790 | http.StatusNoContent, |
791 | } |
792 | - session, err := newX509Session("subscriptionid", "") |
793 | + session, err := newX509Session("subscriptionid", "", "West US") |
794 | c.Assert(err, IsNil) |
795 | |
796 | for _, status := range goodCodes { |
797 | @@ -170,7 +176,7 @@ |
798 | http.StatusInternalServerError, |
799 | http.StatusNotImplemented, |
800 | } |
801 | - session, err := newX509Session("subscriptionid", "") |
802 | + session, err := newX509Session("subscriptionid", "", "West US") |
803 | c.Assert(err, IsNil) |
804 | |
805 | for _, status := range badCodes { |
806 | @@ -181,7 +187,7 @@ |
807 | func (suite *x509SessionSuite) TestGetIssuesRequest(c *C) { |
808 | subscriptionID := "subscriptionID" |
809 | uri := "resource" |
810 | - session, err := newX509Session(subscriptionID, "") |
811 | + session, err := newX509Session(subscriptionID, "", "West US") |
812 | c.Assert(err, IsNil) |
813 | // Record incoming requests, and have them return a given reply. |
814 | fixedResponse := x509Response{ |
815 | @@ -198,14 +204,14 @@ |
816 | |
817 | c.Assert(len(recordedRequests), Equals, 1) |
818 | request := recordedRequests[0] |
819 | - c.Check(request.URL, Equals, AZURE_URL+subscriptionID+"/"+uri) |
820 | + c.Check(request.URL, Equals, defaultManagement+subscriptionID+"/"+uri) |
821 | c.Check(request.Method, Equals, "GET") |
822 | c.Check(request.APIVersion, Equals, version) |
823 | c.Check(*receivedResponse, DeepEquals, fixedResponse) |
824 | } |
825 | |
826 | func (suite *x509SessionSuite) TestGetReportsClientSideError(c *C) { |
827 | - session, err := newX509Session("subscriptionid", "") |
828 | + session, err := newX509Session("subscriptionid", "", "West US") |
829 | msg := "could not dispatch request" |
830 | rigFailingDispatcher(fmt.Errorf(msg)) |
831 | |
832 | @@ -217,7 +223,7 @@ |
833 | } |
834 | |
835 | func (suite *x509SessionSuite) TestGetReportsServerSideError(c *C) { |
836 | - session, err := newX509Session("subscriptionid", "") |
837 | + session, err := newX509Session("subscriptionid", "", "West US") |
838 | fixedResponse := x509Response{ |
839 | StatusCode: http.StatusForbidden, |
840 | Body: []byte("Body"), |
841 | @@ -238,7 +244,7 @@ |
842 | version := "test-version" |
843 | requestBody := []byte("Request body") |
844 | requestContentType := "bogusContentType" |
845 | - session, err := newX509Session(subscriptionID, "") |
846 | + session, err := newX509Session(subscriptionID, "", "West US") |
847 | c.Assert(err, IsNil) |
848 | // Record incoming requests, and have them return a given reply. |
849 | fixedResponse := x509Response{ |
850 | @@ -254,7 +260,7 @@ |
851 | |
852 | c.Assert(len(recordedRequests), Equals, 1) |
853 | request := recordedRequests[0] |
854 | - c.Check(request.URL, Equals, AZURE_URL+subscriptionID+"/"+uri) |
855 | + c.Check(request.URL, Equals, defaultManagement+subscriptionID+"/"+uri) |
856 | c.Check(request.Method, Equals, "POST") |
857 | c.Check(request.APIVersion, Equals, version) |
858 | c.Check(request.ContentType, Equals, requestContentType) |
859 | @@ -263,7 +269,7 @@ |
860 | } |
861 | |
862 | func (suite *x509SessionSuite) TestPostReportsClientSideError(c *C) { |
863 | - session, err := newX509Session("subscriptionid", "") |
864 | + session, err := newX509Session("subscriptionid", "", "West US") |
865 | msg := "could not dispatch request" |
866 | rigFailingDispatcher(fmt.Errorf(msg)) |
867 | |
868 | @@ -275,7 +281,7 @@ |
869 | } |
870 | |
871 | func (suite *x509SessionSuite) TestPostReportsServerSideError(c *C) { |
872 | - session, err := newX509Session("subscriptionid", "") |
873 | + session, err := newX509Session("subscriptionid", "", "West US") |
874 | fixedResponse := x509Response{ |
875 | StatusCode: http.StatusForbidden, |
876 | Body: []byte("Body"), |
877 | @@ -294,7 +300,7 @@ |
878 | subscriptionID := "subscriptionID" |
879 | uri := "resource" |
880 | version := "test-version" |
881 | - session, err := newX509Session(subscriptionID, "") |
882 | + session, err := newX509Session(subscriptionID, "", "West US") |
883 | c.Assert(err, IsNil) |
884 | // Record incoming requests, and have them return a given reply. |
885 | fixedResponse := x509Response{StatusCode: http.StatusOK} |
886 | @@ -308,7 +314,7 @@ |
887 | c.Check(*response, DeepEquals, fixedResponse) |
888 | c.Assert(len(recordedRequests), Equals, 1) |
889 | request := recordedRequests[0] |
890 | - c.Check(request.URL, Equals, AZURE_URL+subscriptionID+"/"+uri) |
891 | + c.Check(request.URL, Equals, defaultManagement+subscriptionID+"/"+uri) |
892 | c.Check(request.Method, Equals, "DELETE") |
893 | c.Check(request.APIVersion, Equals, version) |
894 | } |
895 | @@ -318,7 +324,7 @@ |
896 | uri := "resource" |
897 | version := "test-version" |
898 | requestBody := []byte("Request body") |
899 | - session, err := newX509Session(subscriptionID, "") |
900 | + session, err := newX509Session(subscriptionID, "", "West US") |
901 | c.Assert(err, IsNil) |
902 | // Record incoming requests, and have them return a given reply. |
903 | fixedResponse := x509Response{ |
904 | @@ -333,7 +339,7 @@ |
905 | |
906 | c.Assert(len(recordedRequests), Equals, 1) |
907 | request := recordedRequests[0] |
908 | - c.Check(request.URL, Equals, AZURE_URL+subscriptionID+"/"+uri) |
909 | + c.Check(request.URL, Equals, defaultManagement+subscriptionID+"/"+uri) |
910 | c.Check(request.Method, Equals, "PUT") |
911 | c.Check(request.APIVersion, Equals, version) |
912 | c.Check(request.Payload, DeepEquals, requestBody) |
Looks pretty good! I have some suggestions to make it better of course, but it
can land when you've made the changes.
27 + if strings. Contains( location, "China") {
It's "China East" and "China North" AFAICS, it might be worth being specific
instead of using Contains in case a new one pops up that's not actually in the
same domain. Although sod's law dictates whatever we do it'll be wrong ... but
I think explicit is always better as you end up with fewer surprises.
60 +// StorageAPI returns the base URL for the endpoint's storage API.
61 +func (endpoint APIEndpoint) StorageAPI() string {
Why not a pointer receiver? (same for ManagementAPI)
62 + return prefixHost("blob", string(endpoint))
63 +}
Given there's three types of storage, this should to be named BlobStorageAPI().
I'm not even sure the "API" part is accurate, but I'm not going to bikeshed over it.
Also, the storage URL returned here is still not complete, it would need the account prefixed.
It might be worth encapsulating that functionality in this object so that all the URL
manipulation is done in the same place. I'd have StorageAPI take an account name parameter.
114 +func (*endpointsSuite) TestGetEndpoint MakesGoodGuesse sForUknownRegio ns(c *C) { "Central China West"),
115 + c.Check(
116 + GetEndpoint("South San Marino Highlands"),
117 + Equals,
118 + GetEndpoint("West US"))
119 + c.Check(
120 + GetEndpoint(
121 + Equals,
122 + GetEndpoint("China East"))
123 +}
I'm not sure I like this test at all. Its expected output is a function of the method being tested
rather than something either constant or independent of it, and that's kinda nasty. It also feels
fragile.
I'd much rather you were explicit in testing expected output to be the required endpoint URLs.
535 + // AzureEndpoint specifies a base service endpoint URL for the Azure APIs.
536 + // If this is not set, it will default to the international endpoint which
537 + // will not work in mainland China.
538 + //
539 + // Try to set this if at all possible. Use GetEndpoint() to obtain the
540 + // endpoint associated with a given service location, e.g. "West US" or
541 + // "North Europe" or "East China".
542 + AzureEndpoint APIEndpoint
I don't like the "try to set this" stuff at all here. Let's just panic if it's not set.
Out of interest, I wonder if Go works like C++ here and lets you supply a value that works with
the type's constructor as well as supplying the type itself.
558 + if endpoint == "" {
559 + // No API endpoint specified. Default to the international one.
560 + // This will not work for mainland China.
561 + endpoint = GetEndpoint("West US")
Remove this and panic() if it's not set.
563 + return prefixHost( context. Account, endpoint. StorageAPI( ))
And as I mentioned before, let's encapsulate this in APIEndpoint.
732 +// defaultManagement is the international management API for Azure. /management. core.windows. net/"
733 +// (Mainland China gets a different URL).
734 +const defaultManagement = "https:/
I hate Go for breaking the convention that consts are in CAPS. Grrr.
Anyway this should be defa...