fix aws ssh issue #446

neo-liang-sap · 2020-11-16T05:32:40Z

What this PR does / why we need it:
fix aws ssh issue
Which issue(s) this PR fixes:
Fixes #442

Special notes for your reviewer:

in my env, nc -vtnz ip port returns sth like 127.0.0.1 49269 open so the result should grep open, instead of succeed
not sure my nc is correct one

nc --version                                               (ccee-m3/default)
netcat (The GNU Netcat) 0.7.1
Copyright (C) 2002 - 2003  Giovanni Giacobbi

This program comes with NO WARRANTY, to the extent permitted by law.
You may redistribute copies of this program under the terms of
the GNU General Public License.
For more information about these matters, see the file named COPYING.

Original idea and design by Avian Research <hobbit@avian.org>,
Written by Giovanni Giacobbi <giovanni@giacobbi.net>.

fix a small typo, shh to ssh
when bastion exist, a.BastionPrivIP and a.BastionIP should also be assigned with values (like what they are treat in non bastion exist path) , otherwise following step will fail due to these two properties empty

Release note:

fix aws ssh issue

neo-liang-sap · 2020-11-16T07:25:55Z

testing result

The string output check should be succeeded

dansible · 2020-11-19T21:00:10Z

pkg/cmd/ssh_aws.go

@@ -242,7 +246,7 @@ func (a *AwsInstanceAttribute) sshPortCheck() {
 		cmd := exec.Command("bash", "-c", ncCmd)
 		output, _ := cmd.CombinedOutput()
 		fmt.Println("=>", string(output))
-		if strings.Contains(string(output), "succeeded") {
+		if strings.Contains(string(output), "open") {


It looks like this should be succeeded.

With open, I get:

Bastion host instance running. Opened SSH Port on Node. Waiting 60 seconds for Bastion SSH port open => Connection to ... port 22 [tcp/*] succeeded! => Connection to ... port 22 [tcp/*] succeeded! => Connection to ... port 22 [tcp/*] succeeded! => Connection to ... port 22 [tcp/*] succeeded! => Connection to ... port 22 [tcp/*] succeeded! => Connection to ... port 22 [tcp/*] succeeded! SSH Port Open on Bastion TimeOut (4/4) Cleanup Cleaning up bastion host configurations...

With succeeded:

Bastion host instance running. Opened SSH Port on Node. Waiting 60 seconds for Bastion SSH port open => Connection to ... port 22 [tcp/*] succeeded! Opened SSH Port on Bastion SSH gardener@... => gardener@ip-...compute.internal Warning: Permanently added '...' (ED25519) to the list of known hosts. Warning: Permanently added 'ip-....compute.internal' (ED25519) to the list of known hosts. (4/4) Cleanup Cleaning up bastion host configurations...

Hi @dansible , i used a newly introduced function CheckIPPortReachable which is using native go code (so we don't have dependency on different version of binary like your nc returns succeeded but mine returns open) to perform ssh port check

petersutter · 2020-11-19T21:00:39Z

pkg/cmd/ssh_aws.go

@@ -242,7 +246,7 @@ func (a *AwsInstanceAttribute) sshPortCheck() {
 		cmd := exec.Command("bash", "-c", ncCmd)
 		output, _ := cmd.CombinedOutput()
 		fmt.Println("=>", string(output))
-		if strings.Contains(string(output), "succeeded") {
+		if strings.Contains(string(output), "open") {


instead of sshPortCheck(), can't we reuse the newly introduced [function]?(https://github.com/gardener/gardenctl/pull/445/files#diff-8a970d51dea43e926c0b913c26887464b362c5f723fb1d740a5a07d428434c07R355)

func CheckIPPortReachable(ip string, port string) error

thanks @petersutter , i add retry logic in CheckIPPortReachable function and use it here

that's what I was asking in the other PR #445 (comment)

and this works without having to dial/try multiple times within a certain amount of time?

yes, previously i though it was working as retry but later i found it's not, so i added the logic

petersutter · 2020-11-20T11:28:00Z

pkg/cmd/utils.go

+			fmt.Printf("IP %s port %s is reachable\n", ip, port)
+			return nil
+		}
+		time.Sleep(time.Second * 10)


why is there a need to sleep 10 seconds?

thanks, i removed this line

petersutter · 2020-11-20T11:28:13Z

pkg/cmd/utils.go

@@ -349,17 +349,22 @@ func PrintoutObject(objectToPrint interface{}, writer io.Writer, outputFormat st
 	return nil
 }

-//CheckIPPortReachable check whether IP with port is reachable with 1 min
+//CheckIPPortReachable check whether IP with port is reachable with 2 min


Suggested change

//CheckIPPortReachable check whether IP with port is reachable with 2 min

//CheckIPPortReachable check whether IP with port is reachable within 2 min

thanks, i fixed the typo

petersutter · 2020-11-20T11:30:08Z

pkg/cmd/utils.go

+		timeout := time.Second * 10
+		conn, err := net.DialTimeout("tcp", net.JoinHostPort(ip, port), timeout)
+		if err != nil {
+			fmt.Println("Connecting error:", err)


Either omit the error as long as you are retrying or add a hint in case another try is made

thanks, i omitted the error

neo-liang-sap · 2020-11-20T11:47:07Z

build failed due to #455 ,

petersutter · 2020-11-20T11:50:55Z

build failed due to #455 ,

How could this even happen in the first place, that code slips into master that produces build errors?

neo-liang-sap · 2020-11-20T11:56:28Z

#455

it's very wired - code is in #431 which has all CI check passed.... but actually this PR has some errors...i've notified PR owner to fix this

neo-liang-sap · 2020-11-21T01:34:57Z

all check passed after #455 is fixed , @petersutter could you please review this PR when you have time? thanks!
-Neo

petersutter · 2020-11-23T14:28:32Z

pkg/cmd/utils.go

-		defer conn.Close()
-		fmt.Printf("IP %s port %s is reachable\n", ip, port)
-		return nil
+	attemptCnt := 0


nit: would rather name it attemptCount or just attempt.

petersutter

/lgtm
feel free to also change the variable name as suggested.

neo-liang-sap requested a review from a team as a code owner November 16, 2020 05:32

gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Nov 16, 2020

gardener-robot-ci-2 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 Nov 16, 2020

neo-liang-sap mentioned this pull request Nov 16, 2020

gardenctl ssh <aws_node> fails with InvalidParameterValue #442

Closed

dansible previously approved these changes Nov 19, 2020

View reviewed changes

gardener-robot removed the needs/review Needs review label Nov 19, 2020

dansible suggested changes Nov 19, 2020

View reviewed changes

gardener-robot added the needs/changes Needs (more) changes label Nov 19, 2020

petersutter reviewed Nov 19, 2020

View reviewed changes

gardener-robot-ci-3 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 Nov 19, 2020

neo-liang-sap force-pushed the fix-aws-ssh-issue branch from 87a0b58 to 3637bc6 Compare November 20, 2020 01:27

gardener-robot added size/s Size of pull request is small (see gardener-robot robot/bots/size.py) and removed size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Nov 20, 2020

gardener-robot-ci-1 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 Nov 20, 2020

gardener-robot-ci-3 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 Nov 20, 2020

neo-liang-sap requested review from petersutter and dansible November 20, 2020 01:29

neo-liang-sap mentioned this pull request Nov 20, 2020

gardenctl ssh <openstack_node> fails with Operation timed out #444

Closed

petersutter reviewed Nov 20, 2020

View reviewed changes

neo-liang-sap force-pushed the fix-aws-ssh-issue branch from 3637bc6 to dd3c4df Compare November 20, 2020 11:39

gardener-robot-ci-2 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 Nov 20, 2020

gardener-robot-ci-1 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 Nov 20, 2020

fix aws ssh issue

2eb595c

neo-liang-sap added 3 commits November 21, 2020 09:09

add retry logic in CheckIPPortReachable function, use it in ssh_aws.go

2e20b22

remove extra time.Sleep, fix typo in comment

38891b8

omit error

658ea29

gardener-attic deleted a comment from gardener-robot Nov 21, 2020

re-trigger the test

de1beb7

neo-liang-sap force-pushed the fix-aws-ssh-issue branch from 2d3d580 to de1beb7 Compare November 21, 2020 01:12

gardener-robot-ci-1 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 Nov 21, 2020

gardener-robot-ci-3 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 Nov 21, 2020

remove re-trigger test info

499a4b7

gardener-robot-ci-2 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 Nov 21, 2020

gardener-robot-ci-3 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 Nov 21, 2020

neo-liang-sap requested a review from petersutter November 21, 2020 01:35

petersutter reviewed Nov 23, 2020

View reviewed changes

petersutter approved these changes Nov 23, 2020

View reviewed changes

gardener-robot added reviewed/lgtm Has approval for merging needs/changes Needs (more) changes and removed needs/changes Needs (more) changes reviewed/lgtm Has approval for merging labels Nov 23, 2020

dansible approved these changes Nov 23, 2020

View reviewed changes

rename variable name

f93033b

gardener-robot-ci-2 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 Nov 24, 2020

gardener-robot-ci-1 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 Nov 24, 2020

neo-liang-sap merged commit 4557f92 into gardener-attic:master Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix aws ssh issue #446

fix aws ssh issue #446

neo-liang-sap commented Nov 16, 2020 •

edited

Loading

neo-liang-sap commented Nov 16, 2020

dansible Nov 19, 2020 •

edited

Loading

neo-liang-sap Nov 20, 2020

petersutter Nov 19, 2020

neo-liang-sap Nov 20, 2020

petersutter Nov 20, 2020

neo-liang-sap Nov 20, 2020

petersutter Nov 20, 2020

neo-liang-sap Nov 20, 2020

petersutter Nov 20, 2020

neo-liang-sap Nov 20, 2020

petersutter Nov 20, 2020

neo-liang-sap Nov 20, 2020

neo-liang-sap commented Nov 20, 2020

petersutter commented Nov 20, 2020

neo-liang-sap commented Nov 20, 2020

neo-liang-sap commented Nov 21, 2020

petersutter Nov 23, 2020

petersutter left a comment

	//CheckIPPortReachable check whether IP with port is reachable with 2 min
	//CheckIPPortReachable check whether IP with port is reachable within 2 min

fix aws ssh issue #446

fix aws ssh issue #446

Conversation

neo-liang-sap commented Nov 16, 2020 • edited Loading

neo-liang-sap commented Nov 16, 2020

dansible Nov 19, 2020 • edited Loading

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

neo-liang-sap commented Nov 20, 2020

petersutter commented Nov 20, 2020

neo-liang-sap commented Nov 20, 2020

neo-liang-sap commented Nov 21, 2020

Choose a reason for hiding this comment

petersutter left a comment

Choose a reason for hiding this comment

neo-liang-sap commented Nov 16, 2020 •

edited

Loading

dansible Nov 19, 2020 •

edited

Loading