Merge pull request #43632 from thaJeztah/volumes_fixup_part2

volume/local: refactor validation to allow invalidating early

volume/local/local.go

@@ -5,7 +5,6 @@ package local // import "github.com/docker/docker/volume/local"
 
 import (
 	"encoding/json"
-	"fmt"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -31,7 +30,7 @@ const (
 
 var (
 	// ErrNotFound is the typed error returned when the requested volume name can't be found
-	ErrNotFound = fmt.Errorf("volume not found")
+	ErrNotFound = errors.New("volume not found")
 	// volumeNameRegex ensures the name assigned for the volume is valid.
 	// This name is used to create the bind directory, so we need to avoid characters that
 	// would make the path to escape the root directory.
@@ -75,24 +74,19 @@ func New(scope string, rootIdentity idtools.Identity) (*Root, error) {
 		v := &localVolume{
 			driverName: r.Name(),
 			name:       name,
-			path:       r.DataPath(name),
+			rootPath:   filepath.Join(r.path, name),
+			path:       filepath.Join(r.path, name, volumeDataPathName),
 			quotaCtl:   r.quotaCtl,
 		}
-		r.volumes[name] = v
-		if b, err := os.ReadFile(filepath.Join(r.path, name, "opts.json")); err == nil {
-			opts := optsConfig{}
-			if err := json.Unmarshal(b, &opts); err != nil {
-				return nil, errors.Wrapf(err, "error while unmarshaling volume options for volume: %s", name)
-			}
-			// Make sure this isn't an empty optsConfig.
-			// This could be empty due to buggy behavior in older versions of Docker.
-			if !reflect.DeepEqual(opts, optsConfig{}) {
-				v.opts = &opts
-			}
-			// unmount anything that may still be mounted (for example, from an
-			// unclean shutdown). This is a no-op on windows
-			unmount(v.path)
+
+		// unmount anything that may still be mounted (for example, from an
+		// unclean shutdown). This is a no-op on windows
+		unmount(v.path)
+
+		if err := v.loadOpts(); err != nil {
+			return nil, err
 		}
+		r.volumes[name] = v
 	}
 
 	return r, nil
@@ -120,11 +114,6 @@ func (r *Root) List() ([]volume.Volume, error) {
 	return ls, nil
 }
 
-// DataPath returns the constructed path of this volume.
-func (r *Root) DataPath(volumeName string) string {
-	return filepath.Join(r.path, volumeName, volumeDataPathName)
-}
-
 // Name returns the name of Root, defined in the volume package in the DefaultDriverName constant.
 func (r *Root) Name() string {
 	return volume.DefaultDriverName
@@ -137,6 +126,9 @@ func (r *Root) Create(name string, opts map[string]string) (volume.Volume, error
 	if err := r.validateName(name); err != nil {
 		return nil, err
 	}
+	if err := r.validateOpts(opts); err != nil {
+		return nil, err
+	}
 
 	r.m.Lock()
 	defer r.m.Unlock()
@@ -146,44 +138,33 @@ func (r *Root) Create(name string, opts map[string]string) (volume.Volume, error
 		return v, nil
 	}
 
-	path := r.DataPath(name)
-	volRoot := filepath.Dir(path)
+	v = &localVolume{
+		driverName: r.Name(),
+		name:       name,
+		rootPath:   filepath.Join(r.path, name),
+		path:       filepath.Join(r.path, name, volumeDataPathName),
+		quotaCtl:   r.quotaCtl,
+	}
+
 	// Root dir does not need to be accessed by the remapped root
-	if err := idtools.MkdirAllAndChown(volRoot, 0701, idtools.CurrentIdentity()); err != nil {
-		return nil, errors.Wrapf(errdefs.System(err), "error while creating volume root path '%s'", volRoot)
+	if err := idtools.MkdirAllAndChown(v.rootPath, 0701, idtools.CurrentIdentity()); err != nil {
+		return nil, errors.Wrapf(errdefs.System(err), "error while creating volume root path '%s'", v.rootPath)
 	}
 
 	// Remapped root does need access to the data path
-	if err := idtools.MkdirAllAndChown(path, 0755, r.rootIdentity); err != nil {
-		return nil, errors.Wrapf(errdefs.System(err), "error while creating volume data path '%s'", path)
+	if err := idtools.MkdirAllAndChown(v.path, 0755, r.rootIdentity); err != nil {
+		return nil, errors.Wrapf(errdefs.System(err), "error while creating volume data path '%s'", v.path)
 	}
 
 	var err error
 	defer func() {
 		if err != nil {
-			os.RemoveAll(filepath.Dir(path))
+			os.RemoveAll(v.rootPath)
 		}
 	}()
 
-	v = &localVolume{
-		driverName: r.Name(),
-		name:       name,
-		path:       path,
-		quotaCtl:   r.quotaCtl,
-	}
-
-	if len(opts) != 0 {
-		if err = setOpts(v, opts); err != nil {
-			return nil, err
-		}
-		var b []byte
-		b, err = json.Marshal(v.opts)
-		if err != nil {
-			return nil, err
-		}
-		if err = os.WriteFile(filepath.Join(filepath.Dir(path), "opts.json"), b, 0600); err != nil {
-			return nil, errdefs.System(errors.Wrap(err, "error while persisting volume options"))
-		}
+	if err = v.setOpts(opts); err != nil {
+		return nil, err
 	}
 
 	r.volumes[name] = v
@@ -204,19 +185,22 @@ func (r *Root) Remove(v volume.Volume) error {
 	}
 
 	if lv.active.count > 0 {
-		return errdefs.System(errors.Errorf("volume has active mounts"))
+		return errdefs.System(errors.New("volume has active mounts"))
 	}
 
 	if err := lv.unmount(); err != nil {
 		return err
 	}
 
+	// TODO(thaJeztah) is there a reason we're evaluating the data-path here, and not the volume's rootPath?
 	realPath, err := filepath.EvalSymlinks(lv.path)
 	if err != nil {
 		if !os.IsNotExist(err) {
 			return err
 		}
-		realPath = filepath.Dir(lv.path)
+		// if the volume's data-directory wasn't found, fall back to using the
+		// volume's root path (see 8d27417bfeff316346d00c07a456b0e1b056e788)
+		realPath = lv.rootPath
 	}
 
 	if realPath == r.path || !strings.HasPrefix(realPath, r.path) {
@@ -228,7 +212,7 @@ func (r *Root) Remove(v volume.Volume) error {
 	}
 
 	delete(r.volumes, lv.name)
-	return removePath(filepath.Dir(lv.path))
+	return removePath(lv.rootPath)
 }
 
 func removePath(path string) error {
@@ -273,6 +257,8 @@ type localVolume struct {
 	m sync.Mutex
 	// unique name of the volume
 	name string
+	// rootPath is the volume's root path, where the volume's metadata is stored.
+	rootPath string
 	// path is the path on the host where the data lives
 	path string
 	// driverName is the name of the driver that created the volume.
@@ -350,6 +336,39 @@ func (v *localVolume) Status() map[string]interface{} {
 	return nil
 }
 
+func (v *localVolume) loadOpts() error {
+	b, err := os.ReadFile(filepath.Join(v.rootPath, "opts.json"))
+	if err != nil {
+		if !errors.Is(err, os.ErrNotExist) {
+			logrus.WithError(err).Warnf("error while loading volume options for volume: %s", v.name)
+		}
+		return nil
+	}
+	opts := optsConfig{}
+	if err := json.Unmarshal(b, &opts); err != nil {
+		return errors.Wrapf(err, "error while unmarshaling volume options for volume: %s", v.name)
+	}
+	// Make sure this isn't an empty optsConfig.
+	// This could be empty due to buggy behavior in older versions of Docker.
+	if !reflect.DeepEqual(opts, optsConfig{}) {
+		v.opts = &opts
+	}
+	return nil
+}
+
+func (v *localVolume) saveOpts() error {
+	var b []byte
+	b, err := json.Marshal(v.opts)
+	if err != nil {
+		return err
+	}
+	err = os.WriteFile(filepath.Join(v.rootPath, "opts.json"), b, 0600)
+	if err != nil {
+		return errdefs.System(errors.Wrap(err, "error while persisting volume options"))
+	}
+	return nil
+}
+
 // getAddress finds out address/hostname from options
 func getAddress(opts string) string {
 	optsList := strings.Split(opts, ",")

volume/local/local_linux_test.go

@@ -6,8 +6,10 @@ package local // import "github.com/docker/docker/volume/local"
 import (
 	"os"
 	"path/filepath"
+	"strconv"
 	"testing"
 
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/quota"
 	"gotest.tools/v3/assert"
@@ -95,3 +97,125 @@ func testVolQuotaUnsupported(t *testing.T, mountPoint, backingFsDev, testDir str
 	_, err = vol.Mount("1234")
 	assert.ErrorContains(t, err, "no quota support")
 }
+
+func TestVolCreateValidation(t *testing.T) {
+	r, err := New(t.TempDir(), idtools.Identity{UID: os.Geteuid(), GID: os.Getegid()})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	mandatoryOpts = map[string][]string{
+		"device": {"type"},
+		"type":   {"device"},
+		"o":      {"device", "type"},
+	}
+
+	tests := []struct {
+		doc         string
+		name        string
+		opts        map[string]string
+		expectedErr string
+	}{
+		{
+			doc:  "invalid: name too short",
+			name: "a",
+			opts: map[string]string{
+				"type":   "foo",
+				"device": "foo",
+			},
+			expectedErr: `volume name is too short, names should be at least two alphanumeric characters`,
+		},
+		{
+			doc:  "invalid: name invalid characters",
+			name: "hello world",
+			opts: map[string]string{
+				"type":   "foo",
+				"device": "foo",
+			},
+			expectedErr: `"hello world" includes invalid characters for a local volume name, only "[a-zA-Z0-9][a-zA-Z0-9_.-]" are allowed. If you intended to pass a host directory, use absolute path`,
+		},
+		{
+			doc:         "invalid: unknown option",
+			opts:        map[string]string{"hello": "world"},
+			expectedErr: `invalid option: "hello"`,
+		},
+		{
+			doc:         "invalid: invalid size",
+			opts:        map[string]string{"size": "hello"},
+			expectedErr: `invalid size: 'hello'`,
+		},
+		{
+			doc:         "invalid: size, but no quotactl",
+			opts:        map[string]string{"size": "1234"},
+			expectedErr: `quota size requested but no quota support`,
+		},
+		{
+			doc: "invalid: device without type",
+			opts: map[string]string{
+				"device": "foo",
+			},
+			expectedErr: `missing required option: "type"`,
+		},
+		{
+			doc: "invalid: type without device",
+			opts: map[string]string{
+				"type": "foo",
+			},
+			expectedErr: `missing required option: "device"`,
+		},
+		{
+			doc: "invalid: o without device",
+			opts: map[string]string{
+				"o":    "foo",
+				"type": "foo",
+			},
+			expectedErr: `missing required option: "device"`,
+		},
+		{
+			doc: "invalid: o without type",
+			opts: map[string]string{
+				"o":      "foo",
+				"device": "foo",
+			},
+			expectedErr: `missing required option: "type"`,
+		},
+		{
+			doc:  "valid: short name, no options",
+			name: "ab",
+		},
+		{
+			doc: "valid: device and type",
+			opts: map[string]string{
+				"type":   "foo",
+				"device": "foo",
+			},
+		},
+		{
+			doc: "valid: device, type, and o",
+			opts: map[string]string{
+				"type":   "foo",
+				"device": "foo",
+				"o":      "foo",
+			},
+		},
+	}
+
+	for i, tc := range tests {
+		tc := tc
+		t.Run(tc.doc, func(t *testing.T) {
+			if tc.name == "" {
+				tc.name = "vol-" + strconv.Itoa(i)
+			}
+			v, err := r.Create(tc.name, tc.opts)
+			if v != nil {
+				defer assert.Check(t, r.Remove(v))
+			}
+			if tc.expectedErr == "" {
+				assert.NilError(t, err)
+			} else {
+				assert.Check(t, errdefs.IsInvalidParameter(err), "got: %T", err)
+				assert.ErrorContains(t, err, tc.expectedErr)
+			}
+		})
+	}
+}

volume/local/local_unix.go

@@ -47,39 +47,22 @@ func (o *optsConfig) String() string {
 	return fmt.Sprintf("type='%s' device='%s' o='%s' size='%d'", o.MountType, o.MountDevice, o.MountOpts, o.Quota.Size)
 }
 
-func setOpts(v *localVolume, opts map[string]string) error {
+func (r *Root) validateOpts(opts map[string]string) error {
 	if len(opts) == 0 {
 		return nil
 	}
-	err := validateOpts(opts)
-	if err != nil {
-		return err
-	}
-	v.opts = &optsConfig{
-		MountType:   opts["type"],
-		MountOpts:   opts["o"],
-		MountDevice: opts["device"],
+	for opt := range opts {
+		if _, ok := validOpts[opt]; !ok {
+			return errdefs.InvalidParameter(errors.Errorf("invalid option: %q", opt))
+		}
 	}
 	if val, ok := opts["size"]; ok {
 		size, err := units.RAMInBytes(val)
 		if err != nil {
-			return err
+			return errdefs.InvalidParameter(err)
 		}
-		if size > 0 && v.quotaCtl == nil {
-			return errdefs.InvalidParameter(errors.Errorf("quota size requested but no quota support"))
-		}
-		v.opts.Quota.Size = uint64(size)
-	}
-	return nil
-}
-
-func validateOpts(opts map[string]string) error {
-	if len(opts) == 0 {
-		return nil
-	}
-	for opt := range opts {
-		if _, ok := validOpts[opt]; !ok {
-			return errdefs.InvalidParameter(errors.Errorf("invalid option: %q", opt))
+		if size > 0 && r.quotaCtl == nil {
+			return errdefs.InvalidParameter(errors.New("quota size requested but no quota support"))
 		}
 	}
 	for opt, reqopts := range mandatoryOpts {
@@ -94,6 +77,28 @@ func validateOpts(opts map[string]string) error {
 	return nil
 }
 
+func (v *localVolume) setOpts(opts map[string]string) error {
+	if len(opts) == 0 {
+		return nil
+	}
+	v.opts = &optsConfig{
+		MountType:   opts["type"],
+		MountOpts:   opts["o"],
+		MountDevice: opts["device"],
+	}
+	if val, ok := opts["size"]; ok {
+		size, err := units.RAMInBytes(val)
+		if err != nil {
+			return errdefs.InvalidParameter(err)
+		}
+		if size > 0 && v.quotaCtl == nil {
+			return errdefs.InvalidParameter(errors.New("quota size requested but no quota support"))
+		}
+		v.opts.Quota.Size = uint64(size)
+	}
+	return v.saveOpts()
+}
+
 func unmount(path string) {
 	_ = mount.Unmount(path)
 }
@@ -123,13 +128,13 @@ func (v *localVolume) mount() error {
 			mountOpts = strings.Replace(mountOpts, "addr="+addrValue, "addr="+ipAddr.String(), 1)
 		}
 	}
-	err := mount.Mount(v.opts.MountDevice, v.path, v.opts.MountType, mountOpts)
-	if err != nil {
+	if err := mount.Mount(v.opts.MountDevice, v.path, v.opts.MountType, mountOpts); err != nil {
 		if password := getPassword(v.opts.MountOpts); password != "" {
 			err = errors.New(strings.Replace(err.Error(), "password="+password, "password=********", 1))
 		}
+		return errors.Wrap(err, "failed to mount local volume")
 	}
-	return errors.Wrap(err, "failed to mount local volume")
+	return nil
 }
 
 func (v *localVolume) postMount() error {
@@ -143,7 +148,7 @@ func (v *localVolume) postMount() error {
 				return err
 			}
 		} else {
-			return fmt.Errorf("size quota requested for volume but no quota support")
+			return errors.New("size quota requested for volume but no quota support")
 		}
 	}
 	return nil

volume/local/local_windows.go

@@ -14,10 +14,15 @@ import (
 
 type optsConfig struct{}
 
-func setOpts(v *localVolume, opts map[string]string) error {
-	if len(opts) > 0 {
-		return errdefs.InvalidParameter(errors.New("options are not supported on this platform"))
+func (r *Root) validateOpts(opts map[string]string) error {
+	if len(opts) == 0 {
+		return nil
 	}
+	return errdefs.InvalidParameter(errors.New("options are not supported on this platform"))
+}
+
+func (v *localVolume) setOpts(opts map[string]string) error {
+	// Windows does not support any options currently
 	return nil
 }