feat: support GVK and namespace selector in config match exclusions#4401
feat: support GVK and namespace selector in config match exclusions#4401a7i wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds richer exclusion matching for Gatekeeper Config spec.match entries by introducing GVK and namespace label-selector based filtering, while keeping existing namespace-pattern behavior intact.
Changes:
- Extend
configv1alpha1.MatchEntrywithapiGroups,apiVersions,kinds, andnamespaceSelector. - Rework the process excluder to evaluate exclusions via new
IsObjectExcluded(process, obj, nsLabels)(withIsNamespaceExcludeddelegating for backward compatibility). - Expand unit tests to cover GVK filters, namespace selectors, AND/OR semantics, and
GetExcludedNamespacesbehavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/controller/config/process/excluder.go | Implements processed excluder entries and adds object-level exclusion matching with GVK + namespaceSelector support. |
| pkg/controller/config/process/excluder_test.go | Adds comprehensive tests for the new matching behavior and ensures backward compatibility. |
| apis/config/v1alpha1/config_types.go | Extends the Config CRD MatchEntry schema with new selector/GVK fields. |
| apis/config/v1alpha1/zz_generated.deepcopy.go | Updates deepcopy generation for the newly added MatchEntry fields. |
| func (s *Excluder) Equals(new *Excluder) bool { // nolint:revive | ||
| s.mux.RLock() | ||
| defer s.mux.RUnlock() | ||
| return reflect.DeepEqual(s.excludedNamespaces, new.excludedNamespaces) | ||
| return reflect.DeepEqual(s.entries, new.entries) | ||
| } | ||
|
|
||
| // EqualsForProcess checks if the excluded namespaces for a specific process are equal. | ||
| // EqualsForProcess checks if the entries for a specific process are equal. | ||
| func (s *Excluder) EqualsForProcess(process Process, new *Excluder) bool { // nolint:revive | ||
| s.mux.RLock() | ||
| defer s.mux.RUnlock() | ||
| return reflect.DeepEqual(s.excludedNamespaces[process], new.excludedNamespaces[process]) | ||
| return reflect.DeepEqual(s.entries[process], new.entries[process]) | ||
| } |
There was a problem hiding this comment.
Equals/EqualsForProcess now rely on reflect.DeepEqual over []excluderEntry, which is order-sensitive for both entries and excludedNamespaces. Since match semantics are OR-based across entries and OR-based across namespace patterns, reordering (or duplicating) config entries can now trigger Equals to return false even when behavior is unchanged, causing unnecessary downstream work (e.g., cache wipes). Consider normalizing entries when adding (dedupe/sort patterns, and possibly sort entries by a stable key) or comparing via semantic sets instead of slice order.
| // Check namespace label selector | ||
| if e.namespaceSelector != nil { | ||
| if nsLabels == nil { | ||
| // No namespace labels available; can't evaluate selector, so don't match | ||
| return false, nil | ||
| } | ||
|
|
||
| selector, err := metav1.LabelSelectorAsSelector(e.namespaceSelector) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if !selector.Matches(labels.Set(nsLabels)) { | ||
| return false, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
matches() calls metav1.LabelSelectorAsSelector every time an object is checked. This is on the admission hot-path (IsNamespaceExcluded is called per request) and also means an invalid selector can surface as runtime errors during admission. Consider compiling/validating the selector once when building the excluder entry in Add() (e.g., store a labels.Selector in the entry) and reusing it during matching.
| for _, e := range s.entries[process] { | ||
| if e.apiGroups != nil || e.apiVersions != nil || e.kinds != nil || e.namespaceSelector != nil { | ||
| continue | ||
| } | ||
| for _, ns := range e.excludedNamespaces { | ||
| excludedNamespaces = append(excludedNamespaces, string(ns)) |
There was a problem hiding this comment.
GetExcludedNamespaces() now appends namespace patterns from every matching entry without deduplicating. Previously, the underlying map guaranteed uniqueness, so callers (notably VAP generation) may now get redundant patterns and longer generated match conditions. Consider deduping before returning (e.g., use a map[string]struct{}/sets.String) while preserving deterministic output order if needed.
| for _, e := range s.entries[process] { | |
| if e.apiGroups != nil || e.apiVersions != nil || e.kinds != nil || e.namespaceSelector != nil { | |
| continue | |
| } | |
| for _, ns := range e.excludedNamespaces { | |
| excludedNamespaces = append(excludedNamespaces, string(ns)) | |
| seen := make(map[string]struct{}) | |
| for _, e := range s.entries[process] { | |
| if e.apiGroups != nil || e.apiVersions != nil || e.kinds != nil || e.namespaceSelector != nil { | |
| continue | |
| } | |
| for _, ns := range e.excludedNamespaces { | |
| nsStr := string(ns) | |
| if _, exists := seen[nsStr]; exists { | |
| continue | |
| } | |
| seen[nsStr] = struct{}{} | |
| excludedNamespaces = append(excludedNamespaces, nsStr) |
a7i
force-pushed
the
feat/config-match-namespace-selector
branch
2 times, most recently
from
beba35d to
826974c
Compare
Add APIGroups, APIVersions, Kinds, and NamespaceSelector fields to MatchEntry, enabling resource-level exclusion filtering via GVK and namespace label selectors in the Config resource. Refactor the process Excluder to evaluate entries with AND logic across all criteria and expose IsObjectExcluded for callers that can provide namespace labels. Namespace selectors are compiled once during Add() for efficient per-request matching. GetExcludedNamespaces deduplicates output. Equality comparison ignores compiled selectors, comparing only the original LabelSelector specs. Signed-off-by: Amir Alavi <amiralavi7@gmail.com>
a7i
force-pushed
the
feat/config-match-namespace-selector
branch
from
826974c to
f8ef97b
Compare
| @@ -52,6 +60,59 @@ spec: | |||
| pattern: ^\*?[-:a-z0-9]*\*?$ | |||
| type: string | |||
| type: array | |||
| kinds: | |||
| items: | |||
| type: string | |||
| type: array | |||
There was a problem hiding this comment.
The new CRD fields apiGroups, apiVersions, and kinds are missing descriptions. Consider adding descriptions similar to the other fields to improve API documentation and user experience. For example, apiGroups could describe: "List of API groups to match. An empty list or omitted field matches all groups."
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider adding a test case for the error handling when Add is called with an invalid namespace selector. This would verify that the error is properly returned and help prevent regressions. For example, a test with an invalid label selector operator or malformed selector would be beneficial.
| func TestAddInvalidNamespaceSelector(t *testing.T) { | |
| excluder := New() | |
| invalidSelector := &metav1.LabelSelector{ | |
| MatchExpressions: []metav1.LabelSelectorRequirement{ | |
| { | |
| Key: "foo", | |
| Operator: metav1.LabelSelectorOperator("InvalidOp"), | |
| Values: []string{"bar"}, | |
| }, | |
| }, | |
| } | |
| matchEntries := []configv1alpha1.MatchEntry{ | |
| { | |
| NamespaceSelector: invalidSelector, | |
| }, | |
| } | |
| if err := excluder.Add(matchEntries); err == nil { | |
| t.Fatalf("expected error from Add when using invalid namespace selector, got nil") | |
| } | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4401 +/- ##
===========================================
- Coverage 54.49% 44.08% -10.42%
===========================================
Files 134 282 +148
Lines 12329 20617 +8288
===========================================
+ Hits 6719 9088 +2369
- Misses 5116 10748 +5632
- Partials 494 781 +287
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions. |
What this PR does / why we need it:
Adds
apiGroups,apiVersions,kinds, andnamespaceSelectorfields to the ConfigMatchEntrytype, enabling fine-grained resource-level exclusion filtering.Currently, Config match exclusions can only filter by namespace name patterns (
excludedNamespaces). This PR extendsMatchEntryto support:apiGroups,apiVersions,kinds) — exclude only specific resource typesnamespaceSelector) — exclude resources in namespaces matching a label selectorAll criteria within a single entry are AND-ed (all must match). Multiple entries are OR-ed (any match triggers exclusion). Unspecified fields match everything, preserving full backward compatibility.
Example:
A new
IsObjectExcluded(process, obj, nsLabels)method is added for callers that can provide namespace labels. The existingIsNamespaceExcludedmethod continues to work unchanged and automatically supports namespace selector matching for Namespace objects.Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #
Special notes for your reviewer:
GetExcludedNamespacesintentionally skips entries with GVK or selector filters, since those entries don't unconditionally exclude a namespace (used by VAP generation).IsNamespaceExcluded(webhook, audit, cachemanager) are unchanged. WiringIsObjectExcludedwith namespace label lookups at call sites can be done as a follow-up.zz_generated.deepcopy.gowas updated manually to match the new fields. Amake generaterun should produce equivalent output.