Merge pull request #4316 from AkihiroSuda/rro

mount: add `bind-recursive=<bool|string>` and deprecate `bind-nonrecursive=<bool>`

cli/command/container/opts.go

@@ -14,13 +14,13 @@ import (
 	"time"
 
 	cdi "github.com/container-orchestrated-devices/container-device-interface/pkg/parser"
+	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/compose/loader"
 	"github.com/docker/cli/opts"
 	"github.com/docker/docker/api/types/container"
 	mounttypes "github.com/docker/docker/api/types/mount"
 	networktypes "github.com/docker/docker/api/types/network"
 	"github.com/docker/docker/api/types/strslice"
-	"github.com/docker/docker/api/types/versions"
 	"github.com/docker/docker/errdefs"
 	"github.com/docker/go-connections/nat"
 	"github.com/pkg/errors"
@@ -1119,8 +1119,8 @@ func validateAttach(val string) (string, error) {
 
 func validateAPIVersion(c *containerConfig, serverAPIVersion string) error {
 	for _, m := range c.HostConfig.Mounts {
-		if m.BindOptions != nil && m.BindOptions.NonRecursive && versions.LessThan(serverAPIVersion, "1.40") {
-			return errors.Errorf("bind-nonrecursive requires API v1.40 or later")
+		if err := command.ValidateMountWithAPIVersion(m, serverAPIVersion); err != nil {
+			return err
 		}
 	}
 	return nil

cli/command/service/opts.go

@@ -8,11 +8,11 @@ import (
 	"strings"
 	"time"
 
+	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/opts"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/swarm"
-	"github.com/docker/docker/api/types/versions"
 	"github.com/docker/docker/client"
 	gogotypes "github.com/gogo/protobuf/types"
 	"github.com/google/shlex"
@@ -1043,8 +1043,8 @@ const (
 
 func validateAPIVersion(c swarm.ServiceSpec, serverAPIVersion string) error {
 	for _, m := range c.TaskTemplate.ContainerSpec.Mounts {
-		if m.BindOptions != nil && m.BindOptions.NonRecursive && versions.LessThan(serverAPIVersion, "1.40") {
-			return errors.Errorf("bind-nonrecursive requires API v1.40 or later")
+		if err := command.ValidateMountWithAPIVersion(m, serverAPIVersion); err != nil {
+			return err
 		}
 	}
 	return nil

cli/command/utils.go

@@ -11,6 +11,8 @@ import (
 
 	"github.com/docker/cli/cli/streams"
 	"github.com/docker/docker/api/types/filters"
+	mounttypes "github.com/docker/docker/api/types/mount"
+	"github.com/docker/docker/api/types/versions"
 	"github.com/moby/sys/sequential"
 	"github.com/pkg/errors"
 	"github.com/spf13/pflag"
@@ -195,3 +197,17 @@ func StringSliceReplaceAt(s, old, new []string, requireIndex int) ([]string, boo
 	out = append(out, s[idx+len(old):]...)
 	return out, true
 }
+
+// ValidateMountWithAPIVersion validates a mount with the server API version.
+func ValidateMountWithAPIVersion(m mounttypes.Mount, serverAPIVersion string) error {
+	if m.BindOptions != nil {
+		if m.BindOptions.NonRecursive && versions.LessThan(serverAPIVersion, "1.40") {
+			return errors.Errorf("bind-recursive=disabled requires API v1.40 or later")
+		}
+		// ReadOnlyNonRecursive can be safely ignored when API < 1.44
+		if m.BindOptions.ReadOnlyForceRecursive && versions.LessThan(serverAPIVersion, "1.44") {
+			return errors.Errorf("bind-recursive=readonly requires API v1.44 or later")
+		}
+	}
+	return nil
+}

opts/mount.go

@@ -2,6 +2,7 @@ package opts
 
 import (
 	"encoding/csv"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -10,6 +11,7 @@ import (
 
 	mounttypes "github.com/docker/docker/api/types/mount"
 	"github.com/docker/go-units"
+	"github.com/sirupsen/logrus"
 )
 
 // MountOpt is a Value type for parsing mounts
@@ -112,6 +114,32 @@ func (m *MountOpt) Set(value string) error {
 			if err != nil {
 				return fmt.Errorf("invalid value for %s: %s", key, val)
 			}
+			logrus.Warn("bind-nonrecursive is deprecated, use bind-recursive=disabled instead")
+		case "bind-recursive":
+			valS := val
+			// Allow boolean as an alias to "enabled" or "disabled"
+			if b, err := strconv.ParseBool(valS); err == nil {
+				if b {
+					valS = "enabled"
+				} else {
+					valS = "disabled"
+				}
+			}
+			switch valS {
+			case "enabled": // read-only mounts are recursively read-only if Engine >= v25 && kernel >= v5.12, otherwise writable
+				// NOP
+			case "disabled": // alias of bind-nonrecursive=true
+				bindOptions().NonRecursive = true
+			case "writable": // conforms to the default read-only bind-mount of Docker v24; read-only mounts are recursively mounted but not recursively read-only
+				bindOptions().ReadOnlyNonRecursive = true
+			case "readonly": // force recursively read-only, or raise an error
+				bindOptions().ReadOnlyForceRecursive = true
+				// TODO: implicitly set propagation and error if the user specifies a propagation in a future refactor/UX polish pass
+				// https://github.com/docker/cli/pull/4316#discussion_r1341974730
+			default:
+				return fmt.Errorf("invalid value for %s: %s (must be \"enabled\", \"disabled\", \"writable\", or \"readonly\")",
+					key, val)
+			}
 		case "volume-nocopy":
 			volumeOptions().NoCopy, err = strconv.ParseBool(val)
 			if err != nil {
@@ -161,6 +189,22 @@ func (m *MountOpt) Set(value string) error {
 		return fmt.Errorf("cannot mix 'tmpfs-*' options with mount type '%s'", mount.Type)
 	}
 
+	if mount.BindOptions != nil {
+		if mount.BindOptions.ReadOnlyNonRecursive {
+			if !mount.ReadOnly {
+				return errors.New("option 'bind-recursive=writable' requires 'readonly' to be specified in conjunction")
+			}
+		}
+		if mount.BindOptions.ReadOnlyForceRecursive {
+			if !mount.ReadOnly {
+				return errors.New("option 'bind-recursive=readonly' requires 'readonly' to be specified in conjunction")
+			}
+			if mount.BindOptions.Propagation != mounttypes.PropagationRPrivate {
+				return errors.New("option 'bind-recursive=readonly' requires 'bind-propagation=rprivate' to be specified in conjunction")
+			}
+		}
+	}
+
 	m.values = append(m.values, mount)
 	return nil
 }

opts/mount_test.go

@@ -217,3 +217,86 @@ func TestMountOptSetTmpfsError(t *testing.T) {
 	assert.ErrorContains(t, m.Set("type=tmpfs,target=/foo,tmpfs-mode=foo"), "invalid value for tmpfs-mode")
 	assert.ErrorContains(t, m.Set("type=tmpfs"), "target is required")
 }
+
+func TestMountOptSetBindNonRecursive(t *testing.T) {
+	var mount MountOpt
+	assert.NilError(t, mount.Set("type=bind,source=/foo,target=/bar,bind-nonrecursive"))
+	assert.Check(t, is.DeepEqual([]mounttypes.Mount{
+		{
+			Type:   mounttypes.TypeBind,
+			Source: "/foo",
+			Target: "/bar",
+			BindOptions: &mounttypes.BindOptions{
+				NonRecursive: true,
+			},
+		},
+	}, mount.Value()))
+}
+
+func TestMountOptSetBindRecursive(t *testing.T) {
+	t.Run("enabled", func(t *testing.T) {
+		var mount MountOpt
+		assert.NilError(t, mount.Set("type=bind,source=/foo,target=/bar,bind-recursive=enabled"))
+		assert.Check(t, is.DeepEqual([]mounttypes.Mount{
+			{
+				Type:   mounttypes.TypeBind,
+				Source: "/foo",
+				Target: "/bar",
+			},
+		}, mount.Value()))
+	})
+
+	t.Run("disabled", func(t *testing.T) {
+		var mount MountOpt
+		assert.NilError(t, mount.Set("type=bind,source=/foo,target=/bar,bind-recursive=disabled"))
+		assert.Check(t, is.DeepEqual([]mounttypes.Mount{
+			{
+				Type:   mounttypes.TypeBind,
+				Source: "/foo",
+				Target: "/bar",
+				BindOptions: &mounttypes.BindOptions{
+					NonRecursive: true,
+				},
+			},
+		}, mount.Value()))
+	})
+
+	t.Run("writable", func(t *testing.T) {
+		var mount MountOpt
+		assert.Error(t, mount.Set("type=bind,source=/foo,target=/bar,bind-recursive=writable"),
+			"option 'bind-recursive=writable' requires 'readonly' to be specified in conjunction")
+		assert.NilError(t, mount.Set("type=bind,source=/foo,target=/bar,bind-recursive=writable,readonly"))
+		assert.Check(t, is.DeepEqual([]mounttypes.Mount{
+			{
+				Type:     mounttypes.TypeBind,
+				Source:   "/foo",
+				Target:   "/bar",
+				ReadOnly: true,
+				BindOptions: &mounttypes.BindOptions{
+					ReadOnlyNonRecursive: true,
+				},
+			},
+		}, mount.Value()))
+	})
+
+	t.Run("readonly", func(t *testing.T) {
+		var mount MountOpt
+		assert.Error(t, mount.Set("type=bind,source=/foo,target=/bar,bind-recursive=readonly"),
+			"option 'bind-recursive=readonly' requires 'readonly' to be specified in conjunction")
+		assert.Error(t, mount.Set("type=bind,source=/foo,target=/bar,bind-recursive=readonly,readonly"),
+			"option 'bind-recursive=readonly' requires 'bind-propagation=rprivate' to be specified in conjunction")
+		assert.NilError(t, mount.Set("type=bind,source=/foo,target=/bar,bind-recursive=readonly,readonly,bind-propagation=rprivate"))
+		assert.Check(t, is.DeepEqual([]mounttypes.Mount{
+			{
+				Type:     mounttypes.TypeBind,
+				Source:   "/foo",
+				Target:   "/bar",
+				ReadOnly: true,
+				BindOptions: &mounttypes.BindOptions{
+					ReadOnlyForceRecursive: true,
+					Propagation:            mounttypes.PropagationRPrivate,
+				},
+			},
+		}, mount.Value()))
+	})
+}