[PATCH nft] tests: shell: flush ruleset with -U after feature probing

[PATCH nft] tests: shell: flush ruleset with -U after feature probing

[Pablo Neira Ayuso]

feature probe script leave a ruleset in place, flush it once probing is
complete.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 tests/shell/run-tests.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index e54f8bf3e3ee..86c8312683cc 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -602,6 +602,9 @@ for feat in "${_HAVE_OPTS[@]}" ; do
 		val="$(bool_n "${!var}")"
 	fi
 	eval "export $var=$val"
+	if [ "$NFT_TEST_HAS_UNSHARED" != y ] ; then
+		$NFT flush ruleset
+	fi
 done
 
 if [ "$NFT_TEST_JOBS" -eq 0 ] ; then
-- 
2.30.2

Re: [PATCH nft] tests: shell: flush ruleset with -U after feature probing

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> feature probe script leave a ruleset in place, flush it once probing is
> complete.

Perhaps change feature_probe() to always use 'unshare -n'?

Some scripts also create netdevices.

Re: [PATCH nft] tests: shell: flush ruleset with -U after feature probing

[Thomas Haller]

On Tue, 2023-12-05 at 20:29 +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > feature probe script leave a ruleset in place, flush it once
> > probing is
> > complete.
> 
> Perhaps change feature_probe() to always use 'unshare -n'?

feature_probe already uses unshare, unless the caller opts out of it.
Maybe don't do that. 

> Some scripts also create netdevices.

Some tests also create netdevices and may not clean them up properly.
It's even desirable that tests don't clean them up, because it removes
boilerplate from tests. But more importantly: not deleting those
devices leaves a certain state after the test, that can be checked by
`.nft`/`.json-nft` dumps.


The mode without unshare exists for historic reasons, as unshare was
added initially. At this point, what is the use of supporting or using
that?


Thomas

Re: [PATCH nft] tests: shell: flush ruleset with -U after feature probing

[Pablo Neira Ayuso]

On Wed, Dec 06, 2023 at 07:47:44AM +0100, Thomas Haller wrote:
> On Tue, 2023-12-05 at 20:29 +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > feature probe script leave a ruleset in place, flush it once
> > > probing is
> > > complete.
> > 
> > Perhaps change feature_probe() to always use 'unshare -n'?
> 
> feature_probe already uses unshare, unless the caller opts out of it.

I am opting out with -I as the patch title specifies.

> Maybe don't do that.
>
> > Some scripts also create netdevices.
> 
> Some tests also create netdevices and may not clean them up properly.
> It's even desirable that tests don't clean them up, because it removes
> boilerplate from tests. But more importantly: not deleting those
> devices leaves a certain state after the test, that can be checked by
> `.nft`/`.json-nft` dumps.

I see, those were not a problem for me when running -U so far.

> The mode without unshare exists for historic reasons, as unshare was
> added initially. At this point, what is the use of supporting or using
> that?

This provides an easy way for me to test 'nft monitor'.

I can keep it out of tree if you prefer -U remains broken.

Re: [PATCH nft] tests: shell: flush ruleset with -U after feature probing

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > The mode without unshare exists for historic reasons, as unshare was
> > added initially. At this point, what is the use of supporting or using
> > that?
> 
> This provides an easy way for me to test 'nft monitor'.
> 
> I can keep it out of tree if you prefer -U remains broken.

No no no, I was just asking if '-U' should still run the
feature probes without a netns, which is what its doing right
now.

Perhaps -U should just disable the unshare for the actual shell
tests, not for the feature probe scripts.

Re: [PATCH nft] tests: shell: flush ruleset with -U after feature probing

[Pablo Neira Ayuso]

On Wed, Dec 06, 2023 at 01:18:28PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > The mode without unshare exists for historic reasons, as unshare was
> > > added initially. At this point, what is the use of supporting or using
> > > that?
> > 
> > This provides an easy way for me to test 'nft monitor'.
> > 
> > I can keep it out of tree if you prefer -U remains broken.
> 
> No no no, I was just asking if '-U' should still run the
> feature probes without a netns, which is what its doing right
> now.
> 
> Perhaps -U should just disable the unshare for the actual shell
> tests, not for the feature probe scripts.

Ah, I understand. Fine with me.

Re: [PATCH nft] tests: shell: flush ruleset with -U after feature probing

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

On Wed, Dec 06, 2023 at 01:33:25PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Dec 06, 2023 at 01:18:28PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > The mode without unshare exists for historic reasons, as unshare was
> > > > added initially. At this point, what is the use of supporting or using
> > > > that?
> > > 
> > > This provides an easy way for me to test 'nft monitor'.
> > > 
> > > I can keep it out of tree if you prefer -U remains broken.
> > 
> > No no no, I was just asking if '-U' should still run the
> > feature probes without a netns, which is what its doing right
> > now.
> > 
> > Perhaps -U should just disable the unshare for the actual shell
> > tests, not for the feature probe scripts.
> 
> Ah, I understand. Fine with me.

Maybe this?

[-- Attachment #2: fix.patch --]
[-- Type: text/x-diff, Size: 1023 bytes --]

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 86c8312683cc..55ebbe6d236d 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -497,6 +497,7 @@ detect_unshare() {
 		return 1
 	fi
 	NFT_TEST_UNSHARE_CMD="$1"
+	NFT_REAL_UNSHARE_CMD="$1"
 	return 0
 }
 
@@ -581,12 +582,12 @@ feature_probe()
 	local with_path="$NFT_TEST_BASEDIR/features/$1"
 
 	if [ -r "$with_path.nft" ] ; then
-		$NFT_TEST_UNSHARE_CMD "$NFT_REAL" --check -f "$with_path.nft" &>/dev/null
+		$NFT_REAL_UNSHARE_CMD "$NFT_REAL" --check -f "$with_path.nft" &>/dev/null
 		return $?
 	fi
 
 	if [ -x "$with_path.sh" ] ; then
-		NFT="$NFT_REAL" $NFT_TEST_UNSHARE_CMD "$with_path.sh" &>/dev/null
+		NFT="$NFT_REAL" $NFT_REAL_UNSHARE_CMD "$with_path.sh" &>/dev/null
 		return $?
 	fi
 
@@ -602,9 +603,6 @@ for feat in "${_HAVE_OPTS[@]}" ; do
 		val="$(bool_n "${!var}")"
 	fi
 	eval "export $var=$val"
-	if [ "$NFT_TEST_HAS_UNSHARED" != y ] ; then
-		$NFT flush ruleset
-	fi
 done
 
 if [ "$NFT_TEST_JOBS" -eq 0 ] ; then

Re: [PATCH nft] tests: shell: flush ruleset with -U after feature probing

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Dec 06, 2023 at 01:33:25PM +0100, Pablo Neira Ayuso wrote:
> > On Wed, Dec 06, 2023 at 01:18:28PM +0100, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > > The mode without unshare exists for historic reasons, as unshare was
> > > > > added initially. At this point, what is the use of supporting or using
> > > > > that?
> > > > 
> > > > This provides an easy way for me to test 'nft monitor'.
> > > > 
> > > > I can keep it out of tree if you prefer -U remains broken.
> > > 
> > > No no no, I was just asking if '-U' should still run the
> > > feature probes without a netns, which is what its doing right
> > > now.
> > > 
> > > Perhaps -U should just disable the unshare for the actual shell
> > > tests, not for the feature probe scripts.
> > 
> > Ah, I understand. Fine with me.
> 
> Maybe this?

Fine with me.

Re: [PATCH nft] tests: shell: flush ruleset with -U after feature probing

[Thomas Haller]

On Wed, 2023-12-06 at 13:52 +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Dec 06, 2023 at 01:33:25PM +0100, Pablo Neira Ayuso wrote:
> > > On Wed, Dec 06, 2023 at 01:18:28PM +0100, Florian Westphal wrote:
> > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > > > The mode without unshare exists for historic reasons, as
> > > > > > unshare was
> > > > > > added initially. At this point, what is the use of
> > > > > > supporting or using
> > > > > > that?
> > > > > 
> > > > > This provides an easy way for me to test 'nft monitor'.
> > > > > 
> > > > > I can keep it out of tree if you prefer -U remains broken.
> > > > 
> > > > No no no, I was just asking if '-U' should still run the
> > > > feature probes without a netns, which is what its doing right
> > > > now.
> > > > 
> > > > Perhaps -U should just disable the unshare for the actual shell
> > > > tests, not for the feature probe scripts.
> > > 
> > > Ah, I understand. Fine with me.
> > 
> > Maybe this?
> 
> Fine with me.
> 

Running with -U and still use unshare seems a contradiction.

What is the point of "-U" then? I thought, it's to support a kernel
where `unshare` doesn't work.

If the purpose is "easy way [...] to test 'nft monitor'", can you
elaborate how you do that? Are there other uses of "-U"?


I think there should be just the proper cleanup after a test/feature-
probe. Either by the test/feature-probe themselves, or (better) by
"run-test.sh".


Thomas

Re: [PATCH nft] tests: shell: flush ruleset with -U after feature probing

[Thomas Haller]

On Wed, 2023-12-06 at 13:14 +0100, Pablo Neira Ayuso wrote:
> On Wed, Dec 06, 2023 at 07:47:44AM +0100, Thomas Haller wrote:
> 
> 
> I can keep it out of tree if you prefer -U remains broken.


IMO the mode definitely should be fixed, as much as possible.
Also, I think the patch is fine. Especially if it fixes an obvious
issue.

If -U is well supported, then tests and feature-detection should take
special care to remove interfaces they create. Maybe they could all use
well-known interfaces names (fwtst0, fwtst1, fwtst2). Then run-test.sh
and test-wrapper.sh could automatically clean up those interfaces. It
doesn't scale to let each test re-implement such cleanup.


> This provides an easy way for me to test 'nft monitor'.

OK then.


Thomas

Re: [PATCH nft] tests: shell: flush ruleset with -U after feature probing

[Pablo Neira Ayuso]

On Wed, Dec 06, 2023 at 01:45:21PM +0100, Thomas Haller wrote:
> On Wed, 2023-12-06 at 13:14 +0100, Pablo Neira Ayuso wrote:
> > On Wed, Dec 06, 2023 at 07:47:44AM +0100, Thomas Haller wrote:
> > 
> > 
> > I can keep it out of tree if you prefer -U remains broken.
> 
> 
> IMO the mode definitely should be fixed, as much as possible.
> Also, I think the patch is fine. Especially if it fixes an obvious
> issue.
> 
> If -U is well supported, then tests and feature-detection should take
> special care to remove interfaces they create. Maybe they could all use
> well-known interfaces names (fwtst0, fwtst1, fwtst2). Then run-test.sh
> and test-wrapper.sh could automatically clean up those interfaces. It
> doesn't scale to let each test re-implement such cleanup.

I agree with Florian and you that removing toggles is a good idea, but
this one is useful for me at this stage, we revisit later on. There is
a specific tests infra for the monitor mode but it is rather limited.
I can also crash nft monitor with a few scenarios I am looking at to
fix it. Maybe -U can go away in the near future and monitor tests can
get better coverage.

> > This provides an easy way for me to test 'nft monitor'.
> 
> OK then.

Thanks!

Re: [PATCH nft] tests: shell: flush ruleset with -U after feature probing

[Pablo Neira Ayuso]

On Tue, Dec 05, 2023 at 08:29:29PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > feature probe script leave a ruleset in place, flush it once probing is
> > complete.
> 
> Perhaps change feature_probe() to always use 'unshare -n'?

I am currently using this to test 'nft monitor' and make sure it
survives a run-tests.sh

> Some scripts also create netdevices.

Indeed, they are leaving things in place.