-
Notifications
You must be signed in to change notification settings - Fork 125
Proof-of-concept: set up necessary idmappings for userns_mode=auto in Container.create() #499
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vmsh0 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@giuseppe PTAL |
|
would you be interested to try to fix it in Podman? If you are not interested/familiar with the project or language, then it is fine to fix it here |
I would be interested in attempting that :) I am not a Go programmer by trade, but I am probably familiar enough with it for this particular fix. I have successfully built podman yesterday, now I'm trying to get the tests to run. Can I bother you (or someone else) if I can't get the tests to run? What is the preferred channel, GitHub or something more ephemeral (e.g., IRC)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vmsh0 maybe late to the party, but here are the comms channels :) https://github.com/containers/podman/blob/main/README.md#communications
|
Hi, sorry for reviving this after a year. I have twice attempted to allocate some time last year to get podman to compile and try to get this fixed, but this is just not fitting in in my life at the moment. Would the project still be interested in the Python workaround? |
… Container.create() This is a proof-of-concept commit to gather initial feedback Signed-off-by: Riccardo Paolo Bestetti <pbl@bestov.io>
bdd5b88 to
594c123
Compare
| if "userns_mode" in args: | ||
| params["userns"] = normalize_nsmode(args.pop("userns_mode")) | ||
|
|
||
| # proof-of-concept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this comment a left over?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. So far this patch is just a workaround I have deployed in my software to make things work, but if it is something that you would include in the library I'll clean it up and perhaps see about adding tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwhonce are you fine with it?
This is a proof-of-concept commit to gather initial feedback.
This addresses the issue in #493, i.e., that passing
userns='auto'to Container.create() results in the option being silently ignored.Before this patch,
podman-pyused to set up theusernsAPI parameter when theusernsparameter was given to Container.create(). However, upon investigation, it seems like the Podman service silently ignores the passedusernsif parameteridmappingsis missing.This patch addresses this behaviour by setting up
idmappingswith neutral values (i.e., the values resulting in the same behaviour as the Podman client when called with --userns=auto without more specific options), while specifically retaining any explicit values passed by the user using the undocumented argumentidmappings.I am looking for some feedback about this PR:
merge_dicts()function? Or would it be better altogether to merge the relatively small structure manually and avoid adding that function altogether?podman-py. In your view, what would be a good collection of integration tests for this PR/feature? At a minimum, I will be contributing one integration test which checks that passing theuserns='auto'results in a container with a private user namespace, with IDs not overlapping with the initial namespace, as that is my use case