Merge pull request #47393 from vvoland/rro-backwards-compatible-25

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

api/server/router/container/container_routes.go

@@ -602,17 +602,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"))
 				}
 			}
@@ -644,12 +654,13 @@ 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,
-		AdjustCPUShares:  adjustCPUShares,
-		Platform:         platform,
+		Name:                        name,
+		Config:                      config,
+		HostConfig:                  hostConfig,
+		NetworkingConfig:            networkingConfig,
+		AdjustCPUShares:             adjustCPUShares,
+		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,12 +13,13 @@ 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
-	AdjustCPUShares  bool
+	Name                        string
+	Config                      *container.Config
+	HostConfig                  *container.HostConfig
+	NetworkingConfig            *network.NetworkingConfig
+	Platform                    *ocispec.Platform
+	AdjustCPUShares             bool
+	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

@@ -218,7 +218,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/start.go

@@ -68,7 +68,7 @@ func (daemon *Daemon) ContainerStart(ctx context.Context, name string, hostConfi
 			if err := daemon.mergeAndVerifyLogConfig(&hostConfig.LogConfig); err != nil {
 				return errdefs.InvalidParameter(err)
 			}
-			if err := daemon.setHostConfig(ctr, hostConfig); err != nil {
+			if err := daemon.setHostConfig(ctr, hostConfig, true); err != nil {
 				return errdefs.InvalidParameter(err)
 			}
 			newNetworkMode := ctr.HostConfig.NetworkMode

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,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 !mp.RW && 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,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: "1.24", 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 +522,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 +576,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)
+}

integration/internal/container/container.go

@@ -170,3 +170,28 @@ func Inspect(ctx context.Context, t *testing.T, apiClient client.APIClient, cont
 
 	return c
 }
+
+type ContainerOutput struct {
+	Stdout, Stderr string
+}
+
+// Output waits for the container to end running and returns its output.
+func Output(ctx context.Context, client client.APIClient, id string) (ContainerOutput, error) {
+	logs, err := client.ContainerLogs(ctx, id, container.LogsOptions{Follow: true, ShowStdout: true, ShowStderr: true})
+	if err != nil {
+		return ContainerOutput{}, err
+	}
+
+	defer logs.Close()
+
+	var stdoutBuf, stderrBuf bytes.Buffer
+	_, err = stdcopy.StdCopy(&stdoutBuf, &stderrBuf, logs)
+	if err != nil {
+		return ContainerOutput{}, err
+	}
+
+	return ContainerOutput{
+		Stdout: stdoutBuf.String(),
+		Stderr: stderrBuf.String(),
+	}, nil
+}

volume/mounts/linux_parser.go

@@ -85,7 +85,9 @@ func (p *linuxParser) validateMountConfigImpl(mnt *mount.Mount, validateBindSour
 			if err != nil {
 				return &errMountConfig{mnt, err}
 			}
-			if !exists {
+
+			createMountpoint := mnt.BindOptions != nil && mnt.BindOptions.CreateMountpoint
+			if !exists && !createMountpoint {
 				return &errMountConfig{mnt, errBindSourceDoesNotExist(mnt.Source)}
 			}
 		}