namespaces: allow configuring keep-id userns size #24882

giuseppe · 2024-12-20T09:07:20Z

Introduce a new option "size" to configure the maximum size of the user namespace configured by keep-id.

Now --userns=keep-id supports a new option `size` to configure the size of the user namespace

rhatdan · 2024-12-20T14:06:19Z

LGTM

docs/source/markdown/options/userns.container.md

Jookia · 2024-12-20T18:51:23Z

How should this be integrated in to newer projects like distrobox? A version check to see if podman is > 5.3.1? Othersize size is undefined.

Jookia · 2024-12-20T20:50:22Z

I'm going to test this this weekend and try to integrate it in to distrobox to see if this works for my use case. I'll report back if it's a good solution for this problem.

giuseppe · 2024-12-20T20:53:13Z

How should this be integrated in to newer projects like distrobox? A version check to see if podman is > 5.3.1? Othersize size is undefined.

personally I don't like version checks, it is error prone and it won't work if we backport the feature to older releases.

You could just run a simple container with that option and see if it works podman run --rm --userns=keep-id:size=1 $IMG /bin/true, or if it fails with Error: unknown option specified: "size"

giuseppe · 2024-12-20T20:53:50Z

How should this be integrated in to newer projects like distrobox? A version check to see if podman is > 5.3.1? Othersize size is undefined.

personally I don't like version checks, it is error prone and it won't work if we backport the feature to older releases.

You could just run a simple container with that option and see if it works podman run --rm --userns=keep-id:size=1 $IMG /bin/true, or if it fails with Error: unknown option specified: "size"

even better if you specify the correct size you plan to use, so the image is copied and chowned only once

Jookia · 2024-12-20T22:23:08Z

Okay, thanks for the advice. I guess follow-up question is whether this will cause each container to have a different set of UIDs if I've never run --userns=auto before? It's a bit confusing how these interact.

giuseppe · 2024-12-21T21:14:52Z

Okay, thanks for the advice. I guess follow-up question is whether this will cause each container to have a different set of UIDs if I've never run --userns=auto before? It's a bit confusing how these interact.

keep-id will keep using a static mapping, the size argument just makes sure it won't use all the available IDs, so that some of them can be used for auto containers

Jookia · 2024-12-22T03:57:06Z

Good news: I have tested this with a modified distrobox and it works well! I can now run my userns=auto containers AND userns=keep-id:size=65536 distroboxes on my machine.

Bad news: I started looking at how to use this in distrobox. I can't guarantee 'podman run --rm --userns=keep-id:size=65336 $IMG /bin/true' will work as I can't guarantee the image will have /bin/true or really any specific binary. Would it be safe to take exit code 127 as success in this case?

My current code looks like this:

			# Add :size=65536 if possible
			if podman run --rm --userns=keep-id:size=65536 ${container_image} /bin/true 2>/dev/null || [ "$?" -eq 127 ] ; then
				result_command="${result_command}:size=65536"
			fi

Edit: This works perfectly fine, but it seems a little non-ideal.

giuseppe · 2024-12-22T07:05:37Z

would be enough to just create the container without running it? podman create instead of podman run

Jookia · 2024-12-22T08:15:47Z

Running this on a regular podman doesn't error: ``` podman create archlinux --userns=keep-id:size=65536 ``` Despite no size argument being implemented.

…

On Sat, Dec 21, 2024 at 11:06:00PM -0800, Giuseppe Scrivano wrote: would be enough to just create the container without running it? `podman create` instead of `podman run` -- Reply to this email directly or view it on GitHub: #24882 (comment) You are receiving this because you commented. Message ID: ***@***.***>

mheon · 2025-01-07T01:23:20Z

Can you add the new size option to the E2E or system tests? Maybe just take an existing userns test and add the new syntax to verify it works

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2025-01-07T13:12:45Z

Can you add the new size option to the E2E or system tests? Maybe just take an existing userns test and add the new syntax to verify it works

sure. Added a new test

Luap99

I think the size argument is fine but is this actually what the original use case wants? I feel like forcing everyone to add size doesn't improve the situation that a keep-id makes all other userns=auto fail. That seems backwards because we now keep-id means all uids, just like userns=host (default) means all uids are mapped. So why would we allow userns=host and auto to coexists while userns=keep-id and auto means error.
I think fixing that might be much more useful than requiring all users to manually specify a size for keep-id. Anyway that is no point again having this option.

Luap99 · 2025-01-07T17:37:13Z

pkg/specgen/namespaces.go

+		if opts.MaxSize != nil && !rootless.IsRootless() {
+			return user, fmt.Errorf("cannot set max size for user namespace when not running rootless")
+		}


Why not? Could we not just limit the rootful size in the same way as done for rootless?

keep-id with root is a no-op. It doesn't currently create a user namespace

Luap99 · 2025-01-07T17:37:32Z

docs/source/markdown/options/userns.container.md

@@ -62,6 +62,7 @@ For details see **--uidmap**.

  - *uid*=UID: override the UID inside the container that is used to map the current user to.
  - *gid*=GID: override the GID inside the container that is used to map the current user to.
+  - *size*=SIZE: override the size of the configured user namespace.  It is useful to not saturate all the available IDs.


if size is really not support as root it should be documented here

Jookia · 2025-01-08T00:53:19Z

For sure it would be better if keep-id and nomap and friends just limited their scope to 65536 or similar automatically so everything would just work, but would that break existing tools?

giuseppe · 2025-01-08T09:07:53Z

I think the size argument is fine but is this actually what the original use case wants? I feel like forcing everyone to add size doesn't improve the situation that a keep-id makes all other userns=auto fail. That seems backwards because we now keep-id means all uids, just like userns=host (default) means all uids are mapped. So why would we allow userns=host and auto to coexists while userns=keep-id and auto means error.
I think fixing that might be much more useful than requiring all users to manually specify a size for keep-id. Anyway that is no point again having this option.

users using keep-id usually want the feel of running on the host, e.g. toolbx, so if we limit the user namespace size is 1) breaking change, 2) worse experience for toolbx kind of use cases, as they will then need to specify the maximum size.

I don't see how the two worlds can work well together. auto wants distinct IDs while keep-id better uses as many IDs as possible.

The issue would exist also with containers not using a user namespace, since they use all the available IDs.

Introduce a new option "size" to configure the maximum size of the user namespace configured by keep-id. Closes: containers#24837 Signed-off-by: Giuseppe Scrivano <[email protected]>

Luap99 · 2025-01-08T12:36:37Z

@giuseppe But the thing is

$ podman run -d quay.io/libpod/testimage:20241011 sleep 100
b8402a2257460db59b9d8b2f3120bbae17999444d45d779e892991922d0be9bc
$ podman run -d --userns auto  quay.io/libpod/testimage:20241011 sleep 100
a67fd59a3afac9a5a88a8f7c842962ad7a4644cc80f11f67568cfa9796acafad

This works fine an non user namesapced container does not block all uids

$ podman run -d --userns keep-id  quay.io/libpod/testimage:20241011 sleep 100
72add542da2d901dec93cedb93edf202588fbbb8f981dad437b7312aeab55ff5
$ podman run -d --userns auto  quay.io/libpod/testimage:20241011 sleep 100
Error: creating container storage: not enough unused IDs in user namespace

But keep-id blocks all uids, so that logic seems inconsistent.
It makes sense that auto tries to find something unused for multiple userns=auto containers or custom uidmap but with keep-id mode it just seems overly strict.
And if the behavior really should be that strict that a container without userns must also cause auto to fail.

giuseppe · 2025-01-08T13:26:55Z

But keep-id blocks all uids, so that logic seems inconsistent.
It makes sense that auto tries to find something unused for multiple userns=auto containers or custom uidmap but with keep-id mode it just seems overly strict.
And if the behavior really should be that strict that a container without userns must also cause auto to fail.

are you suggesting we ignore keep-id containers for auto mode? That could work in theory, but we currently have auto implemented in containers/storage, and it finds an available range not used by any other user namespace. containers/storage of course doesn't know about keep-id which is handled by Podman.

We could argue about a better solution, but I don't see any downside if we allow to customize the user namespace size for the keep-id case, it could be helpful in other cases too.

Luap99 · 2025-01-08T13:33:16Z

But keep-id blocks all uids, so that logic seems inconsistent.
It makes sense that auto tries to find something unused for multiple userns=auto containers or custom uidmap but with keep-id mode it just seems overly strict.
And if the behavior really should be that strict that a container without userns must also cause auto to fail.

are you suggesting we ignore keep-id containers for auto mode? That could work in theory, but we currently have auto implemented in containers/storage, and it finds an available range not used by any other user namespace. containers/storage of course doesn't know about keep-id which is handled by Podman.

Yes I know it is not easy, just saying the current way feels inconsistent.

We could argue about a better solution, but I don't see any downside if we allow to customize the user namespace size for the keep-id case, it could be helpful in other cases too.

Right as I said in my original comment I am totally fine with this PR alone. I just do not think the use case of the issue is not solved in the best way with it but it certainly improves it so this is good.

Luap99

LGTM

openshift-ci · 2025-01-08T13:33:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, Luap99

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:

~~OWNERS~~ [Luap99,giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2025-01-08T18:26:05Z

/lgtm

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 20, 2024

giuseppe mentioned this pull request Dec 20, 2024

Allow mixing userns=auto and userns=keep-id #24837

Closed

giuseppe force-pushed the keep-id-size branch from 8f3662a to 67318cf Compare December 20, 2024 09:11

Jookia reviewed Dec 20, 2024

View reviewed changes

docs/source/markdown/options/userns.container.md Show resolved Hide resolved

giuseppe force-pushed the keep-id-size branch from 67318cf to 5f2b57b Compare December 20, 2024 20:48

specgen: fix comment

1e0f03b

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the keep-id-size branch from 5f2b57b to d5911d2 Compare January 7, 2025 11:50

Luap99 reviewed Jan 7, 2025

View reviewed changes

namespaces: allow configuring keep-id userns size

b0b1d19

Introduce a new option "size" to configure the maximum size of the user namespace configured by keep-id. Closes: containers#24837 Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the keep-id-size branch from d5911d2 to b0b1d19 Compare January 8, 2025 09:08

Luap99 approved these changes Jan 8, 2025

View reviewed changes

openshift-ci bot assigned rhatdan Jan 8, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2025

openshift-merge-bot bot merged commit 2e1e710 into containers:main Jan 8, 2025
80 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

namespaces: allow configuring keep-id userns size #24882

namespaces: allow configuring keep-id userns size #24882

giuseppe commented Dec 20, 2024

rhatdan commented Dec 20, 2024

Jookia commented Dec 20, 2024

Jookia commented Dec 20, 2024

giuseppe commented Dec 20, 2024

giuseppe commented Dec 20, 2024

Jookia commented Dec 20, 2024

giuseppe commented Dec 21, 2024

Jookia commented Dec 22, 2024 •

edited

Loading

giuseppe commented Dec 22, 2024

Jookia commented Dec 22, 2024 via email

mheon commented Jan 7, 2025

giuseppe commented Jan 7, 2025

Luap99 left a comment

Luap99 Jan 7, 2025

giuseppe Jan 8, 2025

Luap99 Jan 7, 2025

Jookia commented Jan 8, 2025

giuseppe commented Jan 8, 2025

Luap99 commented Jan 8, 2025

giuseppe commented Jan 8, 2025

Luap99 commented Jan 8, 2025

Luap99 left a comment

openshift-ci bot commented Jan 8, 2025

rhatdan commented Jan 8, 2025

namespaces: allow configuring keep-id userns size #24882

namespaces: allow configuring keep-id userns size #24882

Conversation

giuseppe commented Dec 20, 2024

rhatdan commented Dec 20, 2024

Jookia commented Dec 20, 2024

Jookia commented Dec 20, 2024

giuseppe commented Dec 20, 2024

giuseppe commented Dec 20, 2024

Jookia commented Dec 20, 2024

giuseppe commented Dec 21, 2024

Jookia commented Dec 22, 2024 • edited Loading

giuseppe commented Dec 22, 2024

Jookia commented Dec 22, 2024 via email

mheon commented Jan 7, 2025

giuseppe commented Jan 7, 2025

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 Jan 7, 2025

Choose a reason for hiding this comment

giuseppe Jan 8, 2025

Choose a reason for hiding this comment

Luap99 Jan 7, 2025

Choose a reason for hiding this comment

Jookia commented Jan 8, 2025

giuseppe commented Jan 8, 2025

Luap99 commented Jan 8, 2025

giuseppe commented Jan 8, 2025

Luap99 commented Jan 8, 2025

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 8, 2025

rhatdan commented Jan 8, 2025

Jookia commented Dec 22, 2024 •

edited

Loading