optimize CEL rule evaluation performance

[YakirOren]

Summary

• Reorder CreateEventFromRequest to check packet direction before expensive HTTP parsing
• Eliminate full StructEvent copy in MakeHttpEvent by mutating in-place
• Extract isPrivateIP helper, removing inline IIFE and redundant net.ParseIP closures
• Replace map-based GetPacketDirection with switch statement

Summary by CodeRabbit

Release Notes

• New Features

  • Added parse.basename() function to extract filenames from file paths.
  • Introduced context-based evaluation methods for rules and expressions, enabling more efficient repeated evaluations on the same event.

• Refactor

  • Optimized rule evaluation performance through pre-computed expression indexing by event type.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR refactors the CEL rule evaluation system to support context-based evaluation methods. It separates evaluation context creation from rule evaluation, introduces expression indexing by event type in the Rule struct, adds a new parse.basename function for path handling, and updates the rule manager to leverage pre-built evaluation contexts for efficiency.

Changes

Cohort / File(s)	Summary
CEL Evaluation Core pkg/rulemanager/cel/cel.go, pkg/rulemanager/cel/cel_interface.go	Introduced context-based evaluation methods (EvaluateRuleWithContext, EvaluateExpressionWithContext) and made CreateEvalContext public with new signature accepting K8sEvent. Added wrapper methods EvaluateRule and EvaluateExpression that build context internally. Extended RuleEvaluator interface with three new methods.
CEL Library Extensions pkg/rulemanager/cel/libraries/parse/parse.go, pkg/rulemanager/cel/libraries/parse/parselib.go, pkg/rulemanager/cel/libraries/parse/parsing_test.go	Added parse.basename function to extract filename from path (e.g., '/usr/bin/nmap' → 'nmap'). Includes helper method, library overload registration with cost estimation, and four comprehensive test cases covering edge cases.
Rule Management Layer pkg/rulemanager/types/v1/types.go, pkg/rulemanager/rulecreator/factory.go	Added ExpressionsByEventType field to Rule struct with Init() method to populate expression index by event type. Updated factory methods (RegisterRule, SyncRules, UpdateRule) to call Init() on rules before internal storage.
Rule Evaluation Refactoring pkg/rulemanager/rule_manager.go	Refactored rule evaluation to use pre-built evaluation context instead of per-call event-type filtering. Replaced standalone rule expression retrieval with ExpressionsByEventType lookup. Updated EvaluateRule calls to use EvaluateRuleWithContext, and message/unique ID generation to use EvaluateExpressionWithContext.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• remove toMap function  #693: Modifies CEL evaluator and rule evaluation flow similarly—introduces context-based evaluation methods and refactors rule_manager.go to use the new evaluation API.
• optimize header parsing and add early return in ruleAppliesToContext #692: Modifies CEL evaluation code in cel.go with related changes to evaluation input handling and context reuse patterns.

Poem

🐰 Hop, skip, and a CEL
Context built once—no need to rebuild it well,
Basename extracts with a slice and a bound,
Rule expressions indexed, efficiency found!
The rabbit rejoices—evaluations now soar! 🚀

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'optimize CEL rule evaluation performance' is directly related to the main changes, which focus on optimizing rule evaluation through context-based CEL evaluation and pre-indexing rules by event type.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into main

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch optimize/http-event-creation

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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 | 🟠 Major

Pre-existing bug: CreateRulePolicyRulesByEventType appends to the slice it's iterating over.

This isn't part of the current PR changes, but this method iterates over rules and conditionally appends to rules in the same loop (line 75), which will cause duplicate entries and potentially an infinite loop (in Go, range evaluates 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 using path.Base from the standard library.

The stdlib path.Base handles 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/", whereas path.Base would 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.Base is 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 path parameter and the path import — 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 the LastIndex == -1 branch 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, err is overwritten to nil and 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-receiver Init() are correct. One subtle point: if Init() is ever not called, ExpressionsByEventType remains nil. In Go, indexing a nil map returns the zero value (empty slice) without panicking, so rules would be silently skipped rather than producing an error. The factory currently calls Init() in all mutation paths (RegisterRule, SyncRules, UpdateRule), ensuring all rules in circulation are initialized. However, CreateRuleByID and CreateRuleByName can 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.go where 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]

did you run a Bench to see the improvements? maybe we should commit it too...

[matthyx]

if called twice, it recreates the map and loses the original data, maybe add a guard?

Suggested change

	func (r *Rule) Init() {
	func (r *Rule) Init() {
	if r.ExpressionsByEventType != nil {
	return
	}