fix(opnsense-config): ensure load balancer service configuration is idempotent (#129)

The previous implementation blindly added HAProxy components without checking for existing configurations on the same port, which caused duplicate entries and errors when a service was updated.

This commit refactors the logic to a robust "remove-then-add" strategy. The configure_service method now finds and removes any existing frontend and its dependent components (backend, servers, health check) before adding the new, complete service definition.

This change makes the process fully idempotent, preventing configuration drift and ensuring a predictable state.

Co-authored-by: Ian Letourneau <letourneau.ian@gmail.com>
Reviewed-on: https://git.nationtech.io/NationTech/harmony/pulls/129

Cargo.lock

@@ -1918,6 +1918,8 @@ dependencies = [
  "env_logger",
  "harmony",
  "harmony_macros",
+ "harmony_secret",
+ "harmony_secret_derive",
  "harmony_tui",
  "harmony_types",
  "log",
@@ -3918,6 +3920,7 @@ checksum = "d05e27ee213611ffe7d6348b942e8f942b37114c00cc03cec254295a4a17852e"
 name = "opnsense-config"
 version = "0.1.0"
 dependencies = [
+ "assertor",
  "async-trait",
  "chrono",
  "env_logger",

Cargo.toml

@@ -15,7 +15,8 @@ members = [
   "harmony_inventory_agent",
   "harmony_secret_derive",
   "harmony_secret",
-  "adr/agent_discovery/mdns", "brocade",
+  "adr/agent_discovery/mdns",
+  "brocade",
 ]
 
 [workspace.package]

harmony/src/domain/topology/load_balancer.rs

@@ -28,13 +28,7 @@ pub trait LoadBalancer: Send + Sync {
         &self,
         service: &LoadBalancerService,
     ) -> Result<(), ExecutorError> {
-        debug!(
-            "Listing LoadBalancer services {:?}",
-            self.list_services().await
-        );
-        if !self.list_services().await.contains(service) {
         self.add_service(service).await?;
-        }
         Ok(())
     }
 }

harmony/src/infra/opnsense/load_balancer.rs

@@ -26,19 +26,13 @@ impl LoadBalancer for OPNSenseFirewall {
     }
 
     async fn add_service(&self, service: &LoadBalancerService) -> Result<(), ExecutorError> {
-        warn!(
-            "TODO : the current implementation does not check / cleanup / merge with existing haproxy services properly. Make sure to manually verify that the configuration is correct after executing any operation here"
-        );
         let mut config = self.opnsense_config.write().await;
+        let mut load_balancer = config.load_balancer();
+
         let (frontend, backend, servers, healthcheck) =
             harmony_load_balancer_service_to_haproxy_xml(service);
-        let mut load_balancer = config.load_balancer();
-        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(())
     }
@@ -106,7 +100,7 @@ pub(crate) fn haproxy_xml_config_to_harmony_loadbalancer(
                 .backends
                 .backends
                 .iter()
-                .find(|b| b.uuid == frontend.default_backend);
+                .find(|b| Some(b.uuid.clone()) == frontend.default_backend);
 
             let mut health_check = None;
             match matching_backend {
@@ -116,8 +110,7 @@ pub(crate) fn haproxy_xml_config_to_harmony_loadbalancer(
                 }
                 None => {
                     warn!(
-                        "HAProxy config could not find a matching backend for frontend {:?}",
-                        frontend
+                        "HAProxy config could not find a matching backend for frontend {frontend:?}"
                     );
                 }
             }
@@ -152,11 +145,11 @@ pub(crate) fn get_servers_for_backend(
         .servers
         .iter()
         .filter_map(|server| {
+            let address = server.address.clone()?;
+            let port = server.port?;
+
             if backend_servers.contains(&server.uuid.as_str()) {
-                return Some(BackendServer {
-                    address: server.address.clone(),
-                    port: server.port,
-                });
+                return Some(BackendServer { address, port });
             }
             None
         })
@@ -347,7 +340,7 @@ pub(crate) fn harmony_load_balancer_service_to_haproxy_xml(
         name: format!("frontend_{}", service.listening_port),
         bind: service.listening_port.to_string(),
         mode: "tcp".to_string(), // TODO do not depend on health check here
-        default_backend: backend.uuid.clone(),
+        default_backend: Some(backend.uuid.clone()),
         ..Default::default()
     };
     info!("HAPRoxy frontend and backend mode currently hardcoded to tcp");
@@ -361,8 +354,8 @@ fn server_to_haproxy_server(server: &BackendServer) -> HAProxyServer {
         uuid: Uuid::new_v4().to_string(),
         name: format!("{}_{}", &server.address, &server.port),
         enabled: 1,
-        address: server.address.clone(),
-        port: server.port,
+        address: Some(server.address.clone()),
+        port: Some(server.port),
         mode: "active".to_string(),
         server_type: "static".to_string(),
         ..Default::default()
@@ -385,8 +378,8 @@ mod tests {
         let mut haproxy = HAProxy::default();
         let server = HAProxyServer {
             uuid: "server1".to_string(),
-            address: "192.168.1.1".to_string(),
-            port: 80,
+            address: Some("192.168.1.1".to_string()),
+            port: Some(80),
             ..Default::default()
         };
         haproxy.servers.servers.push(server);
@@ -411,8 +404,8 @@ mod tests {
         let mut haproxy = HAProxy::default();
         let server = HAProxyServer {
             uuid: "server1".to_string(),
-            address: "192.168.1.1".to_string(),
-            port: 80,
+            address: Some("192.168.1.1".to_string()),
+            port: Some(80),
             ..Default::default()
         };
         haproxy.servers.servers.push(server);
@@ -431,8 +424,8 @@ mod tests {
         let mut haproxy = HAProxy::default();
         let server = HAProxyServer {
             uuid: "server1".to_string(),
-            address: "192.168.1.1".to_string(),
-            port: 80,
+            address: Some("192.168.1.1".to_string()),
+            port: Some(80),
             ..Default::default()
         };
         haproxy.servers.servers.push(server);
@@ -453,16 +446,16 @@ mod tests {
         let mut haproxy = HAProxy::default();
         let server = HAProxyServer {
             uuid: "server1".to_string(),
-            address: "some-hostname.test.mcd".to_string(),
-            port: 80,
+            address: Some("some-hostname.test.mcd".to_string()),
+            port: Some(80),
             ..Default::default()
         };
         haproxy.servers.servers.push(server);
 
         let server = HAProxyServer {
             uuid: "server2".to_string(),
-            address: "192.168.1.2".to_string(),
-            port: 8080,
+            address: Some("192.168.1.2".to_string()),
+            port: Some(8080),
             ..Default::default()
         };
         haproxy.servers.servers.push(server);

harmony/src/modules/okd/bootstrap_load_balancer.rs

@@ -77,6 +77,8 @@ impl OKDBootstrapLoadBalancerScore {
             address: topology.bootstrap_host.ip.to_string(),
             port,
         });
+
+        backend.dedup();
         backend
     }
 }

opnsense-config-xml/src/data/haproxy.rs

@@ -77,7 +77,7 @@ impl YaSerializeTrait for HAProxyId {
     }
 }
 
-#[derive(PartialEq, Debug)]
+#[derive(PartialEq, Debug, Clone)]
 pub struct HAProxyId(String);
 
 impl Default for HAProxyId {
@@ -297,7 +297,7 @@ pub struct HAProxyFrontends {
     pub frontend: Vec<Frontend>,
 }
 
-#[derive(Default, PartialEq, Debug, YaSerialize, YaDeserialize)]
+#[derive(Clone, Default, PartialEq, Debug, YaSerialize, YaDeserialize)]
 pub struct Frontend {
     #[yaserde(attribute = true)]
     pub uuid: String,
@@ -310,7 +310,7 @@ pub struct Frontend {
     pub bind_options: MaybeString,
     pub mode: String,
     #[yaserde(rename = "defaultBackend")]
-    pub default_backend: String,
+    pub default_backend: Option<String>,
     pub ssl_enabled: i32,
     pub ssl_certificates: MaybeString,
     pub ssl_default_certificate: MaybeString,
@@ -416,7 +416,7 @@ pub struct HAProxyBackends {
     pub backends: Vec<HAProxyBackend>,
 }
 
-#[derive(Default, PartialEq, Debug, YaSerialize, YaDeserialize)]
+#[derive(Clone, Default, PartialEq, Debug, YaSerialize, YaDeserialize)]
 pub struct HAProxyBackend {
     #[yaserde(attribute = true, rename = "uuid")]
     pub uuid: String,
@@ -535,7 +535,7 @@ pub struct HAProxyServers {
     pub servers: Vec<HAProxyServer>,
 }
 
-#[derive(Default, PartialEq, Debug, YaSerialize, YaDeserialize)]
+#[derive(Clone, Default, PartialEq, Debug, YaSerialize, YaDeserialize)]
 pub struct HAProxyServer {
     #[yaserde(attribute = true, rename = "uuid")]
     pub uuid: String,
@@ -543,8 +543,8 @@ pub struct HAProxyServer {
     pub enabled: u8,
     pub name: String,
     pub description: MaybeString,
-    pub address: String,
-    pub port: u16,
+    pub address: Option<String>,
+    pub port: Option<u16>,
     pub checkport: MaybeString,
     pub mode: String,
     pub multiplexer_protocol: MaybeString,
@@ -589,7 +589,7 @@ pub struct HAProxyHealthChecks {
     pub healthchecks: Vec<HAProxyHealthCheck>,
 }
 
-#[derive(Default, PartialEq, Debug, YaSerialize, YaDeserialize)]
+#[derive(Clone, Default, PartialEq, Debug, YaSerialize, YaDeserialize)]
 pub struct HAProxyHealthCheck {
     #[yaserde(attribute = true)]
     pub uuid: String,

opnsense-config/Cargo.toml

@@ -25,6 +25,7 @@ sha2 = "0.10.9"
 
 [dev-dependencies]
 pretty_assertions.workspace = true
+assertor.workspace = true
 
 [lints.rust]
 unexpected_cfgs = { level = "warn", check-cfg = ['cfg(e2e_test)'] }

opnsense-config/src/config/manager/ssh.rs

@@ -30,8 +30,7 @@ impl SshConfigManager {
 
         self.opnsense_shell
             .exec(&format!(
-                "cp /conf/config.xml /conf/backup/{}",
-                backup_filename
+                "cp /conf/config.xml /conf/backup/{backup_filename}"
             ))
             .await
     }

opnsense-config/src/config/shell/mod.rs

@@ -1,9 +1,7 @@
 mod ssh;
-pub use ssh::*;
-
-use async_trait::async_trait;
-
 use crate::Error;
+use async_trait::async_trait;
+pub use ssh::*;
 
 #[async_trait]
 pub trait OPNsenseShell: std::fmt::Debug + Send + Sync {

opnsense-config/src/modules/load_balancer.rs

@@ -1,11 +1,8 @@
-use std::sync::Arc;
-
-use log::warn;
+use crate::{config::OPNsenseShell, Error};
 use opnsense_config_xml::{
     Frontend, HAProxy, HAProxyBackend, HAProxyHealthCheck, HAProxyServer, OPNsense,
 };
-
-use crate::{config::OPNsenseShell, Error};
+use std::{collections::HashSet, sync::Arc};
 
 pub struct LoadBalancerConfig<'a> {
     opnsense: &'a mut OPNsense,
@@ -31,7 +28,7 @@ impl<'a> LoadBalancerConfig<'a> {
         match &mut self.opnsense.opnsense.haproxy.as_mut() {
             Some(haproxy) => f(haproxy),
             None => unimplemented!(
-                "Adding a backend is not supported when haproxy config does not exist yet"
+                "Cannot configure load balancer when haproxy config does not exist yet"
             ),
         }
     }
@@ -40,21 +37,67 @@ impl<'a> LoadBalancerConfig<'a> {
         self.with_haproxy(|haproxy| haproxy.general.enabled = enabled as i32);
     }
 
-    pub fn add_backend(&mut self, backend: HAProxyBackend) {
-        warn!("TODO make sure this new backend does not refer non-existing entities like servers or health checks");
-        self.with_haproxy(|haproxy| haproxy.backends.backends.push(backend));
+    /// 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.remove_servers(&servers);
+
+        self.add_new_service(frontend, backend, servers, healthcheck);
     }
 
-    pub fn add_frontend(&mut self, frontend: Frontend) {
-        self.with_haproxy(|haproxy| haproxy.frontends.frontend.push(frontend));
+    // Remove the corresponding real servers based on their name if they already exist.
+    fn remove_servers(&mut self, servers: &[HAProxyServer]) {
+        let server_names: HashSet<_> = servers.iter().map(|s| s.name.clone()).collect();
+        self.with_haproxy(|haproxy| {
+            haproxy
+                .servers
+                .servers
+                .retain(|s| !server_names.contains(&s.name));
+        });
     }
 
-    pub fn add_healthcheck(&mut self, healthcheck: HAProxyHealthCheck) {
-        self.with_haproxy(|haproxy| haproxy.healthchecks.healthchecks.push(healthcheck));
+    /// 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_linked_servers(haproxy, &old_backend);
+        });
     }
 
-    pub fn add_servers(&mut self, mut servers: Vec<HAProxyServer>) {
-        self.with_haproxy(|haproxy| haproxy.servers.servers.append(&mut servers));
+    /// Adds the components of a new service to the HAProxy configuration.
+    /// This function de-duplicates servers by name to prevent configuration errors.
+    fn add_new_service(
+        &mut self,
+        frontend: Frontend,
+        backend: HAProxyBackend,
+        servers: Vec<HAProxyServer>,
+        healthcheck: Option<HAProxyHealthCheck>,
+    ) {
+        self.with_haproxy(|haproxy| {
+            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);
+        });
     }
 
     pub async fn reload_restart(&self) -> Result<(), Error> {
@@ -82,3 +125,262 @@ 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 default_backend = old_frontend.default_backend?;
+    let pos = haproxy
+        .backends
+        .backends
+        .iter()
+        .position(|b| b.uuid == 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_linked_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()));
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::config::DummyOPNSenseShell;
+    use assertor::*;
+    use opnsense_config_xml::{
+        Frontend, HAProxy, HAProxyBackend, HAProxyBackends, HAProxyFrontends, HAProxyHealthCheck,
+        HAProxyHealthChecks, HAProxyId, HAProxyServer, HAProxyServers, MaybeString, OPNsense,
+    };
+    use std::sync::Arc;
+
+    use super::LoadBalancerConfig;
+
+    static SERVICE_BIND_ADDRESS: &str = "192.168.1.1:80";
+    static OTHER_SERVICE_BIND_ADDRESS: &str = "192.168.1.1:443";
+
+    static SERVER_ADDRESS: &str = "1.1.1.1:80";
+    static OTHER_SERVER_ADDRESS: &str = "1.1.1.1:443";
+
+    #[test]
+    fn configure_service_should_add_all_service_components_to_haproxy() {
+        let mut opnsense = given_opnsense();
+        let mut load_balancer = given_load_balancer(&mut opnsense);
+        let (healthcheck, servers, backend, frontend) =
+            given_service(SERVICE_BIND_ADDRESS, SERVER_ADDRESS);
+
+        load_balancer.configure_service(
+            frontend.clone(),
+            backend.clone(),
+            servers.clone(),
+            Some(healthcheck.clone()),
+        );
+
+        assert_haproxy_configured_with(
+            opnsense,
+            vec![frontend],
+            vec![backend],
+            servers,
+            vec![healthcheck],
+        );
+    }
+
+    #[test]
+    fn configure_service_should_replace_service_on_same_bind_address() {
+        let (healthcheck, servers, backend, frontend) =
+            given_service(SERVICE_BIND_ADDRESS, SERVER_ADDRESS);
+        let mut opnsense = given_opnsense_with(given_haproxy(
+            vec![frontend.clone()],
+            vec![backend.clone()],
+            servers.clone(),
+            vec![healthcheck.clone()],
+        ));
+        let mut load_balancer = given_load_balancer(&mut opnsense);
+
+        let (updated_healthcheck, updated_servers, updated_backend, updated_frontend) =
+            given_service(SERVICE_BIND_ADDRESS, OTHER_SERVER_ADDRESS);
+
+        load_balancer.configure_service(
+            updated_frontend.clone(),
+            updated_backend.clone(),
+            updated_servers.clone(),
+            Some(updated_healthcheck.clone()),
+        );
+
+        assert_haproxy_configured_with(
+            opnsense,
+            vec![updated_frontend],
+            vec![updated_backend],
+            updated_servers,
+            vec![updated_healthcheck],
+        );
+    }
+
+    #[test]
+    fn configure_service_should_keep_existing_service_on_different_bind_addresses() {
+        let (healthcheck, servers, backend, frontend) =
+            given_service(SERVICE_BIND_ADDRESS, SERVER_ADDRESS);
+        let (other_healthcheck, other_servers, other_backend, other_frontend) =
+            given_service(OTHER_SERVICE_BIND_ADDRESS, OTHER_SERVER_ADDRESS);
+        let mut opnsense = given_opnsense_with(given_haproxy(
+            vec![frontend.clone()],
+            vec![backend.clone()],
+            servers.clone(),
+            vec![healthcheck.clone()],
+        ));
+        let mut load_balancer = given_load_balancer(&mut opnsense);
+
+        load_balancer.configure_service(
+            other_frontend.clone(),
+            other_backend.clone(),
+            other_servers.clone(),
+            Some(other_healthcheck.clone()),
+        );
+
+        assert_haproxy_configured_with(
+            opnsense,
+            vec![frontend, other_frontend],
+            vec![backend, other_backend],
+            [servers, other_servers].concat(),
+            vec![healthcheck, other_healthcheck],
+        );
+    }
+
+    fn assert_haproxy_configured_with(
+        opnsense: OPNsense,
+        frontends: Vec<Frontend>,
+        backends: Vec<HAProxyBackend>,
+        servers: Vec<HAProxyServer>,
+        healthchecks: Vec<HAProxyHealthCheck>,
+    ) {
+        let haproxy = opnsense.opnsense.haproxy.as_ref().unwrap();
+        assert_that!(haproxy.frontends.frontend).contains_exactly(frontends);
+        assert_that!(haproxy.backends.backends).contains_exactly(backends);
+        assert_that!(haproxy.servers.servers).is_equal_to(servers);
+        assert_that!(haproxy.healthchecks.healthchecks).contains_exactly(healthchecks);
+    }
+
+    fn given_opnsense() -> OPNsense {
+        OPNsense::default()
+    }
+
+    fn given_opnsense_with(haproxy: HAProxy) -> OPNsense {
+        let mut opnsense = OPNsense::default();
+        opnsense.opnsense.haproxy = Some(haproxy);
+
+        opnsense
+    }
+
+    fn given_load_balancer<'a>(opnsense: &'a mut OPNsense) -> LoadBalancerConfig<'a> {
+        let opnsense_shell = Arc::new(DummyOPNSenseShell {});
+        if opnsense.opnsense.haproxy.is_none() {
+            opnsense.opnsense.haproxy = Some(HAProxy::default());
+        }
+        LoadBalancerConfig::new(opnsense, opnsense_shell)
+    }
+
+    fn given_service(
+        bind_address: &str,
+        server_address: &str,
+    ) -> (
+        HAProxyHealthCheck,
+        Vec<HAProxyServer>,
+        HAProxyBackend,
+        Frontend,
+    ) {
+        let healthcheck = given_healthcheck();
+        let servers = vec![given_server(server_address)];
+        let backend = given_backend();
+        let frontend = given_frontend(bind_address);
+        (healthcheck, servers, backend, frontend)
+    }
+
+    fn given_haproxy(
+        frontends: Vec<Frontend>,
+        backends: Vec<HAProxyBackend>,
+        servers: Vec<HAProxyServer>,
+        healthchecks: Vec<HAProxyHealthCheck>,
+    ) -> HAProxy {
+        HAProxy {
+            frontends: HAProxyFrontends {
+                frontend: frontends,
+            },
+            backends: HAProxyBackends { backends },
+            servers: HAProxyServers { servers },
+            healthchecks: HAProxyHealthChecks { healthchecks },
+            ..Default::default()
+        }
+    }
+
+    fn given_frontend(bind_address: &str) -> Frontend {
+        Frontend {
+            uuid: "uuid".into(),
+            id: HAProxyId::default(),
+            enabled: 1,
+            name: format!("frontend_{bind_address}"),
+            bind: bind_address.into(),
+            default_backend: Some("backend-uuid".into()),
+            ..Default::default()
+        }
+    }
+
+    fn given_backend() -> HAProxyBackend {
+        HAProxyBackend {
+            uuid: "backend-uuid".into(),
+            id: HAProxyId::default(),
+            enabled: 1,
+            name: "backend_192.168.1.1:80".into(),
+            linked_servers: MaybeString::from("server-uuid"),
+            health_check_enabled: 1,
+            health_check: MaybeString::from("healthcheck-uuid"),
+            ..Default::default()
+        }
+    }
+
+    fn given_server(address: &str) -> HAProxyServer {
+        HAProxyServer {
+            uuid: "server-uuid".into(),
+            id: HAProxyId::default(),
+            name: address.into(),
+            address: Some(address.into()),
+            ..Default::default()
+        }
+    }
+
+    fn given_healthcheck() -> HAProxyHealthCheck {
+        HAProxyHealthCheck {
+            uuid: "healthcheck-uuid".into(),
+            name: "healthcheck".into(),
+            ..Default::default()
+        }
+    }
+}