From eb0659f06d99311a1d3361e36f2f936b5e447c09 Mon Sep 17 00:00:00 2001 From: Adam Zhang Date: Mon, 25 May 2026 10:28:27 +0800 Subject: [PATCH 1/2] Add validations for ClusterScopedFilterPolicy Added validations for ClusterScopedFilterPolicy to report errors for various invalid scenarios. Signed-off-by: Adam Zhang --- changelogs/unreleased/9847-adam-jian-zhang | 1 + .../resourcepolicies/resource_policies.go | 36 ++++++ .../resource_policies_test.go | 108 ++++++++++++++++++ 3 files changed, 145 insertions(+) create mode 100644 changelogs/unreleased/9847-adam-jian-zhang 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 232633484..c0a697f22 100644 --- a/internal/resourcepolicies/resource_policies.go +++ b/internal/resourcepolicies/resource_policies.go @@ -262,6 +262,7 @@ func (p *Policies) Validate() error { } if err := p.validateNamespacedFilterPolicies(); err != nil { + if err := p.validateClusterScopedFilterPolicy(); err != nil { return errors.WithStack(err) } @@ -409,6 +410,41 @@ func (p *Policies) validateNamespacedFilterPolicies() error { return fmt.Errorf( "namespacedFilterPolicies: duplicate namespace pattern '%s' found in policies %v", pattern, policyIndices) +func (p *Policies) validateClusterScopedFilterPolicy() error { + if p.clusterScopedFilterPolicy == nil { + return nil + } + + if len(p.clusterScopedFilterPolicy.ResourceFilters) == 0 { + return fmt.Errorf("clusterScopedFilterPolicy: at least one resourceFilter must be specified") + } + + 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) + } } } diff --git a/internal/resourcepolicies/resource_policies_test.go b/internal/resourcepolicies/resource_policies_test.go index 898c6d1ca..8cd8955a9 100644 --- a/internal/resourcepolicies/resource_policies_test.go +++ b/internal/resourcepolicies/resource_policies_test.go @@ -1244,6 +1244,7 @@ func TestPVCPhaseMatch(t *testing.T) { } func TestNamespacedFilterPolicies(t *testing.T) { +func TestClusterScopedFilterPolicies(t *testing.T) { testCases := []struct { name string yamlData string @@ -1303,6 +1304,49 @@ namespacedFilterPolicies: yamlData: `version: v1 namespacedFilterPolicies: - namespaces: ["test"] + 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: "at least one resourceFilter must be specified", @@ -1420,6 +1464,49 @@ namespacedFilterPolicies: app: web orLabelSelectors: - env: prod`, + 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", }, @@ -1430,6 +1517,11 @@ namespacedFilterPolicies: - namespaces: ["test"] resourceFilters: - kinds: ["Pod"] + name: "invalid - bad glob in names", + yamlData: `version: v1 +clusterScopedFilterPolicy: + resourceFilters: + - kinds: ["ClusterRole"] names: ["[invalid"]`, wantErr: true, errMsg: "invalid glob pattern", @@ -1446,6 +1538,14 @@ namespacedFilterPolicies: - kinds: ["ConfigMap"]`, wantErr: true, errMsg: "duplicate namespace pattern", + name: "invalid - bad glob in excludedNames", + yamlData: `version: v1 +clusterScopedFilterPolicy: + resourceFilters: + - kinds: ["ClusterRole"] + excludedNames: ["[bad"]`, + wantErr: true, + errMsg: "invalid glob pattern", }, } @@ -1457,6 +1557,11 @@ namespacedFilterPolicies: policies := &Policies{} err = policies.BuildPolicy(resPolicies) require.NoError(t, err) // BuildPolicy should always succeed for our test cases + require.NoError(t, err) + + policies := &Policies{} + err = policies.BuildPolicy(resPolicies) + require.NoError(t, err) err = policies.Validate() if tc.wantErr { @@ -1470,6 +1575,9 @@ namespacedFilterPolicies: // Verify that we can retrieve the policies nfPolicies := policies.GetNamespacedFilterPolicies() assert.GreaterOrEqual(t, len(nfPolicies), 1) // Valid test cases have at least 1 policy + assert.Contains(t, err.Error(), tc.errMsg) + } else { + require.NoError(t, err) } }) } From ca0506daa8bd259ebfe421101420bf2be75cda98 Mon Sep 17 00:00:00 2001 From: Adam Zhang Date: Wed, 3 Jun 2026 23:06:45 +0800 Subject: [PATCH 2/2] address review comments improve wording on validation errors for empty resourceFilters Signed-off-by: Adam Zhang --- .../resourcepolicies/resource_policies.go | 13 +- .../resource_policies_test.go | 252 ++++++++++-------- 2 files changed, 155 insertions(+), 110 deletions(-) diff --git a/internal/resourcepolicies/resource_policies.go b/internal/resourcepolicies/resource_policies.go index c0a697f22..1eec8c8e2 100644 --- a/internal/resourcepolicies/resource_policies.go +++ b/internal/resourcepolicies/resource_policies.go @@ -261,11 +261,14 @@ func (p *Policies) Validate() error { } } - if err := p.validateNamespacedFilterPolicies(); err != nil { if err := p.validateClusterScopedFilterPolicy(); err != nil { return errors.WithStack(err) } + if err := p.validateNamespacedFilterPolicies(); err != nil { + return errors.WithStack(err) + } + return nil } @@ -410,13 +413,19 @@ func (p *Policies) validateNamespacedFilterPolicies() error { 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: at least one resourceFilter must be specified") + return fmt.Errorf("clusterScopedFilterPolicy: resourceFilters cannot be empty; remove the policy block entirely if it is not needed") } seenKinds := make(map[string]int) diff --git a/internal/resourcepolicies/resource_policies_test.go b/internal/resourcepolicies/resource_policies_test.go index 8cd8955a9..e5736a0e8 100644 --- a/internal/resourcepolicies/resource_policies_test.go +++ b/internal/resourcepolicies/resource_policies_test.go @@ -1244,7 +1244,6 @@ func TestPVCPhaseMatch(t *testing.T) { } func TestNamespacedFilterPolicies(t *testing.T) { -func TestClusterScopedFilterPolicies(t *testing.T) { testCases := []struct { name string yamlData string @@ -1304,49 +1303,6 @@ namespacedFilterPolicies: yamlData: `version: v1 namespacedFilterPolicies: - namespaces: ["test"] - 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: "at least one resourceFilter must be specified", @@ -1464,49 +1420,6 @@ namespacedFilterPolicies: app: web orLabelSelectors: - env: prod`, - 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", }, @@ -1517,11 +1430,6 @@ namespacedFilterPolicies: - namespaces: ["test"] resourceFilters: - kinds: ["Pod"] - name: "invalid - bad glob in names", - yamlData: `version: v1 -clusterScopedFilterPolicy: - resourceFilters: - - kinds: ["ClusterRole"] names: ["[invalid"]`, wantErr: true, errMsg: "invalid glob pattern", @@ -1538,14 +1446,6 @@ namespacedFilterPolicies: - kinds: ["ConfigMap"]`, wantErr: true, errMsg: "duplicate namespace pattern", - name: "invalid - bad glob in excludedNames", - yamlData: `version: v1 -clusterScopedFilterPolicy: - resourceFilters: - - kinds: ["ClusterRole"] - excludedNames: ["[bad"]`, - wantErr: true, - errMsg: "invalid glob pattern", }, } @@ -1557,11 +1457,6 @@ clusterScopedFilterPolicy: policies := &Policies{} err = policies.BuildPolicy(resPolicies) require.NoError(t, err) // BuildPolicy should always succeed for our test cases - require.NoError(t, err) - - policies := &Policies{} - err = policies.BuildPolicy(resPolicies) - require.NoError(t, err) err = policies.Validate() if tc.wantErr { @@ -1575,9 +1470,6 @@ clusterScopedFilterPolicy: // Verify that we can retrieve the policies nfPolicies := policies.GetNamespacedFilterPolicies() assert.GreaterOrEqual(t, len(nfPolicies), 1) // Valid test cases have at least 1 policy - assert.Contains(t, err.Error(), tc.errMsg) - } else { - require.NoError(t, err) } }) } @@ -1644,3 +1536,147 @@ namespacedFilterPolicies: 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) + } + }) + } +}