If setup_suite() fails for some reason, causing the Docker registry to
not be created, then an unrelated message from 'podman stop' would show
up:
not ok 1 setup_suite
# (from function `setup_suite' in test file
test/system/setup_suite.bash, line 43)
# `_pull_and_cache_distro_image foo || false' failed
# Requested distro (foo) does not have a matching image
# Error: no container with name or ID "docker-registry" found: no such
container
# ...
# ...
This extra error message from 'podman stop' serves no purpose because
it's not related to the cause of the setup_suite() failure. It's just
noise and it's better to silence it.
https://github.com/containers/toolbox/pull/1375
Contrary to what the documentation might seem to imply [1], Bats' 'fail'
helper only aborts a test case under certain circumstances. eg., when
called from setup_suite(), but not from within a child function, and a
@test case, but not from within the 'run' helper.
If 'fail' is called from within 'run', then the code after it will
continue to execute. The test case will only fail if 'run' eventually
catches a non-zero exit code that's caught by 'assert_success' [2].
Similarly, it doesn't abort if called from within a child function in
setup_suite().
Currently, _pull_and_cache_distro_image() is a child function called
from setup_suite(). So 'fail' won't abort if an invalid distribution is
specified.
Fortunately, pull_distro_image() is being called from within @test
cases, but outside 'run'. So, there's no problem with it now. However,
some future code changes can unknowingly alter this reality and it too
can run into unexpected behaviour.
Therefore, it's better to be safe, and explicitly specify a non-zero
exit code after 'fail'. It will ensure that it works as expected under
all circumstances.
[1] https://github.com/bats-core/bats-support
[2] https://github.com/bats-core/bats-asserthttps://github.com/containers/toolbox/pull/1375
Currently, if a Toolbx container's entry point fails to initialize the
container, there's no way to see the debug logs and error messages from
the entry point:
not ok 106 container: Check container starts without issues
# (from function `assert_success' in file
test/system/libs/bats-assert/src/assert.bash, line 114,
# in test file test/system/103-container.bats, line 39)
# `assert_success' failed
#
# -- command failed --
# status : 1
# output :
# --
#
Instead, from now on, they will be visible:
not ok 106 container: Check container starts without issues
# (from function `assert_success' in file
test/system/libs/bats-assert/src/assert.bash, line 114,
# in test file test/system/103-container.bats, line 39)
# `assert_success' failed
#
# -- command failed --
# status : 1
# output (90 lines):
# Failed to initialize container fedora-toolbox-38
# level=debug msg="Running as real user ID 0"
# level=debug msg="Resolved absolute path to the executable as
/usr/bin/toolbox"
# level=debug msg="TOOLBOX_PATH is /opt/bin/toolbox"
# level=debug msg="Migrating to newer Podman"
# level=debug msg="Migration not needed: running inside a container"
# level=debug msg="Setting up configuration"
# ...
# --
#
https://github.com/containers/toolbox/pull/1374
Bats' 'run' helper returns with an exit code of 0 even when the command
that it was given to run failed with a non-zero exit code [1]. This is
to enable making further assertions about the command after 'run' has
finished. If there's nothing that checks for failures, then it will
continue as if everything is alright.
Therefore, currently, if 'podman logs' fails, there's no indication of
it and the test only fails later because it thinks that the container
failed to initialize:
not ok 106 container: Check container starts without issues
# (from function `assert_success' in file
test/system/libs/bats-assert/src/assert.bash, line 114,
# in test file test/system/103-container.bats, line 39)
# `assert_success' failed
#
# -- command failed --
# status : 1
# output :
# --
#
Instead, from now on, it will be more obvious:
not ok 106 container: Check container starts without issues
# (from function `assert_success' in file
test/system/libs/bats-assert/src/assert.bash, line 114,
# in test file test/system/103-container.bats, line 39)
# `assert_success' failed
#
# -- command failed --
# status : 125
# output (2 lines):
# Failed to invoke '/usr/bin/podman logs'
# Error: no container with name or ID "foo" found: no such container
# --
#
One alternative was to use 'assert_success' [2] to assert that the
command given to 'run' succeeded. That would show the 'podman logs'
failure as:
not ok 106 container: Check container starts without issues
# (from function `assert_success' in file
test/system/libs/bats-assert/src/assert.bash, line 114,
# in test file test/system/103-container.bats, line 39)
# `assert_success' failed
#
# -- command failed --
# status : 1
# output (29 lines):
#
# -- command failed --
# status : 125
# output : Error: no container with name or ID "foo" found: no such
container
# --
#
# ...
#
# -- command failed --
# status : 125
# output : Error: no container with name or ID "foo" found: no such
container
# --
# --
#
However, it's a bit too noisy because of the 'assert_success' not
terminating container_started() and continuing to loop for the remaining
attempts.
[1] https://bats-core.readthedocs.io/en/stable/writing-tests.html
[2] https://github.com/bats-core/bats-asserthttps://github.com/containers/toolbox/pull/1372
A subsequent commit will use this variable to set the return value for a
different condition. Therefore, the name needs to be changed to suit
the purpose.
https://github.com/containers/toolbox/pull/1372
Until Bats 1.10.0, 'run' with options had a bug where it would overwrite
the value of the 'i' variable even outside 'run' [1].
In these particular instances, no options are being passed to 'run',
and, hence, currently there's no problem. However, in case a future
commit adds an option, then it could lead to hard-to-debug problems.
eg., --separate-stderr sets 'i' to 1, --show-output-of-passing-tests
sets it to 2, etc.. Therefore, depending on the flag and the loop, the
loop might get terminated prematurely or run infinitely or something
else.
Moreover, Bats 1.10.0 is only available in Fedora >= 39 and is absent
from Fedoras 37 and 38. Therefore, it's not possible to consider this
bug fixed.
Hence, it's better to preemptively work around it to avoid any future
issues.
[1] Bats commit 502dc47dd063c187
https://github.com/bats-core/bats-core/commit/502dc47dd063c187https://github.com/bats-core/bats-core/issues/726https://github.com/containers/toolbox/pull/1373
Otherwise https://www.shellcheck.net/ would complain:
Line 141:
assert_line --index 0 "~/.bash_profile read"
^------------------^ SC2088 (warning): Tilde
does not expand in quotes.
Use $HOME.
See: https://www.shellcheck.net/wiki/SC2088
This is a false positive. There's no need for the tilde to be expanded
because it's not being used for any file system operation. It's merely
a human-readable string.
However, it's easier to change the string to use $HOME than littering
the file with ShellCheck's inline 'disable' directives.
https://github.com/containers/toolbox/pull/1366
These files aren't marked as executable, and shouldn't be, because they
aren't meant to be standalone executable scripts. They're meant to be
part of a test suite driven by Bats. Therefore, it doesn't make sense
for them to have shebangs, because it gives the opposite impression.
The shebangs were actually being used by external tools like Coverity to
deduce the shell when running shellcheck(1). Shellcheck's inline
'shell' directive is a more obvious way to achieve that.
https://github.com/containers/toolbox/pull/1363
The setup_suite.bash file is meant to be written in Bash, and is not
supposed to have any Bats-specific syntax. That's why it has the *.bash
suffix, not *.bats. If Bats finds a setup_suite.bash file, when running
the test suite, it uses Bash's source(1) builtin to read the file.
This is a cosmetic change. The Bats syntax is a superset of the Bash
syntax. Therefore, it didn't make a difference to external tools like
Coverity that use the shebang to deduce the shell for shellcheck(1).
Secondly setup_suite.bash isn't meant to be an executable script and,
hence, the shebang has no effect on how the file is used. However, it's
still a commonly used hint about the contents of the file, and it's
better to be accurate than misleading.
A subsequent commit will replace the shebangs in the test suite with
ShellCheck's 'shell' directives.
Fallout from 7a387dcc8bhttps://github.com/containers/toolbox/pull/1363
It's one less invocation of an external command, which is good because
spawning a new process is generally expensive.
One positive side-effect of this is that on some Active Directory
set-ups, the entry point no longer fails with:
Error: failed to remove password for user login@company.com: failed
to invoke passwd(1)
... because of:
# passwd --delete login@company.com
passwd: Libuser error at line: 210 - name contains invalid char `@'.
This is purely an accident, and isn't meant to be an intential change to
support Active Directory. Tools like useradd(8) and usermod(8) from
Shadow aren't meant to work with Active Directory users, and, hence, it
can still break in other ways. For that, one option is to expose $USER
from the host operating system to the Toolbx container through a Varlink
interface that can be used by nss-systemd inside the container.
Based on an idea from Si.
https://github.com/containers/toolbox/issues/585
Until now, configureUsers() was pushing the burden of deciding whether
to add a new user or modify an existing one on the callers, even though
it can trivially decide itself. Involving the caller loosens the
encapsulation of the user configuration logic by spreading it across
configureUsers() and it's caller, and adds an extra function parameter
that needs to be carefully set and is vulnerable to programmer errors.
Fallout from 9ea6fe5852https://github.com/containers/toolbox/pull/1356
These tests assume that the group and user information on the host
operating system can be provided by different plugins for the GNU Name
Service Switch (or NSS) functionality of the GNU C Library. eg., on
enterprise FreeIPA set-ups. However, it's expected that everything
inside the Toolbx container will be provided by /etc/group, /etc/passwd,
/etc/shadow, etc..
While /etc/group and /etc/passwd can be read by any user, /etc/shadow
can only be read by root. However, it's awkward to use sudo(8) in the
test cases involving /etc/shadow, because they ensure that root and
$USER don't need passwords to authenticate inside the container, and
sudo(8) itself depends on that. If sudo(8) is used, the test suite can
behave unexpectedly if Toolbx didn't set up the container correctly.
eg., it can get blocked waiting for a password.
Hence, 'podman unshare' is used instead to enter the container's initial
user namespace, where $USER from the host appears as root. This is
sufficient because the test cases only need to read /etc/shadow inside
the Toolbx container.
https://github.com/containers/toolbox/pull/1355
Sometimes locations such as /var/lib/flatpak, /var/lib/systemd/coredump
and /var/log/journal sit on security hardened mount points that are
marked as 'nosuid,nodev,noexec' [1]. In such cases, when Toolbx is used
rootless, an attempt to bind mount these locations read-only at runtime
with mount(8) fails because of permission problems:
# mount --rbind -o ro <source> <containerPath>
mount: <containerPath>: filesystem was mounted, but any subsequent
operation failed: Unknown error 5005.
(Note that the above error message from mount(8) was subsequently
improved to show something more meaningful than 'Unknown error' [2].)
The problem is that 'init-container' is running inside the container's
mount and user namespace, and the source paths were mounted inside the
host's namespace with 'nosuid,nodev,noexec'. The above mount(8) call
tries to remove the 'nosuid,nodev,noexec' flags from the mount point and
replace them with only 'ro', which is something that can't be done from
a child namespace.
Note that this doesn't fail when Toolbx is running as root. This is
because the container uses the host's user namespace and is able to
remove the 'nosuid,nodev,noexec' flags from the mount point and replace
them with only 'ro'. Even though it doesn't fail, the flags shouldn't
get replaced like that inside the container, because it removes the
security hardening of those mount points.
There's actually no benefit in bind mounting these paths as read-only.
It was historically done this way 'just to be safe' because a user isn't
expected to write to these locations from inside a container. However,
Toolbx doesn't intend to provide any heightened security beyond what's
already available on the host.
Hence, it's better to get out of the way and leave it to the permissions
on the source location from the host operating system to guard the
castle. This is accomplished by not passing any file system options to
mount(8) [1].
Based on an idea from Si.
[1] https://man7.org/linux/man-pages/man8/mount.8.html
[2] util-linux commit 9420ca34dc8b6f0f
https://github.com/util-linux/util-linux/commit/9420ca34dc8b6f0fhttps://github.com/util-linux/util-linux/pull/2376https://github.com/containers/toolbox/issues/911
The '[' and 'test' implementations from GNU coreutils don't support '-v'
as a way to check if a shell variable is set [1]. Only Bash's built-in
implementations do.
This is quite confusing and makes it difficult to find out what '-v'
actually does. eg., 'man --all test' only shows the manual for the GNU
coreutils version, which doesn't list '-v' [1], and, 'man --all [' only
shows the manual for Bash's built-ins, which also doesn't list '-v'.
One has to go to the bash(1) manual to find it [2].
Elsewhere in the code base [3], the same thing is accomplished with '-z'
and parameter substitution, which are more widely supported and, hence,
easier to find documentation for.
[1] https://manpages.debian.org/testing/coreutils/test.1.en.html
[2] https://linux.die.net/man/1/bash
[3] Commit 84ae385f33https://github.com/containers/toolbox/pull/1334https://github.com/containers/toolbox/pull/1341
'[' is a command that's the same as 'test' and they might be implemented
as standalone executables or shell built-ins. Therefore, the negation
(ie., '!') has to cover the entire command to operate on its exit code.
Instead, if it's writtten as '[ ! ... ]', then the negation becomes an
argument to '[', which isn't the same thing.
Fallout from 54a2ca1eadhttps://github.com/containers/toolbox/pull/1341
First, it's not a good idea to use awk(1) as a grep(1) replacement.
Unless one really needs the AWK programming language, it's better to
stick to grep(1) because it's simpler.
Secondly, it's better to look for a specific os-release(5) field instead
of looking for the occurrence of 'rawhide' anywhere in the file, because
it lowers the possibility of false positives.
https://github.com/containers/toolbox/pull/1336
The Zuul executor contains Ansible 2.13.7 whose 'dnf' module is not
working as it should with Fedora Rawhide because of the DNF5 Change [1].
Unlike DNF4, DNF5 no longer pulls in the python3-dnf RPM, which causes:
TASK [Install RPM packages]
fedora-rawhide | ERROR
fedora-rawhide | {
fedora-rawhide | "msg": "Could not import the dnf python module
using /usr/bin/python3 (3.12.0b3 (main, Jun 21 2023, 00:00:00)
[GCC 13.1.1 20230614 (Red Hat 13.1.1-4)]). Please install
`python3-dnf` or `python2-dnf` package or ensure you have
specified the correct ansible_python_interpreter. (attempted
['/usr/libexec/platform-python', '/usr/bin/python3',
'/usr/bin/python2', '/usr/bin/python'])",
fedora-rawhide | "results": []
fedora-rawhide | }
This adds a workaround that explicitly installs the python3-dnf RPM
using Ansible's 'command' module. It should be removed after Zuul
contains a newer release of Ansible.
[1] https://fedoraproject.org/wiki/Changes/ReplaceDnfWithDnf5https://github.com/containers/toolbox/pull/1338
Signed-off-by: Daniel Pawlik <dpawlik@redhat.com>
First, it's not a good idea to use awk(1) as a grep(1) replacement.
Unless one really needs the AWK programming language, it's better to
stick to grep(1) because it's simpler.
Secondly, it's better to look for a specific os-release(5) field instead
of looking for the occurrence of 'rawhide' anywhere in the file, because
it lowers the possibility of false positives.
https://github.com/containers/toolbox/pull/1332
The following caveats must be noted:
* Podman sets the Toolbx container's soft limit for the maximum number
of open file descriptors to the host's hard limit, which is often
greater than the host's soft limit [1].
* The ulimit(1) options -P, -T, -b, and -k don't work on Fedora 38
because the corresponding resource arguments for getrlimit(2) are
absent from the operating system. These are RLIMIT_NPTS,
RLIMIT_PTHREAD, RLIMIT_SBSIZE and RLIMIT_KQUEUES respectively.
[1] https://github.com/containers/podman/issues/17681https://github.com/containers/toolbox/issues/213
The current approach of extracting the VERSION_ID field from
os-release(5) assumes that the value is not quoted. There's no
guarantee that this will be the case. It only happens to be so on
Fedora by chance, and is different on Ubuntu:
$ cat /etc/os-release
...
VERSION_ID="22.04"
...
This means that "22.04", including the double quotes, is read as the
value of VERSION_ID on Ubuntu, not 22.04. This is wrong because this
value can't be used as is in image and container names. There's no
image called quay.io/toolbx/ubuntu-toolbox:"22.04" and double quotes are
not allowed in container names.
Instead, use the same approach as profile.d/toolbox.sh and the old POSIX
shell implementation that doesn't rely on the quoting of the
os-release(5) values.
Fallout from b27795a03ehttps://github.com/containers/toolbox/pull/1320
The current approach of selecting all the os-release(5) fields that have
'ID' in their name (eg., ID, VERSION_ID, PLATFORM_ID, VARIANT_ID, etc.)
and then picking the first one, assumes that the ID field will always be
placed above the others in os-release(5). There's no guarantee that
this will be the case. It only happens to be so on Fedora by chance,
and is different on Ubuntu:
$ cat /etc/os-release
...
VERSION_ID="22.04"
...
ID=ubuntu
ID_LIKE=debian
...
This means that "22.04" is read as the value of ID on Ubuntu, which is
clearly wrong.
Instead, use the same approach as profile.d/toolbox.sh and the old POSIX
shell implementation that doesn't rely on the order of the os-release(5)
fields.
Fallout from 54a2ca1eadhttps://github.com/containers/toolbox/pull/1320
