Merge lp:~axwalk/gwacl/management-api-307-redirects into lp:gwacl

[Andrew Wilkins]

Add handling of temporary redirects in the management API.

Add handling of temporary redirects in the management API.

[Ian Booth (wallyworld)]

Looks good. Did you test it live? I think that should be done before landing.

review: Approve

[Andrew Wilkins (axwalk)]

I tested bootstrap and destroy-env a bunch of times. I'll run some deployment tests before landing.

[Andrew Wilkins (axwalk)]

Seems there's still a problem.

[lp:~axwalk/gwacl/management-api-307-redirects]

232. By Andrew Wilkins on 2014-02-05

Also check redirect status code for POST/DELETE, etc.

=== modified file 'x509dispatcher.go'
--- x509dispatcher.go	2013-08-09 13:59:43 +0000
+++ x509dispatcher.go	2014-02-05 04:46:43 +0000
@@ -5,9 +5,11 @@
 
 import (
     "bytes"
+    "fmt"
     "io/ioutil"
     "launchpad.net/gwacl/fork/http"
     . "launchpad.net/gwacl/logging"
+    "net/url"
 )
 
 type X509Request struct {
@@ -84,7 +86,7 @@
     httpRequest.Header.Set("Content-Type", request.ContentType)
     httpRequest.Header.Set("x-ms-version", request.APIVersion)
     retrier := session.retryPolicy.getForkedHttpRetrier(session.client)
-    httpResponse, err := retrier.RetryRequest(httpRequest)
+    httpResponse, err := handleRequestRedirects(retrier, httpRequest)
     if err != nil {
         return nil, err
     }
@@ -108,3 +110,35 @@
 
     return response, nil
 }
+
+func handleRequestRedirects(retrier *forkedHttpRetrier, request *http.Request) (*http.Response, error) {
+    const maxRedirects = 10
+    // Handle temporary redirects.
+    redirects := -1
+    for {
+        redirects++
+        if redirects >= maxRedirects {
+            return nil, fmt.Errorf("stopped after %d redirects", redirects)
+        }
+        response, err := retrier.RetryRequest(request)
+        // For GET and HEAD, we cause the request execution
+        // to return httpRedirectErr if a temporary redirect
+        // is returned, and then deal with it here.
+        if err, ok := err.(*url.Error); ok && err.Err == httpRedirectErr {
+            request.URL, err.Err = url.Parse(err.URL)
+            if err.Err != nil {
+                return nil, err
+            }
+            continue
+        }
+        // For other methods, we must check the response code.
+        if err == nil && response.StatusCode == http.StatusTemporaryRedirect {
+            request.URL, err = response.Location()
+            if err != nil {
+                return nil, err
+            }
+            continue
+        }
+        return response, err
+    }
+}

=== modified file 'x509dispatcher_test.go'
--- x509dispatcher_test.go	2013-08-09 14:59:50 +0000
+++ x509dispatcher_test.go	2014-02-05 04:46:43 +0000
@@ -22,6 +22,7 @@
 // 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 *Request, status int, body []byte, headers http.Header) *httptest.Server {
+    var server *httptest.Server
     returnRequest := func(w http.ResponseWriter, r *http.Request) {
         // Capture all the request body content for later inspection.
         requestBody, err := ioutil.ReadAll(r.Body)
@@ -29,7 +30,8 @@
             panic(err)
         }
         requests <- &Request{r, requestBody}
-
+        // Set a default Location so we can test redirect loops easily.
+        w.Header().Set("Location", server.URL+r.URL.Path)
         for header, values := range headers {
             for _, value := range values {
                 w.Header().Set(header, value)
@@ -42,7 +44,8 @@
     }
     serveMux := http.NewServeMux()
     serveMux.HandleFunc("/", returnRequest)
-    return httptest.NewServer(serveMux)
+    server = httptest.NewServer(serveMux)
+    return server
 }
 
 func (*x509DispatcherSuite) TestGetRequestDoesHTTPGET(c *C) {
@@ -184,3 +187,54 @@
 
     c.Check(response.Header[customHeader], DeepEquals, customValue)
 }
+
+func (*x509DispatcherSuite) TestRequestsFollowRedirects(c *C) {
+    httpRequests := make(chan *Request, 2)
+    serverConflict := makeRecordingHTTPServer(httpRequests, http.StatusConflict, nil, nil)
+    defer serverConflict.Close()
+    redirPath := "/else/where"
+    responseHeaders := make(http.Header)
+    responseHeaders.Set("Location", serverConflict.URL+redirPath)
+    serverRedir := makeRecordingHTTPServer(httpRequests, http.StatusTemporaryRedirect, nil, responseHeaders)
+    defer serverRedir.Close()
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
+    c.Assert(err, IsNil)
+    path := "/foo/bar"
+    version := "test-version"
+
+    // Test both GET and DELETE: DELETE does not normally
+    // automatically follow redirects, however Azure requires
+    // us to.
+    requests := []*X509Request{
+        newX509RequestGET(serverRedir.URL+path, version),
+        newX509RequestDELETE(serverRedir.URL+path, version),
+    }
+    for _, request := range requests {
+        response, err := performX509Request(session, request)
+        c.Assert(err, IsNil)
+        c.Assert(response.StatusCode, Equals, http.StatusConflict)
+        c.Assert(httpRequests, HasLen, 2)
+        c.Assert((<-httpRequests).URL.String(), Equals, path)
+        c.Assert((<-httpRequests).URL.String(), Equals, redirPath)
+    }
+}
+
+func (*x509DispatcherSuite) TestRequestsLimitRedirects(c *C) {
+    httpRequests := make(chan *Request, 10)
+    serverRedir := makeRecordingHTTPServer(httpRequests, http.StatusTemporaryRedirect, nil, nil)
+    defer serverRedir.Close()
+    session, err := newX509Session("subscriptionid", "", "West US", NoRetryPolicy)
+    c.Assert(err, IsNil)
+    path := "/foo/bar"
+    version := "test-version"
+    request := newX509RequestGET(serverRedir.URL+path, version)
+
+    response, err := performX509Request(session, request)
+    c.Assert(err, ErrorMatches, "stopped after 10 redirects")
+    c.Assert(response, IsNil)
+    c.Assert(httpRequests, HasLen, 10)
+    close(httpRequests)
+    for req := range httpRequests {
+        c.Assert(req.URL.String(), Equals, path)
+    }
+}

=== modified file 'x509session.go'
--- x509session.go	2013-10-30 22:57:57 +0000
+++ x509session.go	2014-02-05 04:46:43 +0000
@@ -4,6 +4,7 @@
 package gwacl
 
 import (
+    "errors"
     "fmt"
     "launchpad.net/gwacl/fork/http"
     "launchpad.net/gwacl/fork/tls"
@@ -19,6 +20,11 @@
     retryPolicy    RetryPolicy
 }
 
+// httpRedirectErr is a unique error used to prevent
+// net/http from automatically following redirects.
+// See commentary on CheckRedirect in newX509Session.
+var httpRedirectErr = errors.New("redirect")
+
 // newX509Session creates and returns a new x509Session based on credentials
 // and X509 certificate files.
 // For testing purposes, certFile can be passed as the empty string and it
@@ -43,6 +49,13 @@
             // hit the above Go bug.
             DisableKeepAlives: true,
         },
+        // See https://code.google.com/p/go/issues/detail?id=4800
+        // We need to follow temporary redirects (307s), while
+        // retaining headers. We also need to follow redirects
+        // for POST and DELETE automatically.
+        CheckRedirect: func(req *http.Request, via []*http.Request) error {
+            return httpRedirectErr
+        },
     }
 
     endpointURL := GetEndpoint(location).ManagementAPI()