remove old service components (frontend, backend, servers, healthcheck) with same bind address before adding new service

harmony/src/infra/opnsense/load_balancer.rs

@@ -29,12 +29,8 @@ impl LoadBalancer for OPNSenseFirewall {
 
         let (frontend, backend, servers, healthcheck) =
             harmony_load_balancer_service_to_haproxy_xml(service);
-        load_balancer.add_backend(backend);
-        load_balancer.add_frontend(frontend);
-        load_balancer.add_servers(servers);
-        if let Some(healthcheck) = healthcheck {
-            load_balancer.add_healthcheck(healthcheck);
-        }
+
+        load_balancer.configure_service(frontend, backend, servers, healthcheck);
 
         Ok(())
     }

opnsense-config/src/modules/load_balancer.rs

@@ -1,4 +1,4 @@
-use std::sync::Arc;
+use std::{collections::HashSet, sync::Arc};
 
 use log::warn;
 use opnsense_config_xml::{
@@ -40,86 +40,51 @@ impl<'a> LoadBalancerConfig<'a> {
         self.with_haproxy(|haproxy| haproxy.general.enabled = enabled as i32);
     }
 
-    /// Adds or updates a backend pool.
-    /// If a backend with the same name exists, it is updated. Otherwise, it is added.
-    pub fn add_backend(&mut self, mut backend: HAProxyBackend) {
-        warn!("TODO make sure this new backend does not refer non-existing entities like servers or health checks");
-        self.with_haproxy(|haproxy| {
-            let existing_backend = haproxy
-                .backends
-                .backends
-                .iter_mut()
-                .find(|b| b.name == backend.name);
+    /// Configures a service by removing any existing service on the same port
+    /// and then adding the new definition. This ensures idempotency.
+    pub fn configure_service(
+        &mut self,
+        frontend: Frontend,
+        backend: HAProxyBackend,
+        servers: Vec<HAProxyServer>,
+        healthcheck: Option<HAProxyHealthCheck>,
+    ) {
+        self.remove_service_by_bind_address(&frontend.bind);
+        self.add_new_service(frontend, backend, servers, healthcheck);
+    }
 
-            if let Some(existing_backend) = existing_backend {
-                backend.uuid = existing_backend.uuid.clone(); // This breaks the `frontend` config
-                                                              // as it is now relying on a stale uuid
-                backend.id = existing_backend.id.clone();
-                *existing_backend = backend;
-            } else {
-                haproxy.backends.backends.push(backend);
-            }
+    /// Removes a service and its dependent components based on the frontend's bind address.
+    /// This performs a cascading delete of the frontend, backend, servers, and health check.
+    fn remove_service_by_bind_address(&mut self, bind_address: &str) {
+        self.with_haproxy(|haproxy| {
+            let Some(old_frontend) = remove_frontend_by_bind_address(haproxy, bind_address) else {
+                return;
+            };
+
+            let Some(old_backend) = remove_backend(haproxy, old_frontend) else {
+                return;
+            };
+
+            remove_healthcheck(haproxy, &old_backend);
+            remove_servers(haproxy, &old_backend);
         });
     }
 
-    /// Adds or updates a frontend.
-    /// If a frontend with the same name exists, it is updated. Otherwise, it is added.
-    pub fn add_frontend(&mut self, mut frontend: Frontend) {
+    /// Adds the components of a new service to the HAProxy configuration.
+    fn add_new_service(
+        &mut self,
+        frontend: Frontend,
+        backend: HAProxyBackend,
+        servers: Vec<HAProxyServer>,
+        healthcheck: Option<HAProxyHealthCheck>,
+    ) {
         self.with_haproxy(|haproxy| {
-            let existing_frontend = haproxy
-                .frontends
-                .frontend
-                .iter_mut()
-                .find(|f| f.name == frontend.name);
-
-            if let Some(existing_frontend) = existing_frontend {
-                frontend.uuid = existing_frontend.uuid.clone();
-                frontend.id = existing_frontend.id.clone();
-                *existing_frontend = frontend;
-            } else {
-                haproxy.frontends.frontend.push(frontend);
-            }
-        });
-    }
-
-    /// Adds or updates a health check.
-    /// If a health check with the same name exists, it is updated. Otherwise, it is added.
-    pub fn add_healthcheck(&mut self, mut healthcheck: HAProxyHealthCheck) {
-        self.with_haproxy(|haproxy| {
-            let existing_healthcheck = haproxy
-                .healthchecks
-                .healthchecks
-                .iter_mut()
-                .find(|h| h.name == healthcheck.name);
-
-            if let Some(existing_check) = existing_healthcheck {
-                healthcheck.uuid = existing_check.uuid.clone();
-                *existing_check = healthcheck;
-            } else {
-                haproxy.healthchecks.healthchecks.push(healthcheck);
-            }
-        });
-    }
-
-    /// Adds or updates a list of servers to the HAProxy configuration.
-    /// If a server with the same name already exists, it is updated. Otherwise, it is added.
-    pub fn add_servers(&mut self, servers: Vec<HAProxyServer>) {
-        self.with_haproxy(|haproxy| {
-            for server in servers {
-                let existing_server = haproxy
-                    .servers
-                    .servers
-                    .iter_mut()
-                    .find(|s| s.name == server.name);
-
-                if let Some(existing_server) = existing_server {
-                    existing_server.address = server.address;
-                    existing_server.port = server.port;
-                    existing_server.enabled = server.enabled;
-                } else {
-                    haproxy.servers.servers.push(server);
-                }
+            if let Some(check) = healthcheck {
+                haproxy.healthchecks.healthchecks.push(check);
             }
+            haproxy.servers.servers.extend(servers);
+            haproxy.backends.backends.push(backend);
+            haproxy.frontends.frontend.push(frontend);
         });
     }
 
@@ -148,3 +113,49 @@ impl<'a> LoadBalancerConfig<'a> {
         Ok(())
     }
 }
+
+fn remove_frontend_by_bind_address(haproxy: &mut HAProxy, bind_address: &str) -> Option<Frontend> {
+    let pos = haproxy
+        .frontends
+        .frontend
+        .iter()
+        .position(|f| f.bind == bind_address);
+
+    match pos {
+        Some(pos) => Some(haproxy.frontends.frontend.remove(pos)),
+        None => None,
+    }
+}
+
+fn remove_backend(haproxy: &mut HAProxy, old_frontend: Frontend) -> Option<HAProxyBackend> {
+    let pos = haproxy
+        .backends
+        .backends
+        .iter()
+        .position(|b| b.uuid == old_frontend.default_backend);
+
+    match pos {
+        Some(pos) => Some(haproxy.backends.backends.remove(pos)),
+        None => None, // orphaned frontend, shouldn't happen
+    }
+}
+
+fn remove_healthcheck(haproxy: &mut HAProxy, backend: &HAProxyBackend) {
+    if let Some(uuid) = &backend.health_check.content {
+        haproxy
+            .healthchecks
+            .healthchecks
+            .retain(|h| h.uuid != *uuid);
+    }
+}
+
+/// Remove the backend's servers. This assumes servers are not shared between services.
+fn remove_servers(haproxy: &mut HAProxy, backend: &HAProxyBackend) {
+    if let Some(server_uuids_str) = &backend.linked_servers.content {
+        let server_uuids_to_remove: HashSet<_> = server_uuids_str.split(',').collect();
+        haproxy
+            .servers
+            .servers
+            .retain(|s| !server_uuids_to_remove.contains(s.uuid.as_str()));
+    }
+}