Merge pull request #6773 from thaJeztah/improve_mountopts

opts: MountOpt: improve validation, and refactor

opts/mount.go

@@ -33,66 +33,32 @@ func (m *MountOpt) Set(value string) error {
 		return err
 	}
 
-	mount := mounttypes.Mount{}
+	mount := mounttypes.Mount{
+		Type: mounttypes.TypeVolume, // default to volume mounts
+	}
 
-	volumeOptions := func() *mounttypes.VolumeOptions {
-		if mount.VolumeOptions == nil {
-			mount.VolumeOptions = &mounttypes.VolumeOptions{
-				Labels: make(map[string]string),
+	for _, field := range fields {
+		key, val, hasValue := strings.Cut(field, "=")
+		if k := strings.TrimSpace(key); k != key {
+			return fmt.Errorf("invalid option '%s' in '%s': option should not have whitespace", k, field)
+		}
+		if hasValue {
+			v := strings.TrimSpace(val)
+			if v == "" {
+				return fmt.Errorf("invalid value for '%s': value is empty", key)
+			}
+			if v != val {
+				return fmt.Errorf("invalid value for '%s' in '%s': value should not have whitespace", key, field)
 			}
 		}
-		if mount.VolumeOptions.DriverConfig == nil {
-			mount.VolumeOptions.DriverConfig = &mounttypes.Driver{}
-		}
-		return mount.VolumeOptions
-	}
-
-	imageOptions := func() *mounttypes.ImageOptions {
-		if mount.ImageOptions == nil {
-			mount.ImageOptions = new(mounttypes.ImageOptions)
-		}
-		return mount.ImageOptions
-	}
-
-	bindOptions := func() *mounttypes.BindOptions {
-		if mount.BindOptions == nil {
-			mount.BindOptions = new(mounttypes.BindOptions)
-		}
-		return mount.BindOptions
-	}
-
-	tmpfsOptions := func() *mounttypes.TmpfsOptions {
-		if mount.TmpfsOptions == nil {
-			mount.TmpfsOptions = new(mounttypes.TmpfsOptions)
-		}
-		return mount.TmpfsOptions
-	}
-
-	setValueOnMap := func(target map[string]string, value string) {
-		k, v, _ := strings.Cut(value, "=")
-		if k != "" {
-			target[k] = v
-		}
-	}
-
-	mount.Type = mounttypes.TypeVolume // default to volume mounts
-	// Set writable as the default
-	for _, field := range fields {
-		key, val, ok := strings.Cut(field, "=")
 
 		// TODO(thaJeztah): these options should not be case-insensitive.
 		key = strings.ToLower(key)
 
-		if !ok {
+		if !hasValue {
 			switch key {
-			case "readonly", "ro":
-				mount.ReadOnly = true
-				continue
-			case "volume-nocopy":
-				volumeOptions().NoCopy = true
-				continue
-			case "bind-nonrecursive":
-				return errors.New("bind-nonrecursive is deprecated, use bind-recursive=disabled instead")
+			case "readonly", "ro", "volume-nocopy", "bind-nonrecursive":
+				// boolean values
 			default:
 				return fmt.Errorf("invalid field '%s' must be a key=value pair", field)
 			}
@@ -111,14 +77,14 @@ func (m *MountOpt) Set(value string) error {
 		case "target", "dst", "destination":
 			mount.Target = val
 		case "readonly", "ro":
-			mount.ReadOnly, err = strconv.ParseBool(val)
+			mount.ReadOnly, err = parseBoolValue(key, val, hasValue)
 			if err != nil {
-				return fmt.Errorf("invalid value for %s: %s", key, val)
+				return err
 			}
 		case "consistency":
 			mount.Consistency = mounttypes.Consistency(strings.ToLower(val))
 		case "bind-propagation":
-			bindOptions().Propagation = mounttypes.Propagation(strings.ToLower(val))
+			ensureBindOptions(&mount).Propagation = mounttypes.Propagation(strings.ToLower(val))
 		case "bind-nonrecursive":
 			return errors.New("bind-nonrecursive is deprecated, use bind-recursive=disabled instead")
 		case "bind-recursive":
@@ -126,82 +92,52 @@ func (m *MountOpt) Set(value string) error {
 			case "enabled": // read-only mounts are recursively read-only if Engine >= v25 && kernel >= v5.12, otherwise writable
 				// NOP
 			case "disabled": // previously "bind-nonrecursive=true"
-				bindOptions().NonRecursive = true
+				ensureBindOptions(&mount).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
+				ensureBindOptions(&mount).ReadOnlyNonRecursive = true
 			case "readonly": // force recursively read-only, or raise an error
-				bindOptions().ReadOnlyForceRecursive = true
+				ensureBindOptions(&mount).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-subpath":
-			volumeOptions().Subpath = val
+			ensureVolumeOptions(&mount).Subpath = val
 		case "volume-nocopy":
-			volumeOptions().NoCopy, err = strconv.ParseBool(val)
+			ensureVolumeOptions(&mount).NoCopy, err = parseBoolValue(key, val, hasValue)
 			if err != nil {
-				return fmt.Errorf("invalid value for volume-nocopy: %s", val)
+				return err
 			}
 		case "volume-label":
-			setValueOnMap(volumeOptions().Labels, val)
+			volumeOpts := ensureVolumeOptions(&mount)
+			volumeOpts.Labels = setValueOnMap(volumeOpts.Labels, val)
 		case "volume-driver":
-			volumeOptions().DriverConfig.Name = val
+			ensureVolumeDriver(&mount).Name = val
 		case "volume-opt":
-			if volumeOptions().DriverConfig.Options == nil {
-				volumeOptions().DriverConfig.Options = make(map[string]string)
-			}
-			setValueOnMap(volumeOptions().DriverConfig.Options, val)
+			volumeDriver := ensureVolumeDriver(&mount)
+			volumeDriver.Options = setValueOnMap(volumeDriver.Options, val)
 		case "image-subpath":
-			imageOptions().Subpath = val
+			ensureImageOptions(&mount).Subpath = val
 		case "tmpfs-size":
 			sizeBytes, err := units.RAMInBytes(val)
 			if err != nil {
 				return fmt.Errorf("invalid value for %s: %s", key, val)
 			}
-			tmpfsOptions().SizeBytes = sizeBytes
+			ensureTmpfsOptions(&mount).SizeBytes = sizeBytes
 		case "tmpfs-mode":
 			ui64, err := strconv.ParseUint(val, 8, 32)
 			if err != nil {
 				return fmt.Errorf("invalid value for %s: %s", key, val)
 			}
-			tmpfsOptions().Mode = os.FileMode(ui64)
+			ensureTmpfsOptions(&mount).Mode = os.FileMode(ui64)
 		default:
-			return fmt.Errorf("unexpected key '%s' in '%s'", key, field)
+			return fmt.Errorf("unknown option '%s' in '%s'", key, field)
 		}
 	}
 
-	if mount.Type == "" {
-		return errors.New("type is required")
-	}
-
-	if mount.VolumeOptions != nil && mount.Type != mounttypes.TypeVolume {
-		return fmt.Errorf("cannot mix 'volume-*' options with mount type '%s'", mount.Type)
-	}
-	if mount.ImageOptions != nil && mount.Type != mounttypes.TypeImage {
-		return fmt.Errorf("cannot mix 'image-*' options with mount type '%s'", mount.Type)
-	}
-	if mount.BindOptions != nil && mount.Type != mounttypes.TypeBind {
-		return fmt.Errorf("cannot mix 'bind-*' options with mount type '%s'", mount.Type)
-	}
-	if mount.TmpfsOptions != nil && mount.Type != mounttypes.TypeTmpfs {
-		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")
-			}
-		}
+	if err := validateMountOptions(&mount); err != nil {
+		return err
 	}
 
 	m.values = append(m.values, mount)

opts/mount_test.go

@@ -116,17 +116,32 @@ func TestMountOptErrors(t *testing.T) {
 		{
 			doc:    "invalid key=value",
 			value:  "type=volume,target=/foo,bogus=foo",
-			expErr: "unexpected key 'bogus' in 'bogus=foo'",
+			expErr: "unknown option 'bogus' in 'bogus=foo'",
 		},
 		{
 			doc:    "invalid key with leading whitespace",
 			value:  "type=volume, src=/foo,target=/foo",
-			expErr: "unexpected key ' src' in ' src=/foo'",
+			expErr: "invalid option 'src' in ' src=/foo': option should not have whitespace",
 		},
 		{
 			doc:    "invalid key with trailing whitespace",
 			value:  "type=volume,src =/foo,target=/foo",
-			expErr: "unexpected key 'src ' in 'src =/foo'",
+			expErr: "invalid option 'src' in 'src =/foo': option should not have whitespace",
+		},
+		{
+			doc:    "invalid value is empty",
+			value:  "type=volume,src=,target=/foo",
+			expErr: "invalid value for 'src': value is empty",
+		},
+		{
+			doc:    "invalid value with leading whitespace",
+			value:  "type=volume,src= /foo,target=/foo",
+			expErr: "invalid value for 'src' in 'src= /foo': value should not have whitespace",
+		},
+		{
+			doc:    "invalid value with trailing whitespace",
+			value:  "type=volume,src=/foo ,target=/foo",
+			expErr: "invalid value for 'src' in 'src=/foo ': value should not have whitespace",
 		},
 		{
 			doc:    "missing value",
@@ -171,9 +186,9 @@ func TestMountOptReadOnly(t *testing.T) {
 	}{
 		{value: "", exp: false},
 		{value: "readonly", exp: true},
-		{value: "readonly=", expErr: `invalid value for readonly: `},
-		{value: "readonly= true", expErr: `invalid value for readonly:  true`},
-		{value: "readonly=no", expErr: `invalid value for readonly: no`},
+		{value: "readonly=", expErr: `invalid value for 'readonly': value is empty`},
+		{value: "readonly= true", expErr: `invalid value for 'readonly' in 'readonly= true': value should not have whitespace`},
+		{value: "readonly=no", expErr: `invalid value for 'readonly': invalid boolean value ("no"): must be one of "true", "1", "false", or "0" (default "true")`},
 		{value: "readonly=1", exp: true},
 		{value: "readonly=true", exp: true},
 		{value: "readonly=0", exp: false},
@@ -215,9 +230,9 @@ func TestMountOptVolumeNoCopy(t *testing.T) {
 	}{
 		{value: "", exp: false},
 		{value: "volume-nocopy", exp: true},
-		{value: "volume-nocopy=", expErr: `invalid value for volume-nocopy: `},
-		{value: "volume-nocopy= true", expErr: `invalid value for volume-nocopy:  true`},
-		{value: "volume-nocopy=no", expErr: `invalid value for volume-nocopy: no`},
+		{value: "volume-nocopy=", expErr: `invalid value for 'volume-nocopy': value is empty`},
+		{value: "volume-nocopy= true", expErr: `invalid value for 'volume-nocopy' in 'volume-nocopy= true': value should not have whitespace`},
+		{value: "volume-nocopy=no", expErr: `invalid value for 'volume-nocopy': invalid boolean value ("no"): must be one of "true", "1", "false", or "0" (default "true")`},
 		{value: "volume-nocopy=1", exp: true},
 		{value: "volume-nocopy=true", exp: true},
 		{value: "volume-nocopy=0", exp: false},
@@ -267,7 +282,6 @@ func TestMountOptVolumeOptions(t *testing.T) {
 					Labels: map[string]string{
 						"foo": "foo-value",
 					},
-					DriverConfig: &mount.Driver{},
 				},
 			},
 		},
@@ -282,7 +296,6 @@ func TestMountOptVolumeOptions(t *testing.T) {
 						"foo": "foo-value",
 						"bar": "bar-value",
 					},
-					DriverConfig: &mount.Driver{},
 				},
 			},
 		},
@@ -297,7 +310,6 @@ func TestMountOptVolumeOptions(t *testing.T) {
 						"foo": "",
 						"bar": "",
 					},
-					DriverConfig: &mount.Driver{},
 				},
 			},
 		},
@@ -306,12 +318,9 @@ func TestMountOptVolumeOptions(t *testing.T) {
 			doc:   "volume-label empty key",
 			value: `type=volume,target=/foo,volume-label==foo-value`,
 			exp: mount.Mount{
-				Type:   mount.TypeVolume,
-				Target: "/foo",
-				VolumeOptions: &mount.VolumeOptions{
-					Labels:       map[string]string{},
-					DriverConfig: &mount.Driver{},
-				},
+				Type:          mount.TypeVolume,
+				Target:        "/foo",
+				VolumeOptions: &mount.VolumeOptions{},
 			},
 		},
 		{
@@ -321,7 +330,6 @@ func TestMountOptVolumeOptions(t *testing.T) {
 				Type:   mount.TypeVolume,
 				Target: "/foo",
 				VolumeOptions: &mount.VolumeOptions{
-					Labels: map[string]string{},
 					DriverConfig: &mount.Driver{
 						Name: "my-driver",
 					},
@@ -335,7 +343,6 @@ func TestMountOptVolumeOptions(t *testing.T) {
 				Type:   mount.TypeVolume,
 				Target: "/foo",
 				VolumeOptions: &mount.VolumeOptions{
-					Labels: map[string]string{},
 					DriverConfig: &mount.Driver{
 						Options: map[string]string{
 							"foo": "foo-value",
@@ -351,7 +358,6 @@ func TestMountOptVolumeOptions(t *testing.T) {
 				Type:   mount.TypeVolume,
 				Target: "/foo",
 				VolumeOptions: &mount.VolumeOptions{
-					Labels: map[string]string{},
 					DriverConfig: &mount.Driver{
 						Options: map[string]string{
 							"foo": "foo-value",
@@ -368,7 +374,6 @@ func TestMountOptVolumeOptions(t *testing.T) {
 				Type:   mount.TypeVolume,
 				Target: "/foo",
 				VolumeOptions: &mount.VolumeOptions{
-					Labels: map[string]string{},
 					DriverConfig: &mount.Driver{
 						Options: map[string]string{
 							"foo": "",
@@ -386,10 +391,7 @@ func TestMountOptVolumeOptions(t *testing.T) {
 				Type:   mount.TypeVolume,
 				Target: "/foo",
 				VolumeOptions: &mount.VolumeOptions{
-					Labels: map[string]string{},
-					DriverConfig: &mount.Driver{
-						Options: map[string]string{},
-					},
+					DriverConfig: &mount.Driver{},
 				},
 			},
 		},

opts/mount_utils.go

@@ -0,0 +1,135 @@
+package opts
+
+import (
+	"errors"
+	"fmt"
+	"strings"
+
+	"github.com/moby/moby/api/types/mount"
+)
+
+// validateMountOptions performs client-side validation of mount options. Similar
+// validation happens on the daemon side, but this validation allows us to
+// produce user-friendly errors matching command-line options.
+func validateMountOptions(m *mount.Mount) error {
+	if err := validateExclusiveOptions(m); err != nil {
+		return err
+	}
+
+	if m.BindOptions != nil {
+		if m.BindOptions.ReadOnlyNonRecursive && !m.ReadOnly {
+			return errors.New("option 'bind-recursive=writable' requires 'readonly' to be specified in conjunction")
+		}
+		if m.BindOptions.ReadOnlyForceRecursive {
+			if !m.ReadOnly {
+				return errors.New("option 'bind-recursive=readonly' requires 'readonly' to be specified in conjunction")
+			}
+			if m.BindOptions.Propagation != mount.PropagationRPrivate {
+				// FIXME(thaJeztah): this is missing daemon-side validation
+				//
+				//	docker run --rm --mount type=bind,src=/var/run,target=/foo,bind-recursive=readonly,readonly alpine
+				//	# no error
+				return errors.New("option 'bind-recursive=readonly' requires 'bind-propagation=rprivate' to be specified in conjunction")
+			}
+		}
+	}
+
+	return nil
+}
+
+// validateExclusiveOptions checks if the given mount config only contains
+// options for the given mount-type.
+//
+// This is the client-side equivalent of [mounts.validateExclusiveOptions] in
+// the daemon, but with error-messages matching client-side flags / options.
+//
+// [mounts.validateExclusiveOptions]: https://github.com/moby/moby/blob/v2.0.0-beta.6/daemon/volume/mounts/validate.go#L31-L50
+func validateExclusiveOptions(m *mount.Mount) error {
+	if m.Type == "" {
+		return errors.New("type is required")
+	}
+
+	if m.Type != mount.TypeBind && m.BindOptions != nil {
+		return fmt.Errorf("cannot mix 'bind-*' options with mount type '%s'", m.Type)
+	}
+	if m.Type != mount.TypeVolume && m.VolumeOptions != nil {
+		return fmt.Errorf("cannot mix 'volume-*' options with mount type '%s'", m.Type)
+	}
+	if m.Type != mount.TypeImage && m.ImageOptions != nil {
+		return fmt.Errorf("cannot mix 'image-*' options with mount type '%s'", m.Type)
+	}
+	if m.Type != mount.TypeTmpfs && m.TmpfsOptions != nil {
+		return fmt.Errorf("cannot mix 'tmpfs-*' options with mount type '%s'", m.Type)
+	}
+	if m.Type != mount.TypeCluster && m.ClusterOptions != nil {
+		return fmt.Errorf("cannot mix 'cluster-*' options with mount type '%s'", m.Type)
+	}
+	return nil
+}
+
+// parseBoolValue returns the boolean value represented by the string. It returns
+// true if no value is set.
+//
+// It is similar to [strconv.ParseBool], but only accepts 1, true, 0, false.
+// Any other value returns an error.
+func parseBoolValue(key string, val string, hasValue bool) (bool, error) {
+	if !hasValue {
+		return true, nil
+	}
+	switch val {
+	case "1", "true":
+		return true, nil
+	case "0", "false":
+		return false, nil
+	default:
+		return false, fmt.Errorf(`invalid value for '%s': invalid boolean value (%q): must be one of "true", "1", "false", or "0" (default "true")`, key, val)
+	}
+}
+
+func ensureVolumeOptions(m *mount.Mount) *mount.VolumeOptions {
+	if m.VolumeOptions == nil {
+		m.VolumeOptions = &mount.VolumeOptions{}
+	}
+	return m.VolumeOptions
+}
+
+func ensureVolumeDriver(m *mount.Mount) *mount.Driver {
+	ensureVolumeOptions(m)
+	if m.VolumeOptions.DriverConfig == nil {
+		m.VolumeOptions.DriverConfig = &mount.Driver{}
+	}
+	return m.VolumeOptions.DriverConfig
+}
+
+func ensureImageOptions(m *mount.Mount) *mount.ImageOptions {
+	if m.ImageOptions == nil {
+		m.ImageOptions = &mount.ImageOptions{}
+	}
+	return m.ImageOptions
+}
+
+func ensureBindOptions(m *mount.Mount) *mount.BindOptions {
+	if m.BindOptions == nil {
+		m.BindOptions = &mount.BindOptions{}
+	}
+	return m.BindOptions
+}
+
+func ensureTmpfsOptions(m *mount.Mount) *mount.TmpfsOptions {
+	if m.TmpfsOptions == nil {
+		m.TmpfsOptions = &mount.TmpfsOptions{}
+	}
+	return m.TmpfsOptions
+}
+
+func setValueOnMap(target map[string]string, keyValue string) map[string]string {
+	k, v, _ := strings.Cut(keyValue, "=")
+	if k == "" {
+		return target
+	}
+	if target == nil {
+		target = map[string]string{}
+	}
+	target[k] = v
+	return target
+}