Jv rox 33034 track xattr changes test all events#864
Open
JoukoVirtanen wants to merge 6 commits into
Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds end-to-end tracking of Linux extended attribute (xattr) set and remove operations. New eBPF types, LSM hooks, a shared ChangesXattr Event Tracking Feature
Third-party Submodule Update
Sequence DiagramsequenceDiagram
participant LSM as LSM Hook (inode_setxattr / inode_removexattr)
participant handle_xattr
participant submit_xattr_event
participant Ringbuf as eBPF Ringbuf
participant fact_lib as fact Rust Library
participant KernelMetrics
LSM->>handle_xattr: dentry, xattr_name, FILE_ACTIVITY_SETXATTR/REMOVEXATTR
handle_xattr->>handle_xattr: increment hook counter, derive inode keys, check monitored
handle_xattr->>submit_xattr_event: args, event_type, xattr_name
submit_xattr_event->>Ringbuf: reserve slot, write event_t with xattr.name
Ringbuf->>fact_lib: event_t delivered to userspace
fact_lib->>fact_lib: FileData::new → XattrFileData { inner, xattr_name }
fact_lib->>KernelMetrics: accumulate inode_setxattr / inode_removexattr counters
fact_lib-->>fact_lib: Event accessors route through XattrFileData.inner
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## jv-ROX-33034-track-xattr-changes #864 +/- ##
=================================================================
Coverage 28.27% 28.27%
=================================================================
Files 21 21
Lines 2568 2568
Branches 2568 2568
=================================================================
Hits 726 726
Misses 1839 1839
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fact/src/event/mod.rs (1)
498-510:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the missing
Chownequality branch inFileData::eq.At Line 500-510,
FileData::Chownis not matched, so two ownership events compare unequal even when identical.Suggested fix
impl PartialEq for FileData { fn eq(&self, other: &Self) -> bool { match (self, other) { (FileData::Open(this), FileData::Open(other)) => this == other, (FileData::Creation(this), FileData::Creation(other)) => this == other, (FileData::MkDir(this), FileData::MkDir(other)) => this == other, (FileData::RmDir(this), FileData::RmDir(other)) => this == other, (FileData::Unlink(this), FileData::Unlink(other)) => this == other, (FileData::Chmod(this), FileData::Chmod(other)) => this == other, + (FileData::Chown(this), FileData::Chown(other)) => this == other, (FileData::Rename(this), FileData::Rename(other)) => this == other, (FileData::SetXattr(this), FileData::SetXattr(other)) => this == other, (FileData::RemoveXattr(this), FileData::RemoveXattr(other)) => this == other, _ => false, } } }🤖 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 `@fact/src/event/mod.rs` around lines 498 - 510, The PartialEq implementation for FileData is missing a match arm for the Chown variant. Add a branch in the eq method of FileData that matches FileData::Chown(this) against FileData::Chown(other) and compares them with the equality operator, placing it before the catch-all _ => false pattern so that two identical Chown variants will correctly compare as equal instead of falling through to the default false case.
🤖 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.
Outside diff comments:
In `@fact/src/event/mod.rs`:
- Around line 498-510: The PartialEq implementation for FileData is missing a
match arm for the Chown variant. Add a branch in the eq method of FileData that
matches FileData::Chown(this) against FileData::Chown(other) and compares them
with the equality operator, placing it before the catch-all _ => false pattern
so that two identical Chown variants will correctly compare as equal instead of
falling through to the default false case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: ed680bfc-18a7-4c4c-a340-26645efbe5c3
📒 Files selected for processing (10)
fact-ebpf/src/bpf/events.hfact-ebpf/src/bpf/main.cfact-ebpf/src/bpf/types.hfact-ebpf/src/lib.rsfact/src/event/mod.rsfact/src/metrics/kernel_metrics.rstests/event.pytests/test_path_rename.pytests/test_xattr.pythird_party/stackrox
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 `@tests/conftest.py`:
- Around line 20-29: The get_dockerd_process() function can raise exceptions
from the subprocess.run call when pgrep is unavailable and from the
Process.from_proc() call when there is a race condition with the process
disappearing from /proc, which causes tests to abort instead of gracefully
returning None. Wrap the subprocess.run() call and the Process.from_proc() call
in try-except blocks to catch any exceptions (such as FileNotFoundError or
exceptions from Process.from_proc when the pid no longer exists) and return None
on any failure, ensuring the function consistently returns None for all error
cases.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: c78001fd-b64b-4bf7-95e5-ad83281b51c5
📒 Files selected for processing (7)
tests/conftest.pytests/test_file_open.pytests/test_misc.pytests/test_path_chmod.pytests/test_path_chown.pytests/test_path_rename.pytests/test_path_unlink.py
Comment on lines
+20
to
+29
| def get_dockerd_process() -> Process | None: | ||
| result = subprocess.run( | ||
| ['pgrep', 'dockerd'], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if result.returncode != 0: | ||
| return None | ||
| pid = int(result.stdout.strip().split('\n')[0]) | ||
| proc = Process.from_proc(pid) |
There was a problem hiding this comment.
Harden dockerd discovery failure paths.
Line 21 and Line 29 can raise (pgrep unavailable, PID race on /proc), which aborts tests instead of gracefully returning None like the rest of this helper intends.
Suggested fix
def get_dockerd_process() -> Process | None:
- result = subprocess.run(
- ['pgrep', 'dockerd'],
- capture_output=True,
- text=True,
- )
+ try:
+ result = subprocess.run(
+ ['pgrep', 'dockerd'],
+ capture_output=True,
+ text=True,
+ check=False,
+ timeout=2,
+ )
+ except (OSError, subprocess.TimeoutExpired):
+ return None
if result.returncode != 0:
return None
- pid = int(result.stdout.strip().split('\n')[0])
- proc = Process.from_proc(pid)
+ lines = [line for line in result.stdout.splitlines() if line]
+ if not lines:
+ return None
+ try:
+ pid = int(lines[0])
+ proc = Process.from_proc(pid)
+ except (ValueError, FileNotFoundError, PermissionError):
+ return None
return Process(
pid=None,
uid=proc.uid,🧰 Tools
🪛 ast-grep (0.44.0)
[error] 20-24: Command coming from incoming request
Context: subprocess.run(
['pgrep', 'dockerd'],
capture_output=True,
text=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 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 `@tests/conftest.py` around lines 20 - 29, The get_dockerd_process() function
can raise exceptions from the subprocess.run call when pgrep is unavailable and
from the Process.from_proc() call when there is a race condition with the
process disappearing from /proc, which causes tests to abort instead of
gracefully returning None. Wrap the subprocess.run() call and the
Process.from_proc() call in try-except blocks to catch any exceptions (such as
FileNotFoundError or exceptions from Process.from_proc when the pid no longer
exists) and return None on any failure, ensuring the function consistently
returns None for all error cases.
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 `@tests/conftest.py`:
- Around line 33-38: The second pgrep lookup on lines 33-38 can return a
different dockerd process than the one identified at line 28, and the brittle
string parsing with split()[1] can raise an IndexError if the output is empty or
unexpected. Instead of running a new pgrep command that could match any dockerd
process, use the specific pid that was already determined at line 28 to get the
exe_path from that same process (such as by reading from /proc/{pid}/exe or by
incorporating the pid into the command), and add proper error handling to
gracefully handle unexpected or empty output without crashing the test setup.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 398c7110-e380-4587-b8a0-bce7827c3c91
📒 Files selected for processing (1)
tests/conftest.py
Comment on lines
+33
to
+38
| result = subprocess.run( | ||
| ['pgrep', '-a', 'dockerd'], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| exe_path = result.stdout.strip().split('\n')[0].split()[1] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid mixed process identity and brittle parsing in the second pgrep lookup.
Line 33-38 can pair exe_path from a different dockerd process than the pid chosen at Line 28, and split()[1] can raise if output is empty/unexpected. That makes the fixture flaky and can crash test setup.
Suggested fix
- result = subprocess.run(
+ result = subprocess.run(
['pgrep', '-a', 'dockerd'],
capture_output=True,
text=True,
)
- exe_path = result.stdout.strip().split('\n')[0].split()[1]
+ if result.returncode != 0:
+ return None
+
+ exe_path = None
+ for line in result.stdout.splitlines():
+ parts = line.split(maxsplit=1)
+ if len(parts) != 2:
+ continue
+ if parts[0] != str(pid):
+ continue
+ cmdline = parts[1].strip()
+ if not cmdline:
+ continue
+ exe_path = cmdline.split()[0]
+ break
+ if exe_path is None:
+ return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| result = subprocess.run( | |
| ['pgrep', '-a', 'dockerd'], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| exe_path = result.stdout.strip().split('\n')[0].split()[1] | |
| result = subprocess.run( | |
| ['pgrep', '-a', 'dockerd'], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| if result.returncode != 0: | |
| return None | |
| exe_path = None | |
| for line in result.stdout.splitlines(): | |
| parts = line.split(maxsplit=1) | |
| if len(parts) != 2: | |
| continue | |
| if parts[0] != str(pid): | |
| continue | |
| cmdline = parts[1].strip() | |
| if not cmdline: | |
| continue | |
| exe_path = cmdline.split()[0] | |
| break | |
| if exe_path is None: | |
| return None |
🤖 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 `@tests/conftest.py` around lines 33 - 38, The second pgrep lookup on lines
33-38 can return a different dockerd process than the one identified at line 28,
and the brittle string parsing with split()[1] can raise an IndexError if the
output is empty or unexpected. Instead of running a new pgrep command that could
match any dockerd process, use the specific pid that was already determined at
line 28 to get the exe_path from that same process (such as by reading from
/proc/{pid}/exe or by incorporating the pid into the command), and add proper
error handling to gracefully handle unexpected or empty output without crashing
the test setup.
JoukoVirtanen
changed the base branch from
main
to
jv-ROX-33034-track-xattr-changes
6 tasks
JoukoVirtanen
force-pushed
the
jv-ROX-33034-track-xattr-changes
branch
from
4770b44 to
5291250
Compare
JoukoVirtanen
force-pushed
the
jv-ROX-33034-track-xattr-changes-test-all-events
branch
from
24655a1 to
3a60638
Compare
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.
Description
A detailed explanation of the changes in your PR.
Feel free to remove this section if it is overkill for your PR, and the title of your PR is sufficiently descriptive.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.
For more details, ref the Confluence page about this section.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores