Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

user role able to access GCP #297

Merged
merged 1 commit into from
Sep 7, 2020

Conversation

tedteng
Copy link
Contributor

@tedteng tedteng commented Sep 2, 2020

What this PR does / why we need it:
user role able to ssh GCP node

Which issue(s) this PR fixes:
Fixes #294

Special notes for your reviewer:
@petersutter

Release note:


@tedteng tedteng requested a review from a team as a code owner September 2, 2020 08:09
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 2, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 2, 2020
pkg/cmd/ssh_gcp.go Outdated Show resolved Hide resolved
Comment on lines 81 to 88
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)
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 2, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 2, 2020
Comment on lines 80 to 88
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)
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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...)

Copy link
Contributor Author

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

@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 3, 2020
if debugSwitch {
args = append([]string{"-vvv"}, args...)
}
fmt.Println(args)
Copy link
Contributor

@petersutter petersutter Sep 3, 2020

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[:], " "))

Copy link
Contributor Author

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.

@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 3, 2020
Copy link
Contributor

@petersutter petersutter left a 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))

@gardener-robot gardener-robot added the reviewed/lgtm Has approval for merging label Sep 3, 2020
@tedteng
Copy link
Contributor Author

tedteng commented Sep 4, 2020

/lgtm
but I'm not sure if you still want to add the verbose flag to the proxy command or not (#297 (comment))
thanks, refresh my mind, will continue to investigate it after I am completed ssh azure.

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 4, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 4, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 4, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 4, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 4, 2020
@tedteng
Copy link
Contributor Author

tedteng commented Sep 4, 2020

/lgtm
but I'm not sure if you still want to add the verbose flag to the proxy command or not (#297 (comment))

done

@tedteng tedteng deleted the user_gcp branch September 15, 2020 01:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enhance user role able to access GCP host
7 participants