From 02d59ae5101c81f613a0647e62a0f51b0a4a47ad Mon Sep 17 00:00:00 2001 From: Ulysses Souza Date: Mon, 14 Sep 2020 18:32:38 +0200 Subject: [PATCH] Add 'readOnly' capability to volumes on ACI Signed-off-by: Ulysses Souza --- aci/convert/convert.go | 14 ++++- aci/convert/volume.go | 54 +++++++++++++----- aci/convert/volume_test.go | 84 +++++++++++++--------------- cli/cmd/run/run.go | 2 +- cli/cmd/run/testdata/run-help.golden | 2 +- 5 files changed, 90 insertions(+), 66 deletions(-) diff --git a/aci/convert/convert.go b/aci/convert/convert.go index b3db862bc..17d3ad6c8 100644 --- a/aci/convert/convert.go +++ b/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 diff --git a/aci/convert/volume.go b/aci/convert/volume.go index 60daac3bb..171741a46 100644 --- a/aci/convert/volume.go +++ b/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 is of the form `/` +// If only the source is specified then the target is set to `/run/volumes/` +// 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 } diff --git a/aci/convert/volume_test.go b/aci/convert/volume_test.go index fd35c93bb..f283891fe 100644 --- a/aci/convert/volume_test.go +++ b/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/' + "myuser2/myshare2:/my/path/to/target2:ro", + "myuser3/myshare3:/my/path/to/target3:rw", + "myuser4/mydefaultsharename", // Use default placement at '/run/volumes/' } 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), + }, + } +} diff --git a/cli/cmd/run/run.go b/cli/cmd/run/run.go index a4283eba9..e8edf4be6 100644 --- a/cli/cmd/run/run.go +++ b/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") diff --git a/cli/cmd/run/testdata/run-help.golden b/cli/cmd/run/testdata/run-help.golden index 645257263..1b499106e 100644 --- a/cli/cmd/run/testdata/run-help.golden +++ b/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]