Only stop chowning volumes once they're not empty

When an empty volume is mounted into a container, Docker will
chown that volume appropriately for use in the container. Podman
does this as well, but there are differences in the details. In
Podman, a chown is presently a one-and-done deal; in Docker, it
will continue so long as the volume remains empty. Mount into a
dozen containers, but never add content, the chown occurs every
time. The chown is also linked to copy-up; it will always occur
when a copy-up occurred, despite the volume now not being empty.
This PR changes our logic to (mostly) match Docker's.

For some reason, the chowning also stops if the volume is chowned
to root at any point. This feels like a Docker bug, but as they
say, bug for bug compatible.

In retrospect, using bools for NeedsChown and NeedsCopyUp was a
mistake. Docker isn't actually tracking this stuff; they're just
doing a copy-up and permissions change unconditionally as long as
the volume is empty. They also have the two linked as one
operation, seemingly, despite happening at very different times
during container init. Replicating that in our stateful system is
nontrivial, hence the need for the new CopiedUp field. Basically,
we never want to chown a volume with contents in it, except if
that data is a result of a copy-up that resulted from mounting
into the current container. Tracking who did the copy-up is the
easiest way to do this.

Fixes #22571

Signed-off-by: Matthew Heon <matthew.heon@pm.me>

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

@@ -26,25 +26,25 @@ Format volume output using Go template
 
 Valid placeholders for the Go template are listed below:
 
-| **Placeholder**     | **Description**                                        |
-| ------------------- | ------------------------------------------------------ |
-| .Anonymous          | Indicates whether volume is anonymous                  |
-| .CreatedAt ...      | Volume creation time                                   |
-| .Driver             | Volume driver                                          |
-| .GID                | GID the volume was created with                        |
-| .Labels ...         | Label information associated with the volume           |
-| .LockNumber         | Number of the volume's Libpod lock                     |
-| .MountCount         | Number of times the volume is mounted                  |
-| .Mountpoint         | Source of volume mount point                           |
-| .Name               | Volume name                                            |
-| .NeedsChown         | Indicates volume needs to be chowned on first use      |
-| .NeedsCopyUp        | Indicates volume needs dest data copied up on first use|
-| .Options ...        | Volume options                                         |
-| .Scope              | Volume scope                                           |
-| .Status ...         | Status of the volume                                   |
-| .StorageID          | StorageID of the volume                                |
-| .Timeout            | Timeout of the volume                                  |
-| .UID                | UID the volume was created with                        |
+| **Placeholder**     | **Description**                                                             |
+| ------------------- | --------------------------------------------------------------------------- |
+| .Anonymous          | Indicates whether volume is anonymous                                       |
+| .CreatedAt ...      | Volume creation time                                                        |
+| .Driver             | Volume driver                                                               |
+| .GID                | GID the volume was created with                                             |
+| .Labels ...         | Label information associated with the volume                                |
+| .LockNumber         | Number of the volume's Libpod lock                                          |
+| .MountCount         | Number of times the volume is mounted                                       |
+| .Mountpoint         | Source of volume mount point                                                |
+| .Name               | Volume name                                                                 |
+| .NeedsChown         | Indicates volume will be chowned on next use                                |
+| .NeedsCopyUp        | Indicates data at the destination will be copied into the volume on next use|
+| .Options ...        | Volume options                                                              |
+| .Scope              | Volume scope                                                                |
+| .Status ...         | Status of the volume                                                        |
+| .StorageID          | StorageID of the volume                                                     |
+| .Timeout            | Timeout of the volume                                                       |
+| .UID                | UID the volume was created with                                             |
 
 #### **--help**
 

libpod/container_internal.go

@@ -1887,6 +1887,7 @@ func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string)
 		// Set NeedsCopyUp to false since we are about to do first copy
 		// Do not copy second time.
 		vol.state.NeedsCopyUp = false
+		vol.state.CopiedUp = true
 		if err := vol.save(); err != nil {
 			return nil, err
 		}

libpod/container_internal_common.go

@@ -2868,11 +2868,26 @@ func (c *Container) fixVolumePermissions(v *ContainerNamedVolume) error {
 		return err
 	}
 
+	// If the volume is not empty, and it is not the first copy-up event -
+	// we should not do a chown.
+	if vol.state.NeedsChown && !vol.state.CopiedUp {
+		contents, err := os.ReadDir(vol.mountPoint())
+		if err != nil {
+			return fmt.Errorf("reading contents of volume %q: %w", vol.Name(), err)
+		}
+		// Not empty, do nothing and unset NeedsChown.
+		if len(contents) > 0 {
+			vol.state.NeedsChown = false
+			if err := vol.save(); err != nil {
+				return fmt.Errorf("saving volume %q state: %w", vol.Name(), err)
+			}
+			return nil
+		}
+	}
+
 	// Volumes owned by a volume driver are not chowned - we don't want to
 	// mess with a mount not managed by us.
 	if vol.state.NeedsChown && (!vol.UsesVolumeDriver() && vol.config.Driver != "image") {
-		vol.state.NeedsChown = false
-
 		uid := int(c.config.Spec.Process.User.UID)
 		gid := int(c.config.Spec.Process.User.GID)
 
@@ -2891,6 +2906,10 @@ func (c *Container) fixVolumePermissions(v *ContainerNamedVolume) error {
 			gid = newPair.GID
 		}
 
+		if vol.state.CopiedUp {
+			vol.state.NeedsChown = false
+		}
+		vol.state.CopiedUp = false
 		vol.state.UIDChowned = uid
 		vol.state.GIDChowned = gid
 
@@ -2935,6 +2954,16 @@ func (c *Container) fixVolumePermissions(v *ContainerNamedVolume) error {
 				if err := idtools.SafeLchown(mountPoint, uid, gid); err != nil {
 					return err
 				}
+
+				// UID/GID 0 are sticky - if we chown to root,
+				// we stop chowning thereafter.
+				if uid == 0 && gid == 0 && vol.state.NeedsChown {
+					vol.state.NeedsChown = false
+
+					if err := vol.save(); err != nil {
+						return fmt.Errorf("saving volume %q state to database: %w", vol.Name(), err)
+					}
+				}
 			}
 			if err := os.Chmod(mountPoint, st.Mode()); err != nil {
 				return err

libpod/volume.go

@@ -98,6 +98,10 @@ type VolumeState struct {
 	// a container, the container will chown the volume to the container process
 	// UID/GID.
 	NeedsChown bool `json:"notYetChowned,omitempty"`
+	// Indicates that a copy-up event occurred during the current mount of
+	// the volume into a container.
+	// We use this to determine if a chown is appropriate.
+	CopiedUp bool `json:"copiedUp,omitempty"`
 	// UIDChowned is the UID the volume was chowned to.
 	UIDChowned int `json:"uidChowned,omitempty"`
 	// GIDChowned is the GID the volume was chowned to.

libpod/volume_internal.go

@@ -110,4 +110,5 @@ func (v *Volume) refresh() error {
 func resetVolumeState(state *VolumeState) {
 	state.MountCount = 0
 	state.MountPoint = ""
+	state.CopiedUp = false
 }

test/e2e/run_volume_test.go

@@ -30,6 +30,14 @@ var _ = Describe("Podman run with volumes", func() {
 		return strings.Fields(session.OutputToString())
 	}
 
+	//nolint:unparam
+	mountVolumeAndCheckDirectory := func(volName, volPath, expectedOwner, imgName string) {
+		check := podmanTest.Podman([]string{"run", "-v", fmt.Sprintf("%s:%s", volName, volPath), imgName, "stat", "-c", "%U:%G", volPath})
+		check.WaitWithDefaultTimeout()
+		Expect(check).Should(ExitCleanly())
+		Expect(check.OutputToString()).Should(ContainSubstring(fmt.Sprintf("%s:%s", expectedOwner, expectedOwner)))
+	}
+
 	It("podman run with volume flag", func() {
 		mountPath := filepath.Join(podmanTest.TempDir, "secrets")
 		err = os.Mkdir(mountPath, 0755)
@@ -970,4 +978,89 @@ USER testuser`, CITEST_IMAGE)
 
 		Expect(run1.OutputToString()).Should(Equal(run2.OutputToString()))
 	})
+
+	It("podman run -v chowns multiple times on empty volume", func() {
+		imgName := "testimg"
+		dockerfile := fmt.Sprintf(`FROM %s
+RUN addgroup -g 1234 test1
+RUN addgroup -g 4567 test2
+RUN addgroup -g 7890 test3
+RUN adduser -D -u 1234 -G test1 test1
+RUN adduser -D -u 4567 -G test2 test2
+RUN adduser -D -u 7890 -G test3 test3
+RUN mkdir /test1 /test2 /test3 /test4
+RUN chown test1:test1 /test1
+RUN chown test2:test2 /test2
+RUN chown test3:test3 /test4
+RUN chmod 755 /test1 /test2 /test3 /test4`, ALPINE)
+		podmanTest.BuildImage(dockerfile, imgName, "false")
+
+		volName := "testVol"
+		volCreate := podmanTest.Podman([]string{"volume", "create", volName})
+		volCreate.WaitWithDefaultTimeout()
+		Expect(volCreate).Should(ExitCleanly())
+
+		mountVolumeAndCheckDirectory(volName, "/test1", "test1", imgName)
+		mountVolumeAndCheckDirectory(volName, "/test2", "test2", imgName)
+		mountVolumeAndCheckDirectory(volName, "/test3", "root", imgName)
+		mountVolumeAndCheckDirectory(volName, "/test4", "root", imgName)
+	})
+
+	It("podman run -v chowns until copy-up on volume", func() {
+		imgName := "testimg"
+		dockerfile := fmt.Sprintf(`FROM %s
+RUN addgroup -g 1234 test1
+RUN addgroup -g 4567 test2
+RUN addgroup -g 7890 test3
+RUN adduser -D -u 1234 -G test1 test1
+RUN adduser -D -u 4567 -G test2 test2
+RUN adduser -D -u 7890 -G test3 test3
+RUN mkdir /test1 /test2 /test3
+RUN touch /test2/file1
+RUN chown test1:test1 /test1
+RUN chown -R test2:test2 /test2
+RUN chown test3:test3 /test3
+RUN chmod 755 /test1 /test2 /test3`, ALPINE)
+		podmanTest.BuildImage(dockerfile, imgName, "false")
+
+		volName := "testVol"
+		volCreate := podmanTest.Podman([]string{"volume", "create", volName})
+		volCreate.WaitWithDefaultTimeout()
+		Expect(volCreate).Should(ExitCleanly())
+
+		mountVolumeAndCheckDirectory(volName, "/test1", "test1", imgName)
+		mountVolumeAndCheckDirectory(volName, "/test2", "test2", imgName)
+		mountVolumeAndCheckDirectory(volName, "/test3", "test2", imgName)
+	})
+
+	It("podman run -v chowns until volume has contents", func() {
+		imgName := "testimg"
+		dockerfile := fmt.Sprintf(`FROM %s
+RUN addgroup -g 1234 test1
+RUN addgroup -g 4567 test2
+RUN addgroup -g 7890 test3
+RUN adduser -D -u 1234 -G test1 test1
+RUN adduser -D -u 4567 -G test2 test2
+RUN adduser -D -u 7890 -G test3 test3
+RUN mkdir /test1 /test2 /test3
+RUN chown test1:test1 /test1
+RUN chown test2:test2 /test2
+RUN chown test3:test3 /test3
+RUN chmod 755 /test1 /test2 /test3`, ALPINE)
+		podmanTest.BuildImage(dockerfile, imgName, "false")
+
+		volName := "testVol"
+		volCreate := podmanTest.Podman([]string{"volume", "create", volName})
+		volCreate.WaitWithDefaultTimeout()
+		Expect(volCreate).Should(ExitCleanly())
+
+		mountVolumeAndCheckDirectory(volName, "/test1", "test1", imgName)
+		mountVolumeAndCheckDirectory(volName, "/test2", "test2", imgName)
+
+		session := podmanTest.Podman([]string{"run", "-v", fmt.Sprintf("%s:/test2", volName), imgName, "touch", "/test2/file1"})
+		session.WaitWithDefaultTimeout()
+		Expect(session).To(ExitCleanly())
+
+		mountVolumeAndCheckDirectory(volName, "/test3", "test2", imgName)
+	})
 })

test/system/160-volumes.bats

@@ -467,11 +467,13 @@ NeedsChown    | true
     run_podman volume inspect --format '{{ .NeedsCopyUp }}' $myvolume
     is "${output}" "true" "If content in dest '/vol' empty NeedsCopyUP should still be true"
     run_podman volume inspect --format '{{ .NeedsChown }}' $myvolume
-    is "${output}" "false" "After first use within a container NeedsChown should still be false"
+    is "${output}" "true" "No copy up occurred so the NeedsChown will still be true"
 
     run_podman run --rm --volume $myvolume:/etc $IMAGE ls /etc/passwd
     run_podman volume inspect --format '{{ .NeedsCopyUp }}' $myvolume
     is "${output}" "false" "If content in dest '/etc' non-empty NeedsCopyUP should still have happened and be false"
+    run_podman volume inspect --format '{{ .NeedsChown }}' $myvolume
+    is "${output}" "false" "Content has been copied up into volume, needschown will be false"
 
     run_podman volume inspect --format '{{.Mountpoint}}' $myvolume
     mountpoint="$output"