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) } }) }