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
 

helm/values.yaml

@@ -28,7 +28,7 @@ serviceAccount:
   # If not set and create is true, a name is generated using the fullname template
   name: ""
   # This is a list of cluster permissions to apply to the service account.
-  # By default it grants all permissions.
+  # The default is what robotlb needs; a custom list replaces it entirely.
   permissions:
   - apiGroups: [""]
     resources: [services, services/status]
@@ -39,6 +39,9 @@ serviceAccount:
   - apiGroups: [discovery.k8s.io]
     resources: [endpointslices]
     verbs: [get, list, watch]
+  - apiGroups: [events.k8s.io]
+    resources: [events]
+    verbs: [create]
 
 podAnnotations: {}
 podLabels: {}

src/error.rs

@@ -22,65 +22,234 @@ 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),
 
     // HCloud API errors
-    #[error("Cannot attach load balancer to a network. Reason: {0}")]
+    #[error("Cannot attach load balancer to a network. Reason: {}", describe(.0))]
     HCloudLBAttachToNetworkError(
         #[from]
         hcloud::apis::Error<hcloud::apis::load_balancers_api::AttachLoadBalancerToNetworkError>,
     ),
-    #[error("Cannot detach load balancer from network. Reason: {0}")]
+    #[error("Cannot detach load balancer from network. Reason: {}", describe(.0))]
     HcloudLBDetachFromNetworkError(
         #[from]
         hcloud::apis::Error<hcloud::apis::load_balancers_api::DetachLoadBalancerFromNetworkError>,
     ),
-    #[error("Cannot add load balancer target. Reason: {0}")]
+    #[error("Cannot add load balancer target. Reason: {}", describe(.0))]
     HcloudLBAddTargetError(
         #[from] hcloud::apis::Error<hcloud::apis::load_balancers_api::AddTargetError>,
     ),
-    #[error("Cannot remove load balancer target. Reason: {0}")]
+    #[error("Cannot remove load balancer target. Reason: {}", describe(.0))]
     HcloudLBRemoveTargetError(
         #[from] hcloud::apis::Error<hcloud::apis::load_balancers_api::RemoveTargetError>,
     ),
-    #[error("Cannot add service to load balancer. Reason: {0}")]
+    #[error("Cannot add service to load balancer. Reason: {}", describe(.0))]
     HcloudLBAddServiceError(
         #[from] hcloud::apis::Error<hcloud::apis::load_balancers_api::AddServiceError>,
     ),
-    #[error("Cannot remove service from load balancer. Reason: {0}")]
+    #[error("Cannot remove service from load balancer. Reason: {}", describe(.0))]
     HcloudLBRemoveServiceError(
         #[from] hcloud::apis::Error<hcloud::apis::load_balancers_api::DeleteServiceError>,
     ),
-    #[error("Cannot create load balancer. Reason: {0}")]
+    #[error("Cannot create load balancer. Reason: {}", describe(.0))]
     HcloudLBCreateError(
         #[from] hcloud::apis::Error<hcloud::apis::load_balancers_api::CreateLoadBalancerError>,
     ),
-    #[error("Cannot delete load balancer. Reason: {0}")]
+    #[error("Cannot delete load balancer. Reason: {}", describe(.0))]
     HcloudLBDeleteError(
         #[from] hcloud::apis::Error<hcloud::apis::load_balancers_api::DeleteLoadBalancerError>,
     ),
-    #[error("Cannot get load balancer. Reason: {0}")]
+    #[error("Cannot get load balancer. Reason: {}", describe(.0))]
     HcloudLBGetError(
         #[from] hcloud::apis::Error<hcloud::apis::load_balancers_api::GetLoadBalancerError>,
     ),
-    #[error("Cannot update service. Reason: {0}")]
+    #[error("Cannot update service. Reason: {}", describe(.0))]
     HcloudLBUpdateServiceError(
         #[from] hcloud::apis::Error<hcloud::apis::load_balancers_api::UpdateServiceError>,
     ),
-    #[error("Cannot change type of load balancer. Reason: {0}")]
+    #[error("Cannot change type of load balancer. Reason: {}", describe(.0))]
     HcloudLBChangeType(
         #[from]
         hcloud::apis::Error<hcloud::apis::load_balancers_api::ChangeTypeOfLoadBalancerError>,
     ),
-    #[error("Cannot change algorithm of load balancer. Reason: {0}")]
+    #[error("Cannot change algorithm of load balancer. Reason: {}", describe(.0))]
     HcloudLBChangeAlgorithm(
         #[from] hcloud::apis::Error<hcloud::apis::load_balancers_api::ChangeAlgorithmError>,
     ),
-    #[error("Cannot list networks. Reason: {0}")]
+    #[error("Cannot list networks. Reason: {}", describe(.0))]
     HcloudListNetworksError(
         #[from] hcloud::apis::Error<hcloud::apis::networks_api::ListNetworksError>,
     ),
-    #[error("Cannot list load balancers. Reason: {0}")]
+    #[error("Cannot list load balancers. Reason: {}", describe(.0))]
     HcloudListLoadBalancersError(
         #[from] hcloud::apis::Error<hcloud::apis::load_balancers_api::ListLoadBalancersError>,
     ),
 }
+
+impl RobotLBError {
+    /// Whether Hetzner rejected the call because the project ran out of API requests.
+    #[must_use]
+    pub fn is_rate_limited(&self) -> bool {
+        // No wildcard arm: a new variant must be sorted into one of the two groups.
+        match self {
+            Self::HCloudLBAttachToNetworkError(error) => is_rate_limit_response(error),
+            Self::HcloudLBDetachFromNetworkError(error) => is_rate_limit_response(error),
+            Self::HcloudLBAddTargetError(error) => is_rate_limit_response(error),
+            Self::HcloudLBRemoveTargetError(error) => is_rate_limit_response(error),
+            Self::HcloudLBAddServiceError(error) => is_rate_limit_response(error),
+            Self::HcloudLBRemoveServiceError(error) => is_rate_limit_response(error),
+            Self::HcloudLBCreateError(error) => is_rate_limit_response(error),
+            Self::HcloudLBDeleteError(error) => is_rate_limit_response(error),
+            Self::HcloudLBGetError(error) => is_rate_limit_response(error),
+            Self::HcloudLBUpdateServiceError(error) => is_rate_limit_response(error),
+            Self::HcloudLBChangeType(error) => is_rate_limit_response(error),
+            Self::HcloudLBChangeAlgorithm(error) => is_rate_limit_response(error),
+            Self::HcloudListNetworksError(error) => is_rate_limit_response(error),
+            Self::HcloudListLoadBalancersError(error) => is_rate_limit_response(error),
+            Self::InvalidNodeFilter(_)
+            | Self::UnsupportedServiceType
+            | Self::SkipService
+            | Self::PaseIntError(_)
+            | Self::PaseBoolError(_)
+            | Self::HCloudError(_)
+            | Self::KubeError(_)
+            | Self::UnknownLBAlgorithm
+            | Self::ServiceWithoutSelector
+            | Self::NoExposablePorts
+            | Self::RateLimited(_) => false,
+        }
+    }
+}
+
+/// 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 {
+    matches!(error, hcloud::apis::Error::ResponseError(response) if response.status.as_u16() == 429)
+}
+
+/// One line with the HTTP status and the error Hetzner reported, if the body carries one.
+#[must_use]
+pub fn describe<T>(error: &hcloud::apis::Error<T>) -> String {
+    let hcloud::apis::Error::ResponseError(response) = error else {
+        return error.to_string();
+    };
+    let body =
+        k8s_openapi::serde_json::from_str::<k8s_openapi::serde_json::Value>(&response.content).ok();
+    let reported = body.as_ref().and_then(|body| {
+        let error = body.get("error")?;
+        Some(format!(
+            "{}: {}",
+            error.get("code")?.as_str()?,
+            error.get("message")?.as_str()?
+        ))
+    });
+    reported.map_or_else(
+        || response.status.to_string(),
+        |reported| {
+            let one_line = reported.split_whitespace().collect::<Vec<_>>().join(" ");
+            format!("{}: {one_line}", response.status)
+        },
+    )
+}
+
+/// Replace the API token, or any prefix of it long enough to identify it, with a marker.
+/// Hetzner quotes the start of the token in some error messages.
+#[must_use]
+pub fn redact(message: &str, token: &str) -> String {
+    const SHORTEST_PREFIX: usize = 8;
+    let mut redacted = message.to_string();
+    for len in (SHORTEST_PREFIX..=token.len()).rev() {
+        if let Some(prefix) = token.get(..len) {
+            redacted = redacted.replace(prefix, "[REDACTED]");
+        }
+    }
+    redacted
+}
+
+#[cfg(test)]
+mod tests {
+    use super::{describe, is_rate_limit_response, redact, RobotLBError};
+    use hcloud::apis::{load_balancers_api::ListLoadBalancersError, Error, ResponseContent};
+
+    const TOKEN: &str = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ01";
+
+    fn response_error(status: u16, content: &str) -> Error<ListLoadBalancersError> {
+        Error::ResponseError(ResponseContent {
+            status: status.try_into().unwrap(),
+            content: content.to_string(),
+            entity: None,
+        })
+    }
+
+    #[test]
+    fn a_token_prefix_is_redacted() {
+        let message = format!("limit reached for token {}", &TOKEN[..32]);
+        assert_eq!(
+            redact(&message, TOKEN),
+            "limit reached for token [REDACTED]"
+        );
+    }
+
+    #[test]
+    fn the_whole_token_is_redacted() {
+        assert_eq!(redact(&format!("x{TOKEN}y"), TOKEN), "x[REDACTED]y");
+    }
+
+    #[test]
+    fn a_message_without_the_token_is_kept() {
+        assert_eq!(redact("status 500", TOKEN), "status 500");
+    }
+
+    #[test]
+    fn an_empty_token_redacts_nothing() {
+        assert_eq!(redact("status 500", ""), "status 500");
+    }
+
+    #[test]
+    fn the_hetzner_message_is_described_on_one_line() {
+        let error = response_error(
+            429,
+            r#"{"error": {"code": "rate_limit_exceeded", "message": "limit\n  reached"}}"#,
+        );
+        assert_eq!(
+            describe(&error),
+            "429 Too Many Requests: rate_limit_exceeded: limit reached"
+        );
+    }
+
+    #[test]
+    fn a_body_that_is_not_json_falls_back_to_the_status() {
+        let error = response_error(502, "<html>bad gateway</html>");
+        assert_eq!(describe(&error), "502 Bad Gateway");
+    }
+
+    #[test]
+    fn a_pause_shorter_than_a_second_is_not_reported_as_over() {
+        let error = RobotLBError::RateLimited(std::time::Duration::from_millis(300));
+        assert!(error.to_string().ends_with("in 1s"));
+    }
+
+    #[test]
+    fn a_429_is_a_rate_limit() {
+        let error = RobotLBError::from(response_error(429, ""));
+        assert!(error.is_rate_limited());
+    }
+
+    #[test]
+    fn only_a_429_response_is_a_rate_limit() {
+        assert!(is_rate_limit_response(&response_error(429, "")));
+        // Hetzner answers 422 for a target outside the vSwitch subnet.
+        assert!(!is_rate_limit_response(&response_error(422, "")));
+    }
+
+    #[test]
+    fn other_statuses_are_not_a_rate_limit() {
+        assert!(!RobotLBError::from(response_error(500, "")).is_rate_limited());
+        assert!(!RobotLBError::SkipService.is_rate_limited());
+    }
+}

src/lb.rs

@@ -366,9 +366,13 @@ impl LoadBalancer {
             // which must not keep the remaining nodes out of the load balancer.
             match added {
                 Ok(_) => live += 1,
+                // Every further call would be rejected too and only drain the budget.
+                Err(error) if crate::error::is_rate_limit_response(&error) => {
+                    return Err(error.into());
+                }
                 Err(error) => {
                     tracing::warn!("Cannot add target {ip}: {error}");
-                    last_error = Some(error.to_string());
+                    last_error = Some(crate::error::describe(&error));
                 }
             }
         }
@@ -513,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 {
@@ -619,16 +588,9 @@ impl LoadBalancer {
                 }),
             },
         )
-        .await;
-        if let Err(e) = response {
-            tracing::error!("Failed to create load balancer: {:?}", e);
-            return Err(RobotLBError::HCloudError(format!(
-                "Failed to create load balancer: {:?}",
-                e
-            )));
-        }
+        .await?;
 
-        Ok(*response.unwrap().load_balancer)
+        Ok(*response.load_balancer)
     }
 
     /// Get the network from Hetzner Cloud.