Merge pull request #44739 from nicks/nicks/invalid-character

client: improve error messaging on crash

client/container_wait.go

@@ -1,14 +1,19 @@
 package client // import "github.com/docker/docker/client"
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
+	"io"
 	"net/url"
 
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/versions"
 )
 
+const containerWaitErrorMsgLimit = 2 * 1024 /* Max: 2KiB */
+
 // ContainerWait waits until the specified container is in a certain state
 // indicated by the given condition, either "not-running" (default),
 // "next-exit", or "removed".
@@ -46,9 +51,23 @@ func (cli *Client) ContainerWait(ctx context.Context, containerID string, condit
 
 	go func() {
 		defer ensureReaderClosed(resp)
+
+		body := resp.body
+		responseText := bytes.NewBuffer(nil)
+		stream := io.TeeReader(body, responseText)
+
 		var res container.WaitResponse
-		if err := json.NewDecoder(resp.body).Decode(&res); err != nil {
-			errC <- err
+		if err := json.NewDecoder(stream).Decode(&res); err != nil {
+			// NOTE(nicks): The /wait API does not work well with HTTP proxies.
+			// At any time, the proxy could cut off the response stream.
+			//
+			// But because the HTTP status has already been written, the proxy's
+			// only option is to write a plaintext error message.
+			//
+			// If there's a JSON parsing error, read the real error message
+			// off the body and send it to the client.
+			_, _ = io.ReadAll(io.LimitReader(stream, containerWaitErrorMsgLimit))
+			errC <- errors.New(responseText.String())
 			return
 		}
 

client/container_wait_test.go

@@ -62,6 +62,61 @@ func TestContainerWait(t *testing.T) {
 	}
 }
 
+func TestContainerWaitProxyInterrupt(t *testing.T) {
+	expectedURL := "/v1.30/containers/container_id/wait"
+	msg := "copying response body from Docker: unexpected EOF"
+	client := &Client{
+		version: "1.30",
+		client: newMockClient(func(req *http.Request) (*http.Response, error) {
+			if !strings.HasPrefix(req.URL.Path, expectedURL) {
+				return nil, fmt.Errorf("Expected URL '%s', got '%s'", expectedURL, req.URL)
+			}
+			return &http.Response{
+				StatusCode: http.StatusOK,
+				Body:       io.NopCloser(strings.NewReader(msg)),
+			}, nil
+		}),
+	}
+
+	resultC, errC := client.ContainerWait(context.Background(), "container_id", "")
+	select {
+	case err := <-errC:
+		if !strings.Contains(err.Error(), msg) {
+			t.Fatalf("Expected: %s, Actual: %s", msg, err.Error())
+		}
+	case result := <-resultC:
+		t.Fatalf("Unexpected result: %v", result)
+	}
+}
+
+func TestContainerWaitProxyInterruptLong(t *testing.T) {
+	expectedURL := "/v1.30/containers/container_id/wait"
+	msg := strings.Repeat("x", containerWaitErrorMsgLimit*5)
+	client := &Client{
+		version: "1.30",
+		client: newMockClient(func(req *http.Request) (*http.Response, error) {
+			if !strings.HasPrefix(req.URL.Path, expectedURL) {
+				return nil, fmt.Errorf("Expected URL '%s', got '%s'", expectedURL, req.URL)
+			}
+			return &http.Response{
+				StatusCode: http.StatusOK,
+				Body:       io.NopCloser(strings.NewReader(msg)),
+			}, nil
+		}),
+	}
+
+	resultC, errC := client.ContainerWait(context.Background(), "container_id", "")
+	select {
+	case err := <-errC:
+		// LimitReader limiting isn't exact, because of how the Readers do chunking.
+		if len(err.Error()) > containerWaitErrorMsgLimit*2 {
+			t.Fatalf("Expected error to be limited around %d, actual length: %d", containerWaitErrorMsgLimit, len(err.Error()))
+		}
+	case result := <-resultC:
+		t.Fatalf("Unexpected result: %v", result)
+	}
+}
+
 func ExampleClient_ContainerWait_withTimeout() {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()