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

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

[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 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")
+
     check_lib_path = True
     if args.library is None:
         if args.host:
diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index fcc87a8957c8..328c412e66f9 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -22,6 +22,15 @@ if [ "$(id -u)" != "0" ] ; then
 	msg_error "this requires root!"
 fi
 
+if [ "${1}" != "run" ]; then
+	if unshare -f -n true; then
+		unshare -n "${0}" run $@
+		exit $?
+	fi
+	msg_warn "cannot run in own namespace, connectivity might break"
+fi
+shift
+
 [ -z "$NFT" ] && NFT=$SRC_NFT
 ${NFT} > /dev/null 2>&1
 ret=$?
-- 
2.27.0

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

[Pablo Neira Ayuso]

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?

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

[Stefano Brivio]

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

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

[Phil Sutter]

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

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

[Pablo Neira Ayuso]

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.