From d6c6ee0d1b02be7fe59dd479f9585ee14ec7da1a Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Tue, 29 Sep 2026 11:25:46 +0300 Subject: [PATCH 1/3] fix(service): release the balancer when a service stops needing it A service changed from LoadBalancer to another type was skipped, so its Hetzner balancer kept running and its finalizer stayed. The API server clears loadBalancerClass with the type, so ownership is now decided by the finalizer: a service robotlb no longer serves has its balancer deleted and the finalizer removed, the same path a deleted service takes. The balancer to delete is found by the Hetzner ID robotlb now records on the service, since the name annotation may be gone by then. The ID is bound to the service UID, so a manifest exported and applied under another name cannot release the original's balancer. Without a recorded ID, or when no balancer has it, the balancer is looked up by name as before. A service with no TCP port that has a nodePort used to lose its external IP while the balancer behind it stayed. The balancer and the address are now kept, and the service gets a warning event instead, since a missing nodePort is more likely a mistake than a request to delete the balancer. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin --- README.md | 11 +- src/consts.rs | 2 + src/error.rs | 18 +++- src/lb.rs | 47 ++++++-- src/main.rs | 288 ++++++++++++++++++++++++++++++++++++++++---------- 5 files changed, 299 insertions(+), 67 deletions(-) diff --git a/README.md b/README.md index 02c9aae..87e1b21 100644 --- a/README.md +++ b/README.md @@ -46,10 +46,18 @@ Setting `ROBOTLB_DYNAMIC_NODE_SELECTOR` to `false` replaces both with the node s A balancer type caps how many targets it holds: `lb11`, the default type, holds 25. When more nodes are selected than the type holds, the extra ones are dropped in a stable order and a warning names the limit. Pick a bigger type through `ROBOTLB_DEFAULT_LB_TYPE` or the `robotlb/balancer-type` annotation to use the whole cluster. -Every port of the service needs an allocated `nodePort`. A Hetzner load balancer forwards traffic to the IP of a node, so a port is reachable only through its `nodePort`: ports without one are skipped, and `allocateLoadBalancerNodePorts: false` is not supported. When no port of a service can be exposed, no balancer is created for it, and a service that already advertises an external IP loses it. +robotlb records the Hetzner ID of the balancer on the service in the `robotlb/balancer-id` annotation, and deletes the balancer by that ID when the service is deleted or stops being a `LoadBalancer`, since the name annotation may be gone by then. When no balancer has that ID, the balancer is looked up by name. Services that share a balancer name share one balancer, and the ID does not change that: releasing either of them deletes it. Without the `robotlb/balancer` annotation the name is the service name without its namespace. + +Every port of the service needs an allocated `nodePort`. A Hetzner load balancer forwards traffic to the IP of a node, so a port is reachable only through its `nodePort`: ports without one are skipped, and `allocateLoadBalancerNodePorts: false` is not supported. When no port of a service can be exposed, no balancer is created for it, an existing balancer and the service's external IP are kept as they are, and a warning event on the service reports the problem. > Earlier releases treated every service as if it had the `Local` policy. Services that leave `externalTrafficPolicy` unset therefore get the full node list on upgrade, which changes the targets of their existing balancers. +> Earlier releases kept the balancer of a service whose type was changed from `LoadBalancer`, or that moved to another load balancer class. Such a service still carries the `robotlb/finalizer` finalizer, and its Hetzner balancer is deleted on the first start after the upgrade. These services have no balancer ID recorded yet, so their balancer is found by name. Without the `robotlb/balancer` annotation the name is the service name without its namespace, so a `LoadBalancer` service with the same name in another namespace may be using that balancer. List the affected services before upgrading and check their balancers: +> +> ```bash +> kubectl get services --all-namespaces --output json | jq --raw-output '.items[] | select(((.metadata.finalizers // []) | index("robotlb/finalizer")) and (.spec.type != "LoadBalancer" or (.spec.loadBalancerClass // "robotlb") != "robotlb")) | "\(.metadata.namespace)/\(.metadata.name)"' +> ``` + ## Configuration @@ -105,6 +113,7 @@ metadata: annotations: # Custom name of the balancer to create on Hetzner. Defaults to service name. robotlb/balancer: "custom name" + # robotlb/balancer-id is written by robotlb, do not set or copy it. # Hetzner cloud network. If this annotation is missing, the operator will try to # assign external IPs to the load balancer if available. Otherwise, the update won't happen. robotlb/lb-network: "my-net" diff --git a/src/consts.rs b/src/consts.rs index 0cedb34..bc659a7 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -1,4 +1,6 @@ pub const LB_NAME_LABEL_NAME: &str = "robotlb/balancer"; +/// Written by robotlb: the Hetzner ID of the balancer it created for the service. +pub const LB_ID_ANN_NAME: &str = "robotlb/balancer-id"; pub const LB_NODE_SELECTOR: &str = "robotlb/node-selector"; pub const LB_NODE_IP_LABEL_NAME: &str = "robotlb/node-ip"; diff --git a/src/error.rs b/src/error.rs index ec91801..1317eec 100644 --- a/src/error.rs +++ b/src/error.rs @@ -22,6 +22,10 @@ pub enum RobotLBError { UnknownLBAlgorithm, #[error("Cannot get target nodes, because the service has no selector")] ServiceWithoutSelector, + #[error( + "No TCP port of the service has a nodePort, so the load balancer has nothing to forward" + )] + NoExposablePorts, #[error("Hetzner Cloud API rate limit reached, the pause ends in {}s", .0.as_millis().div_ceil(1000))] RateLimited(std::time::Duration), @@ -116,11 +120,17 @@ impl RobotLBError { | Self::KubeError(_) | Self::UnknownLBAlgorithm | Self::ServiceWithoutSelector + | Self::NoExposablePorts | Self::RateLimited(_) => false, } } } +#[must_use] +pub fn is_not_found_response(error: &hcloud::apis::Error) -> bool { + matches!(error, hcloud::apis::Error::ResponseError(response) if response.status.as_u16() == 404) +} + /// Whether Hetzner answered 429 because the project ran out of API requests. #[must_use] pub fn is_rate_limit_response(error: &hcloud::apis::Error) -> bool { @@ -168,7 +178,7 @@ pub fn redact(message: &str, token: &str) -> String { #[cfg(test)] mod tests { - use super::{describe, is_rate_limit_response, redact, RobotLBError}; + use super::{describe, is_not_found_response, is_rate_limit_response, redact, RobotLBError}; use hcloud::apis::{load_balancers_api::ListLoadBalancersError, Error, ResponseContent}; const TOKEN: &str = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ01"; @@ -235,6 +245,12 @@ mod tests { assert!(error.is_rate_limited()); } + #[test] + fn only_a_404_response_is_not_found() { + assert!(is_not_found_response(&response_error(404, ""))); + assert!(!is_not_found_response(&response_error(429, ""))); + } + #[test] fn only_a_429_response_is_a_rate_limit() { assert!(is_rate_limit_response(&response_error(429, ""))); diff --git a/src/lb.rs b/src/lb.rs index e1cc5f3..cee831f 100644 --- a/src/lb.rs +++ b/src/lb.rs @@ -4,8 +4,8 @@ use hcloud::{ load_balancers_api::{ AddServiceParams, AddTargetParams, AttachLoadBalancerToNetworkParams, ChangeAlgorithmParams, ChangeTypeOfLoadBalancerParams, DeleteLoadBalancerParams, - DeleteServiceParams, DetachLoadBalancerFromNetworkParams, ListLoadBalancersParams, - RemoveTargetParams, UpdateServiceParams, + DeleteServiceParams, DetachLoadBalancerFromNetworkParams, GetLoadBalancerParams, + ListLoadBalancersParams, RemoveTargetParams, UpdateServiceParams, }, networks_api::ListNetworksParams, }, @@ -517,11 +517,29 @@ impl LoadBalancer { Ok(()) } - /// Cleanup the load balancer. - /// This method will remove all the services and targets from the - /// load balancer. - pub async fn cleanup(&self) -> RobotLBResult<()> { - let Some(hcloud_balancer) = self.get_hcloud_lb().await? else { + /// Delete the balancer of the service. The recorded ID is preferred over the name, + /// which may have changed since the balancer was found; the name is still tried + /// when no balancer has the recorded ID. + pub async fn cleanup(&self, recorded_id: Option) -> RobotLBResult<()> { + let mut hcloud_balancer = match recorded_id { + Some(id) => { + match hcloud::apis::load_balancers_api::get_load_balancer( + &self.hcloud_config, + GetLoadBalancerParams { id }, + ) + .await + { + Ok(response) => Some(*response.load_balancer), + Err(error) if crate::error::is_not_found_response(&error) => None, + Err(error) => return Err(error.into()), + } + } + None => None, + }; + if falls_back_to_name(recorded_id, hcloud_balancer.is_some()) { + hcloud_balancer = self.get_hcloud_lb().await?; + } + let Some(hcloud_balancer) = hcloud_balancer else { return Ok(()); }; for service in &hcloud_balancer.services { @@ -668,6 +686,10 @@ impl LoadBalancer { } } +const fn falls_back_to_name(recorded_id: Option, found_by_id: bool) -> bool { + recorded_id.is_none() || !found_by_id +} + /// The targets a balancer should end up with: deduplicated, and trimmed to what the /// balancer type holds. Sorted, so that a cluster larger than the limit keeps the same /// targets from one reconciliation to the next instead of trading them back and forth. @@ -704,7 +726,7 @@ impl From for LoadBalancerAlgorithm { #[cfg(test)] mod tests { - use super::plan_targets; + use super::{falls_back_to_name, plan_targets}; #[test] fn targets_are_sorted_and_deduplicated() { @@ -735,4 +757,13 @@ mod tests { let desired = vec!["192.168.100.2".to_string(), "192.168.100.3".to_string()]; assert_eq!(plan_targets(&desired, 25).len(), 2); } + + // A recorded ID goes stale when a reconcile creates a new balancer and fails + // before recording it, and the balancer under the name must still be found. + #[test] + fn a_missing_recorded_balancer_falls_back_to_the_name() { + assert!(falls_back_to_name(Some(4711), false)); + assert!(!falls_back_to_name(Some(4711), true)); + assert!(falls_back_to_name(None, false)); + } } diff --git a/src/main.rs b/src/main.rs index edcfa73..dca50da 100644 --- a/src/main.rs +++ b/src/main.rs @@ -199,26 +199,43 @@ fn event_note(error: &RobotLBError, token: &str) -> String { note } -async fn sync_service(svc: Arc, context: Arc) -> RobotLBResult { - let svc_type = svc - .spec - .as_ref() - .and_then(|s| s.type_.as_ref()) - .map(String::as_str) - .unwrap_or("ClusterIP"); - if svc_type != "LoadBalancer" { - tracing::debug!("Service type is not LoadBalancer. Skipping..."); - return Err(RobotLBError::SkipService); - } +#[derive(Debug, PartialEq, Eq)] +enum ServiceRole { + /// The service is not robotlb's. + Skip, + /// The service needs its load balancer created or updated. + Reconcile, + /// The service had a load balancer and no longer needs it. + Release, +} - let lb_type = svc +/// The API server wipes `loadBalancerClass` together with the `LoadBalancer` type, +/// so once the type changes only the finalizer still marks the service as robotlb's, +/// and a service with the finalizer that robotlb no longer serves gets released. +fn service_role(svc: &Service) -> ServiceRole { + let is_load_balancer = + svc.spec.as_ref().and_then(|s| s.type_.as_deref()) == Some("LoadBalancer"); + let class = svc .spec .as_ref() - .and_then(|s| s.load_balancer_class.as_ref()) - .map(String::as_str) + .and_then(|s| s.load_balancer_class.as_deref()) .unwrap_or(consts::ROBOTLB_LB_CLASS); - if lb_type != consts::ROBOTLB_LB_CLASS { - tracing::debug!("Load balancer class is not robotlb. Skipping..."); + let is_robotlb = is_load_balancer && class == consts::ROBOTLB_LB_CLASS; + let owned = finalizers::check(svc); + let deleting = svc.meta().deletion_timestamp.is_some(); + if (deleting && is_robotlb) || (owned && !is_robotlb) { + ServiceRole::Release + } else if is_robotlb && !deleting { + ServiceRole::Reconcile + } else { + ServiceRole::Skip + } +} + +async fn sync_service(svc: Arc, context: Arc) -> RobotLBResult { + let role = service_role(&svc); + if role == ServiceRole::Skip { + tracing::debug!("Service is not a robotlb load balancer. Skipping..."); return Err(RobotLBError::SkipService); } @@ -232,10 +249,9 @@ async fn sync_service(svc: Arc, context: Arc) -> RobotL let lb = LoadBalancer::try_from_svc(&svc, &context)?; - // If the service is being deleted, we need to clean up the resources. - if svc.meta().deletion_timestamp.is_some() { - tracing::info!("Service deletion detected. Cleaning up resources."); - lb.cleanup().await?; + if role == ServiceRole::Release { + tracing::info!("Service no longer needs a load balancer. Cleaning up resources."); + lb.cleanup(recorded_balancer_id(&svc)).await?; finalizers::remove(context.client.clone(), &svc).await?; return Ok(Action::await_change()); } @@ -516,6 +532,16 @@ pub async fn reconcile_load_balancer( lb.add_service(service.listen_port, service.target_port); } + // A balancer without a single service forwards nothing while still being billed, + // so none is created until the service has a port that can be exposed. + // An existing balancer and the address it gave the service are kept: a missing + // nodePort is far more likely a mistake than a request to delete the balancer. + if lb.services.is_empty() { + return Err(RobotLBError::NoExposablePorts); + } + + let hcloud_lb = lb.reconcile().await?; + let svc_api = kube::Api::::namespaced( context.client.clone(), svc.namespace() @@ -523,16 +549,18 @@ pub async fn reconcile_load_balancer( .as_str(), ); - // A balancer without a single service forwards nothing while still being billed, - // so none is created until the service has a port that can be exposed. - if lb.services.is_empty() { - tracing::warn!("Service has no port that can be exposed. Skipping the load balancer."); - clear_ingress_status(&svc_api, &svc).await?; - return Ok(Action::requeue(Duration::from_secs(30))); + // Releasing the balancer later goes by this ID, since by then the name annotation + // may be gone. + if let Some(patch) = balancer_id_patch(&svc, hcloud_lb.id) { + svc_api + .patch( + svc.name_any().as_str(), + &PatchParams::default(), + &kube::api::Patch::Merge(patch), + ) + .await?; } - let hcloud_lb = lb.reconcile().await?; - let mut ingress = vec![]; let dns_ipv4 = hcloud_lb.public_net.ipv4.dns_ptr.flatten(); @@ -575,33 +603,34 @@ pub async fn reconcile_load_balancer( Ok(Action::requeue(Duration::from_secs(30))) } -/// Drop the external IP a service advertises, so that nothing keeps sending -/// traffic to a load balancer that no longer forwards it. -async fn clear_ingress_status(svc_api: &kube::Api, svc: &Service) -> RobotLBResult<()> { - let advertises_ingress = svc - .status - .as_ref() - .and_then(|status| status.load_balancer.as_ref()) - .and_then(|lb| lb.ingress.as_ref()) - .is_some_and(|ingress| !ingress.is_empty()); - if !advertises_ingress { - return Ok(()); - } - tracing::info!("Removing the external IP from the service status"); - svc_api - .patch_status( - svc.name_any().as_str(), - &PatchParams::default(), - &kube::api::Patch::Merge(json!({ - "status": { - "loadBalancer": { - "ingress": null - } - } - })), - ) - .await?; - Ok(()) +/// The value is `/`: a manifest exported and applied again carries the +/// annotation along, and only the object it was written for may act on it. +fn recorded_balancer_id(svc: &Service) -> Option { + let (uid, id) = svc + .annotations() + .get(consts::LB_ID_ANN_NAME)? + .split_once('/')?; + if svc.uid().as_deref() != Some(uid) { + return None; + } + // Hetzner IDs are positive; anything else was edited by hand and must not + // make every cleanup attempt fail. + id.parse().ok().filter(|id| *id > 0) +} + +/// A patch recording the balancer ID, unless the service already carries it. +fn balancer_id_patch(svc: &Service, id: i64) -> Option { + let uid = svc.uid()?; + if recorded_balancer_id(svc) == Some(id) { + return None; + } + Some(json!({ + "metadata": { + "annotations": { + consts::LB_ID_ANN_NAME: format!("{uid}/{id}") + } + } + })) } /// Handle the error during reconcilation. @@ -630,8 +659,9 @@ fn error_action( #[cfg(test)] mod tests { use super::{ - collect_lb_services, consts, error_action, event_note, is_excluded_from_lb, - is_lb_eligible_node, is_local_traffic_policy, node_source, publishes_event, NodeSource, + balancer_id_patch, collect_lb_services, consts, error_action, event_note, + is_excluded_from_lb, is_lb_eligible_node, is_local_traffic_policy, node_source, + publishes_event, recorded_balancer_id, service_role, NodeSource, ServiceRole, }; use k8s_openapi::{ api::core::v1::{ @@ -908,4 +938,148 @@ mod tests { "boom".to_string() ))); } + + fn owned_service(type_: &str, class: Option<&str>, deleting: bool) -> Service { + Service { + metadata: ObjectMeta { + finalizers: Some(vec![consts::FINALIZER_NAME.to_string()]), + deletion_timestamp: deleting.then(|| { + k8s_openapi::apimachinery::pkg::apis::meta::v1::Time( + k8s_openapi::chrono::Utc::now(), + ) + }), + ..Default::default() + }, + spec: Some(ServiceSpec { + type_: Some(type_.to_string()), + load_balancer_class: class.map(str::to_string), + ..Default::default() + }), + ..Default::default() + } + } + + #[test] + fn a_robotlb_load_balancer_is_reconciled() { + let svc = service(ServiceSpec { + type_: Some("LoadBalancer".to_string()), + ..Default::default() + }); + assert_eq!(service_role(&svc), ServiceRole::Reconcile); + let svc = owned_service("LoadBalancer", Some(consts::ROBOTLB_LB_CLASS), false); + assert_eq!(service_role(&svc), ServiceRole::Reconcile); + } + + #[test] + fn services_robotlb_does_not_own_are_skipped() { + let other_class = service(ServiceSpec { + type_: Some("LoadBalancer".to_string()), + load_balancer_class: Some("example.com/other".to_string()), + ..Default::default() + }); + assert_eq!(service_role(&other_class), ServiceRole::Skip); + let cluster_ip = service(ServiceSpec { + type_: Some("ClusterIP".to_string()), + ..Default::default() + }); + assert_eq!(service_role(&cluster_ip), ServiceRole::Skip); + let mut deleting_cluster_ip = owned_service("ClusterIP", None, true); + deleting_cluster_ip.metadata.finalizers = None; + assert_eq!(service_role(&deleting_cluster_ip), ServiceRole::Skip); + let mut deleting_other_class = + owned_service("LoadBalancer", Some("example.com/other"), true); + deleting_other_class.metadata.finalizers = None; + assert_eq!(service_role(&deleting_other_class), ServiceRole::Skip); + } + + // A type change and a change back with another class can both land before robotlb + // reconciles, for example while the rate limit gate is closed. + #[test] + fn an_owned_service_taken_over_by_another_class_is_released() { + let svc = owned_service("LoadBalancer", Some("example.com/other"), false); + assert_eq!(service_role(&svc), ServiceRole::Release); + } + + // Reachable when the finalizer was removed by hand while another controller's + // finalizer still holds the object. + #[test] + fn a_deleted_robotlb_load_balancer_without_the_finalizer_is_released() { + let mut svc = owned_service("LoadBalancer", None, true); + svc.metadata.finalizers = None; + assert_eq!(service_role(&svc), ServiceRole::Release); + } + + #[test] + fn an_owned_service_that_stopped_being_a_load_balancer_is_released() { + assert_eq!( + service_role(&owned_service("ClusterIP", None, false)), + ServiceRole::Release + ); + } + + #[test] + fn a_deleted_owned_service_is_released() { + assert_eq!( + service_role(&owned_service("LoadBalancer", None, true)), + ServiceRole::Release + ); + assert_eq!( + service_role(&owned_service("ClusterIP", None, true)), + ServiceRole::Release + ); + } + + #[test] + fn a_service_without_exposable_ports_reports_an_event() { + let error = crate::error::RobotLBError::NoExposablePorts; + assert!(publishes_event(&error)); + assert!(error.to_string().starts_with("No TCP port")); + } + + fn annotated(value: &str) -> Service { + Service { + metadata: ObjectMeta { + uid: Some("uid-1".to_string()), + annotations: Some(BTreeMap::from([( + consts::LB_ID_ANN_NAME.to_string(), + value.to_string(), + )])), + ..Default::default() + }, + ..Default::default() + } + } + + #[test] + fn the_recorded_balancer_id_is_read_back() { + assert_eq!(recorded_balancer_id(&annotated("uid-1/4711")), Some(4711)); + assert_eq!(recorded_balancer_id(&annotated("uid-1/not-a-number")), None); + assert_eq!(recorded_balancer_id(&annotated("uid-1/0")), None); + assert_eq!(recorded_balancer_id(&annotated("uid-1/-1")), None); + assert_eq!(recorded_balancer_id(&annotated("4711")), None); + assert_eq!(recorded_balancer_id(&Service::default()), None); + } + + // A manifest exported with kubectl and applied under another name keeps the + // annotation, and must not release the original service's balancer. + #[test] + fn an_id_recorded_for_another_object_is_ignored() { + assert_eq!(recorded_balancer_id(&annotated("uid-2/4711")), None); + } + + #[test] + fn the_balancer_id_is_recorded_only_when_it_changes() { + assert!(balancer_id_patch(&annotated("uid-1/4711"), 4711).is_none()); + let patch = balancer_id_patch(&annotated("uid-1/4711"), 4712).unwrap(); + assert_eq!( + patch["metadata"]["annotations"][consts::LB_ID_ANN_NAME], + "uid-1/4712" + ); + let copied = balancer_id_patch(&annotated("uid-2/4711"), 4711).unwrap(); + assert_eq!( + copied["metadata"]["annotations"][consts::LB_ID_ANN_NAME], + "uid-1/4711" + ); + assert!(balancer_id_patch(&Service::default(), 1).is_none()); + } } From baea7e14faa831cdd487cba2c7fca966cdfe4385 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Tue, 6 Oct 2026 11:51:10 +0300 Subject: [PATCH 2/3] fix(lb): delete the balancer with a single request (#57) cleanup removed every service and every target one request at a time and only then deleted the balancer, so a balancer with 3 ports and 10 targets cost 14 API calls instead of 1. That matters when the project is close to the hourly rate limit. The API reference only says "Deletes a Load Balancer" and does not describe what happens to its services and targets. The change follows how Hetzner's own cloud controller manager and CLI delete a balancer: one delete call, with no prior removal of services or targets and no network detach. Network handling is left as it was. A failure part-way no longer leaves a balancer with some services already removed. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin --- src/lb.rs | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/src/lb.rs b/src/lb.rs index cee831f..8fa5c29 100644 --- a/src/lb.rs +++ b/src/lb.rs @@ -542,39 +542,6 @@ impl LoadBalancer { let Some(hcloud_balancer) = hcloud_balancer else { return Ok(()); }; - for service in &hcloud_balancer.services { - tracing::info!( - "Deleting service that listens for port {} from load-balancer {}", - service.listen_port, - hcloud_balancer.name, - ); - hcloud::apis::load_balancers_api::delete_service( - &self.hcloud_config, - DeleteServiceParams { - id: hcloud_balancer.id, - delete_service_request: Some(DeleteServiceRequest { - listen_port: service.listen_port, - }), - }, - ) - .await?; - } - for target in &hcloud_balancer.targets { - if let Some(target_ip) = target.ip.clone() { - tracing::info!("Removing target {}", target_ip.ip); - hcloud::apis::load_balancers_api::remove_target( - &self.hcloud_config, - RemoveTargetParams { - id: hcloud_balancer.id, - remove_target_request: Some(RemoveTargetRequest { - ip: Some(target_ip), - ..Default::default() - }), - }, - ) - .await?; - } - } hcloud::apis::load_balancers_api::delete_load_balancer( &self.hcloud_config, DeleteLoadBalancerParams { From bea258d680a565ccf0c99d49a81e267fa5c6c76e Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Tue, 6 Oct 2026 12:49:37 +0300 Subject: [PATCH 3/3] fix(service): find the released balancer by name only Recording the balancer ID on the service as an annotation adds state that users can copy between objects or edit by hand, and the operator has to guard against both. Drop the annotation and look the balancer up by name when releasing it, as robotlb did before. A balancer whose name annotation was changed or removed before the release is not found, and the README now says so. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin --- README.md | 5 ++- src/consts.rs | 2 -- src/error.rs | 13 +------ src/lb.rs | 45 ++++-------------------- src/main.rs | 97 +++------------------------------------------------ 5 files changed, 13 insertions(+), 149 deletions(-) diff --git a/README.md b/README.md index 87e1b21..7f3b221 100644 --- a/README.md +++ b/README.md @@ -46,13 +46,13 @@ Setting `ROBOTLB_DYNAMIC_NODE_SELECTOR` to `false` replaces both with the node s A balancer type caps how many targets it holds: `lb11`, the default type, holds 25. When more nodes are selected than the type holds, the extra ones are dropped in a stable order and a warning names the limit. Pick a bigger type through `ROBOTLB_DEFAULT_LB_TYPE` or the `robotlb/balancer-type` annotation to use the whole cluster. -robotlb records the Hetzner ID of the balancer on the service in the `robotlb/balancer-id` annotation, and deletes the balancer by that ID when the service is deleted or stops being a `LoadBalancer`, since the name annotation may be gone by then. When no balancer has that ID, the balancer is looked up by name. Services that share a balancer name share one balancer, and the ID does not change that: releasing either of them deletes it. Without the `robotlb/balancer` annotation the name is the service name without its namespace. +robotlb deletes the balancer when the service is deleted or stops being a `LoadBalancer`, and finds it by the name the service has at that moment. When the `robotlb/balancer` annotation was changed or removed before that, the balancer under the old name stays in the project, and the balancer under the current name is deleted, even when another service uses it. Services that share a balancer name share one balancer: releasing either of them deletes it. Without the `robotlb/balancer` annotation the name is the service name without its namespace. Every port of the service needs an allocated `nodePort`. A Hetzner load balancer forwards traffic to the IP of a node, so a port is reachable only through its `nodePort`: ports without one are skipped, and `allocateLoadBalancerNodePorts: false` is not supported. When no port of a service can be exposed, no balancer is created for it, an existing balancer and the service's external IP are kept as they are, and a warning event on the service reports the problem. > Earlier releases treated every service as if it had the `Local` policy. Services that leave `externalTrafficPolicy` unset therefore get the full node list on upgrade, which changes the targets of their existing balancers. -> Earlier releases kept the balancer of a service whose type was changed from `LoadBalancer`, or that moved to another load balancer class. Such a service still carries the `robotlb/finalizer` finalizer, and its Hetzner balancer is deleted on the first start after the upgrade. These services have no balancer ID recorded yet, so their balancer is found by name. Without the `robotlb/balancer` annotation the name is the service name without its namespace, so a `LoadBalancer` service with the same name in another namespace may be using that balancer. List the affected services before upgrading and check their balancers: +> Earlier releases kept the balancer of a service whose type was changed from `LoadBalancer`, or that moved to another load balancer class. Such a service still carries the `robotlb/finalizer` finalizer, and its Hetzner balancer is deleted on the first start after the upgrade. Its balancer is found by name. Without the `robotlb/balancer` annotation the name is the service name without its namespace, so a `LoadBalancer` service with the same name in another namespace may be using that balancer. List the affected services before upgrading and check their balancers: > > ```bash > kubectl get services --all-namespaces --output json | jq --raw-output '.items[] | select(((.metadata.finalizers // []) | index("robotlb/finalizer")) and (.spec.type != "LoadBalancer" or (.spec.loadBalancerClass // "robotlb") != "robotlb")) | "\(.metadata.namespace)/\(.metadata.name)"' @@ -113,7 +113,6 @@ metadata: annotations: # Custom name of the balancer to create on Hetzner. Defaults to service name. robotlb/balancer: "custom name" - # robotlb/balancer-id is written by robotlb, do not set or copy it. # Hetzner cloud network. If this annotation is missing, the operator will try to # assign external IPs to the load balancer if available. Otherwise, the update won't happen. robotlb/lb-network: "my-net" diff --git a/src/consts.rs b/src/consts.rs index bc659a7..0cedb34 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -1,6 +1,4 @@ pub const LB_NAME_LABEL_NAME: &str = "robotlb/balancer"; -/// Written by robotlb: the Hetzner ID of the balancer it created for the service. -pub const LB_ID_ANN_NAME: &str = "robotlb/balancer-id"; pub const LB_NODE_SELECTOR: &str = "robotlb/node-selector"; pub const LB_NODE_IP_LABEL_NAME: &str = "robotlb/node-ip"; diff --git a/src/error.rs b/src/error.rs index 1317eec..573719b 100644 --- a/src/error.rs +++ b/src/error.rs @@ -126,11 +126,6 @@ impl RobotLBError { } } -#[must_use] -pub fn is_not_found_response(error: &hcloud::apis::Error) -> bool { - matches!(error, hcloud::apis::Error::ResponseError(response) if response.status.as_u16() == 404) -} - /// Whether Hetzner answered 429 because the project ran out of API requests. #[must_use] pub fn is_rate_limit_response(error: &hcloud::apis::Error) -> bool { @@ -178,7 +173,7 @@ pub fn redact(message: &str, token: &str) -> String { #[cfg(test)] mod tests { - use super::{describe, is_not_found_response, is_rate_limit_response, redact, RobotLBError}; + use super::{describe, is_rate_limit_response, redact, RobotLBError}; use hcloud::apis::{load_balancers_api::ListLoadBalancersError, Error, ResponseContent}; const TOKEN: &str = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ01"; @@ -245,12 +240,6 @@ mod tests { assert!(error.is_rate_limited()); } - #[test] - fn only_a_404_response_is_not_found() { - assert!(is_not_found_response(&response_error(404, ""))); - assert!(!is_not_found_response(&response_error(429, ""))); - } - #[test] fn only_a_429_response_is_a_rate_limit() { assert!(is_rate_limit_response(&response_error(429, ""))); diff --git a/src/lb.rs b/src/lb.rs index 8fa5c29..2da9c40 100644 --- a/src/lb.rs +++ b/src/lb.rs @@ -4,8 +4,8 @@ use hcloud::{ load_balancers_api::{ AddServiceParams, AddTargetParams, AttachLoadBalancerToNetworkParams, ChangeAlgorithmParams, ChangeTypeOfLoadBalancerParams, DeleteLoadBalancerParams, - DeleteServiceParams, DetachLoadBalancerFromNetworkParams, GetLoadBalancerParams, - ListLoadBalancersParams, RemoveTargetParams, UpdateServiceParams, + DeleteServiceParams, DetachLoadBalancerFromNetworkParams, ListLoadBalancersParams, + RemoveTargetParams, UpdateServiceParams, }, networks_api::ListNetworksParams, }, @@ -517,29 +517,9 @@ impl LoadBalancer { Ok(()) } - /// Delete the balancer of the service. The recorded ID is preferred over the name, - /// which may have changed since the balancer was found; the name is still tried - /// when no balancer has the recorded ID. - pub async fn cleanup(&self, recorded_id: Option) -> RobotLBResult<()> { - let mut hcloud_balancer = match recorded_id { - Some(id) => { - match hcloud::apis::load_balancers_api::get_load_balancer( - &self.hcloud_config, - GetLoadBalancerParams { id }, - ) - .await - { - Ok(response) => Some(*response.load_balancer), - Err(error) if crate::error::is_not_found_response(&error) => None, - Err(error) => return Err(error.into()), - } - } - None => None, - }; - if falls_back_to_name(recorded_id, hcloud_balancer.is_some()) { - hcloud_balancer = self.get_hcloud_lb().await?; - } - let Some(hcloud_balancer) = hcloud_balancer else { + /// Delete the balancer of the service, found by its name. + pub async fn cleanup(&self) -> RobotLBResult<()> { + let Some(hcloud_balancer) = self.get_hcloud_lb().await? else { return Ok(()); }; hcloud::apis::load_balancers_api::delete_load_balancer( @@ -653,10 +633,6 @@ impl LoadBalancer { } } -const fn falls_back_to_name(recorded_id: Option, found_by_id: bool) -> bool { - recorded_id.is_none() || !found_by_id -} - /// The targets a balancer should end up with: deduplicated, and trimmed to what the /// balancer type holds. Sorted, so that a cluster larger than the limit keeps the same /// targets from one reconciliation to the next instead of trading them back and forth. @@ -693,7 +669,7 @@ impl From for LoadBalancerAlgorithm { #[cfg(test)] mod tests { - use super::{falls_back_to_name, plan_targets}; + use super::plan_targets; #[test] fn targets_are_sorted_and_deduplicated() { @@ -724,13 +700,4 @@ mod tests { let desired = vec!["192.168.100.2".to_string(), "192.168.100.3".to_string()]; assert_eq!(plan_targets(&desired, 25).len(), 2); } - - // A recorded ID goes stale when a reconcile creates a new balancer and fails - // before recording it, and the balancer under the name must still be found. - #[test] - fn a_missing_recorded_balancer_falls_back_to_the_name() { - assert!(falls_back_to_name(Some(4711), false)); - assert!(!falls_back_to_name(Some(4711), true)); - assert!(falls_back_to_name(None, false)); - } } diff --git a/src/main.rs b/src/main.rs index dca50da..e3c4475 100644 --- a/src/main.rs +++ b/src/main.rs @@ -251,7 +251,7 @@ async fn sync_service(svc: Arc, context: Arc) -> RobotL if role == ServiceRole::Release { tracing::info!("Service no longer needs a load balancer. Cleaning up resources."); - lb.cleanup(recorded_balancer_id(&svc)).await?; + lb.cleanup().await?; finalizers::remove(context.client.clone(), &svc).await?; return Ok(Action::await_change()); } @@ -549,18 +549,6 @@ pub async fn reconcile_load_balancer( .as_str(), ); - // Releasing the balancer later goes by this ID, since by then the name annotation - // may be gone. - if let Some(patch) = balancer_id_patch(&svc, hcloud_lb.id) { - svc_api - .patch( - svc.name_any().as_str(), - &PatchParams::default(), - &kube::api::Patch::Merge(patch), - ) - .await?; - } - let mut ingress = vec![]; let dns_ipv4 = hcloud_lb.public_net.ipv4.dns_ptr.flatten(); @@ -603,36 +591,6 @@ pub async fn reconcile_load_balancer( Ok(Action::requeue(Duration::from_secs(30))) } -/// The value is `/`: a manifest exported and applied again carries the -/// annotation along, and only the object it was written for may act on it. -fn recorded_balancer_id(svc: &Service) -> Option { - let (uid, id) = svc - .annotations() - .get(consts::LB_ID_ANN_NAME)? - .split_once('/')?; - if svc.uid().as_deref() != Some(uid) { - return None; - } - // Hetzner IDs are positive; anything else was edited by hand and must not - // make every cleanup attempt fail. - id.parse().ok().filter(|id| *id > 0) -} - -/// A patch recording the balancer ID, unless the service already carries it. -fn balancer_id_patch(svc: &Service, id: i64) -> Option { - let uid = svc.uid()?; - if recorded_balancer_id(svc) == Some(id) { - return None; - } - Some(json!({ - "metadata": { - "annotations": { - consts::LB_ID_ANN_NAME: format!("{uid}/{id}") - } - } - })) -} - /// Handle the error during reconcilation. #[allow(clippy::needless_pass_by_value)] fn on_error(svc: Arc, error: &RobotLBError, context: Arc) -> Action { @@ -659,9 +617,9 @@ fn error_action( #[cfg(test)] mod tests { use super::{ - balancer_id_patch, collect_lb_services, consts, error_action, event_note, - is_excluded_from_lb, is_lb_eligible_node, is_local_traffic_policy, node_source, - publishes_event, recorded_balancer_id, service_role, NodeSource, ServiceRole, + collect_lb_services, consts, error_action, event_note, is_excluded_from_lb, + is_lb_eligible_node, is_local_traffic_policy, node_source, publishes_event, service_role, + NodeSource, ServiceRole, }; use k8s_openapi::{ api::core::v1::{ @@ -1035,51 +993,4 @@ mod tests { assert!(publishes_event(&error)); assert!(error.to_string().starts_with("No TCP port")); } - - fn annotated(value: &str) -> Service { - Service { - metadata: ObjectMeta { - uid: Some("uid-1".to_string()), - annotations: Some(BTreeMap::from([( - consts::LB_ID_ANN_NAME.to_string(), - value.to_string(), - )])), - ..Default::default() - }, - ..Default::default() - } - } - - #[test] - fn the_recorded_balancer_id_is_read_back() { - assert_eq!(recorded_balancer_id(&annotated("uid-1/4711")), Some(4711)); - assert_eq!(recorded_balancer_id(&annotated("uid-1/not-a-number")), None); - assert_eq!(recorded_balancer_id(&annotated("uid-1/0")), None); - assert_eq!(recorded_balancer_id(&annotated("uid-1/-1")), None); - assert_eq!(recorded_balancer_id(&annotated("4711")), None); - assert_eq!(recorded_balancer_id(&Service::default()), None); - } - - // A manifest exported with kubectl and applied under another name keeps the - // annotation, and must not release the original service's balancer. - #[test] - fn an_id_recorded_for_another_object_is_ignored() { - assert_eq!(recorded_balancer_id(&annotated("uid-2/4711")), None); - } - - #[test] - fn the_balancer_id_is_recorded_only_when_it_changes() { - assert!(balancer_id_patch(&annotated("uid-1/4711"), 4711).is_none()); - let patch = balancer_id_patch(&annotated("uid-1/4711"), 4712).unwrap(); - assert_eq!( - patch["metadata"]["annotations"][consts::LB_ID_ANN_NAME], - "uid-1/4712" - ); - let copied = balancer_id_patch(&annotated("uid-2/4711"), 4711).unwrap(); - assert_eq!( - copied["metadata"]["annotations"][consts::LB_ID_ANN_NAME], - "uid-1/4711" - ); - assert!(balancer_id_patch(&Service::default(), 1).is_none()); - } }