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

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

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?

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.

Hi Pablo,

On Mon, Jun 15, 2020 at 12:03:09AM +0200, Pablo Neira Ayuso wrote:
[...]
> 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?

There's one peculiar comment in iptables-test.py which makes me believe
this "run in netns" option is distinct from Stefano's:

|    # Test "ip netns del NETNS" path with rules in place
|    if netns:
|        return 0

I remember calling iptables-test.py with --netns option triggering a
kernel bug that didn't happen if called with 'ip netns exec ...'
instead. And IIUC, the code path executed by --netns option still does
if wrapped by 'ip netns exec ...'. Therefore I vote for keeping --netns
option and still doing that implicit 'unshare -n' to separate the
testing env from the host's.

Cheers, Phil

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.

Applied, thanks.