Skip to content

Use process env for StartContainer hooks when without explicit hook env#3470

Open
bells17 wants to merge 4 commits intoyouki-dev:mainfrom
bells17:inherit-process-environment
Open

Use process env for StartContainer hooks when without explicit hook env#3470
bells17 wants to merge 4 commits intoyouki-dev:mainfrom
bells17:inherit-process-environment

Conversation

@bells17
Copy link
Copy Markdown
Contributor

@bells17 bells17 commented Mar 23, 2026 •
edited
Loading

Description

Fix StartContainer hooks to use the container's process.env when no explicit env is specified in the hook definition, matching runc's behavior.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test updates
  • CI/CD related changes
  • Other (please describe):

Testing

  • Added new unit tests
  • Added new integration tests
  • Ran existing test suite
  • Tested manually (please provide steps)

Related Issues

Fixes #3380

Additional Context

@bells17 bells17 force-pushed the inherit-process-environment branch from e4671c9 to f15f234 Compare March 23, 2026 07:53
@bells17 bells17 changed the title se process env for StartContainer hooks when without explicit hook env Use process env for StartContainer hooks when without explicit hook env Mar 23, 2026
@bells17 bells17 force-pushed the inherit-process-environment branch 2 times, most recently from df1b2d8 to 49ff36e Compare March 23, 2026 09:06
nayuta723
nayuta723 reviewed Mar 25, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@nayuta723 nayuta723 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I've left a few nits. Please take a look when you have a momemnt.

crates/libcontainer/src/hooks.rs Outdated
.args(vec![
String::from("bash"),
String::from("-c"),
String::from("printenv TEST_ENV > /dev/null"),
Copy link
Copy Markdown
Contributor

@nayuta723 nayuta723 Mar 25, 2026

Choose a reason for hiding this comment

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

Maybe we should validate the TEST_ENV value here?

crates/libcontainer/src/hooks.rs Outdated
let hook = HookBuilder::default()
.path("bash")
.args(vec![
String::from("bash"),
Copy link
Copy Markdown
Contributor

@nayuta723 nayuta723 Mar 25, 2026

Choose a reason for hiding this comment

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

Can you use sh instead of bash?

@bells17 bells17 force-pushed the inherit-process-environment branch from f0e22cd to 5c58ad5 Compare March 25, 2026 17:56
@bells17
Copy link
Copy Markdown
Contributor Author

bells17 commented Mar 26, 2026

@nayuta723 Thank you for the review! I've addressed your feedback:

  • Validate the TEST_ENV value instead of just checking its existence
  • Use sh instead of bash for portability

Could you please take another look?

nayuta723
nayuta723 approved these changes Mar 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@nayuta723 nayuta723 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, Thanks!

saku3
saku3 reviewed Apr 1, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@saku3 saku3 left a comment

Choose a reason for hiding this comment

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

Thanks.

Is it possible to add e2e tests?

https://github.com/opencontainers/runc/blob/main/tests/integration/hooks.bats#L44

bells17 added 4 commits April 7, 2026 00:04
@bells17
fix: use process env for StartContainer hooks when without explicit h…
51f1ca9 
…ook env

Signed-off-by: bells17 <bells171@gmail.com>
@bells17
Use sh instead of bash
b935e38 
Signed-off-by: bells17 <bells171@gmail.com>
@bells17
test: validate default_env value instead of just checking existence
506244e 
Signed-off-by: bells17 <bells171@gmail.com>
@bells17
test: add e2e tests for startContainer hook env inheritance
69160bb 
Add two integration tests to verify startContainer hook environment
variable behavior:
- start_container_env_inherit: verifies hooks inherit process.env when
  no explicit env is set
- start_container_env_explicit: verifies hooks use only their explicit
  env and do not inherit process.env

Signed-off-by: bells17 <bells171@gmail.com>
@bells17 bells17 force-pushed the inherit-process-environment branch from 61f2478 to 69160bb Compare April 6, 2026 15:09
@bells17
Copy link
Copy Markdown
Contributor Author

bells17 commented Apr 6, 2026

NOTE: Since there were conflicts, I performed a rebase and a force push.

@bells17
Copy link
Copy Markdown
Contributor Author

bells17 commented Apr 6, 2026

@saku3 I've added the e2e tests. Could you take another look?

@bells17 bells17 requested a review from saku3 April 6, 2026 15:21
saku3
saku3 reviewed Apr 8, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@saku3 saku3 left a comment

Choose a reason for hiding this comment

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

Thank you for adding the e2e test.

Could you take another look at the status handling here?

From what I tested, the following test should fail, but it passes:

fn get_test_explicit_env() -> Test {
    Test::new(
        "start_container_env_explicit",
        Box::new(|| {
            run_hook_env_test(
                "false",
                vec![],
                None,
            )
        }),
    )
}

tests/contest/contest/src/tests/hooks/start_container_env.rs
}

pub fn get_start_container_env_tests() -> TestGroup {
let mut tg = TestGroup::new("start_container_env");
Copy link
Copy Markdown
Member

@saku3 saku3 Apr 8, 2026

Choose a reason for hiding this comment

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

start_container_env sounds a bit too generic. Since this is specifically testing the hook environment, start_container_hook_env_inherit might be clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saku3 saku3 saku3 left review comments

+1 more reviewer

@nayuta723 nayuta723 nayuta723 approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

kind/bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

runc compatibility: startContainer hook should inherit process environment

4 participants

@bells17 @saku3 @nayuta723 @YJDoc2