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 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"

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";
 

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<T>(error: &hcloud::apis::Error<T>) -> 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<T>(error: &hcloud::apis::Error<T>) -> 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, "")));

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<i64>) -> 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<i64>, 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<LBAlgorithm> 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));
+    }
 }