From 30e75b83961cff6be3c406bc9953fae537ebffa9 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 29 Jan 2025 11:46:38 +0100 Subject: [PATCH] client: improve handling of JSON error-responses with incorrect schema Before this patch, an API response that's valid JSON, but not the right schema would be silently discarded by the CLI. For example, due to a bug in Docker Desktop's API proxy, the "normal" (not JSON error) response would be returned together with a non-200 status code when using an unsupported API version; curl -s -w 'STATUS: %{http_code}\n' --unix-socket /var/run/docker.sock 'http://localhost/v1.99/version' {"Platform":{"Name":"Docker Desktop 4.38.0 (181016)"},"Version":"","ApiVersion":"","GitCommit":"","GoVersion":"","Os":"","Arch":""} STATUS: 400 Before this patch, this resulted in no output being shown; DOCKER_API_VERSION=1.99 docker version Client: Version: 27.5.1 API version: 1.99 (downgraded from 1.47) Go version: go1.22.11 Git commit: 9f9e405 Built: Wed Jan 22 13:37:19 2025 OS/Arch: darwin/arm64 Context: desktop-linux Error response from daemon: With this patch, an error is generated based on the status: DOCKER_API_VERSION=1.99 docker version Client: Version: 27.5.1 API version: 1.99 (downgraded from 1.47) Go version: go1.22.11 Git commit: 9f9e405 Built: Wed Jan 22 13:37:19 2025 OS/Arch: darwin/arm64 Context: desktop-linux Error response from daemon: API returned a 400 (Bad Request) but provided no error-message Signed-off-by: Sebastiaan van Stijn --- client/request.go | 29 ++++++++++++++++++++++++++++- client/request_test.go | 3 +-- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/client/request.go b/client/request.go index 6eea9b4e4f27f..5a81e4f865085 100644 --- a/client/request.go +++ b/client/request.go @@ -234,8 +234,35 @@ func (cli *Client) checkResponseErr(serverResp serverResponse) error { if err := json.Unmarshal(body, &errorResponse); err != nil { return errors.Wrap(err, "Error reading JSON") } - daemonErr = errors.New(strings.TrimSpace(errorResponse.Message)) + if errorResponse.Message == "" { + // Error-message is empty, which means that we successfully parsed the + // JSON-response (no error produced), but it didn't contain an error + // message. This could either be because the response was empty, or + // the response was valid JSON, but not with the expected schema + // ([types.ErrorResponse]). + // + // We cannot use "strict" JSON handling (json.NewDecoder with DisallowUnknownFields) + // due to the API using an open schema (we must anticipate fields + // being added to [types.ErrorResponse] in the future, and not + // reject those responses. + // + // For these cases, we construct an error with the status-code + // returned, but we could consider returning (a truncated version + // of) the actual response as-is. + // + // TODO(thaJeztah): consider adding a log.Debug to allow clients to debug the actual response when enabling debug logging. + daemonErr = fmt.Errorf(`API returned a %d (%s) but provided no error-message`, + serverResp.statusCode, + http.StatusText(serverResp.statusCode), + ) + } else { + daemonErr = errors.New(strings.TrimSpace(errorResponse.Message)) + } } else { + // Fall back to returning the response as-is for API versions < 1.24 + // that didn't support JSON error responses, and for situations + // where a plain text error is returned. This branch may also catch + // situations where a proxy is involved, returning a HTML response. daemonErr = errors.New(strings.TrimSpace(string(body))) } return errors.Wrap(daemonErr, "Error response from daemon") diff --git a/client/request_test.go b/client/request_test.go index 91d4b4b499609..42896cd78cecd 100644 --- a/client/request_test.go +++ b/client/request_test.go @@ -154,11 +154,10 @@ func TestResponseErrors(t *testing.T) { }, { // Server response that's valid JSON, but not the expected (types.ErrorResponse) scheme - // TODO(thaJeztah): consider returning (partial) raw response for these doc: "incorrect JSON scheme", contentType: "application/json", response: `{"error":"Some error occurred"}`, - expected: `Error response from daemon: `, + expected: `Error response from daemon: API returned a 400 (Bad Request) but provided no error-message`, }, { // TODO(thaJeztah): improve handling of such errors; we can return the generic "502 Bad Gateway" instead