Repository navigation
atelet: make durable-dir volumes writable by non-root containers - #1906
Open
Davanum Srinivas (dims) wants to merge 1 commit into
Open
Davanum Srinivas (dims) wants to merge 1 commit into
Davanum Srinivas (dims) wants to merge 1 commit into
Conversation
Davanum Srinivas (dims)
marked this pull request as ready for review
September 25, 2026 23:51
Davanum Srinivas (dims)
marked this pull request as draft
September 26, 2026 02:19
This was referenced Sep 26, 2026
Davanum Srinivas (dims)
force-pushed
the
pr/durable-dir-mode
branch
2 times, most recently
from
September 27, 2026 01:30
564ae2c to
e9e122b
Compare
Davanum Srinivas (dims)
marked this pull request as ready for review
September 27, 2026 02:25
Collaborator
Author
pull Bot
pushed a commit
to RolfLobo/substrate
that referenced
this pull request
Sep 30, 2026
…agent-substrate#1910) Follow-up to agent-substrate#1906. Removing a directory entry needs write permission on the directory that holds it, and with all capabilities dropped uid 0 gets no exemption. A non-root container can create a subdirectory in its `0777` durable dir; that subdirectory belongs to the container's uid, typically `0755`, so root cannot empty it. `resetActorDirs` fails after every checkpoint of such an actor and the suspend never completes. Chmod first, as the bundle dir does, is no way out: chmod needs ownership or `CAP_FOWNER`. Why atelet and not ateom, which already holds the capability: an ateom that dies before cleanup leaves the files behind and moves the hang to the next resume on that node. atelet owns the actor directories and needs it on the crash path either way. Manifest only, so no unit test. Exercised on kind with the micro-VM class: uid 65532 writes a durable dir, then suspend and resume. Without the capability the same run hangs in `SUSPENDING`. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR (none needed)
Bowei Du (bowei)
added
area/node
area/security
Maya Wang (mayawang)
left a comment
Maya Wang (mayawang)
left a comment
Collaborator
There was a problem hiding this comment.
Thanks, this reads well. One question about actors that already have a snapshot.
Davanum Srinivas (dims)
force-pushed
the
pr/durable-dir-mode
branch
from
October 1, 2026 11:45
e9e122b to
c95ee1a
Compare
Created 0700, a non-root container could not write its own volume. Now 0777, as Kubernetes creates an emptyDir: one actor's containers can run as different users, and the 0700 actor directory above keeps other actors out. A restore keeps the mode the snapshot recorded; volumes created earlier stay 0700, which is fine while every container runs as root.
Davanum Srinivas (dims)
force-pushed
the
pr/durable-dir-mode
branch
from
October 1, 2026 12:04
c95ee1a to
895ec3c
Compare
Quentin Bisson (QuentinBisson)
pushed a commit
to giantswarm/substrate
that referenced
this pull request
Oct 1, 2026
…agent-substrate#1910) Follow-up to agent-substrate#1906. Removing a directory entry needs write permission on the directory that holds it, and with all capabilities dropped uid 0 gets no exemption. A non-root container can create a subdirectory in its `0777` durable dir; that subdirectory belongs to the container's uid, typically `0755`, so root cannot empty it. `resetActorDirs` fails after every checkpoint of such an actor and the suspend never completes. Chmod first, as the bundle dir does, is no way out: chmod needs ownership or `CAP_FOWNER`. Why atelet and not ateom, which already holds the capability: an ateom that dies before cleanup leaves the files behind and moves the hang to the next resume on that node. atelet owns the actor directories and needs it on the crash path either way. Manifest only, so no unit test. Exercised on kind with the micro-VM class: uid 65532 writes a durable dir, then suspend and resume. Without the capability the same run hangs in `SUSPENDING`. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR (none needed) (cherry picked from commit afb62e1)
Quentin Bisson (QuentinBisson)
pushed a commit
to giantswarm/substrate
that referenced
this pull request
Oct 5, 2026
…agent-substrate#1910) Follow-up to agent-substrate#1906. Removing a directory entry needs write permission on the directory that holds it, and with all capabilities dropped uid 0 gets no exemption. A non-root container can create a subdirectory in its `0777` durable dir; that subdirectory belongs to the container's uid, typically `0755`, so root cannot empty it. `resetActorDirs` fails after every checkpoint of such an actor and the suspend never completes. Chmod first, as the bundle dir does, is no way out: chmod needs ownership or `CAP_FOWNER`. Why atelet and not ateom, which already holds the capability: an ateom that dies before cleanup leaves the files behind and moves the hang to the next resume on that node. atelet owns the actor directories and needs it on the crash path either way. Manifest only, so no unit test. Exercised on kind with the micro-VM class: uid 65532 writes a durable dir, then suspend and resume. Without the capability the same run hangs in `SUSPENDING`. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR (none needed) (cherry picked from commit afb62e1)
Comment on lines
+1545
to
+1546
| // different users. Chmod, because MkdirAll applies the umask and skips an | ||
| // existing directory. A restore then re-applies the mode the snapshot |
Collaborator
There was a problem hiding this comment.
🤖 nit 🟢 – The directory can't already exist here. resetActorDirs removes and recreates the durable-dir parent before both Run and Restore call prepareOCIBundles. So the umask is the only reason for the chmod. Consider dropping "and skips an existing directory" here, and the existing-dir half of TestPrepareDurableDirVolume along with its comment.
Collaborator
There was a problem hiding this comment.
(we just did some hardening of durdir setup)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Durable-dir volumes were created
0700, so a container running as a non-root user could not write its own volume. Now0777, as Kubernetes creates an emptyDir: one actor's containers can run as different users, and the0700actor directory above keeps other actors out.A restore keeps the mode the snapshot recorded, as emptyDir keeps a pod's own chmod. Volumes created earlier stay
0700, fine while every container runs as root. No effect until an image'sUSERis honored (#1918). Cleanup of files a non-root container leaves behind landed in #1910.Tests:
TestPrepareDurableDirVolume. Exercised on kind with the micro-VM class: uid 65532 writes a durable dir, then suspend and resume.