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

fix: let kind run with podman and VirtualBox #3079

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wheelerlaw
Copy link

If a user has VirtualBox installed on the same machine they have kind installed, and they are using the podman experimental provider, they will likely be faced with the following error when they try to run kind create cluster:

$ KIND_EXPERIMENTAL_PROVIDER=podman kind create cluster
using podman due to KIND_EXPERIMENTAL_PROVIDER
enabling experimental podman provider
Creating cluster "kind" ...
 ✓ Ensuring node image (kindest/node:v1.25.3) 🖼
 ✗ Preparing nodes 📦  
ERROR: failed to create cluster: command "podman run --name kind-control-plane --hostname kind-control-plane --label io.x-k8s.kind.role=control-plane --privileged --tmpfs /tmp --tmpfs /run --volume e5b4dd4c571adb5257ddc3dbcc6bb6f0f220ccc935760ea7dc20539aac90d49d:/var:suid,exec,dev --volume /lib/modules:/lib/modules:ro -e KIND_EXPERIMENTAL_CONTAINERD_SNAPSHOTTER --detach --tty --net kind --label io.x-k8s.kind.cluster=kind -e container=podman --volume /dev/mapper:/dev/mapper --device /dev/fuse --publish=127.0.0.1:43527:6443/tcp -e KUBECONFIG=/etc/kubernetes/admin.conf docker.io/kindest/node@sha256:f52781bc0d7a19fb6c405c2af83abfeb311f130707a0e219175677e366cc45d1" failed with error: exit status 126
Command Output: Error: crun: error stat'ing file `/dev/vboxusb/005/006`: Permission denied: OCI permission denied

This is because the devices in the /dev/vboxusb belong to the vboxusers group. If the user executing the kind command is in that group, podman attempts to bind mount those hardware devices in the container when the --privileged flag is passed to podman. However, crun makes a setgroups syscall to reset the user groups so when it comes time for the bind mount to actually occur, it fails because the namespaced user in the container is no longer in that group and therefore does not have permission to stat the devices.

This change passes the --group-add keep-groups to podman (which then passes the run.oci.keep_original_groups=1 annotation to crun) so that the groups are preserved so the bind mount doesn't fail.

(related to containers/podman#14284)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wheelerlaw
Once this PR has been reviewed and has the lgtm label, please assign amwat for approval by writing /assign @amwat in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested a review from amwat January 27, 2023 00:52
@k8s-ci-robot k8s-ci-robot added the area/provider/podman Issues or PRs related to podman label Jan 27, 2023
@k8s-ci-robot k8s-ci-robot requested a review from aojea January 27, 2023 00:52
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 27, 2023
@k8s-ci-robot
Copy link
Contributor

Welcome @wheelerlaw!

It looks like this is your first PR to kubernetes-sigs/kind 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kind has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 27, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @wheelerlaw. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 27, 2023
@wheelerlaw wheelerlaw force-pushed the fix-podman-virtualbox-error branch from ebdacbb to 501f06b Compare January 27, 2023 00:56
@@ -185,6 +185,9 @@ func runArgsForNode(node *config.Node, clusterIPFamily config.ClusterIPFamily, n
// including some ones podman would otherwise do by default.
// for now this is what we want. in the future we may revisit this.
"--privileged",
// For using podman when VirtualBox is also installed
// See https://github.com/containers/podman/issues/14284
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to be needed only for rootless, if you see in the commonArgsmethod we append optional arguments based on some environment details, we should have a check for rootless somewhere so we can use the same pattern for this option instead of setting it for everything

@BenTheElder
Copy link
Member

I'm a little concerned that we have diverging behavior here but I supposed that's always going to be the case with rootless.

I haven't seen similar issues with rootless runc + docker yet. Trying to think what other implications carrying through the groups might have.

@wheelerlaw
Copy link
Author

Just circling back to this now. I've been looking at the code for a little bit, and I am having a hard time understanding why the provision.go file exists at all. It doesn't look like it has a reference to an instance of the Provider (and so can't call the provider's cached Info() method), which would be nice to use to determine if it is running rootless or not. The functions in that file are making repeated calls to podman info instead of using the cached Info() method. I'd like to see if I can refactor it a little bit to reduce how many times it's calling out to podman.

@BenTheElder
Copy link
Member

Just circling back to this now. I've been looking at the code for a little bit, and I am having a hard time understanding why the provision.go file exists at all.

provision.go splits out the lengthy logic for organizing node creation from the rest of the provider logic.

It doesn't look like it has a reference to an instance of the Provider (and so can't call the provider's cached Info() method), which would be nice to use to determine if it is running rootless or not. The functions in that file are making repeated calls to podman info instead of using the cached Info() method. I'd like to see if I can refactor it a little bit to reduce how many times it's calling out to podman.

That debt has happened over time but seems largely orthogonal, and has no relation to being in another file or not ..?

caching info for the duration of the process may also be a mis-feature. execing something like podman info is relatively cheap all things considered but persisting information without any eviction may cause issues.

@BenTheElder
Copy link
Member

This is because the devices in the /dev/vboxusb belong to the vboxusers group. If the user executing the kind command is in that group, podman attempts to bind mount those hardware devices in the container when the --privileged flag is passed to podman.

Thinking about this again, is there any reason to expect that this is desired?
Can we configure podman to not include this mount instead?

containers/podman#14284 (comment) also comes to mind as a potential issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/provider/podman Issues or PRs related to podman cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants