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?;
-        }
+        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()
+        }
+    }
+}