Merge pull request #32127 from ripcurld0/redirect_go18

Return error on client redirect

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)
+	}
+}