-
Notifications
You must be signed in to change notification settings - Fork 42
Conversation
pkg/cmd/ssh_gcp.go
Outdated
if debugSwitch { | ||
sshCmd = fmt.Sprintf("ssh -v -i " + key + " -o \"ProxyCommand ssh -W %%h:%%p -i " + key + " -o IdentitiesOnly=yes -o StrictHostKeyChecking=no " + bastionNode + "\" " + node + " -o IdentitiesOnly=yes -o StrictHostKeyChecking=no") | ||
} else { | ||
sshCmd = fmt.Sprintf("ssh -i " + key + " -o \"ProxyCommand ssh -W %%h:%%p -i " + key + " -o IdentitiesOnly=yes -o StrictHostKeyChecking=no " + bastionNode + "\" " + node + " -o IdentitiesOnly=yes -o StrictHostKeyChecking=no") | ||
} | ||
|
||
fmt.Println(sshCmd) | ||
cmd := exec.Command("bash", "-c", sshCmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, if you already touch this code, directly call ssh instead of bash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't touch this code here, only change just add -v for ssh verbose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see. I will remove it seem just find bash in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, and we have a dedicate issue for remove bash #267 but it seems not start yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but if you touch this code anyhow it makes sense to directly fix at least this part
pkg/cmd/ssh_gcp.go
Outdated
var sshCmd string | ||
if debugSwitch { | ||
sshCmd = fmt.Sprintf("ssh -v -i " + key + " -o \"ProxyCommand ssh -W %%h:%%p -i " + key + " -o IdentitiesOnly=yes -o StrictHostKeyChecking=no " + bastionNode + "\" " + node + " -o IdentitiesOnly=yes -o StrictHostKeyChecking=no") | ||
} else { | ||
sshCmd = fmt.Sprintf("ssh -i " + key + " -o \"ProxyCommand ssh -W %%h:%%p -i " + key + " -o IdentitiesOnly=yes -o StrictHostKeyChecking=no " + bastionNode + "\" " + node + " -o IdentitiesOnly=yes -o StrictHostKeyChecking=no") | ||
} | ||
|
||
fmt.Println(sshCmd) | ||
cmd := exec.Command("bash", "-c", sshCmd) | ||
cmd := exec.Command(sshCmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried it? This does not work. The signature of exec.Command is Command(name string, arg ...string)
, hence the first argument would be ssh and the rest is put in as args
When you have the args as array, you can append the debug flag without the need to duplicate the command. This is how I would do it:
args := []string{"-i" + key, "-oProxyCommand=ssh -W%h:%p -i" + key + " -oIdentitiesOnly=yes -oStrictHostKeyChecking=no " + bastionNode, node, "-oIdentitiesOnly=yes", "-oStrictHostKeyChecking=no"}
if debugSwitch {
args = append([]string{"-vvv"}, args...)
}
fmt.Println("ssh " + strings.Join(args[:], " "))
cmd := exec.Command("ssh", args...)
You could also pass a debug flag to the ssh command in the proxy command argument, I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I like this tips. updated, and did some testing
args = append([]string{"-vvv"}, append(args[:2], append([]string{"-vvv"}, args[2:]...)...)...)
, but unfortunately not working somehow.
[-vvv -i/Users/i333878/.garden/cache/canary-virtual/projects/i333878/b68cklqfl1/key -oProxyCommand=ssh -W%h:%p -vvv -i/Users/i333878/.garden/cache/canary-virtual/projects/i333878/b68cklqfl1/key -oIdentitiesOnly=yes -oStrictHostKeyChecking=no gardener@34.77.207.104 gardener@shoot--i333878--b68cklqfl1-worker-b5hoj-z1-c467bc486-jqsg6 -oIdentitiesOnly=yes -oStrictHostKeyChecking=no]
debug1: identity file /Users/i333878/.garden/cache/canary-virtual/projects/i333878/b68cklqfl1/key-cert type -1
debug1: Local version string SSH-2.0-OpenSSH_8.1
usage: ssh [-46AaCfGgKkMNnqsTtVvXxYy] [-B bind_interface]
[-b bind_address] [-c cipher_spec] [-D [bind_address:]port]
[-E log_file] [-e escape_char] [-F configfile] [-I pkcs11]
[-i identity_file] [-J [user@]host[:port]] [-L address]
[-l login_name] [-m mac_spec] [-O ctl_cmd] [-o option] [-p port]
[-Q query_option] [-R address] [-S ctl_path] [-W host:port]
[-w local_tun[:remote_tun]] destination [command]
kex_exchange_identification: Connection closed by remote host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tedteng if you want to add the verbose flag also to the ProxyCommand you have to do something like this
proxyCommandArgs := []string{"-W%h:%p", "-i" + key, "-oIdentitiesOnly=yes", "-oStrictHostKeyChecking=no", bastionNode}
if debugSwitch {
proxyCommandArgs = append([]string{"-vvv"}, proxyCommandArgs...)
}
args := []string{"-i" + key, "-oProxyCommand=ssh " + strings.Join(proxyCommandArgs[:], " "), node, "-oIdentitiesOnly=yes", "-oStrictHostKeyChecking=no"}
if debugSwitch {
args = append([]string{"-vvv"}, args...)
}
fmt.Println("ssh " + strings.Join(args[:], " "))
cmd := exec.Command("ssh", args...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
give it up, . It seems can't fit in one line. Thanks, It works now
pkg/cmd/ssh_gcp.go
Outdated
if debugSwitch { | ||
args = append([]string{"-vvv"}, args...) | ||
} | ||
fmt.Println(args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously the whole ssh command was printed (including ssh in the beginning). Now you are printing just the args, which could be a bit out of context for the operator reading the gardenctl stdout.
You are also printing it as array representation [arg1, arg2, ..] instead of args1 arg2 ...
That's why my suggestion was the following, which should nicely print out the command
fmt.Println("ssh " + strings.Join(args[:], " "))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, forgot to change it back as it uses for debug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
but I'm not sure if you still want to add the verbose flag to the proxy command or not (#297 (comment))
|
done |
What this PR does / why we need it:
user
role able tossh
GCP nodeWhich issue(s) this PR fixes:
Fixes #294
Special notes for your reviewer:
@petersutter
Release note: