Return error on client redirect

From Go 1.8 HTTP client redirect behaviour is changed:
When status code is 301, 307 or 308, the client
automatically converts it to a new HTTP request.

This behaviour change manifests in the client in that
before the 301 was not followed and the client did not generate
an error, but now results in an error message:
"Error response from daemon: page not found."

To fix that a new redirect policy is forced by setting
HTTP Client's CheckRedirect.
That policy is to return an error for any 301, 307 or 308
in the response's status code to a non-GET request.
The error message specifies that the daemon could not
process the request and it is probably due to bad
arguments that were provided by the user.

Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>

client/client.go

@@ -46,6 +46,7 @@ For example, to list running containers (the equivalent of "docker ps"):
 package client
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -58,6 +59,9 @@ import (
 	"github.com/docker/go-connections/tlsconfig"
 )
 
+// ErrRedirect is the error returned by checkRedirect when the request is non-GET.
+var ErrRedirect = errors.New("unexpected redirect in response")
+
 // Client is the API client that performs all operations
 // against a docker server.
 type Client struct {
@@ -81,6 +85,23 @@ type Client struct {
 	manualOverride bool
 }
 
+// CheckRedirect specifies the policy for dealing with redirect responses:
+// If the request is non-GET return `ErrRedirect`. Otherwise use the last response.
+//
+// Go 1.8 changes behavior for HTTP redirects (specificlaly 301, 307, and 308) in the client .
+// The Docker client (and by extension docker API client) can be made to to send a request
+// like POST /containers//start where what would normally be in the name section of the URL is empty.
+// This triggers an HTTP 301 from the daemon.
+// In go 1.8 this 301 will be converted to a GET request, and ends up getting a 404 from the daemon.
+// This behavior change manifests in the client in that before the 301 was not followed and
+// the client did not generate an error, but now results in a message like Error response from daemon: page not found.
+func CheckRedirect(req *http.Request, via []*http.Request) error {
+	if via[0].Method == http.MethodGet {
+		return http.ErrUseLastResponse
+	}
+	return ErrRedirect
+}
+
 // NewEnvClient initializes a new API client based on environment variables.
 // Use DOCKER_HOST to set the url to the docker server.
 // Use DOCKER_API_VERSION to set the version of the API to reach, leave empty for latest.
@@ -104,6 +125,7 @@ func NewEnvClient() (*Client, error) {
 			Transport: &http.Transport{
 				TLSClientConfig: tlsc,
 			},
+			CheckRedirect: CheckRedirect,
 		}
 	}
 
@@ -147,7 +169,8 @@ func NewClient(host string, version string, client *http.Client, httpHeaders map
 		transport := new(http.Transport)
 		sockets.ConfigureTransport(transport, proto, addr)
 		client = &http.Client{
-			Transport: transport,
+			Transport:     transport,
+			CheckRedirect: CheckRedirect,
 		}
 	}
 

client/client_test.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/docker/docker/api"
 	"github.com/docker/docker/api/types"
+	"github.com/stretchr/testify/assert"
 	"golang.org/x/net/context"
 )
 
@@ -282,3 +283,52 @@ func TestNewEnvClientSetsDefaultVersion(t *testing.T) {
 		os.Setenv(key, envVarValues[key])
 	}
 }
+
+type roundTripFunc func(*http.Request) (*http.Response, error)
+
+func (rtf roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) {
+	return rtf(req)
+}
+
+type bytesBufferClose struct {
+	*bytes.Buffer
+}
+
+func (bbc bytesBufferClose) Close() error {
+	return nil
+}
+
+func TestClientRedirect(t *testing.T) {
+	client := &http.Client{
+		CheckRedirect: CheckRedirect,
+		Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) {
+			if req.URL.String() == "/bla" {
+				return &http.Response{StatusCode: 404}, nil
+			}
+			return &http.Response{
+				StatusCode: 301,
+				Header:     map[string][]string{"Location": {"/bla"}},
+				Body:       bytesBufferClose{bytes.NewBuffer(nil)},
+			}, nil
+		}),
+	}
+
+	cases := []struct {
+		httpMethod  string
+		expectedErr error
+		statusCode  int
+	}{
+		{http.MethodGet, nil, 301},
+		{http.MethodPost, &url.Error{Op: "Post", URL: "/bla", Err: ErrRedirect}, 301},
+		{http.MethodPut, &url.Error{Op: "Put", URL: "/bla", Err: ErrRedirect}, 301},
+		{http.MethodDelete, &url.Error{Op: "Delete", URL: "/bla", Err: ErrRedirect}, 301},
+	}
+
+	for _, tc := range cases {
+		req, err := http.NewRequest(tc.httpMethod, "/redirectme", nil)
+		assert.NoError(t, err)
+		resp, err := client.Do(req)
+		assert.Equal(t, tc.expectedErr, err)
+		assert.Equal(t, tc.statusCode, resp.StatusCode)
+	}
+}