volume NeedsCopyUp breaking nfs volumes
#14722
Comments
|
does it work for you if we exchange the order of the checks? diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index ce48987f6..c11164ce3 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -1666,19 +1666,6 @@ func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string)
if vol.state.NeedsCopyUp {
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())
@@ -1712,6 +1699,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 |
|
thanks for the quick reply. I'm not sure exactly. I'm not familiar with the code. But looking at your snippet (and the original file). It seems the error would still occur. Looking at the code path, the securejoin.SecureJoin(mountpoint, v.Dest) (I'm not sure what that is) error would be triggered first, which would result in an error, or then the same I think its a matter of permissions. Whatever uid context this code is running is doesn't have permission to |
|
but I thought we would first hit the |
|
Oh yes. Sorry, I misread the patch. Assuming:
If srcDir is checked to be empty and we return before attempting to list That said, I don't know what the behavior should be when |
|
I also find this behavior strange but I think that is done for compatibility with Docker. Maybe we could add a new option for the volume that prevents the copy. |
|
Adding an option to prevent copy-up SGTM. This seems like an uncommon case (a volume explicitly not writable by the user running Podman, only the user in the container). |
|
@Ramblurr would you like to open a PR to add a new option to skip the copyup? |
|
I think the idea to switch the order of the checks is the best one. Afaik docker doesn't have a switch to disable this behavior. Later, I can test to see how docker behaves in the case that the vol mount isn't writable but src dir is non empty. I'd be more inclined to automatically skip the copy up when it's not possible rather than introduce yet another flag. |
how would we distinguish a legit failure from something that should be ignored? I think it must be an explicit request from the user through a new flag. |
|
Ok :) Did some testing with docker. I followed the similar methodology to the original issue that introduced this behavior to podman: #12714
This output is expected.
Based on these results
That said, I did some more research, and apparently docker does support a I tested this using the third command above, changing the volume bit to If podman is aiming to keep compatibility with docker, then I suppose this flag could be implemented. |
|
SGTM Switching order and adding nocopy flag. |
|
PR here: #14734 |
github-actions
bot
added
the
locked - please file new issue/PR
Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)
/kind bug
Description
Using nfs named volumes has broken recently in some cases. I think this is related to
The error is:
My current theory is that the copy up routine is being performed as a user that doesn't have permission to write to the nfs share. In my case the only allowed user is
2000so even uid0on the podman host won't have permission.The funny thing is that I'm not even convinced the pre-condition for NeedsCopyUp is met, because the
/var/www/html/datadirectory in the container image is empty to start with.I have manually mounted the share using the same mount options, then
sued into user w/ uid2000and verified the mount works fine.Using
docker-ce(on another fresh host), I've tested it with docker and it works as expected.Steps to reproduce the issue:
2000Describe the results you received:
Describe the results you expected:
I expect the container to start and work with the mount.
This works fine with docker-ce (on centos 9 stream)
Additional information you deem important (e.g. issue happens only occasionally):
podman volume inspect my-nfs-volOutput of
podman version:Output of
podman info --debug:Package info (e.g. output of
rpm -q podmanorapt list podman):Have you tested with the latest version of Podman and have you checked the Podman Troubleshooting Guide? (https://github.com/containers/podman/blob/main/troubleshooting.md)
Yes
Additional environment details (AWS, VirtualBox, physical, etc.): kvm/qemu virtual machine running centos 9 stream
The text was updated successfully, but these errors were encountered: