move the validation logic into executor (#2258)

* move the validation logic into executor To allow more flexibility for the executor, we move the validate logic into the executor. The validate runs in the `create` step before workloads are executed. Instead of implementing the validation in the `exec`, to maintain backward competiability, we have to introduce an extra step. The exec is too late to fail if the spec is not validated. Signed-off-by: yihuaf <[email protected]> * Update the migration guide Signed-off-by: yihuaf <[email protected]> * Add a comment explaining when the validate step runs. Signed-off-by: yihuaf <[email protected]> * Implement different error types for validate Signed-off-by: yihuaf <[email protected]> --------- Signed-off-by: yihuaf <[email protected]>
youki-dev · Aug 22, 2023 · 5f3f4ce · 5f3f4ce
1 parent 2571bab
commit 5f3f4ce
Show file tree

Hide file tree

Showing 8 changed files with 204 additions and 125 deletions.
diff --git a/MirgationGuide.md b/MirgationGuide.md
@@ -1,14 +1,20 @@
+# Migration Guide
+
 This contains information for migrating library versions.
 
 ## V0.1.0 -> v0.2.0
 
 ### libcontainer
 
 - The `Rootless` struct has been re-named as `UserNamespaceConfig` , `RootlessIDMapper` has been re-named to `UserNamespaceIDMapper` , and correspondingly the `RootlessError` has been re-named to `UserNamespaceError` . This is due to the fact that the structure was to be used for containers when a new user namespace is to be created, and that is not strictly only for rootless uses. Accordingly, the fields of various structs has been updated to reflect this change :
-    - rootless (module name) -> user_ns
-    - Rootless::rootless_id_mapper -> UserNamespaceConfig::id_mapper
-    - LibcontainerError::Rootless -> LibcontainerError::UserNamespace
-    - ContainerBuilderImpl::rootless -> ContainerBuilderImpl::user_ns_config
-    - ContainerArgs::rootless -> ContainerArgs::user_ns_config
+  - rootless (module name) -> user_ns
+  - Rootless::rootless_id_mapper -> UserNamespaceConfig::id_mapper
+  - LibcontainerError::Rootless -> LibcontainerError::UserNamespace
+  - ContainerBuilderImpl::rootless -> ContainerBuilderImpl::user_ns_config
+  - ContainerArgs::rootless -> ContainerArgs::user_ns_config
+
+- Changes that will occur for properly running in rootless mode : TODO (@YJDoc2)
+
+- Executor now contains 2 methods for implementation. We introduce a `validate` step in addition to execute. The `validate` should validate the input OCI spec. The step runs after all the namespaces are entered and rootfs is pivoted.
 
-- Changes that will occur for properly running in rootless mode : TODO (@YJDoc2)
+- Executor is now composible instead of an array of executor. To implement multiple executor, create a new executor that runs all the executor. The users are now in control of how multiple executor are run.
diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs
@@ -70,78 +70,14 @@ pub enum InitProcessError {
     NotifyListener(#[from] notify_socket::NotifyListenerError),
     #[error(transparent)]
     Workload(#[from] workload::ExecutorError),
+    #[error(transparent)]
+    WorkloadValidation(#[from] workload::ExecutorValidationError),
     #[error("invalid io priority class: {0}")]
     IoPriorityClass(String),
 }
 
 type Result<T> = std::result::Result<T, InitProcessError>;
 
-fn get_executable_path(name: &str, path_var: &str) -> Option<PathBuf> {
-    // if path has / in it, we have to assume absolute path, as per runc impl
-    if name.contains('/') && PathBuf::from(name).exists() {
-        return Some(PathBuf::from(name));
-    }
-    for path in path_var.split(':') {
-        let potential_path = PathBuf::from(path).join(name);
-        if potential_path.exists() {
-            return Some(potential_path);
-        }
-    }
-    None
-}
-
-fn is_executable(path: &Path) -> std::result::Result<bool, std::io::Error> {
-    use std::os::unix::fs::PermissionsExt;
-    let metadata = path.metadata()?;
-    let permissions = metadata.permissions();
-    // we have to check if the path is file and the execute bit
-    // is set. In case of directories, the execute bit is also set,
-    // so have to check if this is a file or not
-    Ok(metadata.is_file() && permissions.mode() & 0o001 != 0)
-}
-
-// this checks if the binary to run actually exists and if we have
-// permissions to run it.  Taken from
-// https://github.com/opencontainers/runc/blob/25c9e888686773e7e06429133578038a9abc091d/libcontainer/standard_init_linux.go#L195-L206
-fn verify_binary(args: &[String], envs: &[String]) -> Result<()> {
-    let path_vars: Vec<&String> = envs.iter().filter(|&e| e.starts_with("PATH=")).collect();
-    if path_vars.is_empty() {
-        tracing::error!("PATH environment variable is not set");
-        return Err(InitProcessError::InvalidExecutable(args[0].clone()));
-    }
-    let path_var = path_vars[0].trim_start_matches("PATH=");
-    match get_executable_path(&args[0], path_var) {
-        None => {
-            tracing::error!(
-                "executable {} for container process not found in PATH",
-                args[0]
-            );
-            return Err(InitProcessError::InvalidExecutable(args[0].clone()));
-        }
-        Some(path) => match is_executable(&path) {
-            Ok(true) => {
-                tracing::debug!("found executable {:?}", path);
-            }
-            Ok(false) => {
-                tracing::error!(
-                    "executable {:?} does not have the correct permission set",
-                    path
-                );
-                return Err(InitProcessError::InvalidExecutable(args[0].clone()));
-            }
-            Err(err) => {
-                tracing::error!(
-                    "failed to check permissions for executable {:?}: {}",
-                    path,
-                    err
-                );
-                return Err(InitProcessError::Io(err));
-            }
-        },
-    }
-    Ok(())
-}
-
 fn sysctl(kernel_params: &HashMap<String, String>) -> Result<()> {
     let sys = PathBuf::from("/proc/sys");
     for (kernel_param, value) in kernel_params {
@@ -637,9 +573,7 @@ pub fn container_init_process(
         tracing::warn!("seccomp not available, unable to set seccomp privileges!")
     }
 
-    if let Some(args) = proc.args() {
-        verify_binary(args, &envs)?;
-    }
+    args.executor.validate(spec)?;
 
     // Notify main process that the init process is ready to execute the
     // payload.  Note, because we are already inside the pid namespace, the pid
@@ -1091,44 +1025,6 @@ mod tests {
         assert_eq!(0, got.len());
     }
 
-    #[test]
-    fn test_get_executable_path() {
-        let non_existing_abs_path = "/some/non/existent/absolute/path";
-        let existing_abs_path = "/usr/bin/sh";
-        let existing_binary = "sh";
-        let non_existing_binary = "non-existent";
-        let path_value = "/usr/bin:/bin";
-
-        assert_eq!(
-            get_executable_path(existing_abs_path, path_value),
-            Some(PathBuf::from(existing_abs_path))
-        );
-        assert_eq!(get_executable_path(non_existing_abs_path, path_value), None);
-
-        assert_eq!(
-            get_executable_path(existing_binary, path_value),
-            Some(PathBuf::from("/usr/bin/sh"))
-        );
-
-        assert_eq!(get_executable_path(non_existing_binary, path_value), None);
-    }
-
-    #[test]
-    fn test_is_executable() {
-        let tmp = tempfile::tempdir().expect("create temp directory for test");
-        let executable_path = PathBuf::from("/bin/sh");
-        let directory_path = tmp.path();
-        let non_executable_path = directory_path.join("non_executable_file");
-        let non_existent_path = PathBuf::from("/some/non/existent/path");
-
-        std::fs::File::create(non_executable_path.as_path()).unwrap();
-
-        assert!(is_executable(&non_existent_path).is_err());
-        assert!(is_executable(&executable_path).unwrap());
-        assert!(!is_executable(&non_executable_path).unwrap());
-        assert!(!is_executable(directory_path).unwrap());
-    }
-
     #[test]
     fn test_set_io_priority() {
         let test_command = TestHelperSyscall::default();

diff --git a/crates/libcontainer/src/workload/default.rs b/crates/libcontainer/src/workload/default.rs
@@ -1,9 +1,12 @@
-use std::ffi::CString;
+use std::{
+    ffi::CString,
+    path::{Path, PathBuf},
+};
 
 use nix::unistd;
 use oci_spec::runtime::Spec;
 
-use super::{Executor, ExecutorError, EMPTY};
+use super::{Executor, ExecutorError, ExecutorValidationError};
 
 #[derive(Clone)]
 pub struct DefaultExecutor {}
@@ -15,12 +18,10 @@ impl Executor for DefaultExecutor {
             .process()
             .as_ref()
             .and_then(|p| p.args().as_ref())
-            .unwrap_or(&EMPTY);
-
-        if args.is_empty() {
-            tracing::error!("no arguments provided to execute");
-            Err(ExecutorError::InvalidArg)?;
-        }
+            .ok_or_else(|| {
+                tracing::error!("no arguments provided to execute");
+                ExecutorError::InvalidArg
+            })?;
 
         let executable = args[0].as_str();
         let cstring_path = CString::new(executable.as_bytes()).map_err(|err| {
@@ -40,8 +41,123 @@ impl Executor for DefaultExecutor {
         // payload through execvp, so it should never reach here.
         unreachable!();
     }
+
+    fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidationError> {
+        let proc = spec
+            .process()
+            .as_ref()
+            .ok_or(ExecutorValidationError::InvalidArg)?;
+
+        if let Some(args) = proc.args() {
+            let envs: Vec<String> = proc.env().as_ref().unwrap_or(&vec![]).clone();
+            let path_vars: Vec<&String> = envs.iter().filter(|&e| e.starts_with("PATH=")).collect();
+            if path_vars.is_empty() {
+                tracing::error!("PATH environment variable is not set");
+                Err(ExecutorValidationError::InvalidArg)?;
+            }
+            let path_var = path_vars[0].trim_start_matches("PATH=");
+            match get_executable_path(&args[0], path_var) {
+                None => {
+                    tracing::error!(
+                        executable = ?args[0],
+                        "executable for container process not found in PATH",
+                    );
+                    Err(ExecutorValidationError::InvalidArg)?;
+                }
+                Some(path) => match is_executable(&path) {
+                    Ok(true) => {
+                        tracing::debug!(executable = ?path, "found executable in executor");
+                    }
+                    Ok(false) => {
+                        tracing::error!(
+                            executable = ?path,
+                            "executable does not have the correct permission set",
+                        );
+                        Err(ExecutorValidationError::InvalidArg)?;
+                    }
+                    Err(err) => {
+                        tracing::error!(
+                            executable = ?path,
+                            ?err,
+                            "failed to check permissions for executable",
+                        );
+                        Err(ExecutorValidationError::InvalidArg)?;
+                    }
+                },
+            }
+        }
+
+        Ok(())
+    }
 }
 
 pub fn get_executor() -> Box<dyn Executor> {
     Box::new(DefaultExecutor {})
 }
+
+fn get_executable_path(name: &str, path_var: &str) -> Option<PathBuf> {
+    // if path has / in it, we have to assume absolute path, as per runc impl
+    if name.contains('/') && PathBuf::from(name).exists() {
+        return Some(PathBuf::from(name));
+    }
+    for path in path_var.split(':') {
+        let potential_path = PathBuf::from(path).join(name);
+        if potential_path.exists() {
+            return Some(potential_path);
+        }
+    }
+    None
+}
+
+fn is_executable(path: &Path) -> std::result::Result<bool, std::io::Error> {
+    use std::os::unix::fs::PermissionsExt;
+    let metadata = path.metadata()?;
+    let permissions = metadata.permissions();
+    // we have to check if the path is file and the execute bit
+    // is set. In case of directories, the execute bit is also set,
+    // so have to check if this is a file or not
+    Ok(metadata.is_file() && permissions.mode() & 0o001 != 0)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_get_executable_path() {
+        let non_existing_abs_path = "/some/non/existent/absolute/path";
+        let existing_abs_path = "/usr/bin/sh";
+        let existing_binary = "sh";
+        let non_existing_binary = "non-existent";
+        let path_value = "/usr/bin:/bin";
+
+        assert_eq!(
+            get_executable_path(existing_abs_path, path_value),
+            Some(PathBuf::from(existing_abs_path))
+        );
+        assert_eq!(get_executable_path(non_existing_abs_path, path_value), None);
+
+        assert_eq!(
+            get_executable_path(existing_binary, path_value),
+            Some(PathBuf::from("/usr/bin/sh"))
+        );
+
+        assert_eq!(get_executable_path(non_existing_binary, path_value), None);
+    }
+
+    #[test]
+    fn test_is_executable() {
+        let tmp = tempfile::tempdir().expect("create temp directory for test");
+        let executable_path = PathBuf::from("/bin/sh");
+        let directory_path = tmp.path();
+        let non_executable_path = directory_path.join("non_executable_file");
+        let non_existent_path = PathBuf::from("/some/non/existent/path");
+
+        std::fs::File::create(non_executable_path.as_path()).unwrap();
+
+        assert!(is_executable(&non_existent_path).is_err());
+        assert!(is_executable(&executable_path).unwrap());
+        assert!(!is_executable(&non_executable_path).unwrap());
+        assert!(!is_executable(directory_path).unwrap());
+    }
+}
diff --git a/crates/libcontainer/src/workload/mod.rs b/crates/libcontainer/src/workload/mod.rs
@@ -16,6 +16,14 @@ pub enum ExecutorError {
     CantHandle(&'static str),
 }
 
+#[derive(Debug, thiserror::Error)]
+pub enum ExecutorValidationError {
+    #[error("{0} executor can't handle spec")]
+    CantHandle(&'static str),
+    #[error("invalid argument")]
+    InvalidArg,
+}
+
 // 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
@@ -46,6 +54,12 @@ pub trait CloneBoxExecutor {
 pub trait Executor: CloneBoxExecutor {
     /// Executes the workload
     fn exec(&self, spec: &Spec) -> Result<(), ExecutorError>;
+
+    /// Validate if the spec can be executed by the executor. 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.
+    fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidationError>;
 }
 
 impl<T> CloneBoxExecutor for T

diff --git a/crates/youki/src/workload/executor.rs b/crates/youki/src/workload/executor.rs
@@ -1,5 +1,5 @@
 use libcontainer::oci_spec::runtime::Spec;
-use libcontainer::workload::{Executor, ExecutorError};
+use libcontainer::workload::{Executor, ExecutorError, ExecutorValidationError};
 
 #[derive(Clone)]
 pub struct DefaultExecutor {}
@@ -29,6 +29,29 @@ impl Executor for DefaultExecutor {
         // container workloads.
         libcontainer::workload::default::get_executor().exec(spec)
     }
+
+    fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidationError> {
+        #[cfg(feature = "wasm-wasmer")]
+        match super::wasmer::get_executor().validate(spec) {
+            Ok(_) => return Ok(()),
+            Err(ExecutorValidationError::CantHandle(_)) => (),
+            Err(err) => return Err(err),
+        }
+        #[cfg(feature = "wasm-wasmedge")]
+        match super::wasmedge::get_executor().validate(spec) {
+            Ok(_) => return Ok(()),
+            Err(ExecutorValidationError::CantHandle(_)) => (),
+            Err(err) => return Err(err),
+        }
+        #[cfg(feature = "wasm-wasmtime")]
+        match super::wasmtime::get_executor().validate(spec) {
+            Ok(_) => return Ok(()),
+            Err(ExecutorValidationError::CantHandle(_)) => (),
+            Err(err) => return Err(err),
+        }
+
+        libcontainer::workload::default::get_executor().validate(spec)
+    }
 }
 
 pub fn default_executor() -> DefaultExecutor {