From 1ef4fd8a5a4d13f880a038d8c2f5d97434a8c969 Mon Sep 17 00:00:00 2001 From: Priya Modali Date: Tue, 13 Apr 2021 11:40:10 -0700 Subject: [PATCH] Add user flag to all minikube command invocations (#5649) * Adding user flag for minikube. * Adding version to global cache * Unit tests. * Merged version check & minikube binary lookup. * Updates --- pkg/skaffold/cluster/minikube.go | 69 ++++++++++++++++--- pkg/skaffold/cluster/minikube_test.go | 47 +++++++++---- pkg/skaffold/test/structure/structure_test.go | 5 ++ 3 files changed, 97 insertions(+), 24 deletions(-) diff --git a/pkg/skaffold/cluster/minikube.go b/pkg/skaffold/cluster/minikube.go index 2a612540d35..f7dd9cc6573 100644 --- a/pkg/skaffold/cluster/minikube.go +++ b/pkg/skaffold/cluster/minikube.go @@ -24,21 +24,35 @@ import ( "os" "os/exec" "path/filepath" + "sync" + "github.com/blang/semver" "github.com/sirupsen/logrus" "k8s.io/client-go/util/homedir" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/context" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/version" ) -var GetClient = getClient +var ( + GetClient = getClient + minikubeVrsionWithUserFlag = semver.MustParse("1.18.0") +) // To override during tests var ( - minikubeBinaryFunc = minikubeBinary - getClusterInfo = context.GetClusterInfo + FindMinikubeBinary = minikubeBinary + getClusterInfo = context.GetClusterInfo + GetCurrentVersionFunc = getCurrentVersion + + findOnce sync.Once + mk = struct { + err error // determines if version and path are valid + version semver.Version + path string + }{} ) type Client interface { @@ -55,7 +69,7 @@ func getClient() Client { } func (clientImpl) IsMinikube(kubeContext string) bool { - if _, err := minikubeBinaryFunc(); err != nil { + if _, _, err := FindMinikubeBinary(); err != nil { logrus.Tracef("Minikube cluster not detected: %v", err) return false } @@ -89,22 +103,55 @@ func (clientImpl) MinikubeExec(arg ...string) (*exec.Cmd, error) { } func minikubeExec(arg ...string) (*exec.Cmd, error) { - b, err := minikubeBinaryFunc() + b, v, err := FindMinikubeBinary() if err != nil { return nil, fmt.Errorf("getting minikube executable: %w", err) } + + if supportsUserFlag(v) { + arg = append(arg, "--user=skaffold") + } return exec.Command(b, arg...), nil } -func minikubeBinary() (string, error) { - filename, err := exec.LookPath("minikube") +func supportsUserFlag(ver semver.Version) bool { + return ver.GE(minikubeVrsionWithUserFlag) +} + +// Retrieves minikube version +func getCurrentVersion() (semver.Version, error) { + cmd := exec.Command("minikube", "version") + out, err := util.RunCmdOut(cmd) if err != nil { - return "", errors.New("unable to find minikube executable. Please add it to PATH environment variable") + return semver.Version{}, err } - if _, err := os.Stat(filename); os.IsNotExist(err) { - return "", fmt.Errorf("unable to find minikube executable. File not found %s", filename) + + currentVersion, err := version.ParseVersion(string(out)) + if err != nil { + return semver.Version{}, err } - return filename, nil + + return currentVersion, nil +} + +func minikubeBinary() (string, semver.Version, error) { + findOnce.Do(func() { + filename, err := exec.LookPath("minikube") + if err != nil { + mk.err = errors.New("unable to lookup minikube executable. Please add it to PATH environment variable") + } + if _, err := os.Stat(filename); os.IsNotExist(err) { + mk.err = fmt.Errorf("unable to find minikube executable. File not found %s", filename) + } + mk.path = filename + if v, err := GetCurrentVersionFunc(); err != nil { + mk.err = err + } else { + mk.version = v + } + }) + + return mk.path, mk.version, mk.err } // matchClusterCertPath checks if the cluster certificate for this context is from inside the minikube directory diff --git a/pkg/skaffold/cluster/minikube_test.go b/pkg/skaffold/cluster/minikube_test.go index b77008d674f..fbf4e1916e1 100644 --- a/pkg/skaffold/cluster/minikube_test.go +++ b/pkg/skaffold/cluster/minikube_test.go @@ -21,6 +21,7 @@ import ( "path/filepath" "testing" + "github.com/blang/semver" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "k8s.io/client-go/util/homedir" @@ -31,13 +32,14 @@ import ( func TestClientImpl_IsMinikube(t *testing.T) { home := homedir.HomeDir() tests := []struct { - description string - kubeContext string - certPath string - serverURL string - minikubeProfileCmd util.Command - minikubeNotInPath bool - expected bool + description string + kubeContext string + certPath string + serverURL string + minikubeProfileCmd util.Command + minikubeNotInPath bool + expected bool + minikuneWithoutUserFalg bool }{ { description: "context is 'minikube'", @@ -56,10 +58,18 @@ func TestClientImpl_IsMinikube(t *testing.T) { certPath: filepath.Join(home, ".minikube", "ca.crt"), expected: true, }, + { + description: "minikube without user flag", + kubeContext: "test-cluster", + minikubeProfileCmd: testutil.CmdRunOut("minikube profile list -o json --user=skaffold", fmt.Sprintf(profileStr, "test-cluster", "docker", "172.17.0.3", 8443)), + certPath: filepath.Join(home, "foo", "ca.crt"), + expected: false, + minikuneWithoutUserFalg: true, + }, { description: "cluster cert outside minikube dir", kubeContext: "test-cluster", - minikubeProfileCmd: testutil.CmdRunOut("minikube profile list -o json", fmt.Sprintf(profileStr, "test-cluster", "docker", "172.17.0.3", 8443)), + minikubeProfileCmd: testutil.CmdRunOut("minikube profile list -o json --user=skaffold", fmt.Sprintf(profileStr, "test-cluster", "docker", "172.17.0.3", 8443)), certPath: filepath.Join(home, "foo", "ca.crt"), expected: false, }, @@ -67,20 +77,20 @@ func TestClientImpl_IsMinikube(t *testing.T) { description: "minikube node ip matches api server url", kubeContext: "test-cluster", serverURL: "https://192.168.64.10:8443", - minikubeProfileCmd: testutil.CmdRunOut("minikube profile list -o json", fmt.Sprintf(profileStr, "test-cluster", "hyperkit", "192.168.64.10", 8443)), + minikubeProfileCmd: testutil.CmdRunOut("minikube profile list -o json --user=skaffold", fmt.Sprintf(profileStr, "test-cluster", "hyperkit", "192.168.64.10", 8443)), expected: true, }, { description: "cannot parse minikube profile list", kubeContext: "test-cluster", - minikubeProfileCmd: testutil.CmdRunOut("minikube profile list -o json", `Random error`), + minikubeProfileCmd: testutil.CmdRunOut("minikube profile list -o json --user=skaffold", `Random error`), expected: false, }, { description: "minikube node ip different from api server url", kubeContext: "test-cluster", serverURL: "https://192.168.64.10:8443", - minikubeProfileCmd: testutil.CmdRunOut("minikube profile list -o json", fmt.Sprintf(profileStr, "test-cluster", "hyperkit", "192.168.64.11", 8443)), + minikubeProfileCmd: testutil.CmdRunOut("minikube profile list -o json --user=skaffold", fmt.Sprintf(profileStr, "test-cluster", "hyperkit", "192.168.64.11", 8443)), expected: false, }, } @@ -88,9 +98,20 @@ func TestClientImpl_IsMinikube(t *testing.T) { for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { if test.minikubeNotInPath { - t.Override(&minikubeBinaryFunc, func() (string, error) { return "", fmt.Errorf("minikube not in PATH") }) + ver := semver.Version{} + t.Override(&FindMinikubeBinary, func() (string, semver.Version, error) { + return "", ver, fmt.Errorf("minikube not in PATH") + }) } else { - t.Override(&minikubeBinaryFunc, func() (string, error) { return "minikube", nil }) + if test.minikuneWithoutUserFalg { + ver := semver.Version{Major: 1, Minor: 17, Patch: 0} + t.Override(&FindMinikubeBinary, func() (string, semver.Version, error) { + return "", ver, fmt.Errorf("minikube not in PATH") + }) + } else { + ver := semver.Version{Major: 1, Minor: 18, Patch: 1} + t.Override(&FindMinikubeBinary, func() (string, semver.Version, error) { return "minikube", ver, nil }) + } } t.Override(&util.DefaultExecCommand, test.minikubeProfileCmd) t.Override(&getClusterInfo, func(string) (*clientcmdapi.Cluster, error) { diff --git a/pkg/skaffold/test/structure/structure_test.go b/pkg/skaffold/test/structure/structure_test.go index 5d65834fd1a..91c00ff0cde 100644 --- a/pkg/skaffold/test/structure/structure_test.go +++ b/pkg/skaffold/test/structure/structure_test.go @@ -22,6 +22,9 @@ import ( "io/ioutil" "testing" + "github.com/blang/semver" + + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/cluster" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext" @@ -38,6 +41,8 @@ func TestNewRunner(t *testing.T) { testutil.Run(t, "", func(t *testutil.T) { tmpDir := t.NewTempDir().Touch("test.yaml") + t.Override(&cluster.FindMinikubeBinary, func() (string, semver.Version, error) { return "", semver.Version{}, errors.New("not found") }) + t.Override(&util.DefaultExecCommand, testutil.CmdRun("container-structure-test test -v warn --image "+imageName+" --config "+tmpDir.Path("test.yaml"))) cfg := &mockConfig{