feat: richm fprint - #233
Conversation
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>
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>
Signed-off-by: Rich Megginson <rmeggins@redhat.com>
|
Important Review skippedDraft detected. 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:
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #233 +/- ##
==========================================
+ Coverage 78.43% 87.17% +8.74%
==========================================
Files 2 3 +1
Lines 255 468 +213
==========================================
+ Hits 200 408 +208
- Misses 55 60 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| os.fchmod(fd, stat.S_IMODE(orig_stat.st_mode)) | ||
| try: | ||
| os.fchown(fd, orig_stat.st_uid, orig_stat.st_gid) | ||
| except OSError: |
| except BaseException: | ||
| try: | ||
| os.unlink(tmp_path) | ||
| except OSError: |
| def _cleanup_log(log_file): | ||
| for path in (log_file, log_file + ".lock"): | ||
| try: | ||
| os.unlink(path) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@defaults/main.yml`:
- Around line 12-13: Add a README.md entry for the user-facing variable
bootloader_write_log_file, documenting its boolean type, default value of false,
and effect on writing logs to sysroles.jsonl. Include the required warning that
role metadata may be stored on disk when secure logging is enabled.
In `@library/sr_fingerprint.py`:
- Around line 191-232: Update _write_jsonl_log to calculate the required
reduction as cur_size + len(new_line) - max_size, and pass that value to
_trim_log_file instead of len(new_line). Apply trimming whenever max_size is
exceeded, including an empty file with an oversized record, while preserving
normal writes when the limit is not exceeded.
In `@tasks/set_vars.yml`:
- Around line 15-16: Guard the distribution fact lookups in both fingerprint
tasks with an empty-string default: update tasks/set_vars.yml lines 15-16 and
tasks/main.yml lines 186-187 for the begin and success fingerprint tasks,
respectively. Apply the default to both distribution and distribution_version
while preserving the existing module parameter flow.
In `@tests/tests_default.yml`:
- Around line 39-58: Make the test rerunnable by removing
/var/log/sysroles.jsonl and its .lock sidecar before the “Run with default
parameters” task. Wrap that role execution and its journal/log assertions in a
block with an always cleanup section that removes both files, sets failed_when:
false, and applies the tests::cleanup tag.
In `@tests/unit/test_sr_fingerprint.py`:
- Around line 131-140: Collapse the wrapped role_path assignment in
test_format_fingerprint_syslog_quotes_values_with_spaces into a single-line
assignment, preserving the existing string value and test behavior.
🪄 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: c7429eb6-14ba-4ca2-a593-9577f736e054
📒 Files selected for processing (6)
defaults/main.ymllibrary/sr_fingerprint.pytasks/main.ymltasks/set_vars.ymltests/tests_default.ymltests/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)) | ||
| orig_stat = os.stat(log_file) | ||
| dir_name = os.path.dirname(log_file) or "." | ||
| fd, tmp_path = tempfile.mkstemp(dir=dir_name, suffix=".tmp") | ||
| 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: | ||
| tmp_fd.writelines(lines) | ||
| tmp_fd.flush() | ||
| os.fsync(tmp_fd.fileno()) | ||
| os.rename(tmp_path, log_file) | ||
| except BaseException: | ||
| try: | ||
| os.unlink(tmp_path) | ||
| except OSError: | ||
| pass | ||
| raise | ||
|
|
||
|
|
||
| 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") | ||
| 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)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim does not enforce max_log_size for an already oversized file.
_write_jsonl_log calls _trim_log_file(log_file, len(new_line)), and _trim_log_file stops after it removes len(new_line) bytes. The final size is then cur_size - removed + len(new_line), which stays at about cur_size. If cur_size already exceeds max_size, the file never returns to the limit.
Two paths reach that state:
- A single record larger than
max_sizeis written while the file is empty, because line 231 requirescur_size > 0. - The file was written earlier with a larger
max_log_sizeor with trimming disabled.
Pass the required reduction instead of the new line size.
🔧 Proposed fix to trim down to the limit
- if max_size > 0 and cur_size + len(new_line) > max_size and cur_size > 0:
- _trim_log_file(log_file, len(new_line))
+ if max_size > 0 and cur_size + len(new_line) > max_size and cur_size > 0:
+ _trim_log_file(log_file, cur_size + len(new_line) - max_size)📝 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 _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)) | |
| orig_stat = os.stat(log_file) | |
| dir_name = os.path.dirname(log_file) or "." | |
| fd, tmp_path = tempfile.mkstemp(dir=dir_name, suffix=".tmp") | |
| 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: | |
| tmp_fd.writelines(lines) | |
| tmp_fd.flush() | |
| os.fsync(tmp_fd.fileno()) | |
| os.rename(tmp_path, log_file) | |
| except BaseException: | |
| try: | |
| os.unlink(tmp_path) | |
| except OSError: | |
| pass | |
| raise | |
| 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") | |
| 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)) | |
| 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)) | |
| orig_stat = os.stat(log_file) | |
| dir_name = os.path.dirname(log_file) or "." | |
| fd, tmp_path = tempfile.mkstemp(dir=dir_name, suffix=".tmp") | |
| 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: | |
| tmp_fd.writelines(lines) | |
| tmp_fd.flush() | |
| os.fsync(tmp_fd.fileno()) | |
| os.rename(tmp_path, log_file) | |
| except BaseException: | |
| try: | |
| os.unlink(tmp_path) | |
| except OSError: | |
| pass | |
| raise | |
| 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") | |
| 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, cur_size + len(new_line) - max_size) |
🧰 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)
[warning] 223-223: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(lock_path, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[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)
🪛 GitHub Check: CodeQL
[notice] 205-205: Empty except
'except' clause does nothing but pass and there is no explanatory comment.
[notice] 215-215: Empty except
'except' clause does nothing but pass and there is no explanatory comment.
🤖 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 - 232, Update _write_jsonl_log to
calculate the required reduction as cur_size + len(new_line) - max_size, and
pass that value to _trim_log_file instead of len(new_line). Apply trimming
whenever max_size is exceeded, including an empty file with an oversized record,
while preserving normal writes when the limit is not exceeded.
| distribution: "{{ ansible_facts['distribution'] }}" | ||
| distribution_version: "{{ ansible_facts['distribution_version'] }}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unguarded distribution fact lookups in both fingerprint tasks. Both sr_fingerprint tasks read ansible_facts['distribution'] and ansible_facts['distribution_version'] without a default. The setup task in tasks/set_vars.yml runs only when required facts are missing, so a pre-gathered fact subset that excludes distribution makes these lookups undefined and fails the task. The module already defaults both parameters to "".
tasks/set_vars.yml#L15-L16: add| d('')to both fact lookups in the begin fingerprint task.tasks/main.yml#L186-L187: add| d('')to both fact lookups in the success fingerprint task.
📍 Affects 2 files
tasks/set_vars.yml#L15-L16(this comment)tasks/main.yml#L186-L187
🤖 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 `@tasks/set_vars.yml` around lines 15 - 16, Guard the distribution fact lookups
in both fingerprint tasks with an empty-string default: update
tasks/set_vars.yml lines 15-16 and tasks/main.yml lines 186-187 for the begin
and success fingerprint tasks, respectively. Apply the default to both
distribution and distribution_version while preserving the existing module
parameter flow.
| - name: Check that the log file was written | ||
| ansible.builtin.slurp: | ||
| path: /var/log/sysroles.jsonl | ||
| register: __register_log_file | ||
|
|
||
| - name: Show the log file contents | ||
| ansible.builtin.debug: | ||
| msg: "{{ __register_log_file.content | b64decode }}" | ||
|
|
||
| - name: Check contents of the log file | ||
| ansible.builtin.assert: | ||
| that: | ||
| - _log_items | length >= 2 | ||
| - _log_items[0].role_name == _role_name | ||
| - _log_items[0].status == "begin" | ||
| - _log_items[1].role_name == _role_name | ||
| - _log_items[1].status == "success" | ||
| vars: | ||
| _log_items: "{{ (__register_log_file.content | b64decode).split('\n') | select | map('from_json') | list }}" | ||
| _role_name: bootloader |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add cleanup for /var/log/sysroles.jsonl and make the test rerunnable.
The test enables bootloader_write_log_file and then asserts on the first two records of /var/log/sysroles.jsonl. The file is never removed, so on a second run against the same host _log_items[0] and _log_items[1] are records from the earlier run. The assertions then pass without checking the current run.
Remove the log file and its .lock sidecar before the role runs, and place the assertions in a block with an always cleanup section tagged tests::cleanup.
🧪 Proposed test restructure
- name: Remove any previous fingerprint log file
ansible.builtin.file:
path: "{{ item }}"
state: absent
loop:
- /var/log/sysroles.jsonl
- /var/log/sysroles.jsonl.lockPlace that task before the "Run with default parameters" task, then wrap the role run and the checks:
- name: Run the role and verify fingerprints
block:
- name: Run with default parameters
ansible.builtin.include_tasks: tasks/run_role_with_clear_facts.yml
vars:
bootloader_write_log_file: true
# existing journal and log file checks
always:
- name: Clean up the fingerprint log file
ansible.builtin.file:
path: "{{ item }}"
state: absent
loop:
- /var/log/sysroles.jsonl
- /var/log/sysroles.jsonl.lock
failed_when: false
tags:
- tests::cleanupAs per path instructions: "Tests should be run in a block with an always section that runs the test cleanup" and "The cleanup tasks should be tagged with tests::cleanup". Based on learnings, cleanup tasks in an always block use failed_when: false so cleanup failures do not fail the test run.
🤖 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/tests_default.yml` around lines 39 - 58, Make the test rerunnable by
removing /var/log/sysroles.jsonl and its .lock sidecar before the “Run with
default parameters” task. Wrap that role execution and its journal/log
assertions in a block with an always cleanup section that removes both files,
sets failed_when: false, and applies the tests::cleanup tag.
Sources: Path instructions, Learnings
| 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" | ||
| ) | ||
| message = sr_fingerprint._format_fingerprint_syslog(record) | ||
| self.assertIn( | ||
| 'role_path="/usr/share/ansible/roles/linux-system-roles.systemd extra"', | ||
| message, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eo pipefail
pip install --quiet black >/dev/null 2>&1
fd -e py . library tests | xargs black --check --diffRepository: linux-system-roles/bootloader
Length of output: 167
Collapse the wrapped role_path assignment before committing.
The value fits on one line under Black’s line-length limit, so Black will reformat this without the extra parentheses. Use this form to satisfy the Python/Black formatting requirement:
🎨 Proposed formatting fix
- record["role_path"] = (
- "/usr/share/ansible/roles/linux-system-roles.systemd extra"
- )
+ record["role_path"] = "/usr/share/ansible/roles/linux-system-roles.systemd extra"[simple_effort_and_high_reward]
📝 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 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" | |
| ) | |
| 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_syslog_quotes_values_with_spaces(self): | |
| record = _sample_fingerprint_record() | |
| 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, | |
| ) |
🤖 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, Collapse the
wrapped role_path assignment in
test_format_fingerprint_syslog_quotes_values_with_spaces into a single-line
assignment, preserving the existing string value and test behavior.
Source: Path instructions
|
[citest] |
|
fixes incorporated |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes