Uses patchHelper to update kubeadmConfig

Instead of issuing multiple update calls during the KCP upgrade, the reconciler makes the changes to the same kubeadmConfig object and the changes to the object are persisted using a patch() call. This also removes the intermediate methods on the WorkloadCluster interface and the reconciler calls these methods directly on the kubeadmConfig object. Signed-off-by: Sagar Muchhal <[email protected]>
kubernetes-sigs · Jan 6, 2021 · a395518 · a395518
1 parent 5d795d9
commit a395518
Show file tree

Hide file tree

Showing 11 changed files with 165 additions and 811 deletions.
diff --git a/controlplane/kubeadm/controllers/fakes_test.go b/controlplane/kubeadm/controllers/fakes_test.go
@@ -20,7 +20,9 @@ import (
 	"context"
 
 	"github.com/blang/semver"
+	corev1 "k8s.io/api/core/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
+	kubeadmv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/types/v1beta1"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/machinefilters"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -56,6 +58,7 @@ func (f *fakeManagementCluster) GetMachinesForCluster(c context.Context, n clien
 type fakeWorkloadCluster struct {
 	*internal.Workload
 	Status            internal.ClusterStatus
+	KubeadmConfig     fakeKubeadmConfig
 	EtcdMembersResult []string
 }
 
@@ -83,28 +86,64 @@ func (f fakeWorkloadCluster) ReconcileKubeletRBACBinding(ctx context.Context, ve
 	return nil
 }
 
-func (f fakeWorkloadCluster) UpdateKubernetesVersionInKubeadmConfigMap(ctx context.Context, version semver.Version) error {
+func (f fakeWorkloadCluster) UpdateKubeletConfigMap(ctx context.Context, version semver.Version) error {
 	return nil
 }
 
-func (f fakeWorkloadCluster) UpdateEtcdVersionInKubeadmConfigMap(ctx context.Context, imageRepository, imageTag string) error {
+func (f fakeWorkloadCluster) RemoveEtcdMemberForMachine(ctx context.Context, machine *clusterv1.Machine) error {
 	return nil
 }
 
-func (f fakeWorkloadCluster) UpdateKubeletConfigMap(ctx context.Context, version semver.Version) error {
+func (f fakeWorkloadCluster) RemoveMachineFromKubeadmConfigMap(ctx context.Context, machine *clusterv1.Machine) error {
 	return nil
 }
 
-func (f fakeWorkloadCluster) RemoveEtcdMemberForMachine(ctx context.Context, machine *clusterv1.Machine) error {
+func (f fakeWorkloadCluster) EtcdMembers(_ context.Context) ([]string, error) {
+	return f.EtcdMembersResult, nil
+}
+
+func (f fakeWorkloadCluster) GetKubeadmConfig(_ context.Context, _ client.ObjectKey) (internal.KubeadmConfig, error) {
+	return f.KubeadmConfig, nil
+}
+
+type fakeKubeadmConfig struct {
+	ConfigMap *corev1.ConfigMap
+}
+
+func (f fakeKubeadmConfig) GetConfigMap() *corev1.ConfigMap {
+	return f.ConfigMap
+}
+
+func (f fakeKubeadmConfig) RemoveAPIEndpoint(endpoint string) error {
 	return nil
 }
 
-func (f fakeWorkloadCluster) RemoveMachineFromKubeadmConfigMap(ctx context.Context, machine *clusterv1.Machine) error {
+func (f fakeKubeadmConfig) UpdateKubernetesVersion(version semver.Version) error {
 	return nil
 }
 
-func (f fakeWorkloadCluster) EtcdMembers(_ context.Context) ([]string, error) {
-	return f.EtcdMembersResult, nil
+func (f fakeKubeadmConfig) UpdateImageRepository(imageRepository string) error {
+	return nil
+}
+
+func (f fakeKubeadmConfig) UpdateEtcdMeta(imageRepository, imageTag string) (bool, error) {
+	return false, nil
+}
+
+func (f fakeKubeadmConfig) UpdateCoreDNSImageInfo(repository, tag string) error {
+	return nil
+}
+
+func (f fakeKubeadmConfig) UpdateAPIServer(apiServer kubeadmv1.APIServer) (bool, error) {
+	return false, nil
+}
+
+func (f fakeKubeadmConfig) UpdateControllerManager(controllerManager kubeadmv1.ControlPlaneComponent) (bool, error) {
+	return false, nil
+}
+
+func (f fakeKubeadmConfig) UpdateScheduler(scheduler kubeadmv1.ControlPlaneComponent) (bool, error) {
+	return false, nil
 }
 
 type fakeMigrator struct {

diff --git a/controlplane/kubeadm/controllers/scale_test.go b/controlplane/kubeadm/controllers/scale_test.go
@@ -23,15 +23,15 @@ import (
 	"time"
 
 	. "github.com/onsi/gomega"
-	"sigs.k8s.io/cluster-api/util/conditions"
-
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/record"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
 	bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1alpha4"
 	controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha4"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
+	"sigs.k8s.io/cluster-api/util/conditions"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -138,7 +138,11 @@ func TestKubeadmControlPlaneReconciler_scaleUpControlPlane(t *testing.T) {
 		fakeClient := newFakeClient(g, initObjs...)
 		fmc := &fakeManagementCluster{
 			Machines: beforeMachines.DeepCopy(),
-			Workload: fakeWorkloadCluster{},
+			Workload: fakeWorkloadCluster{
+				KubeadmConfig: fakeKubeadmConfig{
+					ConfigMap: &corev1.ConfigMap{},
+				},
+			},
 		}
 
 		r := &KubeadmControlPlaneReconciler{

diff --git a/controlplane/kubeadm/controllers/upgrade.go b/controlplane/kubeadm/controllers/upgrade.go
@@ -21,11 +21,14 @@ import (
 
 	"github.com/blang/semver"
 	"github.com/pkg/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
 	controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha4"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
 	"sigs.k8s.io/cluster-api/util"
+	"sigs.k8s.io/cluster-api/util/patch"
 	ctrl "sigs.k8s.io/controller-runtime"
+	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 func (r *KubeadmControlPlaneReconciler) upgradeControlPlane(
@@ -64,38 +67,54 @@ func (r *KubeadmControlPlaneReconciler) upgradeControlPlane(
 		return ctrl.Result{}, errors.Wrap(err, "failed to set role and role binding for kubeadm")
 	}
 
-	if err := workloadCluster.UpdateKubernetesVersionInKubeadmConfigMap(ctx, parsedVersion); err != nil {
+	configMapKey := ctrlclient.ObjectKey{Name: "kubeadm-config", Namespace: metav1.NamespaceSystem}
+	kubeadmConfig, err := workloadCluster.GetKubeadmConfig(ctx, configMapKey)
+	if err != nil {
+		return ctrl.Result{}, errors.Wrapf(err, "failed to fetch kubeadm config map")
+	}
+
+	// Initialize the patch helper.
+	patchHelper, err := patch.NewHelper(kubeadmConfig.GetConfigMap(), r.Client)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	if err := kubeadmConfig.UpdateKubernetesVersion(parsedVersion); err != nil {
 		return ctrl.Result{}, errors.Wrap(err, "failed to update the kubernetes version in the kubeadm config map")
 	}
 
 	if kcp.Spec.KubeadmConfigSpec.ClusterConfiguration != nil {
 		imageRepository := kcp.Spec.KubeadmConfigSpec.ClusterConfiguration.ImageRepository
-		if err := workloadCluster.UpdateImageRepositoryInKubeadmConfigMap(ctx, imageRepository); err != nil {
+		if err := kubeadmConfig.UpdateImageRepository(imageRepository); err != nil {
 			return ctrl.Result{}, errors.Wrap(err, "failed to update the image repository in the kubeadm config map")
 		}
 	}
 
 	if kcp.Spec.KubeadmConfigSpec.ClusterConfiguration != nil && kcp.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local != nil {
 		meta := kcp.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local.ImageMeta
-		if err := workloadCluster.UpdateEtcdVersionInKubeadmConfigMap(ctx, meta.ImageRepository, meta.ImageTag); err != nil {
+		if changed, err := kubeadmConfig.UpdateEtcdMeta(meta.ImageRepository, meta.ImageTag); err != nil || !changed {
 			return ctrl.Result{}, errors.Wrap(err, "failed to update the etcd version in the kubeadm config map")
 		}
 	}
 
 	if kcp.Spec.KubeadmConfigSpec.ClusterConfiguration != nil {
-		if err := workloadCluster.UpdateAPIServerInKubeadmConfigMap(ctx, kcp.Spec.KubeadmConfigSpec.ClusterConfiguration.APIServer); err != nil {
+		if _, err := kubeadmConfig.UpdateAPIServer(kcp.Spec.KubeadmConfigSpec.ClusterConfiguration.APIServer); err != nil {
 			return ctrl.Result{}, errors.Wrap(err, "failed to update api server in the kubeadm config map")
 		}
 
-		if err := workloadCluster.UpdateControllerManagerInKubeadmConfigMap(ctx, kcp.Spec.KubeadmConfigSpec.ClusterConfiguration.ControllerManager); err != nil {
+		if _, err := kubeadmConfig.UpdateControllerManager(kcp.Spec.KubeadmConfigSpec.ClusterConfiguration.ControllerManager); err != nil {
 			return ctrl.Result{}, errors.Wrap(err, "failed to update controller manager in the kubeadm config map")
 		}
 
-		if err := workloadCluster.UpdateSchedulerInKubeadmConfigMap(ctx, kcp.Spec.KubeadmConfigSpec.ClusterConfiguration.Scheduler); err != nil {
+		if _, err := kubeadmConfig.UpdateScheduler(kcp.Spec.KubeadmConfigSpec.ClusterConfiguration.Scheduler); err != nil {
 			return ctrl.Result{}, errors.Wrap(err, "failed to update scheduler in the kubeadm config map")
 		}
 	}
 
+	if err := patchHelper.Patch(ctx, kubeadmConfig.GetConfigMap()); err != nil {
+		return ctrl.Result{}, errors.Wrapf(err, "unable to patch kubeadm config map")
+	}
+
 	if err := workloadCluster.UpdateKubeletConfigMap(ctx, parsedVersion); err != nil {
 		return ctrl.Result{}, errors.Wrap(err, "failed to upgrade kubelet config map")
 	}

diff --git a/controlplane/kubeadm/controllers/upgrade_test.go b/controlplane/kubeadm/controllers/upgrade_test.go
@@ -22,6 +22,7 @@ import (
 
 	. "github.com/onsi/gomega"
 
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/utils/pointer"
@@ -51,12 +52,18 @@ func TestKubeadmControlPlaneReconciler_upgradeControlPlane(t *testing.T) {
 			Management: &internal.Management{Client: fakeClient},
 			Workload: fakeWorkloadCluster{
 				Status: internal.ClusterStatus{Nodes: 1},
+				KubeadmConfig: fakeKubeadmConfig{
+					ConfigMap: &corev1.ConfigMap{},
+				},
 			},
 		},
 		managementClusterUncached: &fakeManagementCluster{
 			Management: &internal.Management{Client: fakeClient},
 			Workload: fakeWorkloadCluster{
 				Status: internal.ClusterStatus{Nodes: 1},
+				KubeadmConfig: fakeKubeadmConfig{
+					ConfigMap: &corev1.ConfigMap{},
+				},
 			},
 		},
 	}

diff --git a/controlplane/kubeadm/internal/kubeadm_config_map.go b/controlplane/kubeadm/internal/kubeadm_config_map.go
@@ -17,9 +17,11 @@ limitations under the License.
 package internal
 
 import (
+	"fmt"
 	"reflect"
 	"strings"
 
+	"github.com/blang/semver"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -43,11 +45,33 @@ const (
 	schedulerKey             = "scheduler"
 )
 
-// kubeadmConfig wraps up interactions necessary for modifying the kubeadm config during an upgrade.
+// KubeadmConfig wraps up interactions necessary for modifying the kubeadm config during an upgrade.
+type KubeadmConfig interface {
+	GetConfigMap() *corev1.ConfigMap
+	RemoveAPIEndpoint(endpoint string) error
+	UpdateKubernetesVersion(version semver.Version) error
+	UpdateImageRepository(imageRepository string) error
+	UpdateEtcdMeta(imageRepository, imageTag string) (bool, error)
+	UpdateCoreDNSImageInfo(repository, tag string) error
+	UpdateAPIServer(apiServer kubeadmv1.APIServer) (bool, error)
+	UpdateControllerManager(controllerManager kubeadmv1.ControlPlaneComponent) (bool, error)
+	UpdateScheduler(scheduler kubeadmv1.ControlPlaneComponent) (bool, error)
+}
+
+var _ KubeadmConfig = (*kubeadmConfig)(nil)
+
 type kubeadmConfig struct {
 	ConfigMap *corev1.ConfigMap
 }
 
+func NewKubeadmConfig(configMap *corev1.ConfigMap) KubeadmConfig {
+	return &kubeadmConfig{ConfigMap: configMap}
+}
+
+func (k *kubeadmConfig) GetConfigMap() *corev1.ConfigMap {
+	return k.ConfigMap
+}
+
 // RemoveAPIEndpoint removes an APIEndpoint fromt he kubeadm config cluster status config map
 func (k *kubeadmConfig) RemoveAPIEndpoint(endpoint string) error {
 	data, ok := k.ConfigMap.Data[clusterStatusKey]
@@ -75,7 +99,7 @@ func (k *kubeadmConfig) RemoveAPIEndpoint(endpoint string) error {
 }
 
 // UpdateKubernetesVersion changes the kubernetes version found in the kubeadm config map
-func (k *kubeadmConfig) UpdateKubernetesVersion(version string) error {
+func (k *kubeadmConfig) UpdateKubernetesVersion(version semver.Version) error {
 	if k.ConfigMap == nil {
 		return errors.New("unable to operate on a nil config map")
 	}
@@ -87,7 +111,8 @@ func (k *kubeadmConfig) UpdateKubernetesVersion(version string) error {
 	if err != nil {
 		return errors.Wrapf(err, "unable to decode kubeadm ConfigMap's %q to Unstructured object", clusterConfigurationKey)
 	}
-	if err := unstructured.SetNestedField(configuration.UnstructuredContent(), version, configVersionKey); err != nil {
+	versionStr := fmt.Sprintf("v%s", version)
+	if err := unstructured.SetNestedField(configuration.UnstructuredContent(), versionStr, configVersionKey); err != nil {
 		return errors.Wrapf(err, "unable to update %q on kubeadm ConfigMap's %q", configVersionKey, clusterConfigurationKey)
 	}
 	updated, err := yaml.Marshal(configuration)
@@ -177,28 +202,16 @@ func (k *kubeadmConfig) UpdateEtcdMeta(imageRepository, imageTag string) (bool,
 // UpdateCoreDNSImageInfo changes the dns.ImageTag and dns.ImageRepository
 // found in the kubeadm config map
 func (k *kubeadmConfig) UpdateCoreDNSImageInfo(repository, tag string) error {
-	data, ok := k.ConfigMap.Data[clusterConfigurationKey]
-	if !ok {
-		return errors.Errorf("unable to find %q in kubeadm ConfigMap", clusterConfigurationKey)
-	}
-	configuration, err := yamlToUnstructured([]byte(data))
-	if err != nil {
-		return errors.Wrapf(err, "unable to decode kubeadm ConfigMap's %q to Unstructured object", clusterConfigurationKey)
-	}
 	dnsMap := map[string]string{
 		dnsTypeKey:            string(kubeadmv1.CoreDNS),
 		dnsImageRepositoryKey: repository,
 		dnsImageTagKey:        tag,
 	}
-	if err := unstructured.SetNestedStringMap(configuration.UnstructuredContent(), dnsMap, dnsKey); err != nil {
-		return errors.Wrapf(err, "unable to update %q on kubeadm ConfigMap", dnsKey)
-	}
-	updated, err := yaml.Marshal(configuration)
+	_, err := k.updateClusterConfiguration(dnsMap, dnsKey)
 	if err != nil {
-		return errors.Wrapf(err, "unable to encode kubeadm ConfigMap's %q to YAML", clusterConfigurationKey)
+		err = errors.Wrapf(err, "unable to update core DNS image info in kubeadm config map")
 	}
-	k.ConfigMap.Data[clusterConfigurationKey] = string(updated)
-	return nil
+	return err
 }
 
 // UpdateAPIServer sets the api server configuration to values set in `apiServer` in kubeadm config map.