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

[Phil Sutter <phil@nwl.cc>]

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