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 interactive shell when using native-client #1571

Merged
merged 1 commit into from
Jun 9, 2017

Conversation

gbraad
Copy link
Contributor

@gbraad gbraad commented Jun 9, 2017

This fixes the way the ssh command is invoking the shell, and will therefore allow interactive use with the Go native client.

Steps to reproduce:

$ sudo mv /usr/bin/ssh /usr/bin/-ssh
$ minikube start
$ minikube ssh
$ 

Expected:

$ sudo mv /usr/bin/ssh /usr/bin/-ssh
$ minikube start
$ minikube ssh 
                        ##         .
                  ## ## ##        ==
               ## ## ## ## ##    ===
           /"""""""""""""""""\___/ ===
      ~~~ {~~ ~~~~ ~~~ ~~~~ ~~~ ~ /  ===- ~~~
           \______ o           __/
             \    \         __/
              \____\_______/
 docker@minikube:~$

Due to #1531 I am unable to verify the build myself...

We invoke the shell as follows https://github.com/kubernetes/minikube/blob/master/pkg/minikube/cluster/cluster.go#L537

	return client.Shell(strings.Join(args, " "))

What happens is that we provide an empty string instead of no value: https://play.golang.org/p/z7DDUWnuYm You can verify this with:

package main

import (
	"strings"
	"fmt"
	
)

func main() {
	args := []string {}
	command := strings.Join(args, " ")
	fmt.Printf("Type: %T, Length: %v, Value: %v.\n", command, len(command), command)
}

At https://github.com/docker/machine/blob/master/libmachine/ssh/client.go#L269 the use of args ...string will result in args having a length of 1.

func (client *NativeClient) Shell(args ...string) error {

this means that the check at https://github.com/docker/machine/blob/master/libmachine/ssh/client.go#L318 wrongly assumes that arguments are given, and it will execute https://github.com/docker/machine/blob/master/libmachine/ssh/client.go#L324

		session.Run(strings.Join(args, " "))

Docker machine itself uses a different way to invoke the shell https://github.com/docker/machine/blob/e1a03348ad83d8e8adb19d696bc7bcfb18ccd770/commands/ssh.go#L50

	return client.Shell(c.Args().Tail()...)

Therefore fixing this with:

	return client.Shell(args...)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 9, 2017
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@codecov-io
Copy link

Codecov Report

Merging #1571 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1571   +/-   ##
=======================================
  Coverage   39.52%   39.52%           
=======================================
  Files          50       50           
  Lines        2548     2548           
=======================================
  Hits         1007     1007           
  Misses       1375     1375           
  Partials      166      166
Impacted Files Coverage Δ
pkg/minikube/cluster/cluster.go 45.96% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94cb85f...03d3e1f. Read the comment docs.

@dlorenc
Copy link
Contributor

dlorenc commented Jun 9, 2017

@minikube-bot ok to test

@r2d4
Copy link
Contributor

r2d4 commented Jun 9, 2017

Nice, thanks!

@gbraad
Copy link
Contributor Author

gbraad commented Jun 9, 2017

@r2d4

Linux-KVM-Alt fails with:

+ pushd /home/jenkins/go/src/github.com/r2d4/docker-machine-driver-kvm
linux_integration_tests_kvm_alt.sh: line 36: pushd: /home/jenkins/go/src/github.com/r2d4/docker-machine-driver-kvm: No such file or directory
Build step 'Execute shell' marked build as failure

and no visible output for the Windows-HyperV test.

@r2d4
Copy link
Contributor

r2d4 commented Jun 9, 2017

Yeah thats fine - those are just problems with the testing infrastructure. I'll resolve the Linux-Alt driver issue separately later.

@r2d4 r2d4 merged commit 8dd1af7 into kubernetes:master Jun 9, 2017
@gbraad gbraad deleted the interactive-ssh branch June 9, 2017 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants