fix: ensure rich rule handling is idempotent

[richm]

Cause: The in-memory backend was using a key 'rich_rule' for handling the rich
rules, but the code in get_config was using 'rules_str'.

Consequence: The firewall module added the rich rule every time to the 'rich_rule'
key, ignoring the 'rules_str' key, and was not idempotent.

Fix: Ensure that the key 'rich_rule' is used everywhere. When reading the information
from the firewall api, ensure that the data under the key 'rules_str' is normalized
and added to the 'rich_rule' key.

Result: The firewall role can manage rich rules idempotently.

Signed-off-by: Rich Megginson rmeggins@redhat.com

Fixes: #357

Summary by CodeRabbit

• Improvements

  • Normalized firewall rich-rule handling so rules are stored and presented consistently as standardized strings across backends and configuration retrieval.

• Tests

  • Added/updated playbook checks to verify rich-rule presence and offline/online listing for firewall zones, including exact-match verification of an IPv4 reject rich rule.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ade748cf-fb43-4600-98ca-f611a655f3d6

📥 Commits

Reviewing files that changed from the base of the PR and between 2538193 and 4c4e619.

📒 Files selected for processing (3)

• library/firewall_lib.py
• module_utils/firewall_lsr/get_config.py
• tests/tests_zone.yml

🚧 Files skipped from review as they are similar to previous changes (3)

• module_utils/firewall_lsr/get_config.py
• library/firewall_lib.py
• tests/tests_zone.yml

---

📝 Walkthrough

Walkthrough

The pull request standardizes firewall rich rule representation by converting all configuration sources and backends to use a unified rich_rule key with normalized Rich_Rule string objects, replacing the previous rules_str field across in-memory, offline, online, and directory-based configuration paths.

Changes

Rich rule representation and storage standardization

Layer / File(s)	Summary
Configuration reading standardization module_utils/firewall_lsr/get_config.py	Adds Rich_Rule import and converts rules_str or raw rich-rule outputs into rich_rule lists of normalized strings in export, offline command parsing, online extraction, and directory parsing; includes compatibility step removing rules_str.
Backend storage normalization library/firewall_lib.py	In-memory backend now uses zone_config["rich_rule"] for enabling/disabling rich rules (appending/removing normalized strings). OfflineCLI comment updated to reflect item is a normalized string.
Test coverage for rich rule configuration tests/tests_zone.yml	Test playbook adds an IPv4 rich_rule in three setup phases, updates failed_when checks to use result is failed, and adds verification that firewall-offline-cmd --list-rich-rules shows the exact normalized rich rule.

Possibly related issues

• Rich rules always report changed — rules_str vs rich_rules key mismatch #357: This PR addresses the rich-rule key mismatch by aligning readers and backends on the rich_rule normalized-list structure.

Suggested reviewers

• spetrosi

🚥 Pre-merge checks | ✅ 4 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description Format	⚠️ Warning	PR description lacks required template sections: missing "Enhancement:" or "Feature:" and "Reason:" sections. Only "Result:" section is present from required structure.	Rewrite PR description to follow .github/pull_request_template.md: include "Enhancement:", "Reason:", and "Result:" sections as specified in the template.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title follows Conventional Commits format with type 'fix' and clearly describes the main change: ensuring idempotent rich rule handling, which aligns with the changeset.
Description check	✅ Passed	The PR description includes Cause, Consequence, Fix, Result sections and references the fixed issue, meeting the template requirements with clear technical details.
Linked Issues check	✅ Passed	All code changes directly address issue #357: aligning rich_rule key usage across get_config.py, firewall_lib.py, and tests to restore idempotent behavior and eliminate false-positive changed reports.
Out of Scope Changes check	✅ Passed	All changes are focused on resolving the key mismatch issue: get_config.py normalizes rich rules to 'rich_rule', firewall_lib.py uses the aligned key, and tests verify the fix. No unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/tests_zone.yml (1)

48-55: ⚡ Quick win

Add an explicit verification for the newly configured rich rule.

The test now configures rich_rule, but the verify block does not assert that rule is present; this can let rich-rule regressions hide behind unrelated zone changes.

✅ Suggested verification task

+        - name: Verify firewalld zone internal rich rules
+          ansible.builtin.command: firewall-offline-cmd --zone=internal --list-rich-rules
+          register: result
+          changed_when: false
+          failed_when: result.failed
+                      or 'rule family="ipv4" source address="192.0.2.0/24" reject'
+                          not in result.stdout

As per coding guidelines, "Tests should verify both success and failure scenarios" and "Use assert module to verify expected state after role execution".

Also applies to: 99-106, 153-160

🤖 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_zone.yml` around lines 48 - 55, Add an explicit assertion in the
verify block of the tests for the zone that checks the configured rich_rule
value (the 'rich_rule' entry set to 'rule family="ipv4" source
address="192.0.2.0/24" reject') so the test fails if that rule is not applied;
update the verify task(s) that currently check
zone/state/service/port/forward_port to also import and use the assert module
and assert that the actual zone rich_rule equals the expected string (apply same
change for the other occurrences mentioned around lines 99-106 and 153-160).

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@tests/tests_zone.yml`:
- Around line 48-55: Add an explicit assertion in the verify block of the tests
for the zone that checks the configured rich_rule value (the 'rich_rule' entry
set to 'rule family="ipv4" source address="192.0.2.0/24" reject') so the test
fails if that rule is not applied; update the verify task(s) that currently
check zone/state/service/port/forward_port to also import and use the assert
module and assert that the actual zone rich_rule equals the expected string
(apply same change for the other occurrences mentioned around lines 99-106 and
153-160).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d0fe3432-5764-4bc1-aa04-d20bdfddcc21

📥 Commits

Reviewing files that changed from the base of the PR and between 669b47a and 2d5f67b.

📒 Files selected for processing (3)

• library/firewall_lib.py
• module_utils/firewall_lsr/get_config.py
• tests/tests_zone.yml

[codecov]

Codecov Report

❌ Patch coverage is 0% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.99%. Comparing base (2d7c4ba) to head (4c4e619).
⚠️ Report is 167 commits behind head on main.

Files with missing lines	Patch %	Lines
module_utils/firewall_lsr/get_config.py	0.00%	13 Missing ⚠️
library/firewall_lib.py	0.00%	4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (2d7c4ba) and HEAD (4c4e619). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (2d7c4ba)	HEAD (4c4e619)
sanity	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #358       +/-   ##
===========================================
- Coverage   61.09%   42.99%   -18.11%     
===========================================
  Files           2        4        +2     
  Lines         910     2342     +1432     
===========================================
+ Hits          556     1007      +451     
- Misses        354     1335      +981     

Flag	Coverage Δ
sanity	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richm]

[citest]

[richm]

[citest]