Open
Conversation
📝 WalkthroughWalkthroughA prefilter subsystem was added and integrated: new prefilter package and types, wiring into rule creation and bindings, per-event extraction and early rule skipping in the rule manager with metric reporting and a Prometheus counter, a MetricsNoop implementation, tests/benchmarks, and minor syscall/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 docstrings
🧪 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
| case utils.HTTPEventType: | ||
| if e, ok := event.(utils.HttpEvent); ok { | ||
| f.SetDirection(string(e.GetDirection())) | ||
| f.DstPort = e.GetDstPort() |
Contributor
There was a problem hiding this comment.
HTTPEventType sets DstPort but leaves PortEligible = false, so a ports: parameter has no effect on HTTP events
| } | ||
|
|
||
| if p.Dir != DirNone && e.Dir != DirNone && e.Dir != p.Dir { | ||
| return true |
Contributor
There was a problem hiding this comment.
Suggested change
| return true | |
| return true // pass through if event field is indeterminate |
| } | ||
|
|
||
| if p.MethodMask != 0 && e.MethodBit != 0 && p.MethodMask&e.MethodBit == 0 { | ||
| return true |
Contributor
There was a problem hiding this comment.
Suggested change
| return true | |
| return true // pass through if event field is indeterminate |
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
Comment on lines
+223
to
+241
| func toUint16Slice(v interface{}) ([]uint16, bool) { | ||
| if v == nil { | ||
| return nil, false | ||
| } | ||
| switch vals := v.(type) { | ||
| case []interface{}: | ||
| result := make([]uint16, 0, len(vals)) | ||
| for _, item := range vals { | ||
| if f, ok := item.(float64); ok && f >= 0 && f <= 65535 { | ||
| result = append(result, uint16(f)) | ||
| } | ||
| } | ||
| return result, len(result) > 0 | ||
| case []uint16: | ||
| cp := make([]uint16, len(vals)) | ||
| copy(cp, vals) | ||
| return cp, len(vals) > 0 | ||
| } | ||
| return nil, false |
There was a problem hiding this comment.
ports parsing is too narrow and can silently mis-handle values.
toUint16Slice only accepts float64 in []interface{} and truncates decimals via cast. This can drop valid integer-typed inputs or misinterpret non-integer values.
🐛 Suggested fix (strict integer parsing + broader numeric support)
import (
+ "math"
"slices"
"strings"
)
@@
func toUint16Slice(v interface{}) ([]uint16, bool) {
if v == nil {
return nil, false
}
switch vals := v.(type) {
case []interface{}:
result := make([]uint16, 0, len(vals))
for _, item := range vals {
- if f, ok := item.(float64); ok && f >= 0 && f <= 65535 {
- result = append(result, uint16(f))
+ if p, ok := toUint16(item); ok {
+ result = append(result, p)
}
}
return result, len(result) > 0
+ case []int:
+ result := make([]uint16, 0, len(vals))
+ for _, n := range vals {
+ if n >= 0 && n <= 65535 {
+ result = append(result, uint16(n))
+ }
+ }
+ return result, len(result) > 0
case []uint16:
cp := make([]uint16, len(vals))
copy(cp, vals)
return cp, len(vals) > 0
}
return nil, false
}
+
+func toUint16(v interface{}) (uint16, bool) {
+ switch n := v.(type) {
+ case int:
+ if n >= 0 && n <= 65535 {
+ return uint16(n), true
+ }
+ case int64:
+ if n >= 0 && n <= 65535 {
+ return uint16(n), true
+ }
+ case float64:
+ if n >= 0 && n <= 65535 && math.Trunc(n) == n {
+ return uint16(n), true
+ }
+ }
+ return 0, false
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func toUint16Slice(v interface{}) ([]uint16, bool) { | |
| if v == nil { | |
| return nil, false | |
| } | |
| switch vals := v.(type) { | |
| case []interface{}: | |
| result := make([]uint16, 0, len(vals)) | |
| for _, item := range vals { | |
| if f, ok := item.(float64); ok && f >= 0 && f <= 65535 { | |
| result = append(result, uint16(f)) | |
| } | |
| } | |
| return result, len(result) > 0 | |
| case []uint16: | |
| cp := make([]uint16, len(vals)) | |
| copy(cp, vals) | |
| return cp, len(vals) > 0 | |
| } | |
| return nil, false | |
| import ( | |
| "math" | |
| "slices" | |
| "strings" | |
| ) | |
| func toUint16Slice(v interface{}) ([]uint16, bool) { | |
| if v == nil { | |
| return nil, false | |
| } | |
| switch vals := v.(type) { | |
| case []interface{}: | |
| result := make([]uint16, 0, len(vals)) | |
| for _, item := range vals { | |
| if p, ok := toUint16(item); ok { | |
| result = append(result, p) | |
| } | |
| } | |
| return result, len(result) > 0 | |
| case []int: | |
| result := make([]uint16, 0, len(vals)) | |
| for _, n := range vals { | |
| if n >= 0 && n <= 65535 { | |
| result = append(result, uint16(n)) | |
| } | |
| } | |
| return result, len(result) > 0 | |
| case []uint16: | |
| cp := make([]uint16, len(vals)) | |
| copy(cp, vals) | |
| return cp, len(vals) > 0 | |
| } | |
| return nil, false | |
| } | |
| func toUint16(v interface{}) (uint16, bool) { | |
| switch n := v.(type) { | |
| case int: | |
| if n >= 0 && n <= 65535 { | |
| return uint16(n), true | |
| } | |
| case int64: | |
| if n >= 0 && n <= 65535 { | |
| return uint16(n), true | |
| } | |
| case float64: | |
| if n >= 0 && n <= 65535 && math.Trunc(n) == n { | |
| return uint16(n), true | |
| } | |
| } | |
| return 0, false | |
| } |
🤖 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 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.
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
Comment on lines
+211
to
+212
| if e.PortEligible && len(p.Ports) > 0 && !slices.Contains(p.Ports, e.DstPort) { | ||
| return true |
There was a problem hiding this comment.
Port allow-list currently affects HTTP events in the runtime path.
At Line 211, port filtering applies whenever e.PortEligible is true. In pkg/rulemanager/rule_manager.go (Lines 481 and 489-494 from the provided snippet), HTTP events are marked PortEligible = true, so HTTP events are skipped when their destination port is not in Params.Ports. That conflicts with the contract documented at Line 100 (SSH, network) and can suppress expected HTTP rule evaluation.
🤖 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 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.
Contributor
There was a problem hiding this comment.
OMG, I didn't think about that, maybe we should revert, or add a separate discriminator so port filtering only gates SSH/network event types.
There was a problem hiding this comment.
🧩 Analysis chain
Let me check the actual extractEventFields implementation to confirm the issue and propose the right fix.
🏁 Script executed:
#!/bin/bash
# Look for extractEventFields in rule_manager.go
rg -n "extractEventFields|PortEligible" --type=go -A5 -B2
Length of output: 7550
@matthyx Good news — no revert or architectural discriminator is needed. The fix is a single-line removal.
The bug is exclusively in extractEventFields in pkg/rulemanager/rule_manager.go (line 481). The ShouldSkip logic in prefilter.go and its contract (PortEligible bool // true for SSH/network events) are already correct. The HTTP case incorrectly sets PortEligible = true alongside the other fields:
case utils.HttpEventType:
if e, ok := event.(utils.HttpEvent); ok {
f.SetDirection(string(e.GetDirection()))
f.DstPort = e.GetDstPort()
f.PortEligible = true // ← remove this line
if req := e.GetRequest(); req != nil {
f.SetMethod(req.Method)
}
}Removing that one line means HTTP events will have PortEligible: false, and the port allow-list in ShouldSkip will never gate them — which is exactly what the prefilter test at line 134 already validates:
{"HTTP port not filtered by ports", &Params{Ports: []uint16{22}}, EventFields{DstPort: 22, PortEligible: false}, false},So the test suite was already written with the correct expectation; the implementation just diverged from it.
🧠 Learnings used
Learnt from: slashben
Repo: kubescape/node-agent PR: 753
File: pkg/sbomscanner/v1/server.go:62-64
Timestamp: 2026-03-23T09:40:51.594Z
Learning: When reviewing Go code that uses Syft’s `CreateSBOMConfig` builder methods (e.g., `WithCatalogers`, `WithCatalogerSelection`, `WithFilesConfig`, etc.), note they use pointer receivers and mutate the `CreateSBOMConfig` in place, then return the same `*CreateSBOMConfig` for chaining. Therefore, expressions like `cfg.WithCatalogers(...)` are functionally correct even if the return value is not captured, as long as `cfg` is a non-nil `*CreateSBOMConfig` (e.g., from `DefaultCreateSBOMConfig()`). Don’t require assignment purely to “preserve” builder return values for this specific Syft type.
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
2 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