Skip to content
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

cargo pgrx test --runas envar passing #1674

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
cargo pgrx test --runas envar passing
This teaches the test `framework.rs` to pass through to `sudo` all the
various cargo/rust environment variables.

Turns out this might be necessary for `#[pg_test]` tests that use
`std::env::var(...)`.  When using `--runas` we spawn the postmaster with
`sudo`, and its default policy is to reset the environment (`env_reset`
flag in the sudoers file).
eeeebbbbrrrr committed Apr 22, 2024
commit 93b5517d996e897721b43a46cdf58e9d1de6c69c
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -65,6 +65,7 @@ owo-colors = { version = "3.5", features = [ "supports-colors" ] } # for output
proc-macro2 = { version = "1.0.78", features = [ "span-locations" ] }
quote = "1.0.33"
regex = "1.1" # used for build/test
shlex = "1.3" # shell lexing, also used by many of our deps
syn = { version = "2", features = [ "extra-traits", "full", "parsing" ] }
toml = "0.8" # our config files
thiserror = "1"
2 changes: 1 addition & 1 deletion pgrx-pg-sys/Cargo.toml
Original file line number Diff line number Diff line change
@@ -55,10 +55,10 @@ pgrx-pg-config.workspace = true

eyre.workspace = true
proc-macro2.workspace = true
shlex.workspace = true
syn.workspace = true
walkdir.workspace = true

bindgen = { version = "0.69", default-features = false, features = ["runtime"] }
clang-sys = { version = "1", features = ["clang_6_0", "runtime"] }
quote = "1.0.33"
shlex = "1.3" # shell lexing, also used by many of our deps
1 change: 1 addition & 0 deletions pgrx-tests/Cargo.toml
Original file line number Diff line number Diff line change
@@ -56,6 +56,7 @@ eyre.workspace = true
libc.workspace = true
owo-colors.workspace = true
regex.workspace = true
shlex.workspace = true
thiserror.workspace = true

paste = "1"
24 changes: 24 additions & 0 deletions pgrx-tests/src/framework.rs
Original file line number Diff line number Diff line change
@@ -546,7 +546,31 @@ fn start_pg(loglines: LogLines) -> eyre::Result<String> {
cmd.arg(postmaster_path);
cmd
} else if let Some(runas) = get_runas() {
#[inline]
fn accept_envar(var: &str) -> bool {
// taken from https://doc.rust-lang.org/cargo/reference/environment-variables.html
var.starts_with("CARGO")
|| var.starts_with("RUST")
|| var.starts_with("DEP_")
|| ["OUT_DIR", "TARGET", "HOST", "NUM_JOBS", "OPT_LEVEL", "DEBUG", "PROFILE"]
.contains(&var)
Comment on lines +552 to +556
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, regex maybe?

}

let mut cmd = sudo_command(runas);

// when running the `postmaster` process via `sudo`, we need to copy the cargo/rust-related
// environment variables and pass as arguments to sudo, ahead of the `postmaster` command itself
//
// This ensures that any in-processs #[pg_test]s that might otherwise expect some kind of
// `CARGO_xxx` envar to actually exist.
for (var, value) in std::env::vars() {
if accept_envar(&var) {
let env_as_arg = format!("{var}={}", shlex::try_quote(&value)?);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we use shlex, we should just skip the env vars that we can't quote the values of correctly, or we should panic more noisily, or we should stop iterating the env vars entirely. here is one of those options.

Suggested change
let env_as_arg = format!("{var}={}", shlex::try_quote(&value)?);
let Ok(quoted_val) = shlex::try_quote(&value) else {
continue
};
let env_as_arg = format!("{var}={quoted_val}");

cmd.arg(env_as_arg);
}
}
Comment on lines +565 to +570
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively, instead of splitting and then reapplying these, why not use sudo's --preserve-env=var,var,var,...? are we worried about not having the required privilege?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t see that in the man page. Let me investigate that and make sure we can rely on it even being there here in 2023.


// now we can add the `postmaster` as the command for `sudo` to execute
cmd.arg(postmaster_path);
cmd
} else {