linux-system-roles / firewall

Configure firewalld and system-config-firewall
https://linux-system-roles.github.io/firewall/
GNU General Public License v2.0
57 stars 32 forks source link

fix: unmask firewalld on run, disable conflicting services #154

Closed BrennanPaciorek closed 1 year ago

BrennanPaciorek commented 1 year ago

Enhancement: Role will now always attempt to unmask on role execution

add variable 'firewall_disable_conflicting_services' to give the option of disabling of known conflicting services

Update README to document the following behavior of the system role:

Reason: role currently fails if firewalld was masked on run conflicting services have the potential to cause errors on role run

Result:

Issue Tracker Tickets (Jira or BZ if any):

codecov[bot] commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (156614e) 53.62% compared to head (a586d09) 53.62%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #154 +/- ## ======================================= Coverage 53.62% 53.62% ======================================= Files 2 2 Lines 800 800 ======================================= Hits 429 429 Misses 371 371 ``` | Flag | Coverage Δ | | |---|---|---| | sanity | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linux-system-roles#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

BrennanPaciorek commented 1 year ago

This may have become too complex to be just a bug fix, but if we are disabling nftables, I figured it would make sense to disable all known conflicting services and make it an argument for the role to reduce overhead in the cases where doing so is not necessary.

I can additionally separate the two fixes into separate PRs if necessary.

richm commented 1 year ago

This may have become too complex to be just a bug fix, but if we are disabling nftables, I figured it would make sense to disable all known conflicting services and make it an argument for the role to reduce overhead in the cases where doing so is not necessary.

I can additionally separate the two fixes into separate PRs if necessary.

I think it's fine to have a single pr

richm commented 1 year ago

[citest]

richm commented 1 year ago

[citest]

richm commented 1 year ago

We need to move the test from tests_default.yml - that test is only intended to run the role with no parameters. If you can alter another test, please do, otherwise, we will require a new test file.

BrennanPaciorek commented 1 year ago

We need to move the test from tests_default.yml - that test is only intended to run the role with no parameters. If you can alter another test, please do, otherwise, we will require a new test file.

Done. (new test file created, tests_default.yml reverted)

richm commented 1 year ago

[citest]