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"))
 				}
 			}
@@ -606,11 +616,12 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo
 	}
 
 	ccr, err := s.backend.ContainerCreate(ctx, backend.ContainerCreateConfig{
-		Name:             name,
-		Config:           config,
-		HostConfig:       hostConfig,
-		NetworkingConfig: networkingConfig,
-		Platform:         platform,
+		Name:                        name,
+		Config:                      config,
+		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

@@ -13,11 +13,12 @@ import (
 
 // ContainerCreateConfig is the parameter set to ContainerCreate()
 type ContainerCreateConfig struct {
-	Name             string
-	Config           *container.Config
-	HostConfig       *container.HostConfig
-	NetworkingConfig *network.NetworkingConfig
-	Platform         *ocispec.Platform
+	Name                        string
+	Config                      *container.Config
+	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

@@ -12,6 +12,7 @@ import (
 	"github.com/docker/docker/api/types/backend"
 	containertypes "github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/events"
+	"github.com/docker/docker/api/types/mount"
 	networktypes "github.com/docker/docker/api/types/network"
 	"github.com/docker/docker/container"
 	"github.com/docker/docker/daemon/config"
@@ -21,6 +22,7 @@ import (
 	"github.com/docker/docker/internal/multierror"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/runconfig"
+	volumemounts "github.com/docker/docker/volume/mounts"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/opencontainers/selinux/go-selinux"
 	archvariant "github.com/tonistiigi/go-archvariant"
@@ -126,6 +128,41 @@ func (daemon *Daemon) containerCreate(ctx context.Context, daemonCfg *configStor
 	return containertypes.CreateResponse{ID: ctr.ID, Warnings: warnings}, nil
 }
 
+// transformBinds transforms read-only Binds into Mounts and enables
+// ReadOnlyNonRecursive by default.
+func transformBinds(hostConfig *containertypes.HostConfig) error {
+	var newBinds []string
+	parser := volumemounts.NewParser()
+	for _, b := range hostConfig.Binds {
+		if bind, err := parser.ParseMountRaw(b, hostConfig.VolumeDriver); err == nil {
+			relabel := false
+			for _, m := range strings.Split(bind.Mode, ",") {
+				if m == "z" || m == "Z" {
+					relabel = true
+					break
+				}
+			}
+
+			// There's no equivalent of z and Z mode in the long --mount option.
+			if !bind.RW && !relabel {
+				mnt := bind.Spec
+				if mnt.BindOptions == nil {
+					mnt.BindOptions = &mount.BindOptions{}
+				}
+				// This is default for the short bind syntax (-v src:dst).
+				mnt.BindOptions.CreateMountpoint = true
+
+				hostConfig.Mounts = append(hostConfig.Mounts, mnt)
+				continue
+			}
+		}
+		newBinds = append(newBinds, b)
+	}
+	hostConfig.Binds = newBinds
+
+	return nil
+}
+
 // Create creates a new container from the given configuration with a given name.
 func (daemon *Daemon) create(ctx context.Context, daemonCfg *config.Config, opts createOpts) (retC *container.Container, retErr error) {
 	var (
@@ -217,7 +254,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 {
+			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,8 +221,17 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
 			}
 		}
 
-		if mp.Type == mounttypes.TypeBind && (cfg.BindOptions == nil || !cfg.BindOptions.CreateMountpoint) {
-			mp.SkipMountpointCreation = true
+		if mp.Type == mounttypes.TypeBind {
+			if cfg.BindOptions == nil || !cfg.BindOptions.CreateMountpoint {
+				mp.SkipMountpointCreation = true
+			}
+
+			if defaultReadOnlyNonRecursive {
+				if mp.Spec.BindOptions == nil {
+					mp.Spec.BindOptions = &mounttypes.BindOptions{}
+				}
+				mp.Spec.BindOptions.ReadOnlyNonRecursive = true
+			}
 		}
 
 		binds[mp.Destination] = true

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
+	}
+
+	if mode != "" {
+		mopt += ":" + mode
 	}
 
-	cli.DockerCmd(c, "run", "-d", "--name", "test", "-v", prefix+slash+"data:"+prefix+slash+"data:ro"+modifier, "busybox", "cat")
+	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

@@ -426,6 +426,69 @@ func TestContainerCopyLeaksMounts(t *testing.T) {
 	assert.Equal(t, mountsBefore, mountsAfter)
 }
 
+func TestContainerBindMountReadOnlyDefault(t *testing.T) {
+	skip.If(t, testEnv.IsRemoteDaemon)
+
+	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 {
+		version     string
+		expectedOut string
+		name        string
+	}{
+		{version: "", expectedOut: recursive, name: "latest should be the same as 1.44"},
+		{version: "1.44", expectedOut: recursive, name: "submount should be recursive by default on 1.44"},
+
+		{version: "1.43", expectedOut: nonRecursive, name: "older should be non-recursive by default"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			apiClient := testEnv.APIClient()
+
+			if tc.version != "" {
+				skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), tc.version), "requires API v"+tc.version)
+				c, err := client.NewClientWithOpts(client.FromEnv, client.WithVersion(tc.version))
+				assert.NilError(t, err, "failed to create client with version v%s", tc.version)
+				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")