Merge pull request containers#22727 from mheon/chown_all_the_time

Always chown volumes when mounting into a container

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

@@ -1901,6 +1901,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"