From d6c6ee0d1b02be7fe59dd479f9585ee14ec7da1a Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Tue, 29 Sep 2026 11:25:46 +0300 Subject: [PATCH] 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()); + } }