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:
📝 WalkthroughWalkthroughIntroduce a pooled EventActivation evaluation context and context-aware CEL APIs; replace map-based eval contexts with pooled activations; register a pooled HttpEventWrapper type; add parse.basename and tests; precompute rule expressions by event type and use a single evalContext per event. Changes
Sequence Diagram(s)sequenceDiagram
participant RM as RuleManager
participant EV as EnrichedEvent
participant EC as EventActivation
participant CEL as CEL Evaluator
participant PC as ProgramCache
RM->>EV: receive event
RM->>EC: CreateEvalContext(event)
RM->>RM: lookup expressions = rule.ExpressionsByEventType[eventType]
RM->>CEL: EvaluateRuleWithContext(EC, expressions)
CEL->>PC: compile or fetch cached program
PC-->>CEL: program
CEL->>EC: evaluate program against EventActivation
CEL-->>RM: result / error
RM->>EC: Release()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
changed the title
YakirOren
force-pushed
the
optimize/http-event-creation
branch
from
4cca818 to
9a69a13
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/rulemanager/rulecreator/factory.go (1)
71-80:⚠️ Potential issue | 🟠 MajorPre-existing bug:
CreateRulePolicyRulesByEventTypeappends to the slice it's iterating over.This isn't part of the current PR changes, but this method iterates over
rulesand conditionally appends torulesin the same loop (line 75), which will cause duplicate entries and potentially an infinite loop (in Go,rangeevaluates the length once, so it won't be infinite, but duplicates will still be added).Proposed fix
func (r *RuleCreatorImpl) CreateRulePolicyRulesByEventType(eventType utils.EventType) []typesv1.Rule { - rules := r.CreateRulesByEventType(eventType) - for _, rule := range rules { + allRules := r.CreateRulesByEventType(eventType) + var rules []typesv1.Rule + for _, rule := range allRules { if rule.SupportPolicy { rules = append(rules, rule) } } - return rules }
🧹 Nitpick comments (4)
pkg/rulemanager/cel/libraries/parse/parse.go (1)
31-41: Consider usingpath.Basefrom the standard library.The stdlib
path.Basehandles additional edge cases (e.g., trailing slashes, empty strings,"."paths) and is well-tested. Your current implementation returns""for a trailing-slash input like"/usr/bin/", whereaspath.Basewould return"bin".If the empty-string-on-trailing-slash behavior is intentional for your CEL rules, this is fine as-is — just worth documenting that choice. Otherwise,
path.Baseis a drop-in replacement here.♻️ If stdlib behavior is acceptable
-import ( - "strings" - - "github.com/google/cel-go/common/types" - "github.com/google/cel-go/common/types/ref" - "github.com/kubescape/node-agent/pkg/rulemanager/cel/libraries/celparse" -) +import ( + "path" + + "github.com/google/cel-go/common/types" + "github.com/google/cel-go/common/types/ref" + "github.com/kubescape/node-agent/pkg/rulemanager/cel/libraries/celparse" +)func (l *parseLibrary) basename(path ref.Val) ref.Val { s, ok := path.Value().(string) if !ok { return types.MaybeNoSuchOverloadErr(path) } - idx := strings.LastIndex(s, "/") - if idx == -1 { - return types.String(s) - } - return types.String(s[idx+1:]) + return types.String(path.Base(s)) }Note: there would be a name collision between the
pathparameter and thepathimport — rename the parameter (e.g.,val) if you go this route.pkg/rulemanager/cel/libraries/parse/parsing_test.go (1)
45-64: Consider adding a test for the empty-string edge case.An empty string input (
parse.basename('')) would exercise theLastIndex == -1branch and return"". It's a plausible real-world input (e.g., unset field) worth covering.pkg/rulemanager/rule_manager.go (1)
373-386:getUniqueIdAndMessage: only the last error is returned.If the message evaluation (line 374) fails and uniqueID evaluation (line 378) succeeds,
erris overwritten toniland the message error is silently lost. The function would return an empty message with no error indication. This appears to be a pre-existing pattern, but now that both calls use the same eval context, it's worth noting.Optional: propagate the first error
func (rm *RuleManager) getUniqueIdAndMessage(evalContext map[string]any, rule typesv1.Rule) (string, string, error) { message, err := rm.celEvaluator.EvaluateExpressionWithContext(evalContext, rule.Expressions.Message) if err != nil { logger.L().Error("RuleManager - failed to evaluate message", helpers.Error(err)) + return "", "", fmt.Errorf("failed to evaluate message: %w", err) } uniqueID, err := rm.celEvaluator.EvaluateExpressionWithContext(evalContext, rule.Expressions.UniqueID) if err != nil { logger.L().Error("RuleManager - failed to evaluate unique ID", helpers.Error(err)) + return "", "", fmt.Errorf("failed to evaluate unique ID: %w", err) }pkg/rulemanager/types/v1/types.go (1)
36-48: Good design: pre-indexed expressions avoid per-event scanning.The
json:"-" yaml:"-"tags and the pointer-receiverInit()are correct. One subtle point: ifInit()is ever not called,ExpressionsByEventTyperemainsnil. In Go, indexing anilmap returns the zero value (empty slice) without panicking, so rules would be silently skipped rather than producing an error. The factory currently callsInit()in all mutation paths (RegisterRule, SyncRules, UpdateRule), ensuring all rules in circulation are initialized. However,CreateRuleByIDandCreateRuleByNamecan return an empty Rule{} on failed lookup; consider adding a defensive nil-check in the consumer (rule_manager.go) to guard against future code paths.Optional: lazy-init guard in the lookup
In
rule_manager.gowhere you look up expressions:+ if rule.ExpressionsByEventType == nil { + logger.L().Warning("Rule not initialized, skipping", helpers.String("rule", rule.ID)) + continue + } ruleExpressions := rule.ExpressionsByEventType[eventType] if len(ruleExpressions) == 0 {
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
did you run a Bench to see the improvements? maybe we should commit it too...
YakirOren
force-pushed
the
optimize/http-event-creation
branch
from
f65bd00 to
03d2b02
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/rulemanager/cel/cel.go (1)
132-154:⚠️ Potential issue | 🟠 MajorUnchecked type assertion and discarded errors in
CreateEvalContext.Two concerns in this now-public method:
- Line 140:
event.(utils.CelEvent)will panic if the event doesn't implementCelEventand no converter is registered. Since this is now a public API (previously private), the risk surface is wider.- Lines 138, 140: Both
xcel.NewObjectreturn values discard the error. If object creation fails,objwill be a zero value and downstream evaluation will produce confusing errors.Consider using the comma-ok pattern for the type assertion and at least logging or returning an error from
CreateEvalContext(which would require a signature change), or adding a defensive nil-check onobj.Suggested defensive fix (minimal, no signature change)
if converter, exists := c.eventConverters[eventType]; exists { - obj, _ = xcel.NewObject(converter(event)) + var err error + obj, err = xcel.NewObject(converter(event)) + if err != nil { + logger.L().Warning("CEL: failed to create object from converted event", helpers.Error(err)) + } } else { - obj, _ = xcel.NewObject(event.(utils.CelEvent)) + celEvent, ok := event.(utils.CelEvent) + if !ok { + logger.L().Warning("CEL: event does not implement CelEvent", helpers.String("eventType", string(eventType))) + return map[string]any{"eventType": string(eventType)} + } + var err error + obj, err = xcel.NewObject(celEvent) + if err != nil { + logger.L().Warning("CEL: failed to create object", helpers.Error(err)) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/cel/cel.go` around lines 132 - 154, The CreateEvalContext function currently panics on a failed type assertion and discards errors from xcel.NewObject; change it to use the comma-ok form when asserting event.(utils.CelEvent) and capture the error returned by xcel.NewObject in both branches. If the type assertion fails and no converter exists, fall back to calling xcel.NewObject(event) (or set obj to event) but handle the returned error: log it via the CEL receiver's logger (e.g., c.logger) or set obj to a safe fallback (nil or the raw event) instead of ignoring the error; ensure you also guard the "http" assignment by checking obj != nil. Optionally, for a stricter fix, change CreateEvalContext signature to return (map[string]any, error) and propagate xcel.NewObject errors upward. Use symbol references CreateEvalContext, c.eventConverters, xcel.NewObject, utils.CelEvent, and the "http" evalContext key to locate and update the code.
🧹 Nitpick comments (1)
pkg/rulemanager/rule_manager.go (1)
373-386:getUniqueIdAndMessageonly returns the error from theUniqueIDevaluation, silently dropping themessageevaluation error.Line 374 evaluates the message expression and logs the error, but
erris then overwritten at line 378 by the uniqueID evaluation. If the message evaluation fails but the uniqueID evaluation succeeds,errwill beniland the caller won't know the message failed. This appears to be a pre-existing issue, but now is a good time to address it.Suggested fix
func (rm *RuleManager) getUniqueIdAndMessage(evalContext map[string]any, rule typesv1.Rule) (string, string, error) { message, err := rm.celEvaluator.EvaluateExpressionWithContext(evalContext, rule.Expressions.Message) if err != nil { logger.L().Error("RuleManager - failed to evaluate message", helpers.Error(err)) + return "", "", err } uniqueID, err := rm.celEvaluator.EvaluateExpressionWithContext(evalContext, rule.Expressions.UniqueID) if err != nil { logger.L().Error("RuleManager - failed to evaluate unique ID", helpers.Error(err)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/rule_manager.go` around lines 373 - 386, getUniqueIdAndMessage currently overwrites the error from evaluating the message with the error from evaluating the UniqueID, potentially returning nil when the message evaluation failed; update getUniqueIdAndMessage to capture both evaluation errors from rm.celEvaluator.EvaluateExpressionWithContext (for rule.Expressions.Message and rule.Expressions.UniqueID), log each failure as you already do, and return a non-nil error if either evaluation failed (e.g., return the first non-nil error or wrap both errors together) while still hashing uniqueID via hashStringToMD5 before returning; ensure the returned err reflects any failure from evaluating message or uniqueID so callers of getUniqueIdAndMessage see the correct error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/rulemanager/cel/cel.go`:
- Around line 132-154: The CreateEvalContext function currently panics on a
failed type assertion and discards errors from xcel.NewObject; change it to use
the comma-ok form when asserting event.(utils.CelEvent) and capture the error
returned by xcel.NewObject in both branches. If the type assertion fails and no
converter exists, fall back to calling xcel.NewObject(event) (or set obj to
event) but handle the returned error: log it via the CEL receiver's logger
(e.g., c.logger) or set obj to a safe fallback (nil or the raw event) instead of
ignoring the error; ensure you also guard the "http" assignment by checking obj
!= nil. Optionally, for a stricter fix, change CreateEvalContext signature to
return (map[string]any, error) and propagate xcel.NewObject errors upward. Use
symbol references CreateEvalContext, c.eventConverters, xcel.NewObject,
utils.CelEvent, and the "http" evalContext key to locate and update the code.
---
Nitpick comments:
In `@pkg/rulemanager/rule_manager.go`:
- Around line 373-386: getUniqueIdAndMessage currently overwrites the error from
evaluating the message with the error from evaluating the UniqueID, potentially
returning nil when the message evaluation failed; update getUniqueIdAndMessage
to capture both evaluation errors from
rm.celEvaluator.EvaluateExpressionWithContext (for rule.Expressions.Message and
rule.Expressions.UniqueID), log each failure as you already do, and return a
non-nil error if either evaluation failed (e.g., return the first non-nil error
or wrap both errors together) while still hashing uniqueID via hashStringToMD5
before returning; ensure the returned err reflects any failure from evaluating
message or uniqueID so callers of getUniqueIdAndMessage see the correct error.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/rulemanager/rule_manager.go (1)
375-388:⚠️ Potential issue | 🟡 Minor
getUniqueIdAndMessagesilently loses themessageevaluation error.
errfrom theMessageevaluation is logged locally but immediately overwritten by theUniqueIDevaluation result. If the first call fails and the second succeeds, the function returnserr == nil, causing the caller's guard at line 233 (if err != nil { continue }) to fall through and create a rule failure with an emptymessagestring. The practical impact is an alert with no human-readable message.🐛 Proposed fix — preserve both errors
func (rm *RuleManager) getUniqueIdAndMessage(evalContext *cel.EventActivation, rule typesv1.Rule) (string, string, error) { message, err := rm.celEvaluator.EvaluateExpressionWithContext(evalContext, rule.Expressions.Message) if err != nil { logger.L().Error("RuleManager - failed to evaluate message", helpers.Error(err)) + // Return early; a missing message makes the alert unusable. + return "", "", err } uniqueID, err := rm.celEvaluator.EvaluateExpressionWithContext(evalContext, rule.Expressions.UniqueID) if err != nil { logger.L().Error("RuleManager - failed to evaluate unique ID", helpers.Error(err)) + return "", "", err } uniqueID = hashStringToMD5(uniqueID) return message, uniqueID, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/rule_manager.go` around lines 375 - 388, The function getUniqueIdAndMessage currently overwrites the error from evaluating message with the error from evaluating unique ID, which can return nil even when message evaluation failed; update getUniqueIdAndMessage to capture errors from both rm.celEvaluator.EvaluateExpressionWithContext calls into separate variables (e.g., errMessage, errUniqueID), log each appropriately, and return a non-nil error if either failed (preferably combine them into a single error or return errMessage if you want message failures to take precedence); ensure you still hash the uniqueID only when errUniqueID is nil and return the evaluated message, hashed uniqueID, and the combined/non-nil error so the caller's err guard works as intended.
🧹 Nitpick comments (2)
pkg/rulemanager/cel/cel_interface.go (1)
11-16: AddRelease()requirement toCreateEvalContextdocumentation.The doc on
EvaluateRule/EvaluateExpressioncorrectly points callers towardCreateEvalContext, but neither there nor onCreateEvalContextitself is there a note that the returned*EventActivationmust beRelease()d after use. Since it's backed by async.Pool, omitting the call silently leaks pool capacity. A one-liner like// Callers must call Release() on the returned *EventActivation when done.on line 22 would make the contract unambiguous.Also applies to: 22-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/cel/cel_interface.go` around lines 11 - 16, The CreateEvalContext documentation should explicitly require callers to call Release() on the returned *EventActivation to avoid leaking the sync.Pool; update the comment for CreateEvalContext (and consider mirroring on EvaluateRule/EvaluateExpression docs) to include a one-line note like "Callers must call Release() on the returned *EventActivation when done." referencing EventActivation and Release() so the ownership contract is unambiguous.pkg/rulemanager/cel/cel.go (1)
216-238: EventType filtering already occurs at the call site; defensive guard for empty slice is reasonable but not critical.The concerns are partially overstated:
EventType is consulted — filtering happens via
rule.ExpressionsByEventType[eventType]at both call sites inrule_manager.go(lines 198, 337). TheInit()method intypes/v1/types.goexplicitly populates this map by EventType. The design places the filtering gate at the call site, not insideEvaluateRuleWithContext.Empty slice behavior — While
EvaluateRuleWithContextreturns(true, nil)for an empty slice, both actual call sites already guard withlen(ruleExpressions) == 0checks before calling (lines 199–200, 338–339). The publicEvaluateRulewrapper exists but is unused in the codebase, so this poses no practical risk.Defensive programming — Adding a guard for empty expressions is still a reasonable suggestion to prevent misuse if callers are added later or the API is exposed to external packages.
Consider:
- Adding a defensive guard and updating the docstring to document the contract: "Caller is responsible for pre-filtering expressions by event type."
♻️ Optional defensive guard
func (c *CEL) EvaluateRuleWithContext(evalContext *EventActivation, expressions []typesv1.RuleExpression) (bool, error) { + if len(expressions) == 0 { + return false, nil + } for _, expression := range expressions {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/cel/cel.go` around lines 216 - 238, EvaluateRuleWithContext currently treats an empty expressions slice as a successful match; add a defensive guard at the start of CEL.EvaluateRuleWithContext that if len(expressions) == 0 it returns (false, nil) to avoid accidental true results when callers forget to pre-filter by event type, and update the function docstring to state the contract: callers (e.g., code using rule.ExpressionsByEventType or EvaluateRule) must pre-filter expressions by EventType and that an empty expressions slice is treated as no match. Ensure references to EvaluateRuleWithContext, EventActivation, rule.ExpressionsByEventType, and EvaluateRule are used to locate and update the code and docs.
🤖 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/cel/cel.go`:
- Around line 178-183: CreateEvalContext currently uses bare type assertions on
converter(event).(utils.CelEvent) and event.(utils.CelEvent) which can panic for
K8sEvent values that do not implement CelEvent; replace both with comma-ok
checks (e.g., v, ok := converter(event).(utils.CelEvent) and v, ok :=
event.(utils.CelEvent)), handle the !ok path by either returning a nil/err
EventActivation or setting obj.Raw to a safe zero value and ensuring callers
check for nil EventActivation as noted; update references in CreateEvalContext,
the objectPool usage (obj := objectPool.Get().(*xcel.Object[utils.CelEvent]))
and eventConverters handling to avoid panics and propagate an explicit failure
instead of allowing a runtime panic.
---
Outside diff comments:
In `@pkg/rulemanager/rule_manager.go`:
- Around line 375-388: The function getUniqueIdAndMessage currently overwrites
the error from evaluating message with the error from evaluating unique ID,
which can return nil even when message evaluation failed; update
getUniqueIdAndMessage to capture errors from both
rm.celEvaluator.EvaluateExpressionWithContext calls into separate variables
(e.g., errMessage, errUniqueID), log each appropriately, and return a non-nil
error if either failed (preferably combine them into a single error or return
errMessage if you want message failures to take precedence); ensure you still
hash the uniqueID only when errUniqueID is nil and return the evaluated message,
hashed uniqueID, and the combined/non-nil error so the caller's err guard works
as intended.
---
Nitpick comments:
In `@pkg/rulemanager/cel/cel_interface.go`:
- Around line 11-16: The CreateEvalContext documentation should explicitly
require callers to call Release() on the returned *EventActivation to avoid
leaking the sync.Pool; update the comment for CreateEvalContext (and consider
mirroring on EvaluateRule/EvaluateExpression docs) to include a one-line note
like "Callers must call Release() on the returned *EventActivation when done."
referencing EventActivation and Release() so the ownership contract is
unambiguous.
In `@pkg/rulemanager/cel/cel.go`:
- Around line 216-238: EvaluateRuleWithContext currently treats an empty
expressions slice as a successful match; add a defensive guard at the start of
CEL.EvaluateRuleWithContext that if len(expressions) == 0 it returns (false,
nil) to avoid accidental true results when callers forget to pre-filter by event
type, and update the function docstring to state the contract: callers (e.g.,
code using rule.ExpressionsByEventType or EvaluateRule) must pre-filter
expressions by EventType and that an empty expressions slice is treated as no
match. Ensure references to EvaluateRuleWithContext, EventActivation,
rule.ExpressionsByEventType, and EvaluateRule are used to locate and update the
code and docs.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/rulemanager/cel/cel.gopkg/rulemanager/cel/cel_interface.gopkg/rulemanager/rule_manager.go
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>
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
…n CEL evaluation Signed-off-by: Yakir Oren <yakiroren@gmail.com>
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
YakirOren
force-pushed
the
optimize/http-event-creation
branch
from
c6ac4d3 to
71d9554
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/rulemanager/cel/cel.go (1)
183-197:⚠️ Potential issue | 🟠 MajorUnchecked type assertions can panic at runtime.
The bare type assertions at lines 186 and 188 (
converter(event).(utils.CelEvent)andevent.(utils.CelEvent)) will panic if the event does not implementutils.CelEvent. While current implementations satisfy the interface, this provides no compile-time safety.🛡️ Proposed fix — use comma-ok assertions
obj := objectPool.Get().(*xcel.Object[utils.CelEvent]) var celEvent utils.CelEvent if converter, exists := c.eventConverters[eventType]; exists { - celEvent = converter(event).(utils.CelEvent) + converted := converter(event) + var ok bool + celEvent, ok = converted.(utils.CelEvent) + if !ok { + objectPool.Put(obj) + logger.L().Error("CEL - converted event does not implement CelEvent", + helpers.String("eventType", string(eventType))) + return nil + } } else { - celEvent = event.(utils.CelEvent) + var ok bool + celEvent, ok = event.(utils.CelEvent) + if !ok { + objectPool.Put(obj) + logger.L().Error("CEL - event does not implement CelEvent", + helpers.String("eventType", string(eventType))) + return nil + } }Note: Callers must nil-check the returned
*EventActivationafter this change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/cel/cel.go` around lines 183 - 197, The code uses unchecked type assertions for converter(event).(utils.CelEvent) and event.(utils.CelEvent) which can panic; update the block in the function that obtains obj from objectPool to use comma-ok assertions when casting to utils.CelEvent (e.g. result, ok := converter(event).(utils.CelEvent) and evt, ok := event.(utils.CelEvent)), handle the not-ok case by returning an error or setting obj to nil (or a safe default) and ensure any callers of the surrounding function (including those that expect *EventActivation) check for nil/err; apply the same safe-cast pattern when wrapping with utils.HttpEventWrapper from utils.HttpEventWrapperPool and assign obj.Raw only after successful casts.
🧹 Nitpick comments (1)
pkg/utils/cel.go (1)
71-73: Pointer equality inEqualmay cause unexpected CEL behavior.
Equaluses pointer comparison (other == w), which means twoHttpEventWrapperinstances wrapping the sameCelEventwill not be considered equal. This could cause issues if CEL expressions compare request objects.If value-based equality is needed, compare the underlying
CelEvent:♻️ Proposed value-based equality
func (w *HttpEventWrapper) Equal(other ref.Val) ref.Val { - return celtypes.Bool(other == w) + if otherWrapper, ok := other.(*HttpEventWrapper); ok { + return celtypes.Bool(w.CelEvent == otherWrapper.CelEvent) + } + return celtypes.Bool(false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/cel.go` around lines 71 - 73, The Equal method on HttpEventWrapper currently does pointer equality (other == w); change it to perform a type assertion to *HttpEventWrapper (or the concrete wrapper type) and then compare the underlying CelEvent values for equality (e.g., compare w.Event to otherWrapper.Event using a value comparison or reflect.DeepEqual if CelEvent is a struct) and return celtypes.Bool accordingly; also handle non-wrapper or nil other values by returning celtypes.Bool(false). Target the HttpEventWrapper.Equal function and the underlying CelEvent field in your fix.
🤖 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/utils/cel.go`:
- Around line 39-44: The package-level variable httpRequestType can be written
by SetHttpRequestType and concurrently read by HttpEventWrapper.Type(), causing
a data race; change the storage to a concurrency-safe form (e.g.,
atomic.Pointer[celtypes.Type] or protect access with a sync.RWMutex) and update
SetHttpRequestType to store via the chosen synchronization primitive and
HttpEventWrapper.Type() to load/peek via the same primitive (references:
httpRequestType, SetHttpRequestType, HttpEventWrapper.Type()) so that writes and
reads are safe during runtime configuration changes.
---
Duplicate comments:
In `@pkg/rulemanager/cel/cel.go`:
- Around line 183-197: The code uses unchecked type assertions for
converter(event).(utils.CelEvent) and event.(utils.CelEvent) which can panic;
update the block in the function that obtains obj from objectPool to use
comma-ok assertions when casting to utils.CelEvent (e.g. result, ok :=
converter(event).(utils.CelEvent) and evt, ok := event.(utils.CelEvent)), handle
the not-ok case by returning an error or setting obj to nil (or a safe default)
and ensure any callers of the surrounding function (including those that expect
*EventActivation) check for nil/err; apply the same safe-cast pattern when
wrapping with utils.HttpEventWrapper from utils.HttpEventWrapperPool and assign
obj.Raw only after successful casts.
---
Nitpick comments:
In `@pkg/utils/cel.go`:
- Around line 71-73: The Equal method on HttpEventWrapper currently does pointer
equality (other == w); change it to perform a type assertion to
*HttpEventWrapper (or the concrete wrapper type) and then compare the underlying
CelEvent values for equality (e.g., compare w.Event to otherWrapper.Event using
a value comparison or reflect.DeepEqual if CelEvent is a struct) and return
celtypes.Bool accordingly; also handle non-wrapper or nil other values by
returning celtypes.Bool(false). Target the HttpEventWrapper.Equal function and
the underlying CelEvent field in your fix.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.modpkg/rulemanager/cel/cel.gopkg/rulemanager/cel/cel_interface.gopkg/rulemanager/cel/libraries/parse/parse.gopkg/rulemanager/cel/libraries/parse/parselib.gopkg/rulemanager/cel/libraries/parse/parsing_test.gopkg/rulemanager/rule_manager.gopkg/rulemanager/rulecreator/factory.gopkg/rulemanager/types/v1/types.gopkg/utils/cel.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/rulemanager/cel/libraries/parse/parsing_test.go
- pkg/rulemanager/rulecreator/factory.go
- pkg/rulemanager/cel/libraries/parse/parse.go
| var httpRequestType *celtypes.Type | ||
|
|
||
| // SetHttpRequestType sets the CEL type used by HttpEventWrapper. | ||
| func SetHttpRequestType(t *celtypes.Type) { | ||
| httpRequestType = t | ||
| } |
There was a problem hiding this comment.
Potential data race on httpRequestType global.
httpRequestType is a package-level variable that can be written by SetHttpRequestType and read by HttpEventWrapper.Type(). If SetHttpRequestType is called after initialization (e.g., during environment extension), concurrent reads from Type() during CEL evaluation could race.
Consider using atomic.Pointer[celtypes.Type] or ensuring SetHttpRequestType is only called once during initialization before any evaluation occurs.
🛡️ Proposed fix using atomic
-var httpRequestType *celtypes.Type
+var httpRequestType atomic.Pointer[celtypes.Type]
// SetHttpRequestType sets the CEL type used by HttpEventWrapper.
func SetHttpRequestType(t *celtypes.Type) {
- httpRequestType = t
+ httpRequestType.Store(t)
}Then in Type():
func (w *HttpEventWrapper) Type() ref.Type {
- if httpRequestType == nil {
+ t := httpRequestType.Load()
+ if t == nil {
return celtypes.ErrType
}
- return httpRequestType
+ return t
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/utils/cel.go` around lines 39 - 44, The package-level variable
httpRequestType can be written by SetHttpRequestType and concurrently read by
HttpEventWrapper.Type(), causing a data race; change the storage to a
concurrency-safe form (e.g., atomic.Pointer[celtypes.Type] or protect access
with a sync.RWMutex) and update SetHttpRequestType to store via the chosen
synchronization primitive and HttpEventWrapper.Type() to load/peek via the same
primitive (references: httpRequestType, SetHttpRequestType,
HttpEventWrapper.Type()) so that writes and reads are safe during runtime
configuration changes.
71d9554 to
8342b2f
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/rulemanager/rule_manager.go (1)
376-388:⚠️ Potential issue | 🟠 MajorDo not overwrite message-evaluation errors.
errfrom message evaluation is overwritten by uniqueID evaluation. If message fails but uniqueID succeeds, the function returns nil error with a degraded message.🛠️ Suggested fix
import ( "context" "crypto/md5" + "errors" "encoding/hex" "strconv" "time" @@ func (rm *RuleManager) getUniqueIdAndMessage(evalContext *cel.EventActivation, rule typesv1.Rule) (string, string, error) { - message, err := rm.celEvaluator.EvaluateExpressionWithContext(evalContext, rule.Expressions.Message) - if err != nil { - logger.L().Error("RuleManager - failed to evaluate message", helpers.Error(err)) + message, msgErr := rm.celEvaluator.EvaluateExpressionWithContext(evalContext, rule.Expressions.Message) + if msgErr != nil { + logger.L().Error("RuleManager - failed to evaluate message", helpers.Error(msgErr)) } - uniqueID, err := rm.celEvaluator.EvaluateExpressionWithContext(evalContext, rule.Expressions.UniqueID) - if err != nil { - logger.L().Error("RuleManager - failed to evaluate unique ID", helpers.Error(err)) + uniqueID, uidErr := rm.celEvaluator.EvaluateExpressionWithContext(evalContext, rule.Expressions.UniqueID) + if uidErr != nil { + logger.L().Error("RuleManager - failed to evaluate unique ID", helpers.Error(uidErr)) } uniqueID = hashStringToMD5(uniqueID) - return message, uniqueID, err + return message, uniqueID, errors.Join(msgErr, uidErr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/rule_manager.go` around lines 376 - 388, In getUniqueIdAndMessage, the error from evaluating rule.Expressions.Message is being overwritten by the uniqueID evaluation; update the function to capture both errors separately (e.g., messageErr and idErr) when calling rm.celEvaluator.EvaluateExpressionWithContext for rule.Expressions.Message and rule.Expressions.UniqueID, still log each failure with logger.L().Error, hash the uniqueID result with hashStringToMD5, and return the message plus uniqueID along with a non-nil error if either evaluation failed (prefer returning messageErr if present, or a composed error combining both errors) so message-evaluation failures are not lost.pkg/utils/cel.go (1)
598-607:⚠️ Potential issue | 🟠 MajorBody fallback mutates request stream and can truncate downstream reads.
The fallback path consumes
req.BodyviaLimitReader, closes it, and restores only the consumed bytes. For oversized bodies, downstream consumers lose the unread tail.🛠️ Suggested fix
- limitedReader := io.LimitReader(req.Body, maxBodySize) - bodyBytes, err := io.ReadAll(limitedReader) - req.Body.Close() // Close the original body - req.Body = io.NopCloser(bytes.NewReader(bodyBytes)) // Restore for downstream + limitedReader := io.LimitReader(req.Body, maxBodySize) + bodyBytes, err := io.ReadAll(limitedReader) + // Reconstruct full stream for downstream readers: + // consumed prefix + unread remainder. + req.Body = io.NopCloser(io.MultiReader( + bytes.NewReader(bodyBytes), + req.Body, + )) if err != nil { return celtypes.String(""), err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/cel.go` around lines 598 - 607, The current fallback consumes req.Body via io.LimitReader and restores only the consumed bytes, truncating downstream reads; instead, read the entire body into bodyBytes (io.ReadAll(req.Body)), then close and restore req.Body to io.NopCloser(bytes.NewReader(bodyBytes)) so downstream sees the full stream, but when returning the CEL string truncate the returned value to maxBodySize (10*1024*1024) so callers still get the same size-limited view; update the code around maxBodySize, limitedReader, bodyBytes and the return to implement this change.
♻️ Duplicate comments (2)
pkg/rulemanager/cel/cel.go (1)
203-207:⚠️ Potential issue | 🔴 CriticalAvoid panic-prone type assertions in
CreateEvalContext(still unresolved).Direct assertions (
.(utils.CelEvent)) can panic if a converter (or event) returns a non-CelEventimplementation.🛠️ Suggested fix
var celEvent utils.CelEvent if converter, exists := c.eventConverters[eventType]; exists { - celEvent = converter(event).(utils.CelEvent) + converted := converter(event) + ce, ok := converted.(utils.CelEvent) + if !ok { + logger.L().Error("CEL - converted event does not implement CelEvent", + helpers.String("eventType", string(eventType))) + } else { + celEvent = ce + } } else { - celEvent = event.(utils.CelEvent) + ce, ok := event.(utils.CelEvent) + if !ok { + logger.L().Error("CEL - event does not implement CelEvent", + helpers.String("eventType", string(eventType))) + } else { + celEvent = ce + } }#!/bin/bash # Verify panic-prone assertions still exist and locate all affected call sites. rg -n --type go '\.\(utils\.CelEvent\)' pkg/rulemanager/cel/cel.go rg -n --type go 'CreateEvalContext\(' pkg/rulemanager/cel/cel.go pkg/rulemanager/rule_manager.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/cel/cel.go` around lines 203 - 207, In CreateEvalContext, avoid panic-prone direct assertions on converter(event).(utils.CelEvent) and event.(utils.CelEvent): replace these with safe checks (type assertion with the "ok" comma-ok idiom or a type switch) and return or propagate a clear error if the value does not implement utils.CelEvent; locate the checks around c.eventConverters, converter, celEvent and event and either change the converter signature to return (utils.CelEvent, error) or validate the converter result before assigning to celEvent, logging/returning an error instead of panicking.pkg/utils/cel.go (1)
39-44:⚠️ Potential issue | 🟠 MajorProtect
httpRequestTypewith synchronization (still race-prone).
SetHttpRequestTypewrites andHttpEventWrapper.Type()reads a package-global pointer without synchronization. Under concurrent init/runtime reconfiguration this is a data race.🛠️ Suggested fix
import ( "bytes" "errors" "fmt" "io" "net/http" "reflect" "sync" + "sync/atomic" @@ -var httpRequestType *celtypes.Type +var httpRequestType atomic.Pointer[celtypes.Type] @@ func SetHttpRequestType(t *celtypes.Type) { - httpRequestType = t + httpRequestType.Store(t) } @@ func (w *HttpEventWrapper) Type() ref.Type { - if httpRequestType == nil { + t := httpRequestType.Load() + if t == nil { return celtypes.ErrType } - return httpRequestType + return t }#!/bin/bash # Verify unsynchronized shared access points for httpRequestType. rg -n --type go 'httpRequestType|SetHttpRequestType|func \(w \*HttpEventWrapper\) Type\(' pkg/utils/cel.go pkg/rulemanager/cel/cel.goAlso applies to: 75-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/cel.go` around lines 39 - 44, httpRequestType is a package-global pointer written by SetHttpRequestType and read by HttpEventWrapper.Type() without synchronization, causing a data race; fix by making access synchronized (either replace the raw *celtypes.Type with an atomic.Value storing *celtypes.Type or add a package-level sync.RWMutex) and update SetHttpRequestType to perform a write under the lock (or atomic.Store), and update HttpEventWrapper.Type() to read under the read-lock (or atomic.Load) to ensure safe concurrent reads/writes; reference the symbols httpRequestType, SetHttpRequestType, and HttpEventWrapper.Type().
🧹 Nitpick comments (1)
pkg/rulemanager/cel/cel.go (1)
61-72: MakeRelease()defensive against nil/double-release misuse.
Release()assumesa.eventis always non-nil; adding a small guard makes pooling safer and easier to reuse correctly.♻️ Suggested hardening
func (a *EventActivation) Release() { + if a == nil || a.event == nil { + return + } if w, ok := a.event.Raw.(*utils.HttpEventWrapper); ok { w.CelEvent = nil utils.HttpEventWrapperPool.Put(w) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/cel/cel.go` around lines 61 - 72, Make Release() idempotent and defensive: at the start of EventActivation.Release check if the receiver or a.event is nil and return immediately to avoid double-release; when accessing a.event.Raw assert it's non-nil before type-asserting to *utils.HttpEventWrapper and only call utils.HttpEventWrapperPool.Put(w) and objectPool.Put(a.event) if those objects are non-nil; finally clear fields (a.event = nil, a.eventType = "", a.isHTTP = false) and call eventActivationPool.Put(a) once. Reference: EventActivation.Release, a.event, a.event.Raw, utils.HttpEventWrapperPool, objectPool, eventActivationPool.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/rulemanager/rule_manager.go`:
- Around line 376-388: In getUniqueIdAndMessage, the error from evaluating
rule.Expressions.Message is being overwritten by the uniqueID evaluation; update
the function to capture both errors separately (e.g., messageErr and idErr) when
calling rm.celEvaluator.EvaluateExpressionWithContext for
rule.Expressions.Message and rule.Expressions.UniqueID, still log each failure
with logger.L().Error, hash the uniqueID result with hashStringToMD5, and return
the message plus uniqueID along with a non-nil error if either evaluation failed
(prefer returning messageErr if present, or a composed error combining both
errors) so message-evaluation failures are not lost.
In `@pkg/utils/cel.go`:
- Around line 598-607: The current fallback consumes req.Body via io.LimitReader
and restores only the consumed bytes, truncating downstream reads; instead, read
the entire body into bodyBytes (io.ReadAll(req.Body)), then close and restore
req.Body to io.NopCloser(bytes.NewReader(bodyBytes)) so downstream sees the full
stream, but when returning the CEL string truncate the returned value to
maxBodySize (10*1024*1024) so callers still get the same size-limited view;
update the code around maxBodySize, limitedReader, bodyBytes and the return to
implement this change.
---
Duplicate comments:
In `@pkg/rulemanager/cel/cel.go`:
- Around line 203-207: In CreateEvalContext, avoid panic-prone direct assertions
on converter(event).(utils.CelEvent) and event.(utils.CelEvent): replace these
with safe checks (type assertion with the "ok" comma-ok idiom or a type switch)
and return or propagate a clear error if the value does not implement
utils.CelEvent; locate the checks around c.eventConverters, converter, celEvent
and event and either change the converter signature to return (utils.CelEvent,
error) or validate the converter result before assigning to celEvent,
logging/returning an error instead of panicking.
In `@pkg/utils/cel.go`:
- Around line 39-44: httpRequestType is a package-global pointer written by
SetHttpRequestType and read by HttpEventWrapper.Type() without synchronization,
causing a data race; fix by making access synchronized (either replace the raw
*celtypes.Type with an atomic.Value storing *celtypes.Type or add a
package-level sync.RWMutex) and update SetHttpRequestType to perform a write
under the lock (or atomic.Store), and update HttpEventWrapper.Type() to read
under the read-lock (or atomic.Load) to ensure safe concurrent reads/writes;
reference the symbols httpRequestType, SetHttpRequestType, and
HttpEventWrapper.Type().
---
Nitpick comments:
In `@pkg/rulemanager/cel/cel.go`:
- Around line 61-72: Make Release() idempotent and defensive: at the start of
EventActivation.Release check if the receiver or a.event is nil and return
immediately to avoid double-release; when accessing a.event.Raw assert it's
non-nil before type-asserting to *utils.HttpEventWrapper and only call
utils.HttpEventWrapperPool.Put(w) and objectPool.Put(a.event) if those objects
are non-nil; finally clear fields (a.event = nil, a.eventType = "", a.isHTTP =
false) and call eventActivationPool.Put(a) once. Reference:
EventActivation.Release, a.event, a.event.Raw, utils.HttpEventWrapperPool,
objectPool, eventActivationPool.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.modpkg/rulemanager/cel/cel.gopkg/rulemanager/cel/cel_interface.gopkg/rulemanager/cel/cel_test.gopkg/rulemanager/rule_manager.gopkg/utils/cel.go
2 participants
Summary
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores