cmd/openshift-install/gather: Recognize "connection refused" #3615

rna-afk · 2020-05-18T15:41:33Z

Before this commit, bootstrap machines that failed to come up would
look like 1:

  level=info msg="Waiting up to 30m0s for the Kubernetes API at https://api.ci-op-6266tp8r-77109.origin-ci-int-aws.dev.rhcloud.com:6443..."
  level=error msg="Attempted to gather ClusterOperator status after installation failure: listing ClusterOperator objects: Get https://api.ci-op-6266tp8r-77109.origin-ci-int-aws.dev.rhcloud.com:6443/apis/config.openshift.io/v1/clusteroperators: dial tcp 3.221.214.197:6443: connect: connection refused"
  level=info msg="Pulling debug logs from the bootstrap machine"
  level=error msg="Attempted to gather debug logs after installation failure: failed to create SSH client, ensure the proper ssh key is in your keyring or specify with --key: dial tcp 3.84.188.207:22: connect: connection refused"
  level=fatal msg="Bootstrap failed to complete: waiting for Kubernetes API: context deadline exceeded"

With this commit, that last error will look like:

  level=error msg="Attempted to gather debug logs after installation failure: failed to connect to the bootstrap machine: dial tcp 3.84.188.207:22: connect: connection refused"

without the unrelated (to this failure mode) distraction about SSH
keys.

Updated the commit to match with the latest changes.
Clone of #2810

abhinavdahiya · 2020-05-18T16:10:31Z

cmd/openshift-install/gather.go

+		if errno, ok := errors.Unwrap(err).(syscall.Errno); ok && errno == syscall.ECONNREFUSED {
+			return errors.Wrap(err, "failed to connect to the bootstrap machine")
+		} else if len(gatherBootstrapOpts.sshKeys) == 0 {
+			return errors.Wrap(err, "failed to create SSH client, ensure the proper ssh key is in your keyring or specify with --key")
+		}


i think it's better use to the Is function introduced in go1.13, see Overview in https://golang.org/pkg/errors/#Unwrap

github.com/pkg/errors which we use in installer is transparent replacement for std lib errors.

abhinavdahiya · 2020-05-18T17:44:40Z

cmd/openshift-install/gather.go

@@ -145,8 +146,14 @@ func logGatherBootstrap(bootstrap string, port int, masters []string, directory
 	if err != nil && strings.Contains(err.Error(), "ssh: handshake failed: ssh: unable to authenticate") {
 		return errors.Wrap(err, "failed to create SSH client, ensure the private key is added to your authentication agent (ssh-agent) or specified with the --key parameter")
 	} else if err != nil {
+		if errors.Is(err, syscall.ECONNREFUSED) {


this entire block https://github.com/openshift/installer/pull/3615/files#diff-be4790dcfa83ca14e7942b4c3b50c5f9R146-R156 has multi if else for err, can we standardize to using errors.Is for connectionrefused and unable to authenitcate?

Tried checking for the root error type for the unable to authenticate scenario but the root error is fmt.Errorf which is a basic string error. Not sure if errors.Is will work for that scenario. I checked their test files for the function and the strings.Contains function is being used. Will work to make the code a little cleaner.

cgwalters · 2020-05-18T22:03:01Z

Picking this as a victim PR to
/test e2e-azure
since it clearly can't break it and I want to check general Azure health for #3613

abhinavdahiya · 2020-05-19T19:06:50Z

cmd/openshift-install/gather.go

+		if strings.Contains(err.Error(), "ssh: handshake failed: ssh: unable to authenticate") {
+			return errors.Wrap(err, "failed to create SSH client, ensure the private key is added to your authentication agent (ssh-agent) or specified with the --key parameter")
+		} else if errors.Is(err, syscall.ECONNREFUSED) {
+			return errors.Wrap(err, "failed to connect to the bootstrap machine")
+		} else if len(gatherBootstrapOpts.sshKeys) == 0 {
+			return errors.Wrap(err, "failed to create SSH client, ensure the proper ssh key is in your keyring or specify with --key")
+		}


I think it doesn't have to be if else if, it can be written as blocks of if that return.

https://github.com/golang/crypto/blob/master/ssh/client_auth.go#L75 , https://github.com/golang/crypto/blob/master/ssh/client.go#L85 is where the authentication error is coming from and it doesn't look like it supports Unwrap so strings contains will have to do for now.

for unauthenticated error there are 2 cases,

the env was set and https://github.com/openshift/installer/blob/master/pkg/gather/ssh/agent.go#L19
here maybe we can say failed to authenticate with the pre-existing ssh agent, add key to agent

the keys were used
here we can say failed to authenticate using keys, add you should provide keys using --keys

or something like that.

len(gatherBootstrapOpts.sshKeys) == 0
I don't know what this case helps us with.

https://github.com/openshift/installer/blob/master/pkg/gather/ssh/agent.go#L19
uses ssh agent when no keys are provided, but that doesn't match the doc in https://github.com/openshift/installer/blob/master/docs/user/troubleshootingbootstrap.md#authenticating-to-bootstrap-host-for-ipi
also b91707e#diff-be4790dcfa83ca14e7942b4c3b50c5f9 made it so that there is always one key passed to this list making it this ssh agent not used at all.. that would be bad.. maybe we fix that too, drop the len(keys) == 0

len(gatherBootstrapOpts.sshKeys) == 0
I don't know what this case helps us with

I added that case in 00307d1 and I think it is a good idea to remove it in this PR. The generated SSH key solves the problem.

for unauthenticated error there are 2 cases,

Determining which case happened seems to be a little hard based off of only the error message. https://github.com/openshift/installer/blob/master/pkg/gather/ssh/agent.go#L19 checks if the env is set and if it fails to dial, it goes with the other way of pulling the ssh keys from local. Any error message returned after that is in a general way and does not give details as to what case happened. I may be wrong.

The way I see it, I need to check if the env is set and if it dials right again like the above link, in this place here

installer/cmd/openshift-install/gather.go

Line 145 in e2367c4

if err != nil && strings.Contains(err.Error(), "ssh: handshake failed: ssh: unable to authenticate") {

which I think is repetitive and might take up time. Let me know if this is a viable option or if there is a better way.

the function that has the information what was used to create the SSH Agent for authentication is the ssh.NewClient and i think we should look at how that function that provide us the context on which one was used so that we can decide the next steps when we fail to authenticate.

maybe we try that?

patrickdillon · 2020-05-20T01:11:30Z

Fixes #3188

abhinavdahiya · 2020-05-28T19:35:31Z

pkg/gather/ssh/ssh.go

@@ -36,6 +37,12 @@ func NewClient(user, address string, keys []string) (*ssh.Client, error) {
 		HostKeyCallback: ssh.InsecureIgnoreHostKey(),
 	})
 	if err != nil {
+		if strings.Contains(err.Error(), "ssh: handshake failed: ssh: unable to authenticate") {
+			if agentType == "agent" {
+				return nil, errors.Wrap(err, "failed to use pre-existing agent, try adding keys using the --keys option")


Suggested change

return nil, errors.Wrap(err, "failed to use pre-existing agent, try adding keys using the --keys option")

return nil, errors.Wrap(err, "failed to use pre-existing agent, make sure the appropriate keys exist in the agent for authentication")

abhinavdahiya · 2020-05-28T19:36:35Z

pkg/gather/ssh/ssh.go

+			if agentType == "agent" {
+				return nil, errors.Wrap(err, "failed to use pre-existing agent, try adding keys using the --keys option")
+			}
+			return nil, errors.Wrap(err, "failed to use keys, ensure the private key is added to your authentication agent (ssh-agent) or specified with the --key parameter")


Suggested change

return nil, errors.Wrap(err, "failed to use keys, ensure the private key is added to your authentication agent (ssh-agent) or specified with the --key parameter")

return nil, errors.Wrap(err, "failed to use the provided keys for authentication")

Before this commit, bootstrap machines that failed to come up would look like [1]: level=info msg="Waiting up to 30m0s for the Kubernetes API at https://api.ci-op-6266tp8r-77109.origin-ci-int-aws.dev.rhcloud.com:6443..." level=error msg="Attempted to gather ClusterOperator status after installation failure: listing ClusterOperator objects: Get https://api.ci-op-6266tp8r-77109.origin-ci-int-aws.dev.rhcloud.com:6443/apis/config.openshift.io/v1/clusteroperators: dial tcp 3.221.214.197:6443: connect: connection refused" level=info msg="Pulling debug logs from the bootstrap machine" level=error msg="Attempted to gather debug logs after installation failure: failed to create SSH client, ensure the proper ssh key is in your keyring or specify with --key: dial tcp 3.84.188.207:22: connect: connection refused" level=fatal msg="Bootstrap failed to complete: waiting for Kubernetes API: context deadline exceeded" With this commit, that last error will look like: level=error msg="Attempted to gather debug logs after installation failure: failed to connect to the bootstrap machine: dial tcp 3.84.188.207:22: connect: connection refused" without the unrelated (to this failure mode) distraction about SSH keys. [1]: https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/logs/release-openshift-origin-installer-e2e-aws-upgrade/12076 Updated the commit to match with the latest changes.

abhinavdahiya · 2020-05-29T05:28:23Z

/approve

openshift-ci-robot · 2020-05-29T05:28:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

abhinavdahiya · 2020-06-01T14:36:01Z

/lgtm

openshift-bot · 2020-06-01T15:20:41Z

/retest