Skip to content

fix(logfiles): harden ignore rules parsing and validation - #3578

Merged
mkoura merged 3 commits into
masterfrom
logfiles_ignore_rules_hardening
Aug 3, 2026
Merged

fix(logfiles): harden ignore rules parsing and validation#3578
mkoura merged 3 commits into
masterfrom
logfiles_ignore_rules_hardening

Conversation

@mkoura

@mkoura mkoura commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

A malformed line in an ignore rules file crashed the whole log check with a ValueError. The crash happened in the cluster manager fixture teardown, so the cleanup was skipped and every subsequent log check on the cluster instance kept crashing until the rules file was manually removed. Skip malformed lines with a warning instead - skipping a rule can only lead to reporting previously ignored errors.

Validate rules when they are added, so that a hazardous rule fails the offending test loudly at the source:

  • Reject ";;" in the files glob and line breaks in the glob or the regex - they would corrupt the line-based rules file format.
  • Reject a regex that matches an empty string - as an empty branch of the combined alternation it would suppress all errors.
  • Reject a regex that doesn't compile (as str and as bytes - the log search compiles the combined regex as bytes) or that cannot be combined with other rules (e.g. global inline flags) - it would crash every log check on compile.
  • Reject a non-finite or negative expire time - it would silently turn "expire after X" into "never expire".

Apply the same checks with warning and skip when the combined ignore regex is built, so that hand-edited rules files and conflicts between rules (e.g. a redefined group name) cannot crash the log checks either. The rules are combined in sorted order, so the outcome is deterministic when rules conflict. Warnings are logged once per process, as the rules files are parsed again in every log search.

A malformed line in an ignore rules file crashed the whole log check
with a ValueError. The crash happened in the cluster manager fixture
teardown, so the cleanup was skipped and every subsequent log check on
the cluster instance kept crashing until the rules file was manually
removed. Skip malformed lines with a warning instead - skipping a rule
can only lead to reporting previously ignored errors.

Validate rules when they are added, so that a hazardous rule fails the
offending test loudly at the source:

* Reject ";;" in the files glob and line breaks in the glob or the
  regex - they would corrupt the line-based rules file format.
* Reject a regex that matches an empty string - as an empty branch of
  the combined alternation it would suppress all errors.
* Reject a regex that doesn't compile (as str and as bytes - the log
  search compiles the combined regex as bytes) or that cannot be
  combined with other rules (e.g. global inline flags) - it would
  crash every log check on compile.
* Reject a non-finite or negative expire time - it would silently turn
  "expire after X" into "never expire".

Apply the same checks with warning and skip when the combined ignore
regex is built, so that hand-edited rules files and conflicts between
rules (e.g. a redefined group name) cannot crash the log checks either.
The rules are combined in sorted order, so the outcome is deterministic
when rules conflict. Warnings are logged once per process, as the rules
files are parsed again in every log search.
@mkoura
mkoura requested a review from saratomaz as a code owner July 31, 2026 09:45
@mkoura
mkoura requested review from Copilot and removed request for saratomaz July 31, 2026 10:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Hardens ignore-rules handling in utils.logfiles to prevent malformed or hazardous ignore rules from crashing log checks, and adds targeted validation and tests for these cases.

Changes:

  • Add warn-once logging and make ignore-rules parsing resilient to malformed lines.
  • Validate ignore rules at creation time (add_ignore_rule) and skip hazardous/invalid regexes when building the combined ignore regex.
  • Extend test coverage for malformed lines, invalid rules, and hazardous regex combinations.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
framework_tests/test_logfiles.py Adds fixtures and parametrized tests verifying malformed rules are warned+skipped and hazardous rules are rejected or skipped safely.
cardano_node_tests/utils/logfiles.py Implements warn-once mechanism, resilient parsing, regex validation/combination checks, and stronger add_ignore_rule validation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cardano_node_tests/utils/logfiles.py
Comment thread cardano_node_tests/utils/logfiles.py Outdated
Comment thread cardano_node_tests/utils/logfiles.py Outdated
Comment thread cardano_node_tests/utils/logfiles.py
mkoura added 2 commits July 31, 2026 12:29
* Reject non-finite and negative expire times also when parsing the
  rules files, as hand-edited values like "nan" would silently turn
  the expire time into "never expire".
* Bound the warn-once deduplication memory with an LRU cache.
* Avoid rebuilding the valid regexes list on every iteration when
  building the combined ignore regex - append and pop on failure.
* Use explicit utf-8 encoding for consistency.
Use explicit utf-8 encoding also in the write-side regex check, fix the
warn-once docstring for the bounded LRU cache semantics, and pin the
warning deduplication with a test.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

framework_tests/test_logfiles.py:18

  • The new autouse fixture is missing a return type annotation, which makes it inconsistent with the rest of this module’s typed fixtures and reduces type-checker usefulness.
@pytest.fixture(autouse=True)
def _reset_logged_warnings():
    """Reset the warn-once dedup cache, so tests don't depend on the execution order."""
    logfiles._warn_once.cache_clear()

@mkoura
mkoura merged commit f97832d into master Aug 3, 2026
4 checks passed
@mkoura
mkoura deleted the logfiles_ignore_rules_hardening branch August 3, 2026 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants