From 869466eb25a41ab0e6daf9bc6ab951d7300d3f9d Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 15 Apr 2019 13:39:22 -0400 Subject: Add a new function for converting a CreateConfig Right now, there are two major API calls necessary to turn a filled-in CreateConfig into the options and OCI spec necessary to make a libpod Container. I'm intending on refactoring both of these extensively to unify a few things, so make a common frontend to both that will prevent API changes from leaking out of the package. Signed-off-by: Matthew Heon --- pkg/spec/containerconfig.go | 22 ++++++++++++++++++++++ pkg/spec/createconfig.go | 2 +- pkg/spec/spec.go | 2 +- 3 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 pkg/spec/containerconfig.go (limited to 'pkg') diff --git a/pkg/spec/containerconfig.go b/pkg/spec/containerconfig.go new file mode 100644 index 000000000..62108c012 --- /dev/null +++ b/pkg/spec/containerconfig.go @@ -0,0 +1,22 @@ +package createconfig + +import ( + "github.com/containers/libpod/libpod" + spec "github.com/opencontainers/runtime-spec/specs-go" +) + +// MakeContainerConfig generates all configuration necessary to start a +// container with libpod from a completed CreateConfig struct. +func (config *CreateConfig) MakeContainerConfig(runtime *libpod.Runtime, pod *libpod.Pod) (*spec.Spec, []libpod.CtrCreateOption, error) { + runtimeSpec, err := config.createConfigToOCISpec() + if err != nil { + return nil, nil, err + } + + options, err := config.getContainerCreateOptions(runtime, pod) + if err != nil { + return nil, nil, err + } + + return runtimeSpec, options, nil +} diff --git a/pkg/spec/createconfig.go b/pkg/spec/createconfig.go index 9c674d9f1..76ce8032c 100644 --- a/pkg/spec/createconfig.go +++ b/pkg/spec/createconfig.go @@ -397,7 +397,7 @@ func (c *CreateConfig) createExitCommand() ([]string, error) { } // GetContainerCreateOptions takes a CreateConfig and returns a slice of CtrCreateOptions -func (c *CreateConfig) GetContainerCreateOptions(runtime *libpod.Runtime, pod *libpod.Pod) ([]libpod.CtrCreateOption, error) { +func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *libpod.Pod) ([]libpod.CtrCreateOption, error) { var options []libpod.CtrCreateOption var portBindings []ocicni.PortMapping var err error diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index 383eeadf3..4c839921c 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -89,7 +89,7 @@ func getAvailableGids() (int64, error) { } // CreateConfigToOCISpec parses information needed to create a container into an OCI runtime spec -func CreateConfigToOCISpec(config *CreateConfig) (*spec.Spec, error) { //nolint +func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //nolint cgroupPerm := "ro" g, err := generate.New("linux") if err != nil { -- cgit v1.2.3-54-g00ecf From 4540458a5e2809d2ecc2a1c6e67e9752529c1126 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 15 Apr 2019 13:56:47 -0400 Subject: Remove non-config fields from CreateConfig The goal here is to keep only the configuration directly used to build the container in CreateConfig, and scrub temporary state and helpers that we need to generate. We'll keep those internally in MakeContainerConfig. Signed-off-by: Matthew Heon --- cmd/podman/play_kube.go | 1 - cmd/podman/shared/create.go | 9 ++++----- pkg/spec/containerconfig.go | 4 ++-- pkg/spec/createconfig.go | 26 ++++++++++++------------- pkg/spec/spec.go | 47 ++++++++++++++++++++++----------------------- 5 files changed, 41 insertions(+), 46 deletions(-) (limited to 'pkg') diff --git a/cmd/podman/play_kube.go b/cmd/podman/play_kube.go index 967798399..2e51457c4 100644 --- a/cmd/podman/play_kube.go +++ b/cmd/podman/play_kube.go @@ -281,7 +281,6 @@ func kubeContainerToCreateConfig(ctx context.Context, containerYAML v1.Container // The default for MemorySwappiness is -1, not 0 containerConfig.Resources.MemorySwappiness = -1 - containerConfig.Runtime = runtime containerConfig.Image = containerYAML.Image containerConfig.ImageID = newImage.ID() containerConfig.Name = containerYAML.Name diff --git a/cmd/podman/shared/create.go b/cmd/podman/shared/create.go index a1b7cee1c..ab695bcf4 100644 --- a/cmd/podman/shared/create.go +++ b/cmd/podman/shared/create.go @@ -139,7 +139,7 @@ func CreateContainer(ctx context.Context, c *GenericCLIResults, runtime *libpod. return ctr, createConfig, nil } -func parseSecurityOpt(config *cc.CreateConfig, securityOpts []string) error { +func parseSecurityOpt(config *cc.CreateConfig, securityOpts []string, runtime *libpod.Runtime) error { var ( labelOpts []string ) @@ -147,7 +147,7 @@ func parseSecurityOpt(config *cc.CreateConfig, securityOpts []string) error { if config.PidMode.IsHost() { labelOpts = append(labelOpts, label.DisableSecOpt()...) } else if config.PidMode.IsContainer() { - ctr, err := config.Runtime.LookupContainer(config.PidMode.Container()) + ctr, err := runtime.LookupContainer(config.PidMode.Container()) if err != nil { return errors.Wrapf(err, "container %q not found", config.PidMode.Container()) } @@ -161,7 +161,7 @@ func parseSecurityOpt(config *cc.CreateConfig, securityOpts []string) error { if config.IpcMode.IsHost() { labelOpts = append(labelOpts, label.DisableSecOpt()...) } else if config.IpcMode.IsContainer() { - ctr, err := config.Runtime.LookupContainer(config.IpcMode.Container()) + ctr, err := runtime.LookupContainer(config.IpcMode.Container()) if err != nil { return errors.Wrapf(err, "container %q not found", config.IpcMode.Container()) } @@ -604,7 +604,6 @@ func ParseCreateOpts(ctx context.Context, c *GenericCLIResults, runtime *libpod. memorySwappiness := c.Int64("memory-swappiness") config := &cc.CreateConfig{ - Runtime: runtime, Annotations: annotations, BuiltinImgVolumes: ImageVolumes, ConmonPidFile: c.String("conmon-pidfile"), @@ -711,7 +710,7 @@ func ParseCreateOpts(ctx context.Context, c *GenericCLIResults, runtime *libpod. if config.Privileged { config.LabelOpts = label.DisableSecOpt() } else { - if err := parseSecurityOpt(config, c.StringArray("security-opt")); err != nil { + if err := parseSecurityOpt(config, c.StringArray("security-opt"), runtime); err != nil { return nil, err } } diff --git a/pkg/spec/containerconfig.go b/pkg/spec/containerconfig.go index 62108c012..775a2042d 100644 --- a/pkg/spec/containerconfig.go +++ b/pkg/spec/containerconfig.go @@ -8,12 +8,12 @@ import ( // MakeContainerConfig generates all configuration necessary to start a // container with libpod from a completed CreateConfig struct. func (config *CreateConfig) MakeContainerConfig(runtime *libpod.Runtime, pod *libpod.Pod) (*spec.Spec, []libpod.CtrCreateOption, error) { - runtimeSpec, err := config.createConfigToOCISpec() + runtimeSpec, namedVolumes, err := config.createConfigToOCISpec(runtime) if err != nil { return nil, nil, err } - options, err := config.getContainerCreateOptions(runtime, pod) + options, err := config.getContainerCreateOptions(runtime, pod, namedVolumes) if err != nil { return nil, nil, err } diff --git a/pkg/spec/createconfig.go b/pkg/spec/createconfig.go index 76ce8032c..d2ae99de6 100644 --- a/pkg/spec/createconfig.go +++ b/pkg/spec/createconfig.go @@ -65,7 +65,6 @@ type CreateResourceConfig struct { // CreateConfig is a pre OCI spec structure. It represents user input from varlink or the CLI type CreateConfig struct { - Runtime *libpod.Runtime Annotations map[string]string Args []string CapAdd []string // cap-add @@ -129,7 +128,6 @@ type CreateConfig struct { Mounts []spec.Mount //mounts Volumes []string //volume VolumesFrom []string - NamedVolumes []*libpod.ContainerNamedVolume // Filled in by CreateConfigToOCISpec WorkDir string //workdir LabelOpts []string //SecurityOpts NoNewPrivs bool //SecurityOpts @@ -268,7 +266,7 @@ func (c *CreateConfig) GetVolumeMounts(specMounts []spec.Mount) ([]spec.Mount, e // GetVolumesFrom reads the create-config artifact of the container to get volumes from // and adds it to c.Volumes of the current container. -func (c *CreateConfig) GetVolumesFrom() error { +func (c *CreateConfig) GetVolumesFrom(runtime *libpod.Runtime) error { if os.Geteuid() != 0 { return nil } @@ -279,7 +277,7 @@ func (c *CreateConfig) GetVolumesFrom() error { if len(splitVol) == 2 { options = splitVol[1] } - ctr, err := c.Runtime.LookupContainer(splitVol[0]) + ctr, err := runtime.LookupContainer(splitVol[0]) if err != nil { return errors.Wrapf(err, "error looking up container %q", splitVol[0]) } @@ -364,8 +362,8 @@ func (c *CreateConfig) GetTmpfsMounts() []spec.Mount { return m } -func (c *CreateConfig) createExitCommand() ([]string, error) { - config, err := c.Runtime.GetConfig() +func (c *CreateConfig) createExitCommand(runtime *libpod.Runtime) ([]string, error) { + config, err := runtime.GetConfig() if err != nil { return nil, err } @@ -397,7 +395,7 @@ func (c *CreateConfig) createExitCommand() ([]string, error) { } // GetContainerCreateOptions takes a CreateConfig and returns a slice of CtrCreateOptions -func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *libpod.Pod) ([]libpod.CtrCreateOption, error) { +func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *libpod.Pod, namedVolumes []*libpod.ContainerNamedVolume) ([]libpod.CtrCreateOption, error) { var options []libpod.CtrCreateOption var portBindings []ocicni.PortMapping var err error @@ -448,8 +446,8 @@ func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *l options = append(options, libpod.WithUserVolumes(volumes)) } - if len(c.NamedVolumes) != 0 { - options = append(options, libpod.WithNamedVolumes(c.NamedVolumes)) + if len(namedVolumes) != 0 { + options = append(options, libpod.WithNamedVolumes(namedVolumes)) } if len(c.Command) != 0 { @@ -485,7 +483,7 @@ func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *l return nil, err } } else if c.NetMode.IsContainer() { - connectedCtr, err := c.Runtime.LookupContainer(c.NetMode.Container()) + connectedCtr, err := runtime.LookupContainer(c.NetMode.Container()) if err != nil { return nil, errors.Wrapf(err, "container %q not found", c.NetMode.Container()) } @@ -496,7 +494,7 @@ func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *l } if c.PidMode.IsContainer() { - connectedCtr, err := c.Runtime.LookupContainer(c.PidMode.Container()) + connectedCtr, err := runtime.LookupContainer(c.PidMode.Container()) if err != nil { return nil, errors.Wrapf(err, "container %q not found", c.PidMode.Container()) } @@ -505,7 +503,7 @@ func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *l } if c.IpcMode.IsContainer() { - connectedCtr, err := c.Runtime.LookupContainer(c.IpcMode.Container()) + connectedCtr, err := runtime.LookupContainer(c.IpcMode.Container()) if err != nil { return nil, errors.Wrapf(err, "container %q not found", c.IpcMode.Container()) } @@ -517,7 +515,7 @@ func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *l options = append(options, libpod.WithUTSNSFromPod(pod)) } if c.UtsMode.IsContainer() { - connectedCtr, err := c.Runtime.LookupContainer(c.UtsMode.Container()) + connectedCtr, err := runtime.LookupContainer(c.UtsMode.Container()) if err != nil { return nil, errors.Wrapf(err, "container %q not found", c.UtsMode.Container()) } @@ -593,7 +591,7 @@ func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *l } // Always use a cleanup process to clean up Podman after termination - exitCmd, err := c.createExitCommand() + exitCmd, err := c.createExitCommand(runtime) if err != nil { return nil, err } diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index 4c839921c..5ffa6dc4c 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -89,11 +89,11 @@ func getAvailableGids() (int64, error) { } // CreateConfigToOCISpec parses information needed to create a container into an OCI runtime spec -func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //nolint +func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spec.Spec, []*libpod.ContainerNamedVolume, error) { //nolint cgroupPerm := "ro" g, err := generate.New("linux") if err != nil { - return nil, err + return nil, nil, err } // Remove the default /dev/shm mount to ensure we overwrite it g.RemoveMount("/dev/shm") @@ -139,7 +139,7 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli if isRootless { nGids, err := getAvailableGids() if err != nil { - return nil, err + return nil, nil, err } if nGids < 5 { // If we have no GID mappings, the gid=5 default option would fail, so drop it. @@ -214,7 +214,7 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli if hostname == "" && (config.NetMode.IsHost() || config.UtsMode.IsHost()) { hostname, err = os.Hostname() if err != nil { - return nil, errors.Wrap(err, "unable to retrieve hostname") + return nil, nil, errors.Wrap(err, "unable to retrieve hostname") } } g.RemoveHostname() @@ -304,13 +304,13 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli // already adding them all. if !rootless.IsRootless() { if err := config.AddPrivilegedDevices(&g); err != nil { - return nil, err + return nil, nil, err } } } else { for _, devicePath := range config.Devices { if err := devicesFromPath(&g, devicePath); err != nil { - return nil, err + return nil, nil, err } } } @@ -340,7 +340,7 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli spliti := strings.SplitN(i, ":", 2) if len(spliti) > 1 { if _, _, err := mount.ParseTmpfsOptions(spliti[1]); err != nil { - return nil, err + return nil, nil, err } options = strings.Split(spliti[1], ",") } @@ -389,27 +389,27 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli } if err := addRlimits(config, &g); err != nil { - return nil, err + return nil, nil, err } if err := addPidNS(config, &g); err != nil { - return nil, err + return nil, nil, err } if err := addUserNS(config, &g); err != nil { - return nil, err + return nil, nil, err } if err := addNetNS(config, &g); err != nil { - return nil, err + return nil, nil, err } if err := addUTSNS(config, &g); err != nil { - return nil, err + return nil, nil, err } if err := addIpcNS(config, &g); err != nil { - return nil, err + return nil, nil, err } configSpec := g.Config @@ -417,7 +417,7 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli // NOTE: Must happen before SECCOMP if !config.Privileged { if err := setupCapabilities(config, configSpec); err != nil { - return nil, err + return nil, nil, err } } else { g.SetupPrivileged(true) @@ -428,7 +428,7 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli if config.SeccompProfilePath != "unconfined" { seccompConfig, err := getSeccompConfig(config, configSpec) if err != nil { - return nil, err + return nil, nil, err } configSpec.Linux.Seccomp = seccompConfig } @@ -439,13 +439,13 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli } // BIND MOUNTS - if err := config.GetVolumesFrom(); err != nil { - return nil, errors.Wrap(err, "error getting volume mounts from --volumes-from flag") + if err := config.GetVolumesFrom(runtime); err != nil { + return nil, nil, errors.Wrap(err, "error getting volume mounts from --volumes-from flag") } volumeMounts, err := config.GetVolumeMounts(configSpec.Mounts) if err != nil { - return nil, errors.Wrapf(err, "error getting volume mounts") + return nil, nil, errors.Wrapf(err, "error getting volume mounts") } configSpec.Mounts = supercedeUserMounts(volumeMounts, configSpec.Mounts) @@ -455,12 +455,11 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli // Split normal mounts and named volumes newMounts, namedVolumes := splitNamedVolumes(configSpec.Mounts) configSpec.Mounts = newMounts - config.NamedVolumes = namedVolumes // BLOCK IO blkio, err := config.CreateBlockIO() if err != nil { - return nil, errors.Wrapf(err, "error creating block io") + return nil, nil, errors.Wrapf(err, "error creating block io") } if blkio != nil { configSpec.Linux.Resources.BlockIO = blkio @@ -469,7 +468,7 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli if rootless.IsRootless() { if addedResources { - return nil, errors.New("invalid configuration, cannot set resources with rootless containers") + return nil, nil, errors.New("invalid configuration, cannot set resources with rootless containers") } configSpec.Linux.Resources = &spec.LinuxResources{} } @@ -477,7 +476,7 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli // Make sure that the bind mounts keep options like nosuid, noexec, nodev. mounts, err := pmount.GetMounts() if err != nil { - return nil, err + return nil, nil, err } for i := range configSpec.Mounts { m := &configSpec.Mounts[i] @@ -493,7 +492,7 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli } mount, err := findMount(m.Source, mounts) if err != nil { - return nil, err + return nil, nil, err } if mount == nil { continue @@ -511,7 +510,7 @@ func (config *CreateConfig) createConfigToOCISpec() (*spec.Spec, error) { //noli } } - return configSpec, nil + return configSpec, namedVolumes, nil } func findMount(target string, mounts []*pmount.Info) (*pmount.Info, error) { -- cgit v1.2.3-54-g00ecf From 71f65ab07f0e96d59ba3836c176f9aa5e7330276 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 15 Apr 2019 14:19:55 -0400 Subject: Always pass pod into MakeContainerConfig Play kube was passing the pod, but CreateConfig was not. Unify it so they both do, so we can remove some unnecessary duplicate lookup code. Signed-off-by: Matthew Heon --- cmd/podman/shared/create.go | 12 +++++++++++- pkg/spec/containerconfig.go | 7 +++++++ pkg/spec/createconfig.go | 10 ++-------- 3 files changed, 20 insertions(+), 9 deletions(-) (limited to 'pkg') diff --git a/cmd/podman/shared/create.go b/cmd/podman/shared/create.go index ab695bcf4..ebc42457b 100644 --- a/cmd/podman/shared/create.go +++ b/cmd/podman/shared/create.go @@ -114,6 +114,7 @@ func CreateContainer(ctx context.Context, c *GenericCLIResults, runtime *libpod. } } } + createConfig, err := ParseCreateOpts(ctx, c, runtime, imageName, data) if err != nil { return nil, nil, err @@ -123,7 +124,16 @@ func CreateContainer(ctx context.Context, c *GenericCLIResults, runtime *libpod. // at this point. The rest is done by WithOptions. createConfig.HealthCheck = healthCheck - ctr, err := CreateContainerFromCreateConfig(runtime, createConfig, ctx, nil) + // TODO: Should be able to return this from ParseCreateOpts + var pod *libpod.Pod + if createConfig.Pod != "" { + pod, err = runtime.LookupPod(createConfig.Pod) + if err != nil { + return nil, nil, errors.Wrapf(err, "error looking up pod to join") + } + } + + ctr, err := CreateContainerFromCreateConfig(runtime, createConfig, ctx, pod) if err != nil { return nil, nil, err } diff --git a/pkg/spec/containerconfig.go b/pkg/spec/containerconfig.go index 775a2042d..87c1b5155 100644 --- a/pkg/spec/containerconfig.go +++ b/pkg/spec/containerconfig.go @@ -3,11 +3,18 @@ package createconfig import ( "github.com/containers/libpod/libpod" spec "github.com/opencontainers/runtime-spec/specs-go" + "github.com/pkg/errors" ) // MakeContainerConfig generates all configuration necessary to start a // container with libpod from a completed CreateConfig struct. func (config *CreateConfig) MakeContainerConfig(runtime *libpod.Runtime, pod *libpod.Pod) (*spec.Spec, []libpod.CtrCreateOption, error) { + if config.Pod != "" && pod == nil { + return nil, nil, errors.Wrapf(libpod.ErrInvalidArg, "pod was specified but no pod passed") + } else if config.Pod == "" && pod != nil { + return nil, nil, errors.Wrapf(libpod.ErrInvalidArg, "pod was given but no pod is specified") + } + runtimeSpec, namedVolumes, err := config.createConfigToOCISpec(runtime) if err != nil { return nil, nil, err diff --git a/pkg/spec/createconfig.go b/pkg/spec/createconfig.go index d2ae99de6..80c7b586c 100644 --- a/pkg/spec/createconfig.go +++ b/pkg/spec/createconfig.go @@ -408,16 +408,10 @@ func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *l options = append(options, libpod.WithSystemd()) } if c.Name != "" { - logrus.Debugf("appending name %s", c.Name) + logrus.Debugf("setting container name %s", c.Name) options = append(options, libpod.WithName(c.Name)) } - if c.Pod != "" || pod != nil { - if pod == nil { - pod, err = runtime.LookupPod(c.Pod) - if err != nil { - return nil, errors.Wrapf(err, "unable to add container to pod %s", c.Pod) - } - } + if c.Pod != "" { logrus.Debugf("adding container to pod %s", c.Pod) options = append(options, runtime.WithPod(pod)) } -- cgit v1.2.3-54-g00ecf From 9ee50fe2c7d31e5a6209b63f7735a965dc204131 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 15 Apr 2019 15:26:29 -0400 Subject: Migrate to unified volume handling code 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 --- .tool/lint | 2 - cmd/podman/play_kube.go | 5 +- cmd/podman/shared/create.go | 30 +- cmd/podman/shared/create_cli.go | 184 ---------- pkg/spec/containerconfig.go | 17 +- pkg/spec/createconfig.go | 268 ++------------ pkg/spec/spec.go | 116 ++---- pkg/spec/spec_test.go | 18 +- pkg/spec/storage.go | 765 ++++++++++++++++++++++++++++++++++++++++ 9 files changed, 841 insertions(+), 564 deletions(-) create mode 100644 pkg/spec/storage.go (limited to 'pkg') diff --git a/.tool/lint b/.tool/lint index f7bf81c1d..01f44311d 100755 --- a/.tool/lint +++ b/.tool/lint @@ -25,7 +25,6 @@ ${LINTER} \ --deadline=600s --disable-all\ --enable=deadcode\ --enable=errcheck\ - --enable=goconst\ --enable=gofmt\ --enable=golint\ --enable=ineffassign\ @@ -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.*$'\ diff --git a/cmd/podman/play_kube.go b/cmd/podman/play_kube.go index 2e51457c4..e778bafb9 100644 --- a/cmd/podman/play_kube.go +++ b/cmd/podman/play_kube.go @@ -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 @@ -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)) diff --git a/cmd/podman/shared/create.go b/cmd/podman/shared/create.go index ebc42457b..1e185d21f 100644 --- a/cmd/podman/shared/create.go +++ b/cmd/podman/shared/create.go @@ -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" @@ -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") @@ -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"), @@ -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() diff --git a/cmd/podman/shared/create_cli.go b/cmd/podman/shared/create_cli.go index 4f9cb1699..f731e8db5 100644 --- a/cmd/podman/shared/create_cli.go +++ b/cmd/podman/shared/create_cli.go @@ -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" ) @@ -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=,[src=,]target=[,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) diff --git a/pkg/spec/containerconfig.go b/pkg/spec/containerconfig.go index 87c1b5155..f9bb075b8 100644 --- a/pkg/spec/containerconfig.go +++ b/pkg/spec/containerconfig.go @@ -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 @@ -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 } diff --git a/pkg/spec/createconfig.go b/pkg/spec/createconfig.go index 80c7b586c..4d2d8304c 100644 --- a/pkg/spec/createconfig.go +++ b/pkg/spec/createconfig.go @@ -1,7 +1,6 @@ package createconfig import ( - "fmt" "net" "os" "strconv" @@ -12,7 +11,6 @@ import ( "github.com/containers/libpod/libpod" "github.com/containers/libpod/pkg/namespaces" "github.com/containers/storage" - "github.com/containers/storage/pkg/stringid" "github.com/cri-o/ocicni/pkg/ocicni" "github.com/docker/go-connections/nat" spec "github.com/opencontainers/runtime-spec/specs-go" @@ -26,12 +24,6 @@ import ( const ( bps = iota iops - // TypeBind is the type for mounting host dir - TypeBind = "bind" - // TypeVolume is the type for remote storage volumes - // TypeVolume = "volume" // re-enable upon use - // TypeTmpfs is the type for mounting tmpfs - TypeTmpfs = "tmpfs" ) // CreateResourceConfig represents resource elements in CreateConfig @@ -87,6 +79,8 @@ type CreateConfig struct { HostAdd []string //add-host Hostname string //hostname HTTPProxy bool + Init bool // init + InitPath string //init-path Image string ImageID string BuiltinImgVolumes map[string]struct{} // volumes defined in the image config @@ -125,14 +119,16 @@ type CreateConfig struct { UsernsMode namespaces.UsernsMode //userns User string //user UtsMode namespaces.UTSMode //uts - Mounts []spec.Mount //mounts - Volumes []string //volume + Mounts []spec.Mount + MountsFlag []string // mounts + NamedVolumes []*libpod.ContainerNamedVolume + Volumes []string //volume VolumesFrom []string - WorkDir string //workdir - LabelOpts []string //SecurityOpts - NoNewPrivs bool //SecurityOpts - ApparmorProfile string //SecurityOpts - SeccompProfilePath string //SecurityOpts + WorkDir string //workdir + LabelOpts []string //SecurityOpts + NoNewPrivs bool //SecurityOpts + ApparmorProfile string //SecurityOpts + SeccompProfilePath string //SecurityOpts SecurityOpts []string Rootfs string Syslog bool // Whether to enable syslog on exit commands @@ -146,222 +142,6 @@ func (c *CreateConfig) CreateBlockIO() (*spec.LinuxBlockIO, error) { return c.createBlockIO() } -// AddContainerInitBinary adds the init binary specified by path iff the -// container will run in a private PID namespace that is not shared with the -// host or another pre-existing container, where an init-like process is -// already running. -// -// Note that AddContainerInitBinary prepends "/dev/init" "--" to the command -// to execute the bind-mounted binary as PID 1. -func (c *CreateConfig) AddContainerInitBinary(path string) error { - if path == "" { - return fmt.Errorf("please specify a path to the container-init binary") - } - if !c.PidMode.IsPrivate() { - return fmt.Errorf("cannot add init binary as PID 1 (PID namespace isn't private)") - } - if c.Systemd { - return fmt.Errorf("cannot use container-init binary with systemd") - } - if _, err := os.Stat(path); os.IsNotExist(err) { - return errors.Wrap(err, "container-init binary not found on the host") - } - c.Command = append([]string{"/dev/init", "--"}, c.Command...) - c.Mounts = append(c.Mounts, spec.Mount{ - Destination: "/dev/init", - Type: TypeBind, - Source: path, - Options: []string{TypeBind, "ro"}, - }) - return nil -} - -func processOptions(options []string) []string { - var ( - foundrw, foundro bool - rootProp string - ) - options = append(options, "rbind") - for _, opt := range options { - switch opt { - case "rw": - foundrw = true - case "ro": - foundro = true - case "private", "rprivate", "slave", "rslave", "shared", "rshared": - rootProp = opt - } - } - if !foundrw && !foundro { - options = append(options, "rw") - } - if rootProp == "" { - options = append(options, "rprivate") - } - return options -} - -func (c *CreateConfig) initFSMounts() []spec.Mount { - var mounts []spec.Mount - for _, m := range c.Mounts { - m.Options = processOptions(m.Options) - if m.Type == "tmpfs" { - m.Options = append(m.Options, "tmpcopyup") - } else { - mounts = append(mounts, m) - } - } - return mounts -} - -// GetVolumeMounts takes user provided input for bind mounts and creates Mount structs -func (c *CreateConfig) GetVolumeMounts(specMounts []spec.Mount) ([]spec.Mount, error) { - m := []spec.Mount{} - for _, i := range c.Volumes { - var options []string - spliti := strings.Split(i, ":") - if len(spliti) > 2 { - options = strings.Split(spliti[2], ",") - } - - m = append(m, spec.Mount{ - Destination: spliti[1], - Type: string(TypeBind), - Source: spliti[0], - Options: processOptions(options), - }) - - logrus.Debugf("User mount %s:%s options %v", spliti[0], spliti[1], options) - } - - if c.ImageVolumeType == "ignore" { - return m, nil - } - - for vol := range c.BuiltinImgVolumes { - if libpod.MountExists(specMounts, vol) || libpod.MountExists(m, vol) { - continue - } - - mount := spec.Mount{ - Destination: vol, - Type: c.ImageVolumeType, - Options: []string{"rprivate", "rw", "nodev"}, - } - if c.ImageVolumeType == "tmpfs" { - mount.Source = "tmpfs" - mount.Options = append(mount.Options, "tmpcopyup") - } else { - // TODO: Move support for this and tmpfs into libpod - // Should tmpfs also be handled as named volumes? Wouldn't be hard - // This will cause a new local Volume to be created on your system - mount.Source = stringid.GenerateNonCryptoID() - mount.Options = append(mount.Options, TypeBind) - } - m = append(m, mount) - } - - return m, nil -} - -// GetVolumesFrom reads the create-config artifact of the container to get volumes from -// and adds it to c.Volumes of the current container. -func (c *CreateConfig) GetVolumesFrom(runtime *libpod.Runtime) error { - if os.Geteuid() != 0 { - return nil - } - - for _, vol := range c.VolumesFrom { - options := "" - splitVol := strings.SplitN(vol, ":", 2) - if len(splitVol) == 2 { - options = splitVol[1] - } - ctr, err := runtime.LookupContainer(splitVol[0]) - if err != nil { - return errors.Wrapf(err, "error looking up container %q", splitVol[0]) - } - - logrus.Debugf("Adding volumes from container %s", ctr.ID()) - - // Look up the container's user volumes. This gets us the - // destinations of all mounts the user added to the container. - userVolumesArr := ctr.UserVolumes() - - // We're going to need to access them a lot, so convert to a map - // to reduce looping. - // We'll also use the map to indicate if we missed any volumes along the way. - userVolumes := make(map[string]bool) - for _, dest := range userVolumesArr { - userVolumes[dest] = false - } - - // Now we get the container's spec and loop through its volumes - // and append them in if we can find them. - spec := ctr.Spec() - if spec == nil { - return errors.Errorf("error retrieving container %s spec", ctr.ID()) - } - for _, mnt := range spec.Mounts { - if mnt.Type != TypeBind { - continue - } - if _, exists := userVolumes[mnt.Destination]; exists { - userVolumes[mnt.Destination] = true - localOptions := options - if localOptions == "" { - localOptions = strings.Join(mnt.Options, ",") - } - c.Volumes = append(c.Volumes, fmt.Sprintf("%s:%s:%s", mnt.Source, mnt.Destination, localOptions)) - } - } - - // We're done with the spec mounts. Add named volumes. - // Add these unconditionally - none of them are automatically - // part of the container, as some spec mounts are. - namedVolumes := ctr.NamedVolumes() - for _, namedVol := range namedVolumes { - if _, exists := userVolumes[namedVol.Dest]; exists { - userVolumes[namedVol.Dest] = true - } - localOptions := options - if localOptions == "" { - localOptions = strings.Join(namedVol.Options, ",") - } - c.Volumes = append(c.Volumes, fmt.Sprintf("%s:%s:%s", namedVol.Name, namedVol.Dest, localOptions)) - } - - // Check if we missed any volumes - for volDest, found := range userVolumes { - if !found { - logrus.Warnf("Unable to match volume %s from container %s for volumes-from", volDest, ctr.ID()) - } - } - } - return nil -} - -//GetTmpfsMounts takes user provided input for Tmpfs mounts and creates Mount structs -func (c *CreateConfig) GetTmpfsMounts() []spec.Mount { - var m []spec.Mount - for _, i := range c.Tmpfs { - // Default options if nothing passed - options := []string{"rprivate", "rw", "noexec", "nosuid", "nodev", "size=65536k"} - spliti := strings.Split(i, ":") - destPath := spliti[0] - if len(spliti) > 1 { - options = strings.Split(spliti[1], ",") - } - m = append(m, spec.Mount{ - Destination: destPath, - Type: string(TypeTmpfs), - Options: options, - Source: string(TypeTmpfs), - }) - } - return m -} - func (c *CreateConfig) createExitCommand(runtime *libpod.Runtime) ([]string, error) { config, err := runtime.GetConfig() if err != nil { @@ -395,7 +175,7 @@ func (c *CreateConfig) createExitCommand(runtime *libpod.Runtime) ([]string, err } // GetContainerCreateOptions takes a CreateConfig and returns a slice of CtrCreateOptions -func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *libpod.Pod, namedVolumes []*libpod.ContainerNamedVolume) ([]libpod.CtrCreateOption, error) { +func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *libpod.Pod, mounts []spec.Mount, namedVolumes []*libpod.ContainerNamedVolume) ([]libpod.CtrCreateOption, error) { var options []libpod.CtrCreateOption var portBindings []ocicni.PortMapping var err error @@ -422,22 +202,18 @@ func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *l } } - if len(c.Volumes) != 0 { - // Volumes consist of multiple, comma-delineated fields - // The image spec only includes one part of that, so drop the - // others, if they are included - volumes := make([]string, 0, len(c.Volumes)) - for _, vol := range c.Volumes { - // We always want the volume destination - splitVol := strings.SplitN(vol, ":", 3) - if len(splitVol) > 1 { - volumes = append(volumes, splitVol[1]) - } else { - volumes = append(volumes, splitVol[0]) - } + if len(mounts) != 0 || len(namedVolumes) != 0 { + destinations := []string{} + + // Take all mount and named volume destinations. + for _, mount := range mounts { + destinations = append(destinations, mount.Destination) + } + for _, volume := range namedVolumes { + destinations = append(destinations, volume.Dest) } - options = append(options, libpod.WithUserVolumes(volumes)) + options = append(options, libpod.WithUserVolumes(destinations)) } if len(namedVolumes) != 0 { diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index 5ffa6dc4c..591a28703 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -2,7 +2,6 @@ package createconfig import ( "os" - "path" "path/filepath" "strings" @@ -21,61 +20,6 @@ import ( const cpuPeriod = 100000 -func supercedeUserMounts(mounts []spec.Mount, configMount []spec.Mount) []spec.Mount { - if len(mounts) > 0 { - // If we have overlappings mounts, remove them from the spec in favor of - // the user-added volume mounts - destinations := make(map[string]bool) - for _, mount := range mounts { - destinations[path.Clean(mount.Destination)] = true - } - // Copy all mounts from spec to defaultMounts, except for - // - mounts overridden by a user supplied mount; - // - all mounts under /dev if a user supplied /dev is present; - mountDev := destinations["/dev"] - for _, mount := range configMount { - if _, ok := destinations[path.Clean(mount.Destination)]; !ok { - if mountDev && strings.HasPrefix(mount.Destination, "/dev/") { - // filter out everything under /dev if /dev is user-mounted - continue - } - - logrus.Debugf("Adding mount %s", mount.Destination) - mounts = append(mounts, mount) - } - } - return mounts - } - return configMount -} - -// Split named volumes from normal volumes -func splitNamedVolumes(mounts []spec.Mount) ([]spec.Mount, []*libpod.ContainerNamedVolume) { - newMounts := make([]spec.Mount, 0) - namedVolumes := make([]*libpod.ContainerNamedVolume, 0) - for _, mount := range mounts { - // If it's not a named volume, append unconditionally - if mount.Type != TypeBind { - newMounts = append(newMounts, mount) - continue - } - // Volumes that are not named volumes must be an absolute or - // relative path. - // Volume names may not begin with a non-alphanumeric character - // so the HasPrefix() check is safe here. - if strings.HasPrefix(mount.Source, "/") || strings.HasPrefix(mount.Source, ".") { - newMounts = append(newMounts, mount) - } else { - namedVolume := new(libpod.ContainerNamedVolume) - namedVolume.Name = mount.Source - namedVolume.Dest = mount.Destination - namedVolume.Options = mount.Options - namedVolumes = append(namedVolumes, namedVolume) - } - } - return newMounts, namedVolumes -} - func getAvailableGids() (int64, error) { idMap, err := user.ParseIDMapFile("/proc/self/gid_map") if err != nil { @@ -89,11 +33,11 @@ func getAvailableGids() (int64, error) { } // CreateConfigToOCISpec parses information needed to create a container into an OCI runtime spec -func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spec.Spec, []*libpod.ContainerNamedVolume, error) { //nolint +func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime, userMounts []spec.Mount) (*spec.Spec, error) { cgroupPerm := "ro" g, err := generate.New("linux") if err != nil { - return nil, nil, err + return nil, err } // Remove the default /dev/shm mount to ensure we overwrite it g.RemoveMount("/dev/shm") @@ -139,7 +83,7 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spe if isRootless { nGids, err := getAvailableGids() if err != nil { - return nil, nil, err + return nil, err } if nGids < 5 { // If we have no GID mappings, the gid=5 default option would fail, so drop it. @@ -214,7 +158,7 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spe if hostname == "" && (config.NetMode.IsHost() || config.UtsMode.IsHost()) { hostname, err = os.Hostname() if err != nil { - return nil, nil, errors.Wrap(err, "unable to retrieve hostname") + return nil, errors.Wrap(err, "unable to retrieve hostname") } } g.RemoveHostname() @@ -304,13 +248,13 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spe // already adding them all. if !rootless.IsRootless() { if err := config.AddPrivilegedDevices(&g); err != nil { - return nil, nil, err + return nil, err } } } else { for _, devicePath := range config.Devices { if err := devicesFromPath(&g, devicePath); err != nil { - return nil, nil, err + return nil, err } } } @@ -340,7 +284,7 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spe spliti := strings.SplitN(i, ":", 2) if len(spliti) > 1 { if _, _, err := mount.ParseTmpfsOptions(spliti[1]); err != nil { - return nil, nil, err + return nil, err } options = strings.Split(spliti[1], ",") } @@ -384,32 +328,33 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spe g.AddMount(tmpfsMnt) } } + for name, val := range config.Env { g.AddProcessEnv(name, val) } if err := addRlimits(config, &g); err != nil { - return nil, nil, err + return nil, err } if err := addPidNS(config, &g); err != nil { - return nil, nil, err + return nil, err } if err := addUserNS(config, &g); err != nil { - return nil, nil, err + return nil, err } if err := addNetNS(config, &g); err != nil { - return nil, nil, err + return nil, err } if err := addUTSNS(config, &g); err != nil { - return nil, nil, err + return nil, err } if err := addIpcNS(config, &g); err != nil { - return nil, nil, err + return nil, err } configSpec := g.Config @@ -417,7 +362,7 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spe // NOTE: Must happen before SECCOMP if !config.Privileged { if err := setupCapabilities(config, configSpec); err != nil { - return nil, nil, err + return nil, err } } else { g.SetupPrivileged(true) @@ -428,7 +373,7 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spe if config.SeccompProfilePath != "unconfined" { seccompConfig, err := getSeccompConfig(config, configSpec) if err != nil { - return nil, nil, err + return nil, err } configSpec.Linux.Seccomp = seccompConfig } @@ -439,27 +384,14 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spe } // BIND MOUNTS - if err := config.GetVolumesFrom(runtime); err != nil { - return nil, nil, errors.Wrap(err, "error getting volume mounts from --volumes-from flag") - } - - volumeMounts, err := config.GetVolumeMounts(configSpec.Mounts) - if err != nil { - return nil, nil, errors.Wrapf(err, "error getting volume mounts") - } - - configSpec.Mounts = supercedeUserMounts(volumeMounts, configSpec.Mounts) - //--mount - configSpec.Mounts = supercedeUserMounts(config.initFSMounts(), configSpec.Mounts) - - // Split normal mounts and named volumes - newMounts, namedVolumes := splitNamedVolumes(configSpec.Mounts) - configSpec.Mounts = newMounts + configSpec.Mounts = supercedeUserMounts(userMounts, configSpec.Mounts) + // Process mounts to ensure correct options + configSpec.Mounts = initFSMounts(configSpec.Mounts) // BLOCK IO blkio, err := config.CreateBlockIO() if err != nil { - return nil, nil, errors.Wrapf(err, "error creating block io") + return nil, errors.Wrapf(err, "error creating block io") } if blkio != nil { configSpec.Linux.Resources.BlockIO = blkio @@ -468,7 +400,7 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spe if rootless.IsRootless() { if addedResources { - return nil, nil, errors.New("invalid configuration, cannot set resources with rootless containers") + return nil, errors.New("invalid configuration, cannot set resources with rootless containers") } configSpec.Linux.Resources = &spec.LinuxResources{} } @@ -476,7 +408,7 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spe // Make sure that the bind mounts keep options like nosuid, noexec, nodev. mounts, err := pmount.GetMounts() if err != nil { - return nil, nil, err + return nil, err } for i := range configSpec.Mounts { m := &configSpec.Mounts[i] @@ -492,7 +424,7 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spe } mount, err := findMount(m.Source, mounts) if err != nil { - return nil, nil, err + return nil, err } if mount == nil { continue @@ -510,7 +442,7 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime) (*spe } } - return configSpec, namedVolumes, nil + return configSpec, nil } func findMount(target string, mounts []*pmount.Info) (*pmount.Info, error) { diff --git a/pkg/spec/spec_test.go b/pkg/spec/spec_test.go index c037bf69e..b7c9c9c0d 100644 --- a/pkg/spec/spec_test.go +++ b/pkg/spec/spec_test.go @@ -8,22 +8,22 @@ import ( "github.com/stretchr/testify/assert" ) -func TestCreateConfig_GetVolumeMounts(t *testing.T) { +func TestGetVolumeMountsOneVolume(t *testing.T) { data := spec.Mount{ Destination: "/foobar", Type: "bind", - Source: "foobar", + Source: "/foobar", Options: []string{"ro", "rbind", "rprivate"}, } config := CreateConfig{ - Volumes: []string{"foobar:/foobar:ro"}, + Volumes: []string{"/foobar:/foobar:ro"}, } - specMount, err := config.GetVolumeMounts([]spec.Mount{}) + specMount, _, err := config.getVolumeMounts() assert.NoError(t, err) - assert.True(t, reflect.DeepEqual(data, specMount[0])) + assert.True(t, reflect.DeepEqual(data, specMount[data.Destination])) } -func TestCreateConfig_GetTmpfsMounts(t *testing.T) { +func TestGetTmpfsMounts(t *testing.T) { data := spec.Mount{ Destination: "/homer", Type: "tmpfs", @@ -33,7 +33,7 @@ func TestCreateConfig_GetTmpfsMounts(t *testing.T) { config := CreateConfig{ Tmpfs: []string{"/homer:rw,size=787448k,mode=1777"}, } - tmpfsMount := config.GetTmpfsMounts() - assert.True(t, reflect.DeepEqual(data, tmpfsMount[0])) - + tmpfsMount, err := config.getTmpfsMounts() + assert.NoError(t, err) + assert.True(t, reflect.DeepEqual(data, tmpfsMount[data.Destination])) } diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go new file mode 100644 index 000000000..7806a8c72 --- /dev/null +++ b/pkg/spec/storage.go @@ -0,0 +1,765 @@ +package createconfig + +import ( + "fmt" + "os" + "path" + "path/filepath" + "strings" + + "github.com/containers/libpod/libpod" + "github.com/containers/storage/pkg/stringid" + spec "github.com/opencontainers/runtime-spec/specs-go" + "github.com/pkg/errors" + "github.com/sirupsen/logrus" +) + +const ( + // TypeBind is the type for mounting host dir + TypeBind = "bind" + // TypeVolume is the type for named volumes + TypeVolume = "volume" + // TypeTmpfs is the type for mounting tmpfs + TypeTmpfs = "tmpfs" +) + +var ( + errDuplicateDest = errors.Errorf("duplicate mount destination") + badOptionError = errors.Errorf("invalid mount option") + optionArgError = errors.Errorf("must provide an argument for option") + noDestError = errors.Errorf("must set volume destination") +) + +// Parse all volume-related options in the create config into a set of mounts +// and named volumes to add to the container. +// Handles --volumes-from, --volumes, --tmpfs, --init, and --init-path flags. +// TODO: Named volume options - should we default to rprivate? It bakes into a +// bind mount under the hood... +// TODO: Tmpfs options - we should probably check user-given ones, provide sane +// defaults even if the user provides a few... +func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, []*libpod.ContainerNamedVolume, error) { + // Add image volumes. + baseMounts, baseVolumes, err := config.getImageVolumes() + if err != nil { + return nil, nil, err + } + + // Add --volumes-from. + // Overrides image volumes unconditionally. + vFromMounts, vFromVolumes, err := config.getVolumesFrom(runtime) + if err != nil { + return nil, nil, err + } + for dest, mount := range vFromMounts { + baseMounts[dest] = mount + } + for dest, volume := range vFromVolumes { + baseVolumes[dest] = volume + } + + // Next mounts from the --mounts flag. + // Do not override yet. + unifiedMounts, unifiedVolumes, err := config.getMounts() + if err != nil { + return nil, nil, err + } + + // Next --volumes flag. + // Do not override yet. + volumeMounts, volumeVolumes, err := config.getVolumeMounts() + if err != nil { + return nil, nil, err + } + + // Next --tmpfs flag. + // Do not override yet. + tmpfsMounts, err := config.getTmpfsMounts() + if err != nil { + return nil, nil, err + } + + // Unify mounts from --mount, --volume, --tmpfs. + // Start with --volume. + for dest, mount := range volumeMounts { + if _, ok := unifiedMounts[dest]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, dest) + } + unifiedMounts[dest] = mount + } + for dest, volume := range volumeVolumes { + if _, ok := unifiedVolumes[dest]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, dest) + } + unifiedVolumes[dest] = volume + } + // Now --tmpfs + for dest, tmpfs := range tmpfsMounts { + if _, ok := unifiedMounts[dest]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, dest) + } + unifiedMounts[dest] = tmpfs + } + + // If requested, add container init binary + if config.Init { + initPath := config.InitPath + if initPath == "" { + rtc, err := runtime.GetConfig() + if err != nil { + return nil, nil, err + } + initPath = rtc.InitPath + } + initMount, err := config.addContainerInitBinary(initPath) + if err != nil { + return nil, nil, err + } + if _, ok := unifiedMounts[initMount.Destination]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, "conflict with mount added by --init to %q", initMount.Destination) + } + unifiedMounts[initMount.Destination] = initMount + } + + // Supercede volumes-from/image volumes with unified volumes from above. + // This is an unconditional replacement. + for dest, mount := range unifiedMounts { + baseMounts[dest] = mount + } + for dest, volume := range unifiedVolumes { + baseVolumes[dest] = volume + } + + // TODO: Allow an explicit list of named volumes and spec mounts in the + // createconfig. Podman doesn't need them, but other libpod consumers + // might want them. + + // Final step: maps to arrays + finalMounts := make([]spec.Mount, 0, len(baseMounts)) + for _, mount := range baseMounts { + finalMounts = append(finalMounts, mount) + } + finalVolumes := make([]*libpod.ContainerNamedVolume, 0, len(baseVolumes)) + for _, volume := range baseVolumes { + finalVolumes = append(finalVolumes, volume) + } + + return finalMounts, finalVolumes, nil +} + +// Parse volumes from - a set of containers whose volumes we will mount in. +// Grab the containers, retrieve any user-created spec mounts and all named +// volumes, and return a list of them. +// Conflicts are resolved simply - the last container specified wins. +// Container names may be suffixed by mount options after a colon. +func (config *CreateConfig) getVolumesFrom(runtime *libpod.Runtime) (map[string]spec.Mount, map[string]*libpod.ContainerNamedVolume, error) { + // TODO: This can probably be disabled now + if os.Geteuid() != 0 { + return nil, nil, nil + } + + // Both of these are maps of mount destination to mount type. + // We ensure that each destination is only mounted to once in this way. + // We tiebreak only within each category; we'll let libpod tiebreak + // between mounts and named volumes. + // TODO: Should we unmount subtree mounts? E.g., if /tmp/ is mounted by + // one mount, and we already have /tmp/a and /tmp/b, should we remove + // the /tmp/a and /tmp/b mounts in favor of the more general /tmp? + finalMounts := make(map[string]spec.Mount) + finalNamedVolumes := make(map[string]*libpod.ContainerNamedVolume) + + for _, vol := range config.VolumesFrom { + options := []string{} + // TODO: now that container names may contain colons this is no + // longer safe. + // I don't know if there's much we can do here... + splitVol := strings.SplitN(vol, ":", 2) + if len(splitVol) == 2 { + if strings.Contains(splitVol[1], "Z") || + strings.Contains(splitVol[1], "private") || + strings.Contains(splitVol[1], "slave") || + strings.Contains(splitVol[1], "shared") { + return nil, nil, errors.Errorf("invalid options %q, can only specify 'ro', 'rw', and 'z", splitVol[1]) + } + if err := ValidateVolumeOpts(splitVol[1]); err != nil { + return nil, nil, err + } + options = strings.Split(splitVol[1], ",") + } + ctr, err := runtime.LookupContainer(splitVol[0]) + if err != nil { + return nil, nil, errors.Wrapf(err, "error looking up container %q for volumes-from", splitVol[0]) + } + + logrus.Debugf("Adding volumes from container %s", ctr.ID()) + + // Look up the container's user volumes. This gets us the + // destinations of all mounts the user added to the container. + userVolumesArr := ctr.UserVolumes() + + // We're going to need to access them a lot, so convert to a map + // to reduce looping. + // We'll also use the map to indicate if we missed any volumes along the way. + userVolumes := make(map[string]bool) + for _, dest := range userVolumesArr { + userVolumes[dest] = false + } + + // Now we get the container's spec and loop through its volumes + // and append them in if we can find them. + spec := ctr.Spec() + if spec == nil { + return nil, nil, errors.Errorf("error retrieving container %s spec for volumes-from", ctr.ID()) + } + for _, mnt := range spec.Mounts { + if mnt.Type != TypeBind { + continue + } + if _, exists := userVolumes[mnt.Destination]; exists { + userVolumes[mnt.Destination] = true + + if len(options) != 0 { + mnt.Options = options + } + + if _, ok := finalMounts[mnt.Destination]; ok { + logrus.Debugf("Overriding mount to %s with new mount from container %s", mnt.Destination, ctr.ID()) + } + finalMounts[mnt.Destination] = mnt + } + } + + // We're done with the spec mounts. Add named volumes. + // Add these unconditionally - none of them are automatically + // part of the container, as some spec mounts are. + namedVolumes := ctr.NamedVolumes() + for _, namedVol := range namedVolumes { + if _, exists := userVolumes[namedVol.Dest]; exists { + userVolumes[namedVol.Dest] = true + } + + if len(options) != 0 { + namedVol.Options = options + } + + if _, ok := finalMounts[namedVol.Dest]; ok { + logrus.Debugf("Overriding named volume mount to %s with new named volume from container %s", namedVol.Dest, ctr.ID()) + } + finalNamedVolumes[namedVol.Dest] = namedVol + } + + // Check if we missed any volumes + for volDest, found := range userVolumes { + if !found { + logrus.Warnf("Unable to match volume %s from container %s for volumes-from", volDest, ctr.ID()) + } + } + } + + return finalMounts, finalNamedVolumes, nil +} + +// getMounts takes user-provided input from the --mount flag and creates OCI +// spec mounts and Libpod named volumes. +// podman run --mount type=bind,src=/etc/resolv.conf,target=/etc/resolv.conf ... +// podman run --mount type=tmpfs,target=/dev/shm ... +// podman run --mount type=volume,source=test-volume, ... +func (config *CreateConfig) getMounts() (map[string]spec.Mount, map[string]*libpod.ContainerNamedVolume, error) { + finalMounts := make(map[string]spec.Mount) + finalNamedVolumes := make(map[string]*libpod.ContainerNamedVolume) + + errInvalidSyntax := errors.Errorf("incorrect mount format: should be --mount type=,[src=,]target=[,options]") + + // 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. + for _, mount := range config.MountsFlag { + arr := strings.SplitN(mount, ",", 2) + if len(arr) < 2 { + return nil, nil, errors.Wrapf(errInvalidSyntax, "%q", mount) + } + kv := strings.Split(arr[0], "=") + // TODO: type is not explicitly required in Docker. + // If not specified, it defaults to "volume". + if len(kv) != 2 || kv[0] != "type" { + return nil, nil, errors.Wrapf(errInvalidSyntax, "%q", mount) + } + + tokens := strings.Split(arr[1], ",") + switch kv[1] { + case TypeBind: + mount, err := getBindMount(tokens) + if err != nil { + return nil, nil, err + } + if _, ok := finalMounts[mount.Destination]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, mount.Destination) + } + finalMounts[mount.Destination] = mount + case TypeTmpfs: + mount, err := getTmpfsMount(tokens) + if err != nil { + return nil, nil, err + } + if _, ok := finalMounts[mount.Destination]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, mount.Destination) + } + finalMounts[mount.Destination] = mount + case "volume": + volume, err := getNamedVolume(tokens) + if err != nil { + return nil, nil, err + } + if _, ok := finalNamedVolumes[volume.Dest]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, volume.Dest) + } + finalNamedVolumes[volume.Dest] = volume + default: + return nil, nil, errors.Errorf("invalid fylesystem type %q", kv[1]) + } + } + + return finalMounts, finalNamedVolumes, nil +} + +// Parse a single bind mount entry from the --mount flag. +func getBindMount(args []string) (spec.Mount, error) { + newMount := spec.Mount{ + Type: TypeBind, + } + + setSource := false + setDest := false + + for _, val := range args { + kv := strings.Split(val, "=") + switch kv[0] { + case "ro", "nosuid", "nodev", "noexec": + // TODO: detect duplication of these options + newMount.Options = append(newMount.Options, kv[0]) + case "shared", "rshared", "private", "rprivate", "slave", "rslave", "Z", "z": + // TODO: detect duplicate mount propagation options + newMount.Options = append(newMount.Options, kv[0]) + case "bind-propagation": + // TODO: this should conflict with the mount propagation + // options above. + if len(kv) == 1 { + return newMount, errors.Wrapf(optionArgError, kv[0]) + } + newMount.Options = append(newMount.Options, kv[1]) + case "src", "source": + if len(kv) == 1 { + return newMount, errors.Wrapf(optionArgError, kv[0]) + } + if err := ValidateVolumeHostDir(kv[1]); err != nil { + return newMount, err + } + newMount.Source = kv[1] + setSource = true + case "target", "dst", "destination": + if len(kv) == 1 { + return newMount, errors.Wrapf(optionArgError, kv[0]) + } + if err := ValidateVolumeCtrDir(kv[1]); err != nil { + return newMount, err + } + newMount.Destination = kv[1] + setDest = true + default: + return newMount, errors.Wrapf(badOptionError, kv[0]) + } + } + + if !setDest { + return newMount, noDestError + } + + if !setSource { + newMount.Source = newMount.Destination + } + + // Process options + newMount.Options = processOptions(newMount.Options) + + return newMount, nil +} + +// Parse a single tmpfs mount entry from the --mount flag +func getTmpfsMount(args []string) (spec.Mount, error) { + newMount := spec.Mount{ + Type: TypeTmpfs, + Source: TypeTmpfs, + } + + setDest := false + + for _, val := range args { + kv := strings.Split(val, "=") + switch kv[0] { + case "ro", "nosuid", "nodev", "noexec": + // TODO: detect duplication of these options + newMount.Options = append(newMount.Options, kv[0]) + case "tmpfs-mode": + // TODO: detect duplicate modes + if len(kv) == 1 { + return newMount, errors.Wrapf(optionArgError, kv[0]) + } + newMount.Options = append(newMount.Options, fmt.Sprintf("mode=%s", kv[1])) + case "tmpfs-size": + // TODO: detect duplicate sizes + if len(kv) == 1 { + return newMount, errors.Wrapf(optionArgError, kv[0]) + } + newMount.Options = append(newMount.Options, fmt.Sprintf("size=%s", kv[1])) + case "src", "source": + return newMount, errors.Errorf("source is not supported with tmpfs mounts") + case "target", "dst", "destination": + if len(kv) == 1 { + return newMount, errors.Wrapf(optionArgError, kv[0]) + } + if err := ValidateVolumeCtrDir(kv[1]); err != nil { + return newMount, err + } + newMount.Destination = kv[1] + setDest = true + default: + return newMount, errors.Wrapf(badOptionError, kv[0]) + } + } + + if !setDest { + return newMount, noDestError + } + + return newMount, nil +} + +// Parse a single volume mount entry from the --mount flag. +// Note that the volume-label option for named volumes is currently NOT supported. +// TODO: add support for --volume-label +func getNamedVolume(args []string) (*libpod.ContainerNamedVolume, error) { + newVolume := new(libpod.ContainerNamedVolume) + + setSource := false + setDest := false + + for _, val := range args { + kv := strings.Split(val, "=") + switch kv[0] { + case "ro", "nosuid", "nodev", "noexec": + // TODO: detect duplication of these options + newVolume.Options = append(newVolume.Options, kv[0]) + case "volume-label": + return nil, errors.Errorf("the --volume-label option is not presently implemented") + case "src", "source": + if len(kv) == 1 { + return nil, errors.Wrapf(optionArgError, kv[0]) + } + newVolume.Name = kv[1] + setSource = true + case "target", "dst", "destination": + if len(kv) == 1 { + return nil, errors.Wrapf(optionArgError, kv[0]) + } + if err := ValidateVolumeCtrDir(kv[1]); err != nil { + return nil, err + } + newVolume.Dest = kv[1] + setDest = true + default: + return nil, errors.Wrapf(badOptionError, kv[0]) + } + } + + if !setSource { + return nil, errors.Errorf("must set source volume") + } + if !setDest { + return nil, noDestError + } + + return newVolume, nil +} + +// ValidateVolumeHostDir validates a volume mount's source directory +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 validates a volume mount's destination directory. +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 %q, must be an absolute path", ctrDir) + } + return nil +} + +// ValidateVolumeOpts validates a volume's options +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 +} + +// GetVolumeMounts takes user provided input for bind mounts and creates Mount structs +func (config *CreateConfig) getVolumeMounts() (map[string]spec.Mount, map[string]*libpod.ContainerNamedVolume, error) { + mounts := make(map[string]spec.Mount) + volumes := make(map[string]*libpod.ContainerNamedVolume) + + volumeFormatErr := errors.Errorf("incorrect volume format, should be host-dir:ctr-dir[:option]") + + for _, vol := range config.Volumes { + var ( + options []string + src string + dest string + ) + + splitVol := strings.Split(vol, ":") + if len(splitVol) > 3 { + return nil, nil, errors.Wrapf(volumeFormatErr, vol) + } + + src = splitVol[0] + if len(splitVol) == 1 { + dest = src + } else if len(splitVol) > 1 { + dest = splitVol[1] + } + if len(splitVol) > 2 { + if err := ValidateVolumeOpts(splitVol[2]); err != nil { + return nil, nil, err + } + options = strings.Split(splitVol[2], ",") + } + + if err := ValidateVolumeHostDir(src); err != nil { + return nil, nil, err + } + if err := ValidateVolumeCtrDir(dest); err != nil { + return nil, nil, err + } + + if strings.HasPrefix(src, "/") || strings.HasPrefix(src, ".") { + // This is not a named volume + newMount := spec.Mount{ + Destination: dest, + Type: string(TypeBind), + Source: src, + Options: processOptions(options), + } + if _, ok := mounts[newMount.Destination]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, newMount.Destination) + } + mounts[newMount.Destination] = newMount + } else { + // This is a named volume + newNamedVol := new(libpod.ContainerNamedVolume) + newNamedVol.Name = src + newNamedVol.Dest = dest + newNamedVol.Options = options + + if _, ok := volumes[newNamedVol.Dest]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, newNamedVol.Dest) + } + volumes[newNamedVol.Dest] = newNamedVol + } + + logrus.Debugf("User mount %s:%s options %v", src, dest, options) + } + + return mounts, volumes, nil +} + +// Get mounts for container's image volumes +func (config *CreateConfig) getImageVolumes() (map[string]spec.Mount, map[string]*libpod.ContainerNamedVolume, error) { + mounts := make(map[string]spec.Mount) + volumes := make(map[string]*libpod.ContainerNamedVolume) + + if config.ImageVolumeType == "ignore" { + return mounts, volumes, nil + } + + for vol := range config.BuiltinImgVolumes { + if config.ImageVolumeType == "tmpfs" { + // Tmpfs image volumes are handled as mounts + mount := spec.Mount{ + Destination: vol, + Source: TypeTmpfs, + Type: TypeTmpfs, + Options: []string{"rprivate", "rw", "nodev"}, + } + mounts[vol] = mount + } else { + namedVolume := new(libpod.ContainerNamedVolume) + namedVolume.Name = stringid.GenerateNonCryptoID() + namedVolume.Options = []string{"rprivate", "rw", "nodev"} + namedVolume.Dest = vol + volumes[vol] = namedVolume + } + } + + return mounts, volumes, nil +} + +// GetTmpfsMounts creates spec.Mount structs for user-requested tmpfs mounts +func (config *CreateConfig) getTmpfsMounts() (map[string]spec.Mount, error) { + m := make(map[string]spec.Mount) + for _, i := range config.Tmpfs { + // Default options if nothing passed + options := []string{"rprivate", "rw", "noexec", "nosuid", "nodev", "size=65536k"} + spliti := strings.Split(i, ":") + destPath := spliti[0] + if len(spliti) > 1 { + options = strings.Split(spliti[1], ",") + } + + if _, ok := m[destPath]; ok { + return nil, errors.Wrapf(errDuplicateDest, destPath) + } + + // TODO: Do we want to set some default options if the user + // passed options? + // TODO: should we validate the options passed in? + mount := spec.Mount{ + Destination: destPath, + Type: string(TypeTmpfs), + Options: options, + Source: string(TypeTmpfs), + } + m[destPath] = mount + } + return m, nil +} + +// AddContainerInitBinary adds the init binary specified by path iff the +// container will run in a private PID namespace that is not shared with the +// host or another pre-existing container, where an init-like process is +// already running. +// +// Note that AddContainerInitBinary prepends "/dev/init" "--" to the command +// to execute the bind-mounted binary as PID 1. +func (config *CreateConfig) addContainerInitBinary(path string) (spec.Mount, error) { + mount := spec.Mount{ + Destination: "/dev/init", + Type: TypeBind, + Source: path, + Options: []string{TypeBind, "ro"}, + } + + if path == "" { + return mount, fmt.Errorf("please specify a path to the container-init binary") + } + if !config.PidMode.IsPrivate() { + return mount, fmt.Errorf("cannot add init binary as PID 1 (PID namespace isn't private)") + } + if config.Systemd { + return mount, fmt.Errorf("cannot use container-init binary with systemd") + } + if _, err := os.Stat(path); os.IsNotExist(err) { + return mount, errors.Wrap(err, "container-init binary not found on the host") + } + config.Command = append([]string{"/dev/init", "--"}, config.Command...) + return mount, nil +} + +// Handle options for volume mounts +func processOptions(options []string) []string { + var ( + foundrw, foundro bool + rootProp string + ) + options = append(options, "rbind") + for _, opt := range options { + switch opt { + case "rw": + foundrw = true + case "ro": + foundro = true + case "private", "rprivate", "slave", "rslave", "shared", "rshared": + rootProp = opt + } + } + if !foundrw && !foundro { + options = append(options, "rw") + } + if rootProp == "" { + options = append(options, "rprivate") + } + return options +} + +func supercedeUserMounts(mounts []spec.Mount, configMount []spec.Mount) []spec.Mount { + if len(mounts) > 0 { + // If we have overlappings mounts, remove them from the spec in favor of + // the user-added volume mounts + destinations := make(map[string]bool) + for _, mount := range mounts { + destinations[path.Clean(mount.Destination)] = true + } + // Copy all mounts from spec to defaultMounts, except for + // - mounts overridden by a user supplied mount; + // - all mounts under /dev if a user supplied /dev is present; + mountDev := destinations["/dev"] + for _, mount := range configMount { + if _, ok := destinations[path.Clean(mount.Destination)]; !ok { + if mountDev && strings.HasPrefix(mount.Destination, "/dev/") { + // filter out everything under /dev if /dev is user-mounted + continue + } + + logrus.Debugf("Adding mount %s", mount.Destination) + mounts = append(mounts, mount) + } + } + return mounts + } + return configMount +} + +// Ensure mount options on all mounts are correct +func initFSMounts(inputMounts []spec.Mount) []spec.Mount { + var mounts []spec.Mount + for _, m := range inputMounts { + m.Options = processOptions(m.Options) + if m.Type == "tmpfs" { + m.Options = append(m.Options, "tmpcopyup") + } else { + mounts = append(mounts, m) + } + } + return mounts +} -- cgit v1.2.3-54-g00ecf From 2e00d417dd87dd6648f6c3b604fd25db0c05d9f1 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 17 Apr 2019 16:08:39 -0400 Subject: Migrate unit tests from cmd/podman into pkg/spec Several changes made in the interface of pkg/spec make interacting with it without a runtime difficult to impossible, so move the existing limited testing from cmd/podman (which mostly tested pkg/spec) into pkg/spec itself where we can call individual functions that don't break things. Signed-off-by: Matthew Heon --- cmd/podman/run_test.go | 162 -------------------------------------------- pkg/spec/containerconfig.go | 2 - pkg/spec/spec_test.go | 103 ++++++++++++++++++++++------ pkg/spec/storage_test.go | 39 +++++++++++ 4 files changed, 120 insertions(+), 186 deletions(-) delete mode 100644 cmd/podman/run_test.go create mode 100644 pkg/spec/storage_test.go (limited to 'pkg') diff --git a/cmd/podman/run_test.go b/cmd/podman/run_test.go deleted file mode 100644 index af9e6923c..000000000 --- a/cmd/podman/run_test.go +++ /dev/null @@ -1,162 +0,0 @@ -package main - -import ( - "runtime" - "testing" - - "github.com/containers/libpod/cmd/podman/cliconfig" - "github.com/containers/libpod/cmd/podman/shared" - "github.com/containers/libpod/pkg/inspect" - cc "github.com/containers/libpod/pkg/spec" - "github.com/containers/libpod/pkg/sysinfo" - "github.com/docker/go-units" - ociv1 "github.com/opencontainers/image-spec/specs-go/v1" - spec "github.com/opencontainers/runtime-spec/specs-go" - "github.com/spf13/cobra" - "github.com/stretchr/testify/assert" -) - -var ( - sysInfo = sysinfo.New(true) - cmd = []string{"podman", "test", "alpine"} - CLI *cliconfig.PodmanCommand -) - -// generates a mocked ImageData structure based on alpine -func generateAlpineImageData() *inspect.ImageData { - config := &ociv1.ImageConfig{ - User: "", - ExposedPorts: nil, - Env: []string{"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"}, - Entrypoint: []string{}, - Cmd: []string{"/bin/sh"}, - Volumes: nil, - WorkingDir: "", - Labels: nil, - StopSignal: "", - } - - data := &inspect.ImageData{ - ID: "e21c333399e0aeedfd70e8827c9fba3f8e9b170ef8a48a29945eb7702bf6aa5f", - RepoTags: []string{"docker.io/library/alpine:latest"}, - RepoDigests: []string{"docker.io/library/alpine@sha256:5cb04fce748f576d7b72a37850641de8bd725365519673c643ef2d14819b42c6"}, - Comment: "Created:2017-12-01 18:48:48.949613376 +0000", - Author: "", - Architecture: "amd64", - Os: "linux", - Version: "17.06.2-ce", - Config: config, - } - return data -} - -// sets a global CLI -func testCmd(c *cobra.Command) error { - CLI = &cliconfig.PodmanCommand{Command: c} - return nil -} - -// creates the mocked cli pointing to our create flags -// global flags like log-level are not implemented -func createCLI(args []string) *cliconfig.PodmanCommand { - var testCommand = &cliconfig.PodmanCommand{ - Command: &cobra.Command{ - Use: "test", - RunE: func(cmd *cobra.Command, args []string) error { - return testCmd(cmd) - }, - }, - } - rootCmd := testCommand - getCreateFlags(rootCmd) - rootCmd.ParseFlags(args) - return rootCmd -} - -func getRuntimeSpec(c *cliconfig.PodmanCommand) (*spec.Spec, error) { - /* - TODO: This test has never worked. Need to install content - runtime, err := getRuntime(c) - if err != nil { - return nil, err - } - createConfig, err := parseCreateOpts(c, runtime, "alpine", generateAlpineImageData()) - */ - ctx := getContext() - genericResults := shared.NewIntermediateLayer(c, false) - createConfig, err := shared.ParseCreateOpts(ctx, &genericResults, nil, "alpine", generateAlpineImageData()) - if err != nil { - return nil, err - } - runtimeSpec, err := cc.CreateConfigToOCISpec(createConfig) - if err != nil { - return nil, err - } - return runtimeSpec, nil -} - -// TestPIDsLimit verifies the inputted pid-limit is correctly defined in the spec -func TestPIDsLimit(t *testing.T) { - // The default configuration of podman enables seccomp, which is not available on non-Linux systems. - // Thus, any tests that use the default seccomp setting would fail. - // Skip the tests on non-Linux platforms rather than explicitly disable seccomp in the test and possibly affect the test result. - if runtime.GOOS != "linux" { - t.Skip("seccomp, which is enabled by default, is only supported on Linux") - } - if !sysInfo.PidsLimit { - t.Skip("running test not supported by the host system") - } - args := []string{"--pids-limit", "22"} - a := createCLI(args) - a.InputArgs = args - //a.Run(append(cmd, args...)) - runtimeSpec, err := getRuntimeSpec(a) - if err != nil { - t.Fatalf(err.Error()) - } - assert.Equal(t, runtimeSpec.Linux.Resources.Pids.Limit, int64(22)) -} - -// TestBLKIOWeightDevice verifies the inputted blkio weigh device is correctly defined in the spec -func TestBLKIOWeightDevice(t *testing.T) { - // The default configuration of podman enables seccomp, which is not available on non-Linux systems. - // Thus, any tests that use the default seccomp setting would fail. - // Skip the tests on non-Linux platforms rather than explicitly disable seccomp in the test and possibly affect the test result. - if runtime.GOOS != "linux" { - t.Skip("seccomp, which is enabled by default, is only supported on Linux") - } - if !sysInfo.BlkioWeightDevice { - t.Skip("running test not supported by the host system") - } - args := []string{"--blkio-weight-device", "/dev/zero:100"} - a := createCLI(args) - a.InputArgs = args - runtimeSpec, err := getRuntimeSpec(a) - if err != nil { - t.Fatalf(err.Error()) - } - assert.Equal(t, *runtimeSpec.Linux.Resources.BlockIO.WeightDevice[0].Weight, uint16(100)) -} - -// TestMemorySwap verifies that the inputted memory swap is correctly defined in the spec -func TestMemorySwap(t *testing.T) { - // The default configuration of podman enables seccomp, which is not available on non-Linux systems. - // Thus, any tests that use the default seccomp setting would fail. - // Skip the tests on non-Linux platforms rather than explicitly disable seccomp in the test and possibly affect the test result. - if runtime.GOOS != "linux" { - t.Skip("seccomp, which is enabled by default, is only supported on Linux") - } - if !sysInfo.SwapLimit { - t.Skip("running test not supported by the host system") - } - args := []string{"--memory-swap", "45m", "--memory", "40m"} - a := createCLI(args) - a.InputArgs = args - //a.Run(append(cmd, args...)) - runtimeSpec, err := getRuntimeSpec(a) - if err != nil { - t.Fatalf(err.Error()) - } - mem, _ := units.RAMInBytes("45m") - assert.Equal(t, *runtimeSpec.Linux.Resources.Memory.Swap, mem) -} diff --git a/pkg/spec/containerconfig.go b/pkg/spec/containerconfig.go index f9bb075b8..b2f8a268f 100644 --- a/pkg/spec/containerconfig.go +++ b/pkg/spec/containerconfig.go @@ -24,8 +24,6 @@ func (config *CreateConfig) MakeContainerConfig(runtime *libpod.Runtime, pod *li return nil, nil, err } - logrus.Debugf("got mounts as %v", mounts) - runtimeSpec, err := config.createConfigToOCISpec(runtime, mounts) if err != nil { return nil, nil, err diff --git a/pkg/spec/spec_test.go b/pkg/spec/spec_test.go index b7c9c9c0d..0abff491b 100644 --- a/pkg/spec/spec_test.go +++ b/pkg/spec/spec_test.go @@ -1,39 +1,98 @@ package createconfig import ( - "reflect" + "runtime" "testing" - spec "github.com/opencontainers/runtime-spec/specs-go" + "github.com/containers/libpod/pkg/sysinfo" + "github.com/containers/storage" + "github.com/containers/storage/pkg/idtools" + "github.com/docker/go-units" "github.com/stretchr/testify/assert" ) -func TestGetVolumeMountsOneVolume(t *testing.T) { - data := spec.Mount{ - Destination: "/foobar", - Type: "bind", - Source: "/foobar", - Options: []string{"ro", "rbind", "rprivate"}, +var ( + sysInfo = sysinfo.New(true) +) + +// Make createconfig to test with +func makeTestCreateConfig() *CreateConfig { + cc := new(CreateConfig) + cc.Resources = CreateResourceConfig{} + cc.IDMappings = new(storage.IDMappingOptions) + cc.IDMappings.UIDMap = []idtools.IDMap{} + cc.IDMappings.GIDMap = []idtools.IDMap{} + + return cc +} + +// TestPIDsLimit verifies the given pid-limit is correctly defined in the spec +func TestPIDsLimit(t *testing.T) { + // The default configuration of podman enables seccomp, which is not available on non-Linux systems. + // Thus, any tests that use the default seccomp setting would fail. + // Skip the tests on non-Linux platforms rather than explicitly disable seccomp in the test and possibly affect the test result. + if runtime.GOOS != "linux" { + t.Skip("seccomp, which is enabled by default, is only supported on Linux") } - config := CreateConfig{ - Volumes: []string{"/foobar:/foobar:ro"}, + if !sysInfo.PidsLimit { + t.Skip("running test not supported by the host system") } - specMount, _, err := config.getVolumeMounts() + + cc := makeTestCreateConfig() + cc.Resources.PidsLimit = 22 + + spec, err := cc.createConfigToOCISpec(nil, nil) assert.NoError(t, err) - assert.True(t, reflect.DeepEqual(data, specMount[data.Destination])) + + assert.Equal(t, spec.Linux.Resources.Pids.Limit, int64(22)) } -func TestGetTmpfsMounts(t *testing.T) { - data := spec.Mount{ - Destination: "/homer", - Type: "tmpfs", - Source: "tmpfs", - Options: []string{"rw", "size=787448k", "mode=1777"}, +// TestBLKIOWeightDevice verifies the given blkio weight is correctly set in the +// spec. +func TestBLKIOWeightDevice(t *testing.T) { + // The default configuration of podman enables seccomp, which is not available on non-Linux systems. + // Thus, any tests that use the default seccomp setting would fail. + // Skip the tests on non-Linux platforms rather than explicitly disable seccomp in the test and possibly affect the test result. + if runtime.GOOS != "linux" { + t.Skip("seccomp, which is enabled by default, is only supported on Linux") } - config := CreateConfig{ - Tmpfs: []string{"/homer:rw,size=787448k,mode=1777"}, + if !sysInfo.BlkioWeightDevice { + t.Skip("running test not supported by the host system") } - tmpfsMount, err := config.getTmpfsMounts() + + cc := makeTestCreateConfig() + cc.Resources.BlkioWeightDevice = []string{"/dev/zero:100"} + + spec, err := cc.createConfigToOCISpec(nil, nil) assert.NoError(t, err) - assert.True(t, reflect.DeepEqual(data, tmpfsMount[data.Destination])) + + // /dev/zero is guaranteed 1,5 by the Linux kernel + assert.Equal(t, spec.Linux.Resources.BlockIO.WeightDevice[0].Major, int64(1)) + assert.Equal(t, spec.Linux.Resources.BlockIO.WeightDevice[0].Minor, int64(5)) + assert.Equal(t, *(spec.Linux.Resources.BlockIO.WeightDevice[0].Weight), uint16(100)) +} + +// TestMemorySwap verifies that the given swap memory limit is correctly set in +// the spec. +func TestMemorySwap(t *testing.T) { + // The default configuration of podman enables seccomp, which is not available on non-Linux systems. + // Thus, any tests that use the default seccomp setting would fail. + // Skip the tests on non-Linux platforms rather than explicitly disable seccomp in the test and possibly affect the test result. + if runtime.GOOS != "linux" { + t.Skip("seccomp, which is enabled by default, is only supported on Linux") + } + if !sysInfo.SwapLimit { + t.Skip("running test not supported by the host system") + } + + swapLimit, err := units.RAMInBytes("45m") + assert.NoError(t, err) + + cc := makeTestCreateConfig() + cc.Resources.MemorySwap = swapLimit + + spec, err := cc.createConfigToOCISpec(nil, nil) + assert.NoError(t, err) + + assert.Equal(t, *(spec.Linux.Resources.Memory.Swap), swapLimit) } diff --git a/pkg/spec/storage_test.go b/pkg/spec/storage_test.go new file mode 100644 index 000000000..4d96a2de6 --- /dev/null +++ b/pkg/spec/storage_test.go @@ -0,0 +1,39 @@ +package createconfig + +import ( + "reflect" + "testing" + + spec "github.com/opencontainers/runtime-spec/specs-go" + "github.com/stretchr/testify/assert" +) + +func TestGetVolumeMountsOneVolume(t *testing.T) { + data := spec.Mount{ + Destination: "/foobar", + Type: "bind", + Source: "/tmp", + Options: []string{"ro", "rbind", "rprivate"}, + } + config := CreateConfig{ + Volumes: []string{"/tmp:/foobar:ro"}, + } + specMount, _, err := config.getVolumeMounts() + assert.NoError(t, err) + assert.True(t, reflect.DeepEqual(data, specMount[data.Destination])) +} + +func TestGetTmpfsMounts(t *testing.T) { + data := spec.Mount{ + Destination: "/homer", + Type: "tmpfs", + Source: "tmpfs", + Options: []string{"rw", "size=787448k", "mode=1777"}, + } + config := CreateConfig{ + Tmpfs: []string{"/homer:rw,size=787448k,mode=1777"}, + } + tmpfsMount, err := config.getTmpfsMounts() + assert.NoError(t, err) + assert.True(t, reflect.DeepEqual(data, tmpfsMount[data.Destination])) +} -- cgit v1.2.3-54-g00ecf From 70beb57faa1cfb1a2e9be411628477bc1618c3cf Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 23 Apr 2019 15:35:38 -0400 Subject: Fix options for non-bind and non-tmpfs volumes We were unconditionally resetting volume mount options for all mount points (and by the looks of things, completely dropping tmpfs mounts), which was causing runc to refuse to run containers and all the tests to consequently fail. Signed-off-by: Matthew Heon --- pkg/spec/storage.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) (limited to 'pkg') diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go index 7806a8c72..6f8398513 100644 --- a/pkg/spec/storage.go +++ b/pkg/spec/storage.go @@ -100,6 +100,8 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, unifiedMounts[dest] = tmpfs } + // TODO: Check for conflicts between volumes and named volumes. + // If requested, add container init binary if config.Init { initPath := config.InitPath @@ -143,6 +145,9 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, finalVolumes = append(finalVolumes, volume) } + logrus.Debugf("Got mounts: %v", finalMounts) + logrus.Debugf("Got volumes: %v", finalVolumes) + return finalMounts, finalVolumes, nil } @@ -377,9 +382,6 @@ func getBindMount(args []string) (spec.Mount, error) { newMount.Source = newMount.Destination } - // Process options - newMount.Options = processOptions(newMount.Options) - return newMount, nil } @@ -579,7 +581,7 @@ func (config *CreateConfig) getVolumeMounts() (map[string]spec.Mount, map[string Destination: dest, Type: string(TypeBind), Source: src, - Options: processOptions(options), + Options: options, } if _, ok := mounts[newMount.Destination]; ok { return nil, nil, errors.Wrapf(errDuplicateDest, newMount.Destination) @@ -754,12 +756,13 @@ func supercedeUserMounts(mounts []spec.Mount, configMount []spec.Mount) []spec.M func initFSMounts(inputMounts []spec.Mount) []spec.Mount { var mounts []spec.Mount for _, m := range inputMounts { - m.Options = processOptions(m.Options) - if m.Type == "tmpfs" { + if m.Type == TypeBind { + m.Options = processOptions(m.Options) + } + if m.Type == TypeTmpfs { m.Options = append(m.Options, "tmpcopyup") - } else { - mounts = append(mounts, m) } + mounts = append(mounts, m) } return mounts } -- cgit v1.2.3-54-g00ecf From 13451cab5c20b8f28602debbb382e1351930e2d5 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 23 Apr 2019 16:08:48 -0400 Subject: Hit a number of to-do comments in unified volumes code As part of this, move bind mount option validity parsing and modification (adding e.g. rbind on bind mounts that are missing it), which requires test changes (expected values have changed). Signed-off-by: Matthew Heon --- pkg/spec/storage.go | 73 ++++++++++++++++++++++++++++++------------------ pkg/spec/storage_test.go | 2 +- 2 files changed, 47 insertions(+), 28 deletions(-) (limited to 'pkg') diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go index 6f8398513..9660c278f 100644 --- a/pkg/spec/storage.go +++ b/pkg/spec/storage.go @@ -79,6 +79,7 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, } // Unify mounts from --mount, --volume, --tmpfs. + // Also add mounts + volumes directly from createconfig. // Start with --volume. for dest, mount := range volumeMounts { if _, ok := unifiedMounts[dest]; ok { @@ -99,8 +100,21 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, } unifiedMounts[dest] = tmpfs } - - // TODO: Check for conflicts between volumes and named volumes. + // Now spec mounts and volumes + for _, mount := range config.Mounts { + dest := mount.Destination + if _, ok := unifiedMounts[dest]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, dest) + } + unifiedMounts[dest] = mount + } + for _, volume := range config.NamedVolumes { + dest := volume.Dest + if _, ok := unifiedVolumes[dest]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, dest) + } + unifiedVolumes[dest] = volume + } // If requested, add container init binary if config.Init { @@ -131,9 +145,17 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, baseVolumes[dest] = volume } - // TODO: Allow an explicit list of named volumes and spec mounts in the - // createconfig. Podman doesn't need them, but other libpod consumers - // might want them. + // Check for conflicts between named volumes and mounts + for dest := range unifiedMounts { + if _, ok := unifiedVolumes[dest]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, "conflict at mount destination %v", dest) + } + } + for dest := range unifiedVolumes { + if _, ok := unifiedMounts[dest]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, "conflict at mount destination %v", dest) + } + } // Final step: maps to arrays finalMounts := make([]spec.Mount, 0, len(baseMounts)) @@ -164,19 +186,11 @@ func (config *CreateConfig) getVolumesFrom(runtime *libpod.Runtime) (map[string] // Both of these are maps of mount destination to mount type. // We ensure that each destination is only mounted to once in this way. - // We tiebreak only within each category; we'll let libpod tiebreak - // between mounts and named volumes. - // TODO: Should we unmount subtree mounts? E.g., if /tmp/ is mounted by - // one mount, and we already have /tmp/a and /tmp/b, should we remove - // the /tmp/a and /tmp/b mounts in favor of the more general /tmp? finalMounts := make(map[string]spec.Mount) finalNamedVolumes := make(map[string]*libpod.ContainerNamedVolume) for _, vol := range config.VolumesFrom { options := []string{} - // TODO: now that container names may contain colons this is no - // longer safe. - // I don't know if there's much we can do here... splitVol := strings.SplitN(vol, ":", 2) if len(splitVol) == 2 { if strings.Contains(splitVol[1], "Z") || @@ -185,10 +199,10 @@ func (config *CreateConfig) getVolumesFrom(runtime *libpod.Runtime) (map[string] strings.Contains(splitVol[1], "shared") { return nil, nil, errors.Errorf("invalid options %q, can only specify 'ro', 'rw', and 'z", splitVol[1]) } - if err := ValidateVolumeOpts(splitVol[1]); err != nil { + options = strings.Split(splitVol[1], ",") + if err := ValidateVolumeOpts(options); err != nil { return nil, nil, err } - options = strings.Split(splitVol[1], ",") } ctr, err := runtime.LookupContainer(splitVol[0]) if err != nil { @@ -339,14 +353,12 @@ func getBindMount(args []string) (spec.Mount, error) { kv := strings.Split(val, "=") switch kv[0] { case "ro", "nosuid", "nodev", "noexec": - // TODO: detect duplication of these options + // TODO: detect duplication of these options. + // (Is this necessary?) newMount.Options = append(newMount.Options, kv[0]) case "shared", "rshared", "private", "rprivate", "slave", "rslave", "Z", "z": - // TODO: detect duplicate mount propagation options newMount.Options = append(newMount.Options, kv[0]) case "bind-propagation": - // TODO: this should conflict with the mount propagation - // options above. if len(kv) == 1 { return newMount, errors.Wrapf(optionArgError, kv[0]) } @@ -382,6 +394,10 @@ func getBindMount(args []string) (spec.Mount, error) { newMount.Source = newMount.Destination } + if err := ValidateVolumeOpts(newMount.Options); err != nil { + return newMount, err + } + return newMount, nil } @@ -509,28 +525,27 @@ func ValidateVolumeCtrDir(ctrDir string) error { } // ValidateVolumeOpts validates a volume's options -func ValidateVolumeOpts(option string) error { +func ValidateVolumeOpts(options []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) + return errors.Errorf("invalid options %q, can only specify 1 'rw' or 'ro' option", strings.Join(options, ", ")) } case "z", "Z": foundLabelChange++ if foundLabelChange > 1 { - return errors.Errorf("invalid options %q, can only specify 1 'z' or 'Z' option", option) + return errors.Errorf("invalid options %q, can only specify 1 'z' or 'Z' option", strings.Join(options, ", ")) } 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) + return errors.Errorf("invalid options %q, can only specify 1 '[r]shared', '[r]private' or '[r]slave' option", strings.Join(options, ", ")) } default: - return errors.Errorf("invalid option type %q", option) + return errors.Errorf("invalid option type %q", opt) } } return nil @@ -562,10 +577,10 @@ func (config *CreateConfig) getVolumeMounts() (map[string]spec.Mount, map[string dest = splitVol[1] } if len(splitVol) > 2 { - if err := ValidateVolumeOpts(splitVol[2]); err != nil { + options = strings.Split(splitVol[2], ",") + if err := ValidateVolumeOpts(options); err != nil { return nil, nil, err } - options = strings.Split(splitVol[2], ",") } if err := ValidateVolumeHostDir(src); err != nil { @@ -724,6 +739,10 @@ func processOptions(options []string) []string { return options } +// Supercede existing mounts in the spec with new, user-specified mounts. +// TODO: Should we unmount subtree mounts? E.g., if /tmp/ is mounted by +// one mount, and we already have /tmp/a and /tmp/b, should we remove +// the /tmp/a and /tmp/b mounts in favor of the more general /tmp? func supercedeUserMounts(mounts []spec.Mount, configMount []spec.Mount) []spec.Mount { if len(mounts) > 0 { // If we have overlappings mounts, remove them from the spec in favor of diff --git a/pkg/spec/storage_test.go b/pkg/spec/storage_test.go index 4d96a2de6..2439efb6f 100644 --- a/pkg/spec/storage_test.go +++ b/pkg/spec/storage_test.go @@ -13,7 +13,7 @@ func TestGetVolumeMountsOneVolume(t *testing.T) { Destination: "/foobar", Type: "bind", Source: "/tmp", - Options: []string{"ro", "rbind", "rprivate"}, + Options: []string{"ro"}, } config := CreateConfig{ Volumes: []string{"/tmp:/foobar:ro"}, -- cgit v1.2.3-54-g00ecf From 7a421a807c38dbe878b59c637f67c289cea7d418 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 23 Apr 2019 16:10:42 -0400 Subject: Use EqualValues instead of reflect equality Same result, but notably better error messages when things go wrong. Signed-off-by: Matthew Heon --- pkg/spec/storage_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'pkg') diff --git a/pkg/spec/storage_test.go b/pkg/spec/storage_test.go index 2439efb6f..04a9d6976 100644 --- a/pkg/spec/storage_test.go +++ b/pkg/spec/storage_test.go @@ -1,7 +1,6 @@ package createconfig import ( - "reflect" "testing" spec "github.com/opencontainers/runtime-spec/specs-go" @@ -20,7 +19,7 @@ func TestGetVolumeMountsOneVolume(t *testing.T) { } specMount, _, err := config.getVolumeMounts() assert.NoError(t, err) - assert.True(t, reflect.DeepEqual(data, specMount[data.Destination])) + assert.EqualValues(t, data, specMount[data.Destination]) } func TestGetTmpfsMounts(t *testing.T) { @@ -35,5 +34,5 @@ func TestGetTmpfsMounts(t *testing.T) { } tmpfsMount, err := config.getTmpfsMounts() assert.NoError(t, err) - assert.True(t, reflect.DeepEqual(data, tmpfsMount[data.Destination])) + assert.EqualValues(t, data, tmpfsMount[data.Destination]) } -- cgit v1.2.3-54-g00ecf From 2698c82240e4b9be5aaac787f8008fd6b74f645b Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 24 Apr 2019 13:42:41 -0400 Subject: Add options parsing for tmpfs mounts This ensures that all tmpfs mounts added by the user, even with the --mount flag, share a few common options (nosuid, noexec, nodev), and options for tmpfs mounts are properly validated to ensure they are correct. Signed-off-by: Matthew Heon --- pkg/spec/storage.go | 73 ++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 7 deletions(-) (limited to 'pkg') diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go index 9660c278f..c838e7204 100644 --- a/pkg/spec/storage.go +++ b/pkg/spec/storage.go @@ -28,6 +28,7 @@ var ( badOptionError = errors.Errorf("invalid mount option") optionArgError = errors.Errorf("must provide an argument for option") noDestError = errors.Errorf("must set volume destination") + dupeOptionError = errors.Errorf("duplicate option passed") ) // Parse all volume-related options in the create config into a set of mounts @@ -160,6 +161,14 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, // Final step: maps to arrays finalMounts := make([]spec.Mount, 0, len(baseMounts)) for _, mount := range baseMounts { + // All user-added tmpfs mounts need their options processed. + if mount.Type == TypeTmpfs { + opts, err := processTmpfsOptions(mount.Options) + if err != nil { + return nil, nil, err + } + mount.Options = opts + } finalMounts = append(finalMounts, mount) } finalVolumes := make([]*libpod.ContainerNamedVolume, 0, len(baseVolumes)) @@ -414,16 +423,13 @@ func getTmpfsMount(args []string) (spec.Mount, error) { kv := strings.Split(val, "=") switch kv[0] { case "ro", "nosuid", "nodev", "noexec": - // TODO: detect duplication of these options newMount.Options = append(newMount.Options, kv[0]) case "tmpfs-mode": - // TODO: detect duplicate modes if len(kv) == 1 { return newMount, errors.Wrapf(optionArgError, kv[0]) } newMount.Options = append(newMount.Options, fmt.Sprintf("mode=%s", kv[1])) case "tmpfs-size": - // TODO: detect duplicate sizes if len(kv) == 1 { return newMount, errors.Wrapf(optionArgError, kv[0]) } @@ -657,7 +663,7 @@ func (config *CreateConfig) getTmpfsMounts() (map[string]spec.Mount, error) { m := make(map[string]spec.Mount) for _, i := range config.Tmpfs { // Default options if nothing passed - options := []string{"rprivate", "rw", "noexec", "nosuid", "nodev", "size=65536k"} + var options []string spliti := strings.Split(i, ":") destPath := spliti[0] if len(spliti) > 1 { @@ -668,9 +674,6 @@ func (config *CreateConfig) getTmpfsMounts() (map[string]spec.Mount, error) { return nil, errors.Wrapf(errDuplicateDest, destPath) } - // TODO: Do we want to set some default options if the user - // passed options? - // TODO: should we validate the options passed in? mount := spec.Mount{ Destination: destPath, Type: string(TypeTmpfs), @@ -739,6 +742,62 @@ func processOptions(options []string) []string { return options } +// Handle options for tmpfs mounts +func processTmpfsOptions(options []string) ([]string, error) { + var ( + foundWrite, foundSize, foundProp, foundMode bool + ) + + baseOpts := []string{"noexec", "nosuid", "nodev"} + for _, opt := range options { + // Some options have parameters - size, mode + splitOpt := strings.SplitN(opt, "=", 2) + switch splitOpt[0] { + case "rw", "ro": + if foundWrite { + return nil, errors.Wrapf(dupeOptionError, "only one of rw and ro can be used") + } + foundWrite = true + baseOpts = append(baseOpts, opt) + case "private", "rprivate", "slave", "rslave", "shared", "rshared": + if foundProp { + return nil, errors.Wrapf(dupeOptionError, "only one root propagation mode can be used") + } + foundProp = true + baseOpts = append(baseOpts, opt) + case "size": + if foundSize { + return nil, errors.Wrapf(dupeOptionError, "only one tmpfs size can be specified") + } + foundSize = true + baseOpts = append(baseOpts, opt) + case "mode": + if foundMode { + return nil, errors.Wrapf(dupeOptionError, "only one tmpfs mode can be specified") + } + foundMode = true + baseOpts = append(baseOpts, opt) + case "noexec", "nodev", "nosuid": + // Do nothing. We always include these even if they are + // not explicitly requested. + default: + return nil, errors.Wrapf(badOptionError, "unknown tmpfs option %q", opt) + } + } + + if !foundWrite { + baseOpts = append(baseOpts, "rw") + } + if !foundSize { + baseOpts = append(baseOpts, "size=65536k") + } + if !foundProp { + baseOpts = append(baseOpts, "rprivate") + } + + return baseOpts, nil +} + // Supercede existing mounts in the spec with new, user-specified mounts. // TODO: Should we unmount subtree mounts? E.g., if /tmp/ is mounted by // one mount, and we already have /tmp/a and /tmp/b, should we remove -- cgit v1.2.3-54-g00ecf From 30257cf07314404cba90b1972bf75db5812971fd Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 24 Apr 2019 14:14:26 -0400 Subject: Ensure that named volumes have their options parsed This involves moving some code out of pkg/spec/ into util/ so it can also be used by libpod. Signed-off-by: Matthew Heon --- libpod/options.go | 3 +- pkg/spec/storage.go | 95 ++++--------------------------------------------- pkg/util/mountOpts.go | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 90 deletions(-) create mode 100644 pkg/util/mountOpts.go (limited to 'pkg') diff --git a/libpod/options.go b/libpod/options.go index 9932d5453..dde9bc64c 100644 --- a/libpod/options.go +++ b/libpod/options.go @@ -11,6 +11,7 @@ import ( "github.com/containers/image/manifest" "github.com/containers/libpod/pkg/namespaces" "github.com/containers/libpod/pkg/rootless" + "github.com/containers/libpod/pkg/util" "github.com/containers/storage" "github.com/containers/storage/pkg/idtools" "github.com/cri-o/ocicni/pkg/ocicni" @@ -1288,7 +1289,7 @@ func WithNamedVolumes(volumes []*ContainerNamedVolume) CtrCreateOption { ctr.config.NamedVolumes = append(ctr.config.NamedVolumes, &ContainerNamedVolume{ Name: vol.Name, Dest: vol.Dest, - Options: vol.Options, + Options: util.ProcessOptions(vol.Options), }) } diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go index c838e7204..3993c2940 100644 --- a/pkg/spec/storage.go +++ b/pkg/spec/storage.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/containers/libpod/libpod" + "github.com/containers/libpod/pkg/util" "github.com/containers/storage/pkg/stringid" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" @@ -25,10 +26,8 @@ const ( var ( errDuplicateDest = errors.Errorf("duplicate mount destination") - badOptionError = errors.Errorf("invalid mount option") optionArgError = errors.Errorf("must provide an argument for option") noDestError = errors.Errorf("must set volume destination") - dupeOptionError = errors.Errorf("duplicate option passed") ) // Parse all volume-related options in the create config into a set of mounts @@ -163,7 +162,7 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, for _, mount := range baseMounts { // All user-added tmpfs mounts need their options processed. if mount.Type == TypeTmpfs { - opts, err := processTmpfsOptions(mount.Options) + opts, err := util.ProcessTmpfsOptions(mount.Options) if err != nil { return nil, nil, err } @@ -391,7 +390,7 @@ func getBindMount(args []string) (spec.Mount, error) { newMount.Destination = kv[1] setDest = true default: - return newMount, errors.Wrapf(badOptionError, kv[0]) + return newMount, errors.Wrapf(util.ErrBadMntOption, kv[0]) } } @@ -446,7 +445,7 @@ func getTmpfsMount(args []string) (spec.Mount, error) { newMount.Destination = kv[1] setDest = true default: - return newMount, errors.Wrapf(badOptionError, kv[0]) + return newMount, errors.Wrapf(util.ErrBadMntOption, kv[0]) } } @@ -490,7 +489,7 @@ func getNamedVolume(args []string) (*libpod.ContainerNamedVolume, error) { newVolume.Dest = kv[1] setDest = true default: - return nil, errors.Wrapf(badOptionError, kv[0]) + return nil, errors.Wrapf(util.ErrBadMntOption, kv[0]) } } @@ -716,88 +715,6 @@ func (config *CreateConfig) addContainerInitBinary(path string) (spec.Mount, err return mount, nil } -// Handle options for volume mounts -func processOptions(options []string) []string { - var ( - foundrw, foundro bool - rootProp string - ) - options = append(options, "rbind") - for _, opt := range options { - switch opt { - case "rw": - foundrw = true - case "ro": - foundro = true - case "private", "rprivate", "slave", "rslave", "shared", "rshared": - rootProp = opt - } - } - if !foundrw && !foundro { - options = append(options, "rw") - } - if rootProp == "" { - options = append(options, "rprivate") - } - return options -} - -// Handle options for tmpfs mounts -func processTmpfsOptions(options []string) ([]string, error) { - var ( - foundWrite, foundSize, foundProp, foundMode bool - ) - - baseOpts := []string{"noexec", "nosuid", "nodev"} - for _, opt := range options { - // Some options have parameters - size, mode - splitOpt := strings.SplitN(opt, "=", 2) - switch splitOpt[0] { - case "rw", "ro": - if foundWrite { - return nil, errors.Wrapf(dupeOptionError, "only one of rw and ro can be used") - } - foundWrite = true - baseOpts = append(baseOpts, opt) - case "private", "rprivate", "slave", "rslave", "shared", "rshared": - if foundProp { - return nil, errors.Wrapf(dupeOptionError, "only one root propagation mode can be used") - } - foundProp = true - baseOpts = append(baseOpts, opt) - case "size": - if foundSize { - return nil, errors.Wrapf(dupeOptionError, "only one tmpfs size can be specified") - } - foundSize = true - baseOpts = append(baseOpts, opt) - case "mode": - if foundMode { - return nil, errors.Wrapf(dupeOptionError, "only one tmpfs mode can be specified") - } - foundMode = true - baseOpts = append(baseOpts, opt) - case "noexec", "nodev", "nosuid": - // Do nothing. We always include these even if they are - // not explicitly requested. - default: - return nil, errors.Wrapf(badOptionError, "unknown tmpfs option %q", opt) - } - } - - if !foundWrite { - baseOpts = append(baseOpts, "rw") - } - if !foundSize { - baseOpts = append(baseOpts, "size=65536k") - } - if !foundProp { - baseOpts = append(baseOpts, "rprivate") - } - - return baseOpts, nil -} - // Supercede existing mounts in the spec with new, user-specified mounts. // TODO: Should we unmount subtree mounts? E.g., if /tmp/ is mounted by // one mount, and we already have /tmp/a and /tmp/b, should we remove @@ -835,7 +752,7 @@ func initFSMounts(inputMounts []spec.Mount) []spec.Mount { var mounts []spec.Mount for _, m := range inputMounts { if m.Type == TypeBind { - m.Options = processOptions(m.Options) + m.Options = util.ProcessOptions(m.Options) } if m.Type == TypeTmpfs { m.Options = append(m.Options, "tmpcopyup") diff --git a/pkg/util/mountOpts.go b/pkg/util/mountOpts.go new file mode 100644 index 000000000..59459807c --- /dev/null +++ b/pkg/util/mountOpts.go @@ -0,0 +1,98 @@ +package util + +import ( + "strings" + + "github.com/pkg/errors" +) + +var ( + // ErrBadMntOption indicates that an invalid mount option was passed. + ErrBadMntOption = errors.Errorf("invalid mount option") + // ErrDupeMntOption indicates that a duplicate mount option was passed. + ErrDupeMntOption = errors.Errorf("duplicate option passed") +) + +// ProcessOptions parses the options for a bind mount and ensures that they are +// sensible and follow convention. +func ProcessOptions(options []string) []string { + var ( + foundrw, foundro bool + rootProp string + ) + options = append(options, "rbind") + for _, opt := range options { + switch opt { + case "rw": + foundrw = true + case "ro": + foundro = true + case "private", "rprivate", "slave", "rslave", "shared", "rshared": + rootProp = opt + } + } + if !foundrw && !foundro { + options = append(options, "rw") + } + if rootProp == "" { + options = append(options, "rprivate") + } + return options +} + +// ProcessTmpfsOptions parses the options for a tmpfs mountpoint and ensures +// that they are sensible and follow convention. +func ProcessTmpfsOptions(options []string) ([]string, error) { + var ( + foundWrite, foundSize, foundProp, foundMode bool + ) + + baseOpts := []string{"noexec", "nosuid", "nodev"} + for _, opt := range options { + // Some options have parameters - size, mode + splitOpt := strings.SplitN(opt, "=", 2) + switch splitOpt[0] { + case "rw", "ro": + if foundWrite { + return nil, errors.Wrapf(ErrDupeMntOption, "only one of rw and ro can be used") + } + foundWrite = true + baseOpts = append(baseOpts, opt) + case "private", "rprivate", "slave", "rslave", "shared", "rshared": + if foundProp { + return nil, errors.Wrapf(ErrDupeMntOption, "only one root propagation mode can be used") + } + foundProp = true + baseOpts = append(baseOpts, opt) + case "size": + if foundSize { + return nil, errors.Wrapf(ErrDupeMntOption, "only one tmpfs size can be specified") + } + foundSize = true + baseOpts = append(baseOpts, opt) + case "mode": + if foundMode { + return nil, errors.Wrapf(ErrDupeMntOption, "only one tmpfs mode can be specified") + } + foundMode = true + baseOpts = append(baseOpts, opt) + case "noexec", "nodev", "nosuid": + // Do nothing. We always include these even if they are + // not explicitly requested. + default: + return nil, errors.Wrapf(ErrBadMntOption, "unknown tmpfs option %q", opt) + } + } + + if !foundWrite { + baseOpts = append(baseOpts, "rw") + } + if !foundSize { + baseOpts = append(baseOpts, "size=65536k") + } + if !foundProp { + baseOpts = append(baseOpts, "rprivate") + } + + return baseOpts, nil +} -- cgit v1.2.3-54-g00ecf From 606cee93bfabe2b8177dad53168e51cd1aeeb9ee Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 29 Apr 2019 16:01:05 -0400 Subject: Move handling of ReadOnlyTmpfs into new mounts code Signed-off-by: Matthew Heon --- pkg/spec/spec.go | 52 ---------------------------------------------------- pkg/spec/storage.go | 43 ++++++++++++++++++++++++++++++++++++------- 2 files changed, 36 insertions(+), 59 deletions(-) (limited to 'pkg') diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index 591a28703..20c649f9a 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -7,7 +7,6 @@ import ( "github.com/containers/libpod/libpod" "github.com/containers/libpod/pkg/rootless" - "github.com/containers/storage/pkg/mount" pmount "github.com/containers/storage/pkg/mount" "github.com/docker/docker/oci/caps" "github.com/docker/go-units" @@ -278,57 +277,6 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime, userM addedResources = true } - for _, i := range config.Tmpfs { - // Default options if nothing passed - options := []string{"rw", "rprivate", "noexec", "nosuid", "nodev", "size=65536k"} - spliti := strings.SplitN(i, ":", 2) - if len(spliti) > 1 { - if _, _, err := mount.ParseTmpfsOptions(spliti[1]); err != nil { - return nil, err - } - options = strings.Split(spliti[1], ",") - } - tmpfsMnt := spec.Mount{ - Destination: spliti[0], - Type: "tmpfs", - Source: "tmpfs", - Options: append(options, "tmpcopyup"), - } - g.AddMount(tmpfsMnt) - } - - for _, m := range config.Mounts { - if m.Type == "tmpfs" { - g.AddMount(m) - } - } - - if config.ReadOnlyRootfs && config.ReadOnlyTmpfs { - options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup"} - for _, i := range []string{"/tmp", "/var/tmp"} { - if libpod.MountExists(g.Config.Mounts, i) { - continue - } - // Default options if nothing passed - tmpfsMnt := spec.Mount{ - Destination: i, - Type: "tmpfs", - Source: "tmpfs", - Options: options, - } - g.AddMount(tmpfsMnt) - } - if !libpod.MountExists(g.Config.Mounts, "/run") { - tmpfsMnt := spec.Mount{ - Destination: "/run", - Type: "tmpfs", - Source: "tmpfs", - Options: append(options, "noexec", "size=65536k"), - } - g.AddMount(tmpfsMnt) - } - } - for name, val := range config.Env { g.AddProcessEnv(name, val) } diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go index 3993c2940..55148b606 100644 --- a/pkg/spec/storage.go +++ b/pkg/spec/storage.go @@ -35,8 +35,7 @@ var ( // Handles --volumes-from, --volumes, --tmpfs, --init, and --init-path flags. // TODO: Named volume options - should we default to rprivate? It bakes into a // bind mount under the hood... -// TODO: Tmpfs options - we should probably check user-given ones, provide sane -// defaults even if the user provides a few... +// TODO: handle options parsing/processing via containers/storage/pkg/mount func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, []*libpod.ContainerNamedVolume, error) { // Add image volumes. baseMounts, baseVolumes, err := config.getImageVolumes() @@ -136,6 +135,34 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, unifiedMounts[initMount.Destination] = initMount } + // If requested, add tmpfs filesystems for read-only containers. + // Need to keep track of which we created, so we don't modify options + // for them later... + readonlyTmpfs := map[string]bool{ + "/tmp": false, + "/var/tmp": false, + "/run": false, + } + if config.ReadOnlyRootfs && config.ReadOnlyTmpfs { + options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup", "size=65536k"} + for dest := range readonlyTmpfs { + if _, ok := unifiedMounts[dest]; ok { + continue + } + localOpts := options + if dest == "/run" { + localOpts = append(localOpts, "noexec") + } + unifiedMounts[dest] = spec.Mount{ + Destination: dest, + Type: "tmpfs", + Source: "tmpfs", + Options: localOpts, + } + readonlyTmpfs[dest] = true + } + } + // Supercede volumes-from/image volumes with unified volumes from above. // This is an unconditional replacement. for dest, mount := range unifiedMounts { @@ -146,13 +173,13 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, } // Check for conflicts between named volumes and mounts - for dest := range unifiedMounts { - if _, ok := unifiedVolumes[dest]; ok { + for dest := range baseMounts { + if _, ok := baseVolumes[dest]; ok { return nil, nil, errors.Wrapf(errDuplicateDest, "conflict at mount destination %v", dest) } } - for dest := range unifiedVolumes { - if _, ok := unifiedMounts[dest]; ok { + for dest := range baseVolumes { + if _, ok := baseMounts[dest]; ok { return nil, nil, errors.Wrapf(errDuplicateDest, "conflict at mount destination %v", dest) } } @@ -161,7 +188,9 @@ func (config *CreateConfig) parseVolumes(runtime *libpod.Runtime) ([]spec.Mount, finalMounts := make([]spec.Mount, 0, len(baseMounts)) for _, mount := range baseMounts { // All user-added tmpfs mounts need their options processed. - if mount.Type == TypeTmpfs { + // Exception: mounts added by the ReadOnlyTmpfs option, which + // contain several exceptions to normal options rules. + if mount.Type == TypeTmpfs && !readonlyTmpfs[mount.Destination] { opts, err := util.ProcessTmpfsOptions(mount.Options) if err != nil { return nil, nil, err -- cgit v1.2.3-54-g00ecf