From d4cbacd0b5d6a0ef081c6aa6d09ee3d6f422710b Mon Sep 17 00:00:00 2001 From: Tom Dyas Date: Fri, 3 Jan 2025 10:58:40 +0000 Subject: [PATCH 01/10] fragments already have newlines so use write! --- src/rust/engine/process_execution/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index 7e74770b807..51da2afae06 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -1115,15 +1115,15 @@ fn maybe_make_wrapper_script( writeln!(&mut script, "#!{shebang}").map_err(|err| format!("write! failed: {err:?}"))?; if let Some(sandbox_root_fragment) = sandbox_root_fragment { - writeln!(&mut script, "{sandbox_root_fragment}") + write!(&mut script, "{sandbox_root_fragment}") .map_err(|err| format!("write! failed: {err:?}"))?; } if let Some(caches_fragment) = caches_fragment { - writeln!(&mut script, "{caches_fragment}") + write!(&mut script, "{caches_fragment}") .map_err(|err| format!("write! failed: {err:?}"))?; } if let Some(chdir_fragment) = chdir_fragment { - writeln!(&mut script, "{chdir_fragment}") + write!(&mut script, "{chdir_fragment}") .map_err(|err| format!("write! failed: {err:?}"))?; } From f2d200570489cc0fc3aeb8abf278ec10bfacbce3 Mon Sep 17 00:00:00 2001 From: Tom Dyas Date: Fri, 3 Jan 2025 11:18:30 +0000 Subject: [PATCH 02/10] substitute any env vars with the `{chroot}` marker --- src/rust/engine/process_execution/src/lib.rs | 41 +++++++++++++++++--- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index 51da2afae06..46b87b6d892 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -1029,6 +1029,7 @@ fn maybe_make_wrapper_script( append_only_caches_base_path: Option<&str>, working_directory: Option<&str>, sandbox_root_token: Option<&str>, + env_vars_to_substitute: &[&str], ) -> Result, String> { let caches_fragment = match append_only_caches_base_path { Some(base_path) if !caches.is_empty() => { @@ -1088,7 +1089,7 @@ fn maybe_make_wrapper_script( // Generate code to replace `{chroot}` markers if any are present in the command. let sandbox_root_fragment = if let Some(token) = sandbox_root_token { - let fragment = format!( + let mut fragment = format!( concat!( "sandbox_root=\"$(/bin/pwd)\"\n", "args=(\"${{@//{0}/$sandbox_root}}\")\n", @@ -1096,6 +1097,17 @@ fn maybe_make_wrapper_script( ), token ); + if !env_vars_to_substitute.is_empty() { + let env_vars_str = env_vars_to_substitute.join(" "); // TODO: shlex the strings + writeln!(&mut fragment, "for env_var in {env_vars_str}; do") + .map_err(|err| format!("write! failed: {err}"))?; + writeln!( + &mut fragment, + " eval \"export $env_var=\\${{$env_var//{token}/$sandbox_root}}\"" + ) + .map_err(|err| format!("write! failed: {err}"))?; + writeln!(&mut fragment, "done").map_err(|err| format!("write! failed: {err}"))?; + } Some(fragment) } else { None @@ -1148,16 +1160,35 @@ pub async fn make_execute_request( // Implement append-only caches by running a wrapper script before the actual program // to be invoked in the remote environment. let wrapper_script_content_opt = { - let sandbox_root_token = req - .argv + let args_have_chroot_marker = req.argv.iter().any(|arg| arg.contains("{chroot}")); + + let env_vars_with_chroot_marker = req + .env + .iter() + .filter_map(|(key, value)| { + if value.contains("{chroot}") { + Some(key.to_owned()) + } else { + None + } + }) + .collect::>(); + + let env_var_with_chroot_marker_refs = env_vars_with_chroot_marker .iter() - .any(|arg| arg.contains("{chroot}")) - .then_some(SANDBOX_ROOT_TOKEN); + .map(|s| s.as_str()) + .collect::>(); + + let sandbox_root_token = (args_have_chroot_marker + || !env_vars_with_chroot_marker.is_empty()) + .then_some(SANDBOX_ROOT_TOKEN); + maybe_make_wrapper_script( &req.append_only_caches, append_only_caches_base_path, req.working_directory.as_ref().and_then(|p| p.to_str()), sandbox_root_token, + env_var_with_chroot_marker_refs.as_slice(), )? }; let wrapper_script_digest_opt = if let Some(script) = wrapper_script_content_opt { From 0efc3abae8bbf9461a3309d229a89c37381a688a Mon Sep 17 00:00:00 2001 From: Tom Dyas Date: Fri, 3 Jan 2025 11:19:28 +0000 Subject: [PATCH 03/10] fix tests --- src/rust/engine/process_execution/src/tests.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/rust/engine/process_execution/src/tests.rs b/src/rust/engine/process_execution/src/tests.rs index 6cbfba66d4c..61c5f49452b 100644 --- a/src/rust/engine/process_execution/src/tests.rs +++ b/src/rust/engine/process_execution/src/tests.rs @@ -182,6 +182,7 @@ async fn wrapper_script_supports_append_only_caches() { dummy_caches_base_path.path().to_str(), Some(SUBDIR_NAME), None, + &[], ) .unwrap() .unwrap(); @@ -242,7 +243,7 @@ async fn wrapper_script_supports_append_only_caches() { async fn wrapper_script_supports_sandbox_root_replacements_in_args() { let caches = BTreeMap::new(); - let script_content = maybe_make_wrapper_script(&caches, None, None, Some("__ROOT__")) + let script_content = maybe_make_wrapper_script(&caches, None, None, Some("__ROOT__"), &[]) .unwrap() .unwrap(); From 6b6fc432f8d8d3a1edb34bdef51c6974c0508a24 Mon Sep 17 00:00:00 2001 From: Tom Dyas Date: Fri, 3 Jan 2025 11:25:57 +0000 Subject: [PATCH 04/10] test for env var substitution --- .../engine/process_execution/src/tests.rs | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/rust/engine/process_execution/src/tests.rs b/src/rust/engine/process_execution/src/tests.rs index 61c5f49452b..2c294a5a697 100644 --- a/src/rust/engine/process_execution/src/tests.rs +++ b/src/rust/engine/process_execution/src/tests.rs @@ -282,3 +282,50 @@ async fn wrapper_script_supports_sandbox_root_replacements_in_args() { .unwrap(); assert_eq!(content, "xyzzy\n"); } + +#[tokio::test] +async fn wrapper_script_supports_sandbox_root_replacements_in_environmenbt() { + let caches = BTreeMap::new(); + + let script_content = + maybe_make_wrapper_script(&caches, None, None, Some("__ROOT__"), &["TEST_FILE_PATH"]) + .unwrap() + .unwrap(); + + let dummy_sandbox_path = TempDir::new().unwrap(); + let script_path = dummy_sandbox_path.path().join("wrapper"); + tokio::fs::write(&script_path, script_content.as_bytes()) + .await + .unwrap(); + tokio::fs::set_permissions(&script_path, Permissions::from_mode(0o755)) + .await + .unwrap(); + + let mut cmd = tokio::process::Command::new("./wrapper"); + cmd.args(&[ + "/bin/sh", + "-c", + "echo xyzzy > $TEST_FILE_PATH && echo $TEST_FILE_PATH", + ]); + cmd.env("TEST_FILE_PATH", "__ROOT__/foo.txt"); + cmd.current_dir(dummy_sandbox_path.path()); + cmd.stdin(Stdio::null()); + cmd.stdout(Stdio::piped()); + cmd.stderr(Stdio::piped()); + + let child = cmd.spawn().unwrap(); + let output = child.wait_with_output().await.unwrap(); + if output.status.code() != Some(0) { + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + println!("stdout:{}\n\nstderr: {}", &stdout, &stderr); + panic!("Wrapper script failed to run: {}", output.status); + } + + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(!stdout.contains("__ROOT__")); + let content = tokio::fs::read_to_string(Path::new(stdout.trim())) + .await + .unwrap(); + assert_eq!(content, "xyzzy\n"); +} From 525d03652fadabcbce6f5c86525707f2e332902a Mon Sep 17 00:00:00 2001 From: Tom Dyas Date: Fri, 3 Jan 2025 11:27:43 +0000 Subject: [PATCH 05/10] substitute `{chroot}` to the sandbox root token --- src/rust/engine/process_execution/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index 46b87b6d892..8d796aeee38 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -1238,7 +1238,7 @@ pub async fn make_execute_request( .environment_variables .push(remexec::command::EnvironmentVariable { name: name.to_string(), - value: value.to_string(), + value: value.replace("{chroot}", SANDBOX_ROOT_TOKEN), }); } From 210ce83c31182d501280bd46c49d52a25bea5dca Mon Sep 17 00:00:00 2001 From: Tom Dyas Date: Fri, 3 Jan 2025 11:30:48 +0000 Subject: [PATCH 06/10] fix up test digests for wrapper changes --- .../engine/process_execution/remote/src/remote_tests.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rust/engine/process_execution/remote/src/remote_tests.rs b/src/rust/engine/process_execution/remote/src/remote_tests.rs index 2ef7f27496a..916028b0a61 100644 --- a/src/rust/engine/process_execution/remote/src/remote_tests.rs +++ b/src/rust/engine/process_execution/remote/src/remote_tests.rs @@ -724,7 +724,7 @@ async fn make_execute_request_with_append_only_caches() { input_root_digest: Some( (Digest::new( Fingerprint::from_hex_string( - "d6f65a7f56380de341fac43fcb4e2006fd0e716579325c4640ec11b7c040bb9c", + "40ff253312fe7bb50acad02e81e3a16b0109eb934c37bb61c3735792b8bead5f", ) .unwrap(), 178, @@ -739,7 +739,7 @@ async fn make_execute_request_with_append_only_caches() { action_digest: Some( (&Digest::new( Fingerprint::from_hex_string( - "d516c6b3b9781777486025ebc87cb4810bc7088dc45092a70a592d8ca74f48d8", + "798ac2aaf68b36d571fcb90719e8dafdf5929081c491f178ac7f4663b591183e", ) .unwrap(), 146, @@ -752,7 +752,7 @@ async fn make_execute_request_with_append_only_caches() { let want_input_root_digest = DirectoryDigest::from_persisted_digest(Digest::new( Fingerprint::from_hex_string( - "d6f65a7f56380de341fac43fcb4e2006fd0e716579325c4640ec11b7c040bb9c", + "40ff253312fe7bb50acad02e81e3a16b0109eb934c37bb61c3735792b8bead5f", ) .unwrap(), 178, From 7209177e4e0358ecf759ebcb62adef8a98b7692e Mon Sep 17 00:00:00 2001 From: Tom Dyas Date: Fri, 3 Jan 2025 11:34:13 +0000 Subject: [PATCH 07/10] use constant for `{chroot}` literal --- src/rust/engine/process_execution/src/lib.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index 8d796aeee38..22d897487e2 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -1023,7 +1023,6 @@ fn quote_path(path: &Path) -> Result { /// Check supplied parameters from an execution request and create a wrapper script if a wrapper script /// is needed. -// TODO: Support `{chroot}` replacement in environment variables and not just program arguments. fn maybe_make_wrapper_script( caches: &BTreeMap, append_only_caches_base_path: Option<&str>, @@ -1156,17 +1155,18 @@ pub async fn make_execute_request( ) -> Result { const WRAPPER_SCRIPT: &str = "./__pants_wrapper__"; const SANDBOX_ROOT_TOKEN: &str = "__PANTS_SANDBOX_ROOT__"; + const CHROOT_MARKER: &str = "{chroot}"; // Implement append-only caches by running a wrapper script before the actual program // to be invoked in the remote environment. let wrapper_script_content_opt = { - let args_have_chroot_marker = req.argv.iter().any(|arg| arg.contains("{chroot}")); + let args_have_chroot_marker = req.argv.iter().any(|arg| arg.contains(CHROOT_MARKER)); let env_vars_with_chroot_marker = req .env .iter() .filter_map(|(key, value)| { - if value.contains("{chroot}") { + if value.contains(CHROOT_MARKER) { Some(key.to_owned()) } else { None @@ -1212,7 +1212,7 @@ pub async fn make_execute_request( args.extend( req.argv .iter() - .map(|orig_arg| orig_arg.replace("{chroot}", SANDBOX_ROOT_TOKEN)), + .map(|orig_arg| orig_arg.replace(CHROOT_MARKER, SANDBOX_ROOT_TOKEN)), ); args } @@ -1238,7 +1238,7 @@ pub async fn make_execute_request( .environment_variables .push(remexec::command::EnvironmentVariable { name: name.to_string(), - value: value.replace("{chroot}", SANDBOX_ROOT_TOKEN), + value: value.replace(CHROOT_MARKER, SANDBOX_ROOT_TOKEN), }); } From 266645c31b5ee22304681210f5e22824dc4abeed Mon Sep 17 00:00:00 2001 From: Tom Dyas Date: Fri, 3 Jan 2025 11:40:35 +0000 Subject: [PATCH 08/10] sort the env vars for consistent ordering --- src/rust/engine/process_execution/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index 22d897487e2..49b2918a6cf 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -1162,7 +1162,7 @@ pub async fn make_execute_request( let wrapper_script_content_opt = { let args_have_chroot_marker = req.argv.iter().any(|arg| arg.contains(CHROOT_MARKER)); - let env_vars_with_chroot_marker = req + let mut env_vars_with_chroot_marker = req .env .iter() .filter_map(|(key, value)| { @@ -1173,6 +1173,7 @@ pub async fn make_execute_request( } }) .collect::>(); + env_vars_with_chroot_marker.sort(); let env_var_with_chroot_marker_refs = env_vars_with_chroot_marker .iter() From 476049e1199ccca2bbbbc2ca848718238214207c Mon Sep 17 00:00:00 2001 From: Tom Dyas Date: Fri, 3 Jan 2025 11:57:42 +0000 Subject: [PATCH 09/10] shlex the env var names use Vec<&str> due to IntoIterator issues with &[&str] --- src/rust/engine/process_execution/src/lib.rs | 7 ++++--- src/rust/engine/process_execution/src/tests.rs | 17 +++++++++++------ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs index 49b2918a6cf..19bee39114c 100644 --- a/src/rust/engine/process_execution/src/lib.rs +++ b/src/rust/engine/process_execution/src/lib.rs @@ -1028,7 +1028,7 @@ fn maybe_make_wrapper_script( append_only_caches_base_path: Option<&str>, working_directory: Option<&str>, sandbox_root_token: Option<&str>, - env_vars_to_substitute: &[&str], + env_vars_to_substitute: Vec<&str>, ) -> Result, String> { let caches_fragment = match append_only_caches_base_path { Some(base_path) if !caches.is_empty() => { @@ -1097,7 +1097,8 @@ fn maybe_make_wrapper_script( token ); if !env_vars_to_substitute.is_empty() { - let env_vars_str = env_vars_to_substitute.join(" "); // TODO: shlex the strings + let env_vars_str = shlex::try_join(env_vars_to_substitute) + .map_err(|e| format!("Failed to shell quote environment names: {e}"))?; writeln!(&mut fragment, "for env_var in {env_vars_str}; do") .map_err(|err| format!("write! failed: {err}"))?; writeln!( @@ -1189,7 +1190,7 @@ pub async fn make_execute_request( append_only_caches_base_path, req.working_directory.as_ref().and_then(|p| p.to_str()), sandbox_root_token, - env_var_with_chroot_marker_refs.as_slice(), + env_var_with_chroot_marker_refs, )? }; let wrapper_script_digest_opt = if let Some(script) = wrapper_script_content_opt { diff --git a/src/rust/engine/process_execution/src/tests.rs b/src/rust/engine/process_execution/src/tests.rs index 2c294a5a697..ab82d61bf14 100644 --- a/src/rust/engine/process_execution/src/tests.rs +++ b/src/rust/engine/process_execution/src/tests.rs @@ -182,7 +182,7 @@ async fn wrapper_script_supports_append_only_caches() { dummy_caches_base_path.path().to_str(), Some(SUBDIR_NAME), None, - &[], + vec![], ) .unwrap() .unwrap(); @@ -243,7 +243,7 @@ async fn wrapper_script_supports_append_only_caches() { async fn wrapper_script_supports_sandbox_root_replacements_in_args() { let caches = BTreeMap::new(); - let script_content = maybe_make_wrapper_script(&caches, None, None, Some("__ROOT__"), &[]) + let script_content = maybe_make_wrapper_script(&caches, None, None, Some("__ROOT__"), vec![]) .unwrap() .unwrap(); @@ -287,10 +287,15 @@ async fn wrapper_script_supports_sandbox_root_replacements_in_args() { async fn wrapper_script_supports_sandbox_root_replacements_in_environmenbt() { let caches = BTreeMap::new(); - let script_content = - maybe_make_wrapper_script(&caches, None, None, Some("__ROOT__"), &["TEST_FILE_PATH"]) - .unwrap() - .unwrap(); + let script_content = maybe_make_wrapper_script( + &caches, + None, + None, + Some("__ROOT__"), + vec!["TEST_FILE_PATH"], + ) + .unwrap() + .unwrap(); let dummy_sandbox_path = TempDir::new().unwrap(); let script_path = dummy_sandbox_path.path().join("wrapper"); From 1edac7164b6bf0a7c83d3d096c825ff4d980d61d Mon Sep 17 00:00:00 2001 From: Tom Dyas Date: Fri, 3 Jan 2025 14:18:29 +0000 Subject: [PATCH 10/10] release notes --- docs/notes/2.25.x.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/notes/2.25.x.md b/docs/notes/2.25.x.md index f893dcda712..75995b77cd7 100644 --- a/docs/notes/2.25.x.md +++ b/docs/notes/2.25.x.md @@ -29,7 +29,7 @@ Pants now sends a `user-agent` header with every request to a remote store or a even when other headers are configured. If necessary, the user may override the user agent by specifying one in `remote_store_headers` or `remote_execution_headers`. -Pants now supports the `{chroot}` replacement marker in program arguments for remote execution. (With local and Docker execution, the `{chroot}` marker is replaced with the absolute path of the sandbox directory if it appears in program arguments or environment variables. Pants will now replace `{chroot}` for program arguments with remote execution, but still does not yet support doing so in environment variables. This requires `/bin/bash` to be available on the remote execution server.) +Pants now supports the `{chroot}` replacement marker in remote execution contexts. (With local and Docker execution, the `{chroot}` marker is replaced with the absolute path of the sandbox directory if it appears in program arguments or environment variables. Pants will do the same as well in remote execution contexts. This requires `/bin/bash` to be available on the remote execution server.) ### New Options System