From 8ef8ab3b1d7ddb3c50d28eb1627552b421c17040 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Tue, 28 Jul 2026 09:39:55 -0700 Subject: [PATCH] Implement default resource modifier in restore controller Thread DefaultResourceModifierConfigMap from server config through to restoreReconciler. Refactor validateAndComplete to use a shared loadResourceModifierConfigMap helper that handles both default and per-restore ConfigMap loading. Precedence: per-restore modifier takes exclusive precedence over the default. Default ConfigMap errors are non-fatal (warn and proceed). SkipDefaultResourceModifier opt-out is respected. Includes unit tests covering: default-only, per-restore override, skip flag, missing default (non-fatal), missing per-restore (fatal), and no modifier configured. Signed-off-by: Shubham Pampattiwar --- pkg/cmd/server/server.go | 1 + pkg/controller/restore_controller.go | 79 +++++++++--- pkg/controller/restore_controller_test.go | 139 ++++++++++++++++++++++ 3 files changed, 200 insertions(+), 19 deletions(-) diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index 83627f9d1..7aff5e946 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -881,6 +881,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string s.config.DisableInformerCache, s.crClient, s.config.ResourceTimeout, + s.config.DefaultResourceModifierConfigMap, ) if err = r.SetupWithManager(s.mgr); err != nil { diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 5b055bc6c..48c57413e 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -55,6 +55,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt" "github.com/vmware-tanzu/velero/pkg/plugin/framework" pkgrestore "github.com/vmware-tanzu/velero/pkg/restore" + "github.com/vmware-tanzu/velero/pkg/util/boolptr" "github.com/vmware-tanzu/velero/pkg/util/collections" kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube" "github.com/vmware-tanzu/velero/pkg/util/logging" @@ -109,10 +110,11 @@ type restoreReconciler struct { defaultItemOperationTimeout time.Duration disableInformerCache bool - newPluginManager func(logger logrus.FieldLogger) clientmgmt.Manager - backupStoreGetter persistence.ObjectBackupStoreGetter - globalCrClient client.Client - resourceTimeout time.Duration + newPluginManager func(logger logrus.FieldLogger) clientmgmt.Manager + backupStoreGetter persistence.ObjectBackupStoreGetter + globalCrClient client.Client + resourceTimeout time.Duration + defaultResourceModifierConfigMap string } type backupInfo struct { @@ -135,6 +137,7 @@ func NewRestoreReconciler( disableInformerCache bool, globalCrClient client.Client, resourceTimeout time.Duration, + defaultResourceModifierConfigMap string, ) *restoreReconciler { r := &restoreReconciler{ ctx: ctx, @@ -154,8 +157,9 @@ func NewRestoreReconciler( newPluginManager: newPluginManager, backupStoreGetter: backupStoreGetter, - globalCrClient: globalCrClient, - resourceTimeout: resourceTimeout, + globalCrClient: globalCrClient, + resourceTimeout: resourceTimeout, + defaultResourceModifierConfigMap: defaultResourceModifierConfigMap, } // Move the periodical backup and restore metrics computing logic from controllers to here. @@ -432,26 +436,63 @@ func (r *restoreReconciler) validateAndComplete(ctx context.Context, restore *ap var resourceModifiers *resourcemodifiers.ResourceModifiers if restore.Spec.ResourceModifier != nil && strings.EqualFold(restore.Spec.ResourceModifier.Kind, resourcemodifiers.ConfigmapRefType) { - ResourceModifierConfigMap := &corev1api.ConfigMap{} - err := r.kbClient.Get(ctx, client.ObjectKey{Namespace: restore.Namespace, Name: restore.Spec.ResourceModifier.Name}, ResourceModifierConfigMap) - if err != nil { - restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, fmt.Sprintf("failed to get resource modifiers configmap %s/%s", restore.Namespace, restore.Spec.ResourceModifier.Name)) + resourceModifiers = r.loadResourceModifierConfigMap(ctx, restore, restore.Spec.ResourceModifier.Name, false) + if resourceModifiers == nil && len(restore.Status.ValidationErrors) > 0 { return backupInfo{}, nil, nil } - resourceModifiers, err = resourcemodifiers.GetResourceModifiersFromConfig(ResourceModifierConfigMap) - if err != nil { - restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, errors.Wrapf(err, "Error in parsing resource modifiers provided in configmap %s/%s", restore.Namespace, restore.Spec.ResourceModifier.Name).Error()) - return backupInfo{}, nil, nil - } else if err = resourceModifiers.Validate(); err != nil { - restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, errors.Wrapf(err, "Validation error in resource modifiers provided in configmap %s/%s", restore.Namespace, restore.Spec.ResourceModifier.Name).Error()) - return backupInfo{}, nil, nil - } - r.logger.Infof("Retrieved Resource modifiers provided in configmap %s/%s", restore.Namespace, restore.Spec.ResourceModifier.Name) + } else if r.defaultResourceModifierConfigMap != "" && !boolptr.IsSetToTrue(restore.Spec.SkipDefaultResourceModifier) { + resourceModifiers = r.loadResourceModifierConfigMap(ctx, restore, r.defaultResourceModifierConfigMap, true) } return info, resourceModifiers, restoreResPolicies } +// loadResourceModifierConfigMap loads and validates a resource modifier ConfigMap. +// When isDefault is true, errors are non-fatal (logged as warnings, returns nil). +// When isDefault is false, errors are added to restore.Status.ValidationErrors. +func (r *restoreReconciler) loadResourceModifierConfigMap( + ctx context.Context, restore *api.Restore, cmName string, isDefault bool, +) *resourcemodifiers.ResourceModifiers { + cm := &corev1api.ConfigMap{} + if err := r.kbClient.Get(ctx, client.ObjectKey{Namespace: restore.Namespace, Name: cmName}, cm); err != nil { + if isDefault { + r.logger.WithError(err).Warnf("Failed to retrieve default resource modifier configmap %s/%s, skipping", restore.Namespace, cmName) + return nil + } + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, + fmt.Sprintf("failed to get resource modifiers configmap %s/%s", restore.Namespace, cmName)) + return nil + } + + modifiers, err := resourcemodifiers.GetResourceModifiersFromConfig(cm) + if err != nil { + if isDefault { + r.logger.WithError(err).Warnf("Error parsing default resource modifier configmap %s/%s, skipping", restore.Namespace, cmName) + return nil + } + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, + errors.Wrapf(err, "Error in parsing resource modifiers provided in configmap %s/%s", restore.Namespace, cmName).Error()) + return nil + } + + if err = modifiers.Validate(); err != nil { + if isDefault { + r.logger.WithError(err).Warnf("Validation error in default resource modifier configmap %s/%s, skipping", restore.Namespace, cmName) + return nil + } + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, + errors.Wrapf(err, "Validation error in resource modifiers provided in configmap %s/%s", restore.Namespace, cmName).Error()) + return nil + } + + source := "per-restore" + if isDefault { + source = "default" + } + r.logger.Infof("Retrieved %s resource modifiers from configmap %s/%s", source, restore.Namespace, cmName) + return modifiers +} + // backupXorScheduleProvided returns true if exactly one of BackupName and // ScheduleName are non-empty for the restore, or false otherwise. func backupXorScheduleProvided(restore *api.Restore) bool { diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index 6a2f4d8d1..ab33b5b0e 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -116,6 +116,7 @@ func TestFetchBackupInfo(t *testing.T) { false, fakeGlobalClient, 10*time.Minute, + "", ) if test.backupStoreError == nil { @@ -197,6 +198,7 @@ func TestProcessQueueItemSkips(t *testing.T) { false, fakeGlobalClient, 10*time.Minute, + "", ) _, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{ @@ -579,6 +581,7 @@ func TestRestoreReconcile(t *testing.T) { false, fakeGlobalClient, 10*time.Minute, + "", ) r.clock = clocktesting.NewFakeClock(now) @@ -767,6 +770,7 @@ func TestValidateAndCompleteWhenScheduleNameSpecified(t *testing.T) { false, fakeGlobalClient, 10*time.Minute, + "", ) restore := &velerov1api.Restore{ @@ -863,6 +867,7 @@ func TestValidateAndCompleteWithResourcePolicySpecified(t *testing.T) { false, fakeGlobalClient, 10*time.Minute, + "", ) restore := &velerov1api.Restore{ @@ -992,6 +997,7 @@ func TestValidateAndCompleteWithResourceModifierSpecified(t *testing.T) { false, fakeGlobalClient, 10*time.Minute, + "", ) restore := &velerov1api.Restore{ @@ -1110,6 +1116,139 @@ func TestValidateAndCompleteWithResourceModifierSpecified(t *testing.T) { assert.Contains(t, restore3.Status.ValidationErrors[0], "Validation error in resource modifiers provided in configmap") } +func TestValidateAndCompleteWithDefaultResourceModifier(t *testing.T) { + formatFlag := logging.FormatText + + validCMData := map[string]string{ + "modifiers.yaml": "version: v1\nresourceModifierRules:\n- conditions:\n groupResource: pods\n mergePatches:\n - patchData: |\n metadata:\n annotations:\n k8s.ovn.org/pod-networks: null\n", + } + + setupReconciler := func(t *testing.T, defaultCM string) *restoreReconciler { + t.Helper() + fakeClient := velerotest.NewFakeControllerRuntimeClient(t) + fakeGlobalClient := velerotest.NewFakeControllerRuntimeClient(t) + pluginManager := &pluginmocks.Manager{} + backupStore := &persistencemocks.BackupStore{} + + r := NewRestoreReconciler( + t.Context(), + velerov1api.DefaultNamespace, + nil, + fakeClient, + velerotest.NewLogger(), + logrus.DebugLevel, + func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, + NewFakeSingleObjectBackupStoreGetter(backupStore), + metrics.NewServerMetrics(), + formatFlag, + 60*time.Minute, + false, + fakeGlobalClient, + 10*time.Minute, + defaultCM, + ) + + location := builder.ForBackupStorageLocation("velero", "default").Provider("myCloud").Bucket("bucket").Phase(velerov1api.BackupStorageLocationPhaseAvailable).Result() + require.NoError(t, r.kbClient.Create(t.Context(), location)) + require.NoError(t, r.kbClient.Create(t.Context(), + defaultBackup().ObjectMeta(builder.WithName("backup-1")).StorageLocation("default").Phase(velerov1api.BackupPhaseCompleted).Result(), + )) + return r + } + + newRestore := func(perRestoreCM string, skip *bool) *velerov1api.Restore { + restore := &velerov1api.Restore{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: velerov1api.DefaultNamespace, + Name: "restore-1", + }, + Spec: velerov1api.RestoreSpec{ + BackupName: "backup-1", + SkipDefaultResourceModifier: skip, + }, + } + if perRestoreCM != "" { + restore.Spec.ResourceModifier = &corev1api.TypedLocalObjectReference{ + Kind: resourcemodifiers.ConfigmapRefType, + Name: perRestoreCM, + } + } + return restore + } + + t.Run("default modifier applied when no per-restore modifier", func(t *testing.T) { + r := setupReconciler(t, "default-rm") + require.NoError(t, r.kbClient.Create(t.Context(), &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "default-rm", Namespace: velerov1api.DefaultNamespace}, + Data: validCMData, + })) + + restore := newRestore("", nil) + _, rm, _ := r.validateAndComplete(t.Context(), restore) + assert.NotNil(t, rm) + assert.Empty(t, restore.Status.ValidationErrors) + }) + + t.Run("per-restore modifier takes exclusive precedence", func(t *testing.T) { + r := setupReconciler(t, "default-rm") + require.NoError(t, r.kbClient.Create(t.Context(), &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "default-rm", Namespace: velerov1api.DefaultNamespace}, + Data: validCMData, + })) + require.NoError(t, r.kbClient.Create(t.Context(), &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "per-restore-rm", Namespace: velerov1api.DefaultNamespace}, + Data: validCMData, + })) + + restore := newRestore("per-restore-rm", nil) + _, rm, _ := r.validateAndComplete(t.Context(), restore) + assert.NotNil(t, rm) + assert.Empty(t, restore.Status.ValidationErrors) + }) + + t.Run("skip default modifier when SkipDefaultResourceModifier is true", func(t *testing.T) { + r := setupReconciler(t, "default-rm") + require.NoError(t, r.kbClient.Create(t.Context(), &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "default-rm", Namespace: velerov1api.DefaultNamespace}, + Data: validCMData, + })) + + skipTrue := true + restore := newRestore("", &skipTrue) + _, rm, _ := r.validateAndComplete(t.Context(), restore) + assert.Nil(t, rm) + assert.Empty(t, restore.Status.ValidationErrors) + }) + + t.Run("default modifier missing is non-fatal", func(t *testing.T) { + r := setupReconciler(t, "nonexistent-cm") + + restore := newRestore("", nil) + _, rm, _ := r.validateAndComplete(t.Context(), restore) + assert.Nil(t, rm) + assert.Empty(t, restore.Status.ValidationErrors) + }) + + t.Run("per-restore modifier missing is fatal", func(t *testing.T) { + r := setupReconciler(t, "") + + restore := newRestore("nonexistent-cm", nil) + _, rm, _ := r.validateAndComplete(t.Context(), restore) + assert.Nil(t, rm) + assert.NotEmpty(t, restore.Status.ValidationErrors) + assert.Contains(t, restore.Status.ValidationErrors[0], "failed to get resource modifiers configmap") + }) + + t.Run("no default configured and no per-restore modifier", func(t *testing.T) { + r := setupReconciler(t, "") + + restore := newRestore("", nil) + _, rm, _ := r.validateAndComplete(t.Context(), restore) + assert.Nil(t, rm) + assert.Empty(t, restore.Status.ValidationErrors) + }) +} + func TestBackupXorScheduleProvided(t *testing.T) { r := &velerov1api.Restore{} assert.False(t, backupXorScheduleProvided(r))