Browse code
client: Client.Ping: improve error handling and fallback
The Ping function first tries to do a HEAD request, but the parsePingResponse
was written with the assumption that a Body could be present in the response
that may include errors returned by the API server.
HEAD responses don't include a body, so there's no response to handle, and
no errors to return by the API, other than a HTTP status code.
This patch:
- Rewrites `parsePingResponse` to a `newPingResponse`, removing the error-
handling for the response-body. It's also simplified, because a non-nil
response is guaranteed to have a non-nil Header (but it may not have
any of the headers set that are used for the Ping).
- Rewrites the `Client.Ping` to only return a Ping-response from the HEAD
request if no error was returned (i.e., we connected with the API) and
a successful status-code, otherwise it will fallback to a GET request,
which allows (for non "OK" (200) status-codes) returning errors from
the daemon (for example, if the daemon is in an unhealthy state).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sebastiaan van Stijn authored on 2025/08/13 04:48:12Showing 3 changed files
| ... | ... |
@@ -19,63 +19,56 @@ import ( |
| 19 | 19 |
// for other non-success status codes, failing to connect to the API, or failing |
| 20 | 20 |
// to parse the API response. |
| 21 | 21 |
func (cli *Client) Ping(ctx context.Context) (types.Ping, error) {
|
| 22 |
- var ping types.Ping |
|
| 23 |
- |
|
| 24 | 22 |
// Using cli.buildRequest() + cli.doRequest() instead of cli.sendRequest() |
| 25 | 23 |
// because ping requests are used during API version negotiation, so we want |
| 26 | 24 |
// to hit the non-versioned /_ping endpoint, not /v1.xx/_ping |
| 27 | 25 |
req, err := cli.buildRequest(ctx, http.MethodHead, path.Join(cli.basePath, "/_ping"), nil, nil) |
| 28 | 26 |
if err != nil {
|
| 29 |
- return ping, err |
|
| 27 |
+ return types.Ping{}, err
|
|
| 30 | 28 |
} |
| 31 | 29 |
resp, err := cli.doRequest(req) |
| 32 |
- if err != nil {
|
|
| 33 |
- if IsErrConnectionFailed(err) {
|
|
| 34 |
- return ping, err |
|
| 35 |
- } |
|
| 36 |
- // We managed to connect, but got some error; continue and try GET request. |
|
| 37 |
- } else {
|
|
| 38 |
- defer ensureReaderClosed(resp) |
|
| 39 |
- switch resp.StatusCode {
|
|
| 40 |
- case http.StatusOK, http.StatusInternalServerError: |
|
| 41 |
- // Server handled the request, so parse the response |
|
| 42 |
- return parsePingResponse(resp) |
|
| 43 |
- } |
|
| 30 |
+ defer ensureReaderClosed(resp) |
|
| 31 |
+ if err == nil && resp.StatusCode == http.StatusOK {
|
|
| 32 |
+ // Fast-path; successfully connected using a HEAD request and |
|
| 33 |
+ // we got a "OK" (200) status. For non-200 status-codes, we fall |
|
| 34 |
+ // back to doing a GET request, as a HEAD request won't have a |
|
| 35 |
+ // response-body to get error details from. |
|
| 36 |
+ return newPingResponse(resp), nil |
|
| 44 | 37 |
} |
| 45 | 38 |
|
| 46 |
- // HEAD failed; fallback to GET. |
|
| 39 |
+ // HEAD failed or returned a non-OK status; fallback to GET. |
|
| 47 | 40 |
req.Method = http.MethodGet |
| 48 | 41 |
resp, err = cli.doRequest(req) |
| 49 | 42 |
defer ensureReaderClosed(resp) |
| 50 | 43 |
if err != nil {
|
| 51 |
- return ping, err |
|
| 44 |
+ // Failed to connect. |
|
| 45 |
+ return types.Ping{}, err
|
|
| 52 | 46 |
} |
| 53 |
- return parsePingResponse(resp) |
|
| 47 |
+ |
|
| 48 |
+ // GET request succeeded but may have returned a non-200 status. |
|
| 49 |
+ // Return a Ping response, together with any error returned by |
|
| 50 |
+ // the API server. |
|
| 51 |
+ return newPingResponse(resp), checkResponseErr(resp) |
|
| 54 | 52 |
} |
| 55 | 53 |
|
| 56 |
-func parsePingResponse(resp *http.Response) (types.Ping, error) {
|
|
| 54 |
+func newPingResponse(resp *http.Response) types.Ping {
|
|
| 57 | 55 |
if resp == nil {
|
| 58 |
- return types.Ping{}, nil
|
|
| 59 |
- } |
|
| 60 |
- |
|
| 61 |
- var ping types.Ping |
|
| 62 |
- if resp.Header == nil {
|
|
| 63 |
- return ping, checkResponseErr(resp) |
|
| 64 |
- } |
|
| 65 |
- ping.APIVersion = resp.Header.Get("Api-Version")
|
|
| 66 |
- ping.OSType = resp.Header.Get("Ostype")
|
|
| 67 |
- if resp.Header.Get("Docker-Experimental") == "true" {
|
|
| 68 |
- ping.Experimental = true |
|
| 69 |
- } |
|
| 70 |
- if bv := resp.Header.Get("Builder-Version"); bv != "" {
|
|
| 71 |
- ping.BuilderVersion = build.BuilderVersion(bv) |
|
| 56 |
+ return types.Ping{}
|
|
| 72 | 57 |
} |
| 58 |
+ var swarmStatus *swarm.Status |
|
| 73 | 59 |
if si := resp.Header.Get("Swarm"); si != "" {
|
| 74 | 60 |
state, role, _ := strings.Cut(si, "/") |
| 75 |
- ping.SwarmStatus = &swarm.Status{
|
|
| 61 |
+ swarmStatus = &swarm.Status{
|
|
| 76 | 62 |
NodeState: swarm.LocalNodeState(state), |
| 77 | 63 |
ControlAvailable: role == "manager", |
| 78 | 64 |
} |
| 79 | 65 |
} |
| 80 |
- return ping, checkResponseErr(resp) |
|
| 66 |
+ |
|
| 67 |
+ return types.Ping{
|
|
| 68 |
+ APIVersion: resp.Header.Get("Api-Version"),
|
|
| 69 |
+ OSType: resp.Header.Get("Ostype"),
|
|
| 70 |
+ Experimental: resp.Header.Get("Docker-Experimental") == "true",
|
|
| 71 |
+ BuilderVersion: build.BuilderVersion(resp.Header.Get("Builder-Version")),
|
|
| 72 |
+ SwarmStatus: swarmStatus, |
|
| 73 |
+ } |
|
| 81 | 74 |
} |
| ... | ... |
@@ -90,23 +90,23 @@ func TestPingSuccess(t *testing.T) {
|
| 90 | 90 |
func TestPingHeadFallback(t *testing.T) {
|
| 91 | 91 |
tests := []struct {
|
| 92 | 92 |
status int |
| 93 |
- expected string |
|
| 93 |
+ expected []string |
|
| 94 | 94 |
}{
|
| 95 | 95 |
{
|
| 96 | 96 |
status: http.StatusOK, |
| 97 |
- expected: http.MethodHead, |
|
| 97 |
+ expected: []string{http.MethodHead},
|
|
| 98 | 98 |
}, |
| 99 | 99 |
{
|
| 100 | 100 |
status: http.StatusInternalServerError, |
| 101 |
- expected: http.MethodHead, |
|
| 101 |
+ expected: []string{http.MethodHead, http.MethodGet},
|
|
| 102 | 102 |
}, |
| 103 | 103 |
{
|
| 104 | 104 |
status: http.StatusNotFound, |
| 105 |
- expected: "HEAD, GET", |
|
| 105 |
+ expected: []string{http.MethodHead, http.MethodGet},
|
|
| 106 | 106 |
}, |
| 107 | 107 |
{
|
| 108 | 108 |
status: http.StatusMethodNotAllowed, |
| 109 |
- expected: "HEAD, GET", |
|
| 109 |
+ expected: []string{http.MethodHead, http.MethodGet},
|
|
| 110 | 110 |
}, |
| 111 | 111 |
} |
| 112 | 112 |
|
| ... | ... |
@@ -116,17 +116,17 @@ func TestPingHeadFallback(t *testing.T) {
|
| 116 | 116 |
client := &Client{
|
| 117 | 117 |
client: newMockClient(func(req *http.Request) (*http.Response, error) {
|
| 118 | 118 |
reqs = append(reqs, req.Method) |
| 119 |
- resp := &http.Response{StatusCode: http.StatusOK}
|
|
| 119 |
+ resp := &http.Response{StatusCode: http.StatusOK, Header: http.Header{}}
|
|
| 120 | 120 |
if req.Method == http.MethodHead {
|
| 121 | 121 |
resp.StatusCode = tc.status |
| 122 | 122 |
} |
| 123 |
- resp.Header = http.Header{}
|
|
| 124 |
- resp.Header.Add("Api-Version", strings.Join(reqs, ", "))
|
|
| 123 |
+ resp.Header.Add("Api-Version", "v1.2.3")
|
|
| 125 | 124 |
return resp, nil |
| 126 | 125 |
}), |
| 127 | 126 |
} |
| 128 | 127 |
ping, _ := client.Ping(context.Background()) |
| 129 |
- assert.Check(t, is.Equal(ping.APIVersion, tc.expected)) |
|
| 128 |
+ assert.Check(t, is.Equal(ping.APIVersion, "v1.2.3")) |
|
| 129 |
+ assert.Check(t, is.DeepEqual(reqs, tc.expected)) |
|
| 130 | 130 |
}) |
| 131 | 131 |
} |
| 132 | 132 |
} |
| ... | ... |
@@ -19,63 +19,56 @@ import ( |
| 19 | 19 |
// for other non-success status codes, failing to connect to the API, or failing |
| 20 | 20 |
// to parse the API response. |
| 21 | 21 |
func (cli *Client) Ping(ctx context.Context) (types.Ping, error) {
|
| 22 |
- var ping types.Ping |
|
| 23 |
- |
|
| 24 | 22 |
// Using cli.buildRequest() + cli.doRequest() instead of cli.sendRequest() |
| 25 | 23 |
// because ping requests are used during API version negotiation, so we want |
| 26 | 24 |
// to hit the non-versioned /_ping endpoint, not /v1.xx/_ping |
| 27 | 25 |
req, err := cli.buildRequest(ctx, http.MethodHead, path.Join(cli.basePath, "/_ping"), nil, nil) |
| 28 | 26 |
if err != nil {
|
| 29 |
- return ping, err |
|
| 27 |
+ return types.Ping{}, err
|
|
| 30 | 28 |
} |
| 31 | 29 |
resp, err := cli.doRequest(req) |
| 32 |
- if err != nil {
|
|
| 33 |
- if IsErrConnectionFailed(err) {
|
|
| 34 |
- return ping, err |
|
| 35 |
- } |
|
| 36 |
- // We managed to connect, but got some error; continue and try GET request. |
|
| 37 |
- } else {
|
|
| 38 |
- defer ensureReaderClosed(resp) |
|
| 39 |
- switch resp.StatusCode {
|
|
| 40 |
- case http.StatusOK, http.StatusInternalServerError: |
|
| 41 |
- // Server handled the request, so parse the response |
|
| 42 |
- return parsePingResponse(resp) |
|
| 43 |
- } |
|
| 30 |
+ defer ensureReaderClosed(resp) |
|
| 31 |
+ if err == nil && resp.StatusCode == http.StatusOK {
|
|
| 32 |
+ // Fast-path; successfully connected using a HEAD request and |
|
| 33 |
+ // we got a "OK" (200) status. For non-200 status-codes, we fall |
|
| 34 |
+ // back to doing a GET request, as a HEAD request won't have a |
|
| 35 |
+ // response-body to get error details from. |
|
| 36 |
+ return newPingResponse(resp), nil |
|
| 44 | 37 |
} |
| 45 | 38 |
|
| 46 |
- // HEAD failed; fallback to GET. |
|
| 39 |
+ // HEAD failed or returned a non-OK status; fallback to GET. |
|
| 47 | 40 |
req.Method = http.MethodGet |
| 48 | 41 |
resp, err = cli.doRequest(req) |
| 49 | 42 |
defer ensureReaderClosed(resp) |
| 50 | 43 |
if err != nil {
|
| 51 |
- return ping, err |
|
| 44 |
+ // Failed to connect. |
|
| 45 |
+ return types.Ping{}, err
|
|
| 52 | 46 |
} |
| 53 |
- return parsePingResponse(resp) |
|
| 47 |
+ |
|
| 48 |
+ // GET request succeeded but may have returned a non-200 status. |
|
| 49 |
+ // Return a Ping response, together with any error returned by |
|
| 50 |
+ // the API server. |
|
| 51 |
+ return newPingResponse(resp), checkResponseErr(resp) |
|
| 54 | 52 |
} |
| 55 | 53 |
|
| 56 |
-func parsePingResponse(resp *http.Response) (types.Ping, error) {
|
|
| 54 |
+func newPingResponse(resp *http.Response) types.Ping {
|
|
| 57 | 55 |
if resp == nil {
|
| 58 |
- return types.Ping{}, nil
|
|
| 59 |
- } |
|
| 60 |
- |
|
| 61 |
- var ping types.Ping |
|
| 62 |
- if resp.Header == nil {
|
|
| 63 |
- return ping, checkResponseErr(resp) |
|
| 64 |
- } |
|
| 65 |
- ping.APIVersion = resp.Header.Get("Api-Version")
|
|
| 66 |
- ping.OSType = resp.Header.Get("Ostype")
|
|
| 67 |
- if resp.Header.Get("Docker-Experimental") == "true" {
|
|
| 68 |
- ping.Experimental = true |
|
| 69 |
- } |
|
| 70 |
- if bv := resp.Header.Get("Builder-Version"); bv != "" {
|
|
| 71 |
- ping.BuilderVersion = build.BuilderVersion(bv) |
|
| 56 |
+ return types.Ping{}
|
|
| 72 | 57 |
} |
| 58 |
+ var swarmStatus *swarm.Status |
|
| 73 | 59 |
if si := resp.Header.Get("Swarm"); si != "" {
|
| 74 | 60 |
state, role, _ := strings.Cut(si, "/") |
| 75 |
- ping.SwarmStatus = &swarm.Status{
|
|
| 61 |
+ swarmStatus = &swarm.Status{
|
|
| 76 | 62 |
NodeState: swarm.LocalNodeState(state), |
| 77 | 63 |
ControlAvailable: role == "manager", |
| 78 | 64 |
} |
| 79 | 65 |
} |
| 80 |
- return ping, checkResponseErr(resp) |
|
| 66 |
+ |
|
| 67 |
+ return types.Ping{
|
|
| 68 |
+ APIVersion: resp.Header.Get("Api-Version"),
|
|
| 69 |
+ OSType: resp.Header.Get("Ostype"),
|
|
| 70 |
+ Experimental: resp.Header.Get("Docker-Experimental") == "true",
|
|
| 71 |
+ BuilderVersion: build.BuilderVersion(resp.Header.Get("Builder-Version")),
|
|
| 72 |
+ SwarmStatus: swarmStatus, |
|
| 73 |
+ } |
|
| 81 | 74 |
} |