Skip to content

Proof-of-concept: set up necessary idmappings for userns_mode=auto in Container.create()#499

Open
vmsh0 wants to merge 1 commit into
containers:mainfrom
vmsh0:container-userns
Open

Proof-of-concept: set up necessary idmappings for userns_mode=auto in Container.create()#499
vmsh0 wants to merge 1 commit into
containers:mainfrom
vmsh0:container-userns

Conversation

@vmsh0
Copy link
Copy Markdown
Contributor

@vmsh0 vmsh0 commented Jan 12, 2025

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-py used to set up the userns API parameter when the userns parameter was given to Container.create(). However, upon investigation, it seems like the Podman service silently ignores the passed userns if parameter idmappings is missing.

This patch addresses this behaviour by setting up idmappings with 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 argument idmappings.

I am looking for some feedback about this PR:

  • Is the general structure of the solution acceptable?
  • I do not see any other instances where a dict merge approach is used to set up default values. Is there perhaps a better place to put the merge_dicts() function? Or would it be better altogether to merge the relatively small structure manually and avoid adding that function altogether?
  • I see that, in general, integration testing is pretty minimal and doesn't cover very many of the possible use cases of 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 the userns='auto' results in a container with a private user namespace, with IDs not overlapping with the initial namespace, as that is my use case

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jan 12, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vmsh0
Once this PR has been reviewed and has the lgtm label, please assign mheon for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details 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

@rhatdan
Copy link
Copy Markdown
Member

rhatdan commented Jan 14, 2025

@giuseppe PTAL

@giuseppe
Copy link
Copy Markdown
Member

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

@vmsh0
Copy link
Copy Markdown
Contributor Author

vmsh0 commented Jan 15, 2025

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)?

@inknos inknos self-requested a review January 15, 2025 14:47
Copy link
Copy Markdown
Contributor

@inknos inknos left a 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

@vmsh0
Copy link
Copy Markdown
Contributor Author

vmsh0 commented Jan 25, 2026

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>
if "userns_mode" in args:
params["userns"] = normalize_nsmode(args.pop("userns_mode"))

# proof-of-concept
Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@vmsh0 I can live with the work-around if you add more detail on why we are doing it and under what conditions it should be removed. As for testing, more the better. :-)

@inknos The errors reported during the build don't seem to match the code in the patch. ideas?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

they are only linting errors. @vmsh0

diff --git a/podman/domain/containers_create.py b/podman/domain/containers_create.py
index 066902c..308549d 100644
--- a/podman/domain/containers_create.py
+++ b/podman/domain/containers_create.py
@@ -847,7 +847,9 @@ class CreateMixin:  # pylint: disable=too-few-public-methods
 
             default_idmappings = {
                 "AutoUserNs": False if params["userns"]["nsmode"] != "auto" else True,
-                "AutoUserNsOpts": None if params["userns"]["nsmode"] != "auto" else {
+                "AutoUserNsOpts": None
+                if params["userns"]["nsmode"] != "auto"
+                else {
                     "AdditionalUIDMappings": None,
                     "AdditionalGIDMappings": None,
                     "PasswdFile": "",

please reformat and change the subject line to < 72 characters so all linters are happy :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also, my two cents: I am ok with the workaround with an open issue in Podman that cross-references the change in the code. @vmsh0 I am active on podman-py, so if you need help working on both repos feel free to tag me around or in our matrix channel and I'll be happy to help

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jwhonce

@vmsh0 I can live with the work-around if you add more detail on why we are doing it and under what conditions it should be removed.

Here, in the code, or somewhere else?

By the way, today I have validated that the workaround is still needed in recent Podman, as last year when I discovered this I was running Podman 4.

As for testing, more the better. :-)

👍

@inknos

also, my two cents: I am ok with the workaround with an open issue in Podman that cross-references the change in the code. @vmsh0 I am active on podman-py, so if you need help working on both repos feel free to tag me around or in our matrix channel and I'll be happy to help

The best case scenario for me would be if you could provide some support in getting podman to compile so I can still try to fix it there. But if that's too much, I would still appreciate your guidance in getting this patch ready for podman-py instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Uh. I tried to build latest Podman, and this time it looks like I have succeeded without any hiccups. Could also reproduce the issue there. Let me look into this a bit more, and I'll find you on Matrix if I need help.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants