feat: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] - #254
feat: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip]#254spetrosi wants to merge 2 commits into
Conversation
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe module now accepts structured role and host metadata, creates canonical fingerprint records, formats syslog output, and optionally writes locked JSONL logs with size-based trimming. Check mode returns data without writing. Tests cover formatting, persistence, retention, validation, and failures. ChangesStructured fingerprint logging
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/unit/test_sr_fingerprint.py (3)
131-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRun Black on this file; some wrapping looks non-canonical.
Line 133 wraps a short assignment in redundant parentheses. Black removes those parentheses when the statement fits the line limit. Similar wrapping appears at lines 206-208 and 246-248. Run
tox -e black,flake8and commit the result.♻️ Expected Black output for lines 133-135
- record["role_path"] = ( - "/usr/share/ansible/roles/linux-system-roles.systemd extra" - ) + record["role_path"] = "/usr/share/ansible/roles/linux-system-roles.systemd extra"As per path instructions: "Must follow PEP 8 and be formatted with Python Black" and "Run
tox -e black,flake8before committing".🤖 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/unit/test_sr_fingerprint.py` around lines 131 - 140, Run Black on tests/unit/test_sr_fingerprint.py and apply its canonical formatting, including removing redundant parentheses around the short role_path assignment and correcting the similar wrapping near the other referenced sections. Verify the file with tox -e black,flake8.Source: Path instructions
352-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the syslog line is emitted in the normal execution path.
No test verifies
module.log._FakeModulerecords messages inself.logged, so the assertion is cheap. Add a non-check-mode test withwrite_log_file: Falsethat assertsmodule.loggedholds one message containingrole_name=systemd. This locks in the default logging behavior the PR states it retains.🤖 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/unit/test_sr_fingerprint.py` around lines 352 - 381, Add a non-check-mode test for _handle_fingerprint using write_log_file=False, then assert _FakeModule.logged contains exactly one message including “role_name=systemd”. Preserve the existing failure test and use the normal execution path to verify the default syslog logging behavior.
195-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a trim test for a file that already exceeds
max_size.The current trim tests always start at or below the limit. They do not cover a file that is already far above
max_size, which is the case after an operator lowersmax_log_size. That case exposes the trim behavior described in the_trim_log_filecomment inlibrary/sr_fingerprint.py. Add a test that pre-writes many records with a large limit, then writes one record with a small limit, and asserts the resulting file size is at or below the small limit.🤖 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/unit/test_sr_fingerprint.py` around lines 195 - 219, Add a test alongside test_trim_removes_oldest_lines that first writes many records using a large max_size, then appends one record with a substantially smaller max_size. Assert the resulting log file size is at or below the smaller limit, covering _write_jsonl_log and _trim_log_file behavior when the existing file already exceeds the limit.library/sr_fingerprint.py (1)
200-217: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClose the temporary file descriptor if setup fails before
os.fdopen.
tempfile.mkstempreturns an open descriptor. Ifos.fchmodraises, theexcept BaseExceptionbranch removestmp_pathbut never closesfd. Close the descriptor in that branch.♻️ Proposed fix
fd, tmp_path = tempfile.mkstemp(dir=dir_name, suffix=".tmp") + fd_open = True try: os.fchmod(fd, stat.S_IMODE(orig_stat.st_mode)) try: os.fchown(fd, orig_stat.st_uid, orig_stat.st_gid) except OSError: pass with os.fdopen(fd, "w") as tmp_fd: + fd_open = False tmp_fd.writelines(lines) tmp_fd.flush() os.fsync(tmp_fd.fileno()) os.rename(tmp_path, log_file) except BaseException: + if fd_open: + try: + os.close(fd) + except OSError: + pass try: os.unlink(tmp_path) except OSError: pass raise🤖 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 `@library/sr_fingerprint.py` around lines 200 - 217, Close the descriptor returned by tempfile.mkstemp in the BaseException cleanup path when setup fails before os.fdopen. Update the exception handler around the temporary-file workflow to close fd safely, while preserving the existing tmp_path removal and exception re-raise behavior.
🤖 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 `@library/sr_fingerprint.py`:
- Around line 191-197: Update library/sr_fingerprint.py lines 191-197 in
_trim_log_file and its caller to use max_size, removing oldest records until the
remaining content plus the new record fits within that limit rather than
stopping after the new record’s size; add coverage in
tests/unit/test_sr_fingerprint.py lines 195-219 that starts with an oversized
log, writes using a smaller limit, and asserts the final file is within the
smaller limit.
- Around line 283-287: Update _format_fingerprint_key_value to neutralize
newline and carriage-return characters in values before formatting them, while
preserving the existing quoting and double-quote escaping behavior for other
special characters. Ensure playbook-derived fields cannot create multi-line
syslog records.
In `@tests/unit/test_sr_fingerprint.py`:
- Line 17: Add pytest bootstrap support for the import in test_sr_fingerprint.py
so sr_fingerprint resolves from the library directory during test collection.
Configure pytest’s import path using the repository’s existing test
configuration conventions, or update the test import to use the module’s
collection-role path; ensure the unit test runs without requiring a manual
PYTHONPATH adjustment.
---
Nitpick comments:
In `@library/sr_fingerprint.py`:
- Around line 200-217: Close the descriptor returned by tempfile.mkstemp in the
BaseException cleanup path when setup fails before os.fdopen. Update the
exception handler around the temporary-file workflow to close fd safely, while
preserving the existing tmp_path removal and exception re-raise behavior.
In `@tests/unit/test_sr_fingerprint.py`:
- Around line 131-140: Run Black on tests/unit/test_sr_fingerprint.py and apply
its canonical formatting, including removing redundant parentheses around the
short role_path assignment and correcting the similar wrapping near the other
referenced sections. Verify the file with tox -e black,flake8.
- Around line 352-381: Add a non-check-mode test for _handle_fingerprint using
write_log_file=False, then assert _FakeModule.logged contains exactly one
message including “role_name=systemd”. Preserve the existing failure test and
use the normal execution path to verify the default syslog logging behavior.
- Around line 195-219: Add a test alongside test_trim_removes_oldest_lines that
first writes many records using a large max_size, then appends one record with a
substantially smaller max_size. Assert the resulting log file size is at or
below the smaller limit, covering _write_jsonl_log and _trim_log_file behavior
when the existing file already exceeds the limit.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ce9a9378-0857-483f-88fe-dad8529dd35d
📒 Files selected for processing (2)
library/sr_fingerprint.pytests/unit/test_sr_fingerprint.py
| def _trim_log_file(log_file, size_needed): | ||
| """Remove oldest records until the file can accommodate size_needed bytes.""" | ||
| with open(log_file, "r") as log_fd: | ||
| lines = log_fd.readlines() | ||
| size_removed = 0 | ||
| while lines and size_removed < size_needed: | ||
| size_removed += len(lines.pop(0)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Retention is bounded by the new record size, not by max_size. _trim_log_file stops after it removes len(new_line) bytes, so a file that is already far above the limit shrinks by only one record per write. The tests never start above the limit, so this behavior is not detected.
library/sr_fingerprint.py#L191-L197: passmax_sizeinto_trim_log_fileand remove oldest records until the remaining size plus the new record fits withinmax_size.tests/unit/test_sr_fingerprint.py#L195-L219: add a test that pre-writes many records with a large limit, then writes one record with a small limit, and asserts the final file size is at or below the small limit.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 192-192: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(log_file, "r")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
📍 Affects 2 files
library/sr_fingerprint.py#L191-L197(this comment)tests/unit/test_sr_fingerprint.py#L195-L219
🤖 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 `@library/sr_fingerprint.py` around lines 191 - 197, Update
library/sr_fingerprint.py lines 191-197 in _trim_log_file and its caller to use
max_size, removing oldest records until the remaining content plus the new
record fits within that limit rather than stopping after the new record’s size;
add coverage in tests/unit/test_sr_fingerprint.py lines 195-219 that starts with
an oversized log, writes using a smaller limit, and asserts the final file is
within the smaller limit.
| def _format_fingerprint_key_value(field, value): | ||
| text = "" if value is None else str(value) | ||
| if any(char in text for char in ' "='): | ||
| return '%s="%s"' % (field, text.replace('"', '""')) | ||
| return "%s=%s" % (field, text) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Newlines in a value can split the syslog line.
_format_fingerprint_key_value quotes values that contain a space, a double quote, or =. It does not neutralize \n or \r. role_name, role_path, distribution, and distribution_version come from playbook variables. A value with a newline produces a multi-line syslog message, and downstream key=value parsers then read a partial record.
♻️ Proposed fix
def _format_fingerprint_key_value(field, value):
text = "" if value is None else str(value)
+ text = text.replace("\r", " ").replace("\n", " ")
if any(char in text for char in ' "='):
return '%s="%s"' % (field, text.replace('"', '""'))
return "%s=%s" % (field, text)📝 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.
| def _format_fingerprint_key_value(field, value): | |
| text = "" if value is None else str(value) | |
| if any(char in text for char in ' "='): | |
| return '%s="%s"' % (field, text.replace('"', '""')) | |
| return "%s=%s" % (field, text) | |
| def _format_fingerprint_key_value(field, value): | |
| text = "" if value is None else str(value) | |
| text = text.replace("\r", " ").replace("\n", " ") | |
| if any(char in text for char in ' "='): | |
| return '%s="%s"' % (field, text.replace('"', '""')) | |
| return "%s=%s" % (field, text) |
🤖 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 `@library/sr_fingerprint.py` around lines 283 - 287, Update
_format_fingerprint_key_value to neutralize newline and carriage-return
characters in values before formatting them, while preserving the existing
quoting and double-quote escaping behavior for other special characters. Ensure
playbook-derived fields cannot create multi-line syslog records.
| import tempfile | ||
| import unittest | ||
|
|
||
| import sr_fingerprint |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate test bootstrap and path configuration for unit tests.
fd -H -t f 'conftest.py|tox.ini|pytest.ini|setup.cfg|pyproject.toml' | while IFS= read -r f; do
echo "=== $f ==="
rg -n 'library|sys\.path|pythonpath|rootdir|testpaths|changedir' "$f" || true
doneRepository: linux-system-roles/ssh
Length of output: 176
🏁 Script executed:
#!/bin/bash
set -eu
echo "=== tracked relevant files ==="
git ls-files | rg '(^pytest\.ini$|^tox\.ini$|^setup\.cfg$|^pyproject\.toml$|conftest\.py$|^tests/unit/|^library/)' || true
echo "=== tox.ini lines ==="
if [ -f tox.ini ]; then cat -n tox.ini | sed -n '1,220p'; fi
echo "=== pytest/config bootstrap lines ==="
for f in pytest.ini setup.cfg pyproject.toml; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f" | sed -n '1,220p'
fi
done
echo "=== conftest files and content ==="
git ls-files '*conftest.py' | while IFS= read -r f; do
echo "--- $f ---"
cat -n "$f" | sed -n '1,220p'
done
echo "=== library directory outline ==="
if [ -d library ]; then
find library -maxdepth 2 -type f | sort | sed -n '1,120p'
python3 - <<'PY'
import sys, os, glob
p=os.path.abspath('library')
print("pwd", os.getcwd())
print("abs library", p)
print("sys.path_contains_library_prefix", any(x.rstrip('/')=='library' or (x+os.sep).startswith(p+os.sep) for x in sys.path))
print("import finders for module names", [x for x in [m for m in ('sr_fingerprint','ssh') if __import__('importlib.util').util.find_spec(m)==None][:1]])
PY
fi
echo "=== references to library path in test/config files ==="
rg -n "library|sys\.path|pythonpath|rootdir|testpaths|changedir|tests/unit" tox.ini pytest.ini setup.cfg pyproject.toml tests unit conftest.py 2>/dev/null || trueRepository: linux-system-roles/ssh
Length of output: 785
🏁 Script executed:
#!/bin/bash
set -eu
echo "=== repository root files ==="
git ls-files -z | tr '\0' '\n' | sed -n '1,200p'
echo "=== tox.ini full ==="
cat -n tox.ini | sed -n '1,220p'
echo "=== search for pytest/tox references ==="
rg -n "pytest|py.test|flake8|lint|lints|python|pythonpath|env\s*=|commands\s*=" tox.ini README.* .github . 2>/dev/null || trueRepository: linux-system-roles/ssh
Length of output: 15373
Add pytest bootstrap for import sr_fingerprint.
tests/unit/test_sr_fingerprint.py imports library/sr_fingerprint.py, but this repo has no conftest.py, pytest.ini, pytest tox environment, or workflow command that adds library/ to sys.path. Add the pytest/config bootstrap or import the module by its collection role path before running this unit test.
🤖 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/unit/test_sr_fingerprint.py` at line 17, Add pytest bootstrap support
for the import in test_sr_fingerprint.py so sr_fingerprint resolves from the
library directory during test collection. Configure pytest’s import path using
the repository’s existing test configuration conventions, or update the test
import to use the module’s collection-role path; ensure the unit test runs
without requiring a manual PYTHONPATH adjustment.
Feature: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] Reason: By default logs are printed to rsyslog. This change adds a possibility to write logs to a file on the system for the downstream users. Result: For the upstream, this makes rsyslog log message more detailed. For the downstream - also writes logs to /var/log/sysroles.jsonl Signed-off-by: Sergei Petrosian <spetrosi@redhat.com>
74ab1bb to
ba29d56
Compare
The sr_fingerprint module was rewritten to accept structured parameters (status, role_name, role_path, etc.) instead of a free-form sr_message. Update the role tasks and tests to match the new module interface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[citest] |
1 participant
Feature: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip]
Reason: By default logs are printed to rsyslog. This change adds a possibility to write logs to a file on the system for the downstream users.
Result: For the upstream, this makes rsyslog log message more detailed. For the downstream - also writes logs to /var/log/sysroles.jsonl