Merge pull request #1175 from endocode/dongsu/fix-race-ssh-tests

Fix data races in unit tests

cmd/minikube/cmd/start.go

@@ -177,8 +177,9 @@ func runStart(cmd *cobra.Command, args []string) {
 		ClientKey:            constants.MakeMiniPath("apiserver.key"),
 		CertificateAuthority: constants.MakeMiniPath("ca.crt"),
 		KeepContext:          viper.GetBool(keepContext),
-		KubeConfigFile:       kubeConfigFile,
 	}
+	kubeCfgSetup.SetKubeConfigFile(kubeConfigFile)
+
 	if err := kubeconfig.SetupKubeConfig(kubeCfgSetup); err != nil {
 		glog.Errorln("Error setting up kubeconfig: ", err)
 		cmdUtil.MaybeReportErrorAndExit(err)

pkg/minikube/cluster/cluster_test.go

@@ -355,23 +355,23 @@ func TestGetLocalkubeStatus(t *testing.T) {
 	}
 	api.Hosts[constants.MachineName] = &host.Host{Driver: d}
 
-	s.CommandToOutput = map[string]string{
+	s.SetCommandToOutput(map[string]string{
 		localkubeStatusCommand: state.Running.String(),
-	}
+	})
 	if _, err := GetLocalkubeStatus(api); err != nil {
 		t.Fatalf("Error getting localkube status: %s", err)
 	}
 
-	s.CommandToOutput = map[string]string{
+	s.SetCommandToOutput(map[string]string{
 		localkubeStatusCommand: state.Stopped.String(),
-	}
+	})
 	if _, err := GetLocalkubeStatus(api); err != nil {
 		t.Fatalf("Error getting localkube status: %s", err)
 	}
 
-	s.CommandToOutput = map[string]string{
+	s.SetCommandToOutput(map[string]string{
 		localkubeStatusCommand: "Bad Output",
-	}
+	})
 	if _, err := GetLocalkubeStatus(api); err == nil {
 		t.Fatalf("Expected error in getting localkube status as ssh returned bad output")
 	}
@@ -520,7 +520,7 @@ func TestCreateSSHShell(t *testing.T) {
 		t.Fatalf("Error running ssh command: %s", err)
 	}
 
-	if !s.HadASessionRequested {
+	if !s.IsSessionRequested() {
 		t.Fatalf("Expected ssh session to be run")
 	}
 }

pkg/minikube/kubeconfig/config.go

@@ -20,6 +20,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"sync/atomic"
 
 	"github.com/golang/glog"
 	"github.com/pkg/errors"
@@ -47,18 +48,27 @@ type KubeConfigSetup struct {
 	// Should the current context be kept when setting up this one
 	KeepContext bool
 
-	// KubeConfigFile is the path where the kube config is stored
-	KubeConfigFile string
+	// kubeConfigFile is the path where the kube config is stored
+	// Only access this with atomic ops
+	kubeConfigFile atomic.Value
+}
+
+func (k *KubeConfigSetup) SetKubeConfigFile(kubeConfigFile string) {
+	k.kubeConfigFile.Store(kubeConfigFile)
+}
+
+func (k *KubeConfigSetup) GetKubeConfigFile() string {
+	return k.kubeConfigFile.Load().(string)
 }
 
 // SetupKubeconfig reads config from disk, adds the minikube settings, and writes it back.
 // activeContext is true when minikube is the CurrentContext
 // If no CurrentContext is set, the given name will be used.
 func SetupKubeConfig(cfg *KubeConfigSetup) error {
-	glog.Infoln("Using kubeconfig: ", cfg.KubeConfigFile)
+	glog.Infoln("Using kubeconfig: ", cfg.GetKubeConfigFile())
 
 	// read existing config or create new if does not exist
-	config, err := ReadConfigOrNew(cfg.KubeConfigFile)
+	config, err := ReadConfigOrNew(cfg.GetKubeConfigFile())
 	if err != nil {
 		return err
 	}
@@ -89,7 +99,7 @@ func SetupKubeConfig(cfg *KubeConfigSetup) error {
 	}
 
 	// write back to disk
-	if err := WriteConfig(config, cfg.KubeConfigFile); err != nil {
+	if err := WriteConfig(config, cfg.GetKubeConfigFile()); err != nil {
 		return err
 	}
 	return nil

pkg/minikube/kubeconfig/config_test.go

@@ -100,9 +100,9 @@ func TestSetupKubeConfig(t *testing.T) {
 			if err != nil {
 				t.Fatalf("Error making temp directory %s", err)
 			}
-			test.cfg.KubeConfigFile = filepath.Join(tmpDir, "kubeconfig")
+			test.cfg.SetKubeConfigFile(filepath.Join(tmpDir, "kubeconfig"))
 			if len(test.existingCfg) != 0 {
-				ioutil.WriteFile(test.cfg.KubeConfigFile, test.existingCfg, 0600)
+				ioutil.WriteFile(test.cfg.GetKubeConfigFile(), test.existingCfg, 0600)
 			}
 			err = SetupKubeConfig(test.cfg)
 			if err != nil && !test.err {
@@ -111,7 +111,7 @@ func TestSetupKubeConfig(t *testing.T) {
 			if err == nil && test.err {
 				t.Errorf("Expected error but got none")
 			}
-			config, err := ReadConfigOrNew(test.cfg.KubeConfigFile)
+			config, err := ReadConfigOrNew(test.cfg.GetKubeConfigFile())
 			if err != nil {
 				t.Errorf("Error reading kubeconfig file: %s", err)
 			}

pkg/minikube/tests/ssh_mock.go

@@ -20,9 +20,11 @@ import (
 	"bytes"
 	"crypto/rand"
 	"crypto/rsa"
+	"fmt"
 	"io"
 	"net"
 	"strconv"
+	"sync/atomic"
 
 	"github.com/golang/glog"
 	"github.com/pkg/errors"
@@ -33,12 +35,14 @@ import (
 type SSHServer struct {
 	Config *ssh.ServerConfig
 	// Commands stores the raw commands executed against the server.
-	Commands             map[string]int
-	Connected            bool
-	Transfers            *bytes.Buffer
-	HadASessionRequested bool
-	// CommandsToOutput can be used to mock what the SSHServer returns for a given command
-	CommandToOutput map[string]string
+	Commands  map[string]int
+	Connected bool
+	Transfers *bytes.Buffer
+	// Only access this with atomic ops
+	hadASessionRequested int32
+	// commandsToOutput can be used to mock what the SSHServer returns for a given command
+	// Only access this with atomic ops
+	commandToOutput atomic.Value
 }
 
 // NewSSHServer returns a NewSSHServer instance, ready for use.
@@ -59,6 +63,8 @@ func NewSSHServer() (*SSHServer, error) {
 		return nil, errors.Wrap(err, "Error creating signer from key")
 	}
 	s.Config.AddHostKey(signer)
+	s.SetSessionRequested(false)
+	s.SetCommandToOutput(map[string]string{})
 	return s, nil
 }
 
@@ -92,7 +98,7 @@ func (s *SSHServer) Start() (int, error) {
 				// Service the incoming Channel channel.
 				for newChannel := range chans {
 					if newChannel.ChannelType() == "session" {
-						s.HadASessionRequested = true
+						s.SetSessionRequested(true)
 					}
 					channel, requests, err := newChannel.Accept()
 					s.Connected = true
@@ -112,7 +118,7 @@ func (s *SSHServer) Start() (int, error) {
 					s.Commands[cmd.Command] = 1
 
 					// Write specified command output as mocked ssh output
-					if val, ok := s.CommandToOutput[cmd.Command]; ok {
+					if val, err := s.GetCommandToOutput(cmd.Command); err == nil {
 						channel.Write([]byte(val))
 					}
 					channel.SendRequest("exit-status", false, []byte{0, 0, 0, 0})
@@ -136,3 +142,28 @@ func (s *SSHServer) Start() (int, error) {
 	}
 	return port, nil
 }
+
+func (s *SSHServer) SetCommandToOutput(cmdToOutput map[string]string) {
+	s.commandToOutput.Store(cmdToOutput)
+}
+
+func (s *SSHServer) GetCommandToOutput(cmd string) (string, error) {
+	cmdMap := s.commandToOutput.Load().(map[string]string)
+	val, ok := cmdMap[cmd]
+	if !ok {
+		return "", fmt.Errorf("unavailable command %s", cmd)
+	}
+	return val, nil
+}
+
+func (s *SSHServer) SetSessionRequested(b bool) {
+	var i int32
+	if b {
+		i = 1
+	}
+	atomic.StoreInt32(&s.hadASessionRequested, i)
+}
+
+func (s *SSHServer) IsSessionRequested() bool {
+	return atomic.LoadInt32(&s.hadASessionRequested) != 0
+}