Conversation
📝 WalkthroughWalkthroughRefactors CEL evaluation to separate context creation from evaluation, adds context-aware evaluation APIs, indexes rule expressions by event type, exposes parse.basename, updates rule manager to build and reuse evalContext, and adjusts rule factory to initialize rules. Changes
Sequence Diagram(s)mermaid 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...
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
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.
2 participants
Summary
Summary by CodeRabbit
New Features
Refactor
Tests
Chores