Re: [PATCH nft] tests: Run in separate network namespace, don't break connectivity

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 15 Jun 2020 00:03:09 +0200
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Sun, Jun 14, 2020 at 11:41: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 can't run them this way.
> > 
> > If kernel implementation (CONFIG_NET_NS), unshare(1), or Python
> > bindings for unshare() are not available, warn and continue.
> > 
> > Suggested-by: Phil Sutter <phil@nwl.cc>
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  tests/py/nft-test.py     | 6 ++++++
> >  tests/shell/run-tests.sh | 9 +++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/tests/py/nft-test.py b/tests/py/nft-test.py
> > index 01ee6c980ad4..df97ed8eefb7 100755
> > --- a/tests/py/nft-test.py
> > +++ b/tests/py/nft-test.py
> > @@ -1394,6 +1394,12 @@ def main():
> >      # Change working directory to repository root
> >      os.chdir(TESTS_PATH + "/../..")
> >  
> > +    try:
> > +        import unshare
> > +        unshare.unshare(unshare.CLONE_NEWNET)
> > +    except:
> > +        print_warning("cannot run in own namespace, connectivity might break")
> > +  
> 
> In iptables-tests.py, there is an option for this:
> 
>         parser.add_argument('-N', '--netns', action='store_true',
>                             help='Test netnamespace path')
> 
> Is it worth keeping this in sync with it?

I'm not sure: keeping it in sync would mean that's not the default,
which might result in significant frustration. By the way,
iptables/tests/shell/run-tests.sh just calls unshare -n, instead.

In the past, I guess the --netns flag led to noise and a number of new
errors being found, but nowadays that part should be quite robust and I
don't see the noise.

I think that today the added benefit of an explicit option would come
from the fact that one can check what happened, on failure, on a
single test run, by checking rulesets in the init namespace afterwards.

However, the clean-up strategy is a bit inconsistent right now,
especially with the Python tests: after running some of them as single
runs you might still have a number of chains while rules are deleted --
so I think we should fix that before a flag becomes useful in this
sense.

That could also be achieved by running in a namespace with a known
name, which would need either a patch in the Python bindings here:
	https://github.com/shubham1172/unshare/blob/3e3cc4cd5128976a56601eb5764741fec9c242f8/unshare.c#L26

or wrapping commands with 'ip netns exec' as iptables-tests.py does.

If we do that, I guess we should also introduce a pause-on-fail
mechanism, cf.:
	https://lore.kernel.org/netfilter-devel/20191206101549.4b05f74a@elisabeth/

All in all, from a functionality perspective, I don't think we're
losing anything with this patch -- if you ask me, I'd rather keep this
to avoid the current annoyance, make an explicit option usable in the
future, and then implement the option.

-- 
Stefano