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

Conversation

eeeebbbbrrrr
Copy link
Contributor

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

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
Copy link
Contributor Author

we're now doing the latter version:

$ FOO=bar sudo -u testuser printenv FOO
$ sudo -u testuser FOO=bar printenv FOO
bar

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

this looks plausible but it seems like we could avoid most of this logic if we use sudo's flags more fully.

Comment on lines +552 to +556
var.starts_with("CARGO")
|| var.starts_with("RUST")
|| var.starts_with("DEP_")
|| ["OUT_DIR", "TARGET", "HOST", "NUM_JOBS", "OPT_LEVEL", "DEBUG", "PROFILE"]
.contains(&var)
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?

// This ensures that in-process #[pg_test]s will see the `CARGO_xxx` envars they expect
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}");

Comment on lines +565 to +570
for (var, value) in std::env::vars() {
if accept_envar(&var) {
let env_as_arg = format!("{var}={}", shlex::try_quote(&value)?);
cmd.arg(env_as_arg);
}
}
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.

@workingjubilee
Copy link
Member

Needs a rebase, unforch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants