fix(nodes): route load balancer traffic according to externalTrafficPolicy

.pre-commit-config.yaml

@@ -27,6 +27,17 @@ repos:
           - clippy
           - --all
 
+      - id: test
+        types:
+          - rust
+        name: cargo test
+        language: system
+        pass_filenames: false
+        entry: cargo
+        args:
+          - test
+          - --all
+
       - id: check
         types:
           - rust

README.md

@@ -35,7 +35,20 @@ After the chart is installed, you should be able to create `LoadBalancer` servic
 
 The operator listens to the Kubernetes API for services of type `LoadBalancer` and creates Hetzner load balancers that point to nodes based on `node-ip`.
 
-Nodes are selected based on where the service's target pods are deployed, which is determined by searching for pods with the service's selector. This behavior can be configured.
+Target nodes are selected according to the service's `externalTrafficPolicy`:
+
+- `Cluster`, the Kubernetes default: every node of the cluster becomes a target, since kube-proxy forwards the traffic to a node that hosts a pod. Cordoned and not-ready nodes are left out, as they would only take up target slots.
+- `Local`: only the nodes where the service's target pods run, found through the service selector, or through the service's `EndpointSlice` resources when it has no selector.
+
+Nodes labelled `node.kubernetes.io/exclude-from-external-load-balancers`, which kubeadm puts on control-plane nodes, stay out of every balancer under either policy.
+
+Setting `ROBOTLB_DYNAMIC_NODE_SELECTOR` to `false` replaces both with the node selector from the `robotlb/node-selector` annotation.
+
+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.
+
+> 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.
 
 
 ## Configuration

src/consts.rs

@@ -22,5 +22,10 @@ pub const DEFAULT_LB_LOCATION: &str = "hel1";
 pub const DEFAULT_LB_ALGORITHM: &str = "least-connections";
 pub const DEFAULT_LB_BALANCER_TYPE: &str = "lb11";
 
+/// Well-known Kubernetes label marking a node that must never be a load balancer target.
+/// kubeadm applies it to every control-plane node.
+pub const EXCLUDE_FROM_LB_LABEL_NAME: &str =
+    "node.kubernetes.io/exclude-from-external-load-balancers";
+
 pub const FINALIZER_NAME: &str = "robotlb/finalizer";
 pub const ROBOTLB_LB_CLASS: &str = "robotlb";

src/lb.rs

@@ -300,11 +300,28 @@ impl LoadBalancer {
         &self,
         hcloud_balancer: &hcloud::models::LoadBalancer,
     ) -> RobotLBResult<()> {
+        let max_targets =
+            usize::try_from(hcloud_balancer.load_balancer_type.max_targets).unwrap_or(usize::MAX);
+        let planned = plan_targets(&self.targets, max_targets);
+        if planned.len() < self.targets.len()
+            // While a type change is in flight the balancer still reports the old type,
+            // whose limit says nothing about the type the service asked for.
+            && hcloud_balancer.load_balancer_type.name == self.balancer_type
+        {
+            tracing::warn!(
+                "Selected {} node(s), but a {} balancer holds at most {}. \
+                 Use a bigger balancer type or externalTrafficPolicy: Local.",
+                self.targets.len(),
+                hcloud_balancer.load_balancer_type.name,
+                max_targets,
+            );
+        }
+
         for target in &hcloud_balancer.targets {
             let Some(target_ip) = target.ip.clone() else {
                 continue;
             };
-            if !self.targets.contains(&target_ip.ip) {
+            if !planned.contains(&target_ip.ip.as_str()) {
                 tracing::info!("Removing target {}", target_ip.ip);
                 hcloud::apis::load_balancers_api::remove_target(
                     &self.hcloud_config,
@@ -320,27 +337,50 @@ impl LoadBalancer {
             }
         }
 
-        for ip in &self.targets {
-            if !hcloud_balancer
+        let mut live = 0_usize;
+        let mut last_error = None;
+        for ip in &planned {
+            if hcloud_balancer
                 .targets
                 .iter()
-                .any(|t| t.ip.as_ref().map(|i| i.ip.as_str()) == Some(ip))
+                .any(|t| t.ip.as_ref().map(|i| i.ip.as_str()) == Some(*ip))
             {
-                tracing::info!("Adding target {}", ip);
-                hcloud::apis::load_balancers_api::add_target(
-                    &self.hcloud_config,
-                    AddTargetParams {
-                        id: hcloud_balancer.id,
-                        body: Some(LoadBalancerAddTarget {
-                            ip: Some(Box::new(hcloud::models::LoadBalancerTargetIp {
-                                ip: ip.clone(),
-                            })),
-                            ..Default::default()
-                        }),
-                    },
-                )
-                .await?;
+                live += 1;
+                continue;
             }
+            tracing::info!("Adding target {}", ip);
+            let added = hcloud::apis::load_balancers_api::add_target(
+                &self.hcloud_config,
+                AddTargetParams {
+                    id: hcloud_balancer.id,
+                    body: Some(LoadBalancerAddTarget {
+                        ip: Some(Box::new(hcloud::models::LoadBalancerTargetIp {
+                            ip: (*ip).to_string(),
+                        })),
+                        ..Default::default()
+                    }),
+                },
+            )
+            .await;
+            // Hetzner rejects IPs outside the vSwitch subnet of the attached network,
+            // which must not keep the remaining nodes out of the load balancer.
+            match added {
+                Ok(_) => live += 1,
+                Err(error) => {
+                    tracing::warn!("Cannot add target {ip}: {error}");
+                    last_error = Some(error.to_string());
+                }
+            }
+        }
+        // A balancer left without a single target forwards nothing, so the service
+        // must not be reported as ready. Counting what is live rather than what this
+        // run attempted keeps a permanently rejected node from failing every run.
+        if !planned.is_empty() && live == 0 {
+            return Err(RobotLBError::HCloudError(format!(
+                "No target could be added to load balancer {}: {}",
+                self.name,
+                last_error.unwrap_or_else(|| "no reason reported".to_string()),
+            )));
         }
         Ok(())
     }
@@ -631,6 +671,17 @@ impl LoadBalancer {
     }
 }
 
+/// 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.
+fn plan_targets(desired: &[String], max_targets: usize) -> Vec<&str> {
+    let mut planned = desired.iter().map(String::as_str).collect::<Vec<_>>();
+    planned.sort_unstable();
+    planned.dedup();
+    planned.truncate(max_targets);
+    planned
+}
+
 impl FromStr for LBAlgorithm {
     type Err = RobotLBError;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
@@ -653,3 +704,38 @@ impl From<LBAlgorithm> for LoadBalancerAlgorithm {
         Self { r#type }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::plan_targets;
+
+    #[test]
+    fn targets_are_sorted_and_deduplicated() {
+        let desired = vec![
+            "192.168.100.4".to_string(),
+            "192.168.100.2".to_string(),
+            "192.168.100.4".to_string(),
+        ];
+        assert_eq!(
+            plan_targets(&desired, 25),
+            vec!["192.168.100.2", "192.168.100.4"]
+        );
+    }
+
+    #[test]
+    fn targets_beyond_the_balancer_limit_are_dropped() {
+        let desired = (1..=30)
+            .map(|host| format!("192.168.100.{host:03}"))
+            .collect::<Vec<_>>();
+        let planned = plan_targets(&desired, 25);
+        assert_eq!(planned.len(), 25);
+        assert_eq!(planned[0], "192.168.100.001");
+        assert_eq!(planned[24], "192.168.100.025");
+    }
+
+    #[test]
+    fn a_plan_within_the_limit_keeps_every_target() {
+        let desired = vec!["192.168.100.2".to_string(), "192.168.100.3".to_string()];
+        assert_eq!(plan_targets(&desired, 25).len(), 2);
+    }
+}