Merge branch 'make-volumes-to-work-on-linux-docker-on-windows' into 'master'

Make volumes to work on linux docker on windows

Closes #4251

See merge request !1363

executors/docker/consts.go

@@ -13,3 +13,5 @@ const waitForContainerTimeout = 15 * time.Second
 
 const osTypeLinux = "linux"
 const osTypeWindows = "windows"
+
+const metadataOSType = "OSType"

executors/docker/executor_docker.go

@@ -52,8 +52,9 @@ var errVolumesManagerUndefined = errors.New("volumesManager is undefined")
 
 type executor struct {
 	executors.AbstractExecutor
-	client docker_helpers.Client
-	info   types.Info
+	client       docker_helpers.Client
+	volumeParser parser.Parser
+	info         types.Info
 
 	temporary []string // IDs of containers that should be removed
 
@@ -984,8 +985,8 @@ func (e *executor) connectDocker() error {
 // validateOSType checks if the ExecutorOptions metadata matches with the docker
 // info response.
 func (e *executor) validateOSType() error {
-	executorOSType, ok := e.ExecutorOptions.Metadata["OSType"]
-	if !ok {
+	executorOSType := e.ExecutorOptions.Metadata[metadataOSType]
+	if executorOSType == "" {
 		return common.MakeBuildError("%s does not have any OSType specified", e.Config.Executor)
 	}
 
@@ -1139,21 +1140,14 @@ func (e *executor) Prepare(options common.ExecutorPrepareOptions) error {
 	return nil
 }
 
-var (
-	buildDirectoryNotAbsoluteErr = common.MakeBuildError("build directory needs to be an absolute path")
-
-	buildDirectoryIsRootPathErr = common.MakeBuildError("build directory needs to be a non-root path")
-)
-
 func (e *executor) prepareBuildsDir(options common.ExecutorPrepareOptions) error {
-	volumeParser, err := parser.New(e.info)
-	if err != nil {
-		return fmt.Errorf("couldn't create volumes parser: %v", err)
+	if e.volumeParser == nil {
+		return common.MakeBuildError("missing volume parser")
 	}
 
-	isHostMounted, err := volumes.IsHostMountedVolume(volumeParser, e.RootDir(), options.Config.Docker.Volumes...)
+	isHostMounted, err := volumes.IsHostMountedVolume(e.volumeParser, e.RootDir(), options.Config.Docker.Volumes...)
 	if err != nil {
-		return err
+		return &common.BuildError{Inner: err}
 	}
 
 	// We need to set proper value for e.SharedBuildsDir because
@@ -1165,14 +1159,6 @@ func (e *executor) prepareBuildsDir(options common.ExecutorPrepareOptions) error
 		e.SharedBuildsDir = true
 	}
 
-	if !filepath.IsAbs(e.RootDir()) {
-		return buildDirectoryNotAbsoluteErr
-	}
-
-	if e.RootDir() == "/" {
-		return buildDirectoryIsRootPathErr
-	}
-
 	return nil
 }
 

executors/docker/executor_docker_command.go

@@ -9,6 +9,7 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-runner/common"
 	"gitlab.com/gitlab-org/gitlab-runner/executors"
+	"gitlab.com/gitlab-org/gitlab-runner/executors/docker/internal/volumes/parser"
 )
 
 type commandExecutor struct {
@@ -115,7 +116,7 @@ func init() {
 		},
 		ShowHostname: true,
 		Metadata: map[string]string{
-			"OSType": osTypeLinux,
+			metadataOSType: osTypeLinux,
 		},
 	}
 
@@ -125,6 +126,7 @@ func init() {
 				AbstractExecutor: executors.AbstractExecutor{
 					ExecutorOptions: options,
 				},
+				volumeParser: parser.NewLinuxParser(),
 			},
 		}
 		e.SetCurrentStage(common.ExecutorStageCreated)

executors/docker/executor_docker_command_windows.go

@@ -3,6 +3,7 @@ package docker
 import (
 	"gitlab.com/gitlab-org/gitlab-runner/common"
 	"gitlab.com/gitlab-org/gitlab-runner/executors"
+	"gitlab.com/gitlab-org/gitlab-runner/executors/docker/internal/volumes/parser"
 )
 
 func init() {
@@ -18,7 +19,7 @@ func init() {
 		},
 		ShowHostname: true,
 		Metadata: map[string]string{
-			"OSType": osTypeWindows,
+			metadataOSType: osTypeWindows,
 		},
 	}
 
@@ -28,6 +29,7 @@ func init() {
 				AbstractExecutor: executors.AbstractExecutor{
 					ExecutorOptions: options,
 				},
+				volumeParser: parser.NewWindowsParser(),
 			},
 		}
 		e.SetCurrentStage(common.ExecutorStageCreated)

executors/docker/executor_docker_ssh.go

@@ -7,6 +7,7 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-runner/common"
 	"gitlab.com/gitlab-org/gitlab-runner/executors"
+	"gitlab.com/gitlab-org/gitlab-runner/executors/docker/internal/volumes/parser"
 	"gitlab.com/gitlab-org/gitlab-runner/helpers/ssh"
 )
 
@@ -93,7 +94,7 @@ func init() {
 		},
 		ShowHostname: true,
 		Metadata: map[string]string{
-			"OSType": osTypeLinux,
+			metadataOSType: osTypeLinux,
 		},
 	}
 
@@ -103,6 +104,7 @@ func init() {
 				AbstractExecutor: executors.AbstractExecutor{
 					ExecutorOptions: options,
 				},
+				volumeParser: parser.NewLinuxParser(),
 			},
 		}
 		e.SetCurrentStage(common.ExecutorStageCreated)

executors/docker/executor_docker_test.go

@@ -205,6 +205,7 @@ func testServiceFromNamedImage(t *testing.T, description, imageName, serviceName
 			OSType:       helperimage.OSTypeLinux,
 			Architecture: "amd64",
 		},
+		volumeParser: parser.NewLinuxParser(),
 	}
 
 	options := buildImagePullOptions(e, imageName)
@@ -567,54 +568,54 @@ func TestDockerGetExistingDockerImageIfPullFails(t *testing.T) {
 
 func TestPrepareBuildsDir(t *testing.T) {
 	tests := map[string]struct {
-		osType                  string
+		parser                  parser.Parser
 		rootDir                 string
 		volumes                 []string
 		expectedSharedBuildsDir bool
-		expectedError           error
+		expectedError           string
 	}{
 		"rootDir mounted as host based volume": {
-			osType:                  parser.OSTypeLinux,
+			parser:                  parser.NewLinuxParser(),
 			rootDir:                 "/build",
 			volumes:                 []string{"/build:/build"},
 			expectedSharedBuildsDir: true,
 		},
 		"rootDir mounted as container based volume": {
-			osType:                  parser.OSTypeLinux,
+			parser:                  parser.NewLinuxParser(),
 			rootDir:                 "/build",
 			volumes:                 []string{"/build"},
 			expectedSharedBuildsDir: false,
 		},
 		"rootDir not mounted as volume": {
-			osType:                  parser.OSTypeLinux,
+			parser:                  parser.NewLinuxParser(),
 			rootDir:                 "/build",
 			volumes:                 []string{"/folder:/folder"},
 			expectedSharedBuildsDir: false,
 		},
 		"rootDir's parent mounted as volume": {
-			osType:                  parser.OSTypeLinux,
+			parser:                  parser.NewLinuxParser(),
 			rootDir:                 "/build/other/directory",
 			volumes:                 []string{"/build/:/build"},
 			expectedSharedBuildsDir: true,
 		},
 		"rootDir is not an absolute path": {
-			osType:        parser.OSTypeLinux,
+			parser:        parser.NewLinuxParser(),
 			rootDir:       "builds",
-			expectedError: buildDirectoryNotAbsoluteErr,
+			expectedError: "build directory needs to be an absolute path",
 		},
 		"rootDir is /": {
-			osType:        parser.OSTypeLinux,
+			parser:        parser.NewLinuxParser(),
 			rootDir:       "/",
-			expectedError: buildDirectoryIsRootPathErr,
+			expectedError: "build directory needs to be a non-root path",
 		},
 		"error on volume parsing": {
-			osType:        parser.OSTypeLinux,
+			parser:        parser.NewLinuxParser(),
 			rootDir:       "/build",
 			volumes:       []string{""},
-			expectedError: parser.NewInvalidVolumeSpecErr(""),
+			expectedError: "invalid volume specification",
 		},
 		"error on volume parser creation": {
-			expectedError: errors.New(`couldn't create volumes parser: unsupported OSType ""`),
+			expectedError: `missing volume parser`,
 		},
 	}
 
@@ -637,13 +638,17 @@ func TestPrepareBuildsDir(t *testing.T) {
 				AbstractExecutor: executors.AbstractExecutor{
 					Config: c,
 				},
-				info: types.Info{
-					OSType: test.osType,
-				},
+				volumeParser: test.parser,
 			}
 
 			err := e.prepareBuildsDir(options)
-			assert.Equal(t, test.expectedError, err)
+			if test.expectedError != "" {
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), test.expectedError)
+				return
+			}
+
+			assert.NoError(t, err)
 			assert.Equal(t, test.expectedSharedBuildsDir, e.SharedBuildsDir)
 		})
 	}
@@ -1408,10 +1413,11 @@ func TestDockerWatchOn_1_12_4(t *testing.T) {
 		AbstractExecutor: executors.AbstractExecutor{
 			ExecutorOptions: executors.ExecutorOptions{
 				Metadata: map[string]string{
-					"OSType": "linux",
+					metadataOSType: osTypeLinux,
 				},
 			},
 		},
+		volumeParser: parser.NewLinuxParser(),
 	}
 	e.Context = context.Background()
 	e.Build = &common.Build{
@@ -1486,6 +1492,7 @@ func prepareTestDockerConfiguration(t *testing.T, dockerConfig *common.DockerCon
 
 	e := &executor{}
 	e.client = c
+	e.volumeParser = parser.NewLinuxParser()
 	e.info = types.Info{
 		OSType:       helperimage.OSTypeLinux,
 		Architecture: "amd64",
@@ -1733,21 +1740,21 @@ func TestCheckOSType(t *testing.T) {
 	}{
 		"executor and docker info mismatch": {
 			executorMetadata: map[string]string{
-				"OSType": "windows",
+				metadataOSType: osTypeWindows,
 			},
-			dockerInfoOSType: "linux",
+			dockerInfoOSType: osTypeLinux,
 			expectedErr:      "executor requires OSType=windows, but Docker Engine supports only OSType=linux",
 		},
 		"executor and docker info match": {
 			executorMetadata: map[string]string{
-				"OSType": "linux",
+				metadataOSType: osTypeLinux,
 			},
-			dockerInfoOSType: "linux",
+			dockerInfoOSType: osTypeLinux,
 			expectedErr:      "",
 		},
 		"executor OSType not defined": {
 			executorMetadata: nil,
-			dockerInfoOSType: "linux",
+			dockerInfoOSType: osTypeLinux,
 			expectedErr:      " does not have any OSType specified",
 		},
 	}

executors/docker/internal/volumes/manager.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"path/filepath"
 
 	"gitlab.com/gitlab-org/gitlab-runner/executors/docker/internal/volumes/parser"
 )
@@ -76,9 +75,14 @@ func (m *manager) Create(volume string) error {
 }
 
 func (m *manager) addHostVolume(volume *parser.Volume) error {
-	volume.Destination = m.getAbsoluteContainerPath(volume.Destination)
+	var err error
 
-	err := m.managedVolumes.Add(volume.Destination)
+	volume.Destination, err = m.getAbsoluteContainerPath(volume.Destination)
+	if err != nil {
+		return err
+	}
+
+	err = m.managedVolumes.Add(volume.Destination)
 	if err != nil {
 		return err
 	}
@@ -88,16 +92,21 @@ func (m *manager) addHostVolume(volume *parser.Volume) error {
 	return nil
 }
 
-func (m *manager) getAbsoluteContainerPath(dir string) string {
-	if filepath.IsAbs(dir) {
-		return dir
+func (m *manager) getAbsoluteContainerPath(dir string) (string, error) {
+	if m.parser.Path().IsRoot(dir) {
+		return "", errDirectoryIsRootPath
+	}
+
+	if m.parser.Path().IsAbs(dir) {
+		return dir, nil
 	}
 
-	return filepath.Join(m.config.BaseContainerPath, dir)
+	return m.parser.Path().Join(m.config.BaseContainerPath, dir), nil
 }
 
 func (m *manager) appendVolumeBind(volume *parser.Volume) {
 	m.logger.Debugln(fmt.Sprintf("Using host-based %q for %q...", volume.Source, volume.Destination))
+
 	m.volumeBindings = append(m.volumeBindings, volume.Definition())
 }
 
@@ -120,19 +129,20 @@ func (m *manager) addCacheVolume(volume *parser.Volume) error {
 }
 
 func (m *manager) createHostBasedCacheVolume(containerPath string) error {
-	containerPath = m.getAbsoluteContainerPath(containerPath)
+	var err error
 
-	err := m.managedVolumes.Add(containerPath)
+	containerPath, err = m.getAbsoluteContainerPath(containerPath)
 	if err != nil {
 		return err
 	}
 
-	hostPath := filepath.Join(m.config.CacheDir, m.config.UniqueName, hashContainerPath(containerPath))
-	hostPath, err = filepath.Abs(hostPath)
+	err = m.managedVolumes.Add(containerPath)
 	if err != nil {
 		return err
 	}
 
+	hostPath := m.parser.Path().Join(m.config.CacheDir, m.config.UniqueName, hashContainerPath(containerPath))
+
 	m.appendVolumeBind(&parser.Volume{
 		Source:      hostPath,
 		Destination: containerPath,
@@ -142,9 +152,12 @@ func (m *manager) createHostBasedCacheVolume(containerPath string) error {
 }
 
 func (m *manager) createContainerBasedCacheVolume(containerPath string) (string, error) {
-	containerPath = m.getAbsoluteContainerPath(containerPath)
+	containerPath, err := m.getAbsoluteContainerPath(containerPath)
+	if err != nil {
+		return "", err
+	}
 
-	err := m.managedVolumes.Add(containerPath)
+	err = m.managedVolumes.Add(containerPath)
 	if err != nil {
 		return "", err
 	}

executors/docker/internal/volumes/manager_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-runner/executors/docker/internal/volumes/parser"
+	"gitlab.com/gitlab-org/gitlab-runner/helpers/path"
 )
 
 func newDebugLoggerMock() *mockDebugLogger {
@@ -51,8 +52,9 @@ func addCacheContainerManager(manager *manager) *MockCacheContainersManager {
 
 func addParser(manager *manager) *parser.MockParser {
 	parserMock := new(parser.MockParser)
-	manager.parser = parserMock
+	parserMock.On("Path").Return(path.NewUnixPath())
 
+	manager.parser = parserMock
 	return parserMock
 }
 
@@ -101,6 +103,12 @@ func TestDefaultManager_CreateUserVolumes_HostVolume(t *testing.T) {
 			parsedVolume:    &parser.Volume{Source: "/host/new", Destination: "/my/path", Mode: "ro"},
 			expectedBinding: []string{"/host:/duplicated", "/host/new:/my/path:ro"},
 		},
+		"root volume specified": {
+			volume:          "/host/new:/:ro",
+			parsedVolume:    &parser.Volume{Source: "/host/new", Destination: "/", Mode: "ro"},
+			expectedBinding: []string{"/host:/duplicated"},
+			expectedError:   errDirectoryIsRootPath,
+		},
 	}
 
 	for testName, testCase := range testCases {
@@ -451,8 +459,9 @@ func TestDefaultManager_CreateUserVolumes_CacheVolume_ContainerBased_WithError(t
 func TestDefaultManager_CreateUserVolumes_ParserError(t *testing.T) {
 	m := newDefaultManager(ManagerConfig{})
 
-	volumeParser := addParser(m)
+	volumeParser := new(parser.MockParser)
 	defer volumeParser.AssertExpectations(t)
+	m.parser = volumeParser
 
 	volumeParser.On("ParseVolume", "volume").
 		Return(nil, errors.New("parser-test-error")).

executors/docker/internal/volumes/parser/base_parser.go

@@ -2,9 +2,13 @@ package parser
 
 import (
 	"regexp"
+
+	"gitlab.com/gitlab-org/gitlab-runner/helpers/path"
 )
 
-type baseParser struct{}
+type baseParser struct {
+	path path.Path
+}
 
 // The way how matchesToVolumeSpecParts parses the volume mount specification and assigns
 // parts was inspired by how Docker Engine's `windowsParser` is created. The original sources
@@ -41,3 +45,7 @@ func (p *baseParser) matchesToVolumeSpecParts(spec string, specExp *regexp.Regex
 
 	return parts, nil
 }
+
+func (p *baseParser) Path() path.Path {
+	return p.path
+}

executors/docker/internal/volumes/parser/linux_parser.go

@@ -2,6 +2,8 @@ package parser
 
 import (
 	"regexp"
+
+	"gitlab.com/gitlab-org/gitlab-runner/helpers/path"
 )
 
 const (
@@ -18,8 +20,12 @@ type linuxParser struct {
 	baseParser
 }
 
-func newLinuxParser() Parser {
-	return new(linuxParser)
+func NewLinuxParser() Parser {
+	return &linuxParser{
+		baseParser: baseParser{
+			path: path.NewUnixPath(),
+		},
+	}
 }
 
 func (p *linuxParser) ParseVolume(spec string) (*Volume, error) {

executors/docker/internal/volumes/parser/linux_parser_test.go

@@ -68,7 +68,7 @@ func TestLinuxParser_ParseVolume(t *testing.T) {
 
 	for testName, testCase := range testCases {
 		t.Run(testName, func(t *testing.T) {
-			parser := newLinuxParser()
+			parser := NewLinuxParser()
 			parts, err := parser.ParseVolume(testCase.volumeSpec)
 
 			if testCase.expectedError == nil {

executors/docker/internal/volumes/parser/mock_Parser.go

@@ -5,6 +5,7 @@
 package parser
 
 import mock "github.com/stretchr/testify/mock"
+import path "gitlab.com/gitlab-org/gitlab-runner/helpers/path"
 
 // MockParser is an autogenerated mock type for the Parser type
 type MockParser struct {
@@ -33,3 +34,19 @@ func (_m *MockParser) ParseVolume(spec string) (*Volume, error) {
 
 	return r0, r1
 }
+
+// Path provides a mock function with given fields:
+func (_m *MockParser) Path() path.Path {
+	ret := _m.Called()
+
+	var r0 path.Path
+	if rf, ok := ret.Get(0).(func() path.Path); ok {
+		r0 = rf()
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(path.Path)
+		}
+	}
+
+	return r0
+}

executors/docker/internal/volumes/parser/parser.go

 package parser
 
-import (
-	"github.com/docker/docker/api/types"
-
-	"gitlab.com/gitlab-org/gitlab-runner/helpers/docker/errors"
-)
-
-const (
-	OSTypeLinux   = "linux"
-	OSTypeWindows = "windows"
-)
+import "gitlab.com/gitlab-org/gitlab-runner/helpers/path"
 
 type Parser interface {
 	ParseVolume(spec string) (*Volume, error)
-}
-
-type parserFactory func() Parser
-
-var supportedOSTypes = map[string]parserFactory{
-	OSTypeLinux:   newLinuxParser,
-	OSTypeWindows: newWindowsParser,
-}
-
-func New(info types.Info) (Parser, error) {
-	factory, ok := supportedOSTypes[info.OSType]
-	if !ok {
-		return nil, errors.NewErrOSNotSupported(info.OSType)
-	}
-	return factory(), nil
+	Path() path.Path
 }

executors/docker/internal/volumes/parser/parser_test.go

-package parser
-
-import (
-	"testing"
-
-	"github.com/docker/docker/api/types"
-	"github.com/stretchr/testify/assert"
-
-	"gitlab.com/gitlab-org/gitlab-runner/helpers/docker/errors"
-)
-
-func TestNew(t *testing.T) {
-	testCases := map[string]struct {
-		expectedParserType interface{}
-		expectedError      error
-	}{
-		OSTypeLinux:   {expectedParserType: &linuxParser{}, expectedError: nil},
-		OSTypeWindows: {expectedParserType: &windowsParser{}, expectedError: nil},
-		"unsupported": {expectedParserType: nil, expectedError: errors.NewErrOSNotSupported("unsupported")},
-	}
-
-	for osType, testCase := range testCases {
-		t.Run(osType, func(t *testing.T) {
-			parser, err := New(types.Info{OSType: osType})
-
-			assert.IsType(t, testCase.expectedParserType, parser)
-
-			if testCase.expectedError == nil {
-				assert.NoError(t, err)
-			} else {
-				assert.EqualError(t, err, testCase.expectedError.Error())
-			}
-		})
-	}
-}

executors/docker/internal/volumes/parser/windows_parser.go

+// +build windows
+
 package parser
 
 import (
 	"regexp"