Conversation
Avoid firewall freeze when too many packets are logged
There was a problem hiding this comment.
PR Overview
This PR introduces log limits for firewall zones and rules as part of the new "feat(firewall): add log limits" feature, addressing NethServer/nethsecurity#1105.
- Adds conditional logging configuration and default log limits in add_zone, edit_zone, and setup_rule.
- Introduces helper functions get_default_logging_options and apply_default_logging_options.
- Updates tests to verify new log limit settings and behavior in rules, zones, and firewall statistics.
Reviewed Changes
| File | Description |
|---|---|
| src/nethsec/firewall/init.py | Updates to zone and rule functions to implement log limits and default logging options. |
| tests/test_firewall.py | New tests for log limits and default logging option changes. |
| tests/test_inventory.py | Adjusted assertions for updated firewall rule counts. |
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/nethsec/firewall/init.py:863
- The conditional setting of the 'log' option in edit_zone is duplicated on line 863, which overwrites the earlier block that conditionally sets both 'log' and 'log_limit'. Consider removing the duplicate call to avoid potential conflicts.
uci.set('firewall', zone_config_name, 'log', '1' if log else '0')
There was a problem hiding this comment.
PR Overview
This PR adds logging limits for firewall zones and rules while updating default logging options and tests accordingly.
- Improved logging configuration in add_zone, edit_zone, and setup_rule functions
- Added default logging options functions and updated tests to reflect new logging limits
- Adjusted test expectations for forwarding rules counts
Reviewed Changes
| File | Description |
|---|---|
| src/nethsec/firewall/init.py | Changes to zone and rule logging configuration and default logging options |
| tests/test_firewall.py | Added tests to verify that log limits are set as expected |
| tests/test_inventory.py | Updated expected numbers for forward rules count |
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/nethsec/firewall/init.py:871
- In add_zone, the redundant call to set the 'log' field after the if/else block may override the previously set log_limit value. Consider removing this duplicate call to preserve the intended logging configuration.
uci.set('firewall', zone_config_name, 'log', '1' if log else '0')
src/nethsec/firewall/init.py:871
- In edit_zone, there is a redundant call that resets the 'log' field after the if/else block, potentially undoing the custom log_limit configuration. Removing this extra call will ensure the logging options remain consistent.
uci.set('firewall', zone_config_name, 'log', '1' if log else '0')
There was a problem hiding this comment.
PR Overview
This pull request adds log limit functionality to firewall zones and rules by updating the log configuration logic and introducing default logging options. The changes include refactoring log configuration in add_zone, edit_zone, and setup_rule; adding new functions get_default_logging_options and apply_default_logging_options; and updating tests to verify the new log limit behavior.
Reviewed Changes
| File | Description |
|---|---|
| src/nethsec/firewall/init.py | Refactored log flag handling and log_limit assignment for zones and rules; added default logging options functions. |
| tests/test_firewall.py | Added tests for verifying default log limits and custom log limit behavior. |
| tests/test_inventory.py | Updated expected forward rule counts in firewall statistics. |
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
NethServer/nethsecurity#1105