fix(kube): gate SELinux volume warning note on host SELinux state#29230
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates podman kube generate (ABI/local implementation) to avoid emitting the SELinux volume-permissions NOTE comment unless the host reports SELinux as enabled, reducing noise in generated Kubernetes YAML on non-SELinux systems.
Changes:
- Import
github.com/opencontainers/selinux/go-selinuxin the ABI kube generator. - Gate rendering of the SELinux volume warning NOTE on
selinux.GetEnabled()in addition tolen(po.Spec.Volumes) != 0.
Comments suppressed due to low confidence (1)
pkg/domain/infra/abi/generate.go:226
- The NOTE text is explicitly scoped to “unprivileged and rootless” usage, but the new gating only checks host SELinux state. On SELinux-enabled hosts this will still emit the NOTE for rootful/privileged runs, which makes the rendered output misleading/noisy.
Consider either (a) also gating the NOTE on rootless/unprivileged state, or (b) rewording the NOTE to match the broader condition.
if len(po.Spec.Volumes) != 0 && selinux.GetEnabled() {
warning := `
# NOTE: If you generated this yaml from an unprivileged and rootless podman container on an SELinux
# enabled system, check the podman generate kube man page for steps to follow to ensure that your pod/container
# has the right permissions to access the volumes added.
| return nil, err | ||
| } | ||
| if len(po.Spec.Volumes) != 0 { | ||
| if len(po.Spec.Volumes) != 0 && selinux.GetEnabled() { |
There was a problem hiding this comment.
Fixed in the latest amended commit by coupling rootless.IsRootless() and introducing native regression tests in generate_test.go.
fab9048 to
ecf3bf8
Compare
Luap99
left a comment
There was a problem hiding this comment.
As a more general not I do not know why the man page and the warning special cases rootless podman, selinux labelling applies to the rootful containers in the same way but I guess this is unrelated to this change itself
| // selinuxGetEnabled and rootlessIsRootless are indirections over selinux.GetEnabled | ||
| // and rootless.IsRootless so tests can override them. | ||
| var ( | ||
| selinuxGetEnabled = selinux.GetEnabled | ||
| rootlessIsRootless = rootless.IsRootless | ||
| ) |
There was a problem hiding this comment.
I don't see the point in doing this it just complicated the code a bit for no benefit unit testing two bool conditions is rather pointless.
There was a problem hiding this comment.
Removed the vars and the helper, just inlined the check directly now.
|
|
||
| import "testing" | ||
|
|
||
| func TestShouldWarnSELinuxVolumes(t *testing.T) { |
There was a problem hiding this comment.
I would just delete the test, testing a one line boolean condition like that does notto really seem to help us.
Instead we should just have a e2e test case here test/e2e/generate_kube_test.go, ideally instead of adding yet another case there just piggy back onto an existing generate command with volumes, i.e. in "with volume"
and then just check the string is in the output only as rootless user and when selnix is enabled.
There was a problem hiding this comment.
ok, i have dropped the unit test and added the check to the existing "with volume" e2e case instead — it verifies the note only shows up when running rootless with selinux enabled.
ecf3bf8 to
dbbfab8
Compare
I noticed that too while digging into it. The man page only mentions rootless so I kept the behavior matching that for now, didn't want to change the scope here. Can open a separate issue for the rootful case if you think it's worth looking into. |
|
Thanks for the review @Luap99. Made both changes — inlined the condition and swapped the unit test for e2e coverage in the "with volume" case. Let me know if there's anything else. |
| if isRootless() && selinux.GetEnabled() { | ||
| Expect(string(b)).To(ContainSubstring("check the podman generate kube man page")) | ||
| } else { | ||
| Expect(string(b)).NotTo(ContainSubstring("check the podman generate kube man page")) | ||
| } |
There was a problem hiding this comment.
The assertion was sitting on the pod-generation path where the note isn't emitted — moved it to a new case that generates from a standalone container with a volume, so the gating actually gets exercised now.
The SELinux volume-permissions NOTE only applies to unprivileged, rootless containers on an SELinux-enabled host. Emit it only when both conditions hold, instead of on every volume-bearing object. Add an e2e case generating from a standalone container with a volume, asserting the NOTE appears only when rootless and SELinux is enabled. Fixes: podman-container-tools#17743 Signed-off-by: i-OmSharma <sharmaom1201@gmail.com>
dbbfab8 to
cfd3a3f
Compare
|
@Luap99 heads up — the existing "with volume" case generates from a pod, and the note only emits on the container path, so I added a separate container-based case for the assertion instead of piggybacking on that one. Everything else is as you suggested. |
|
LGTM |
| session := podmanTest.Podman([]string{"run", "-d", "--name", ctrName, "-v", vol1 + ":/volume/:z", CITEST_IMAGE, "top"}) | ||
| session.WaitWithDefaultTimeout() | ||
| Expect(session).Should(ExitCleanly()) |
There was a problem hiding this comment.
nit: no need to run the container, create should suffice:
| session := podmanTest.Podman([]string{"run", "-d", "--name", ctrName, "-v", vol1 + ":/volume/:z", CITEST_IMAGE, "top"}) | |
| session.WaitWithDefaultTimeout() | |
| Expect(session).Should(ExitCleanly()) | |
| session := podmanTest.PodmanExitCleanly([]string{"create", "--name", ctrName, "-v", vol1 + ":/volume/:z", CITEST_IMAGE}) |
Fixes #17743
podman kube generatealways printed a NOTE about SELinux volumepermissions whenever a pod had volumes, even on non-SELinux hosts or
rootful setups. This added noise for most users.
This gates the warning so it only fires when it's actually relevant:
volumes exist AND SELinux is enabled on the host AND podman is running
rootless. The condition is inlined directly at the call site.
Coverage is added to the existing "with volume" e2e case in
generate_kube_test.go, checking the note shows up only when running
rootless with selinux enabled.