volume/mounts: fix anonymous volume not being labeled

thaJeztah · thaJeztah · commit 10d57fde4497 · 2024-10-25T01:57:22.000+02:00
`Parser.ParseMountRaw()` labels anonymous volumes with a `AnonymousLabel` label (`com.docker.volume.anonymous`) label based on whether a volume has a name (named volume) or no name (anonymous) (see [1]). However both `VolumesService.Create()` (see [1]) and `Parser.ParseMountRaw()` (see [2], [3]) were generating a random name for anonymous volumes. The latter is called before `VolumesService.Create()` is called, resulting in such volumes not being labeled as anonymous. Generating the name was originally done in Create (fc7b904), but duplicated in b3b7eb2 with the introduction of the new Mounts field in HostConfig. Duplicating this effort didn't have a real effect until (`Create` would just skip generating the name), until 618f26c introduced the `AnonymousLabel` in (v24.0.0, backported to v23.0.0). Parsing generally should not fill in defaults / generate names, so this patch; - Removes generating volume names from `Parser.ParseMountRaw()` - Adds a debug-log entry to `VolumesService.Create()` - Touches up some logs to use structured logs for easier correlating logs With this patch applied: docker run --rm --mount=type=volume,target=/toto hello-world DEBU[2024-10-24T22:50:36.359990376Z] creating anonymous volume volume-name=0cfd63d4df363571e7b3e9c04e37c74054cc16ff1d00d9a005232d83e92eda02 DEBU[2024-10-24T22:50:36.360069209Z] probing all drivers for volume volume-name=0cfd63d4df363571e7b3e9c04e37c74054cc16ff1d00d9a005232d83e92eda02 DEBU[2024-10-24T22:50:36.360341209Z] Registering new volume reference driver=local volume-name=0cfd63d4df363571e7b3e9c04e37c74054cc16ff1d00d9a005232d83e92eda02 [1]: https://github.com/moby/moby/blob/032721ff75fe4a71a45b4a7a9b7e7c4c80c6431b/volume/service/service.go#L72-L83 [2]: https://github.com/moby/moby/blob/032721ff75fe4a71a45b4a7a9b7e7c4c80c6431b/volume/mounts/linux_parser.go#L330-L336 [3]: https://github.com/moby/moby/blob/032721ff75fe4a71a45b4a7a9b7e7c4c80c6431b/volume/mounts/windows_parser.go#L394-L400 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
diff --git a/integration/container/mounts_linux_test.go b/integration/container/mounts_linux_test.go
@@ -393,6 +393,37 @@ func TestContainerVolumesMountedAsSlave(t *testing.T) {
 	}
 }
 
+// TestContainerVolumeAnonymous verifies that anonymous volumes created through
+// the Mounts API get a random name generated, and have the "AnonymousLabel"
+// (com.docker.volume.anonymous) label set.
+//
+// regression test for https://github.com/moby/moby/issues/48748
+func TestContainerVolumeAnonymous(t *testing.T) {
+	skip.If(t, testEnv.IsRemoteDaemon)
+
+	ctx := setupTest(t)
+
+	mntOpts := mounttypes.Mount{Type: mounttypes.TypeVolume, Target: "/foo"}
+
+	apiClient := testEnv.APIClient()
+	cID := container.Create(ctx, t, apiClient, container.WithMount(mntOpts))
+
+	inspect := container.Inspect(ctx, t, apiClient, cID)
+	assert.Assert(t, is.Len(inspect.HostConfig.Mounts, 1))
+	assert.Check(t, is.Equal(inspect.HostConfig.Mounts[0], mntOpts))
+
+	assert.Assert(t, is.Len(inspect.Mounts, 1))
+	volName := inspect.Mounts[0].Name
+	assert.Check(t, is.Len(volName, 64), "volume name should be 64 bytes (from stringid.GenerateRandomID())")
+
+	volInspect, err := apiClient.VolumeInspect(ctx, volName)
+	assert.NilError(t, err)
+
+	// see [daemon.AnonymousLabel]; we don't want to import the daemon package here.
+	const expectedAnonymousLabel = "com.docker.volume.anonymous"
+	assert.Check(t, is.Contains(volInspect.Labels, expectedAnonymousLabel))
+}
+
 // Regression test for #38995 and #43390.
 func TestContainerCopyLeaksMounts(t *testing.T) {
 	ctx := setupTest(t)
diff --git a/volume/mounts/linux_parser.go b/volume/mounts/linux_parser.go
@@ -8,7 +8,6 @@ import (
 	"strings"
 
 	"github.com/docker/docker/api/types/mount"
-	"github.com/docker/docker/pkg/stringid"
 	"github.com/docker/docker/volume"
 )
 
@@ -329,9 +328,8 @@ func (p *linuxParser) parseMountSpec(cfg mount.Mount, validateBindSourceExists b
 
 	switch cfg.Type {
 	case mount.TypeVolume:
-		if cfg.Source == "" {
-			mp.Name = stringid.GenerateRandomID()
-		} else {
+		if cfg.Source != "" {
+			// non-anonymous volume
 			mp.Name = cfg.Source
 		}
 		mp.CopyData = p.DefaultCopyMode()
diff --git a/volume/mounts/windows_parser.go b/volume/mounts/windows_parser.go
@@ -10,7 +10,6 @@ import (
 	"strings"
 
 	"github.com/docker/docker/api/types/mount"
-	"github.com/docker/docker/pkg/stringid"
 )
 
 // NewWindowsParser creates a parser with Windows semantics.
@@ -393,9 +392,8 @@ func (p *windowsParser) parseMountSpec(cfg mount.Mount, convertTargetToBackslash
 
 	switch cfg.Type {
 	case mount.TypeVolume:
-		if cfg.Source == "" {
-			mp.Name = stringid.GenerateRandomID()
-		} else {
+		if cfg.Source != "" {
+			// non-anonymous volume
 			mp.Name = cfg.Source
 		}
 		mp.CopyData = p.DefaultCopyMode()
diff --git a/volume/service/service.go b/volume/service/service.go
@@ -73,6 +73,9 @@ func (s *VolumesService) Create(ctx context.Context, name, driverName string, op
 	if name == "" {
 		name = stringid.GenerateRandomID()
 		options = append(options, opts.WithCreateLabel(AnonymousLabel, ""))
+		log.G(ctx).WithField("volume-name", name).Error("Creating anonymous volume")
+	} else {
+		log.G(ctx).WithField("volume-name", name).Error("Creating named volume")
 	}
 	v, err := s.vs.Create(ctx, name, driverName, options...)
 	if err != nil {
diff --git a/volume/service/store.go b/volume/service/store.go
@@ -732,7 +732,7 @@ func (s *VolumeStore) getVolume(ctx context.Context, name, driverName string) (v
 		return volumeWrapper{vol, meta.Labels, scope, meta.Options}, nil
 	}
 
-	log.G(ctx).Debugf("Probing all drivers for volume with name: %s", name)
+	log.G(ctx).WithField("volume-name", name).Debugf("Probing all drivers for volume")
 	drvrs, err := s.drivers.GetAllDrivers()
 	if err != nil {
 		return nil, err

Original file line number	Diff line number	Diff line change
`@@ -732,7 +732,7 @@ func (s *VolumeStore) getVolume(ctx context.Context, name, driverName string) (v`
`732`	`732`	`return volumeWrapper{vol, meta.Labels, scope, meta.Options}, nil`
`733`	`733`	`}`
`734`	`734`
`735`		`- log.G(ctx).Debugf("Probing all drivers for volume with name: %s", name)`
	`735`	`+ log.G(ctx).WithField("volume-name", name).Debugf("Probing all drivers for volume")`
`736`	`736`	`drvrs, err := s.drivers.GetAllDrivers()`
`737`	`737`	`if err != nil {`
`738`	`738`	`return nil, err`