On Thu, 2023-08-31 at 20:18 +0200, Florian Westphal wrote: > Thomas Haller <thaller@xxxxxxxxxx> wrote: > > On Thu, 2023-08-31 at 18:08 +0200, Florian Westphal wrote: > > > > For that to be useful, we must also unshare the PID and user > > > > namespace > > > > and map the root user inside that namespace. > > > > > > Are you sure PIDNS unshare is needed for this? > > > > Probably not, but does it hurt? I think it should be > > > > unshare -f -p -U -n --mount-proc --map-root-user \ > > bash -xc 'whoami; ip link; sleep 1 & ps aux' > > Its an extra kernel option that needs to be enabled for this to work. > Probably on for all distro kernels by now but whats the added > benefit? I think we should isolate more, if we can. In v2, there is a test an fallback to without "-P". How about that? > > > > > Test that don't work without real root should check for > > > > [ "$NFT_TEST_HAVE_REALROOT" != 1 ] and skip gracefully. > > > > > > Thats fine, see my recent RFC to add such environment > > > variables to check if a particular feature is supported or not. > > > > > > What I don't like here is the NFT_TEST_ROOTLESS environment > > > variable to alter behaviour of run-tests.sh behavior, but see > > > below. > > > > If you don't run with real-root, then you are maybe not testing the > > full thing and miss something. Hence, you have to opt-in to the new > > rootless functionality with NFT_TEST_ROOTLESS=1. > > I disagree, I think a notice is fine, this disclaimer could be > repeated after test summary. In v2, NFT_TEST_ROOTLESS is dropped and a notice added. > > Especially if we start skipping tests we should also have > an indication that not all tests were run (ideally, we'd see which > ones and how many...). > > > The check is to preserve the previous behavior, which failed > > without > > real-root. > > If you absolutely insist then fine. I don't. Dropped in v2. > > > > Why not get rid of the check? Just auto-switch to unpriv userns > > > and > > > error out if that fails. You could just print a warning/notice > > > here > > > and > > > then try userns mode. And/or print a notice at the together with > > > the > > > test summary. > > > > Which check? About NFT_TEST_HAVE_REALROOT? > > I meant $(id) -eq 0 || barf > > I think the best is to: > ./run-tests.sh called with uid 0 -> no changes > ./run-tests.sh called with uid > 0 -> try unpriv netns and set > NFT_TEST_HAVE_USERNS=1 in the environment to allow test cases to > adjust > if needed (e.g. bail out early). Addressed in v2 (with some variations). > > > podman run --privileged -ti fedora:latest > > > > then `id -u` would wrongly indicate that the test has real-root. > > You > > can override that with `export NFT_TEST_HAVE_REALROOT=0` to run in > > rootless mode. > > Ah. Well, with my proposal above you can still set > NFT_TEST_HAVE_USERNS=1 manually before ./run-tests.sh if uid 0 isn't > really uid 0. > > > > > +if [ "$NFT_TEST_NO_UNSHARE" = 1 ]; then > > > > + # The user opts-out from unshare. Proceed without. > > > > > > Whats the use case? If there is a good one, then i'd prefer a > > > command > > > line switch rather than environment. > > > > I think command line switches are inferior. They are cumbersome to > > implement (in the script, the order of options surprisingly > > matters). > > They are also more limited in their usage. The script should > > integrate > > with a `make check-xyz` step or be called from others scripts, > > where > > the command line is not directly editable. Also, the name > > NFT_TEST_NO_UNSHARE is something unique that you can grep for > > (unlike > > the "-g" command line switch. > > Yuck. Do we need a totally different script then? No. run-tests.sh is currently *the* runner script for running tests. It must always be useful to call directly. But it *might* be useful to call it from a wrapper script (like a make target). > > ./run-tests.sh is for humans, not machines. I want sane defaults. > > also expected a failure to `unshare`, then printed a warning and > > proceeded without separate namespace. I think that fallback is > > undesirable, I would not want to run the test accidentally in the > > main > > namespace. > > Then add a warning? I very much dislike these environment variables, > developers will add them to their bash init scripts and then there is > big surprise why someone reports unexpected results/behaviours. > > Command line options are typically known, environment not. > Don't get me wrong, environment variables are good, I have no > objections > to setting NFT_TEST_HAVE_USERNS or the like for the individual tests > to > consume. Already before, the script supports various command line options, but those options were also settable via an environment variable. E.g. "VERBOSE=y" vs. "-v". I followed that style in v2. How about that? > > > Now, by default it always tries to unshare, and aborts on failure. > > The > > variable is there to opt-out from that. > > Can't we have a sane default without a need for new command line > options > or enviroment variables? The only unsane default was NFT_TEST_ROOTLESS to opt-in to make something work. It now works by default. > > > I'd like to integrate tests more into `make check-*`. I'd also like > > to > > add unit tests (that is, both statically and dynamically link with > > libnftables and to be able to unit test code specifically). As a > > developer, I'd like to type `make check` to run the the unit tests > > and > > have a clear make target for the integration tests. > > Thats a good thing. I do not care if I have to call ./run-tests.sh > or 'make tests', but please, sane defaults without code path > explosion. > Thomas
