From 6ad6132987f7392e1c1e3f4f4c691f71107830a2 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 16 May 2024 09:36:50 -0400 Subject: [PATCH] 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 --- .../markdown/podman-volume-inspect.1.md | 38 ++++---- libpod/container_internal.go | 1 + libpod/container_internal_common.go | 33 ++++++- libpod/volume.go | 4 + libpod/volume_internal.go | 1 + test/e2e/run_volume_test.go | 93 +++++++++++++++++++ test/system/160-volumes.bats | 4 +- 7 files changed, 152 insertions(+), 22 deletions(-) diff --git a/docs/source/markdown/podman-volume-inspect.1.md b/docs/source/markdown/podman-volume-inspect.1.md index 7e7e831febd9..ed1a109d057e 100644 --- a/docs/source/markdown/podman-volume-inspect.1.md +++ b/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** diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 7f909b78db71..26ac6c985932 100644 --- a/libpod/container_internal.go +++ b/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 } diff --git a/libpod/container_internal_common.go b/libpod/container_internal_common.go index d76dea299580..70f6f741f5de 100644 --- a/libpod/container_internal_common.go +++ b/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 diff --git a/libpod/volume.go b/libpod/volume.go index 4b5a224f8009..05acb2a6e55a 100644 --- a/libpod/volume.go +++ b/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. diff --git a/libpod/volume_internal.go b/libpod/volume_internal.go index 3d308c862060..e51258230574 100644 --- a/libpod/volume_internal.go +++ b/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 } diff --git a/test/e2e/run_volume_test.go b/test/e2e/run_volume_test.go index df6fd368f869..c9cf65816330 100644 --- a/test/e2e/run_volume_test.go +++ b/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) + }) }) diff --git a/test/system/160-volumes.bats b/test/system/160-volumes.bats index ad64d719360f..2323c33a7da0 100644 --- a/test/system/160-volumes.bats +++ b/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"