feat(rulemanager): CEL rule state store for cross-event correlation - #875
feat(rulemanager): CEL rule state store for cross-event correlation#875slashben wants to merge 17 commits into
Conversation
Gives the CRD contract -- including the new StateWrites clause -- exactly one definition shared with the operator, without retiring typesv1.Rule. Expressions and ProfileDataRequired stay shadowed because their types genuinely differ: utils.EventType covers all node-agent event streams, and FieldRequirement carries a Declared flag plus strict unknown-key rejection that armotypes.ProfileDataField has neither of. The two decoders that reach Rule disagree about the shadows. encoding/json resolves same-tag conflicts by depth, so only the depth-0 fields are populated. apimachinery's converter -- the production CRD path, via DefaultUnstructuredConverter.FromUnstructured -- has no depth rule and visits every field independently, so it fills the embedded copies too. Either way the depth-0 fields are what node-agent code reads. rule_embedding_test.go pins both decoders, including the apimachinery path the plan originally left untested. Docs-exempt: struct-embedding refactor; StateWrites is inert until Task 6 reads it Signed-off-by: Ben <ben@armosec.io>
Ordering guards need to compare when events happened, not when the worker pool observed them. ResolveEventTime prefers the event's kernel timestamp and falls back to enrichment time only when it is zero. Exposed as a top-level 'timestamp' variable rather than an event field: CelFields getters receive an xcel wrapper around the event and cannot reach EnrichedEvent.Timestamp, so a field would be a second, divergent source of truth. The store will stamp entries from this same function. The CEL-level tests use a real utils.StructEvent rather than a fake: the eval context casts the event to utils.CelEvent and calls GetEventType(), so a fake embedding a nil utils.K8sEvent panics before reaching any assertion. They also assert instants rather than rendered text -- time.Unix yields a local-zone Time, so the rendered offset is whatever the node's TZ is. Signed-off-by: Ben <ben@armosec.io>
Walks the creator's global process map instead of GetPidBranch, which resolves a container shim and therefore errors for every host / cgroup-0 process -- leaving those events with a zero-value ProcessTree. Ancestor matching must work identically on a VM and in a pod. maxDepth bounds the walk, and a seen-set breaks parent cycles that a reparenting race could produce, so a malformed tree cannot hang rule evaluation. A PPID of 0 terminates the walk: it means "parent unknown", and recording it would put a key in the ancestor list that no state entry can ever be stored under. The manager mock takes a settable ancestor chain rather than always returning nil, because the rule-level tests for ancestor matching need to stub a chain without building a real process tree. Docs-exempt: internal API; the CEL surface that exposes it is documented when it lands Signed-off-by: Ben <ben@armosec.io>
Sharded by scope ID so the per-scope cap is a plain len(), container-removal purge is one map delete, and one container's churn stays off its neighbours' locks. Over-cap writes are REJECTED, never satisfied by eviction: evicting would let a container that sprays events silently disable detection for itself or a neighbour. The host bucket (c:__host__) gets its own larger cap because it holds the whole node's process space and never receives a removal purge. Two properties worth knowing when reading this code: Replacing an existing key bypasses the cap check, because it does not grow the scope. Without that, a scope sitting at its cap could never update its own markers and a bidirectional rule would freeze on stale state. The global ceiling is approximate under concurrency and says so in a comment. Concurrent writers can each pass the size check before any increments, so the store can overshoot MaxSize by up to the number of in-flight writers. Making it exact would serialise every write on one lock. The per-scope cap is exact, and that is the one that bounds a single workload. Knows nothing about CEL or rules, so it is testable without an evaluator. Docs-exempt: internal store; the rule-facing surface is documented in Tasks 5-8 Signed-off-by: Ben <ben@armosec.io>
Reads are pure and rule-private. The plan called for ruleID, scope IDs and the
ancestor list to be "injected into the eval context", which needed a concrete
mechanism: cel-go hands a function binding only its arguments, never the
activation, so a global function named "state.has" cannot discover which rule or
container it is evaluating for.
So "state" is a VARIABLE whose value is a per-(rule, event) Accessor, and the
read functions are member overloads on it. The authored syntax is unchanged --
state.has("mount_exec", "4471") -- but the context now lives in the receiver,
which is what makes cross-rule and cross-container reads inexpressible rather
than merely forbidden. It also keeps the library itself immutable, so it is safe
to share across the worker pool; a library holding per-evaluation state would
race, since node-agent evaluates events concurrently against one shared cel.Env.
A read resolves its scope by looking the name up in the rule's own stateWrites
declarations, per the spec: state is rule-private, so a name determines its
scope. An undeclared name reads as a miss rather than an error -- load-time
validation is what rejects it, and erroring here would take out a working rule.
The member functions are named "has" and "get", which collide with CEL's built-in
has() macro by identifier. TestState_DoesNotShadowTheHasMacro pins that
has(event.field) still parses and evaluates, because if that ever regressed it
would break every existing rule using field presence.
The cost estimator keys off overloadID, not the function name, for the same
reason: "has" and "get" are too generic to match on. It returns nil for unknown
overloads. Note these estimators are currently inert -- nothing in the repo calls
NewCompositeCostEstimator -- but it is written to be correct if wired up.
Signed-off-by: Ben <ben@armosec.io>
Writes run AFTER the predicate, so a predicate only ever sees state from earlier events -- otherwise a rule reading and writing the same name on one event type would trivially satisfy itself. To make that possible the per-rule body of the event loop is now its own function, evaluateRuleAndAlert. Its early exits were continues, which would have skipped the write clause whenever an alert was suppressed -- silently breaking the NEXT leg of the chain. As returns, the caller still runs the writes. Cooldown in particular must not suppress a write: writes are evidence gathering. Validation is at load, not runtime: an unknown event type, the `all` binding wildcard, a bad or non-positive TTL, an identity scope (operator-only) or a reserved _-prefixed name or value key fails loudly instead of producing a rule that silently never matches. A malformed clause degrades that one rule to non-correlating rather than breaking evaluation for the rest of the CRD. ValidateAll also rejects one name declared in two scopes. Reads take no scope argument -- they infer it from the name -- so that would make every read of the name ambiguous. The same name across several event types in one scope is the normal bidirectional idiom and stays legal. isSupportedEventType now also considers stateWrites event types; without that, write-only legs are filtered out before the loop and no chain ever forms. Two deliberate choices worth recording: Compilation happens per event, not once at rule load. It is pure string and duration parsing -- the CEL expressions are compiled and cached by the evaluator, keyed by expression text -- and it only runs for rules that declare writes, a small minority. Caching it on the Rule would need invalidation on every CRD change, and rules reach the loop by two paths of which only one populates load-time derived fields, so the cached field would be silently empty for host rules. utils.IsValidEventType is new and narrower than armotypes.IsKnownEventType, which spans both engines: k8s-admission is a real armotypes event type node-agent never emits, so a node-agent rule naming it has to be rejected at load. Also brings forward the celStateStore config field and the five state metrics from Task 8, since the executor and cost estimator need them to compile. Signed-off-by: Ben <ben@armosec.io>
Entries the predicate actually read become armotypes.CorrelationEvidence on the alert, so a correlation alert describes BOTH ends of the chain -- without it the alert would say only 'a process made an outbound connection' and drop the exec that makes it interesting. Populated in CreateRuleFailure rather than an event adapter, because SetFailureMetadata is per-event-type while correlation is not. InfectedPID and RuntimeProcessDetails still describe the triggering event: correlation enriches an incident, it does not re-key it, so backend grouping is unchanged. TestCorrelationEvidence_DoesNotRekeyTheAlert pins that, and an alert with no correlations still serializes with no correlations key at all. message/uniqueId now reuse the predicate's eval context so state.get() resolves against the same entries, and uniqueId can be derived from the join key -- which is what lets rulecooldown collapse both legs of a bidirectional rule. Note the plan specified `Scope: string(h.Scope)` here; CorrelationEvidence.Scope shipped as a typed armotypes.StateScope in v0.0.739 (a review change on armoapi-go #681), so the copy is direct and the plan text was stale. Signed-off-by: Ben <ben@armosec.io>
…purge
Adds the celStateStore config defaults and immediate scope purge on container
removal, so a churning node does not hold markers for containers that no longer
exist.
The purge deliberately uses Runtime.ContainerID VERBATIM. The plan specified
utils.TrimRuntimePrefix here, which would have been actively destructive: that
helper returns "" for an ID with no "//" separator, a bare runtime container ID
has none, and ContainerScopeID("") resolves to the HOST bucket -- so every
container exit would have wiped all host-process state instead of that
container's. The write path stores under the untrimmed Runtime.ContainerID
(EnrichedEvent.ContainerID is assigned from it in containercallback.go), so
untrimmed is also the only form that matches.
TestContainerScopeID_TrimmedRuntimeIDWouldHitTheHostBucket pins the trap.
main.go needs no change: the store is constructed inside CreateRuleManager, which
already has the ctx to run the sweeper on, and NewCEL does not need the store
because the per-rule Accessor carries it.
state_join_fired_total is NOT added. It has no call site until plan 3's
bidirectional component test exercises it, and an unwired metric is worse than a
noted gap.
Signed-off-by: Ben <ben@armosec.io>
…refactor The pre-refactor loop reached ReportRuleProcessed only by falling off the end, so an eval error or a cooldown-suppressed alert did not count as processed. Those were continues; extracting the alert path turned them into returns, which would have silently started counting them -- a changed metric meaning for every existing rule, and a violation of the plan's byte-for-byte constraint for rules with no stateWrites. evaluateRuleAndAlert now reports whether it ran to completion and the caller gates the metric on it. A predicate that simply did not match still counts as processed, exactly as the old fall-through did. Docs-exempt: restores pre-existing metric semantics; no documented behaviour changes Signed-off-by: Ben <ben@armosec.io>
Four rules rather than one, for the same reason the TTY test has four: a CEL expression that fails to compile returns (false, nil) rather than erroring, so 'no alert' is ambiguous between 'the predicate was false' and 'the rule never ran'. R9912/R9913 must always fire and R9914 must never, which turns a silent R9911 into a diagnosable result instead of a mystery. R9911 deliberately has no exec ruleExpression -- only a stateWrites clause on exec -- so it also exercises write-without-alerting, the shape every cross-event rule depends on. Docs-exempt: test fixtures only Signed-off-by: Ben <ben@armosec.io>
The alert's existence is the proof: R9911's network-leg predicate is state.has(...), so if the store does not work no alert is emitted. That needs only the existing Alertmanager label assertion -- no payload receiver. The trigger puts 8 seconds between the write and the read by sleeping inside the shell and then exec-ing nc, which replaces the image without forking so the pid is stable across both legs. Without that gap the two events are milliseconds apart and node-agent evaluates on a concurrent worker pool, so a failure could be reordering rather than a defect. Controls are asserted before the correlation rule on purpose: if they are silent their messages are the only diagnostic, and they are gone once the test fails. Docs-exempt: test only Signed-off-by: Ben <ben@armosec.io>
Test_35 was written but never added to the matrix, so the TTY field has only ever been verified by hand. Both are wired in now. Docs-exempt: CI configuration only Signed-off-by: Ben <ben@armosec.io>
…uned
The Rules CRD has a structural schema and no x-kubernetes-preserve-unknown-fields
at the rule level, so the API server SILENTLY STRIPPED stateWrites on write. The
field never reached node-agent, and every correlation rule loaded cleanly and
then never fired.
Nothing in the Go code was wrong. Every unit test passed -- including a new one
added here that drives the real production CEL env -- because none of them go
through the API server. Only the component test found it, which is exactly what
it was written for. Verified directly: before this change
`kubectl get rules ... -o jsonpath={.spec.rules[0].stateWrites}` returned null
after a successful apply.
statewiring_test.go is the regression test that was missing. The state library's
own tests build a bare cel.NewEnv; production builds an env with an xcel
TypeAdapter/TypeProvider, every other library and a static optimizer. This drives
the write guard, the key expression and the cross-leg read through THAT env, so a
wiring problem that only appears in the real evaluator is caught in unit tests
rather than on a cluster.
IMPORTANT -- this fixes only the copy of the CRD in tests/chart. The canonical
Rules CRD ships from the kubescape/helm-charts repo, and the same property must
be added there or the feature is inert in production no matter what node-agent
does.
Docs-exempt: CRD schema fix; the feature page already documents stateWrites
Signed-off-by: Ben <ben@armosec.io>
…isite The status block claimed the feature was untested on a cluster. It is now proven end-to-end against real eBPF by Test_36. Adds the deployment prerequisite that cost this a debugging cycle: the canonical Rules CRD lives in kubescape/helm-charts, and without a stateWrites property there the API server strips the clause silently -- rules load cleanly and never fire, with no error in any log. Signed-off-by: Ben <ben@armosec.io>
…rebase main's SUB-7845 added GetProcessBootTimeNs to ProcessTreeCreator, so the stub creator in ancestors_test.go no longer satisfied the interface. Ancestor walking does not consult start times, so the stub reports "unknown" (0) rather than inventing values. Docs-exempt: test-only rebase integration fix Signed-off-by: Ben <ben@armosec.io>
Every way a correlation rule can fail to fire is silent -- the rule applies, loads, and never matches. This orders the causes by likelihood and cost to check, leading with CRD pruning because that is the one that cost a debugging cycle and the one nobody would guess: kubectl apply reports success and no log or metric records the loss. Signed-off-by: Ben <ben@armosec.io>
slashben
added
ai-assisted
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis change adds a bounded CEL rule state store. Rules can write and read state across events, use ancestor process lookup, attach correlation evidence to alerts, expose state metrics, and validate the feature through unit and component tests. ChangesCEL rule state store
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
pkg/rulemanager/cel/cel.go (1)
216-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared logic from the two new context-aware evaluators.
EvaluateBoolExpressionWithContextandEvaluateStringExpressionWithContextduplicate the same three-step pattern already used byEvaluateRuleWithContext: callevaluateProgramWithContext, treat anilresult as a typed zero value, then type-assert the result. Extract a small shared helper that takes the target type's zero value and a type-assertion function, and have all three methods call it. This keeps the "cached compile failure" behavior in one place instead of three.♻️ Example of a shared helper
+func evalTyped[T any](c *CEL, evalContext map[string]any, expression string) (T, error) { + var zero T + out, err := c.evaluateProgramWithContext(expression, evalContext) + if err != nil { + return zero, err + } + if out == nil { + return zero, nil + } + val, ok := out.Value().(T) + if !ok { + return zero, fmt.Errorf("expression returned %T, expected %T", out.Value(), zero) + } + return val, nil +} func (c *CEL) EvaluateBoolExpressionWithContext(evalContext map[string]any, expression string) (bool, error) { - out, err := c.evaluateProgramWithContext(expression, evalContext) - if err != nil { - return false, err - } - if out == nil { - return false, nil - } - boolVal, ok := out.Value().(bool) - if !ok { - return false, fmt.Errorf("expression returned %T, expected bool", out.Value()) - } - return boolVal, nil + return evalTyped[bool](c, evalContext, expression) } func (c *CEL) EvaluateStringExpressionWithContext(evalContext map[string]any, expression string) (string, error) { - out, err := c.evaluateProgramWithContext(expression, evalContext) - if err != nil { - return "", err - } - if out == nil { - return "", nil - } - strVal, ok := out.Value().(string) - if !ok { - return "", fmt.Errorf("expression returned %T, expected string", out.Value()) - } - return strVal, nil + return evalTyped[string](c, evalContext, expression) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/rulemanager/cel/cel.go` around lines 216 - 254, Extract the duplicated evaluateProgramWithContext, nil-result, and type-assertion flow from EvaluateRuleWithContext, EvaluateBoolExpressionWithContext, and EvaluateStringExpressionWithContext into one shared helper. Have the helper accept the expression context and a typed zero value plus a type-assertion callback, preserving cached compile failures as typed zero results and returning the existing type-mismatch errors; update all three evaluators to use it.pkg/rulemanager/statewrites/validate.go (1)
44-48: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftCompile the
key,when, andvalueexpressions at load time.Validate accepts these three fields as opaque strings. A malformed expression therefore passes rule load and fails only during execution, where the executor logs at debug and drops the write (
executor.golines 118-124 and 158-164). That is the exact failure mode this package documents as unacceptable: the rule loads cleanly and never fires.Pass the CEL environment into validation and compile each expression, so a typo fails the rule at load.
Also applies to: 93-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/rulemanager/statewrites/validate.go` around lines 44 - 48, Update Validate to accept the CEL environment and compile the key, when, and value expressions during rule loading, rejecting malformed expressions before runtime. Propagate the environment from the caller and preserve the existing converted-write behavior for valid expressions.pkg/rulemanager/cel/libraries/state/accessor.go (1)
156-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
_containeror restrict it to container scope.
e.ScopeIDholds the ID of whatever scope the write declared. For a pod-scoped or node-scoped entry,_containerthen carriesp:ns/podorn:node. Rule authors read this key directly, so the name becomes part of the public CEL surface and a later rename breaks existing rules. Expose a scope-neutral key, and optionally keep_containeronly whene.Scope == armotypes.StateScopeContainer.♻️ Proposed change
m := map[string]any{ "_ts": e.Timestamp, "_eventType": string(e.EventType), - "_container": e.ScopeID, + "_scope": string(e.Scope), + "_scopeId": e.ScopeID, } + if e.Scope == armotypes.StateScopeContainer { + m["_container"] = e.ScopeID + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/rulemanager/cel/libraries/state/accessor.go` around lines 156 - 161, Update entryToMap so the public CEL field is scope-neutral rather than exposing every entry’s ScopeID as _container. Rename the key to an appropriate scope-neutral symbol, or only populate _container when e.Scope equals armotypes.StateScopeContainer; preserve the existing timestamp and event-type mappings.pkg/metricsmanager/otel/otel_metrics_manager.go (1)
545-556: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRename the
ReportStateWriteattribute key to match the Prometheus label.
ReportStateWritereusessuppressedOption, which hardcodes the attribute key"reason"for its second argument. The Prometheus implementation of the same metric explicitly labels this dimension"result"(node_agent_state_writes_totaluses[]string{prometheusRuleIdLabel, "result"}). As a result, the OTEL and Prometheus exports of the same logical metric use different attribute names for the write outcome, which breaks cross-backend dashboard or alert-rule consistency.Add a dedicated attribute-set helper (or a
result-keyed variant) forReportStateWriteinstead of reusingsuppressedOption.♻️ Proposed fix
+func (m *OTELMetricsManager) stateWriteOption(ruleID, result string) metric.MeasurementOption { + key := ruleID + "\x00" + result + if v, ok := m.stateWriteCache.Load(key); ok { + return v.(metric.MeasurementOption) + } + opt := metric.WithAttributeSet(attribute.NewSet( + attribute.String("rule_id", ruleID), + attribute.String("result", result), + )) + m.stateWriteCache.Store(key, opt) + return opt +} + func (m *OTELMetricsManager) ReportStateWrite(ruleID, result string) { - m.stateWritesTotal.Add(context.Background(), 1, m.suppressedOption(ruleID, result)) + m.stateWritesTotal.Add(context.Background(), 1, m.stateWriteOption(ruleID, result)) }(Add a
stateWriteCache sync.Mapfield alongside the other attribute-set caches.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/metricsmanager/otel/otel_metrics_manager.go` around lines 545 - 556, Update ReportStateWrite to use a dedicated result-keyed attribute helper or cache, with the second attribute named "result" instead of reusing suppressedOption’s "reason" key. Add the corresponding stateWriteCache field alongside the existing attribute caches, while leaving ReportStateWriteRejected on the reason-keyed path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rulemanager/rule_manager.go`:
- Around line 714-726: Update getUniqueIdAndMessage so a failure from evaluating
rule.Expressions.Message is preserved and returned instead of being overwritten
by the unique-ID evaluation assignment. Keep the existing logging, evaluate the
unique ID independently, and ensure the returned error reflects either
evaluation failure so evaluateRuleAndAlert cannot send an alert with an invalid
message.
In `@pkg/rulemanager/statewrites/executor_test.go`:
- Around line 299-303: Update TestScopeIDs_ResolvesFromTheEventOnly to compare
the node scope against rulestate.NodeScopeID() instead of the literal "n:",
while preserving the existing container and pod assertions.
In `@pkg/rulemanager/statewrites/executor.go`:
- Around line 141-146: Update the store-rejection branch in the executor’s
e.store.Set(entry) handling to report the rejection through the same counting
mechanism used by the guard, scope, and key paths, while preserving the existing
debug log and error context.
- Around line 81-85: Update the guard at the start of the relevant executor
method to return when enriched is nil or enriched.Event is nil, before calling
enriched.Event.GetEventType(). Preserve the existing checks and subsequent
processing for valid enriched events, and extend
TestApply_NilSafeOnMissingPieces to cover nil enriched/event input if
appropriate.
In `@pkg/rulestate/store.go`:
- Around line 53-58: Update Store.scopeCap to treat NodeScopeID() the same as
IsHostScopeID, returning MaxEntriesForHost for both node and host scopes while
retaining MaxEntriesPerContainer for container scopes. Add a regression test
alongside TestStore_HostBucketHasItsOwnLargerCap verifying node scope receives
the larger cap.
- Around line 62-105: Update Store.Set to determine whether the entry key
already exists before applying the global-cap gate, and bypass global-cap
rejection for replacing writes while retaining the gate for new entries.
Preserve the existing replacement behavior and add a regression test alongside
TestStore_GlobalCapSweepsBeforeRejecting that fills the store to MaxSize,
overwrites an existing key, and asserts success.
---
Nitpick comments:
In `@pkg/metricsmanager/otel/otel_metrics_manager.go`:
- Around line 545-556: Update ReportStateWrite to use a dedicated result-keyed
attribute helper or cache, with the second attribute named "result" instead of
reusing suppressedOption’s "reason" key. Add the corresponding stateWriteCache
field alongside the existing attribute caches, while leaving
ReportStateWriteRejected on the reason-keyed path.
In `@pkg/rulemanager/cel/cel.go`:
- Around line 216-254: Extract the duplicated evaluateProgramWithContext,
nil-result, and type-assertion flow from EvaluateRuleWithContext,
EvaluateBoolExpressionWithContext, and EvaluateStringExpressionWithContext into
one shared helper. Have the helper accept the expression context and a typed
zero value plus a type-assertion callback, preserving cached compile failures as
typed zero results and returning the existing type-mismatch errors; update all
three evaluators to use it.
In `@pkg/rulemanager/cel/libraries/state/accessor.go`:
- Around line 156-161: Update entryToMap so the public CEL field is
scope-neutral rather than exposing every entry’s ScopeID as _container. Rename
the key to an appropriate scope-neutral symbol, or only populate _container when
e.Scope equals armotypes.StateScopeContainer; preserve the existing timestamp
and event-type mappings.
In `@pkg/rulemanager/statewrites/validate.go`:
- Around line 44-48: Update Validate to accept the CEL environment and compile
the key, when, and value expressions during rule loading, rejecting malformed
expressions before runtime. Propagate the environment from the caller and
preserve the existing converted-write behavior for valid expressions.
🪄 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 Plus
Run ID: e6f42675-9045-4640-abf1-b228ac6effb2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (49)
.github/workflows/component-tests.yamldocs/features/cel-rule-state-store.mdgo.modpkg/config/config.gopkg/config/config_test.gopkg/exporters/http_exporter.gopkg/metricsmanager/metrics_manager_interface.gopkg/metricsmanager/metrics_manager_mock.gopkg/metricsmanager/metrics_manager_noop.gopkg/metricsmanager/otel/otel_metrics_manager.gopkg/metricsmanager/prometheus/prometheus.gopkg/objectcache/containerprofilecache/projection_compile_test.gopkg/processtree/ancestors.gopkg/processtree/ancestors_test.gopkg/processtree/process_tree_manager_interface.gopkg/processtree/process_tree_manager_mock.gopkg/rulemanager/cel/cel.gopkg/rulemanager/cel/cel_interface.gopkg/rulemanager/cel/eventtime.gopkg/rulemanager/cel/eventtime_test.gopkg/rulemanager/cel/libraries/state/accessor.gopkg/rulemanager/cel/libraries/state/readtracker.gopkg/rulemanager/cel/libraries/state/statelib.gopkg/rulemanager/cel/libraries/state/statelib_test.gopkg/rulemanager/cel/statewiring_test.gopkg/rulemanager/containercallbacks.gopkg/rulemanager/rule_manager.gopkg/rulemanager/ruleadapters/correlation_test.gopkg/rulemanager/ruleadapters/creator.gopkg/rulemanager/ruleadapters/creator_interface.gopkg/rulemanager/rulecreator/ruleengine_mock.gopkg/rulemanager/statecontext.gopkg/rulemanager/statecontext_test.gopkg/rulemanager/statewrites/executor.gopkg/rulemanager/statewrites/executor_test.gopkg/rulemanager/statewrites/validate.gopkg/rulemanager/statewrites/validate_test.gopkg/rulemanager/types/failure.gopkg/rulemanager/types/v1/rule_embedding_test.gopkg/rulemanager/types/v1/types.gopkg/rulestate/store.gopkg/rulestate/store_test.gopkg/rulestate/types.gopkg/utils/events.gotests/chart/crds/rules.crd.yamltests/component_test.gotests/resources/cel-state-deployment.yamltests/resources/cel-state-rulebinding.yamltests/resources/cel-state-rules.yaml
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
…nts, nil guard Five of CodeRabbit's six findings, with tests for the two that were real correctness bugs. Node scope now gets the larger cap. It is a single node-wide bucket shared by every rule and workload, and PurgeScope is only ever called with a container's scope ID so it is never reclaimed on container removal -- exactly the reasoning that already justified the host bucket's headroom. Bounding it by the per-container cap (256) would have starved node-scoped correlation on a busy node. The global ceiling no longer rejects a write that merely REPLACES an existing key. A replacement does not grow the store, so the per-scope cap already exempted it; the ceiling did not, which meant a rule lost the ability to refresh an established marker exactly when the store was under most pressure -- i.e. when an incident is most likely in progress. The existence peek costs an extra RLock but runs only on the already-degraded path (at the ceiling, nothing reclaimable), so the hot path is unchanged. Apply now guards a nil enriched/Event. podIdentity and processOf already tolerate a nil Event, so without this the very next line panicked instead and the package's nil handling was inconsistent. Not reachable from the rule loop, which dereferences Event earlier, but an exported entry point should not depend on that. getUniqueIdAndMessage no longer shadows err. Behaviour is deliberately unchanged: only the uniqueId error is returned and only it drops the alert, because uniqueId drives cooldown and backend dedup while a failed message costs description only. Dropping a real detection because its text did not render is the worse failure. That asymmetry is now stated in a comment rather than being an accident of shadowing. Docs-exempt: review fixes; no change to the documented rule-authoring surface Signed-off-by: Ben <ben@armosec.io>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed at 811a4826, built and tested locally: go build ./... clean, go test green for pkg/rulestate, pkg/rulemanager/... (incl. statewrites, types/v1, cel/libraries/state, ruleadapters), pkg/processtree, pkg/config.
The design is sound and unusually well documented — writes-after-predicate, receiver-scoped reads that make cross-rule/cross-container access inexpressible, reject-never-evict, and the evaluateRuleAndAlert extraction are all the right calls. The e2e test with positive controls and a negative control is real proof, not theatre. Nothing below is architectural; it's hot-path cost plus a few "silently never fires" gaps of exactly the kind this PR is elsewhere careful about.
Separate non-blocking comments follow with four more items and three questions.
1. gofmt violation, contradicting the PR's testing claim
pkg/metricsmanager/prometheus/prometheus.go is not gofmt-clean on this branch; the same file is clean on main. The new struct block at ~line 106 has one space too many:
stateWritesCounter *prometheus.CounterVec → stateWritesCounter *prometheus.CounterVec
Reproduce with:
gofmt -l $(git diff --name-only origin/main...HEAD | grep '\.go$')2. node_agent_state_entries{scope} is a dead gauge
ReportStateEntries is plumbed through the rulestate.Metrics interface, NoopMetrics, the mock, the OTEL manager and the Prometheus manager, and it is documented in docs/features/cel-rule-state-store.md — but nothing ever calls it. pkg/rulestate/store.go never invokes it, so the gauge never receives a value and the documented metric silently does not exist.
Either emit it from Sweep() (per-scope counts are already in hand there, and it already walks every bucket), or drop it from the interface, both managers and the docs table.
3. Validation runs per event, not at load — so one bad rule floods the log
compileStateWrites is called inside the per-rule loop in ReportEnrichedEvent (pkg/rulemanager/rule_manager.go:361), not once at rule load. Two consequences:
- A rule with a malformed clause hits
logger.L().Error("RuleManager - invalid stateWrites clause...")on every matching event, forever. - A rule with
ttlabovemaxTtlhitslogger.L().Warning("statewrites - clamping ttl...")(pkg/rulemanager/statewrites/validate.go:85) on every matching event.
On a busy node that is thousands of identical lines per second from a single misconfigured rule — a self-inflicted outage risk from a rule an operator can push at any time.
Related: docs/features/cel-rule-state-store.md says "Validation happens at load" and "rejected when the rule loads", which contradicts the code comment directly above compileStateWrites. At minimum, dedupe these two logs per rule ID — the alertLogDedup expirable LRU already in rule_manager.go is the pattern — and align the doc with what the code does.
4. Per-rule, per-event allocations on the hot path for rules that never use state
stateTracker.Reset() and rm.seedStateContext(...) run unconditionally for every rule that survives the prefilter (rule_manager.go:392-393). seedStateContext allocates a statewrites.ScopeIDs() map plus an Accessor per rule, per event — so a node running 40 rules pays ~80 allocations per event for a feature almost none of them use. The "keeps the common no-state path to a single allocation" comment covers the tracker, but not this.
Gating on len(stateScopes) > 0 makes the no-state path free. The only cost is that a rule reading a name it never declared would get an unknown-variable eval error instead of a silent miss — arguably the better failure anyway, and consistent with the load-time-rejection philosophy elsewhere in this PR.
Same argument applies to compileStateWrites itself: caching per rule version rather than re-parsing durations on every event would remove the rest of it.
5. Store.Set has a performance cliff at the global ceiling
Once size >= MaxSize, every write calls Sweep(), which write-locks all 16 shards in turn and walks up to MaxSize (100k default) entries — with no rate limit:
if s.currentSize() >= s.cfg.MaxSize {
if s.Sweep() == 0 {
...
}
}At the ceiling with nothing expirable, a container writing at any rate turns each write into an O(n) all-shard stall, and the rule loop is concurrent, so it stalls every worker. Reaching 100k is plausible on a dense node: ~300 containers × 256, plus pod scopes, plus the node and host buckets.
An atomic.Int64 holding the last sweep time, so at most one sweep per sweepInterval runs from the write path, keeps the backstop without the cliff.
6. currentSize() takes a global mutex on every write
sizeMu is acquired on every Set, which partly defeats the shard design the store's own doc comment is built around. An atomic.Int64 instead of sizeMu + int removes the contention point outright and simplifies addSize/currentSize to one-liners.
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Four more items — non-blocking, but 8 in particular is worth a decision rather than a shrug.
7. ReportRuleProcessed semantics did drift, in the one case the refactor didn't consider
The refactor is otherwise faithful — the processed bool reproduces the old control flow exactly for eval errors, cooldown, getUniqueIdAndMessage failure and a nil ruleFailure. But:
processed := true
if len(ruleExpressions) > 0 {
processed = rm.evaluateRuleAndAlert(...)
}means a write-only leg — a rule with no ruleExpression for this event type — now increments ReportRuleProcessed, where the old loop continued at len(ruleExpressions) == 0 and never counted it. That inflates the counter for exactly the new case this PR introduces.
processed := len(ruleExpressions) > 0 restores the pre-refactor meaning.
8. The prefilter can silently eat a write leg
The table in docs/features/cel-rule-state-store.md honestly lists prefilter / policy / profile-dependency as suppressing writes, so this is a known choice rather than an oversight. It still deserves a second look, because rule.Prefilter is built from the rule as a whole — in practice mostly from the alerting leg's params.
Concretely: a correlation rule whose network leg carries ignorePrefixes or excludeProcesses will have those same params applied to its exec event, silently dropping the write. The chain then never forms, with no signal anywhere — precisely the failure class the write-after-cooldown ordering was designed to prevent, arriving through a different door.
If the behaviour stays as documented, please at least make it visible: state_write_rejected_total{reason="prefiltered"} turns an invisible failure into a countable one, and it slots straight into step 4 of the "When a correlation rule does not fire" list.
9. Pod-scope buckets are never purged
PurgeScope is only ever called from ContainerCallback, with a container scope ID. p:<ns>/<pod> entries therefore survive pod deletion until TTL — and scopeCap gives them MaxEntriesPerContainer (256) rather than the larger host cap, since the larger cap is reserved for c:__host__ and node scope.
So pod scope is both un-reclaimed on churn and on the smaller cap, which makes it the most likely bucket to hit ErrScopeCapReached on a busy node. Either purge pod scope when the pod's last container goes, or document it next to the existing host-bucket caveat so the behaviour is at least expected.
10. Store.Get's scope parameter is unused
func (s *Store) Get(ruleID string, _ armotypes.StateScope, scopeID, name, key string) (*Entry, bool)An ignored scope argument on the read path of a scope-keyed store invites a future bug where someone passes a scope that disagrees with scopeID and assumes it is checked. Drop it, or assert it against the scope encoded in scopeID.
|
Three questions, none of them blocking — but each is a place where the answer being "no" is invisible. Correlation evidence only reaches
|
2 participants
Summary
Gives CEL rules memory across events via a declarative
stateWrites:clause andstate.has/state.getread functions, so cross-event detections (exec → network, webshell chains, create/exec/delete) become expressible for the first time. Proven end-to-end on kind against real eBPF, not just unit tested.Blocking follow-up: the canonical
RulesCRD inkubescape/helm-chartsmust declarestateWrites, or the API server silently prunes it and the feature is inert in production. Details below.What this adds
CEL rules can currently only be pure predicates over a single event. This gives them memory across events, so a rule can remember a fact on one stream and read it back on another —
exec→network, webshell chains, create/exec/delete of a pod. None of those are expressible today.A rule declares what it remembers in a new
stateWrites:clause and reads it back withstate.has(...)/state.get(...):Design doc:
shared-designs-and-docs/projects/2026-07-28-cel-rule-state-store/spec.md.User-facing docs:
docs/features/cel-rule-state-store.md(in this PR) — the best entry point for review.Verified end-to-end, not just unit tested
Test_36_CelStateStoreCorrelationruns on kind against real eBPF events and passes in ~137s. The alert it produces:Same pid on both legs, and
remembered=shis thevalue:written on the exec leg read back throughstate.get(...)in the network leg's message. A negative control (reading a name no rule writes) produces zero alerts, so the positive result is not vacuous.The test also wires
Test_35_ExecTTYFieldTestinto CI, which was written earlier but never added to the matrix.The canonical
RulesCRD inkubescape/helm-chartsmust declarestateWrites.The CRD has a structural schema with no
x-kubernetes-preserve-unknown-fieldsat the rule level, so the API server silently prunes the clause. It fails in the worst possible way:kubectl applysucceeds, the rule loads, and it never fires — no error in kubectl, node-agent logs, or metrics.--validate=falsedoes not help (client-side only).This PR fixes only
tests/chart/crds/rules.crd.yaml, which is a test-only copy. Merging this without the helm-charts change ships a feature that is inert in production. Verify with:Empty output after a successful apply means it is still being pruned.
Design decisions worth a reviewer's attention
stateis a CEL variable with member functions, not astate.*function namespace. cel-go hands a function binding only its arguments, never the activation, so a globalstate.hascould not discover which rule or container it was evaluating for. Putting that context in a receiver is also the security property: the rule ID, scope IDs and ancestor list live there and no CEL syntax can supply or override them, so reading another rule's or another container's state is inexpressible, not merely forbidden.Writes are declarative, never a CEL setter. A setter inside a predicate could be skipped by short-circuiting, reordered by the static optimiser, and could never express "remember without alerting" — which is exactly what the first leg of every cross-event rule needs.
Writes run after the predicate, so a predicate only ever sees state from earlier events. This required extracting the per-rule body of the event loop into
evaluateRuleAndAlert(pkg/rulemanager/rule_manager.go) so its early exits arereturnrather thancontinue— otherwise a cooldown-suppressed alert would also skip the write and break the next leg of the chain. It returns a bool soReportRuleProcessedkeeps its exact pre-refactor meaning.Caps reject writes, never evict. Eviction would let one container silently disable detection for its neighbours. Host processes get their own
c:__host__bucket with a larger cap, since it holds the whole node's process space and never receives a container-removal purge.typesv1.Rulenow embedsarmotypes.RuntimeRuleso the CRD contract has one definition shared with the operator.ExpressionsandProfileDataRequiredstay shadowed because their types genuinely differ. Note the two decoders disagree:encoding/jsonresolves same-tag conflicts by depth, while apimachinery's converter (the production CRD path) has no depth rule and fills both. Both are pinned byrule_embedding_test.go.Reviewing a 50-file diff
Suggested order:
docs/features/cel-rule-state-store.md— what it does and how it failspkg/rulestate/— the store; no CEL or rule knowledge, readable standalonepkg/rulemanager/statewrites/— validation and executionpkg/rulemanager/cel/libraries/state/— the read functions and the receiverpkg/rulemanager/rule_manager.go— the riskiest change, the loop refactortests/+tests/chart/crds/rules.crd.yaml— the end-to-end proofTesting
go build ./...clean; full unit suite green.go test -race ./pkg/rulestate/... ./pkg/rulemanager/...clean.gofmtviolations relative tomain.main—containerwatcher/v2/tracersandpkg/validator— both needing host/eBPF prerequisites. Unrelated to this change.Not in this PR
correlations[]evidence payload in the component test. Alertmanager labels are a flat string map and structurally cannot carry a nested array, so that needs an in-cluster receiver. Planned, optional, and does not gate this.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Tests