Merge pull request #787 from docker/add-abs-path-secret-target

Support absolute paths for secrets

aci/convert/convert.go

@@ -23,6 +23,7 @@ import (
 	"io/ioutil"
 	"math"
 	"os"
+	"path/filepath"
 	"strconv"
 	"strings"
 
@@ -50,7 +51,8 @@ const (
 	volumeDriveroptsAccountNameKey = "storage_account_name"
 	volumeReadOnly                 = "read_only"
 
-	serviceSecretPrefix = "aci-service-secret-"
+	defaultSecretsPath         = "/run/secrets"
+	serviceSecretAbsPathPrefix = "aci-service-secret-path-"
 )
 
 // ToContainerGroup converts a compose project into a ACI container group
@@ -188,13 +190,15 @@ func getDNSSidecar(containers []containerinstance.Container) containerinstance.C
 
 type projectAciHelper types.Project
 
+func getServiceSecretKey(serviceName, targetDir string) string {
+	return fmt.Sprintf("%s-%s--%s",
+		serviceSecretAbsPathPrefix, serviceName, strings.ReplaceAll(targetDir, "/", "-"))
+}
+
 func (p projectAciHelper) getAciSecretVolumes() ([]containerinstance.Volume, error) {
 	var secretVolumes []containerinstance.Volume
 	for _, svc := range p.Services {
-		secretServiceVolume := containerinstance.Volume{
-			Name:   to.StringPtr(serviceSecretPrefix + svc.Name),
-			Secret: make(map[string]*string),
-		}
+		squashedTargetVolumes := make(map[string]containerinstance.Volume)
 		for _, scr := range svc.Secrets {
 			data, err := ioutil.ReadFile(p.Secrets[scr.Source].File)
 			if err != nil {
@@ -207,14 +211,31 @@ func (p projectAciHelper) getAciSecretVolumes() ([]containerinstance.Volume, err
 			if scr.Target == "" {
 				scr.Target = scr.Source
 			}
-			if strings.ContainsAny(scr.Target, "\\/") {
+
+			if !filepath.IsAbs(scr.Target) && strings.ContainsAny(scr.Target, "\\/") {
 				return []containerinstance.Volume{},
-					errors.Errorf("in service %q, secret with source %q cannot have a path as target. Found %q", svc.Name, scr.Source, scr.Target)
+					errors.Errorf("in service %q, secret with source %q cannot have a relative path as target. "+
+						"Only absolute paths are allowed. Found %q",
+						svc.Name, scr.Source, scr.Target)
 			}
-			secretServiceVolume.Secret[scr.Target] = &dataStr
+
+			if !filepath.IsAbs(scr.Target) {
+				scr.Target = filepath.Join(defaultSecretsPath, scr.Target)
+			}
+
+			targetDir := filepath.Dir(scr.Target)
+			targetDirKey := getServiceSecretKey(svc.Name, targetDir)
+			if _, ok := squashedTargetVolumes[targetDir]; !ok {
+				squashedTargetVolumes[targetDir] = containerinstance.Volume{
+					Name:   to.StringPtr(targetDirKey),
+					Secret: make(map[string]*string),
+				}
+			}
+
+			squashedTargetVolumes[targetDir].Secret[filepath.Base(scr.Target)] = &dataStr
 		}
-		if len(secretServiceVolume.Secret) > 0 {
-			secretVolumes = append(secretVolumes, secretServiceVolume)
+		for _, v := range squashedTargetVolumes {
+			secretVolumes = append(secretVolumes, v)
 		}
 	}
 
@@ -326,15 +347,62 @@ func (s serviceConfigAciHelper) getAciFileVolumeMounts(volumesCache map[string]b
 	return aciServiceVolumes, nil
 }
 
-func (s serviceConfigAciHelper) getAciSecretsVolumeMount() *containerinstance.VolumeMount {
-	if len(s.Secrets) == 0 {
-		return nil
+func (s serviceConfigAciHelper) getAciSecretsVolumeMounts() ([]containerinstance.VolumeMount, error) {
+	vms := []containerinstance.VolumeMount{}
+	presenceSet := make(map[string]bool)
+	for _, scr := range s.Secrets {
+		if scr.Target == "" {
+			scr.Target = scr.Source
+		}
+		if !filepath.IsAbs(scr.Target) {
+			scr.Target = filepath.Join(defaultSecretsPath, scr.Target)
+		}
+
+		presenceKey := filepath.Dir(scr.Target)
+		if !presenceSet[presenceKey] {
+			vms = append(vms, containerinstance.VolumeMount{
+				Name:      to.StringPtr(getServiceSecretKey(s.Name, filepath.Dir(scr.Target))),
+				MountPath: to.StringPtr(filepath.Dir(scr.Target)),
+				ReadOnly:  to.BoolPtr(true),
+			})
+			presenceSet[presenceKey] = true
+		}
 	}
-	return &containerinstance.VolumeMount{
-		Name:      to.StringPtr(serviceSecretPrefix + s.Name),
-		MountPath: to.StringPtr("/run/secrets"),
-		ReadOnly:  to.BoolPtr(true),
+	err := validateMountPathCollisions(vms)
+	if err != nil {
+		return []containerinstance.VolumeMount{}, err
 	}
+	return vms, nil
+}
+
+func validateMountPathCollisions(vms []containerinstance.VolumeMount) error {
+	for i, vm1 := range vms {
+		for j, vm2 := range vms {
+			if i == j {
+				continue
+			}
+			var (
+				biggerVMPath  = strings.Split(*vm1.MountPath, string(filepath.Separator))
+				smallerVMPath = strings.Split(*vm2.MountPath, string(filepath.Separator))
+			)
+			if len(smallerVMPath) > len(biggerVMPath) {
+				tmp := biggerVMPath
+				biggerVMPath = smallerVMPath
+				smallerVMPath = tmp
+			}
+			isPrefixed := true
+			for i := 0; i < len(smallerVMPath); i++ {
+				if smallerVMPath[i] != biggerVMPath[i] {
+					isPrefixed = false
+					break
+				}
+			}
+			if isPrefixed {
+				return errors.Errorf("mount paths %q and %q collide. A volume mount cannot include another one.", *vm1.MountPath, *vm2.MountPath)
+			}
+		}
+	}
+	return nil
 }
 
 func (s serviceConfigAciHelper) getAciContainer(volumesCache map[string]bool) (containerinstance.Container, error) {
@@ -342,11 +410,11 @@ func (s serviceConfigAciHelper) getAciContainer(volumesCache map[string]bool) (c
 	if err != nil {
 		return containerinstance.Container{}, err
 	}
-	allVolumes := aciServiceVolumes
-	secretVolumeMount := s.getAciSecretsVolumeMount()
-	if secretVolumeMount != nil {
-		allVolumes = append(allVolumes, *secretVolumeMount)
+	serviceSecretVolumes, err := s.getAciSecretsVolumeMounts()
+	if err != nil {
+		return containerinstance.Container{}, err
 	}
+	allVolumes := append(aciServiceVolumes, serviceSecretVolumes...)
 	var volumes *[]containerinstance.VolumeMount
 	if len(allVolumes) > 0 {
 		volumes = &allVolumes

aci/convert/convert_test.go

@@ -18,7 +18,10 @@ package convert
 
 import (
 	"context"
+	"fmt"
+	"io/ioutil"
 	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/mock"
@@ -712,6 +715,151 @@ func TestConvertContainerGroupStatus(t *testing.T) {
 	assert.Equal(t, "Unknown", GetStatus(container(nil), group(nil)))
 }
 
+func TestConvertSecrets(t *testing.T) {
+	serviceName := "testservice"
+	secretName := "testsecret"
+	absBasePath := "/home/user"
+	tmpFile, err := ioutil.TempFile(os.TempDir(), "TestConvertProjectSecrets-")
+	assert.NilError(t, err)
+	_, err = tmpFile.Write([]byte("test content"))
+	assert.NilError(t, err)
+	t.Cleanup(func() {
+		assert.NilError(t, os.Remove(tmpFile.Name()))
+	})
+
+	t.Run("mix default and absolute", func(t *testing.T) {
+		pSquashedDefaultAndAbs := projectAciHelper{
+			Services: []types.ServiceConfig{
+				{
+					Name: serviceName,
+					Secrets: []types.ServiceSecretConfig{
+						{
+							Source: secretName,
+							Target: "some_target1",
+						},
+						{
+							Source: secretName,
+						},
+						{
+							Source: secretName,
+							Target: filepath.Join(defaultSecretsPath, "some_target2"),
+						},
+						{
+							Source: secretName,
+							Target: filepath.Join(absBasePath, "some_target3"),
+						},
+						{
+							Source: secretName,
+							Target: filepath.Join(absBasePath, "some_target4"),
+						},
+					},
+				},
+			},
+			Secrets: map[string]types.SecretConfig{
+				secretName: {
+					File: tmpFile.Name(),
+				},
+			},
+		}
+		vs, err := pSquashedDefaultAndAbs.getAciSecretVolumes()
+		assert.NilError(t, err)
+		assert.Equal(t, len(vs), 2)
+
+		defaultVolumeName := getServiceSecretKey(serviceName, defaultSecretsPath)
+		assert.Equal(t, *vs[0].Name, defaultVolumeName)
+		assert.Equal(t, len(vs[0].Secret), 3)
+
+		homeVolumeName := getServiceSecretKey(serviceName, absBasePath)
+		assert.Equal(t, *vs[1].Name, homeVolumeName)
+		assert.Equal(t, len(vs[1].Secret), 2)
+
+		s := serviceConfigAciHelper(pSquashedDefaultAndAbs.Services[0])
+		vms, err := s.getAciSecretsVolumeMounts()
+		assert.NilError(t, err)
+		assert.Equal(t, len(vms), 2)
+
+		assert.Equal(t, *vms[0].Name, defaultVolumeName)
+		assert.Equal(t, *vms[0].MountPath, defaultSecretsPath)
+
+		assert.Equal(t, *vms[1].Name, homeVolumeName)
+		assert.Equal(t, *vms[1].MountPath, absBasePath)
+	})
+
+	t.Run("convert invalid target", func(t *testing.T) {
+		targetName := "some/invalid/relative/path/target"
+		pInvalidRelativePathTarget := projectAciHelper{
+			Services: []types.ServiceConfig{
+				{
+					Name: serviceName,
+					Secrets: []types.ServiceSecretConfig{
+						{
+							Source: secretName,
+							Target: targetName,
+						},
+					},
+				},
+			},
+			Secrets: map[string]types.SecretConfig{
+				secretName: {
+					File: tmpFile.Name(),
+				},
+			},
+		}
+		_, err := pInvalidRelativePathTarget.getAciSecretVolumes()
+		assert.Equal(t, err.Error(),
+			fmt.Sprintf(`in service %q, secret with source %q cannot have a relative path as target. Only absolute paths are allowed. Found %q`,
+				serviceName, secretName, targetName))
+	})
+
+	t.Run("convert colliding default targets", func(t *testing.T) {
+		targetName1 := filepath.Join(defaultSecretsPath, "target1")
+		targetName2 := filepath.Join(defaultSecretsPath, "sub/folder/target2")
+
+		service := serviceConfigAciHelper{
+			Name: serviceName,
+			Secrets: []types.ServiceSecretConfig{
+				{
+					Source: secretName,
+					Target: targetName1,
+				},
+				{
+					Source: secretName,
+					Target: targetName2,
+				},
+			},
+		}
+
+		_, err := service.getAciSecretsVolumeMounts()
+		assert.Equal(t, err.Error(),
+			fmt.Sprintf(`mount paths %q and %q collide. A volume mount cannot include another one.`,
+				filepath.Dir(targetName1), filepath.Dir(targetName2)))
+	})
+
+	t.Run("convert colliding absolute targets", func(t *testing.T) {
+		targetName1 := filepath.Join(absBasePath, "target1")
+		targetName2 := filepath.Join(absBasePath, "sub/folder/target2")
+
+		service := serviceConfigAciHelper{
+			Name: serviceName,
+			Secrets: []types.ServiceSecretConfig{
+				{
+					Source: secretName,
+					Target: targetName1,
+				},
+				{
+					Source: secretName,
+					Target: targetName2,
+				},
+			},
+		}
+
+		_, err := service.getAciSecretsVolumeMounts()
+		assert.Equal(t, err.Error(),
+			fmt.Sprintf(`mount paths %q and %q collide. A volume mount cannot include another one.`,
+				filepath.Dir(targetName1), filepath.Dir(targetName2)))
+	})
+}
+
 func container(status *string) containerinstance.Container {
 	var state *containerinstance.ContainerState = nil
 	if status != nil {

tests/aci-e2e/e2e-aci_test.go

@@ -564,19 +564,12 @@ func TestUpSecrets(t *testing.T) {
 		secret2Value = "another_password\n"
 	)
 	var (
-		basefilePath                 = filepath.Join("..", "composefiles", composeProjectName)
-		composefilePath              = filepath.Join(basefilePath, "compose.yml")
-		composefileInvalidTargetPath = filepath.Join(basefilePath, "compose-invalid-target.yml")
+		basefilePath    = filepath.Join("..", "composefiles", composeProjectName)
+		composefilePath = filepath.Join(basefilePath, "compose.yml")
 	)
 	c := NewParallelE2eCLI(t, binDir)
 	_, _, _ = setupTestResourceGroup(t, c)
 
-	t.Run("compose up invalid target", func(t *testing.T) {
-		res := c.RunDockerOrExitError("compose", "up", "-f", composefileInvalidTargetPath, "--project-name", composeProjectName)
-		assert.Equal(t, res.ExitCode, 1)
-		assert.Equal(t, res.Combined(), "in service \"web\", secret with source \"mysecret1\" cannot have a path as target. Found \"my/invalid/target1\"\n")
-	})
-
 	t.Run("compose up", func(t *testing.T) {
 		c.RunDockerCmd("compose", "up", "-f", composefilePath, "--project-name", composeProjectName)
 		res := c.RunDockerCmd("ps")

tests/composefiles/aci_secrets/compose-invalid-target.yml

@@ -1,16 +0,0 @@
-services:
-  web:
-    build: .
-    image: ulyssessouza/secrets_server
-    ports:
-      - "80:80"
-    secrets:
-      - source: mysecret1
-        target: my/invalid/target1
-      - mysecret2
-
-secrets:
-  mysecret1:
-    file: ./my_secret1.txt
-  mysecret2:
-    file: ./my_secret2.txt