api/pre-1.44: Default ReadOnlyNonRecursive to true

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>

api/server/router/container/container_routes.go

@@ -556,17 +556,27 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo
 		hostConfig.Annotations = nil
 	}
 
+	defaultReadOnlyNonRecursive := false
 	if versions.LessThan(version, "1.44") {
 		if config.Healthcheck != nil {
 			// StartInterval was added in API 1.44
 			config.Healthcheck.StartInterval = 0
 		}
 
+		// Set ReadOnlyNonRecursive to true because it was added in API 1.44
+		// Before that all read-only mounts were non-recursive.
+		// Keep that behavior for clients on older APIs.
+		defaultReadOnlyNonRecursive = true
+
 		for _, m := range hostConfig.Mounts {
-			if m.BindOptions != nil {
-				// Ignore ReadOnlyNonRecursive because it was added in API 1.44.
-				m.BindOptions.ReadOnlyNonRecursive = false
-				if m.BindOptions.ReadOnlyForceRecursive {
+			if m.Type == mount.TypeBind {
+				if m.BindOptions != nil && m.BindOptions.ReadOnlyForceRecursive {
+					// NOTE: that technically this is a breaking change for older
+					// API versions, and we should ignore the new field.
+					// However, this option may be incorrectly set by a client with
+					// the expectation that the failing to apply recursive read-only
+					// is enforced, so we decided to produce an error instead,
+					// instead of silently ignoring.
 					return errdefs.InvalidParameter(errors.New("BindOptions.ReadOnlyForceRecursive needs API v1.44 or newer"))
 				}
 			}
@@ -611,6 +621,7 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo
 		HostConfig:                  hostConfig,
 		NetworkingConfig:            networkingConfig,
 		Platform:                    platform,
+		DefaultReadOnlyNonRecursive: defaultReadOnlyNonRecursive,
 	})
 	if err != nil {
 		return err

api/swagger.yaml

@@ -391,7 +391,11 @@ definitions:
           ReadOnlyNonRecursive:
             description: |
                Make the mount non-recursively read-only, but still leave the mount recursive
-               (unless NonRecursive is set to true in conjunction).
+               (unless NonRecursive is set to `true` in conjunction).
+
+               Addded in v1.44, before that version all read-only mounts were
+               non-recursive by default. To match the previous behaviour this
+               will default to `true` for clients on versions prior to v1.44.
             type: "boolean"
             default: false
           ReadOnlyForceRecursive:

api/types/backend/backend.go

@@ -18,6 +18,7 @@ type ContainerCreateConfig struct {
 	HostConfig                  *container.HostConfig
 	NetworkingConfig            *network.NetworkingConfig
 	Platform                    *ocispec.Platform
+	DefaultReadOnlyNonRecursive bool
 }
 
 // ContainerRmConfig holds arguments for the container remove

daemon/container.go

@@ -203,10 +203,10 @@ func (daemon *Daemon) setSecurityOptions(cfg *config.Config, container *containe
 	return daemon.parseSecurityOpt(cfg, &container.SecurityOptions, hostConfig)
 }
 
-func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig *containertypes.HostConfig) error {
+func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig *containertypes.HostConfig, defaultReadOnlyNonRecursive bool) error {
 	// Do not lock while creating volumes since this could be calling out to external plugins
 	// Don't want to block other actions, like `docker ps` because we're waiting on an external plugin
-	if err := daemon.registerMountPoints(container, hostConfig); err != nil {
+	if err := daemon.registerMountPoints(container, hostConfig, defaultReadOnlyNonRecursive); err != nil {
 		return err
 	}
 

daemon/create.go

@@ -217,7 +217,7 @@ func (daemon *Daemon) create(ctx context.Context, daemonCfg *config.Config, opts
 		return nil, err
 	}
 
-	if err := daemon.setHostConfig(ctr, opts.params.HostConfig); err != nil {
+	if err := daemon.setHostConfig(ctr, opts.params.HostConfig, opts.params.DefaultReadOnlyNonRecursive); err != nil {
 		return nil, err
 	}
 

daemon/volumes.go

@@ -54,7 +54,7 @@ func (m mounts) parts(i int) int {
 // 2. Select the volumes mounted from another containers. Overrides previously configured mount point destination.
 // 3. Select the bind mounts set by the client. Overrides previously configured mount point destinations.
 // 4. Cleanup old volumes that are about to be reassigned.
-func (daemon *Daemon) registerMountPoints(container *container.Container, hostConfig *containertypes.HostConfig) (retErr error) {
+func (daemon *Daemon) registerMountPoints(container *container.Container, hostConfig *containertypes.HostConfig, defaultReadOnlyNonRecursive bool) (retErr error) {
 	binds := map[string]bool{}
 	mountPoints := map[string]*volumemounts.MountPoint{}
 	parser := volumemounts.NewParser()
@@ -158,6 +158,15 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
 			}
 		}
 
+		if bind.Type == mount.TypeBind && !bind.RW {
+			if defaultReadOnlyNonRecursive {
+				if bind.Spec.BindOptions == nil {
+					bind.Spec.BindOptions = &mounttypes.BindOptions{}
+				}
+				bind.Spec.BindOptions.ReadOnlyNonRecursive = true
+			}
+		}
+
 		binds[bind.Destination] = true
 		dereferenceIfExists(bind.Destination)
 		mountPoints[bind.Destination] = bind
@@ -212,10 +221,19 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
 			}
 		}
 
-		if mp.Type == mounttypes.TypeBind && (cfg.BindOptions == nil || !cfg.BindOptions.CreateMountpoint) {
+		if mp.Type == mounttypes.TypeBind {
+			if cfg.BindOptions == nil || !cfg.BindOptions.CreateMountpoint {
 				mp.SkipMountpointCreation = true
 			}
 
+			if !mp.RW && defaultReadOnlyNonRecursive {
+				if mp.Spec.BindOptions == nil {
+					mp.Spec.BindOptions = &mounttypes.BindOptions{}
+				}
+				mp.Spec.BindOptions.ReadOnlyNonRecursive = true
+			}
+		}
+
 		binds[mp.Destination] = true
 		dereferenceIfExists(mp.Destination)
 		mountPoints[mp.Destination] = mp

integration-cli/docker_cli_inspect_test.go

@@ -175,15 +175,22 @@ func (s *DockerCLIInspectSuite) TestInspectContainerFilterInt(c *testing.T) {
 }
 
 func (s *DockerCLIInspectSuite) TestInspectBindMountPoint(c *testing.T) {
-	modifier := ",z"
 	prefix, slash := getPrefixAndSlashFromDaemonPlatform()
+	mopt := prefix + slash + "data:" + prefix + slash + "data"
+
+	mode := ""
 	if testEnv.DaemonInfo.OSType == "windows" {
-		modifier = ""
 		// Linux creates the host directory if it doesn't exist. Windows does not.
 		os.Mkdir(`c:\data`, os.ModeDir)
+	} else {
+		mode = "z" // Relabel
 	}
 
-	cli.DockerCmd(c, "run", "-d", "--name", "test", "-v", prefix+slash+"data:"+prefix+slash+"data:ro"+modifier, "busybox", "cat")
+	if mode != "" {
+		mopt += ":" + mode
+	}
+
+	cli.DockerCmd(c, "run", "-d", "--name", "test", "-v", mopt, "busybox", "cat")
 
 	vol := inspectFieldJSON(c, "test", "Mounts")
 
@@ -200,10 +207,8 @@ func (s *DockerCLIInspectSuite) TestInspectBindMountPoint(c *testing.T) {
 	assert.Equal(c, m.Driver, "")
 	assert.Equal(c, m.Source, prefix+slash+"data")
 	assert.Equal(c, m.Destination, prefix+slash+"data")
-	if testEnv.DaemonInfo.OSType != "windows" { // Windows does not set mode
-		assert.Equal(c, m.Mode, "ro"+modifier)
-	}
-	assert.Equal(c, m.RW, false)
+	assert.Equal(c, m.Mode, mode)
+	assert.Equal(c, m.RW, true)
 }
 
 func (s *DockerCLIInspectSuite) TestInspectNamedMountPoint(c *testing.T) {

integration/container/mounts_linux_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/docker/docker/api"
 	containertypes "github.com/docker/docker/api/types/container"
 	mounttypes "github.com/docker/docker/api/types/mount"
 	"github.com/docker/docker/api/types/network"
@@ -426,6 +427,78 @@ func TestContainerCopyLeaksMounts(t *testing.T) {
 	assert.Equal(t, mountsBefore, mountsAfter)
 }
 
+func TestContainerBindMountReadOnlyDefault(t *testing.T) {
+	skip.If(t, testEnv.IsRemoteDaemon)
+	skip.If(t, !isRROSupported(), "requires recursive read-only mounts")
+
+	ctx := setupTest(t)
+
+	// The test will run a container with a simple readonly /dev bind mount (-v /dev:/dev:ro)
+	// It will then check /proc/self/mountinfo for the mount type of /dev/shm (submount of /dev)
+	// If /dev/shm is rw, that will mean that the read-only mounts are NOT recursive by default.
+	const nonRecursive = " /dev/shm rw,"
+	// If /dev/shm is ro, that will mean that the read-only mounts ARE recursive by default.
+	const recursive = " /dev/shm ro,"
+
+	for _, tc := range []struct {
+		clientVersion string
+		expectedOut   string
+		name          string
+	}{
+		{clientVersion: "", expectedOut: recursive, name: "latest should be the same as 1.44"},
+		{clientVersion: "1.44", expectedOut: recursive, name: "submount should be recursive by default on 1.44"},
+
+		{clientVersion: "1.43", expectedOut: nonRecursive, name: "older than 1.44 should be non-recursive by default"},
+
+		// TODO: Remove when MinSupportedAPIVersion >= 1.44
+		{clientVersion: api.MinSupportedAPIVersion, expectedOut: nonRecursive, name: "minimum API should be non-recursive by default"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			apiClient := testEnv.APIClient()
+
+			minDaemonVersion := tc.clientVersion
+			if minDaemonVersion == "" {
+				minDaemonVersion = "1.44"
+			}
+			skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), minDaemonVersion), "requires API v"+minDaemonVersion)
+
+			if tc.clientVersion != "" {
+				c, err := client.NewClientWithOpts(client.FromEnv, client.WithVersion(tc.clientVersion))
+				assert.NilError(t, err, "failed to create client with version v%s", tc.clientVersion)
+				apiClient = c
+			}
+
+			for _, tc2 := range []struct {
+				subname  string
+				mountOpt func(*container.TestContainerConfig)
+			}{
+				{"mount", container.WithMount(mounttypes.Mount{
+					Type:     mounttypes.TypeBind,
+					Source:   "/dev",
+					Target:   "/dev",
+					ReadOnly: true,
+				})},
+				{"bind mount", container.WithBindRaw("/dev:/dev:ro")},
+			} {
+				t.Run(tc2.subname, func(t *testing.T) {
+					cid := container.Run(ctx, t, apiClient, tc2.mountOpt,
+						container.WithCmd("sh", "-c", "grep /dev/shm /proc/self/mountinfo"),
+					)
+					out, err := container.Output(ctx, apiClient, cid)
+					assert.NilError(t, err)
+
+					assert.Check(t, is.Equal(out.Stderr, ""))
+					// Output should be either:
+					// 545 526 0:160 / /dev/shm ro,nosuid,nodev,noexec,relatime shared:90 - tmpfs shm rw,size=65536k
+					// or
+					// 545 526 0:160 / /dev/shm rw,nosuid,nodev,noexec,relatime shared:90 - tmpfs shm rw,size=65536k
+					assert.Check(t, is.Contains(out.Stdout, tc.expectedOut))
+				})
+			}
+		})
+	}
+}
+
 func TestContainerBindMountRecursivelyReadOnly(t *testing.T) {
 	skip.If(t, testEnv.IsRemoteDaemon)
 	skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.44"), "requires API v1.44")
@@ -450,7 +523,7 @@ func TestContainerBindMountRecursivelyReadOnly(t *testing.T) {
 		}
 	}()
 
-	rroSupported := kernel.CheckKernelVersion(5, 12, 0)
+	rroSupported := isRROSupported()
 
 	nonRecursiveVerifier := []string{`/bin/sh`, `-xc`, `touch /foo/mnt/file; [ $? = 0 ]`}
 	forceRecursiveVerifier := []string{`/bin/sh`, `-xc`, `touch /foo/mnt/file; [ $? != 0 ]`}
@@ -504,3 +577,7 @@ func TestContainerBindMountRecursivelyReadOnly(t *testing.T) {
 		poll.WaitOn(t, container.IsSuccessful(ctx, apiClient, c), poll.WithDelay(100*time.Millisecond))
 	}
 }
+
+func isRROSupported() bool {
+	return kernel.CheckKernelVersion(5, 12, 0)
+}