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

letian · 2025-09-01T11:42:55Z

letian commented

2025-09-01 11:42:55 +00:00

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.

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.

letian added 1 commit 2025-09-01 11:42:56 +00:00

fix: merge existing services in load balancer config

Run Check Script / check (pull_request) Successful in 1m13s

Details

e9a1aa4831

letian reviewed 2025-09-01 11:44:09 +00:00

harmony/src/domain/topology/load_balancer.rs

						
				@ -35,3 +31,1 @@

				        if !self.list_services().await.contains(service) {

				            self.add_service(service).await?;

				        }

				        self.add_service(service).await?;

letian commented

2025-09-01 11:44:09 +00:00

we might want to rename this add_or_update_service instead of just add_service

we might want to rename this `add_or_update_service` instead of just `add_service`

letian marked this conversation as resolved

letian reviewed 2025-09-01 11:49:05 +00:00

opnsense-config/src/modules/load_balancer.rs Outdated

						
				@ -46,0 +53,4 @@

				            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

letian commented

2025-09-01 11:49:05 +00:00

As of now, the first time you add a load balancer service, everything will work as expected.

Though if you run it again, as we update in place the existing backend and we reuse its uuid, the frontend will be referring to a non-existent uuid for the default_backend (see harmony/src/infra/opnsense/load_balancer.rs on line 323). So we need to fix this before merging.

As of now, the first time you add a load balancer service, everything will work as expected. Though if you run it again, as we update in place the existing backend and we reuse its uuid, the frontend will be referring to a non-existent uuid for the `default_backend` (see `harmony/src/infra/opnsense/load_balancer.rs` on line 323). So we need to fix this before merging.

letian marked this conversation as resolved

letian reviewed 2025-09-01 11:49:46 +00:00

harmony/src/infra/opnsense/load_balancer.rs Outdated

						
				@ -28,3 +27,2 @@

				            "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();

letian commented

2025-09-01 11:49:46 +00:00

For now we only merge the existing config with the service (if it already exists). Should we also remove stale services?

letian marked this conversation as resolved

johnride commented

2025-09-01 13:58:50 +00:00

Ouais j'y repense un peu et c'est pas evident. Il y a beaucoup de cas.

Il faut simplifier. Probablement que la seule logique qui est raisonnable d'implementer maintenant c'est de prendre controle des ports et reset la config pour ces ports la chaque fois. Donc la logique serait probablement quelque chose comme :

configure_service(service, servers, health_check)
  current_config.destroy_service_components_on_port(service.port) // deletes services AND related backend pool AND servers that this service points to
  current_config.add_service(service, servers)

Ouais j'y repense un peu et c'est pas evident. Il y a beaucoup de cas. Il faut simplifier. Probablement que la seule logique qui est raisonnable d'implementer maintenant c'est de prendre controle des ports et reset la config pour ces ports la chaque fois. Donc la logique serait probablement quelque chose comme : ``` configure_service(service, servers, health_check) current_config.destroy_service_components_on_port(service.port) // deletes services AND related backend pool AND servers that this service points to current_config.add_service(service, servers) ```

letian added 1 commit 2025-09-03 19:58:32 +00:00

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

Run Check Script / check (pull_request) Successful in 1m10s

Details

fc4c18ccea

letian changed title from ~~WIP: fix: merge existing services in load balancer config~~ to fix: remove existing services (by bind address) in load balancer config

2025-09-03 19:59:47 +00:00

letian changed title from ~~fix: remove existing services (by bind address) in load balancer config~~ to fix(opnsense-config): ensure load balancer service configuration is idempotent

2025-09-03 20:03:07 +00:00

letian added 1 commit 2025-09-03 21:18:29 +00:00

de-duplicate stuff

Run Check Script / check (pull_request) Successful in 1m11s

Details

01206f5db1

letian added 2 commits 2025-09-04 01:55:39 +00:00

support optional server fields 3d8dd4d8e6

fix: de-duplicate backend servers list mapped from topology

Run Check Script / check (pull_request) Successful in 1m11s

Details

3ecda84530