summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMatthew Heon <mheon@redhat.com>2021-02-10 09:46:12 -0500
committerMatthew Heon <mheon@redhat.com>2021-02-10 14:21:37 -0500
commitea910fc53537d8c6f8cd1e1eaad49eaa5906c5f5 (patch)
treeeb8c9c48634c82617b83ac09b316c0ed57fc64e9
parent763d522983b819ecd38689c9c0840069d1e2b530 (diff)
downloadpodman-ea910fc53537d8c6f8cd1e1eaad49eaa5906c5f5.tar.gz
podman-ea910fc53537d8c6f8cd1e1eaad49eaa5906c5f5.tar.bz2
podman-ea910fc53537d8c6f8cd1e1eaad49eaa5906c5f5.zip
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 <mheon@redhat.com>
-rw-r--r--libpod/container_internal.go75
-rw-r--r--libpod/container_internal_linux.go17
-rw-r--r--libpod/volume.go11
-rw-r--r--test/e2e/run_volume_test.go18
4 files changed, 78 insertions, 43 deletions
diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index b280e79d1..9ea9e02cd 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 3583f8fdd..1c4537a27 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 4c137cb8e..5cc5e7e40 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 bc89b59de..19d82c974 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()