Add readonly paths tests in integration tests and runtimetest

crates/integration_test/src/main.rs

@@ -11,7 +11,7 @@ use crate::utils::support::{set_runtime_path, set_runtimetest_path};
 use anyhow::{Context, Result};
 use clap::Parser;
 use integration_test::logger;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use test_framework::TestManager;
 use tests::cgroups;
 
@@ -112,7 +112,7 @@ fn main() -> Result<()> {
     Ok(())
 }
 
-fn get_abs_path(rel_path: &PathBuf) -> PathBuf {
+fn get_abs_path(rel_path: &Path) -> PathBuf {
     match std::fs::canonicalize(rel_path) {
         // path is relative or resolved correctly
         Ok(path) => path,

crates/integration_test/src/tests/readonly_paths/readonly_paths_tests.rs

@@ -1,5 +1,5 @@
 use crate::utils::test_inside_container;
-use anyhow::bail;
+use anyhow::{anyhow, bail};
 use nix::sys::stat::SFlag;
 use oci_spec::runtime::LinuxBuilder;
 use oci_spec::runtime::{ProcessBuilder, Spec, SpecBuilder};
@@ -138,7 +138,7 @@ fn check_readonly_symlinks() -> TestResult {
 
     let spec = get_spec(ro_paths);
 
-    test_inside_container(spec, &|bundle_path| {
+    let res = test_inside_container(spec, &|bundle_path| {
         use std::{fs, io};
         let test_file = bundle_path.join(ro_symlink);
 
@@ -172,7 +172,14 @@ fn check_readonly_symlinks() -> TestResult {
                 }
             }
         }
-    })
+    });
+    if let TestResult::Passed = res {
+        TestResult::Failed(anyhow!(
+            "expected error in container creation with invalid symlink, found no error"
+        ))
+    } else {
+        TestResult::Passed
+    }
 }
 
 fn test_node(mode: u32) -> TestResult {

crates/integration_test/src/utils/test_utils.rs

@@ -109,6 +109,8 @@ pub fn get_state<P: AsRef<Path>>(id: &Uuid, dir: P) -> Result<(String, String)>
 
 pub fn start_container<P: AsRef<Path>>(id: &Uuid, dir: P) -> Result<Child> {
     let res = Command::new(get_runtime_path())
+        .stderr(Stdio::piped())
+        .stdout(Stdio::piped())
         .arg("--root")
         .arg(dir.as_ref().join("runtime"))
         .arg("start")
@@ -187,7 +189,7 @@ pub fn test_inside_container(
     // Thus to make sure the container is created, we just wait for sometime, and
     // assume that the create command was successful. If it wasn't we can catch that error
     // in the start_container, as we can not start a non-created container anyways
-    std::thread::sleep(std::time::Duration::from_millis(2000));
+    std::thread::sleep(std::time::Duration::from_millis(1000));
     match start_container(&id, &bundle).unwrap().wait_with_output() {
         Ok(c) => c,
         Err(e) => return TestResult::Failed(anyhow!("container start failed : {:?}", e)),

runtimetest/src/main.rs

@@ -4,7 +4,7 @@ mod utils;
 use oci_spec::runtime::Spec;
 use std::path::PathBuf;
 
-const SPEC_PATH: &'static str = "/config.json";
+const SPEC_PATH: &str = "/config.json";
 
 fn get_spec() -> Spec {
     let path = PathBuf::from(SPEC_PATH);

runtimetest/src/tests.rs

@@ -1,4 +1,5 @@
-use crate::utils::{test_read_access, test_write_access, AccessibilityStatus};
+use crate::utils::{test_read_access, test_write_access};
+use nix::errno::Errno;
 use oci_spec::runtime::Spec;
 
 pub fn validate_readonly_paths(spec: &Spec) {
@@ -14,43 +15,47 @@ pub fn validate_readonly_paths(spec: &Spec) {
         eprintln!("in readonly paths, expected some readonly paths to be set, found none");
         return;
     }
+    // TODO when https://github.com/rust-lang/rust/issues/86442 stabilizes,
+    // change manual matching of i32 to e.kind() and match statement
 
     for path in ro_paths {
-        match test_read_access(path) {
+        if let std::io::Result::Err(e) = test_read_access(path) {
-            std::io::Result::Err(e) => {
+            let errno = Errno::from_i32(e.raw_os_error().unwrap());
+            // In the integration tests we test for both existing and non-existing readonly paths
+            // to be specified in the spec, so we allow ENOENT here
+            if errno == Errno::ENOENT {
+                /* This is expected */
+            } else {
                 eprintln!(
                     "in readonly paths, error in testing read access for path {} : {:?}",
                     path, e
                 );
                 return;
             }
-            Ok(readability) => {
+        } else {
-                match readability {
+            /* Expected */
-                    AccessibilityStatus::Accessible => { /* This is expected */ }
-                    AccessibilityStatus::Blocked => {
-                        eprintln!("in readonly paths, path {} expected to be readable, found non readable",path);
-                        return;
-                    }
-                }
-            }
         }
-        match test_write_access(path) {
+
-            std::io::Result::Err(e) => {
+        if let std::io::Result::Err(e) = test_write_access(path) {
+            let errno = Errno::from_i32(e.raw_os_error().unwrap());
+            // In the integration tests we test for both existing and non-existing readonly paths
+            // being specified in the spec, so we allow ENOENT, and we expect EROFS as the paths
+            // should be read-only
+            if errno == Errno::ENOENT || errno == Errno::EROFS {
+                /* This is expected */
+            } else {
                 eprintln!(
                     "in readonly paths, error in testing write access for path {} : {:?}",
                     path, e
                 );
                 return;
             }
-            Ok(readability) => {
+        } else {
-                match readability {
+            eprintln!(
-                    AccessibilityStatus::Accessible => {
+                "in readonly paths, path {} expected to not be writable, found writable",
-                        eprintln!("in readonly paths, path {} expected to not be writable, found writable",path);
+                path
-                        return;
+            );
-                    }
+            return;
-                    AccessibilityStatus::Blocked => { /* This is expected */ }
-                }
-            }
         }
     }
 }

runtimetest/src/utils.rs

@@ -3,62 +3,31 @@ use std::path::PathBuf;
 use nix::sys::stat::stat;
 use nix::sys::stat::SFlag;
 
-pub enum AccessibilityStatus {
+fn test_file_read_access(path: &str) -> Result<(), std::io::Error> {
-    Accessible,
+    let _ = std::fs::OpenOptions::new()
-    Blocked,
-}
-
-fn test_file_read_access(path: &str) -> Result<AccessibilityStatus, std::io::Error> {
-    match std::fs::OpenOptions::new()
         .create(false)
         .read(true)
-        .open(path)
+        .open(path)?;
-    {
+    Ok(())
-        Ok(_) => {
-            // we can directly return accessible, as if we are allowed to open with read access,
-            // we can read the file
-            Ok(AccessibilityStatus::Accessible)
-        }
-        Err(e) => {
-            if e.kind() == std::io::ErrorKind::PermissionDenied {
-                // we can get permission denied error if we try to read a
-                // file which we do not have read read access for, in
-                // which case that is not an error, but a valid accessibility status
-                Ok(AccessibilityStatus::Blocked)
-            } else {
-                Err(e)
-            }
-        }
-    }
 }
 
-fn test_dir_read_access(path: &str) -> Result<AccessibilityStatus, std::io::Error> {
+fn test_dir_read_access(path: &str) -> Result<(), std::io::Error> {
-    match std::fs::read_dir(path) {
+    let _ = std::fs::read_dir(path)?;
-        Ok(_) => Ok(AccessibilityStatus::Accessible),
+    Ok(())
-        Err(e) => {
-            if e.kind() == std::io::ErrorKind::PermissionDenied {
-                Ok(AccessibilityStatus::Blocked)
-            } else {
-                Err(e)
-            }
-        }
-    }
 }
 
 fn is_file_like(mode: u32) -> bool {
     // for this please refer
     // https://stackoverflow.com/questions/40163270/what-is-s-isreg-and-what-does-it-do
     // https://linux.die.net/man/2/stat
-    mode & SFlag::S_IFREG.bits() != 0
+    mode & SFlag::S_IFREG.bits() != 0 || mode & SFlag::S_IFCHR.bits() != 0
-        || mode & SFlag::S_IFBLK.bits() != 0
-        || mode & SFlag::S_IFCHR.bits() != 0
 }
 
 fn is_dir(mode: u32) -> bool {
     mode & SFlag::S_IFDIR.bits() != 0
 }
 
-pub fn test_read_access(path: &str) -> Result<AccessibilityStatus, std::io::Error> {
+pub fn test_read_access(path: &str) -> Result<(), std::io::Error> {
     let fstat = stat(path)?;
     let mode = fstat.st_mode;
     if is_file_like(mode) {
@@ -77,44 +46,20 @@ pub fn test_read_access(path: &str) -> Result<AccessibilityStatus, std::io::Erro
     ))
 }
 
-fn test_file_write_access(path: &str) -> Result<AccessibilityStatus, std::io::Error> {
+fn test_file_write_access(path: &str) -> Result<(), std::io::Error> {
-    match std::fs::OpenOptions::new().write(true).open(path) {
+    let _ = std::fs::OpenOptions::new().write(true).open(path)?;
-        std::io::Result::Ok(_) => {
+    Ok(())
-            // we don't have to check if we can actually write or not, as
-            // if we are allowed to open file with write access, we can write to it
-            Ok(AccessibilityStatus::Accessible)
-        }
-        std::io::Result::Err(e) => {
-            if e.kind() == std::io::ErrorKind::PermissionDenied {
-                return Ok(AccessibilityStatus::Blocked);
-            } else {
-                return Err(e);
-            }
-        }
-    }
 }
 
-fn test_dir_write_access(path: &str) -> Result<AccessibilityStatus, std::io::Error> {
+fn test_dir_write_access(path: &str) -> Result<(), std::io::Error> {
-    match std::fs::OpenOptions::new()
+    let _ = std::fs::OpenOptions::new()
         .create(true)
         .write(true)
-        .open(PathBuf::from(path).join("test.txt"))
+        .open(PathBuf::from(path).join("test.txt"))?;
-    {
+    Ok(())
-        std::io::Result::Ok(_) => Ok(AccessibilityStatus::Accessible),
-        std::io::Result::Err(e) => {
-            if e.kind() == std::io::ErrorKind::PermissionDenied {
-                // technically we can still get permission denied even if
-                // we have write access, but do not have execute access, but by default
-                // dirs are created with execute access so that should not be an issue
-                return Ok(AccessibilityStatus::Blocked);
-            } else {
-                return Err(e);
-            }
-        }
-    }
 }
 
-pub fn test_write_access(path: &str) -> Result<AccessibilityStatus, std::io::Error> {
+pub fn test_write_access(path: &str) -> Result<(), std::io::Error> {
     let fstat = stat(path)?;
     let mode = fstat.st_mode;
     if is_file_like(mode) {