Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 1 commit into from
Oct 2, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cli/command/container/opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions cli/command/service/opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
AkihiroSuda marked this conversation as resolved.
Show resolved Hide resolved
return err
}
}
return nil
Expand Down
16 changes: 16 additions & 0 deletions cli/command/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the overhead is neglectable, so probably not worth doing; we could consider having a if versions.GreaterThanOrEqualTo(serverAPIVersion, "1.44") somewhere to skip the checks.

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
}
44 changes: 44 additions & 0 deletions opts/mount.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package opts

import (
"encoding/csv"
"errors"
"fmt"
"os"
"path/filepath"
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Comment on lines +119 to +128
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened a PR for discussion to remove the boolean variants (motivation outlined in that PR);

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 {
Expand Down Expand Up @@ -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
}
Expand Down
83 changes: 83 additions & 0 deletions opts/mount_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
})
}
Loading