Skip to content

Commit 27ef09a

Browse files
cpuguy83cpuguy83
committed
Set ping version even on error
In some cases a server may return an error on the ping response but still provide version details. The client should use these values when available. Signed-off-by: Brian Goff <[email protected]>
1 parent 8a672ba commit 27ef09a

3 files changed

Lines changed: 132 additions & 33 deletions

File tree

‎client/ping.go‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,15 @@ func (cli *Client) Ping(ctx context.Context) (types.Ping, error) {
1818
}
1919
defer ensureReaderClosed(serverResp)
2020

21-
ping.APIVersion = serverResp.header.Get("API-Version")
21+
if serverResp.header != nil {
22+
ping.APIVersion = serverResp.header.Get("API-Version")
2223

23-
if serverResp.header.Get("Docker-Experimental") == "true" {
24-
ping.Experimental = true
24+
if serverResp.header.Get("Docker-Experimental") == "true" {
25+
ping.Experimental = true
26+
}
27+
ping.OSType = serverResp.header.Get("OSType")
2528
}
2629

27-
ping.OSType = serverResp.header.Get("OSType")
28-
29-
return ping, nil
30+
err = cli.checkResponseErr(serverResp)
31+
return ping, err
3032
}

‎client/ping_test.go‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
package client
2+
3+
import (
4+
"errors"
5+
"io/ioutil"
6+
"net/http"
7+
"strings"
8+
"testing"
9+
10+
"github.com/stretchr/testify/assert"
11+
"golang.org/x/net/context"
12+
)
13+
14+
// TestPingFail tests that when a server sends a non-successful response that we
15+
// can still grab API details, when set.
16+
// Some of this is just excercising the code paths to make sure there are no
17+
// panics.
18+
func TestPingFail(t *testing.T) {
19+
var withHeader bool
20+
client := &Client{
21+
client: newMockClient(func(req *http.Request) (*http.Response, error) {
22+
resp := &http.Response{StatusCode: http.StatusInternalServerError}
23+
if withHeader {
24+
resp.Header = http.Header{}
25+
resp.Header.Set("API-Version", "awesome")
26+
resp.Header.Set("Docker-Experimental", "true")
27+
}
28+
resp.Body = ioutil.NopCloser(strings.NewReader("some error with the server"))
29+
return resp, nil
30+
}),
31+
}
32+
33+
ping, err := client.Ping(context.Background())
34+
assert.Error(t, err)
35+
assert.Equal(t, false, ping.Experimental)
36+
assert.Equal(t, "", ping.APIVersion)
37+
38+
withHeader = true
39+
ping2, err := client.Ping(context.Background())
40+
assert.Error(t, err)
41+
assert.Equal(t, true, ping2.Experimental)
42+
assert.Equal(t, "awesome", ping2.APIVersion)
43+
}
44+
45+
// TestPingWithError tests the case where there is a protocol error in the ping.
46+
// This test is mostly just testing that there are no panics in this code path.
47+
func TestPingWithError(t *testing.T) {
48+
client := &Client{
49+
client: newMockClient(func(req *http.Request) (*http.Response, error) {
50+
resp := &http.Response{StatusCode: http.StatusInternalServerError}
51+
resp.Header = http.Header{}
52+
resp.Header.Set("API-Version", "awesome")
53+
resp.Header.Set("Docker-Experimental", "true")
54+
resp.Body = ioutil.NopCloser(strings.NewReader("some error with the server"))
55+
return resp, errors.New("some error")
56+
}),
57+
}
58+
59+
ping, err := client.Ping(context.Background())
60+
assert.Error(t, err)
61+
assert.Equal(t, false, ping.Experimental)
62+
assert.Equal(t, "", ping.APIVersion)
63+
}
64+
65+
// TestPingSuccess tests that we are able to get the expected API headers/ping
66+
// details on success.
67+
func TestPingSuccess(t *testing.T) {
68+
client := &Client{
69+
client: newMockClient(func(req *http.Request) (*http.Response, error) {
70+
resp := &http.Response{StatusCode: http.StatusInternalServerError}
71+
resp.Header = http.Header{}
72+
resp.Header.Set("API-Version", "awesome")
73+
resp.Header.Set("Docker-Experimental", "true")
74+
resp.Body = ioutil.NopCloser(strings.NewReader("some error with the server"))
75+
return resp, nil
76+
}),
77+
}
78+
ping, err := client.Ping(context.Background())
79+
assert.Error(t, err)
80+
assert.Equal(t, true, ping.Experimental)
81+
assert.Equal(t, "awesome", ping.APIVersion)
82+
}

‎client/request.go‎

Lines changed: 42 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ type serverResponse struct {
2424
body io.ReadCloser
2525
header http.Header
2626
statusCode int
27+
reqURL *url.URL
2728
}
2829

2930
// head sends an http request to the docker API using the method HEAD.
@@ -118,11 +119,18 @@ func (cli *Client) sendRequest(ctx context.Context, method, path string, query u
118119
if err != nil {
119120
return serverResponse{}, err
120121
}
121-
return cli.doRequest(ctx, req)
122+
resp, err := cli.doRequest(ctx, req)
123+
if err != nil {
124+
return resp, err
125+
}
126+
if err := cli.checkResponseErr(resp); err != nil {
127+
return resp, err
128+
}
129+
return resp, nil
122130
}
123131

124132
func (cli *Client) doRequest(ctx context.Context, req *http.Request) (serverResponse, error) {
125-
serverResp := serverResponse{statusCode: -1}
133+
serverResp := serverResponse{statusCode: -1, reqURL: req.URL}
126134

127135
resp, err := ctxhttp.Do(ctx, cli.client, req)
128136
if err != nil {
@@ -179,35 +187,42 @@ func (cli *Client) doRequest(ctx context.Context, req *http.Request) (serverResp
179187

180188
if resp != nil {
181189
serverResp.statusCode = resp.StatusCode
190+
serverResp.body = resp.Body
191+
serverResp.header = resp.Header
182192
}
193+
return serverResp, nil
194+
}
183195

184-
if serverResp.statusCode < 200 || serverResp.statusCode >= 400 {
185-
body, err := ioutil.ReadAll(resp.Body)
186-
if err != nil {
187-
return serverResp, err
188-
}
189-
if len(body) == 0 {
190-
return serverResp, fmt.Errorf("Error: request returned %s for API route and version %s, check if the server supports the requested API version", http.StatusText(serverResp.statusCode), req.URL)
191-
}
196+
func (cli *Client) checkResponseErr(serverResp serverResponse) error {
197+
if serverResp.statusCode >= 200 && serverResp.statusCode < 400 {
198+
return nil
199+
}
192200

193-
var errorMessage string
194-
if (cli.version == "" || versions.GreaterThan(cli.version, "1.23")) &&
195-
resp.Header.Get("Content-Type") == "application/json" {
196-
var errorResponse types.ErrorResponse
197-
if err := json.Unmarshal(body, &errorResponse); err != nil {
198-
return serverResp, fmt.Errorf("Error reading JSON: %v", err)
199-
}
200-
errorMessage = errorResponse.Message
201-
} else {
202-
errorMessage = string(body)
203-
}
201+
body, err := ioutil.ReadAll(serverResp.body)
202+
if err != nil {
203+
return err
204+
}
205+
if len(body) == 0 {
206+
return fmt.Errorf("Error: request returned %s for API route and version %s, check if the server supports the requested API version", http.StatusText(serverResp.statusCode), serverResp.reqURL)
207+
}
204208

205-
return serverResp, fmt.Errorf("Error response from daemon: %s", strings.TrimSpace(errorMessage))
209+
var ct string
210+
if serverResp.header != nil {
211+
ct = serverResp.header.Get("Content-Type")
206212
}
207213

208-
serverResp.body = resp.Body
209-
serverResp.header = resp.Header
210-
return serverResp, nil
214+
var errorMessage string
215+
if (cli.version == "" || versions.GreaterThan(cli.version, "1.23")) && ct == "application/json" {
216+
var errorResponse types.ErrorResponse
217+
if err := json.Unmarshal(body, &errorResponse); err != nil {
218+
return fmt.Errorf("Error reading JSON: %v", err)
219+
}
220+
errorMessage = errorResponse.Message
221+
} else {
222+
errorMessage = string(body)
223+
}
224+
225+
return fmt.Errorf("Error response from daemon: %s", strings.TrimSpace(errorMessage))
211226
}
212227

213228
func (cli *Client) addHeaders(req *http.Request, headers headers) *http.Request {
@@ -239,9 +254,9 @@ func encodeData(data interface{}) (*bytes.Buffer, error) {
239254
}
240255

241256
func ensureReaderClosed(response serverResponse) {
242-
if body := response.body; body != nil {
257+
if response.body != nil {
243258
// Drain up to 512 bytes and close the body to let the Transport reuse the connection
244-
io.CopyN(ioutil.Discard, body, 512)
259+
io.CopyN(ioutil.Discard, response.body, 512)
245260
response.body.Close()
246261
}
247262
}

0 commit comments

Comments
 (0)