buck2_test: add an allowlist for env-vars in buckconfig

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<OsString> {
+    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<Arc<[(String, OsString)]>> = 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<Arc<[(String, OsString)]>> = 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<Vec<(&'static str, OsString)>> = 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<OsString>,
+    ) -> 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<Item = (&'static str, &'static OsString)> + use<> {
-        self.values.iter().map(|(k, v)| (*k, v))
+    pub fn values(&self) -> impl Iterator<Item = (&str, &OsString)> + use<'_> {
+        self.values.iter().map(|(k, v)| (k.as_str(), v))
     }
 
     pub fn exclusions(&self) -> impl Iterator<Item = &'static str> + 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<OsString> {
+        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<String> = 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);
+    }
+}

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<Vec<String>> {
+        let root_cell = dice.get_cell_resolver().await?.root_cell();
+        Ok(dice
+            .parse_legacy_config_list_property::<String>(
+                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::<HasResourceControl>()
             .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)

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"))