feat: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] - #334
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:
📝 WalkthroughWalkthroughChangesThe module now collects structured role fingerprints, formats them for syslog, and optionally writes locked, trimmed JSONL records. The interface replaces Structured fingerprint logging
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 1
🧹 Nitpick comments (7)
tests/unit/test_sr_fingerprint.py (4)
172-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the cleanup so it cannot mask a test failure.
If
_write_jsonl_lograises before it createssubdir,os.listdir(subdir)in thefinallyblock raisesFileNotFoundError. That error replaces the original failure and hides the cause. The other tests in this file avoid the problem by using_cleanup_log, which swallowsOSError.♻️ Proposed change
finally: subdir = os.path.dirname(log_file) - for name in os.listdir(subdir): - os.unlink(os.path.join(subdir, name)) - os.rmdir(subdir) - os.rmdir(tmpdir) + shutil.rmtree(tmpdir, ignore_errors=True)This needs
import shutilat the top of the file.shutil.rmtreealso removes the.locksidecar.🤖 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 172 - 177, Update the finally cleanup in the test around _write_jsonl_log to use shutil.rmtree on the temporary directory, adding the required shutil import, so missing subdirectories do not mask the original test failure and the .lock sidecar is removed.
131-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the Black formatting, and cover the remaining escape branches.
Two points:
- Lines 133-135 wrap the assignment in parentheses. The single-line form is about 80 characters, which is under the Black default of 88, so
tox -e blackcollapses it and reports this file as unformatted. Runtox -e black,flake8as required by the path instructions._format_fingerprint_key_valuehas three output branches. This test covers only the space branch. The=-in-value branch and the quote-escaping branch are untested. Those branches decide how downstreamkey=valueparsers read the record.♻️ Proposed change
def test_format_fingerprint_syslog_quotes_values_with_spaces(self): record = _sample_fingerprint_record() - record["role_path"] = ( - "/usr/share/ansible/roles/linux-system-roles.systemd extra" - ) + record["role_path"] = "/usr/share/ansible/roles/linux-system-roles.systemd extra" message = sr_fingerprint._format_fingerprint_syslog(record) self.assertIn( 'role_path="/usr/share/ansible/roles/linux-system-roles.systemd extra"', message, ) + + def test_format_fingerprint_key_value_quotes_equals_and_escapes_quotes(self): + self.assertEqual( + sr_fingerprint._format_fingerprint_key_value("role_name", "a=b"), + 'role_name="a=b"', + ) + # Update the expected text if the escape convention changes. + self.assertEqual( + sr_fingerprint._format_fingerprint_key_value("role_name", 'a"b'), + 'role_name="a""b"', + ) + + def test_format_fingerprint_key_value_handles_none(self): + self.assertEqual( + sr_fingerprint._format_fingerprint_key_value("role_name", None), + "role_name=", + )The replacement line for
role_pathis 82 characters, which stays under the Black limit.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, Update test_format_fingerprint_syslog_quotes_values_with_spaces to use Black’s single-line assignment for role_path, then add focused assertions covering _format_fingerprint_key_value values containing “=” and embedded quotes, verifying the expected escaping and parser-safe output. Run tox -e black,flake8 to validate formatting and linting.Source: Path instructions
195-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test that the trim preserves the file mode.
_trim_log_filereplaces the log throughtempfile.mkstempplusos.rename, and restores the mode withos.fchmod.mkstempcreates files with mode0600, so a regression in thatfchmodcall silently changes the permissions of the live log. No test covers it.♻️ Proposed test
def test_trim_preserves_file_mode(self): with tempfile.NamedTemporaryFile(delete=False, suffix=".jsonl") as tmp: log_file = tmp.name try: os.chmod(log_file, 0o640) record = _sample_fingerprint_record() line_size = len(sr_fingerprint._format_fingerprint_jsonl(record) + "\n") max_size = line_size * 3 for _i in range(6): sr_fingerprint._write_jsonl_log(log_file, record, max_size=max_size) mode = stat.S_IMODE(os.stat(log_file).st_mode) self.assertEqual(mode, 0o640) finally: _cleanup_log(log_file)This needs
import statat the top of the file.🤖 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 import stat and add a test method test_trim_preserves_file_mode alongside test_trim_removes_oldest_lines. Set the log file to mode 0o640, write enough records through _write_jsonl_log to trigger _trim_log_file, then assert stat.S_IMODE(os.stat(log_file).st_mode) remains 0o640 and clean up the temporary file in finally.
306-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the normal-execution success path.
The four handler tests cover check mode, the write failure, and the validation failure. No test covers the main path this PR adds:
write_log_file=trueoutside check mode, where_handle_fingerprintcallsmodule.logand then appends a real line to the log file._FakeModule.loggedexists at line 36 but no test reads it.♻️ Proposed test
def test_handle_fingerprint_writes_log_and_syslog(self): log_path = os.path.join(tempfile.mkdtemp(), "fingerprint.jsonl") module = _FakeModule( { "status": "success", "write_log_file": True, "log_file": log_path, "max_log_size": 2000000, "role_name": "systemd", "role_path": "/usr/share/ansible/roles/linux-system-roles.systemd", "ansible_play_hosts_all": ["host1", "host2"], "distribution": "RedHat", "distribution_version": "9.4", }, check_mode=False, ) try: with self.assertRaises(_ExitJsonException) as ctx: sr_fingerprint._handle_fingerprint(module) result = ctx.exception.kwargs self.assertFalse(result["changed"]) self.assertEqual(result["fingerprint"]["status"], "success") self.assertNotIn("jsonl_row", result) self.assertEqual(len(module.logged), 1) self.assertIn("role_name=systemd", module.logged[0]) self.assertIn("status=success", module.logged[0]) with open(log_file_dir_safe(log_path), "r") as log_fd: lines = log_fd.read().splitlines() self.assertEqual(len(lines), 1) parsed = json.loads(lines[0]) self.assertEqual(parsed, result["fingerprint"]) self.assertEqual(parsed["play_hosts_number"], 2) self.assertFalse(parsed["ansible_check_mode"]) finally: shutil.rmtree(os.path.dirname(log_path), ignore_errors=True)Replace
log_file_dir_safe(log_path)withlog_path; that placeholder is not needed. The test also needsimport shutil. It asserts the two outputs stay consistent: the syslog text and the JSONL record derive from the same fingerprint thatexit_jsonreturns.🤖 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 306 - 402, Add a normal-execution success test near the existing _handle_fingerprint tests, using write_log_file=True and a temporary log path. Assert _ExitJsonException returns unchanged status and no jsonl_row, verify module.logged contains one entry with the role and status, then read log_path directly and confirm the single JSONL record equals result["fingerprint"] and includes two hosts with ansible_check_mode false. Import shutil and clean up the temporary directory in a finally block.library/sr_fingerprint.py (3)
220-237: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCreate the log file and lock sidecar with restrictive permissions.
open(log_file, "a")andopen(lock_path, "w")create new files with0666 & ~umask, which is normally0644. The log records role names, role paths, host counts, and the managed node distribution for every play. Other log files under/var/lognormally use0600or0640. Set the mode explicitly on creation so the umask of the Ansible connection does not decide it.♻️ Proposed change to set an explicit mode
def _write_jsonl_log(log_file, record, max_size=0): _ensure_parent_dir(log_file) new_line = _format_fingerprint_jsonl(record) + "\n" lock_path = log_file + ".lock" - lock_fd = open(lock_path, "w") + lock_fd = os.fdopen( + os.open(lock_path, os.O_WRONLY | os.O_CREAT, 0o600), "w" + ) try: fcntl.flock(lock_fd, fcntl.LOCK_EX) try: cur_size = os.path.getsize(log_file) except OSError: cur_size = 0 if max_size > 0 and cur_size + len(new_line) > max_size and cur_size > 0: _trim_log_file(log_file, len(new_line)) - with open(log_file, "a") as log_fd: + with os.fdopen( + os.open(log_file, os.O_WRONLY | os.O_APPEND | os.O_CREAT, 0o600), "a" + ) as log_fd: log_fd.write(new_line)Note that
_trim_log_filepreserves the existing mode throughos.fchmod, so the mode set at creation stays stable across trims.🤖 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 220 - 237, Update _write_jsonl_log to create both log_file and lock_path with explicitly restrictive permissions, such as 0600, rather than relying on the process umask. Apply the mode only when each file is first created, preserving existing permissions for already-created files and the current locking, writing, and trimming behavior.
328-331: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWiden the exception handling around the log write.
In Python 3,
IOErroris an alias ofOSError, so this tuple catches onlyOSError._trim_log_fileopens the existing log in text mode without an explicit encoding. If the log holds bytes that the locale codec cannot decode,readlines()raisesUnicodeDecodeError, which derives fromValueError. The module then aborts with a traceback instead of a cleanfail_json.♻️ Proposed change
- except (IOError, OSError) as exc: + except (OSError, ValueError) as exc: module.fail_json( msg="Failed to write fingerprint log file %s: %s" % (log_file, exc) )Opening the file in
_trim_log_filewith an explicitencoding="utf-8"anderrors="replace"would also remove the decode failure at the source.🤖 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 328 - 331, Widen the exception handling around the `_trim_log_file` log-write path to include `UnicodeDecodeError` (or its appropriate `ValueError` base) so decode failures reach `module.fail_json` with the existing file context. Also open the trimmed log with explicit UTF-8 encoding and replacement error handling to prevent locale-dependent decode failures.
283-287: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winEscape control characters, and reconsider the quote escape convention.
Two points about
_format_fingerprint_key_value:
- No field strips or escapes newlines. If any value contains
\n,module.log()emits a record that a syslog reader splits into two lines. A craftedrole_nameorrole_pathcan then forge a second fingerprint entry. The values originate from role variables today, so the risk is low, but the guard is cheap.- The function escapes
"by doubling it. The common convention forkey=valuesyslog text, including logfmt and rsyslog field parsers, is backslash escaping. A value with a quote becomesrole_name="a""b", which mostkey=valueparsers read as the valueafollowed by a parse error. The PR targets upstream consumers of these messages, so the escape convention affects them.♻️ Proposed change
def _format_fingerprint_key_value(field, value): text = "" if value is None else str(value) + text = text.replace("\\", "\\\\") + for char, escape in (("\n", "\\n"), ("\r", "\\r"), ("\t", "\\t")): + text = text.replace(char, escape) if any(char in text for char in ' "='): - return '%s="%s"' % (field, text.replace('"', '""')) + return '%s="%s"' % (field, text.replace('"', '\\"')) return "%s=%s" % (field, text)If the doubling convention is deliberate and matches an existing downstream parser, keep it and add a comment that records the parser. Update
test_format_fingerprint_syslog_quotes_values_with_spacesintests/unit/test_sr_fingerprint.pyand add a case for a value that contains a quote and a newline.🤖 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 escape control characters, including newlines, so values cannot inject additional syslog records, and use backslash escaping for embedded quotes to match downstream key=value parsers. Update test_format_fingerprint_syslog_quotes_values_with_spaces and add coverage for values containing both a quote and a newline; if quote doubling is intentionally required by an existing parser, preserve it and document that parser instead.
🤖 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/unit/test_sr_fingerprint.py`:
- Line 17: Configure the test environment so pytest can resolve the library
directory when collecting tests/unit/test_sr_fingerprint.py and importing
sr_fingerprint. Add the repository’s established pytest, conftest.py, or tox
path configuration for library/, preserving the existing test import without
changing the test module itself.
---
Nitpick comments:
In `@library/sr_fingerprint.py`:
- Around line 220-237: Update _write_jsonl_log to create both log_file and
lock_path with explicitly restrictive permissions, such as 0600, rather than
relying on the process umask. Apply the mode only when each file is first
created, preserving existing permissions for already-created files and the
current locking, writing, and trimming behavior.
- Around line 328-331: Widen the exception handling around the `_trim_log_file`
log-write path to include `UnicodeDecodeError` (or its appropriate `ValueError`
base) so decode failures reach `module.fail_json` with the existing file
context. Also open the trimmed log with explicit UTF-8 encoding and replacement
error handling to prevent locale-dependent decode failures.
- Around line 283-287: Update _format_fingerprint_key_value to escape control
characters, including newlines, so values cannot inject additional syslog
records, and use backslash escaping for embedded quotes to match downstream
key=value parsers. Update
test_format_fingerprint_syslog_quotes_values_with_spaces and add coverage for
values containing both a quote and a newline; if quote doubling is intentionally
required by an existing parser, preserve it and document that parser instead.
In `@tests/unit/test_sr_fingerprint.py`:
- Around line 172-177: Update the finally cleanup in the test around
_write_jsonl_log to use shutil.rmtree on the temporary directory, adding the
required shutil import, so missing subdirectories do not mask the original test
failure and the .lock sidecar is removed.
- Around line 131-140: Update
test_format_fingerprint_syslog_quotes_values_with_spaces to use Black’s
single-line assignment for role_path, then add focused assertions covering
_format_fingerprint_key_value values containing “=” and embedded quotes,
verifying the expected escaping and parser-safe output. Run tox -e black,flake8
to validate formatting and linting.
- Around line 195-219: Add import stat and add a test method
test_trim_preserves_file_mode alongside test_trim_removes_oldest_lines. Set the
log file to mode 0o640, write enough records through _write_jsonl_log to trigger
_trim_log_file, then assert stat.S_IMODE(os.stat(log_file).st_mode) remains
0o640 and clean up the temporary file in finally.
- Around line 306-402: Add a normal-execution success test near the existing
_handle_fingerprint tests, using write_log_file=True and a temporary log path.
Assert _ExitJsonException returns unchanged status and no jsonl_row, verify
module.logged contains one entry with the role and status, then read log_path
directly and confirm the single JSONL record equals result["fingerprint"] and
includes two hosts with ansible_check_mode false. Import shutil and clean up the
temporary directory in a finally block.
🪄 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: faedb1f8-84b0-4cc5-aef8-b90c2184657d
📒 Files selected for processing (2)
library/sr_fingerprint.pytests/unit/test_sr_fingerprint.py
| import tempfile | ||
| import unittest | ||
|
|
||
| import sr_fingerprint |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the test runner configuration and any sys.path setup for library/.
fd -H -t f -g 'tox.ini' -g 'pytest.ini' -g 'setup.cfg' -g 'pyproject.toml' -g 'conftest.py' .
echo "--- path setup references ---"
rg -n 'PYTHONPATH|pythonpath|sys\.path|rootdir|testpaths|library' \
-g 'tox.ini' -g 'pytest.ini' -g 'setup.cfg' -g 'pyproject.toml' -g 'conftest.py' \
-g '.github/workflows/*' .Repository: linux-system-roles/metrics
Length of output: 495
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
import ast
from pathlib import Path
for filename in ['tests/unit/test_sr_fingerprint.py', 'tox.ini']:
p = Path(filename)
print(f'--- {filename}: exists={p.exists()}, size={p.stat().st_size if p.exists() else None} ---')
if p.exists():
print(p.read_text())
print()
# Locate pytest-related files and inspect relevant contents without executing repo code.
for p in sorted(Path('.').rglob('*')):
if p.is_file() and p.name in {'pytest.ini', 'setup.cfg', 'pyproject.toml', 'conftest.py'}:
try:
text = p.read_text(errors='replace')
except Exception:
continue
if any(s in text for s in ['pythonpath', 'python_paths', 'rootdir', 'testpaths', 'collect_ignore', 'addopts', 'sys.path', 'PYTHONPATH', 'library', 'sr_fingerprint']):
print(f'--- potential config: {p} ---')
print(text)
</PY
echo "--- direct pytest/action workflow files ---"
git ls-files | rg '(^|/)(tox\.ini|pytest\.ini|setup\.cfg|pyproject\.toml|\.github/workflows/.*\.ya?ml|conftest\.py)$' || true
echo "--- search for sys.path setup and python package paths ---"
rg -n -C 2 'sys\.path|pythonpath|PYTHONPATH|python_paths|rootdir|testpaths|collect_ignore|library/st*|sr_fingerprint' \
-g 'tox.ini' -g 'pytest.ini' -g 'setup.cfg' -g 'pyproject.toml' \
-g '.github/workflows/*' \
-g 'conftest.py' \
.
echo "--- tox actions/scripts ---"
sed -n '1,220p' tox.ini 2>/dev/null || trueRepository: linux-system-roles/metrics
Length of output: 320
🏁 Script executed:
#!/bin/bash
set -u
echo "--- tracked pytest/config/workflow files ---"
git ls-files | rg '(^|/)(tox\.ini|pytest\.ini|setup\.cfg|pyproject\.toml|conftest\.py$|\.github/workflows/.*\.ya?ml)$' || true
echo "--- tox.ini contents ---"
if [ -f tox.ini ]; then
sed -n '1,240p' tox.ini
else
echo "tox.ini not found"
fi
echo "--- pytest.ini, setup.cfg, pyproject.toml contents ---"
for f in pytest.ini setup.cfg pyproject.toml; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,240p' "$f"
else
echo "--- $f not found ---"
fi
done
echo "--- conftest contents ---"
if [ -f conftest.py ]; then
sed -n '1,240p' conftest.py
else
echo "conftest.py not found"
fi
echo "--- path setup references in config/workflows/context ---"
rg -n -C 2 'sys\.path|sys-path|pythonpath|python_paths|PYTHONPATH|rootdir|testpaths|collect_ignore|library|sr_fingerprint|pytest' \
tox.ini pytest.ini setup.cfg pyproject.toml conftest.py .github/workflows . 2>/dev/null | head -n 240 || trueRepository: linux-system-roles/metrics
Length of output: 18123
🏁 Script executed:
#!/bin/bash
set -u
echo "--- .github/workflows files referencing tox/pytest/ansible-test/pythonpath ---"
for f in .github/workflows/*.yml .github/workflows/*.yaml; do
[ -f "$f" ] || continue
if rg -q 'tox|pytest|ansible-test|pythonpath|PYTHONPATH|lsr_enable|lsr_unit|unit' "$f"; then
echo "--- $f ---"
sed -n '1,220p' "$f"
fi
done
echo "--- read-only import lookup simulation for tests unit runner cwd scenarios ---"
python3 - <<'EOF'
from pathlib import Path
root = Path.cwd()
examples = [
("current", [root]),
("pytest tests/unit", [root / "tests" / "unit"]),
("pytest tests", [root / "tests"]),
]
for name, path_list in examples:
import sys
sys.path[:] = [str(p) for p in path_list]
try:
import importlib.util
spec = importlib.util.find_spec("sr_fingerprint")
print(f"{name}: found={bool(spec)}")
except ModuleNotFoundError:
print("current: found=False")
EOFRepository: linux-system-roles/metrics
Length of output: 16321
Add a pytest import path for library/sr_fingerprint.
tests/unit/test_sr_fingerprint.py imports sr_fingerprint, but this repository does not include a pytest.ini, setup.cfg, pyproject.toml, conftest.py, or tox/workflow path setup for library/. Add a pytest pythonpath/python_paths entry, sys.path setup in conftest.py, or a tox path variable so collection does not fail when running the import under tests/unit.
🤖 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, Configure the test environment
so pytest can resolve the library directory when collecting
tests/unit/test_sr_fingerprint.py and importing sr_fingerprint. Add the
repository’s established pytest, conftest.py, or tox path configuration for
library/, preserving the existing test import without changing the test module
itself.
Source: Path instructions
spetrosi
changed the title
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>
c1024a5 to
3dc5059
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] |
2 participants
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