Merge pull request #53043 from kad/upgrade-ux

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Allow to use version labels in kubeadm upgrade apply command.

**What this PR does / why we need it**:

kubeadm upgrade apply now is able to utilize all possible combinations
of version argument, including labels (latest, stable-1.8, ci/latest-1.9)
as well as specific builds (v1.8.0-rc.1, ci/v1.9.0-alpha.1.123_01234567889)

As side effect, specifying exact build to deploy from CI area is now also
possible in kubeadm init command.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes kubernetes/kubeadm#451

**Special notes for your reviewer**:
cc @luxas 

**Release note**:
```release-note
- kubeadm init can now deploy exact build from CI area by specifying ID with "ci/" prefix. Example: "ci/v1.9.0-alpha.1.123+01234567889"
- kubeadm upgrade apply supports all standard ways of specifying version via labels. Examples: stable-1.8, latest-1.8, ci/latest-1.9 and similar.
```

cmd/kubeadm/app/cmd/upgrade/BUILD

@@ -19,6 +19,7 @@ go_library(
         "//cmd/kubeadm/app/preflight:go_default_library",
         "//cmd/kubeadm/app/util:go_default_library",
         "//cmd/kubeadm/app/util/apiclient:go_default_library",
+        "//cmd/kubeadm/app/util/config:go_default_library",
         "//cmd/kubeadm/app/util/dryrun:go_default_library",
         "//cmd/kubeadm/app/util/kubeconfig:go_default_library",
         "//pkg/api:go_default_library",
@@ -41,7 +42,6 @@ go_test(
     deps = [
         "//cmd/kubeadm/app/apis/kubeadm/v1alpha1:go_default_library",
         "//cmd/kubeadm/app/phases/upgrade:go_default_library",
-        "//pkg/util/version:go_default_library",
     ],
 )
 

cmd/kubeadm/app/cmd/upgrade/apply.go

@@ -19,7 +19,6 @@ package upgrade
 import (
 	"fmt"
 	"os"
-	"strings"
 	"time"
 
 	"github.com/spf13/cobra"
@@ -32,6 +31,7 @@ import (
 	"k8s.io/kubernetes/cmd/kubeadm/app/phases/upgrade"
 	kubeadmutil "k8s.io/kubernetes/cmd/kubeadm/app/util"
 	"k8s.io/kubernetes/cmd/kubeadm/app/util/apiclient"
+	configutil "k8s.io/kubernetes/cmd/kubeadm/app/util/config"
 	dryrunutil "k8s.io/kubernetes/cmd/kubeadm/app/util/dryrun"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/util/version"
@@ -123,6 +123,19 @@ func RunApply(flags *applyFlags) error {
 	internalcfg := &kubeadmapi.MasterConfiguration{}
 	api.Scheme.Convert(upgradeVars.cfg, internalcfg, nil)
 
+	// Validate requested and validate actual version
+	if err := configutil.NormalizeKubernetesVersion(internalcfg); err != nil {
+		return err
+	}
+
+	// Use normalized version string in all following code.
+	flags.newK8sVersionStr = internalcfg.KubernetesVersion
+	k8sVer, err := version.ParseSemantic(flags.newK8sVersionStr)
+	if err != nil {
+		return fmt.Errorf("unable to parse normalized version %q as a semantic version", flags.newK8sVersionStr)
+	}
+	flags.newK8sVersion = k8sVer
+
 	// Enforce the version skew policies
 	if err := EnforceVersionPolicies(flags, upgradeVars.versionGetter); err != nil {
 		return fmt.Errorf("[upgrade/version] FATAL: %v", err)
@@ -170,15 +183,8 @@ func SetImplicitFlags(flags *applyFlags) error {
 		flags.nonInteractiveMode = true
 	}
 
-	k8sVer, err := version.ParseSemantic(flags.newK8sVersionStr)
+	if len(flags.newK8sVersionStr) == 0 {
-	if err != nil {
+		return fmt.Errorf("version string can't be empty")
-		return fmt.Errorf("couldn't parse version %q as a semantic version", flags.newK8sVersionStr)
-	}
-	flags.newK8sVersion = k8sVer
-
-	// Automatically add the "v" prefix to the string representation in case it doesn't exist
-	if !strings.HasPrefix(flags.newK8sVersionStr, "v") {
-		flags.newK8sVersionStr = fmt.Sprintf("v%s", flags.newK8sVersionStr)
 	}
 
 	return nil

cmd/kubeadm/app/cmd/upgrade/apply_test.go

@@ -19,8 +19,6 @@ package upgrade
 import (
 	"reflect"
 	"testing"
-
-	"k8s.io/kubernetes/pkg/util/version"
 )
 
 func TestSetImplicitFlags(t *testing.T) {
@@ -38,7 +36,6 @@ func TestSetImplicitFlags(t *testing.T) {
 			},
 			expectedFlags: applyFlags{
 				newK8sVersionStr:   "v1.8.0",
-				newK8sVersion:      version.MustParseSemantic("v1.8.0"),
 				dryRun:             false,
 				force:              false,
 				nonInteractiveMode: false,
@@ -53,7 +50,6 @@ func TestSetImplicitFlags(t *testing.T) {
 			},
 			expectedFlags: applyFlags{
 				newK8sVersionStr:   "v1.8.0",
-				newK8sVersion:      version.MustParseSemantic("v1.8.0"),
 				dryRun:             false,
 				force:              false,
 				nonInteractiveMode: true,
@@ -68,7 +64,6 @@ func TestSetImplicitFlags(t *testing.T) {
 			},
 			expectedFlags: applyFlags{
 				newK8sVersionStr:   "v1.8.0",
-				newK8sVersion:      version.MustParseSemantic("v1.8.0"),
 				dryRun:             true,
 				force:              false,
 				nonInteractiveMode: true,
@@ -83,7 +78,6 @@ func TestSetImplicitFlags(t *testing.T) {
 			},
 			expectedFlags: applyFlags{
 				newK8sVersionStr:   "v1.8.0",
-				newK8sVersion:      version.MustParseSemantic("v1.8.0"),
 				dryRun:             false,
 				force:              true,
 				nonInteractiveMode: true,
@@ -98,7 +92,6 @@ func TestSetImplicitFlags(t *testing.T) {
 			},
 			expectedFlags: applyFlags{
 				newK8sVersionStr:   "v1.8.0",
-				newK8sVersion:      version.MustParseSemantic("v1.8.0"),
 				dryRun:             true,
 				force:              true,
 				nonInteractiveMode: true,
@@ -113,7 +106,6 @@ func TestSetImplicitFlags(t *testing.T) {
 			},
 			expectedFlags: applyFlags{
 				newK8sVersionStr:   "v1.8.0",
-				newK8sVersion:      version.MustParseSemantic("v1.8.0"),
 				dryRun:             true,
 				force:              true,
 				nonInteractiveMode: true,
@@ -128,45 +120,6 @@ func TestSetImplicitFlags(t *testing.T) {
 			},
 			errExpected: true,
 		},
-		{ // if the new version is invalid; it should error out
-			flags: &applyFlags{
-				newK8sVersionStr: "foo",
-			},
-			expectedFlags: applyFlags{
-				newK8sVersionStr: "foo",
-			},
-			errExpected: true,
-		},
-		{ // if the new version is valid but without the "v" prefix; it parse and prepend v
-			flags: &applyFlags{
-				newK8sVersionStr: "1.8.0",
-			},
-			expectedFlags: applyFlags{
-				newK8sVersionStr: "v1.8.0",
-				newK8sVersion:    version.MustParseSemantic("v1.8.0"),
-			},
-			errExpected: false,
-		},
-		{ // valid version should succeed
-			flags: &applyFlags{
-				newK8sVersionStr: "v1.8.1",
-			},
-			expectedFlags: applyFlags{
-				newK8sVersionStr: "v1.8.1",
-				newK8sVersion:    version.MustParseSemantic("v1.8.1"),
-			},
-			errExpected: false,
-		},
-		{ // valid version should succeed
-			flags: &applyFlags{
-				newK8sVersionStr: "1.8.0-alpha.3",
-			},
-			expectedFlags: applyFlags{
-				newK8sVersionStr: "v1.8.0-alpha.3",
-				newK8sVersion:    version.MustParseSemantic("v1.8.0-alpha.3"),
-			},
-			errExpected: false,
-		},
 	}
 	for _, rt := range tests {
 		actualErr := SetImplicitFlags(rt.flags)

cmd/kubeadm/app/phases/upgrade/prepull.go

@@ -59,7 +59,7 @@ func NewDaemonSetPrepuller(client clientset.Interface, waiter apiclient.Waiter,
 
 // CreateFunc creates a DaemonSet for making the image available on every relevant node
 func (d *DaemonSetPrepuller) CreateFunc(component string) error {
-	image := images.GetCoreImage(component, d.cfg.ImageRepository, d.cfg.KubernetesVersion, d.cfg.UnifiedControlPlaneImage)
+	image := images.GetCoreImage(component, d.cfg.GetControlPlaneImageRepository(), d.cfg.KubernetesVersion, d.cfg.UnifiedControlPlaneImage)
 	ds := buildPrePullDaemonSet(component, image)
 
 	// Create the DaemonSet in the API Server

cmd/kubeadm/app/util/config/masterconfig.go

@@ -45,25 +45,11 @@ func SetInitDynamicDefaults(cfg *kubeadmapi.MasterConfiguration) error {
 	}
 	cfg.API.AdvertiseAddress = ip.String()
 
-	// Requested version is automatic CI build, thus use KubernetesCI Image Repository for core images
+	// Resolve possible version labels and validate version string
-	if kubeadmutil.KubernetesIsCIVersion(cfg.KubernetesVersion) {
+	err = NormalizeKubernetesVersion(cfg)
-		cfg.CIImageRepository = kubeadmconstants.DefaultCIImageRepository
-	}
-	// Validate version argument
-	ver, err := kubeadmutil.KubernetesReleaseVersion(cfg.KubernetesVersion)
 	if err != nil {
 		return err
 	}
-	cfg.KubernetesVersion = ver
-
-	// Parse the given kubernetes version and make sure it's higher than the lowest supported
-	k8sVersion, err := version.ParseSemantic(cfg.KubernetesVersion)
-	if err != nil {
-		return fmt.Errorf("couldn't parse kubernetes version %q: %v", cfg.KubernetesVersion, err)
-	}
-	if k8sVersion.LessThan(kubeadmconstants.MinimumControlPlaneVersion) {
-		return fmt.Errorf("this version of kubeadm only supports deploying clusters with the control plane version >= %s. Current version: %s", kubeadmconstants.MinimumControlPlaneVersion.String(), cfg.KubernetesVersion)
-	}
 
 	if cfg.Token == "" {
 		var err error
@@ -123,3 +109,30 @@ func ConfigFileAndDefaultsToInternalConfig(cfgPath string, defaultversionedcfg *
 	}
 	return internalcfg, nil
 }
+
+// NormalizeKubernetesVersion resolves version labels, sets alternative
+// image registry if requested for CI builds, and validates minimal
+// version that kubeadm supports.
+func NormalizeKubernetesVersion(cfg *kubeadmapi.MasterConfiguration) error {
+	// Requested version is automatic CI build, thus use KubernetesCI Image Repository for core images
+	if kubeadmutil.KubernetesIsCIVersion(cfg.KubernetesVersion) {
+		cfg.CIImageRepository = kubeadmconstants.DefaultCIImageRepository
+	}
+
+	// Parse and validate the version argument and resolve possible CI version labels
+	ver, err := kubeadmutil.KubernetesReleaseVersion(cfg.KubernetesVersion)
+	if err != nil {
+		return err
+	}
+	cfg.KubernetesVersion = ver
+
+	// Parse the given kubernetes version and make sure it's higher than the lowest supported
+	k8sVersion, err := version.ParseSemantic(cfg.KubernetesVersion)
+	if err != nil {
+		return fmt.Errorf("couldn't parse kubernetes version %q: %v", cfg.KubernetesVersion, err)
+	}
+	if k8sVersion.LessThan(kubeadmconstants.MinimumControlPlaneVersion) {
+		return fmt.Errorf("this version of kubeadm only supports deploying clusters with the control plane version >= %s. Current version: %s", kubeadmconstants.MinimumControlPlaneVersion.String(), cfg.KubernetesVersion)
+	}
+	return nil
+}

cmd/kubeadm/app/util/version.go

@@ -49,17 +49,22 @@ var (
 //  latest-1    (latest release in 1.x, including alpha/beta)
 //  latest-1.0  (and similarly 1.1, 1.2, 1.3, ...)
 func KubernetesReleaseVersion(version string) (string, error) {
-	if kubeReleaseRegex.MatchString(version) {
+	ver := normalizedBuildVersion(version)
-		if strings.HasPrefix(version, "v") {
+	if len(ver) != 0 {
-			return version, nil
+		return ver, nil
-		}
-		return "v" + version, nil
 	}
 
 	bucketURL, versionLabel, err := splitVersion(version)
 	if err != nil {
 		return "", err
 	}
+
+	// revalidate, if exact build from e.g. CI bucket requested.
+	ver = normalizedBuildVersion(versionLabel)
+	if len(ver) != 0 {
+		return ver, nil
+	}
+
 	if kubeReleaseLabelRegex.MatchString(versionLabel) {
 		url := fmt.Sprintf("%s/%s.txt", bucketURL, versionLabel)
 		body, err := fetchFromURL(url)
@@ -92,6 +97,18 @@ func KubernetesIsCIVersion(version string) bool {
 	return false
 }
 
+// Internal helper: returns normalized build version (with "v" prefix if needed)
+// If input doesn't match known version pattern, returns empty string.
+func normalizedBuildVersion(version string) string {
+	if kubeReleaseRegex.MatchString(version) {
+		if strings.HasPrefix(version, "v") {
+			return version
+		}
+		return "v" + version
+	}
+	return ""
+}
+
 // Internal helper: split version parts,
 // Return base URL and cleaned-up version
 func splitVersion(version string) (string, string, error) {

cmd/kubeadm/app/util/version_test.go

@@ -220,6 +220,8 @@ func TestKubernetesIsCIVersion(t *testing.T) {
 		// CI builds
 		{"ci/latest-1", true},
 		{"ci-cross/latest", true},
+		{"ci/v1.9.0-alpha.1.123+acbcbfd53bfa0a", true},
+		{"ci-cross/v1.9.0-alpha.1.123+acbcbfd53bfa0a", true},
 	}
 
 	for _, tc := range cases {
@@ -231,3 +233,58 @@ func TestKubernetesIsCIVersion(t *testing.T) {
 	}
 
 }
+
+// Validate KubernetesReleaseVersion but with bucket prefixes
+func TestCIBuildVersion(t *testing.T) {
+	type T struct {
+		input    string
+		expected string
+		valid    bool
+	}
+	cases := []T{
+		// Official releases
+		{"v1.7.0", "v1.7.0", true},
+		{"release/v1.8.0", "v1.8.0", true},
+		{"1.4.0-beta.0", "v1.4.0-beta.0", true},
+		{"release/0invalid", "", false},
+		// CI or custom builds
+		{"ci/v1.9.0-alpha.1.123+acbcbfd53bfa0a", "v1.9.0-alpha.1.123+acbcbfd53bfa0a", true},
+		{"ci-cross/v1.9.0-alpha.1.123+acbcbfd53bfa0a", "v1.9.0-alpha.1.123+acbcbfd53bfa0a", true},
+		{"ci/1.9.0-alpha.1.123+acbcbfd53bfa0a", "v1.9.0-alpha.1.123+acbcbfd53bfa0a", true},
+		{"ci-cross/1.9.0-alpha.1.123+acbcbfd53bfa0a", "v1.9.0-alpha.1.123+acbcbfd53bfa0a", true},
+		{"ci/0invalid", "", false},
+	}
+
+	for _, tc := range cases {
+		ver, err := KubernetesReleaseVersion(tc.input)
+		t.Logf("Input: %q. Result: %q, Error: %v", tc.input, ver, err)
+		switch {
+		case err != nil && tc.valid:
+			t.Errorf("KubernetesReleaseVersion: unexpected error for input %q. Error: %v", tc.input, err)
+		case err == nil && !tc.valid:
+			t.Errorf("KubernetesReleaseVersion: error expected for input %q, but result is %q", tc.input, ver)
+		case ver != tc.expected:
+			t.Errorf("KubernetesReleaseVersion: unexpected result for input %q. Expected: %q Actual: %q", tc.input, tc.expected, ver)
+		}
+	}
+}
+
+func TestNormalizedBuildVersionVersion(t *testing.T) {
+	type T struct {
+		input    string
+		expected string
+	}
+	cases := []T{
+		{"v1.7.0", "v1.7.0"},
+		{"v1.8.0-alpha.2.1231+afabd012389d53a", "v1.8.0-alpha.2.1231+afabd012389d53a"},
+		{"1.7.0", "v1.7.0"},
+		{"unknown-1", ""},
+	}
+
+	for _, tc := range cases {
+		output := normalizedBuildVersion(tc.input)
+		if output != tc.expected {
+			t.Errorf("normalizedBuildVersion: unexpected output %q for input %q. Expected: %q", output, tc.input, tc.expected)
+		}
+	}
+}