WIP: hyperv: Assert that the user has correct permissions #4745

blueelvis · 2019-07-12T19:49:14Z

This adds a check to ensure that we are running Hyper-V with administrator privileges.
Virtualbox seems to work fine without administrator privileges.
Moved the validateUser() so that it is effective even when cluster is already provisioned.

-Pranav

… to a separate Util function.

k8s-ci-robot · 2019-07-12T19:49:22Z

Hi @blueelvis. 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/test-infra repository.

blueelvis · 2019-07-12T19:50:01Z

/assign medyagh

minikube-bot · 2019-07-12T19:50:03Z

Can one of the admins verify this patch?

tstromberg

Great work here! I hope you don't mind that I have a handful of minor suggestions.

tstromberg · 2019-07-15T16:05:08Z

pkg/util/utils.go

@@ -291,3 +293,31 @@ func ContainsString(slice []string, s string) bool {
 	}
 	return false
 }
+
+// ValidateUser validates minikube is run by the recommended user (privileged or regular)


I like the idea, but let's find a better home for it with a more appropriate function signature. What do you think about moving this to pkg/drivers/drivers.go so that the call can be `driver.ValidatePermissions()?

Since this function will no longer be in the main package, it should return an error instead of exiting.

I have moved this to the drivers file. I was wondering, do we have any middleware setup? With this type of implementation, I really don't like how in some places, I need to check the commandline arguments and in some places, what is loaded from the config file. If we don't have it, I think it would be best to have a middleware setup which can validate config/permissions after arguments are saved to config file and we validate the config file only.

That way, in future, it would be easier to add more checks like validatePermissions. What do you think?

pkg/util/utils.go

tstromberg · 2019-07-15T16:26:21Z

pkg/util/utils.go

+		// If Hyper-V, ensure we are having Administrator Privilleges.
+		if driverName == constants.DriverHyperv {
+			// This tries opening the first disk attached to the system for a direct I/O operation. This operation requires Administrator privileges and will fail without that.
+			rawDisk, err := os.Open("\\\\.\\PHYSICALDRIVE0")


I just realized that this may break some users: Hyper-V has a separate set of ACL's, and in fact there is support for a Hyper-V administrators group. Rather than requiring full administrative access, I think we should:

Make this check more specific so that HyperV admins can continue to use minikube. Perhaps there we could call Get-VM, Get-VMConnectAccess or some other kind of command line instead to see if we have access to Hyper-V?

Add a --force flag so that these checks can be skipped.

We can start with one of those approaches, but both would be ideal in the long term.

Do you think we could track this in a separate bug/ work item? I fully agree with this and I think that we could check what other approaches could be in this case instead of running raw Powershell commands. Not sure if we already have a package to execute Powershell commands as part of golang. Pretty sure that there would be a native Windows API to check this.

@tstromberg - I was browsing around and I see that what you are asking is already part of the machine library - https://github.com/docker/machine/blob/a555e4f7a8f518a8b1b174824c377e46cbfc4fe2/drivers/hyperv/powershell.go#L71

Let me add that and push a commit.

@tstromberg - I noticed that these checks are private to the package for now. I am in the process of creating a PR to make those checks public and we can revisit this again. As of now, this PR will fix the issues with most of the client users. The very small user base which will be affected would be the ones running Hyper-V on Windows Server environments with that role.

WDYT?

tstromberg · 2019-07-15T16:26:40Z

@minikube-bot OK to test

k8s-ci-robot · 2019-07-21T13:24:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: blueelvis
To complete the pull request process, please assign medyagh
You can assign the PR to them by writing /assign @medyagh in a comment when ready.

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

… to a separate Util function.

josedonizetti · 2019-07-29T15:48:31Z

pkg/drivers/drivers.go

@@ -136,3 +139,32 @@ func fixPermissions(path string) error {
 	}
 	return nil
 }
+
+// ValidatePermissions validates minikube is run by the recommended user (privileged or regular)
+func ValidatePermissions(driverName string) error {


I don't think this function should be in this package. This is related to what @afbjorklund was taking in the issue #4909. Anything under pkg/drivers shouldn't be using minikube code directly, because the idea is that those drivers will be moved upstream. In this case, we have translations here. Drivers code related to minikube should be in pkg/minikube/drivers.

wdyt?

In one of the review comments above, Thomas mentioned that it would be better if we could place this function at this path.

Gotcha! No strong feelings about it. Although I think pkg/drivers should be drivers only, not using minikube code.

@tstromberg thoughts?

SGTM. I think this should go in pkg/minikube/drivers/drivers.go then?

blueelvis · 2019-08-04T14:52:30Z

@tstromberg - ping. The integration tests are failing because now more output is coming instead of just Running -

 02:28:33 | > Running
--- FAIL: TestDocker (158.46s)
    util.go:248: got state: "! Please don't run minikube as root or with 'sudo' privileges. It isn't necessary with kvm2 driver.\n! Please don't run minikube as root or with 'sudo' privileges. It isn't necessary with kvm2 driver.\n! Please don't run minikube as root or with 'sudo' privileges. It isn't necessary with kvm2 driver.\nRunning", expected "Running"

I also cannot run the integration tests on my Windows box :( . Could you please guide me on where I need to make a fix for this?

tstromberg · 2019-08-06T15:35:16Z

@tstromberg - ping. The integration tests are failing because now more output is coming instead of just Running -
 02:28:33 | > Running
--- FAIL: TestDocker (158.46s)
    util.go:248: got state: "! Please don't run minikube as root or with 'sudo' privileges. It isn't necessary with kvm2 driver.\n! Please don't run minikube as root or with 'sudo' privileges. It isn't necessary with kvm2 driver.\n! Please don't run minikube as root or with 'sudo' privileges. It isn't necessary with kvm2 driver.\nRunning", expected "Running"
I also cannot run the integration tests on my Windows box :( . Could you please guide me on where I need to make a fix for this?

Is there a bug about integration tests on Windows? I'd like to make sure this gets sorted out. Presumably this should work even if make isn't available, but there are probably other dragons lurking:

go test -v -test.timeout=60m ./test/integration --tags="integration container_image_ostree_stub containers_image_openpgp" -minikube-start-args='--vm-driver=hyperv' -test.run=TestDocker

blueelvis · 2019-08-18T15:18:56Z

@tstromberg @medyagh - If I run that command, I am getting the following. This is happening even after cleaning the packages and getting them again.

PS D:\dev\minikube> go test -v -test.timeout=60m ./test/integration --tags="integration container_image_ostree_stub containers_image_openpgp" -minikube-start-args='--vm-driver=hyperv --memory=1.3g --hyperv-virtual-switch="Default Switch"' -test.run=TestDocker
can't load package: package k8s.io/minikube: unknown import path "k8s.io/minikube": cannot find module providing package k8s.io/minikube

Any other way, this can be fixed?

k8s-ci-robot · 2019-08-18T23:17:20Z

@blueelvis: 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/test-infra repository.

medyagh · 2019-08-22T01:25:41Z

@tstromberg @medyagh - If I run that command, I am getting the following. This is happening even after cleaning the packages and getting them again.
PS D:\dev\minikube> go test -v -test.timeout=60m ./test/integration --tags="integration container_image_ostree_stub containers_image_openpgp" -minikube-start-args='--vm-driver=hyperv --memory=1.3g --hyperv-virtual-switch="Default Switch"' -test.run=TestDocker
can't load package: package k8s.io/minikube: unknown import path "k8s.io/minikube": cannot find module providing package k8s.io/minikube
Any other way, this can be fixed?

sorry for the late reply, does this help ? https://minikube.sigs.k8s.io/docs/contributing/testing/

env TEST_ARGS="-minikube-start-args=--vm-driver=hyperv -test.run TestStartStop" make integration

@blueelvis

blueelvis · 2019-09-15T14:42:46Z

@medyagh - I tried running that in an Admin Git bash prompt but I am still not able to run the integration tests against Hyper-V :(

bluee@DESKTOP-8TI9P3N MINGW64 /d/dev/minikube (master)
$ env TEST_ARGS="-minikube-start-args=--vm-driver=hyperv -test.run TestStartStop" make integration
go test -v -test.timeout=60m ./test/integration --tags="integration container_image_ostree_stub containers_image_openpgp" -minikube-start-args=--vm-driver=hyperv -test.run TestStartStop
panic: Failed to define asset deploy/addons/gvisor/gvisor-runtimeclass.yaml: Asset deploy/addons/gvisor/gvisor-runtimeclass.yaml not found

goroutine 1 [running]:
k8s.io/minikube/pkg/minikube/assets.MustBinAsset(0x1a58de7, 0x2d, 0x1a2f85e, 0x16, 0x1a34147, 0x18, 0x1a13f2e, 0x4, 0x100, 0xc000524a80)
        D:/dev/minikube/pkg/minikube/assets/vm_assets.go:156 +0x1b2
k8s.io/minikube/pkg/minikube/assets.init.ializers()
        D:/dev/minikube/pkg/minikube/assets/addons.go:317 +0x23a5
FAIL    k8s.io/minikube/test/integration        0.528s
make: *** [Makefile:220: integration] Error 1

I am in the root folder of the project and I can see the file in the console but not sure why this is not able to find it. Any thoughts?

tstromberg · 2019-09-16T18:05:44Z

@sharifelgamal has some experience here.

priyawadhwa · 2019-10-09T19:01:03Z

Hey @blueelvis, we may have merged in a change that solves your issue ~3 weeks ago. Could you resolve your merge conflicts and try again? If you're still seeing issues, please ping @sharifelgamal.

blueelvis · 2019-10-11T06:31:32Z

@priyawadhwa - Are you talking about the integration tests? Let me try again over the weekend and get back on this.

priyawadhwa · 2019-10-11T18:46:04Z

@blueelvis yup I meant the integration tests, sorry for not clarifying! And sound great, thanks.

tstromberg · 2019-10-17T22:13:28Z

pkg/drivers/drivers.go

+		// If Hyper-V, ensure we are having Administrator Privilleges.
+		if driverName == constants.DriverHyperv {
+			// This tries opening the first disk attached to the system for a direct I/O operation. This operation requires Administrator privileges and will fail without that.
+			rawDisk, err := os.Open("\\\\.\\PHYSICALDRIVE0")


Alternatively: https://coolaj86.com/articles/golang-and-windows-and-admins-oh-my/

Also, we should perhaps be checking for membership to the hyperv admins group.

blueelvis · 2019-10-20T07:59:57Z

@priyawadhwa @tstromberg - I am closing this Pull Request. I will submit a new one after copying the functions from the machine-drivers fork. That would be really easy to achieve what is mentioned in the article as well.

Discussion - machine-drivers/machine#20

Added Hyper-V Administrator check. Also moved the user related checks…

0227079

… to a separate Util function.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 12, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 12, 2019

k8s-ci-robot requested review from afbjorklund and RA489 July 12, 2019 19:49

k8s-ci-robot assigned medyagh Jul 12, 2019

tstromberg suggested changes Jul 15, 2019

View reviewed changes

tstromberg changed the title ~~Added Hyper-V & Other Driver checks~~ hyperv: Assert that the user has correct permissions Jul 15, 2019

tstromberg suggested changes Jul 15, 2019

View reviewed changes

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

tstromberg added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 17, 2019

medyagh changed the title ~~hyperv: Assert that the user has correct permissions~~ wip: hyperv: Assert that the user has correct permissions Jul 17, 2019

medyagh changed the title ~~wip: hyperv: Assert that the user has correct permissions~~ WIP: hyperv: Assert that the user has correct permissions Jul 17, 2019

afbjorklund mentioned this pull request Jul 17, 2019

VM_BOOT_FAILED_HYPERV_ENABLED message should mention --driver=hyperv as a solution #4783

Closed

Fixed as per review comments

bff56f6

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 21, 2019

Merge branch 'master' into 4511-check-windows-user-admin

ba9cc95

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2019

Fixed build error

d222948

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

blueelvis added 3 commits July 25, 2019 02:53

Added Hyper-V Administrator check. Also moved the user related checks…

1e967b0

… to a separate Util function.

Rebased

1dc4f08

Merge branch 'master' into 4511-check-windows-user-admin

d100130

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 24, 2019

Merge branch 'master' into 4511-check-windows-user-admin

6a681af

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2019

Fixed the import error

f96fce9

josedonizetti reviewed Jul 29, 2019

View reviewed changes

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

Merge branch 'master' into 4511-check-windows-user-admin

0a53698

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2019

Fixed linting errors <_<

fb15a53

blueelvis mentioned this pull request Aug 7, 2019

Check for Username instead of Name for root #4995

Closed

blueelvis mentioned this pull request Aug 18, 2019

Altered root user check for vm-driver=none #5123

Closed

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

Xuanwo mentioned this pull request Sep 6, 2019

v1.3.1: sudo minikube start requires sudo with vm-driver none #5110

Closed

blueelvis mentioned this pull request Oct 2, 2019

REQUEST: New membership for blueelvis kubernetes/org#1246

Closed

6 tasks

tstromberg reviewed Oct 17, 2019

View reviewed changes

blueelvis closed this Oct 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: hyperv: Assert that the user has correct permissions #4745

WIP: hyperv: Assert that the user has correct permissions #4745

blueelvis commented Jul 12, 2019

k8s-ci-robot commented Jul 12, 2019

blueelvis commented Jul 12, 2019

minikube-bot commented Jul 12, 2019

tstromberg left a comment

tstromberg Jul 15, 2019

blueelvis Jul 21, 2019

tstromberg Jul 15, 2019

blueelvis Jul 20, 2019

blueelvis Jul 25, 2019

blueelvis Jul 31, 2019

tstromberg commented Jul 15, 2019

k8s-ci-robot commented Jul 21, 2019

josedonizetti Jul 29, 2019

blueelvis Jul 29, 2019

josedonizetti Jul 29, 2019 •

edited

Loading

tstromberg Sep 5, 2019

blueelvis commented Aug 4, 2019

tstromberg commented Aug 6, 2019

blueelvis commented Aug 18, 2019

k8s-ci-robot commented Aug 18, 2019

medyagh commented Aug 22, 2019 •

edited

Loading

blueelvis commented Sep 15, 2019

tstromberg commented Sep 16, 2019

priyawadhwa commented Oct 9, 2019

blueelvis commented Oct 11, 2019

priyawadhwa commented Oct 11, 2019

tstromberg Oct 17, 2019

blueelvis commented Oct 20, 2019

WIP: hyperv: Assert that the user has correct permissions #4745

WIP: hyperv: Assert that the user has correct permissions #4745

Conversation

blueelvis commented Jul 12, 2019

k8s-ci-robot commented Jul 12, 2019

blueelvis commented Jul 12, 2019

minikube-bot commented Jul 12, 2019

tstromberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstromberg commented Jul 15, 2019

k8s-ci-robot commented Jul 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josedonizetti Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueelvis commented Aug 4, 2019

tstromberg commented Aug 6, 2019

blueelvis commented Aug 18, 2019

k8s-ci-robot commented Aug 18, 2019

medyagh commented Aug 22, 2019 • edited Loading

blueelvis commented Sep 15, 2019

tstromberg commented Sep 16, 2019

priyawadhwa commented Oct 9, 2019

blueelvis commented Oct 11, 2019

priyawadhwa commented Oct 11, 2019

Choose a reason for hiding this comment

blueelvis commented Oct 20, 2019

josedonizetti Jul 29, 2019 •

edited

Loading

medyagh commented Aug 22, 2019 •

edited

Loading