Provide warning message for unnecessary sudo #4455

djmgit · 2019-06-10T07:52:07Z

Fixes #4420. If minikube is started with root privilege and vm-driver is
not hyperV or none, then a warning message is displayed.

k8s-ci-robot · 2019-06-10T07:52:08Z

Welcome @djmgit!

It looks like this is your first PR to kubernetes/minikube 🎉. 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/minikube 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 · 2019-06-10T07:52:10Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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. I understand the commands that are listed here.

k8s-ci-robot · 2019-06-10T07:52:15Z

Hi @djmgit. 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.

minikube-bot · 2019-06-10T07:53:03Z

Can one of the admins verify this patch?

djmgit · 2019-06-10T08:10:21Z

/assign @tstromberg

djmgit · 2019-06-10T08:43:57Z

I signed it

medyagh · 2019-06-10T16:17:50Z

cmd/minikube/cmd/start.go

+	currentUser, err := user.Current()
+
+	// Display warning if minikube is being started with root and vmDriver is not hyperv
+	if err == nil && currentUser.Name == "root" && viper.GetString(vmDriver) != "hyperv" {


could you check if this works under windows too ? is it called "root" or "admin" or "adminstrator" in windows?

Using sudo (or running as root) is required, when using the none driver (as a special case).

cmd/minikube/cmd/start.go

medyagh · 2019-06-10T16:21:25Z

Thanks for making this PR, I am excited to review the PR. could you please sign the CLA?

djmgit · 2019-06-10T16:30:54Z

@medyagh I will make the changes you suggested and update my PR soon.
I have already signed the CLA, however its not reflecting here. Should I close this PR and create a new one?

medyagh · 2019-06-10T16:47:19Z

@medyagh I will make the changes you suggested and update my PR soon.
I have already signed the CLA, however its not reflecting here. Should I close this PR and create a new one?

no you don't need to create the a new PR, just make sure to use the same email for git that is used in linux foundation. (https://help.github.com/en/articles/setting-your-commit-email-address-in-git)
you need to re-base your PR to make a commit message vaild. the current commit message is not valid because it has "Issue : #4420 " in it.

afbjorklund · 2019-06-11T19:41:41Z

cmd/minikube/cmd/start.go

+	currentUser, err := user.Current()
+
+	// Display warning if minikube is being started with root and vmDriver is not hyperv
+	if err == nil && currentUser.Name == "root" && viper.GetString(vmDriver) != "hyperv" {


Using sudo (or running as root) is required, when using the none driver (as a special case).

djmgit · 2019-06-12T06:43:41Z

@afbjorklund cool, will add that case too

k8s-ci-robot · 2019-06-12T17:37:48Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: djmgit
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

djmgit · 2019-06-12T18:02:49Z

I signed it

djmgit · 2019-06-12T18:12:24Z

@medyagh I have updated my PR to log a message when err is not nil. Also I have added the case mentioned by @afbjorklund
I tested golang's user.Current() on windows. The problem with windows is, when the user.Current() runs with admin privilege (run as administrator), currentUser.Name does not return root. Rather it returns the name of the user who is running the code with admin privilege. So in windows, just the return value of currentUser.Name I guess does not ensure whether it is running with admin privileges or not

medyagh · 2019-06-14T05:31:43Z

@minikube-bot okay to test

sharifelgamal · 2019-06-14T19:14:41Z

Yeah, testing whether a Windows users has raised privileges is a little bit tricky, but there are ways to do it.

medyagh

please add none driver also to the ones exempt from warning

cmd/minikube/cmd/start.go

djmgit · 2019-06-15T06:18:49Z

@medyagh I have already considered none driver case after @afbjorklund pointed it out.

djmgit · 2019-06-19T01:58:24Z

@medyagh is there anything more needed in this PR?

medyagh

please change the fmt to glog.Errorf

cmd/minikube/cmd/start.go

djmgit · 2019-06-19T03:04:47Z

@medyagh done

medyagh · 2019-06-19T16:35:05Z

cmd/minikube/cmd/start.go

+
+	// Display warning if minikube is being started with root and vmDriver is not HyperV
+	if err != nil {
+		glog.Errorln("Error getting the current user: ", err)


please always use %v for errror.
like glog.Errorf("Error getting the current user %v", err)

medyagh · 2019-06-19T16:35:40Z

@minikube-bot OK to test

djmgit · 2019-06-19T16:38:29Z

@medyagh should I update it, or is it alright?
Actually I saw some examples in the codebase where glog.Errorln was used so I used like that.

medyagh · 2019-06-19T17:13:02Z

@medyagh should I update it, or is it alright?
Actually I saw some examples in the codebase where glog.Errorln was used so I used like that.

for err it is always better to use %v, lets use %v for err

If minikube is started with root privilege and vm-driver is not hyperV or none, then a warning message is displayed.

djmgit · 2019-06-20T02:14:33Z

@medyagh done

medyagh · 2019-06-20T02:22:40Z

Awesome thanks for the contribution and your patience to make quality code on minikube, It looks good to me after the tests run.

medyagh · 2019-06-20T03:57:13Z

@minikube-bot OK to test

k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jun 10, 2019

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jun 10, 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 10, 2019

k8s-ci-robot requested review from balopat and sharifelgamal June 10, 2019 07:52

djmgit force-pushed the sudo_message branch from 69f0d19 to a7a0f61 Compare June 10, 2019 07:57

k8s-ci-robot assigned tstromberg Jun 10, 2019

medyagh requested changes Jun 10, 2019

View reviewed changes

medyagh assigned medyagh and unassigned tstromberg Jun 10, 2019

medyagh changed the title ~~Issue : #4420 - Provide warning message for unnecessary sudo~~ Provide warning message for unnecessary sudo Jun 10, 2019

afbjorklund requested changes Jun 11, 2019

View reviewed changes

djmgit force-pushed the sudo_message branch from a7a0f61 to 463d4d4 Compare June 12, 2019 17:37

k8s-ci-robot removed the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jun 12, 2019

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

medyagh requested changes Jun 14, 2019

View reviewed changes

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved

medyagh added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2019

medyagh requested changes Jun 19, 2019

View reviewed changes

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved

djmgit force-pushed the sudo_message branch from 463d4d4 to c854e4c Compare June 19, 2019 03:04

medyagh requested changes Jun 19, 2019

View reviewed changes

Provide warning message for unnecessary sudo

2cee972

If minikube is started with root privilege and vm-driver is not hyperV or none, then a warning message is displayed.

djmgit force-pushed the sudo_message branch from c854e4c to 2cee972 Compare June 20, 2019 02:14

medyagh approved these changes Jun 20, 2019

View reviewed changes

medyagh removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 20, 2019

tstromberg merged commit ce74af2 into kubernetes:master Jun 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide warning message for unnecessary sudo #4455

Provide warning message for unnecessary sudo #4455

djmgit commented Jun 10, 2019 •

edited by sharifelgamal

Loading

k8s-ci-robot commented Jun 10, 2019

k8s-ci-robot commented Jun 10, 2019

k8s-ci-robot commented Jun 10, 2019

minikube-bot commented Jun 10, 2019

djmgit commented Jun 10, 2019

djmgit commented Jun 10, 2019

medyagh Jun 10, 2019

afbjorklund Jun 11, 2019

medyagh commented Jun 10, 2019

djmgit commented Jun 10, 2019

medyagh commented Jun 10, 2019

afbjorklund Jun 11, 2019

djmgit commented Jun 12, 2019

k8s-ci-robot commented Jun 12, 2019

djmgit commented Jun 12, 2019

djmgit commented Jun 12, 2019

medyagh commented Jun 14, 2019

sharifelgamal commented Jun 14, 2019

medyagh left a comment

djmgit commented Jun 15, 2019

djmgit commented Jun 19, 2019

medyagh left a comment

djmgit commented Jun 19, 2019

medyagh Jun 19, 2019 •

edited

Loading

medyagh commented Jun 19, 2019

djmgit commented Jun 19, 2019

medyagh commented Jun 19, 2019

djmgit commented Jun 20, 2019

medyagh commented Jun 20, 2019

medyagh commented Jun 20, 2019

Provide warning message for unnecessary sudo #4455

Provide warning message for unnecessary sudo #4455

Conversation

djmgit commented Jun 10, 2019 • edited by sharifelgamal Loading

k8s-ci-robot commented Jun 10, 2019

k8s-ci-robot commented Jun 10, 2019

k8s-ci-robot commented Jun 10, 2019

minikube-bot commented Jun 10, 2019

djmgit commented Jun 10, 2019

djmgit commented Jun 10, 2019

medyagh Jun 10, 2019

Choose a reason for hiding this comment

afbjorklund Jun 11, 2019

Choose a reason for hiding this comment

medyagh commented Jun 10, 2019

djmgit commented Jun 10, 2019

medyagh commented Jun 10, 2019

afbjorklund Jun 11, 2019

Choose a reason for hiding this comment

djmgit commented Jun 12, 2019

k8s-ci-robot commented Jun 12, 2019

djmgit commented Jun 12, 2019

djmgit commented Jun 12, 2019

medyagh commented Jun 14, 2019

sharifelgamal commented Jun 14, 2019

medyagh left a comment

Choose a reason for hiding this comment

djmgit commented Jun 15, 2019

djmgit commented Jun 19, 2019

medyagh left a comment

Choose a reason for hiding this comment

djmgit commented Jun 19, 2019

medyagh Jun 19, 2019 • edited Loading

Choose a reason for hiding this comment

medyagh commented Jun 19, 2019

djmgit commented Jun 19, 2019

medyagh commented Jun 19, 2019

djmgit commented Jun 20, 2019

medyagh commented Jun 20, 2019

medyagh commented Jun 20, 2019

djmgit commented Jun 10, 2019 •

edited by sharifelgamal

Loading

medyagh Jun 19, 2019 •

edited

Loading