diff --git a/opts/mount.go b/opts/mount.go index a4b5744fee77..642f6500ab4c 100644 --- a/opts/mount.go +++ b/opts/mount.go @@ -33,66 +33,32 @@ func (m *MountOpt) Set(value string) error { return err } - mount := mounttypes.Mount{} - - volumeOptions := func() *mounttypes.VolumeOptions { - if mount.VolumeOptions == nil { - mount.VolumeOptions = &mounttypes.VolumeOptions{ - Labels: make(map[string]string), - } - } - if mount.VolumeOptions.DriverConfig == nil { - mount.VolumeOptions.DriverConfig = &mounttypes.Driver{} - } - return mount.VolumeOptions + mount := mounttypes.Mount{ + Type: mounttypes.TypeVolume, // default to volume mounts } - 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) + 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) } - return mount.TmpfsOptions - } - - setValueOnMap := func(target map[string]string, value string) { - k, v, _ := strings.Cut(value, "=") - if k != "" { - target[k] = v + 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) + } } - } - - 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) diff --git a/opts/mount_test.go b/opts/mount_test.go index cacba20b57df..9520948b9af1 100644 --- a/opts/mount_test.go +++ b/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{}, }, }, }, diff --git a/opts/mount_utils.go b/opts/mount_utils.go new file mode 100644 index 000000000000..974f54dc0405 --- /dev/null +++ b/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 +}