From a4d5b6b4d081fadfe933e49bd4d71d8c91ffa06f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Tue, 16 Apr 2024 13:45:12 +0200 Subject: [PATCH 1/2] builder/normalizeWorkdir: Always return cleaned path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `normalizeWorkdir` function has two branches, one that returns a result of `filepath.Join` which always returns a cleaned path, and another one where the input string is returned unmodified. To make these two outputs consistent, also clean the path in the second branch. This also makes the cleaning of the container workdir explicit in the `normalizeWorkdir` function instead of relying on the `SetupWorkingDirectory` to mutate it. Signed-off-by: Paweł Gronowski --- builder/dockerfile/dispatchers_unix.go | 2 +- integration-cli/docker_cli_build_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/builder/dockerfile/dispatchers_unix.go b/builder/dockerfile/dispatchers_unix.go index ba8e1d9053..d69e184220 100644 --- a/builder/dockerfile/dispatchers_unix.go +++ b/builder/dockerfile/dispatchers_unix.go @@ -22,7 +22,7 @@ func normalizeWorkdir(_ string, current string, requested string) (string, error if !filepath.IsAbs(requested) { return filepath.Join(string(os.PathSeparator), current, requested), nil } - return requested, nil + return filepath.Clean(requested), nil } // resolveCmdLine takes a command line arg set and optionally prepends a platform-specific diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index 858d496b40..05f9e4b8af 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -4296,7 +4296,7 @@ func (s *DockerCLIBuildSuite) TestBuildBuildTimeArgExpansion(c *testing.T) { imgName := "bldvarstest" wdVar := "WDIR" - wdVal := "/tmp/" + wdVal := "/tmp" addVar := "AFILE" addVal := "addFile" copyVar := "CFILE" From 7532420f3b4f7c62fb6c0de3c92b24ad91c380a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Tue, 16 Apr 2024 13:42:56 +0200 Subject: [PATCH 2/2] container/SetupWorkingDirectory: Don't mutate config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don't mutate the container's `Config.WorkingDir` permanently with a cleaned path when creating a working directory. Move the `filepath.Clean` to the `translateWorkingDir` instead. Signed-off-by: Paweł Gronowski --- container/container.go | 4 +- daemon/container.go | 2 +- integration/build/build_test.go | 66 +++++++++++++++++++++++++ integration/container/run_linux_test.go | 30 +++++++++++ 4 files changed, 99 insertions(+), 3 deletions(-) diff --git a/container/container.go b/container/container.go index 018300350d..9809124d8e 100644 --- a/container/container.go +++ b/container/container.go @@ -299,8 +299,8 @@ func (container *Container) SetupWorkingDirectory(rootIdentity idtools.Identity) return nil } - container.Config.WorkingDir = filepath.Clean(container.Config.WorkingDir) - pth, err := container.GetResourcePath(container.Config.WorkingDir) + workdir := filepath.Clean(container.Config.WorkingDir) + pth, err := container.GetResourcePath(workdir) if err != nil { return err } diff --git a/daemon/container.go b/daemon/container.go index c86ecf8c98..f8df74e4be 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -364,6 +364,6 @@ func translateWorkingDir(config *containertypes.Config) error { if !system.IsAbs(wd) { return fmt.Errorf("the working directory '%s' is invalid, it needs to be an absolute path", config.WorkingDir) } - config.WorkingDir = wd + config.WorkingDir = filepath.Clean(wd) return nil } diff --git a/integration/build/build_test.go b/integration/build/build_test.go index 826f4d5eb4..8d3da03a96 100644 --- a/integration/build/build_test.go +++ b/integration/build/build_test.go @@ -689,6 +689,72 @@ func TestBuildPlatformInvalid(t *testing.T) { assert.Check(t, is.ErrorType(err, errdefs.IsInvalidParameter)) } +// TestBuildWorkdirNoCacheMiss is a regression test for https://github.com/moby/moby/issues/47627 +func TestBuildWorkdirNoCacheMiss(t *testing.T) { + ctx := setupTest(t) + + for _, tc := range []struct { + name string + dockerfile string + }{ + {name: "trailing slash", dockerfile: "FROM busybox\nWORKDIR /foo/"}, + {name: "no trailing slash", dockerfile: "FROM busybox\nWORKDIR /foo"}, + } { + dockerfile := tc.dockerfile + t.Run(tc.name, func(t *testing.T) { + source := fakecontext.New(t, "", fakecontext.WithDockerfile(dockerfile)) + defer source.Close() + + apiClient := testEnv.APIClient() + + buildAndGetID := func() string { + resp, err := apiClient.ImageBuild(ctx, source.AsTarReader(t), types.ImageBuildOptions{ + Version: types.BuilderV1, + }) + assert.NilError(t, err) + defer resp.Body.Close() + + id := readBuildImageIDs(t, resp.Body) + assert.Check(t, id != "") + return id + } + + firstId := buildAndGetID() + secondId := buildAndGetID() + + assert.Check(t, is.Equal(firstId, secondId), "expected cache to be used") + }) + } +} + +func readBuildImageIDs(t *testing.T, rd io.Reader) string { + t.Helper() + decoder := json.NewDecoder(rd) + for { + var jm jsonmessage.JSONMessage + if err := decoder.Decode(&jm); err != nil { + if err == io.EOF { + break + } + assert.NilError(t, err) + } + + if jm.Aux == nil { + continue + } + + var auxId struct { + ID string `json:"ID"` + } + + if json.Unmarshal(*jm.Aux, &auxId); auxId.ID != "" { + return auxId.ID + } + } + + return "" +} + func writeTarRecord(t *testing.T, w *tar.Writer, fn, contents string) { err := w.WriteHeader(&tar.Header{ Name: fn, diff --git a/integration/container/run_linux_test.go b/integration/container/run_linux_test.go index 810ede184b..867544705d 100644 --- a/integration/container/run_linux_test.go +++ b/integration/container/run_linux_test.go @@ -326,3 +326,33 @@ func TestStaticIPOutsideSubpool(t *testing.T) { assert.Check(t, is.Contains(b.String(), "inet 10.42.1.3/16")) } + +func TestWorkingDirNormalization(t *testing.T) { + ctx := setupTest(t) + apiClient := testEnv.APIClient() + + for _, tc := range []struct { + name string + workdir string + }{ + {name: "trailing slash", workdir: "/tmp/"}, + {name: "no trailing slash", workdir: "/tmp"}, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + cID := container.Run(ctx, t, apiClient, + container.WithImage("busybox"), + container.WithWorkingDir(tc.workdir), + ) + + defer container.Remove(ctx, t, apiClient, cID, containertypes.RemoveOptions{Force: true}) + + inspect := container.Inspect(ctx, t, apiClient, cID) + + assert.Check(t, is.Equal(inspect.Config.WorkingDir, "/tmp")) + }) + + } +}