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

kube generate/play restores the user namespace configuration #23249

Merged
merged 7 commits into from
Jul 24, 2024

Conversation

giuseppe
Copy link
Member

a few fixes to improve how "kube generate" and "kube apply" deal with user namespaces.

Now the yaml generated by "kube generate" stores the user namespace configuration for the pod, which is later used by "kube apply"

Closes: https://issues.redhat.com/browse/RHEL-13033

Does this PR introduce a user-facing change?

Now "kube generate" stores the user namespace configuration that is used by "kube apply"

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 11, 2024
@Luap99
Copy link
Member

Luap99 commented Jul 11, 2024

looks like this doesn't work via remote

@giuseppe giuseppe force-pushed the play-kube-userns-fixes branch from c801e6e to 1b539e2 Compare July 11, 2024 13:27
@giuseppe giuseppe marked this pull request as draft July 11, 2024 13:28
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2024
@giuseppe giuseppe force-pushed the play-kube-userns-fixes branch 5 times, most recently from aca6d44 to cac5914 Compare July 11, 2024 20:06
@giuseppe giuseppe marked this pull request as ready for review July 12, 2024 08:38
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 12, 2024
test/system/700-play.bats Outdated Show resolved Hide resolved
if is_rootless; then
grep -E -q "^$(id -un):" /etc/subuid || skip "no IDs allocated for current user"
else
grep -E -q "^containers:" /etc/subuid || skip "no IDs allocated for user 'containers'"
Copy link
Member

Choose a reason for hiding this comment

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

All theses tests are skipped in CI btw, seems there are lof of them so we likely should fix that for our CI setup to add the containers entry (not for this PR of course).

Copy link
Member

Choose a reason for hiding this comment

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

created #23383 for that

test/system/700-play.bats Outdated Show resolved Hide resolved
@giuseppe
Copy link
Member Author

@containers/podman-maintainers PTAL

giuseppe added 5 commits July 24, 2024 12:10
it increases readability as it doesn't need the negation, and the
first branch is shorter.

Signed-off-by: Giuseppe Scrivano <[email protected]>
The pod spec HostUsers boolean only specifies whether a user namespace
is used or not.  Hene, the podman specific annotation must have a
higher precedence since it defines how the user namespace is created.

Signed-off-by: Giuseppe Scrivano <[email protected]>
if there is an annotation that specifies the user namespace for the
infra container, then make sure it is used for the entire pod.

Signed-off-by: Giuseppe Scrivano <[email protected]>
currently there is no way to specify the mappings, so at least treat a
private user namespace as "auto".

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the play-kube-userns-fixes branch from cac5914 to d8d8e93 Compare July 24, 2024 11:02
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Jul 24, 2024
Copy link

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

@giuseppe giuseppe force-pushed the play-kube-userns-fixes branch from d8d8e93 to 7b8a56b Compare July 24, 2024 11:36
Copy link

Cockpit tests failed for commit 7b8a56b0bd954a7caa63ddaeacd37e17512ca9cc. @martinpitt, @jelly, @mvollmer please check.

@martinpitt
Copy link
Contributor

@thrix : Halp! Today we are getting a flood of failed stratis and podman test failures like this due to a broken shadow-utils. The broken 4.15.1-8.fc41 was unpushed last night and this update fixes it. But tmt machines are somehow stuck with the broken one. Can the image be refreshed or something similar? Why doesn't the upgrade pick up the new version from the tag repo?

Thanks!

@Luap99 FYI -- selinux/firewalld yesterday, shadow-utils today, argh rawhide

@martinpitt
Copy link
Contributor

@thrix: unping -- I'm able to hack around that on the client side, see cockpit-project/cockpit-podman#1801 . That should land within the next 30 mins or so, after that rawhide should go green again.

@martinpitt
Copy link
Contributor

Landed the shadow-utils b0rkage workaround. For the recent podman PRs where you care, please retry the rawhide failure. Thanks!

validate that a "podman generate" and "podman play" cycle restores the
specified user namespace.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the play-kube-userns-fixes branch from 7b8a56b to d9c2806 Compare July 24, 2024 15:36
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.

LGTM

Copy link
Contributor

openshift-ci bot commented Jul 24, 2024

[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:

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

@mheon
Copy link
Member

mheon commented Jul 24, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 1da89dd into containers:main Jul 24, 2024
82 checks passed
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Oct 23, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants