Repository navigation
Add User Namespaces - #977
Conversation
Reviewer's GuideAdds qm-specific system users and user-namespace mappings via a systemd sysusers config and RPM post scripts, wires the sysusers file into the package, and updates SELinux-related RPM dependencies while removing an obsolete selinux build requirement. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces systemd sysusers configuration (qm-sysusers.conf) to manage system users and groups, configures the container to run in the qm_root user namespace, and cleans up the obsolete comment-tz-local workaround. Feedback on these changes suggests explicitly running systemd-sysusers in the RPM %post scriptlet to support older RPM versions, replacing bashisms with POSIX-compliant redirection in %preun, correcting a typo in the container configuration comments, and explicitly defining GIDs for all users in the sysusers configuration to avoid ambiguity.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
4f1448d to
75dd758
Compare
|
After doing some builds with aib, I figured out that the tz workaround seems to be still needed - so I dropped that commit from this PR. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The spec file comment explicitly says to keep
Versionat 0 for upstream and let Packit set it; bumping it to1.1may break that workflow and should be double-checked or reverted. - You added
Requires(post): shadow-utilsbutusermodis also used in%preun; consider addingRequires(preun): shadow-utils(or a generic scriptlet dependency) to avoid uninstall failures on systems lacking it. - The
%postscript unconditionally runssystemd-sysusers; consider guarding this with a fallback (e.g.command -v systemd-sysusers >/dev/null && systemd-sysusers || :) so that a missing or failing binary doesn’t abort the entire post-install.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The spec file comment explicitly says to keep `Version` at 0 for upstream and let Packit set it; bumping it to `1.1` may break that workflow and should be double-checked or reverted.
- You added `Requires(post): shadow-utils` but `usermod` is also used in `%preun`; consider adding `Requires(preun): shadow-utils` (or a generic scriptlet dependency) to avoid uninstall failures on systems lacking it.
- The `%post` script unconditionally runs `systemd-sysusers`; consider guarding this with a fallback (e.g. `command -v systemd-sysusers >/dev/null && systemd-sysusers || :`) so that a missing or failing binary doesn’t abort the entire post-install.
## Individual Comments
### Comment 1
<location path="rpm/qm.spec" line_range="149-151" />
<code_context>
+systemd-sysusers
+# Add user namespace entries to /etc/subuid and /etc/subgid for the qm_root user
+#
+if ! grep -q "^qm_root:" /etc/subuid 2>/dev/null; then
+ usermod --add-subuids 1000000000-2500000000 qm_root
+fi
+if ! grep -q "^qm_root:" /etc/subgid 2>/dev/null; then
+ usermod --add-subgids 1000000000-2500000000 qm_root
</code_context>
<issue_to_address>
**issue (bug_risk):** Make `usermod --add-subuids/--add-subgids` failures non-fatal to avoid breaking installs.
These `%post` `usermod` calls will currently abort the RPM transaction on any failure (e.g., locked `/etc/subuid`, missing `qm_root`, concurrent edits), whereas `%postun` already treats similar failures as non-fatal with `|| :`. Please apply the same pattern here so install/upgrade doesn’t fail due to non-critical `usermod` issues:
```sh
if ! grep -q '^qm_root:' /etc/subuid 2>/dev/null; then
usermod --add-subuids 1000000000-2500000000 qm_root || :
fi
if ! grep -q '^qm_root:' /etc/subgid 2>/dev/null; then
usermod --add-subgids 1000000000-2500000000 qm_root || :
fi
```
</issue_to_address>
### Comment 2
<location path="rpm/qm.spec" line_range="150" />
<code_context>
+# Add user namespace entries to /etc/subuid and /etc/subgid for the qm_root user
+#
+if ! grep -q "^qm_root:" /etc/subuid 2>/dev/null; then
+ usermod --add-subuids 1000000000-2500000000 qm_root
+fi
+if ! grep -q "^qm_root:" /etc/subgid 2>/dev/null; then
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing reusable macros (and optionally a helper script) for the qm_root user and subuid/subgid range to avoid duplicated scriptlet logic and centralize these parameters.
You can reduce the duplication and make future changes less error‑prone by parameterizing the user and range, and optionally encapsulating the shell logic.
### 1. Define macros for the user and range
Near the top of the spec, define reusable macros:
```spec
%global qm_root_user qm_root
%global qm_root_subrange 1000000000-2500000000
```
Then use them in `%post` and `%postun`:
```spec
%post
...
systemd-sysusers
# Add user namespace entries to /etc/subuid and /etc/subgid for the qm_root user
if ! grep -q "^%{qm_root_user}:" /etc/subuid 2>/dev/null; then
usermod --add-subuids %{qm_root_subrange} %{qm_root_user}
fi
if ! grep -q "^%{qm_root_user}:" /etc/subgid 2>/dev/null; then
usermod --add-subgids %{qm_root_subrange} %{qm_root_user}
fi
```
```spec
%postun
if [ $1 -eq 0 ]; then
...
if grep -q "^%{qm_root_user}:" /etc/subuid 2>/dev/null; then
usermod --del-subuids %{qm_root_subrange} %{qm_root_user} 2> /dev/null || :
fi
if grep -q "^%{qm_root_user}:" /etc/subgid 2>/dev/null; then
usermod --del-subgids %{qm_root_subrange} %{qm_root_user} 2> /dev/null || :
fi
fi
```
This keeps the behavior but centralizes the “qm_root + 1000000000‑2500000000” parameters.
### 2. Optional: move subuid/subgid logic into a helper script
If you want to further simplify the spec scriptlets, you can ship a tiny helper script and call it from `%post`/`%postun`:
```sh
# in, e.g., tools/qm-subids.sh (installed to %{_datadir}/qm/qm-subids.sh)
#!/bin/sh
set -eu
user=${1:-qm_root}
range=${2:-1000000000-2500000000}
action=${3:-add} # "add" or "del"
if [ "$action" = "add" ]; then
if ! grep -q "^${user}:" /etc/subuid 2>/dev/null; then
usermod --add-subuids "${range}" "${user}"
fi
if ! grep -q "^${user}:" /etc/subgid 2>/dev/null; then
usermod --add-subgids "${range}" "${user}"
fi
elif [ "$action" = "del" ]; then
if grep -q "^${user}:" /etc/subuid 2>/dev/null; then
usermod --del-subuids "${range}" "${user}" 2>/dev/null || :
fi
if grep -q "^${user}:" /etc/subgid 2>/dev/null; then
usermod --del-subgids "${range}" "${user}" 2>/dev/null || :
fi
fi
```
Install it and use in the spec:
```spec
%install
...
install -m 0755 tools/qm-subids.sh %{buildroot}%{_datadir}/qm/qm-subids.sh
```
```spec
%post
...
systemd-sysusers
%{_datadir}/qm/qm-subids.sh %{qm_root_user} %{qm_root_subrange} add
```
```spec
%postun
if [ $1 -eq 0 ]; then
...
%{_datadir}/qm/qm-subids.sh %{qm_root_user} %{qm_root_subrange} del
fi
```
This keeps the spec file shorter and centralizes the branching logic, while preserving the current functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
a436ceb to
9bf3371
Compare
| g qm_systemd-coredump 1000000997 | ||
| g qm_render 1000000998 | ||
| g qm_input 1000000999 | ||
| g qm_nobody 1000065534 |
There was a problem hiding this comment.
While I agree with this idea, i wonder if its correct to hardcode this list in the general qm package. I mean, it is specific to the cs10 user list. Maybe it could be used as a base, and then we replace it in a-i-b with a generated one based on the actual qm users?
There was a problem hiding this comment.
Maybe what we want is to add some tool to qm to generate this, and then we'd have a-i-b launch that at build time?
There was a problem hiding this comment.
Yes, a tool for generating the sysusers conf makes sense. There is already the setup script shipped with QM - to bootstrap QM in non-aib environments. So we could do something similar and then invoke it via a custom osbuild-auto stage org.osbuild.qm.userns in aib.
It should be
- able to get the users from inside of QM (which should always be
/etc/qm/passwdwith current quadlet configuration) - add the subuid/subgid to it as an offset
- and save it as a sysusers config in the root partition.
There was a problem hiding this comment.
It probably makes sense to move the usermod for the subuid/subgid assignment into that script as well - then its also not hard-coded.
There was a problem hiding this comment.
Decided to completely isolate the userns support in the new tool (the userns bash script). This way we do not break anything, can use and refine it. And when we are certain nothing breaks, we can make it the default.
fda4629 to
acbf25f
Compare
The support for userns is done via a bash script which sets up the proper systemd-sysusers configuration based on the passwd and group files from the mounted /etc. It also generates the QM drop-in setting the SubUIDMAP= and SUBGIDMap= fields set and adds the desired range of UIDs/GIDs to the subid files. This tool needs to be invoked manually, so userns are disabled by default so to not break anything and can be activated on demand. Signed-off-by: Michael Engel <mengel@redhat.com>
Co-Authored-By: Claude Signed-off-by: Michael Engel <mengel@redhat.com>
Signed-off-by: Michael Engel <mengel@redhat.com>
Yarboa
left a comment
Yarboa
left a comment
There was a problem hiding this comment.
LGTM
Opened ticket for CI failures
I would expect userns integration tests other then unittest
Thanks. |
|
LGTM but have small question |
4 participants
This PR adds the support for running QM with User Namespaces. For this, it
SubUIDMapandSubGIDMapto the quadlet,/etc/subuidand/etc/subgidduring the post-install phase of the RPM installIt also removes the tz workaround since the issues to fix it are resolved:Gap discovered for installing QM in OSTREE image type, during setup #367qm - ostree image cannot start qm service - /etc/localtime is readonly #394Summary by Sourcery
Add qm-specific system users and user namespace mappings to support running QM with user namespaces, and clean up obsolete timezone workaround tooling.
Enhancements:
Build:
Chores:
Summary by Sourcery
Add packaging support for QM to run with dedicated user and group namespaces managed via systemd-sysusers and subuid/subgid mappings.
New Features:
Enhancements: