From ea910fc53537d8c6f8cd1e1eaad49eaa5906c5f5 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 10 Feb 2021 09:46:12 -0500 Subject: [PATCH] Rewrite copy-up to use buildah Copier The old copy-up implementation was very unhappy with symlinks, which could cause containers to fail to start for unclear reasons when a directory we wanted to copy-up contained one. Rewrite to use the Buildah Copier, which is more recent and should be both safer and less likely to blow up over links. At the same time, fix a deadlock in copy-up for volumes requiring mounting - the Mountpoint() function tried to take the already-acquired volume lock. Fixes #6003 Signed-off-by: Matthew Heon --- libpod/container_internal.go | 75 ++++++++++++++++++++---------- libpod/container_internal_linux.go | 17 ------- libpod/volume.go | 11 ++++- test/e2e/run_volume_test.go | 18 +++++++ 4 files changed, 78 insertions(+), 43 deletions(-) diff --git a/libpod/container_internal.go b/libpod/container_internal.go index b280e79d1e..9ea9e02cd4 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "github.com/containers/buildah/copier" "github.com/containers/common/pkg/secrets" "github.com/containers/podman/v2/libpod/define" "github.com/containers/podman/v2/libpod/events" @@ -1582,18 +1583,8 @@ func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string) return nil, err } - // HACK HACK HACK - copy up into a volume driver is 100% broken - // right now. - if vol.UsesVolumeDriver() { - logrus.Infof("Not copying up into volume %s as it uses a volume driver", vol.Name()) - return vol, nil - } - // If the volume is not empty, we should not copy up. - volMount, err := vol.MountPoint() - if err != nil { - return nil, err - } + volMount := vol.mountPoint() contents, err := ioutil.ReadDir(volMount) if err != nil { return nil, errors.Wrapf(err, "error listing contents of volume %s mountpoint when copying up from container %s", vol.Name(), c.ID()) @@ -1609,8 +1600,55 @@ func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string) if err != nil { return nil, errors.Wrapf(err, "error calculating destination path to copy up container %s volume %s", c.ID(), vol.Name()) } - if err := c.copyWithTarFromImage(srcDir, volMount); err != nil && !os.IsNotExist(err) { - return nil, errors.Wrapf(err, "error copying content from container %s into volume %s", c.ID(), vol.Name()) + // Do a manual stat on the source directory to verify existence. + // Skip the rest if it exists. + // TODO: Should this be stat or lstat? I'm using lstat because I + // think copy-up doesn't happen when the source is a link. + srcStat, err := os.Lstat(srcDir) + if err != nil { + if os.IsNotExist(err) { + // Source does not exist, don't bother copying + // up. + return vol, nil + } + return nil, errors.Wrapf(err, "error identifying source directory for copy up into volume %s", vol.Name()) + } + // If it's not a directory we're mounting over it. + if !srcStat.IsDir() { + return vol, nil + } + + // Buildah Copier accepts a reader, so we'll need a pipe. + reader, writer := io.Pipe() + defer reader.Close() + + errChan := make(chan error, 1) + + logrus.Infof("About to copy up into volume %s", vol.Name()) + + // Copy, container side: get a tar archive of what needs to be + // streamed into the volume. + go func() { + defer writer.Close() + getOptions := copier.GetOptions{ + KeepDirectoryNames: false, + } + errChan <- copier.Get(mountpoint, "", getOptions, []string{v.Dest + "/."}, writer) + }() + + // Copy, volume side: stream what we've written to the pipe, into + // the volume. + copyOpts := copier.PutOptions{} + if err := copier.Put(volMount, "", copyOpts, reader); err != nil { + err2 := <-errChan + if err2 != nil { + logrus.Errorf("Error streaming contents of container %s directory for volume copy-up: %v", c.ID(), err2) + } + return nil, errors.Wrapf(err, "error copying up to volume %s", vol.Name()) + } + + if err := <-errChan; err != nil { + return nil, errors.Wrapf(err, "error streaming container content for copy up into volume %s", vol.Name()) } } return vol, nil @@ -2060,17 +2098,6 @@ func (c *Container) unmount(force bool) error { return nil } -// this should be from chrootarchive. -// Container MUST be mounted before calling. -func (c *Container) copyWithTarFromImage(source, dest string) error { - mappings := idtools.NewIDMappingsFromMaps(c.config.IDMappings.UIDMap, c.config.IDMappings.GIDMap) - a := archive.NewArchiver(mappings) - if err := c.copyOwnerAndPerms(source, dest); err != nil { - return err - } - return a.CopyWithTar(source, dest) -} - // checkReadyForRemoval checks whether the given container is ready to be // removed. // These checks are only used if force-remove is not specified. diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index 3583f8fddb..1c4537a27d 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -2278,23 +2278,6 @@ func (c *Container) generatePasswdAndGroup() (string, string, error) { return passwdPath, groupPath, nil } -func (c *Container) copyOwnerAndPerms(source, dest string) error { - info, err := os.Stat(source) - if err != nil { - if os.IsNotExist(err) { - return nil - } - return err - } - if err := os.Chmod(dest, info.Mode()); err != nil { - return err - } - if err := os.Chown(dest, int(info.Sys().(*syscall.Stat_t).Uid), int(info.Sys().(*syscall.Stat_t).Gid)); err != nil { - return err - } - return nil -} - // Get cgroup path in a format suitable for the OCI spec func (c *Container) getOCICgroupPath() (string, error) { unified, err := cgroups.IsCgroup2UnifiedMode() diff --git a/libpod/volume.go b/libpod/volume.go index 4c137cb8e9..5cc5e7e40c 100644 --- a/libpod/volume.go +++ b/libpod/volume.go @@ -130,11 +130,18 @@ func (v *Volume) MountPoint() (string, error) { if err := v.update(); err != nil { return "", err } + } + + return v.mountPoint(), nil +} - return v.state.MountPoint, nil +// Internal-only helper for volume mountpoint +func (v *Volume) mountPoint() string { + if v.UsesVolumeDriver() { + return v.state.MountPoint } - return v.config.MountPoint, nil + return v.config.MountPoint } // Options return the volume's options diff --git a/test/e2e/run_volume_test.go b/test/e2e/run_volume_test.go index bc89b59ded..19d82c9749 100644 --- a/test/e2e/run_volume_test.go +++ b/test/e2e/run_volume_test.go @@ -304,6 +304,24 @@ var _ = Describe("Podman run with volumes", func() { Expect(separateVolumeSession.OutputToString()).To(Equal(baselineOutput)) }) + It("podman named volume copyup symlink", func() { + imgName := "testimg" + dockerfile := `FROM alpine +RUN touch /testfile +RUN sh -c "cd /etc/apk && ln -s ../../testfile"` + podmanTest.BuildImage(dockerfile, imgName, "false") + + baselineSession := podmanTest.Podman([]string{"run", "--rm", "-t", "-i", imgName, "ls", "/etc/apk/"}) + baselineSession.WaitWithDefaultTimeout() + Expect(baselineSession.ExitCode()).To(Equal(0)) + baselineOutput := baselineSession.OutputToString() + + outputSession := podmanTest.Podman([]string{"run", "-t", "-i", "-v", "/etc/apk/", imgName, "ls", "/etc/apk/"}) + outputSession.WaitWithDefaultTimeout() + Expect(outputSession.ExitCode()).To(Equal(0)) + Expect(outputSession.OutputToString()).To(Equal(baselineOutput)) + }) + It("podman read-only tmpfs conflict with volume", func() { session := podmanTest.Podman([]string{"run", "--rm", "-t", "-i", "--read-only", "-v", "tmp_volume:" + dest, ALPINE, "touch", dest + "/a"}) session.WaitWithDefaultTimeout()