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

Warn if incompatible kubectl version is in use #5596

Merged
merged 2 commits into from
Oct 17, 2019
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 57 additions & 4 deletions cmd/minikube/cmd/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package cmd

import (
"encoding/json"
"fmt"
"net"
"net/url"
Expand Down Expand Up @@ -130,6 +131,17 @@ var (
extraOptions cfg.ExtraOptionSlice
)

type kubectlversion struct {
CVersion ClientVersion `json:"clientVersion"`
SVersion ClientVersion `json:"serverVersion"`
}

type ClientVersion struct {
Major string `json:"major"`
Minor string `json:"minor"`
GitVersion string `json:"gitVersion"`
}

func init() {
initMinikubeFlags()
initKubernetesFlags()
Expand Down Expand Up @@ -477,16 +489,57 @@ func showVersionInfo(k8sVersion string, cr cruntime.Manager) {
}
}

/**
Function to check for kubectl. The checking is to compare
the version reported by both the client and server. It is
that kubectl will be of the same version or 1 lower than
the kubernetes version.
*/
func showKubectlConnectInfo(kcs *kubeconfig.Settings) {
var output []byte
clientVersion := kubectlversion{}
serverVersion := kubectlversion{}

path, err := exec.LookPath("kubectl")

if err != nil {
out.T(out.Tip, "For best results, install kubectl: https://kubernetes.io/docs/tasks/tools/install-kubectl/")
} else {
glog.Infof("Kubectl found at the following location %s. Let's execute and get the result", path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While good for initial debugging, I think this log message can be removed.

output, err = exec.Command(path, "version", "--output=json").Output()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may show the wrong server version, as it queries the active context kubectl is configured for, which may not be minikube.

My suggestion is to use kubectl version --short --output=json, and add k8sVersion as an argument to the function.


// ...once we get something back do some version checking
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mention this later, but try to structure functions so that exceptional conditions are handled first, and exited early. So for istance:

if err != nil {
.. return ..
}
.. do the rest with one level less indentation

glog.Infof("Received output from kubectl %s", output)

// unmarshal both the {client}
clientjsonErr := json.Unmarshal(output, &clientVersion)

// ....... and {server} json
serverjsonErr := json.Unmarshal(output, &serverVersion)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to unmarshal the same output twice: the server version is already in clientVersion.SVersion


if (clientjsonErr != nil) || (serverjsonErr != nil) {
glog.Infof("There was an error processing the json output")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is another opportunity to return early.

} else {
// obtain the minor version for both
serverMinor, _ := strconv.Atoi(serverVersion.SVersion.Minor)
clientMinor, _ := strconv.Atoi(serverVersion.CVersion.Minor)

if clientMinor < serverMinor {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not quite correct yet. https://kubernetes.io/docs/setup/release/version-skew-policy/#kubectl dictates that you are allowed up to one version difference. One possibility is:

if math.Abs(float64(clientMinor - serverMinor)) > 1 {

out.T(out.Tip, "The version of kubectl {{.kubectl}} installed is incompatible with your Kubernetes {{.kubernetes}} deployment. Please upgrade/downgrade as necessary, or use minikube kubectlto connect to your cluster",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful to include the path. How about something like:

{{.path}} is version {{.version}}, and is incompatible with your specified Kubernetes version. You will need to update {{.path}} or use 'minikube kubectl' to connect with this cluster

out.V{"kubectl": clientVersion.CVersion.GitVersion, "kubernetes": serverVersion.SVersion.GitVersion})
}
}
} else {
out.T(out.Tip, "For best results, install kubectl: https://kubernetes.io/docs/tasks/tools/install-kubectl/")
}
}

if kcs.KeepContext {
Copy link
Contributor

@tstromberg tstromberg Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this connection output to the top of showKubectlConnectInfo.

This will allow both the version mismatch error to come at the end, which is more visible, and will allow you to exit early if kubectl does not exist, which will allow for more readable code as the version checking won't have to be indented.

out.T(out.Kubectl, "To connect to this cluster, use: kubectl --context={{.name}}", out.V{"name": kcs.ClusterName})
} else {
out.T(out.Ready, `Done! kubectl is now configured to use "{{.name}}"`, out.V{"name": cfg.GetMachineName()})
}
_, err := exec.LookPath("kubectl")
if err != nil {
out.T(out.Tip, "For best results, install kubectl: https://kubernetes.io/docs/tasks/tools/install-kubectl/")
}
}

func selectDriver(oldConfig *cfg.Config) string {
Expand Down