Merge pull request #617 from ulyssessouza/volumes_ro

Add 'readOnly' capability to volumes on ACI

aci/convert/convert.go

@@ -49,6 +49,7 @@ const (
 	azureFileDriverName            = "azure_file"
 	volumeDriveroptsShareNameKey   = "share_name"
 	volumeDriveroptsAccountNameKey = "storage_account_name"
+	volumeReadOnly                 = "read_only"
 	secretInlineMark               = "inline:"
 )
 
@@ -69,9 +70,7 @@ func ToContainerGroup(ctx context.Context, aciContext store.AciContext, p types.
 	}
 	allVolumes := append(volumesSlice, secretVolumes...)
 	var volumes *[]containerinstance.Volume
-	if len(allVolumes) == 0 {
-		volumes = nil
-	} else {
+	if len(allVolumes) > 0 {
 		volumes = &allVolumes
 	}
 
@@ -213,6 +212,14 @@ func (p projectAciHelper) getAciFileVolumes(ctx context.Context, helper login.St
 			if !ok {
 				return nil, nil, fmt.Errorf("cannot retrieve account name for Azurefile")
 			}
+			readOnly, ok := v.DriverOpts[volumeReadOnly]
+			if !ok {
+				readOnly = "false"
+			}
+			ro, err := strconv.ParseBool(readOnly)
+			if err != nil {
+				return nil, nil, fmt.Errorf("invalid mode %q for volume", readOnly)
+			}
 			accountKey, err := helper.GetAzureStorageAccountKey(ctx, accountName)
 			if err != nil {
 				return nil, nil, err
@@ -223,6 +230,7 @@ func (p projectAciHelper) getAciFileVolumes(ctx context.Context, helper login.St
 					ShareName:          to.StringPtr(shareName),
 					StorageAccountName: to.StringPtr(accountName),
 					StorageAccountKey:  to.StringPtr(accountKey),
+					ReadOnly:           &ro,
 				},
 			}
 			azureFileVolumesMap[name] = true

aci/convert/volume.go

@@ -18,6 +18,7 @@ package convert
 
 import (
 	"fmt"
+	"strconv"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -38,18 +39,21 @@ func GetRunVolumes(volumes []string) (map[string]types.VolumeConfig, []types.Ser
 		if err != nil {
 			return nil, nil, err
 		}
+		readOnly := strconv.FormatBool(vi.readonly)
 		projectVolumes[vi.name] = types.VolumeConfig{
 			Name:   vi.name,
 			Driver: azureFileDriverName,
 			DriverOpts: map[string]string{
 				volumeDriveroptsAccountNameKey: vi.storageAccount,
 				volumeDriveroptsShareNameKey:   vi.fileshare,
+				volumeReadOnly:                 readOnly,
 			},
 		}
 		sv := types.ServiceVolumeConfig{
-			Type:   azureFileDriverName,
-			Source: vi.name,
-			Target: vi.target,
+			Type:     azureFileDriverName,
+			Source:   vi.name,
+			Target:   vi.target,
+			ReadOnly: vi.readonly,
 		}
 		serviceConfigVolumes = append(serviceConfigVolumes, sv)
 	}
@@ -62,26 +66,46 @@ type volumeInput struct {
 	storageAccount string
 	fileshare      string
 	target         string
+	readonly       bool
 }
 
-func (v *volumeInput) parse(name string, s string) error {
+// parse takes a candidate string and creates a volumeInput
+// Candidates take the form of <source>[:<target>][:<permissions>]
+// Source is of the form `<storage account>/<fileshare>`
+// If only the source is specified then the target is set to `/run/volumes/<fileshare>`
+// Target is an absolute path in the container of the form `/path/to/mount`
+// Permissions can only be set if the target is set
+// If set, permissions must be `rw` or `ro`
+func (v *volumeInput) parse(name string, candidate string) error {
 	v.name = name
-	tokens := strings.Split(s, ":")
-	source := tokens[0]
-	sourceTokens := strings.Split(source, "/")
-	if len(sourceTokens) < 2 || sourceTokens[0] == "" {
-		return errors.Wrapf(errdefs.ErrParsingFailed, "volume specification %q does not include a storage account before '/'", v)
+
+	tokens := strings.Split(candidate, ":")
+
+	sourceTokens := strings.Split(tokens[0], "/")
+	if len(sourceTokens) != 2 || sourceTokens[0] == "" {
+		return errors.Wrapf(errdefs.ErrParsingFailed, "volume specification %q does not include a storage account before '/'", candidate)
+	}
+	if sourceTokens[1] == "" {
+		return errors.Wrapf(errdefs.ErrParsingFailed, "volume specification %q does not include a storage file fileshare after '/'", candidate)
 	}
 	v.storageAccount = sourceTokens[0]
-	if sourceTokens[1] == "" {
-		return errors.Wrapf(errdefs.ErrParsingFailed, "volume specification %q does not include a storage file fileshare after '/'", v)
-	}
 	v.fileshare = sourceTokens[1]
 
-	if len(tokens) > 1 {
-		v.target = tokens[1]
-	} else {
+	switch len(tokens) {
+	case 1: // source only
 		v.target = "/run/volumes/" + v.fileshare
+	case 2: // source and target
+		v.target = tokens[1]
+	case 3: // source, target, and permissions
+		v.target = tokens[1]
+		permissions := strings.ToLower(tokens[2])
+		if permissions != "ro" && permissions != "rw" {
+			return errors.Wrapf(errdefs.ErrParsingFailed, "volume specification %q has an invalid mode %q", candidate, permissions)
+		}
+		v.readonly = permissions == "ro"
+	default:
+		return errors.Wrapf(errdefs.ErrParsingFailed, "volume specification %q has invalid format", candidate)
 	}
+
 	return nil
 }

aci/convert/volume_test.go

@@ -17,65 +17,31 @@
 package convert
 
 import (
+	"strconv"
 	"testing"
 
 	"github.com/compose-spec/compose-go/types"
 	"gotest.tools/v3/assert"
 )
 
-const (
-	storageAccountNameKey = "storage_account_name"
-	shareNameKey          = "share_name"
-)
-
 func TestGetRunVolumes(t *testing.T) {
 	volumeStrings := []string{
 		"myuser1/myshare1:/my/path/to/target1",
-		"myuser2/myshare2:/my/path/to/target2",
-		"myuser3/mydefaultsharename", // Use default placement at '/run/volumes/<share_name>'
+		"myuser2/myshare2:/my/path/to/target2:ro",
+		"myuser3/myshare3:/my/path/to/target3:rw",
+		"myuser4/mydefaultsharename", // Use default placement at '/run/volumes/<share_name>'
 	}
 	var goldenVolumeConfigs = map[string]types.VolumeConfig{
-		"volume-0": {
-			Name:   "volume-0",
-			Driver: "azure_file",
-			DriverOpts: map[string]string{
-				storageAccountNameKey: "myuser1",
-				shareNameKey:          "myshare1",
-			},
-		},
-		"volume-1": {
-			Name:   "volume-1",
-			Driver: "azure_file",
-			DriverOpts: map[string]string{
-				storageAccountNameKey: "myuser2",
-				shareNameKey:          "myshare2",
-			},
-		},
-		"volume-2": {
-			Name:   "volume-2",
-			Driver: "azure_file",
-			DriverOpts: map[string]string{
-				storageAccountNameKey: "myuser3",
-				shareNameKey:          "mydefaultsharename",
-			},
-		},
+		"volume-0": getAzurefileVolumeConfig("volume-0", "myuser1", "myshare1", false),
+		"volume-1": getAzurefileVolumeConfig("volume-1", "myuser2", "myshare2", true),
+		"volume-2": getAzurefileVolumeConfig("volume-2", "myuser3", "myshare3", false),
+		"volume-3": getAzurefileVolumeConfig("volume-3", "myuser4", "mydefaultsharename", false),
 	}
 	goldenServiceVolumeConfigs := []types.ServiceVolumeConfig{
-		{
-			Type:   "azure_file",
-			Source: "volume-0",
-			Target: "/my/path/to/target1",
-		},
-		{
-			Type:   "azure_file",
-			Source: "volume-1",
-			Target: "/my/path/to/target2",
-		},
-		{
-			Type:   "azure_file",
-			Source: "volume-2",
-			Target: "/run/volumes/mydefaultsharename",
-		},
+		getServiceVolumeConfig("volume-0", "/my/path/to/target1", false),
+		getServiceVolumeConfig("volume-1", "/my/path/to/target2", true),
+		getServiceVolumeConfig("volume-2", "/my/path/to/target3", false),
+		getServiceVolumeConfig("volume-3", "/run/volumes/mydefaultsharename", false),
 	}
 
 	volumeConfigs, serviceVolumeConfigs, err := GetRunVolumes(volumeStrings)
@@ -102,3 +68,29 @@ func TestGetRunVolumesNoShare(t *testing.T) {
 	_, _, err := GetRunVolumes([]string{"noshare"})
 	assert.ErrorContains(t, err, "does not include a storage account before '/'")
 }
+
+func TestGetRunVolumesInvalidOption(t *testing.T) {
+	_, _, err := GetRunVolumes([]string{"myuser4/myshare4:/my/path/to/target4:invalid"})
+	assert.ErrorContains(t, err, `volume specification "myuser4/myshare4:/my/path/to/target4:invalid" has an invalid mode "invalid"`)
+}
+
+func getServiceVolumeConfig(source string, target string, readOnly bool) types.ServiceVolumeConfig {
+	return types.ServiceVolumeConfig{
+		Type:     "azure_file",
+		Source:   source,
+		Target:   target,
+		ReadOnly: readOnly,
+	}
+}
+
+func getAzurefileVolumeConfig(name string, accountNameKey string, shareNameKey string, readOnly bool) types.VolumeConfig {
+	return types.VolumeConfig{
+		Name:   name,
+		Driver: "azure_file",
+		DriverOpts: map[string]string{
+			volumeDriveroptsAccountNameKey: accountNameKey,
+			volumeDriveroptsShareNameKey:   shareNameKey,
+			volumeReadOnly:                 strconv.FormatBool(readOnly),
+		},
+	}
+}

cli/cmd/run/run.go

@@ -47,7 +47,7 @@ func Command() *cobra.Command {
 	cmd.Flags().StringArrayVarP(&opts.Publish, "publish", "p", []string{}, "Publish a container's port(s). [HOST_PORT:]CONTAINER_PORT")
 	cmd.Flags().StringVar(&opts.Name, "name", "", "Assign a name to the container")
 	cmd.Flags().StringArrayVarP(&opts.Labels, "label", "l", []string{}, "Set meta data on a container")
-	cmd.Flags().StringArrayVarP(&opts.Volumes, "volume", "v", []string{}, "Volume. Ex: user:key@my_share:/absolute/path/to/target")
+	cmd.Flags().StringArrayVarP(&opts.Volumes, "volume", "v", []string{}, "Volume. Ex: storageaccount/my_share[:/absolute/path/to/target][:ro]")
 	cmd.Flags().BoolVarP(&opts.Detach, "detach", "d", false, "Run container in background and print container ID")
 	cmd.Flags().Float64Var(&opts.Cpus, "cpus", 1., "Number of CPUs")
 	cmd.Flags().VarP(&opts.Memory, "memory", "m", "Memory limit")

cli/cmd/run/testdata/run-help.golden

@@ -12,4 +12,4 @@ Flags:
       --name string           Assign a name to the container
   -p, --publish stringArray   Publish a container's port(s). [HOST_PORT:]CONTAINER_PORT
       --restart string        Restart policy to apply when a container exits (default "none")
-  -v, --volume stringArray    Volume. Ex: user:key@my_share:/absolute/path/to/target
+  -v, --volume stringArray    Volume. Ex: storageaccount/my_share[:/absolute/path/to/target][:ro]