From 476637676fb1530d793ba0a0bc3f5f807c6032b6 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Wed, 30 Sep 2026 02:13:39 +0300 Subject: [PATCH 1/2] fix(lb): reject a balancer name Hetzner does not accept The name from the robotlb/balancer annotation went to Hetzner as is. Hetzner takes 1 to 128 characters matching ^\S(.*\S)?$, so a name with whitespace at an edge, a line break or more than 128 characters failed with 422 on every retry. An empty annotation was taken as the name too and went into the name lookup before the create. Check the name before the balancer is looked up by name or created, and report the rule in a warning event on the service. A service whose balancer already carries its UID label does not use the name, so it keeps being managed whatever the annotation says, as before. A release still reads no annotations. Signed-off-by: Aleksei Sviridkin Assisted-by: LLM --- src/error.rs | 13 ++++++ src/lb.rs | 109 ++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 113 insertions(+), 9 deletions(-) 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"); + } } From cfbcc9a6f633a22b1849c878a3ecbedb57695bc8 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Wed, 30 Sep 2026 02:14:01 +0300 Subject: [PATCH 2/2] chore(consts): drop the unused node IP annotation name Nothing reads robotlb/node-ip: neither the code nor the README or the Helm chart mention it, so the constant only suggests an annotation that does nothing. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin --- src/consts.rs | 1 - 1 file changed, 1 deletion(-) 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";