diff --git a/app/buck2_execute/src/execute/environment_inheritance.rs b/app/buck2_execute/src/execute/environment_inheritance.rs index e7df138fe8224..24ec8ab607d3a 100644 --- a/app/buck2_execute/src/execute/environment_inheritance.rs +++ b/app/buck2_execute/src/execute/environment_inheritance.rs @@ -9,6 +9,7 @@ */ use std::ffi::OsString; +use std::sync::Arc; use std::sync::OnceLock; use dupe::Dupe; @@ -66,34 +67,78 @@ const ENV_ALLOW_LIST: &[&str] = &[ "WINDIR", ]; -#[derive(Copy, Clone, Dupe, Debug)] +/// N.B. this forces [`std::env::var_os()`] to be instantiated with concrete +/// type parameters. +fn real_getenv(key: &str) -> Option { + std::env::var_os(key) +} + +/// The values captured from the built-in allowlist alone. We compute this *once* since getenv is +/// actually not cheap (being O(n) of the environment size). +fn builtin_test_allowlist_values() -> &'static Arc<[(String, OsString)]> { + static TEST_CELL: OnceLock> = OnceLock::new(); + + TEST_CELL.get_or_init(|| { + EnvironmentInheritance::test_allowlist_from(ENV_ALLOW_LIST, &[], &real_getenv).values + }) +} + +fn no_values() -> Arc<[(String, OsString)]> { + static EMPTY: OnceLock> = OnceLock::new(); + EMPTY.get_or_init(|| Vec::new().into()).dupe() +} + +#[derive(Clone, Dupe, Debug)] pub struct EnvironmentInheritance { clear: bool, - values: &'static [(&'static str, OsString)], + values: Arc<[(String, OsString)]>, exclusions: &'static [&'static str], } impl EnvironmentInheritance { pub fn test_allowlist() -> Self { - // This is made to be a list of lists in case we want to include lists from different - // provenances, like the test_env_allowlist::ENV_LIST_HACKY. - let allowlists = &[ENV_ALLOW_LIST]; - - // We create this *once* since getenv is actually not cheap (being O(n) of the environment - // size). - static TEST_CELL: OnceLock> = OnceLock::new(); - - let values = TEST_CELL.get_or_init(|| { - let mut ret = Vec::new(); - for list in allowlists.iter() { - for key in list.iter() { - if let Some(value) = std::env::var_os(key) { - ret.push((*key, value)); - } + Self::test_allowlist_with_extra(&[]) + } + + /// The built-in test allowlist, plus any extra variable names supplied by + /// the caller (in practice, the `[test] env_allowlist` buckconfig). + /// + /// Note: The environment of tests derives from the *daemon's* environment, + /// not the client's. + pub fn test_allowlist_with_extra(extra: &[String]) -> Self { + if extra.is_empty() { + return Self { + clear: true, + values: builtin_test_allowlist_values().dupe(), + exclusions: &[], + }; + } + Self::test_allowlist_from(ENV_ALLOW_LIST, extra, &real_getenv) + } + + /// Capture the values of `builtin` and `extra` according to `getenv`. + fn test_allowlist_from( + builtin: &[&str], + extra: &[String], + getenv: &dyn Fn(&str) -> Option, + ) -> Self { + let values = builtin + .iter() + .copied() + .chain(extra.iter().map(String::as_str)) + .filter_map(|key| { + // Trim and skip empties, as buck2's other comma-separated list configs do — see + // `IgnoreSet::from_ignore_spec` and `CellPackageBoundaryExceptions::new`. + // Otherwise `FOO, BAR` would silently do nothing for `BAR`. + let key = key.trim(); + if key.is_empty() { + return None; } - } - ret - }); + // A name appearing in both lists is harmless: both copies get the same value from + // `getenv`, and every consumer of `values()` is last-wins. + Some((key.to_owned(), getenv(key)?)) + }) + .collect(); Self { clear: true, @@ -107,7 +152,7 @@ impl EnvironmentInheritance { pub fn local_command_exclusions() -> Self { Self { clear: false, - values: &[], + values: no_values(), exclusions: &[ "PYTHONPATH", "PYTHONHOME", @@ -120,14 +165,14 @@ impl EnvironmentInheritance { pub fn empty() -> Self { Self { - values: &[], + values: no_values(), exclusions: &[], clear: true, } } - pub fn values(&self) -> impl Iterator + use<> { - self.values.iter().map(|(k, v)| (*k, v)) + pub fn values(&self) -> impl Iterator + use<'_> { + self.values.iter().map(|(k, v)| (k.as_str(), v)) } pub fn exclusions(&self) -> impl Iterator + use<> { @@ -138,3 +183,113 @@ impl EnvironmentInheritance { self.clear } } + +#[cfg(test)] +mod tests { + use super::*; + + const BUILTIN: &[&str] = &["SET_BUILTIN", "UNSET_BUILTIN"]; + + /// The only variables that exist as far as these tests are concerned. + fn getenv(key: &str) -> Option { + match key { + "SET_BUILTIN" => Some(OsString::from("builtin-value")), + "SET_EXTRA" => Some(OsString::from("extra-value")), + "SET_EXTRA_2" => Some(OsString::from("extra-value-2")), + _ => None, + } + } + + fn captured(extra: &[&str]) -> Vec<(String, OsString)> { + let extra: Vec = extra.iter().map(|k| (*k).to_owned()).collect(); + EnvironmentInheritance::test_allowlist_from(BUILTIN, &extra, &getenv) + .values() + .map(|(k, v)| (k.to_owned(), v.clone())) + .collect() + } + + fn pairs(expected: &[(&str, &str)]) -> Vec<(String, OsString)> { + expected + .iter() + .map(|(k, v)| ((*k).to_owned(), OsString::from(*v))) + .collect() + } + + #[test] + fn test_builtin_only_drops_unset_names() { + assert_eq!(captured(&[]), pairs(&[("SET_BUILTIN", "builtin-value")])); + } + + #[test] + fn test_extra_names_are_appended() { + assert_eq!( + captured(&["SET_EXTRA", "SET_EXTRA_2"]), + pairs(&[ + ("SET_BUILTIN", "builtin-value"), + ("SET_EXTRA", "extra-value"), + ("SET_EXTRA_2", "extra-value-2"), + ]), + ); + } + + #[test] + fn test_extra_names_are_trimmed() { + // As the names arrive from `env_allowlist = SET_EXTRA, SET_EXTRA_2`. + assert_eq!( + captured(&["SET_EXTRA", " SET_EXTRA_2"]), + captured(&["SET_EXTRA", "SET_EXTRA_2"]), + ); + } + + #[test] + fn test_extra_ignores_unset_names() { + assert_eq!(captured(&["NEVER_SET"]), captured(&[])); + } + + #[test] + fn test_extra_ignores_empty_names() { + // What `env_allowlist =` and `env_allowlist = SET_EXTRA,` parse to. + assert_eq!(captured(&["", " "]), captured(&[])); + assert_eq!(captured(&["SET_EXTRA", ""]), captured(&["SET_EXTRA"])); + } + + #[test] + fn test_repeating_a_builtin_name_keeps_the_same_value() { + // Duplicates are tolerated because every consumer of `values()` is last-wins, which is + // only safe as long as both copies carry the same value. + assert_eq!( + captured(&["SET_BUILTIN"]), + pairs(&[ + ("SET_BUILTIN", "builtin-value"), + ("SET_BUILTIN", "builtin-value"), + ]), + ); + } + + #[test] + fn test_allowlist_clears_the_environment_and_excludes_nothing() { + let inheritance = EnvironmentInheritance::test_allowlist_from( + BUILTIN, + &["SET_EXTRA".to_owned()], + &getenv, + ); + assert!(inheritance.clear()); + assert_eq!(inheritance.exclusions().count(), 0); + } + + #[test] + fn test_empty_extra_takes_the_memoized_path() { + // The `extra.is_empty()` fast path in `test_allowlist_with_extra` reads from a different + // (memoized) source than `test_allowlist_from`, so check the two agree. + let memoized: Vec<_> = EnvironmentInheritance::test_allowlist() + .values() + .map(|(k, v)| (k.to_owned(), v.clone())) + .collect(); + let direct: Vec<_> = + EnvironmentInheritance::test_allowlist_from(ENV_ALLOW_LIST, &[], &real_getenv) + .values() + .map(|(k, v)| (k.to_owned(), v.clone())) + .collect(); + assert_eq!(memoized, direct); + } +} diff --git a/app/buck2_test/src/orchestrator.rs b/app/buck2_test/src/orchestrator.rs index e962ab8610ce4..965f5f27b465b 100644 --- a/app/buck2_test/src/orchestrator.rs +++ b/app/buck2_test/src/orchestrator.rs @@ -1715,6 +1715,30 @@ impl BuckTestOrchestrator<'_> { }) } + /// Environment variables to inherit into test processes on top of the built-in allowlist, + /// specified in the root cell's `[test] env_allowlist` buckconfig. The built-in list is + /// deliberately minimal, and this is the escape hatch for passing variables which shouldn't + /// invaliate test executions (tracing context, proxy settings, and so on). + /// + /// Note that inherited variables are deliberately not part of the action digest, so changing + /// the *value* of one of these will not invalidate test executions. Changing the config itself + /// will, since reading it here records a dependency edge. + async fn extra_test_env_allowlist( + dice: &mut DiceComputations<'_>, + ) -> buck2_error::Result> { + let root_cell = dice.get_cell_resolver().await?.root_cell(); + Ok(dice + .parse_legacy_config_list_property::( + root_cell, + BuckconfigKeyRef { + section: "test", + property: "env_allowlist", + }, + ) + .await? + .unwrap_or_default()) + } + async fn create_command_execution_request( dice: &mut DiceComputations<'_>, cwd: ProjectRelativePathBuf, @@ -1765,9 +1789,12 @@ impl BuckTestOrchestrator<'_> { .get::() .unwrap() .0; + let extra_env_allowlist = Self::extra_test_env_allowlist(dice).await?; request = request .with_working_directory(cwd) - .with_local_environment_inheritance(EnvironmentInheritance::test_allowlist()) + .with_local_environment_inheritance(EnvironmentInheritance::test_allowlist_with_extra( + &extra_env_allowlist, + )) .with_disable_miniperf(!has_resource_control) .with_worker(worker) .with_remote_execution_custom_image(re_dynamic_image) diff --git a/tests/core/test/test_env_allowlist.py b/tests/core/test/test_env_allowlist.py new file mode 100644 index 0000000000000..1ef0d17af1711 --- /dev/null +++ b/tests/core/test/test_env_allowlist.py @@ -0,0 +1,101 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is dual-licensed under either the MIT license found in the +# LICENSE-MIT file in the root directory of this source tree or the Apache +# License, Version 2.0 found in the LICENSE-APACHE file in the root directory +# of this source tree. You may select, at your option, one of the +# above-listed licenses. + +# pyre-strict + +from buck2.tests.e2e_util.api.buck import Buck +from buck2.tests.e2e_util.asserts import expect_failure +from buck2.tests.e2e_util.buck_workspace import buck_test, env + +# `@env` sets these on the buck2 client, so the daemon it spawns inherits them. +# The `//:expect_*` targets then assert on what actually reaches the test +# process, which is what `[test] env_allowlist` controls. +PROBE = "BUCK2_E2E_ENV_PROBE" +PROBE_2 = "BUCK2_E2E_ENV_PROBE_2" + +# The allowlist only applies to local execution, so pin every test to it. +LOCAL_ONLY = [ + "-c", + "test.local_enabled=true", + "-c", + "test.remote_enabled=false", +] + + +@buck_test() +@env(PROBE, "probe-value") +async def test_env_not_inherited_by_default(buck: Buck) -> None: + # The var is in the daemon's environment but not in the allowlist, so the + # test process must not see it. + await buck.test(*LOCAL_ONLY, "//:expect_unset") + + +@buck_test() +@env(PROBE, "probe-value") +async def test_env_inherited_when_allowlisted(buck: Buck) -> None: + await buck.test( + *LOCAL_ONLY, + "-c", + f"test.env_allowlist={PROBE}", + "//:expect_set", + ) + + +@buck_test() +@env(PROBE, "probe-value") +@env(PROBE_2, "probe-value-2") +async def test_env_allowlist_accepts_a_list(buck: Buck) -> None: + await buck.test( + *LOCAL_ONLY, + "-c", + f"test.env_allowlist={PROBE},{PROBE_2}", + "//:expect_both_set", + ) + + +@buck_test() +@env(PROBE, "probe-value") +@env(PROBE_2, "probe-value-2") +async def test_env_allowlist_only_covers_what_it_names(buck: Buck) -> None: + # Allowlisting one var must not drag its neighbour along. + await expect_failure( + buck.test( + *LOCAL_ONLY, + "-c", + f"test.env_allowlist={PROBE}", + "//:expect_both_set", + ), + ) + + +@buck_test() +@env(PROBE, "probe-value") +async def test_env_allowlist_ignores_unset_vars(buck: Buck) -> None: + # Naming a var that isn't in the daemon's environment is not an error, and + # must not show up in the test process as an empty string. + await buck.test( + *LOCAL_ONLY, + "-c", + "test.env_allowlist=BUCK2_E2E_ENV_PROBE_NEVER_SET", + "//:expect_unset", + ) + + +@buck_test() +@env(PROBE, "probe-value") +async def test_changing_env_allowlist_reruns_the_test(buck: Buck) -> None: + # Reading the config records a dep edge on it, so dropping the var from the + # allowlist has to invalidate the successful run above rather than let the + # daemon reuse it. + await buck.test( + *LOCAL_ONLY, + "-c", + f"test.env_allowlist={PROBE}", + "//:expect_set", + ) + await expect_failure(buck.test(*LOCAL_ONLY, "//:expect_set")) diff --git a/tests/core/test/test_env_allowlist_data/.buckconfig b/tests/core/test/test_env_allowlist_data/.buckconfig new file mode 100644 index 0000000000000..65dec17e1f663 --- /dev/null +++ b/tests/core/test/test_env_allowlist_data/.buckconfig @@ -0,0 +1,15 @@ +[buildfile] +name=TARGETS.fixture + +[repositories] +root = . +nano_prelude = nano_prelude + +[cell_aliases] +prelude = nano_prelude + +[external_cells] +nano_prelude = bundled + +[build] +execution_platforms = root//platforms:platforms diff --git a/tests/core/test/test_env_allowlist_data/.buckroot b/tests/core/test/test_env_allowlist_data/.buckroot new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/test/test_env_allowlist_data/TARGETS.fixture b/tests/core/test/test_env_allowlist_data/TARGETS.fixture new file mode 100644 index 0000000000000..66e58fd591fc9 --- /dev/null +++ b/tests/core/test/test_env_allowlist_data/TARGETS.fixture @@ -0,0 +1,20 @@ +load(":rules.bzl", "env_probe_test") + +# An empty expected value means "must not be set at all". +env_probe_test( + name = "expect_unset", + expect_env = {"BUCK2_E2E_ENV_PROBE": ""}, +) + +env_probe_test( + name = "expect_set", + expect_env = {"BUCK2_E2E_ENV_PROBE": "probe-value"}, +) + +env_probe_test( + name = "expect_both_set", + expect_env = { + "BUCK2_E2E_ENV_PROBE": "probe-value", + "BUCK2_E2E_ENV_PROBE_2": "probe-value-2", + }, +) diff --git a/tests/core/test/test_env_allowlist_data/platforms/TARGETS.fixture b/tests/core/test/test_env_allowlist_data/platforms/TARGETS.fixture new file mode 100644 index 0000000000000..e7a5179f98653 --- /dev/null +++ b/tests/core/test/test_env_allowlist_data/platforms/TARGETS.fixture @@ -0,0 +1,18 @@ +local_enabled = read_config("test", "local_enabled", "true") +remote_enabled = read_config("test", "remote_enabled", "false") + +platform( + name = "platform", +) + +execution_platform( + name = "exec_platform", + platform = ":platform", + local_enabled = local_enabled == "true", + remote_enabled = remote_enabled == "true", +) + +execution_platforms( + name = "platforms", + platforms = [":exec_platform"], +) diff --git a/tests/core/test/test_env_allowlist_data/rules.bzl b/tests/core/test/test_env_allowlist_data/rules.bzl new file mode 100644 index 0000000000000..437def757ef9e --- /dev/null +++ b/tests/core/test/test_env_allowlist_data/rules.bzl @@ -0,0 +1,64 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is dual-licensed under either the MIT license found in the +# LICENSE-MIT file in the root directory of this source tree or the Apache +# License, Version 2.0 found in the LICENSE-APACHE file in the root directory +# of this source tree. You may select, at your option, one of the +# above-listed licenses. + +# A test whose command asserts that specific environment variables were +# inherited from the daemon with specific values, and fails otherwise. Used to +# observe what the local execution env allowlist actually lets through. + +_SCRIPT_PREFIX = """ +import os +import sys + +if "--list" in sys.argv: + print("test1\\n") + sys.exit(0) + +failures = [] + +def check(name, want): + got = os.environ.get(name) + if got != want: + failures.append("expected " + name + "=" + repr(want) + ", got " + repr(got)) + +""" + +_SCRIPT_SUFFIX = """ +if failures: + sys.stderr.write("\\n".join(failures) + "\\n") + sys.exit(1) +sys.exit(0) +""" + +def _env_probe_test_impl(ctx): + # An empty expected value means "must not be set at all". + checks = [ + 'check("{}", {})'.format(name, '"{}"'.format(want) if want else "None") + for name, want in ctx.attrs.expect_env.items() + ] + script = _SCRIPT_PREFIX + "\n".join(checks) + _SCRIPT_SUFFIX + + out = ctx.actions.declare_output("file", has_content_based_path = False) + ctx.actions.run( + ["touch", out.as_output()], + category = "touch", + ) + return [ + DefaultInfo(out), + ExternalRunnerTestInfo( + command = ["fbpython", "-c", script], + use_project_relative_paths = True, + type = "lionhead", + ), + ] + +env_probe_test = rule( + attrs = { + "expect_env": attrs.dict(attrs.string(), attrs.string(), default = {}), + }, + impl = _env_probe_test_impl, +)