-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add FreeBSD Jail execution environment support #224
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
99692a8
to
0a43bb8
Compare
The following topics are closed by current state of the patch:
|
a33e429
to
08699b3
Compare
08699b3
to
9721be7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As much as I'm not a fan of the term "container" because it focuses on the Linux/Solaris technological implementations, I believe that it would be wise to use "container" in lieu of "jail". "jail"s are a concept that are very FreeBSD specific and don't exist outside of FreeBSD/FreeBSD derivatives. I realize this project is hosted in the freebsd GitHub org, but it's a good idea to keep things as generic as possible to avoid having to rearchitect things later if someone contributes an actual Linux or Solaris container implementation (for instance).
Cross-references to jail(8), et al, are missing from the manpage edits (this seems to be where most of the jail kernel subsystem documentation lives). This is important because you're adding a feature which isn't fully documented and not providing enough information for more casual FreeBSD users to use the feature.
.It Va execenv | ||
Whitespace-separated list of execution environment names. | ||
.Pp | ||
Only tests which require one of the given execution environments will be run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioning what execenv's are supported would be a good idea -- otherwise it's unclear (without looking at the source code) what is or isn't supported.
Referencing alternative documentation is a good thing to do in place of hardcoding the values, but I think being explicit avoids the need for the documentation potentially becoming out of sync/less implicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already covered in kyuafile.5
: https://github.com/ihoro/kyua/blob/9721be7abbd48f2ee020e5deaee1e1daa44c0087/doc/kyuafile.5.in#L181
I guess we could simply make a reference to the kyuafile.5
for the possible values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the mentioned reference.
doc/kyuafile.5.in
Outdated
@@ -1,4 +1,4 @@ | |||
.\" Copyright 2012 The Kyua Authors. | |||
.\" Copyright 2024 The Kyua Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.\" Copyright 2024 The Kyua Authors. | |
.\" Copyright 2015-2024 The Kyua Authors. |
Copyright's should always represent the full period that they cover.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
doc/kyuafile.5.in
Outdated
It makes sense only if execenv is set to | ||
.Sq jail . | ||
.sp | ||
Kyua implicitly adds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "adds" the appropriate concept to use here? I will look at the surrounding documentation to compare/contrast.
Also: kyua(1) should be used here:
Kyua implicitly adds | |
.Xr kyua 1 | |
implicitly adds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Plus a little rephrasing with s/adds/passes/.
doc/kyuafile.5.in
Outdated
.It jail | ||
The | ||
.Fx | ||
jail environment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whenever mentioning jail, I would probably reference other documentation.
I'm reaching out to other developers to get input, as it's not clear if .Xr jail 2
or .Xr jail 3
should be used here (I'm leaning towards the former).
Again, this should only be discussed in the actual implementation -- the general documentation should use the term, "containers".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.Xr jail 8
feels like the best one due to kyua users are expected to be usual users of jail(8)
, instead of the system calls and the jail library. And the execenv.jail
parameters are actually parameters for jail(8)
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put .Xr jail 8
for now.
doc/kyuafile.5.in
Outdated
It creates a temporary jail to run the test and its optional cleanup logic | ||
within. | ||
.El | ||
.It Va execenv_jail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of parameters are you envisioning adding for host vs container style execenv's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, there are no extra things for the existing default env, it works the same way without change. A new metadata property like execenv.host.*
could be added in future if there is a production need, probably to pass some env variables (just thinking out loud).
model/metadata.hpp
Outdated
const std::string& execenv(void) const; | ||
bool has_execenv(void) const; | ||
const std::string& execenv_jail(void) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be put directly below description
to be sorted alphanumerically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
const int /* value_index */) | ||
lutok::state& state, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems superfluous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? It adds a capability to read a string set config values which are needed for the execenv
engine config variable. It looks the function was left as a stub for future implementation when it's really needed. The patch adds string set support.
utils/process/executor.cpp
Outdated
@@ -689,6 +689,34 @@ struct utils::process::executor::executor_handle::impl : utils::noncopyable { | |||
data._pimpl->state_owners, | |||
all_exec_handles))); | |||
} | |||
|
|||
executor::exit_handle | |||
reap(const int original_pid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use pid_t
, not int
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
executor::exit_handle | ||
reap(const int original_pid) | ||
{ | ||
const exec_handles_map::iterator iter = all_exec_handles.find( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error handling is missing for values like 0
, -1
, etc. These values have special meanings and can result in the process either committing suicide OR a system-wide reboot/shutdown/etc. Neither case is desired.
From kill(2):
If pid is zero:
The sig signal is sent to all processes whose group ID is equal
to the process group ID of the sender, and for which the process
has permission; this is a variant of killpg(2).
If pid is -1:
If the user has super-user privileges, the signal is sent to all
processes excluding system processes (with P_SYSTEM flag set),
process with ID 1 (usually init(8)), and the process sending the
signal. If the user is not the super user, the signal is sent to
all processes which the caller has permissions to, excluding the
process sending the signal. No error is returned if any process
could be signaled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks the function name is misleading, it's not the same reaping as in the general process management context. During my work on this patch back in Oct, I found a defect in the existing Kyua mechanism of abrupt termination handling -- it was not collecting all exit handles of its children. It was easier to reproduce with parallelism>1, and the parallelism is what I was actively working on. The pid passed here as a parameter is a pid of a spawn child previously saved by kyua, the kyua simply expects to see an exit handle object per each child spawn and this "kyua specifics" reaping process helps with that. That is, it's named after the same term of the general context, but this is about kyua internals.
freebsd/utils/jail.cpp
Outdated
// invoke jail | ||
std::auto_ptr< process::child > child = child::fork_capture( | ||
run(fs::path("/usr/sbin/jail"), av)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like sanity checking that the jail(8) program exists and skipping with a helpful message would be wise--instead of just marking all of the tests broken.
This is especially important for systems where WITHOUT_JAIL=
is specified in src.conf(5).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was working on a concern from Olivier: https://lists.freebsd.org/archives/freebsd-hackers/2024-February/003016.html. Currently it's covered such way that for the WITHOUT_JAIL
system kyua is built without execenv=jail
support. It makes kyua config
to report execenv
list with a single host
item. As a result it triggers the existing mechanism of requirements checking and marks execenv=jail
based tests as skipped with execenv="jail" requires FreeBSD with jail feature.
message. It looks to be simpler, on the higher level, without additional checks of the system consistency in cases like WITH_JAIL system + missing binary, or WITHOUT_JAIL system + kyua binary copied from WITH_JAIL system, etc. Also, the same mechanism and message is used in case of execenv=jail
tests running on Linux and other systems. Any not-covered inconsistency will be reported in a test log with messages like /usr/sbin/jail: file not found
and so on.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the code warrants adding or modifying isolation_test.cpp
, et al.
In general, it seems like the PR is proposing a number of changes without providing sufficient numbers of negative tests and integration tests. This is concerning from a code maintenance/support perspective.
Also, the jail feature creates an exploitable vector for security issues which doesn't seem to have an adequate disclaimer regarding its use. Although it's an implied bad practice to run tests from a potentially untrusted sources as root on production systems, this concern isn't called out in the documentation. In particular, this feature requires root to function, even if required.user='unprivileged'
is implemented such that jails are run as unprivileged users. jail(8) on FreeBSD always requires superuser privileges, whereas other container implementations, such as docker, allow specific groups to run administrative commands.
Another warning should be added noting that multiple parallel kyua processes can conflict with one another. This is true if is_exclusive = true
is not set, but also if multiple kyua instances are run in parallel, despite other guarantees (sqlite3 db locking) which prevents multiple processes for conflicting with one another.
It would be really good to call this out (as a warning) too.
doc/kyuafile.5.in
Outdated
atf_test_program{name='network_test', | ||
execenv='jail', | ||
execenv_jail='vnet allow.raw_sockets', | ||
required_user='root'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(picking a line)
The code below doesn't explicitly check to make sure the user has permissions to create jails.
atf_test_program
allows a config file to incorrectly specify execenv='jail'
and required_user='unprivileged_user'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this and it looks to be simpler to keep the mechanism without additional built-in policy. Who knows, probably in future we will have a capability to create some sort of jails by non-superuser.
For now I propose to improve the documentation (kyuafile.5
). I've added the key message that root is needed and the examples section was extended with extra explanation. The non-working jail+unprivileged
combination is mentioned explicitly. What do you think?
Anyway, explicit documentation is a good change here.
@ngie-eign, thank you very much for your time. Going to work on the outcome of the review. |
Sounds interesting to consider. I have concerns that two levels of abstraction could add extra misunderstanding from end user perspective and extra interface limitations for the possible future execution environments added. The There was no intention to provide an automatic containerization feature which uses the local capabilities like jail for FreeBSD or cgroups et al. for Linux. It simply provides a new concept, execution environment, and implements the first non-default (aka host) environment -- FreeBSD jail based one. It's merely an outcome that it can be used to add extra isolation of some FreeBSD tests to improve parallelism of the test suite. A test author is expected to understand the background if jail execution environment is picked for a test. This is a short elaboration of the current vision. What do you think? Probably I have not caught accurately the idea with |
Anyway, now I think that |
10330c3
to
6553ba0
Compare
@ngie-eign, I'm leaving the "Resolve conversation" button for you. I guess it must be convenient for you. |
2b9f217
to
4fe2a93
Compare
@ngie-eign, I think it's ready for the next round of your review. |
3d51835
to
3ef1a3a
Compare
The latest push introduces the following changes:
|
A new Kyua concept is added -- "execution environment". A test can be configured to be run within a specific environment. The test case lifecycle is extended respectively: - execenv init (creates a jail or does nothing for default execenv="host") - test exec - cleanup exec (optional) - execenv cleanup (removes a jail or does nothing for default execenv="host") The following new functionality is provided, from bottom to top: 1 ATF based tests - The new "execenv" metadata property can be set to explicitly ask for an execution environment: "host" or "jail". If it's not defined, as all existing tests do, then it implicitly means "host". - The new "execenv.jail.params" metadata property can be optionally defined to ask Kyua to use specific jail(8) parameters during creation of a temporary jail. An example is "vnet allow.raw_sockets". Kyua implicitly adds "children.max" to "execenv_jail_params" parameters with the maximum possible value. A test case can override it. 2 Kyuafile - The same new metadata properties can be defined on Kyuafile level: "execenv" and "execenv_jail_params". - Note that historically ATF uses dotted style of metadata naming, while Kyua uses underscore style. Hence "execenv.jail.params" vs. "execenv_jail_params". 3 kyua.conf, kyua CLI - The new "execenvs" engine configuration variable can be set to a list of execution environments to run only tests designed for. Tests of not listed environments are skipped. - By default, this variable lists all execution environments supported by a Kyua binary, e.g. execenvs="host jail". - This variable can be changed via "kyua.conf" or via kyua CLI's "-v" parameter. For example, "kyua -v execenvs=host test" will run only host-based tests and skip jail-based ones. - Current value of this variable can be examined with "kyua config". Signed-off-by: Igor Ostapenko <pm@igoro.pro>
3ef1a3a
to
d8212aa
Compare
I've moved |
I agree with Igor's view about the container/jail naming. There are lots of interface differences between jails, cgroups, zones, etc.., and it's premature to generalize from a single backend implementation. If in the future someone shows up with an equivalent backend for a different OS, it may have constraints which mean that a generic "container" engine will need backwards-incompatible changes to support both jails and the new container type. Meanwhile, this feature provides major benefits to FreeBSD src developers today - with very little effort we can use this to speed up network tests substantially. As a test, I integrated this PR into FreeBSD and tried running the pf tests with and without @ngie-eign is there anything I can do to help speed up review here? I spent a fair bit of time looking at early iterations of the patch and would quite like to see this feature progress into FreeBSD. Should we perhaps import it directly into FreeBSD and refine it there, then reconcile the two copies of kyua later? |
@ihoro I believe there is some kind of memory leak in the patch. So far I narrowed it down to something in the FreeBSD sbin tests. If I run them with |
The main work and review was conducted here: https://reviews.freebsd.org/D42350.
The respective mailing list discussion: https://lists.freebsd.org/archives/freebsd-hackers/2024-February/003032.html.