From 38a2ebf175bfaaa39d17ad5be76c0fc3e6cafa3d Mon Sep 17 00:00:00 2001 From: Adam Zhang Date: Tue, 2 Jun 2026 09:42:57 +0800 Subject: [PATCH] Merge pull request #9840 from adam-jian-zhang/legacy_filters_incompatibility_validation validate incompatiblity with legacy filters Signed-off-by: Adam Zhang --- changelogs/unreleased/9840-adam-jian-zhang | 1 + pkg/controller/backup_controller.go | 7 + pkg/controller/backup_controller_test.go | 236 +++++++++++++++++++++ 3 files changed, 244 insertions(+) create mode 100644 changelogs/unreleased/9840-adam-jian-zhang diff --git a/changelogs/unreleased/9840-adam-jian-zhang b/changelogs/unreleased/9840-adam-jian-zhang new file mode 100644 index 000000000..8758c8251 --- /dev/null +++ b/changelogs/unreleased/9840-adam-jian-zhang @@ -0,0 +1 @@ +Fix issue #9812, validate ClusterScopedFilterPolicy and NamespacedFilterPolicy incompatible with legacy filters diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index d638fc460..7bb9ee5f9 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -603,6 +603,13 @@ func (b *backupReconciler) prepareBackupRequest(ctx context.Context, backup *vel request.Status.ValidationErrors = append(request.Status.ValidationErrors, "include-resources, exclude-resources and include-cluster-resources are old filter parameters.\n"+ "They cannot be used with include-exclude policies.") } + // namespacedFilterPolicies and clusterScopedFilterPolicy incompatible with old-style filters + if resourcePolicies != nil && + (len(resourcePolicies.GetNamespacedFilterPolicies()) > 0 || resourcePolicies.GetClusterScopedFilterPolicy() != nil) && + collections.UseOldResourceFilters(request.Spec) { + request.Status.ValidationErrors = append(request.Status.ValidationErrors, "include-resources, exclude-resources and include-cluster-resources are old filter parameters.\n"+ + "They cannot be used with namespace-scoped or fine-grained global filter policies.") + } request.ResPolicies = resourcePolicies return request } diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 9f45e829e..0ff76cf63 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -21,6 +21,7 @@ import ( "fmt" "io" "reflect" + "slices" "sort" "strings" "testing" @@ -33,6 +34,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + corev1api "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -2019,3 +2021,237 @@ func TestPatchResourceWorksWithStatus(t *testing.T) { }) } } + +// TestPrepareBackupRequest_NamespacedFilterPoliciesIncompatibleWithOldFilters verifies +// that a backup referencing a ResourcePolicy ConfigMap with namespacedFilterPolicies +// produces a validation error when old-style resource filters are also set on the spec. +func TestPrepareBackupRequest_NamespacedFilterPoliciesIncompatibleWithOldFilters(t *testing.T) { + formatFlag := logging.FormatText + logger := logging.DefaultLogger(logrus.DebugLevel, formatFlag) + + policyYAML := `version: v1 +namespacedFilterPolicies: +- namespaces: ["production"] + resourceFilters: + - kinds: ["Deployment"] + names: ["api-server"] +` + policyConfigMap := &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-filter-policy", + Namespace: velerov1api.DefaultNamespace, + }, + Data: map[string]string{"policy": policyYAML}, + } + + backup := defaultBackup().IncludedResources("deployments").Result() + backup.Spec.ResourcePolicy = &corev1api.TypedLocalObjectReference{ + Kind: "configmap", + Name: "my-filter-policy", + } + + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, policyConfigMap) + + apiServer := velerotest.NewAPIServer(t) + discoveryHelper, err := discovery.NewHelper(apiServer.DiscoveryClient, logger) + require.NoError(t, err) + + c := &backupReconciler{ + logger: logger, + discoveryHelper: discoveryHelper, + kbClient: fakeClient, + clock: &clock.RealClock{}, + formatFlag: formatFlag, + } + + res := c.prepareBackupRequest(ctx, backup, logger) + + require.NotEmpty(t, res.Status.ValidationErrors) + + hasTargetError := slices.ContainsFunc(res.Status.ValidationErrors, func(e string) bool { + return strings.Contains(e, "namespace-scoped or fine-grained global filter policies") + }) + + assert.True(t, hasTargetError, "expected validation error about namespacedFilterPolicies incompatibility with old-style filters, got: %v", res.Status.ValidationErrors) +} + +// TestPrepareBackupRequest_ClusterScopedFilterPolicyIncompatibleWithOldFilters verifies +// that a backup referencing a ResourcePolicy ConfigMap with clusterScopedFilterPolicy +// produces a validation error when old-style resource filters are also set on the spec. +func TestPrepareBackupRequest_ClusterScopedFilterPolicyIncompatibleWithOldFilters(t *testing.T) { + formatFlag := logging.FormatText + logger := logging.DefaultLogger(logrus.DebugLevel, formatFlag) + + policyYAML := `version: v1 +clusterScopedFilterPolicy: + resourceFilters: + - kinds: ["ClusterRole"] + names: ["my-app-*"] +` + policyConfigMap := &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster-filter-policy", + Namespace: velerov1api.DefaultNamespace, + }, + Data: map[string]string{"policy": policyYAML}, + } + + backup := defaultBackup().IncludedResources("clusterroles").Result() + backup.Spec.ResourcePolicy = &corev1api.TypedLocalObjectReference{ + Kind: "configmap", + Name: "my-cluster-filter-policy", + } + + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, policyConfigMap) + + apiServer := velerotest.NewAPIServer(t) + discoveryHelper, err := discovery.NewHelper(apiServer.DiscoveryClient, logger) + require.NoError(t, err) + + c := &backupReconciler{ + logger: logger, + discoveryHelper: discoveryHelper, + kbClient: fakeClient, + clock: &clock.RealClock{}, + formatFlag: formatFlag, + } + + res := c.prepareBackupRequest(ctx, backup, logger) + + require.NotEmpty(t, res.Status.ValidationErrors) + + hasClusterError := slices.ContainsFunc(res.Status.ValidationErrors, func(e string) bool { + return strings.Contains(e, "namespace-scoped or fine-grained global filter policies") + }) + + assert.True(t, hasClusterError, "expected validation error about clusterScopedFilterPolicy incompatibility with old-style filters, got: %v", res.Status.ValidationErrors) +} + +const ( + namespacedFilterPolicyYAML = `version: v1 +namespacedFilterPolicies: +- namespaces: ["production"] + resourceFilters: + - kinds: ["Deployment"] + names: ["api-server"] +` + clusterScopedFilterPolicyYAML = `version: v1 +clusterScopedFilterPolicy: + resourceFilters: + - kinds: ["ClusterRole"] + names: ["my-app-*"] +` + bothFilterPoliciesYAML = `version: v1 +namespacedFilterPolicies: +- namespaces: ["production"] + resourceFilters: + - kinds: ["Deployment"] + names: ["api-server"] +clusterScopedFilterPolicy: + resourceFilters: + - kinds: ["ClusterRole"] + names: ["my-app-*"] +` +) + +// TestPrepareBackupRequest_FilterPoliciesWithNewFilters verifies that backups referencing +// a ResourcePolicy ConfigMap with namespacedFilterPolicies and/or clusterScopedFilterPolicy +// succeed when old-style resource filters are not set on the spec. +func TestPrepareBackupRequest_FilterPoliciesWithNewFilters(t *testing.T) { + tests := []struct { + name string + policyYAML string + policyConfigMapName string + backup *velerov1api.Backup + expectNamespacedPolicies int + expectClusterScopedPolicy bool + }{ + { + name: "namespacedFilterPolicies only", + policyYAML: namespacedFilterPolicyYAML, + policyConfigMapName: "my-filter-policy", + backup: defaultBackup().StorageLocation("loc-1").Result(), + expectNamespacedPolicies: 1, + }, + { + name: "clusterScopedFilterPolicy only", + policyYAML: clusterScopedFilterPolicyYAML, + policyConfigMapName: "my-cluster-filter-policy", + backup: defaultBackup().StorageLocation("loc-1").Result(), + expectClusterScopedPolicy: true, + }, + { + name: "both filter policies", + policyYAML: bothFilterPoliciesYAML, + policyConfigMapName: "my-combined-filter-policy", + backup: defaultBackup().StorageLocation("loc-1").Result(), + expectNamespacedPolicies: 1, + expectClusterScopedPolicy: true, + }, + { + name: "with new-style spec filters", + policyYAML: bothFilterPoliciesYAML, + policyConfigMapName: "my-combined-filter-policy", + backup: defaultBackup(). + StorageLocation("loc-1"). + IncludedNamespaceScopedResources("deployments"). + IncludedClusterScopedResources("clusterroles"). + Result(), + expectNamespacedPolicies: 1, + expectClusterScopedPolicy: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + formatFlag := logging.FormatText + logger := logging.DefaultLogger(logrus.DebugLevel, formatFlag) + + policyConfigMap := &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: test.policyConfigMapName, + Namespace: velerov1api.DefaultNamespace, + }, + Data: map[string]string{"policy": test.policyYAML}, + } + + test.backup.Spec.ResourcePolicy = &corev1api.TypedLocalObjectReference{ + Kind: "configmap", + Name: test.policyConfigMapName, + } + + backupLocation := builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "loc-1"). + Phase(velerov1api.BackupStorageLocationPhaseAvailable).Result() + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, backupLocation, policyConfigMap) + + apiServer := velerotest.NewAPIServer(t) + discoveryHelper, err := discovery.NewHelper(apiServer.DiscoveryClient, logger) + require.NoError(t, err) + + c := &backupReconciler{ + logger: logger, + discoveryHelper: discoveryHelper, + kbClient: fakeClient, + clock: &clock.RealClock{}, + formatFlag: formatFlag, + } + + res := c.prepareBackupRequest(ctx, test.backup, logger) + defer res.WorkerPool.Stop() + + assert.Empty(t, res.Status.ValidationErrors) + hasIncompatibilityError := slices.ContainsFunc(res.Status.ValidationErrors, func(e string) bool { + return strings.Contains(e, "namespace-scoped or fine-grained global filter policies") + }) + assert.False(t, hasIncompatibilityError) + + require.NotNil(t, res.ResPolicies) + assert.Len(t, res.ResPolicies.GetNamespacedFilterPolicies(), test.expectNamespacedPolicies) + if test.expectClusterScopedPolicy { + assert.NotNil(t, res.ResPolicies.GetClusterScopedFilterPolicy()) + } else { + assert.Nil(t, res.ResPolicies.GetClusterScopedFilterPolicy()) + } + }) + } +}