Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

libpod: change mountpoint ownership when creating overlays on top of external rootfs #11937

Merged
merged 1 commit into from
Oct 19, 2021
Merged

libpod: change mountpoint ownership when creating overlays on top of external rootfs #11937

merged 1 commit into from
Oct 19, 2021

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Oct 12, 2021 •
edited
Loading

Allow chainging ownership of mountpoint created on top of external overlay
rootfs to support use-cases when custom --uidmap and --gidmap are
specified.

TLDR
Supports uidmap and gidmap with overlays on top of external rootfs specified with
--rootfs </path>:O

Example

podman run --uidmap 0:1000:1000 --rm --rootfs /tmp/foo:O bash

@flouthoc
Copy link
Collaborator Author

@giuseppe PTAL

@flouthoc flouthoc marked this pull request as draft October 12, 2021 11:19
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 12, 2021
@flouthoc flouthoc changed the title libpod: change mountpoint ownership overlays on top of external rootfs libpod: change mountpoint ownership when creating overlays on top of external rootfs Oct 12, 2021
giuseppe
giuseppe requested changes Oct 12, 2021
View reviewed changes
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see an issue when using this patch:

# bin/podman run --uidmap 0:1000:1000 --rm --rootfs /tmp/foo:O bash
Error: make `/var/lib/containers/storage/overlay-containers/1df9c17515d7ebb92d6b36fa2790c5c7c501a7509a0acffefc03711c9b96909f/rootfs/merge` private: Permission denied: OCI permission denied

libpod/container_internal.go Outdated
@@ -1515,6 +1515,10 @@ func (c *Container) mountStorage() (_ string, deferredErr error) {
}

mountPoint = overlayMount.Source
// change ownership of `merged` created from overlay on top of external rootfs
if err := os.Chown(mountPoint, c.RootUID(), c.RootGID()); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to map the /merge directory for the overlay mount (and make sure it is accessible with makeAccessible) not the mount point.

@flouthoc flouthoc force-pushed the overlay-rootfs-chown branch 2 times, most recently from ff76123 to 0e0f607 Compare October 13, 2021 12:15
@flouthoc flouthoc mentioned this pull request Oct 18, 2021
Merged
@flouthoc flouthoc force-pushed the overlay-rootfs-chown branch 2 times, most recently from b642825 to 638781e Compare October 19, 2021 09:53
@flouthoc flouthoc marked this pull request as ready for review October 19, 2021 10:23
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 19, 2021
giuseppe
giuseppe reviewed Oct 19, 2021
View reviewed changes
libpod/container_internal.go Outdated
if err != nil {
return "", errors.Wrap(err, "unable to get host UID and host GID")
}
if err := chown.ChangeHostPathOwnership(mountPoint, true, int(hostUID), int(hostGID)); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can't be recursive, otherwise we change the ownership of each file in the underlying rootfs

Copy link
Collaborator Author

@flouthoc flouthoc Oct 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes changed to false should we add to man page, that users are responsible for ownership configs of underlying rootfs.

@flouthoc
libpod: change mountpoint ownership c.Root when using overlay on top …
9500e11 
…of external rootfs

Allow chainging ownership of mountpoint created on top external overlay
rootfs to support use-cases when custom --uidmap and --gidmap are
specified.

Signed-off-by: Aditya Rajan <[email protected]>
@flouthoc flouthoc force-pushed the overlay-rootfs-chown branch from 638781e to 9500e11 Compare October 19, 2021 10:42
@flouthoc flouthoc requested a review from giuseppe October 19, 2021 13:55
giuseppe
giuseppe approved these changes Oct 19, 2021
View reviewed changes
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2021
@flouthoc flouthoc requested a review from rhatdan October 19, 2021 13:59
@flouthoc
Copy link
Collaborator Author

@rhatdan @giuseppe final review and merge needed on this one.

@rhatdan
Copy link
Member

rhatdan commented Oct 19, 2021

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 19, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, giuseppe, rhatdan

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 82fd299 into containers:main Oct 19, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@giuseppe giuseppe giuseppe approved these changes

@rhatdan rhatdan Awaiting requested review from rhatdan

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@flouthoc @rhatdan @giuseppe @openshift-merge-robot