set correct uid/gid on volumes #6730
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci-robot
added
the
approved
|
@rhatdan PTAL |
When setting up the volumes for a container, make sure that new volumes are created with the correct uid/gid which we're looking up in the container config. Fixes: containers#5698 Signed-off-by: Valentin Rothberg <[email protected]>
| @@ -308,8 +309,15 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai | |||
|
|
|||
| logrus.Debugf("Creating new volume %s for container", vol.Name) | |||
|
|
|||
| // Get the uid/gid of the container user to create the volume | |||
| // with the correct uid/gid. See github.com/containers/libpod/issues/5698. | |||
| uid, gid, _, err := chrootuser.GetUser(ctr.state.Mountpoint, ctr.config.User) | |||
There was a problem hiding this comment.
This is not safe - ctr.state.Mountpoint is unset at this point, because the container has not been mounted.
There was a problem hiding this comment.
If you pass a user by name instead of UID, this will break horribly and use the host's /etc/passwd instead of the container's.
There was a problem hiding this comment.
When I went over this with Sujil when he was working on it, we agreed that mounting the container this early in creation is prohibitively difficult, so he was investigating chown'ing the volume as part of copy-up.
There was a problem hiding this comment.
Eek. What shall we do?
There was a problem hiding this comment.
The best of a series of bad options was to do it as part of this block: https://github.com/containers/libpod/blob/f4c3b718eb22a161a897a6ed55d10f3a07e31aa8/libpod/container_internal.go#L1457-L1487
This only happens once, on first creation of the volume
There was a problem hiding this comment.
If you pass a user by name instead of UID, this will break horribly and use the host's
/etc/passwdinstead of the container's.
Can we support by name here at all?
There was a problem hiding this comment.
If that's OK, it makes things much simpler
There was a problem hiding this comment.
can we move the chown later (container_internal_linux.go generateSpec)? We already parse once the /etc/passwd and /etc/group files
There was a problem hiding this comment.
looking at it more in details, I think a possible solution is to have an operation like ChownIfEmpty on the volumes that we can use once we read /etc/passwd from the image. We can use it on new volumes that didn't exist before we created the container.
| for _, uidmap := range c.config.IDMappings.UIDMap { | ||
| if uidmap.ContainerID == 0 { | ||
| if uidmap.ContainerID == uid { |
There was a problem hiding this comment.
is the function supposed to be used only with findUID(0) as it seems in the current version? Otherwise it needs to check if the uid is in the [idmap.ContainerID; idmap.ContainerID+idmap.Size) range and the offset must be added to the return value.
| for _, gidmap := range c.config.IDMappings.GIDMap { | ||
| if gidmap.ContainerID == 0 { | ||
| if gidmap.ContainerID == gid { |
|
Closing in favour of #6747. |
github-actions
bot
added
the
locked - please file new issue/PR
When setting up the volumes for a container, make sure that new volumes
are created with the correct uid/gid which we're looking up in the
container config.
Fixes: #5698
Signed-off-by: Valentin Rothberg [email protected]