Fix --volume flag for kpod create and run

Enable the --volume flag to validate user input
and add functionality for :z and :Z options

Signed-off-by: umohnani8 <umohnani@redhat.com>

Closes: #84
Approved by: mheon

cmd/kpod/create.go

@@ -343,6 +343,10 @@ func parseCreateOpts(c *cli.Context, runtime *libpod.Runtime) (*createConfig, er
 		blkioWeight = uint16(u)
 	}
 
+	if err = parseVolumes(c.StringSlice("volume")); err != nil {
+		return nil, err
+	}
+
 	// Because we cannot do a non-terminal attach, we need to set tty to true
 	// if detach is not false
 	// TODO Allow non-terminal attach

cmd/kpod/create_cli.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"fmt"
+	"os"
 	"strings"
 
 	"github.com/docker/docker/pkg/sysinfo"
@@ -40,6 +41,71 @@ func addWarning(warnings []string, msg string) []string {
 	return append(warnings, msg)
 }
 
+func parseVolumes(volumes []string) error {
+	if len(volumes) == 0 {
+		return nil
+	}
+	for _, volume := range volumes {
+		arr := strings.SplitN(volume, ":", 3)
+		if len(arr) < 2 {
+			return errors.Errorf("incorrect volume format %q, should be host-dir:ctr-dir:[option]", volume)
+		}
+		if err := validateVolumeHostDir(arr[0]); err != nil {
+			return err
+		}
+		if err := validateVolumeCtrDir(arr[1]); err != nil {
+			return err
+		}
+		if len(arr) > 2 {
+			if err := validateVolumeOpts(arr[2]); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+func validateVolumeHostDir(hostDir string) error {
+	if _, err := os.Stat(hostDir); err != nil {
+		return errors.Wrapf(err, "error checking path %q", hostDir)
+	}
+	return nil
+}
+
+func validateVolumeCtrDir(ctrDir string) error {
+	if ctrDir[0] != '/' {
+		return errors.Errorf("invalid container directory path %q", ctrDir)
+	}
+	return nil
+}
+
+func validateVolumeOpts(option string) error {
+	var foundRootPropagation, foundRWRO, foundLabelChange int
+	options := strings.Split(option, ",")
+	for _, opt := range options {
+		switch opt {
+		case "rw", "ro":
+			if foundRWRO > 1 {
+				return errors.Errorf("invalid options %q, can only specify 1 'rw' or 'ro' option", option)
+			}
+			foundRWRO++
+		case "z", "Z":
+			if foundLabelChange > 1 {
+				return errors.Errorf("invalid options %q, can only specify 1 'z' or 'Z' option", option)
+			}
+			foundLabelChange++
+		case "private", "rprivate", "shared", "rshared", "slave", "rslave":
+			if foundRootPropagation > 1 {
+				return errors.Errorf("invalid options %q, can only specify 1 '[r]shared', '[r]private' or '[r]slave' option", option)
+			}
+			foundRootPropagation++
+		default:
+			return errors.Errorf("invalid option type %q", option)
+		}
+	}
+	return nil
+}
+
 func verifyContainerResources(config *createConfig, update bool) ([]string, error) {
 	warnings := []string{}
 	sysInfo := sysinfo.New(true)

cmd/kpod/spec.go

@@ -11,6 +11,7 @@ import (
 	"github.com/docker/go-units"
 	spec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/opencontainers/runtime-tools/generate"
+	"github.com/opencontainers/selinux/go-selinux/label"
 	"github.com/pkg/errors"
 	"github.com/projectatomic/libpod/libpod"
 	ann "github.com/projectatomic/libpod/pkg/annotations"
@@ -224,7 +225,11 @@ func createConfigToOCISpec(config *createConfig) (*spec.Spec, error) {
 	}
 
 	// BIND MOUNTS
-	configSpec.Mounts = append(configSpec.Mounts, config.GetVolumeMounts()...)
+	mounts, err := config.GetVolumeMounts()
+	if err != nil {
+		return nil, errors.Wrapf(err, "error getting volume mounts")
+	}
+	configSpec.Mounts = append(configSpec.Mounts, mounts...)
 
 	// HANDLE CAPABILITIES
 	if err := setupCapabilities(config, configSpec); err != nil {
@@ -357,7 +362,7 @@ func getDefaultAnnotations() map[string]string {
 }
 
 //GetVolumeMounts takes user provided input for bind mounts and creates Mount structs
-func (c *createConfig) GetVolumeMounts() []spec.Mount {
+func (c *createConfig) GetVolumeMounts() ([]spec.Mount, error) {
 	var m []spec.Mount
 	var options []string
 	for _, i := range c.volumes {
@@ -366,8 +371,36 @@ func (c *createConfig) GetVolumeMounts() []spec.Mount {
 		if len(spliti) > 2 {
 			options = strings.Split(spliti[2], ",")
 		}
-		// always add rbind bc mount ignores the bind filesystem when mounting
 		options = append(options, "rbind")
+		// var foundrw, foundro,
+		var foundz, foundZ bool
+		for _, opt := range options {
+			switch opt {
+			// case "rw":
+			// 	foundrw = true
+			// case "ro":
+			// 	foundro = true
+			case "z":
+				foundz = true
+			case "Z":
+				foundZ = true
+			}
+		}
+		// if !foundro && !foundrw {
+		// 	// rw option is default
+		// 	options = append(options, "rw")
+		// }
+		if foundz {
+			if err := label.Relabel(spliti[0], c.mountLabel, true); err != nil {
+				return nil, errors.Wrapf(err, "relabel failed %q", spliti[0])
+			}
+		}
+		if foundZ {
+			if err := label.Relabel(spliti[0], c.mountLabel, false); err != nil {
+				return nil, errors.Wrapf(err, "relabel failed %q", spliti[0])
+			}
+		}
+
 		m = append(m, spec.Mount{
 			Destination: spliti[1],
 			Type:        string(TypeBind),
@@ -375,7 +408,7 @@ func (c *createConfig) GetVolumeMounts() []spec.Mount {
 			Options:     options,
 		})
 	}
-	return m
+	return m, nil
 }
 
 //GetTmpfsMounts takes user provided input for tmpfs mounts and creates Mount structs

cmd/kpod/spec_test.go

@@ -18,7 +18,8 @@ func TestCreateConfig_GetVolumeMounts(t *testing.T) {
 	config := createConfig{
 		volumes: []string{"foobar:/foobar:ro"},
 	}
-	specMount := config.GetVolumeMounts()
+	specMount, err := config.GetVolumeMounts()
+	assert.NoError(t, err)
 	assert.True(t, reflect.DeepEqual(data, specMount[0]))
 }
 

docs/kpod-create.1.md

@@ -484,26 +484,18 @@ standard input.
      **host**: use the host's UTS namespace inside the container.
      Note: the host mode gives the container access to changing the host's hostname and is therefore considered insecure.
 
-**-v**|**--volume**[=*[[HOST-DIR:]CONTAINER-DIR[:OPTIONS]]*]
+**-v**|**--volume**[=*[HOST-DIR:CONTAINER-DIR[:OPTIONS]]*]
    Create a bind mount. If you specify, ` -v /HOST-DIR:/CONTAINER-DIR`, kpod
    bind mounts `/HOST-DIR` in the host to `/CONTAINER-DIR` in the kpod
-   container. If 'HOST-DIR' is omitted,  kpod automatically creates the new
+   container. The `OPTIONS` are a comma delimited list and can be:
-   volume on the host.  The `OPTIONS` are a comma delimited list and can be:
 
    * [rw|ro]
    * [z|Z]
    * [`[r]shared`|`[r]slave`|`[r]private`]
 
 The `CONTAINER-DIR` must be an absolute path such as `/src/docs`. The `HOST-DIR`
-can be an absolute path or a `name` value. A `name` value must start with an
+must be an absolute path as well. kpod bind-mounts the `HOST-DIR` to the
-alphanumeric character, followed by `a-z0-9`, `_` (underscore), `.` (period) or
+path you specify. For example, if you supply the `/foo` value, kpod creates a bind-mount.
-`-` (hyphen). An absolute path starts with a `/` (forward slash).
-
-If you supply a `HOST-DIR` that is an absolute path,  kpod bind-mounts to the
-path you specify. If you supply a `name`, kpod creates a named volume by that
-`name`. For example, you can specify either `/foo` or `foo` for a `HOST-DIR`
-value. If you supply the `/foo` value, kpod creates a bind-mount. If you
-supply the `foo` specification, kpod creates a named volume.
 
 You can specify multiple  **-v** options to mount one or more mounts to a
 container. To use these same mounts in other containers, specify the

docs/kpod-run.1.md

@@ -489,27 +489,18 @@ standard input.
      **host**: use the host's UTS namespace inside the container.
      Note: the host mode gives the container access to changing the host's hostname and is therefore considered insecure.
 
-**-v**|**--volume**[=*[[HOST-DIR:]CONTAINER-DIR[:OPTIONS]]*]
+**-v**|**--volume**[=*[HOST-DIR:CONTAINER-DIR[:OPTIONS]]*]
    Create a bind mount. If you specify, ` -v /HOST-DIR:/CONTAINER-DIR`, kpod
    bind mounts `/HOST-DIR` in the host to `/CONTAINER-DIR` in the kpod
-   container. If 'HOST-DIR' is omitted,  kpod automatically creates the new
+   container. The `OPTIONS` are a comma delimited list and can be:
-   volume on the host.  The `OPTIONS` are a comma delimited list and can be:
 
    * [rw|ro]
    * [z|Z]
    * [`[r]shared`|`[r]slave`|`[r]private`]
-   * [nocopy]
 
 The `CONTAINER-DIR` must be an absolute path such as `/src/docs`. The `HOST-DIR`
-can be an absolute path or a `name` value. A `name` value must start with an
+must be an absolute path as well. kpod bind-mounts the `HOST-DIR` to the
-alphanumeric character, followed by `a-z0-9`, `_` (underscore), `.` (period) or
+path you specify. For example, if you supply the `/foo` value, kpod creates a bind-mount.
-`-` (hyphen). An absolute path starts with a `/` (forward slash).
-
-If you supply a `HOST-DIR` that is an absolute path,  kpod bind-mounts to the
-path you specify. If you supply a `name`, kpod creates a named volume by that
-`name`. For example, you can specify either `/foo` or `foo` for a `HOST-DIR`
-value. If you supply the `/foo` value, kpod creates a bind-mount. If you
-supply the `foo` specification, kpod creates a named volume.
 
 You can specify multiple  **-v** options to mount one or more mounts to a
 container. To use these same mounts in other containers, specify the

test/kpod_run.bats

@@ -120,3 +120,18 @@ IMAGE="docker.io/library/fedora:latest"
     [ "$output" = 100 ]
 
 }
+
+@test "kpod run with volume flag" {
+    run bash -c "${KPOD_BINARY} ${KPOD_OPTIONS} run -v ${MOUNT_PATH}:/run/test ${FEDORA_MINIMAL} cat /proc/self/mountinfo | grep '${MOUNT_PATH} /run/test rw,relatime'"
+    echo $output
+    [ "$status" -eq 0 ]
+    run bash -c "${KPOD_BINARY} ${KPOD_OPTIONS} run -v ${MOUNT_PATH}:/run/test:ro ${FEDORA_MINIMAL} cat /proc/self/mountinfo | grep '${MOUNT_PATH} /run/test ro,relatime'"
+    echo $output
+    [ "$status" -eq 0 ]
+    #run bash -c "${KPOD_BINARY} ${KPOD_OPTIONS} run -v ${MOUNT_PATH}:/run/test:shared ${FEDORA_MINIMAL} cat /proc/self/mountinfo | grep '${MOUNT_PATH} /run/test rw,relatime shared:'"
+    #echo $output
+    #[ "$status" -eq 0 ]
+    #run bash -c "${KPOD_BINARY} ${KPOD_OPTIONS} run -v ${MOUNT_PATH}:/run/test:rslave ${FEDORA_MINIMAL} cat /proc/self/mountinfo | grep '${MOUNT_PATH} /run/test rw,relatime master:'"
+    #echo $output
+    #[ "$status" -eq 0 ]
+}