fix: inherit config.json env vars in exec processes#3439
fix: inherit config.json env vars in exec processes#3439saku3 merged 7 commits intoyouki-dev:mainfrom
Conversation
5efb350 to
be3c32c
Compare
nayuta723
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The implementation looks solid. I’ve left a few comments mainly around simplification and avoiding unnecessary allocations.
b1e5100 to
bc4bddc
Compare
Previously, youki exec only used env vars passed via --env on the CLI, plus PATH from the container spec. All other spec env vars were silently dropped. This differs from runc, crun, and runsc, which inherit the full process.env from config.json. Now the spec's env vars are used as a baseline, with CLI --env overriding by variable name. This matches the behavior of other OCI runtimes. Closes youki-dev#3428 Signed-off-by: KevinKickass <k@ppload.eu>
b7ed342 to
46857df
Compare
|
Could you please make sure the CI passes? Also, it would be great if we could add an e2e test for this. Ref: https://github.com/youki-dev/youki/tree/main/tests/contest/contest |
Add contest e2e tests for exec environment variable handling: - spec env inheritance, CLI override, CLI addition, process.json Merge duplicate test modules (mod test + mod tests) in tenant_builder.rs into a single mod tests block and fix rustfmt chain formatting. Signed-off-by: KevinKickass <k@ppload.eu>
Matches the pattern used in other contest tests (e.g., mount_test.rs). Without this check, start_container would fail silently if the container creation step had issues. Signed-off-by: KevinKickass <k@ppload.eu>
d549311 to
863c24a
Compare
saku3
left a comment
There was a problem hiding this comment.
Thank you for the update.
I left a comment, so could you please take a look?
| self.get_process(process)? | ||
| } else { | ||
| let original_path_env = get_path_from_spec(spec); | ||
| // Inherit spec env vars for exec processes (matches runc/crun/runsc). |
There was a problem hiding this comment.
// Use the spec's process env as the baseline for exec.
is enough. The same point is already described in get_environment(), so if the behavior changes later, having it in both places would hurt maintainability.
| env_exists = true; | ||
| } | ||
| format!("{k}={v}") | ||
| /// Builds the environment for an exec process. The spec's env vars are |
There was a problem hiding this comment.
| /// Builds the environment for an exec process. The spec's env vars are | |
| /// Builds the environment for an exec process. | |
| /// The spec's env vars are used as the baseline, and env vars provided to the | |
| /// builder, such as those from the CLI, override entries with the same key. | |
| /// This follows runc's behavior. | |
| /// See <https://github.com/youki-dev/youki/issues/3428>. |
I checked the behavior of crun and runsc as well.
That said, mentioning them in the comment would make us implicitly responsible for keeping that comparison accurate, and there may still be subtle behavioral differences.
Since youki primarily aims for compatibility with runc, I think it would be better to mention only runc here.
| } | ||
|
|
||
| #[test] | ||
| fn cli_env_overrides_spec() { |
There was a problem hiding this comment.
When I think of this as a unit test for TenantContainerBuilder, it is not clear that this is specifically about the CLI.
How about the following name instead?
builder_env_overrides_spec
| } | ||
|
|
||
| #[test] | ||
| fn cli_env_adds_new_vars() { |
There was a problem hiding this comment.
Similarly, how about:
builder_env_adds_new_vars
| SpecBuilder::default() | ||
| .process( | ||
| ProcessBuilder::default() | ||
| .args(vec!["sleep".to_string(), "10000".to_string()]) |
There was a problem hiding this comment.
| .args(vec!["sleep".to_string(), "10000".to_string()]) | |
| .args(vec!["sleep".to_string(), "1000".to_string()]) |
| } | ||
|
|
||
| /// Helper: run exec with --env flags via the CLI. | ||
| fn exec_with_env<P: AsRef<Path>>( |
There was a problem hiding this comment.
It might be better to consolidate this into the existing exec_container helper, but that does not need to be done right now.
| } | ||
|
|
||
| #[test] | ||
| fn empty_spec_env_uses_cli_only() { |
There was a problem hiding this comment.
| fn empty_spec_env_uses_cli_only() { | |
| fn empty_spec_env_uses_builder_env_only() { |
675aaa1 to
9e5f5a9
Compare
- Shorten inline comment per review suggestion - Update get_environment docstring to mention only runc - Rename unit tests: cli_env_* -> builder_env_* - Reduce sleep from 10000 to 1000 in exec_env tests - Use full PATH in exec_env test specs so sleep is found in the busybox rootfs (/bin/sleep) - Add env parameter to exec_container helper instead of duplicating the logic in a separate function Signed-off-by: KevinKickass <k@ppload.eu>
9e5f5a9 to
3eccc4e
Compare
|
Thanks for taking the time to review this so thoroughly — really good catches all around. Pushed the updates:
|
|
Thank you for the update. Running |
Signed-off-by: KevinKickass <k@ppload.eu>
* fix: inherit spec env vars in exec process Previously, youki exec only used env vars passed via --env on the CLI, plus PATH from the container spec. All other spec env vars were silently dropped. This differs from runc, crun, and runsc, which inherit the full process.env from config.json. Now the spec's env vars are used as a baseline, with CLI --env overriding by variable name. This matches the behavior of other OCI runtimes. Closes youki-dev#3428 Signed-off-by: KevinKickass <k@ppload.eu> * test: add e2e tests for exec --env and merge test modules Add contest e2e tests for exec environment variable handling: - spec env inheritance, CLI override, CLI addition, process.json Merge duplicate test modules (mod test + mod tests) in tenant_builder.rs into a single mod tests block and fix rustfmt chain formatting. Signed-off-by: KevinKickass <k@ppload.eu> * fix: add check_container_created before start in exec_env tests Matches the pattern used in other contest tests (e.g., mount_test.rs). Without this check, start_container would fail silently if the container creation step had issues. Signed-off-by: KevinKickass <k@ppload.eu> * fix: address review feedback and fix exec_env test PATH - Shorten inline comment per review suggestion - Update get_environment docstring to mention only runc - Rename unit tests: cli_env_* -> builder_env_* - Reduce sleep from 10000 to 1000 in exec_env tests - Use full PATH in exec_env test specs so sleep is found in the busybox rootfs (/bin/sleep) - Add env parameter to exec_container helper instead of duplicating the logic in a separate function Signed-off-by: KevinKickass <k@ppload.eu> * style: cargo fmt Signed-off-by: KevinKickass <k@ppload.eu> --------- Signed-off-by: KevinKickass <k@ppload.eu>
Closes #3428
Problem
youki execdoes not inherit environment variables from the container'sconfig.jsonprocess.env. Only variables passed via--envon the CLI (plusPATHfrom the spec) are available to the exec process. This differs from runc, crun, and runsc, which all inherit the fullprocess.env.Reproduction:
Root cause
In
crates/libcontainer/src/container/tenant_builder.rs, the functionget_path_from_spec()only extracted thePATHvariable from the spec's env. This single value was passed toget_environment(), which built the exec env from CLI--envargs + that one PATH fallback. All other spec env vars were silently dropped.Changes
crates/libcontainer/src/container/tenant_builder.rs(only file changed):Replaced
get_path_from_spec()withget_env_from_spec()— returns all env vars from the container spec instead of just PATH. The old function was only used in one place, so nothing else is affected.Rewrote
get_environment()— now takes the full spec env as a baseline. CLI--envoverrides by variable name (matching by the key before=). This means:--env FOO=baron exec still overridesFOOfrom the spec--envare appended as beforeAdded 5 unit tests for
get_environment():env_inherits_spec_vars— spec vars are passed throughcli_env_overrides_spec— CLI--envtakes precedence by namecli_env_adds_new_vars— new CLI vars are appendedempty_spec_env_uses_cli_only— no spec env, CLI onlyno_env_at_all— empty in, empty outBackward compatibility
Fully backward compatible. The
--envflag still works exactly as before (overrides by name). The only difference is that spec env vars are now inherited as the baseline, which is what users expect from the OCI runtime spec and how every other runtime handles it.