feat: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] - #514
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 Ansible module now collects structured role fingerprints, formats them for syslog, and optionally writes JSONL records with locking and size trimming. Unit tests cover collection, formatting, persistence, check mode, validation, errors, and timestamps. Structured fingerprint reporting
🚥 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
🤖 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 283-287: Update _format_fingerprint_key_value to escape or reject
carriage-return and newline characters in value before constructing either
output branch, ensuring formatted fields contain no literal CR or LF while
preserving existing quote escaping. Add a regression test covering field values
containing both control characters and asserting the formatted output contains
neither literal character.
- Around line 231-234: Update the log rotation logic around _trim_log_file to
remove the full excess, calculated from cur_size plus the new-line size minus
max_size, before appending whenever the write would exceed max_size. Explicitly
define the policy for a single JSONL row larger than max_size, implement that
behavior consistently, and add tests covering both existing oversized logs and
oversized individual rows.
- Around line 336-346: Update the remaining sr_fingerprint invocations in
tasks/set_vars.yml and tasks/main.yml to pass the structured arguments required
by run_module, including status, role_name, role_path, ansible_play_hosts_all,
and applicable logging or distribution fields. Remove the obsolete sr_message
argument and ensure each call matches the validation defined in
sr_fingerprint.py.
🪄 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: ae937c6b-8a36-45c8-87c4-6b8297ecc518
📒 Files selected for processing (2)
library/sr_fingerprint.pytests/unit/test_sr_fingerprint.py
| 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: | ||
| log_fd.write(new_line) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enforce max_log_size for existing oversized logs.
The trim call removes only len(new_line). If an operator lowers max_log_size below the current file size, the next write leaves the file far above the configured limit. A new row larger than max_log_size also exceeds the limit without a defined policy.
Calculate the excess as cur_size + new_line_size - max_log_size. Remove that amount before appending. Define and test the behavior when one JSONL row exceeds the limit.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 232-232: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(log_file, "a")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-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 `@library/sr_fingerprint.py` around lines 231 - 234, Update the log rotation
logic around _trim_log_file to remove the full excess, calculated from cur_size
plus the new-line size minus max_size, before appending whenever the write would
exceed max_size. Explicitly define the policy for a single JSONL row larger than
max_size, implement that behavior consistently, and add tests covering both
existing oversized logs and oversized individual rows.
| 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.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape CR and LF characters before writing syslog fields.
A value containing \n or \r does not enter the quoted branch. It can create forged syslog lines and corrupt the downstream key-value record stream. Escape or reject control characters before formatting the field. Add a regression test that asserts the output contains no literal CR or LF from field values.
🤖 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 or reject carriage-return and newline
characters in value before constructing either output branch, ensuring formatted
fields contain no literal CR or LF while preserving existing quote escaping. Add
a regression test covering field values containing both control characters and
asserting the formatted output contains neither literal character.
| def run_module(): | ||
| module_args = dict( | ||
| status=dict(type="str", required=True, choices=["begin", "success"]), | ||
| write_log_file=dict(type="bool", default=False), | ||
| log_file=dict(type="path", default="/var/log/sysroles.jsonl"), | ||
| max_log_size=dict(type="int", default=2000000), | ||
| role_name=dict(type="str", required=True), | ||
| role_path=dict(type="path", required=True), | ||
| ansible_play_hosts_all=dict(type="list", elements="str", required=True), | ||
| distribution=dict(type="str", default=""), | ||
| distribution_version=dict(type="str", default=""), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
--glob '*.yml' --glob '*.yaml' --glob '*.py' \
'(^[[:space:]]*([[:alnum:]_]+\.)?sr_fingerprint:[[:space:]]*$|^[[:space:]]*sr_message:)' \
.Repository: linux-system-roles/logging
Length of output: 2807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant fingerprint module args =="
sed -n '1,125p' library/sr_fingerprint.py
sed -n '320,380p' library/sr_fingerprint.py
echo
echo "== call sites =="
sed -n '10,18p' tasks/set_vars.yml
sed -n '138,146p' tasks/main.yml
echo
echo "== structural search for free-form dict and sr_message =="
python3 - <<'PY'
import glob, re
for p in glob.glob('library/sr_fingerprint.py'):
text=open(p, encoding='utf-8').read()
print(p, 'has sr_message arg:', 'sr_message' in text, 'status required:', 'choices=["begin", "success"]' in text)
PYRepository: linux-system-roles/logging
Length of output: 6620
Update the remaining sr_fingerprint call sites.
tasks/set_vars.yml and tasks/main.yml still call sr_fingerprint with sr_message, but library/sr_fingerprint.py now requires structured fields (status, role_name, role_path, ansible_play_hosts_all, etc.). Migrate these calls or argument validation will reject the role tasks at runtime.
🤖 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 336 - 346, Update the remaining
sr_fingerprint invocations in tasks/set_vars.yml and tasks/main.yml to pass the
structured arguments required by run_module, including status, role_name,
role_path, ansible_play_hosts_all, and applicable logging or distribution
fields. Remove the obsolete sr_message argument and ensure each call matches the
validation defined in sr_fingerprint.py.
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>
8db551a to
a078f1e
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