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.

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

Support idmap for image volume mounts #23224

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jul 8, 2024

Fixes: #23211

Does this PR introduce a user-facing change?

podman run --mount type=image now supports idmap mounts for rootful containers.

Copy link
Contributor

openshift-ci bot commented Jul 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2024
@rhatdan
Copy link
Member Author

rhatdan commented Jul 8, 2024

@giuseppe This fix still gets a permission denied when doing the mount with --userns=auto, any idea why?

Copy link

Cockpit tests failed for commit 1a2a619. @martinpitt, @jelly, @mvollmer please check.

@giuseppe
Copy link
Member

giuseppe commented Jul 8, 2024

@giuseppe This fix still gets a permission denied when doing the mount with --userns=auto, any idea why?

idmap is not an option understood by the kernel. We need to create the idmapped mount through drivers.MountOpts.{UidMaps,GidMaps}. MountImage() doesn't offer to specify a mapping at the moment so we need a new API in c/storage first

@martinpitt
Copy link
Contributor

That cockpit f40 failure was a temporary network outage. Please retry or ignore.

@giuseppe
Copy link
Member

giuseppe commented Jul 9, 2024

idmap is not an option understood by the kernel. We need to create the idmapped mount through drivers.MountOpts.{UidMaps,GidMaps}. MountImage() doesn't offer to specify a mapping at the moment so we need a new API in c/storage first

I had a look and it gets tricky in c/storage, as we will need to deal with an image mounted multiple times with different mappings. To avoid duplicating functionalities we already have for handling containers, we could create a temporary storage container based on the desired image and specify there the mappings

@Luap99
Copy link
Member

Luap99 commented Jul 9, 2024

To avoid duplicating functionalities we already have for handling containers, we could create a temporary storage container based on the desired image and specify there the mappings

If I remember correctly I saw a use case for rw image mount which actually changes the image content so if the changes are now temporary we we would break that.

@rhatdan
Copy link
Member Author

rhatdan commented Jul 9, 2024

rw image mounts are just an overlay mounted on the image, the underlying image is never changed.

@Luap99
Copy link
Member

Luap99 commented Jul 9, 2024

Ah right, I guess in this case a tmp storage copy container should be fine. I guess if do that we could also chown the image by default to the right userns then? Because idmap will not work rootless.

@giuseppe
Copy link
Member

yes, it automatically uses recursive chown if there is no native idmap or fuse-overlayfs to do the mapping at runtime.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 7, 2024
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link

A friendly reminder that this PR had no activity for 30 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note stale-pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"OCI permission denied" when using --mount=type=image with --userns=auto
5 participants