Did a quick check if the read-only would potentially cause issues when propagating the content;

Created an image with content existing at the destination;

docker build -t foo -<<'EOF'
FROM alpine
RUN mkdir -p /data/dest && echo "hello" > /data/dest/file.txt
EOF

docker run --rm --mount type=volume,dst=/data/dest,readonly -w /data/dest foo sh -c 'ls -l && echo "foo" > new-file.txt'
total 4
-rw-r--r--    1 root     root             6 Dec 11 11:20 file.txt
sh: can't create new-file.txt: Read-only file system

Also checking --volumes-from;

docker run -dit --rm --name container1 --mount type=volume,dst=/data/dest,readonly -w /data/dest foo

docker run -it --volumes-from container1 -w /data/dest alpine sh -c 'ls -l && echo "foo" > new-file.txt'
total 4
-rw-r--r--    1 root     root             6 Dec 11 11:20 file.txt
sh: can't create new-file.txt: Read-only file system

There's one issue we should look at, and whether we want to support or not; the shorthand -v / --volume notation (<hostpath>:<containerpath>[:<option>...]) notation currently expects either 2 components (<hostpath>:<containerpath>), or three (<hostpath>:<containerpath>:<options>). Currently it fails if only 2 components are passed; likely it attempts to use ro as destination-path, and refusing it, because that's required to be an absolute path (orthogonal, but the error message could probably be improved to be more clear on that).

docker run -it --rm -v /data/dest:ro -w /data/dest foo
docker: Error response from daemon: invalid volume specification: '/data/dest:ro'

When specifying a source (myvolume), it does work;

docker run -it --rm -v myvolume:/data/dest:ro -w /data/dest foo

In general, it's OK for the advanced (--mount) flag to provide more features than the short-hand; we mostly froze the shorthand syntax, because the colon-separated format was problematic already (on Windows, the paths themselves would contain colons due to drive-letters (C:)), but in this case, I wonder if it's unexpected to not allow an option (ro / rw) that's otherwise supported by the shorthand.

That said; I guess that's already the case for other options, like SELinux labels (-v /data/dest:z), so maybe something we just need to document (use the advanced syntax for this)

docker run --rm -v myvolume:/data/dest:z -w /data/dest foo

docker run --rm -v data/dest:z -w /data/dest foo
docker: Error response from daemon: invalid volume specification: 'data/dest:nocopy'

We should add some tests for this though before merging; let me know if you need help with that/

Took a stab at some simple tests, adding Test*ValidateMounts to linux_parser_test.go and windows_parser_test.go. These tests just test mount.Mount validity. Let me know if I'm off track or if the tests should be expanded.

CI failure:

=== FAIL: daemon/volume/mounts TestLinuxValidateMounts (0.00s)
    linux_parser_test.go:336: assertion failed: error is not nil: invalid mount config for type "bind": bind source path does not exist: /tmp

@vvoland Really sorry if I'm just missing it (still getting used to the CI/test setup) but is there a chance that failure is from a previous version of the PR? I can't find that failure in the most recent results, and the test passes locally for me in the dev container:

root@9cf7f5178494:/go/src/github.com/docker/docker# TESTDIRS='./daemon/volume/mounts' TESTFLAGS='-test.run TestLinuxValidateMounts' ./hack/test/unit 2>&1 | tail -n 4
ok      github.com/moby/moby/v2/daemon/volume/mounts    (cached)        coverage: 9.4% of statements

DONE 1 tests in 0.000s
+ '[' -n '' ']'

At one point I had used a mockFiProvider in that test

        parser := NewLinuxParser()
+       if p, ok := parser.(*linuxParser); ok {
+               p.fi = mockFiProvider{}
+       }

and if I add that back it fails with the bind source path does not exist: /tmp message, but that mockFiProvider has been removed from TestLinuxValidateMounts and is not in the current PR.

I gave CI a kick to run

One failure; looks like it may be related;

=== Failed
=== FAIL: daemon/volume/mounts TestLinuxValidateMounts (0.00s)
    linux_parser_test.go:336: assertion failed: error is not nil: invalid mount config for type "bind": bind source path does not exist: /tmp

Thanks very much for the review and suggestions. I applied all of them, confirmed that tests pass locally for me as well, rebased against master, and pushed the changes. 🤞 on CI passing this time...

Seems likeTestLinuxValidateMounts needs to be configured to not run on Windows? Should I add something like below to the subtest logic, or is there a better way?

    if runtime.GOOS == "windows" {
      t.Skip("TODO: skip irrelevant Linux test on Windows")
    }

I've adjusted the tests (one is skipped on Windows).

It's a bit of a workaround, because the parser itself shouldn't care if the source directory exists or not... but it's unrelated to this change so let's not try to fix it here.