test to cover preference for bind API

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

Author: ndeloof

pkg/compose/create.go

@@ -39,7 +39,6 @@ import (
 	"github.com/docker/docker/api/types/blkiodev"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/filters"
-	"github.com/docker/docker/api/types/image"
 	"github.com/docker/docker/api/types/mount"
 	"github.com/docker/docker/api/types/network"
 	"github.com/docker/docker/api/types/strslice"
@@ -828,7 +827,6 @@ func getDependentServiceFromMode(mode string) string {
 	return ""
 }
 
-//nolint:gocyclo
 func (s *composeService) buildContainerVolumes(
 	ctx context.Context,
 	p types.Project,
@@ -838,13 +836,7 @@ func (s *composeService) buildContainerVolumes(
 	var mounts []mount.Mount
 	var binds []string
 
-	img := api.GetImageNameOrDefault(service, p.Name)
-	imgInspect, err := s.apiClient().ImageInspect(ctx, img)
-	if err != nil {
-		return nil, nil, err
-	}
-
-	mountOptions, err := buildContainerMountOptions(p, service, imgInspect, inherit)
+	mountOptions, err := s.buildContainerMountOptions(ctx, p, service, inherit)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -857,11 +849,10 @@ func (s *composeService) buildContainerVolumes(
 			// see https://github.com/moby/moby/issues/43483
 			v := findVolumeByTarget(service.Volumes, m.Target)
 			if v != nil {
-				switch {
-				case v.Type != types.VolumeTypeBind:
+				if v.Type != types.VolumeTypeBind {
 					v.Source = m.Source
-					fallthrough
-				case !requireMountAPI(v.Bind):
+				}
+				if !bindRequiresMountAPI(v.Bind) {
 					source := m.Source
 					if vol := findVolumeByName(p.Volumes, m.Source); vol != nil {
 						source = m.Source
@@ -874,8 +865,8 @@ func (s *composeService) buildContainerVolumes(
 			v := findVolumeByTarget(service.Volumes, m.Target)
 			vol := findVolumeByName(p.Volumes, m.Source)
 			if v != nil && vol != nil {
-				if _, ok := vol.DriverOpts["device"]; ok && vol.Driver == "local" && vol.DriverOpts["o"] == "bind" {
-					// Looks like a volume, but actually a bind mount which requires the bind API
+				// Prefer the bind API if no advanced option is used, to preserve backward compatibility
+				if !volumeRequiresMountAPI(v.Volume) {
 					binds = append(binds, toBindString(vol.Name, v))
 					continue
 				}
@@ -930,9 +921,9 @@ func findVolumeByTarget(volumes []types.ServiceVolumeConfig, target string) *typ
 	return nil
 }
 
-// requireMountAPI check if Bind declaration can be implemented by the plain old Bind API or uses any of the advanced
+// bindRequiresMountAPI check if Bind declaration can be implemented by the plain old Bind API or uses any of the advanced
 // options which require use of Mount API
-func requireMountAPI(bind *types.ServiceVolumeBind) bool {
+func bindRequiresMountAPI(bind *types.ServiceVolumeBind) bool {
 	switch {
 	case bind == nil:
 		return false
@@ -947,7 +938,24 @@ func requireMountAPI(bind *types.ServiceVolumeBind) bool {
 	}
 }
 
-func buildContainerMountOptions(p types.Project, s types.ServiceConfig, img image.InspectResponse, inherit *container.Summary) ([]mount.Mount, error) {
+// volumeRequiresMountAPI check if Volume declaration can be implemented by the plain old Bind API or uses any of the advanced
+// options which require use of Mount API
+func volumeRequiresMountAPI(vol *types.ServiceVolumeVolume) bool {
+	switch {
+	case vol == nil:
+		return false
+	case len(vol.Labels) > 0:
+		return true
+	case vol.Subpath != "":
+		return true
+	case vol.NoCopy:
+		return true
+	default:
+		return false
+	}
+}
+
+func (s *composeService) buildContainerMountOptions(ctx context.Context, p types.Project, service types.ServiceConfig, inherit *container.Summary) ([]mount.Mount, error) {
 	mounts := map[string]mount.Mount{}
 	if inherit != nil {
 		for _, m := range inherit.Mounts {
@@ -959,6 +967,11 @@ func buildContainerMountOptions(p types.Project, s types.ServiceConfig, img imag
 				src = m.Name
 			}
 
+			img, err := s.apiClient().ImageInspect(ctx, api.GetImageNameOrDefault(service, p.Name))
+			if err != nil {
+				return nil, err
+			}
+
 			if img.Config != nil {
 				if _, ok := img.Config.Volumes[m.Destination]; ok {
 					// inherit previous container's anonymous volume
@@ -971,7 +984,7 @@ func buildContainerMountOptions(p types.Project, s types.ServiceConfig, img imag
 				}
 			}
 			volumes := []types.ServiceVolumeConfig{}
-			for _, v := range s.Volumes {
+			for _, v := range service.Volumes {
 				if v.Target != m.Destination || v.Source != "" {
 					volumes = append(volumes, v)
 					continue
@@ -984,11 +997,11 @@ func buildContainerMountOptions(p types.Project, s types.ServiceConfig, img imag
 					ReadOnly: !m.RW,
 				}
 			}
-			s.Volumes = volumes
+			service.Volumes = volumes
 		}
 	}
 
-	mounts, err := fillBindMounts(p, s, mounts)
+	mounts, err := fillBindMounts(p, service, mounts)
 	if err != nil {
 		return nil, err
 	}

pkg/compose/create_test.go

@@ -17,13 +17,16 @@
 package compose
 
 import (
+	"context"
 	"os"
 	"path/filepath"
 	"sort"
 	"testing"
 
+	composeloader "github.com/compose-spec/compose-go/v2/loader"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/image"
+	"go.uber.org/mock/gomock"
 	"gotest.tools/v3/assert/cmp"
 
 	"github.com/docker/compose/v2/pkg/api"
@@ -154,7 +157,16 @@ func TestBuildContainerMountOptions(t *testing.T) {
 		},
 	}
 
-	mounts, err := buildContainerMountOptions(project, project.Services["myService"], image.InspectResponse{}, inherit)
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	mock, cli := prepareMocks(mockCtrl)
+	s := composeService{
+		dockerCli: cli,
+	}
+	mock.EXPECT().ImageInspect(gomock.Any(), "myProject-myService").AnyTimes().Return(image.InspectResponse{}, nil)
+
+	mounts, err := s.buildContainerMountOptions(context.TODO(), project, project.Services["myService"], inherit)
 	sort.Slice(mounts, func(i, j int) bool {
 		return mounts[i].Target < mounts[j].Target
 	})
@@ -166,7 +178,7 @@ func TestBuildContainerMountOptions(t *testing.T) {
 	assert.Equal(t, mounts[2].VolumeOptions.Subpath, "etc")
 	assert.Equal(t, mounts[3].Target, "\\\\.\\pipe\\docker_engine")
 
-	mounts, err = buildContainerMountOptions(project, project.Services["myService"], image.InspectResponse{}, inherit)
+	mounts, err = s.buildContainerMountOptions(context.TODO(), project, project.Services["myService"], inherit)
 	sort.Slice(mounts, func(i, j int) bool {
 		return mounts[i].Target < mounts[j].Target
 	})
@@ -321,3 +333,123 @@ func TestCreateEndpointSettings(t *testing.T) {
 		IPv6Gateway: "fdb4:7a7f:373a:3f0c::42",
 	}))
 }
+
+func Test_buildContainerVolumes(t *testing.T) {
+	pwd, err := os.Getwd()
+	assert.NilError(t, err)
+
+	tests := []struct {
+		name   string
+		yaml   string
+		binds  []string
+		mounts []mountTypes.Mount
+	}{
+		{
+			name: "bind mount local path",
+			yaml: `
+services:
+  test:
+    volumes:
+      - ./data:/data
+`,
+			binds:  []string{filepath.Join(pwd, "data") + ":/data:rw"},
+			mounts: nil,
+		},
+		{
+			name: "bind mount, not create host path",
+			yaml: `
+services:
+  test:
+    volumes:
+      - type: bind
+        source: ./data
+        target: /data
+        bind:
+          create_host_path: false
+`,
+			binds: nil,
+			mounts: []mountTypes.Mount{
+				{
+					Type:        "bind",
+					Source:      filepath.Join(pwd, "data"),
+					Target:      "/data",
+					BindOptions: &mountTypes.BindOptions{CreateMountpoint: false},
+				},
+			},
+		},
+		{
+			name: "mount volume",
+			yaml: `
+services:
+  test:
+    volumes:
+      - data:/data
+volumes:
+  data:
+    name: my_volume
+`,
+			binds:  []string{"my_volume:/data:rw"},
+			mounts: nil,
+		},
+		{
+			name: "mount volume, readonly",
+			yaml: `
+services:
+  test:
+    volumes:
+      - data:/data:ro
+volumes:
+  data:
+    name: my_volume
+`,
+			binds:  []string{"my_volume:/data:ro"},
+			mounts: nil,
+		},
+		{
+			name: "mount volume subpath",
+			yaml: `
+services:
+  test:
+    volumes:
+      - type: volume
+        source: data
+        target: /data
+        volume:
+          subpath: test/
+volumes:
+  data: 
+    name: my_volume
+`,
+			binds: nil,
+			mounts: []mountTypes.Mount{
+				{
+					Type:          "volume",
+					Source:        "my_volume",
+					Target:        "/data",
+					VolumeOptions: &mountTypes.VolumeOptions{Subpath: "test/"},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			p, err := composeloader.LoadWithContext(context.TODO(), composetypes.ConfigDetails{
+				ConfigFiles: []composetypes.ConfigFile{
+					{
+						Filename: "test",
+						Content:  []byte(tt.yaml),
+					},
+				},
+			}, func(options *composeloader.Options) {
+				options.SkipValidation = true
+				options.SkipConsistencyCheck = true
+			})
+			assert.NilError(t, err)
+			s := &composeService{}
+			binds, mounts, err := s.buildContainerVolumes(context.TODO(), *p, p.Services["test"], nil)
+			assert.NilError(t, err)
+			assert.DeepEqual(t, tt.binds, binds)
+			assert.DeepEqual(t, tt.mounts, mounts)
+		})
+	}
+}