fix(lb): act only on balancers labelled for the service

README.md

@@ -46,13 +46,21 @@ 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.
 
-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.
+robotlb labels every balancer it creates with `robotlb/service-uid` set to the UID of the service, finds the balancer of a service by that label, and changes or deletes only a balancer labelled for that service. A balancer is never deleted by its name. Without the `robotlb/balancer` annotation the balancer is named `<service>.<namespace>`. The name is only used to create the balancer: changing the annotation later does not rename it. When the name is taken by a balancer labelled for another service, or by an unlabelled balancer that does not target the nodes of the service, robotlb leaves that balancer alone and reports it in a warning event on the service. Two services therefore cannot share a balancer through the same `robotlb/balancer` annotation: the second one gets the warning. A service recreated with a new UID, for example restored from a backup, is another service to robotlb, because its old balancer is labelled with the old UID. When that balancer has the name the service asks for, the warning names the old UID. When it has the name of an earlier release, robotlb creates a new balancer with a new IP instead, without a warning, and the old one stays in the project and keeps being billed; `hcloud load-balancer list --selector robotlb/service-uid=<old UID>` finds it. After checking that no service with the old UID is left, hand the balancer over with `hcloud load-balancer add-label --overwrite '<balancer>' robotlb/service-uid=<new UID>`, or delete it. An unlabelled balancer whose targets are all IPs, at least one of them a node of the service, is adopted: robotlb adds its label, keeps the name, and from then on manages and deletes it like its own. This is how balancers of earlier releases are taken over, and it applies to any balancer created later under that name that matches.
 
 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:
+> Earlier releases created balancers without a label and named them after the service without its namespace. On the first reconcile after the upgrade, a service with no labelled balancer adopts the balancer under its old name, or under the name from its `robotlb/balancer` annotation, if every target of that balancer is an IP and at least one of them is a node of the service: robotlb adds its label and keeps the name. Otherwise the balancer is left alone: under the old name robotlb creates a new balancer with a new IP under `<service>.<namespace>`, under an annotated name the service gets a warning event instead. Services with the same name in different namespaces used to share one balancer: one of them keeps it, the others get a new balancer and a new IP. When they reconcile at the same moment, the others may configure the shared balancer and report its address once more before they get their own. A balancer without targets, for example because Hetzner refused every node, does not look like the service's. Under the old name it is replaced by a new balancer with a new IP and stays in the project, unlabelled and billed; under an annotated name the service gets a warning event. To hand an unlabelled balancer to a service yourself, label it: `hcloud load-balancer add-label '<balancer>' robotlb/service-uid=<service UID>`. While a service has no target nodes, for example a `Local` service without ready pods, robotlb cannot tell whether an unlabelled balancer is its own, so it waits and reports that in a warning event.
+>
+> A balancer is deleted only when it carries the label, so the balancer of a service deleted or changed from `LoadBalancer` before its first successful reconcile on this release stays in the project: a reconcile that stops early, for example because the service has no port to expose or no target nodes, adopts nothing. This includes services that earlier releases kept a balancer for after their type changed from `LoadBalancer` or they moved to another load balancer class: they still carry the `robotlb/finalizer` finalizer, robotlb removes it on the first start, and their balancers stay. List the balancers without the label, check which of them are still used, and delete the rest:
+>
+> ```bash
+> hcloud load-balancer list --selector '!robotlb/service-uid'
+> ```
+>
+> The services that lose their finalizer this way can be listed before the upgrade:
 >
 > ```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)"'
@@ -111,9 +119,8 @@ kind: Service
 metadata:
   name: target
   annotations:
-    # Custom name of the balancer to create on Hetzner. Defaults to service name.
+    # Name of the balancer to create on Hetzner. Defaults to <service>.<namespace>.
     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,6 +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";
+/// Hetzner label on every balancer robotlb manages: the UID of the service it serves.
+pub const LB_OWNER_LABEL: &str = "robotlb/service-uid";
 pub const LB_NODE_SELECTOR: &str = "robotlb/node-selector";
 pub const LB_NODE_IP_LABEL_NAME: &str = "robotlb/node-ip";
 

src/error.rs

@@ -26,6 +26,22 @@ pub enum RobotLBError {
         "No TCP port of the service has a nodePort, so the load balancer has nothing to forward"
     )]
     NoExposablePorts,
+    #[error(
+        "Load balancer '{name}' is labelled for the service with UID {owner}, so robotlb leaves it alone. Set another name through the robotlb/balancer annotation"
+    )]
+    ForeignBalancer { name: String, owner: String },
+    #[error(
+        "Load balancer '{name}' has no {label} label and does not look like a balancer robotlb made for this service (IP targets only, at least one of them a node of the service), so robotlb leaves it alone. If it belongs to this service: hcloud load-balancer add-label '{name}' {label}={uid}",
+        label = crate::consts::LB_OWNER_LABEL
+    )]
+    UnrecognisedBalancer { name: String, uid: String },
+    #[error(
+        "Load balancer '{0}' has no {label} label, and whether it belongs to this service cannot be told before the service has target nodes",
+        label = crate::consts::LB_OWNER_LABEL
+    )]
+    NoNodesToRecogniseBalancer(String),
+    #[error("More than one load balancer matches {0}")]
+    AmbiguousBalancer(String),
     #[error("Hetzner Cloud API rate limit reached, the pause ends in {}s", .0.as_millis().div_ceil(1000))]
     RateLimited(std::time::Duration),
 
@@ -81,6 +97,10 @@ pub enum RobotLBError {
     HcloudLBChangeAlgorithm(
         #[from] hcloud::apis::Error<hcloud::apis::load_balancers_api::ChangeAlgorithmError>,
     ),
+    #[error("Cannot label load balancer. Reason: {}", describe(.0))]
+    HcloudLBReplaceError(
+        #[from] hcloud::apis::Error<hcloud::apis::load_balancers_api::ReplaceLoadBalancerError>,
+    ),
     #[error("Cannot list networks. Reason: {}", describe(.0))]
     HcloudListNetworksError(
         #[from] hcloud::apis::Error<hcloud::apis::networks_api::ListNetworksError>,
@@ -109,6 +129,7 @@ impl RobotLBError {
             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::HcloudLBReplaceError(error) => is_rate_limit_response(error),
             Self::HcloudListNetworksError(error) => is_rate_limit_response(error),
             Self::HcloudListLoadBalancersError(error) => is_rate_limit_response(error),
             Self::InvalidNodeFilter(_)
@@ -121,16 +142,15 @@ impl RobotLBError {
             | Self::UnknownLBAlgorithm
             | Self::ServiceWithoutSelector
             | Self::NoExposablePorts
+            | Self::ForeignBalancer { .. }
+            | Self::UnrecognisedBalancer { .. }
+            | Self::NoNodesToRecogniseBalancer(_)
+            | Self::AmbiguousBalancer(_)
             | 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 {
@@ -178,7 +198,7 @@ pub fn redact(message: &str, token: &str) -> String {
 
 #[cfg(test)]
 mod tests {
-    use super::{describe, is_not_found_response, is_rate_limit_response, redact, RobotLBError};
+    use super::{describe, is_rate_limit_response, redact, RobotLBError};
     use hcloud::apis::{load_balancers_api::ListLoadBalancersError, Error, ResponseContent};
 
     const TOKEN: &str = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ01";
@@ -245,12 +265,6 @@ 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, "")));
@@ -262,5 +276,37 @@ mod tests {
     fn other_statuses_are_not_a_rate_limit() {
         assert!(!RobotLBError::from(response_error(500, "")).is_rate_limited());
         assert!(!RobotLBError::SkipService.is_rate_limited());
+        assert!(!RobotLBError::AmbiguousBalancer("web".to_string()).is_rate_limited());
+    }
+
+    #[test]
+    fn an_unrecognised_balancer_names_the_handover_command() {
+        let error = RobotLBError::UnrecognisedBalancer {
+            name: "custom name".to_string(),
+            uid: "uid-1".to_string(),
+        };
+        assert!(!error.is_rate_limited());
+        assert!(error
+            .to_string()
+            .contains("hcloud load-balancer add-label 'custom name' robotlb/service-uid=uid-1"));
+    }
+
+    // Relabelling would take the balancer from a service that may still use it.
+    #[test]
+    fn a_balancer_of_another_service_names_its_owner_and_no_handover() {
+        let error = RobotLBError::ForeignBalancer {
+            name: "web".to_string(),
+            owner: "uid-2".to_string(),
+        };
+        assert!(!error.is_rate_limited());
+        assert!(error.to_string().contains("uid-2"));
+        assert!(!error.to_string().contains("add-label"));
+    }
+
+    #[test]
+    fn a_service_without_nodes_is_told_to_wait() {
+        let error = RobotLBError::NoNodesToRecogniseBalancer("web".to_string());
+        assert!(!error.is_rate_limited());
+        assert!(!error.to_string().contains("add-label"));
     }
 }