From 02671a103f7991a3c472f90d343a5979f3d3636a Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 27 Apr 2020 10:36:41 -0400 Subject: Add support for volumes-from, image volumes, init This should complete Podmanv2's support for volume-related flags. Most code was sourced from the old pkg/spec implementation with modifications to account for the split between frontend flags (volume, mount, tmpfs) and the backend flags implemented here. Also enables tests for podman run with volumes Signed-off-by: Matthew Heon --- cmd/podman/common/specgen.go | 10 +- pkg/api/handlers/libpod/containers_create.go | 3 +- pkg/domain/infra/abi/containers.go | 4 +- pkg/specgen/container_validate.go | 2 +- pkg/specgen/generate/container_create.go | 47 +++-- pkg/specgen/generate/oci.go | 17 +- pkg/specgen/generate/storage.go | 303 +++++++++++++++++++++++++++ pkg/specgen/specgen.go | 19 +- test/e2e/run_volume_test.go | 8 +- 9 files changed, 375 insertions(+), 38 deletions(-) diff --git a/cmd/podman/common/specgen.go b/cmd/podman/common/specgen.go index ba9022aff..e57581ca0 100644 --- a/cmd/podman/common/specgen.go +++ b/cmd/podman/common/specgen.go @@ -406,13 +406,11 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *ContainerCLIOpts, args []string s.StaticMAC = c.Net.StaticMAC s.UseImageHosts = c.Net.NoHosts - // deferred, must be added on libpod side - //var ImageVolumes map[string]struct{} - //if data != nil && c.String("image-volume") != "ignore" { - // ImageVolumes = data.Config.Volumes - //} - s.ImageVolumeMode = c.ImageVolume + if s.ImageVolumeMode == "bind" { + s.ImageVolumeMode = "anonymous" + } + systemd := c.SystemdD == "always" if !systemd && command != nil { x, err := strconv.ParseBool(c.SystemdD) diff --git a/pkg/api/handlers/libpod/containers_create.go b/pkg/api/handlers/libpod/containers_create.go index f64132d55..40b6cacdb 100644 --- a/pkg/api/handlers/libpod/containers_create.go +++ b/pkg/api/handlers/libpod/containers_create.go @@ -1,6 +1,7 @@ package libpod import ( + "context" "encoding/json" "net/http" @@ -26,7 +27,7 @@ func CreateContainer(w http.ResponseWriter, r *http.Request) { utils.InternalServerError(w, err) return } - ctr, err := generate.MakeContainer(runtime, &sg) + ctr, err := generate.MakeContainer(context.Background(), runtime, &sg) if err != nil { utils.InternalServerError(w, err) return diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index 9844d1d96..2a49eb0ca 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -481,7 +481,7 @@ func (ic *ContainerEngine) ContainerCreate(ctx context.Context, s *specgen.SpecG if err := generate.CompleteSpec(ctx, ic.Libpod, s); err != nil { return nil, err } - ctr, err := generate.MakeContainer(ic.Libpod, s) + ctr, err := generate.MakeContainer(ctx, ic.Libpod, s) if err != nil { return nil, err } @@ -669,7 +669,7 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta if err := generate.CompleteSpec(ctx, ic.Libpod, opts.Spec); err != nil { return nil, err } - ctr, err := generate.MakeContainer(ic.Libpod, opts.Spec) + ctr, err := generate.MakeContainer(ctx, ic.Libpod, opts.Spec) if err != nil { return nil, err } diff --git a/pkg/specgen/container_validate.go b/pkg/specgen/container_validate.go index 87fc59dfe..94e456c52 100644 --- a/pkg/specgen/container_validate.go +++ b/pkg/specgen/container_validate.go @@ -14,7 +14,7 @@ var ( // SystemDValues describes the only values that SystemD can be SystemDValues = []string{"true", "false", "always"} // ImageVolumeModeValues describes the only values that ImageVolumeMode can be - ImageVolumeModeValues = []string{"ignore", "tmpfs", "bind"} + ImageVolumeModeValues = []string{"ignore", "tmpfs", "anonymous"} ) func exclusiveOptions(opt1, opt2 string) error { diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go index 49a717c5d..7de47988d 100644 --- a/pkg/specgen/generate/container_create.go +++ b/pkg/specgen/generate/container_create.go @@ -15,7 +15,7 @@ import ( ) // MakeContainer creates a container based on the SpecGenerator -func MakeContainer(rt *libpod.Runtime, s *specgen.SpecGenerator) (*libpod.Container, error) { +func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGenerator) (*libpod.Container, error) { rtc, err := rt.GetConfig() if err != nil { return nil, err @@ -75,16 +75,8 @@ func MakeContainer(rt *libpod.Runtime, s *specgen.SpecGenerator) (*libpod.Contai s.CgroupNS = defaultNS } - options, err := createContainerOptions(rt, s, pod) - if err != nil { - return nil, err - } + options := []libpod.CtrCreateOption{} - podmanPath, err := os.Executable() - if err != nil { - return nil, err - } - options = append(options, createExitCommandOption(s, rt.StorageConfig(), rtc, podmanPath)) var newImage *image.Image if s.Rootfs != "" { options = append(options, libpod.WithRootFS(s.Rootfs)) @@ -99,14 +91,31 @@ func MakeContainer(rt *libpod.Runtime, s *specgen.SpecGenerator) (*libpod.Contai return nil, errors.Wrap(err, "invalid config provided") } - runtimeSpec, err := SpecGenToOCI(s, rt, newImage) + finalMounts, finalVolumes, err := finalizeMounts(ctx, s, rt, rtc, newImage) + if err != nil { + return nil, err + } + + opts, err := createContainerOptions(rt, s, pod, finalVolumes) + if err != nil { + return nil, err + } + options = append(options, opts...) + + podmanPath, err := os.Executable() + if err != nil { + return nil, err + } + options = append(options, createExitCommandOption(s, rt.StorageConfig(), rtc, podmanPath)) + + runtimeSpec, err := SpecGenToOCI(s, rt, rtc, newImage, finalMounts) if err != nil { return nil, err } - return rt.NewContainer(context.Background(), runtimeSpec, options...) + return rt.NewContainer(ctx, runtimeSpec, options...) } -func createContainerOptions(rt *libpod.Runtime, s *specgen.SpecGenerator, pod *libpod.Pod) ([]libpod.CtrCreateOption, error) { +func createContainerOptions(rt *libpod.Runtime, s *specgen.SpecGenerator, pod *libpod.Pod, volumes []*specgen.NamedVolume) ([]libpod.CtrCreateOption, error) { var options []libpod.CtrCreateOption var err error @@ -133,21 +142,21 @@ func createContainerOptions(rt *libpod.Runtime, s *specgen.SpecGenerator, pod *l for _, mount := range s.Mounts { destinations = append(destinations, mount.Destination) } - for _, volume := range s.Volumes { + for _, volume := range volumes { destinations = append(destinations, volume.Dest) } options = append(options, libpod.WithUserVolumes(destinations)) - if len(s.Volumes) != 0 { - var volumes []*libpod.ContainerNamedVolume - for _, v := range s.Volumes { - volumes = append(volumes, &libpod.ContainerNamedVolume{ + if len(volumes) != 0 { + var vols []*libpod.ContainerNamedVolume + for _, v := range volumes { + vols = append(vols, &libpod.ContainerNamedVolume{ Name: v.Name, Dest: v.Dest, Options: v.Options, }) } - options = append(options, libpod.WithNamedVolumes(volumes)) + options = append(options, libpod.WithNamedVolumes(vols)) } if len(s.Command) != 0 { diff --git a/pkg/specgen/generate/oci.go b/pkg/specgen/generate/oci.go index 8ca95016e..6b65ab10a 100644 --- a/pkg/specgen/generate/oci.go +++ b/pkg/specgen/generate/oci.go @@ -3,6 +3,7 @@ package generate import ( "strings" + "github.com/containers/common/pkg/config" "github.com/containers/libpod/libpod" "github.com/containers/libpod/libpod/image" "github.com/containers/libpod/pkg/rootless" @@ -48,7 +49,7 @@ func addRlimits(s *specgen.SpecGenerator, g *generate.Generator) error { return nil } -func SpecGenToOCI(s *specgen.SpecGenerator, rt *libpod.Runtime, newImage *image.Image) (*spec.Spec, error) { +func SpecGenToOCI(s *specgen.SpecGenerator, rt *libpod.Runtime, rtc *config.Config, newImage *image.Image, mounts []spec.Mount) (*spec.Spec, error) { var ( inUserNS bool ) @@ -173,7 +174,17 @@ func SpecGenToOCI(s *specgen.SpecGenerator, rt *libpod.Runtime, newImage *image. g.AddMount(cgroupMnt) } g.SetProcessCwd(s.WorkDir) - g.SetProcessArgs(s.Command) + if s.Init { + initPath := s.InitPath + if initPath == "" && rtc != nil { + initPath = rtc.Engine.InitPath + } + cmd := []string{initPath, "--"} + cmd = append(cmd, s.Command...) + g.SetProcessArgs(cmd) + } else { + g.SetProcessArgs(s.Command) + } g.SetProcessTerminal(s.Terminal) for key, val := range s.Annotations { @@ -227,7 +238,7 @@ func SpecGenToOCI(s *specgen.SpecGenerator, rt *libpod.Runtime, newImage *image. } // BIND MOUNTS - configSpec.Mounts = SupercedeUserMounts(s.Mounts, configSpec.Mounts) + configSpec.Mounts = SupercedeUserMounts(mounts, configSpec.Mounts) // Process mounts to ensure correct options if err := InitFSMounts(configSpec.Mounts); err != nil { return nil, err diff --git a/pkg/specgen/generate/storage.go b/pkg/specgen/generate/storage.go index 7650e4e9a..241c9adeb 100644 --- a/pkg/specgen/generate/storage.go +++ b/pkg/specgen/generate/storage.go @@ -1,12 +1,20 @@ package generate import ( + "context" + "fmt" + "os" "path" "path/filepath" "strings" + "github.com/containers/common/pkg/config" + "github.com/containers/libpod/libpod" + "github.com/containers/libpod/libpod/image" + "github.com/containers/libpod/pkg/specgen" "github.com/containers/libpod/pkg/util" spec "github.com/opencontainers/runtime-spec/specs-go" + "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -20,6 +28,301 @@ const ( TypeTmpfs = "tmpfs" ) +var ( + errDuplicateDest = errors.Errorf("duplicate mount destination") +) + +// Produce final mounts and named volumes for a container +func finalizeMounts(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Runtime, rtc *config.Config, img *image.Image) ([]spec.Mount, []*specgen.NamedVolume, error) { + // Get image volumes + baseMounts, baseVolumes, err := getImageVolumes(ctx, img, s) + if err != nil { + return nil, nil, err + } + + // Get volumes-from mounts + volFromMounts, volFromVolumes, err := getVolumesFrom(s.VolumesFrom, rt) + if err != nil { + return nil, nil, err + } + + // Supercede from --volumes-from. + for dest, mount := range volFromMounts { + baseMounts[dest] = mount + } + for dest, volume := range volFromVolumes { + baseVolumes[dest] = volume + } + + // Need to make map forms of specgen mounts/volumes. + unifiedMounts := map[string]spec.Mount{} + unifiedVolumes := map[string]*specgen.NamedVolume{} + for _, m := range s.Mounts { + if _, ok := unifiedMounts[m.Destination]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, "conflict in specified mounts - multiple mounts at %q", m.Destination) + } + unifiedMounts[m.Destination] = m + } + for _, v := range s.Volumes { + if _, ok := unifiedVolumes[v.Dest]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, "conflict in specified volumes - multiple volumes at %q", v.Dest) + } + unifiedVolumes[v.Dest] = v + } + + // If requested, add container init binary + if s.Init { + initPath := s.InitPath + if initPath == "" && rtc != nil { + initPath = rtc.Engine.InitPath + } + initMount, err := addContainerInitBinary(s, 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 + } + + // Before superseding, we need to find volume mounts which conflict with + // named volumes, and vice versa. + // We'll delete the conflicts here as we supersede. + for dest := range unifiedMounts { + if _, ok := baseVolumes[dest]; ok { + delete(baseVolumes, dest) + } + } + for dest := range unifiedVolumes { + if _, ok := baseMounts[dest]; ok { + delete(baseMounts, dest) + } + } + + // Supersede 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: Investigate moving readonlyTmpfs into here. Would be more + // correct. + + // Check for conflicts between named volumes and mounts + for dest := range baseMounts { + if _, ok := baseVolumes[dest]; ok { + return nil, nil, errors.Wrapf(errDuplicateDest, "conflict at mount destination %v", dest) + } + } + for dest := range baseVolumes { + if _, ok := baseMounts[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)) + for _, mount := range baseMounts { + if mount.Type == TypeBind { + absSrc, err := filepath.Abs(mount.Source) + if err != nil { + return nil, nil, errors.Wrapf(err, "error getting absolute path of %s", mount.Source) + } + mount.Source = absSrc + } + finalMounts = append(finalMounts, mount) + } + finalVolumes := make([]*specgen.NamedVolume, 0, len(baseVolumes)) + for _, volume := range baseVolumes { + finalVolumes = append(finalVolumes, volume) + } + + return finalMounts, finalVolumes, nil +} + +// Get image volumes from the given image +func getImageVolumes(ctx context.Context, img *image.Image, s *specgen.SpecGenerator) (map[string]spec.Mount, map[string]*specgen.NamedVolume, error) { + mounts := make(map[string]spec.Mount) + volumes := make(map[string]*specgen.NamedVolume) + + mode := strings.ToLower(s.ImageVolumeMode) + + // Image may be nil (rootfs in use), or image volume mode may be ignore. + if img == nil || mode == "ignore" { + return mounts, volumes, nil + } + + inspect, err := img.InspectNoSize(ctx) + if err != nil { + return nil, nil, errors.Wrapf(err, "error inspecting image to get image volumes") + } + for volume := range inspect.Config.Volumes { + logrus.Debugf("Image has volume at %q", volume) + cleanDest := filepath.Clean(volume) + switch mode { + case "", "anonymous": + // Anonymous volumes have no name. + newVol := new(specgen.NamedVolume) + newVol.Dest = cleanDest + newVol.Options = []string{"rprivate", "rw", "nodev", "exec"} + volumes[cleanDest] = newVol + logrus.Debugf("Adding anonymous image volume at %q", cleanDest) + case "tmpfs": + mount := spec.Mount{ + Destination: cleanDest, + Source: TypeTmpfs, + Type: TypeTmpfs, + Options: []string{"rprivate", "rw", "nodev", "exec"}, + } + mounts[cleanDest] = mount + logrus.Debugf("Adding tmpfs image volume at %q", cleanDest) + } + } + + return mounts, volumes, nil +} + +func getVolumesFrom(volumesFrom []string, runtime *libpod.Runtime) (map[string]spec.Mount, map[string]*specgen.NamedVolume, error) { + finalMounts := make(map[string]spec.Mount) + finalNamedVolumes := make(map[string]*specgen.NamedVolume) + + for _, volume := range volumesFrom { + var options []string + + splitVol := strings.SplitN(volume, ":", 2) + if len(splitVol) == 2 { + splitOpts := strings.Split(splitVol[1], ",") + for _, opt := range splitOpts { + setRORW := false + setZ := false + switch opt { + case "z": + if setZ { + return nil, nil, errors.Errorf("cannot set :z more than once in mount options") + } + setZ = true + case "ro", "rw": + if setRORW { + return nil, nil, errors.Errorf("cannot set ro or rw options more than once") + } + setRORW = true + default: + return nil, nil, errors.Errorf("invalid option %q specified - volumes from another container can only use z,ro,rw options", opt) + } + } + options = splitOpts + } + + 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()) + } + + newVol := new(specgen.NamedVolume) + newVol.Dest = namedVol.Dest + newVol.Options = namedVol.Options + newVol.Name = namedVol.Name + + finalNamedVolumes[namedVol.Dest] = newVol + } + + // 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 +} + +// 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. +// This does *NOT* modify the container command - that must be done elsewhere. +func addContainerInitBinary(s *specgen.SpecGenerator, 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 !s.PidNS.IsPrivate() { + return mount, fmt.Errorf("cannot add init binary as PID 1 (PID namespace isn't private)") + } + if s.Systemd == "true" || s.Systemd == "always" { + 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") + } + return mount, nil +} + // Supersede 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 diff --git a/pkg/specgen/specgen.go b/pkg/specgen/specgen.go index 275af1f49..20c8f8800 100644 --- a/pkg/specgen/specgen.go +++ b/pkg/specgen/specgen.go @@ -154,14 +154,23 @@ type ContainerStorageConfig struct { // ImageVolumeMode indicates how image volumes will be created. // Supported modes are "ignore" (do not create), "tmpfs" (create as // tmpfs), and "anonymous" (create as anonymous volumes). - // The default is anonymous. + // The default if unset is anonymous. // Optional. ImageVolumeMode string `json:"image_volume_mode,omitempty"` - // VolumesFrom is a list of containers whose volumes will be added to - // this container. Supported mount options may be added after the - // container name with a : and include "ro" and "rw". - // Optional. + // VolumesFrom is a set of containers whose volumes will be added to + // this container. The name or ID of the container must be provided, and + // may optionally be followed by a : and then one or more + // comma-separated options. Valid options are 'ro', 'rw', and 'z'. + // Options will be used for all volumes sourced from the container. VolumesFrom []string `json:"volumes_from,omitempty"` + // Init specifies that an init binary will be mounted into the + // container, and will be used as PID1. + Init bool `json:"init,omitempty"` + // InitPath specifies the path to the init binary that will be added if + // Init is specified above. If not specified, the default set in the + // Libpod config will be used. Ignored if Init above is not set. + // Optional. + InitPath string `json:"init_path,omitempty"` // Mounts are mounts that will be added to the container. // These will supersede Image Volumes and VolumesFrom volumes where // there are conflicts. diff --git a/test/e2e/run_volume_test.go b/test/e2e/run_volume_test.go index 9da3c1340..1d9538912 100644 --- a/test/e2e/run_volume_test.go +++ b/test/e2e/run_volume_test.go @@ -27,7 +27,6 @@ var _ = Describe("Podman run with volumes", func() { ) BeforeEach(func() { - Skip(v2fail) tempdir, err = CreateTempDirInTempDir() if err != nil { os.Exit(1) @@ -223,6 +222,7 @@ var _ = Describe("Podman run with volumes", func() { }) It("podman run with tmpfs named volume mounts and unmounts", func() { + Skip(v2fail) SkipIfRootless() volName := "testvol" mkVolume := podmanTest.Podman([]string{"volume", "create", "--opt", "type=tmpfs", "--opt", "device=tmpfs", "--opt", "o=nodev", "testvol"}) @@ -279,6 +279,7 @@ var _ = Describe("Podman run with volumes", func() { }) It("podman named volume copyup", func() { + Skip(v2fail) baselineSession := podmanTest.Podman([]string{"run", "--rm", "-t", "-i", ALPINE, "ls", "/etc/apk/"}) baselineSession.WaitWithDefaultTimeout() Expect(baselineSession.ExitCode()).To(Equal(0)) @@ -310,6 +311,7 @@ var _ = Describe("Podman run with volumes", func() { }) It("podman run with anonymous volume", func() { + Skip(v2fail) list1 := podmanTest.Podman([]string{"volume", "list", "--quiet"}) list1.WaitWithDefaultTimeout() Expect(list1.ExitCode()).To(Equal(0)) @@ -328,6 +330,7 @@ var _ = Describe("Podman run with volumes", func() { }) It("podman rm -v removes anonymous volume", func() { + Skip(v2fail) list1 := podmanTest.Podman([]string{"volume", "list", "--quiet"}) list1.WaitWithDefaultTimeout() Expect(list1.ExitCode()).To(Equal(0)) @@ -356,6 +359,7 @@ var _ = Describe("Podman run with volumes", func() { }) It("podman rm -v retains named volume", func() { + Skip(v2fail) list1 := podmanTest.Podman([]string{"volume", "list", "--quiet"}) list1.WaitWithDefaultTimeout() Expect(list1.ExitCode()).To(Equal(0)) @@ -394,6 +398,7 @@ var _ = Describe("Podman run with volumes", func() { }) It("podman mount with invalid option fails", func() { + Skip(v2fail) volName := "testVol" volCreate := podmanTest.Podman([]string{"volume", "create", "--opt", "type=tmpfs", "--opt", "device=tmpfs", "--opt", "o=invalid", volName}) volCreate.WaitWithDefaultTimeout() @@ -405,6 +410,7 @@ var _ = Describe("Podman run with volumes", func() { }) It("Podman fix for CVE-2020-1726", func() { + Skip(v2fail) volName := "testVol" volCreate := podmanTest.Podman([]string{"volume", "create", volName}) volCreate.WaitWithDefaultTimeout() -- cgit v1.2.3-54-g00ecf From 67ec4e1d272d20610f885f91cc322edf57a13f45 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 27 Apr 2020 11:46:28 -0400 Subject: Improve Entrypoint and Command support We should not be overwriting the Specgen's Command and Entrypoint when building the final command to pass in the OCI spec. Both of these will be provided to Libpod for use in `podman inspect` and committing containers, and both must be set to the user's input, not overwritten by the image if unset. Fix this by moving command generation into OCI spec generation and not modifying the SpecGenerator when we do so. Signed-off-by: Matthew Heon --- pkg/specgen/generate/container.go | 25 ------------- pkg/specgen/generate/container_create.go | 2 +- pkg/specgen/generate/oci.go | 64 ++++++++++++++++++++++++++------ 3 files changed, 54 insertions(+), 37 deletions(-) diff --git a/pkg/specgen/generate/container.go b/pkg/specgen/generate/container.go index 669b1f05f..b27dd1cc2 100644 --- a/pkg/specgen/generate/container.go +++ b/pkg/specgen/generate/container.go @@ -8,13 +8,10 @@ import ( envLib "github.com/containers/libpod/pkg/env" "github.com/containers/libpod/pkg/signal" "github.com/containers/libpod/pkg/specgen" - "github.com/pkg/errors" "golang.org/x/sys/unix" ) func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerator) error { - var appendEntryPoint bool - // If a rootfs is used, then there is no image data if s.ContainerStorageConfig.Rootfs != "" { return nil @@ -107,28 +104,6 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat } s.Annotations = annotations - // entrypoint - entrypoint, err := newImage.Entrypoint(ctx) - if err != nil { - return err - } - if len(s.Entrypoint) < 1 && len(entrypoint) > 0 { - appendEntryPoint = true - s.Entrypoint = entrypoint - } - command, err := newImage.Cmd(ctx) - if err != nil { - return err - } - if len(s.Command) < 1 && len(command) > 0 { - if appendEntryPoint { - s.Command = entrypoint - } - s.Command = append(s.Command, command...) - } - if len(s.Command) < 1 && len(s.Entrypoint) < 1 { - return errors.Errorf("No command provided or as CMD or ENTRYPOINT in this image") - } // workdir workingDir, err := newImage.WorkingDir(ctx) if err != nil { diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go index 7de47988d..bb84f0618 100644 --- a/pkg/specgen/generate/container_create.go +++ b/pkg/specgen/generate/container_create.go @@ -108,7 +108,7 @@ func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGener } options = append(options, createExitCommandOption(s, rt.StorageConfig(), rtc, podmanPath)) - runtimeSpec, err := SpecGenToOCI(s, rt, rtc, newImage, finalMounts) + runtimeSpec, err := SpecGenToOCI(ctx, s, rt, rtc, newImage, finalMounts) if err != nil { return nil, err } diff --git a/pkg/specgen/generate/oci.go b/pkg/specgen/generate/oci.go index 6b65ab10a..87262684e 100644 --- a/pkg/specgen/generate/oci.go +++ b/pkg/specgen/generate/oci.go @@ -1,6 +1,7 @@ package generate import ( + "context" "strings" "github.com/containers/common/pkg/config" @@ -11,6 +12,7 @@ import ( "github.com/opencontainers/runc/libcontainer/user" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/opencontainers/runtime-tools/generate" + "github.com/pkg/errors" ) func addRlimits(s *specgen.SpecGenerator, g *generate.Generator) error { @@ -49,7 +51,51 @@ func addRlimits(s *specgen.SpecGenerator, g *generate.Generator) error { return nil } -func SpecGenToOCI(s *specgen.SpecGenerator, rt *libpod.Runtime, rtc *config.Config, newImage *image.Image, mounts []spec.Mount) (*spec.Spec, error) { +// Produce the final command for the container. +func makeCommand(ctx context.Context, s *specgen.SpecGenerator, img *image.Image, rtc *config.Config) ([]string, error) { + finalCommand := []string{} + + entrypoint := s.Entrypoint + if len(entrypoint) == 0 && img != nil { + newEntry, err := img.Entrypoint(ctx) + if err != nil { + return nil, err + } + entrypoint = newEntry + } + + finalCommand = append(finalCommand, entrypoint...) + + command := s.Command + if len(command) == 0 && img != nil { + newCmd, err := img.Cmd(ctx) + if err != nil { + return nil, err + } + command = newCmd + } + + finalCommand = append(finalCommand, command...) + + if len(finalCommand) == 0 { + return nil, errors.Errorf("no command or entrypoint provided, and no CMD or ENTRYPOINT from image") + } + + if s.Init { + initPath := s.InitPath + if initPath == "" && rtc != nil { + initPath = rtc.Engine.InitPath + } + if initPath == "" { + return nil, errors.Errorf("no path to init binary found but container requested an init") + } + finalCommand = append([]string{initPath, "--"}, finalCommand...) + } + + return finalCommand, nil +} + +func SpecGenToOCI(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Runtime, rtc *config.Config, newImage *image.Image, mounts []spec.Mount) (*spec.Spec, error) { var ( inUserNS bool ) @@ -174,17 +220,13 @@ func SpecGenToOCI(s *specgen.SpecGenerator, rt *libpod.Runtime, rtc *config.Conf g.AddMount(cgroupMnt) } g.SetProcessCwd(s.WorkDir) - if s.Init { - initPath := s.InitPath - if initPath == "" && rtc != nil { - initPath = rtc.Engine.InitPath - } - cmd := []string{initPath, "--"} - cmd = append(cmd, s.Command...) - g.SetProcessArgs(cmd) - } else { - g.SetProcessArgs(s.Command) + + finalCmd, err := makeCommand(ctx, s, newImage, rtc) + if err != nil { + return nil, err } + g.SetProcessArgs(finalCmd) + g.SetProcessTerminal(s.Terminal) for key, val := range s.Annotations { -- cgit v1.2.3-54-g00ecf