test netavark nftables driver #23234
Conversation
openshift-ci
bot
added
do-not-merge/work-in-progress
|
We were not able to find or create Copr project Please check your configuration for:
|
|
Looks like we have a few tests that make assumptions on iptables that have to fixed. And upgrade tests will not work like this as you cannot change the firewall driver with running containers and then expect things to keep working so I guess I have to force iptables there. But overall this looks good. cc @mheon |
|
Sure, LGTM. I don't think we need to worry about conditionally testing both, once the upstream default is swapped I don't really care about upstream iptables testing. |
Luap99
force-pushed
the
test-nftables
branch
2 times, most recently
from
3edb6fb to
fba33af
Compare
Make sure this passes podman CI before we push out a default change. ref: https://fedoraproject.org/wiki/Changes/NetavarkNftablesDefault Signed-off-by: Paul Holzinger <[email protected]>
This test checks a simple publish which is already covered in many other places, it also used iptables wich is a invalid assumption going forward as we start to enable nftables as firewall driver. The only thing these tests added where checking that we cannot resuse the same port. Given there was more than one kernel regression[1,2] about correctly failing with EADDRINUSE I also added the distro-integration tag to make sure we catch this early in fedora testing. [1] https://lore.kernel.org/regressions/[email protected]/ [2] https://lore.kernel.org/regressions/CAFsF8vL4CGFzWMb38_XviiEgxoKX0GYup=JiUFXUOmagdk9CRg@mail.gmail.com/ Signed-off-by: Paul Holzinger <[email protected]>
Stop using iptables to check anything, it does not work rootless and will no longer work with nftables which will be used in the future. Also fix up the test that say podman run to actually use podman run and then just check via inspect that the ports are set correctly. Signed-off-by: Paul Holzinger <[email protected]>
netavark can use iptables or nftables as firewall driver, thus if we try to flush rules make sure we try both to keep the test working when we switch the default to nftables. Signed-off-by: Paul Holzinger <[email protected]>
Old netavark version only supported iptables, however a new version on th ehost might use nftables. This breaks the networking tests here as they are not compatible and you would need to reboot to fix that. Because this is not possible for our tests make sure we force the iptables driver always to keep the test working. Signed-off-by: Paul Holzinger <[email protected]>
Luap99
changed the title
openshift-ci
bot
removed
do-not-merge/release-note-label-needed
|
@edsantiago PTAL I expect this only to be temporary until we changed the default in fedora. |
| iptables -t nat -nvL || true | ||
| nft list ruleset || true |
There was a problem hiding this comment.
Will a future maintainer, looking at failure logs, know what these are? I know it's duplication, but I like to include the commands in the output:
echo "$_LOG_PROMPT iptables -t nat -nvL"
iptables -t nat -nvL || true
echo "$_LOG_PROMPT nft list ruleset"
nft list rulesetThere was a problem hiding this comment.
yeah I can add this although format is very unique for both so it is easy to know what they are, but I guess I can say this because I look at this stuff often. I will include if I have to repush again.
| iptables -t nat -F "NETAVARK-HOSTPORT-DNAT" || true | ||
| nft delete table inet netavark || true |
There was a problem hiding this comment.
If nft is the desired default, shouldn't that be the first one run?
And, what happens on systems where both are installed? Would it be safer to do this instead?
nft delete table inet netavark \
|| iptables -t nat -F "..." \
|| trueThere was a problem hiding this comment.
That is what I tried before as well, there is big bug in system there that use CONTAINERS_CONF which will mean they ignore the firewall driver opt.
I had the order iptables || nft which does not work, using nft || iptables would work but not if somebody inverts the order one day where the containers.conf forces iptables and the normal the default is nft. Thus I decied to just always flush both.
In general using CONTAINERS_CONF in system tests seems wrong but well there is a test that excpliclty checks how CONTAINERS_CONF works so it is not like I can just remove/fix it either...
| # Test nftables driver, https://fedoraproject.org/wiki/Changes/NetavarkNftablesDefault | ||
| # We can drop this once this implemented and pushed into fedora stable. We cannot test it on | ||
| # debian because the netavark version there is way to old for nftables support. | ||
| printf "[network]\nfirewall_driver=\"nftables\"\n" > /etc/containers/containers.conf.d/90-nftables.conf |
There was a problem hiding this comment.
This will work for system tests, but can you confirm that it works in e2e? Those have a nasty habit of ignoring all conf files, and requiring command-line overrides for everything.
There was a problem hiding this comment.
This works for e2e they do not ignore global configs they just overwrite most options via cli but there is no option for the firewall driver so it works (well until they use CONTAINERS_CONF env).
I know it does before the first version failed with the iptables checks in e2e which is why I removed it.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
stale-locking-app
bot
added
the
locked - please file new issue/PR
Make sure this passes podman CI before push out a default change.
ref: https://fedoraproject.org/wiki/Changes/NetavarkNftablesDefault
Does this PR introduce a user-facing change?