fix: correct IgnoreRuleBindings handling for container monitoring - #839
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 56 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughIn ChangesSkip unregistration when IgnoreRuleBindings is enabled
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/containerwatcher/v2/containercallback.go`:
- Around line 190-192: Add test cases to the existing test suite in
`container_watcher_private_test.go` that specifically cover the new early return
behavior in the `unregisterContainer` method when `IgnoreRuleBindings`
configuration is true. Create test scenarios that verify the early return
prevents container removal, confirm the container remains in the
`containerCollection`, and ensure shared container data is not deleted when this
configuration flag is enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2aa352c2-2fa6-4bda-a619-fb75c43fb189
📒 Files selected for processing (1)
pkg/containerwatcher/v2/containercallback.go
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
slashben
left a comment
slashben
left a comment
There was a problem hiding this comment.
Confirmed this fixes the bug. Root cause: in IgnoreRuleBindings mode ruleManagedPods is never populated (it's only fed from binding notifications), so when a container's ApplicationProfile reaches end-of-life the EOL-driven unregisterContainer tears down monitoring — detection silently stops after learning completes. Returning early when the flag is set keeps the container monitored. On the production config (EnableRuntimeDetection=true) this behaves the same as a guard placed inside the runtime-detection branch.
A few comments:
1. (Should-fix) Add a regression test. This is a silent-failure bug — detection just stops, no error — so it's exactly the kind that regresses unnoticed. The existing TestUnregisterContainer table covers it with one case (add an ignoreRuleBindings field to the table struct + wire it into the config.Config{...}):
{
name: "Keep container in ignoreRuleBindings mode with no matching binding",
unregisterContainer: "container1",
unregisterContainersPod: "pod1",
podToContainers: map[string][]string{"pod1": {"container1"}, "pod2": {"container2"}},
preRuleManagedPods: []string{}, // empty: no bindings
expectedContainers: []string{"container1", "container2"}, // both kept
enableRuntimeDetection: true,
ruleBindingsInitialized: true,
ignoreRuleBindings: true,
},Fails on main, passes with this fix.
2. (Nit) Add a one-line comment. A bare if cw.cfg.IgnoreRuleBindings { return } doesn't tell the next reader why — one sentence ("in this mode every pod is evaluated against all rules, so ruleManagedPods stays empty and a container must never be unregistered for lacking a binding") would help.
3. (Question, not a blocker) Effect on the excluded-namespace path. unregisterContainer is also called from the IgnoreContainer branch in containercallback.go (the excludeNamespaces/labels cleanup for kube-system, kubescape, the agent itself). With this early-return, in IgnoreRuleBindings mode those ignored containers are no longer explicitly removed from the container collection. Likely benign — the monitoring callbacks are already skipped for ignored containers — but since armo mode runs with IgnoreRuleBindings=true in production, worth confirming excluded namespaces don't start lingering/monitored. If there's any doubt, scoping the guard inside the if cw.cfg.EnableRuntimeDetection block confines it to the actual buggy decision.
Net: LGTM with the test added; #2/#3 are polish.
Validated end-to-end on a live cluster (chart sets IGNORERULEBINDINGS=true; with no binding, attack-suite containers were unregistered at profile-EOL and a command-injection produced zero detections; restoring monitoring brought the alerts back).
Why CI didn't catch this (component-test blind spot)
The component-test chart sets two things together:
This bug only triggers with
CI runs the one config where the binding hides the bug; the shipped chart default runs the one config where it bites. Suggested coverage: a component-test variant with |
d643bad to
0386943
Compare
matthyx
changed the title
|
Thanks @slashben — addressed your review: #3 (excluded-namespace path / scope concern): Confirmed real, and fixed exactly as you suggested. The #1/#2 (regression test + comment): done (replied inline). While in here I also stopped the now-dead rule-binding work in this mode: RBCache no longer watches CI blind spot: great catch — left as a noted follow-up in the PR description (component-test variant with |
The previous fix short-circuited the entire unregisterContainer when IgnoreRuleBindings was true, which also disabled removal of explicitly ignored containers and cleanup of learning-complete containers when runtime detection is off. Scope the behavior correctly: - unregisterContainer: only treat every pod as rule-managed (keep monitoring) inside the runtime-detection block when IgnoreRuleBindings is set; extract the actual removal into removeContainer. - containerCallback: ignored containers now call removeContainer directly so they are always dropped regardless of rule bindings. Also stop the wasted rule-binding work when bindings are ignored: - Do not watch RuntimeAlertRuleBinding objects (RBCache is the sole watcher), removing the misleading "refreshed rule bindings rules" log. - RefreshRuleBindingsRules skips the per-binding rebuild but still notifies consumers so RuleManager keeps recompiling the projection spec on rule changes. - Skip pod->binding bookkeeping in the RBCache handlers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
0386943 to
3713b72
Compare
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
2 participants
What
Corrects how
IgnoreRuleBindingsis handled across container monitoring and the rule-binding cache.Background / root cause
In
IgnoreRuleBindingsmode every pod is evaluated against the full rule set, soruleManagedPodsis never populated (it is only fed from binding notifications). When a container's ApplicationProfile reaches end-of-life, the EOL-drivenunregisterContainertore down monitoring — detection silently stopped after learning completed. Validated on a live cluster (chart setsIGNORERULEBINDINGS=true; with no binding, attack-suite containers were unregistered at profile-EOL and a command-injection produced zero detections).The fix
The original one-line fix short-circuited the entire
unregisterContainer, which also disabled removal of explicitly ignored containers (excludeNamespaces/labels, e.g. kube-system) and cleanup of learning-complete containers when runtime detection is off. This PR scopes the behavior correctly:unregisterContainer— theIgnoreRuleBindingsearly-return is placed inside theEnableRuntimeDetectionblock (the actual buggy decision), treating every pod as rule-managed so the container keeps being monitored. The real removal is extracted intoremoveContainer.containerCallback— ignored containers now callremoveContainerdirectly, so they are always dropped regardless of rule bindings / runtime detection.Stop wasted rule-binding work when bindings are ignored
resourcesToWatchno longer watchesRuntimeAlertRuleBindingobjects (RBCache is the sole watcher), which also removes the misleadingRBCache - refreshed rule bindings ruleslog.RefreshRuleBindingsRulesskips the per-binding rule rebuild but still notifies consumers, soRuleManagerkeeps recompiling the profile projection spec onRuleCRD changes.addPod/deletePod) since it is never read in this mode.Tests
TestUnregisterContainer: new case keeping a container inignoreRuleBindingsmode with no matching binding (fails onmain, passes here).TestResourcesToWatch: new case asserting rule bindings are not watched when ignored.go build ./...clean;pkg/rulebindingmanager/...,pkg/rulemanager,pkg/containerwatcher/v2green.Docs
docs/CONFIGURATION.md— expanded theignoreRuleBindingsentry to describe the now-explicit behavior.Follow-up (not in this PR)
Component-test blind spot: the test chart ships an unconditional
all-rules-all-podsbinding, soruleManagedPodsis always populated and the bug is masked. A component-test variant withignoreRuleBindings=trueand no binding would close the gap.