| Message ID | 9742365b595a791d4bd47abee6ad6271abe0950b.1590323912.git.sbrivio@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft] tests: shell: Avoid breaking basic connectivity when run | expand |
Hi, On Sun, May 24, 2020 at 02:59:57PM +0200, Stefano Brivio wrote: > It might be convenient to run tests from a development branch that > resides on another host, and if we break connectivity on the test > host as tests are executed, we con't run them this way. > > To preserve connectivity, for shell tests, we can simply use the > 'forward' hook instead of 'input' in chains/0036_policy_variable_0 > and transactions/0011_chain_0, without affecting test coverage. > > For py tests, this is more complicated as some test cases install > chains for all the available hooks, and we would probably need a > more refined approach to avoid dropping relevant traffic, so I'm > not covering that right now. This is a recurring issue, iptables testsuites suffer from this problem as well. There it was solved by running everything in a dedicated netns: iptables/tests/shell: Call testscripts via 'unshare -n <file>'. iptables-test.py: If called with --netns, 'ip netns exec <foo>' is added as prefix to any of the iptables commands. I considered doing the same in nftables testsuites several times but never managed to keep me motivated enough. Maybe you want to give it a try? Cheers, Phil
On Mon, 25 May 2020 17:59:52 +0200 Phil Sutter <phil@nwl.cc> wrote: > Hi, > > On Sun, May 24, 2020 at 02:59:57PM +0200, Stefano Brivio wrote: > > It might be convenient to run tests from a development branch that > > resides on another host, and if we break connectivity on the test > > host as tests are executed, we con't run them this way. > > > > To preserve connectivity, for shell tests, we can simply use the > > 'forward' hook instead of 'input' in chains/0036_policy_variable_0 > > and transactions/0011_chain_0, without affecting test coverage. > > > > For py tests, this is more complicated as some test cases install > > chains for all the available hooks, and we would probably need a > > more refined approach to avoid dropping relevant traffic, so I'm > > not covering that right now. > > This is a recurring issue, iptables testsuites suffer from this problem > as well. There it was solved by running everything in a dedicated netns: > > iptables/tests/shell: Call testscripts via 'unshare -n <file>'. > iptables-test.py: If called with --netns, 'ip netns exec <foo>' is > added as prefix to any of the iptables commands. Funny, I thought about doing that in the past and stopped before I could even type 'unshare'. Silly, everything will break, I thought. Nope, not at all, now that you say that... both 'unshare -n ./nft-test.py' and 'unshare -n ./run-tests.sh' worked flawlessly. A minor concern I have is that if CONFIG_NETNS is not set we can't do that. But we could add a check and run them in the init namespace if namespaces are not available, that looks reasonable enough. > I considered doing the same in nftables testsuites several times but > never managed to keep me motivated enough. Maybe you want to give it a > try? I would do that from the main script itself (and figure out how it should be done in Python, too), just once, it looks cleaner and we don't change how test scripts are invoked. Something like this: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/testing/selftests/netfilter/nft_concat_range.sh#n1463
Hi, On Tue, May 26, 2020 at 01:12:36AM +0200, Stefano Brivio wrote: > On Mon, 25 May 2020 17:59:52 +0200 > Phil Sutter <phil@nwl.cc> wrote: > > On Sun, May 24, 2020 at 02:59:57PM +0200, Stefano Brivio wrote: > > > It might be convenient to run tests from a development branch that > > > resides on another host, and if we break connectivity on the test > > > host as tests are executed, we con't run them this way. > > > > > > To preserve connectivity, for shell tests, we can simply use the > > > 'forward' hook instead of 'input' in chains/0036_policy_variable_0 > > > and transactions/0011_chain_0, without affecting test coverage. > > > > > > For py tests, this is more complicated as some test cases install > > > chains for all the available hooks, and we would probably need a > > > more refined approach to avoid dropping relevant traffic, so I'm > > > not covering that right now. > > > > This is a recurring issue, iptables testsuites suffer from this problem > > as well. There it was solved by running everything in a dedicated netns: > > > > iptables/tests/shell: Call testscripts via 'unshare -n <file>'. > > iptables-test.py: If called with --netns, 'ip netns exec <foo>' is > > added as prefix to any of the iptables commands. > > Funny, I thought about doing that in the past and stopped before I could > even type 'unshare'. Silly, everything will break, I thought. > > Nope, not at all, now that you say that... both 'unshare -n > ./nft-test.py' and 'unshare -n ./run-tests.sh' worked flawlessly. > > A minor concern I have is that if CONFIG_NETNS is not set we can't do > that. But we could add a check and run them in the init namespace if > namespaces are not available, that looks reasonable enough. Sounds like over-engineering, although your point is valid, of course. In iptables shell testsuite was changed to always call unshare back in 2018, I don't think anyone has complained yet. So either everyone has netns support already (likely, given the container inflation) or nobody apart from hardliners actually run those tests (even more likely). :) > > I considered doing the same in nftables testsuites several times but > > never managed to keep me motivated enough. Maybe you want to give it a > > try? > > I would do that from the main script itself (and figure out how it > should be done in Python, too), just once, it looks cleaner and we > don't change how test scripts are invoked. Something like this: > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/testing/selftests/netfilter/nft_concat_range.sh#n1463 Maybe support a hidden parameter and do a self-call wrapped by 'unshare' unless that parameter was passed? Cheers, Phil
[I was going once more over your comments before sending the new patch, realised I didn't answer...] On Tue, 26 May 2020 15:52:49 +0200 Phil Sutter <phil@nwl.cc> wrote: > Hi, > > On Tue, May 26, 2020 at 01:12:36AM +0200, Stefano Brivio wrote: > > On Mon, 25 May 2020 17:59:52 +0200 > > Phil Sutter <phil@nwl.cc> wrote: > > > On Sun, May 24, 2020 at 02:59:57PM +0200, Stefano Brivio wrote: > > > > It might be convenient to run tests from a development branch that > > > > resides on another host, and if we break connectivity on the test > > > > host as tests are executed, we con't run them this way. > > > > > > > > To preserve connectivity, for shell tests, we can simply use the > > > > 'forward' hook instead of 'input' in chains/0036_policy_variable_0 > > > > and transactions/0011_chain_0, without affecting test coverage. > > > > > > > > For py tests, this is more complicated as some test cases install > > > > chains for all the available hooks, and we would probably need a > > > > more refined approach to avoid dropping relevant traffic, so I'm > > > > not covering that right now. > > > > > > This is a recurring issue, iptables testsuites suffer from this problem > > > as well. There it was solved by running everything in a dedicated netns: > > > > > > iptables/tests/shell: Call testscripts via 'unshare -n <file>'. > > > iptables-test.py: If called with --netns, 'ip netns exec <foo>' is > > > added as prefix to any of the iptables commands. > > > > Funny, I thought about doing that in the past and stopped before I could > > even type 'unshare'. Silly, everything will break, I thought. > > > > Nope, not at all, now that you say that... both 'unshare -n > > ./nft-test.py' and 'unshare -n ./run-tests.sh' worked flawlessly. > > > > A minor concern I have is that if CONFIG_NETNS is not set we can't do > > that. But we could add a check and run them in the init namespace if > > namespaces are not available, that looks reasonable enough. > > Sounds like over-engineering, although your point is valid, of course. > In iptables shell testsuite was changed to always call unshare back in > 2018, I don't think anyone has complained yet. So either everyone has > netns support already (likely, given the container inflation) or nobody > apart from hardliners actually run those tests (even more likely). :) On embedded systems, in my experience, even in the post-modern era, it's quite common to run kernels without support for networking namespaces, or a busybox build without the unshare(1) applet (which was implemented not so long ago, in 2016). And one might want to run tests there because of some specific peculiarities (say, endianness, word size, or just debugging). Perhaps more commonly, Python bindings for unshare() might not be installed. The check is quite straigthforward, so I would rather add it. > > > I considered doing the same in nftables testsuites several times but > > > never managed to keep me motivated enough. Maybe you want to give it a > > > try? > > > > I would do that from the main script itself (and figure out how it > > should be done in Python, too), just once, it looks cleaner and we > > don't change how test scripts are invoked. Something like this: > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/testing/selftests/netfilter/nft_concat_range.sh#n1463 > > Maybe support a hidden parameter and do a self-call wrapped by 'unshare' > unless that parameter was passed? Yes, it's what the example I quoted does. Well, I hope you meant the same thing. :)
diff --git a/tests/shell/testcases/chains/0036policy_variable_0 b/tests/shell/testcases/chains/0036policy_variable_0 index d4d98ede0d8d..e9246dd9e974 100755 --- a/tests/shell/testcases/chains/0036policy_variable_0 +++ b/tests/shell/testcases/chains/0036policy_variable_0 @@ -9,7 +9,7 @@ define default_policy = \"drop\" table inet global { chain prerouting { - type filter hook prerouting priority filter + type filter hook forward priority filter policy \$default_policy } }" diff --git a/tests/shell/testcases/transactions/0011chain_0 b/tests/shell/testcases/transactions/0011chain_0 index 3bed16dddf40..bdfa14975180 100755 --- a/tests/shell/testcases/transactions/0011chain_0 +++ b/tests/shell/testcases/transactions/0011chain_0 @@ -5,7 +5,7 @@ set -e RULESET="add table x add chain x y delete chain x y -add chain x y { type filter hook input priority 0; } +add chain x y { type filter hook forward priority 0; } add chain x y { policy drop; }" $NFT -f - <<< "$RULESET" diff --git a/tests/shell/testcases/transactions/dumps/0011chain_0.nft b/tests/shell/testcases/transactions/dumps/0011chain_0.nft index df88ad47c5d9..a12726069efc 100644 --- a/tests/shell/testcases/transactions/dumps/0011chain_0.nft +++ b/tests/shell/testcases/transactions/dumps/0011chain_0.nft @@ -1,5 +1,5 @@ table ip x { chain y { - type filter hook input priority filter; policy drop; + type filter hook forward priority filter; policy drop; } }
It might be convenient to run tests from a development branch that resides on another host, and if we break connectivity on the test host as tests are executed, we con't run them this way. To preserve connectivity, for shell tests, we can simply use the 'forward' hook instead of 'input' in chains/0036_policy_variable_0 and transactions/0011_chain_0, without affecting test coverage. For py tests, this is more complicated as some test cases install chains for all the available hooks, and we would probably need a more refined approach to avoid dropping relevant traffic, so I'm not covering that right now. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- tests/shell/testcases/chains/0036policy_variable_0 | 2 +- tests/shell/testcases/transactions/0011chain_0 | 2 +- tests/shell/testcases/transactions/dumps/0011chain_0.nft | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)