kubeadm: Merge getK8sVersionFromUserInput into enforceRequirements

`getK8sVersionFromUserInput` would attempt to load the config from a user
specified YAML file (via the `--config` option of `kubeadm upgrade plan` or
`kubeadm upgrade apply`). This is done in order to fetch the `KubernetesVersion`
field of the `ClusterConfiguration`. The complete config is then immediately
discarded. The actual config that is used during the upgrade process is fetched
from within `enforceRequirements`.

This, along with the fact that `getK8sVersionFromUserInput` is always called
immediately after `enforceRequirements` makes it possible to merge the two.
Merging them would help us simplify things and avoid future problems in
component config related patches.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>

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

@@ -73,12 +73,7 @@ func NewCmdApply(apf *applyPlanFlags) *cobra.Command {
 		DisableFlagsInUseLine: true,
 		Short:                 "Upgrade your Kubernetes cluster to the specified version",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			userVersion, err := getK8sVersionFromUserInput(flags.applyPlanFlags, args, true)
+			return runApply(flags, args)
-			if err != nil {
-				return err
-			}
-
-			return runApply(flags, userVersion)
 		},
 	}
 
@@ -110,12 +105,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, userVersion string) error {
+func runApply(flags *applyFlags, args []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).Infoln("[upgrade/apply] verifying health of cluster")
 	klog.V(1).Infoln("[upgrade/apply] retrieving configuration from cluster")
-	client, versionGetter, cfg, err := enforceRequirements(flags.applyPlanFlags, flags.dryRun, userVersion)
+	client, versionGetter, cfg, err := enforceRequirements(flags.applyPlanFlags, args, flags.dryRun, true)
 	if err != nil {
 		return err
 	}

cmd/kubeadm/app/cmd/upgrade/common.go

@@ -46,37 +46,8 @@ import (
 	kubeconfigutil "k8s.io/kubernetes/cmd/kubeadm/app/util/kubeconfig"
 )
 
-func getK8sVersionFromUserInput(flags *applyPlanFlags, args []string, versionIsMandatory bool) (string, error) {
-	var userVersion string
-
-	// 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 userVersion based on --config
-		cfg, err := configutil.LoadInitConfigurationFromFile(flags.cfgPath)
-		if err != nil {
-			return "", err
-		}
-
-		userVersion = cfg.KubernetesVersion
-	}
-
-	// the version arg is mandatory unless version is specified in the config file
-	if versionIsMandatory && userVersion == "" {
-		if err := cmdutil.ValidateExactArgNumber(args, []string{"version"}); err != nil {
-			return "", err
-		}
-	}
-
-	// If option was specified in both args and config file, args will overwrite the config file.
-	if len(args) == 1 {
-		userVersion = args[0]
-	}
-
-	return userVersion, nil
-}
-
 // enforceRequirements verifies that it's okay to upgrade and then returns the variables needed for the rest of the procedure
-func enforceRequirements(flags *applyPlanFlags, dryRun bool, newK8sVersion string) (clientset.Interface, upgrade.VersionGetter, *kubeadmapi.InitConfiguration, error) {
+func enforceRequirements(flags *applyPlanFlags, args []string, dryRun bool, upgradeApply bool) (clientset.Interface, upgrade.VersionGetter, *kubeadmapi.InitConfiguration, error) {
 	client, err := getClient(flags.kubeConfigPath, dryRun)
 	if err != nil {
 		return nil, nil, nil, errors.Wrapf(err, "couldn't create a Kubernetes client from file %q", flags.kubeConfigPath)
@@ -90,10 +61,18 @@ func enforceRequirements(flags *applyPlanFlags, dryRun bool, newK8sVersion strin
 	// Fetch the configuration from a file or ConfigMap and validate it
 	fmt.Println("[upgrade/config] Making sure the configuration is correct:")
 
+	var newK8sVersion string
 	var cfg *kubeadmapi.InitConfiguration
+
 	if flags.cfgPath != "" {
 		klog.Warning("WARNING: Usage of the --config flag for reconfiguring the cluster during upgrade is not recommended!")
 		cfg, err = configutil.LoadInitConfigurationFromFile(flags.cfgPath)
+
+		// Initialize newK8sVersion to the value in the ClusterConfiguration. This is done, so that users who use the --config option
+		// don't have to specify the Kubernetes version twice if they don't want to upgrade, but just change a setting.
+		if err != nil {
+			newK8sVersion = cfg.KubernetesVersion
+		}
 	} else {
 		cfg, err = configutil.FetchInitConfigurationFromCluster(client, os.Stdout, "upgrade/config", false)
 	}
@@ -131,8 +110,16 @@ func enforceRequirements(flags *applyPlanFlags, dryRun bool, newK8sVersion strin
 		return nil, nil, nil, errors.Wrap(err, "[upgrade/health] FATAL")
 	}
 
-	// If a new k8s version should be set, apply the change before printing the config
+	// The version arg is mandatory, during upgrade apply, unless it's specified in the config file
-	if len(newK8sVersion) != 0 {
+	if upgradeApply && newK8sVersion == "" {
+		if err := cmdutil.ValidateExactArgNumber(args, []string{"version"}); err != nil {
+			return nil, nil, nil, err
+		}
+	}
+
+	// If option was specified in both args and config file, args will overwrite the config file.
+	if len(args) == 1 {
+		newK8sVersion = args[0]
 		cfg.KubernetesVersion = newK8sVersion
 	}
 

cmd/kubeadm/app/cmd/upgrade/common_test.go

@@ -18,98 +18,11 @@ package upgrade
 
 import (
 	"bytes"
-	"io/ioutil"
-	"os"
 	"testing"
 
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
-	"k8s.io/kubernetes/cmd/kubeadm/app/constants"
 )
 
-func TestGetK8sVersionFromUserInput(t *testing.T) {
-	currentVersion := "v" + constants.CurrentKubernetesVersion.String()
-	validConfig := "apiVersion: kubeadm.k8s.io/v1beta2\n" +
-		"kind: ClusterConfiguration\n" +
-		"kubernetesVersion: " + currentVersion
-
-	var tcases = []struct {
-		name               string
-		isVersionMandatory bool
-		clusterConfig      string
-		args               []string
-		expectedErr        bool
-		expectedVersion    string
-	}{
-		{
-			name:               "No config and version as an argument",
-			isVersionMandatory: true,
-			args:               []string{"v1.13.1"},
-			expectedVersion:    "v1.13.1",
-		},
-		{
-			name:               "Neither config nor version specified",
-			isVersionMandatory: true,
-			expectedErr:        true,
-		},
-		{
-			name:               "No config and empty version as an argument",
-			isVersionMandatory: true,
-			args:               []string{""},
-			expectedErr:        true,
-		},
-		{
-			name:               "Valid config, but no version specified",
-			isVersionMandatory: true,
-			clusterConfig:      validConfig,
-			expectedVersion:    currentVersion,
-		},
-		{
-			name:               "Valid config and different version specified",
-			isVersionMandatory: true,
-			clusterConfig:      validConfig,
-			args:               []string{"v1.13.1"},
-			expectedVersion:    "v1.13.1",
-		},
-		{
-			name: "Version is optional",
-		},
-	}
-	for _, tt := range tcases {
-		t.Run(tt.name, func(t *testing.T) {
-			flags := &applyPlanFlags{}
-			if len(tt.clusterConfig) > 0 {
-				file, err := ioutil.TempFile("", "kubeadm-upgrade-common-test-*.yaml")
-				if err != nil {
-					t.Fatalf("Failed to create test config file: %+v", err)
-				}
-
-				tmpFileName := file.Name()
-				defer os.Remove(tmpFileName)
-
-				_, err = file.WriteString(tt.clusterConfig)
-				file.Close()
-				if err != nil {
-					t.Fatalf("Failed to write test config file contents: %+v", err)
-				}
-
-				flags.cfgPath = tmpFileName
-			}
-
-			userVersion, err := getK8sVersionFromUserInput(flags, tt.args, tt.isVersionMandatory)
-
-			if err == nil && tt.expectedErr {
-				t.Error("Expected error, but got success")
-			}
-			if err != nil && !tt.expectedErr {
-				t.Errorf("Unexpected error: %+v", err)
-			}
-			if userVersion != tt.expectedVersion {
-				t.Errorf("Expected %q, but got %q", tt.expectedVersion, userVersion)
-			}
-		})
-	}
-}
-
 func TestEnforceRequirements(t *testing.T) {
 	tcases := []struct {
 		name          string
@@ -139,7 +52,7 @@ func TestEnforceRequirements(t *testing.T) {
 	}
 	for _, tt := range tcases {
 		t.Run(tt.name, func(t *testing.T) {
-			_, _, _, err := enforceRequirements(&tt.flags, tt.dryRun, tt.newK8sVersion)
+			_, _, _, err := enforceRequirements(&tt.flags, nil, tt.dryRun, false)
 
 			if err == nil && tt.expectedErr {
 				t.Error("Expected error, but got success")

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

@@ -48,12 +48,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",
 		RunE: func(_ *cobra.Command, args []string) error {
-			userVersion, err := getK8sVersionFromUserInput(flags.applyPlanFlags, args, false)
+			return runPlan(flags, args)
-			if err != nil {
-				return err
-			}
-
-			return runPlan(flags, userVersion)
 		},
 	}
 
@@ -63,11 +58,11 @@ func NewCmdPlan(apf *applyPlanFlags) *cobra.Command {
 }
 
 // runPlan takes care of outputting available versions to upgrade to for the user
-func runPlan(flags *planFlags, userVersion string) error {
+func runPlan(flags *planFlags, args []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).Infoln("[upgrade/plan] verifying health of cluster")
 	klog.V(1).Infoln("[upgrade/plan] retrieving configuration from cluster")
-	client, versionGetter, cfg, err := enforceRequirements(flags.applyPlanFlags, false, userVersion)
+	client, versionGetter, cfg, err := enforceRequirements(flags.applyPlanFlags, args, false, false)
 	if err != nil {
 		return err
 	}