From fa93831f66e1e05b89caa44f95e1b05aa8031198 Mon Sep 17 00:00:00 2001 From: Maksym Pavlenko Date: Tue, 19 Jan 2021 20:23:11 -0800 Subject: [PATCH] agent: Address linter and tests - Fix clippy complains - Use #[tokio::test] for async tests - Improve IPv6 check Fixes: #1294 Signed-off-by: Maksym Pavlenko --- src/agent/src/netlink.rs | 50 +++++++++++++++++++++++++++------------- src/agent/src/rpc.rs | 4 ++-- src/agent/src/sandbox.rs | 37 ++++++++++++++--------------- 3 files changed, 55 insertions(+), 36 deletions(-) diff --git a/src/agent/src/netlink.rs b/src/agent/src/netlink.rs index 202f1a9901..274487a6c6 100644 --- a/src/agent/src/netlink.rs +++ b/src/agent/src/netlink.rs @@ -1,5 +1,9 @@ -use anyhow::Context; -use anyhow::{anyhow, Result}; +// Copyright (c) 2021 Kata Maintainers +// +// SPDX-License-Identifier: Apache-2.0 +// + +use anyhow::{anyhow, Context, Result}; use futures::{future, StreamExt, TryStreamExt}; use ipnetwork::{IpNetwork, Ipv4Network, Ipv6Network}; use protobuf::RepeatedField; @@ -189,7 +193,7 @@ impl Handle { }; next.map(|msg| msg.into()) - .ok_or(anyhow!("Link not found ({})", filter)) + .ok_or_else(|| anyhow!("Link not found ({})", filter)) } async fn list_links(&self) -> Result> { @@ -313,12 +317,12 @@ impl Handle { // Split the list so we add routes with no gateway first. // Note: `partition_in_place` is a better fit here, since it reorders things inplace (instead of // allocating two separate collections), however it's not yet in stable Rust. - let (a, b): (Vec, Vec) = list.into_iter().partition(|p| p.gateway.len() == 0); + let (a, b): (Vec, Vec) = list.into_iter().partition(|p| p.gateway.is_empty()); let list = a.iter().chain(&b); for route in list { let link = self.find_link(LinkFilter::Name(&route.device)).await?; - let is_v6 = route.gateway.contains("::") || route.dest.contains("::"); + let is_v6 = is_ipv6(route.get_gateway()) || is_ipv6(route.get_dest()); const MAIN_TABLE: u8 = packet::constants::RT_TABLE_MAIN; const UNICAST: u8 = packet::constants::RTN_UNICAST; @@ -332,7 +336,7 @@ impl Handle { // This if branch is a bit clumsy because it does almost the same. // TODO: Simplify this once https://github.com/little-dude/netlink/pull/140 is merged and released if is_v6 { - let dest_addr = if route.dest.len() > 0 { + let dest_addr = if !route.dest.is_empty() { Ipv6Network::from_str(&route.dest)? } else { Ipv6Network::new(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 0), 0)? @@ -350,7 +354,7 @@ impl Handle { .destination_prefix(dest_addr.ip(), dest_addr.prefix()) .output_interface(link.index()); - if route.source.len() > 0 { + if !route.source.is_empty() { let network = Ipv6Network::from_str(&route.source)?; if network.prefix() > 0 { request = request.source_prefix(network.ip(), network.prefix()); @@ -362,7 +366,7 @@ impl Handle { } } - if route.gateway.len() > 0 { + if !route.gateway.is_empty() { let ip = Ipv6Addr::from_str(&route.gateway)?; request = request.gateway(ip); } @@ -376,7 +380,7 @@ impl Handle { ) })?; } else { - let dest_addr = if route.dest.len() > 0 { + let dest_addr = if !route.dest.is_empty() { Ipv4Network::from_str(&route.dest)? } else { Ipv4Network::new(Ipv4Addr::new(0, 0, 0, 0), 0)? @@ -394,7 +398,7 @@ impl Handle { .destination_prefix(dest_addr.ip(), dest_addr.prefix()) .output_interface(link.index()); - if route.source.len() > 0 { + if !route.source.is_empty() { let network = Ipv4Network::from_str(&route.source)?; if network.prefix() > 0 { request = request.source_prefix(network.ip(), network.prefix()); @@ -406,7 +410,7 @@ impl Handle { } } - if route.gateway.len() > 0 { + if !route.gateway.is_empty() { let ip = Ipv4Addr::from_str(&route.gateway)?; request = request.gateway(ip); } @@ -605,8 +609,12 @@ fn format_address(data: &[u8]) -> Result { } } +fn is_ipv6(str: &str) -> bool { + Ipv6Addr::from_str(str).is_ok() +} + fn parse_mac_address(addr: &str) -> Result<[u8; 6]> { - let mut split = addr.splitn(6, ":"); + let mut split = addr.splitn(6, ':'); // Parse single Mac address block let mut parse_next = || -> Result { @@ -648,7 +656,7 @@ impl Link { None } }) - .unwrap_or(String::new()) + .unwrap_or_default() } /// Extract Mac address. @@ -663,7 +671,7 @@ impl Link { None } }) - .unwrap_or(String::new()) + .unwrap_or_default() } /// Returns whether the link is UP @@ -750,7 +758,7 @@ impl Address { None } }) - .unwrap_or(String::new()) + .unwrap_or_default() } fn local(&self) -> String { @@ -765,7 +773,7 @@ impl Address { None } }) - .unwrap_or(String::new()) + .unwrap_or_default() } } @@ -939,6 +947,16 @@ mod tests { assert_eq!(bytes, [0xAB, 0x0C, 0xDE, 0x12, 0x34, 0x56]); } + #[test] + fn check_ipv6() { + assert!(is_ipv6("::1")); + assert!(is_ipv6("2001:0:3238:DFE1:63::FEFB")); + + assert!(!is_ipv6("")); + assert!(!is_ipv6("127.0.0.1")); + assert!(!is_ipv6("10.10.10.10")); + } + fn clean_env_for_test_add_one_arp_neighbor(dummy_name: &str, ip: &str) { // ip link delete dummy Command::new("ip") diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index b06e48fa4a..4a1363d73e 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -1702,8 +1702,8 @@ mod tests { assert!(result.is_ok(), "load module should success"); } - #[test] - fn test_append_guest_hooks() { + #[tokio::test] + async fn test_append_guest_hooks() { let logger = slog::Logger::root(slog::Discard, o!()); let mut s = Sandbox::new(&logger).unwrap(); s.hooks = Some(Hooks { diff --git a/src/agent/src/sandbox.rs b/src/agent/src/sandbox.rs index 5e42881281..1101afbbee 100644 --- a/src/agent/src/sandbox.rs +++ b/src/agent/src/sandbox.rs @@ -433,8 +433,8 @@ mod tests { baremount.mount() } - #[test] - fn set_sandbox_storage() { + #[tokio::test] + async fn set_sandbox_storage() { let logger = slog::Logger::root(slog::Discard, o!()); let mut s = Sandbox::new(&logger).unwrap(); @@ -467,8 +467,8 @@ mod tests { ); } - #[test] - fn remove_sandbox_storage() { + #[tokio::test] + async fn remove_sandbox_storage() { skip_if_not_root!(); let logger = slog::Logger::root(slog::Discard, o!()); @@ -523,9 +523,9 @@ mod tests { assert!(s.remove_sandbox_storage(destdir_path).is_ok()); } - #[test] + #[tokio::test] #[allow(unused_assignments)] - fn unset_and_remove_sandbox_storage() { + async fn unset_and_remove_sandbox_storage() { skip_if_not_root!(); let logger = slog::Logger::root(slog::Discard, o!()); @@ -575,8 +575,8 @@ mod tests { assert!(s.unset_and_remove_sandbox_storage(&other_dir_str).is_err()); } - #[test] - fn unset_sandbox_storage() { + #[tokio::test] + async fn unset_sandbox_storage() { let logger = slog::Logger::root(slog::Discard, o!()); let mut s = Sandbox::new(&logger).unwrap(); @@ -658,8 +658,8 @@ mod tests { .unwrap() } - #[test] - fn get_container_entry_exist() { + #[tokio::test] + async fn get_container_entry_exist() { skip_if_not_root!(); let logger = slog::Logger::root(slog::Discard, o!()); let mut s = Sandbox::new(&logger).unwrap(); @@ -671,8 +671,8 @@ mod tests { assert!(cnt.is_some()); } - #[test] - fn get_container_no_entry() { + #[tokio::test] + async fn get_container_no_entry() { let logger = slog::Logger::root(slog::Discard, o!()); let mut s = Sandbox::new(&logger).unwrap(); @@ -690,8 +690,9 @@ mod tests { s.add_container(linux_container); assert!(s.get_container("some_id").is_some()); } - #[test] - fn update_shared_pidns() { + + #[tokio::test] + async fn update_shared_pidns() { skip_if_not_root!(); let logger = slog::Logger::root(slog::Discard, o!()); let mut s = Sandbox::new(&logger).unwrap(); @@ -731,8 +732,8 @@ mod tests { assert!(s.hooks.as_ref().unwrap().poststop.is_empty()); } - #[test] - pub fn test_sandbox_is_running() { + #[tokio::test] + async fn test_sandbox_is_running() { let logger = slog::Logger::root(slog::Discard, o!()); let mut s = Sandbox::new(&logger).unwrap(); s.running = true; @@ -741,8 +742,8 @@ mod tests { assert!(!s.is_running()); } - #[test] - fn test_sandbox_set_hostname() { + #[tokio::test] + async fn test_sandbox_set_hostname() { let logger = slog::Logger::root(slog::Discard, o!()); let mut s = Sandbox::new(&logger).unwrap(); let hostname = "abc123";