Repository navigation
fix: k8s volume ownership for non-root services - #1502
Syed-Ali-Abbas-568 wants to merge 5 commits into
Conversation
Faraz32123
left a comment
There was a problem hiding this comment.
Thanks for digging into this. The diagnosis matches what we saw on #1304, and checking the volume's group rather than its owner is a nice touch for volumes where fsGroup works. A few things before this can go in:
Requested changes
- Root init container on every k8s deployment. As written, every operator gets a
runAsUser: 0init container on their mongodb/mysql/redis/meilisearch pods, including clusters on EBS/GCE PD/Azure Disk wherefsGroupalready works. Clusters enforcing Pod Security Admissionrestricted(or Kyverno/Gatekeeper policies requiringrunAsNonRoot) will reject these pods, so a fix for Minikube/kind could take down the databases on production clusters. Could this be opt-in, e.g. aK8S_FIX_VOLUME_PERMISSIONSsetting (defaultfalse) that wraps theinitContainersblock (and the ConfigMap), documented for Minikube/kind users? - Test plan. The live
kindrepro andmake test-unit test-format test-typesare still unchecked, and the description says "marking as draft", but the PR isn't a draft. Could you confirm the live repro before review?
Nits
- Partial chown after a timeout. I believe busybox
chown -Rchanges the top directory before recursing (worth confirming). If the 60stimeoutkills it partway, the top directory already has the new gid, so every later restart reports "nothing to do" and the volume stays partly root-owned. Changing ownership of the contents first and the top directory last (or checking more than the top directory) would avoid that. - Hidden failure. On failure the script exits 0, so the main container then crashes with the original "Permission denied" and the real cause is only in the init container logs. Fine as a design choice, but worth a line in the k8s troubleshooting docs.
- Changelog / air-gapped clusters. The changelog should say that init containers are added to all k8s database deployments (not only Minikube/kind ones), and that air-gapped clusters will also need to mirror
DOCKER_IMAGE_PERMISSIONS.
Question: target branch. This is a bug fix, but it currently targets main. Per the contributing guidelines, fixes with no regression risk go to release and changes that could disrupt production operators go to main. Was main deliberate because of the risk in (1)? If it becomes opt-in, would it make sense to retarget release so current users get the fix?
|
/smoke |
|
Smoke tests requested by @Syed-Ali-Abbas-568, on de58d43:
Plugins: |
de58d43 to
c8cb75c
Compare
…e in CI Addresses review feedback from @Faraz32123 on overhangio#1502, plus the k8s smoke test failure on that PR. - The init container now runs with runAsNonRoot + a single added CHOWN capability instead of runAsUser: 0. This was the main requested change: a root init container would get rejected outright on clusters that enforce a non-root policy (via Kyverno/Gatekeeper or PSA restricted), taking down the databases on exactly the production clusters this fix must not break. Non-root + CAP_CHOWN satisfies that class of policy without a new opt-in setting. Caveat: I could not fully verify in this environment that Kubernetes propagates the added capability into the non-root process's effective set the way plain `docker run` does not by default - needs confirming on an actual cluster. The failure mode if it doesn't is safe either way: the chown just fails harmlessly, same as the pre-existing bug, not a new one. - Documented (not changed) why the partial-chown-after-timeout concern doesn't apply as raised: verified via strace that busybox `chown -R` processes children before the parent directory, so an interrupted run leaves the top-level directory's group stale, which is exactly what the "is this already done" check inspects. An interrupted run still correctly reads as not-done on the next restart. - CI: smoke-k8s.yml never preloaded DOCKER_IMAGE_PERMISSIONS into kind (only the `images` workflow input list is preloaded, and this image was never in it before this PR). That means kubelet pulls it from the registry cold on every one of the four new init containers' first run - a very likely contributor to the smoke test's observed ~17 minutes of job retries before the LMS/CMS 502s. Now appended unconditionally to IMAGES so it's always built and kind-loaded like every other image the k8s path depends on. - Changelog fragment reworded per review: clarifies the init container is added to all four deployments unconditionally (not only Minikube/kind), and that air-gapped clusters must now mirror the `openedx-permissions` image for k8s too. - docs/troubleshooting.rst: added a short entry pointing at the fix-permissions init container's logs, since a failed/timed-out fix is non-fatal by design and the main container then crashes with the original, less informative permission error. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…ices MongoDB (and MySQL, Redis, Meilisearch) run as non-root but the PersistentVolume directory a storage provisioner hands them is often root-owned. The pod-level fsGroup is meant to bridge that gap, but it is silently skipped on volume types that don't support it (hostPath / local-path, i.e. what Minikube and kind use), so the pod crashes on first launch with a permission error. Add a root init container per affected service that checks whether the volume's group already matches the service's gid and, if not, chowns it. Volumes where fsGroup already did its job (real cloud block storage) are left untouched; a failed chown logs a warning instead of blocking the pod. Fixes overhangio#1304 Co-Authored-By: Claude Sonnet 5 <[email protected]>
Replaces the per-service inline shell one-liners with a single script (tutor/templates/apps/permissions/fix-volume-permissions.sh) delivered via kustomize's configMapGenerator, the same mechanism already used for redis.conf and the Caddyfile. All four init containers now call the same script instead of duplicating the check-then-chown logic four times. Also wraps the chown in `timeout 60` so a hung chown (e.g. a stuck NFS mount) fails cleanly after a minute instead of blocking the pod forever in Init:0/1 — the previous version only handled chown returning an error, not chown hanging. Verified against the actual alpine:3.13.6 base image (docker run): fresh root-owned volume gets fixed recursively, an already-correct root:<gid> volume (simulating a working cloud fsGroup) is left completely untouched (mtime and ownership unchanged), and a simulated hang is killed at ~61s instead of running indefinitely, with the script still exiting 0. Also verified both templates render to valid, correctly-structured YAML via the real Jinja/PyYAML toolchain, with every RUN_* flag combination on and off. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…e in CI Addresses review feedback from @Faraz32123 on overhangio#1502, plus the k8s smoke test failure on that PR. - The init container now runs with runAsNonRoot + a single added CHOWN capability instead of runAsUser: 0. This was the main requested change: a root init container would get rejected outright on clusters that enforce a non-root policy (via Kyverno/Gatekeeper or PSA restricted), taking down the databases on exactly the production clusters this fix must not break. Non-root + CAP_CHOWN satisfies that class of policy without a new opt-in setting. Caveat: I could not fully verify in this environment that Kubernetes propagates the added capability into the non-root process's effective set the way plain `docker run` does not by default - needs confirming on an actual cluster. The failure mode if it doesn't is safe either way: the chown just fails harmlessly, same as the pre-existing bug, not a new one. - Documented (not changed) why the partial-chown-after-timeout concern doesn't apply as raised: verified via strace that busybox `chown -R` processes children before the parent directory, so an interrupted run leaves the top-level directory's group stale, which is exactly what the "is this already done" check inspects. An interrupted run still correctly reads as not-done on the next restart. - CI: smoke-k8s.yml never preloaded DOCKER_IMAGE_PERMISSIONS into kind (only the `images` workflow input list is preloaded, and this image was never in it before this PR). That means kubelet pulls it from the registry cold on every one of the four new init containers' first run - a very likely contributor to the smoke test's observed ~17 minutes of job retries before the LMS/CMS 502s. Now appended unconditionally to IMAGES so it's always built and kind-loaded like every other image the k8s path depends on. - Changelog fragment reworded per review: clarifies the init container is added to all four deployments unconditionally (not only Minikube/kind), and that air-gapped clusters must now mirror the `openedx-permissions` image for k8s too. - docs/troubleshooting.rst: added a short entry pointing at the fix-permissions init container's logs, since a failed/timed-out fix is non-fatal by design and the main container then crashes with the original, less informative permission error. Co-Authored-By: Claude Sonnet 5 <[email protected]>
c8cb75c to
6598c8d
Compare
|
/smoke |
|
Smoke tests requested by @Syed-Ali-Abbas-568, on 6598c8d:
Plugins: |
Faraz32123
left a comment
There was a problem hiding this comment.
Thanks for the updates! Retargeting release, the post-order chown, the troubleshooting docs and the changelog all look good.
One concern with the new non-root approach: Kubernetes doesn't support ambient capabilities (kubernetes/kubernetes#56374), so capabilities.add: ["CHOWN"] on a container running as uid 999/1000 is dropped at execve: chown should fail with EPERM on a root-owned volume, and the script then just logs the warning. I don't think the green k8s smoke test covers this, since kind's local-path volumes are created 0777 and the services start either way. Could you do the live repro from the test plan (volume reset to root:root 0755 on Minikube/kind) and confirm the init container actually changes ownership?
If it does need root, I'd still suggest making the init container opt-in (e.g. K8S_FIX_VOLUME_PERMISSIONS, default false, documented for Minikube/kind), since PSA restricted rejects both root containers and added capabilities. That also makes release an easier target.
Could you also update the PR description? It still mentions a root init container, the draft status and unchecked test items.
…ies.add @Faraz32123 was right: verified on an actual kind cluster that capabilities.add: ["CHOWN"] on a non-root container is silently dropped at execve (Kubernetes does not support ambient capabilities, see kubernetes/kubernetes#56374) - chown failed with EPERM on a root-owned volume, so the init container as shipped did not actually fix anything. Fix: setcap cap_chown=ep on /bin/busybox in the permissions image at build time. A file capability is a property of the binary, not the pod spec, so it is unaffected by the ambient-capability gap, and - also verified empirically, since it surprised me - still works with allowPrivilegeEscalation: false. Verified end-to-end against a real kind cluster using the actual fix-volume-permissions.sh script and the exact pod securityContext from deployments.yml (runAsNonRoot, capabilities: add [CHOWN] drop [ALL]): a fresh root-owned volume gets fixed, and a volume already at root:<gid> (simulating a working fsGroup) is left untouched, same as before. This does mean the published openedx-permissions image needs rebuilding for the fix to take effect - already-mirrored copies predate the setcap call. Co-Authored-By: Claude Sonnet 5 <[email protected]>
|
Hi @Faraz32123 , I tested this on a real cluster and capabilities.add definitely drops at execve when running as non-root. Because of that, chown was just failing silently this whole time. To fix it, I updated the image build to use setcap cap_chown=ep on busybox instead. Since it's a file capability, it completely avoids the ambient-cap issue. It does mean we need an image rebuild for it to take effect, which I've noted in the PR. I just re-ran the script on a kind cluster with this fix, and it works perfectly: it fixes fresh root-owned volumes, ignores ones that are already correct, and surprisingly, it still works even with allowPrivilegeEscalation: false. I decided to skip adding the opt-in flag, since running non-root with this capability should be enough to satisfy Kyverno/Gatekeeper. It still won't pass a fully restricted PSA profile (since that only allows NET_BIND_SERVICE), but our current setup doesn't either, so no regressions there. I've updated the PR description with all of this as well. |
Faraz32123
left a comment
There was a problem hiding this comment.
Thanks, the file-capability approach makes sense, and the allowPrivilegeEscalation: false result checks out: no_new_privs only caps the file caps at the pre-exec permitted set, which still contains CHOWN thanks to capabilities.add, so both pieces are needed. I'm fine with skipping the opt-in given the init container is now non-root and baseline-compatible.
Before approving:
- Could you re-run
/smokeon bb77a91? The last run was on 6598c8d, before the image change. - Could you tick the full
tutor k8s launchrepro (fresh mongodb volume reset toroot:root0755)? kind's local-path volumes are 0777, so the green smoke test doesn't really exercise this fix.
Nits:
setcapon/bin/busyboxgives CAP_CHOWN to every applet in the image (shincluded) whenever the pod allows it. Setting it on a dedicated copy (e.g./usr/local/bin/chown-busybox) and calling that from the script would narrow it to the chown.- File caps live in the
security.capabilityxattr, which some mirroring/re-layering tools strip. Worth a sentence next to the air-gapped note in the changelog, since that would silently bring back the original crash.
|
/smoke |
|
Smoke tests requested by @Syed-Ali-Abbas-568, on bb77a91:
Plugins: |
…risk Addresses the two nits from @Faraz32123's latest review. Attempted his suggestion of scoping cap_chown to a dedicated copy of busybox used only for the chown step, so sh/timeout/etc. wouldn't carry it. Verified on an actual kind cluster that this does not work: with allowPrivilegeEscalation: false, a file capability can only elevate a process to what it already had in its own pre-exec permitted set, and that's empty for anything other than the container's single direct entrypoint exec. fix-volume-permissions.sh runs as a shell script, so `sh` is that one exec; chown is a separate, later exec from inside an already-unprivileged process regardless of which binary it targets. Reverted to setcap on /bin/busybox itself, which was already proven working (same file for sh/timeout/chown, so the capability carries through the whole chain) - confirmed again after reverting. Also added a line to the changelog's air-gapped note: the capability lives in a security.capability xattr that some image mirroring/ re-layering tools strip, which would silently bring back the original crash with no obvious cause. Mirror by digest or with a tool that preserves xattrs. Co-Authored-By: Claude Sonnet 5 <[email protected]>
|
I tested out the dedicated binary approach on a real cluster, but unfortunately, it's still throwing EPERM errors. Given how our script is structured, I don't think this method is actually going to work for us. The issue comes down to allowPrivilegeEscalation: false. With that set, a file capability can only elevate a process up to whatever was already in its permitted set before the exec. Because our script runs through sh, by the time it calls chown, it's doing a second exec from a shell that's already unprivileged. Its permitted set is completely empty, so it doesn't matter which binary we point it to—there are no privileges left to grant. I ended up reverting back to running setcap directly on /bin/busybox. I re-tested it and everything is working smoothly again. I also made sure to add the xattr warning to the changelog! |
|
/smoke |
|
Smoke tests requested by @Syed-Ali-Abbas-568, on 1936c59:
Plugins: |
Faraz32123
left a comment
There was a problem hiding this comment.
Thanks for the thorough follow-up! Everything I asked for is covered:
/smokepasses on 1936c59 (local and k8s), and the fulltutor k8s launchrepro on a freshroot:root0755 mongodb volume is done, including the no-oproot:999case.- The explanation for keeping the capability on
/bin/busyboxmakes sense: withallowPrivilegeEscalation: false, the laterchownexec can only keep what theshprocess already holds, soshitself has to be the capable file. Thanks for documenting that in the Dockerfile. - The changelog now covers the
security.capabilityxattr risk for mirrored images.
LGTM 👍
Summary
tutor k8s launchcrashes the MongoDB pod on a fresh volume:Root cause: MongoDB (and MySQL, Redis, Meilisearch) run as non-root, but the PersistentVolume directory a storage provisioner hands them is root-owned. The pod-level
fsGroupis meant to bridge that gap, but it's silently skipped on volume types that don't implement it — hostPath / local-path, i.e. what Minikube and kind use. Real cloud block storage (EBS, Azure Disk, GCP PD) implementsfsGroupcorrectly, which is why this has sat open without affecting production users.Builds on the diagnosis @HammadYousaf01 posted on the issue.
What this does
Adds a non-root init container to each of the four affected Deployments (mongodb, mysql, redis, meilisearch) that fixes volume ownership before the main container starts:
tutor/templates/apps/permissions/fix-volume-permissions.sh, delivered via kustomize'sconfigMapGenerator— the same mechanism already used forredis.confand theCaddyfile.fsGroupalready did its job (real cloud storage ends uproot:<gid>), this is a no-op. This matters because the owner never changes on those volumes — an owner-based check would recursivelychowna live, working production volume on every single pod restart.runAsNonRoot: true), not root. It gets the ability tochownvia a file capability baked into thepermissionsimage at build time (setcap cap_chown=ep /bin/busybox), not via the pod'scapabilities.add— Kubernetes does not support ambient capabilities (see kubernetes/kubernetes#56374), so a pod-level capability grant alone is silently dropped for a non-root container. This was caught in review — the first version of this PR did usecapabilities.addalone and didn't actually work. A file capability is a property of the binary rather than the pod spec, so it's unaffected by that gap, and keeps working withallowPrivilegeEscalation: false. It has to be set on/bin/busyboxitself, not a narrower dedicated copy used only for the chown step — also tried and reverted, see commit history for why. This means theopenedx-permissionsimage needs to be rebuilt for this fix to take effect — already-mirrored or already-pulled copies predate thesetcapcall, and the capability lives in asecurity.capabilityxattr that some image re-layering tools strip silently.timeout 60, and any failure just logs a warning instead of blocking the pod. This bounds a merely-slow/busy chown; it can't do anything about a process stuck in an uninterruptible kernel I/O wait (e.g. a truly dead NFS server) — no userspace mechanism can preempt that. The existing non-k8spermissionsfix has no timeout at all today, so this is strictly additive protection, not a complete guarantee.Known limitation found during testing
The fix fully resolves the reported bug: a fresh, empty volume (the actual precondition in #1304 — a provisioner hands over a brand-new directory before mongod has created anything in it). It does not fully resolve a different scenario: a volume that already has mongod-created files from a prior successful run (e.g.
journal/,diagnostic.data/, both0700) whose ownership later gets reset to root while those subdirectories keep their restrictive mode.CAP_CHOWNpermits changing a file's owner regardless of current ownership, but it doesn't grant permission to traverse into a directory you can't read — so the recursive chown fixes the top-level directory but can't descend into those two, and mongod still fails. This is a narrower, different precondition than the reported issue (it requires a volume that already has data, not a fresh one), so I didn't want to silently expand this PR's scope to fix it without flagging it first — happy to take a pass at it if that's wanted.Test plan
kindcluster, using the actualfix-volume-permissions.shscript and the exact podsecurityContextfromdeployments.yml(runAsNonRoot,capabilities: {add: [CHOWN], drop: [ALL]},allowPrivilegeEscalation: false): a fresh root-owned volume gets fixed, and a volume already atroot:<gid>(simulating a workingfsGroup) is left untouched — same mtime, same ownership.RUN_*flag on and off — valid YAML, correctinitContainers/volumeswiring, no regressions when all four services are disabled.tutor k8s launchitself, on a real kind cluster, installing this branch and building/loading the updatedpermissionsimage: scaled mongodb down, reset its PVC directory on the node to a fresh, emptyroot:root0755 (the issue's actual precondition), scaled back up — pod reaches1/1 Running,mongod startup completein the logs, no permission errors. Also reconfirmed the no-op case (root:999) in this same real deployment: ownership left unchanged, ready in 10s.make test-unit test-format test-typesFixes #1304