[PATCH nft v2 0/3] tests/shell: allow running tests as non-root

[PATCH nft v2 0/3] tests/shell: allow running tests as non-root

[Thomas Haller]

Changes to v2:

- new patch: rework the parsing of command line options
- new patch: add a "--list-tests" option to show the found tests
- call "unshare" for each test individually.
- drop NFT_TEST_ROOTLESS environment variable. You no longer have to
  opt-in to run rootless. However, if any tests fail and we ran
  rootless, then an info is printed at the end.
- the environment variables NFT_TEST_HAVE_REALROOT and
  NFT_TEST_NO_UNSHARE can still be set to configure the script.
  Those are now also configurable via command line options.
  Usually you would not have to set them.

Thomas Haller (3):
  tests/shell: rework command line parsing in "run-tests.sh"
  tests/shell: rework finding tests and add "--list-tests" option
  tests/shell: run each test in separate namespace and allow rootless

 tests/shell/run-tests.sh | 191 +++++++++++++++++++++++++++------------
 1 file changed, 132 insertions(+), 59 deletions(-)

-- 
2.41.0

[PATCH nft v2 1/3] tests/shell: rework command line parsing in "run-tests.sh"

[Thomas Haller]

Parse the arguments in a loop, so that their order does not matter.
Also, it would be easier to parse flags with parameters.

Currently this is after the is-root check and after unshare. That makes
no sense with the "--help" option. That will be addressed next.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/run-tests.sh | 94 +++++++++++++++++++++++++++-------------
 1 file changed, 64 insertions(+), 30 deletions(-)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index b66ef4fa4d1f..2ece280a2408 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -1,10 +1,5 @@
 #!/bin/bash
 
-# Configuration
-TESTDIR="./$(dirname $0)/testcases"
-SRC_NFT="$(dirname $0)/../../src/nft"
-DIFF=$(which diff)
-
 msg_error() {
 	echo "E: $1 ..." >&2
 	exit 1
@@ -18,6 +13,28 @@ msg_info() {
 	echo "I: $1"
 }
 
+usage() {
+	echo " $0 [OPTIONS]"
+	echo
+	echo "OPTIONS:"
+	echo " \"-v\" : also VERBOSE=y"
+	echo " \"-g\" : also DUMPGEN=y"
+	echo " \"-V\" : also VALGRIND=y"
+	echo " \"-K\" : also KMEMLEAK=y"
+	echo
+	echo "VARIABLES:"
+	echo "  NFT=<PATH>   : Path to nft executable"
+	echo "  VERBOSE=*|y  : See also \"-v\" option"
+	echo "  DUMPGEN=*|y  : See also \"-g\" option"
+	echo "  VALGRIND=*|y : See also \"-V\" option"
+	echo "  KMEMLEAK=*|y : See also \"-y\" option"
+}
+
+# Configuration
+TESTDIR="./$(dirname $0)/testcases"
+SRC_NFT="$(dirname $0)/../../src/nft"
+DIFF=$(which diff)
+
 if [ "$(id -u)" != "0" ] ; then
 	msg_error "this requires root!"
 fi
@@ -31,6 +48,48 @@ if [ "${1}" != "run" ]; then
 fi
 shift
 
+VERBOSE="$VERBOSE"
+DUMPGEN="$DUMPGEN"
+VALGRIND="$VALGRIND"
+KMEMLEAK="$KMEMLEAK"
+
+TESTS=()
+
+while [ $# -gt 0 ] ; do
+	A="$1"
+	shift
+	case "$A" in
+		-v)
+			VERBOSE=y
+			;;
+		-g)
+			DUMPGEN=y
+			;;
+		-V)
+			VALGRIND=y
+			;;
+		-K)
+			KMEMLEAK=y
+			;;
+		-h|--help)
+			usage
+			exit 0
+			;;
+		--)
+			TESTS=("$@")
+			VERBOSE=y
+			shift $#
+			;;
+		*)
+			TESTS=("$A" "$@")
+			VERBOSE=y
+			shift $#
+			;;
+	esac
+done
+
+SINGLE="${TESTS[*]}"
+
 [ -z "$NFT" ] && NFT=$SRC_NFT
 ${NFT} > /dev/null 2>&1
 ret=$?
@@ -59,31 +118,6 @@ if [ ! -x "$DIFF" ] ; then
 	DIFF=true
 fi
 
-if [ "$1" == "-v" ] ; then
-	VERBOSE=y
-	shift
-fi
-
-if [ "$1" == "-g" ] ; then
-	DUMPGEN=y
-	shift
-fi
-
-if [ "$1" == "-V" ] ; then
-	VALGRIND=y
-	shift
-fi
-
-if [ "$1" == "-K" ]; then
-	KMEMLEAK=y
-	shift
-fi
-
-for arg in "$@"; do
-	SINGLE+=" $arg"
-	VERBOSE=y
-done
-
 kernel_cleanup() {
 	$NFT flush ruleset
 	$MODPROBE -raq \
-- 
2.41.0

[PATCH nft v2 2/3] tests/shell: rework finding tests and add "--list-tests" option

[Thomas Haller]

Cleanup finding the test files. Also add a "--list-tests" option to see
which tests are found and would run.

Also get rid of the FIND="$(which find)" detection. It's not that the
user could set the used find program via an environment variable. Also,
which system doesn't have "find"? Just fail when our call to "find"
fails.

This is still  after "unshare", which makes no sense and will be
addressed next.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/run-tests.sh | 52 ++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 2ece280a2408..147185cb548a 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -17,10 +17,11 @@ usage() {
 	echo " $0 [OPTIONS]"
 	echo
 	echo "OPTIONS:"
-	echo " \"-v\" : also VERBOSE=y"
-	echo " \"-g\" : also DUMPGEN=y"
-	echo " \"-V\" : also VALGRIND=y"
-	echo " \"-K\" : also KMEMLEAK=y"
+	echo " \"-v\"                 : also VERBOSE=y"
+	echo " \"-g\"                 : also DUMPGEN=y"
+	echo " \"-V\"                 : also VALGRIND=y"
+	echo " \"-K\"                 : also KMEMLEAK=y"
+	echo " \"-L\"|\"-list-tests\" : list the test name and quit"
 	echo
 	echo "VARIABLES:"
 	echo "  NFT=<PATH>   : Path to nft executable"
@@ -31,8 +32,9 @@ usage() {
 }
 
 # Configuration
-TESTDIR="./$(dirname $0)/testcases"
-SRC_NFT="$(dirname $0)/../../src/nft"
+BASEDIR="$(dirname "$0")"
+TESTDIR="$BASEDIR/testcases"
+SRC_NFT="$BASEDIR/../../src/nft"
 DIFF=$(which diff)
 
 if [ "$(id -u)" != "0" ] ; then
@@ -52,6 +54,7 @@ VERBOSE="$VERBOSE"
 DUMPGEN="$DUMPGEN"
 VALGRIND="$VALGRIND"
 KMEMLEAK="$KMEMLEAK"
+DO_LIST_TESTS=
 
 TESTS=()
 
@@ -75,6 +78,9 @@ while [ $# -gt 0 ] ; do
 			usage
 			exit 0
 			;;
+		-L|--list-tests)
+			DO_LIST_TESTS=y
+			;;
 		--)
 			TESTS=("$@")
 			VERBOSE=y
@@ -88,7 +94,19 @@ while [ $# -gt 0 ] ; do
 	esac
 done
 
-SINGLE="${TESTS[*]}"
+if [ ! -d "$TESTDIR" ] ; then
+	msg_error "missing testdir $TESTDIR"
+fi
+
+if [ "${#TESTS[@]}" -eq 0 ]; then
+	TESTS=( $(find "$TESTDIR" -type f -executable | sort) ) || \
+		msg_error "Could not find tests"
+fi
+
+if [ "$DO_LIST_TESTS" = y ] ; then
+	printf '%s\n' "${TESTS[@]}"
+	exit 0
+fi
 
 [ -z "$NFT" ] && NFT=$SRC_NFT
 ${NFT} > /dev/null 2>&1
@@ -99,15 +117,6 @@ else
 	msg_info "using nft command: ${NFT}"
 fi
 
-if [ ! -d "$TESTDIR" ] ; then
-	msg_error "missing testdir $TESTDIR"
-fi
-
-FIND="$(which find)"
-if [ ! -x "$FIND" ] ; then
-	msg_error "no find binary found"
-fi
-
 MODPROBE="$(which modprobe)"
 if [ ! -x "$MODPROBE" ] ; then
 	msg_error "no modprobe binary found"
@@ -142,14 +151,6 @@ kernel_cleanup() {
 	nft_xfrm
 }
 
-find_tests() {
-	if [ ! -z "$SINGLE" ] ; then
-		echo $SINGLE
-		return
-	fi
-	${FIND} ${TESTDIR} -type f -executable | sort
-}
-
 printscript() { # (cmd, tmpd)
 	cat <<EOF
 #!/bin/bash
@@ -247,8 +248,7 @@ check_kmemleak()
 
 check_taint
 
-for testfile in $(find_tests)
-do
+for testfile in "${TESTS[@]}" ; do
 	read taint < /proc/sys/kernel/tainted
 	kernel_cleanup
 
-- 
2.41.0

[PATCH nft v2 3/3] tests/shell: run each test in separate namespace and allow rootless

[Thomas Haller]

Don't unshare the entire shell script, calling itself again. Instead,
call unshare separately for each test invocation. That means, all tests
use now a different sandbox.

Also, allow to run rootless/unprivileged.

The user still can opt-out from unshare via -U/--no-unshare option
or NFT_TEST_NO_UNSHARE=y. That might be useful if unshare for some
reason doesn't work for the user, or if you want to test on your host
system.

Also try to also run a separate PID namespace, to get more isolation. If
that is not working, then fallback without separate PID namespace.

We no longer require [ `id -u` = 0 ] to proceed, so a rootless user can
run the tests. We detect whether we have real root and set
NFT_TEST_HAVE_REALROOT=y. Some tests won't work rootless. For example,
the socket buffers is limited by /proc/sys/net/core/{wmem_max,rmem_max}
which real-root can override, but rootless tests cannot. Such tests
should check for [ "$NFT_TEST_HAVE_REALROOT" != y ] and skip the test
gracefully. Optimally, tests also work in the rootless environment and
most tests should pass in both, and not check for have-realroot.

Usually, the user doesn't need to tell the script whether they have real
root. The script will autodetect it via [ `id -u` = 0 ]. But that won't
work if you run inside a rootless container already. In that case, you
would want to tell the script that we don't have real-root. Either via
-R/--without-root option or NFT_TEST_HAVE_REALROOT=n.

If they wish, the test can know whether they run inside "unshare"
environment by checking for [ "$NFT_TEST_IS_UNSHARED" = y ].

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/run-tests.sh | 83 +++++++++++++++++++++++++++++-----------
 1 file changed, 61 insertions(+), 22 deletions(-)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 147185cb548a..fda738983ec9 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -17,11 +17,13 @@ usage() {
 	echo " $0 [OPTIONS]"
 	echo
 	echo "OPTIONS:"
-	echo " \"-v\"                 : also VERBOSE=y"
-	echo " \"-g\"                 : also DUMPGEN=y"
-	echo " \"-V\"                 : also VALGRIND=y"
-	echo " \"-K\"                 : also KMEMLEAK=y"
-	echo " \"-L\"|\"-list-tests\" : list the test name and quit"
+	echo " \"-v\"                   : also VERBOSE=y"
+	echo " \"-g\"                   : also DUMPGEN=y"
+	echo " \"-V\"                   : also VALGRIND=y"
+	echo " \"-K\"                   : also KMEMLEAK=y"
+	echo " \"-L\"|\"--list-tests\"  : list the test name and quit"
+	echo " \"-R\"|\"--without-realroot\" : sets NFT_TEST_HAVE_REALROOT=n"
+	echo " \"-U\"|\"--no-unshare\"  : sets NFT_TEST_NO_UNSHARE=y"
 	echo
 	echo "VARIABLES:"
 	echo "  NFT=<PATH>   : Path to nft executable"
@@ -29,31 +31,28 @@ usage() {
 	echo "  DUMPGEN=*|y  : See also \"-g\" option"
 	echo "  VALGRIND=*|y : See also \"-V\" option"
 	echo "  KMEMLEAK=*|y : See also \"-y\" option"
+	echo "  NFT_TEST_HAVE_REALROOT=*|y : To indicate whether the test has real root permissions."
+	echo "                 Usually, you don't need this and it gets autodetected."
+	echo "                 You might want to set it, if you know better than the"
+	echo "                 \`id -u\` check, whether the user is root in the main namespace."
+	echo "                 Note that without real root, certain tests may not work,"
+	echo "                 e.g. due to limited /proc/sys/net/core/{wmem_max,rmem_max}."
+	echo "                 Checks that cannot pass in such environment should check for"
+	echo "                 [ \"$NFT_TEST_HAVE_REALROOT\" != y ] and skip gracefully."
+	echo " NFT_TEST_NO_UNSHARE=*|y : Usually, each test will run in a separate namespace."
+	echo "                 You can opt-out from that by setting NFT_TEST_NO_UNSHARE=y."
 }
 
-# Configuration
 BASEDIR="$(dirname "$0")"
 TESTDIR="$BASEDIR/testcases"
 SRC_NFT="$BASEDIR/../../src/nft"
-DIFF=$(which diff)
-
-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
 
 VERBOSE="$VERBOSE"
 DUMPGEN="$DUMPGEN"
 VALGRIND="$VALGRIND"
 KMEMLEAK="$KMEMLEAK"
+NFT_TEST_HAVE_REALROOT="$NFT_TEST_HAVE_REALROOT"
+NFT_TEST_NO_UNSHARE="$NFT_TEST_NO_UNSHARE"
 DO_LIST_TESTS=
 
 TESTS=()
@@ -81,6 +80,12 @@ while [ $# -gt 0 ] ; do
 		-L|--list-tests)
 			DO_LIST_TESTS=y
 			;;
+		-R|--without-realroot)
+			NFT_TEST_HAVE_REALROOT=n
+			;;
+		-U|--no-unshare)
+			NFT_TEST_NO_UNSHARE=y
+			;;
 		--)
 			TESTS=("$@")
 			VERBOSE=y
@@ -108,6 +113,33 @@ if [ "$DO_LIST_TESTS" = y ] ; then
 	exit 0
 fi
 
+if [ "$NFT_TEST_HAVE_REALROOT" = "" ] ; then
+	# The caller didn't set NFT_TEST_HAVE_REALROOT and didn't specify
+	# -R/--without-root option. Autodetect it based on `id -u`.
+	export NFT_TEST_HAVE_REALROOT="$(test "$(id -u)" = "0" && echo y || echo n)"
+fi
+
+UNSHARE=()
+NFT_TEST_IS_UNSHARED=n
+if [ "$NFT_TEST_NO_UNSHARE" != y ]; then
+	# We unshare both if we NFT_TEST_HAVE_REALROOT and the rootless/unpriv
+	# case. Without real root, some tests may fail. Tests that don't work
+	# without real root should check for [ "$NFT_TEST_HAVE_REALROOT" != y ]
+	# and skip gracefully.
+	UNSHARE=( unshare -f -p --mount-proc -U --map-root-user -n )
+	if ! "${UNSHARE[@]}" true ; then
+		# Try without PID namespace.
+		UNSHARE=( unshare -f -U --map-root-user -n )
+		if ! "${UNSHARE[@]}" true ; then
+			msg_error "Unshare does not work. Rerun with -U/--no-unshare or NFT_TEST_NO_UNSHARE=y"
+		fi
+	fi
+	NFT_TEST_IS_UNSHARED=y
+fi
+
+# If they wish, test can check whether they run unshared.
+export NFT_TEST_IS_UNSHARED
+
 [ -z "$NFT" ] && NFT=$SRC_NFT
 ${NFT} > /dev/null 2>&1
 ret=$?
@@ -128,7 +160,9 @@ if [ ! -x "$DIFF" ] ; then
 fi
 
 kernel_cleanup() {
-	$NFT flush ruleset
+	if [ "$NFT_TEST_IS_UNSHARED" != y ] ; then
+		$NFT flush ruleset
+	fi
 	$MODPROBE -raq \
 	nft_reject_ipv4 nft_reject_bridge nft_reject_ipv6 nft_reject \
 	nft_redir_ipv4 nft_redir_ipv6 nft_redir \
@@ -253,7 +287,7 @@ for testfile in "${TESTS[@]}" ; do
 	kernel_cleanup
 
 	msg_info "[EXECUTING]	$testfile"
-	test_output=$(NFT="$NFT" DIFF=$DIFF ${testfile} 2>&1)
+	test_output=$(NFT="$NFT" DIFF=$DIFF "${UNSHARE[@]}" "$testfile" 2>&1)
 	rc_got=$?
 	echo -en "\033[1A\033[K" # clean the [EXECUTING] foobar line
 
@@ -312,4 +346,9 @@ check_kmemleak_force
 msg_info "results: [OK] $ok [FAILED] $failed [TOTAL] $((ok+failed))"
 
 kernel_cleanup
+
+if [ "$failed" -gt 0 -a "$NFT_TEST_HAVE_REALROOT" != y ] ; then
+	msg_info "test was not running as real root"
+fi
+
 [ "$failed" -eq 0 ]
-- 
2.41.0

Re: [PATCH nft v2 3/3] tests/shell: run each test in separate namespace and allow rootless

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> +	UNSHARE=( unshare -f -p --mount-proc -U --map-root-user -n )
> +	if ! "${UNSHARE[@]}" true ; then
> +		# Try without PID namespace.
> +		UNSHARE=( unshare -f -U --map-root-user -n )
> +		if ! "${UNSHARE[@]}" true ; then
> +			msg_error "Unshare does not work. Rerun with -U/--no-unshare or NFT_TEST_NO_UNSHARE=y"

This will always fail here due to
user.max_user_namespaces=0

in sysctl.cfg.

So please add a fallback to plain unshare -n or only use unpriv userns
if the script isn't called with uid 0.

>  	msg_info "[EXECUTING]	$testfile"
> -	test_output=$(NFT="$NFT" DIFF=$DIFF ${testfile} 2>&1)
> +	test_output=$(NFT="$NFT" DIFF=$DIFF "${UNSHARE[@]}" "$testfile" 2>&1)
>  	rc_got=$?

This is more complicated because we'll also need to collect the ruleset
dump from within the temporary ns.

Once all of that works you can remove kernel_cleanup().

Re: [PATCH nft v2 0/3] tests/shell: allow running tests as non-root

[Thomas Haller]

On Fri, 2023-09-01 at 17:05 +0200, Thomas Haller wrote:
> Changes to v2:
> 
> - new patch: rework the parsing of command line options
> - new patch: add a "--list-tests" option to show the found tests
> - call "unshare" for each test individually.
> - drop NFT_TEST_ROOTLESS environment variable. You no longer have to
>   opt-in to run rootless. However, if any tests fail and we ran
>   rootless, then an info is printed at the end.
> - the environment variables NFT_TEST_HAVE_REALROOT and
>   NFT_TEST_NO_UNSHARE can still be set to configure the script.
>   Those are now also configurable via command line options.
>   Usually you would not have to set them.
> 
> Thomas Haller (3):
>   tests/shell: rework command line parsing in "run-tests.sh"
>   tests/shell: rework finding tests and add "--list-tests" option
>   tests/shell: run each test in separate namespace and allow rootless
> 
>  tests/shell/run-tests.sh | 191 +++++++++++++++++++++++++++----------
> --
>  1 file changed, 132 insertions(+), 59 deletions(-)
> 

v2 has still several problems. I got too impatient before sending the
patches.

v3 will fix those, and add several further improvements... hold on.


Thomas