From 5ee9e0b2f9e07dc585904d38137095d95951bceb Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Tue, 26 Mar 2024 23:55:04 +0000 Subject: [PATCH 01/14] deserialize SocketAddr for dns_config --- crates/ott-common/src/discovery/dns.rs | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index f675d3095..2c03b45f5 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -1,8 +1,11 @@ +use std::{str::FromStr}; + use async_trait::async_trait; use hickory_resolver::{ config::{NameServerConfig, Protocol, ResolverConfig, ResolverOpts}, TokioAsyncResolver, }; +use serde::Deserializer; use tracing::info; use super::*; @@ -12,6 +15,7 @@ pub struct DnsDiscoveryConfig { /// The port that monoliths should be listening on for load balancer connections. pub service_port: u16, /// The DNS server to query. Optional. If not provided, the system configuration will be used instead. + #[serde(deserialize_with = "deserialize_dns_server")] pub dns_server: Option, /// The A record to query. If using docker-compose, this should be the service name for the monolith. pub query: String, @@ -21,6 +25,22 @@ pub struct DnsDiscoveryConfig { pub polling_interval: Option, } +fn deserialize_dns_server<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + let mut buf = String::deserialize(deserializer)?; + + match buf.find(':') { + None => buf.push_str(":53"), + _ => (), + } + + return Ok(Some( + SocketAddr::from_str(&buf).expect("Error: Could not unwrap"), + )); +} + pub struct DnsServiceDiscoverer { config: DnsDiscoveryConfig, } @@ -79,13 +99,13 @@ mod test { fn server_deserializes_correctly() { let json = json!({ "service_port": 8080, - "dns_server": "127.0.0.1:100", + "dns_server": "127.0.0.1", "query": "".to_string(), }); let config: DnsDiscoveryConfig = serde_json::from_value(json).expect("Failed to deserialize json"); - assert_eq!(config.dns_server, Some(([127, 0, 0, 1], 100).into())); + assert_eq!(config.dns_server, Some(([127, 0, 0, 1], 53).into())); } } From 1d00dadb2498e42770f483704fed128bb619d02c Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Wed, 27 Mar 2024 00:01:17 +0000 Subject: [PATCH 02/14] cargo fmt --- crates/ott-common/src/discovery/dns.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index 2c03b45f5..754a847c3 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -1,4 +1,4 @@ -use std::{str::FromStr}; +use std::str::FromStr; use async_trait::async_trait; use hickory_resolver::{ From 37627320b0bb6e2bcf09c5be335a6a11575409f4 Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Wed, 27 Mar 2024 00:03:39 +0000 Subject: [PATCH 03/14] clippy --- crates/ott-common/src/discovery/dns.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index 754a847c3..9bcf621e2 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -36,9 +36,9 @@ where _ => (), } - return Ok(Some( + Ok(Some( SocketAddr::from_str(&buf).expect("Error: Could not unwrap"), - )); + )) } pub struct DnsServiceDiscoverer { From 23f97e536e2e93909aaae489a9f5625e08fac908 Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Wed, 27 Mar 2024 00:06:29 +0000 Subject: [PATCH 04/14] clippy --- crates/ott-common/src/discovery/dns.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index 9bcf621e2..a286f44b8 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -31,9 +31,8 @@ where { let mut buf = String::deserialize(deserializer)?; - match buf.find(':') { - None => buf.push_str(":53"), - _ => (), + if (buf.find(':') == None) { + buf.push_str(":53"); } Ok(Some( From f490a77ef09541130379b08679320b822429fb4e Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Wed, 27 Mar 2024 00:09:34 +0000 Subject: [PATCH 05/14] clippy x3 --- crates/ott-common/src/discovery/dns.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index a286f44b8..c2ac793f9 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -31,7 +31,7 @@ where { let mut buf = String::deserialize(deserializer)?; - if (buf.find(':') == None) { + if Option::is_none(buf.find(':')) { buf.push_str(":53"); } From 8e0c2328e0eae8c89e287c6f87dafe076fdf6461 Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Wed, 27 Mar 2024 00:16:19 +0000 Subject: [PATCH 06/14] oops --- crates/ott-common/src/discovery/dns.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index c2ac793f9..163d7f3aa 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -31,7 +31,7 @@ where { let mut buf = String::deserialize(deserializer)?; - if Option::is_none(buf.find(':')) { + if Option::is_none(&buf.find(':')) { buf.push_str(":53"); } From 8ccc26157f0abc8d7cb4950798e6cc2f88a09bb3 Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Wed, 27 Mar 2024 16:11:49 +0000 Subject: [PATCH 07/14] update string parsing --- crates/ott-common/src/discovery/dns.rs | 44 ++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index 163d7f3aa..1fdf2116a 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -29,15 +29,17 @@ fn deserialize_dns_server<'de, D>(deserializer: D) -> Result, where D: Deserializer<'de>, { - let mut buf = String::deserialize(deserializer)?; + let buf = String::deserialize(deserializer)?; - if Option::is_none(&buf.find(':')) { - buf.push_str(":53"); + if buf.is_empty() { + return Ok(None); } - Ok(Some( - SocketAddr::from_str(&buf).expect("Error: Could not unwrap"), - )) + if IpAddr::from_str(&buf).is_ok() { + Ok(Some(SocketAddr::new(IpAddr::from_str(&buf).unwrap(), 53))) + } else { + Ok(Some(SocketAddr::from_str(&buf).unwrap())) + } } pub struct DnsServiceDiscoverer { @@ -95,7 +97,21 @@ mod test { use serde_json::json; #[test] - fn server_deserializes_correctly() { + fn dns_server_deserializes_correctly() { + let json = json!({ + "service_port": 8080, + "dns_server": "127.0.0.1:100", + "query": "".to_string(), + }); + + let config: DnsDiscoveryConfig = + serde_json::from_value(json).expect("Failed to deserialize json"); + + assert_eq!(config.dns_server, Some(([127, 0, 0, 1], 100).into())); + } + + #[test] + fn dns_server_deserialization_defaults_to_port_53() { let json = json!({ "service_port": 8080, "dns_server": "127.0.0.1", @@ -107,4 +123,18 @@ mod test { assert_eq!(config.dns_server, Some(([127, 0, 0, 1], 53).into())); } + + #[test] + fn dns_server_is_optional() { + let json = json!({ + "service_port": 8080, + "dns_server": "", + "query": "".to_string(), + }); + + let config: DnsDiscoveryConfig = + serde_json::from_value(json).expect("Failed to deserialize json"); + + assert_eq!(config.dns_server, None); + } } From beb288746500b35776af1a5be84847bea4397a1b Mon Sep 17 00:00:00 2001 From: Christopher Roddy <76755988+cjrkoa@users.noreply.github.com> Date: Wed, 27 Mar 2024 12:39:54 -0400 Subject: [PATCH 08/14] Update crates/ott-common/src/discovery/dns.rs Co-authored-by: Carson McManus --- crates/ott-common/src/discovery/dns.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index 1fdf2116a..8666794d5 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -128,7 +128,6 @@ mod test { fn dns_server_is_optional() { let json = json!({ "service_port": 8080, - "dns_server": "", "query": "".to_string(), }); From 7ec5ffd8b0c7d88ede9c2fa725641a28550be5af Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Wed, 27 Mar 2024 21:26:20 +0000 Subject: [PATCH 09/14] I can't get the port to be completely optional --- crates/ott-common/src/discovery/dns.rs | 39 ++++++++++++++++++++------ 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index 8666794d5..bf8491dfb 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -5,7 +5,7 @@ use hickory_resolver::{ config::{NameServerConfig, Protocol, ResolverConfig, ResolverOpts}, TokioAsyncResolver, }; -use serde::Deserializer; +use serde::{Deserializer, Serialize}; use tracing::info; use super::*; @@ -25,6 +25,25 @@ pub struct DnsDiscoveryConfig { pub polling_interval: Option, } +trait FromValue { + fn from_value(value: serde_json::Value) -> Result; +} + +impl FromValue for DnsDiscoveryConfig { + fn from_value(mut value: serde_json::Value) -> Result { + if value.get("dns_server").is_none() { + let mut kv = serde_json::Map::new(); + kv.insert( + "dns_server".to_string(), + serde_json::Value::from_str("").unwrap(), + ); + let value = value.as_object_mut().insert(&mut kv); + } + + return serde_json::from_value(value); + } +} + fn deserialize_dns_server<'de, D>(deserializer: D) -> Result, D::Error> where D: Deserializer<'de>, @@ -35,10 +54,12 @@ where return Ok(None); } - if IpAddr::from_str(&buf).is_ok() { - Ok(Some(SocketAddr::new(IpAddr::from_str(&buf).unwrap(), 53))) - } else { - Ok(Some(SocketAddr::from_str(&buf).unwrap())) + match IpAddr::from_str(&buf) { + Ok(ip) => Ok(Some(SocketAddr::new(ip, 53))), + Err(_) => match SocketAddr::from_str(&buf) { + Ok(socket) => Ok(Some(socket)), + Err(_) => Ok(None), + }, } } @@ -105,7 +126,7 @@ mod test { }); let config: DnsDiscoveryConfig = - serde_json::from_value(json).expect("Failed to deserialize json"); + DnsDiscoveryConfig::from_value(json).expect("Failed to deserialize json"); assert_eq!(config.dns_server, Some(([127, 0, 0, 1], 100).into())); } @@ -119,7 +140,7 @@ mod test { }); let config: DnsDiscoveryConfig = - serde_json::from_value(json).expect("Failed to deserialize json"); + DnsDiscoveryConfig::from_value(json).expect("Failed to deserialize json"); assert_eq!(config.dns_server, Some(([127, 0, 0, 1], 53).into())); } @@ -132,8 +153,8 @@ mod test { }); let config: DnsDiscoveryConfig = - serde_json::from_value(json).expect("Failed to deserialize json"); + DnsDiscoveryConfig::from_value(json).expect("Failed to deserialize json"); - assert_eq!(config.dns_server, None); + assert_eq!(config.dns_server.is_some(), true); } } From 4814c9e31e34ce34b1c56784645afaf84a94b259 Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Wed, 27 Mar 2024 21:40:14 +0000 Subject: [PATCH 10/14] clippy --- crates/ott-common/src/discovery/dns.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index bf8491dfb..173dac9c8 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -5,7 +5,7 @@ use hickory_resolver::{ config::{NameServerConfig, Protocol, ResolverConfig, ResolverOpts}, TokioAsyncResolver, }; -use serde::{Deserializer, Serialize}; +use serde::Deserializer; use tracing::info; use super::*; @@ -37,10 +37,10 @@ impl FromValue for DnsDiscoveryConfig { "dns_server".to_string(), serde_json::Value::from_str("").unwrap(), ); - let value = value.as_object_mut().insert(&mut kv); + let _value = value.as_object_mut().insert(&mut kv); } - return serde_json::from_value(value); + serde_json::from_value(value) } } @@ -155,6 +155,6 @@ mod test { let config: DnsDiscoveryConfig = DnsDiscoveryConfig::from_value(json).expect("Failed to deserialize json"); - assert_eq!(config.dns_server.is_some(), true); + assert_eq!(config.dns_server.is_some()); } } From 700b574975590faf735bffddaeb5e4f0112897b6 Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Thu, 28 Mar 2024 01:08:33 +0000 Subject: [PATCH 11/14] Add default parsing for dns_server --- crates/ott-common/src/discovery/dns.rs | 39 +++++++++----------------- 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index 173dac9c8..12a6b3466 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -1,4 +1,4 @@ -use std::str::FromStr; +use std::{net::Ipv4Addr, str::FromStr}; use async_trait::async_trait; use hickory_resolver::{ @@ -16,6 +16,7 @@ pub struct DnsDiscoveryConfig { pub service_port: u16, /// The DNS server to query. Optional. If not provided, the system configuration will be used instead. #[serde(deserialize_with = "deserialize_dns_server")] + #[serde(default = "default_dns_server")] pub dns_server: Option, /// The A record to query. If using docker-compose, this should be the service name for the monolith. pub query: String, @@ -25,25 +26,6 @@ pub struct DnsDiscoveryConfig { pub polling_interval: Option, } -trait FromValue { - fn from_value(value: serde_json::Value) -> Result; -} - -impl FromValue for DnsDiscoveryConfig { - fn from_value(mut value: serde_json::Value) -> Result { - if value.get("dns_server").is_none() { - let mut kv = serde_json::Map::new(); - kv.insert( - "dns_server".to_string(), - serde_json::Value::from_str("").unwrap(), - ); - let _value = value.as_object_mut().insert(&mut kv); - } - - serde_json::from_value(value) - } -} - fn deserialize_dns_server<'de, D>(deserializer: D) -> Result, D::Error> where D: Deserializer<'de>, @@ -58,11 +40,15 @@ where Ok(ip) => Ok(Some(SocketAddr::new(ip, 53))), Err(_) => match SocketAddr::from_str(&buf) { Ok(socket) => Ok(Some(socket)), - Err(_) => Ok(None), + Err(_e) => Ok(None), }, } } +fn default_dns_server() -> Option { + Some(SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 53)) +} + pub struct DnsServiceDiscoverer { config: DnsDiscoveryConfig, } @@ -126,7 +112,7 @@ mod test { }); let config: DnsDiscoveryConfig = - DnsDiscoveryConfig::from_value(json).expect("Failed to deserialize json"); + serde_json::from_value(json).expect("Failed to deserialize json"); assert_eq!(config.dns_server, Some(([127, 0, 0, 1], 100).into())); } @@ -140,7 +126,7 @@ mod test { }); let config: DnsDiscoveryConfig = - DnsDiscoveryConfig::from_value(json).expect("Failed to deserialize json"); + serde_json::from_value(json).expect("Failed to deserialize json"); assert_eq!(config.dns_server, Some(([127, 0, 0, 1], 53).into())); } @@ -153,8 +139,11 @@ mod test { }); let config: DnsDiscoveryConfig = - DnsDiscoveryConfig::from_value(json).expect("Failed to deserialize json"); + serde_json::from_value(json).expect("Failed to deserialize json"); - assert_eq!(config.dns_server.is_some()); + assert_eq!( + config.dns_server, + Some(SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 53)) + ); } } From f20614babfff0ec498b8e114e0f6013ee8000588 Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Thu, 28 Mar 2024 13:35:13 +0000 Subject: [PATCH 12/14] throw error when all deserialization attempts fail --- crates/ott-common/src/discovery/dns.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index 12a6b3466..bbb9b852b 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -40,7 +40,7 @@ where Ok(ip) => Ok(Some(SocketAddr::new(ip, 53))), Err(_) => match SocketAddr::from_str(&buf) { Ok(socket) => Ok(Some(socket)), - Err(_e) => Ok(None), + Err(e) => Err(serde::de::Error::custom(e)), }, } } @@ -146,4 +146,16 @@ mod test { Some(SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 53)) ); } + + #[test] + fn dns_server_failed_deserialization_throws_error() { + let json = json!({ + "not": "valid", + "DnsDiscoveryConfig": true, + }); + + let config: Result = serde_json::from_value(json); + + assert!(config.is_err()) + } } From 0e5f523dd57c79de481a10ecfeb712a2d113ee01 Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Thu, 28 Mar 2024 14:20:08 +0000 Subject: [PATCH 13/14] change default --- crates/ott-common/src/discovery/dns.rs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index bbb9b852b..362fb7446 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -1,4 +1,4 @@ -use std::{net::Ipv4Addr, str::FromStr}; +use std::str::FromStr; use async_trait::async_trait; use hickory_resolver::{ @@ -16,7 +16,7 @@ pub struct DnsDiscoveryConfig { pub service_port: u16, /// The DNS server to query. Optional. If not provided, the system configuration will be used instead. #[serde(deserialize_with = "deserialize_dns_server")] - #[serde(default = "default_dns_server")] + #[serde(default)] pub dns_server: Option, /// The A record to query. If using docker-compose, this should be the service name for the monolith. pub query: String, @@ -45,10 +45,6 @@ where } } -fn default_dns_server() -> Option { - Some(SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 53)) -} - pub struct DnsServiceDiscoverer { config: DnsDiscoveryConfig, } @@ -141,10 +137,7 @@ mod test { let config: DnsDiscoveryConfig = serde_json::from_value(json).expect("Failed to deserialize json"); - assert_eq!( - config.dns_server, - Some(SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 53)) - ); + assert!(config.dns_server.is_none()) } #[test] From 93d65330c533b1055b9ef571287843186c2eda6a Mon Sep 17 00:00:00 2001 From: Christopher Roddy Date: Thu, 28 Mar 2024 14:23:55 +0000 Subject: [PATCH 14/14] changes --- crates/ott-common/src/discovery/dns.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/crates/ott-common/src/discovery/dns.rs b/crates/ott-common/src/discovery/dns.rs index 362fb7446..eb5797c3f 100644 --- a/crates/ott-common/src/discovery/dns.rs +++ b/crates/ott-common/src/discovery/dns.rs @@ -32,10 +32,6 @@ where { let buf = String::deserialize(deserializer)?; - if buf.is_empty() { - return Ok(None); - } - match IpAddr::from_str(&buf) { Ok(ip) => Ok(Some(SocketAddr::new(ip, 53))), Err(_) => match SocketAddr::from_str(&buf) {