ateom gvisor: drop privileged: true#496
Conversation
Actor packets enter the worker pod via the host-side veth and leave through the pod's eth0, which requires IPv4 forwarding. Without privileged the runtime bind-mounts /proc/sys read-only, so the worker (which holds CAP_SYS_ADMIN with no user namespace, leaving the ro flag unlocked) clears the flag with a bind-remount, writes the sysctl, and restores read-only.
Replace the --ignore-cgroups workaround with real cgroup delegation so runsc creates a per-actor-container cgroup leaf (with cpu/memory/pids accounting) nested under the worker pod's own cgroup, so those stats roll up to the pod and kubelet pod metrics capture the actor. At startup the ateom prepares the pod cgroup for delegation (setupCgroupDelegation): the unprivileged worker runs in a private cgroup namespace, so /sys/fs/cgroup is the pod's own scope. It remounts that scope read-write (the runtime bind-mounts it read-only), moves the worker's own processes into a dedicated "ateom" leaf to satisfy the cgroup v2 no-internal-processes rule, and enables the delegated controllers in cgroup.subtree_control. This only runs inside a private cgroup namespace; a privileged worker inherits the host cgroup namespace (the true root, full of unmovable kernel threads), so inPrivateCgroupNamespace() detects that via /proc/self/cgroup and skips delegation. Each container's cgroupsPath is set to "/<containerName>" in the bundle's OCI spec before runsc create/restore (ensureContainerCgroupsPath). atelet emits a runtime-agnostic spec, so the gVisor ateom fills in its own convention here, mirroring how the micro-VM ateom assigns /ateomchv/<id>.
Give the ateom container a per-sandbox-class security context instead of always running privileged. gVisor workers now run unprivileged: drop ALL capabilities and add only the set runsc needs (the gofer's user-namespace identity map needs SETUID/SETGID/SETPCAP/SETFCAP; the sandbox needs SYS_ADMIN/SYS_CHROOT/SYS_PTRACE; actor networking needs NET_ADMIN/NET_RAW; OCI rootfs setup needs DAC_OVERRIDE/FOWNER/CHOWN/MKNOD). The default seccomp profile is retained; AppArmor is set to Unconfined because runsc's mounts and the worker's cgroup remount are denied by the default profile (enforced on GKE COS/Ubuntu), which a privileged worker got implicitly. Micro-VM workers stay privileged (kata + cloud-hypervisor needs broad host access). A tailored AppArmor profile for the gVisor worker is left as a follow-up.
Davanum Srinivas (dims)
left a comment
Davanum Srinivas (dims)
left a comment
There was a problem hiding this comment.
LGTM in principle :)
|
I tested the cgroups fix locally and got the output below I'm wondering why is the "counter" cgroup empty with no process IDs in it? is this intended? |
|
Ugh, I guess I didn't actually submit my reply?
Yes. So the pause / sandbox container has the whole gvisor ~pod under it. The other leaf cgroups are ~harmless. I left it versus adding more branching/complexity to the code for now but I could see either way. gvisor should internally enforce the resources etc. |
Zoe Zhao (zoez7)
left a comment
Zoe Zhao (zoez7)
left a comment
There was a problem hiding this comment.
Left some questions. To be honest I'm totally pretending to know what I'm reviewing in this PR :). Manual verification looked good. LGTM to ship this since this is a improvement from what we have now, and let Tim and Taahir complain when they are back from vacation.
| // only ever lists processes that are not already in a child cgroup, and the list | ||
| // shrinks as we drain it, so loop until the source is empty. | ||
| func moveProcs(ctx context.Context, srcProcs, dstProcs string) error { | ||
| for range 100 { |
There was a problem hiding this comment.
Nit: could you leave a comment to explain where the number "100" comes from?
There was a problem hiding this comment.
Good point.
There was a problem hiding this comment.
This is a somewhat arbitrary upper bound on attempts. Added a code comment.
| if private, err := inPrivateCgroupNamespace(); err != nil { | ||
| return fmt.Errorf("while detecting cgroup namespace: %w", err) | ||
| } else if !private { | ||
| slog.InfoContext(ctx, "not in a private cgroup namespace; skipping cgroup delegation (worker is likely privileged)") |
There was a problem hiding this comment.
When do we expect worker to be privileged when using gvisor sandbox class?
There was a problem hiding this comment.
This is a skew guard.
Though technically we could also hit this on say, a cgroup v1 host.
Also very technically: cgroupv2 unprivileged container == private cgroupns is more of a convention across docker/podman/containerd/cri-o.
You could also explicitly opt into into it in cgroupv1, but there was some sort of agreement to make it default with the migration to v2. But they're not actually linked from the kernel perspective, only major container runtimes.
| sc := corev1ac.SecurityContext(). | ||
| WithRunAsUser(0). | ||
| WithRunAsGroup(0) | ||
| if class == atev1alpha1.SandboxClassMicroVM { |
There was a problem hiding this comment.
Does this mean the "shape" of cgroup hierarchy will look different depending on microvm vs. givsor?
There was a problem hiding this comment.
Yeah, though that's only mildly relevant, since the actual actor cgroups are inside the microVM guest environment anyhow. It's a different kernel etc.
I'm also looking at patching uVM for this, for overlapping reasons (the device passthrough angle), but it's tricky.
|
Also fyi Haven Xia (@HavenXia) changes like this will rely on the upgrade strategy that you are working on :) |
Review feedback on agent-substrate#496: document why the loop re-reads cgroup.procs and where the 100-iteration cap comes from.
|
Can we hold this? for egress/ingress work - we probably going to need privileges for installing nftable rules for redirection from the actor to the tunneling component. |
We already install nftables rules, the actor has |
Yes - for system upgrade with such change we will need some batch suspend and resume mechanism to handle instead of dropping everything. |
5 participants
Fixes #288
x-ref #174
Also, by not running with
privileged: truewe should stop mounting all host devices, so we can start thinking about passing through specific devices. cc Davanum Srinivas (@dims) Omer Yahud (@omeryahud)We do this by:
privileged: true, which also ensures we're in a private cgroupns (we assume cgroups v2 container ecosystem which does this, cgroup v1 is broadly deprecated).To be clear: ateom-gvisor is still highly permissioned, it's just not literally
privileged: trueanymore, so we drop unwanted side-effects from that.This PR was AI assisted. I have tested everything on kind + GKE locally and reviewed the changes.