Refactor command runner interface, allow stdin writes

Makefile

@@ -15,12 +15,12 @@
 # Bump these on release - and please check ISO_VERSION for correctness.
 VERSION_MAJOR ?= 1
 VERSION_MINOR ?= 5
-VERSION_BUILD ?= 0
+VERSION_BUILD ?= 1
 RAW_VERSION=$(VERSION_MAJOR).$(VERSION_MINOR).${VERSION_BUILD}
 VERSION ?= v$(RAW_VERSION)
 
 # Default to .0 for higher cache hit rates, as build increments typically don't require new ISO versions
-ISO_VERSION ?= v$(VERSION_MAJOR).$(VERSION_MINOR).0
+ISO_VERSION ?= v$(VERSION_MAJOR).$(VERSION_MINOR).$(VERSION_BUILD)
 # Dashes are valid in semver, but not Linux packaging. Use ~ to delimit alpha/beta
 DEB_VERSION ?= $(subst -,~,$(RAW_VERSION))
 RPM_VERSION ?= $(DEB_VERSION)
@@ -572,4 +572,4 @@ site: site/themes/docsy/assets/vendor/bootstrap/package.js out/hugo/hugo
 
 .PHONY: out/mkcmp
 out/mkcmp:
-	GOOS=$(GOOS) GOARCH=$(GOARCH) go build -o $@ cmd/performance/main.go
+	GOOS=$(GOOS) GOARCH=$(GOARCH) go build -o $@ cmd/performance/main.go

cmd/minikube/cmd/start.go

@@ -1024,27 +1024,29 @@ Suggested workarounds:
 		defer conn.Close()
 	}
 
-	if err := r.Run("nslookup kubernetes.io"); err != nil {
+	if _, err := r.RunCmd(exec.Command("nslookup", "kubernetes.io")); err != nil {
 		out.WarningT("VM is unable to resolve DNS hosts: {[.error}}", out.V{"error": err})
 	}
 
 	// Try both UDP and ICMP to assert basic external connectivity
-	if err := r.Run("nslookup k8s.io 8.8.8.8 || nslookup k8s.io 1.1.1.1 || ping -c1 8.8.8.8"); err != nil {
+	if _, err := r.RunCmd(exec.Command("/bin/bash", "-c", "nslookup k8s.io 8.8.8.8 || nslookup k8s.io 1.1.1.1 || ping -c1 8.8.8.8")); err != nil {
 		out.WarningT("VM is unable to directly connect to the internet: {{.error}}", out.V{"error": err})
 	}
 
 	// Try an HTTPS connection to the
 	proxy := os.Getenv("HTTPS_PROXY")
-	opts := "-sS"
+	opts := []string{"-sS"}
 	if proxy != "" && !strings.HasPrefix(proxy, "localhost") && !strings.HasPrefix(proxy, "127.0") {
-		opts = fmt.Sprintf("-x %s %s", proxy, opts)
+		opts = append([]string{"-x", proxy}, opts...)
 	}
 
 	repo := viper.GetString(imageRepository)
 	if repo == "" {
 		repo = images.DefaultImageRepo
 	}
-	if err := r.Run(fmt.Sprintf("curl %s https://%s/", opts, repo)); err != nil {
+
+	opts = append(opts, fmt.Sprintf("https://%s/", repo))
+	if _, err := r.RunCmd(exec.Command("curl", opts...)); err != nil {
 		out.WarningT("VM is unable to connect to the selected image repository: {{.error}}", out.V{"error": err})
 	}
 	return ip

go.mod

@@ -43,6 +43,7 @@ require (
 	github.com/juju/testing v0.0.0-20190723135506-ce30eb24acd2 // indirect
 	github.com/juju/utils v0.0.0-20180820210520-bf9cc5bdd62d // indirect
 	github.com/juju/version v0.0.0-20180108022336-b64dbd566305 // indirect
+	github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51
 	github.com/libvirt/libvirt-go v3.4.0+incompatible
 	github.com/machine-drivers/docker-machine-driver-vmware v0.1.1
 	github.com/mattn/go-isatty v0.0.8

go.sum

@@ -294,6 +294,8 @@ github.com/juju/version v0.0.0-20180108022336-b64dbd566305 h1:lQxPJ1URr2fjsKnJRt
 github.com/juju/version v0.0.0-20180108022336-b64dbd566305/go.mod h1:kE8gK5X0CImdr7qpSKl3xB2PmpySSmfj7zVbkZFs81U=
 github.com/kardianos/osext v0.0.0-20150410034420-8fef92e41e22/go.mod h1:1NbS8ALrpOvjt0rHPNLyCIeMtbizbir8U//inJ+zuB8=
 github.com/karrick/godirwalk v1.7.5/go.mod h1:2c9FRhkDxdIbgkOnCEvnSWs71Bhugbl46shStcFDJ34=
+github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 h1:Z9n2FFNUXsshfwJMBgNA0RU6/i7WVaAegv3PtuIHPMs=
+github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51/go.mod h1:CzGEWj7cYgsdH8dAjBGEr58BoE7ScuLd+fwFZ44+/x8=
 github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=
 github.com/konsorten/go-windows-terminal-sequences v1.0.1 h1:mweAR1A6xJ3oS2pRaGiHgQ4OO8tzTaLawm8vnODuwDk=
 github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ=

pkg/drivers/none/none.go

@@ -17,8 +17,8 @@ limitations under the License.
 package none
 
 import (
-	"bytes"
 	"fmt"
+	"os/exec"
 	"strings"
 	"time"
 
@@ -168,8 +168,8 @@ func (d *Driver) Remove() error {
 		return errors.Wrap(err, "kill")
 	}
 	glog.Infof("Removing: %s", cleanupPaths)
-	cmd := fmt.Sprintf("sudo rm -rf %s", strings.Join(cleanupPaths, " "))
-	if err := d.exec.Run(cmd); err != nil {
+	args := append([]string{"rm", "-rf"}, cleanupPaths...)
+	if _, err := d.exec.RunCmd(exec.Command("sudo", args...)); err != nil {
 		glog.Errorf("cleanup incomplete: %v", err)
 	}
 	return nil
@@ -217,22 +217,20 @@ func (d *Driver) RunSSHCommandFromDriver() error {
 }
 
 // stopKubelet idempotently stops the kubelet
-func stopKubelet(exec command.Runner) error {
+func stopKubelet(cr command.Runner) error {
 	glog.Infof("stopping kubelet.service ...")
 	stop := func() error {
-		cmdStop := "sudo systemctl stop kubelet.service"
-		cmdCheck := "sudo systemctl show -p SubState kubelet"
-		err := exec.Run(cmdStop)
-		if err != nil {
-			glog.Errorf("temporary error for %q : %v", cmdStop, err)
+		cmd := exec.Command("sudo", "systemctl", "stop", "kubelet.service")
+		if rr, err := cr.RunCmd(cmd); err != nil {
+			glog.Errorf("temporary error for %q : %v", rr.Command(), err)
 		}
-		var out bytes.Buffer
-		errStatus := exec.CombinedOutputTo(cmdCheck, &out)
-		if errStatus != nil {
-			glog.Errorf("temporary error: for %q : %v", cmdCheck, errStatus)
+		cmd = exec.Command("sudo", "systemctl", "show", "-p", "SubState", "kubelet")
+		rr, err := cr.RunCmd(cmd)
+		if err != nil {
+			glog.Errorf("temporary error: for %q : %v", rr.Command(), err)
 		}
-		if !strings.Contains(out.String(), "dead") && !strings.Contains(out.String(), "failed") {
-			return fmt.Errorf("unexpected kubelet state: %q", out)
+		if !strings.Contains(rr.Stdout.String(), "dead") && !strings.Contains(rr.Stdout.String(), "failed") {
+			return fmt.Errorf("unexpected kubelet state: %q", rr.Stdout.String())
 		}
 		return nil
 	}
@@ -245,13 +243,21 @@ func stopKubelet(exec command.Runner) error {
 }
 
 // restartKubelet restarts the kubelet
-func restartKubelet(exec command.Runner) error {
+func restartKubelet(cr command.Runner) error {
 	glog.Infof("restarting kubelet.service ...")
-	return exec.Run("sudo systemctl restart kubelet.service")
+	c := exec.Command("sudo", "systemctl", "restart", "kubelet.service")
+	if _, err := cr.RunCmd(c); err != nil {
+		return err
+	}
+	return nil
 }
 
 // checkKubelet returns an error if the kubelet is not running.
-func checkKubelet(exec command.Runner) error {
+func checkKubelet(cr command.Runner) error {
 	glog.Infof("checking for running kubelet ...")
-	return exec.Run("systemctl is-active --quiet service kubelet")
+	c := exec.Command("systemctl", "is-active", "--quiet", "service", "kubelet")
+	if _, err := cr.RunCmd(c); err != nil {
+		return errors.Wrap(err, "check kubelet")
+	}
+	return nil
 }

pkg/minikube/bootstrapper/certs.go

@@ -22,6 +22,7 @@ import (
 	"io/ioutil"
 	"net"
 	"os"
+	"os/exec"
 	"path"
 	"path/filepath"
 	"strings"
@@ -141,7 +142,7 @@ func SetupCerts(cmd command.Runner, k8s config.KubernetesConfig) error {
 
 	// configure CA certificates
 	if err := configureCACerts(cmd, caCerts); err != nil {
-		return errors.Wrapf(err, "error configuring CA certificates during provisioning %v", err)
+		return errors.Wrapf(err, "Configuring CA certs")
 	}
 	return nil
 }
@@ -318,21 +319,21 @@ func collectCACerts() (map[string]string, error) {
 }
 
 // getSubjectHash calculates Certificate Subject Hash for creating certificate symlinks
-func getSubjectHash(cmd command.Runner, filePath string) (string, error) {
-	out, err := cmd.CombinedOutput(fmt.Sprintf("openssl x509 -hash -noout -in '%s'", filePath))
+func getSubjectHash(cr command.Runner, filePath string) (string, error) {
+	rr, err := cr.RunCmd(exec.Command("openssl", "x509", "-hash", "-noout", "-in", filePath))
 	if err != nil {
-		return "", err
+		return "", errors.Wrapf(err, rr.Command())
 	}
-
-	stringHash := strings.TrimSpace(out)
+	stringHash := strings.TrimSpace(rr.Stdout.String())
 	return stringHash, nil
 }
 
 // configureCACerts looks up and installs all uploaded PEM certificates in /usr/share/ca-certificates to system-wide certificate store (/etc/ssl/certs).
 // OpenSSL binary required in minikube ISO
-func configureCACerts(cmd command.Runner, caCerts map[string]string) error {
+func configureCACerts(cr command.Runner, caCerts map[string]string) error {
 	hasSSLBinary := true
-	if err := cmd.Run("which openssl"); err != nil {
+	_, err := cr.RunCmd(exec.Command("openssl", "version"))
+	if err != nil {
 		hasSSLBinary = false
 	}
 
@@ -343,24 +344,25 @@ func configureCACerts(cmd command.Runner, caCerts map[string]string) error {
 	for _, caCertFile := range caCerts {
 		dstFilename := path.Base(caCertFile)
 		certStorePath := path.Join(SSLCertStoreDir, dstFilename)
-		if err := cmd.Run(fmt.Sprintf("sudo test -f '%s'", certStorePath)); err != nil {
-			if err := cmd.Run(fmt.Sprintf("sudo ln -s '%s' '%s'", caCertFile, certStorePath)); err != nil {
-				return errors.Wrapf(err, "error making symbol link for certificate %s", caCertFile)
+		_, err := cr.RunCmd(exec.Command("sudo", "test", "-f", certStorePath))
+		if err != nil {
+			if _, err := cr.RunCmd(exec.Command("sudo", "ln", "-s", caCertFile, certStorePath)); err != nil {
+				return errors.Wrapf(err, "create symlink for %s", caCertFile)
 			}
 		}
 		if hasSSLBinary {
-			subjectHash, err := getSubjectHash(cmd, caCertFile)
+			subjectHash, err := getSubjectHash(cr, caCertFile)
 			if err != nil {
-				return errors.Wrapf(err, "error calculating subject hash for certificate %s", caCertFile)
+				return errors.Wrapf(err, "calculate hash for cacert %s", caCertFile)
 			}
 			subjectHashLink := path.Join(SSLCertStoreDir, fmt.Sprintf("%s.0", subjectHash))
-			if err := cmd.Run(fmt.Sprintf("sudo test -f '%s'", subjectHashLink)); err != nil {
-				if err := cmd.Run(fmt.Sprintf("sudo ln -s '%s' '%s'", certStorePath, subjectHashLink)); err != nil {
-					return errors.Wrapf(err, "error making subject hash symbol link for certificate %s", caCertFile)
+			_, err = cr.RunCmd(exec.Command("sudo", "test", "-f", subjectHashLink))
+			if err != nil {
+				if _, err := cr.RunCmd(exec.Command("sudo", "ln", "-s", certStorePath, subjectHashLink)); err != nil {
+					return errors.Wrapf(err, "linking caCertFile %s", caCertFile)
 				}
 			}
 		}
 	}
-
 	return nil
 }

pkg/minikube/bootstrapper/certs_test.go

@@ -62,9 +62,9 @@ func TestSetupCerts(t *testing.T) {
 		certStorePath := path.Join(SSLCertStoreDir, dst)
 		certNameHash := "abcdef"
 		remoteCertHashLink := path.Join(SSLCertStoreDir, fmt.Sprintf("%s.0", certNameHash))
-		cmdMap[fmt.Sprintf("sudo ln -s '%s' '%s'", certFile, certStorePath)] = "1"
-		cmdMap[fmt.Sprintf("openssl x509 -hash -noout -in '%s'", certFile)] = certNameHash
-		cmdMap[fmt.Sprintf("sudo ln -s '%s' '%s'", certStorePath, remoteCertHashLink)] = "1"
+		cmdMap[fmt.Sprintf("sudo ln -s %s %s", certFile, certStorePath)] = "1"
+		cmdMap[fmt.Sprintf("openssl x509 -hash -noout -in %s", certFile)] = certNameHash
+		cmdMap[fmt.Sprintf("sudo ln -s %s %s", certStorePath, remoteCertHashLink)] = "1"
 	}
 	f := command.NewFakeCommandRunner()
 	f.SetCommandToOutput(cmdMap)