Skip to content

feat(projection): add ExecsByPath composite-key surface to ProjectedContainerProfile#812

Open
entlein wants to merge 1 commit into
kubescape:mainfrom
k8sstormcenter:upstream-pr/execs-na-execsbypath
Open

feat(projection): add ExecsByPath composite-key surface to ProjectedContainerProfile#812
entlein wants to merge 1 commit into
kubescape:mainfrom
k8sstormcenter:upstream-pr/execs-na-execsbypath

Conversation

@entlein
Copy link
Copy Markdown

@entlein entlein commented May 16, 2026

Tier 2 in the atomic-split decomposition CodeRabbit advised on storage#323 ("Dependency map across all 8 PRs"). This PR carries ONLY the minimal projection surface needed to unblock node-agent#807 (R0040 args consumer), so #807 doesn't have to wait for the much larger node-agent#808 to land.

What's in this PR

Two surgical additions and one set of unit tests:

  1. pkg/objectcache/projection_types.go — adds ExecsByPath map[string][]string to ProjectedContainerProfile. Keyed by Exec.Path (same key used in Execs.Values / Execs.Patterns). Carries the per-Path Args slice from cp.Spec.Execs so downstream consumers can run argv-aware matching against the projected profile.

  2. pkg/objectcache/containerprofilecache/projection_apply.go — populates the new field inside Apply and adds the extractExecsByPath helper. The helper clones each Args slice (Apply is contract-bound to be a pure transform — aliasing would let a consumer mutate the source profile by editing the projected map). Nil-Args entries are stored as empty (non-nil) slices; downstream matchers distinguish "path absent" (key missing) from "path present, ran with no args" (key present, value []string{}).

  3. pkg/objectcache/containerprofilecache/projection_apply_test.go — two new tests pin the contract: TestApply_ExecsByPath_PopulatesFromSpec covers last-write-wins on duplicate Paths, nil→empty-slice projection, and the cloned-slice invariant (mutating the projection MUST NOT propagate to the source). TestApply_ExecsByPath_NilWhenSpecEmpty pins the nil-for-empty convention.

Why split it out

Node-agent#808 currently bundles four independent concerns: ExecsByPath field, tamper detection (R1016), non-blocking notification fan-out, and NetworkNeighborhood resource support. The ExecsByPath piece is ~7 lines of struct field + the helper + tests — it's the SINGLE BLOCKER for node-agent#807 to compile. Extracting it as this micro-PR cuts #807's blast radius from "wait on a 1300-line PR" to "wait on this micro-PR".

After this lands, node-agent#808 narrows to (R1016 + fanout + NN resource), which are all independent of #807 and can each land in their own time.

Dependencies

  • Conceptual: aligns with the CollapseConfig direction in storage#323b / storage#325 but does NOT import any new storage symbols. Compiles against today's storage main.
  • Required-by: node-agent#807 (R0040 args consumer) depends on this field being present.

Cross-references

…ontainerProfile

Signed-off-by: entlein <einentlein@gmail.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

Warning

Rate limit exceeded

@entlein has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 58 minutes and 40 seconds before requesting another review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7268a41a-9476-4abc-b50d-61f0a50fc2e3

📥 Commits

Reviewing files that changed from the base of the PR and between cc59fa0 and 58ce8d9.

📒 Files selected for processing (3)
  • pkg/objectcache/containerprofilecache/projection_apply.go
  • pkg/objectcache/containerprofilecache/projection_apply_test.go
  • pkg/objectcache/projection_types.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

@entlein
Copy link
Copy Markdown
Author

entlein commented May 16, 2026

@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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR `#812` details and diff
gh pr view 812 --repo kubescape/node-agent --json title,body,files,baseRefName,headRefName,state

# List changed files
gh pr diff 812 --repo kubescape/node-agent --name-only

# Get the actual diff for review
gh pr diff 812 --repo kubescape/node-agent

Length of output: 9271


@entlein — full diff reviewed. Atomicity of #812 is confirmed: purely additive, zero coupling beyond being the declared dependency of #807. A few things to address before merge:


pkg/objectcache/projection_types.go

Field placementExecsByPath is inserted after IngressAddresses and before SpecHash/SyncChecksum, separated from the logical exec group. Grouping it with other exec-surface fields improves readability and makes the struct self-documenting:

// near Execs / exec-related fields
Execs           ProjectedField
ExecsByPath     map[string][]string   // add here, not after ingress fields

pkg/objectcache/containerprofilecache/projection_apply.go

Last-write-wins data loss — this is the most material concern. When two ExecCalls entries share a Path but carry different Args slices, only the second survives in ExecsByPath, while Execs.Values (fed by extractExecsPaths) retains both entries. A consumer that gates on Execs ("is this path allowed?") and then looks up ExecsByPath ("what args were seen?") will silently miss the first entry's args. Consider one of:

  1. Accumulate — change the value type to [][]string (path → list of observed arg slices) so no entry is dropped. Breaking change to the field type, but catches the semantic mismatch cleanly.
  2. Union — flatten all args across duplicates into one []string, deduped. Lossy but simple.
  3. Document explicitly and let callers decide. If the storage layer guarantees at most one ExecCalls per path per profile, add an invariant comment + a test that asserts extractExecsPaths and extractExecsByPath produce the same key-set cardinality.

Capacity hintmake(map[string][]string, len(cp.Spec.Execs)) is a valid upper-bound hint even when duplicates collapse. No change needed, but worth a comment so future readers don't "fix" it.


pkg/objectcache/containerprofilecache/projection_apply_test.go

sourceCopy ordering dependency — in TestApply_ExecsByPath_PopulatesFromSpec, sourceCopy is captured after Apply() returns:

pcp := Apply(&objectcache.RuleProjectionSpec{}, cp, nil)
// ...
sourceCopy := append([]string{}, cp.Spec.Execs[2].Args...) // ← post-Apply
pcp.ExecsByPath["/bin/sh"][0] = "MUTATED"
assert.Equal(t, sourceCopy, cp.Spec.Execs[2].Args, ...)

This is currently safe because Apply is documented as a pure transform and does not mutate cp. But the test's clone-invariant check is supposed to verify that purity; capturing sourceCopy after Apply makes the test pass vacuously if Apply had already mutated the source. Capture before Apply to make the invariant self-proving:

// Capture BEFORE Apply so the clone-invariant check is non-vacuous.
sourceArgs := append([]string{}, cp.Spec.Execs[2].Args...)

pcp := Apply(&objectcache.RuleProjectionSpec{}, cp, nil)
require.NotNil(t, pcp)

pcp.ExecsByPath["/bin/sh"][0] = "MUTATED"
assert.Equal(t, sourceArgs, cp.Spec.Execs[2].Args,
    "mutating the projected slice must not propagate to the source profile (cloned, not aliased)")

Tier-map atomicity confirmation

PR Verdict Notes
#812 (this) ✅ atomic Purely additive; no new imports; no changes to existing behaviour
#807#812 ✅ clean dep #807 needs ExecsByPath present; #812 is the sole provider
#808#812 ⚠️ needs rebase note #808 must drop its own ExecsByPath addition to avoid a merge conflict after #812 lands; confirm #808's PR description calls this out
#812 → storage ✅ no coupling No new storage symbols imported; compiles against storage main today

No other cross-PR coupling found in the files touched by #812.

✅ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant