Refactor BootstrapConfig/InfraMachine managedFields for in-place

Author: sbueringer

controlplane/kubeadm/controllers/alias.go

@@ -33,6 +33,7 @@ import (
 // KubeadmControlPlaneReconciler reconciles a KubeadmControlPlane object.
 type KubeadmControlPlaneReconciler struct {
 	Client              client.Client
+	APIReader           client.Reader
 	SecretCachingClient client.Client
 	RuntimeClient       runtimeclient.Client
 	ClusterCache        clustercache.ClusterCache
@@ -51,6 +52,7 @@ type KubeadmControlPlaneReconciler struct {
 func (r *KubeadmControlPlaneReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
 	return (&kubeadmcontrolplanecontrollers.KubeadmControlPlaneReconciler{
 		Client:                      r.Client,
+		APIReader:                   r.APIReader,
 		SecretCachingClient:         r.SecretCachingClient,
 		RuntimeClient:               r.RuntimeClient,
 		ClusterCache:                r.ClusterCache,

controlplane/kubeadm/internal/controllers/controller.go

@@ -48,7 +48,6 @@ import (
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
 	runtimeclient "sigs.k8s.io/cluster-api/exp/runtime/client"
 	"sigs.k8s.io/cluster-api/feature"
-	"sigs.k8s.io/cluster-api/internal/contract"
 	"sigs.k8s.io/cluster-api/internal/util/ssa"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/cache"
@@ -66,6 +65,7 @@ import (
 
 const (
 	kcpManagerName          = "capi-kubeadmcontrolplane"
+	kcpMetadataManagerName  = "capi-kubeadmcontrolplane-metadata"
 	kubeadmControlPlaneKind = "KubeadmControlPlane"
 )
 
@@ -80,6 +80,7 @@ const (
 // KubeadmControlPlaneReconciler reconciles a KubeadmControlPlane object.
 type KubeadmControlPlaneReconciler struct {
 	Client              client.Client
+	APIReader           client.Reader
 	SecretCachingClient client.Client
 	RuntimeClient       runtimeclient.Client
 	controller          controller.Controller
@@ -106,6 +107,9 @@ type KubeadmControlPlaneReconciler struct {
 	overridePreflightChecksFunc        func(ctx context.Context, controlPlane *internal.ControlPlane, excludeFor ...*clusterv1.Machine) ctrl.Result
 	overrideCanUpdateMachineFunc       func(ctx context.Context, machine *clusterv1.Machine, machineUpToDateResult internal.UpToDateResult) (bool, error)
 	overrideCanExtensionsUpdateMachine func(ctx context.Context, machine *clusterv1.Machine, machineUpToDateResult internal.UpToDateResult, extensionHandlers []string) (bool, []string, error)
+	// Note: This field is only used for unit tests that use fake client because the fake client does not properly set resourceVersion
+	//       on BootstrapConfig/InfraMachine after ssa.Patch and then ssa.RemoveManagedFieldsForLabelsAndAnnotations would fail.
+	disableRemoveManagedFieldsForLabelsAndAnnotations bool
 }
 
 func (r *KubeadmControlPlaneReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
@@ -857,23 +861,15 @@ func (r *KubeadmControlPlaneReconciler) syncMachines(ctx context.Context, contro
 		}
 		patchHelpers[machineName] = patchHelper
 
-		labelsAndAnnotationsManagedFieldPaths := []contract.Path{
-			{"f:metadata", "f:annotations"},
-			{"f:metadata", "f:labels"},
-		}
 		infraMachine, infraMachineFound := controlPlane.InfraResources[machineName]
 		// Only update the InfraMachine if it is already found, otherwise just skip it.
 		// This could happen e.g. if the cache is not up-to-date yet.
 		if infraMachineFound {
-			// Cleanup managed fields of all InfrastructureMachines to drop ownership of labels and annotations
-			// from "manager". We do this so that InfrastructureMachines that are created using the Create method
-			// can also work with SSA. Otherwise, labels and annotations would be co-owned by our "old" "manager"
-			// and "capi-kubeadmcontrolplane" and then we would not be able to e.g. drop labels and annotations.
-			if err := ssa.DropManagedFields(ctx, r.Client, infraMachine, kcpManagerName, labelsAndAnnotationsManagedFieldPaths); err != nil {
+			if err := ssa.MigrateManagedFields(ctx, r.Client, infraMachine, kcpManagerName, kcpMetadataManagerName); err != nil {
 				return errors.Wrapf(err, "failed to clean up managedFields of InfrastructureMachine %s", klog.KObj(infraMachine))
 			}
 			// Update in-place mutating fields on InfrastructureMachine.
-			if err := r.updateExternalObject(ctx, infraMachine, infraMachine.GroupVersionKind(), controlPlane.KCP, controlPlane.Cluster); err != nil {
+			if err := r.updateLabelsAndAnnotations(ctx, infraMachine, infraMachine.GroupVersionKind(), controlPlane.KCP, controlPlane.Cluster); err != nil {
 				return errors.Wrapf(err, "failed to update InfrastructureMachine %s", klog.KObj(infraMachine))
 			}
 		}
@@ -882,15 +878,11 @@ func (r *KubeadmControlPlaneReconciler) syncMachines(ctx context.Context, contro
 		// Only update the KubeadmConfig if it is already found, otherwise just skip it.
 		// This could happen e.g. if the cache is not up-to-date yet.
 		if kubeadmConfigFound {
-			// Cleanup managed fields of all KubeadmConfigs to drop ownership of labels and annotations
-			// from "manager". We do this so that KubeadmConfigs that are created using the Create method
-			// can also work with SSA. Otherwise, labels and annotations would be co-owned by our "old" "manager"
-			// and "capi-kubeadmcontrolplane" and then we would not be able to e.g. drop labels and annotations.
-			if err := ssa.DropManagedFields(ctx, r.Client, kubeadmConfig, kcpManagerName, labelsAndAnnotationsManagedFieldPaths); err != nil {
+			if err := ssa.MigrateManagedFields(ctx, r.Client, kubeadmConfig, kcpManagerName, kcpMetadataManagerName); err != nil {
 				return errors.Wrapf(err, "failed to clean up managedFields of KubeadmConfig %s", klog.KObj(kubeadmConfig))
 			}
 			// Update in-place mutating fields on BootstrapConfig.
-			if err := r.updateExternalObject(ctx, kubeadmConfig, bootstrapv1.GroupVersion.WithKind("KubeadmConfig"), controlPlane.KCP, controlPlane.Cluster); err != nil {
+			if err := r.updateLabelsAndAnnotations(ctx, kubeadmConfig, bootstrapv1.GroupVersion.WithKind("KubeadmConfig"), controlPlane.KCP, controlPlane.Cluster); err != nil {
 				return errors.Wrapf(err, "failed to update KubeadmConfig %s", klog.KObj(kubeadmConfig))
 			}
 		}

controlplane/kubeadm/internal/controllers/controller_test.go

@@ -33,8 +33,10 @@ import (
 	"github.com/pkg/errors"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/client-go/tools/clientcmd"
@@ -1705,6 +1707,14 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 	namespace, testCluster := setup(t, g)
 	defer teardown(t, g, namespace, testCluster)
 
+	reconciler := &KubeadmControlPlaneReconciler{
+		// Note: Ensure the fieldManager defaults to manager like in prod.
+		//       Otherwise it defaults to the binary name which is not manager in tests.
+		Client:              client.WithFieldOwner(env.Client, "manager"),
+		SecretCachingClient: secretCachingClient,
+		ssaCache:            ssa.NewCache("test-controller"),
+	}
+
 	classicManager := "manager"
 	duration5s := ptr.To(int32(5))
 	duration10s := ptr.To(int32(10))
@@ -1720,12 +1730,12 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 			"metadata": map[string]interface{}{
 				"name":      "existing-inframachine",
 				"namespace": testCluster.Namespace,
-				"labels": map[string]string{
+				"labels": map[string]interface{}{
 					"preserved-label": "preserved-value",
 					"dropped-label":   "dropped-value",
 					"modified-label":  "modified-value",
 				},
-				"annotations": map[string]string{
+				"annotations": map[string]interface{}{
 					"preserved-annotation": "preserved-value",
 					"dropped-annotation":   "dropped-value",
 					"modified-annotation":  "modified-value",
@@ -1739,8 +1749,9 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 		Name:     "existing-inframachine",
 		APIGroup: clusterv1.GroupVersionInfrastructure.Group,
 	}
-	// Note: use "manager" as the field owner to mimic the manager used before ClusterAPI v1.4.0.
-	g.Expect(env.Create(ctx, existingInfraMachine, client.FieldOwner("manager"))).To(Succeed())
+	// Note: Matches up with createInfraMachine.
+	g.Expect(ssa.Patch(ctx, env.Client, kcpManagerName, existingInfraMachine)).To(Succeed())
+	g.Expect(ssa.RemoveManagedFieldsForLabelsAndAnnotations(ctx, env.Client, env.GetAPIReader(), existingInfraMachine, kcpManagerName)).To(Succeed())
 
 	// Existing KubeadmConfig
 	bootstrapSpec := &bootstrapv1.KubeadmConfigSpec{
@@ -1772,8 +1783,9 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 		Name:     "existing-kubeadmconfig",
 		APIGroup: bootstrapv1.GroupVersion.Group,
 	}
-	// Note: use "manager" as the field owner to mimic the manager used before ClusterAPI v1.4.0.
-	g.Expect(env.Create(ctx, existingKubeadmConfig, client.FieldOwner("manager"))).To(Succeed())
+	// Note: Matches up with createKubeadmConfig.
+	g.Expect(ssa.Patch(ctx, env.Client, kcpManagerName, existingKubeadmConfig)).To(Succeed())
+	g.Expect(ssa.RemoveManagedFieldsForLabelsAndAnnotations(ctx, env.Client, env.GetAPIReader(), existingKubeadmConfig, kcpManagerName)).To(Succeed())
 
 	// Existing Machine to validate in-place mutation
 	fd := "fd1"
@@ -1812,7 +1824,7 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 			},
 		},
 	}
-	g.Expect(env.PatchAndWait(ctx, inPlaceMutatingMachine, client.FieldOwner(kcpManagerName))).To(Succeed())
+	g.Expect(reconciler.createMachine(ctx, &controlplanev1.KubeadmControlPlane{}, inPlaceMutatingMachine)).To(Succeed())
 
 	// Existing machine that is in deleting state
 	deletingMachine := &clusterv1.Machine{
@@ -1835,7 +1847,11 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 				Name:     "inframachine",
 			},
 			Bootstrap: clusterv1.Bootstrap{
-				DataSecretName: ptr.To("machine-bootstrap-secret"),
+				ConfigRef: clusterv1.ContractVersionedObjectReference{
+					Kind:     "KubeadmConfig",
+					Name:     "non-existing-kubeadmconfig",
+					APIGroup: bootstrapv1.GroupVersion.Group,
+				},
 			},
 			Deletion: clusterv1.MachineDeletionSpec{
 				NodeDrainTimeoutSeconds:        duration5s,
@@ -1845,7 +1861,7 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 			ReadinessGates: desiredstate.MandatoryMachineReadinessGates,
 		},
 	}
-	g.Expect(env.PatchAndWait(ctx, deletingMachine, client.FieldOwner(kcpManagerName))).To(Succeed())
+	g.Expect(reconciler.createMachine(ctx, &controlplanev1.KubeadmControlPlane{}, deletingMachine)).To(Succeed())
 	// Delete the machine to put it in the deleting state
 	g.Expect(env.Delete(ctx, deletingMachine)).To(Succeed())
 	// Wait till the machine is marked for deletion
@@ -1877,12 +1893,15 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 				Name:     "inframachine",
 			},
 			Bootstrap: clusterv1.Bootstrap{
-				DataSecretName: ptr.To("machine-bootstrap-secret"),
+				ConfigRef: clusterv1.ContractVersionedObjectReference{
+					Kind:     "KubeadmConfig",
+					Name:     "non-existing-kubeadmconfig",
+					APIGroup: bootstrapv1.GroupVersion.Group,
+				},
 			},
 		},
 	}
-	g.Expect(env.Create(ctx, nilInfraMachineMachine, client.FieldOwner(classicManager))).To(Succeed())
-	// Delete the machine to put it in the deleting state
+	g.Expect(reconciler.createMachine(ctx, &controlplanev1.KubeadmControlPlane{}, nilInfraMachineMachine)).To(Succeed())
 
 	kcp := &controlplanev1.KubeadmControlPlane{
 		TypeMeta: metav1.TypeMeta{
@@ -1950,11 +1969,6 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 
 	// Run syncMachines to clean up managed fields and have proper field ownership
 	// for Machines, InfrastructureMachines and KubeadmConfigs.
-	reconciler := &KubeadmControlPlaneReconciler{
-		Client:              env,
-		SecretCachingClient: secretCachingClient,
-		ssaCache:            ssa.NewCache("test-controller"),
-	}
 	g.Expect(reconciler.syncMachines(ctx, controlPlane)).To(Succeed())
 
 	// The inPlaceMutatingMachine should have cleaned up managed fields.
@@ -1970,41 +1984,39 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 		"in-place mutable machine should not contain an entry for old manager",
 	)
 
-	// The InfrastructureMachine should have ownership of "labels" and "annotations" transferred to
-	// "capi-kubeadmcontrolplane" manager.
 	updatedInfraMachine := existingInfraMachine.DeepCopy()
 	g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(updatedInfraMachine), updatedInfraMachine)).To(Succeed())
 
 	// Verify ManagedFields
+	// capi-kubeadmcontrolplane-metadata owns labels and annotations
 	g.Expect(updatedInfraMachine.GetManagedFields()).Should(
-		ssa.MatchFieldOwnership(kcpManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:metadata", "f:labels"}))
+		ssa.MatchFieldOwnership(kcpMetadataManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:metadata", "f:labels"}))
 	g.Expect(updatedInfraMachine.GetManagedFields()).Should(
-		ssa.MatchFieldOwnership(kcpManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:metadata", "f:annotations"}))
+		ssa.MatchFieldOwnership(kcpMetadataManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:metadata", "f:annotations"}))
 	g.Expect(updatedInfraMachine.GetManagedFields()).ShouldNot(
-		ssa.MatchFieldOwnership(classicManager, metav1.ManagedFieldsOperationUpdate, contract.Path{"f:metadata", "f:labels"}))
+		ssa.MatchFieldOwnership(kcpManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:metadata", "f:labels"}))
 	g.Expect(updatedInfraMachine.GetManagedFields()).ShouldNot(
-		ssa.MatchFieldOwnership(classicManager, metav1.ManagedFieldsOperationUpdate, contract.Path{"f:metadata", "f:annotations"}))
-	// Verify ownership of other fields is not changed.
+		ssa.MatchFieldOwnership(kcpManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:metadata", "f:annotations"}))
+	// capi-kubeadmcontrolplane owns spec
 	g.Expect(updatedInfraMachine.GetManagedFields()).Should(
-		ssa.MatchFieldOwnership(classicManager, metav1.ManagedFieldsOperationUpdate, contract.Path{"f:spec"}))
+		ssa.MatchFieldOwnership(kcpManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:spec"}))
 
-	// The KubeadmConfig should have ownership of "labels" and "annotations" transferred to
-	// "capi-kubeadmcontrolplane" manager.
 	updatedKubeadmConfig := existingKubeadmConfig.DeepCopy()
 	g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(updatedKubeadmConfig), updatedKubeadmConfig)).To(Succeed())
 
 	// Verify ManagedFields
+	// capi-kubeadmcontrolplane-metadata owns labels and annotations
 	g.Expect(updatedKubeadmConfig.GetManagedFields()).Should(
-		ssa.MatchFieldOwnership(kcpManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:metadata", "f:labels"}))
+		ssa.MatchFieldOwnership(kcpMetadataManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:metadata", "f:labels"}))
 	g.Expect(updatedKubeadmConfig.GetManagedFields()).Should(
-		ssa.MatchFieldOwnership(kcpManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:metadata", "f:annotations"}))
+		ssa.MatchFieldOwnership(kcpMetadataManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:metadata", "f:annotations"}))
 	g.Expect(updatedKubeadmConfig.GetManagedFields()).ShouldNot(
-		ssa.MatchFieldOwnership(classicManager, metav1.ManagedFieldsOperationUpdate, contract.Path{"f:metadata", "f:labels"}))
+		ssa.MatchFieldOwnership(kcpManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:metadata", "f:labels"}))
 	g.Expect(updatedKubeadmConfig.GetManagedFields()).ShouldNot(
-		ssa.MatchFieldOwnership(classicManager, metav1.ManagedFieldsOperationUpdate, contract.Path{"f:metadata", "f:annotations"}))
-	// Verify ownership of other fields is not changed.
+		ssa.MatchFieldOwnership(kcpManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:metadata", "f:annotations"}))
+	// capi-kubeadmcontrolplane owns spec
 	g.Expect(updatedKubeadmConfig.GetManagedFields()).Should(
-		ssa.MatchFieldOwnership(classicManager, metav1.ManagedFieldsOperationUpdate, contract.Path{"f:spec"}))
+		ssa.MatchFieldOwnership(kcpManagerName, metav1.ManagedFieldsOperationApply, contract.Path{"f:spec"}))
 
 	//
 	// Verify In-place mutating fields
@@ -2098,9 +2110,10 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 		ContainElement(ssa.MatchManagedFieldsEntry(kcpManagerName, metav1.ManagedFieldsOperationApply)),
 		"deleting machine should contain an entry for SSA manager",
 	)
-	g.Expect(updatedDeletingMachine.ManagedFields).ShouldNot(
+	// "manager" will own fields propagated to the deleting Machine with the pachHelper.
+	g.Expect(updatedDeletingMachine.ManagedFields).Should(
 		ContainElement(ssa.MatchManagedFieldsEntry(classicManager, metav1.ManagedFieldsOperationUpdate)),
-		"deleting machine should not contain an entry for old manager",
+		"deleting machine should contain an entry for old manager",
 	)
 
 	// Verify the machine labels and annotations are unchanged.
@@ -3923,9 +3936,17 @@ func TestObjectsPendingDelete(t *testing.T) {
 // test utils.
 
 func newFakeClient(initObjs ...client.Object) client.WithWatch {
+	// Use a new scheme to avoid side effects if multiple tests are sharing the same global scheme.
+	scheme := runtime.NewScheme()
+	_ = appsv1.AddToScheme(scheme)
+	_ = corev1.AddToScheme(scheme)
+	_ = apiextensionsv1.AddToScheme(scheme)
+	_ = clusterv1.AddToScheme(scheme)
+	_ = bootstrapv1.AddToScheme(scheme)
+	_ = controlplanev1.AddToScheme(scheme)
 	return &fakeClient{
 		startTime: time.Now(),
-		WithWatch: fake.NewClientBuilder().WithObjects(initObjs...).WithStatusSubresource(&controlplanev1.KubeadmControlPlane{}).Build(),
+		WithWatch: fake.NewClientBuilder().WithScheme(scheme).WithObjects(initObjs...).WithStatusSubresource(&controlplanev1.KubeadmControlPlane{}).Build(),
 	}
 }