-
Notifications
You must be signed in to change notification settings - Fork 2.1k
opts: MountOpt: improve validation, and refactor #6773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+200
−127
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
e888a6e
opts: remove outdated comment
thaJeztah 9620e41
opts: MountOpt: improve validation for whitespace in options
thaJeztah 5de99e6
opts: MountOpt: improve validation for whitespace in values
thaJeztah fe1af92
opts: MountOpt: improve validation of boolean values
thaJeztah d781df8
opts: MountOpt: extract validation to a separate function
thaJeztah df3e923
opts: MountOpt: extract utility functions and don't set empty values
thaJeztah File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,97 +77,67 @@ 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": | ||
| switch val { | ||
| 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) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message format is inconsistent with newly added error messages. The new validation code uses quotes around the key (e.g., "invalid value for '%s'"), but this existing error message doesn't use quotes. Consider updating to match the new format: "invalid value for '%s': %s"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I had some changes stashed to align these; probably fine for a follow-up