Fix user namespace validation for containers in pods #27413
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 0xDVC 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 |
0xDVC
force-pushed
the
fix-26848-userns-pod-validation
branch
from
6059c1f to
5a7b73a
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
2 similar comments
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
0xDVC
force-pushed
the
fix-26848-userns-pod-validation
branch
9 times, most recently
from
88b5145 to
d5700bc
Compare
libpod/container_validate.go
Outdated
| // Linux requires containers sharing network or IPC namespaces (like in a pod) to use the same | ||
| // user namespace. Trying to use different ones will fail at the kernel level. | ||
| // Most of this is already checked in pkg/specgen/generate/namespaces.go before pod containers drop their ID mappings. | ||
| func (c *Container) validateUserNSInPod() error { |
There was a problem hiding this comment.
this is not my area of expertise, but what am I missing here? All conditions including the default return nil?
There was a problem hiding this comment.
good catch, totally missed removing that. Earlier in development, I was focusing on libpod/container_validate.go as suggested when I started work. I realized that by the time that call runs, the ID mappings had already been dropped (in namespaces.go lines 265-269), so I would have missed catching the error. namespaces.go is where the actual validation is happening.
removing it now.
0xDVC
force-pushed
the
fix-26848-userns-pod-validation
branch
3 times, most recently
from
ff9fe1c to
181f6ca
Compare
Remove incomplete CLI validation that only checked --pod flag and missed --pod-id-file (used by quadlet). Move validation to libpod/container_validate.go to catch all cases where --userns is set with --pod. The new validation checks if container's ID mappings differ from the pod's infra container and returns a clearer error message: 'cannot set user namespace mappings that differ from pod' This addresses the issue request for a better error message that explains the kernel limitation more clearly. Fixes: containers#26848 Signed-off-by: 0xdvc <[email protected]>
0xDVC
force-pushed
the
fix-26848-userns-pod-validation
branch
from
181f6ca to
502a145
Compare
|
Tests are unhappy, I'm not sure what's what with them. I'm going to try a rerun of the failed ones. |
Literally😂. For most, quite a learning experience. The rest? Got me at a choke hold. |
3 participants
Remove incomplete CLI validation that only checked --pod flag and missed --pod-id-file (used by quadlet). Move validation to libpod/container_validate.go to catch all cases where --userns is set with --pod.
The new validation checks if container's ID mappings differ from the pod's
infra container and returns a clearer error message:
'cannot set user namespace mappings that differ from pod'
Fixes: #26848
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?