Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add user flag to all minikube command invocations #5649

Merged
merged 8 commits into from
Apr 13, 2021
73 changes: 63 additions & 10 deletions pkg/skaffold/cluster/minikube.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,36 @@ 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"
)

const (
minikubeVersionWithUserFlag = "1.18.0"
PriyaModali marked this conversation as resolved.
Show resolved Hide resolved
)

var GetClient = getClient

// To override during tests
var (
minikubeBinaryFunc = minikubeBinary
getClusterInfo = context.GetClusterInfo
minikubeBinaryFunc = minikubeBinary
getClusterInfo = context.GetClusterInfo
GetCurrentVersionFunc = getCurrentVersion

findOnce sync.Once
mk = struct {
version semver.Version
path string
err error
PriyaModali marked this conversation as resolved.
Show resolved Hide resolved
}{}
)

type Client interface {
Expand All @@ -55,7 +70,7 @@ func getClient() Client {
}

func (clientImpl) IsMinikube(kubeContext string) bool {
if _, err := minikubeBinaryFunc(); err != nil {
if _, _, err := minikubeBinaryFunc(); err != nil {
logrus.Tracef("Minikube cluster not detected: %v", err)
return false
}
Expand Down Expand Up @@ -89,22 +104,60 @@ func (clientImpl) MinikubeExec(arg ...string) (*exec.Cmd, error) {
}

func minikubeExec(arg ...string) (*exec.Cmd, error) {
b, err := minikubeBinaryFunc()
b, v, err := minikubeBinaryFunc()
if err != nil {
return nil, fmt.Errorf("getting minikube executable: %w", err)
}

isSupported, _ := supportsUserFlag(v)
if isSupported {
arg = append(arg, "--user=skaffold")
PriyaModali marked this conversation as resolved.
Show resolved Hide resolved
}
return exec.Command(b, arg...), nil
}

func minikubeBinary() (string, error) {
filename, err := exec.LookPath("minikube")
func supportsUserFlag(ver semver.Version) (bool, error) {
versionWithFlag, err := semver.Make(minikubeVersionWithUserFlag)
if err != nil {
return false, err
}
return ver.GE(versionWithFlag), nil
}

// 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
Expand Down
47 changes: 34 additions & 13 deletions pkg/skaffold/cluster/minikube_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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
PriyaModali marked this conversation as resolved.
Show resolved Hide resolved
}{
{
description: "context is 'minikube'",
Expand All @@ -56,41 +58,60 @@ 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)),
PriyaModali marked this conversation as resolved.
Show resolved Hide resolved
certPath: filepath.Join(home, "foo", "ca.crt"),
expected: false,
},
{
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,
},
}

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.Make("1.18.0")
PriyaModali marked this conversation as resolved.
Show resolved Hide resolved
t.Override(&minikubeBinaryFunc, 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.Make("1.17.0")
PriyaModali marked this conversation as resolved.
Show resolved Hide resolved
t.Override(&minikubeBinaryFunc, func() (string, semver.Version, error) {
return "", ver, fmt.Errorf("minikube not in PATH")
})
} else {
ver, _ := semver.Make("1.18.1")
PriyaModali marked this conversation as resolved.
Show resolved Hide resolved
t.Override(&minikubeBinaryFunc, func() (string, semver.Version, error) { return "minikube", ver, nil })
}
}
t.Override(&util.DefaultExecCommand, test.minikubeProfileCmd)
t.Override(&getClusterInfo, func(string) (*clientcmdapi.Cluster, error) {
Expand Down
5 changes: 5 additions & 0 deletions pkg/skaffold/test/structure/structure_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.GetCurrentVersionFunc, func() (semver.Version, error) { return semver.Version{}, nil })
PriyaModali marked this conversation as resolved.
Show resolved Hide resolved

t.Override(&util.DefaultExecCommand, testutil.CmdRun("container-structure-test test -v warn --image "+imageName+" --config "+tmpDir.Path("test.yaml")))

cfg := &mockConfig{
Expand Down