Skip to content

Commit

Permalink
Only stop chowning volumes once they're not empty
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@mheon
mheon committed May 22, 2024
1 parent f7bd15d commit 046c0e5
Show file tree
Hide file tree
Showing 7 changed files with 152 additions and 22 deletions.
38 changes: 19 additions & 19 deletions docs/source/markdown/podman-volume-inspect.1.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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**

Expand Down
1 change: 1 addition & 0 deletions libpod/container_internal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
33 changes: 31 additions & 2 deletions libpod/container_internal_common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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

Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions libpod/volume.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions libpod/volume_internal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,5 @@ func (v *Volume) refresh() error {
func resetVolumeState(state *VolumeState) {
state.MountCount = 0
state.MountPoint = ""
state.CopiedUp = false
}
93 changes: 93 additions & 0 deletions test/e2e/run_volume_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
})
})
4 changes: 3 additions & 1 deletion test/system/160-volumes.bats
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 046c0e5

Please sign in to comment.