Repository: code-ready/crc
Pull request #1719 (2020-11-24 13:00:24)

Improve os.MkdirAll use
`os.MkdirAll()` returns `nil` when the directory already exists,
so we don't need to explicitly check errors for `os.IsExists()`.

## Testing

There should be no user-visible changes. The mkdir calls which are changed are the one related to system tray and oc installation, bundle unpacking, and creation of ~/.crc and ~/.crc/cache.
They should still be created as before.

openshift-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/code-ready/crc/pull/1719#" title="Author self-approved">cfergeau</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=code-ready%2Fcrc).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/code-ready/crc/blob/master/OWNERS)~~ [cfergeau]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/crc/cache/cache.go b/pkg/crc/cache/cache.go
index 8856b8dc1..d903e8908 100644
--- a/pkg/crc/cache/cache.go
+++ b/pkg/crc/cache/cache.go
@@ -121,7 +121,7 @@ func (c *Cache) CacheExecutable() error {
 
 	// Copy the requested asset into its final destination
 	err = os.MkdirAll(c.destDir, 0750)
-	if err != nil && !os.IsExist(err) {
+	if err != nil {
 		return errors.Wrap(err, "Cannot create the target directory.")
 	}
 
diff --git a/pkg/crc/constants/constants.go b/pkg/crc/constants/constants.go
index edbd3ec5b..c5d5e3744 100644
--- a/pkg/crc/constants/constants.go
+++ b/pkg/crc/constants/constants.go
@@ -116,17 +116,9 @@ func GetHomeDir() string {
 	return os.Getenv("HOME")
 }
 
-// EnsureBaseDirectoriesExist create the ~/.crc and ~/.crc/oc dirs if they are not present
+// EnsureBaseDirectoryExists create the ~/.crc directory if it is not present
 func EnsureBaseDirectoriesExist() error {
-	if _, err := os.Stat(CrcBaseDir); err != nil {
-		if !os.IsNotExist(err) {
-			return err
-		}
-		if err := os.Mkdir(CrcBaseDir, 0750); err != nil {
-			return err
-		}
-	}
-	return nil
+	return os.MkdirAll(CrcBaseDir, 0750)
 }
 
 // IsBundleEmbedded returns true if the executable was compiled to contain the bundle
diff --git a/pkg/crc/machine/bundle/metadata.go b/pkg/crc/machine/bundle/metadata.go
index 9bb1ea8c1..4d25908b7 100644
--- a/pkg/crc/machine/bundle/metadata.go
+++ b/pkg/crc/machine/bundle/metadata.go
@@ -98,13 +98,8 @@ func GetCachedBundleInfo(bundleName string) (*CrcBundleInfo, error) {
 func (bundle *CrcBundleInfo) installSymlinkOrCopy() error {
 	ocInBundle := filepath.Join(bundle.cachedPath, constants.OcExecutableName)
 	ocInBinDir := filepath.Join(constants.CrcOcBinDir, constants.OcExecutableName)
-	if _, err := os.Stat(constants.CrcOcBinDir); err != nil {
-		if !os.IsNotExist(err) {
-			return err
-		}
-		if err := os.MkdirAll(constants.CrcOcBinDir, 0750); err != nil {
-			return err
-		}
+	if err := os.MkdirAll(constants.CrcOcBinDir, 0750); err != nil {
+		return err
 	}
 	_ = os.Remove(ocInBinDir)
 	if runtime.GOOS == "windows" {
diff --git a/pkg/crc/machine/machine.go b/pkg/crc/machine/machine.go
index 07a878ea2..ab7e51455 100644
--- a/pkg/crc/machine/machine.go
+++ b/pkg/crc/machine/machine.go
@@ -269,7 +269,11 @@ func (client *client) Start(startConfig StartConfig) (*StartResult, error) {
 	if err != nil {
 		return nil, errors.Wrap(err, "Error getting the IP")
 	}
-	sshRunner := crcssh.CreateRunner(instanceIP, getSSHPort(client.useVSock()), crcBundleMetadata.GetSSHKeyPath(), constants.GetPrivateKeyPath())
+	sshRunner, err := crcssh.CreateRunner(instanceIP, getSSHPort(client.useVSock()), crcBundleMetadata.GetSSHKeyPath(), constants.GetPrivateKeyPath())
+	if err != nil {
+		return nil, errors.Wrap(err, "Error creating the ssh client")
+	}
+	defer sshRunner.Close()
 
 	logging.Debug("Waiting until ssh is available")
 	if err := cluster.WaitForSSH(sshRunner); err != nil {
@@ -602,7 +606,11 @@ func (client *client) Status() (*ClusterStatusResult, error) {
 	if err != nil {
 		return nil, errors.Wrap(err, "Error getting ip")
 	}
-	sshRunner := crcssh.CreateRunner(ip, getSSHPort(client.useVSock()), constants.GetPrivateKeyPath())
+	sshRunner, err := crcssh.CreateRunner(ip, getSSHPort(client.useVSock()), constants.GetPrivateKeyPath())
+	if err != nil {
+		return nil, errors.Wrap(err, "Error creating the ssh client")
+	}
+	defer sshRunner.Close()
 	// check if all the clusteroperators are running
 	diskSize, diskUse, err := cluster.GetRootPartitionUsage(sshRunner)
 	if err != nil {
diff --git a/pkg/crc/preflight/preflight_checks_tray_darwin.go b/pkg/crc/preflight/preflight_checks_tray_darwin.go
index 5ea5b60cc..05961eee0 100644
--- a/pkg/crc/preflight/preflight_checks_tray_darwin.go
+++ b/pkg/crc/preflight/preflight_checks_tray_darwin.go
@@ -207,7 +207,7 @@ func downloadOrExtractTrayApp() error {
 	archivePath := filepath.Join(tmpArchivePath, filepath.Base(constants.GetCRCMacTrayDownloadURL()))
 	outputPath := constants.CrcBinDir
 	err = goos.MkdirAll(outputPath, 0750)
-	if err != nil && !goos.IsExist(err) {
+	if err != nil {
 		return errors.Wrap(err, "Cannot create the target directory.")
 	}
 	_, err = extract.Uncompress(archivePath, outputPath, false)
diff --git a/pkg/crc/ssh/client.go b/pkg/crc/ssh/client.go
index d75154bda..1cd5c6023 100644
--- a/pkg/crc/ssh/client.go
+++ b/pkg/crc/ssh/client.go
@@ -2,7 +2,6 @@ package ssh
 
 import (
 	"fmt"
-	"io"
 	"io/ioutil"
 	"net"
 	"strconv"
@@ -14,46 +13,43 @@ import (
 
 type Client interface {
 	Output(command string) (string, error)
+	Close()
 }
 
 type NativeClient struct {
-	Config   ssh.ClientConfig
+	User     string
 	Hostname string
 	Port     int
-}
+	Keys     []string
 
-type Auth struct {
-	Keys []string
+	conn *ssh.Client
 }
 
-func NewClient(user string, host string, port int, auth *Auth) (Client, error) {
-	config, err := NewNativeConfig(user, auth)
-	if err != nil {
-		return nil, fmt.Errorf("Error getting config for native Go SSH: %s", err)
-	}
-
+func NewClient(user string, host string, port int, keys ...string) (Client, error) {
 	return &NativeClient{
-		Config:   config,
+		User:     user,
 		Hostname: host,
 		Port:     port,
+		Keys:     keys,
 	}, nil
 }
 
-func NewNativeConfig(user string, auth *Auth) (ssh.ClientConfig, error) {
+func clientConfig(user string, keys []string) (*ssh.ClientConfig, error) {
 	var (
 		privateKeys []ssh.Signer
 		authMethods []ssh.AuthMethod
 	)
 
-	for _, k := range auth.Keys {
+	for _, k := range keys {
 		key, err := ioutil.ReadFile(k)
 		if err != nil {
-			return ssh.ClientConfig{}, err
+			log.Debugf("Cannot read private ssh key %s", k)
+			continue
 		}
 
 		privateKey, err := ssh.ParsePrivateKey(key)
 		if err != nil {
-			return ssh.ClientConfig{}, err
+			return nil, err
 		}
 
 		privateKeys = append(privateKeys, privateKey)
@@ -63,7 +59,7 @@ func NewNativeConfig(user string, auth *Auth) (ssh.ClientConfig, error) {
 		authMethods = append(authMethods, ssh.PublicKeys(privateKeys...))
 	}
 
-	return ssh.ClientConfig{
+	return &ssh.ClientConfig{
 		User: user,
 		Auth: authMethods,
 		// #nosec G106
@@ -72,25 +68,30 @@ func NewNativeConfig(user string, auth *Auth) (ssh.ClientConfig, error) {
 	}, nil
 }
 
-func (client *NativeClient) session() (*ssh.Client, *ssh.Session, error) {
-	conn, err := ssh.Dial("tcp", net.JoinHostPort(client.Hostname, strconv.Itoa(client.Port)), &client.Config)
-	if err != nil {
-		return nil, nil, err
+func (client *NativeClient) session() (*ssh.Session, error) {
+	if client.conn == nil {
+		var err error
+		config, err := clientConfig(client.User, client.Keys)
+		if err != nil {
+			return nil, fmt.Errorf("Error getting config for native Go SSH: %s", err)
+		}
+		client.conn, err = ssh.Dial("tcp", net.JoinHostPort(client.Hostname, strconv.Itoa(client.Port)), config)
+		if err != nil {
+			return nil, err
+		}
 	}
-	session, err := conn.NewSession()
+	session, err := client.conn.NewSession()
 	if err != nil {
-		_ = conn.Close()
-		return nil, nil, err
+		return nil, err
 	}
-	return conn, session, err
+	return session, err
 }
 
 func (client *NativeClient) Output(command string) (string, error) {
-	conn, session, err := client.session()
+	session, err := client.session()
 	if err != nil {
 		return "", err
 	}
-	defer closeConn(conn)
 	defer session.Close()
 
 	output, err := session.CombinedOutput(command)
@@ -100,8 +101,11 @@ func (client *NativeClient) Output(command string) (string, error) {
 	return string(output), nil
 }
 
-func closeConn(c io.Closer) {
-	err := c.Close()
+func (client *NativeClient) Close() {
+	if client.conn == nil {
+		return
+	}
+	err := client.conn.Close()
 	if err != nil {
 		log.Debugf("Error closing SSH Client: %s", err)
 	}
diff --git a/pkg/crc/ssh/ssh.go b/pkg/crc/ssh/ssh.go
index 13fc20e2c..170ae4400 100644
--- a/pkg/crc/ssh/ssh.go
+++ b/pkg/crc/ssh/ssh.go
@@ -13,17 +13,21 @@ import (
 )
 
 type Runner struct {
-	ip             string
-	port           int
-	privateSSHKeys []string
+	client Client
 }
 
-func CreateRunner(ip string, port int, privateKeys ...string) *Runner {
-	return &Runner{
-		ip:             ip,
-		port:           port,
-		privateSSHKeys: privateKeys,
+func CreateRunner(ip string, port int, privateKeys ...string) (*Runner, error) {
+	client, err := NewClient(constants.DefaultSSHUser, ip, port, privateKeys...)
+	if err != nil {
+		return nil, err
 	}
+	return &Runner{
+		client: client,
+	}, nil
+}
+
+func (runner *Runner) Close() {
+	runner.client.Close()
 }
 
 // Create a host using the driver's config
@@ -53,27 +57,13 @@ func (runner *Runner) CopyFile(srcFilename string, destFilename string, mode os.
 }
 
 func (runner *Runner) runSSHCommandFromDriver(command string, runPrivate bool) (string, error) {
-	var availableKeys []string
-	for _, privateKey := range runner.privateSSHKeys {
-		if _, err := os.Stat(privateKey); err == nil {
-			availableKeys = append(availableKeys, privateKey)
-		}
-	}
-
-	client, err := NewClient(constants.DefaultSSHUser, runner.ip, runner.port, &Auth{
-		Keys: availableKeys,
-	})
-	if err != nil {
-		return "", err
-	}
-
 	if runPrivate {
 		logging.Debugf("About to run SSH command with hidden output")
 	} else {
 		logging.Debugf("About to run SSH command:\n%s", command)
 	}
 
-	output, err := client.Output(command)
+	output, err := runner.client.Output(command)
 	if runPrivate {
 		if err != nil {
 			logging.Debugf("SSH command failed")
diff --git a/pkg/extract/extract.go b/pkg/extract/extract.go
index 26e798ba7..592c648dd 100644
--- a/pkg/extract/extract.go
+++ b/pkg/extract/extract.go
@@ -115,10 +115,8 @@ func untar(reader io.Reader, targetDir string, fileFilter func(string) bool, sho
 
 		// if its a dir and it doesn't exist create it
 		case tar.TypeDir:
-			if _, err := os.Stat(path); err != nil {
-				if err := os.MkdirAll(path, header.FileInfo().Mode()); err != nil {
-					return nil, err
-				}
+			if err := os.MkdirAll(path, header.FileInfo().Mode()); err != nil {
+				return nil, err
 			}
 
 		// if it's a file create it


Test patch:
diff --git a/pkg/crc/ssh/ssh_test.go b/pkg/crc/ssh/ssh_test.go
index 9ef829b56..96bf698cc 100644
--- a/pkg/crc/ssh/ssh_test.go
+++ b/pkg/crc/ssh/ssh_test.go
@@ -36,7 +36,7 @@ func TestRunner(t *testing.T) {
 	require.NoError(t, err)
 	defer listener.Close()
 
-	createSSHServer(t, listener, clientKey, func(input string) (byte, string) {
+	totalConn := createSSHServer(t, listener, clientKey, func(input string) (byte, string) {
 		escaped := fmt.Sprintf("%q", input)
 		if escaped == `"echo hello"` {
 			return 0, "hello"
@@ -48,7 +48,9 @@ func TestRunner(t *testing.T) {
 	})
 
 	addr := listener.Addr().String()
-	runner := CreateRunner(ipFor(addr), portFor(addr), clientKeyFile)
+	runner, err := CreateRunner(ipFor(addr), portFor(addr), clientKeyFile)
+	assert.NoError(t, err)
+	defer runner.Close()
 
 	bin, err := runner.Run("echo hello")
 	assert.NoError(t, err)
@@ -60,9 +62,12 @@ func TestRunner(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, "hello", stdout)
 	assert.Empty(t, stderr)
+
+	assert.Equal(t, 1, *totalConn)
 }
 
-func createSSHServer(t *testing.T, listener net.Listener, clientKey *rsa.PrivateKey, fun func(string) (byte, string)) {
+func createSSHServer(t *testing.T, listener net.Listener, clientKey *rsa.PrivateKey, fun func(string) (byte, string)) *int {
+	totalConn := 0
 	config := &ssh.ServerConfig{
 		PublicKeyCallback: func(c ssh.ConnMetadata, pubKey ssh.PublicKey) (*ssh.Permissions, error) {
 			pub, err := ssh.NewPublicKey(&clientKey.PublicKey)
@@ -93,6 +98,7 @@ func createSSHServer(t *testing.T, listener net.Listener, clientKey *rsa.Private
 				logrus.Debugf("cannot accept connection: %v", err)
 				return
 			}
+			totalConn++
 
 			conn, chans, reqs, err := ssh.NewServerConn(nConn, config)
 			require.NoError(t, err)
@@ -126,6 +132,7 @@ func createSSHServer(t *testing.T, listener net.Listener, clientKey *rsa.Private
 			}
 		}
 	}()
+	return &totalConn
 }
 
 func writePrivateKey(t *testing.T, clientKeyFile string, clientKey *rsa.PrivateKey) {
diff --git a/test/integration/crcsuite/collect.go b/test/integration/crcsuite/collect.go
index 9d1ace498..38e7050cb 100644
--- a/test/integration/crcsuite/collect.go
+++ b/test/integration/crcsuite/collect.go
@@ -182,9 +182,7 @@ func (collector *VMCommandCollector) Collect(w Writer) error {
 	if err != nil {
 		return err
 	}
-	ssh, err := ssh.NewClient(constants.DefaultSSHUser, ip, 22, &ssh.Auth{
-		Keys: []string{constants.GetPrivateKeyPath()},
-	})
+	ssh, err := ssh.NewClient(constants.DefaultSSHUser, ip, 22, constants.GetPrivateKeyPath())
 	if err != nil {
 		return err
 	}
@@ -205,9 +203,7 @@ func (collector *ContainerLogCollector) Collect(w Writer) error {
 	if err != nil {
 		return err
 	}
-	ssh, err := ssh.NewClient(constants.DefaultSSHUser, ip, 22, &ssh.Auth{
-		Keys: []string{constants.GetPrivateKeyPath()},
-	})
+	ssh, err := ssh.NewClient(constants.DefaultSSHUser, ip, 22, constants.GetPrivateKeyPath())
 	if err != nil {
 		return err
 	}