Lower priority for NeedsSudo drivers when no sudo available (#21633) #21687

henry3260 · 2025-10-03T12:50:26Z

What this PR does

When sudo is not available on the host, drivers that require sudo (NeedsSudo = true) will have their priority lowered.
Drivers that do not require sudo remain unaffected. This ensures the Available() function returns a properly prioritized driver list depending on the environment.

How I tested

Manually verified that a "NeedsSudo" driver gets a lower priority when sudo is unavailable.
Existing unit tests for driver registration and availability remain unaffected.

k8s-ci-robot · 2025-10-03T12:50:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: henry3260
Once this PR has been reviewed and has the lgtm label, please assign comradeprogrammer for approval. For more information see the 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:

OWNERS

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 · 2025-10-03T12:50:36Z

Hi @henry3260. Thanks for your PR.

I'm waiting for a kubernetes 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-sigs/prow repository.

minikube-bot · 2025-10-03T12:51:04Z

Can one of the admins verify this patch?

nirs

This change is wrong in many ways:

The idea to change the priority based on sudo requirement is wrong
The check if sudo is available is wrong
The information about which drivers requires sudo are mostly wrong

In general almost anything worth using requires sudo , and we should document the requirements. We should not change driver priority based on sudo requirements for setting up the system for the driver.

nirs · 2025-10-03T13:49:27Z

pkg/minikube/driver/driver.go

+	if !HasSudo() {
+		for i := range options {
+			if options[i].NeedsSudo {
+				options[i].Priority = registry.Discouraged


We must not do this. Driver priority must not change based on sudo availability.

We can add more info on the driver status, but the priority must not change.

It should be in the "healthy", I think?

nirs · 2025-10-03T13:51:48Z

pkg/minikube/driver/driver.go

+			Default:   d.Default,
+			Priority:  d.Priority,
+			State:     s,
+			NeedsSudo: d.NeedsSudo,


This is problematic since this info is dynamic. For example vfkit does not need sudo with the default network (nat), but need sudo for setup with vmnet-shared network. This should be done in the driver documentation and not here.

nirs · 2025-10-03T13:53:56Z

pkg/minikube/driver/driver.go

+func realHasSudo() bool {
+	err := exec.Command("sudo", "-n", "true").Run()
+	return err == nil
+}


This is incorrect check. Nobody configures a system so you can run any command as root without a password. This configuration is use only in throwaway systems like CI VM that is deleted after the CI run, or inside minikube VM where you are not expected to modify the host.

We configured minikube to use sudo -n podman, since there was no other way (no "podman" group)

So this check seems to match what the driver is using, even if there are some details that is differing.

nirs · 2025-10-03T13:55:03Z

pkg/minikube/registry/drvs/docker/docker.go

 		Default:  true,
 		Priority: registry.HighlyPreferred,
+		// Docker driver does not require sudo
+		NeedsSudo: false,


You must have sudo to install docker on Linux or add your self to docker group. This is same as kvm.

nirs · 2025-10-03T13:56:12Z

pkg/minikube/registry/drvs/hyperkit/hyperkit.go

 		Default:  true,
 		Priority: registry.Deprecated,
+		// Hyperkit driver requires sudo
+		NeedsSudo: true,


Minikube has code to configure hyperkit to run without a password, but hyperkit is deprecated so it is not interesting to add this info.

nirs · 2025-10-03T13:57:20Z

pkg/minikube/registry/drvs/kvm2/kvm2.go

 		Default:  true,
 		Priority: registry.Preferred,
+		// KVM2 driver does not require sudo
+		NeedsSudo: false,


Requires sudo to install libvirt and add yourself to the libvirt group.

Those can be done by a sysadmin, without having to use sudo

nirs · 2025-10-03T14:01:19Z

pkg/minikube/registry/drvs/podman/podman.go

 		Default:  true,
 		Priority: priority,
+		// Podman driver may require sudo (on linux)
+		NeedsSudo: true,


This depends on the os:

Linux: requires sudo to install podman. Using rootless mode by default so no need to configure special group

macOS: does not require sudo to install and run podman (using vfkit and gvproxy)

The minikube driver may have limitations and issues with using podman, it is not not well maintained.

The minikube driver for podman was using "sudo podman", and it now has a rootless config.

Since it was so buggy with rootless in the beginning, the default has always been to use rootful

nirs · 2025-10-03T14:04:00Z

pkg/minikube/registry/drvs/qemu2/qemu2.go

 		Default:  true,
 		Priority: priority,
+		// QEMU2 driver does not require sudo
+		NeedsSudo: false,


Wrong, qemu can be installed via brew, but to use socket_vment (e.g to support multiple clusters accessing each other, or multi-node clusters), you need root - same as vmnet-helper.

This also depends on the system:

Linux: sudo required to install qemu

macOS: sudo not required to install qemu (e.g. brew install qemu)

It didn't need sudo for the "user" networking, and you don't need sudo if you install in an odd location

nirs · 2025-10-03T14:04:51Z

pkg/minikube/registry/drvs/ssh/ssh.go

 		Priority: registry.Discouraged,
 		Init:     func() drivers.Driver { return ssh.NewDriver(ssh.Config{}) },
+		// SSH driver does not require sudo
+		NeedsSudo: false,


I think you will need sudo on the host you connect to.

nirs · 2025-10-03T14:05:39Z

pkg/minikube/registry/drvs/vfkit/vfkit.go

 		Default:  true,
 		Priority: priority,
+		// VFKit driver requires sudo
+		NeedsSudo: true,


sudo required only for the initial setup to install vment-helper.

afbjorklund · 2025-10-26T10:44:35Z

In general almost anything worth using requires sudo , and we should document the requirements. We should not change driver priority based on sudo requirements for setting up the system for the driver.

I think there is a difference if minikube itself is calling sudo, or if some setup is needed that normally requires root or sudo during the installation...

But if the driver is using sudo, then it is reasonable to check if sudo has been setup. And I was under the impression that was already done, for health?

k8s-ci-robot · 2025-10-28T01:36:34Z

PR needs rebase.

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-sigs/prow repository.

Lower priority for NeedsSudo drivers when no sudo available

a157999

k8s-ci-robot requested a review from ComradeProgrammer October 3, 2025 12:50

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 3, 2025

k8s-ci-robot requested a review from prezha October 3, 2025 12:50

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 Oct 3, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 3, 2025

nirs suggested changes Oct 3, 2025

View reviewed changes

nirs mentioned this pull request Oct 3, 2025

Check if we have Sudo Permission, if not Lower The priority of the drivers that Need Sudo #21633

Open

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2025

Lower priority for NeedsSudo drivers when no sudo available (#21633) #21687

Are you sure you want to change the base?

Lower priority for NeedsSudo drivers when no sudo available (#21633) #21687

Conversation

henry3260 commented Oct 3, 2025

What this PR does

How I tested

Uh oh!

k8s-ci-robot commented Oct 3, 2025

Uh oh!

k8s-ci-robot commented Oct 3, 2025

Uh oh!

minikube-bot commented Oct 3, 2025

Uh oh!

nirs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afbjorklund Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afbjorklund commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

afbjorklund Oct 26, 2025 •

edited

Loading

afbjorklund commented Oct 26, 2025 •

edited

Loading