From 0786fcc941b768bbcb68822d85cc0462a808fe30 Mon Sep 17 00:00:00 2001 From: Dave Chen Date: Fri, 8 Sep 2023 17:40:25 +0800 Subject: [PATCH] kubeadm: Remove the support of configurable component configs `kubeadm upgrade plan` uses to support the configure of component configs(kubeproxy and kubelet) in a config file and then check if the version is supported or not, if it's not supported it will be marked as a unsupported version and require to manually upgrade the component. This feature will make the upgrade config API much harder as this violates the no-mutation principle for upgrade, and we have seen it's quite problematic to do like this. This change removes the support of configurable component configs for `kubeadm upgrade plan`, along with the removal, the logic to parse the config file to decide whether a manual upgrade for the component configs is needed is removed as well. NOTE that API is not changed, i.e. `ManualUpgradeRequired` is not removed from `ComponentConfigVersionState` but it's no-op now. Signed-off-by: Dave Chen --- cmd/kubeadm/app/cmd/upgrade/plan.go | 23 +---- cmd/kubeadm/app/componentconfigs/configset.go | 26 +---- .../app/componentconfigs/fakeconfig_test.go | 97 +++++-------------- 3 files changed, 28 insertions(+), 118 deletions(-) diff --git a/cmd/kubeadm/app/cmd/upgrade/plan.go b/cmd/kubeadm/app/cmd/upgrade/plan.go index 74df27dca4e..64a9dc2473c 100644 --- a/cmd/kubeadm/app/cmd/upgrade/plan.go +++ b/cmd/kubeadm/app/cmd/upgrade/plan.go @@ -32,16 +32,13 @@ import ( "k8s.io/apimachinery/pkg/util/version" "k8s.io/cli-runtime/pkg/genericclioptions" "k8s.io/cli-runtime/pkg/printers" - clientset "k8s.io/client-go/kubernetes" "k8s.io/klog/v2" - kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm" outputapischeme "k8s.io/kubernetes/cmd/kubeadm/app/apis/output/scheme" outputapiv1alpha2 "k8s.io/kubernetes/cmd/kubeadm/app/apis/output/v1alpha2" "k8s.io/kubernetes/cmd/kubeadm/app/componentconfigs" "k8s.io/kubernetes/cmd/kubeadm/app/constants" "k8s.io/kubernetes/cmd/kubeadm/app/phases/upgrade" - kubeadmutil "k8s.io/kubernetes/cmd/kubeadm/app/util" "k8s.io/kubernetes/cmd/kubeadm/app/util/output" ) @@ -268,7 +265,7 @@ func runPlan(flags *planFlags, args []string, printer output.Printer) error { // Fetch the current state of the component configs klog.V(1).Infoln("[upgrade/plan] analysing component config version states") - configVersionStates, err := getComponentConfigVersionStates(&cfg.ClusterConfiguration, client, flags.cfgPath) + configVersionStates, err := componentconfigs.GetVersionStates(&cfg.ClusterConfiguration, client) if err != nil { return errors.WithMessage(err, "[upgrade/versions] FATAL") } @@ -352,24 +349,6 @@ func genUpgradePlan(up *upgrade.Upgrade, isExternalEtcd bool) (*outputapiv1alpha return &outputapiv1alpha2.UpgradePlan{Components: components}, unstableVersionFlag, nil } -func getComponentConfigVersionStates(cfg *kubeadmapi.ClusterConfiguration, client clientset.Interface, cfgPath string) ([]outputapiv1alpha2.ComponentConfigVersionState, error) { - docmap := kubeadmapi.DocumentMap{} - - if cfgPath != "" { - bytes, err := os.ReadFile(cfgPath) - if err != nil { - return nil, errors.Wrapf(err, "unable to read config file %q", cfgPath) - } - - docmap, err = kubeadmutil.SplitYAMLDocuments(bytes) - if err != nil { - return nil, err - } - } - - return componentconfigs.GetVersionStates(cfg, client, docmap) -} - // printUpgradePlan prints a UX-friendly overview of what versions are available to upgrade to func printUpgradePlan(up *upgrade.Upgrade, plan *outputapiv1alpha2.UpgradePlan, unstableVersionFlag string, isExternalEtcd bool, writer io.Writer, printer output.Printer) { printHeader := true diff --git a/cmd/kubeadm/app/componentconfigs/configset.go b/cmd/kubeadm/app/componentconfigs/configset.go index 6c2f8e74b80..0be33d0a292 100644 --- a/cmd/kubeadm/app/componentconfigs/configset.go +++ b/cmd/kubeadm/app/componentconfigs/configset.go @@ -289,38 +289,22 @@ func FetchFromClusterWithLocalOverwrites(clusterCfg *kubeadmapi.ClusterConfigura // GetVersionStates returns a slice of ComponentConfigVersionState structs // describing all supported component config groups that were identified on the cluster -func GetVersionStates(clusterCfg *kubeadmapi.ClusterConfiguration, client clientset.Interface, docmap kubeadmapi.DocumentMap) ([]outputapiv1alpha2.ComponentConfigVersionState, error) { +func GetVersionStates(clusterCfg *kubeadmapi.ClusterConfiguration, client clientset.Interface) ([]outputapiv1alpha2.ComponentConfigVersionState, error) { // We don't want to modify clusterCfg so we make a working deep copy of it. // Also, we don't want the defaulted component configs so we get rid of them. scratchClusterCfg := clusterCfg.DeepCopy() scratchClusterCfg.ComponentConfigs = kubeadmapi.ComponentConfigMap{} - // Call FetchFromClusterWithLocalOverwrites. This will populate the configs it can load and will return all - // UnsupportedConfigVersionError(s) in a sinle instance of a MultipleUnsupportedConfigVersionsError. - var multipleVerErrs UnsupportedConfigVersionsErrorMap - err := FetchFromClusterWithLocalOverwrites(scratchClusterCfg, client, docmap) + err := FetchFromCluster(scratchClusterCfg, client) if err != nil { - if vererrs, ok := err.(UnsupportedConfigVersionsErrorMap); ok { - multipleVerErrs = vererrs - } else { - // This seems to be a genuine error so we end here - return nil, err - } + // This seems to be a genuine error so we end here + return nil, err } results := []outputapiv1alpha2.ComponentConfigVersionState{} for _, handler := range known { group := handler.GroupVersion.Group - if vererr, ok := multipleVerErrs[group]; ok { - // If there is an UnsupportedConfigVersionError then we are dealing with a case where the config was user - // supplied and requires manual upgrade - results = append(results, outputapiv1alpha2.ComponentConfigVersionState{ - Group: group, - CurrentVersion: vererr.OldVersion.Version, - PreferredVersion: vererr.CurrentVersion.Version, - ManualUpgradeRequired: true, - }) - } else if _, ok := scratchClusterCfg.ComponentConfigs[group]; ok { + if _, ok := scratchClusterCfg.ComponentConfigs[group]; ok { // Normally loaded component config. No manual upgrade required on behalf of users. results = append(results, outputapiv1alpha2.ComponentConfigVersionState{ Group: group, diff --git a/cmd/kubeadm/app/componentconfigs/fakeconfig_test.go b/cmd/kubeadm/app/componentconfigs/fakeconfig_test.go index 520246c9a30..36227bfd613 100644 --- a/cmd/kubeadm/app/componentconfigs/fakeconfig_test.go +++ b/cmd/kubeadm/app/componentconfigs/fakeconfig_test.go @@ -617,72 +617,30 @@ func TestGetVersionStates(t *testing.T) { CurrentVersion: currentClusterConfigVersion, PreferredVersion: currentClusterConfigVersion, } - versionStateOld := outputapiv1alpha2.ComponentConfigVersionState{ - Group: kubeadmapiv1.GroupName, - CurrentVersion: oldClusterConfigVersion, - PreferredVersion: currentClusterConfigVersion, - ManualUpgradeRequired: true, - } cases := []struct { - desc string - obj runtime.Object - config string - expected outputapiv1alpha2.ComponentConfigVersionState + desc string + obj runtime.Object + expectedErr bool + expected outputapiv1alpha2.ComponentConfigVersionState }{ { - desc: "appropriate cluster object without overwrite", + desc: "appropriate cluster object", obj: testClusterConfigMap(currentFooClusterConfig, false), expected: versionStateCurrent, }, { - desc: "appropriate cluster object with appropriate overwrite", - obj: testClusterConfigMap(currentFooClusterConfig, false), - config: dedent.Dedent(currentBarClusterConfig), - expected: versionStateCurrent, + desc: "old config returns an error", + obj: testClusterConfigMap(oldFooClusterConfig, false), + expectedErr: true, }, { - desc: "appropriate cluster object with old overwrite", - obj: testClusterConfigMap(currentFooClusterConfig, false), - config: dedent.Dedent(oldBarClusterConfig), - expected: versionStateOld, - }, - { - desc: "old config without overwrite returns an error", - obj: testClusterConfigMap(oldFooClusterConfig, false), - expected: versionStateOld, - }, - { - desc: "old config with appropriate overwrite", - obj: testClusterConfigMap(oldFooClusterConfig, false), - config: dedent.Dedent(currentBarClusterConfig), - expected: versionStateCurrent, - }, - { - desc: "old config with old overwrite", - obj: testClusterConfigMap(oldFooClusterConfig, false), - config: dedent.Dedent(oldBarClusterConfig), - expected: versionStateOld, - }, - { - desc: "appropriate signed cluster object without overwrite", + desc: "appropriate signed cluster object", obj: testClusterConfigMap(currentFooClusterConfig, true), expected: versionStateCurrent, }, { - desc: "appropriate signed cluster object with appropriate overwrite", - obj: testClusterConfigMap(currentFooClusterConfig, true), - config: dedent.Dedent(currentBarClusterConfig), - expected: versionStateCurrent, - }, - { - desc: "appropriate signed cluster object with old overwrit", - obj: testClusterConfigMap(currentFooClusterConfig, true), - config: dedent.Dedent(oldBarClusterConfig), - expected: versionStateOld, - }, - { - desc: "old signed config without an overwrite", + desc: "old signed config", obj: testClusterConfigMap(oldFooClusterConfig, true), expected: outputapiv1alpha2.ComponentConfigVersionState{ Group: kubeadmapiv1.GroupName, @@ -690,38 +648,27 @@ func TestGetVersionStates(t *testing.T) { PreferredVersion: currentClusterConfigVersion, }, }, - { - desc: "old signed config with appropriate overwrite", - obj: testClusterConfigMap(oldFooClusterConfig, true), - config: dedent.Dedent(currentBarClusterConfig), - expected: versionStateCurrent, - }, - { - desc: "old signed config with old overwrite", - obj: testClusterConfigMap(oldFooClusterConfig, true), - config: dedent.Dedent(oldBarClusterConfig), - expected: versionStateOld, - }, } for _, test := range cases { t.Run(test.desc, func(t *testing.T) { client := clientsetfake.NewSimpleClientset(test.obj) - docmap, err := kubeadmutil.SplitYAMLDocuments([]byte(test.config)) - if err != nil { - t.Fatalf("unexpected failure of SplitYAMLDocuments: %v", err) - } - clusterCfg := testClusterCfg() - got, err := GetVersionStates(clusterCfg, client, docmap) - if err != nil { + got, err := GetVersionStates(clusterCfg, client) + if err != nil && !test.expectedErr { t.Errorf("unexpected error: %v", err) - } else if len(got) != 1 { - t.Errorf("got %d, but expected only a single result: %v", len(got), got) - } else if got[0] != test.expected { - t.Errorf("unexpected result:\n\texpected: %v\n\tgot: %v", test.expected, got[0]) + } + if err == nil { + if test.expectedErr { + t.Errorf("expected error not found: %v", test.expectedErr) + } + if len(got) != 1 { + t.Errorf("got %d, but expected only a single result: %v", len(got), got) + } else if got[0] != test.expected { + t.Errorf("unexpected result:\n\texpected: %v\n\tgot: %v", test.expected, got[0]) + } } }) }