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

Add new setup_envs method for the Executor trait #2820

Merged
merged 7 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
22 changes: 10 additions & 12 deletions crates/libcontainer/src/process/container_init_process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ pub enum InitProcessError {
Workload(#[from] workload::ExecutorError),
#[error(transparent)]
WorkloadValidation(#[from] workload::ExecutorValidationError),
#[error(transparent)]
WorkloadSetEnvs(#[from] workload::ExecutorSetEnvsError),
musaprg marked this conversation as resolved.
Show resolved Hide resolved
#[error("invalid io priority class: {0}")]
IoPriorityClass(String),
#[error("call exec sched_setattr error: {0}")]
Expand Down Expand Up @@ -558,18 +560,6 @@ pub fn container_init_process(
err
})?;

// add HOME into envs if not exists
if !envs.contains_key("HOME") {
if let Some(dir_home) = utils::get_user_home(proc.user().uid()) {
envs.insert("HOME".to_owned(), dir_home.to_string_lossy().to_string());
}
}

// Reset the process env based on oci spec.
env::vars().for_each(|(key, _value)| env::remove_var(key));
envs.iter()
.for_each(|(key, value)| env::set_var(key, value));

// Initialize seccomp profile right before we are ready to execute the
// payload so as few syscalls will happen between here and payload exec. The
// notify socket will still need network related syscalls.
Expand All @@ -591,7 +581,15 @@ pub fn container_init_process(
tracing::warn!("seccomp not available, unable to set seccomp privileges!")
}

// add HOME into envs if not exists
if !envs.contains_key("HOME") {
if let Some(dir_home) = utils::get_user_home(proc.user().uid()) {
envs.insert("HOME".to_owned(), dir_home.to_string_lossy().to_string());
}
}

args.executor.validate(spec)?;
args.executor.setup_envs(envs)?;

// Notify main process that the init process is ready to execute the
// payload. Note, because we are already inside the pid namespace, the pid
Expand Down
36 changes: 36 additions & 0 deletions crates/libcontainer/src/workload/default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,11 @@ fn is_executable(path: &Path) -> std::result::Result<bool, std::io::Error> {

#[cfg(test)]
mod tests {
use std::collections::HashMap;
use std::env;

use serial_test::serial;

use super::*;

#[test]
Expand Down Expand Up @@ -177,4 +182,35 @@ mod tests {
assert!(!is_executable(&non_executable_path).unwrap());
assert!(!is_executable(directory_path).unwrap());
}

#[test]
#[serial]
fn test_executor_set_envs() {
musaprg marked this conversation as resolved.
Show resolved Hide resolved
// Store original environment variables to restore later
let original_envs: HashMap<String, String> = env::vars().collect();

// Test setting environment variables
{
let executor = get_executor();
let envs = HashMap::from([
("FOO".to_owned(), "hoge".to_owned()),
("BAR".to_owned(), "fuga".to_owned()),
("BAZ".to_owned(), "piyo".to_owned()),
]);
assert!(executor.setup_envs(envs).is_ok());

// Check if the environment variables are set correctly
let current_envs = std::env::vars().collect::<HashMap<String, String>>();
assert_eq!(current_envs.get("FOO").unwrap(), "hoge");
assert_eq!(current_envs.get("BAR").unwrap(), "fuga");
assert_eq!(current_envs.get("BAZ").unwrap(), "piyo");
// No other environment variables should be set
assert_eq!(current_envs.len(), 3);
}

// Restore original environment variables
original_envs.iter().for_each(|(key, value)| {
env::set_var(key, value);
});
}
}
29 changes: 29 additions & 0 deletions crates/libcontainer/src/workload/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
use std::collections::HashMap;
use std::env;

use oci_spec::runtime::Spec;

pub mod default;
Expand All @@ -24,6 +27,14 @@ pub enum ExecutorValidationError {
ArgValidationError(String),
}

#[derive(Debug, thiserror::Error)]
pub enum ExecutorSetEnvsError {
#[error("failed to set envs")]
SetEnvs(#[from] Box<dyn std::error::Error + Send + Sync>),
#[error("{0}")]
Other(String),
}

// Here is an explanation about the complexity below regarding to
// CloneBoxExecutor and Executor traits. This is one of the places rust actually
// makes our life harder. The usecase for the executor is to allow users of
Expand Down Expand Up @@ -60,6 +71,24 @@ pub trait Executor: CloneBoxExecutor {
/// namespace and cgroups, and pivot_root into the rootfs. But this step
/// runs before waiting for the container start signal.
fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidationError>;

/// Set environment variables for the container process to be executed.
Copy link
Member

Choose a reason for hiding this comment

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

It'd be better to explain the situation this method is called. For example, this method is called the init process and hasn't cleared the host's envs yet.

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've added a detailed description for the setup_envs in b20b8fa.

/// This step runs after the container init process is created, entered
/// into the correct namespace and cgroups, and pivot_root into the rootfs.
/// But this step runs before waiting for the container start signal.
/// The host's environment variables are not cleared yet at this point.
/// They should be cleared explicitly if needed.
fn setup_envs(&self, envs: HashMap<String, String>) -> Result<(), ExecutorSetEnvsError> {
// The default implementation resets the process env based on the OCI spec.
// First, clear all host's envs.
env::vars().for_each(|(key, _value)| env::remove_var(key));

// Next, set envs based on the spec
envs.iter()
.for_each(|(key, value)| env::set_var(key, value));

Ok(())
}
}

impl<T> CloneBoxExecutor for T
Expand Down
Loading