Feature/cel prefilter#759
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a prefilter subsystem (types, parsing, and runtime filtering), wired it into rule creation/bindings and RuleManager to skip CEL evaluation per-rule, added metrics/reporting and a Prometheus counter, plus noop metrics backend and tests/benchmarks; minor syscall and testdata tweaks. Changes
Sequence DiagramsequenceDiagram
participant Binding as Rule Binding Manager
participant Cache as Rule Cache
participant Prefilter as Prefilter Package
participant RuleMgr as Rule Manager
participant Metrics as Metrics Manager
Binding->>Cache: createRule(bindingRule)
Cache->>Prefilter: ParseWithDefaults(rule.State, binding.Params)
Prefilter-->>Cache: *Params (Ignore/Include prefixes, Ports, Dir, Methods)
Cache->>Cache: rule.Prefilter = Params
Cache-->>Binding: Rule with Prefilter
rect rgba(100, 150, 200, 0.5)
RuleMgr->>RuleMgr: extractEventFields(event)
RuleMgr->>RuleMgr: iterate enabled rules
alt rule.Prefilter != nil
RuleMgr->>Prefilter: ShouldSkip(extractedFields)
Prefilter-->>RuleMgr: bool (skip)
alt skip == true
RuleMgr->>Metrics: ReportRulePrefiltered(rule.Name)
RuleMgr-->>RuleMgr: continue (skip CEL)
else
RuleMgr->>RuleMgr: run CEL evaluation
end
else rule.Prefilter == nil
RuleMgr->>RuleMgr: run CEL evaluation
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
YakirOren
force-pushed
the
feature/cel-prefilter
branch
from
fde54c5 to
9c33ec6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/metricsmanager/metrics_manager_mock.go (1)
63-63: Track prefiltered calls in the mock for testability.
ReportRulePrefilteredis currently a no-op, so tests can’t assert this new behavior path.♻️ Proposed mock enhancement
type MetricsMock struct { FailedEventCounter atomic.Int32 RuleProcessedCounter maps.SafeMap[string, int] + RulePrefilteredCounter maps.SafeMap[string, int] RuleAlertCounter maps.SafeMap[string, int] EventCounter maps.SafeMap[utils.EventType, int] RuleEvaluationTime maps.SafeMap[string, time.Duration] // key: "ruleID:eventType" } -func (m *MetricsMock) ReportRulePrefiltered(ruleName string) {} +func (m *MetricsMock) ReportRulePrefiltered(ruleName string) { + m.RulePrefilteredCounter.Set(ruleName, m.RulePrefilteredCounter.Get(ruleName)+1) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/metricsmanager/metrics_manager_mock.go` at line 63, The MetricsMock.ReportRulePrefiltered method is a no-op so tests cannot observe prefiltered events; update the MetricsMock struct to track calls (e.g., add an exported field like PrefilteredCalls map[string]int or Prefiltered []string plus a sync.Mutex for concurrency safety) and implement ReportRulePrefiltered to record/increment the entry for the provided ruleName; use the new exported field in tests to assert the mock received the prefiltered report.pkg/rulebindingmanager/cache/cache_test.go (1)
1045-1086: Expand this test to cover all changedcreateRulebranches.Current cases validate only the
RuleIDpath. Please add cases forRuleNameandRuleTagsso all new hydration branches are covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulebindingmanager/cache/cache_test.go` around lines 1045 - 1086, Extend TestCreateRulePrefilter to exercise the other createRule hydration branches: add test cases where the binding uses RuleName (set RuntimeAlertRuleBindingRule.RuleName to a string) and where it uses RuleTags (set RuntimeAlertRuleBindingRule.RuleTags to a slice of strings) in addition to the existing RuleID case; for each new case call NewCacheMock("").createRule(binding), assert you get one rule and then validate its Prefilter fields (IgnorePrefixes/IncludePrefixes) behave the same as the RuleID case when Parameters are present and that Prefilter is nil when Parameters are absent—this will cover the RuleName and RuleTags code paths in createRule.pkg/rulemanager/prefilter/prefilter.go (1)
243-249:ensureTrailingSlashmutates input slice in place.This function modifies the input slice directly. While
toStringSlicecreates a copy before calling this function, ifensureTrailingSlashis ever called directly on a caller-owned slice, it would cause unexpected mutation. Consider returning a new slice for safety.♻️ Safer implementation that creates a copy
func ensureTrailingSlash(prefixes []string) []string { + result := make([]string, len(prefixes)) for i, p := range prefixes { if !strings.HasSuffix(p, "/") { - prefixes[i] = p + "/" + result[i] = p + "/" + } else { + result[i] = p } } - return prefixes + return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/prefilter/prefilter.go` around lines 243 - 249, ensureTrailingSlash currently mutates its input slice in place; change it to allocate and return a new slice instead to avoid unexpected mutations (e.g., when callers pass a caller-owned slice). In the implementation of ensureTrailingSlash, create a new slice of the same length, copy or transform each element from the input into the new slice adding a trailing "/" when needed, and return the new slice; update callers (e.g., where toStringSlice currently copies) to use the returned slice but no caller-side mutation should be required.pkg/rulemanager/prefilter/prefilter_test.go (1)
66-80: Consider usingassert.Equaldirectly on struct for cleaner assertions.The field-by-field comparison works, but comparing the full struct would be more concise and catch any future fields that get added.
♻️ Suggested simplification
t.Run(tt.name, func(t *testing.T) { got := ParseWithDefaults(tt.ruleState, tt.bindingParams) if tt.expect == nil { assert.Nil(t, got) } else { require.NotNil(t, got) - assert.Equal(t, tt.expect.IgnorePrefixes, got.IgnorePrefixes) - assert.Equal(t, tt.expect.IncludePrefixes, got.IncludePrefixes) - assert.Equal(t, tt.expect.AllowedPorts, got.AllowedPorts) - assert.Equal(t, tt.expect.Dir, got.Dir) - assert.Equal(t, tt.expect.MethodMask, got.MethodMask) + assert.Equal(t, tt.expect, got) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/prefilter/prefilter_test.go` around lines 66 - 80, The test currently compares fields of the ParseWithDefaults result one-by-one; replace that with a single struct equality assertion to be more concise and future-proof: inside the loop in prefiler_test.go, keep the nil-check branch (if tt.expect == nil { assert.Nil(t, got) }) but in the non-nil branch remove require.NotNil and the per-field asserts and call assert.Equal(t, tt.expect, got) to compare the whole struct returned by ParseWithDefaults (variables: got and tt.expect).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/rulemanager/prefilter/prefilter.go`:
- Around line 185-187: The check using p.AllowedPorts currently causes events to
be skipped when the destination port is present (slices.Contains(p.AllowedPorts,
e.DstPort)), which conflicts with the "Allowed" name; either rename AllowedPorts
to SkipPorts/IgnorePorts (and update usages and comments around the prefilter
logic where e.PortEligible and e.DstPort are evaluated) or invert the condition
so the prefilter only skips when the port is NOT in the allow-list (i.e., change
the logical test around p.AllowedPorts in the block referencing e.PortEligible,
p.AllowedPorts and e.DstPort to match the chosen semantics and update the
variable name/comment accordingly).
In `@pkg/rulemanager/rule_manager.go`:
- Around line 224-236: The prefilter is being skipped for non-K8s rules and also
runs before event-type filtering, causing overcounting; update the logic in the
rule evaluation loop so you first call rm.getRuleExpressions(rule, eventType)
and skip rules with zero expressions, then perform prefiltering only if a
prefilter exists or, if nil, attempt to hydrate the rule from the RBCache via
the same path used by CreateAllRules (e.g., call the rule-hydration method used
by RBCache.createRule) to populate rule.Prefilter before calling
rule.Prefilter.ShouldSkip(eventFields); keep using
extractEventFields(enrichedEvent.Event) when eventFields.Extracted is false and
continue to call rm.metrics.ReportRulePrefiltered(rule.Name) only when a
prefilter actually caused the skip.
---
Nitpick comments:
In `@pkg/metricsmanager/metrics_manager_mock.go`:
- Line 63: The MetricsMock.ReportRulePrefiltered method is a no-op so tests
cannot observe prefiltered events; update the MetricsMock struct to track calls
(e.g., add an exported field like PrefilteredCalls map[string]int or Prefiltered
[]string plus a sync.Mutex for concurrency safety) and implement
ReportRulePrefiltered to record/increment the entry for the provided ruleName;
use the new exported field in tests to assert the mock received the prefiltered
report.
In `@pkg/rulebindingmanager/cache/cache_test.go`:
- Around line 1045-1086: Extend TestCreateRulePrefilter to exercise the other
createRule hydration branches: add test cases where the binding uses RuleName
(set RuntimeAlertRuleBindingRule.RuleName to a string) and where it uses
RuleTags (set RuntimeAlertRuleBindingRule.RuleTags to a slice of strings) in
addition to the existing RuleID case; for each new case call
NewCacheMock("").createRule(binding), assert you get one rule and then validate
its Prefilter fields (IgnorePrefixes/IncludePrefixes) behave the same as the
RuleID case when Parameters are present and that Prefilter is nil when
Parameters are absent—this will cover the RuleName and RuleTags code paths in
createRule.
In `@pkg/rulemanager/prefilter/prefilter_test.go`:
- Around line 66-80: The test currently compares fields of the ParseWithDefaults
result one-by-one; replace that with a single struct equality assertion to be
more concise and future-proof: inside the loop in prefiler_test.go, keep the
nil-check branch (if tt.expect == nil { assert.Nil(t, got) }) but in the non-nil
branch remove require.NotNil and the per-field asserts and call assert.Equal(t,
tt.expect, got) to compare the whole struct returned by ParseWithDefaults
(variables: got and tt.expect).
In `@pkg/rulemanager/prefilter/prefilter.go`:
- Around line 243-249: ensureTrailingSlash currently mutates its input slice in
place; change it to allocate and return a new slice instead to avoid unexpected
mutations (e.g., when callers pass a caller-owned slice). In the implementation
of ensureTrailingSlash, create a new slice of the same length, copy or transform
each element from the input into the new slice adding a trailing "/" when
needed, and return the new slice; update callers (e.g., where toStringSlice
currently copies) to use the returned slice but no caller-side mutation should
be required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f148052b-06e5-4673-8799-d2877da86e4c
📒 Files selected for processing (12)
cmd/main.gopkg/metricsmanager/metrics_manager_interface.gopkg/metricsmanager/metrics_manager_mock.gopkg/metricsmanager/metrics_manager_noop.gopkg/metricsmanager/prometheus/prometheus.gopkg/rulebindingmanager/cache/cache.gopkg/rulebindingmanager/cache/cache_test.gopkg/rulemanager/extract_event_fields_test.gopkg/rulemanager/prefilter/prefilter.gopkg/rulemanager/prefilter/prefilter_test.gopkg/rulemanager/rule_manager.gopkg/rulemanager/types/v1/types.go
YakirOren
force-pushed
the
feature/cel-prefilter
branch
from
bed06bb to
9451661
Compare
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
…s first Signed-off-by: Yakir Oren <yakiroren@gmail.com>
YakirOren
force-pushed
the
feature/cel-prefilter
branch
from
9451661 to
266a39c
Compare
matthyx
requested changes
Apr 5, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/rulemanager/prefilter/prefilter.go`:
- Around line 223-241: The toUint16Slice function accepts only float64 from
[]interface{} and silently truncates/omits inputs; update it to strictly parse
integers and support broader numeric types: in toUint16Slice iterate vals :=
v.([]interface{}) and for each item handle types int, int8/16/32/64,
uint/uint8/16/32/64, float64 (only accept when value is whole number via
math.Trunc == value), and json.Number (use Int64 parsing), also optionally
handle numeric strings via strconv.ParseInt; validate each parsed value is
between 0 and 65535 and if any item is non-integer or out of range return
nil,false (do not silently drop), otherwise append uint16(parsed) to result and
return result,true; keep the existing []uint16 branch that copies and returns
len>0.
In `@pkg/rulemanager/rulecreator/factory.go`:
- Around line 96-99: The loop is mutating a range-copy `rule` so setting
`rule.Prefilter` never updates the slice element and causes repeated re-parsing;
change the loop to hydrate the Prefilter in-place by obtaining a pointer or
index into the slice (e.g. use &r.Rules[i] or r.Rules[i] directly) and assign
Prefilter on that element (call prefilter.ParseWithDefaults(rule.State, nil) and
store it into the slice element), ensuring the persisted `r.Rules` entries have
their Prefilter populated so downstream code (e.g. the non-K8s path in
rule_manager.go) sees the hydrated value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e78abae-34c2-418e-9e64-27bb909c19b1
📒 Files selected for processing (15)
cmd/main.gopkg/containerwatcher/v2/tracers/httpparse.gopkg/metricsmanager/metrics_manager_interface.gopkg/metricsmanager/metrics_manager_mock.gopkg/metricsmanager/metrics_manager_noop.gopkg/metricsmanager/prometheus/prometheus.gopkg/rulebindingmanager/cache/cache.gopkg/rulebindingmanager/cache/cache_test.gopkg/rulebindingmanager/testdata/ruleparamsfiles/rule-ssh-params.yamlpkg/rulemanager/extract_event_fields_test.gopkg/rulemanager/prefilter/prefilter.gopkg/rulemanager/prefilter/prefilter_test.gopkg/rulemanager/rule_manager.gopkg/rulemanager/rulecreator/factory.gopkg/rulemanager/types/v1/types.go
✅ Files skipped from review due to trivial changes (6)
- pkg/rulebindingmanager/testdata/ruleparamsfiles/rule-ssh-params.yaml
- pkg/containerwatcher/v2/tracers/httpparse.go
- pkg/rulebindingmanager/cache/cache_test.go
- pkg/rulebindingmanager/cache/cache.go
- pkg/rulemanager/extract_event_fields_test.go
- pkg/metricsmanager/metrics_manager_noop.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/metricsmanager/metrics_manager_mock.go
- pkg/rulemanager/types/v1/types.go
- pkg/metricsmanager/metrics_manager_interface.go
- pkg/metricsmanager/prometheus/prometheus.go
- pkg/rulemanager/prefilter/prefilter_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/rulemanager/prefilter/prefilter.go (1)
85-92: Normalize event input case in setters to avoid silent filter bypasses.Line 85 and Line 90 currently rely on caller casing. Making normalization local keeps behavior consistent with
ParseWithDefaultsand avoids accidentalDirNone/MethodBit=0on mixed-case inputs.♻️ Proposed change
func (f *EventFields) SetDirection(s string) { - f.Dir = parseDirection(s) + f.Dir = parseDirection(strings.ToLower(s)) } @@ func (f *EventFields) SetMethod(method string) { - f.MethodBit = methodToBit(method) + f.MethodBit = methodToBit(strings.ToUpper(method)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/prefilter/prefilter.go` around lines 85 - 92, SetDirection and SetMethod currently pass caller-provided casing to parseDirection and methodToBit which can yield DirNone/MethodBit=0 for mixed-case inputs; update SetDirection to normalize the direction string (e.g., strings.ToLower) before calling parseDirection, and update SetMethod to normalize the HTTP method (e.g., strings.ToUpper) before calling methodToBit so behavior matches ParseWithDefaults and avoids silent filter bypasses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/rulemanager/prefilter/prefilter.go`:
- Around line 211-212: The port allow-list check in prefilter.go currently uses
e.PortEligible alone and thus excludes HTTP events that were marked
PortEligible; restrict this filter to only apply to real network/SSH events by
adding an explicit event-type check (e.g., verify the event kind/category is
network or SSH and not HTTP) before consulting p.Ports and e.DstPort; update the
condition that references e.PortEligible, p.Ports and e.DstPort to require the
event be of the network/SSH type (use the event kind/type field or helper like
IsHTTP/IsNetwork if available) so HTTP rule evaluation is not skipped.
---
Nitpick comments:
In `@pkg/rulemanager/prefilter/prefilter.go`:
- Around line 85-92: SetDirection and SetMethod currently pass caller-provided
casing to parseDirection and methodToBit which can yield DirNone/MethodBit=0 for
mixed-case inputs; update SetDirection to normalize the direction string (e.g.,
strings.ToLower) before calling parseDirection, and update SetMethod to
normalize the HTTP method (e.g., strings.ToUpper) before calling methodToBit so
behavior matches ParseWithDefaults and avoids silent filter bypasses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c3d1eec-dbf7-45d1-b5fc-84566c2ca761
📒 Files selected for processing (2)
pkg/rulemanager/prefilter/prefilter.gopkg/rulemanager/prefilter/prefilter_test.go
matthyx
approved these changes
Apr 13, 2026
…tion Signed-off-by: Yakir Oren <yakiroren@gmail.com>
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
cache_test Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> Signed-off-by: Yakir Oren <yakiroren@gmail.com>
YakirOren
force-pushed
the
feature/cel-prefilter
branch
from
7d11c2b to
fe77947
Compare
matthyx
approved these changes
Apr 13, 2026
slashben
approved these changes
Apr 13, 2026
This was referenced Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Metrics & Observability
Behavior Changes
Tests