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()); + } }