fix(service): release the balancer when a service stops needing it

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 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. 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)"'
+> ```
+
 
 ## Configuration
 

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,6 +120,7 @@ impl RobotLBError {
             | Self::KubeError(_)
             | Self::UnknownLBAlgorithm
             | Self::ServiceWithoutSelector
+            | Self::NoExposablePorts
             | Self::RateLimited(_) => false,
         }
     }

src/lb.rs

@@ -517,46 +517,11 @@ impl LoadBalancer {
         Ok(())
     }
 
-    /// Cleanup the load balancer.
-    /// This method will remove all the services and targets from the
-    /// load balancer.
+    /// 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(());
         };
-        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 {

src/main.rs

@@ -199,26 +199,43 @@ fn event_note(error: &RobotLBError, token: &str) -> String {
     note
 }
 
-async fn sync_service(svc: Arc<Service>, context: Arc<CurrentContext>) -> RobotLBResult<Action> {
-    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<Service>, context: Arc<CurrentContext>) -> RobotLBResult<Action> {
+    let role = service_role(&svc);
+    if role == ServiceRole::Skip {
+        tracing::debug!("Service is not a robotlb load balancer. Skipping...");
         return Err(RobotLBError::SkipService);
     }
 
@@ -232,9 +249,8 @@ async fn sync_service(svc: Arc<Service>, context: Arc<CurrentContext>) -> 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.");
+    if role == ServiceRole::Release {
+        tracing::info!("Service no longer needs a load balancer. Cleaning up resources.");
         lb.cleanup().await?;
         finalizers::remove(context.client.clone(), &svc).await?;
         return Ok(Action::await_change());
@@ -516,23 +532,23 @@ pub async fn reconcile_load_balancer(
         lb.add_service(service.listen_port, service.target_port);
     }
 
-    let svc_api = kube::Api::<Service>::namespaced(
-        context.client.clone(),
-        svc.namespace()
-            .unwrap_or_else(|| context.client.default_namespace().to_string())
-            .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.
+    // 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() {
-        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)));
+        return Err(RobotLBError::NoExposablePorts);
     }
 
     let hcloud_lb = lb.reconcile().await?;
 
+    let svc_api = kube::Api::<Service>::namespaced(
+        context.client.clone(),
+        svc.namespace()
+            .unwrap_or_else(|| context.client.default_namespace().to_string())
+            .as_str(),
+    );
+
     let mut ingress = vec![];
 
     let dns_ipv4 = hcloud_lb.public_net.ipv4.dns_ptr.flatten();
@@ -575,35 +591,6 @@ 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<Service>, 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(())
-}
-
 /// Handle the error during reconcilation.
 #[allow(clippy::needless_pass_by_value)]
 fn on_error(svc: Arc<Service>, error: &RobotLBError, context: Arc<CurrentContext>) -> Action {
@@ -631,7 +618,8 @@ fn error_action(
 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,
+        is_lb_eligible_node, is_local_traffic_policy, node_source, publishes_event, service_role,
+        NodeSource, ServiceRole,
     };
     use k8s_openapi::{
         api::core::v1::{
@@ -908,4 +896,101 @@ 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"));
+    }
 }