api/pre-1.44: Default `ReadOnlyNonRecursive` to true by vvoland · Pull Request #47391 · moby/moby

vvoland · 2024-02-15T13:29:36Z

closes: Disable upgrading RO mounts to RRO when a client speaks API older than v1.44 (Docker v25) #47355

Don't change the behavior for older clients and keep the same behavior.

- How to verify it
TestContainerBindMountReadOnlyDefault

- Description for the changelog

- To preserve backwards compatibility, read-only mounts are not recursive by default when using older clients (API version < v1.44).

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

vvoland · 2024-02-15T13:38:17Z

api/server/router/container/container_routes.go

 					return errdefs.InvalidParameter(errors.New("BindOptions.ReadOnlyForceRecursive needs API v1.44 or newer"))
+				} else {
+					bo = &mount.BindOptions{}
+					hostConfig.Mounts[idx].BindOptions = bo


Note: This is needed, because m.BindOptions = ... would only overwrite the BindOptions in the m variable which is a local copy of the original mount and not a reference to the Mount in hostConfig.Mounts.

vvoland · 2024-02-15T13:52:27Z

Failure related, test needs adjustment:

=== Failed
=== FAIL: amd64.integration-cli TestDockerCLIRunSuite/TestRunMount (0.08s)
    docker_cli_run_test.go:4396: assertion failed: err is not nil: got error while creating a container with [--mount type=bind,src=/tmp/mount1181042325/mnt1,dst=/foo --mount type=bind,src=/tmp/mount1181042325/mnt2,dst=/bar] (mount-0-0)
    --- FAIL: TestDockerCLIRunSuite/TestRunMount (0.08s)

EDIT: Oh, actually it's not the test, ReadOnlyNonRecursive was set true also for non-readonly mounts which made it error out. Changed the default to only apply for m.ReadOnly mounts.

api/server/router/container/container_routes.go

AkihiroSuda

Thanks

vvoland · 2024-02-16T16:16:47Z

Looks like some failures are related:

      Command:  /usr/local/cli-integration/docker run -d -v /vol1 -v /tmp/test-put-container-archive-err-symlink-in-volume-to-read-only-rootfs-1579682919:/vol2 -v /tmp/test-put-container-archive-err-symlink-in-volume-to-read-only-rootfs-1579682919/vol3:/vol3 -v /tmp/test-put-container-archive-err-symlink-in-volume-to-read-only-rootfs-1579682919/vol_ro:/vol_ro:ro --read-only busybox /bin/sh -c #(nop)
        ExitCode: 125
        Error:    exit status 125
        Stdout:   
        Stderr:   /usr/local/cli-integration/docker: Error response from daemon: invalid mount config for type "bind": bind source path does not exist: /tmp/test-put-container-archive-err-symlink-in-volume-to-read-only-rootfs-1579682919/vol_ro.
        See '/usr/local/cli-integration/docker run --help'.

Parsing added by this PR doesn't return-early in case of error, so not sure yet why that happens..

api/server/router/container/container_routes.go

thaJeztah · 2024-02-19T13:59:54Z

api/server/router/container/container_routes.go

+					continue
 				}
 			}
+			newBinds = append(newBinds, b)


Shouldn't this clear the Binds field after they've been migrated?

Also wondering if this migration should always happen, so also on current API versions 🤔

This doesn't migrate all Binds (yet), only the read-only ones to reduce potential blast area.

Was planning to make a separate PR that would also deprecate the Binds field and transform all of them to Mounts.

vvoland · 2024-02-20T11:29:04Z

This looks green now, failures are unrelated and are caused by Docker Hub search API being down.

api/server/router/container/container_routes.go

api/types/backend/backend.go

daemon/create.go

cpuguy83 · 2024-02-22T23:51:43Z

integration/container/mounts_linux_test.go

 	assert.Equal(t, mountsBefore, mountsAfter)
 }

+func TestContainerBindMountReadOnlyDefault(t *testing.T) {


I think we need a to check if rro is supported on the kernel for the test to function properly.

Alternatively, we could just check the registered mountpoints in the API to see if the option is set as expected.

Also sorry, one more check, test should be skipped if api version is < 1.44.

Why? It also tests that the behavior doesn't change when <1.44 API is requested.

So yes technically the 1.43 test should pass on an older daemon.
But the 1.44 test would fail since it is not a supported version.

1.44 test will be skipped: 0f8270b#diff-0ac47068153ec59b77b2044bd459fb6395f028e6e112dc03645f7ba72f324bf2R456

But yeah the version: "" would fail, updated it to also be skipped.

Also, added the test for api.MinSupportedAPIVersion client version.

Don't error out when mount source doesn't exist and mounts has `CreateMountpoint` option enabled. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

Don't change the behavior for older clients and keep the same behavior. Otherwise client can't opt-out (because `ReadOnlyNonRecursive` is unsupported before 1.44). Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

thaJeztah

LGTM, thanks!

vvoland added area/api API status/2-code-review impact/api impact/changelog process/cherry-pick kind/bugfix PR's that fix bugs process/cherry-pick/25.0 labels Feb 15, 2024

vvoland added this to the 26.0.0 milestone Feb 15, 2024

vvoland self-assigned this Feb 15, 2024

vvoland force-pushed the rro-backwards-compatible branch from cb8306a to a995be2 Compare February 15, 2024 13:31

vvoland commented Feb 15, 2024

View reviewed changes

vvoland force-pushed the rro-backwards-compatible branch from a995be2 to 16bafea Compare February 15, 2024 13:58

vvoland mentioned this pull request Feb 15, 2024

[25.0 backport] api/pre-1.44: Default ReadOnlyNonRecursive to true #47393

Merged

vvoland added process/cherry-picked and removed process/cherry-pick process/cherry-pick/25.0 labels Feb 15, 2024

AkihiroSuda reviewed Feb 15, 2024

View reviewed changes

api/server/router/container/container_routes.go Outdated Show resolved Hide resolved

vvoland force-pushed the rro-backwards-compatible branch from 16bafea to c247ce3 Compare February 16, 2024 14:42

AkihiroSuda approved these changes Feb 16, 2024

View reviewed changes

vvoland force-pushed the rro-backwards-compatible branch from c247ce3 to c12c8b0 Compare February 16, 2024 14:48

vvoland force-pushed the rro-backwards-compatible branch 2 times, most recently from 6694464 to 8057434 Compare February 19, 2024 13:04

thaJeztah reviewed Feb 19, 2024

View reviewed changes

vvoland force-pushed the rro-backwards-compatible branch 3 times, most recently from f4db0f9 to 1f7ccf9 Compare February 20, 2024 10:47

cpuguy83 requested changes Feb 20, 2024

View reviewed changes

api/server/router/container/container_routes.go Outdated Show resolved Hide resolved

vvoland force-pushed the rro-backwards-compatible branch from 1f7ccf9 to 020b64e Compare February 21, 2024 10:10

cpuguy83 requested changes Feb 21, 2024

View reviewed changes

api/types/backend/backend.go Outdated Show resolved Hide resolved

daemon/create.go Outdated Show resolved Hide resolved

vvoland force-pushed the rro-backwards-compatible branch 2 times, most recently from 8539542 to 6e21f36 Compare February 22, 2024 10:23

vvoland requested a review from cpuguy83 February 22, 2024 19:22

cpuguy83 requested changes Feb 22, 2024

View reviewed changes

mounts/validate: Don't check source exists with CreateMountpoint

05b883b

Don't error out when mount source doesn't exist and mounts has `CreateMountpoint` option enabled. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the rro-backwards-compatible branch from 6e21f36 to 0f8270b Compare February 23, 2024 10:21

api/pre-1.44: Default ReadOnlyNonRecursive to true

4323903

Don't change the behavior for older clients and keep the same behavior. Otherwise client can't opt-out (because `ReadOnlyNonRecursive` is unsupported before 1.44). Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the rro-backwards-compatible branch from 0f8270b to 4323903 Compare February 26, 2024 10:37

cpuguy83 approved these changes Feb 26, 2024

View reviewed changes

thaJeztah approved these changes Feb 27, 2024

View reviewed changes

thaJeztah merged commit f6fa6ff into moby:master Feb 27, 2024

This was referenced Apr 30, 2024

[23.0] mounts/validate: Don't check source exists with CreateMountpoint #47780

Merged

[24.0] mounts/validate: Don't check source exists with CreateMountpoint #47779

Merged

Conversation

vvoland commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vvoland Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

vvoland commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

vvoland Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

vvoland commented Feb 20, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpuguy83 Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

vvoland Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

vvoland Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

vvoland commented Feb 15, 2024 •

edited

Loading

vvoland Feb 15, 2024 •

edited

Loading

vvoland commented Feb 15, 2024 •

edited

Loading

vvoland commented Feb 16, 2024 •

edited

Loading

vvoland Feb 26, 2024 •

edited

Loading