From dc12c4029ae303f6a2d0a7105e7b62e373a930e6 Mon Sep 17 00:00:00 2001 From: Sascha Grunert Date: Wed, 31 Aug 2022 13:42:50 +0200 Subject: [PATCH] Add listener unit tests Increase the test coverage of the listener module by adding unit tests. Signed-off-by: Sascha Grunert --- conmon-rs/server/src/attach.rs | 8 +- conmon-rs/server/src/listener.rs | 193 +++++++++++++++++++++++++++---- conmon-rs/server/src/server.rs | 4 +- conmon-rs/server/src/terminal.rs | 4 +- 4 files changed, 180 insertions(+), 29 deletions(-) diff --git a/conmon-rs/server/src/attach.rs b/conmon-rs/server/src/attach.rs index 2706feca65..826979297a 100644 --- a/conmon-rs/server/src/attach.rs +++ b/conmon-rs/server/src/attach.rs @@ -1,4 +1,7 @@ -use crate::{container_io::Pipe, listener}; +use crate::{ + container_io::Pipe, + listener::{DefaultListener, Listener}, +}; use anyhow::{bail, Context, Result}; use nix::{ errno::Errno, @@ -128,7 +131,8 @@ impl Attach { .context("bind socket")?; // keep parent_fd in scope until the bind, or else the socket will not work - let (shortened_path, _parent_dir) = listener::shorten_socket_path(path)?; + let (shortened_path, _parent_dir) = + Listener::::default().shorten_socket_path(path)?; let addr = UnixAddr::new(&shortened_path).context("create socket addr")?; bind(fd, &addr).context("bind socket fd")?; diff --git a/conmon-rs/server/src/listener.rs b/conmon-rs/server/src/listener.rs index d2fec3cfc0..5b8f703a46 100644 --- a/conmon-rs/server/src/listener.rs +++ b/conmon-rs/server/src/listener.rs @@ -1,34 +1,179 @@ use anyhow::{Context, Result}; use std::{ - fs, + fs::{self, File}, + io, os::unix::io::AsRawFd, path::{Path, PathBuf}, }; use tokio::net::UnixListener; -pub fn bind_long_path(path: &Path) -> Result { - // keep parent_fd in scope until the bind, or else the socket will not work - let (path, _parent_dir) = shorten_socket_path(path)?; - UnixListener::bind(&path).context("bind server socket") +#[cfg(test)] +use mockall::{automock, predicate::*}; + +#[derive(Debug, Default)] +/// The main structure for this module. +pub struct Listener { + imp: T, +} + +impl Listener +where + T: ListenerImpl, +{ + pub fn bind_long_path

(&self, path: P) -> Result + where + P: AsRef, + { + // keep parent_fd in scope until the bind, or else the socket will not work + let (path, _parent_dir) = self.shorten_socket_path(path)?; + self.imp.bind(path.as_ref()).context("bind server socket") + } + + pub fn shorten_socket_path

(&self, path: P) -> Result<(PathBuf, File)> + where + P: AsRef, + { + let path = path.as_ref(); + + let parent = path.parent().context(format!( + "tried to specify / as socket to bind to: {}", + path.display() + ))?; + let name = path.file_name().context(format!( + "tried to specify '..' as socket to bind to: {}", + path.display(), + ))?; + + self.imp + .create_dir_all(parent) + .context("create parent directory")?; + + let parent = self.imp.open(parent).context("open parent directory")?; + let fd = parent.as_raw_fd(); + + Ok(( + PathBuf::from("/proc/self/fd") + .join(fd.to_string()) + .join(name), + parent, + )) + } +} + +#[cfg_attr(test, automock)] +pub trait ListenerImpl { + fn bind(&self, path: &Path) -> io::Result; + fn create_dir_all(&self, path: &Path) -> io::Result<()>; + fn open(&self, path: &Path) -> io::Result; } -pub fn shorten_socket_path(path: &Path) -> Result<(PathBuf, fs::File)> { - let parent = path.parent().context(format!( - "tried to specify / as socket to bind to: {}", - path.display() - ))?; - let name = path.file_name().context(format!( - "tried to specify '..' as socket to bind to: {}", - path.display(), - ))?; - - fs::create_dir_all(parent).context("create parent directory")?; - let parent = fs::File::open(parent).context("open parent directory")?; - let fd = parent.as_raw_fd(); - Ok(( - PathBuf::from("/proc/self/fd") - .join(fd.to_string()) - .join(name), - parent, - )) +#[derive(Debug, Default)] +/// The default implementation for the Listener. +pub struct DefaultListener; + +impl ListenerImpl for DefaultListener { + fn bind(&self, path: &Path) -> io::Result { + UnixListener::bind(&path) + } + + fn create_dir_all(&self, path: &Path) -> io::Result<()> { + fs::create_dir_all(path) + } + + fn open(&self, path: &Path) -> io::Result { + File::open(path) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::io::ErrorKind; + use tempfile::{tempdir, tempfile}; + + fn new_sut(mock: MockListenerImpl) -> Listener { + Listener:: { imp: mock } + } + + fn permission_denied() -> Result { + Err(io::Error::new(ErrorKind::PermissionDenied, "")) + } + + #[tokio::test] + async fn bind_long_path_success() -> Result<()> { + let mut mock = MockListenerImpl::new(); + + mock.expect_create_dir_all().returning(|_| Ok(())); + mock.expect_open().returning(|_| tempfile()); + mock.expect_bind() + .returning(|_| UnixListener::bind(tempdir()?.path().join("foo"))); + + let sut = new_sut(mock); + let first = "foo"; + let listener = sut.bind_long_path(PathBuf::from(first).join("bar"))?; + + let addr = listener.local_addr()?; + assert!(addr.as_pathname().context("no path name")?.ends_with(first)); + + Ok(()) + } + + #[tokio::test] + async fn bind_long_path_failure_on_bind() { + let mut mock = MockListenerImpl::new(); + + mock.expect_create_dir_all().returning(|_| Ok(())); + mock.expect_open().returning(|_| tempfile()); + mock.expect_bind().returning(|_| permission_denied()); + + let sut = new_sut(mock); + assert!(sut + .bind_long_path(PathBuf::from("foo").join("bar")) + .is_err()); + } + + #[test] + fn shorten_socket_path_success() -> Result<()> { + let mut mock = MockListenerImpl::new(); + + mock.expect_create_dir_all().returning(|_| Ok(())); + mock.expect_open().returning(|_| tempfile()); + + let sut = new_sut(mock); + let last = "bar"; + let (res_file_path, res_parent) = + sut.shorten_socket_path(PathBuf::from("/foo").join(last))?; + + assert!(res_file_path.ends_with(last)); + assert!(res_file_path + .display() + .to_string() + .contains(&res_parent.as_raw_fd().to_string())); + + Ok(()) + } + + #[test] + fn shorten_socket_path_failure_on_open() { + let mut mock = MockListenerImpl::new(); + + mock.expect_create_dir_all().returning(|_| Ok(())); + mock.expect_open().returning(|_| permission_denied()); + + let sut = new_sut(mock); + + assert!(sut.shorten_socket_path("/foo/bar").is_err()); + } + + #[test] + fn shorten_socket_path_failure_on_create_dir_all() { + let mut mock = MockListenerImpl::new(); + + mock.expect_create_dir_all() + .returning(|_| permission_denied()); + + let sut = new_sut(mock); + + assert!(sut.shorten_socket_path("/foo/bar").is_err()); + } } diff --git a/conmon-rs/server/src/server.rs b/conmon-rs/server/src/server.rs index 625c236820..35d385f566 100644 --- a/conmon-rs/server/src/server.rs +++ b/conmon-rs/server/src/server.rs @@ -5,6 +5,7 @@ use crate::{ config::{CgroupManager, Config, LogDriver}, container_io::{ContainerIO, ContainerIOType}, init::{DefaultInit, Init}, + listener::{DefaultListener, Listener}, version::Version, }; use anyhow::{format_err, Context, Result}; @@ -194,7 +195,8 @@ impl Server { } async fn start_backend(self, mut shutdown_rx: oneshot::Receiver<()>) -> Result<()> { - let listener = crate::listener::bind_long_path(&self.config().socket())?; + let listener = + Listener::::default().bind_long_path(&self.config().socket())?; let client: conmon::Client = capnp_rpc::new_client(self); loop { diff --git a/conmon-rs/server/src/terminal.rs b/conmon-rs/server/src/terminal.rs index e45a474777..cd40d4b03f 100644 --- a/conmon-rs/server/src/terminal.rs +++ b/conmon-rs/server/src/terminal.rs @@ -4,7 +4,7 @@ use crate::{ attach::SharedContainerAttach, container_io::{ContainerIO, Message, Pipe}, container_log::SharedContainerLog, - listener, + listener::{DefaultListener, Listener}, }; use anyhow::{bail, format_err, Context, Result}; use getset::{Getters, MutGetters, Setters}; @@ -137,7 +137,7 @@ impl Terminal { ) -> Result<()> { let path = config.path(); debug!("Listening terminal socket on {}", path.display()); - let listener = listener::bind_long_path(path)?; + let listener = Listener::::default().bind_long_path(path)?; // Update the permissions let mut perms = fs::metadata(path).await?.permissions();