Merge pull request #74256 from rojkov/kubeadm-refactor-drop-newK8sVersionStr

kubeadm: drop applyFlags.newK8sVersionStr field

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

@@ -54,7 +54,6 @@ type applyFlags struct {
 	dryRun             bool
 	etcdUpgrade        bool
 	criSocket          string
-	newK8sVersionStr   string
 	imagePullTimeout   time.Duration
 }
 
@@ -77,6 +76,7 @@ func NewCmdApply(apf *applyPlanFlags) *cobra.Command {
 		DisableFlagsInUseLine: true,
 		Short:                 "Upgrade your Kubernetes cluster to the specified version.",
 		Run: func(cmd *cobra.Command, args []string) {
+			var userVersion string
 			var err error
 			flags.ignorePreflightErrorsSet, err = validation.ValidateIgnorePreflightErrors(flags.ignorePreflightErrors)
 			kubeadmutil.CheckErr(err)
@@ -88,31 +88,30 @@ func NewCmdApply(apf *applyPlanFlags) *cobra.Command {
 
 			// If the version is specified in config file, pick up that value.
 			if flags.cfgPath != "" {
-				// Note that cfg isn't preserved here, it's just an one-off to populate flags.newK8sVersionStr based on --config
+				// Note that cfg isn't preserved here, it's just an one-off to populate userVersion based on --config
 				cfg, err := configutil.LoadInitConfigurationFromFile(flags.cfgPath)
 				kubeadmutil.CheckErr(err)
 
 				if cfg.KubernetesVersion != "" {
-					flags.newK8sVersionStr = cfg.KubernetesVersion
+					userVersion = cfg.KubernetesVersion
 				}
 			}
 
 			// If the new version is already specified in config file, version arg is optional.
-			if flags.newK8sVersionStr == "" {
+			if userVersion == "" {
 				err = cmdutil.ValidateExactArgNumber(args, []string{"version"})
 				kubeadmutil.CheckErr(err)
 			}
 
 			// If option was specified in both args and config file, args will overwrite the config file.
 			if len(args) == 1 {
-				flags.newK8sVersionStr = args[0]
+				userVersion = args[0]
 			}
 
-			// Default the flags dynamically, based on each others' value
+			err = assertVersionStringIsEmpty(userVersion)
-			err = SetImplicitFlags(flags)
 			kubeadmutil.CheckErr(err)
 
-			err = runApply(flags)
+			err = runApply(flags, userVersion)
 			kubeadmutil.CheckErr(err)
 		},
 	}
@@ -145,12 +144,12 @@ func NewCmdApply(apf *applyPlanFlags) *cobra.Command {
 //   - Creating the RBAC rules for the bootstrap tokens and the cluster-info ConfigMap
 //   - Applying new kube-dns and kube-proxy manifests
 //   - Uploads the newly used configuration to the cluster ConfigMap
-func runApply(flags *applyFlags) error {
+func runApply(flags *applyFlags, userVersion string) error {
 
 	// Start with the basics, verify that the cluster is healthy and get the configuration from the cluster (using the ConfigMap)
 	klog.V(1).Infof("[upgrade/apply] verifying health of cluster")
 	klog.V(1).Infof("[upgrade/apply] retrieving configuration from cluster")
-	client, versionGetter, cfg, err := enforceRequirements(flags.applyPlanFlags, flags.dryRun, flags.newK8sVersionStr)
+	client, versionGetter, cfg, err := enforceRequirements(flags.applyPlanFlags, flags.dryRun, userVersion)
 	if err != nil {
 		return err
 	}
@@ -167,7 +166,6 @@ func runApply(flags *applyFlags) error {
 	}
 
 	// Use normalized version string in all following code.
-	flags.newK8sVersionStr = cfg.KubernetesVersion
 	newK8sVersion, err := version.ParseSemantic(cfg.KubernetesVersion)
 	if err != nil {
 		return errors.Errorf("unable to parse normalized version %q as a semantic version", cfg.KubernetesVersion)
@@ -179,7 +177,7 @@ func runApply(flags *applyFlags) error {
 
 	// Enforce the version skew policies
 	klog.V(1).Infof("[upgrade/version] enforcing version skew policies")
-	if err := EnforceVersionPolicies(newK8sVersion, flags, versionGetter); err != nil {
+	if err := EnforceVersionPolicies(cfg.KubernetesVersion, newK8sVersion, flags, versionGetter); err != nil {
 		return errors.Wrap(err, "[upgrade/version] FATAL")
 	}
 
@@ -222,16 +220,15 @@ func runApply(flags *applyFlags) error {
 	}
 
 	fmt.Println("")
-	fmt.Printf("[upgrade/successful] SUCCESS! Your cluster was upgraded to %q. Enjoy!\n", flags.newK8sVersionStr)
+	fmt.Printf("[upgrade/successful] SUCCESS! Your cluster was upgraded to %q. Enjoy!\n", cfg.KubernetesVersion)
 	fmt.Println("")
 	fmt.Println("[upgrade/kubelet] Now that your control plane is upgraded, please proceed with upgrading your kubelets if you haven't already done so.")
 
 	return nil
 }
 
-// SetImplicitFlags handles dynamically defaulting flags based on each other's value
+func assertVersionStringIsEmpty(version string) error {
-func SetImplicitFlags(flags *applyFlags) error {
+	if len(version) == 0 {
-	if len(flags.newK8sVersionStr) == 0 {
 		return errors.New("version string can't be empty")
 	}
 
@@ -240,10 +237,10 @@ func SetImplicitFlags(flags *applyFlags) error {
 
 // EnforceVersionPolicies makes sure that the version the user specified is valid to upgrade to
 // There are both fatal and skippable (with --force) errors
-func EnforceVersionPolicies(newK8sVersion *version.Version, flags *applyFlags, versionGetter upgrade.VersionGetter) error {
+func EnforceVersionPolicies(newK8sVersionStr string, newK8sVersion *version.Version, flags *applyFlags, versionGetter upgrade.VersionGetter) error {
-	fmt.Printf("[upgrade/version] You have chosen to change the cluster version to %q\n", flags.newK8sVersionStr)
+	fmt.Printf("[upgrade/version] You have chosen to change the cluster version to %q\n", newK8sVersionStr)
 
-	versionSkewErrs := upgrade.EnforceVersionPolicies(versionGetter, flags.newK8sVersionStr, newK8sVersion, flags.allowExperimentalUpgrades, flags.allowRCUpgrades)
+	versionSkewErrs := upgrade.EnforceVersionPolicies(versionGetter, newK8sVersionStr, newK8sVersion, flags.allowExperimentalUpgrades, flags.allowRCUpgrades)
 	if versionSkewErrs != nil {
 
 		if len(versionSkewErrs.Mandatory) > 0 {
@@ -268,7 +265,7 @@ func EnforceVersionPolicies(newK8sVersion *version.Version, flags *applyFlags, v
 func PerformControlPlaneUpgrade(flags *applyFlags, client clientset.Interface, waiter apiclient.Waiter, internalcfg *kubeadmapi.InitConfiguration) error {
 
 	// OK, the cluster is hosted using static pods. Upgrade a static-pod hosted cluster
-	fmt.Printf("[upgrade/apply] Upgrading your Static Pod-hosted control plane to version %q...\n", flags.newK8sVersionStr)
+	fmt.Printf("[upgrade/apply] Upgrading your Static Pod-hosted control plane to version %q...\n", internalcfg.KubernetesVersion)
 
 	if flags.dryRun {
 		return DryRunStaticPodUpgrade(internalcfg)

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

@@ -19,48 +19,31 @@ package upgrade
 import (
 	"io/ioutil"
 	"os"
-	"reflect"
 	"testing"
 
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
 	"k8s.io/kubernetes/cmd/kubeadm/app/constants"
 )
 
-func TestSetImplicitFlags(t *testing.T) {
+func TestAssertVersionStringIsEmpty(t *testing.T) {
-	var tests = []struct {
+	var tcases = []struct {
-		name          string
+		name        string
-		flags         *applyFlags
+		version     string
-		expectedFlags applyFlags
+		expectedErr bool
-		errExpected   bool
 	}{
 		{
-			name: "if the new version is empty; it should error out",
+			name:    "Version string is not empty",
-			flags: &applyFlags{
+			version: "some string",
-				newK8sVersionStr: "",
+		},
-			},
+		{
-			expectedFlags: applyFlags{
+			name:        "Version string is empty",
-				newK8sVersionStr: "",
+			expectedErr: true,
-			},
-			errExpected: true,
 		},
 	}
-	for _, rt := range tests {
+	for _, tt := range tcases {
-		t.Run(rt.name, func(t *testing.T) {
+		t.Run(tt.name, func(t *testing.T) {
-			actualErr := SetImplicitFlags(rt.flags)
+			if assertVersionStringIsEmpty(tt.version) == nil && tt.expectedErr {
-
+				t.Errorf("No error triggered for string '%s'", tt.version)
-			if !reflect.DeepEqual(*rt.flags, rt.expectedFlags) {
-				t.Errorf(
-					"failed SetImplicitFlags:\n\texpected flags: %v\n\t  actual: %v",
-					rt.expectedFlags,
-					*rt.flags,
-				)
-			}
-			if (actualErr != nil) != rt.errExpected {
-				t.Errorf(
-					"failed SetImplicitFlags:\n\texpected error: %t\n\t  actual: %t",
-					rt.errExpected,
-					(actualErr != nil),
-				)
 			}
 		})
 	}

cmd/kubeadm/app/cmd/upgrade/plan.go

@@ -38,8 +38,6 @@ import (
 
 type planFlags struct {
 	*applyPlanFlags
-
-	newK8sVersionStr string
 }
 
 // NewCmdPlan returns the cobra command for `kubeadm upgrade plan`
@@ -52,6 +50,7 @@ func NewCmdPlan(apf *applyPlanFlags) *cobra.Command {
 		Use:   "plan [version] [flags]",
 		Short: "Check which versions are available to upgrade to and validate whether your current cluster is upgradeable. To skip the internet check, pass in the optional [version] parameter.",
 		Run: func(_ *cobra.Command, args []string) {
+			var userVersion string
 			var err error
 			flags.ignorePreflightErrorsSet, err = validation.ValidateIgnorePreflightErrors(flags.ignorePreflightErrors)
 			kubeadmutil.CheckErr(err)
@@ -65,15 +64,15 @@ func NewCmdPlan(apf *applyPlanFlags) *cobra.Command {
 				kubeadmutil.CheckErr(err)
 
 				if cfg.KubernetesVersion != "" {
-					flags.newK8sVersionStr = cfg.KubernetesVersion
+					userVersion = cfg.KubernetesVersion
 				}
 			}
 			// If option was specified in both args and config file, args will overwrite the config file.
 			if len(args) == 1 {
-				flags.newK8sVersionStr = args[0]
+				userVersion = args[0]
 			}
 
-			err = runPlan(flags)
+			err = runPlan(flags, userVersion)
 			kubeadmutil.CheckErr(err)
 		},
 	}
@@ -84,11 +83,11 @@ func NewCmdPlan(apf *applyPlanFlags) *cobra.Command {
 }
 
 // runPlan takes care of outputting available versions to upgrade to for the user
-func runPlan(flags *planFlags) error {
+func runPlan(flags *planFlags, userVersion string) error {
 	// Start with the basics, verify that the cluster is healthy, build a client and a versionGetter. Never dry-run when planning.
 	klog.V(1).Infof("[upgrade/plan] verifying health of cluster")
 	klog.V(1).Infof("[upgrade/plan] retrieving configuration from cluster")
-	client, versionGetter, cfg, err := enforceRequirements(flags.applyPlanFlags, false, flags.newK8sVersionStr)
+	client, versionGetter, cfg, err := enforceRequirements(flags.applyPlanFlags, false, userVersion)
 	if err != nil {
 		return err
 	}