[PATCH nft] tests: shell: Avoid breaking basic connectivity when run

[PATCH nft] tests: shell: Avoid breaking basic connectivity when run

[Stefano Brivio]

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(-)

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;
 	}
 }
-- 
2.26.2

Re: [PATCH nft] tests: shell: Avoid breaking basic connectivity when run

[Phil Sutter]

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

Re: [PATCH nft] tests: shell: Avoid breaking basic connectivity when run

[Stefano Brivio]

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

-- 
Stefano

Re: [PATCH nft] tests: shell: Avoid breaking basic connectivity when run

[Phil Sutter]

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

Re: [PATCH nft] tests: shell: Avoid breaking basic connectivity when run

[Stefano Brivio]

[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. :)

-- 
Stefano