From 5c6b38f7b0b237072283e65e51c3fc1119f71482 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kaan=20Barmore-Gen=C3=A7?= Date: Mon, 13 Feb 2023 00:58:23 -0500 Subject: [PATCH] Do retry update after a failure & fix tests (#94) * Do retry update after a failure & fix tests * Fix formatting * Fix clippy errors * Add codecov ignore for ip_source files --- .clippy.toml | 3 + .codecov.yml | 4 + Cargo.lock | 1 + Cargo.toml | 1 + src/ip_source/{ip_source.rs => common.rs} | 0 src/ip_source/icanhazip.rs | 4 +- src/ip_source/ipify.rs | 2 +- src/ip_source/mod.rs | 2 +- src/ip_source/seeip.rs | 2 +- src/main.rs | 133 +++++++++++++++++++--- 10 files changed, 129 insertions(+), 23 deletions(-) create mode 100644 .clippy.toml create mode 100644 .codecov.yml rename src/ip_source/{ip_source.rs => common.rs} (100%) diff --git a/.clippy.toml b/.clippy.toml new file mode 100644 index 0000000..d1c7c93 --- /dev/null +++ b/.clippy.toml @@ -0,0 +1,3 @@ +# assert_eq!(..., true) or false is a lot clearer when testing functions that +# return booleans. +bool_assert_comparison = false diff --git a/.codecov.yml b/.codecov.yml new file mode 100644 index 0000000..00f56e2 --- /dev/null +++ b/.codecov.yml @@ -0,0 +1,4 @@ +ignore: + # These are tested, but the tests hit external services which is not + # necessarily smart to do in CI, so they get skipped. + - "src/ip_source" diff --git a/Cargo.lock b/Cargo.lock index 3eab244..40a821c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -663,6 +663,7 @@ dependencies = [ "governor", "httpmock", "json", + "lazy_static", "regex", "reqwest", "serde", diff --git a/Cargo.toml b/Cargo.toml index 9351ff1..0fd18d0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,6 +38,7 @@ thiserror = "1.0.38" [dev-dependencies] httpmock = "0.6" regex = "1.6" +lazy_static = "1.4.0" [dev-dependencies.die-exit] version = "0.4" diff --git a/src/ip_source/ip_source.rs b/src/ip_source/common.rs similarity index 100% rename from src/ip_source/ip_source.rs rename to src/ip_source/common.rs diff --git a/src/ip_source/icanhazip.rs b/src/ip_source/icanhazip.rs index 9d26aac..5b56ef0 100644 --- a/src/ip_source/icanhazip.rs +++ b/src/ip_source/icanhazip.rs @@ -2,7 +2,7 @@ use async_trait::async_trait; use crate::ClientError; -use super::ip_source::IPSource; +use super::common::IPSource; pub(crate) struct IPSourceIcanhazip; @@ -34,7 +34,7 @@ impl IPSource for IPSourceIcanhazip { mod tests { use regex::Regex; - use crate::ip_source::ip_source::IPSource; + use crate::ip_source::common::IPSource; use super::IPSourceIcanhazip; diff --git a/src/ip_source/ipify.rs b/src/ip_source/ipify.rs index 2a0eb96..8d8c2ff 100644 --- a/src/ip_source/ipify.rs +++ b/src/ip_source/ipify.rs @@ -2,7 +2,7 @@ use async_trait::async_trait; use crate::ClientError; -use super::ip_source::IPSource; +use super::common::IPSource; pub(crate) struct IPSourceIpify; diff --git a/src/ip_source/mod.rs b/src/ip_source/mod.rs index fe39cc1..062df4b 100644 --- a/src/ip_source/mod.rs +++ b/src/ip_source/mod.rs @@ -1,4 +1,4 @@ +pub(crate) mod common; pub(crate) mod icanhazip; -pub(crate) mod ip_source; pub(crate) mod ipify; pub(crate) mod seeip; diff --git a/src/ip_source/seeip.rs b/src/ip_source/seeip.rs index d87c526..4976a68 100644 --- a/src/ip_source/seeip.rs +++ b/src/ip_source/seeip.rs @@ -2,7 +2,7 @@ use async_trait::async_trait; use crate::ClientError; -use super::ip_source::IPSource; +use super::common::IPSource; pub(crate) struct IPSourceSeeIP; diff --git a/src/main.rs b/src/main.rs index dcac06f..06b2c62 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,6 +1,6 @@ use crate::config::Config; use crate::gandi::GandiAPI; -use crate::ip_source::{ip_source::IPSource, ipify::IPSourceIpify}; +use crate::ip_source::{common::IPSource, ipify::IPSourceIpify}; use clap::Parser; use config::{ConfigError, IPSourceName}; use ip_source::icanhazip::IPSourceIcanhazip; @@ -57,7 +57,7 @@ pub enum ApiError { fn api_client(api_key: &str) -> Result { let client_builder = ClientBuilder::new(); - let key = format!("Apikey {}", api_key); + let key = format!("Apikey {api_key}"); let mut auth_value = header::HeaderValue::from_str(&key)?; let mut headers = header::HeaderMap::new(); auth_value.set_sensitive(true); @@ -82,6 +82,9 @@ struct ResponseFeedback { } #[derive(Deserialize)] +// Allowing dead code because this is the API response we get from Gandi. +// We don't necessarily need all the fields, but we get them anyway. +#[allow(dead_code)] struct ApiResponse { message: String, cause: Option, @@ -105,12 +108,12 @@ async fn run( let ipv6 = ipv6_result.as_ref(); println!("Found these:"); match ipv4 { - Ok(ip) => println!("\tIPv4: {}", ip), - Err(err) => eprintln!("\tIPv4 failed: {}", err), + Ok(ip) => println!("\tIPv4: {ip}"), + Err(err) => eprintln!("\tIPv4 failed: {err}"), } match ipv6 { - Ok(ip) => println!("\tIPv6: {}", ip), - Err(err) => eprintln!("\tIPv6 failed: {}", err), + Ok(ip) => println!("\tIPv6: {ip}"), + Err(err) => eprintln!("\tIPv6 failed: {err}"), } let ipv4_same = last_ipv4 @@ -122,9 +125,6 @@ async fn run( .map(|p| ipv6.map(|q| p == q).unwrap_or(false)) .unwrap_or(false); - last_ipv4 = ipv4.ok().map(|v| v.to_string()); - last_ipv6 = ipv6.ok().map(|v| v.to_string()); - if !ipv4_same || !ipv6_same || conf.always_update { let client = api_client(&conf.api_key)?; let mut tasks: Vec>> = Vec::new(); @@ -229,11 +229,11 @@ async fn run( .filter(|item| item.response.is_ok()) .count() ); - for item in results { + for item in &results { match item { Ok(value) => println!( "{}", - match value.response { + match &value.response { Ok(val) => format!( "Record '{}' ({}): {}", value.entry_name, value.entry_type, val @@ -244,9 +244,21 @@ async fn run( ), } ), - Err(err) => println!("{}", err), + Err(err) => println!("{err}"), } } + if results + .iter() + // all tasks finished OK, and all responses were OK as well + .all(|result| result.as_ref().map(|v| v.response.is_ok()).unwrap_or(false)) + { + // Only then we update the last seen IP, because we want to + // retry updates in case the last update just happened to fail + last_ipv4 = ipv4.ok().map(|v| v.to_string()); + last_ipv6 = ipv6.ok().map(|v| v.to_string()); + } else if opts.repeat.is_some() { + println!("Some operations failed. They will be retried during the next repeat.") + } } else { println!("IP address has not changed since last update"); } @@ -280,11 +292,15 @@ async fn main() -> anyhow::Result<()> { #[cfg(test)] mod tests { - use std::{env::temp_dir, time::Duration}; - - use crate::{config, ip_source::ip_source::IPSource, opts::Opts, run, ClientError}; + use crate::{config, ip_source::common::IPSource, opts::Opts, run, ClientError}; use async_trait::async_trait; use httpmock::MockServer; + use lazy_static::lazy_static; + use std::{ + env::temp_dir, + sync::atomic::{AtomicBool, Ordering::SeqCst}, + time::Duration, + }; use tokio::{fs, task::LocalSet, time::sleep}; struct IPSourceMock; @@ -322,7 +338,8 @@ mod tests { "/v5/livedns/domains/{fqdn}/records/{rname}/{rtype}" )) .body_contains("192.168.0.0"); - then.status(200); + then.status(201) + .body("{\"cause\":\"\", \"code\":201, \"message\":\"\", \"object\":\"\"}"); }); let opts = Opts { @@ -369,7 +386,8 @@ mod tests { "/v5/livedns/domains/{fqdn}/records/{rname}/{rtype}" )) .body_contains("192.168.0.0"); - then.status(200); + then.status(201) + .body("{\"cause\":\"\", \"code\":201, \"message\":\"\", \"object\":\"\"}"); }); let server_url = server.base_url(); @@ -394,6 +412,85 @@ mod tests { }); } + #[test] + fn repeat_with_failure() { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + + LocalSet::new().block_on(&runtime, async { + let mut temp = temp_dir().join("gandi-live-dns-test"); + fs::create_dir_all(&temp) + .await + .expect("Failed to create test dir"); + temp.push("test.toml"); + fs::write( + &temp, + "fqdn = \"example.com\"\napi_key = \"xxx\"\nttl = 300\n[[entry]]\nname =\"@\"\n", + ) + .await + .expect("Failed to write test config file"); + + let fqdn = "example.com"; + let rname = "@"; + let rtype = "A"; + let server = MockServer::start(); + let mock = server.mock(|when, then| { + when.method("PUT") + .path(format!( + "/v5/livedns/domains/{fqdn}/records/{rname}/{rtype}" + )) + .body_contains("192.168.0.0") + .matches(|_| { + // Don't match during the first call, but do during the second call + lazy_static! { + static ref FIRST_CALL: AtomicBool = AtomicBool::new(true); + } + if FIRST_CALL.load(SeqCst) { + FIRST_CALL.store(false, SeqCst); + return true; + } + false + }); + then.status(500) + .body("{\"cause\":\"\", \"code\":500, \"message\":\"Something went wrong\", \"object\":\"\"}"); + }); + let mock_fail = server.mock(|when, then| { + when.method("PUT") + .path(format!( + "/v5/livedns/domains/{fqdn}/records/{rname}/{rtype}" + )) + .body_contains("192.168.0.0"); + then.status(201) + .body("{\"cause\":\"\", \"code\":201, \"message\":\"\", \"object\":\"\"}"); + }); + + let server_url = server.base_url(); + let handle = tokio::task::spawn_local(async move { + let opts = Opts { + config: Some(temp.to_string_lossy().to_string()), + repeat: Some(1), + ..Opts::default() + }; + let conf = config::load_config(&opts).expect("Failed to load config"); + let ip_source: Box = Box::new(IPSourceMock); + run(&server_url, &ip_source, &conf, &opts) + .await + .expect("Failed when running the update"); + }); + + sleep(Duration::from_secs(4)).await; + handle.abort(); + + // The first call failed + mock_fail.assert(); + // We then retried since the first call failed. The retry succeeds + // so we don't retry again. + mock.assert(); + }); + } + #[test] fn repeat_always_update() { let runtime = tokio::runtime::Builder::new_current_thread() @@ -424,7 +521,7 @@ mod tests { "/v5/livedns/domains/{fqdn}/records/{rname}/{rtype}" )) .body_contains("192.168.0.0"); - then.status(200); + then.status(201).body("{\"cause\":\"\", \"code\":201, \"message\":\"\", \"object\":\"\"}"); }); let server_url = server.base_url();