Add validations for NamespacedFilterPolicies

Add validation rules for NamespacedFilterPolicies to fail fast for invalid NamespacedFilterPolicies. Signed-off-by: Adam Zhang <adam.zhang@broadcom.com>
2026-08-15 19:56:06 +00:00 · 2026-05-25 11:08:40 +08:00
parent 40025fbbe1
commit b278d38f7e
3 changed files with 399 additions and 0 deletions
@@ -0,0 +1 @@
+Fix issue #9814, add validations for NamespacedFilterPolicies
@@ -23,12 +23,14 @@ import (

 	"k8s.io/apimachinery/pkg/util/sets"

+	"github.com/gobwas/glob"
 	"github.com/pkg/errors"
 	"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,10 @@ func (p *Policies) Validate() error {
 		}
 	}

+	if err := p.validateNamespacedFilterPolicies(); err != nil {
+		return errors.WithStack(err)
+	}
+
 	return nil
 }

@@ -335,3 +341,101 @@ func getResourcePoliciesFromConfig(cm *corev1api.ConfigMap) (*Policies, error) {

 	return policies, nil
 }
+
+func (p *Policies) validateNamespacedFilterPolicies() error {
+	// 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)
+		}
+
+		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: Validate no duplicate namespace patterns across filter policies
+	if err := p.validateNoDuplicateNamespacePatterns(); err != nil {
+		return err
+	}
+
+	// Rule 9: Validate glob patterns for namespace names
+	if err := p.validateGlobPatterns(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (p *Policies) validateNoDuplicateNamespacePatterns() error {
+	seenPatterns := make(map[string][]int) // pattern -> list of policy indices
+
+	for i, policy := range p.namespacedFilterPolicies {
+		for _, pattern := range policy.Namespaces {
+			seenPatterns[pattern] = append(seenPatterns[pattern], i)
+		}
+	}
+
+	// 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) validateGlobPatterns() error {
+	for i, policy := range p.namespacedFilterPolicies {
+		for j, pattern := range policy.Namespaces {
+			// Use existing Velero wildcard validation for namespace patterns
+			if err := wildcard.ValidateNamespaceName(pattern); err != nil {
+				return fmt.Errorf(
+					"namespacedFilterPolicies[%d].namespaces[%d]: %w",
+					i, j, err)
+			}
+		}
+	}
+	return nil
+}
@@ -1242,3 +1242,297 @@ 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)
+}
				`@@ -0,0 +1 @@`
				`Fix issue #9814, add validations for NamespacedFilterPolicies`