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

--kubernetes-version: assume latest patch version if not specified by user #16569

Merged
merged 9 commits into from
Jun 1, 2023
88 changes: 78 additions & 10 deletions cmd/minikube/cmd/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ type versionJSON struct {
Commit string `json:"commit"`
}

// ErrKubernetesPatchNotFound is when a patch was not found for the given <major>.<minor> version
var ErrKubernetesPatchNotFound = errors.New("Unable to detect the latest patch release for specified Kubernetes version")

var (
registryMirror []string
insecureRegistry []string
Expand Down Expand Up @@ -318,7 +321,10 @@ func provisionWithDriver(cmd *cobra.Command, ds registry.DriverState, existing *
stopk8s = true
}

k8sVersion := getKubernetesVersion(existing)
k8sVersion, err := getKubernetesVersion(existing)
if err != nil {
klog.Warningf("failed getting Kubernetes version: %v", err)
}
rtime := getContainerRuntime(existing)
cc, n, err := generateClusterConfig(cmd, existing, k8sVersion, rtime, driverName)
if err != nil {
Expand Down Expand Up @@ -1585,17 +1591,24 @@ func createNode(cc config.ClusterConfig, existing *config.ClusterConfig) (config
// Create the initial node, which will necessarily be a control plane
if existing != nil {
cp, err := config.PrimaryControlPlane(existing)
cp.KubernetesVersion = getKubernetesVersion(&cc)
cp.ContainerRuntime = getContainerRuntime(&cc)
if err != nil {
return cc, config.Node{}, err
}
newVer, err := getKubernetesVersion(&cc)
cp.KubernetesVersion = newVer
if err != nil {
klog.Warningf("failed getting Kubernetes version: %v", err)
}
cp.ContainerRuntime = getContainerRuntime(&cc)

// Make sure that existing nodes honor if KubernetesVersion gets specified on restart
// KubernetesVersion is the only attribute that the user can override in the Node object
nodes := []config.Node{}
for _, n := range existing.Nodes {
n.KubernetesVersion = getKubernetesVersion(&cc)
n.KubernetesVersion, err = getKubernetesVersion(&cc)
if err != nil {
klog.Warningf("failed getting Kubernetes version: %v", err)
}
n.ContainerRuntime = getContainerRuntime(&cc)
nodes = append(nodes, n)
}
Expand All @@ -1604,9 +1617,13 @@ func createNode(cc config.ClusterConfig, existing *config.ClusterConfig) (config
return cc, cp, nil
}

kubeVer, err := getKubernetesVersion(&cc)
if err != nil {
klog.Warningf("failed getting Kubernetes version: %v", err)
}
cp := config.Node{
Port: cc.KubernetesConfig.NodePort,
KubernetesVersion: getKubernetesVersion(&cc),
KubernetesVersion: kubeVer,
ContainerRuntime: getContainerRuntime(&cc),
ControlPlane: true,
Worker: true,
Expand Down Expand Up @@ -1653,12 +1670,32 @@ func autoSetDriverOptions(cmd *cobra.Command, drvName string) (err error) {

// validateKubernetesVersion ensures that the requested version is reasonable
func validateKubernetesVersion(old *config.ClusterConfig) {
nvs, _ := semver.Make(strings.TrimPrefix(getKubernetesVersion(old), version.VersionPrefix))
kubernetesVer, err := getKubernetesVersion(old)
if err != nil {
paramVersion := viper.GetString(kubernetesVersion)
paramVersion = strings.TrimPrefix(strings.ToLower(paramVersion), version.VersionPrefix)
if errors.Is(err, ErrKubernetesPatchNotFound) {
exit.Message(reason.PatchNotFound, "Unable to detect the latest patch release for specified major.minor version v{{.majorminor}}",
out.V{"majorminor": paramVersion})
}
exit.Message(reason.Usage, `Unable to parse "{{.kubernetes_version}}": {{.error}}`, out.V{"kubernetes_version": paramVersion, "error": err})

}

nvs, _ := semver.Make(strings.TrimPrefix(kubernetesVer, version.VersionPrefix))
oldestVersion := semver.MustParse(strings.TrimPrefix(constants.OldestKubernetesVersion, version.VersionPrefix))
defaultVersion := semver.MustParse(strings.TrimPrefix(constants.DefaultKubernetesVersion, version.VersionPrefix))
newestVersion := semver.MustParse(strings.TrimPrefix(constants.NewestKubernetesVersion, version.VersionPrefix))
zeroVersion := semver.MustParse(strings.TrimPrefix(constants.NoKubernetesVersion, version.VersionPrefix))

paramVersion := viper.GetString(kubernetesVersion)
Copy link
Member

Choose a reason for hiding this comment

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

while we are at there could u plz check if captical V works too ? if not I suggest while we are touching this, convert the version to lowercase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Capital V does not work. Do we want it so that the paramVersion is completely set to lowercase so that this is accounted for? Or do we want it to specifically set the first character to lowercase? I think the former would mean someone could input something like "StAble" as the flag and it would recognize it as "stable"

Copy link
Member

Choose a reason for hiding this comment

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

yeah lets convert the user input to lowercase for all cases, that makes our life easier

paramVersion = strings.TrimPrefix(strings.ToLower(paramVersion), version.VersionPrefix)

if isTwoDigitSemver(paramVersion) {
if getLatestPatch(paramVersion) != "" {
out.Styled(style.Workaround, `Using Kubernetes {{.version}} since patch version was unspecified`, out.V{"version": nvs})
}
}
if nvs.Equals(zeroVersion) {
klog.Infof("No Kubernetes version set for minikube, setting Kubernetes version to %s", constants.NoKubernetesVersion)
return
Expand Down Expand Up @@ -1729,7 +1766,7 @@ func isBaseImageApplicable(drv string) bool {
return registry.IsKIC(drv)
}

func getKubernetesVersion(old *config.ClusterConfig) string {
func getKubernetesVersion(old *config.ClusterConfig) (string, error) {
if viper.GetBool(noKubernetes) {
// Exit if --kubernetes-version is specified.
if viper.GetString(kubernetesVersion) != "" {
Expand Down Expand Up @@ -1758,12 +1795,21 @@ $ minikube config unset kubernetes-version`)
paramVersion = constants.NewestKubernetesVersion
}

nvs, err := semver.Make(strings.TrimPrefix(paramVersion, version.VersionPrefix))
kubernetesSemver := strings.TrimPrefix(strings.ToLower(paramVersion), version.VersionPrefix)
if isTwoDigitSemver(kubernetesSemver) {
potentialPatch := getLatestPatch(kubernetesSemver)
if potentialPatch != "" {
kubernetesSemver = potentialPatch
} else {
return "", ErrKubernetesPatchNotFound
}
}
nvs, err := semver.Make(kubernetesSemver)
if err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if here we want to send back an error here instead of just exiting. With the error return value it could be better to now return an error and handle it outside getKubernetesVersion

Copy link
Member

Choose a reason for hiding this comment

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

good point and good catch ! I think since it is already been implemented like that we can leave this one alone, and if we want we can refactor it in a follow up PR (for sake of keeping this PR small)

exit.Message(reason.Usage, `Unable to parse "{{.kubernetes_version}}": {{.error}}`, out.V{"kubernetes_version": paramVersion, "error": err})
}

return version.VersionPrefix + nvs.String()
return version.VersionPrefix + nvs.String(), nil
}

// validateDockerStorageDriver checks that docker is using overlay2
Expand Down Expand Up @@ -1848,7 +1894,11 @@ func validateBareMetal(drvName string) {
}

// conntrack is required starting with Kubernetes 1.18, include the release candidates for completion
version, _ := util.ParseKubernetesVersion(getKubernetesVersion(nil))
kubeVer, err := getKubernetesVersion(nil)
if err != nil {
klog.Warningf("failed getting Kubernetes version: %v", err)
}
version, _ := util.ParseKubernetesVersion(kubeVer)
if version.GTE(semver.MustParse("1.18.0-beta.1")) {
if _, err := exec.LookPath("conntrack"); err != nil {
exit.Message(reason.GuestMissingConntrack, "Sorry, Kubernetes {{.k8sVersion}} requires conntrack to be installed in root's path", out.V{"k8sVersion": version.String()})
Expand Down Expand Up @@ -1878,3 +1928,21 @@ func exitGuestProvision(err error) {
}
exit.Error(reason.GuestProvision, "error provisioning guest", err)
}

// Example input = 1.26 => output = "1.26.5"
// Example input = 1.26.5 => output = "1.26.5"
// Example input = 1.26.6 => output = ""
func getLatestPatch(majorMinorVer string) string {
for _, k := range constants.ValidKubernetesVersions {
if strings.HasPrefix(k, fmt.Sprintf("v%s.", majorMinorVer)) {
return strings.TrimPrefix(k, version.VersionPrefix)
}

}
return ""
}

func isTwoDigitSemver(ver string) bool {
majorMinorOnly := regexp.MustCompile(`^(?P<major>0|[1-9]\d*)\.(?P<minor>0|[1-9]\d*)$`)
return majorMinorOnly.MatchString(ver)
}
6 changes: 5 additions & 1 deletion cmd/minikube/cmd/start_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,11 @@ func updateExistingConfigFromFlags(cmd *cobra.Command, existing *config.ClusterC
updateStringFromFlag(cmd, &cc.SocketVMnetPath, socketVMnetPath)

if cmd.Flags().Changed(kubernetesVersion) {
cc.KubernetesConfig.KubernetesVersion = getKubernetesVersion(existing)
kubeVer, err := getKubernetesVersion(existing)
if err != nil {
klog.Warningf("get kubernetesVersion failed : %v", err)
}
cc.KubernetesConfig.KubernetesVersion = kubeVer
}
if cmd.Flags().Changed(containerRuntime) {
cc.KubernetesConfig.ContainerRuntime = getContainerRuntime(existing)
Expand Down
109 changes: 108 additions & 1 deletion cmd/minikube/cmd/start_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/spf13/cobra"
"github.com/spf13/viper"

"k8s.io/klog/v2"
cfg "k8s.io/minikube/pkg/minikube/config"
"k8s.io/minikube/pkg/minikube/constants"
"k8s.io/minikube/pkg/minikube/cruntime"
Expand Down Expand Up @@ -62,6 +63,16 @@ func TestGetKubernetesVersion(t *testing.T) {
paramVersion: "v1.16.0",
cfg: &cfg.ClusterConfig{KubernetesConfig: cfg.KubernetesConfig{KubernetesVersion: "v1.15.0"}},
},
{
description: "kubernetes-version without patch version",
expectedVersion: "v1.16.15",
paramVersion: "v1.16",
},
{
description: "kubernetes-version without patch version",
expectedVersion: "v1.16.15",
paramVersion: "1.16",
},
{
description: "kubernetes-version given as 'stable', no config",
expectedVersion: constants.DefaultKubernetesVersion,
Expand Down Expand Up @@ -92,7 +103,10 @@ func TestGetKubernetesVersion(t *testing.T) {
for _, test := range tests {
t.Run(test.description, func(t *testing.T) {
viper.SetDefault(kubernetesVersion, test.paramVersion)
version := getKubernetesVersion(test.cfg)
version, err := getKubernetesVersion(test.cfg)
if err != nil {
klog.Warningf("get kubernetesVersion failed : %v", err)
}

// check whether we are getting the expected version
if version != test.expectedVersion {
Expand Down Expand Up @@ -451,6 +465,99 @@ func TestValidateRuntime(t *testing.T) {
}
}

func TestIsTwoDigitSemver(t *testing.T) {
var tcs = []struct {
desc string
version string
expected bool
}{
{
desc: "a valid three digit version",
version: "1.26.5",
expected: false,
},
{
desc: "a valid two digit version",
version: "1.26",
expected: true,
},
{
desc: "a valid two digit version with a period",
version: "1.26.",
expected: false,
},
{
desc: "an invalid major version",
version: "2",
expected: false,
},
{
desc: "a valid major version",
version: "1",
expected: false,
},
{
desc: "a two digit version with a 0 as the major/minor components",
version: "0.0",
expected: true,
},
{
desc: "a two digit version with negative major version",
version: "-1.0",
expected: false,
},
{
desc: "a two digit vesion with negative minor version",
version: "1.-1",
expected: false,
},
{
desc: "a missing minor version",
version: "1.",
expected: false,
},
{
desc: "a missing major version",
version: ".2",
expected: false,
},
{
desc: "a valid two digit version with whitespace between components",
version: "1. 1",
expected: false,
},
{
desc: "a two digit version with a nondigit major component",
version: "a.12",
expected: false,
},
{
desc: "a two digit vesion with a nondigit minor component",
version: "1.a",
expected: false,
},
{
desc: "a two digit vesion with extraneous nondigits in minor component",
version: "1.2a",
expected: false,
},
{
desc: "a two digit vesion larger major/minor components",
version: "123456789.987654321",
expected: true,
},
}
for _, tc := range tcs {
t.Run(tc.desc, func(t *testing.T) {
actual := isTwoDigitSemver(tc.version)
// check whether the function correctly verifies if it is a 2 digit semver
if actual != tc.expected {
t.Fatalf("test failed. Expected version v%s to return %v", tc.version, tc.expected)
}
})
}
}

func TestValidatePorts(t *testing.T) {
isMicrosoftWSL := detect.IsMicrosoftWSL()
type portTest struct {
Expand Down
2 changes: 1 addition & 1 deletion pkg/minikube/out/out.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ import (
"bytes"
"fmt"
"html"
"html/template"
"io"
"os"
"path/filepath"
"strconv"
"strings"
"text/template"
"time"

"github.com/Delta456/box-cli-maker/v2"
Expand Down
5 changes: 5 additions & 0 deletions pkg/minikube/reason/reason.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ func (k *Kind) IssueURLs() []string {

// Sections are ordered roughly by stack dependencies
var (
// minikube could not find a patch for the provided major.minor version
PatchNotFound = Kind{ID: "MK_PATCH_NOT_FOUND", ExitCode: ExProgramUsage,
Advice: translate.T("Specify --kubernetes-version in v<major>.<minor.<build> form. example: 'v1.1.14'"),
}

// minikube has been passed an incorrect parameter
Usage = Kind{ID: "MK_USAGE", ExitCode: ExProgramUsage}
// minikube has no current cluster running
Expand Down