From e7427c66f3b5cc0d3b4c39361ed22b40a9475ceb Mon Sep 17 00:00:00 2001 From: "Rostislav M. Georgiev" Date: Thu, 4 Jun 2020 12:50:39 +0300 Subject: [PATCH] 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 --- cmd/kubeadm/app/cmd/upgrade/apply.go | 11 +-- cmd/kubeadm/app/cmd/upgrade/common.go | 51 +++++-------- cmd/kubeadm/app/cmd/upgrade/common_test.go | 89 +--------------------- cmd/kubeadm/app/cmd/upgrade/plan.go | 11 +-- 4 files changed, 26 insertions(+), 136 deletions(-) diff --git a/cmd/kubeadm/app/cmd/upgrade/apply.go b/cmd/kubeadm/app/cmd/upgrade/apply.go index 9ca4f5c515e..536e12e392b 100644 --- a/cmd/kubeadm/app/cmd/upgrade/apply.go +++ b/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) - if err != nil { - return err - } - - return runApply(flags, userVersion) + return runApply(flags, args) }, } @@ -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 } diff --git a/cmd/kubeadm/app/cmd/upgrade/common.go b/cmd/kubeadm/app/cmd/upgrade/common.go index 824c765657b..5da9a486e12 100644 --- a/cmd/kubeadm/app/cmd/upgrade/common.go +++ b/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 - if len(newK8sVersion) != 0 { + // The version arg is mandatory, during upgrade apply, unless it's specified in the config file + 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 } diff --git a/cmd/kubeadm/app/cmd/upgrade/common_test.go b/cmd/kubeadm/app/cmd/upgrade/common_test.go index 920a1f7279b..76fbfedcff0 100644 --- a/cmd/kubeadm/app/cmd/upgrade/common_test.go +++ b/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") diff --git a/cmd/kubeadm/app/cmd/upgrade/plan.go b/cmd/kubeadm/app/cmd/upgrade/plan.go index 0274e750f48..a6e4d4bebd8 100644 --- a/cmd/kubeadm/app/cmd/upgrade/plan.go +++ b/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) - if err != nil { - return err - } - - return runPlan(flags, userVersion) + return runPlan(flags, args) }, } @@ -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 }