builder: Fix `WORKDIR` with a trailing slash causing a cache miss by vvoland · Pull Request #47723 · moby/moby

vvoland · 2024-04-16T12:26:08Z

closes: WORKDIR is not cached by legacy builder with trailing slash #47627

- What I did

- How I did it
Removed SetupWorkingDirectory side effect (mutating the container Config).

- How to verify it
TestBuildWorkdirNoCacheMiss

- Description for the changelog

classic builder: Fix cache miss on `WORKDIR <directory>/` build step (workdir with a trailing slash).

- A picture of a cute animal (not mandatory but encouraged)

container/container.go

coolljt0725

LGTM

laurazard

LGTM

thaJeztah

LGTM; left a small comment about t.Helper() and a tiny nit (both not hard blockers)

thaJeztah · 2024-04-19T10:05:30Z

integration/build/build_test.go

+// Test for #47627
+func TestBuildWorkdirNoCacheMiss(t *testing.T) {


Really minor nit here, that it can be more convenient to put a full link to the ticket here, and perhaps to describe the original issue (so that it may not be needed at all to refer to github. I guess "technically" this comment should also follow // TestBuildWorkdirNoCacheMiss .... GoDoc format (but I don't think we have linters verifying GoDoc on tests)

Not a blocker; just if you still want to adjust before merging.

thaJeztah · 2024-04-19T10:06:51Z

integration/build/build_test.go

+		{name: "trailing slash", dockerfile: "FROM busybox\nWORKDIR /foo/"},
+		{name: "no trailing slash", dockerfile: "FROM busybox\nWORKDIR /foo"},


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)

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.

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 .. 🤷)

thaJeztah · 2024-04-19T10:07:28Z

integration/build/build_test.go

+func readBuildImageIDs(t *testing.T, rd io.Reader) string {
+	decoder := json.NewDecoder(rd)


This should probably be a t.Helper() ?

We should look if we need proper helpers for build tests; ISTR there's various implementations for doing these kind of things (and it's admittedly awkward to test these, as you need to handle the progress stream 😞)

I guess also somewhat related to;

client: make streaming functions easier to consume #46528

Tbh, instead of working on proper helpers, I'd rather just work on a better API.. 😅

Tbh, instead of working on proper helpers, I'd rather just work on a better API.. 😅

Yup, agreed, and I think we may have to start working on that, also in light of improvements in output / progress with multi-arch etc.

That said; we can't change old API versions, so we'd have to outweigh the amount of work needed to provide more decent helpers for the existing bits, or to fully ignore those, and only focus on "next".

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 <[email protected]>

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 <[email protected]>

thaJeztah

LGTM, thanks!

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [docker/docker](https://togithub.com/docker/docker) | minor | `26.0.2` -> `26.1.0` | --- ### Release Notes <details> <summary>docker/docker (docker/docker)</summary> ### [`v26.1.0`](https://togithub.com/moby/moby/releases/tag/v26.1.0) [Compare Source](https://togithub.com/docker/docker/compare/v26.0.2...v26.1.0) #### 26.1.0 For a full list of pull requests and changes in this release, refer to the relevant GitHub milestones: - [docker/cli, 26.1.0 milestone](https://togithub.com/docker/cli/issues?q=is%3Aclosed+milestone%3A26.1.0) - [moby/moby, 26.1.0 milestone](https://togithub.com/moby/moby/issues?q=is%3Aclosed+milestone%3A26.1.0) - Deprecated and removed features, see [Deprecated Features](https://togithub.com/docker/cli/blob/v26.1.0/docs/deprecated.md). - Changes to the Engine API, see [API version history](https://togithub.com/moby/moby/blob/v26.1.0/docs/api/version-history.md). ##### New - Add configurable OpenTelemetry utilities and basic instrumentation to commands. For more information, see [OpenTelemetry for the Docker CLI](https://docs.docker.com/config/otel). [docker/cli#4889](https://togithub.com/docker/cli/pull/4889) ##### Bug fixes and enhancements - Native Windows containers are configured with an internal DNS server for container name resolution, and external DNS servers for other lookups. Not all resolvers, including `nslookup`, fall back to the external resolvers when they get a `SERVFAIL` answer from the internal server. So, the internal DNS server can now be configured to forward requests to the external resolvers, by setting `"features": {"windows-dns-proxy": true }` in the `daemon.json` file. [moby/moby#47584](https://togithub.com/moby/moby/pull/47584) > \[!NOTE] > This will be the new default behavior in Docker Engine 27.0. > \[!WARNING] > The `windows-dns-proxy` feature flag will be removed in a future release. - Swarm: Fix `Subpath` not being passed to the container config. [moby/moby#47711](https://togithub.com/moby/moby/pull/47711) - Classic builder: Fix cache miss on `WORKDIR <directory>/` build step (directory with a trailing slash). [moby/moby#47723](https://togithub.com/moby/moby/pull/47723) - containerd image store: Fix `docker images` failing when any image in the store has unexpected target. [moby/moby#47738](https://togithub.com/moby/moby/pull/47738) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6am on monday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/earthly/dind).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: idodod <[email protected]>

vvoland added area/builder Build status/2-code-review kind/bugfix PR's that fix bugs area/builder/classic-builder Build process/cherry-pick/26.0 labels Apr 16, 2024

vvoland added this to the 26.1.0 milestone Apr 16, 2024

vvoland self-assigned this Apr 16, 2024

coolljt0725 reviewed Apr 17, 2024

View reviewed changes

container/container.go Show resolved Hide resolved

vvoland force-pushed the builder-fix-workdir-slash branch from b106d63 to 2fcb3ed Compare April 17, 2024 09:59

vvoland requested a review from tonistiigi as a code owner April 17, 2024 09:59

vvoland force-pushed the builder-fix-workdir-slash branch 2 times, most recently from 8c7bbde to 217d538 Compare April 18, 2024 10:05

coolljt0725 approved these changes Apr 19, 2024

View reviewed changes

laurazard approved these changes Apr 19, 2024

View reviewed changes

thaJeztah approved these changes Apr 19, 2024

View reviewed changes

vvoland force-pushed the builder-fix-workdir-slash branch 2 times, most recently from 8843125 to c02e2eb Compare April 19, 2024 10:41

vvoland added 2 commits April 19, 2024 12:42

container/SetupWorkingDirectory: Don't mutate config

7532420

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 <[email protected]>

vvoland force-pushed the builder-fix-workdir-slash branch from c02e2eb to 7532420 Compare April 19, 2024 10:42

thaJeztah approved these changes Apr 19, 2024

View reviewed changes

vvoland merged commit 96c9353 into moby:master Apr 19, 2024

thaJeztah removed the process/cherry-pick/26.0 label Jun 8, 2024

		// Test for #47627
		func TestBuildWorkdirNoCacheMiss(t *testing.T) {

		{name: "trailing slash", dockerfile: "FROM busybox\nWORKDIR /foo/"},
		{name: "no trailing slash", dockerfile: "FROM busybox\nWORKDIR /foo"},

		func readBuildImageIDs(t *testing.T, rd io.Reader) string {
		decoder := json.NewDecoder(rd)

Conversation

vvoland commented Apr 16, 2024

Uh oh!

Uh oh!

coolljt0725 left a comment

Choose a reason for hiding this comment

Uh oh!

laurazard left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants