refactor: improve maintainability (issue #58)

src/consts.rs

@@ -4,32 +4,32 @@
 //! for load balancer configuration.
 
 /// Annotation key for specifying the load balancer name.
-pub const LB_NAME_LABEL_NAME: &str = "robotlb/balancer";
+pub const LB_NAME_ANNOTATION: &str = "robotlb/balancer";
 /// Annotation key for custom node selector.
-pub const LB_NODE_SELECTOR: &str = "robotlb/node-selector";
+pub const LB_NODE_SELECTOR_ANNOTATION: &str = "robotlb/node-selector";
 /// Annotation key for specifying node IP.
-pub const LB_NODE_IP_LABEL_NAME: &str = "robotlb/node-ip";
+pub const LB_NODE_IP_ANNOTATION: &str = "robotlb/node-ip";
 
 // LB config
 /// Annotation key for health check interval (seconds).
-pub const LB_CHECK_INTERVAL_ANN_NAME: &str = "robotlb/lb-check-interval";
+pub const LB_CHECK_INTERVAL_ANNOTATION: &str = "robotlb/lb-check-interval";
 /// Annotation key for health check timeout (seconds).
-pub const LB_TIMEOUT_ANN_NAME: &str = "robotlb/lb-timeout";
+pub const LB_TIMEOUT_ANNOTATION: &str = "robotlb/lb-timeout";
 /// Annotation key for health check retries.
-pub const LB_RETRIES_ANN_NAME: &str = "robotlb/lb-retries";
+pub const LB_RETRIES_ANNOTATION: &str = "robotlb/lb-retries";
 /// Annotation key for enabling proxy mode.
-pub const LB_PROXY_MODE_LABEL_NAME: &str = "robotlb/lb-proxy-mode";
+pub const LB_PROXY_MODE_ANNOTATION: &str = "robotlb/lb-proxy-mode";
 /// Annotation key for network name.
-pub const LB_NETWORK_LABEL_NAME: &str = "robotlb/lb-network";
+pub const LB_NETWORK_ANNOTATION: &str = "robotlb/lb-network";
 /// Annotation key for private IP mode.
-pub const LB_PRIVATE_IP_LABEL_NAME: &str = "robotlb/lb-private-ip";
+pub const LB_PRIVATE_IP_ANNOTATION: &str = "robotlb/lb-private-ip";
 
 /// Annotation key for load balancer location.
-pub const LB_LOCATION_LABEL_NAME: &str = "robotlb/lb-location";
+pub const LB_LOCATION_ANNOTATION: &str = "robotlb/lb-location";
 /// Annotation key for load balancing algorithm.
-pub const LB_ALGORITHM_LABEL_NAME: &str = "robotlb/lb-algorithm";
+pub const LB_ALGORITHM_ANNOTATION: &str = "robotlb/lb-algorithm";
 /// Annotation key for load balancer type.
-pub const LB_BALANCER_TYPE_LABEL_NAME: &str = "robotlb/balancer-type";
+pub const LB_BALANCER_TYPE_ANNOTATION: &str = "robotlb/balancer-type";
 
 /// Default number of health check retries.
 pub const DEFAULT_LB_RETRIES: i32 = 3;

src/controller/mod.rs

@@ -233,35 +233,7 @@ pub async fn run(
 #[cfg(test)]
 mod tests {
     use super::*;
-    use k8s_openapi::{
-        api::core::v1::{ServicePort, ServiceSpec},
-        apimachinery::pkg::apis::meta::v1::ObjectMeta,
-    };
-
-    fn service_with_spec(spec: ServiceSpec) -> Service {
-        Service {
-            metadata: ObjectMeta {
-                name: Some("svc".to_string()),
-                namespace: Some("default".to_string()),
-                ..Default::default()
-            },
-            spec: Some(spec),
-            ..Default::default()
-        }
-    }
-
-    fn service_spec(
-        service_type: &str,
-        lb_class: Option<&str>,
-        ports: Vec<ServicePort>,
-    ) -> ServiceSpec {
-        ServiceSpec {
-            type_: Some(service_type.to_string()),
-            load_balancer_class: lb_class.map(str::to_string),
-            ports: Some(ports),
-            ..Default::default()
-        }
-    }
+    use crate::test_utils::fixtures::{service_spec, service_with_spec};
 
     #[test]
     fn service_filter_rejects_non_load_balancer_type() {

src/controller/nodes.rs

@@ -168,7 +168,7 @@ async fn get_nodes_by_selector(
 ) -> RobotLBResult<Vec<Node>> {
     let node_selector = svc
         .annotations()
-        .get(consts::LB_NODE_SELECTOR)
+        .get(consts::LB_NODE_SELECTOR_ANNOTATION)
         .map(String::as_str)
         .ok_or(RobotLBError::ServiceWithoutSelector)?;
     let label_filter = LabelFilter::from_str(node_selector)?;
@@ -185,35 +185,8 @@ async fn get_nodes_by_selector(
 #[cfg(test)]
 mod tests {
     use super::*;
-    use k8s_openapi::{
-        api::core::v1::{NodeAddress, NodeStatus, ServicePort, ServiceSpec},
-        apimachinery::pkg::apis::meta::v1::ObjectMeta,
-    };
-
-    fn service_with_spec(spec: ServiceSpec) -> Service {
-        Service {
-            metadata: ObjectMeta {
-                name: Some("svc".to_string()),
-                namespace: Some("default".to_string()),
-                ..Default::default()
-            },
-            spec: Some(spec),
-            ..Default::default()
-        }
-    }
-
-    fn service_spec(
-        service_type: &str,
-        lb_class: Option<&str>,
-        ports: Vec<ServicePort>,
-    ) -> ServiceSpec {
-        ServiceSpec {
-            type_: Some(service_type.to_string()),
-            load_balancer_class: lb_class.map(str::to_string),
-            ports: Some(ports),
-            ..Default::default()
-        }
-    }
+    use crate::test_utils::fixtures::{service_spec, service_with_spec};
+    use k8s_openapi::api::core::v1::{NodeAddress, NodeStatus, ServicePort};
 
     #[test]
     fn derive_targets_picks_matching_address_type() {

src/lb/config.rs

@@ -30,46 +30,46 @@ pub fn parse_load_balancer_config(
     config: &OperatorConfig,
 ) -> RobotLBResult<ParsedLoadBalancerConfig> {
     let retries =
-        parse_annotation(svc, consts::LB_RETRIES_ANN_NAME)?.unwrap_or(config.default_lb_retries);
+        parse_annotation(svc, consts::LB_RETRIES_ANNOTATION)?.unwrap_or(config.default_lb_retries);
 
     let timeout =
-        parse_annotation(svc, consts::LB_TIMEOUT_ANN_NAME)?.unwrap_or(config.default_lb_timeout);
+        parse_annotation(svc, consts::LB_TIMEOUT_ANNOTATION)?.unwrap_or(config.default_lb_timeout);
 
-    let check_interval = parse_annotation(svc, consts::LB_CHECK_INTERVAL_ANN_NAME)?
+    let check_interval = parse_annotation(svc, consts::LB_CHECK_INTERVAL_ANNOTATION)?
         .unwrap_or(config.default_lb_interval);
 
-    let proxy_mode = parse_annotation(svc, consts::LB_PROXY_MODE_LABEL_NAME)?
+    let proxy_mode = parse_annotation(svc, consts::LB_PROXY_MODE_ANNOTATION)?
         .unwrap_or(config.default_lb_proxy_mode_enabled);
 
     let location = svc
         .annotations()
-        .get(consts::LB_LOCATION_LABEL_NAME)
+        .get(consts::LB_LOCATION_ANNOTATION)
         .cloned()
         .unwrap_or_else(|| config.default_lb_location.clone());
 
     let balancer_type = svc
         .annotations()
-        .get(consts::LB_BALANCER_TYPE_LABEL_NAME)
+        .get(consts::LB_BALANCER_TYPE_ANNOTATION)
         .cloned()
         .unwrap_or_else(|| config.default_balancer_type.clone());
 
     let algorithm = parse_algorithm(svc, config)?;
 
     let network_name = svc
         .annotations()
-        .get(consts::LB_NETWORK_LABEL_NAME)
+        .get(consts::LB_NETWORK_ANNOTATION)
         .or(config.default_network.as_ref())
         .cloned();
 
     let name = svc
         .annotations()
-        .get(consts::LB_NAME_LABEL_NAME)
+        .get(consts::LB_NAME_ANNOTATION)
         .cloned()
         .unwrap_or_else(|| svc.name_any());
 
     let private_ip = svc
         .annotations()
-        .get(consts::LB_PRIVATE_IP_LABEL_NAME)
+        .get(consts::LB_PRIVATE_IP_ANNOTATION)
         .cloned();
 
     Ok(ParsedLoadBalancerConfig {
@@ -103,7 +103,7 @@ where
 /// Parse the algorithm annotation or fall back to default.
 fn parse_algorithm(svc: &Service, config: &OperatorConfig) -> RobotLBResult<LBAlgorithm> {
     svc.annotations()
-        .get(consts::LB_ALGORITHM_LABEL_NAME)
+        .get(consts::LB_ALGORITHM_ANNOTATION)
         .map(String::as_str)
         .or(Some(&config.default_lb_algorithm))
         .map(LBAlgorithm::from_str)
@@ -184,16 +184,16 @@ mod tests {
         let mut config = base_config();
         config.default_network = None;
         let svc = service_with_annotations([
-            (consts::LB_NAME_LABEL_NAME, "custom-lb"),
-            (consts::LB_RETRIES_ANN_NAME, "5"),
-            (consts::LB_TIMEOUT_ANN_NAME, "8"),
-            (consts::LB_CHECK_INTERVAL_ANN_NAME, "20"),
-            (consts::LB_PROXY_MODE_LABEL_NAME, "true"),
-            (consts::LB_LOCATION_LABEL_NAME, "nbg1"),
-            (consts::LB_BALANCER_TYPE_LABEL_NAME, "lb31"),
-            (consts::LB_ALGORITHM_LABEL_NAME, "round-robin"),
-            (consts::LB_NETWORK_LABEL_NAME, "private-net"),
-            (consts::LB_PRIVATE_IP_LABEL_NAME, "10.10.0.5"),
+            (consts::LB_NAME_ANNOTATION, "custom-lb"),
+            (consts::LB_RETRIES_ANNOTATION, "5"),
+            (consts::LB_TIMEOUT_ANNOTATION, "8"),
+            (consts::LB_CHECK_INTERVAL_ANNOTATION, "20"),
+            (consts::LB_PROXY_MODE_ANNOTATION, "true"),
+            (consts::LB_LOCATION_ANNOTATION, "nbg1"),
+            (consts::LB_BALANCER_TYPE_ANNOTATION, "lb31"),
+            (consts::LB_ALGORITHM_ANNOTATION, "round-robin"),
+            (consts::LB_NETWORK_ANNOTATION, "private-net"),
+            (consts::LB_PRIVATE_IP_ANNOTATION, "10.10.0.5"),
         ]);
 
         let parsed = parse_load_balancer_config(&svc, &config).expect("parse should succeed");
@@ -216,7 +216,7 @@ mod tests {
     #[test]
     fn returns_error_for_invalid_algorithm_annotation() {
         let config = base_config();
-        let svc = service_with_annotations([(consts::LB_ALGORITHM_LABEL_NAME, "weighted")]);
+        let svc = service_with_annotations([(consts::LB_ALGORITHM_ANNOTATION, "weighted")]);
 
         let result = parse_load_balancer_config(&svc, &config);
         assert!(matches!(result, Err(RobotLBError::UnknownLBAlgorithm)));

src/lb/mod.rs

@@ -27,10 +27,7 @@ use crate::{
 
 pub(crate) use api::{HcloudLoadBalancerApi, LiveHcloudLoadBalancerApi};
 pub(crate) use config::parse_load_balancer_config;
-pub use types::LBService;
-pub(crate) use types::{
-    ServiceReconcileAction, TargetReconcileAction, normalize_ip, service_matches_desired,
-};
+use types::{ServiceReconcileAction, TargetReconcileAction, normalize_ip, service_matches_desired};
 
 /// Struct representing a load balancer.
 ///
@@ -412,45 +409,17 @@ impl LoadBalancer {
         &self,
         hcloud_balancer: &hcloud::models::LoadBalancer,
     ) -> RobotLBResult<()> {
-        // If the network name is not provided, and load balancer is not attached to any network,
-        // we can skip this step.
         if self.network_name.is_none() && hcloud_balancer.private_net.is_empty() {
             return Ok(());
         }
 
         let desired_network = self.get_network().await?.map(|network| network.id);
-        // If the network name is not provided, but the load balancer is attached to a network,
-        // we need to detach it from the network.
-        let mut contain_desired_network = false;
-        if !hcloud_balancer.private_net.is_empty() {
-            for private_net in &hcloud_balancer.private_net {
-                let Some(private_net_id) = private_net.network else {
-                    continue;
-                };
-                // The load balancer is attached to a target network.
-                if desired_network == Some(private_net_id) {
-                    // Specific IP was provided, we need to check if the IP is the same.
-                    if self.private_ip.is_some() {
-                        // if IPs match, we can leave everything as it is.
-                        if private_net.ip == self.private_ip {
-                            contain_desired_network = true;
-                            continue;
-                        }
-                    } else {
-                        // No specific IP was provided, we can leave everything as it is.
-                        contain_desired_network = true;
-                        continue;
-                    }
-                }
-                tracing::info!("Detaching balancer from network {}", private_net_id);
-                api::detach_from_network(&self.hcloud_config, hcloud_balancer.id, private_net_id)
-                    .await?;
-            }
-        }
-        if !contain_desired_network {
-            let Some(network_id) = desired_network else {
-                return Ok(());
-            };
+
+        let is_attached = self
+            .detach_unwanted_networks(hcloud_balancer, desired_network)
+            .await?;
+
+        if !is_attached && let Some(network_id) = desired_network {
             tracing::info!("Attaching balancer to network {}", network_id);
             api::attach_to_network(
                 &self.hcloud_config,
@@ -463,6 +432,42 @@ impl LoadBalancer {
         Ok(())
     }
 
+    async fn detach_unwanted_networks(
+        &self,
+        hcloud_balancer: &hcloud::models::LoadBalancer,
+        desired_network: Option<i64>,
+    ) -> RobotLBResult<bool> {
+        let mut is_attached = false;
+
+        for private_net in &hcloud_balancer.private_net {
+            let Some(private_net_id) = private_net.network else {
+                continue;
+            };
+
+            if desired_network == Some(private_net_id)
+                && private_net
+                    .ip
+                    .as_ref()
+                    .is_some_and(|ip| self.matches_desired_ip(ip))
+            {
+                is_attached = true;
+                continue;
+            }
+
+            tracing::info!("Detaching balancer from network {}", private_net_id);
+            api::detach_from_network(&self.hcloud_config, hcloud_balancer.id, private_net_id)
+                .await?;
+        }
+
+        Ok(is_attached)
+    }
+
+    fn matches_desired_ip(&self, current_ip: &str) -> bool {
+        self.private_ip
+            .as_ref()
+            .is_none_or(|desired_ip| desired_ip == current_ip)
+    }
+
     /// Cleanup the load balancer.
     ///
     /// This method will remove all the services and targets from the