Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

builder: Fix WORKDIR with a trailing slash causing a cache miss #47723

Merged
merged 2 commits into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion builder/dockerfile/dispatchers_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions container/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,8 @@ func (container *Container) SetupWorkingDirectory(rootIdentity idtools.Identity)
return nil
}

container.Config.WorkingDir = filepath.Clean(container.Config.WorkingDir)
coolljt0725 marked this conversation as resolved.
Show resolved Hide resolved
pth, err := container.GetResourcePath(container.Config.WorkingDir)
workdir := filepath.Clean(container.Config.WorkingDir)
pth, err := container.GetResourcePath(workdir)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion daemon/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
2 changes: 1 addition & 1 deletion integration-cli/docker_cli_build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
66 changes: 66 additions & 0 deletions integration/build/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Comment on lines +700 to +701
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment / thinking out loud; wondering how we do cache-matching on Windows, where directories are case-insensitive 🤔 (not sure if we should care.. it would be a corner-case at most, mostly curious)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think mixing the case sensitivity will produce a cache miss on Windows.
But that's out of scope of this PR, and probably not a too big deal, as we mostly care about caching the same Dockerfile builds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, for sure! No need to spend time on that, unless it'd be an actual issue (but if we don't support it, it's likely been like that for years already, so .. 🤷)

} {
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,
Expand Down
30 changes: 30 additions & 0 deletions integration/container/run_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
})

}
}