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

[CI:DOCS] podman --userns: clarify auto works for rootful containers, too #17156

Closed

Conversation

dilyanpalauzov
Copy link
Contributor

and the default when PODMAN_USERNS is missing.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 18, 2023

@dilyanpalauzov: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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/test-infra repository.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Please remove your text about the DCO from the commit message.

@@ -4,14 +4,14 @@
####> are applicable to all of those.
#### **--userns**=*mode*

Set the user namespace mode for the container. It defaults to the **PODMAN_USERNS** environment variable. An empty value ("") means user namespaces are disabled unless an explicit mapping is set with the **--uidmap** and **--gidmap** options.
Set the user namespace mode for the container. It defaults to the **PODMAN_USERNS** environment variable. When the environment variable is not set, it defaults to **host**, not to "". An empty value ("") means user namespaces are disabled unless an explicit mapping is set with the **--uidmap** and **--gidmap** options.
Copy link
Member

Choose a reason for hiding this comment

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

an empty string "" or host should be equivalent so I don't think it is necessary to specify the not to "" part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An empty value ("") means user namespaces are disabled unless an explicit mapping is set with the --uidmap and --gidmap options.

I think this is not correct. When I call rootless podman without --userns=, for that user /etc/subuid contains d:100000:65536, and the container creates (from its perspective as root) a file, on the host the file is owned by UID 100000. For me this means that newuidmap is utilized and therefore user-namespace is entered.

The cited sentence says in this case: user namespaces are enabled. Isn’t this a contradiction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cited sentence says in this case: user namespaces are enabled.

READ

The cited sentence says in this case: user namespaces are DISABLED. Isn’t this a contradiction?

Copy link
Member

Choose a reason for hiding this comment

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

rootless will always run in a user namespace, there is no way to make podman work without this
host or "" just means that there is no extra user namespace created for this container, instead it will use the one from the current process.

@dilyanpalauzov
Copy link
Contributor Author

Please remove your text about the DCO from the commit message.

In the past such texts in DCO were accepted, while commits without DCO were not accepted.

@Luap99
Copy link
Member

Luap99 commented Jan 18, 2023

A DCO is required, your rant about is not. I read commit messages and that useless extra content just wastes my time.

@dilyanpalauzov dilyanpalauzov force-pushed the userns_rootful_auto branch 2 times, most recently from ce7ce5e to d9a9105 Compare January 18, 2023 18:17
Key | Host User | Container User
----------|-------------|---------------------
"" or host|$UID | 0 (Default User account mapped to root user in container.)
keep-id |$UID | $UID (Map user account to same UID within container.)
Copy link
Member

@rhatdan rhatdan Jan 18, 2023

Choose a reason for hiding this comment

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

Does this work in rootful mode.

# podman run --userns=keep-id alpine echo hi
Error: keep-id is only supported in rootless mode
 podman run --userns=nomap alpine echo hi
Error: nomap is only supported in rootless mode

Key | Host User | Container User
----------|-------------|---------------------
"" or host|$UID | 0 (Default User account mapped to root user in container.)
keep-id |$UID | $UID (Map user account to same UID within container.)
keep-id:uid=200,gid=210 |$UID| 200:210 (Map user account to specified uid, gid value within container.)
auto |$UID | nil (Host User UID is not mapped into container.)
nomap |$UID | nil (Host User UID is not mapped into container.)
Copy link
Member

Choose a reason for hiding this comment

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

# podman run --userns=nomap alpine echo hi
Error: nomap is only supported in rootless mode

"" |$UID |0 (Default User account mapped to root user in container.)
keep-id |$UID |$UID (Map user account to same UID within container.)
"" or host|$UID | 0 (Default User account mapped to root user in container.)
keep-id |$UID | $UID (Map user account to same UID within container.)
Copy link
Member

Choose a reason for hiding this comment

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

# podman run --userns=keep-id alpine echo hi
Error: keep-id is only supported in rootless mode
# podman run --userns=nomap alpine echo hi
Error: nomap is only supported in rootless mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is correct, but the next lines contain:

The rootless option --userns=keep-id uses all the subuids and subgids of the user.

keep-id: creates a user namespace where the current rootless user's UID:GID are mapped to the same values in the container. This option is not allowed for containers created by the root user

nomap: creates a user namespace where the current rootless user's UID:GID are not mapped into the container. This option is not allowed for containers created by the root user.

To be precise, this text:

The rootless option --userns=keep-id uses all the subuids and subgids of the user.

is in fact present in userns.container.md and absent from userns.pod.md.

I think the above lines spell sufficiently clear, that keep-id and nomap are not allowed for rootful podman.

@rhatdan
Copy link
Member

rhatdan commented Jan 18, 2023

Ok you need to sign your commits.
git commit -a -s --amend
git push --force

@rhatdan rhatdan changed the title podman --userns: clarify auto works for rootful containers, too [CI:DOCS] podman --userns: clarify auto works for rootful containers, too Jan 18, 2023
@@ -4,16 +4,16 @@
####> are applicable to all of those.
#### **--userns**=*mode*

Set the user namespace mode for the container. It defaults to the **PODMAN_USERNS** environment variable. An empty value ("") means user namespaces are disabled unless an explicit mapping is set with the **--uidmap** and **--gidmap** options.
Set the user namespace mode for the container. It defaults to the **PODMAN_USERNS** environment variable. An empty value ("") or **host** means user namespaces are disabled, unless an explicit mapping is set with the **--uidmap** and **--gidmap** options or podman is rootless.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not at all gronking "or host". Does that mean --userns=127.0.0.1 and --userns=myhost.mycompany.com are valid? There is no mention of host prior to this, and only one way down lower in this page. Examples for this, and other possible settings that might be found in this file would be a very welcomed addition to this man page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An empty value ("") or host means …

This wording is chosen in order to emphasize that empty value and host have the same meaning. I am not aware of open tickets, claiming that it is unclear how to interpret host, except #15764, which is from me.

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 host is fine, that has the same meaning as any other namespace

@dilyanpalauzov
Copy link
Contributor Author

Please remove your text about the DCO from the commit message.

I am not going to remove the text. Or rather, the commit contains now Signed-off-by: Francis <pope@va>. You can accept this, or I can also revert the original wording.

@rhatdan
Copy link
Member

rhatdan commented Jan 29, 2023

I think the DCO check is looking for a blank line between the title and the first line of description, please rewrite your description like that and the DCO should pass.

@giuseppe
Copy link
Member

please provide a valid DCO, the commit cannot be accepted otherwise

and the default when PODMAN_USERNS is missing.

Insisting on “DCO” imposes formalities, that serve self-purpose.  One cannot
assume that the submitter has time or will to read texts about symbolism in
software contributions.  If the system wants to see the text

  nrEAUIEUAIe eanuitdnuae EAIUEAUIAIE »ℓ§444.3.72b)°»°ℓ§euaieauuae

in each commit, people will write this, or any other text, that the system wants to
see.  All such text, which presence is mandated by the system, has the same value.

Signed-off-by: Дилян Палаузов <git-dpa@aegee.org>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 10, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found 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

@Luap99
Copy link
Member

Luap99 commented Feb 10, 2023

Insisting on “DCO” imposes formalities, that serve self-purpose.  One cannot
assume that the submitter has time or will to read texts about symbolism in
software contributions.  If the system wants to see the text

  nrEAUIEUAIe eanuitdnuae EAIUEAUIAIE »ℓ§444.3.72b)°»°ℓ§euaieauuae

in each commit, people will write this, or any other text, that the system wants to
see.  All such text, which presence is mandated by the system, has the same value.

Please remove this this part of your commit message.
If you want to discussion about the DCO feel free to do so but the commit message is not the place for it.

@dilyanpalauzov
Copy link
Contributor Author

Insisting on “DCO” imposes formalities, that serve self-purpose.  One cannot
assume that the submitter has time or will to read texts about symbolism in
software contributions.  If the system wants to see the text

  nrEAUIEUAIe eanuitdnuae EAIUEAUIAIE »ℓ§444.3.72b)°»°ℓ§euaieauuae

in each commit, people will write this, or any other text, that the system wants to
see.  All such text, which presence is mandated by the system, has the same value.

Please remove this this part of your commit message. If you want to discussion about the DCO feel free to do so but the commit message is not the place for it.

In the past this text was accepted here: 056917c, ce080d2.

@vrothberg
Copy link
Member

In the past this text was accepted here: 056917c, ce080d2.

That was likely an oversight while reviewing. I respect the rebellion but also agree with @Luap99 that the commit message is not the right place.

@dilyanpalauzov
Copy link
Contributor Author

I am not going to submit changesets to this project anymore.

@rhatdan
Copy link
Member

rhatdan commented Feb 14, 2023

Sorry you feel that way @dilyanpalauzov

@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 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants