[RHELC-1180] OL 8: Prevent conversion failure with firewalld running #953

r0x0d · 2023-10-11T13:04:53Z

Firewalld is having problems with the python-nftables package after we replace the packages in the system to their rhel counterparts. This commit introduces a way to check if firewalld is running and the CleanupModulesOnExit firewalld option is enabled, and if so, we ask the user to disable the option.

If firewalld is left running with the option on during the conversion, the user will see a set of errors happening in the systemctl logs for firewalld and they will be unable to ssh into the machine after the conversion is done.

Jira Issues: RHELC-1180

Checklist

PR has been tested manually in a VM (either author or reviewer)
Jira issue has been made public if possible
[RHELC-] is part of the PR title
GitHub label has been added to help with Release notes
PR title explains the change from the user's point of view
Code and tests are documented properly
The commits are squashed to as few commits as possible (without losing data)
When merged: Jira issue has been updated to Release Pending if relevant

codecov · 2023-10-11T13:07:06Z

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (44483f6) 93.87% compared to head (28d8493) 93.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #953      +/-   ##
==========================================
- Coverage   93.87%   93.77%   -0.10%     
==========================================
  Files          46       47       +1     
  Lines        4296     4340      +44     
  Branches      755      766      +11     
==========================================
+ Hits         4033     4070      +37     
- Misses        188      192       +4     
- Partials       75       78       +3

Flag	Coverage Δ
centos-linux-7	`88.70% <59.57%> (-0.33%)`	⬇️
centos-linux-8	`89.76% <85.10%> (-0.06%)`	⬇️
centos-linux-9	`89.83% <85.10%> (-0.06%)`	⬇️

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

Files	Coverage Δ
...nvert2rhel/actions/pre_ponr_changes/transaction.py	`100.00% <ø> (ø)`
convert2rhel/systeminfo.py	`97.08% <100.00%> (ø)`
...ions/system_checks/check_firewalld_availability.py	`84.09% <84.09%> (ø)`

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

has-bot · 2023-10-11T13:17:01Z

/packit test --labels tier0

@oamg/conversions-qe please review results and provide ack.

Log | Bot Usage

convert2rhel/actions/system_checks/check_firewalld_availability.py

r0x0d · 2023-10-18T10:59:33Z

/packit test

has-bot · 2023-10-20T11:54:22Z

/packit test

@oamg/conversions-qe please review results and provide ack.

Log | Bot Usage

has-bot · 2023-10-20T12:40:17Z

/packit test --labels sanity

@oamg/conversions-qe please review results and provide ack.

Log | Bot Usage

kokesak · 2023-10-20T12:47:41Z

/packit test

mportman12

Left suggested edits.

convert2rhel/actions/system_checks/check_firewalld_availability.py

danmyway

A couple of nitpicks, otherwise looks, good, thank you!

tests/ansible_collections/roles/oracle-linux-specific/tasks/stop_firewalld_workaround.yml

tests/integration/tier0/non-destructive/firewalld-inhibitor/test_firewalld_inhibitor.py

tests/integration/tier0/non-destructive/firewalld-inhibitor/main.fmf

kokesak · 2023-10-23T14:53:17Z

/packit test

danmyway · 2023-10-24T15:28:04Z

/packit test

danmyway

LGTM, thanks!

danmyway · 2023-10-25T14:05:49Z

/packit retest-failed

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Co-authored-by: Michal Bocek <mbocek@redhat.com>

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Fixed the detection of the CleanupModulesOnExit inside the firewalld.conf and updated the tests cases Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Co-authored-by: Michal Bocek <mbocek@redhat.com>

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

kokesak · 2023-10-30T11:17:24Z

/packit test

convert2rhel/actions/system_checks/check_firewalld_availability.py

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

r0x0d · 2023-10-30T13:44:26Z

/packit test

kokesak · 2023-10-30T14:10:39Z

/packit test

kokesak · 2023-10-30T15:11:48Z

convert2rhel/actions/system_checks/check_firewalld_availability.py

+                )
+                return
+
+        logger.info("Skipping the check as it is relevant only for Oracle Linux 8.8 and above.")


Not a PR blocker, but: If none of the above condition is true, then this message is printed.

Scenario:

we are on oracle8.8

the firewalld is running

the config file contains the CleanupModulesOnExit=no
then all the ifs are skipped and no default return is present, so the logger.info("Skipping the check as it is relevant only for Oracle Linux 8.8 and above.") is printed even though we are on 8.8 :D

14:28:50 out: [2023-10-30T14:28:50+0000] TASK - [Prepare: Gather system information] ******************************* 14:28:50 out: [2023-10-30T14:28:50+0000] DEBUG - Calling command 'uname -i' 14:28:50 out: Skipping the execution of 'rpm -Va'. 14:28:50 out: [2023-10-30T14:28:50+0000] DEBUG - Calling command 'uname -r' 14:28:50 out: [2023-10-30T14:28:50+0000] DEBUG - Booted kernel VRA (version, release, architecture): 4.18.0-477.27.1.el8_8.x86_64 14:28:50 out: Checking internet connectivity using address 'https://static.redhat.com/test/rhel-networkmanager.txt'. 14:28:50 out: Successfully connected to address 'https://static.redhat.com/test/rhel-networkmanager.txt', internet connection seems to be available. 14:28:50 out: [2023-10-30T14:28:50+0000] DEBUG - Calling command '/usr/bin/systemctl show -p ActiveState dbus' 14:28:50 out: Name: Oracle Linux Server 14:28:50 out: OS version: 8.8 14:28:50 out: Architecture: x86_64 14:28:50 out: Config filename: oracle-8-x86_64.cfg 14:28:53 out: 14:28:53 out: [2023-10-30T14:28:53+0000] TASK - [Prepare: Clear YUM/DNF version locks] ***************************** 14:28:53 out: Usage of YUM/DNF versionlock plugin not detected. 14:28:53 out: 14:28:53 out: [2023-10-30T14:28:53+0000] TASK - [Prepare: Clean yum cache metadata] ******************************** 14:28:53 out: [2023-10-30T14:28:53+0000] DEBUG - Calling command 'yum clean metadata --enablerepo=* --quiet' 14:28:53 out: [2023-10-30T14:28:53+0000] DEBUG - Output of yum clean metadata: 14:28:53 out: 14:28:53 out: Cached repositories metadata cleaned successfully. 14:28:53 out: 14:28:53 out: [2023-10-30T14:28:53+0000] TASK - [Prepare: Check whether system is ready for conversion] ************ 14:28:53 out: 14:28:53 out: [2023-10-30T14:28:53+0000] TASK - [Prepare: Check that firewalld is running] ************************* 14:28:53 out: [2023-10-30T14:28:53+0000] DEBUG - Calling command '/usr/bin/systemctl show -p ActiveState firewalld' 14:28:53 out: [2023-10-30T14:28:53+0000] DEBUG - CleanupModulesOnExit option is disabled in /etc/firewalld/firewalld.conf 14:28:53 out: Skipping the check as it is relevant only for Oracle Linux 8.8 and above. 14:28:53 out: CHECK_FIREWALLD_AVAILABILITY has succeeded

http://artifacts.osci.redhat.com/testing-farm/cfa41dc4-a364-4e38-9237-b41e280da9dc/work-rhsmf6bxgaaj/log.txt

danmyway

Just couple of nitpicks, we can discuss and revisit later @kokesak.
I wouldn't block the merge on this.

danmyway · 2023-10-30T15:57:57Z

tests/integration/tier0/non-destructive/firewalld-inhibitor/test_firewalld_inhibitor.py

+
+from envparse import env
+
+from convert2rhel.actions.system_checks.check_firewalld_availability import FIREWALLD_CONFIG_FILE


I would rather see this hardcoded than imported TBH.

danmyway · 2023-10-30T15:59:58Z

tests/integration/tier0/non-destructive/firewalld-inhibitor/test_firewalld_inhibitor.py

+    Verify that on the OL8.8 the conversion is inhibited due to
+    running firewalld on the system and the `CleanupModulesOnExit` configuration
+    option is set to `yes` in firewalld configuration file.
+    The reference ticket: https://issues.redhat.com/browse/RHELC-1180


Suggested change

Verify that on the OL8.8 the conversion is inhibited due to

running firewalld on the system and the `CleanupModulesOnExit` configuration

option is set to `yes` in firewalld configuration file.

The reference ticket: https://issues.redhat.com/browse/RHELC-1180

Verify that on the OL8.8 the conversion is inhibited with the firewalld system unit

running on the system with the `CleanupModulesOnExit` configuration

option is set to `yes` in the firewalld configuration file.

The reference ticket: https://issues.redhat.com/browse/RHELC-1180

danmyway · 2023-10-30T16:02:22Z

tests/integration/tier0/non-destructive/firewalld-inhibitor/test_firewalld_inhibitor.py

+    assert shell(f"grep 'CleanupModulesOnExit=yes' {FIREWALLD_CONFIG_FILE}").returncode == 0
+


This might be tricky, I mean we have more or less the system under our control, but if for whatever reason the Oracle changes this, we won't match and the test will fail too soon.
I would either backup the original and provide our own config with the option disabled, or try regex matching more variants.

danmyway · 2023-10-30T16:03:54Z

tests/integration/tier0/non-destructive/firewalld-inhibitor/test_firewalld_inhibitor.py

+    assert shell(
+        "grep 'Firewalld running on Oracle Linux 8 can lead to a conversion failure' /var/log/convert2rhel/convert2rhel.log"
+    )


This might be unnecessary since we check for the WARNING in the c2r runtime already.

danmyway · 2023-10-30T16:04:58Z

tests/integration/tier1/destructive/firewalld-config-ol8/main.fmf

@@ -0,0 +1,16 @@
+summary: |
+    Running conversion when firewalld is running and the proper configuration option is used.


Suggested change

Running conversion when firewalld is running and the proper configuration option is used.

Running firewalld remediation

danmyway · 2023-10-30T16:09:14Z

tests/integration/tier1/destructive/firewalld-config-ol8/test_firewalld_config_ol8.py

+
+from envparse import env
+
+from convert2rhel.actions.system_checks.check_firewalld_availability import FIREWALLD_CONFIG_FILE


Same as in the other case, I would rather see this hardcoded than imported.

danmyway · 2023-10-30T16:09:53Z

tests/integration/tier1/destructive/firewalld-config-ol8/test_firewalld_config_ol8.py

+@pytest.mark.test_firewalld_config
+def test_firewalld_config_ol8(shell, convert2rhel):


Suggested change

@pytest.mark.test_firewalld_config

def test_firewalld_config_ol8(shell, convert2rhel):

@pytest.mark.test_running_firewalld_remediation

def test_running_firewalld_remediation_ol8(shell, convert2rhel):

danmyway · 2023-10-30T16:12:38Z

tests/integration/tier1/destructive/firewalld-config-ol8/test_firewalld_config_ol8.py

+    if not os.path.exists(FIREWALLD_CONFIG_FILE):
+        pytest.fail("Firewalld configuration file does not exist")


Suggested change

if not os.path.exists(FIREWALLD_CONFIG_FILE):

pytest.fail("Firewalld configuration file does not exist")

if not os.path.exists(FIREWALLD_CONFIG_FILE):

pytest.fail("Firewalld configuration file does not exist")

This might come and bite us in the future.
Consider this being not so probable edge case, but if for whatever reason a new image will be provided without the config, the test will fail at this point providing no valid results.
Firewalld will fall back to a default option in case no config file is provided, that being CleanupModulesOnExit=yes.

danmyway · 2023-10-30T16:14:34Z

tests/integration/tier1/destructive/firewalld-config-ol8/test_firewalld_config_ol8.py

+    assert (
+        shell(f"sed -i 's/CleanupModulesOnExit=yes/CleanupModulesOnExit=no/g' {FIREWALLD_CONFIG_FILE}").returncode == 0
+    )


This will fail if we fail to match CleanupModulesOnExit=yes.
We might consider the suggestion from the other (non-destructive) test case. To provide and copy over our own config file and back up the existing one.

danmyway · 2023-10-30T16:17:41Z

tests/integration/tier1/destructive/firewalld-config-ol8/test_firewalld_config_ol8.py

+    assert c2r.exitstatus == 0
+
+    # There should not be any problems
+    assert shell("grep -i 'traceback' /var/log/convert2rhel/convert2rhel.log").returncode == 1


Not sure why this is here.
If there would be a traceback, the c2r.exitstatus won't return 0.
On the other hand we might evaluate occurences of traceback in the logfile in one of the checks after conversion, which scans through the log file.

bocekm · 2023-10-30T16:34:01Z

convert2rhel/actions/system_checks/check_firewalld_availability.py

+    # config parser for firewalld as well:
+    # https://github.com/firewalld/firewalld/blob/46d54dcbdff94423686d67befc78ca8d601fce60/src/firewall/core/io/firewalld_conf.py#L85
+    option_present = any(
+        item.strip().startswith("CleanupModulesOnExit") for item in contents if not item.startswith(("#", ";"))


$ ipython In [1]: lines = ["option", "#option"] In [2]: for line in lines: ...: print(line.strip().startswith("option")) True False

This is what I meant. When the line starts with # or ;, the startswith("CleanupModulesOnExit") returns False.

bocekm · 2023-10-30T17:10:57Z

convert2rhel/actions/system_checks/check_firewalld_availability.py

+                        " sed -i -- 's/CleanupModulesOnExit=yes/CleanupModulesOnExit=no/g' /etc/firewalld/firewalld.conf\n && firewall-cmd --reload."
+                        " You can change the option back to yes after the system reboot "


Suggested change

" sed -i -- 's/CleanupModulesOnExit=yes/CleanupModulesOnExit=no/g' /etc/firewalld/firewalld.conf\n && firewall-cmd --reload."

" You can change the option back to yes after the system reboot "

" sed -i -- 's/^CleanupModulesOnExit.*/CleanupModulesOnExit=no/g' /etc/firewalld/firewalld.conf\n && firewall-cmd --reload\n"

"You can change the option back to yes after the system reboot "

This is safer as it ignores the line if commented out (^) and it replaces the line even when it's not exactly CleanupModulesOnExit=yes because CleanupModulesOnExit = True is legit as well.

This PR introduces a few cleanups that were made as comments in oamg#953. Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

This PR introduces a few cleanups that were made as comments in #953. Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

r0x0d added kind/feature New feature or request tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. labels Oct 11, 2023

Venefilyn reviewed Oct 11, 2023

View reviewed changes

convert2rhel/actions/system_checks/check_firewalld_availability.py Outdated Show resolved Hide resolved

Venefilyn reviewed Oct 11, 2023

View reviewed changes

convert2rhel/actions/system_checks/check_firewalld_availability.py Outdated Show resolved Hide resolved

Venefilyn approved these changes Oct 19, 2023

View reviewed changes

kokesak force-pushed the firewalld-workaround branch from db4b15d to a3367f9 Compare October 20, 2023 11:25

kokesak added the tests/all Run the full test suite. Equivalent to `/packit test`. label Oct 20, 2023

r0x0d force-pushed the firewalld-workaround branch from 93ad69f to 7d8f4a9 Compare October 20, 2023 12:21

kokesak added the tests/sanity PR ready to run the sanity test suit. Equivalent to `/packit test --labels sanity`. label Oct 20, 2023

mportman12 reviewed Oct 20, 2023

View reviewed changes

kokesak requested a review from danmyway October 23, 2023 12:17

danmyway requested changes Oct 23, 2023

View reviewed changes

danmyway reviewed Oct 23, 2023

View reviewed changes

tests/integration/tier0/non-destructive/firewalld-inhibitor/main.fmf Outdated Show resolved Hide resolved

danmyway reviewed Oct 23, 2023

View reviewed changes

tests/integration/tier0/non-destructive/firewalld-inhibitor/main.fmf Outdated Show resolved Hide resolved

danmyway reviewed Oct 23, 2023

View reviewed changes

tests/integration/tier0/non-destructive/firewalld-inhibitor/main.fmf Outdated Show resolved Hide resolved

kokesak force-pushed the firewalld-workaround branch from d7efd86 to 52636df Compare October 23, 2023 14:35

kokesak removed tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. tests/sanity PR ready to run the sanity test suit. Equivalent to `/packit test --labels sanity`. labels Oct 23, 2023

kokesak requested a review from danmyway October 23, 2023 15:00

danmyway approved these changes Oct 25, 2023

View reviewed changes

kokesak force-pushed the firewalld-workaround branch from 324565d to f9f456a Compare October 25, 2023 16:27

kokesak and others added 9 commits October 30, 2023 12:01

Address int test expected string

2d40c2c

Update firewalld action to check for config file

1593038

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Apply suggestions from code review

2c75961

Co-authored-by: Michal Bocek <mbocek@redhat.com>

Apply comments suggestion and update tests

50bf0ee

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Fix the detection of CleanupModulesOnExit option

731f5b2

Fixed the detection of the CleanupModulesOnExit inside the firewalld.conf and updated the tests cases Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Apply suggestions from code review

92411dd

Co-authored-by: Michal Bocek <mbocek@redhat.com>

Update to the config parsing for firewalld

52af885

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Skip comments from parsing

c71c6dc

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Add firewalld config integration test

5709cb9

kokesak force-pushed the firewalld-workaround branch from 0db1f6d to 5709cb9 Compare October 30, 2023 11:05

kokesak reviewed Oct 30, 2023

View reviewed changes

convert2rhel/actions/system_checks/check_firewalld_availability.py Outdated Show resolved Hide resolved

kokesak reviewed Oct 30, 2023

View reviewed changes

convert2rhel/actions/system_checks/check_firewalld_availability.py Outdated Show resolved Hide resolved

r0x0d added 3 commits October 30, 2023 10:16

Code cleanup and some unit tests fixes

7dcd8b7

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Update result message

5730471

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Add debug log for fallback

5f73855

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Integration tests workaround adjustment

28d8493

kokesak reviewed Oct 30, 2023

View reviewed changes

danmyway approved these changes Oct 30, 2023

View reviewed changes

bocekm approved these changes Oct 30, 2023

View reviewed changes

bocekm merged commit bc07912 into oamg:main Oct 30, 2023
44 of 49 checks passed

r0x0d deleted the firewalld-workaround branch October 30, 2023 17:20

r0x0d added a commit to r0x0d/convert2rhel that referenced this pull request Oct 30, 2023

Firewalld action cleanup

50b3782

This PR introduces a few cleanups that were made as comments in oamg#953. Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

r0x0d mentioned this pull request Oct 30, 2023

[RHELC-1300] Cleanup firewalld action #975

Merged

8 tasks

bocekm changed the title ~~[RHELC-1180] Check for firewalld system unit~~ [RHELC-1180] OL 8: Prevent conversion failure with firewalld running Oct 30, 2023

bocekm mentioned this pull request Oct 30, 2023

Bump to version 1.5.0 #976

Merged

r0x0d added a commit to r0x0d/convert2rhel that referenced this pull request Jan 3, 2024

Firewalld action cleanup

13bb2e4

This PR introduces a few cleanups that were made as comments in oamg#953. Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Venefilyn pushed a commit that referenced this pull request Jan 4, 2024

[RHELC-1300] Cleanup firewalld action (#975)

18add35

This PR introduces a few cleanups that were made as comments in #953. Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHELC-1180] OL 8: Prevent conversion failure with firewalld running #953

[RHELC-1180] OL 8: Prevent conversion failure with firewalld running #953

r0x0d commented Oct 11, 2023 •

edited by bocekm

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading

has-bot commented Oct 11, 2023

r0x0d commented Oct 18, 2023

has-bot commented Oct 20, 2023

has-bot commented Oct 20, 2023

kokesak commented Oct 20, 2023

mportman12 left a comment

danmyway left a comment

kokesak commented Oct 23, 2023

danmyway commented Oct 24, 2023

danmyway left a comment

danmyway commented Oct 25, 2023

kokesak commented Oct 30, 2023

r0x0d commented Oct 30, 2023

kokesak commented Oct 30, 2023

kokesak Oct 30, 2023

danmyway left a comment

danmyway Oct 30, 2023

danmyway Oct 30, 2023

danmyway Oct 30, 2023

danmyway Oct 30, 2023

danmyway Oct 30, 2023

danmyway Oct 30, 2023

danmyway Oct 30, 2023

danmyway Oct 30, 2023

danmyway Oct 30, 2023

danmyway Oct 30, 2023

bocekm Oct 30, 2023

bocekm Oct 30, 2023 •

edited

Loading


		from envparse import env

		from convert2rhel.actions.system_checks.check_firewalld_availability import FIREWALLD_CONFIG_FILE

		assert shell(f"grep 'CleanupModulesOnExit=yes' {FIREWALLD_CONFIG_FILE}").returncode == 0

		@@ -0,0 +1,16 @@
		summary: \|
		Running conversion when firewalld is running and the proper configuration option is used.

	Running conversion when firewalld is running and the proper configuration option is used.
	Running firewalld remediation

		@pytest.mark.test_firewalld_config
		def test_firewalld_config_ol8(shell, convert2rhel):

		if not os.path.exists(FIREWALLD_CONFIG_FILE):
		pytest.fail("Firewalld configuration file does not exist")

		" sed -i -- 's/CleanupModulesOnExit=yes/CleanupModulesOnExit=no/g' /etc/firewalld/firewalld.conf\n && firewall-cmd --reload."
		" You can change the option back to yes after the system reboot "

[RHELC-1180] OL 8: Prevent conversion failure with firewalld running #953

[RHELC-1180] OL 8: Prevent conversion failure with firewalld running #953

Conversation

r0x0d commented Oct 11, 2023 • edited by bocekm Loading

codecov bot commented Oct 11, 2023 • edited Loading

Codecov Report

has-bot commented Oct 11, 2023

r0x0d commented Oct 18, 2023

has-bot commented Oct 20, 2023

has-bot commented Oct 20, 2023

kokesak commented Oct 20, 2023

mportman12 left a comment

Choose a reason for hiding this comment

danmyway left a comment

Choose a reason for hiding this comment

kokesak commented Oct 23, 2023

danmyway commented Oct 24, 2023

danmyway left a comment

Choose a reason for hiding this comment

danmyway commented Oct 25, 2023

kokesak commented Oct 30, 2023

r0x0d commented Oct 30, 2023

kokesak commented Oct 30, 2023

Choose a reason for hiding this comment

danmyway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bocekm Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

r0x0d commented Oct 11, 2023 •

edited by bocekm

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading

bocekm Oct 30, 2023 •

edited

Loading