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 #1652

Merged

Conversation

eeeebbbbrrrr
Copy link
Contributor

@eeeebbbbrrrr eeeebbbbrrrr commented Apr 12, 2024

$ cargo pgrx test pg14 --runas testuser --pgdata=/tmp/pgrxtest

This adds two new arguments to cargo pgrx test: --runas <username> and --pgdata </writable/path>

Together, these will initdb, createdb, dropdb, and run postgres as the "runas" user.

Extension artifacts are copied with cargo pgrx install --sudo, which will also require the current user be able to become root.

In general, these options are designed for the unusual situation where one needs to be "root" in order to run extension tests, but, of course, Postgres is not allowed to be root.

```shell
$ cargo pgrx test pg14 --runas testuser --pgdata=/tmp/pgrxtest
```

This adds two new arguments to `cargo pgrx test`: `--runas <username>` and `--pgdata </writable/path>`

Together, these will initidb, createdb, dropdb, and run postgres as the "runas" user.

Extension artifacts are copied with `cargo pgrx install --sudo`, which will also require the current user be able to become root.

In general, these options are designed for the unusual situation where one needs to be "root" in order to run extension tests, but, of course, Postgres is not allowed to be root.
cargo-pgrx/src/command/test.rs Show resolved Hide resolved
pgrx-tests/src/framework.rs Outdated Show resolved Hide resolved
@workingjubilee workingjubilee changed the title cargp pgrx test --runas cargo pgrx test --runas Apr 12, 2024
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.

I have several questions! Hopefully not all of them are relevant

pgrx-tests/src/framework.rs Show resolved Hide resolved
pgrx-tests/src/framework.rs Outdated Show resolved Hide resolved
pgrx-tests/src/framework.rs Outdated Show resolved Hide resolved
pgrx-tests/src/framework.rs Outdated Show resolved Hide resolved
pgrx-tests/src/framework.rs Outdated Show resolved Hide resolved
.github/workflows/runas.yml Outdated Show resolved Hide resolved
cargo-pgrx/README.md Outdated Show resolved Hide resolved
cargo-pgrx/src/command/test.rs Outdated Show resolved Hide resolved
cargo-pgrx/src/command/test.rs Outdated Show resolved Hide resolved
cargo-pgrx/src/command/test.rs Outdated Show resolved Hide resolved
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.

I unfortunately have more questions about the implementation now that my first batch of "do we have any alternatives?" questions has been exhausted.

pgrx-tests/src/framework.rs Outdated Show resolved Hide resolved
Comment on lines +1 to +38
name: cargo pgrx test --runas

on:
push:
branches: [ develop ]
pull_request:
branches: [ develop ]

env:
CARGO_TERM_COLOR: always
RUST_BACKTRACE: 1

jobs:
ubuntu:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v2
- uses: Swatinem/rust-cache@v2
with:
prefix-key: "v1-cargo-pgrx-test--runas"

- name: Install Postgres deps
run: |
sudo apt-get update -y -qq --fix-missing
sudo apt-get install -y postgresql-server-dev-14

- name: Report version
run: cargo --version

- name: Install cargo pgrx
run: cd cargo-pgrx && cargo install --path . --debug

- name: cargo pgrx init
run: cargo pgrx init --pg14=$(which pg_config)

- name: Test cargo pgrx test --runas
run: cd pgrx-examples/arrays && cargo pgrx test pg14 --runas postgres --pgdata=/tmp/pgdata
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a clone of this that uses just the env vars?

Comment on lines +153 to +159
if let Some(runas) = runas {
command.env("CARGO_PGRX_TEST_RUNAS", runas);
}

if let Some(pgdata) = pgdata {
command.env("CARGO_PGRX_TEST_PGDATA", pgdata);
}
Copy link
Member

Choose a reason for hiding this comment

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

What should win if they do

CARGO_PGRX_TEST_PGDATA="/some/path/somewhere" cargo pgrx test --pgdata="/another/path/elsewhere"?

note that as far as I am concerned "cargo pgrx should explode in their face and tell them not to do that" is a fine answer, but if we want, we can take the value from the environment by adding , env = "VARIABLE" to the list of "arguments" to the #[clap] attribute for that CLI arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that cargo pgrx test doesn't actually use the environment variables. It sets them as part of the cargo test ... Command it runs. As such, it's pgrx-tests/src/framework.rs that reads them.

So... I think cargo pgrx test should just continue to ignore it, like it does every other envar it doesn't use.

Copy link
Member

Choose a reason for hiding this comment

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

That's fair.

Hm. Maybe the test vars introduced here should be named PGRX_TEST_*? Not a serious concern, just wondering.

cargo-pgrx/README.md Show resolved Hide resolved
pgrx-tests/src/framework.rs Outdated Show resolved Hide resolved
pgrx-tests/src/framework.rs Outdated Show resolved Hide resolved
Comment on lines 503 to 506
let mut sudo_command = Command::new("sudo")
.arg("-u")
.arg(&runas)
.arg("tee")
Copy link
Member

Choose a reason for hiding this comment

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

You mentioned making a SudoCommand type and I think what I'd like to actually see is these ~4 lines made into a function/whatever that returns Command.

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.

You can make the final changes I mentioned. Or not. I'm good with this now.

@workingjubilee
Copy link
Member

I should note that I am "good with this" in the sense that I have made my peace with the horrors of the world that is Unix access control.

I do encourage you to bother the actions/checkout people, @theory, and try to fix things on their end. If this code turns out to have a security flaw of any significant import... anything that's not "you can pwn a computer that's already been pwned", which sometimes gets reported as a "vuln" anyways, then I will not hesitate to rip it out. Likely --pgdata will remain, I suppose.

@theory
Copy link
Contributor

theory commented Apr 16, 2024

Fair enough, I've gone and complained in actions/checkout#956.

@theory
Copy link
Contributor

theory commented Apr 16, 2024

There's also this bit of documentation, though:

Docker actions must be run by the default Docker user (root). Do not use the USER instruction in your Dockerfile, because you won't be able to access the GITHUB_WORKSPACE directory. For more information, see "Variables" and USER reference in the Docker documentation.

This is for action containers, but I think it applies to workflows steps that run with container.

@workingjubilee
Copy link
Member

Since Docker does support setting a user, I believe the problem is that GHA doesn't support it, so deescalating privileges seems like something GHA could support nonetheless.

@eeeebbbbrrrr eeeebbbbrrrr merged commit 0c823db into pgcentralfoundation:develop Apr 18, 2024
12 checks passed
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.

None yet

3 participants