Chore/retire headless proc regex in favor of opens-gadget fix -- remove band-aid from last week - #889
Merged
matthyx merged 4 commits intoAug 14, 2026
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a locally built eBPF gadget that traces ChangesTrace open path attribution
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This change replaces the temporary path-handling workaround with a targeted gadget fix and adds coverage for relative opens; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant RelativeOpenWorkload
participant trace_open
participant filesystem_helpers
participant ApplicationProfile
RelativeOpenWorkload->>trace_open: Call open/openat with relative paths
trace_open->>filesystem_helpers: Resolve cwd or directory fd
filesystem_helpers-->>trace_open: Return absolute path
trace_open->>ApplicationProfile: Record successful and failed open paths
ApplicationProfile-->>RelativeOpenWorkload: Expose learned paths for validation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
entlein
force-pushed
the
chore/retire-headless-proc-regex
branch
2 times, most recently
from
August 12, 2026 17:01
766a411 to
60d0d2f
Compare
trace_open resolved a full path only from the descriptor a successful open returned, so a relative open had no absolute path and a failed open had no descriptor at all. Userspace fell back to the raw relative name, which was then promoted to a bogus root: a process chdir'd into a directory and opening base/<oid>/<relfile> or backup_label, or speculatively probing files that do not exist yet, was recorded as /base/<oid>/<relfile> or /backup_label. Carry the dirfd through to the exit probe (AT_FDCWD for open, the openat argument otherwise) and, when the resolved path is empty and the name is relative, join the name against its base -- the process cwd for AT_FDCWD, else the dirfd's path -- using the existing dentry walk. This runs regardless of the syscall return value, so failed opens resolve too. The empty-walk case in get_path_str now returns NULL instead of a pointer into the never-cleared per-cpu scratch buffer, and the failed-open branch clears fpath. trace_open is vendored from IG v0.48.1 and built from source under the same image name node-agent pins, with the builder image pinned so the build is reproducible across ig versions. Test_43_RelativeOpenPathResolution learns a chdir'd relative-open workload and asserts the resolved absolute paths, no fabricated roots, and that the failed open resolves. Resolves kubescape#874 Signed-off-by: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com>
The gadget now resolves relative opens against their dirfd/cwd, so /proc/<pid> paths arrive already rooted. The headlessProcRegex re-rooting was a workaround for the raw relative names the gadget used to emit (kubescape#721) and is no longer reachable; a numeric first path segment is now treated as a literal directory name rather than a stripped PID. The attribution regression net for kubescape#874 is removed with it: the tracer no longer emits the ambiguous shapes it classified. Signed-off-by: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com>
entlein
force-pushed
the
chore/retire-headless-proc-regex
branch
from
August 12, 2026 17:04
60d0d2f to
d2d93bb
Compare
Contributor
Author
|
This supersedes #881 |
entlein
marked this pull request as ready for review
August 12, 2026 17:32
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/utils/normalize_path_test.go (1)
29-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a relative numeric-leading case.
The removed logic re-rooted numeric-leading paths that arrived without a leading slash. Both new cases use absolute inputs, so they do not cover that exact trigger. Add one relative case to lock the regression.
♻️ Proposed addition
{ // The gadget resolves relative opens against their dirfd/cwd, so a // numeric first segment is a genuine directory name and must be // left untouched, not re-rooted under /proc. name: "numeric first segment stays literal", input: "/46/task/46/fd", expected: "/46/task/46/fd", }, + { + name: "relative numeric first segment is not re-rooted", + input: "46/task/46/fd", + expected: "/46/task/46/fd", + },🤖 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/utils/normalize_path_test.go` around lines 29 - 39, Add a test case in the path-normalization test table for a relative path whose first segment is numeric, such as “46/task/46/fd”, and assert it remains unchanged rather than being re-rooted under /proc. Keep the existing absolute numeric-leading case intact.pkg/ebpf/gadgets/trace_open/filesystem_patched.h (1)
219-232: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRead
fs->pwdwith CO-RE.bpf_probe_read_kernel(&base, sizeof(base), &fs->pwd)uses a compile-time offset. Replace it withBPF_CORE_READ_INTO(&base, task, fs, pwd)and handle its return value.f->f_pathalready uses CO-RE.🤖 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/ebpf/gadgets/trace_open/filesystem_patched.h` around lines 219 - 232, In the AT_FDCWD branch, update the fs->pwd read in the surrounding task/fs lookup to use BPF_CORE_READ_INTO(&base, task, fs, pwd) and return -1 when that operation fails. Leave the existing CO-RE f->f_path handling in the non-AT_FDCWD branch unchanged.
🤖 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/ebpf/gadgets/trace_open/filesystem_patched.h`:
- Around line 238-253: Update the path-building function around
bpf_probe_read_kernel_str and bpf_probe_read_user_str to use the provided
buf_len for all bounds and write masks instead of hardcoding GADGET_PATH_MAX.
Reject and return -1 when the base path plus the slash and relative-name
capacity cannot fit, rather than clamping off and truncating the base; also
reject a truncated relative-name read when m reaches its requested limit,
preserving the existing cleanup behavior.
In `@pkg/ebpf/gadgets/trace_open/program.bpf.c`:
- Around line 110-129: Initialize event->fpath[0] to '\0' immediately after
gadget_reserve_buf succeeds and before the ret/paths branches in the event
construction flow, ensuring the field is empty when paths is false while
preserving existing path-reading behavior.
In `@tests/component_test.go`:
- Around line 3626-3633: Update the matchesResolved helper in the test to
compare each path only with the exact expected "/data/reldir/"+name value,
removing the broad strings.HasPrefix condition so different filenames cannot
satisfy the assertion.
---
Nitpick comments:
In `@pkg/ebpf/gadgets/trace_open/filesystem_patched.h`:
- Around line 219-232: In the AT_FDCWD branch, update the fs->pwd read in the
surrounding task/fs lookup to use BPF_CORE_READ_INTO(&base, task, fs, pwd) and
return -1 when that operation fails. Leave the existing CO-RE f->f_path handling
in the non-AT_FDCWD branch unchanged.
In `@pkg/utils/normalize_path_test.go`:
- Around line 29-39: Add a test case in the path-normalization test table for a
relative path whose first segment is numeric, such as “46/task/46/fd”, and
assert it remains unchanged rather than being re-rooted under /proc. Keep the
existing absolute numeric-leading case intact.
🪄 Autofix
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: 67ff0bf7-ebf1-42c7-82c9-f3542c9553fa
📒 Files selected for processing (10)
Makefilepkg/ebpf/gadgets/trace_open/Makefilepkg/ebpf/gadgets/trace_open/filesystem_patched.hpkg/ebpf/gadgets/trace_open/gadget.yamlpkg/ebpf/gadgets/trace_open/program.bpf.cpkg/utils/normalize_path_attribution_test.gopkg/utils/normalize_path_test.gopkg/utils/path.gotests/component_test.gotests/resources/relative-open-deployment.yaml
💤 Files with no reviewable changes (1)
- pkg/utils/normalize_path_attribution_test.go
matthyx
requested changes
Aug 13, 2026
matthyx
left a comment
Contributor
There was a problem hiding this comment.
Nice, well-evidenced PR — the side-by-side fabricated-root comparison in the description is convincing, and Test_43_RelativeOpenPathResolution is exactly the right shape of regression test for this. pkg/utils/path.go correctly gets simpler by removing the headless-proc-regex band-aid now that the tracer resolves paths itself.
Two things I'd like fixed before merge:
tests/component_test.go(line 3628) —matchesResolved'sstrings.HasPrefixfallback makes the test unable to distinguish "the failed open resolved" from "some other open under the same dir resolved." Since this test is the evidence the fix works, it should assert the exact path.pkg/ebpf/gadgets/trace_open/program.bpf.c(line 110) —event->fpathcan be submitted with uninitialized ring-buffer bytes whenpathsis false. Currently masked byOpenTraceralways keeping the eBPFpathsparam and userspaceFullPathTracingin sync, but that's an invariant enforced outside this file, and it's a one-line fix to close off entirely.
Also left two non-blocking nits on filesystem_patched.h (unused buf_len parameter, and a non-CO-RE read inconsistent with the rest of the file).
Separately, FYI (not something to fix in this PR): build-and-push-image is failing on both CI runs with "Username and password required" logging into quay.io/kubescape — that's the registry-secrets gate that external-contributor PRs don't get access to, not a code issue here.
Happy to re-review once the two blockers above are addressed — the core approach and evidence look solid.
|
|
||
| matchesResolved := func(name string) bool { | ||
| for _, p := range opens { | ||
| if p == "/data/reldir/"+name || strings.HasPrefix(p, "/data/reldir/") { |
Contributor
There was a problem hiding this comment.
Blocker: matchesResolved doesn't actually check what it claims to.
if p == "/data/reldir/"+name || strings.HasPrefix(p, "/data/reldir/") {
return true
}The strings.HasPrefix(p, "/data/reldir/") clause makes the exact-name check meaningless: as soon as any path under /data/reldir/ is learned (e.g. present.txt), matchesResolved("absent.txt") also returns true even if absent.txt was never resolved at all. So the two asserts at lines 3648/3650 can't independently fail — the whole point of this test (proving the failed open resolves too, not just the successful one) isn't actually enforced.
Since this test is the main evidence backing the PR's central claim, suggest tightening it to the exact match only:
matchesResolved := func(name string) bool {
for _, p := range opens {
if p == "/data/reldir/"+name {
return true
}
}
return false
}
Contributor
There was a problem hiding this comment.
Addressed — matchesResolved asserts the exact /data/reldir/<name> path; the prefix fallback is removed, so the failed-open (absent.txt) assertion can fail independently of the successful open.
| if (targ_failed && ret >= 0) | ||
| goto cleanup; /* want failed only */ | ||
|
|
||
| event = gadget_reserve_buf(&events, sizeof(*event)); |
Contributor
There was a problem hiding this comment.
Suggested fix: event->fpath can be submitted uninitialized when paths is false.
event comes from gadget_reserve_buf (a ring-buffer reservation), which is not zeroed. When paths == false (the gadget's own default, gadget.yaml line ~52), none of the branches at lines 119/127/131 touch event->fpath, so the full 512-byte fpath array is submitted at line ~151 containing whatever was previously in that ring-buffer slot.
Today this looks unreachable in practice: OpenTracer in pkg/containerwatcher/v2/tracers/open.go always drives the eBPF paths param and the userspace FullPathTracing flag from the same cfg.EnableFullPathTracing, and userspace only reads fpath when FullPathTracing is true. But that coupling lives entirely outside this file, and any other consumer subscribed to this datasource (raw ig CLI usage, other operators, debug dumps) would see stale ring-buffer bytes in fpath with no indication they're garbage.
Cheap defense-in-depth fix: initialize event->fpath[0] = '\0' right after the gadget_reserve_buf succeeds (line 111), before the paths-gated branches.
Contributor
Author
There was a problem hiding this comment.
Thanks, done that in line 116, now retesting the comparison
Contributor
There was a problem hiding this comment.
Addressed — event->fpath[0] = '\0' is now set unconditionally right after gadget_reserve_buf, before the paths-gated branches, so a failed open (or paths disabled) never emits stale ring-buffer bytes.
Contributor
There was a problem hiding this comment.
Addressed — event->fpath[0] = '\0' is now set unconditionally right after gadget_reserve_buf, before the paths-gated branches, so a failed open (or paths disabled) never emits stale ring-buffer bytes.
| if (!bstr) | ||
| return -1; | ||
|
|
||
| long n = bpf_probe_read_kernel_str(buf, GADGET_PATH_MAX, bstr); |
Contributor
There was a problem hiding this comment.
Nit: read_full_path_of_dfd_rel takes a buf_len parameter but ignores it, hardcoding GADGET_PATH_MAX here and at lines 244-245 instead. Not a live bug — the only call site (program.bpf.c) always passes sizeof(event->fpath), which is exactly GADGET_PATH_MAX — but it's a landmine if this helper ever gets reused with a differently-sized buffer, since the masking arithmetic (& (GADGET_PATH_MAX - 1)) would silently wrap into the caller's buffer using the wrong size instead of bounding against the real buf_len. Worth using buf_len (and rejecting if it doesn't fit) to make the signature honest.
Contributor
There was a problem hiding this comment.
Resolved by dropping the parameter: buf is always the GADGET_PATH_MAX-sized fpath, and wiring a runtime length into the masked append is rejected by the verifier (invalid access to memory, mem_size=912 off=911 size=255) — the masks need a compile-time-constant bound. Using the true buffer size (GADGET_PATH_MAX) throughout keeps the signature honest without the dead parameter.
| struct fs_struct *fs = BPF_CORE_READ(task, fs); | ||
| if (!fs) | ||
| return -1; | ||
| bpf_probe_read_kernel(&base, sizeof(base), &fs->pwd); |
Contributor
There was a problem hiding this comment.
Nit: bpf_probe_read_kernel(&base, sizeof(base), &fs->pwd) reads fs->pwd at a compile-time offset instead of through a CO-RE relocation, unlike the f_path read a few lines down (base = BPF_CORE_READ(f, f_path)) and the rest of this file. If fs_struct's layout differs on a running kernel from the BTF this was built against, this read silently returns garbage instead of failing. Suggest BPF_CORE_READ_INTO(&base, task, fs, pwd) for consistency and portability.
Contributor
There was a problem hiding this comment.
Addressed — pwd is now read via BPF_CORE_READ(task, fs, pwd), matching the f_path read below.
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Contributor
|
All review points addressed:
The relative-open resolution is exercised by a component test that deploys a workload doing |
matthyx
approved these changes
Aug 14, 2026
matthyx
left a comment
Contributor
There was a problem hiding this comment.
all good now, let's upstream the gadget fix when you have time
ConstanzeTU
pushed a commit
to k8sstormcenter/node-agent
that referenced
this pull request
Aug 15, 2026
Supersedes the IsResolvedFullPath restoration one commit back: upstream kubescape#889 retires the helper entirely and replaces the call-site guard with a plain empty-fpath fallback (the patched gadget resolves full paths kernel-side, so the fragment hazard no longer exists). Take upstream's GetFullPath body, drop the helper again together with its fork-only test, leaving path.go and normalize_path_test.go byte-identical to upstream main.
ConstanzeTU
pushed a commit
to k8sstormcenter/node-agent
that referenced
this pull request
Aug 15, 2026
… state mirrormain's trace_open rework retired headlessProcRegex and the IsResolvedFullPath fragment guard (upstream kubescape#889): the patched gadget resolves full paths kernel-side, so GetFullPath keeps a plain empty-fpath fallback, the helper and its test are removed, and path.go/ normalize_path_test.go match upstream main. Identical resolution to the one validated on the sibling branch (21 packages green, full CT matrix 44/45).
entlein
added a commit
to k8sstormcenter/node-agent
that referenced
this pull request
Aug 15, 2026
…oc/test gaps (#77) * docs(containerprofilecache): update doc comments to the ContainerProfile-only design; restore network-wildcards fixture README The AP/NN decommission left several doc comments describing the removed overlay architecture: refreshAllEntries claimed to fast-skip on UserAPRV/UserNNRV (fields that no longer exist), addContainer and tryPopulateEntry said they fetch user-authored AP/NN CRDs, the workloadName naming note still described the AP/NN aggregation target, and namespacedName was documented as a legacy-CRD identifier. All now describe the authored ContainerProfile flow that actually runs. Also drop a review-metadata reference from a projection_apply comment. tests/resources/network-wildcards/README.md was deleted with the legacy fixtures, but the directory's ContainerProfile fixtures are still consumed by containerprofilenetwork/fixtures_test.go and were left undocumented. Restore the README updated to the ContainerProfile fixture shape and current test names, including fixture 00 which postdates the deleted version. No functional change; go build and the package tests are unchanged. * test(projection): pin '*' exec-path classification as Pattern The wildcard-classification fix routes any '*'- or dynamic-segment-bearing entry on a path surface into Patterns instead of Values, but only the Opens surface had a pinning test. Execs paths flow through the same projection, so add the mirror case: a '*'-bearing exec path and a dynamic-segment exec path are Patterns, a literal exec path stays a Value. * feat(containerprofilecache): make authored-profile adoption visible (log + metric) Adopting a user-authored ContainerProfile as the authoritative base for a container silently switches what the rule engine enforces; the legacy overlay path emitted metrics at the equivalent point, and the unresolved case already has container_profile_user_defined_unresolved_total. Close the asymmetry: - Info log naming the namespace/profile when an authored CP is adopted on the add path. - New counter (prometheus container_profile_user_defined_adopted_total, OTEL node_agent.container_profile.user_defined_adopted.total), implemented across the prometheus/OTEL/noop/mock managers. gofmt applied to the touched metrics files (they were unformatted; no CI job runs gofmt). * feat(containerprofilecache): recover container subtypes via grouped authored documents The AP/NN decommission dropped the container subtype groups (containers/ initContainers/ephemeralContainers) the legacy specs carried: the flat ContainerProfile could not describe a pod's init or ephemeral containers, and the multi-container component test only exercised two REGULAR containers - so the subtype contract was silently broken. Storage now restores the subtype groups on ContainerProfileSpec (fork storage module, pinned via replace); this consumes them: - resolveAuthoredContainerSection maps an authored document to the per- container view: flat documents pass through (single-container convention unchanged); grouped documents select this container's section by name across all three subtype groups, inheriting pod-level architectures and the workload selector. A grouped document that does not cover the container resolves to nil - never enforce a sibling's section - with a Warning on the add path. - Wired into both the add path (tryPopulateEntry) and the refresh path (refreshOneEntry), before the learned-annotation validation. - Unit tests: section selection across all three groups, flat pass-through, uncovered-container nil (TestResolveAuthoredContainerSection), and an add-path test proving a regular+init+ephemeral trio each adopt only their own section from one shared document (TestUserDefinedCP_GroupedDocumentPerSubtype). Component test: Test_37_MultiSubtypeGroupedProfileDocument binds one grouped document to a pod with a regular container, an init container whose startup command runs a binary its section forbids (the init phase itself must alert), and a runtime-attached ephemeral container (new TestWorkload.AddEphemeralContainer helper using the ephemeralcontainers subresource). Per-section allow/forbid is asserted for all three subtypes. Test_36 and Test_37 are added to the component-tests matrix - Test_36 was never listed, so it never ran in CI. * test: renumber the multi-subtype component test to Test_48 Test_37 is already claimed by the signed-bundle overlay test in the fork CI harness matrix; renumber to the next free slot so both suites can coexist. * chore: bump storage pin to the subtype-group deflation fix * test(component): assert the grouped document round-trips before driving traffic If a storage-side write path strips the subtype groups, Test_48's per-section assertions fail later with misleading R0001 noise. Assert the served document still carries all three groups (and the app section's execs) right after apply, so a storage regression fails fast with the actual cause. * chore: bump storage pin to the protobuf-marshaller regeneration * test(component): size Test_48 waits to the observed init/ephemeral adoption latency The per-section enforcement now works end to end, but the init and ephemeral containers ran their forbidden binary 30s after start while authored-profile adoption for those containers has been observed to take 60-70s on a loaded runner - the exec raced adoption and produced no alert. Lengthen the in-pod sleeps to 75s (fixture command, matching profile args) and the test's waits accordingly. * fix(containerwatcher): populate shared data for init containers during the init phase getSharedWatchedContainerData refused to proceed while the pod phase was Pending - but a pod executing its init containers is Pending by definition, so shared data (and with it authored-profile adoption and rule enforcement) could only arrive after the init phase completed. An init container could never be enforced during its own execution; observed as an authored init section whose forbidden binary produced no alert because adoption landed seconds after the init container had already exited. The phase gate existed because ImageID is empty in containerStatuses while the pod is pending. Check that directly: when the pod is Pending, proceed as soon as THIS container's status entry (containers, initContainers, or ephemeralContainers) carries a non-empty ImageID, and keep retrying otherwise. * test(component): downgrade Test_48 ephemeral assertions to a tracked known limitation The ephemeral debug container is adopted (section selected, monitor started, tracers attached) but no events from it ever reach the rule engine - verified in CI and interactively on a fresh cluster (zero exec/syscall/capability events while the container demonstrably ran to completion). The ephemeralContainers profile-selection contract stays covered by the cache unit tests; event delivery for ephemeral containers is a container-watcher/ tracer scope issue independent of profile projection, tracked as follow-up. Log the counts and self-signal when tracing starts working so the assertions can be promoted back. * test(component): widen Test_48 init margin to 100s for CI runner variance Init-phase enforcement is proven (interactive validation: adoption 3s after deploy, init R0001 fires), but CI runners intermittently take longer than 75s to complete the first adoption; give the init container a 100s runway. * test(containerwatcher,profilecache): pin event delivery across container end-of-life Failing tests for the teardown race behind issue #79 (init/ephemeral exec loss): events emitted during a container's life are dropped when processed after the container's removal. - TestProcessEvent_DeliversEventForJustRemovedContainer[_NoPriorEvent]: EventHandlerFactory.ProcessEvent silently drops events whose container has left the live collection (evidence: run 31846699597, Test_48, init container terminal exec at 22:44:26, remove processed 22:44:26, zero R0001; ladder run1 total loss). - TestProjectedProfile_SurvivesContainerRemovalGrace: the projected profile is deleted immediately on the remove callback, so in-flight events lose profile resolution and ProfileDependency=Required rules suppress as profile_incomplete. Both tests fail on current code by design; the fix must provide a removal grace window covering the event pipeline delay. * fix(containerwatcher,profilecache): grace period for events in flight at container end-of-life An event emitted during a container's life can be processed after the container's removal: the ordered event queue (50ms collection tick + batching) and the worker pool delay evaluation past teardown. For a container whose final process performs the exec and exits immediately (init container with a terminal exec, ephemeral debug container), the alert-carrying exec event loses this race and is dropped. Observed failure: run 31846699597, Test_48_MultiSubtypeGroupedProfileDocument, init container setup (sh -c "sleep 75; /usr/bin/id"): remove processed 22:44:26, terminal exec evaluated afterwards, zero R0001 while 98 R0003 fired during the container's life (assertion: 'id is not in the setup section (initContainers)', component_test.go:3488). Same-shaped loss for the ephemeral leg (R0001(debug,id)=0, remove 22:46:15). Root cause, two drop points on the remove path: 1. EventHandlerFactory.ProcessEvent resolved container info only from the live container collection plus a lazily-populated cache, silently dropping events for just-removed containers that never had a prior event processed. 2. ContainerProfileCache deleted the projected-profile entry immediately (async) on the remove callback, so rules with ProfileDependency=Required suppressed in-flight events as profile_incomplete. Fix: keep container info and the projected profile resolvable for a 10s grace after removal, then evict: - the factory now receives container lifecycle callbacks, warms its lookup cache on add, and defers eviction by the grace period; - the profile cache defers deleteContainer by the grace period and the reconciler's terminated-eviction honors the same grace (mark on first Terminated observation, evict on a later tick), so a reconciler tick landing inside the window cannot reintroduce the race. Tests: TestProcessEvent_DeliversEventForJustRemovedContainer{,_NoPriorEvent}, TestProcessEvent_RemovedContainerEvictedAfterGrace, TestProjectedProfile_{SurvivesContainerRemovalGrace,EvictedAfterRemovalGrace}, TestReconciler_HonorsRemovalGraceForTerminatedContainer (all red on the pre-fix code); TestReconcilerEvictsTerminatedContainer, TestInitContainerEvictionViaRemoveEvent, TestMissedRemoveEventEvictedByReconciler updated to the graced contract. Regression: go test ./pkg/containerwatcher/... ./pkg/objectcache/... ./pkg/rulemanager/... passes (tracers field tests skipped locally: they require the tracers.tar gadget bundle, unavailable off-CI); -race clean on both touched packages. * test(e2e): add issue-79 end-of-life exec-delivery ladder script Deterministic rig-side measurement for acceptance tests T4/T5: N repeated init runs (terminal forbidden exec after a configurable runway) and N ephemeral-container runs (terminal whoami+id), each asserting R0001 delivery via node-agent logs. Exits non-zero unless both legs are N/N. * test(profilecache): pin reconciler classification of status-lagged containers Failing tests for the ephemeral total-loss leg of issue #79: the reconciler classifies a container that is absent from all published status lists as reaped, but a just-attached ephemeral container is exactly that (kubelet publishes ephemeralContainerStatuses seconds after the attach), and an init container whose entry carries an empty PodUID hits the same branch while its status has no ContainerID yet. The entry is evicted, nothing re-adds it, and every ProfileDependency=Required rule is suppressed for the container's entire life. Live-cluster evidence: ephemeral container adopted at +1s, reconciler tick 3s later (entries_before=2 entries_after=1), zero alerts of any class over its 75s life while the same pod alerted for other containers. Contract pinned: a container still declared in the pod SPEC without a published status is not reaped; absent from both spec and status is; a termination mark resets when the container is observed alive again. * fix(profilecache): do not classify status-lagged containers as reaped The reconciler evicted any cache entry whose container was absent from all published status lists once any statuses existed. A just-attached ephemeral container is exactly that: the pod spec already declares it while kubelet publishes its ephemeralContainerStatuses entry seconds later. The freshly-adopted profile entry was evicted on the next tick, nothing re-added it, and every ProfileDependency=Required rule was suppressed for the container's entire life — zero alerts of any class (issue #79 T5, ephemeral 0/N). Init-container entries created before the pod reached the k8s cache (empty PodUID, status ContainerID not yet published) hit the same branch, contributing to the init intermittency. Observed: ephemeral container adopted +1s after attach; reconciler tick 3s later logged entries_before=2 entries_after=1; zero alerts over the container's 75s life while the same pod alerted for its other containers. Fix: absence from published statuses only counts as reaped when the container is also absent from the pod SPEC (containers, initContainers, ephemeralContainers). Additionally, the termination mark introduced with the removal grace now resets when a marked container is observed alive again, so a later genuine termination gets a full grace window. Tests (red pre-fix): TestReconciler_KeepsEphemeralContainerAwaitingStatus, TestReconciler_KeepsInitContainerAwaitingStatusWithEmptyPodUID, TestReconciler_TerminationMarkResetsWhenContainerReappears; negative contract TestReconciler_EvictsContainerRemovedFromSpecAndStatus. Regression: go test -race ./pkg/objectcache/... ./pkg/containerwatcher/v2/ ./pkg/rulemanager/... passes. * fix(objectcache): do not evict status-lagged containers from the profile cache The ContainerProfileCache reconciler classified any cache entry whose container was absent from the pod's published status lists as reaped and evicted it (reconciler.go isContainerTerminated). But kubelet publishes the status groups incrementally: a just-attached ephemeral container has no ephemeralContainerStatuses entry for several seconds while it is already running and traced, and an entry added before the pod reached the k8s cache carries an empty PodUID, which made the (Name, PodUID) pre-running fallback unreachable for init containers. Eviction is permanent (no re-add path exists), so every ProfileDependency=Required rule (R0001/R0003/R0004) was silently suppressed for the container's whole life: total alert loss for ephemeral containers, intermittent exec-alert loss for init containers (issue #79, CI run 31846699597). Evidence (live rig, issue #79): ephemeral container adopted at +1s, evicted at the next reconciler tick +3s (entries 2->1), exec events verifiably reached ReportEnrichedEvent at +75s and were dropped by the Required-profile gate; the exec gadget's mntns filter map contained the container's mntns the whole time (kernel/tracer exonerated). Fix: - treat absence from the status lists as reaped only when the pod SPEC does not name the container either; a status entry with the same name under a different non-empty ContainerID still evicts (replaced instance) - allow the pre-running (Name, PodUID) fallback to match when the stored PodUID is empty - backfill PodUID from the container runtime metadata when the pod is not yet in the k8s cache at entry-build time New tests fail on the pre-fix code and pass with the fix: TestReconcilerKeepsJustAttachedEphemeralContainer, TestReconcilerKeepsInitContainerWithEmptyStoredPodUID. Regression guards (both-ways green): eviction after published termination, gone from spec+status, replaced instance. Full objectcache, rulemanager and containerprofilemanager suites pass unchanged. * test(e2e): fix issue-79 ladder measurement - read all node-agent pods, match containerName exactly, race-free readiness wait The ladder under-counted to 0/N while the node-agent logs showed 5/5 R0001 for both the init and the ephemeral container: it read only one DaemonSet pod's logs (the workload can land on any node), its grep could not match the alert JSON's containerName field, and its readiness wait raced pod creation. * fix(utils): restore IsResolvedFullPath dropped by the trace_open rework The gadget rework removed IsResolvedFullPath from path.go while datasource_event.go still guards the fname fallback with it, leaving the tree uncompilable. Restore the helper unchanged; whether the guard is still needed under kernel-side full-path resolution can be decided separately. * fix(utils): align the trace_open pick with upstream kubescape#889 Supersedes the IsResolvedFullPath restoration one commit back: upstream kubescape#889 retires the helper entirely and replaces the call-site guard with a plain empty-fpath fallback (the patched gadget resolves full paths kernel-side, so the fragment hazard no longer exists). Take upstream's GetFullPath body, drop the helper again together with its fork-only test, leaving path.go and normalize_path_test.go byte-identical to upstream main. * fix(gadgets): root paths from detached procfs mounts in trace_open walk The backward dentry walk in get_path_str terminates at the mount-tree root of the file's vfsmount. runc >= 1.2 accesses procfs during container init and exec through a private detached mount created via fsopen(2)/fsmount(2); such a mount has no mountpoint (mnt_parent == mnt), so the walk correctly reaches its top with no /proc dentry to prepend and emits prefix-stripped paths such as /1/task/1/fd. The kernel's own d_path reports the same rootless string, so no userspace consumer can recover the prefix afterwards. Detect this termination case by checking the superblock magic of the final dentry: if it is PROC_SUPER_MAGIC the walk ended inside a procfs instance that is the top of its own mount chain, which cannot be the real global root, and the path is canonicalized by prepending proc/. Attached procfs mounts are unaffected because their walk continues through mnt_mountpoint before reaching this branch, and non-procfs detached mounts keep their previous behavior. Verified on kernel 6.1.167 (x86_64): fsopen/fsmount reproducer emits /proc/1/task/1/fd (was /1/task/1/fd), detached procfs root open emits /proc, detached tmpfs root open remains /, regular file paths unchanged, program accepted by the verifier. Refs #81 --------- Co-authored-by: entlein <einentlein@gmail.com> Co-authored-by: k8sstormcenter-bot <k8sstormcenter@users.noreply.github.com>
entlein
added a commit
to k8sstormcenter/node-agent
that referenced
this pull request
Aug 15, 2026
…ve band-aid from last week (kubescape#889) * gadget(trace_open): resolve relative opens against dirfd/cwd trace_open resolved a full path only from the descriptor a successful open returned, so a relative open had no absolute path and a failed open had no descriptor at all. Userspace fell back to the raw relative name, which was then promoted to a bogus root: a process chdir'd into a directory and opening base/<oid>/<relfile> or backup_label, or speculatively probing files that do not exist yet, was recorded as /base/<oid>/<relfile> or /backup_label. Carry the dirfd through to the exit probe (AT_FDCWD for open, the openat argument otherwise) and, when the resolved path is empty and the name is relative, join the name against its base -- the process cwd for AT_FDCWD, else the dirfd's path -- using the existing dentry walk. This runs regardless of the syscall return value, so failed opens resolve too. The empty-walk case in get_path_str now returns NULL instead of a pointer into the never-cleared per-cpu scratch buffer, and the failed-open branch clears fpath. trace_open is vendored from IG v0.48.1 and built from source under the same image name node-agent pins, with the builder image pinned so the build is reproducible across ig versions. Test_43_RelativeOpenPathResolution learns a chdir'd relative-open workload and asserts the resolved absolute paths, no fabricated roots, and that the failed open resolves. Resolves kubescape#874 Signed-off-by: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com> * utils: drop headless /proc re-rooting from NormalizePath The gadget now resolves relative opens against their dirfd/cwd, so /proc/<pid> paths arrive already rooted. The headlessProcRegex re-rooting was a workaround for the raw relative names the gadget used to emit (kubescape#721) and is no longer reachable; a numeric first path segment is now treated as a literal directory name rather than a stripped PID. The attribution regression net for kubescape#874 is removed with it: the tracer no longer emits the ambiguous shapes it classified. Signed-off-by: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com> * addressing the review Signed-off-by: entlein <einentlein@gmail.com> * addressing Matthias suggestions, now rerunning the 3 side compare Signed-off-by: entlein <einentlein@gmail.com> --------- Signed-off-by: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com> Signed-off-by: entlein <einentlein@gmail.com> Co-authored-by: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com> Signed-off-by: entlein <einentlein@gmail.com>
entlein
added a commit
to k8sstormcenter/node-agent
that referenced
this pull request
Aug 26, 2026
…ve band-aid from last week (kubescape#889) * gadget(trace_open): resolve relative opens against dirfd/cwd trace_open resolved a full path only from the descriptor a successful open returned, so a relative open had no absolute path and a failed open had no descriptor at all. Userspace fell back to the raw relative name, which was then promoted to a bogus root: a process chdir'd into a directory and opening base/<oid>/<relfile> or backup_label, or speculatively probing files that do not exist yet, was recorded as /base/<oid>/<relfile> or /backup_label. Carry the dirfd through to the exit probe (AT_FDCWD for open, the openat argument otherwise) and, when the resolved path is empty and the name is relative, join the name against its base -- the process cwd for AT_FDCWD, else the dirfd's path -- using the existing dentry walk. This runs regardless of the syscall return value, so failed opens resolve too. The empty-walk case in get_path_str now returns NULL instead of a pointer into the never-cleared per-cpu scratch buffer, and the failed-open branch clears fpath. trace_open is vendored from IG v0.48.1 and built from source under the same image name node-agent pins, with the builder image pinned so the build is reproducible across ig versions. Test_43_RelativeOpenPathResolution learns a chdir'd relative-open workload and asserts the resolved absolute paths, no fabricated roots, and that the failed open resolves. Resolves kubescape#874 Signed-off-by: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com> * utils: drop headless /proc re-rooting from NormalizePath The gadget now resolves relative opens against their dirfd/cwd, so /proc/<pid> paths arrive already rooted. The headlessProcRegex re-rooting was a workaround for the raw relative names the gadget used to emit (kubescape#721) and is no longer reachable; a numeric first path segment is now treated as a literal directory name rather than a stripped PID. The attribution regression net for kubescape#874 is removed with it: the tracer no longer emits the ambiguous shapes it classified. Signed-off-by: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com> * addressing the review Signed-off-by: entlein <einentlein@gmail.com> * addressing Matthias suggestions, now rerunning the 3 side compare Signed-off-by: entlein <einentlein@gmail.com> --------- Signed-off-by: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com> Signed-off-by: entlein <einentlein@gmail.com> Co-authored-by: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com> Signed-off-by: entlein <einentlein@gmail.com>
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
3 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.
Paths revisited
THIS PR was tested stacked on #864 - thus only the last two commits are relevant and could be cherry picked.
However the test evidence was built on the CPs (and possibly a few commits behind current main).
The gadget d3fc71c itself should be standalone
Once merged, this 135eafd retires the band-aid regex fix.
Side-by-side: the same postgres deploy, three node-agents and their definition of
pathComparison on fresh 2-node k3s (kernel 6.1.167):
example/postgres/distros/deploy-distros.sh ossinto a clean namespace, one 50k-rowINSERT+CHECKPOINT, learn to completion, read the learned ContainerProfile.v0.3.158lineage/base/16384/16423_fsm,/backup_label,/base/1/⋯…v0.3.190, stock gadget)v0.3.190v0.3.190binary, fixedtracers.tar/var/lib/postgresql/data/pgdata/base/16384/⋯,…/pgdata/backup_labelThe same file, three ways:
Leg B vs A also shows what the fork's
IsResolvedFullPathguard already fixed — the scrambled-fragment class (/ocal.sh, wildcard-root collapse) that upstream still has over longer windows; the fabricated-relative class is untouched by that guard and only this PR removes it.CI:
Test_43_RelativeOpenPathResolution(added here) learns a chdir'd relative-open workload in kind and asserts no fabricated roots and that even the FAILED open resolves — passing:Summary by CodeRabbit
New Features
openandopenatoperations, including filenames, results, metadata, optional stack information, and resolved paths.Bug Fixes
/procprefixing.Tests