Do not chown newly created volume sources #16782
Conversation
openshift-ci
bot
added
release-note
approved
|
@containers/podman-maintainers PTAL |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -151,9 +151,6 @@ func (r *Runtime) newVolume(ctx context.Context, noCreatePluginVolume bool, opti | |||
| if err := os.MkdirAll(volPathRoot, 0700); err != nil { | |||
| return nil, fmt.Errorf("creating volume directory %q: %w", volPathRoot, err) | |||
| } | |||
| if err := idtools.SafeChown(volPathRoot, volume.config.UID, volume.config.GID); err != nil { | |||
There was a problem hiding this comment.
This isn't going to work, volume.config.UID and volume.config.GID are actual options that can be set during volume creation.
Should we stop defaulting them to the container's UID/GID instead, and make this happen if and only if they were deliberately set?
There was a problem hiding this comment.
I think the tests checks that case.
The problem was the higher level directories were being set incorrectly. We just want them to default to the users uid running podman.
There was a problem hiding this comment.
BTW, the answer is yes they should only be set if the user sets them. but not sure how that info can be recorded, I guess we could default the UID/GID to -1?
|
I was thinking about changing UID and GID to *int instead so we would have
a clear unset state, but negative integer works fine too.
…On Mon, Dec 12, 2022 at 3:30 PM Daniel J Walsh ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In libpod/runtime_volume_common.go
<#16782 (comment)>:
> @@ -151,9 +151,6 @@ func (r *Runtime) newVolume(ctx context.Context, noCreatePluginVolume bool, opti
if err := os.MkdirAll(volPathRoot, 0700); err != nil {
return nil, fmt.Errorf("creating volume directory %q: %w", volPathRoot, err)
}
- if err := idtools.SafeChown(volPathRoot, volume.config.UID, volume.config.GID); err != nil {
BTW, the answer is yes they should only be set if the user sets them. but
not sure how that info can be recorded, I guess we could default the
UID/GID to -1?
—
Reply to this email directly, view it on GitHub
<#16782 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3AOCDEARJGS5GNJVOBD3DWM6DO7ANCNFSM6AAAAAASXKKDYY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
rhatdan
force-pushed
the
volume
branch
2 times, most recently
from
686e37a to
11cbb28
Compare
|
@mheon PTAL |
rhatdan
force-pushed
the
volume
branch
3 times, most recently
from
0c54ae9 to
d609f69
Compare
| return os.Getuid(), gid | ||
| case gid == -1: | ||
| return uid, os.Getgid() | ||
| } |
There was a problem hiding this comment.
Personal nit, YMMV.
In general, I've not been a fan of equality tests for -1. Can we convert these to uid > -1 or uid < 0 or some such?
|
@rhatdan tests are still being touchy. |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
A friendly reminder that this PR had no activity for 30 days. |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
When running containers with podman run --userns=keep-id --userns $UID:$GID -v test:/tmp/test ... The volumes directory ends up being owned by root within the user namespace, which is not root of the general namespace nor the users uid. If we just allow podman to create these directories with the same UID that is running podman, IE don't chown, they end up with the correct UID in all cases. The actual volume will be chowned to the UID of the container. Fixes: containers#16741 Signed-off-by: Daniel J Walsh <[email protected]>
|
Friendly ping, @rhatdan. I am going through open PRs and this one seems to have fallen off the radar. |
|
Yup. this one I think we really should have @mheon take over, since he understands this the best and I have little time to look into it. |
|
Going through old inactive PRs... |
github-actions
bot
added
the
locked - please file new issue/PR
When running containers with
podman run --userns=keep-id --userns $UID:$GID -v test:/tmp/test ...
The volumes directory ends up being owned by root within the user namespace, which is not root of the general namespace nor the users uid.
If we just allow podman to create these directories with the same UID that is running podman, IE don't chown, they end up with the correct UID in all cases.
The actual volume will be chowned to the UID of the container.
Fixes: #16741
Signed-off-by: Daniel J Walsh [email protected]
Does this PR introduce a user-facing change?