diff --git a/src/consts.rs b/src/consts.rs index 5f5b01a..ca03046 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -2,7 +2,6 @@ pub const LB_NAME_LABEL_NAME: &str = "robotlb/balancer"; /// Hetzner label on every balancer robotlb manages: the UID of the service it serves. pub const LB_OWNER_LABEL: &str = "robotlb/service-uid"; pub const LB_NODE_SELECTOR: &str = "robotlb/node-selector"; -pub const LB_NODE_IP_LABEL_NAME: &str = "robotlb/node-ip"; // LB config pub const LB_CHECK_INTERVAL_ANN_NAME: &str = "robotlb/lb-check-interval"; diff --git a/src/error.rs b/src/error.rs index e82953d..ded6f26 100644 --- a/src/error.rs +++ b/src/error.rs @@ -40,6 +40,10 @@ pub enum RobotLBError { label = crate::consts::LB_OWNER_LABEL )] NoNodesToRecogniseBalancer(String), + #[error( + "The robotlb/balancer annotation needs a name Hetzner accepts: 1 to 128 characters on one line, without whitespace at the start or end. Got {0:?}" + )] + InvalidBalancerName(String), #[error("More than one load balancer matches {0}")] AmbiguousBalancer(String), #[error("Hetzner Cloud API rate limit reached, the pause ends in {}s", .0.as_millis().div_ceil(1000))] @@ -146,6 +150,7 @@ impl RobotLBError { | Self::UnrecognisedBalancer { .. } | Self::NoNodesToRecogniseBalancer(_) | Self::AmbiguousBalancer(_) + | Self::InvalidBalancerName(_) | Self::RateLimited(_) => false, } } @@ -309,4 +314,12 @@ mod tests { assert!(!error.is_rate_limited()); assert!(!error.to_string().contains("add-label")); } + + // Event notes are cut to 1024 bytes, and the name may be far longer. + #[test] + fn an_invalid_name_is_reported_after_the_rule() { + let name = "a".repeat(2000); + let message = RobotLBError::InvalidBalancerName(name.clone()).to_string(); + assert!(message.find("128 characters").unwrap() < message.find(&name).unwrap()); + } } diff --git a/src/lb.rs b/src/lb.rs index f2a61c7..f4d5ed8 100644 --- a/src/lb.rs +++ b/src/lb.rs @@ -569,7 +569,7 @@ impl LoadBalancer { if let Some(balancer) = single(labelled, &selector)? { return Ok(Some(balancer)); } - for (name, legacy) in candidate_names(purpose, &self.name, self.legacy_name.as_deref()) { + for (name, legacy) in candidate_names(purpose, &self.name, self.legacy_name.as_deref())? { let named = self .list_hcloud_lbs(ListLoadBalancersParams { name: Some(name.to_string()), @@ -710,6 +710,21 @@ fn default_name(service: &str, namespace: &str) -> String { format!("{service}.{namespace}") } +/// Hetzner checks a name against `^\S(.*\S)?$` and 1 to 128 characters and answers +/// a create with a name that fails with 422. The pattern comes from its API +/// schema, where regexes are ECMA-262: `.` does not match a line terminator. +fn validate_name(name: &str) -> RobotLBResult<()> { + // ECMA `\s` differs from Rust whitespace in U+FEFF (in) and U+0085 (out). + let not_space = |c: char| c != '\u{feff}' && (!c.is_whitespace() || c == '\u{85}'); + let edges_ok = name.starts_with(not_space) && name.ends_with(not_space); + let one_line = !name.contains(['\n', '\r', '\u{2028}', '\u{2029}']); + if edges_ok && one_line && name.chars().count() <= 128 { + Ok(()) + } else { + Err(RobotLBError::InvalidBalancerName(name.to_string())) + } +} + fn owner_selector(uid: &str) -> String { format!("{}={uid}", consts::LB_OWNER_LABEL) } @@ -733,15 +748,16 @@ fn candidate_names<'a>( purpose: Purpose, name: &'a str, legacy_name: Option<&'a str>, -) -> Vec<(&'a str, bool)> { +) -> RobotLBResult> { if purpose == Purpose::Release { - return vec![]; + return Ok(vec![]); } - legacy_name + validate_name(name)?; + Ok(legacy_name .map(|legacy| (legacy, true)) .into_iter() .chain([(name, false)]) - .collect() + .collect()) } #[derive(Debug, PartialEq, Eq)] @@ -841,7 +857,7 @@ impl From for LoadBalancerAlgorithm { mod tests { use super::{ candidate_names, decide, default_name, owner_labels, owner_selector, plan_targets, single, - Decision, LoadBalancer, Purpose, + validate_name, Decision, LoadBalancer, Purpose, }; use crate::{consts, error::RobotLBError}; use hcloud::apis::configuration::Configuration as HcloudConfig; @@ -996,17 +1012,31 @@ mod tests { #[test] fn a_release_never_looks_a_balancer_up_by_name() { - assert!(candidate_names(Purpose::Release, "web.shop", Some("web")).is_empty()); + assert!(candidate_names(Purpose::Release, "web.shop", Some("web")) + .unwrap() + .is_empty()); assert_eq!( - candidate_names(Purpose::Reconcile, "web.shop", Some("web")), + candidate_names(Purpose::Reconcile, "web.shop", Some("web")).unwrap(), vec![("web", true), ("web.shop", false)] ); assert_eq!( - candidate_names(Purpose::Reconcile, "custom", None), + candidate_names(Purpose::Reconcile, "custom", None).unwrap(), vec![("custom", false)] ); } + #[test] + fn an_invalid_name_is_never_looked_up_or_created() { + assert!(matches!( + candidate_names(Purpose::Reconcile, " web", None), + Err(RobotLBError::InvalidBalancerName(_)) + )); + // A release carries no name and must not fail on it. + assert!(candidate_names(Purpose::Release, "", None) + .unwrap() + .is_empty()); + } + #[test] fn more_than_one_match_is_an_error() { assert!(single(Vec::::new(), "x").unwrap().is_none()); @@ -1031,4 +1061,65 @@ mod tests { let lb = LoadBalancer::for_release(&svc, HcloudConfig::default()).unwrap(); assert_eq!(lb.service_uid, "uid-1"); } + + #[test] + fn a_name_hetzner_rejects_is_invalid() { + for name in [ + "", + " web", + "web ", + "\tweb", + "web\n", + "a\nb", + "\u{feff}web", + &"a".repeat(129), + ] { + assert!( + matches!( + validate_name(name), + Err(RobotLBError::InvalidBalancerName(_)) + ), + "{name:?}" + ); + } + } + + #[test] + fn a_name_hetzner_takes_is_valid() { + for name in [ + "w", + "custom name", + "web\u{85}", + &"a".repeat(128), + &"รค".repeat(128), + ] { + assert!(validate_name(name).is_ok(), "{name:?}"); + } + } + + // The name only matters when no balancer carries the service UID, so a labelled + // balancer keeps being managed whatever the annotation says. + #[tokio::test] + async fn an_invalid_balancer_name_does_not_stop_a_service_from_loading() { + use clap::Parser; + let config = + crate::config::OperatorConfig::try_parse_from(["robotlb", "--hcloud-token", "t"]) + .unwrap(); + let client = + kube::Client::try_from(kube::Config::new("http://127.0.0.1:1".parse().unwrap())) + .unwrap(); + let context = crate::CurrentContext::new(client, config, HcloudConfig::default()); + let svc = Service { + metadata: ObjectMeta { + uid: Some("uid-1".to_string()), + annotations: Some( + [(consts::LB_NAME_LABEL_NAME.to_string(), " web".to_string())].into(), + ), + ..Default::default() + }, + ..Default::default() + }; + let lb = LoadBalancer::try_from_svc(&svc, &context).unwrap(); + assert_eq!(lb.name, " web"); + } }