diff --git a/changelogs/unreleased/9847-adam-jian-zhang b/changelogs/unreleased/9847-adam-jian-zhang new file mode 100644 index 000000000..82de39ca0 --- /dev/null +++ b/changelogs/unreleased/9847-adam-jian-zhang @@ -0,0 +1 @@ +Fix issue #9813, add validations for ClusterScopedFilterPolicy diff --git a/internal/resourcepolicies/resource_policies.go b/internal/resourcepolicies/resource_policies.go index 6a1d69576..9d7ed25f6 100644 --- a/internal/resourcepolicies/resource_policies.go +++ b/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 +} diff --git a/internal/resourcepolicies/resource_policies_test.go b/internal/resourcepolicies/resource_policies_test.go index 06b1bea1c..e5736a0e8 100644 --- a/internal/resourcepolicies/resource_policies_test.go +++ b/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) + } + }) + } +}