diff --git a/crates/integration_test/src/main.rs b/crates/integration_test/src/main.rs index ff1643ee..07acbff6 100644 --- a/crates/integration_test/src/main.rs +++ b/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, diff --git a/crates/integration_test/src/tests/readonly_paths/readonly_paths_tests.rs b/crates/integration_test/src/tests/readonly_paths/readonly_paths_tests.rs index a22cbbc3..84eb8a0c 100644 --- a/crates/integration_test/src/tests/readonly_paths/readonly_paths_tests.rs +++ b/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 { diff --git a/crates/integration_test/src/utils/test_utils.rs b/crates/integration_test/src/utils/test_utils.rs index d5a9bf12..af3b3188 100644 --- a/crates/integration_test/src/utils/test_utils.rs +++ b/crates/integration_test/src/utils/test_utils.rs @@ -109,6 +109,8 @@ pub fn get_state>(id: &Uuid, dir: P) -> Result<(String, String)> pub fn start_container>(id: &Uuid, dir: P) -> Result { 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)), diff --git a/runtimetest/src/main.rs b/runtimetest/src/main.rs index 0304bdc8..8c4e751a 100644 --- a/runtimetest/src/main.rs +++ b/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); diff --git a/runtimetest/src/tests.rs b/runtimetest/src/tests.rs index ff740b9f..6e37f01b 100644 --- a/runtimetest/src/tests.rs +++ b/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) { - std::io::Result::Err(e) => { + if let std::io::Result::Err(e) = test_read_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 + // 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) => { - match readability { - AccessibilityStatus::Accessible => { /* This is expected */ } - AccessibilityStatus::Blocked => { - eprintln!("in readonly paths, path {} expected to be readable, found non readable",path); - return; - } - } - } + } else { + /* Expected */ } - 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) => { - match readability { - AccessibilityStatus::Accessible => { - eprintln!("in readonly paths, path {} expected to not be writable, found writable",path); - return; - } - AccessibilityStatus::Blocked => { /* This is expected */ } - } - } + } else { + eprintln!( + "in readonly paths, path {} expected to not be writable, found writable", + path + ); + return; } } } diff --git a/runtimetest/src/utils.rs b/runtimetest/src/utils.rs index 428446db..b370be94 100644 --- a/runtimetest/src/utils.rs +++ b/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 { - Accessible, - Blocked, -} - -fn test_file_read_access(path: &str) -> Result { - match std::fs::OpenOptions::new() +fn test_file_read_access(path: &str) -> Result<(), std::io::Error> { + let _ = std::fs::OpenOptions::new() .create(false) .read(true) - .open(path) - { - 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) - } - } - } + .open(path)?; + Ok(()) } -fn test_dir_read_access(path: &str) -> Result { - match std::fs::read_dir(path) { - Ok(_) => Ok(AccessibilityStatus::Accessible), - Err(e) => { - if e.kind() == std::io::ErrorKind::PermissionDenied { - Ok(AccessibilityStatus::Blocked) - } else { - Err(e) - } - } - } +fn test_dir_read_access(path: &str) -> Result<(), std::io::Error> { + let _ = std::fs::read_dir(path)?; + Ok(()) } 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_IFBLK.bits() != 0 - || mode & SFlag::S_IFCHR.bits() != 0 + mode & SFlag::S_IFREG.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 { +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 Result { - match std::fs::OpenOptions::new().write(true).open(path) { - std::io::Result::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_file_write_access(path: &str) -> Result<(), std::io::Error> { + let _ = std::fs::OpenOptions::new().write(true).open(path)?; + Ok(()) } -fn test_dir_write_access(path: &str) -> Result { - match std::fs::OpenOptions::new() +fn test_dir_write_access(path: &str) -> Result<(), std::io::Error> { + let _ = std::fs::OpenOptions::new() .create(true) .write(true) - .open(PathBuf::from(path).join("test.txt")) - { - 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); - } - } - } + .open(PathBuf::from(path).join("test.txt"))?; + Ok(()) } -pub fn test_write_access(path: &str) -> Result { +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) {