From a6f800c591b5c7bae9a0263aba5c782679e80d8e Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Tue, 28 Jul 2026 10:44:31 -0700 Subject: [PATCH] Address review feedback on default resource modifier - Fix fallthrough bug: when ResourceModifier is set with a non-ConfigMap kind, do not fall through to applying the server default. The outer check on ResourceModifier != nil now prevents default application regardless of the Kind value. - Include underlying error in fatal validation message for ConfigMap retrieval failures. - Strengthen exclusive precedence test: default ConfigMap intentionally does not exist while per-restore does, proving the default is never consulted. - Add test for invalid default ConfigMap data (non-fatal, warn and proceed). Signed-off-by: Shubham Pampattiwar --- pkg/controller/restore_controller.go | 12 ++++++----- pkg/controller/restore_controller_test.go | 25 +++++++++++++++++------ 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 48c57413e..daf5cbf0c 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -435,10 +435,12 @@ 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) { - resourceModifiers = r.loadResourceModifierConfigMap(ctx, restore, restore.Spec.ResourceModifier.Name, false) - if resourceModifiers == nil && len(restore.Status.ValidationErrors) > 0 { - return backupInfo{}, nil, nil + if restore.Spec.ResourceModifier != nil { + if strings.EqualFold(restore.Spec.ResourceModifier.Kind, resourcemodifiers.ConfigmapRefType) { + resourceModifiers = r.loadResourceModifierConfigMap(ctx, restore, restore.Spec.ResourceModifier.Name, false) + if resourceModifiers == nil && len(restore.Status.ValidationErrors) > 0 { + return backupInfo{}, nil, nil + } } } else if r.defaultResourceModifierConfigMap != "" && !boolptr.IsSetToTrue(restore.Spec.SkipDefaultResourceModifier) { resourceModifiers = r.loadResourceModifierConfigMap(ctx, restore, r.defaultResourceModifierConfigMap, true) @@ -460,7 +462,7 @@ func (r *restoreReconciler) loadResourceModifierConfigMap( return nil } restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, - fmt.Sprintf("failed to get resource modifiers configmap %s/%s", restore.Namespace, cmName)) + fmt.Sprintf("failed to get resource modifiers configmap %s/%s: %v", restore.Namespace, cmName, err)) return nil } diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index ab33b5b0e..26c607efe 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -1189,12 +1189,10 @@ func TestValidateAndCompleteWithDefaultResourceModifier(t *testing.T) { 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, - })) + t.Run("per-restore modifier takes exclusive precedence over default", func(t *testing.T) { + // Default ConfigMap does NOT exist, but per-restore does. + // If default were applied, it would fail. Per-restore should succeed. + r := setupReconciler(t, "nonexistent-default") require.NoError(t, r.kbClient.Create(t.Context(), &corev1api.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: "per-restore-rm", Namespace: velerov1api.DefaultNamespace}, Data: validCMData, @@ -1220,6 +1218,21 @@ func TestValidateAndCompleteWithDefaultResourceModifier(t *testing.T) { assert.Empty(t, restore.Status.ValidationErrors) }) + t.Run("default modifier with invalid data is non-fatal", func(t *testing.T) { + r := setupReconciler(t, "invalid-default") + require.NoError(t, r.kbClient.Create(t.Context(), &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "invalid-default", Namespace: velerov1api.DefaultNamespace}, + Data: map[string]string{ + "modifiers.yaml": "not-valid-yaml: [", + }, + })) + + restore := newRestore("", nil) + _, 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")