Merge lp:~rvb/gwacl/handle-307 into lp:gwacl

[Raphaël Badin]

Follow temporary redirection.

When polling after having created or removed an instance, I've noticed that from time to time, the polling gets interrupted because it gets a 307 response (temporary redirect) from the Azure server. This branch fixes this problem by using curl's CURLOPT_FOLLOWLOCATION option so that curl will follow the redirection.

[Julian Edwards (julian-edwards)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 review: approve

Thanks for this, nice change. I must admit, I'm surprised that curl
doesn't do this by default, wget does.

Some minor points below.

On 01/07/13 23:08, Raphaël Badin wrote:
> === modified file 'x509dispatcher.go' --- x509dispatcher.go
> 2013-06-27 10:55:35 +0000 +++ x509dispatcher.go 2013-07-01 13:05:38
> +0000 @@ -178,6 +178,9 @@ return response, nil }
>
> +// The maximum number of redirection-followings allowed. +var
> _CURL_MAX_REDIRECTS = 10

Why did you choose 10? Document that here in the comment.

> + // makeCurlRequest produces a curl.CURL representing the
> request. // Clean up the request by calling its Cleanup() method
> after you're done // with it. @@ -188,6 +191,8 @@
> ch.Setopt(curl.OPT_KEYPASSWD, "") ch.Setopt(curl.OPT_URL,
> request.URL) //ch.Setopt(curl.OPT_VERBOSE, true) +
> ch.Setopt(curl.OPT_FOLLOWLOCATION, true) +
> ch.Setopt(curl.OPT_MAXREDIRS, _CURL_MAX_REDIRECTS)
> ch.Setopt(curl.OPT_SSLVERSION, 3) versionHeader := "x-ms-version: "
> + request.APIVersion headers := []string{versionHeader}
>
> === modified file 'x509dispatcher_test.go' ---
> x509dispatcher_test.go 2013-03-26 10:23:51 +0000 +++
> x509dispatcher_test.go 2013-07-01 13:05:38 +0000 @@ -14,9 +14,12
> @@ // that serves at the given base URL, copies incoming requests
> into the given // channel, and finally returns the given status
> code. If body is not nil, it // will be returned as the request
> body. -func makeRecordingHTTPServer(requests chan *http.Request,
> status int, body []byte) *httptest.Server { +func
> makeRecordingHTTPServer(requests chan *http.Request, status int,
> body []byte, headers map[string]string

Consider using http.Header instead of map[string]string here (like
performRequest() in storage_base.go does), it is the same thing
underneath but conveys intent better.

) *httptest.Server {
> returnRequest := func(w http.ResponseWriter, r *http.Request) {
> requests <- r + for key, value := range headers { +
> w.Header().Set(key, value) + } w.WriteHeader(status) if body
> != nil { w.Write(body) @@ -55,7 +58,7 @@
>
> func (*x509DispatcherSuite) TestGetRequestDoesHTTPGET(c *C) {
> httpRequests := make(chan *http.Request, 1) - server :=
> makeRecordingHTTPServer(httpRequests, http.StatusOK, nil) +
> server := makeRecordingHTTPServer(httpRequests, http.StatusOK, nil,
> nil) defer server.Close() // No real certificate needed since we're
> testing on http, not https. session, err :=
> newX509Session("subscriptionid", "cert.pem") @@ -72,12 +75,49 @@
> c.Check(httpRequest.URL.String(), Equals, path) }
>
> +func (*x509DispatcherSuite) TestGetFollowsRedirects(c *C) { +
> redirectPath := "/redirect/path" + httpRequests := make(chan
> *http.Request, _CURL_MAX_REDIRECTS+1) + locationHeaders :=
> map[string]string{"Location": redirectPath} + server :=
> makeRecordingHTTPServer(httpRequests, http.StatusTemporaryRedirect,
> nil, locationHeaders) + defer server.Close() + // No real
> certificate needed since we're testing on http, not https. +
> session, err := newX509Session("subscriptionid", "cert.pem") +
> c.Assert(err, IsNil) + path := "/foo/bar...

review: Approve

[Raphaël Badin (rvb)]

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> review: approve
>
> Thanks for this, nice change. I must admit, I'm surprised that curl
> doesn't do this by default, wget does.

Thanks for the review!

> > +// The maximum number of redirection-followings allowed. +var
> > _CURL_MAX_REDIRECTS = 10
>
> Why did you choose 10? Document that here in the comment.

Well, I confess I sort of chose it at random :). Looks like something between 7 and 10 is reasonable (judging by what people use) and it sounds reasonable to me :).

> Consider using http.Header instead of map[string]string here (like
> performRequest() in storage_base.go does), it is the same thing
> underneath but conveys intent better.

Okay.

> Consider making this a separate test, it muddies the focus of the rest
> of the test below. Short, focused are much easier to read and less flaky.

I considered that but I rejected it because I'm testing a very simple functionality in curl itself and the main goal of this test is not really to test the functionality in itself but to make sure that the option is turned on in CURL. Also, this test is pretty expensive to set up (in terms of number of lines) and that's why I prefer to do it in one test.

[lp:~rvb/gwacl/handle-307]

149. By Raphaël Badin on 2013-07-02

Review fixes.

=== modified file 'x509dispatcher.go'
--- x509dispatcher.go	2013-06-27 10:55:35 +0000
+++ x509dispatcher.go	2013-07-02 07:18:24 +0000
@@ -178,6 +178,9 @@
     return response, nil
 }
 
+// The maximum number of redirection-followings allowed. 
+var _CURL_MAX_REDIRECTS = 10
+
 // makeCurlRequest produces a curl.CURL representing the request.
 // Clean up the request by calling its Cleanup() method after you're done
 // with it.
@@ -188,6 +191,8 @@
     ch.Setopt(curl.OPT_KEYPASSWD, "")
     ch.Setopt(curl.OPT_URL, request.URL)
     //ch.Setopt(curl.OPT_VERBOSE, true)
+    ch.Setopt(curl.OPT_FOLLOWLOCATION, true)
+    ch.Setopt(curl.OPT_MAXREDIRS, _CURL_MAX_REDIRECTS)
     ch.Setopt(curl.OPT_SSLVERSION, 3)
     versionHeader := "x-ms-version: " + request.APIVersion
     headers := []string{versionHeader}

=== modified file 'x509dispatcher_test.go'
--- x509dispatcher_test.go	2013-03-26 10:23:51 +0000
+++ x509dispatcher_test.go	2013-07-02 07:18:24 +0000
@@ -14,9 +14,14 @@
 // that serves at the given base URL, copies incoming requests into the given
 // channel, and finally returns the given status code.  If body is not nil, it
 // will be returned as the request body.
-func makeRecordingHTTPServer(requests chan *http.Request, status int, body []byte) *httptest.Server {
+func makeRecordingHTTPServer(requests chan *http.Request, status int, body []byte, headers http.Header) *httptest.Server {
     returnRequest := func(w http.ResponseWriter, r *http.Request) {
         requests <- r
+        for header, values := range headers {
+            for _, value := range values {
+                w.Header().Set(header, value)
+            }
+        }
         w.WriteHeader(status)
         if body != nil {
             w.Write(body)
@@ -55,7 +60,7 @@
 
 func (*x509DispatcherSuite) TestGetRequestDoesHTTPGET(c *C) {
     httpRequests := make(chan *http.Request, 1)
-    server := makeRecordingHTTPServer(httpRequests, http.StatusOK, nil)
+    server := makeRecordingHTTPServer(httpRequests, http.StatusOK, nil, nil)
     defer server.Close()
     // No real certificate needed since we're testing on http, not https.
     session, err := newX509Session("subscriptionid", "cert.pem")
@@ -72,12 +77,50 @@
     c.Check(httpRequest.URL.String(), Equals, path)
 }
 
+func (*x509DispatcherSuite) TestGetFollowsRedirects(c *C) {
+    redirectPath := "/redirect/path"
+    httpRequests := make(chan *http.Request, _CURL_MAX_REDIRECTS+1)
+    locationHeaders := http.Header{}
+    locationHeaders.Add("Location", redirectPath)
+    server := makeRecordingHTTPServer(httpRequests, http.StatusTemporaryRedirect, nil, locationHeaders)
+    defer server.Close()
+    // No real certificate needed since we're testing on http, not https.
+    session, err := newX509Session("subscriptionid", "cert.pem")
+    c.Assert(err, IsNil)
+    path := "/foo/bar"
+    request := newX509RequestGET(server.URL + path)
+
+    _, err = performX509CurlRequest(session, request)
+    c.Check(err, ErrorMatches, ".*Number of redirects hit maximum amount.*")
+
+    var httpRequest *http.Request
+    // The original GET request has been performed.
+    select {
+    case httpRequest = <-httpRequests:
+    default:
+        c.Error("The original request has not been performed.")
+    }
+    c.Check(httpRequest.Method, Equals, "GET")
+    c.Check(httpRequest.URL.String(), Equals, path)
+
+    // _CURL_MAX_REDIRECTS redirected requests have been performed.
+    for i := 0; i < _CURL_MAX_REDIRECTS; i++ {
+        select {
+        case httpRequest := <-httpRequests:
+            c.Check(httpRequest.Method, Equals, "GET")
+            c.Check(httpRequest.URL.String(), Equals, redirectPath)
+        default:
+            c.Error("No redirection has happened.")
+        }
+    }
+}
+
 func (*x509DispatcherSuite) TestPostRequestDoesHTTPPOST(c *C) {
     httpRequests := make(chan *http.Request, 1)
     requestBody := []byte{1, 2, 3}
     responseBody := []byte{4, 5, 6}
     requestContentType := "bogusContentType"
-    server := makeRecordingHTTPServer(httpRequests, http.StatusOK, responseBody)
+    server := makeRecordingHTTPServer(httpRequests, http.StatusOK, responseBody, nil)
     defer server.Close()
     // No real certificate needed since we're testing on http, not https.
     session, err := newX509Session("subscriptionid", "cert.pem")
@@ -99,7 +142,7 @@
 
 func (*x509DispatcherSuite) TestDeleteRequestDoesHTTPDELETE(c *C) {
     httpRequests := make(chan *http.Request, 1)
-    server := makeRecordingHTTPServer(httpRequests, http.StatusOK, []byte{})
+    server := makeRecordingHTTPServer(httpRequests, http.StatusOK, nil, nil)
     defer server.Close()
     // No real certificate needed since we're testing on http, not https.
     session, err := newX509Session("subscriptionid", "cert.pem")
@@ -120,7 +163,7 @@
     httpRequests := make(chan *http.Request, 1)
     requestBody := []byte{1, 2, 3}
     responseBody := []byte{4, 5, 6}
-    server := makeRecordingHTTPServer(httpRequests, http.StatusOK, responseBody)
+    server := makeRecordingHTTPServer(httpRequests, http.StatusOK, responseBody, nil)
     defer server.Close()
     // No real certificate needed since we're testing on http, not https.
     session, err := newX509Session("subscriptionid", "cert.pem")