From 45ed7ae05179ee4c5f5646bea9a748d07892fe3c Mon Sep 17 00:00:00 2001 From: Dmitry Rozhkov Date: Wed, 9 Jan 2019 12:10:57 +0200 Subject: [PATCH] kubeadm: drop applyFlags.newK8sVersionStr field The structure `applyFlags` is meant to keep a user's input from command line and as such should be immutable. Use either a variable or the validated `InitConfig.KubernetesVersion` field instead. --- cmd/kubeadm/app/cmd/upgrade/apply.go | 37 ++++++++---------- cmd/kubeadm/app/cmd/upgrade/apply_test.go | 47 ++++++++--------------- cmd/kubeadm/app/cmd/upgrade/plan.go | 13 +++---- 3 files changed, 38 insertions(+), 59 deletions(-) diff --git a/cmd/kubeadm/app/cmd/upgrade/apply.go b/cmd/kubeadm/app/cmd/upgrade/apply.go index 1fe9ae5efd..d4ce6b5a1a 100644 --- a/cmd/kubeadm/app/cmd/upgrade/apply.go +++ b/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 = SetImplicitFlags(flags) + err = assertVersionStringIsEmpty(userVersion) 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 SetImplicitFlags(flags *applyFlags) error { - if len(flags.newK8sVersionStr) == 0 { +func assertVersionStringIsEmpty(version string) error { + if len(version) == 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 { - fmt.Printf("[upgrade/version] You have chosen to change the cluster version to %q\n", flags.newK8sVersionStr) +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", 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) diff --git a/cmd/kubeadm/app/cmd/upgrade/apply_test.go b/cmd/kubeadm/app/cmd/upgrade/apply_test.go index 5fe83fff84..36b0cb168f 100644 --- a/cmd/kubeadm/app/cmd/upgrade/apply_test.go +++ b/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) { - var tests = []struct { - name string - flags *applyFlags - expectedFlags applyFlags - errExpected bool +func TestAssertVersionStringIsEmpty(t *testing.T) { + var tcases = []struct { + name string + version string + expectedErr bool }{ { - name: "if the new version is empty; it should error out", - flags: &applyFlags{ - newK8sVersionStr: "", - }, - expectedFlags: applyFlags{ - newK8sVersionStr: "", - }, - errExpected: true, + name: "Version string is not empty", + version: "some string", + }, + { + name: "Version string is empty", + expectedErr: true, }, } - for _, rt := range tests { - t.Run(rt.name, func(t *testing.T) { - actualErr := SetImplicitFlags(rt.flags) - - 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), - ) + 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) } }) } diff --git a/cmd/kubeadm/app/cmd/upgrade/plan.go b/cmd/kubeadm/app/cmd/upgrade/plan.go index 8585d15e5a..1e671ec198 100644 --- a/cmd/kubeadm/app/cmd/upgrade/plan.go +++ b/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 }