SBOB wildcards: DNS + IP + path exec wildcards, sign/tamper #810
SBOB wildcards: DNS + IP + path exec wildcards, sign/tamper #810entlein wants to merge 4 commits into
Conversation
….7%/-12.4% memory) Signed-off-by: entlein <einentlein@gmail.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (90)
✨ 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 |
|
@CodeRabbit review this together with kubescape/storage#326 please |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 27018 🏁 Script executed: Length of output: 38600 🏁 Script executed: Length of output: 43360
🔴 High – should address before individual PRs are merged[storage
|
|
Dear @CodeRabbit,
Will wait for matthyx general direction of how he wants me to address all the other points . Happy Weekend :) |
|
Tip For best results, initiate chat on the files or code changes.
Happy to re-evaluate once matthyx weighs in on direction. Happy weekend! 🐇 ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes.
Enjoy the weekend, and feel free to ping me again once matthyx shares direction on the remaining medium items! 🐇 |
…, cache, opens/exec CEL) Signed-off-by: entlein <einentlein@gmail.com>
|
┌───────────────────┬──────────────────────────────┬───────────┬───────┐ Numbers shrank from the earlier -11.7%/-12.4% (commit 8b34f59) after the ctx-prop revert |
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
|
@coderabbitai review Collective re-review request after applying your atomic-split advisory + rabbit-feedback fixes across the full SBOB upstream PR set. Each PR is now scoped to a single tier per your dependency map: Tier 0 (opens):
Tier 2 (execs):
Tier 3 (network):
Tier 4 (tamper/sign):
Umbrellas:
Please confirm atomicity per your earlier advisory and flag any remaining cross-PR coupling we missed. |
🧩 Analysis chain🏁 Script executed: Length of output: 10027 🏁 Script executed: Length of output: 5186
✅ Atomicity confirmation by tiergraph TD
subgraph storage
S322["`#322` CompareExecArgs"]
S323["`#323` path-wildcards core"]
S324["`#324` networkmatch pkg"]
S325["`#325` CollapseConfig CRD"]
S326["`#326` umbrella"]
S322 --> S323
S323 --> S325
S322 --> S326
S323 --> S326
S324 --> S326
S325 --> S326
end
subgraph node-agent
NA811["`#811` was_path_opened* CEL"]
NA805["`#805` resolveExecPath + parse"]
NA807["`#807` R0040 args consumer"]
NA812["`#812` ExecsByPath surface"]
NA806["`#806` network wildcards CEL"]
NA808["`#808` projection + tamper + fanout"]
NA809["`#809` cosign sign/verify"]
NA810["`#810` umbrella"]
NA811 --> NA807
NA805 --> NA807
NA808 --> NA809
NA808 --> NA812
NA805 --> NA810
NA806 --> NA810
NA807 --> NA810
NA808 --> NA810
NA809 --> NA810
NA811 --> NA810
NA812 --> NA810
end
S323 --> NA807
S324 --> NA806
S325 --> NA808
🔴 Three atomicity violations — file-level conflicts between sibling PRsThese three require resolution before the atomic PRs can be merged independently: 1.
|
| PR | Tier | Files touched |
|---|---|---|
#811 |
0 | applicationprofile/ap.go, applicationprofile/open.go |
#807 |
2 | applicationprofile/exec.go, applicationprofile/open.go, exec_test.go, open_test.go |
Fix: #807 must be rebased on top of #811 (Tier 0 merges first, Tier 2 picks up the open.go changes). The current graph already has #811 → #807``, so the rebase is the right action — just needs to be done explicitly.
2. tamper_alert.go + tamper_alert_test.go — shared by #808 (Tier 4) and #809 (Tier 4)
| PR | Files touched |
|---|---|
#808 |
containerprofilecache/tamper_alert.go, containerprofilecache/tamper_alert_test.go (+ projection/cache infra) |
#809 |
containerprofilecache/tamper_alert.go, containerprofilecache/tamper_alert_test.go (+ full pkg/signature/ package) |
Fix options (pick one):
- Option A: Keep tamper infrastructure (struct + non-cosign path) entirely in
#808and striptamper_alert.*from#809, having#809only add cosign wiring via the interface defined in#808. - Option B: Move all tamper logic (including cosign wiring) into
#809and removetamper_alert.*from#808entirely. Then#808merges first without tamper alert code, and#809adds it in full.
Option A is cleaner for atomicity (infra in #808, cosign-specific wiring in #809) since #808 already has the alert structure.
3. projection_apply.go + projection_types.go — shared by #808 (Tier 4) and #812 (Tier 2)
| PR | Files touched |
|---|---|
#808 |
projection_apply.go, projection_types.go, containerprofilecache.go, projection.go, ... |
#812 |
projection_apply.go, projection_apply_test.go, projection_types.go |
This is the most problematic one: #812 (Tier 2, ExecsByPath) modifies the same projection files as #808 (Tier 4). This is a tier ordering violation — Tier 2 should not depend on Tier 4 infrastructure.
Fix: Either:
- Option A: Promote
#812into Tier 4 (merge after#808), since it adds to the projection surface that#808defines. Update the tier chart accordingly. - Option B: Move the
projection_types.go/projection_apply.gobase shape (the type definitionsExecsByPathneeds) out of#808into a new earlier PR or into#807, so#812can sit cleanly in Tier 2.
✅ Clean PRs — no intra-tier coupling
| PR | Assessment |
|---|---|
| storage#322 | ✅ Atomic — only compare_exec_args.go + its own test |
| storage#323 | ✅ Atomic — path-wildcards core; depends on #322 (expected) |
| storage#324 | ✅ Atomic — full networkmatch package, self-contained |
| storage#325 | ✅ Atomic — CollapseConfig CRD; depends on #323's types.go (expected) |
| node-agent#805 | ✅ Atomic — parse/ library + event_reporting.go only |
| node-agent#806 | ✅ Atomic — network.go + 20 YAML fixtures; depends on storage#324 (expected) |
Revised merge order
storage: `#322` → `#323` → `#324` (parallel) → `#325` → `#326`
↓ ↓
node-agent: `#811` → `#805` (parallel) → `#807` `#808` → `#809`
↓ ↓
`#806` `#812` (resolve tier conflict first)
↓
`#810`
The three violations above are the only blockers to being able to cherry-merge the atomic PRs independently. Happy to look at the specific diff hunks for any of them if it helps decide which split approach to take. 🐇
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
Umbrella PR consolidating the SBOB-wildcards feature surface for node-agent. Tier-by-tier breakdown per CodeRabbit's atomic-split advisory on
kubescape/storage#323:kubescape/node-agent#811(was_path_opened* CEL helpers)kubescape/node-agent#805(resolveExecPath + 3-arg parse),#807(R0040 args consumer),#812(ExecsByPath projection surface)kubescape/node-agent#806(network wildcards CEL)kubescape/node-agent#808(projection overlay + tamper R1016 + non-blocking fanout + NN resource),#809(cosign sign/verify + tamper detection)This umbrella carries the union of the above, plus the projection-feature chart restorations and cross-theme component tests (Test_29..Test_33) for end-to-end coverage. If the maintainer prefers to cherry-merge the small per-feature PRs, this umbrella shrinks naturally as each one lands.
Performance evidence
Validated on workflow run 25959674209:
Storage companion
Storage umbrella:
kubescape/storage#326. Storage and node-agent run on different syft chains by design (storage onanchore/syft v1.42.4, node-agent onkubescape/syft v1.32.0-ks.2via replace) per the upstream-NA decision in PR #798 — see node-agent issue #45 for context on the moby/moby umbrella vsmoby/moby/clientsubmodule transitive coupling that gates a single-chain alignment.Notes
CompareDynamicperf: rabbit flagged the recursivesplitPathallocation pattern onkubescape/storage#326; matthyx indicated earlier he'd own the zero-alloc rewrite. We've not touched it here.was_path_opened_with_flagsCEL helper restored after we initially removed it — it's a public CEL surface, removing was a mistake.isTriggerAlert, R0011enabled+isTriggerAlert, R0008uniqueId) are flagged in the audit document and open for discussion.