tests: add `set -u` by kolyshkin · Pull Request #3367 · opencontainers/runc

kolyshkin · 2022-02-07T22:48:22Z

Fix all existing cases where set -u can be problematic, and add it globally for all tests.

This is a way to prevent the code doing something really bad when a
variable it uses is not set. Good to have since it helps to catch some
logical errors etc.

This also fixes a bug of not running CRIU tests; see #3367 (comment) for details.

For brevity, this PR concentrates on enabling set -u only. Various cleanups on top of it are in #3369.

kolyshkin · 2022-02-09T02:16:24Z

Aaand it's finally working as it should.

thaJeztah · 2022-02-09T10:47:41Z

 	# Killing the CRIU on the checkpoint side will let the container
 	# continue to run if the migration failed at some point.
-	[ -n "$RUNC_USE_SYSTEMD" ] && set_cgroups_path
+	[ -v RUNC_USE_SYSTEMD ] && set_cgroups_path


I'm not familiar with the -v option (tried searching the man page, but couldn't find it 😅); what does it do? Is it something Bash specific? (in which case perhaps this one should explicitly use [[)

If it's to differentiate between "var not set" and "var set, but empty", I'm wondering if it's more error-prone 🤔 (maybe not, not sure, would have to think of various scenarios)

help [
help [[

It tells if the variable is set.

It is bash specific, but we use bats, which is already bash-specific, and have /bin/bash shebang everywhere.

[ and [[ have slightly different rules and both support -v in bash.

The alternative, also bash-specific, is something like [ -n "${VAR:-x}" ], which looks way more cryptic than -v.

Interesting; neither help [ / help [[ nor man [ / man test (man [[ returns an error on Ubuntu) showed it (at least, couldn't find it).

So my concern is a bit that -v ENABLE_FOO only checks if it's set, but can be confusing if (e.g.) ENABLE_FOO=0

(set -eu; [ -v ENABLE_FOO ] && echo "FOO is enabled"; echo $?) 1 (set -eu; ENABLE_FOO=0; [ -v ENABLE_FOO ] && echo "FOO is enabled"; echo $?) FOO is enabled 0 (set -eu; ENABLE_FOO=1; [ -v ENABLE_FOO ] && echo "FOO is enabled"; echo $?) FOO is enabled 0

From that perspective, [ "${ENABLE_FOO}" = "1" ] (e.g.) feels more transparent (and possibly less error prone?)
But if we really need -u everywhere, that would probably need some initialization code to be sure variables
are set to a default, like

: "${ENABLE_FOO:=0}"

(which we do for $ROOTLESS_FEATURES I see)

Interesting; neither help [ / help [[ nor man [ / man test (man [[ returns an error on Ubuntu) showed it (at least, couldn't find it).

Both [ and [[ are bash built-ins so man won't help. There is a stand-alone test binary though (on my system, that is).

help test actually shows all these flags, including -v.

Since bash is a GNU project, the main documentation is in info format (i.e. info bash; info is supposed to be a man replacement with better feature set and using tex rather than groff as a backend, although it haven't got much traction). It's also available online at https://www.gnu.org/software/bash/manual/; we are interested in

https://www.gnu.org/software/bash/manual/html_node/Bash-Conditional-Expressions.html

https://www.gnu.org/software/bash/manual/html_node/Conditional-Constructs.html

It looks like test/[/[[ -v appeared in bash-4.2, released in November 2011.

IOW this is addressed in #3369

I agree that -v VAR is very subtly different from -n $VAR -- when I make these conversions, I usually just use :- (or -) such that it doesn't trip set -u (unless it's used multiple times, then I use the : "${VAR:=}" as suggested by Sebastiaan):

Suggested change

[ -v RUNC_USE_SYSTEMD ] && set_cgroups_path

[ -n "${RUNC_USE_SYSTEMD:-}" ] && set_cgroups_path

It looks more cryptic, but it matches the old behavior (where both empty and unset are considered "false" -- in many cases/places, it's trivial to set a variable to false and impossible to unset it, like Docker's --env for example).

@tianon thanks for your input!

I am very open to use '${VAR:-}where appropriate, but to me it seems that-v VAR` is a tad cleaner, because it is more readable, and readability is a big factor in my book.

Note that I only switch to -v in cases where the flag is essentially a binary 0/1, since set/unset fits it better than

empty/non-empty (because we do not have to provide a kludge to satisfy set -u),

"yes"/"no", 1/0 etc. (because string or numeric values are not standardized anywhere in the code, and we don't have to set the "no"/0 case).

I can certainly rework the whole set again to use -n|-z "${VAR:-}" instead of [!] -v VAR, if I clearly see its benefits.

You point out to two things

Matching the old behavior. I agree, but it is not an issue here since this is an isolated codebase which we fully control, so we can convert it all in one sitting (which is what this PR does) and won't add any new bugs (well, hopefully).

Inability to unset a variable when using Docker. Also true, but I don't see this is practically an issue (for the same reason -- this is an isolated set of tests which we run in some specific environments we fully control).

My third reason is sheer laziness -- it took a while to convert to -v, and I have another PR which is based on this one (brining in some more cleanups). I can't convince myself to spend a few hours to rework this (and that PR) one more time.

All it all, I'd like to leave this as is, unless there's a strong objection. I am listening and can definitely reconsider.

Totally fair 👍

thaJeztah · 2022-02-09T10:57:47Z

-		args+=("--rootless")
-	fi
+	local rootless=""
+	[ "$ROOTLESS" -ne 0 ] && rootless="--rootless"


I got confused here for a bit, and thought that $ROOTLESS was a boolean(isn) variable, but it looks to be the uid (from a couple of lines up)

# Check if we're in rootless mode. ROOTLESS=$(id -u)

Do we need the actual id in the tests? If not, I'm wondering if we should change that line to set $ROOTLESS to a boolean(ish) value (empty or "1"); that would make the comment on it more matching reality, and perhaps make it easier (less error-prone) to use overall

I have a follow-up PR which removes ROOTLESS entirely and uses UID instead. The mere aim of this PR is to enable set -u (which turned out to be quite challenging).

IOW this is addressed in #3369

kolyshkin · 2022-02-15T03:42:15Z

@thaJeztah I think I have addressed all of your review comments; please take another look at this 🙏🏻

kolyshkin · 2022-02-22T03:27:44Z

Now, this PR is not about "making our shell scripts nicer". It is a way to catch very bad errors.

For example, consider the following use case, where setup_test sets TESTDIR, and then its value is used:

setup_test
VARDIR="$TESTDIR/var"
mkdir -p "$VARDIR"
....
rm -rf "$VARDIR"

All is fine and dandy. Now, someone replicates this code, but makes one small mistake:

VARDIR="$TESTDIR/var"
setup_test
mkdir -p $"VARDIR"
....
rm -rf "$VARDIR

This error is not getting caught by bash itself or e.g. shellcheck, and as a result, host /var will be removed as a result of running this "test". This is a somewhat extreme but quite realistic example; in fact, this PR was motivated by a very similar issue found in cri-o.

PTAL @AkihiroSuda @thaJeztah @cyphar

kolyshkin · 2022-02-22T03:28:19Z

@tianon I'd appreciate your review on that, if you can spare some time.

thaJeztah

SGTM to fix the issue, but always open to suggestions from @tianon :)

tianon

I was scrolling through the comments here and had almost written off reviewing this when I saw your explicit call-out -- I'm honored to review. 😅 ❤️ 🙇

I agree with @thaJeztah re: -v vs -n/-z, but I've put a bit more detail in the appropriate conversation below.

(I think the next "goal" I'd suggest is consistent use of set -e and even set -o pipefail, but as noted below, that's probably an even bigger bridge 😇)

tianon · 2022-02-24T15:38:37Z

 	# Killing the CRIU on the checkpoint side will let the container
 	# continue to run if the migration failed at some point.
-	[ -n "$RUNC_USE_SYSTEMD" ] && set_cgroups_path
+	[ -v RUNC_USE_SYSTEMD ] && set_cgroups_path


I agree that -v VAR is very subtly different from -n $VAR -- when I make these conversions, I usually just use :- (or -) such that it doesn't trip set -u (unless it's used multiple times, then I use the : "${VAR:=}" as suggested by Sebastiaan):

Suggested change

[ -v RUNC_USE_SYSTEMD ] && set_cgroups_path

[ -n "${RUNC_USE_SYSTEMD:-}" ] && set_cgroups_path

It looks more cryptic, but it matches the old behavior (where both empty and unset are considered "false" -- in many cases/places, it's trivial to set a variable to false and impossible to unset it, like Docker's --env for example).

Older bash versions treats variable as unset if nothing has been assigned to it. Here is an example from CentOS 7 system: [kir@localhost ~]$ bash -u -c 'x() { local args=(); echo "${args[@]}"; }; x' bash: args[@]: unbound variable [kir@localhost ~]$ echo $BASH_VERSION 4.2.46(2)-release Rewrite to work around this. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Some tests (those in help.bats and version.bats) do not use setup_bundle (as they do not need to start any containers), and thus they do not set $ROOT. As a consequence, these tests now call "runc --root /state" which is not nice. Make adding --root conditional (only if $ROOT is set). Amazingly, this change breaks help.bats tests under rootless, because "sudo rootless" does not change the value of XDG_RUNTIME_DIR which still points to root-owned directory, and as a result we have this: > runc foo -h (status=1): > the path in $XDG_RUNTIME_DIR must be writable by the user > time="2022-02-08T07:04:57Z" level=error msg="mkdir /run/user/0/runc: permission denied" This could be fixed by adding proper $ROOT, but it's easier just to skip those tests under non-root. NOTE that version.bats is not broken because -v is handled by urfave/cli very early, so app.Before function is not run. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

1. Add "unset ALT_ROOT" since it should not be used after teardown is called. 2. Remove "rm -rf $ALT_ROOT". It is not needed, because ALT_ROOT is a subdirectory of ROOT, which is removed in teardown_bundle. 3. Checking for ALT_ROOT being non-empty is a leftover from the era when teardown() was called as the first step from setup(). Since commit 41670e2 this is no longer the case, so the condition is no longer needed (plus, the `set -u` which is about to be added should catch any possible use of unset ALT_ROOT). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Audit all checks for non-empty variables (i.e. ' -z ', ' -n ', ' != ""' and '= ""'), and fix those cases where a variable might be unset. Those variables (that might not be set) are - RUNC_USE_SYSTEMD - BATS_RUN_TMPDIR - AUX_UID - AUX_DIR - SD_PARENT_NAME - REL_PARENT_PATH - ROOT - HAVE_CRIU - ROOTLESS_FEATURES - and a few test-specific or file-specific variables This should allow us to enable set -u. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This is a way to prevent the code doing something really bad when a variable it uses is not set. Good to have since it helps to catch some logical errors etc. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2022-03-09T01:52:14Z

@tianon can you please take another look? I think I have addressed all of your review comments.

tianon · 2022-03-09T19:37:14Z

Mea culpa 🙇

It seems fine to me! 👍

kolyshkin · 2022-03-10T03:25:53Z

 		case $var in
 		criu)
-			if [ -n "$HAVE_CRIU" ]; then
+			if [ ! -v HAVE_CRIU ]; then


So, this fixes the bug of not running criu tests; the old check for -n "$HAVE_CRIU, introduced by commit 6e1d476, is wrong, it should have been -z "$HAVE_CRIU". This is also fixed by #3380.

kolyshkin · 2022-03-10T03:27:14Z

@thaJeztah @AkihiroSuda PTAL

kolyshkin · 2022-03-16T23:08:53Z

@thaJeztah @AkihiroSuda PTAL (this is now blessed by @tianon, although I guess he forgot the formal "approve")

kolyshkin force-pushed the tests-add-set-u branch 9 times, most recently from ab0bd70 to c004a25 Compare February 8, 2022 03:45

kolyshkin requested a review from AkihiroSuda February 8, 2022 03:45

kolyshkin marked this pull request as ready for review February 8, 2022 03:46

kolyshkin mentioned this pull request Feb 8, 2022

tests/int: nits and cleanups #3369

Merged

kolyshkin requested a review from thaJeztah February 8, 2022 03:49

kolyshkin force-pushed the tests-add-set-u branch 3 times, most recently from aaf941b to 370fab4 Compare February 9, 2022 00:34

thaJeztah reviewed Feb 9, 2022

View reviewed changes

kolyshkin force-pushed the tests-add-set-u branch from 370fab4 to 71109ae Compare February 10, 2022 22:11

kolyshkin requested a review from thaJeztah February 22, 2022 03:18

kolyshkin force-pushed the tests-add-set-u branch from 71109ae to 6aff447 Compare February 22, 2022 19:27

thaJeztah previously approved these changes Feb 22, 2022

View reviewed changes

tianon reviewed Feb 24, 2022

View reviewed changes

kolyshkin added 4 commits February 28, 2022 18:32

tests/integration/helpers: set -u

38c2169

This is a way to prevent the code doing something really bad when a variable it uses is not set. Good to have since it helps to catch some logical errors etc. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin dismissed thaJeztah’s stale review via 38c2169 March 1, 2022 02:41

kolyshkin force-pushed the tests-add-set-u branch from 6aff447 to 38c2169 Compare March 1, 2022 02:41

kolyshkin mentioned this pull request Mar 1, 2022

shell files: enable set -u -e -o pipefail. #3399

Open

3 tasks

kolyshkin requested a review from thaJeztah March 1, 2022 02:49

kolyshkin commented Mar 10, 2022

View reviewed changes

kolyshkin mentioned this pull request Mar 10, 2022

integration tests: skip criu tests incorrectly #3380

Closed

kolyshkin added the area/ci label Mar 10, 2022

AkihiroSuda approved these changes Mar 17, 2022

View reviewed changes

kolyshkin requested a review from cyphar March 17, 2022 20:45

mrunalp approved these changes Mar 18, 2022

View reviewed changes

mrunalp merged commit b9d55d5 into opencontainers:main Mar 22, 2022

	[ -v RUNC_USE_SYSTEMD ] && set_cgroups_path
	[ -n "${RUNC_USE_SYSTEMD:-}" ] && set_cgroups_path

Conversation

kolyshkin commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin commented Feb 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Feb 15, 2022

Uh oh!

kolyshkin commented Feb 22, 2022

Uh oh!

kolyshkin commented Feb 22, 2022

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kolyshkin commented Mar 9, 2022

Uh oh!

tianon commented Mar 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Mar 10, 2022

Uh oh!

kolyshkin commented Mar 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kolyshkin commented Feb 7, 2022 •

edited

Loading

kolyshkin Feb 10, 2022 •

edited

Loading