Merge pull request #14734 from giuseppe/copyup-switch-order

volume: add two new options copy and nocopy

docs/source/markdown/podman-create.1.md

@@ -1295,13 +1295,14 @@ The _options_ is a comma-separated list and can be:
 
 * **rw**|**ro**
 * **z**|**Z**
-* [**r**]**shared**|[**r**]**slave**|[**r**]**private**[**r**]**unbindable**
-* [**r**]**bind**
-* [**no**]**exec**
-* [**no**]**dev**
-* [**no**]**suid**
 * [**O**]
 * [**U**]
+* [**no**]**copy**
+* [**no**]**dev**
+* [**no**]**exec**
+* [**no**]**suid**
+* [**r**]**bind**
+* [**r**]**shared**|[**r**]**slave**|[**r**]**private**[**r**]**unbindable**
 
 The `CONTAINER-DIR` must be an absolute path such as `/src/docs`. The volume
 will be mounted into the container at this directory.

docs/source/markdown/podman-run.1.md

@@ -1362,13 +1362,14 @@ The _options_ is a comma-separated list and can be: <sup>[[1]](#Footnote1)</sup>
 
 * **rw**|**ro**
 * **z**|**Z**
-* [**r**]**shared**|[**r**]**slave**|[**r**]**private**[**r**]**unbindable**
-* [**r**]**bind**
-* [**no**]**exec**
-* [**no**]**dev**
-* [**no**]**suid**
 * [**O**]
 * [**U**]
+* [**no**]**copy**
+* [**no**]**dev**
+* [**no**]**exec**
+* [**no**]**suid**
+* [**r**]**bind**
+* [**r**]**shared**|[**r**]**slave**|[**r**]**private**[**r**]**unbindable**
 
 The `CONTAINER-DIR` must be an absolute path such as `/src/docs`. The volume
 will be mounted into the container at this directory.

docs/source/markdown/podman-volume-create.1.md

@@ -31,9 +31,10 @@ Set metadata for a volume (e.g., --label mykey=value).
 
 Set driver specific options.
 For the default driver, **local**, this allows a volume to be configured to mount a filesystem on the host.
-For the `local` driver the following options are supported: `type`, `device`, and `o`.
+For the `local` driver the following options are supported: `type`, `device`, `o`, and `[no]copy`.
 The `type` option sets the type of the filesystem to be mounted, and is equivalent to the `-t` flag to **mount(8)**.
 The `device` option sets the device to be mounted, and is equivalent to the `device` argument to **mount(8)**.
+The `copy` option enables copying files from the container image path where the mount is created to the newly created volume on the first run.  `copy` is the default.
 
 The `o` option sets options for the mount, and is equivalent to the `-o` flag to **mount(8)** with these exceptions:
 

go.mod

@@ -12,7 +12,7 @@ require (
 	github.com/containernetworking/cni v1.1.1
 	github.com/containernetworking/plugins v1.1.1
 	github.com/containers/buildah v1.26.1-0.20220609225314-e66309ebde8c
-	github.com/containers/common v0.48.1-0.20220624132904-722a80e139ec
+	github.com/containers/common v0.48.1-0.20220627112538-97d9656daba8
 	github.com/containers/conmon v2.0.20+incompatible
 	github.com/containers/image/v5 v5.21.2-0.20220617075545-929f14a56f5c
 	github.com/containers/ocicrypt v1.1.5

go.sum

@@ -338,8 +338,8 @@ github.com/containernetworking/plugins v1.1.1/go.mod h1:Sr5TH/eBsGLXK/h71HeLfX19
 github.com/containers/buildah v1.26.1-0.20220609225314-e66309ebde8c h1:/fKyiLFFuceBPZGJ0Lig7ElURhfsslAOw1BOcItD+X8=
 github.com/containers/buildah v1.26.1-0.20220609225314-e66309ebde8c/go.mod h1:b0L+u2Dam7soWGn5sVTK31L++Xrf80AbGvK5z9D2+lw=
 github.com/containers/common v0.48.1-0.20220608111710-dbecabbe82c9/go.mod h1:WBLwq+i7bicCpH54V70HM6s7jqDAESTlYnd05XXp0ac=
-github.com/containers/common v0.48.1-0.20220624132904-722a80e139ec h1:kcfKciMCi6/A72M8TSX2ZYJFa5Yu9hC2Mct8FkuW1Xw=
-github.com/containers/common v0.48.1-0.20220624132904-722a80e139ec/go.mod h1:KDNk8Lazjjjqs9643cKH9dnq6IXB4Lf7evya5GiFcg0=
+github.com/containers/common v0.48.1-0.20220627112538-97d9656daba8 h1:d9CnUqml4SeEWGfQ781UR4quow9xdf7Q0hYqBYFRH4E=
+github.com/containers/common v0.48.1-0.20220627112538-97d9656daba8/go.mod h1:UDe7OTpNdtJA2T80Sp7yB0yTaj79f4kMNQbTsNxsqoY=
 github.com/containers/conmon v2.0.20+incompatible h1:YbCVSFSCqFjjVwHTPINGdMX1F6JXHGTUje2ZYobNrkg=
 github.com/containers/conmon v2.0.20+incompatible/go.mod h1:hgwZ2mtuDrppv78a/cOBNiCm6O0UMWGx1mu7P00nu5I=
 github.com/containers/image/v5 v5.21.2-0.20220511203756-fe4fd4ed8be4/go.mod h1:OsX9sFexyGF0FCNAjfcVFv3IwMqDyLyV/WQY/roLPcE=
@@ -1268,7 +1268,6 @@ github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/stretchr/testify v1.7.2/go.mod h1:R6va5+xMeoiuVRoj+gSkQ7d3FALtqAAGI1FQKckRals=
-github.com/stretchr/testify v1.7.4/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 github.com/stretchr/testify v1.7.5 h1:s5PTfem8p8EbKQOctVV53k6jCJt3UX4IEJzwh+C324Q=
 github.com/stretchr/testify v1.7.5/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw=

libpod/container_internal.go

@@ -21,6 +21,7 @@ import (
 	"github.com/containers/common/pkg/cgroups"
 	"github.com/containers/common/pkg/chown"
 	"github.com/containers/common/pkg/config"
+	cutil "github.com/containers/common/pkg/util"
 	"github.com/containers/podman/v4/libpod/define"
 	"github.com/containers/podman/v4/libpod/events"
 	"github.com/containers/podman/v4/pkg/ctime"
@@ -1639,30 +1640,16 @@ func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string)
 	if err := vol.update(); err != nil {
 		return nil, err
 	}
-	if vol.state.NeedsCopyUp {
+	_, hasNoCopy := vol.config.Options["nocopy"]
+	if vol.state.NeedsCopyUp && !cutil.StringInSlice("nocopy", v.Options) && !hasNoCopy {
 		logrus.Debugf("Copying up contents from container %s to volume %s", c.ID(), vol.Name())
 
-		// If the volume is not empty, we should not copy up.
-		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())
-		}
-		if len(contents) > 0 {
-			// The volume is not empty. It was likely modified
-			// outside of Podman. For safety, let's not copy up into
-			// it. Fixes CVE-2020-1726.
-			return vol, nil
-		}
-
 		srcDir, err := securejoin.SecureJoin(mountpoint, v.Dest)
 		if err != nil {
 			return nil, errors.Wrapf(err, "error calculating destination path to copy up container %s 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) {
@@ -1688,6 +1675,19 @@ func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string)
 			return vol, nil
 		}
 
+		// If the volume is not empty, we should not copy up.
+		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())
+		}
+		if len(contents) > 0 {
+			// The volume is not empty. It was likely modified
+			// outside of Podman. For safety, let's not copy up into
+			// it. Fixes CVE-2020-1726.
+			return vol, nil
+		}
+
 		// Set NeedsCopyUp to false since we are about to do first copy
 		// Do not copy second time.
 		vol.state.NeedsCopyUp = false

libpod/runtime_volume_linux.go

@@ -76,7 +76,7 @@ func (r *Runtime) newVolume(noCreatePluginVolume bool, options ...VolumeCreateOp
 						return nil, errors.Wrapf(err, "invalid volume option %s for driver 'local'", key)
 					}
 				}
-			case "o", "type", "uid", "gid", "size", "inodes", "noquota":
+			case "o", "type", "uid", "gid", "size", "inodes", "noquota", "copy", "nocopy":
 				// Do nothing, valid keys
 			default:
 				return nil, errors.Wrapf(define.ErrInvalidArg, "invalid mount option %s for driver 'local'", key)

libpod/volume_internal.go

@@ -55,6 +55,12 @@ func (v *Volume) needsMount() bool {
 	if _, ok := v.config.Options["NOQUOTA"]; ok {
 		index++
 	}
+	if _, ok := v.config.Options["nocopy"]; ok {
+		index++
+	}
+	if _, ok := v.config.Options["copy"]; ok {
+		index++
+	}
 	// when uid or gid is set there is also the "o" option
 	// set so we have to ignore this one as well
 	if index > 0 {

pkg/util/mountOpts.go

@@ -25,7 +25,7 @@ type defaultMountOptions struct {
 // The sourcePath variable, if not empty, contains a bind mount source.
 func ProcessOptions(options []string, isTmpfs bool, sourcePath string) ([]string, error) {
 	var (
-		foundWrite, foundSize, foundProp, foundMode, foundExec, foundSuid, foundDev, foundCopyUp, foundBind, foundZ, foundU, foundOverlay, foundIdmap bool
+		foundWrite, foundSize, foundProp, foundMode, foundExec, foundSuid, foundDev, foundCopyUp, foundBind, foundZ, foundU, foundOverlay, foundIdmap, foundCopy bool
 	)
 
 	newOptions := make([]string, 0, len(options))
@@ -55,6 +55,11 @@ func ProcessOptions(options []string, isTmpfs bool, sourcePath string) ([]string
 		}
 
 		switch splitOpt[0] {
+		case "copy", "nocopy":
+			if foundCopy {
+				return nil, errors.Wrapf(ErrDupeMntOption, "only one of 'nocopy' and 'copy' can be used")
+			}
+			foundCopy = true
 		case "O":
 			foundOverlay = true
 		case "volume-opt":

test/e2e/run_volume_test.go

@@ -452,6 +452,14 @@ var _ = Describe("Podman run with volumes", func() {
 		separateVolumeSession.WaitWithDefaultTimeout()
 		Expect(separateVolumeSession).Should(Exit(0))
 		Expect(separateVolumeSession.OutputToString()).To(Equal(baselineOutput))
+
+		copySession := podmanTest.Podman([]string{"run", "--rm", "-v", "testvol3:/etc/apk:copy", ALPINE, "stat", "-c", "%h", "/etc/apk/arch"})
+		copySession.WaitWithDefaultTimeout()
+		Expect(copySession).Should(Exit(0))
+
+		noCopySession := podmanTest.Podman([]string{"run", "--rm", "-v", "testvol4:/etc/apk:nocopy", ALPINE, "stat", "-c", "%h", "/etc/apk/arch"})
+		noCopySession.WaitWithDefaultTimeout()
+		Expect(noCopySession).Should(Exit(1))
 	})
 
 	It("podman named volume copyup symlink", func() {

vendor/github.com/containers/common/pkg/parse/parse.go

@@ -14,7 +14,7 @@ import (
 
 // ValidateVolumeOpts validates a volume's options
 func ValidateVolumeOpts(options []string) ([]string, error) {
-	var foundRootPropagation, foundRWRO, foundLabelChange, bindType, foundExec, foundDev, foundSuid, foundChown, foundUpperDir, foundWorkDir int
+	var foundRootPropagation, foundRWRO, foundLabelChange, bindType, foundExec, foundDev, foundSuid, foundChown, foundUpperDir, foundWorkDir, foundCopy int
 	finalOpts := make([]string, 0, len(options))
 	for _, opt := range options {
 		// support advanced options like upperdir=/path, workdir=/path
@@ -88,6 +88,11 @@ func ValidateVolumeOpts(options []string) ([]string, error) {
 			// are intended to be always safe to use, even not on OS
 			// X).
 			continue
+		case "copy", "nocopy":
+			foundCopy++
+			if foundCopy > 1 {
+				return nil, errors.Errorf("invalid options %q, can only specify 1 'copy' or 'nocopy' option", strings.Join(options, ", "))
+			}
 		default:
 			return nil, errors.Errorf("invalid option type %q", opt)
 		}

vendor/modules.txt

@@ -111,7 +111,7 @@ github.com/containers/buildah/pkg/rusage
 github.com/containers/buildah/pkg/sshagent
 github.com/containers/buildah/pkg/util
 github.com/containers/buildah/util
-# github.com/containers/common v0.48.1-0.20220624132904-722a80e139ec
+# github.com/containers/common v0.48.1-0.20220627112538-97d9656daba8
 ## explicit
 github.com/containers/common/libimage
 github.com/containers/common/libimage/define