refactor: Add backend abstration to firewall_lib, remove obsolete firewall_lib offline code #271
Conversation
Reviewer's GuideThis PR refactors firewall_lib by introducing a unified OnlineAPIBackend for all FirewallClient operations, removing legacy offline support and related tests, and simplifying main() to delegate work to the new backend. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[citest] |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
lgtm |
martinpitt
force-pushed
the
backend-abstraction
branch
from
6ccefd5 to
e5918cb
Compare
|
I poured a bucket of black paint over this and fixed a typo, now it should pass the unit tests. Otherwise this was fairly green. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #271 +/- ##
==========================================
+ Coverage 61.09% 61.16% +0.06%
==========================================
Files 2 2
Lines 910 927 +17
==========================================
+ Hits 556 567 +11
- Misses 354 360 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
[citest] |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @martinpitt - I've reviewed your changes - here's some feedback:
- The OnlineAPIBackend class is very large and handles many responsibilities—consider splitting out per-resource handlers or moving chunks into helper modules to improve maintainability.
- You’ve duplicated definitions of PCI_REGEX and lsr_parse_version; consolidate those into a single definition at the top to avoid confusion.
- You removed the offline tests but haven’t added coverage for the new backend methods—add focused unit tests for key OnlineAPIBackend.set_* methods (including check-mode vs normal mode) to validate the abstraction.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| self.need_reload = True | ||
|
|
||
| def set_zone(self): | ||
| if self.state == "present" and not self.zone_exists: |
There was a problem hiding this comment.
issue (bug_risk): Missing initialization of fw_zone and fw_settings after creating a zone
Initialize self.fw_zone and self.fw_settings after addZone(): fetch the new zone via config().getZoneByName() and call .getSettings() to avoid None errors.
| fw_service_settings.addSourcePort(_port, _protocol) | ||
| self.changed = True | ||
| for _module in helper_module: | ||
| if fw_service_settings.queryModule(_module): |
There was a problem hiding this comment.
issue (bug_risk): Incorrect condition for adding helper modules
Flip the condition to if not fw_service_settings.queryModule(_module): so modules are added only when they don’t already exist.
| if destination_ipv4: | ||
| if not fw_service_settings.queryDestination( | ||
| "ipv4", destination_ipv4 | ||
| ): | ||
| if not self.module.check_mode: | ||
| fw_service_settings.setDestination("ipv4", destination_ipv4) | ||
| self.changed = True |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if destination_ipv4: | |
| if not fw_service_settings.queryDestination( | |
| "ipv4", destination_ipv4 | |
| ): | |
| if not self.module.check_mode: | |
| fw_service_settings.setDestination("ipv4", destination_ipv4) | |
| self.changed = True | |
| if destination_ipv4 and not fw_service_settings.queryDestination( | |
| "ipv4", destination_ipv4 | |
| ): | |
| if not self.module.check_mode: | |
| fw_service_settings.setDestination("ipv4", destination_ipv4) | |
| self.changed = True | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
| if destination_ipv6: | ||
| if not fw_service_settings.queryDestination( | ||
| "ipv6", destination_ipv6 | ||
| ): | ||
| if not self.module.check_mode: | ||
| fw_service_settings.setDestination("ipv6", destination_ipv6) | ||
| self.changed = True |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if destination_ipv6: | |
| if not fw_service_settings.queryDestination( | |
| "ipv6", destination_ipv6 | |
| ): | |
| if not self.module.check_mode: | |
| fw_service_settings.setDestination("ipv6", destination_ipv6) | |
| self.changed = True | |
| if destination_ipv6 and not fw_service_settings.queryDestination( | |
| "ipv6", destination_ipv6 | |
| ): | |
| if not self.module.check_mode: | |
| fw_service_settings.setDestination("ipv6", destination_ipv6) | |
| self.changed = True | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
| if destination_ipv4: | ||
| if fw_service_settings.queryDestination("ipv4", destination_ipv4): | ||
| if not self.module.check_mode: | ||
| fw_service_settings.removeDestination( | ||
| "ipv4", destination_ipv4 | ||
| ) | ||
| self.changed = True |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if destination_ipv4: | |
| if fw_service_settings.queryDestination("ipv4", destination_ipv4): | |
| if not self.module.check_mode: | |
| fw_service_settings.removeDestination( | |
| "ipv4", destination_ipv4 | |
| ) | |
| self.changed = True | |
| if destination_ipv4 and fw_service_settings.queryDestination("ipv4", destination_ipv4): | |
| if not self.module.check_mode: | |
| fw_service_settings.removeDestination( | |
| "ipv4", destination_ipv4 | |
| ) | |
| self.changed = True | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
| err_str = "Permanent" | ||
|
|
||
| if not zone_exists and not zone_operation: | ||
| module.fail_json(msg="%s zone '%s' does not exist." % (err_str, zone)) |
There was a problem hiding this comment.
suggestion (code-quality): Replace interpolated string formatting with f-string (replace-interpolation-with-fstring)
| module.fail_json(msg="%s zone '%s' does not exist." % (err_str, zone)) | |
| module.fail_json(msg=f"{err_str} zone '{zone}' does not exist.") |
| self.fw.setDefaultZone(zone) | ||
| self.changed = True | ||
|
|
||
| def set_service( |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Merge else clause's nested if statement into elif (
merge-else-if-into-elif) - Use f-string instead of string concatenation (
use-fstring-for-concatenation) - Low code quality found in OnlineAPIBackend.set_service - 3% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
| else: | ||
| self.module.fail_json(msg="INVALID SERVICE - " + item) | ||
|
|
||
| def set_ipset(self, ipset, description, short, ipset_type, ipset_entries): |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring) - Split conditional into multiple branches (
split-or-ifs) - Merge duplicate blocks in conditional (
merge-duplicate-blocks) - Remove redundant conditional [×2] (
remove-redundant-if) - Low code quality found in OnlineAPIBackend.set_ipset - 12% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
| "%s does not exist - ensure it is defined in a previous task before running play outside check mode" | ||
| % item |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring) - Low code quality found in OnlineAPIBackend.set_source - 23% (
low-code-quality)
| "%s does not exist - ensure it is defined in a previous task before running play outside check mode" | |
| % item | |
| f"{item} does not exist - ensure it is defined in a previous task before running play outside check mode" |
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
| self.fw_settings.removeSource(item) | ||
| self.changed = True | ||
|
|
||
| def set_interface(self, interface): |
There was a problem hiding this comment.
issue (code-quality): Low code quality found in OnlineAPIBackend.set_interface - 15% (low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
|
Yes yes sourcery.ai, I know the code isn't good. There are lots of issues, but I don't want to change the logic here. We can't use f-strings yet, this has to work on RHEL 7.
Erm, no, I didn't. |
Yeah, just downvote or delete those annoying comments. |
This code is very problematic: * It has not worked for years, at least not since RHEL 9 (probably earlier). If firewalld isn't running, this code already crashes in the `FirewallClient()` instantiation. * There haven't been any integration tests that cover this. The unit tests don't cover this as they mock the entire firewalld API, i.e. they test an API that simply does not exist. * `firewall.core.fw` is not supported API, but the internal CLI implementation. That can change at any time (and apparently did, assuming that the old code worked at *some* point). There is no offline API [1]. * Even in the current state the implementation was very incomplete. None of the service/ipset/zone/etc. settings are implemented, only some parts of zone handling (and not even that, e.g. setting the default zone is missing as well). Trying to implement the missing bits with the internal API would be a major piece of work and go in the wrong direction. Let's rather use the official methods (`firewall-offline-cmd` or XML editing). To spare the next person from falling into the "oh, offline is supported" pitfall, delete all the offline code. [1] https://issues.redhat.com/browse/RHEL-88425?focusedId=27062772&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-27062772
martinpitt
force-pushed
the
backend-abstraction
branch
from
e5918cb to
cea4a88
Compare
|
I am getting quite far in my bootc-container-test branch which implements an offline backend, and now gained enough faith in this structure. However, there are two global functions |
martinpitt
force-pushed
the
backend-abstraction
branch
from
cea4a88 to
a74b4f9
Compare
|
Fixed an asymmetry, this makes the offline implementation a bit simpler. It is a no-op (by happenstance and a code smell) with the old code. |
Introduce a backend class into firewall_lib, and move all `FirewallClient` operations into it. This gets rid of some global variables, separates the Ansible/parameter parsing from the firewalld operations, shrinks the massive main() function, and allows us to add another implementation for offline mode in the future (based on `firewalld-offline-cmd` or XML editing). This does not change the logic or code much, other than adding a truckload of `self.` prefixes and some `black` reformatting. There is still way too much global state, which makes the code hard to understand, but let's clean this up in steps. The only other changes aside from moving are the elimination of `handle_interface_permanent()` and `set_the_default_zone()` global functions. They are FirewallClient API specific, only get called once (the former with two unused parameters), and are simple enough to just get inlined.
martinpitt
force-pushed
the
backend-abstraction
branch
from
a74b4f9 to
7565dc4
Compare
|
[citest] |
2 participants
Prerequisite for adding a backend with
firewall-offline-cmdor XML editing. I'll experiment with that next. But I first want a complete test run to ensure I didn't break anything.Summary by Sourcery
Introduce an OnlineAPIBackend to centralize all firewall operations via the FirewallClient API, streamline the module’s entry point, and drop legacy offline code paths.
Enhancements:
Tests: