Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Jul 13, 2023

Conversation

BrennanPaciorek
Copy link
Collaborator

@BrennanPaciorek BrennanPaciorek commented Jul 12, 2023

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

  • Set to false by default

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

  • linux-system-roles.firewall will attempt to install, unmask, and enable firewalld
  • linux-system-roles.firewall can attempt to disable directly conflicting services to firewalld
    • and that is enabled by setting the variable 'firewall_disable_conflicting_services' to true
    • list of conflicting services present in vars/main.yml

test cases for these changes in tests/tests_default.yml

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

  • set to false by default due to runtime overhead associated with disabling conflicting services. An example of where this overhead may be a problem is our integration tests that have no need to use the feature.
  • Reason for specific implementation - ansible.builtin.service module fails when run to manage services that are not installed on the system, causing errors. While ignoring errors is a potential solution, it seemed like an improper solution as it would not be able to differentiate between an installed service that failing to be stopped and disabled vs a disable that failed due to not being installed.

Result:

  • role no longer fails if firewalld is masked
  • users have the option to disable conflicting services (iptables.service, nftables.service, ufw.service respectively)

Issue Tracker Tickets (Jira or BZ if any):

@BrennanPaciorek BrennanPaciorek changed the title fix - unmask firewalld on run, disable conflicting services fix: unmask firewalld on run, disable conflicting services Jul 12, 2023
@codecov
Copy link

codecov bot commented Jul 12, 2023

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
@@           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 to find out more.

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

@BrennanPaciorek
Copy link
Collaborator Author

BrennanPaciorek commented Jul 12, 2023

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.

README.md Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
vars/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
@richm
Copy link
Contributor

richm commented Jul 12, 2023

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

Role will now always attempt to unmask on role run

add variable 'firewall_disable_conflicting_services' to enable the
disabling of conflicting services
- Set to false by default
  - Requires that services are enumerated on managed nodes, which can
    introduce potentially unnecessary runtime overhead

Update README to document the following behavior of the system role:
- linux-system-roles.firewall will attempt to install, unmask, and enable firewalld
- linux-system-roles.firewall can attempt to disable directly conflicting services to firewalld
  - and that is enabled by setting the variable 'firewall_disable_conflicting_services' to true
  - list of conflicting services present in vars/main.yml
test cases for these changes in tests/tests_default.yml

Addresses GitHub Issues: linux-system-roles#103, linux-system-roles#136
tasks/main.yml Outdated Show resolved Hide resolved
Brennan Paciorek added 2 commits July 12, 2023 16:23
List out conflicting services checked for in README, move some instruction in the introduction down to relevant variable.
@richm
Copy link
Contributor

richm commented Jul 12, 2023

[citest]

@richm
Copy link
Contributor

richm commented Jul 13, 2023

[citest]

@richm
Copy link
Contributor

richm commented Jul 13, 2023

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.

Return tests_default.yml to its original state

Move tests for unmasking firewalld and removing conflicting services to tests_startup_conflicts.yml
@BrennanPaciorek
Copy link
Collaborator Author

BrennanPaciorek commented Jul 13, 2023

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
Copy link
Contributor

richm commented Jul 13, 2023

[citest]

@richm richm merged commit e130634 into linux-system-roles:main Jul 13, 2023
32 checks passed
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.

None yet

2 participants