server: Always include UID/GID mappings for user namespace containers#9712
Conversation
When containers join a user namespace via path, UID/GID mappings must still be included in the OCI spec. While the namespace path indicates which namespace to join, the mappings are essential for the runtime to properly handle file ownership operations, particularly cgroup delegation. Without these mappings, runtimes cannot chown the cgroup directory to the mapped UID 0, causing systemd containers to fail with "Permission denied" when attempting to create cgroups like /init.scope. This restores the behavior from v1.34.4 where both namespace path and mappings coexisted successfully. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
📝 WalkthroughWalkthroughThe change removes the conditional that skipped adding UID/GID mappings when the sandbox used a user namespace. UID and GID mappings from containerIDMappings are now always applied to the OCI spec; comments were updated accordingly and a test was added to verify mappings for cgroup delegation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saschagrunert The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cherry-pick release-1.35 |
|
@saschagrunert: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
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-sigs/prow repository. |
Add integration test to verify that containers joining a user namespace via path still have UID/GID mappings in their OCI spec. This ensures proper cgroup delegation for systemd containers. The test verifies: - User namespace path is set (joining sandbox's userns) - uidMappings and gidMappings are present in config.json - Container can start successfully - Container can access cgroups This prevents regression of the issue fixed in the previous commit where missing mappings caused systemd containers to fail with "Permission denied" when creating cgroups. Regression test for: cri-o#9705 Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
|
Reverting this change c2ebfba#diff-1ec6bb9cf833fd4ded46ce1feb6440f1d7532294e0445623f2f2e0bb2726ea63L1451-L1456 We want the test cases for systemd container. I tried once and unfortunately the exisiting images in quay.io/cri-o can't be used. |
|
I added a testcase in 0f68aa8 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/cgroups.bats`:
- Around line 296-299: The assertion for the user namespace path is weak because
jq -r can emit the literal string "null" so the current check using user_ns_path
and [[ -n "$user_ns_path" ]] can pass incorrectly; update the extraction/check
to ensure .path is non-null and non-empty by either changing the jq expression
to filter out nulls (e.g., select(.path != null) | .path) when setting
user_ns_path from container_info or add an explicit test that user_ns_path is
neither empty nor the string "null" before asserting; reference the variable
user_ns_path and the source container_info to locate and fix the check.
🧹 Nitpick comments (2)
test/cgroups.bats (2)
300-312: Preferjq -eassertions for mappings (more robust + clearer failures)You can replace the “!= null” string checks + separate hostID extraction with a single
jq -eassertion, and also verifycontainerIDandsize(helps catch partial/incorrect mappings).Proposed refactor
- uid_mappings=$(echo "$container_info" | jq -r '.info.runtimeSpec.linux.uidMappings') - gid_mappings=$(echo "$container_info" | jq -r '.info.runtimeSpec.linux.gidMappings') - [[ "$uid_mappings" != "null" ]] - [[ "$gid_mappings" != "null" ]] - - # Verify the mappings contain the expected values - uid_host_id=$(echo "$container_info" | jq -r '.info.runtimeSpec.linux.uidMappings[0].hostID') - gid_host_id=$(echo "$container_info" | jq -r '.info.runtimeSpec.linux.gidMappings[0].hostID') - [[ "$uid_host_id" == "100000" ]] - [[ "$gid_host_id" == "100000" ]] + echo "$container_info" | jq -e ' + .info.runtimeSpec.linux.uidMappings[0].hostID == 100000 and + .info.runtimeSpec.linux.uidMappings[0].containerID == 0 and + .info.runtimeSpec.linux.uidMappings[0].size == 65536 and + .info.runtimeSpec.linux.gidMappings[0].hostID == 100000 and + .info.runtimeSpec.linux.gidMappings[0].containerID == 0 and + .info.runtimeSpec.linux.gidMappings[0].size == 65536 + ' >/dev/null
313-316: Assert/proc/self/cgroupcontent (not just command success)Since you already require cgroup v2, explicitly assert the output contains a v2 entry (typically
0::...) and is non-empty, so failures are easier to diagnose.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/cgroups.bats
🧰 Additional context used
📓 Path-based instructions (1)
**/*.bats
📄 CodeRabbit inference engine (AGENTS.md)
Use
.batsfile extension for BATS integration test files
Files:
test/cgroups.bats
🧬 Code graph analysis (1)
test/cgroups.bats (1)
test/helpers.bash (3)
is_cgroup_v2(564-566)start_crio(232-236)crictl(86-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (32)
- GitHub Check: rpm-build:fedora-43-aarch64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-aarch64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-x86_64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-aarch64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-aarch64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-aarch64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-x86_64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-aarch64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-aarch64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-aarch64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-aarch64:fedora-rawhide
- GitHub Check: rpm-build:fedora-43-x86_64:fedora-rawhide
- GitHub Check: rpm-build:fedora-rawhide-x86_64:fedora-rawhide
- GitHub Check: rpm-build:centos-stream-9-x86_64:fedora-rawhide
- GitHub Check: binaries / amd64
- GitHub Check: binaries / arm64
- GitHub Check: unit / amd64 / root
- GitHub Check: security-checks
- GitHub Check: build static / amd64
- GitHub Check: unit / arm64 / root
- GitHub Check: build static / arm64
- GitHub Check: unit / amd64 / rootless
- GitHub Check: build static / s390x
- GitHub Check: build static / ppc64le
- GitHub Check: codeql-build
- GitHub Check: build
- GitHub Check: docs
- GitHub Check: lint
🔇 Additional comments (1)
test/cgroups.bats (1)
253-259: Skip gating looks reasonable; ensure it matches how userns is enabled in CILine 254 checks
CONTAINER_UID_MAPPINGSto detect “userNS already enabled globally”. If CI enables userns via a config file (not that env var), this might not skip when it should.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # Check that user namespace has a path (joining the sandbox's userns) | ||
| user_ns_path=$(echo "$container_info" | jq -r '.info.runtimeSpec.linux.namespaces[] | select(.type == "user") | .path') | ||
| [[ -n "$user_ns_path" ]] | ||
|
|
There was a problem hiding this comment.
Fix user namespace path assertion (current check can pass with "null")
jq -r prints null as the string "null", so [[ -n "$user_ns_path" ]] (Line 298) would still pass if .path is null. This weakens the “join by path” regression coverage.
Proposed fix
- user_ns_path=$(echo "$container_info" | jq -r '.info.runtimeSpec.linux.namespaces[] | select(.type == "user") | .path')
- [[ -n "$user_ns_path" ]]
+ user_ns_path=$(
+ echo "$container_info" | jq -r '.info.runtimeSpec.linux.namespaces[]? | select(.type == "user") | (.path // empty)'
+ )
+ [[ -n "$user_ns_path" ]]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check that user namespace has a path (joining the sandbox's userns) | |
| user_ns_path=$(echo "$container_info" | jq -r '.info.runtimeSpec.linux.namespaces[] | select(.type == "user") | .path') | |
| [[ -n "$user_ns_path" ]] | |
| # Check that user namespace has a path (joining the sandbox's userns) | |
| user_ns_path=$( | |
| echo "$container_info" | jq -r '.info.runtimeSpec.linux.namespaces[]? | select(.type == "user") | (.path // empty)' | |
| ) | |
| [[ -n "$user_ns_path" ]] | |
🤖 Prompt for AI Agents
In `@test/cgroups.bats` around lines 296 - 299, The assertion for the user
namespace path is weak because jq -r can emit the literal string "null" so the
current check using user_ns_path and [[ -n "$user_ns_path" ]] can pass
incorrectly; update the extraction/check to ensure .path is non-null and
non-empty by either changing the jq expression to filter out nulls (e.g.,
select(.path != null) | .path) when setting user_ns_path from container_info or
add an explicit test that user_ns_path is neither empty nor the string "null"
before asserting; reference the variable user_ns_path and the source
container_info to locate and fix the check.
|
@bitoku While I don't work with CRI-O directly, just through various Kubernetes distributions, I've been running systemd-based containers and GitHub Actions in https://github.com/freeipa/freeipa-container for years. Let me know if I can help getting that high-level use case (for example with |
|
@adelton |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9712 +/- ##
==========================================
- Coverage 67.58% 67.51% -0.08%
==========================================
Files 209 209
Lines 29075 29074 -1
==========================================
- Hits 19650 19628 -22
- Misses 7746 7772 +26
+ Partials 1679 1674 -5 🚀 New features to boost your workflow:
|
|
/test ci-e2e-conmonrs |
|
/lgtm The fix was confirmed though we still need the continuous way to test it. |
|
@saschagrunert: new pull request created: #9713 DetailsIn response to this:
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-sigs/prow repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
When containers join a user namespace via path, UID/GID mappings must still be included in the OCI spec. While the namespace path indicates which namespace to join, the mappings are essential for the runtime to properly handle file ownership operations, particularly cgroup delegation.
Without these mappings, runtimes cannot chown the cgroup directory to the mapped UID 0, causing systemd containers to fail with "Permission denied" when attempting to create cgroups like /init.scope.
This restores the behavior from v1.34.4 where both namespace path and mappings coexisted successfully.
Which issue(s) this PR fixes:
Fixes #9705
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.