Merge pull request #11991 from sbueringer/pr-improve-crd-migration

🐛 CRD migration: Fix cases where update validation fails

Author: k8s-ci-robot

bootstrap/kubeadm/main.go

@@ -365,7 +365,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) {
 		// Note: The kubebuilder RBAC markers above has to be kept in sync
 		// with the CRDs that should be migrated by this provider.
 		Config: map[client.Object]crdmigrator.ByObjectConfig{
-			&bootstrapv1.KubeadmConfig{}:         {UseCache: true},
+			&bootstrapv1.KubeadmConfig{}:         {UseCache: true, UseStatusForStorageVersionMigration: true},
 			&bootstrapv1.KubeadmConfigTemplate{}: {UseCache: false},
 		},
 		// The CRDMigrator is run with only concurrency 1 to ensure we don't overwhelm the apiserver by patching a

config/rbac/role.yaml

@@ -159,6 +159,13 @@ rules:
   - patch
   - update
   - watch
+- apiGroups:
+  - ipam.cluster.x-k8s.io
+  resources:
+  - ipaddressclaims/status
+  verbs:
+  - patch
+  - update
 - apiGroups:
   - runtime.cluster.x-k8s.io
   resources:

controllers/crdmigrator/crd_migrator.go

@@ -22,6 +22,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"strconv"
+	"time"
 
 	"github.com/pkg/errors"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -101,6 +102,12 @@ type ByObjectConfig struct {
 	// Note: If this is enabled, we will use the corresponding Go type of the object for Get & List calls to avoid
 	// creating additional informers for UnstructuredList/PartialObjectMetadataList.
 	UseCache bool
+
+	// UseStatusForStorageVersionMigration configures if the storage version migration for this CRD should
+	// be triggered via the status endpoint instead of an update on the CRs directly (which is the default).
+	// As mutating and validating webhooks are usually not configured on the status subresource this can help to
+	// avoid mutating & validation webhook errors that would block the no-op updates and thus the storage migration.
+	UseStatusForStorageVersionMigration bool
 }
 
 func (r *CRDMigrator) SetupWithManager(ctx context.Context, mgr ctrl.Manager, controllerOptions controller.Options) error {
@@ -164,7 +171,7 @@ func (r *CRDMigrator) setup(scheme *runtime.Scheme) error {
 		r.configByCRDName[contract.CalculateCRDName(gvk.Group, gvk.Kind)] = cfg
 	}
 
-	r.storageVersionMigrationCache = cache.New[objectEntry]()
+	r.storageVersionMigrationCache = cache.New[objectEntry](1 * time.Hour)
 	return nil
 }
 
@@ -230,7 +237,7 @@ func (r *CRDMigrator) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.R
 
 	// If phase should be run and .status.storedVersions != [storageVersion], run storage version migration.
 	if r.crdMigrationPhasesToRun.Has(StorageVersionMigrationPhase) && storageVersionMigrationRequired(crd, storageVersion) {
-		if err := r.reconcileStorageVersionMigration(ctx, crd, customResourceObjects, storageVersion); err != nil {
+		if err := r.reconcileStorageVersionMigration(ctx, crd, migrationConfig, customResourceObjects, storageVersion); err != nil {
 			return ctrl.Result{}, err
 		}
 
@@ -252,18 +259,13 @@ func (r *CRDMigrator) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.R
 }
 
 func storageVersionForCRD(crd *apiextensionsv1.CustomResourceDefinition) (string, error) {
-	var storageVersion string
 	for _, v := range crd.Spec.Versions {
 		if v.Storage {
-			storageVersion = v.Name
-			break
+			return v.Name, nil
 		}
 	}
-	if storageVersion == "" {
-		return "", errors.Errorf("could not find storage version for CustomResourceDefinition %s", crd.Name)
-	}
 
-	return storageVersion, nil
+	return "", errors.Errorf("could not find storage version for CustomResourceDefinition %s", crd.Name)
 }
 
 func storageVersionMigrationRequired(crd *apiextensionsv1.CustomResourceDefinition, storageVersion string) bool {
@@ -360,7 +362,7 @@ func listObjectsFromAPIReader(ctx context.Context, c client.Reader, objectList c
 	return objs, nil
 }
 
-func (r *CRDMigrator) reconcileStorageVersionMigration(ctx context.Context, crd *apiextensionsv1.CustomResourceDefinition, customResourceObjects []client.Object, storageVersion string) error {
+func (r *CRDMigrator) reconcileStorageVersionMigration(ctx context.Context, crd *apiextensionsv1.CustomResourceDefinition, migrationConfig ByObjectConfig, customResourceObjects []client.Object, storageVersion string) error {
 	if len(customResourceObjects) == 0 {
 		return nil
 	}
@@ -374,6 +376,7 @@ func (r *CRDMigrator) reconcileStorageVersionMigration(ctx context.Context, crd
 		Kind:    crd.Spec.Names.Kind,
 	}
 
+	errs := []error{}
 	for _, obj := range customResourceObjects {
 		e := objectEntry{
 			Kind:      gvk.Kind,
@@ -384,6 +387,9 @@ func (r *CRDMigrator) reconcileStorageVersionMigration(ctx context.Context, crd
 		}
 
 		if _, alreadyMigrated := r.storageVersionMigrationCache.Has(e.Key()); alreadyMigrated {
+			// Refresh the cache entry, so that we don't try to migrate the storage version for CRs that were
+			// already migrated successfully in cases where storage migrations failed for a subset of the CRs.
+			r.storageVersionMigrationCache.Add(e)
 			continue
 		}
 
@@ -402,16 +408,26 @@ func (r *CRDMigrator) reconcileStorageVersionMigration(ctx context.Context, crd
 		u.SetResourceVersion(obj.GetResourceVersion())
 
 		log.V(4).Info("Migrating to new storage version", gvk.Kind, klog.KObj(u))
-		err := r.Client.Patch(ctx, u, client.Apply, client.FieldOwner("crdmigrator"))
+		var err error
+		if migrationConfig.UseStatusForStorageVersionMigration {
+			err = r.Client.Status().Patch(ctx, u, client.Apply, client.FieldOwner("crdmigrator"))
+		} else {
+			err = r.Client.Patch(ctx, u, client.Apply, client.FieldOwner("crdmigrator"))
+		}
 		// If we got a NotFound error, the object no longer exists so no need to update it.
 		// If we got a Conflict error, another client wrote the object already so no need to update it.
 		if err != nil && !apierrors.IsNotFound(err) && !apierrors.IsConflict(err) {
-			return errors.Wrapf(err, "failed to migrate storage version of %s %s", gvk.Kind, klog.KObj(u))
+			errs = append(errs, errors.Wrap(err, klog.KObj(u).String()))
+			continue
 		}
 
 		r.storageVersionMigrationCache.Add(e)
 	}
 
+	if len(errs) > 0 {
+		return errors.Wrapf(kerrors.NewAggregate(errs), "failed to migrate storage version of %s objects", gvk.Kind)
+	}
+
 	return nil
 }
 
@@ -431,6 +447,7 @@ func (r *CRDMigrator) reconcileCleanupManagedFields(ctx context.Context, crd *ap
 		}
 	}
 
+	errs := []error{}
 	for _, obj := range customResourceObjects {
 		if len(obj.GetManagedFields()) == 0 {
 			continue
@@ -512,10 +529,15 @@ func (r *CRDMigrator) reconcileCleanupManagedFields(ctx context.Context, crd *ap
 			// Note: We always have to return the conflict error directly (instead of an aggregate) so retry on conflict works.
 			return err
 		}); err != nil {
-			return errors.Wrapf(kerrors.NewAggregate([]error{err, getErr}), "failed to cleanup managedFields of %s %s", crd.Spec.Names.Kind, klog.KObj(obj))
+			errs = append(errs, errors.Wrap(kerrors.NewAggregate([]error{err, getErr}), klog.KObj(obj).String()))
+			continue
 		}
 	}
 
+	if len(errs) > 0 {
+		return errors.Wrapf(kerrors.NewAggregate(errs), "failed to cleanup managedFields of %s objects", crd.Spec.Names.Kind)
+	}
+
 	return nil
 }
 

controllers/crdmigrator/crd_migrator_test.go

@@ -63,43 +63,63 @@ func TestReconcile(t *testing.T) {
 	crdObjectKey := client.ObjectKey{Name: crdName}
 
 	tests := []struct {
-		name                   string
-		skipCRDMigrationPhases []Phase
-		useCache               bool
+		name                                string
+		skipCRDMigrationPhases              []Phase
+		useCache                            bool
+		useStatusForStorageVersionMigration bool
 	}{
 		{
-			name:                   "run both StorageVersionMigration and CleanupManagedFields with cache",
-			skipCRDMigrationPhases: nil,
-			useCache:               true,
+			name:                                "run both StorageVersionMigration and CleanupManagedFields with cache",
+			skipCRDMigrationPhases:              nil,
+			useCache:                            true,
+			useStatusForStorageVersionMigration: false,
 		},
 		{
-			name:                   "run both StorageVersionMigration and CleanupManagedFields without cache",
-			skipCRDMigrationPhases: nil,
-			useCache:               false,
+			name:                                "run both StorageVersionMigration and CleanupManagedFields without cache",
+			skipCRDMigrationPhases:              nil,
+			useCache:                            false,
+			useStatusForStorageVersionMigration: false,
 		},
 		{
-			name:                   "run only CleanupManagedFields with cache",
-			skipCRDMigrationPhases: []Phase{StorageVersionMigrationPhase},
-			useCache:               true,
+			name:                                "run both StorageVersionMigration and CleanupManagedFields with cache (using status)",
+			skipCRDMigrationPhases:              nil,
+			useCache:                            true,
+			useStatusForStorageVersionMigration: true,
 		},
 		{
-			name:                   "run only CleanupManagedFields without cache",
-			skipCRDMigrationPhases: []Phase{StorageVersionMigrationPhase},
-			useCache:               false,
+			name:                                "run both StorageVersionMigration and CleanupManagedFields without cache (using status)",
+			skipCRDMigrationPhases:              nil,
+			useCache:                            false,
+			useStatusForStorageVersionMigration: true,
 		},
 		{
-			name:                   "run only StorageVersionMigration with cache",
-			skipCRDMigrationPhases: []Phase{CleanupManagedFieldsPhase},
-			useCache:               true,
+			name:                                "run only CleanupManagedFields with cache",
+			skipCRDMigrationPhases:              []Phase{StorageVersionMigrationPhase},
+			useCache:                            true,
+			useStatusForStorageVersionMigration: false,
 		},
 		{
-			name:                   "run only StorageVersionMigration without cache",
-			skipCRDMigrationPhases: []Phase{CleanupManagedFieldsPhase},
-			useCache:               false,
+			name:                                "run only CleanupManagedFields without cache",
+			skipCRDMigrationPhases:              []Phase{StorageVersionMigrationPhase},
+			useCache:                            false,
+			useStatusForStorageVersionMigration: false,
 		},
 		{
-			name:                   "skip all",
-			skipCRDMigrationPhases: []Phase{StorageVersionMigrationPhase, CleanupManagedFieldsPhase},
+			name:                                "run only StorageVersionMigration with cache",
+			skipCRDMigrationPhases:              []Phase{CleanupManagedFieldsPhase},
+			useCache:                            true,
+			useStatusForStorageVersionMigration: false,
+		},
+		{
+			name:                                "run only StorageVersionMigration without cache",
+			skipCRDMigrationPhases:              []Phase{CleanupManagedFieldsPhase},
+			useCache:                            false,
+			useStatusForStorageVersionMigration: false,
+		},
+		{
+			name:                                "skip all",
+			skipCRDMigrationPhases:              []Phase{StorageVersionMigrationPhase, CleanupManagedFieldsPhase},
+			useStatusForStorageVersionMigration: false,
 		},
 	}
 	for _, tt := range tests {
@@ -127,19 +147,19 @@ func TestReconcile(t *testing.T) {
 			// Create manager for all steps.
 			skipCRDMigrationPhases := sets.Set[Phase]{}.Insert(tt.skipCRDMigrationPhases...)
 			managerT1, err := createManagerWithCRDMigrator(tt.skipCRDMigrationPhases, map[client.Object]ByObjectConfig{
-				&t1v1beta1.TestCluster{}: {UseCache: tt.useCache},
+				&t1v1beta1.TestCluster{}: {UseCache: tt.useCache, UseStatusForStorageVersionMigration: tt.useStatusForStorageVersionMigration},
 			}, t1v1beta1.AddToScheme)
 			g.Expect(err).ToNot(HaveOccurred())
 			managerT2, err := createManagerWithCRDMigrator(tt.skipCRDMigrationPhases, map[client.Object]ByObjectConfig{
-				&t2v1beta2.TestCluster{}: {UseCache: tt.useCache},
+				&t2v1beta2.TestCluster{}: {UseCache: tt.useCache, UseStatusForStorageVersionMigration: tt.useStatusForStorageVersionMigration},
 			}, t2v1beta2.AddToScheme)
 			g.Expect(err).ToNot(HaveOccurred())
 			managerT3, err := createManagerWithCRDMigrator(tt.skipCRDMigrationPhases, map[client.Object]ByObjectConfig{
-				&t3v1beta2.TestCluster{}: {UseCache: tt.useCache},
+				&t3v1beta2.TestCluster{}: {UseCache: tt.useCache, UseStatusForStorageVersionMigration: tt.useStatusForStorageVersionMigration},
 			}, t3v1beta2.AddToScheme)
 			g.Expect(err).ToNot(HaveOccurred())
 			managerT4, err := createManagerWithCRDMigrator(tt.skipCRDMigrationPhases, map[client.Object]ByObjectConfig{
-				&t4v1beta2.TestCluster{}: {UseCache: tt.useCache},
+				&t4v1beta2.TestCluster{}: {UseCache: tt.useCache, UseStatusForStorageVersionMigration: tt.useStatusForStorageVersionMigration},
 			}, t4v1beta2.AddToScheme)
 			g.Expect(err).ToNot(HaveOccurred())
 

controlplane/kubeadm/main.go

@@ -408,7 +408,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) {
 		// Note: The kubebuilder RBAC markers above has to be kept in sync
 		// with the CRDs that should be migrated by this provider.
 		Config: map[client.Object]crdmigrator.ByObjectConfig{
-			&controlplanev1.KubeadmControlPlane{}:         {UseCache: true},
+			&controlplanev1.KubeadmControlPlane{}:         {UseCache: true, UseStatusForStorageVersionMigration: true},
 			&controlplanev1.KubeadmControlPlaneTemplate{}: {UseCache: false},
 		},
 		// The CRDMigrator is run with only concurrency 1 to ensure we don't overwhelm the apiserver by patching a

docs/book/src/developer/providers/migrations/v1.9-to-v1.10.md

@@ -39,3 +39,4 @@ maintainers of providers and consumers of our Go API.
         - Note: The CRD migrator will add the `crd-migration.cluster.x-k8s.io/observed-generation` annotation on the CRD object,
                 please ensure that if these CRD objects are deployed with a tool like kapp / Argo / Flux the annotation is not continuously removed.
       - For all CRs that should be migrated by the `CRDMigrator`: verbs: `get;list;watch;patch;update`
+      - For all CRs with `UseStatusForStorageVersionMigration: true` verbs: `update;patch` on their `/status` resource (e.g. `ipaddressclaims/status`)

internal/controllers/clusterclass/clusterclass_controller.go

@@ -100,7 +100,7 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt
 		return errors.Wrap(err, "failed setting up with a controller manager")
 	}
 
-	r.discoverVariablesCache = cache.New[runtimeclient.CallExtensionCacheEntry]()
+	r.discoverVariablesCache = cache.New[runtimeclient.CallExtensionCacheEntry](cache.DefaultTTL)
 	return nil
 }
 

internal/controllers/clusterclass/clusterclass_controller_test.go

@@ -1158,7 +1158,7 @@ func TestReconciler_reconcileVariables(t *testing.T) {
 
 			r := &Reconciler{
 				RuntimeClient:          fakeRuntimeClient,
-				discoverVariablesCache: cache.New[runtimeclient.CallExtensionCacheEntry](),
+				discoverVariablesCache: cache.New[runtimeclient.CallExtensionCacheEntry](cache.DefaultTTL),
 			}
 
 			// Pin the compatibility version used in variable CEL validation to 1.29, so we don't have to continuously refactor

internal/controllers/machine/machine_controller.go

@@ -184,7 +184,7 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt
 		Scheme:          mgr.GetScheme(),
 		PredicateLogger: r.predicateLog,
 	}
-	r.reconcileDeleteCache = cache.New[cache.ReconcileEntry]()
+	r.reconcileDeleteCache = cache.New[cache.ReconcileEntry](cache.DefaultTTL)
 	return nil
 }
 

internal/controllers/machine/machine_controller_test.go

@@ -827,8 +827,6 @@ func TestReconcileRequest(t *testing.T) {
 		},
 	}
 
-	time := metav1.Now()
-
 	testCluster := clusterv1.Cluster{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "test-cluster",
@@ -922,7 +920,7 @@ func TestReconcileRequest(t *testing.T) {
 						clusterv1.MachineControlPlaneLabel: "",
 					},
 					Finalizers:        []string{clusterv1.MachineFinalizer},
-					DeletionTimestamp: &time,
+					DeletionTimestamp: ptr.To(metav1.Now()),
 				},
 				Spec: clusterv1.MachineSpec{
 					ClusterName: "test-cluster",
@@ -958,7 +956,7 @@ func TestReconcileRequest(t *testing.T) {
 				Client:               clientFake,
 				ClusterCache:         clustercache.NewFakeClusterCache(clientFake, client.ObjectKey{Name: testCluster.Name, Namespace: testCluster.Namespace}),
 				recorder:             record.NewFakeRecorder(10),
-				reconcileDeleteCache: cache.New[cache.ReconcileEntry](),
+				reconcileDeleteCache: cache.New[cache.ReconcileEntry](cache.DefaultTTL),
 				externalTracker: external.ObjectTracker{
 					Controller:      externalfake.Controller{},
 					Cache:           &informertest.FakeInformers{},
@@ -1314,7 +1312,7 @@ func TestRemoveMachineFinalizerAfterDeleteReconcile(t *testing.T) {
 	mr := &Reconciler{
 		Client:               c,
 		ClusterCache:         clustercache.NewFakeClusterCache(c, client.ObjectKeyFromObject(testCluster)),
-		reconcileDeleteCache: cache.New[cache.ReconcileEntry](),
+		reconcileDeleteCache: cache.New[cache.ReconcileEntry](cache.DefaultTTL),
 	}
 	_, err := mr.Reconcile(ctx, reconcile.Request{NamespacedName: key})
 	g.Expect(err).ToNot(HaveOccurred())
@@ -1709,7 +1707,7 @@ func TestDrainNode(t *testing.T) {
 			r := &Reconciler{
 				Client:               c,
 				ClusterCache:         clustercache.NewFakeClusterCache(remoteClient, client.ObjectKeyFromObject(testCluster)),
-				reconcileDeleteCache: cache.New[cache.ReconcileEntry](),
+				reconcileDeleteCache: cache.New[cache.ReconcileEntry](cache.DefaultTTL),
 			}
 
 			testMachine.Status.NodeRef = &corev1.ObjectReference{
@@ -1840,7 +1838,7 @@ func TestDrainNode_withCaching(t *testing.T) {
 		WithObjects(remoteObjs...).
 		Build()
 
-	reconcileDeleteCache := cache.New[cache.ReconcileEntry]()
+	reconcileDeleteCache := cache.New[cache.ReconcileEntry](cache.DefaultTTL)
 	r := &Reconciler{
 		Client:               c,
 		ClusterCache:         clustercache.NewFakeClusterCache(remoteClient, client.ObjectKeyFromObject(testCluster)),
@@ -2498,7 +2496,7 @@ func TestShouldWaitForNodeVolumes(t *testing.T) {
 			r := &Reconciler{
 				Client:               fakeClient,
 				ClusterCache:         clustercache.NewFakeClusterCache(remoteFakeClient, client.ObjectKeyFromObject(testCluster)),
-				reconcileDeleteCache: cache.New[cache.ReconcileEntry](),
+				reconcileDeleteCache: cache.New[cache.ReconcileEntry](cache.DefaultTTL),
 			}
 
 			testMachine.Status.NodeRef = &corev1.ObjectReference{
@@ -3335,7 +3333,7 @@ func TestNodeDeletion(t *testing.T) {
 				ClusterCache:             clustercache.NewFakeClusterCache(fakeClient, client.ObjectKeyFromObject(&testCluster)),
 				recorder:                 record.NewFakeRecorder(10),
 				nodeDeletionRetryTimeout: 10 * time.Millisecond,
-				reconcileDeleteCache:     cache.New[cache.ReconcileEntry](),
+				reconcileDeleteCache:     cache.New[cache.ReconcileEntry](cache.DefaultTTL),
 			}
 
 			cluster := testCluster.DeepCopy()
@@ -3469,7 +3467,7 @@ func TestNodeDeletionWithoutNodeRefFallback(t *testing.T) {
 				ClusterCache:             clustercache.NewFakeClusterCache(fakeClient, client.ObjectKeyFromObject(&testCluster)),
 				recorder:                 record.NewFakeRecorder(10),
 				nodeDeletionRetryTimeout: 10 * time.Millisecond,
-				reconcileDeleteCache:     cache.New[cache.ReconcileEntry](),
+				reconcileDeleteCache:     cache.New[cache.ReconcileEntry](cache.DefaultTTL),
 			}
 
 			s := &scope{