Add new command: `ignite exec` #232

BenTheElder · 2019-07-21T09:00:18Z

This is a pretty simple / MVP implementation, however it is enough to perform basic exec (over ssh) against a running ignite VM.

TODO:

inherit out/err/in writers from the cobra command (other commands seem to be ignoring these inputs anyhow...)
docs (?)
support for setting environment variables (also we should consider other docker exec options? though -i already conflicts ...)
more extensive testing
consider porting ignite ssh to use the same packages

How this works:

We open an ssh connection to the VM and create a session with golang.org/x/crypto/ssh
We then request a pseudo tty in the session and wire up stdin / stdout / stderr to the host
Since ssh requires sending the command as a single string, args[1:] are converted to args[1] + strings.Join(shellEscape(args[2:...], " ") and run that way (pseudo code)

Additionally, the exit code from the command is preserved if possible.

$ bin/ignite exec centos echo '"hello world"'
"hello world"

$ bin/ignite exec centos -- /bin/bash -c 'echo hello world "$HOSTNAME"'
hello world 50a7f9aa26409ab3

Fixes #65

BenTheElder · 2019-07-21T09:03:21Z

tracking issue: #142

alexellis · 2019-07-22T08:00:22Z

This looks very useful Ben 👍 Out of interest, is it required for KinD to work?

alexellis · 2019-07-22T08:01:51Z

cmd/ignite/run/exec.go

+
+func newSSHConfig(publicKey ssh.Signer, timeout uint32) *ssh.ClientConfig {
+	return &ssh.ClientConfig{
+		User: "root",


Is there a scenario where root may not be the user wanted for ssh - VMs with "disable root login"?

I think so, but for the moment this matches ignite ssh which also assumes the user is root.

The disable root login option default in most distributions prevents password-based authentication, but SSH keys should work just fine. We should also open an issue about allowing user selection in ssh and exec.

alexellis · 2019-07-22T08:03:49Z

cmd/ignite/run/exec.go

+}
+
+// joinShellCommand joins command parts into a single string safe for passing to sh -c (or SSH)
+func joinShellCommand(command []string) string {


@rgee0 @matipan is this method/package any use to us with faas-cli or ofc-bootstrap?

BenTheElder · 2019-07-22T09:20:14Z

This looks very useful Ben 👍 Out of interest, is it required for KinD to work?

kind requires some way to execute commands, we could implement this in kind some other way (driving SSH? Implementing this logic there?), but I think it's more generally useful to have this in ignite.

One other note vs docker exec, docker exec will treat everything after the container name/ID as part of the command. IE docker exec centos bash --version works with --version as a argument to bash, not docker vs this PR requiring ignite exec centos -- bash --version.

We could implement this but I think we'd need our own flag processing step, I don't think pflag / cobra alone can support this

BenTheElder · 2019-07-22T09:22:35Z

cmd/ignite/run/exec.go

+		return joined
+	}
+	for _, arg := range command[1:] {
+		// NOTE: we need to escape nested single quotes


Slightly stale comment from before using the shellescape package.

You can go ahead and remove the comment then 👍

updated to a less stale comment

twelho

Good work, thanks!
Some nits, after those are fixed, let's go ahead and merge 👍
Remember to run make tidy again to update the doc descriptions after editing them.

twelho · 2019-07-23T09:58:13Z

cmd/ignite/cmd/vmcmd/exec.go

+	"github.com/weaveworks/ignite/pkg/errutils"
+)
+
+// NewCmdExec exec's into a running vm


twelho · 2019-07-23T09:58:49Z

cmd/ignite/cmd/vmcmd/exec.go

+
+	cmd := &cobra.Command{
+		Use:   "exec <vm> <command...>",
+		Short: "exec command in a running vm",


"execute a command in a running VM"

twelho · 2019-07-23T09:59:27Z

cmd/ignite/cmd/vmcmd/exec.go

+		Use:   "exec <vm> <command...>",
+		Short: "exec command in a running vm",
+		Long: dedent.Dedent(`
+			exec command in a running VM using ssh and the private key created for it during generation.


Execute a command...
ssh -> SSH

twelho · 2019-07-23T09:59:45Z

cmd/ignite/cmd/vmcmd/exec.go

+}
+
+func addExecFlags(fs *pflag.FlagSet, ef *run.ExecFlags) {
+	fs.StringVarP(&ef.IdentityFile, "identity", "i", "", "Override the vm's default identity file")


this matches the ssh command's flags exactly at the moment, should we update it there too?

twelho · 2019-07-23T10:00:24Z

cmd/ignite/run/exec.go

+	api "github.com/weaveworks/ignite/pkg/apis/ignite"
+	"github.com/weaveworks/ignite/pkg/constants"
+	"github.com/weaveworks/ignite/pkg/util"
+


Remove this blank line and run make tidy again 🙂

twelho · 2019-07-23T10:05:49Z

cmd/ignite/run/exec.go

+	// TODO: we should probably configure the terminal modes
+	modes := ssh.TerminalModes{}
+	if err := session.RequestPty("xterm", 80, 40, modes); err != nil {
+		return fmt.Errorf("request for psuedo terminal failed: %v", err)


psuedo -> pseudo

twelho · 2019-07-23T10:06:54Z

cmd/ignite/run/exec.go

+	// TODO: should we request something other than xterm?
+	// TODO: we should probably configure the terminal modes
+	modes := ssh.TerminalModes{}
+	if err := session.RequestPty("xterm", 80, 40, modes); err != nil {


Defaulting to xterm is fine for now, we could later also allow changing this via a flag. Anyways this is non-blocking for this initial implementation.

twelho · 2019-07-23T10:07:59Z

cmd/ignite/run/exec.go

+	go io.Copy(os.Stderr, stderr)
+	stdout, err := session.StdoutPipe()
+	if err != nil {
+		return fmt.Errorf("failed to connect stderr: %v", err)


stderr -> stdout

twelho · 2019-07-23T10:08:30Z

cmd/ignite/run/exec.go

+	go io.Copy(os.Stdout, stdout)
+	stdin, err := session.StdinPipe()
+	if err != nil {
+		return fmt.Errorf("failed to connect stderr: %v", err)


stderr -> stdin

twelho · 2019-07-23T10:10:02Z

cmd/ignite/run/exec.go

+		return joined
+	}
+	for _, arg := range command[1:] {
+		// NOTE: we need to escape nested single quotes


You can go ahead and remove the comment then 👍

BenTheElder

addressed comments

BenTheElder · 2019-07-24T05:57:59Z

cmd/ignite/cmd/vmcmd/exec.go

+	"github.com/weaveworks/ignite/pkg/errutils"
+)
+
+// NewCmdExec exec's into a running vm


BenTheElder · 2019-07-24T05:58:05Z

cmd/ignite/cmd/vmcmd/exec.go

+
+	cmd := &cobra.Command{
+		Use:   "exec <vm> <command...>",
+		Short: "exec command in a running vm",


BenTheElder · 2019-07-24T05:58:11Z

cmd/ignite/cmd/vmcmd/exec.go

+		Use:   "exec <vm> <command...>",
+		Short: "exec command in a running vm",
+		Long: dedent.Dedent(`
+			exec command in a running VM using ssh and the private key created for it during generation.


BenTheElder · 2019-07-24T05:58:20Z

cmd/ignite/run/exec.go

+	api "github.com/weaveworks/ignite/pkg/apis/ignite"
+	"github.com/weaveworks/ignite/pkg/constants"
+	"github.com/weaveworks/ignite/pkg/util"
+


BenTheElder · 2019-07-24T05:59:44Z

cmd/ignite/run/exec.go

+		ExecFlags: ef,
+		command:   command,
+	}
+	eo.vm, err = getVMForMatch(vmMatch)


BenTheElder · 2019-07-24T06:00:17Z

cmd/ignite/run/exec.go

+		return fmt.Errorf("failed to dial: %v", err)
+	}
+
+	// run the command, DO NOT wrap this error as the caller can check for the command exit


BenTheElder · 2019-07-24T06:00:41Z

cmd/ignite/run/exec.go

+	// TODO: we should probably configure the terminal modes
+	modes := ssh.TerminalModes{}
+	if err := session.RequestPty("xterm", 80, 40, modes); err != nil {
+		return fmt.Errorf("request for psuedo terminal failed: %v", err)


BenTheElder · 2019-07-24T06:01:03Z

cmd/ignite/run/exec.go

+	go io.Copy(os.Stderr, stderr)
+	stdout, err := session.StdoutPipe()
+	if err != nil {
+		return fmt.Errorf("failed to connect stderr: %v", err)


BenTheElder · 2019-07-24T06:01:07Z

cmd/ignite/run/exec.go

+	go io.Copy(os.Stdout, stdout)
+	stdin, err := session.StdinPipe()
+	if err != nil {
+		return fmt.Errorf("failed to connect stderr: %v", err)


BenTheElder · 2019-07-24T06:02:15Z

cmd/ignite/run/exec.go

+		return joined
+	}
+	for _, arg := range command[1:] {
+		// NOTE: we need to escape nested single quotes


updated to a less stale comment

BenTheElder · 2019-07-24T06:08:36Z

rebased

luxas

Thanks A LOT @BenTheElder ✨! This looks wonderful :)

LGTM

luxas · 2019-07-29T13:20:53Z

We can follow-up the possible improvements here in an other issue later, but for now, merging this to get us started. Next, integrate with kind 😉

alexellis reviewed Jul 22, 2019

View reviewed changes

BenTheElder commented Jul 22, 2019

View reviewed changes

twelho reviewed Jul 23, 2019

View reviewed changes

BenTheElder force-pushed the exec branch from e6fece6 to 2a60cdb Compare July 24, 2019 06:03

BenTheElder commented Jul 24, 2019

View reviewed changes

BenTheElder added 3 commits July 23, 2019 23:06

vendor golang.org/x/crypto/ssh

fb7d001

vendor shellescape

72875d0

implement ignite exec

a403351

BenTheElder force-pushed the exec branch from 2a60cdb to a403351 Compare July 24, 2019 06:08

luxas approved these changes Jul 29, 2019

View reviewed changes

luxas added this to the v0.5.0 milestone Jul 29, 2019

luxas added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 29, 2019

luxas self-assigned this Jul 29, 2019

luxas merged commit 9ce303e into weaveworks:master Jul 29, 2019

luxas mentioned this pull request Jul 31, 2019

Follow-up improvements for ignite exec #253

Open

luxas changed the title ~~implement ignite exec~~ Add new command: ignite exec Aug 6, 2019

Add new command: ignite exec #232

Add new command: ignite exec #232

Conversation

BenTheElder commented Jul 21, 2019 • edited by luxas Loading

BenTheElder commented Jul 21, 2019

alexellis commented Jul 22, 2019

Choose a reason for hiding this comment

BenTheElder Jul 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Jul 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twelho 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

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

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

BenTheElder 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

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

BenTheElder commented Jul 24, 2019

luxas left a comment

Choose a reason for hiding this comment

luxas commented Jul 29, 2019

Add new command: `ignite exec` #232

Add new command: `ignite exec` #232

BenTheElder commented Jul 21, 2019 •

edited by luxas

Loading

BenTheElder Jul 22, 2019 •

edited

Loading