Merge pull request #9847 from adam-jian-zhang/cluster-scoped-filter-policy-validation

Add validations for ClusterScopedFilterPolicy

Signed-off-by: Adam Zhang <adam.zhang@broadcom.com>

changelogs/unreleased/9847-adam-jian-zhang

@@ -0,0 +1 @@
+Fix issue #9813, add validations for ClusterScopedFilterPolicy

internal/resourcepolicies/resource_policies.go

@@ -24,11 +24,13 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	"github.com/cockroachdb/errors"
+	"github.com/gobwas/glob"
 	"github.com/sirupsen/logrus"
 	corev1api "k8s.io/api/core/v1"
 	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	"github.com/vmware-tanzu/velero/pkg/util/wildcard"
 )
 
 type VolumeActionType string
@@ -259,6 +261,14 @@ func (p *Policies) Validate() error {
 		}
 	}
 
+	if err := p.validateClusterScopedFilterPolicy(); err != nil {
+		return errors.WithStack(err)
+	}
+
+	if err := p.validateNamespacedFilterPolicies(); err != nil {
+		return errors.WithStack(err)
+	}
+
 	return nil
 }
 
@@ -335,3 +345,117 @@ func getResourcePoliciesFromConfig(cm *corev1api.ConfigMap) (*Policies, error) {
 
 	return policies, nil
 }
+
+func (p *Policies) validateNamespacedFilterPolicies() error {
+	seenPatterns := make(map[string][]int) // pattern -> list of policy indices
+
+	// Rule 1-7: Basic validation rules
+	for i, nfp := range p.namespacedFilterPolicies {
+		if len(nfp.Namespaces) == 0 {
+			return fmt.Errorf("namespacedFilterPolicies[%d]: at least one namespace must be specified", i)
+		}
+		if len(nfp.ResourceFilters) == 0 {
+			return fmt.Errorf("namespacedFilterPolicies[%d]: at least one resourceFilter must be specified", i)
+		}
+
+		// Rule 8 & 9: Validate glob patterns and collect namespace patterns for duplicate check
+		for j, pattern := range nfp.Namespaces {
+			if err := wildcard.ValidateNamespaceName(pattern); err != nil {
+				return fmt.Errorf("namespacedFilterPolicies[%d].namespaces[%d]: %w", i, j, err)
+			}
+			seenPatterns[pattern] = append(seenPatterns[pattern], i)
+		}
+
+		seenKinds := make(map[string]int)
+		hasCatchAll := false
+		for j, rf := range nfp.ResourceFilters {
+			if rf.IsCatchAll() {
+				if hasCatchAll {
+					return fmt.Errorf("namespacedFilterPolicies[%d]: only one catch-all resource filter is allowed", i)
+				}
+				hasCatchAll = true
+				if len(rf.Names) > 0 || len(rf.ExcludedNames) > 0 {
+					return fmt.Errorf("namespacedFilterPolicies[%d].resourceFilters[%d]: names or excludedNames cannot be specified for catch-all filters", i, j)
+				}
+			}
+
+			for _, kind := range rf.Kinds {
+				if kind == "*" {
+					continue // "*" is handled by IsCatchAll, no need to check duplicates against other kinds
+				}
+				if prevJ, ok := seenKinds[kind]; ok {
+					return fmt.Errorf("namespacedFilterPolicies[%d]: kind %q appears in both resourceFilters[%d] and resourceFilters[%d]", i, kind, prevJ, j)
+				}
+				seenKinds[kind] = j
+			}
+
+			if len(rf.LabelSelector) > 0 && len(rf.OrLabelSelectors) > 0 {
+				return fmt.Errorf("namespacedFilterPolicies[%d].resourceFilters[%d]: labelSelector and orLabelSelectors cannot co-exist", i, j)
+			}
+
+			// Validate glob patterns for names and excludedNames using gobwas/glob
+			for k, pattern := range rf.Names {
+				if _, err := glob.Compile(pattern); err != nil {
+					return fmt.Errorf("namespacedFilterPolicies[%d].resourceFilters[%d].names[%d]: invalid glob pattern %q: %v", i, j, k, pattern, err)
+				}
+			}
+			for k, pattern := range rf.ExcludedNames {
+				if _, err := glob.Compile(pattern); err != nil {
+					return fmt.Errorf("namespacedFilterPolicies[%d].resourceFilters[%d].excludedNames[%d]: invalid glob pattern %q: %v", i, j, k, pattern, err)
+				}
+			}
+		}
+	}
+
+	// Rule 8: Report exact duplicates only
+	for pattern, policyIndices := range seenPatterns {
+		if len(policyIndices) > 1 {
+			return fmt.Errorf(
+				"namespacedFilterPolicies: duplicate namespace pattern '%s' found in policies %v",
+				pattern, policyIndices)
+		}
+	}
+
+	return nil
+}
+
+func (p *Policies) validateClusterScopedFilterPolicy() error {
+	if p.clusterScopedFilterPolicy == nil {
+		return nil
+	}
+
+	if len(p.clusterScopedFilterPolicy.ResourceFilters) == 0 {
+		return fmt.Errorf("clusterScopedFilterPolicy: resourceFilters cannot be empty; remove the policy block entirely if it is not needed")
+	}
+
+	seenKinds := make(map[string]int)
+	for j, rf := range p.clusterScopedFilterPolicy.ResourceFilters {
+		if rf.IsCatchAll() {
+			return fmt.Errorf("clusterScopedFilterPolicy.resourceFilters[%d]: kinds must be specified (catch-all is not supported)", j)
+		}
+
+		for _, kind := range rf.Kinds {
+			if prevJ, ok := seenKinds[kind]; ok {
+				return fmt.Errorf("clusterScopedFilterPolicy: kind %q appears in both resourceFilters[%d] and resourceFilters[%d]", kind, prevJ, j)
+			}
+			seenKinds[kind] = j
+		}
+
+		if len(rf.LabelSelector) > 0 && len(rf.OrLabelSelectors) > 0 {
+			return fmt.Errorf("clusterScopedFilterPolicy.resourceFilters[%d]: labelSelector and orLabelSelectors cannot co-exist", j)
+		}
+
+		for k, pattern := range rf.Names {
+			if _, err := glob.Compile(pattern); err != nil {
+				return fmt.Errorf("clusterScopedFilterPolicy.resourceFilters[%d].names[%d]: invalid glob pattern %q: %v", j, k, pattern, err)
+			}
+		}
+		for k, pattern := range rf.ExcludedNames {
+			if _, err := glob.Compile(pattern); err != nil {
+				return fmt.Errorf("clusterScopedFilterPolicy.resourceFilters[%d].excludedNames[%d]: invalid glob pattern %q: %v", j, k, pattern, err)
+			}
+		}
+	}
+
+	return nil
+}

internal/resourcepolicies/resource_policies_test.go

@@ -1242,3 +1242,441 @@ func TestPVCPhaseMatch(t *testing.T) {
 		})
 	}
 }
+
+func TestNamespacedFilterPolicies(t *testing.T) {
+	testCases := []struct {
+		name     string
+		yamlData string
+		wantErr  bool
+		errMsg   string
+	}{
+		{
+			name: "valid namespacedFilterPolicies with multiple kinds",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["frontend", "backend"]
+  resourceFilters:
+  - kinds: ["Pod", "ConfigMap"]
+    labelSelector:
+      app: web
+    names: ["app-*"]
+  - kinds: ["Secret"]
+    excludedNames: ["temp-*"]`,
+			wantErr: false,
+		},
+		{
+			name: "valid namespacedFilterPolicies with glob patterns",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["team-*"]
+  resourceFilters:
+  - kinds: ["Pod"]
+    orLabelSelectors:
+    - env: prod
+    - env: staging`,
+			wantErr: false,
+		},
+		{
+			name: "valid - overlapping patterns allowed (first-match semantics)",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["team-frontend-*"]
+  resourceFilters:
+  - kinds: ["Pod", "ConfigMap", "Secret"]
+- namespaces: ["team-*"]
+  resourceFilters:
+  - kinds: ["Deployment", "Service"]`,
+			wantErr: false,
+		},
+		{
+			name: "invalid - no namespaces",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: []
+  resourceFilters:
+  - kinds: ["Pod"]`,
+			wantErr: true,
+			errMsg:  "at least one namespace must be specified",
+		},
+		{
+			name: "invalid - no resourceFilters",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["test"]
+  resourceFilters: []`,
+			wantErr: true,
+			errMsg:  "at least one resourceFilter must be specified",
+		},
+		{
+			name: "valid - asterisk catch-all",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["test"]
+  resourceFilters:
+  - kinds: ["*"]
+    labelSelector:
+      app: web`,
+			wantErr: false,
+		},
+		{
+			name: "invalid - multiple asterisk kinds",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["test"]
+  resourceFilters:
+  - kinds: ["*"]
+    labelSelector:
+      app: web
+  - kinds: ["*"]
+    labelSelector:
+      app: db`,
+			wantErr: true,
+			errMsg:  "only one catch-all resource filter is allowed",
+		},
+		{
+			name: "invalid - empty and asterisk kinds",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["test"]
+  resourceFilters:
+  - kinds: []
+    labelSelector:
+      app: web
+  - kinds: ["*"]
+    labelSelector:
+      app: db`,
+			wantErr: true,
+			errMsg:  "only one catch-all resource filter is allowed",
+		},
+		{
+			name: "invalid - multiple empty kinds",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["test"]
+  resourceFilters:
+  - kinds: []
+    labelSelector:
+      app: web
+  - kinds: []
+    labelSelector:
+      app: db`,
+			wantErr: true,
+			errMsg:  "only one catch-all resource filter is allowed",
+		},
+		{
+			name: "invalid - names with empty kinds",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["test"]
+  resourceFilters:
+  - kinds: []
+    names: ["app-*"]
+    labelSelector:
+      app: web`,
+			wantErr: true,
+			errMsg:  "names or excludedNames cannot be specified for catch-all filters",
+		},
+		{
+			name: "invalid - excludedNames with empty kinds",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["test"]
+  resourceFilters:
+  - kinds: []
+    excludedNames: ["app-*"]
+    labelSelector:
+      app: web`,
+			wantErr: true,
+			errMsg:  "names or excludedNames cannot be specified for catch-all filters",
+		},
+		{
+			name: "valid - no label selectors with catch-all",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["test"]
+  resourceFilters:
+  - kinds: ["*"]`,
+			wantErr: false,
+		},
+		{
+			name: "invalid - duplicate kinds",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["test"]
+  resourceFilters:
+  - kinds: ["Pod"]
+  - kinds: ["Pod", "ConfigMap"]`,
+			wantErr: true,
+			errMsg:  "kind \"Pod\" appears in both resourceFilters",
+		},
+		{
+			name: "invalid - both labelSelector and orLabelSelectors",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["test"]
+  resourceFilters:
+  - kinds: ["Pod"]
+    labelSelector:
+      app: web
+    orLabelSelectors:
+    - env: prod`,
+			wantErr: true,
+			errMsg:  "labelSelector and orLabelSelectors cannot co-exist",
+		},
+		{
+			name: "invalid - bad glob pattern in names",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["test"]
+  resourceFilters:
+  - kinds: ["Pod"]
+    names: ["[invalid"]`,
+			wantErr: true,
+			errMsg:  "invalid glob pattern",
+		},
+		{
+			name: "invalid - duplicate namespace pattern",
+			yamlData: `version: v1
+namespacedFilterPolicies:
+- namespaces: ["production"]
+  resourceFilters:
+  - kinds: ["Pod"]
+- namespaces: ["production"]
+  resourceFilters:
+  - kinds: ["ConfigMap"]`,
+			wantErr: true,
+			errMsg:  "duplicate namespace pattern",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			resPolicies, err := unmarshalResourcePolicies(&tc.yamlData)
+			require.NoError(t, err) // Unmarshal should always succeed for our test cases
+
+			policies := &Policies{}
+			err = policies.BuildPolicy(resPolicies)
+			require.NoError(t, err) // BuildPolicy should always succeed for our test cases
+
+			err = policies.Validate()
+			if tc.wantErr {
+				require.Error(t, err)
+				if tc.errMsg != "" {
+					assert.Contains(t, err.Error(), tc.errMsg)
+				}
+			} else {
+				require.NoError(t, err)
+
+				// Verify that we can retrieve the policies
+				nfPolicies := policies.GetNamespacedFilterPolicies()
+				assert.GreaterOrEqual(t, len(nfPolicies), 1) // Valid test cases have at least 1 policy
+			}
+		})
+	}
+}
+
+func TestNamespacedFilterPoliciesAccessor(t *testing.T) {
+	yamlData := `version: v1
+namespacedFilterPolicies:
+- namespaces: ["frontend"]
+  resourceFilters:
+  - kinds: ["Pod"]
+    labelSelector:
+      app: web`
+
+	resPolicies, err := unmarshalResourcePolicies(&yamlData)
+	require.NoError(t, err)
+
+	policies := &Policies{}
+	err = policies.BuildPolicy(resPolicies)
+	require.NoError(t, err)
+
+	nfPolicies := policies.GetNamespacedFilterPolicies()
+	require.Len(t, nfPolicies, 1)
+
+	policy := nfPolicies[0]
+	assert.Equal(t, []string{"frontend"}, policy.Namespaces)
+	assert.Len(t, policy.ResourceFilters, 1)
+
+	rf := policy.ResourceFilters[0]
+	assert.Equal(t, []string{"Pod"}, rf.Kinds)
+	assert.Equal(t, map[string]string{"app": "web"}, rf.LabelSelector)
+}
+
+func TestFirstMatchSemantics(t *testing.T) {
+	yamlData := `version: v1
+namespacedFilterPolicies:
+- namespaces: ["team-frontend-*", "specific-ns"]
+  resourceFilters:
+  - kinds: ["Pod", "ConfigMap", "Secret"]
+- namespaces: ["team-*", "another-pattern"] 
+  resourceFilters:
+  - kinds: ["Deployment", "Service"]`
+
+	resPolicies, err := unmarshalResourcePolicies(&yamlData)
+	require.NoError(t, err)
+
+	policies := &Policies{}
+	err = policies.BuildPolicy(resPolicies)
+	require.NoError(t, err)
+
+	err = policies.Validate()
+	require.NoError(t, err)
+
+	nfPolicies := policies.GetNamespacedFilterPolicies()
+	require.Len(t, nfPolicies, 2)
+
+	// Verify the first policy has the more specific patterns
+	policy1 := nfPolicies[0]
+	assert.Equal(t, []string{"team-frontend-*", "specific-ns"}, policy1.Namespaces)
+	assert.Equal(t, []string{"Pod", "ConfigMap", "Secret"}, policy1.ResourceFilters[0].Kinds)
+
+	// Verify the second policy has the broader patterns
+	policy2 := nfPolicies[1]
+	assert.Equal(t, []string{"team-*", "another-pattern"}, policy2.Namespaces)
+	assert.Equal(t, []string{"Deployment", "Service"}, policy2.ResourceFilters[0].Kinds)
+}
+
+func TestClusterScopedFilterPolicies(t *testing.T) {
+	testCases := []struct {
+		name     string
+		yamlData string
+		wantErr  bool
+		errMsg   string
+	}{
+		{
+			name: "valid - single kind with names",
+			yamlData: `version: v1
+clusterScopedFilterPolicy:
+  resourceFilters:
+  - kinds: ["ClusterRole"]
+    names: ["my-app-*"]`,
+			wantErr: false,
+		},
+		{
+			name: "valid - multi-kind with labelSelector",
+			yamlData: `version: v1
+clusterScopedFilterPolicy:
+  resourceFilters:
+  - kinds: ["ClusterRole", "ClusterRoleBinding"]
+    labelSelector:
+      app: my-app`,
+			wantErr: false,
+		},
+		{
+			name: "valid - orLabelSelectors",
+			yamlData: `version: v1
+clusterScopedFilterPolicy:
+  resourceFilters:
+  - kinds: ["CustomResourceDefinition"]
+    orLabelSelectors:
+    - app: my-app
+    - app: other-app`,
+			wantErr: false,
+		},
+		{
+			name: "valid - excludedNames",
+			yamlData: `version: v1
+clusterScopedFilterPolicy:
+  resourceFilters:
+  - kinds: ["ClusterRole"]
+    names: ["my-*"]
+    excludedNames: ["my-debug-*"]`,
+			wantErr: false,
+		},
+		{
+			name: "invalid - empty resourceFilters",
+			yamlData: `version: v1
+clusterScopedFilterPolicy:
+  resourceFilters: []`,
+			wantErr: true,
+			errMsg:  "resourceFilters cannot be empty; remove the policy block entirely if it is not needed",
+		},
+		{
+			name: "invalid - empty kinds in clusterScopedFilterPolicy",
+			yamlData: `version: v1
+clusterScopedFilterPolicy:
+  resourceFilters:
+  - kinds: []
+    names: ["my-app-*"]`,
+			wantErr: true,
+			errMsg:  "kinds must be specified",
+		},
+		{
+			name: "invalid - asterisk kinds (explicit catch-all) in clusterScopedFilterPolicy",
+			yamlData: `version: v1
+clusterScopedFilterPolicy:
+  resourceFilters:
+  - kinds: ["*"]
+    labelSelector:
+      app: my-app`,
+			wantErr: true,
+			errMsg:  "kinds must be specified",
+		},
+		{
+			name: "invalid - duplicate kinds across entries",
+			yamlData: `version: v1
+clusterScopedFilterPolicy:
+  resourceFilters:
+  - kinds: ["ClusterRole"]
+    names: ["my-app-*"]
+  - kinds: ["ClusterRole"]
+    labelSelector:
+      app: other`,
+			wantErr: true,
+			errMsg:  `kind "ClusterRole" appears in both`,
+		},
+		{
+			name: "invalid - labelSelector and orLabelSelectors co-exist",
+			yamlData: `version: v1
+clusterScopedFilterPolicy:
+  resourceFilters:
+  - kinds: ["ClusterRole"]
+    labelSelector:
+      app: my-app
+    orLabelSelectors:
+    - app: other`,
+			wantErr: true,
+			errMsg:  "labelSelector and orLabelSelectors cannot co-exist",
+		},
+		{
+			name: "invalid - bad glob in names",
+			yamlData: `version: v1
+clusterScopedFilterPolicy:
+  resourceFilters:
+  - kinds: ["ClusterRole"]
+    names: ["[invalid"]`,
+			wantErr: true,
+			errMsg:  "invalid glob pattern",
+		},
+		{
+			name: "invalid - bad glob in excludedNames",
+			yamlData: `version: v1
+clusterScopedFilterPolicy:
+  resourceFilters:
+  - kinds: ["ClusterRole"]
+    excludedNames: ["[bad"]`,
+			wantErr: true,
+			errMsg:  "invalid glob pattern",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			resPolicies, err := unmarshalResourcePolicies(&tc.yamlData)
+			require.NoError(t, err)
+
+			policies := &Policies{}
+			err = policies.BuildPolicy(resPolicies)
+			require.NoError(t, err)
+
+			err = policies.Validate()
+			if tc.wantErr {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tc.errMsg)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}