From f55e59d9dfeee7efb8acefc40734f10799f1f4a9 Mon Sep 17 00:00:00 2001 From: Brent Baude Date: Thu, 15 Dec 2022 08:44:18 -0600 Subject: [PATCH] Refactor to stronger typing and consolidate CONSTs Moved CONSTs that were generic to the project into a singular module. Created accessors for getting the run dir, the cache file path, and the lease file path. Signed-off-by: Brent Baude --- Cargo.toml | 4 + src/cache.rs | 29 ++-- src/client/client.rs | 4 +- src/ip.rs | 4 +- src/lib.rs | 13 +- src/proxy_conf.rs | 303 +++++++++++++++++++++++++++++++++++++++++ src/server.rs | 30 ++-- test/003-teardown.bats | 4 +- test/helpers.bash | 4 +- 9 files changed, 345 insertions(+), 50 deletions(-) create mode 100644 src/proxy_conf.rs diff --git a/Cargo.toml b/Cargo.toml index e635d22..95d1692 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,5 +35,9 @@ nv = { package = "netavark", version = "1.4"} rtnetlink = "0.11.0" ipnet = { version = "2", features = ["serde"] } +[dev-dependencies] +once_cell = "1.8.0" +rand = "0.8.5" + [build-dependencies] tonic-build = "0.8" diff --git a/src/cache.rs b/src/cache.rs index 19c05f1..8b58b65 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -3,10 +3,7 @@ use log::debug; use std::collections::HashMap; use std::fs::OpenOptions; use std::io; -use std::path::{Path, PathBuf}; - -/// Path to the lease json cache -pub const DEFAULT_CACHE_DIR: &str = "/var/tmp/"; +use std::path::PathBuf; /// LeaseCache holds the locked memory map of the mac address to a vector of leases - for multi homing /// It also stores a locked path buffer to change the FS cache @@ -29,15 +26,20 @@ impl LeaseCache { /// On success a new lease cache instance will be returned. On failure an IO error will /// be returned. /// This likely means it could not find the file directory - pub fn new(dir: String) -> Result { - let fq_path = Path::new(&dir).join("nv-leases"); - debug!("lease cache file: {:?}", fq_path.to_str().unwrap_or("")); + pub fn new(file_path: PathBuf) -> Result { + debug!( + "lease cache file: {:?}", + file_path.to_str().unwrap_or_default() + ); - OpenOptions::new().write(true).create(true).open(&fq_path)?; + OpenOptions::new() + .write(true) + .create(true) + .open(&file_path)?; Ok(LeaseCache { mem: HashMap::new(), - path: fq_path, + path: file_path, }) } @@ -148,15 +150,6 @@ impl LeaseCache { } } -impl Default for LeaseCache { - fn default() -> Self { - LeaseCache { - mem: HashMap::>::new(), - path: PathBuf::from(DEFAULT_CACHE_DIR), - } - } -} - #[cfg(test)] mod cache_tests { #[test] diff --git a/src/client/client.rs b/src/client/client.rs index 20bfee0..969fd50 100644 --- a/src/client/client.rs +++ b/src/client/client.rs @@ -4,7 +4,7 @@ use std::process; use tonic::{Code, Status}; use netavark_proxy::g_rpc::{Lease, NetworkConfig}; -use netavark_proxy::{DEFAULT_NETWORK_CONFIG, DEFAULT_UDS_PATH}; +use netavark_proxy::proxy_conf::{DEFAULT_NETWORK_CONFIG, DEFAULT_UDS_PATH}; pub mod commands; @@ -58,7 +58,7 @@ async fn main() -> Result<(), Box> { Ok(r) => r, Err(e) => { eprintln!("Error: {}", e.message()); - eprintln!("Error: {}", e); + eprintln!("Error: {e}"); process_failure(e) } }; diff --git a/src/ip.rs b/src/ip.rs index 8c03951..3ec07df 100644 --- a/src/ip.rs +++ b/src/ip.rs @@ -82,7 +82,7 @@ fn handle_gws(g: Vec, netmask: &str) -> Result, ProxyError> { }; let gw = match IpNet::new(IpAddr::from(ip), prefix as u8) { Ok(r) => r, - Err(e) => return Err(ProxyError::new(format!("{}:'{}'", e, route))), + Err(e) => return Err(ProxyError::new(format!("{e}:'{route}'"))), }; gws.push(gw); } @@ -116,7 +116,7 @@ impl Address for MacVLAN { let address = match IpAddr::from_str(&l.yiaddr) { Ok(a) => a, Err(e) => { - return Err(ProxyError::new(format!("bad address: {}", e))); + return Err(ProxyError::new(format!("bad address: {e}"))); } }; let gateways = match handle_gws(l.gateways.clone(), &l.subnet_mask) { diff --git a/src/lib.rs b/src/lib.rs index 1b311d1..96e8f1b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -4,6 +4,7 @@ use std::error::Error; pub mod cache; pub mod dhcp_service; pub mod ip; +pub mod proxy_conf; pub mod types; use crate::g_rpc::netavark_proxy_client::NetavarkProxyClient; @@ -18,16 +19,6 @@ use tonic::transport::{Channel, Endpoint}; use tonic::{Request, Status}; use tower::service_fn; -// TODO these constant destinations are not final. -// Default UDS path for gRPC to communicate on. -pub const DEFAULT_UDS_PATH: &str = "/run/podman/nv-proxy.sock"; -// Default configuration directory. -pub const DEFAULT_CONFIG_DIR: &str = ""; -// Default Network configuration path -pub const DEFAULT_NETWORK_CONFIG: &str = "/dev/stdin"; -// Default epoll wait time before dhcp socket times out -pub const DEFAULT_TIMEOUT: isize = 8; - #[allow(clippy::unwrap_used)] pub mod g_rpc { include!("../proto-build/netavark_proxy.rs"); @@ -160,7 +151,7 @@ pub mod g_rpc { use std::str::FromStr; let mut ips: Vec = Vec::new(); for i in 0..5 { - let ip = format!("10.1.{}.1", i); + let ip = format!("10.1.{i}.1"); let ipv4 = std::net::Ipv4Addr::from_str(&ip).expect("failed hard"); ips.push(ipv4); } diff --git a/src/proxy_conf.rs b/src/proxy_conf.rs new file mode 100644 index 0000000..1ebee2d --- /dev/null +++ b/src/proxy_conf.rs @@ -0,0 +1,303 @@ +// TODO these constant destinations are not final. + +use std::env; +use std::path::{Path, PathBuf}; + +// Where the cache and socket are stored by default +pub const NETAVARK_PROXY_RUN_DIR: &str = "/run/podman"; + +pub const NETAVARK_PROXY_RUN_DIR_ENV: &str = "NETAVARK_PROXY_RUN_DIR_ENV"; + +// Default UDS path for gRPC to communicate on. +pub const DEFAULT_UDS_PATH: &str = "/run/podman/nv-proxy.sock"; +// Default configuration directory. +pub const DEFAULT_CONFIG_DIR: &str = ""; +// Default Network configuration path +pub const DEFAULT_NETWORK_CONFIG: &str = "/dev/stdin"; +// Default epoll wait time before dhcp socket times out +pub const DEFAULT_TIMEOUT: isize = 8; +// Proxy server gRPC socket file name +pub const PROXY_SOCK_NAME: &str = "nv-proxy.sock"; +// Where leases are stored on the filesystem +pub const CACHE_FILE_NAME: &str = "nv-proxy.lease"; + +/// Get the RUN_DIR where the proxy cache and socket +/// are stored +/// +/// +/// # Arguments +/// +/// * `run_cli`: +/// +/// returns: String +/// +/// # Examples +/// +/// ``` +/// +/// ``` +pub fn get_run_dir(run_cli: Option<&str>) -> String { + // if environment, return it + // if opt, return it + // return default + + match env::var(NETAVARK_PROXY_RUN_DIR_ENV) { + // env::var returns an error if the key doesnt exist + Ok(v) => return v, + Err(_) => { + if let Some(val) = run_cli { + return val.to_string(); + } + } + } + NETAVARK_PROXY_RUN_DIR.to_string() +} + +/// Returns the fully qualified path of the proxy socket file including +/// the socket file name +/// +/// # Arguments +/// +/// * `run_dir_opt`: +/// +/// returns: PathBuf +/// +/// # Examples +/// +/// ``` +/// +/// ``` +pub fn get_proxy_sock_fqname(run_dir_opt: Option<&str>) -> PathBuf { + let run_dir = get_run_dir(run_dir_opt); + Path::new(&run_dir).join(PROXY_SOCK_NAME) +} + +/// Returns the fully qualified path of the cache file including the cache +/// file name +/// +/// +/// # Arguments +/// +/// * `run_dir`: +/// +/// returns: PathBuf +/// +/// # Examples +/// +/// ``` +/// +/// ``` +pub fn get_cache_fqname(run_dir: Option<&str>) -> PathBuf { + let run_dir = get_run_dir(run_dir); + Path::new(&run_dir).join(CACHE_FILE_NAME) +} + +#[cfg(test)] +mod conf_tests { + use crate::proxy_conf::{ + get_cache_fqname, get_proxy_sock_fqname, get_run_dir, CACHE_FILE_NAME, + NETAVARK_PROXY_RUN_DIR, NETAVARK_PROXY_RUN_DIR_ENV, PROXY_SOCK_NAME, + }; + use std::path::Path; + + use std::collections::HashMap; + use std::env; + use std::ffi::OsStr; + use std::hash::Hash; + use std::panic::{self, RefUnwindSafe, UnwindSafe}; + use std::sync::Mutex; + + use once_cell::sync::Lazy; + use rand::distributions::Alphanumeric; + use rand::{thread_rng, Rng}; + + /// Make sure that the environment isn't modified concurrently. + static SERIAL_TEST: Lazy> = Lazy::new(Default::default); + + /// + /// The following stanzas of code should be attributed to https://github.com/vmx/temp-env + /// + + /// The previous value is restored when the closure completes or panics, before unwinding the + /// panic. + /// + /// If `value` is set to `None`, then the environment variable is unset. + pub fn with_var(key: K, value: Option, closure: F) -> R + where + K: AsRef + Clone + Eq + Hash, + V: AsRef + Clone, + F: Fn() -> R + UnwindSafe + RefUnwindSafe, + { + with_vars(vec![(key, value)], closure) + } + + /// Unsets a single environment variable for the duration of the closure. + /// + /// The previous value is restored when the closure completes or panics, before unwinding the + /// panic. + /// + /// This is a shorthand and identical to the following: + /// ```rust + /// temp_env::with_var("MY_ENV_VAR", None::<&str>, || { + /// // Run some code where `MY_ENV_VAR` is unset. + /// }); + /// ``` + pub fn with_var_unset(key: K, closure: F) -> R + where + K: AsRef + Clone + Eq + Hash, + F: Fn() -> R + UnwindSafe + RefUnwindSafe, + { + with_var(key, None::<&str>, closure) + } + + /// Sets environment variables for the duration of the closure. + /// + /// The previous values are restored when the closure completes or panics, before unwinding the + /// panic. + /// + /// If a `value` is set to `None`, then the environment variable is unset. + /// + /// If the variable with the same name is set multiple times, the last one wins. + pub fn with_vars(kvs: Vec<(K, Option)>, closure: F) -> R + where + K: AsRef + Clone + Eq + Hash, + V: AsRef + Clone, + F: Fn() -> R + UnwindSafe + RefUnwindSafe, + { + let guard = SERIAL_TEST.lock().unwrap(); + let mut old_kvs: HashMap> = HashMap::new(); + for (key, value) in kvs { + // If the same key is given several times, the original/old value is only correct before + // the environment was updated. + if !old_kvs.contains_key(&key) { + let old_value = env::var(&key).ok(); + old_kvs.insert(key.clone(), old_value); + } + update_env(&key, value); + } + + match panic::catch_unwind(closure) { + Ok(result) => { + for (key, value) in old_kvs { + update_env(key, value); + } + result + } + Err(err) => { + for (key, value) in old_kvs { + update_env(key, value); + } + drop(guard); + panic::resume_unwind(err); + } + } + } + + fn update_env(key: K, value: Option) + where + K: AsRef, + V: AsRef, + { + match value { + Some(v) => env::set_var(key, v), + None => env::remove_var(key), + } + } + + fn random_string(len: usize) -> String { + let rand_string: String = thread_rng() + .sample_iter(&Alphanumeric) + .take(len) + .map(char::from) + .collect(); + format!("/{rand_string}") + } + + // The following tests seem to be susceptible to the environment variables poisoning + // each other when run in parallel (default rust behavior). If we set --test-threads=1, + // this will not happen. For now, I wrap the tests in `with_var_unset`. + + #[test] + fn test_run_dir_env() { + let r = random_string(25); + with_var(NETAVARK_PROXY_RUN_DIR_ENV, Some(&r), || { + assert_eq!(get_run_dir(None), r) + }); + } + + #[test] + fn test_run_dir_with_opt() { + let r = random_string(25); + with_var_unset(NETAVARK_PROXY_RUN_DIR_ENV, || { + assert_eq!(get_run_dir(Some(&r)), r) + }); + } + + #[test] + fn test_run_dir_as_none() { + with_var_unset(NETAVARK_PROXY_RUN_DIR_ENV, || { + assert_eq!(get_run_dir(None), NETAVARK_PROXY_RUN_DIR) + }); + } + + #[test] + fn test_get_cache_env() { + let r = random_string(25); + with_var(NETAVARK_PROXY_RUN_DIR_ENV, Some(&r), || { + assert_eq!(get_cache_fqname(None), Path::new(&r).join(CACHE_FILE_NAME)); + }); + } + + #[test] + fn test_get_cache_with_opt() { + let r = random_string(25); + with_var_unset(NETAVARK_PROXY_RUN_DIR_ENV, || { + assert_eq!( + get_cache_fqname(Some(&r)), + Path::new(&r).join(CACHE_FILE_NAME) + ) + }) + } + + #[test] + fn test_get_cache_as_none() { + with_var_unset(NETAVARK_PROXY_RUN_DIR_ENV, || { + assert_eq!( + get_cache_fqname(None), + Path::new(NETAVARK_PROXY_RUN_DIR).join(CACHE_FILE_NAME) + ) + }); + } + + #[test] + fn test_get_proxy_sock_env() { + let r = random_string(25); + with_var(NETAVARK_PROXY_RUN_DIR_ENV, Some(&r), || { + assert_eq!( + get_proxy_sock_fqname(None), + Path::new(&r).join(PROXY_SOCK_NAME) + ); + }); + } + + #[test] + fn test_get_proxy_sock_with_opt() { + let r = random_string(25); + with_var_unset(NETAVARK_PROXY_RUN_DIR_ENV, || { + assert_eq!( + get_proxy_sock_fqname(Some(&r)), + Path::new(&r).join(PROXY_SOCK_NAME) + ) + }) + } + + #[test] + fn test_get_proxy_sock_as_none() { + with_var_unset(NETAVARK_PROXY_RUN_DIR_ENV, || { + assert_eq!( + get_proxy_sock_fqname(None), + Path::new(NETAVARK_PROXY_RUN_DIR).join(PROXY_SOCK_NAME) + ) + }); + } +} diff --git a/src/server.rs b/src/server.rs index 09361a9..0ee2f2f 100644 --- a/src/server.rs +++ b/src/server.rs @@ -6,9 +6,10 @@ use netavark_proxy::cache::LeaseCache; use netavark_proxy::dhcp_service::DhcpService; use netavark_proxy::g_rpc::netavark_proxy_server::{NetavarkProxy, NetavarkProxyServer}; use netavark_proxy::g_rpc::{Empty, Lease as NetavarkLease, NetworkConfig, OperationResponse}; -use netavark_proxy::{ip, DEFAULT_CONFIG_DIR, DEFAULT_TIMEOUT, DEFAULT_UDS_PATH}; +use netavark_proxy::ip; +use netavark_proxy::proxy_conf::{get_cache_fqname, get_proxy_sock_fqname, DEFAULT_TIMEOUT}; use std::fs; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::str::FromStr; use std::sync::{Arc, Mutex}; #[cfg(unix)] @@ -71,7 +72,7 @@ impl NetavarkProxy for NetavarkProxyService { { return Err(Status::new( Internal, - format!("Error caching the lease: {}", e), + format!("Error caching the lease: {e}"), )); } @@ -149,7 +150,7 @@ struct Opts { /// Handle SIGINT signal. /// /// Will wait until process receives a SIGINT/ ctrl+c signal and then clean up and shut down -async fn handle_signal(uds_path: String) { +async fn handle_signal(uds_path: PathBuf) { tokio::spawn(async move { // Handle signal hooks with expect, it is important these are setup so data is not corrupted let mut sigterm = signal(SignalKind::terminate()).expect("Could not set up SIGTERM hook"); @@ -176,13 +177,15 @@ async fn handle_signal(uds_path: String) { pub async fn main() -> Result<(), Box> { env_logger::builder().format_timestamp(None).init(); let opts = Opts::parse(); - - // where we store the cache file - let conf_dir = opts.dir.unwrap_or_else(|| DEFAULT_CONFIG_DIR.to_string()); - // location of the grpc port - let uds_path = opts.uds.unwrap_or_else(|| DEFAULT_UDS_PATH.to_string()); - // timeout time if no leases are found + let optional_run_dir = opts.dir.as_deref(); let timeout = opts.timeout.unwrap_or(DEFAULT_TIMEOUT); + + let uds_path = get_proxy_sock_fqname(optional_run_dir); + debug!( + "socket path: {}", + &uds_path.clone().into_os_string().into_string().unwrap() + ); + // Create a new uds socket path match Path::new(&uds_path).parent() { None => { @@ -193,23 +196,24 @@ pub async fn main() -> Result<(), Box> { } // Watch for signals after the uds path has been created, so that the socket can be closed. handle_signal(uds_path.clone()).await; + // Bind to the UDS socket for gRPC calls let uds = UnixListener::bind(&uds_path)?; let uds_stream = UnixListenerStream::new(uds); - let cache = match LeaseCache::new(conf_dir) { + let fq_cache_path = get_cache_fqname(optional_run_dir); + let cache = match LeaseCache::new(fq_cache_path) { Ok(c) => Arc::new(Mutex::new(c)), Err(e) => { log::error!("Could not setup the cache: {}", e.to_string()); return Ok(()); } }; - // let dhcp_service = DhcpService::new() + let netavark_proxy_service = NetavarkProxyService(cache, timeout); Server::builder() .add_service(NetavarkProxyServer::new(netavark_proxy_service)) .serve_with_incoming(uds_stream) .await?; Ok(()) - //Clean up UDS on exit } diff --git a/test/003-teardown.bats b/test/003-teardown.bats index d797895..2250ab1 100644 --- a/test/003-teardown.bats +++ b/test/003-teardown.bats @@ -21,7 +21,7 @@ EOF run_setup "$input_config" # Read the lease file - run_helper cat "$TMP_TESTDIR/nv-leases" + run_helper cat "$TMP_TESTDIR/nv-proxy.lease" before=$output # Check that our mac address is in the lease file which # ensures that it was added @@ -29,7 +29,7 @@ EOF assert "$output" == "true" # Run teardown run_teardown "$input_config" - run_helper cat "$TMP_TESTDIR/nv-leases" + run_helper cat "$TMP_TESTDIR/nv-proxy.lease" # Check that the length of the lease file is now zero run_helper jq ". | length" <<<"$output" assert "$output" == 0 diff --git a/test/helpers.bash b/test/helpers.bash index 6c484dd..ef53bcd 100644 --- a/test/helpers.bash +++ b/test/helpers.bash @@ -378,7 +378,7 @@ function stop_dhcp() { } function start_proxy() { - ip netns exec "$NS_NAME" ./bin/netavark-proxy --dir "$TMP_TESTDIR" --uds "$TMP_TESTDIR/socket" & + ip netns exec "$NS_NAME" ./bin/netavark-proxy --dir "$TMP_TESTDIR" --uds "$TMP_TESTDIR" & PROXY_PID=$! } @@ -408,7 +408,7 @@ function run_teardown(){ function run_client(){ local verb=$1 local conf=$2 - run_in_container_netns "./bin/client" --uds "$TMP_TESTDIR/socket" -f "${conf}" "${verb}" "foo" + run_in_container_netns "./bin/client" --uds "$TMP_TESTDIR/nv-proxy.sock" -f "${conf}" "${verb}" "foo" } ###################