addon fix for the old issue to /proc full paths not being recorded - #872
addon fix for the old issue to /proc full paths not being recorded#872entlein wants to merge 2 commits into
Conversation
Signed-off-by: entlein <einentlein@gmail.com>
📝 WalkthroughWalkthroughThe headless proc path allowlist now covers additional process metadata and container setup paths. Regression tests verify these paths and confirm that numeric-looking segments in non-proc paths remain unchanged. ChangesHeadless proc path normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
… RCA why this regressed in the first place Signed-off-by: entlein <einentlein@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/utils/normalize_path_test.go (1)
53-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand regression coverage for the full allowlist.
pkg/utils/path.goaddsstatus,stat,cgroup,mountinfo,maps,environ,comm,cmdline, andns, but this table tests only three of the new entries. Add one positive case for each remaining entry.Use a numeric leading segment in the negative case. The current
/data/appendonlydir/xinput bypasses the matcher and does not test the allowlist boundary.Suggested negative case
- input: "/data/appendonlydir/x", - expected: "/data/appendonlydir/x", + input: "/123/data/appendonlydir/x", + expected: "/123/data/appendonlydir/x",🤖 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 53 - 76, Expand the table in normalize_path_test.go with positive cases covering each missing allowlist entry from path.go: status, stat, cgroup, mountinfo, maps, environ, comm, cmdline, and ns, using numeric leading segments and expected /proc-prefixed paths. Replace the current non-proc negative case with a numeric-leading path whose trailing segment resembles a proc entry but is not allowlisted, verifying it remains 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.
Nitpick comments:
In `@pkg/utils/normalize_path_test.go`:
- Around line 53-76: Expand the table in normalize_path_test.go with positive
cases covering each missing allowlist entry from path.go: status, stat, cgroup,
mountinfo, maps, environ, comm, cmdline, and ns, using numeric leading segments
and expected /proc-prefixed paths. Replace the current non-proc negative case
with a numeric-leading path whose trailing segment resembles a proc entry but is
not allowlisted, verifying it remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f59ae2fb-a7b1-4e45-8ff0-82f8aed1ccfa
📒 Files selected for processing (2)
pkg/utils/normalize_path_test.gopkg/utils/path.go
We re seeing again :
This is a band-aid type patch for the resurface of #721
In my tests on dragonfly and argocd, it worked, but that list is not exhaustive, so will look if we have a real regression as soon as I have some time
Summary by CodeRabbit
/proc, including namespace, user/group mapping, status, and cgroup paths.