On Mon, 15 Jun 2020 00:03:09 +0200
Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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@xxxxxx>
> > Signed-off-by: Stefano Brivio <sbrivio@xxxxxxxxxx>
> > ---
> > 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
