profile-compaction: CollapseConfig CRD + projection overlay + user-ma…#808
Open
entlein wants to merge 1 commit into
Open
profile-compaction: CollapseConfig CRD + projection overlay + user-ma…#808entlein wants to merge 1 commit into
entlein wants to merge 1 commit into
Conversation
…naged lifecycle Signed-off-by: entlein <einentlein@gmail.com>
📝 WalkthroughWalkthroughThis PR extends container profile caching with tamper detection for user-supplied overlays, refactors projection to classify dynamic/wildcard patterns and support exec-arguments matching, adds overlay-identity stamping into cache checksums, and refactors rule-binding cache to dispatch notifications without holding locks. ChangesContainer Profile Cache: Tamper Detection, Overlay Identity, and Projection
Rule Binding Cache: Non-Blocking Notification Dispatch
Safety Fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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/objectcache/containerprofilecache/containerprofilecache.go`:
- Around line 412-414: The verification calls to
c.verifyUserApplicationProfile(userAP, sharedData.Wlid) currently ignore its
boolean result; update both call sites (the one using userAP at lines ~412 and
the similar one around ~428-430) to check the returned bool and, when it is
false and the cache is running in strict verification mode (e.g., the instance
flag controlling strict verification such as c.strictVerify or equivalent),
avoid projecting/merging the overlay (skip the merge/projectOverlay path), log
the verification failure with context (Wlid and which overlay), and return or
surface an error instead of continuing; if not in strict mode, continue but log
a warning. Ensure you reference and use c.verifyUserApplicationProfile(...) and
the strict-mode flag when implementing the conditional.
🪄 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: acff329b-9e89-4538-a272-7877e0ad1e70
📒 Files selected for processing (12)
pkg/objectcache/containerprofilecache/containerprofilecache.gopkg/objectcache/containerprofilecache/projection.gopkg/objectcache/containerprofilecache/projection_apply.gopkg/objectcache/containerprofilecache/tamper_alert.gopkg/objectcache/containerprofilecache/tamper_alert_test.gopkg/objectcache/containerprofilecache/test32_projection_test.gopkg/objectcache/projection_types.gopkg/objectcache/shared_container_data.gopkg/objectcache/v1/mock.gopkg/rulebindingmanager/cache/cache.gopkg/rulebindingmanager/cache/cache_test.gopkg/rulemanager/cel/libraries/cache/function_cache.go
Comment on lines
+412
to
+414
| if userAP != nil { | ||
| c.verifyUserApplicationProfile(userAP, sharedData.Wlid) | ||
| } |
There was a problem hiding this comment.
Honor verification result before merging user overlays.
Line 413 and Line 429 call verification but ignore the boolean outcome. In strict mode, failed verification should prevent projecting that overlay; otherwise tampered/failed overlays are still merged.
Suggested fix
if userAP != nil {
- c.verifyUserApplicationProfile(userAP, sharedData.Wlid)
+ if !c.verifyUserApplicationProfile(userAP, sharedData.Wlid) {
+ userAP = nil
+ }
}
@@
if userNN != nil {
- c.verifyUserNetworkNeighborhood(userNN, sharedData.Wlid)
+ if !c.verifyUserNetworkNeighborhood(userNN, sharedData.Wlid) {
+ userNN = nil
+ }
}Also applies to: 428-430
🤖 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/objectcache/containerprofilecache/containerprofilecache.go` around lines
412 - 414, The verification calls to c.verifyUserApplicationProfile(userAP,
sharedData.Wlid) currently ignore its boolean result; update both call sites
(the one using userAP at lines ~412 and the similar one around ~428-430) to
check the returned bool and, when it is false and the cache is running in strict
verification mode (e.g., the instance flag controlling strict verification such
as c.strictVerify or equivalent), avoid projecting/merging the overlay (skip the
merge/projectOverlay path), log the verification failure with context (Wlid and
which overlay), and return or surface an error instead of continuing; if not in
strict mode, continue but log a warning. Ensure you reference and use
c.verifyUserApplicationProfile(...) and the strict-mode flag when implementing
the conditional.
entlein
pushed a commit
to k8sstormcenter/node-agent
that referenced
this pull request
May 16, 2026
…verlay CodeRabbit upstream PR kubescape#808 / containerprofilecache.go:414 (Major). The verifyUserApplicationProfile and verifyUserNetworkNeighborhood methods already return a boolean reflecting verification outcome — true when the overlay is unsigned OR when verification succeeded OR in permissive mode (EnableSignatureVerification=false); false only in strict mode on actual tamper. The two call sites in projection-load were discarding that return, so tampered overlays in strict mode silently merged anyway. The R1016 alert was emitted but the protection was advisory only. Now: when verify returns false (strict mode + tamper detected) the overlay is nilled out before the merge step so the cache never projects a known-tampered profile. Permissive mode is unchanged — verify always returns true, the overlay still merges, R1016 still fires. New tests: - TestVerifyAP_StrictMode_ReturnsFalseOnTamper — sign + tamper an ApplicationProfile, construct a cache with EnableSignatureVerification=true, and assert verifyUserApplicationProfile returns false (caller drops overlay). - TestVerifyNN_StrictMode_ReturnsFalseOnTamper — symmetric pin for the NetworkNeighborhood path. The existing legacy-permissive tamper test (TestVerifyAP_TamperedProfile_PopulatesDedupMap) continues to pass unchanged — that path still returns true with the R1016 emitted.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
here things got confusing during the rebase, this is the sister PR to storage and need the signature PR (from matthyx)
Summary by CodeRabbit
New Features
Bug Fixes
Tests