Merge pull request #74511 from rojkov/kubeadm-refactor-enforceRequirements

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

@@ -27,7 +27,6 @@ import (
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/klog"
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
-	"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/validation"
 	"k8s.io/kubernetes/cmd/kubeadm/app/cmd/options"
 	cmdutil "k8s.io/kubernetes/cmd/kubeadm/app/cmd/util"
 	"k8s.io/kubernetes/cmd/kubeadm/app/constants"
@@ -76,39 +75,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)
-
-			// Ensure the user is root
-			klog.V(1).Infof("running preflight checks")
-			err = runPreflightChecks(flags.ignorePreflightErrorsSet)
-			kubeadmutil.CheckErr(err)
-
-			// 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)
-				kubeadmutil.CheckErr(err)
-
-				if cfg.KubernetesVersion != "" {
-					userVersion = cfg.KubernetesVersion
-				}
-			}
-
-			// If the new version is already specified in config file, version arg is optional.
-			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 {
-				userVersion = args[0]
-			}
-
-			err = assertVersionStringIsEmpty(userVersion)
+			userVersion, err := getK8sVersionFromUserInput(flags.applyPlanFlags, args, true)
 			kubeadmutil.CheckErr(err)
 
 			err = runApply(flags, userVersion)
@@ -227,14 +194,6 @@ func runApply(flags *applyFlags, userVersion string) error {
 	return nil
 }
 
-func assertVersionStringIsEmpty(version string) error {
-	if len(version) == 0 {
-		return errors.New("version string can't be empty")
-	}
-
-	return nil
-}
-
 // 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(newK8sVersionStr string, newK8sVersion *version.Version, flags *applyFlags, versionGetter upgrade.VersionGetter) error {

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

@@ -25,30 +25,6 @@ import (
 	"k8s.io/kubernetes/cmd/kubeadm/app/constants"
 )
 
-func TestAssertVersionStringIsEmpty(t *testing.T) {
-	var tcases = []struct {
-		name        string
-		version     string
-		expectedErr bool
-	}{
-		{
-			name:    "Version string is not empty",
-			version: "some string",
-		},
-		{
-			name:        "Version string is empty",
-			expectedErr: true,
-		},
-	}
-	for _, tt := range tcases {
-		t.Run(tt.name, func(t *testing.T) {
-			if assertVersionStringIsEmpty(tt.version) == nil && tt.expectedErr {
-				t.Errorf("No error triggered for string '%s'", tt.version)
-			}
-		})
-	}
-}
-
 func TestSessionIsInteractive(t *testing.T) {
 	var tcases = []struct {
 		name     string

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

@@ -31,7 +31,10 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	fakediscovery "k8s.io/client-go/discovery/fake"
 	clientset "k8s.io/client-go/kubernetes"
+	"k8s.io/klog"
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
+	"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/validation"
+	cmdutil "k8s.io/kubernetes/cmd/kubeadm/app/cmd/util"
 	"k8s.io/kubernetes/cmd/kubeadm/app/constants"
 	"k8s.io/kubernetes/cmd/kubeadm/app/features"
 	"k8s.io/kubernetes/cmd/kubeadm/app/phases/upgrade"
@@ -42,8 +45,47 @@ 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) {
+	ignorePreflightErrorsSet, err := validation.ValidateIgnorePreflightErrors(flags.ignorePreflightErrors)
+	if err != nil {
+		return nil, nil, nil, err
+	}
+
+	// Ensure the user is root
+	klog.V(1).Info("running preflight checks")
+	if err := runPreflightChecks(ignorePreflightErrorsSet); err != nil {
+		return nil, nil, nil, err
+	}
 
 	client, err := getClient(flags.kubeConfigPath, dryRun)
 	if err != nil {
@@ -56,7 +98,7 @@ func enforceRequirements(flags *applyPlanFlags, dryRun bool, newK8sVersion strin
 	}
 
 	// Run healthchecks against the cluster
-	if err := upgrade.CheckClusterHealth(client, flags.ignorePreflightErrorsSet); err != nil {
+	if err := upgrade.CheckClusterHealth(client, ignorePreflightErrorsSet); err != nil {
 		return nil, nil, nil, errors.Wrap(err, "[upgrade/health] FATAL")
 	}
 

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

@@ -18,11 +18,134 @@ package upgrade
 
 import (
 	"bytes"
+	"fmt"
+	"io/ioutil"
+	"os"
 	"testing"
+	"time"
 
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
 )
 
+const (
+	validConfig = `apiVersion: kubeadm.k8s.io/v1beta1
+kind: ClusterConfiguration
+kubernetesVersion: 1.13.0
+`
+)
+
+func TestGetK8sVersionFromUserInput(t *testing.T) {
+	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:    "v1.13.0",
+		},
+		{
+			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 tnum, tt := range tcases {
+		t.Run(tt.name, func(t *testing.T) {
+			flags := &applyPlanFlags{}
+			if len(tt.clusterConfig) > 0 {
+				tmpfile := fmt.Sprintf("/tmp/kubeadm-upgrade-common-test-%d-%d.yaml", tnum, time.Now().Unix())
+				if err := ioutil.WriteFile(tmpfile, []byte(tt.clusterConfig), 0666); err != nil {
+					t.Fatalf("Failed to create test config file: %+v", err)
+				}
+				defer os.Remove(tmpfile)
+
+				flags.cfgPath = tmpfile
+			}
+
+			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 '%s', but got '%s'", tt.expectedVersion, userVersion)
+			}
+		})
+	}
+}
+
+func TestEnforceRequirements(t *testing.T) {
+	tcases := []struct {
+		name          string
+		newK8sVersion string
+		dryRun        bool
+		flags         applyPlanFlags
+		expectedErr   bool
+	}{
+		{
+			name:        "Fail pre-flight check",
+			expectedErr: true,
+		},
+		{
+			name: "Bogus preflight check disabled when also 'all' is specified",
+			flags: applyPlanFlags{
+				ignorePreflightErrors: []string{"bogusvalue", "all"},
+			},
+			expectedErr: true,
+		},
+		{
+			name: "Fail to create client",
+			flags: applyPlanFlags{
+				ignorePreflightErrors: []string{"all"},
+			},
+			expectedErr: true,
+		},
+	}
+	for _, tt := range tcases {
+		t.Run(tt.name, func(t *testing.T) {
+			_, _, _, err := enforceRequirements(&tt.flags, tt.dryRun, tt.newK8sVersion)
+
+			if err == nil && tt.expectedErr {
+				t.Error("Expected error, but got success")
+			}
+			if err != nil && !tt.expectedErr {
+				t.Errorf("Unexpected error: %+v", err)
+			}
+		})
+	}
+}
+
 func TestPrintConfiguration(t *testing.T) {
 	var tests = []struct {
 		name          string

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

@@ -29,10 +29,8 @@ import (
 	"k8s.io/apimachinery/pkg/util/version"
 	"k8s.io/klog"
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
-	"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/validation"
 	"k8s.io/kubernetes/cmd/kubeadm/app/phases/upgrade"
 	kubeadmutil "k8s.io/kubernetes/cmd/kubeadm/app/util"
-	configutil "k8s.io/kubernetes/cmd/kubeadm/app/util/config"
 	etcdutil "k8s.io/kubernetes/cmd/kubeadm/app/util/etcd"
 )
 
@@ -50,27 +48,8 @@ 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)
+			userVersion, err := getK8sVersionFromUserInput(flags.applyPlanFlags, args, false)
 			kubeadmutil.CheckErr(err)
-			// Ensure the user is root
-			err = runPreflightChecks(flags.ignorePreflightErrorsSet)
-			kubeadmutil.CheckErr(err)
-
-			// If the version is specified in config file, pick up that value.
-			if flags.cfgPath != "" {
-				cfg, err := configutil.LoadInitConfigurationFromFile(flags.cfgPath)
-				kubeadmutil.CheckErr(err)
-
-				if 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 {
-				userVersion = args[0]
-			}
 
 			err = runPlan(flags, userVersion)
 			kubeadmutil.CheckErr(err)

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

@@ -23,7 +23,6 @@ import (
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/kubernetes/cmd/kubeadm/app/cmd/options"
 	cmdutil "k8s.io/kubernetes/cmd/kubeadm/app/cmd/util"
 	kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
@@ -39,7 +38,6 @@ type applyPlanFlags struct {
 	allowRCUpgrades           bool
 	printConfig               bool
 	ignorePreflightErrors     []string
-	ignorePreflightErrorsSet  sets.String
 	out                       io.Writer
 }
 
@@ -52,7 +50,6 @@ func NewCmdUpgrade(out io.Writer) *cobra.Command {
 		allowExperimentalUpgrades: false,
 		allowRCUpgrades:           false,
 		printConfig:               false,
-		ignorePreflightErrorsSet:  sets.NewString(),
 		out:                       out,
 	}