From 013d6488888d2362185b0738e9f600d4ba95d88a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 20 Mar 2022 17:13:04 +0100 Subject: [PATCH] client: CopyToContainer(), CopyFromContainer(): remove status-code handling This was added in 93c3e6c91ec5eb4202b86b44b011d06f5e048dab, at which time only some basic handling of non-succesful status codes was present; https://github.com/moby/moby/blob/93c3e6c91ec5eb4202b86b44b011d06f5e048dab/api/client/utils.go#L112-L121 Given that since 38e6d474affe08230043fa36fa02f623c2ab2661 non-successful status- codes are already handled, and a 204 ("no content") status should not be an error, this special case should no longer be needed. Signed-off-by: Sebastiaan van Stijn --- client/container_copy.go | 10 ------- client/container_copy_test.go | 35 ++++++++++++++++++------ integration/container/copy_test.go | 44 ++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 19 deletions(-) diff --git a/client/container_copy.go b/client/container_copy.go index d8b7bb6b91..883be7fa34 100644 --- a/client/container_copy.go +++ b/client/container_copy.go @@ -50,11 +50,6 @@ func (cli *Client) CopyToContainer(ctx context.Context, containerID, dstPath str return err } - // TODO this code converts non-error status-codes (e.g., "204 No Content") into an error; verify if this is the desired behavior - if response.statusCode != http.StatusOK { - return fmt.Errorf("unexpected status code from daemon: %d", response.statusCode) - } - return nil } @@ -70,11 +65,6 @@ func (cli *Client) CopyFromContainer(ctx context.Context, containerID, srcPath s return nil, types.ContainerPathStat{}, err } - // TODO this code converts non-error status-codes (e.g., "204 No Content") into an error; verify if this is the desired behavior - if response.statusCode != http.StatusOK { - return nil, types.ContainerPathStat{}, fmt.Errorf("unexpected status code from daemon: %d", response.statusCode) - } - // In order to get the copy behavior right, we need to know information // about both the source and the destination. The response headers include // stat info about the source that we can use in deciding exactly how to diff --git a/client/container_copy_test.go b/client/container_copy_test.go index c94aa5e1aa..d8ad5811dc 100644 --- a/client/container_copy_test.go +++ b/client/container_copy_test.go @@ -115,14 +115,15 @@ func TestCopyToContainerNotFoundError(t *testing.T) { } } -// TODO TestCopyToContainerNotStatusOKError expects a non-error status-code ("204 No Content") to produce an error; verify if this is the desired behavior -func TestCopyToContainerNotStatusOKError(t *testing.T) { +// TestCopyToContainerEmptyResponse verifies that no error is returned when a +// "204 No Content" is returned by the API. +func TestCopyToContainerEmptyResponse(t *testing.T) { client := &Client{ client: newMockClient(errorMock(http.StatusNoContent, "No content")), } err := client.CopyToContainer(context.Background(), "container_id", "path/to/file", bytes.NewReader([]byte("")), types.CopyToContainerOptions{}) - if err == nil || err.Error() != "unexpected status code from daemon: 204" { - t.Fatalf("expected an unexpected status code error, got %v", err) + if err != nil { + t.Fatalf("unexpected error: %v", err) } } @@ -192,14 +193,30 @@ func TestCopyFromContainerNotFoundError(t *testing.T) { } } -// TODO TestCopyFromContainerNotStatusOKError expects a non-error status-code ("204 No Content") to produce an error; verify if this is the desired behavior -func TestCopyFromContainerNotStatusOKError(t *testing.T) { +// TestCopyFromContainerEmptyResponse verifies that no error is returned when a +// "204 No Content" is returned by the API. +func TestCopyFromContainerEmptyResponse(t *testing.T) { client := &Client{ - client: newMockClient(errorMock(http.StatusNoContent, "No content")), + client: newMockClient(func(req *http.Request) (*http.Response, error) { + content, err := json.Marshal(types.ContainerPathStat{ + Name: "path/to/file", + Mode: 0700, + }) + if err != nil { + return nil, err + } + base64PathStat := base64.StdEncoding.EncodeToString(content) + return &http.Response{ + StatusCode: http.StatusNoContent, + Header: http.Header{ + "X-Docker-Container-Path-Stat": []string{base64PathStat}, + }, + }, nil + }), } _, _, err := client.CopyFromContainer(context.Background(), "container_id", "path/to/file") - if err == nil || err.Error() != "unexpected status code from daemon: 204" { - t.Fatalf("expected an unexpected status code error, got %v", err) + if err != nil { + t.Fatalf("unexpected error: %v", err) } } diff --git a/integration/container/copy_test.go b/integration/container/copy_test.go index e7405e638c..b50c1757ed 100644 --- a/integration/container/copy_test.go +++ b/integration/container/copy_test.go @@ -2,15 +2,18 @@ package container // import "github.com/docker/docker/integration/container" import ( "archive/tar" + "bytes" "context" "encoding/json" "io" "os" + "path/filepath" "testing" "github.com/docker/docker/api/types" "github.com/docker/docker/client" "github.com/docker/docker/integration/internal/container" + "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/jsonmessage" "github.com/docker/docker/testutil/fakecontext" "gotest.tools/v3/assert" @@ -59,6 +62,47 @@ func TestCopyToContainerPathDoesNotExist(t *testing.T) { assert.ErrorContains(t, err, "Could not find the file /dne in container "+cid) } +func TestCopyEmptyFile(t *testing.T) { + defer setupTest(t)() + + tmpDir := t.TempDir() + srcPath := filepath.Join(tmpDir, "empty-file.txt") + err := os.WriteFile(srcPath, []byte(""), 0400) + assert.NilError(t, err) + + // TODO(thaJeztah) Add utilities to the client to make steps below less complicated. + // Code below is taken from copyToContainer() in docker/cli. + srcInfo, err := archive.CopyInfoSourcePath(srcPath, false) + assert.NilError(t, err) + + srcArchive, err := archive.TarResource(srcInfo) + assert.NilError(t, err) + defer srcArchive.Close() + + ctrPath := "/empty-file.txt" + dstInfo := archive.CopyInfo{Path: ctrPath} + dstDir, preparedArchive, err := archive.PrepareArchiveCopy(srcArchive, srcInfo, dstInfo) + assert.NilError(t, err) + defer preparedArchive.Close() + + ctx := context.Background() + apiclient := testEnv.APIClient() + cid := container.Create(ctx, t, apiclient) + + // empty content + err = apiclient.CopyToContainer(ctx, cid, dstDir, bytes.NewReader([]byte("")), types.CopyToContainerOptions{}) + assert.NilError(t, err) + + // tar with empty file + err = apiclient.CopyToContainer(ctx, cid, dstDir, preparedArchive, types.CopyToContainerOptions{}) + assert.NilError(t, err) + + // copy from empty file + rdr, _, err := apiclient.CopyFromContainer(ctx, cid, dstDir) + assert.NilError(t, err) + defer rdr.Close() +} + func TestCopyToContainerPathIsNotDir(t *testing.T) { defer setupTest(t)()