From 9d2b3f64639fe378169709895487f6cadfc013ba Mon Sep 17 00:00:00 2001 From: Boaz Shuster Date: Mon, 27 Mar 2017 12:05:35 +0300 Subject: [PATCH] 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 Upstream-commit: eb36d6021618f788012c166a533f1b321cda9695 Component: engine --- components/engine/client/client.go | 25 ++++++++++++- components/engine/client/client_test.go | 50 +++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/components/engine/client/client.go b/components/engine/client/client.go index df3698adc6..f8f2fc6ad5 100644 --- a/components/engine/client/client.go +++ b/components/engine/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, } } diff --git a/components/engine/client/client_test.go b/components/engine/client/client_test.go index 0816d8a9bf..77214bc53c 100644 --- a/components/engine/client/client_test.go +++ b/components/engine/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) + } +}