Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

worker: drop CAP_SYS_{BOOT,MODULE} from the list of capabilities #8791

Closed
wants to merge 1 commit into from

Conversation

flokli
Copy link

@flokli flokli commented Jul 17, 2023

worker/runtime/spec/spec.go@defaultGardenOCISpec calls out to OciCapabilities(privileged bool), returning a list of capabilities to put in the OCI spec, which is then passed to runc.

Note this is independent of what the container payload might actually need, it always asks for these capabilities.

This causes problems when running concourse-worker in a Talos cluster, which does not allow asking for CAP_SYS_MODULE and CAP_SYS_BOOT (Concourse doesn't ask for the latter):

concourse-worker-2 concourse-worker {"timestamp":"2023-07-13T11:17:47.529591744Z","level":"error","source":"guardian","message":"guardian.api.garden-server.create.failed","data":{"error":"runc run: exit status 1: container_linux.go:380: starting container process caused: apply caps: operation not permitted","request":{"Handle":"af712415-e9aa-4ba7-639f-b291f6e2caaf","GraceTime":0,"RootFSPath":"raw:///concourse-work-dir/volumes/live/e5bce4ac-4d45-45c5-6338-38aaaaf27e72/volume","BindMounts":[{"src_path":"/concourse-work-dir/volumes/live/1b925d7a-e33b-41dc-6f4f-9cdc701583f0/volume","dst_path":"/scratch","mode":1}],"Network":"","Privileged":true,"Limits":{"bandwidth_limits":{},"cpu_limits":{},"disk_limits":{},"memory_limits":{},"pid_limits":{}}},"session":"3.1.140807"}}

See https://www.talos.dev/v1.4/learn-more/process-capabilities/ for details.

Removing these capabilities from the list should gets runc to succeed further, it's now erroring with the same error message as with unprivileged workloads:

runc run failed: unable to start container process: can't get final child's PID from pipe: EOF

This change might cause problems for people trying to modprobe kernel modules inside Concourse, or reboot the nodes, but I hope noone does that ;-)

Changes proposed by this PR

closes #

  • done
  • todo

Notes to reviewer

Release Note

`worker/runtime/spec/spec.go@defaultGardenOCISpec` calls out to
`OciCapabilities(privileged bool)`, returning a list of capabilities to
put in the OCI spec, which is then passed to runc.

Note this is independent of what the container payload might actually
need, it always asks for these capabilities.

This causes problems when running concourse-worker in a Talos cluster,
which does not allow asking for CAP_SYS_MODULE and CAP_SYS_BOOT
(Concourse doesn't  ask for the latter):

```
concourse-worker-2 concourse-worker {"timestamp":"2023-07-13T11:17:47.529591744Z","level":"error","source":"guardian","message":"guardian.api.garden-server.create.failed","data":{"error":"runc run: exit status 1: container_linux.go:380: starting container process caused: apply caps: operation not permitted","request":{"Handle":"af712415-e9aa-4ba7-639f-b291f6e2caaf","GraceTime":0,"RootFSPath":"raw:///concourse-work-dir/volumes/live/e5bce4ac-4d45-45c5-6338-38aaaaf27e72/volume","BindMounts":[{"src_path":"/concourse-work-dir/volumes/live/1b925d7a-e33b-41dc-6f4f-9cdc701583f0/volume","dst_path":"/scratch","mode":1}],"Network":"","Privileged":true,"Limits":{"bandwidth_limits":{},"cpu_limits":{},"disk_limits":{},"memory_limits":{},"pid_limits":{}}},"session":"3.1.140807"}}
```

See https://www.talos.dev/v1.4/learn-more/process-capabilities/ for
details.

Removing these capabilities from the list should gets runc to succeed
further, it's now erroring with the same error message as with
unprivileged workloads:

runc run failed: unable to start container process: can't get final child's PID from pipe: EOF

This change might cause problems for people trying to modprobe kernel
modules inside Concourse, or reboot the nodes, but I hope noone does
that ;-)

Signed-off-by: Florian Klink <flokli@flokli.de>
@xtremerui
Copy link
Contributor

Hi thank you for the PR. I am wondering if this PR doesn't address the issue completely, do you have plan to go further?

@flokli
Copy link
Author

flokli commented Jul 18, 2023

Hey, yes, I was a bit stuck on debugging exactly what fails in runc, and was hoping to get some guidance on how to debug it. Happy to dig further in a followup.

I was thinking this could already land, as it gets the privileged runc invocations at least as far as the same non-privileged ones.

@flokli
Copy link
Author

flokli commented Aug 31, 2023

@xtremerui poke. Also looks like @bdellegrazie is running into the same issues with Bottlerocket: #8792 (comment)

@xtremerui
Copy link
Contributor

@flokli is there a way you could apply this change to a dev version of you own and test if it solve the problem? Reason of asking is because we don't have a way to test by ourselves.

@flokli
Copy link
Author

flokli commented Aug 31, 2023

As described in the initial PR description, this solves part of the problem, and I tested it with a custom dev container (see concourse/concourse-chart#330 (comment)).

It doesn't solve the whole problem, but it gets privileged containers to at least not ask for CAP_SYS_{BOOT,MODULE}.

Solving #8792 (comment) is more work, but this PR is also a prerequisite for it, at least on Talos.

@flokli
Copy link
Author

flokli commented Sep 20, 2023

@xtremerui poke - can we get this in?

@xtremerui
Copy link
Contributor

@flokli I am leaning towards holding on this until the issue is solved. I think the context is well established here for anyone who wants to fix #8792 . Right now I don't see benefit of merging other than bringing potential problems for current users.

Since this PR doesn't really solve your problem, is there any other reason you want to merge it now?

@taylorsilva
Copy link
Member

Like Rui said, I think it doesn't make sense to move forward with this change as-is. We don't want to break things for existing users who may depend on these capabilities for their privileged containers.

For that reason I'm going to close this PR. I'm going to post some alternative ideas in the linked issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants