Add 'native-ssh' flag to 'minikube start' and 'minikube ssh' #4510

yinzara · 2019-06-17T17:40:38Z

Docker machine gives an option for sshing into a running container of using the Golang SSH native client or using the external 'ssh' command line executable.

Normally, "docker-machine ssh" uses the "external" client by default and a "--native-ssh" flag must be passed to enable the native client.

Minikube by set the client to native by default with no option to change this behavior. This causes issues with some VM drivers.

Though the native SSH client for Golang is more efficient than the command line 'ssh' command, there are times in which using the 'ssh' CLI is necessary.

This PR adds a "--no-native-ssh" flag to the minikube start command an and associated "no-native-ssh" config option (through minikube config set no-native-ssh true.

k8s-ci-robot · 2019-06-17T17:40:39Z

Welcome @yinzara!

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-17T17:40:40Z

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: [email protected]

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-17T17:40:45Z

Hi @yinzara. 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-17T17:41:05Z

Can one of the admins verify this patch?

afbjorklund · 2019-06-17T17:42:30Z

pkg/minikube/cluster/cluster.go

@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"flag"
 	"fmt"
+	"github.com/docker/machine/libmachine/ssh"


this change looks odd

Agree. That was definitely added in error. I'll fix.

cmd/minikube/cmd/config/config.go

medyagh · 2019-06-17T20:37:57Z

cmd/minikube/cmd/start.go

@@ -154,6 +157,7 @@ func init() {
 	startCmd.Flags().Bool(gpu, false, "Enable experimental NVIDIA GPU support in minikube (works only with kvm2 driver on Linux)")
 	startCmd.Flags().Bool(hidden, false, "Hide the hypervisor signature from the guest in minikube (works only with kvm2 driver on Linux)")
 	startCmd.Flags().Bool(noVTXCheck, false, "Disable checking for the availability of hardware virtualization before the vm is started (virtualbox)")
+	startCmd.Flags().Bool(noNativeSSH, false, "Disable using the native Golang SSH client and instead uses the command line 'ssh' command when accessing the docker machine. Useful for the machine drivers when they will not start with 'Waiting for SSH'.")


I would make the description shorter, instead of Useful for the machine drivers when they will not start with 'Waiting for SSH'. I would link to an issue or a docs.md

like see github.com/kubernetes/minikube/docs/drivers#nossh.md

or do you know list of all vm-drivers that have that problem to list here instead ? if we know that, does it worth to make an automatic option for specific drivers? we already created an automatic option for none driver.
#4465

There have been NUMEROUS issues related to "Waiting for SSH" for almost every VM driver which is why I specifically mentioned it as a possible fix when you encounter that issue.

The issue is that it isn't "required" by the Hyperkit driver. It's required if you have a USB network adapter like a USB dongle or a docking station that provides wired network access for a laptop (which is why I made it an optional flag instead of setting it automatically for the hyperkit driver).

Fair enough, I know this issue has happened for sure for none driver, I wonder if it has happened to virtualbox as well ? I would okay with testing this PR

My only debate with naming in this PR relates to the name of the arg itself.

Should it be "--no-native-ssh" since it mirrors the "--native-ssh" arg from 'docker machine ssh' and effectively affects its opposite or should it be "--external-ssh" (or maybe --ssh-cli) so that it implies what it actually does?

medyagh · 2019-06-19T05:42:51Z

@minikubebot OK to test

RA489 · 2019-06-19T10:25:15Z

@minikube-bot OK to test

yinzara · 2019-06-26T22:25:43Z

I need a little help deciphering what went wrong. It seems the Linux-VirtualBox test somehow failed but I can't figure out what's causing it. Help!

yinzara · 2019-06-28T17:34:39Z

I've rebased against master and force pushed. All builds are "Pending" now, does something need to be done?

yinzara · 2019-06-28T21:14:57Z

Nvm everything is good now :) . Can I get a lgtm?

medyagh · 2019-06-28T21:19:18Z

lets wait for @tstromberg's input

RA489 · 2019-07-01T07:08:58Z

@minikube-bot OK to test

yinzara · 2019-07-01T16:28:09Z

I still don't understand how the VirtualBox-Linux test is failing. My change is so minor and should have no affect unless specifically enabled (which it isn't by default). Is it possible this is unrelated to my changes? The errors are so cryptic I can't decipher what's going on.

yinzara · 2019-07-01T16:33:23Z

2028 ssh_runner.go:101] SSH: docker load -i /tmp/k8s-dns-kube-dns-amd64_1.14.13

That seems to be the line that's erroring. All of the docker load... lines before it are passing but that one seems to fail.

medyagh · 2019-07-02T02:20:09Z

I believe the vbox failing test was timing out

medyagh · 2019-07-02T02:20:30Z

/retest this please

yinzara · 2019-07-02T17:20:13Z

Yay! All we need is a lgtm :) . @tstromberg ?

tstromberg · 2019-07-02T18:02:55Z

Though the native SSH client for Golang is more efficient than the command line 'ssh' command, there are times in which using the 'ssh' CLI is necessary.

First, thank you for this PR!

I want to make sure I understand the technical merits of adding another flag before LGTM'ing, as once we do, we are stuck supporting it for the long haul. Do you mind elaborating what specific use cases this flag this flag would be useful for in the PR description and help text?

If the minikube VM used an exotic authentication mechanism, I would completely understand why using the external ssh mechanism is useful, but I don't yet understand other reasons for it. I would expect that the packets will be routed similarly.

In general, I would personally would prefer having an automatic fallback/selection mechanism for flags like this, rather than forcing the user to fail first and try again, but I don't understand enough about the problem yet to have a more specific recommendation.

yinzara · 2019-07-02T18:25:30Z

Unfortunately the reason for needing this flag are not always clear. In my example with the Hyperkit driver, when a USB network adapter was added to the system, the Golang SSH library was unable to connect to the Hyperkit container. It just sat waiting forever at "Waiting for SSH" without ever timing out. It was literally frozen and didn't time out (so I couldn't have "fallen back automatically" even if I wanted to).

While I would love to have solved the cause for this, there are TONS of issues over the years related to issues with "Waiting for SSH..." that trying this option may have allowed a user to continue. If we know the command line SSH client does work, having a method for enabling this is necessary (as docker-machine ssh already does). The docker-machine library doesn't even provide more information about the use of this flag other than its existence, how to use it, and that it may provide different functionality.
https://docs.docker.com/machine/reference/ssh/

To be perfectly honest, I'd actually rather model the docker-machine ssh library and change the default behavior to use the CLI like docker-machine does and provide a "--native-ssh" flag just like they do. However, that would cause a change in the default behavior of minikube and is a much bigger ask.

yinzara · 2019-07-02T19:12:45Z

Looks like this was changed in 2017 as part of:
eafb4fc

RA489 · 2019-07-03T01:17:38Z

@minikube-bot OK to test

tstromberg · 2019-07-02T17:50:05Z

cmd/minikube/cmd/start.go

@@ -160,6 +163,7 @@ func init() {
 	startCmd.Flags().Bool(gpu, false, "Enable experimental NVIDIA GPU support in minikube (works only with kvm2 driver on Linux)")
 	startCmd.Flags().Bool(hidden, false, "Hide the hypervisor signature from the guest in minikube (works only with kvm2 driver on Linux)")
 	startCmd.Flags().Bool(noVTXCheck, false, "Disable checking for the availability of hardware virtualization before the vm is started (virtualbox)")
+	startCmd.Flags().Bool(noNativeSSH, false, "Disable using the native Golang SSH client and instead uses the command line 'ssh' command when accessing the docker machine. Useful for the machine drivers when they will not start with 'Waiting for SSH'.")


Boolean flags should not be named with a negative prefix, because otherwise it is really difficult to understand what this means:

--no-native-ssh=false

I regret that we did not catch this for --no-vtx-check.

So then should we change the parameter to be "--native-ssh" to match with the corresponding parameter in docker-machine ssh and the default behavior to use the "External" CLI SSH (as it was before) eafb4fc (and as it is in docker-machine ssh) or should I change the parameter to be "--external-ssh" and maintain the current default behavior (even though it is the opposite of docker-machine ssh)

Correct. Name the flag --native-ssh, but do not yet change the default behavior.

tstromberg · 2019-07-17T20:00:10Z

Needs rebase.

yinzara · 2019-07-17T23:11:11Z

lol I think you missed the two options. I can either name the parameter "--native-ssh" (like docker-machine ssh names it) and change the default behavior to use External SSH (like docker-machine ssh) by removing the line from the eafb4fc or I can name the parameter "--external-ssh" and leave the default the way it is currently. Adding a "--native-ssh" parameter would do nothing unless I change the default behavior.

RA489 · 2019-07-18T00:28:40Z

@yinzara Please resolve the conflicts.

sharifelgamal · 2019-09-05T18:25:54Z

Could you change the name to be --native-ssh while still defaulting the behavior to true? Ideally changing the default behavior would be in a separate PR. This way, it can still be set to false when needed.

yinzara · 2019-09-05T21:34:52Z

I don't know how to make myself clearer lol

If I call it --native-ssh while still defaulting the native behavior to true, this PR will literally do nothing. That would mean if I call "minikube ssh" it would use native SSH no matter if I use the --native-ssh flag or not and there will be no way of using the external CLI ssh (the exact reason for this PR).

There are literally only two options (and a variation on the 1st in naming).

Call it --no-native-ssh (this is the current state of the PR) or --external-ssh and keep the default the same (i.e. native SSH) as it is now (even though its the opposite of docker-machine ssh). Specifying the flag would cause it to use the "external" CLI SSH.

OR

Call it --native-ssh (like docker-machine ssh) and change the default to use "external" CLI SSH matching the behavior of docker-machine ssh. Specifying the flag would cause it to use native SSH.

sharifelgamal · 2019-09-05T21:59:17Z

--native-ssh=false would do exactly what you want without having to change the parameter name or the default behavior.

yinzara · 2019-09-05T22:24:20Z

INTERESTING!

I didn't realize that was an option (I can't see any other minikube options like that).

I guess I could call it --native-ssh and make it default to "true" when not specified but if specified as false, follow the value given. Then if we change the default behavior, the CLI flag doesn't change. I didn't realize viper had that ability.

k8s-ci-robot · 2019-09-06T00:37:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yinzara
To complete the pull request process, please assign sharifelgamal
You can assign the PR to them by writing /assign @sharifelgamal 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

yinzara · 2019-09-06T00:40:17Z

There. Rebased against master and renamed to --native-ssh (where --native-ssh=false will enable use of the "external" CLI ssh).

Also added the flag to the minikube ssh command

yinzara · 2019-09-06T00:40:45Z

/assign @sharifelgamal

sharifelgamal · 2019-09-06T16:56:27Z

Excellent, thanks for fixing that!

vmvmpvm · 2020-07-31T15:56:36Z

Does this flag work?

I have minikube version 1.12.1, I still cannot use external ssh, I am running below command on Windows 10 using Hyper-V.

minikube start -v=8 --alsologtostderr --driver=hyperv --force --native-ssh=false

yinzara · 2020-07-31T18:08:03Z

Yup we use it all the time and it works great. What do you mean by "I still cannot use external ssh"?

vmvmpvm · 2020-07-31T19:32:38Z

Sorry, I should have provided more details, it always uses native ssh in my case

I am using minikube version: v1.12.1.

I execute below command

minikube start -v8 --alsologtostderr --profile minikube03 --driver hyperv --force --hyperv-virtual-switch "External Switch" --native-ssh=false

I see it is using native ssh

I0731 15:17:41.393688    8300 main.go:115] libmachine: **Using SSH client type: native**
I0731 15:17:41.394690    8300 main.go:115] libmachine: &{{{<nil> 0 [] [] []} docker [0x7b7070] 0x7b7040 <nil>  [] 0s} myvmip 22 <nil> <nil>}
I0731 15:17:41.395689    8300 main.go:115] libmachine: About to run SSH command:
sudo poweroff
I0731 15:17:52.360497    8300 main.go:115] libmachine: Error dialing TCP: dial tcp myvmip:22: connectex: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.
I0731 15:18:02.398113    8300 main.go:115] libmachine: Error dialing TCP: dial tcp myvmip:22: connectex: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.
I0731 15:18:16.363667    8300 main.go:115] libmachine: Error dialing TCP: dial tcp myvmip:22: connectex: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.
I0731 15:18:26.399598    8300 main.go:115] libmachine: Error dialing TCP: dial tcp myvmip:22: connectex: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.
....

How do I make it detect my External SSH?, Just fyi, when I run minishift it calls my external ssh just fine.

vmvmpvm · 2020-07-31T19:46:28Z

using Hyper-V on Windows 10

vmvmpvm · 2020-07-31T23:21:29Z

My external ssh config is configured to map myvmip ssh to a forwarded port, I need minikube to use the external ssh config for it to work.

sharifelgamal · 2020-08-03T16:50:25Z

@vmvmpvm I've opened #8907 to fix your issue.

vmvmpvm · 2020-08-03T20:35:31Z

Thanks @sharifelgamal

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

k8s-ci-robot requested review from afbjorklund and balopat June 17, 2019 17:41

afbjorklund reviewed Jun 17, 2019

View reviewed changes

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 17, 2019

medyagh reviewed Jun 17, 2019

View reviewed changes

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 Jun 29, 2019

tstromberg suggested changes Jul 11, 2019

View reviewed changes

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

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

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

Add "native-ssh" flag to "start" and "ssh" commands and config

f1b3afe

k8s-ci-robot assigned sharifelgamal Sep 6, 2019

sharifelgamal changed the title ~~Add 'no-native-ssh' flag to 'minikube start'~~ Add 'native-ssh' flag to 'minikube start' and 'minikube ssh' Sep 6, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 6, 2019

sharifelgamal approved these changes Sep 6, 2019

View reviewed changes

sharifelgamal merged commit e974ce3 into kubernetes:master Sep 6, 2019

Add 'native-ssh' flag to 'minikube start' and 'minikube ssh' #4510

Add 'native-ssh' flag to 'minikube start' and 'minikube ssh' #4510

Conversation

yinzara commented Jun 17, 2019 • edited Loading

k8s-ci-robot commented Jun 17, 2019

k8s-ci-robot commented Jun 17, 2019

k8s-ci-robot commented Jun 17, 2019

minikube-bot commented Jun 17, 2019

afbjorklund Jun 17, 2019

Choose a reason for hiding this comment

yinzara Jun 17, 2019

Choose a reason for hiding this comment

medyagh Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

yinzara Jun 18, 2019

Choose a reason for hiding this comment

medyagh Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

yinzara Jun 19, 2019

Choose a reason for hiding this comment

medyagh commented Jun 19, 2019

RA489 commented Jun 19, 2019

yinzara commented Jun 26, 2019

yinzara commented Jun 28, 2019

yinzara commented Jun 28, 2019

medyagh commented Jun 28, 2019

RA489 commented Jul 1, 2019

yinzara commented Jul 1, 2019

yinzara commented Jul 1, 2019

medyagh commented Jul 2, 2019

medyagh commented Jul 2, 2019

yinzara commented Jul 2, 2019

tstromberg commented Jul 2, 2019

yinzara commented Jul 2, 2019 • edited Loading

yinzara commented Jul 2, 2019

RA489 commented Jul 3, 2019

tstromberg Jul 2, 2019

Choose a reason for hiding this comment

yinzara Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

tstromberg Jul 17, 2019

Choose a reason for hiding this comment

tstromberg commented Jul 17, 2019

yinzara commented Jul 17, 2019 • edited Loading

RA489 commented Jul 18, 2019

sharifelgamal commented Sep 5, 2019

yinzara commented Sep 5, 2019 • edited Loading

sharifelgamal commented Sep 5, 2019

yinzara commented Sep 5, 2019

k8s-ci-robot commented Sep 6, 2019

yinzara commented Sep 6, 2019

yinzara commented Sep 6, 2019

sharifelgamal commented Sep 6, 2019

vmvmpvm commented Jul 31, 2020

yinzara commented Jul 31, 2020

vmvmpvm commented Jul 31, 2020 • edited Loading

vmvmpvm commented Jul 31, 2020

vmvmpvm commented Jul 31, 2020

sharifelgamal commented Aug 3, 2020

vmvmpvm commented Aug 3, 2020

yinzara commented Jun 17, 2019 •

edited

Loading

medyagh Jun 17, 2019 •

edited

Loading

medyagh Jun 18, 2019 •

edited

Loading

yinzara commented Jul 2, 2019 •

edited

Loading

yinzara Jul 11, 2019 •

edited

Loading

yinzara commented Jul 17, 2019 •

edited

Loading

yinzara commented Sep 5, 2019 •

edited

Loading

vmvmpvm commented Jul 31, 2020 •

edited

Loading