Skip to content

Commit

Permalink
[core] Add explicit, testing-only fs root support.
Browse files Browse the repository at this point in the history
This change reverts the behvaior change of the `FS_ROOT` environment
variable in #2729 and introduces a new, testing only environment
variable to manipulate the filesystem root when being used in
integration and functional testing.

In the Habitat codebase we try to discourage reading environment
variables inside library code which could affect a program's behavior at
runtime. Instead, we highly encourage reading and dealing with
environment variables during the CLI argument parsing phase of a program
and using explicit Rust types when passing data into inner libraries.
Note that there can still be default-generating functions which could
read from an environment variable, but the calling sites for these
functions should still be at a program CLI parsing phase.

In order to better support running the Supervisor on Windows in a Studio
environment, we relax this rule due to the behavior of Windows path and
PowerShell filesystem mounts. We hope that this is temporary and will be
resolved once we add better Studio isolation on Windows. The non-Windows
platforms however remain unchanged.

There is a very limited use case for an `FS_ROOT` environment variable
which only the `hab` CLI uses for certain subcommands including `hab pkg
install` and `hab pkg binlink`. This is present to support use cases
where a Habitat user may need to install packages on a mounted volume
that will become the bootable, root filesystem at a later point in time
(think operating system installers). The Supervisor and most other `hab`
CLI subcommands don't honor this environment variable because the
internal linking in most Habitat packages will not support running
packages out of another directory (linked libraries in Habitat are
linked via absolute pathing to maximize portability and correct behavior).

Recently, some excellent work was done to improve testing of the
Supervisor as a black box program. Some of the tests only require
minimal packages with no executing code and could therefore be run out
of temporary directories, and so these tests used the `FS_ROOT`
environment variable to manipulate the root of all Habitat paths.

This change preserves the strategy of these tests but rather introduces
a test-only environment variable which the test suite can set, knowing
what the consequences will be (see above). Additionally, if this
environment variable is used, a message will be printed to the standard
error stream notifying any user that this is intended only for testing
and not production. Note that by default, the test suites have their
stdout/stderr streams redirected and so this warning wouldn't appear
unless the `--nocapture` flag is added. This is one instance where the
rule above ("don't honor environment varibles in inner library code")
isn't an unbreakable law, it just needs to be carefully considered.

Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
  • Loading branch information
fnichol committed Aug 29, 2017
1 parent 86a5db5 commit 90503bd
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 25 deletions.
69 changes: 52 additions & 17 deletions components/core/src/fs.rs
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

use std::env;
use std::io::{self, Write};
use std::path::{Path, PathBuf};

use users;
Expand Down Expand Up @@ -47,23 +48,7 @@ lazy_static! {
///
/// WARNING: On Windows this variable mutates on first call if an environment variable with
/// the key of `FS_ROOT_ENVVAR` is set.
pub static ref FS_ROOT_PATH: PathBuf = {
// JW TODO: When Windows container studios are available the platform reflection should
// be removed.
if cfg!(target_os = "windows") {
match (henv::var(FS_ROOT_ENVVAR), henv::var(SYSTEMDRIVE_ENVVAR)) {
(Ok(path), _) => PathBuf::from(path),
(Err(_), Ok(system_drive)) => PathBuf::from(format!("{}{}", system_drive, "/")),
(Err(_), Err(_)) => unreachable!("Windows should always have a SYSTEMDRIVE \
environment variable.")
}
} else {
match henv::var(FS_ROOT_ENVVAR) {
Ok(path) => PathBuf::from(path),
Err(_) => PathBuf::from("/")
}
}
};
pub static ref FS_ROOT_PATH: PathBuf = fs_root_path();

static ref EUID: u32 = users::get_effective_uid();

Expand Down Expand Up @@ -341,6 +326,56 @@ pub fn am_i_root() -> bool {
*EUID == 0u32
}

/// Returns a `PathBuf` which represents the filesystem root for Habitat.
///
/// **Note** with the current exception of behavior on Windows (see below), an absolute default
/// path of `"/"` should always be returned. This function is used to populate a one-time static
/// value which cannot be altered for the execution length of a program. Packages in Habitat may
/// contain binaries and libraries having dependent libraries which are located in absolute paths
/// meaning that changing the value from this function will render existing packages un-runnable in
/// the Supervisor. Furthermore as a rule in this codebase, external environment variables should
/// *not* influence the behavior of inner libraries--any environment variables should be detected
/// in a program at CLI parsing time and explicitly passed to inner module functions.
///
/// There is one exception to this rule which is supported for testing only--primarily exercising
/// the Supervisor behavior. It allows setting a testing-only environment variable to influence the
/// file system root for the duration of a running program. Note that when using such an
/// environment varible, any existing/actual Habitat packages may not run correctly due to the
/// presence of absolute paths in package binaries and libraries. The environment variable will not
/// be referenced, exported, or consumed anywhere else in the system to ensure that it is *only*
/// used internally in test suites.
///
/// Please contact a project maintainer or current owner with any questions. Thanks!
fn fs_root_path() -> PathBuf {
// This behavior must never be expected, used, or counted on in production. This is explicitly
// unsupported.
if let Ok(path) = henv::var("TESTING_FS_ROOT") {
writeln!(
io::stderr(),
"DEBUG: setting custom filesystem root for testing only (TESTING_FS_ROOT='{}')",
&path
).expect("Could not write to stderr");
return PathBuf::from(path);
}

// JW TODO: When Windows container studios are available the platform reflection should
// be removed.
if cfg!(target_os = "windows") {
match (henv::var(FS_ROOT_ENVVAR), henv::var(SYSTEMDRIVE_ENVVAR)) {
(Ok(path), _) => PathBuf::from(path),
(Err(_), Ok(system_drive)) => PathBuf::from(format!("{}{}", system_drive, "/")),
(Err(_), Err(_)) => {
unreachable!(
"Windows should always have a SYSTEMDRIVE \
environment variable."
)
}
}
} else {
PathBuf::from("/")
}
}

#[cfg(test)]
mod test_find_command {

Expand Down
6 changes: 4 additions & 2 deletions components/sup/tests/compilation.rs
Expand Up @@ -17,13 +17,15 @@
/// Integration tests for exercising the hook and config recompilation
/// behavior of the supervisor

mod utils;

extern crate habitat_core as hcore;
extern crate habitat_sup as sup;
#[macro_use]
extern crate lazy_static;
extern crate rand;
extern crate tempdir;

mod utils;

// The fixture location is derived from the name of this test
// suite. By convention, it is the same as the file name.
lazy_static! {
Expand Down
15 changes: 9 additions & 6 deletions components/sup/tests/utils/test_sup.rs
Expand Up @@ -24,10 +24,11 @@ use std::sync::Mutex;
use std::thread;
use std::time::Duration;

use super::test_butterfly;
use hcore::url::DEPOT_URL_ENVVAR;
use rand;
use rand::distributions::{IndependentSample, Range};

extern crate rand;
use self::rand::distributions::{IndependentSample, Range};
use super::test_butterfly;

lazy_static! {
/// Keep track of all TCP ports currently being used by TestSup
Expand Down Expand Up @@ -217,9 +218,11 @@ impl TestSup {
let pkg_name = pkg_name.to_string();
let service_group = service_group.to_string();

cmd.env("FS_ROOT", fs_root.as_ref().to_string_lossy().as_ref())
.env("HAB_SUP_BINARY", &sup_exe)
.env("HAB_DEPOT_URL", "http://hab.sup.test/v1/depot")
cmd.env(
"TESTING_FS_ROOT",
fs_root.as_ref().to_string_lossy().as_ref(),
).env("HAB_SUP_BINARY", &sup_exe)
.env(DEPOT_URL_ENVVAR, "http://hab.sup.test/v1/depot")
.arg("start")
.arg("--listen-gossip")
.arg(format!("{}:{}", listen_host, butterfly_port))
Expand Down

0 comments on commit 90503bd

Please sign in to comment.