diff --git a/cmd/kubeadm/app/cmd/upgrade/apply.go b/cmd/kubeadm/app/cmd/upgrade/apply.go index c139a3e1c1..5e7f0288fc 100644 --- a/cmd/kubeadm/app/cmd/upgrade/apply.go +++ b/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 { diff --git a/cmd/kubeadm/app/cmd/upgrade/apply_test.go b/cmd/kubeadm/app/cmd/upgrade/apply_test.go index 36b0cb168f..5df2204252 100644 --- a/cmd/kubeadm/app/cmd/upgrade/apply_test.go +++ b/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 diff --git a/cmd/kubeadm/app/cmd/upgrade/common.go b/cmd/kubeadm/app/cmd/upgrade/common.go index 0b99a4b854..923ac8c857 100644 --- a/cmd/kubeadm/app/cmd/upgrade/common.go +++ b/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") } diff --git a/cmd/kubeadm/app/cmd/upgrade/common_test.go b/cmd/kubeadm/app/cmd/upgrade/common_test.go index a82e5d9bec..f2c228ba28 100644 --- a/cmd/kubeadm/app/cmd/upgrade/common_test.go +++ b/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 diff --git a/cmd/kubeadm/app/cmd/upgrade/plan.go b/cmd/kubeadm/app/cmd/upgrade/plan.go index b507665dda..7aa98b51bc 100644 --- a/cmd/kubeadm/app/cmd/upgrade/plan.go +++ b/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) diff --git a/cmd/kubeadm/app/cmd/upgrade/upgrade.go b/cmd/kubeadm/app/cmd/upgrade/upgrade.go index e4740f5c37..092427defe 100644 --- a/cmd/kubeadm/app/cmd/upgrade/upgrade.go +++ b/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, }