Fix ordering of iptables arguments

[nilssonk]

In many cases the ordering of iptables arguments is up to the user, however there are two things to consider, namely:

1. Some arguments have related sub-arguments which are always expected to follow directly. For example "-j DNAT --to-destination foo". If the order is not respected, the command will at best fail or at worst result in a segfault.
2. It is standard practice to list predicates before targets. Rarely if ever will anyone say that e.g. -j MASQUERADE should be the first argument to iptables - in that respect the current code slightly violates the principle of least astonishment.

This change reorders arguments to obey iptables rules and better align with standard practice.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nilssonk
Once this PR has been reviewed and has the lgtm label, please assign mheon for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[nilssonk]

As an aside, I chose to fix this because the current code simply refuses to work on my system.

Error response from daemon: netavark (exit code 1): unable to append rule '-j DNAT -p tcp --to-destination 10.89.0.5:8081 --destination-port 8080' to table 'nat': code: -1, msg:
Error: executing /usr/libexec/docker/cli-plugins/docker-compose up: exit status 1

iptables[175508]: segfault at 100000000 ip 00007f48e7b9b9c1 sp 00007fff84724618 error 4 in libc.so.6[19b9c1,7f48e7a24000+196000] likely on CPU 12 (core 4, socket 0)

After the change everything works fine.

I also realize that you are in the process of moving away from iptables, and that is fine, but perhaps the fix could be backported to the versions that still officially support iptables?

[Luap99]

I just made our "last" release before the iptables removal so I am reluctant to accept more iptables changes.

We have been using this code for years on many distributions with many different iptables-legacy and -nft versions without ever someone reporting an issue like this so I don't see why this would be a problem in netavark.

If the iptables command segfault's that sounds like iptables bug, regardless of arguments I doubt a segfault on their end is ever expected here. So what version of iptables is that and have you reported that to the iptables maintainers?

[nilssonk]

I just made our "last" release before the iptables removal so I am reluctant to accept more iptables changes.

We have been using this code for years on many distributions with many different iptables-legacy and -nft versions without ever someone reporting an issue like this so I don't see why this would be a problem in netavark.

If the iptables command segfault's that sounds like iptables bug, regardless of arguments I doubt a segfault on their end is ever expected here. So what version of iptables is that and have you reported that to the iptables maintainers?

It is very strange. All argument permutations seem to work in containers for ubuntu:jammy (glibc-2.35-0ubuntu3.11, iptables-1.8.7) and ubuntu:latest (glibc-2.39-0ubuntu8.6, iptables-1.8.7). My particular system has glibc-2.42 and iptables-1.8.11 and segfaults consistently for bad argument orders. I've gone through the git history of both iptables and glibc, but there haven't really been any obvious changes to the relevant code for quite some time.

Perhaps I have been lucky enough to discover a compiler bug.

Disregarding my local issue and instead viewing the PR as a grammar nit, the intentional usage has been codified in the official manpage for at least 17 years (paraphrased):

iptables [matches...] [target]
match := -m matchname [per-match-options]
target := -j targetname [per-target-options]

[Luap99]

Disregarding my local issue and instead viewing the PR as a grammar nit, the intentional usage has been codified in the official manpage for at least 17 years (paraphrased):

Sure not arguing against the logic behind that reason but I am not gonna merge this when we nuke the iptables code right away, in fact the PR is already ready and was just waiting for our release today. #1353
If that would not be the case I would not object this patch.

So merging this would do nothing other than causing more work to rebase again. And I also have no interest in backporting this to have more work with releases if the actual bug here seems to be iptables.

[nilssonk]

While searching for a related problem with iptables-1.8.11 I encountered this bug report, and sure enough I also had some stale library files messing things up, so sorry for that particular bit of noise.

Perhaps this is out of scope, but what is the motivation behind dropping support for iptables entirely? There are legacy systems out there which will likely never be able to migrate to nftables because of vendored kernels.

[Luap99]

Perhaps this is out of scope, but what is the motivation behind dropping support for iptables entirely? There are legacy systems out there which will likely never be able to migrate to nftables because of vendored kernels.

Yes, but if you think about it nftables is significantly older than podman or netavark FWIW.

Overall there is no strong technical reason for removing it atm, the main motivator is that we bump to 2.0/podman 6.0 so if we make breaking changes they can only happen now. It will be a long time until we make another major bump.
So the main benefit is just for us maintainers and contributors because if we add new firewall features (and we will) we no longer have to worry about mapping them to iptables commands. The less code I have to worry about the better.

Practically speaking nftables has a much better API to work with, we can properly batch insert things which makes things much faster (like 75% of totally netavark setup runtime is juts iptables) so really to me there is no reason not to use nftables overall.