From 4e2b284f37f75e662af6d4b7138b93578063f174 Mon Sep 17 00:00:00 2001
From: Marc Schreiber <info@schrieveslaach.de>
Date: Tue, 12 Mar 2024 09:59:37 +0100
Subject: [PATCH] Fix Duplicate Deployments in Bootstrapped Apps

If the application is bootstrapped by a bootstap container and then
companions have been converted into instances, and then the instances
should be updated with a subsequent HTTP request, the application ends
up with a second version of the instance. This circumstance is not
intended and this commit makes sure that subsequent HTTP requests update
existing deployments while taking the bootstrapping into account.
---
 .../kubernetes/deployment_unit.rs             | 45 ++++++++++---------
 .../kubernetes/infrastructure.rs              | 18 +++++++-
 2 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/api/src/infrastructure/kubernetes/deployment_unit.rs b/api/src/infrastructure/kubernetes/deployment_unit.rs
index b3d87d0..a76eb1d 100644
--- a/api/src/infrastructure/kubernetes/deployment_unit.rs
+++ b/api/src/infrastructure/kubernetes/deployment_unit.rs
@@ -35,6 +35,7 @@ use kube::{
 };
 use serde::Deserialize;
 use std::{
+    borrow::Borrow,
     collections::{BTreeMap, HashSet},
     str::FromStr,
 };
@@ -586,17 +587,17 @@ impl K8sDeploymentUnit {
     /// This filters bootstrapped [Deployments](Deployment), [Stateful Sets](StatefulSet), or
     /// [Pods](Pod) by the existing [services](Service) in already deployed application to avoid
     /// that deployments of instances overwrite each other
-    pub(super) fn filter_by_instances_and_replicas(
-        &mut self,
-        services: &[crate::models::service::Service],
-    ) {
+    pub(super) fn filter_by_instances_and_replicas<S>(&mut self, services: S)
+    where
+        S: Iterator,
+        <S as Iterator>::Item: Borrow<crate::models::service::Service>,
+    {
         let service_not_to_be_retained = services
-            .iter()
             .filter(|s| {
-                s.container_type() == &ContainerType::Instance
-                    || s.container_type() == &ContainerType::Replica
+                s.borrow().container_type() == &ContainerType::Instance
+                    || s.borrow().container_type() == &ContainerType::Replica
             })
-            .map(|s| s.service_name())
+            .map(|s| s.borrow().service_name().clone())
             .collect::<HashSet<_>>();
 
         self.deployments.retain(|deployment| {
@@ -1145,12 +1146,14 @@ mod tests {
         )
         .await;
 
-        unit.filter_by_instances_and_replicas(dbg!(&[ServiceBuilder::new()
-            .app_name(AppName::master().to_string())
-            .id(String::from("test"))
-            .config(crate::sc!("nginx", "nginx:1.15"))
-            .build()
-            .unwrap()]));
+        unit.filter_by_instances_and_replicas(std::iter::once(
+            ServiceBuilder::new()
+                .app_name(AppName::master().to_string())
+                .id(String::from("test"))
+                .config(crate::sc!("nginx", "nginx:1.15"))
+                .build()
+                .unwrap(),
+        ));
 
         assert!(unit.deployments.is_empty());
     }
@@ -1183,12 +1186,14 @@ mod tests {
         )
         .await;
 
-        unit.filter_by_instances_and_replicas(dbg!(&[ServiceBuilder::new()
-            .app_name(AppName::master().to_string())
-            .id(String::from("test"))
-            .config(crate::sc!("postgres", "postgres"))
-            .build()
-            .unwrap()]));
+        unit.filter_by_instances_and_replicas(std::iter::once(
+            ServiceBuilder::new()
+                .app_name(AppName::master().to_string())
+                .id(String::from("test"))
+                .config(crate::sc!("postgres", "postgres"))
+                .build()
+                .unwrap(),
+        ));
 
         assert!(!unit.deployments.is_empty());
     }
diff --git a/api/src/infrastructure/kubernetes/infrastructure.rs b/api/src/infrastructure/kubernetes/infrastructure.rs
index 78969c8..0ce97bf 100644
--- a/api/src/infrastructure/kubernetes/infrastructure.rs
+++ b/api/src/infrastructure/kubernetes/infrastructure.rs
@@ -64,7 +64,7 @@ use kube::{
 use log::{debug, warn};
 use multimap::MultiMap;
 use secstr::SecUtf8;
-use std::collections::{BTreeMap, HashMap};
+use std::collections::{BTreeMap, HashMap, HashSet};
 use std::convert::{From, TryFrom};
 use std::net::IpAddr;
 use std::str::FromStr;
@@ -522,8 +522,22 @@ impl Infrastructure for KubernetesInfrastructure {
         )
         .await?;
 
+        let deployment_unit_service_names = deployment_unit
+            .services()
+            .iter()
+            .map(|s| s.service_name())
+            .collect::<HashSet<_>>();
+
         let services = self.get_services_of_app(app_name).await?;
-        k8s_deployment_unit.filter_by_instances_and_replicas(&services);
+
+        k8s_deployment_unit.filter_by_instances_and_replicas(
+            services
+                .iter()
+                // we must not exclude the services that are provided by the deployment_unit
+                // because without that filter a second update of the service would create an
+                // additional deployment instead of updating/merging the existing one.
+                .filter(|s| !deployment_unit_service_names.contains(s.service_name())),
+        );
 
         for deployable_service in deployment_unit.services() {
             let (secret, service, deployment, ingress_route, middlewares) = self