Skip to content

Commit

Permalink
Migrate to unified volume handling code
Browse files Browse the repository at this point in the history 
Unify handling for the --volume, --mount, --volumes-from, --tmpfs
and --init flags into a single file and set of functions. This
will greatly improve readability and maintainability.

Further, properly handle superceding and conflicting mounts. Our
current patchwork has serious issues when mounts conflict, or
when a mount from --volumes-from or an image volume should be
overwritten by a user volume or named volume.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>
  • Loading branch information
@mheon
mheon committed May 1, 2019
1 parent 71f65ab commit 9ee50fe
Show file tree
Hide file tree
Showing 9 changed files with 841 additions and 564 deletions.
2 changes: 0 additions & 2 deletions .tool/lint
View file
Expand Up @@ -25,7 +25,6 @@ ${LINTER} \
--deadline=600s --disable-all\
--enable=deadcode\
--enable=errcheck\
--enable=goconst\
--enable=gofmt\
--enable=golint\
--enable=ineffassign\
Expand All @@ -41,7 +40,6 @@ ${LINTER} \
--exclude='duplicate of.*_test.go.*\(dupl\)$'\
--exclude='cmd\/client\/.*\.go.*\(dupl\)$'\
--exclude='libpod\/.*_easyjson.go:.*'\
--exclude='.* other occurrence\(s\) of "(container|host|tmpfs|unknown)" found in: .*\(goconst\)$'\
--exclude='vendor\/.*'\
--exclude='podman\/.*'\
--exclude='server\/seccomp\/.*\.go.*$'\
Expand Down
5 changes: 3 additions & 2 deletions cmd/podman/play_kube.go
View file
Expand Up @@ -205,7 +205,8 @@ func playKubeYAMLCmd(c *cliconfig.KubePlayValues, ctx context.Context, runtime *
return pod, errors.Errorf("Directories are the only supported HostPath type")
}
}
if err := shared.ValidateVolumeHostDir(hostPath.Path); err != nil {

if err := createconfig.ValidateVolumeHostDir(hostPath.Path); err != nil {
return pod, errors.Wrapf(err, "Error in parsing HostPath in YAML")
}
volumes[volume.Name] = hostPath.Path
Expand Down Expand Up @@ -351,7 +352,7 @@ func kubeContainerToCreateConfig(ctx context.Context, containerYAML v1.Container
if !exists {
return nil, errors.Errorf("Volume mount %s specified for container but not configured in volumes", volume.Name)
}
if err := shared.ValidateVolumeCtrDir(volume.MountPath); err != nil {
if err := createconfig.ValidateVolumeCtrDir(volume.MountPath); err != nil {
return nil, errors.Wrapf(err, "error in parsing MountPath")
}
containerConfig.Volumes = append(containerConfig.Volumes, fmt.Sprintf("%s:%s", host_path, volume.MountPath))
Expand Down
30 changes: 3 additions & 27 deletions cmd/podman/shared/create.go
View file
Expand Up @@ -25,7 +25,6 @@ import (
"github.com/docker/go-connections/nat"
"github.com/docker/go-units"
"github.com/google/shlex"
spec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/opencontainers/selinux/go-selinux/label"
"github.com/opentracing/opentracing-go"
"github.com/pkg/errors"
Expand Down Expand Up @@ -341,18 +340,6 @@ func ParseCreateOpts(ctx context.Context, c *GenericCLIResults, runtime *libpod.
}
blkioWeight = uint16(u)
}
var mountList []spec.Mount
if mountList, err = parseMounts(c.StringArray("mount")); err != nil {
return nil, err
}

if err = parseVolumes(c.StringArray("volume")); err != nil {
return nil, err
}

if err = parseVolumesFrom(c.StringSlice("volumes-from")); err != nil {
return nil, err
}

tty := c.Bool("tty")

Expand Down Expand Up @@ -636,6 +623,8 @@ func ParseCreateOpts(ctx context.Context, c *GenericCLIResults, runtime *libpod.
HTTPProxy: c.Bool("http-proxy"),
NoHosts: c.Bool("no-hosts"),
IDMappings: idmappings,
Init: c.Bool("init"),
InitPath: c.String("init-path"),
Image: imageName,
ImageID: imageID,
Interactive: c.Bool("interactive"),
Expand Down Expand Up @@ -696,26 +685,13 @@ func ParseCreateOpts(ctx context.Context, c *GenericCLIResults, runtime *libpod.
Tty: tty,
User: user,
UsernsMode: usernsMode,
Mounts: mountList,
MountsFlag: c.StringArray("mount"),
Volumes: c.StringArray("volume"),
WorkDir: workDir,
Rootfs: rootfs,
VolumesFrom: c.StringSlice("volumes-from"),
Syslog: c.Bool("syslog"),
}
if c.Bool("init") {
initPath := c.String("init-path")
if initPath == "" {
rtc, err := runtime.GetConfig()
if err != nil {
return nil, err
}
initPath = rtc.InitPath
}
if err := config.AddContainerInitBinary(initPath); err != nil {
return nil, err
}
}

if config.Privileged {
config.LabelOpts = label.DisableSecOpt()
Expand Down
184 changes: 0 additions & 184 deletions cmd/podman/shared/create_cli.go
View file
Expand Up @@ -2,15 +2,11 @@ package shared

import (
"fmt"
"os"
"path/filepath"
"strings"

"github.com/containers/libpod/cmd/podman/shared/parse"
cc "github.com/containers/libpod/pkg/spec"
"github.com/containers/libpod/pkg/sysinfo"
"github.com/docker/go-units"
spec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
)
Expand Down Expand Up @@ -78,186 +74,6 @@ func addWarning(warnings []string, msg string) []string {
return append(warnings, msg)
}

// Format supported.
// podman run --mount type=bind,src=/etc/resolv.conf,target=/etc/resolv.conf ...
// podman run --mount type=tmpfs,target=/dev/shm ..
func parseMounts(mounts []string) ([]spec.Mount, error) {
// TODO(vrothberg): the manual parsing can be replaced with a regular expression
// to allow a more robust parsing of the mount format and to give
// precise errors regarding supported format versus suppored options.
var mountList []spec.Mount
errInvalidSyntax := errors.Errorf("incorrect mount format: should be --mount type=<bind|tmpfs>,[src=<host-dir>,]target=<ctr-dir>[,options]")
for _, mount := range mounts {
var tokenCount int
var mountInfo spec.Mount

arr := strings.SplitN(mount, ",", 2)
if len(arr) < 2 {
return nil, errors.Wrapf(errInvalidSyntax, "%q", mount)
}
kv := strings.Split(arr[0], "=")
if kv[0] != "type" {
return nil, errors.Wrapf(errInvalidSyntax, "%q", mount)
}
switch kv[1] {
case "bind":
mountInfo.Type = string(cc.TypeBind)
case "tmpfs":
mountInfo.Type = string(cc.TypeTmpfs)
mountInfo.Source = string(cc.TypeTmpfs)
mountInfo.Options = append(mountInfo.Options, []string{"rprivate", "noexec", "nosuid", "nodev", "size=65536k"}...)

default:
return nil, errors.Errorf("invalid filesystem type %q", kv[1])
}

tokens := strings.Split(arr[1], ",")
for i, val := range tokens {
if i == (tokenCount - 1) {
//Parse tokens before options.
break
}
kv := strings.Split(val, "=")
switch kv[0] {
case "ro", "nosuid", "nodev", "noexec":
mountInfo.Options = append(mountInfo.Options, kv[0])
case "shared", "rshared", "private", "rprivate", "slave", "rslave", "Z", "z":
if mountInfo.Type != "bind" {
return nil, errors.Errorf("%s can only be used with bind mounts", kv[0])
}
mountInfo.Options = append(mountInfo.Options, kv[0])
case "tmpfs-mode":
if mountInfo.Type != "tmpfs" {
return nil, errors.Errorf("%s can only be used with tmpfs mounts", kv[0])
}
mountInfo.Options = append(mountInfo.Options, fmt.Sprintf("mode=%s", kv[1]))
case "tmpfs-size":
if mountInfo.Type != "tmpfs" {
return nil, errors.Errorf("%s can only be used with tmpfs mounts", kv[0])
}
shmSize, err := units.FromHumanSize(kv[1])
if err != nil {
return nil, errors.Wrapf(err, "unable to translate tmpfs-size")
}

mountInfo.Options = append(mountInfo.Options, fmt.Sprintf("size=%d", shmSize))

case "bind-propagation":
if mountInfo.Type != "bind" {
return nil, errors.Errorf("%s can only be used with bind mounts", kv[0])
}
mountInfo.Options = append(mountInfo.Options, kv[1])
case "src", "source":
if mountInfo.Type == "tmpfs" {
return nil, errors.Errorf("cannot use src= on a tmpfs file system")
}
if err := ValidateVolumeHostDir(kv[1]); err != nil {
return nil, err
}
mountInfo.Source = kv[1]
case "target", "dst", "destination":
if err := ValidateVolumeCtrDir(kv[1]); err != nil {
return nil, err
}
mountInfo.Destination = kv[1]
default:
return nil, errors.Errorf("incorrect mount option : %s", kv[0])
}
}
mountList = append(mountList, mountInfo)
}
return mountList, nil
}

func parseVolumes(volumes []string) error {
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 parseVolumesFrom(volumesFrom []string) error {
for _, vol := range volumesFrom {
arr := strings.SplitN(vol, ":", 2)
if len(arr) == 2 {
if strings.Contains(arr[1], "Z") || strings.Contains(arr[1], "private") || strings.Contains(arr[1], "slave") || strings.Contains(arr[1], "shared") {
return errors.Errorf("invalid options %q, can only specify 'ro', 'rw', and 'z", arr[1])
}
if err := validateVolumeOpts(arr[1]); err != nil {
return err
}
}
}
return nil
}

// ValidateVolumeHostDir ...
func ValidateVolumeHostDir(hostDir string) error {
if len(hostDir) == 0 {
return errors.Errorf("host directory cannot be empty")
}
if filepath.IsAbs(hostDir) {
if _, err := os.Stat(hostDir); err != nil {
return errors.Wrapf(err, "error checking path %q", hostDir)
}
}
// If hostDir is not an absolute path, that means the user wants to create a
// named volume. This will be done later on in the code.
return nil
}

// ValidateVolumeCtrDir ...
func ValidateVolumeCtrDir(ctrDir string) error {
if len(ctrDir) == 0 {
return errors.Errorf("container directory cannot be empty")
}
if !filepath.IsAbs(ctrDir) {
return errors.Errorf("invalid container path, must be an absolute 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":
foundRWRO++
if foundRWRO > 1 {
return errors.Errorf("invalid options %q, can only specify 1 'rw' or 'ro' option", option)
}
case "z", "Z":
foundLabelChange++
if foundLabelChange > 1 {
return errors.Errorf("invalid options %q, can only specify 1 'z' or 'Z' option", option)
}
case "private", "rprivate", "shared", "rshared", "slave", "rslave":
foundRootPropagation++
if foundRootPropagation > 1 {
return errors.Errorf("invalid options %q, can only specify 1 '[r]shared', '[r]private' or '[r]slave' option", option)
}
default:
return errors.Errorf("invalid option type %q", option)
}
}
return nil
}

func verifyContainerResources(config *cc.CreateConfig, update bool) ([]string, error) {
warnings := []string{}
sysInfo := sysinfo.New(true)
Expand Down
17 changes: 15 additions & 2 deletions pkg/spec/containerconfig.go
View file
Expand Up @@ -4,6 +4,7 @@ import (
"github.com/containers/libpod/libpod"
spec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
)

// MakeContainerConfig generates all configuration necessary to start a
Expand All @@ -15,15 +16,27 @@ func (config *CreateConfig) MakeContainerConfig(runtime *libpod.Runtime, pod *li
return nil, nil, errors.Wrapf(libpod.ErrInvalidArg, "pod was given but no pod is specified")
}

runtimeSpec, namedVolumes, err := config.createConfigToOCISpec(runtime)
// Parse volumes flag into OCI spec mounts and libpod Named Volumes.
// If there is an identical mount in the OCI spec, we will replace it
// with a mount generated here.
mounts, namedVolumes, err := config.parseVolumes(runtime)
if err != nil {
return nil, nil, err
}

options, err := config.getContainerCreateOptions(runtime, pod, namedVolumes)
logrus.Debugf("got mounts as %v", mounts)

runtimeSpec, err := config.createConfigToOCISpec(runtime, mounts)
if err != nil {
return nil, nil, err
}

options, err := config.getContainerCreateOptions(runtime, pod, mounts, namedVolumes)
if err != nil {
return nil, nil, err
}

logrus.Debugf("created OCI spec and options for new container")

return runtimeSpec, options, nil
}

0 comments on commit 9ee50fe

Please sign in to comment.