contrib: add firewall reload services #20249
Conversation
Signed-off-by: danishprakash <danish.prakash@suse.com>
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: danishprakash The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
1 similar comment
|
Ephemeral COPR build failed. @containers/packit-build please check. |
Co-authored-by: Dan Čermák <45594031+dcermak@users.noreply.github.com> Signed-off-by: Danish Prakash <danish.prakash@suse.com>
| systemctl restart firewalld.service | ||
|
|
||
| # ensure the rules are still present | ||
| fout=$(run firewall-cmd --zone=trusted --list-all | grep "sources") |
There was a problem hiding this comment.
run is not intended to be used this way. First, it produces no output. Second, it hides exit status.
|
|
||
| # ensure the rules are still present | ||
| fout=$(run firewall-cmd --zone=trusted --list-all | grep "sources") | ||
| assert "$fout" != " sources: " # non-empty |
There was a problem hiding this comment.
...therefore this assertion is a NOP. $fout will always be an empty string.
| run_podman rm $cname | ||
|
|
||
| systemctl stop $reload_service | ||
| systemctl stop $restart_service |
There was a problem hiding this comment.
I'm not seeing a cleanup of the unit files...
| systemctl is-active $restart_service | ||
|
|
||
| cname="testctr" | ||
| run_podman run -d --rm --name $cname fedora:latest sleep 10d |
There was a problem hiding this comment.
Please don't use fedora:latest like that. Use $IMAGE if possible, or $SYSTEMD_IMAGE if for some reason you actually need fedora
| assert "$fout" != " sources: " # non-empty | ||
|
|
||
| # restart firewalld service | ||
| systemctl restart firewalld.service |
There was a problem hiding this comment.
This seems unlikely to work rootless...?
|
|
||
| [Service] | ||
| Type=simple | ||
| ExecStart=/usr/bin/bash -c '/usr/bin/busctl monitor --system --match "interface=org.fedoraproject.FirewallD1,member=Reloaded" --match "interface=org.fedoraproject.FirewallD1,member=PropertiesChanged" | while read -r line ; do @@PODMAN@@ network reload --all ; done' |
There was a problem hiding this comment.
What is the purpose of the read -r line, if $line is never used?
| @test "podman network reload on firewall-cmd --reload" { | ||
| setup_firewalld_services |
There was a problem hiding this comment.
This test must be skipped as rootless and when firewalld is not installed or not active.
But in general I am not sure we really want to test this in systems test, reloading the firewalld could have negative consequences for other applications as well outside of the tests. Given the high amount of different environments these tests are run I don't think it is a good idea to do it.
| firewall-cmd --reload | ||
|
|
||
| # ensure the rules are present | ||
| fout=$(run firewall-cmd --zone=trusted --list-all | grep "sources") | ||
| assert "$fout" != " sources: " # non-empty | ||
|
|
||
| # restart firewalld service | ||
| systemctl restart firewalld.service | ||
|
|
||
| # ensure the rules are still present | ||
| fout=$(run firewall-cmd --zone=trusted --list-all | grep "sources") | ||
| assert "$fout" != " sources: " # non-empty |
There was a problem hiding this comment.
This is just asking for flakes, there is no guarantee that by the time you check the network reload command was already run.
Also it would make much more sense to actually test the rules are restored by ding connectivity tests like done in the networking tests.
|
Reading the comments/reactions, I don't think we should ship this and wait for the fixes. |
From #5431 (comment) I don't anticipate these fixes to arrive anytime soon. It would be nice to have at least some intermediate solution. |
|
We added the proper fix to our release planning for 4.8 so it makes no sense to try to work around this now as this PR would also only land in 4.8. |
github-actions
bot
added
the
locked - please file new issue/PR
Should we package this until this is fixed in netavark/aardvark-dns as has been mentioned over on #5431? The unit files have been taken from @githubcek's comment here.
Refers #5431
cc/ @dcermak