Post rebase fixes

DataBiosphere · Nov 30, 2022 · 942f815 · 942f815
1 parent 8f53b95
commit 942f815
Show file tree

Hide file tree

Showing 7 changed files with 129 additions and 133 deletions.
diff --git a/...scala/org/broadinstitute/dsde/workbench/leonardo/monitor/LeoPubsubMessageSubscriber.scala b/...scala/org/broadinstitute/dsde/workbench/leonardo/monitor/LeoPubsubMessageSubscriber.scala
@@ -1477,11 +1477,31 @@ class LeoPubsubMessageSubscriber[F[_]](
       ctx <- ev.ask
       _ <- msg.cloudContext match {
         case CloudContext.Azure(c) =>
-          val task =
-            azurePubsubHandler.deleteApp(msg.appId, msg.appName, msg.workspaceId, msg.landingZoneResourcesOpt, c)
-          asyncTasks.offer(
-            Task(ctx.traceId, task, Some(handleKubernetesError), ctx.now, "deleteAppV2")
-          )
+          for {
+            landingZoneResources <- F.fromOption(
+              msg.landingZoneResourcesOpt,
+              PubsubKubernetesError(
+                AppError(s"Landing zone required for Azure apps",
+                         ctx.now,
+                         ErrorAction.CreateApp,
+                         ErrorSource.App,
+                         None,
+                         Some(ctx.traceId)
+                ),
+                Some(msg.appId),
+                false,
+                None,
+                None,
+                None
+              )
+            )
+            task =
+              azurePubsubHandler.deleteApp(msg.appId, msg.appName, msg.workspaceId, landingZoneResources, c)
+            _ <- asyncTasks.offer(
+              Task(ctx.traceId, task, Some(handleKubernetesError), ctx.now, "deleteAppV2")
+            )
+          } yield ()
+
         case CloudContext.Gcp(c) =>
           F.raiseError(
             PubsubKubernetesError(

diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/util/AKSAlgebra.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/util/AKSAlgebra.scala
@@ -25,7 +25,7 @@ final case class CreateAKSAppParams(appId: AppId,
 final case class DeleteAKSAppParams(
   appName: AppName,
   workspaceId: WorkspaceId,
-  landingZoneResourcesOpt: Option[LandingZoneResources],
+  landingZoneResourcesOpt: LandingZoneResources,
   cloudContext: AzureCloudContext,
   keepHistory: Boolean = false
 )
diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/util/AKSInterpreter.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/util/AKSInterpreter.scala
@@ -25,7 +25,7 @@ import io.kubernetes.client.util.Config
 import org.broadinstitute.dsde.workbench.DoneCheckableSyntax._
 import org.broadinstitute.dsde.workbench.azure._
 import org.broadinstitute.dsde.workbench.google2.KubernetesModels.{KubernetesNamespace, PodStatus}
-import org.broadinstitute.dsde.workbench.google2.KubernetesSerializableName.NamespaceName
+import org.broadinstitute.dsde.workbench.google2.KubernetesSerializableName.{NamespaceName, ServiceAccountName}
 import org.broadinstitute.dsde.workbench.google2.util.RetryPredicates
 import org.broadinstitute.dsde.workbench.google2.util.RetryPredicates.whenStatusCode
 import org.broadinstitute.dsde.workbench.google2.{
@@ -36,8 +36,8 @@ import org.broadinstitute.dsde.workbench.google2.{
   tracedRetryF
 }
 import org.broadinstitute.dsde.workbench.leonardo.SamResourceId.AppSamResourceId
-import org.broadinstitute.dsde.workbench.leonardo.config.{AppMonitorConfig, CoaAppConfig, SamConfig}
-import org.broadinstitute.dsde.workbench.leonardo.dao.{CbasDAO, CromwellDAO, SamDAO, WdsDAO}
+import org.broadinstitute.dsde.workbench.leonardo.config.{AppMonitorConfig, CoaAppConfig, HttpWsmDaoConfig, SamConfig}
+import org.broadinstitute.dsde.workbench.leonardo.dao._
 import org.broadinstitute.dsde.workbench.leonardo.db._
 import org.broadinstitute.dsde.workbench.leonardo.http._
 import org.broadinstitute.dsde.workbench.leonardo.http.service.AppNotFoundException
@@ -152,7 +152,7 @@ class AKSInterpreter[F[_]](config: AKSInterpreterConfig,
 
       // Create relay hybrid connection pool
       hcName = RelayHybridConnectionName(params.appName.value)
-      primaryKey <- azureRelayService.createRelayHybridConnection(landingZoneResources.relayNamespace,
+      primaryKey <- azureRelayService.createRelayHybridConnection(params.landingZoneResources.relayNamespace,
                                                                   hcName,
                                                                   params.cloudContext
       )
@@ -237,6 +237,98 @@ class AKSInterpreter[F[_]](config: AKSInterpreterConfig,
 
     } yield ()
 
+  override def deleteApp(params: DeleteAKSAppParams)(implicit ev: Ask[F, AppContext]): F[Unit] = {
+    val DeleteAKSAppParams(appName, workspaceId, landingZoneResources, cloudContext, keepHistory) = params
+    for {
+      ctx <- ev.ask
+
+      // Grab records from the database
+      dbAppOpt <- KubernetesServiceDbQueries
+        .getActiveFullAppByName(CloudContext.Azure(cloudContext), params.appName)
+        .transaction
+      dbApp <- F.fromOption(
+        dbAppOpt,
+        AppNotFoundException(CloudContext.Azure(cloudContext), params.appName, ctx.traceId, "No active app found in DB")
+      )
+      _ <- logger.info(ctx.loggingCtx)(s"Deleting app $appName in workspace $workspaceId")
+
+      app = dbApp.app
+      namespaceName = app.appResources.namespace.name
+      kubernetesNamespace = KubernetesNamespace(namespaceName)
+      dbCluster = dbApp.cluster
+
+      clusterName = landingZoneResources.clusterName // NOT the same as dbCluster.clusterName
+      client <- buildCoreV1Client(cloudContext, landingZoneResources.clusterName)
+
+      // Authenticate helm client
+      authContext <- getHelmAuthContext(landingZoneResources.clusterName, cloudContext, namespaceName)
+
+      _ <- helmClient.uninstall(app.release, keepHistory).run(authContext)
+
+      // poll until the app pods are deleted
+      last <- streamFUntilDone(
+        listPodStatus(client, KubernetesNamespace(app.appResources.namespace.name)),
+        config.appMonitorConfig.deleteApp.maxAttempts,
+        config.appMonitorConfig.deleteApp.interval
+      ).compile.lastOrError
+
+      _ <-
+        if (!podDoneCheckable.isDone(last)) {
+          val msg =
+            s"Helm deletion has failed or timed out for app ${app.appName.value} in cluster ${dbCluster.getClusterId.toString}."
+          logger.error(ctx.loggingCtx)(msg) >>
+            F.raiseError[Unit](AppDeletionException(msg))
+        } else F.unit
+
+      // helm uninstall the setup chart
+      _ <- helmClient
+        .uninstall(
+          getTerraAppSetupChartReleaseName(app.release),
+          keepHistory
+        )
+        .run(authContext)
+
+      // delete the namespace only after the helm uninstall completes.
+      _ <- deleteNamespace(client, kubernetesNamespace)
+
+      fa = namespaceExists(client, kubernetesNamespace)
+        .map(
+          !_
+        ) // mapping to inverse because booleanDoneCheckable defines `Done` when it becomes `true`...In this case, the namespace will exists for a while, and eventually becomes non-existent
+
+      _ <- streamUntilDoneOrTimeout(fa,
+                                    config.appMonitorConfig.deleteApp.maxAttempts,
+                                    config.appMonitorConfig.deleteApp.initialDelay,
+                                    "delete namespace timed out"
+      )
+
+      userEmail = app.auditInfo.creator
+      tokenOpt <- samDao.getCachedArbitraryPetAccessToken(userEmail)
+
+      _ <- tokenOpt match {
+        case Some(token) =>
+          for {
+            _ <- samDao.deleteResourceInternal(dbApp.app.samResourceId,
+                                               Authorization(Credentials.Token(AuthScheme.Bearer, token))
+            )
+
+          } yield ()
+        case None =>
+          logger.warn(
+            s"Could not find pet service account for user ${userEmail} in Sam. Skipping resource deletion in Sam."
+          )
+      }
+
+      _ <- logger.info(
+        s"Delete app operation has finished for app ${app.appName.value} in cluster ${clusterName}"
+      )
+
+      _ <- appQuery.updateStatus(app.id, AppStatus.Deleted).transaction
+
+      _ <- logger.info(s"Done deleting app $appName in workspace $workspaceId")
+    } yield ()
+  }
+
   private[util] def pollCromwellAppCreation(userEmail: WorkbenchEmail, relayBaseUri: Uri)(implicit
     ev: Ask[F, AppContext]
   ): F[Boolean] =
@@ -449,108 +541,6 @@ class AKSInterpreter[F[_]](config: AKSInterpreterConfig,
     F.delay(ComputeManager.authenticate(clientSecretCredential, azureProfile))
   }
 
-  override def deleteApp(params: DeleteAKSAppParams)(implicit ev: Ask[F, AppContext]): F[Unit] = {
-    val DeleteAKSAppParams(appName, workspaceId, landingZoneResourcesOpt, cloudContext, keepHistory) = params
-    for {
-      ctx <- ev.ask
-
-      // Grab records from the database
-      dbAppOpt <- KubernetesServiceDbQueries
-        .getActiveFullAppByName(CloudContext.Azure(cloudContext), params.appName)
-        .transaction
-      dbApp <- F.fromOption(
-        dbAppOpt,
-        AppNotFoundException(CloudContext.Azure(cloudContext), params.appName, ctx.traceId, "No active app found in DB")
-      )
-      _ <- logger.info(ctx.loggingCtx)(s"Deleting app $appName in workspace $workspaceId")
-
-      app = dbApp.app
-      namespaceName = app.appResources.namespace.name
-      kubernetesNamespace = KubernetesNamespace(namespaceName)
-      dbCluster = dbApp.cluster
-
-      // Get resources from landing zone
-      landingZoneResources <- F.fromOption(
-        landingZoneResourcesOpt,
-        AppCreationException(
-          s"Landing Zone Resources not found in app creation params for app ${app.appName.value}",
-          Some(ctx.traceId)
-        )
-      )
-
-      clusterName = landingZoneResources.clusterName // NOT the same as dbCluster.clusterName
-      client <- buildCoreV1Client(cloudContext, landingZoneResources.clusterName)
-
-      // Authenticate helm client
-      authContext <- getHelmAuthContext(landingZoneResources.clusterName, cloudContext, namespaceName)
-
-      _ <- helmClient.uninstall(app.release, keepHistory).run(authContext)
-
-      // poll until the app pods are deleted
-      last <- streamFUntilDone(
-        listPodStatus(client, KubernetesNamespace(app.appResources.namespace.name)),
-        config.appMonitorConfig.deleteApp.maxAttempts,
-        config.appMonitorConfig.deleteApp.interval
-      ).compile.lastOrError
-
-      _ <-
-        if (!podDoneCheckable.isDone(last)) {
-          val msg =
-            s"Helm deletion has failed or timed out for app ${app.appName.value} in cluster ${dbCluster.getClusterId.toString}."
-          logger.error(ctx.loggingCtx)(msg) >>
-            F.raiseError[Unit](AppDeletionException(msg))
-        } else F.unit
-
-      // helm uninstall the setup chart
-      _ <- helmClient
-        .uninstall(
-          getTerraAppSetupChartReleaseName(app.release),
-          keepHistory
-        )
-        .run(authContext)
-
-      // delete the namespace only after the helm uninstall completes.
-      _ <- deleteNamespace(client, kubernetesNamespace)
-
-      fa = namespaceExists(client, kubernetesNamespace)
-        .map(
-          !_
-        ) // mapping to inverse because booleanDoneCheckable defines `Done` when it becomes `true`...In this case, the namespace will exists for a while, and eventually becomes non-existent
-
-      _ <- streamUntilDoneOrTimeout(fa,
-                                    config.appMonitorConfig.deleteApp.maxAttempts,
-                                    config.appMonitorConfig.deleteApp.initialDelay,
-                                    "delete namespace timed out"
-      )
-
-      userEmail = app.auditInfo.creator
-      tokenOpt <- samDao.getCachedArbitraryPetAccessToken(userEmail)
-
-      _ <- tokenOpt match {
-        case Some(token) =>
-          for {
-            _ <- samDao.deleteResourceInternal(dbApp.app.samResourceId,
-                                               Authorization(Credentials.Token(AuthScheme.Bearer, token))
-            )
-
-          } yield ()
-        case None =>
-          logger.warn(
-            s"Could not find pet service account for user ${userEmail} in Sam. Skipping resource deletion in Sam."
-          )
-      }
-
-      _ <- logger.info(
-        s"Delete app operation has finished for app ${app.appName.value} in cluster ${clusterName}"
-      )
-
-      _ <- appQuery.updateStatus(app.id, AppStatus.Deleted).transaction
-
-      _ <- logger.info(s"Done deleting app $appName in workspace $workspaceId")
-    } yield ()
-
-  }
-
   private[util] def buildCoreV1Client(cloudContext: AzureCloudContext, clusterName: AKSClusterName): F[CoreV1Api] = {
     // we do not want to have to specify this at resource (class) creation time, so we create one on each load here
     implicit val traceId = Ask.const[F, TraceId](TraceId(UUID.randomUUID()))

diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/util/AzurePubsubHandler.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/util/AzurePubsubHandler.scala
@@ -783,14 +783,14 @@ class AzurePubsubHandlerInterp[F[_]: Parallel](
     appId: AppId,
     appName: AppName,
     workspaceId: WorkspaceId,
-    landingZoneResourcesOpt: Option[LandingZoneResources],
+    landingZoneResources: LandingZoneResources,
     cloudContext: AzureCloudContext
   )(implicit
     ev: Ask[F, AppContext]
   ): F[Unit] =
     for {
       ctx <- ev.ask
-      params = DeleteAKSAppParams(appName, workspaceId, landingZoneResourcesOpt, cloudContext)
+      params = DeleteAKSAppParams(appName, workspaceId, landingZoneResources, cloudContext)
       _ <- aksAlgebra.deleteApp(params).adaptError { case e =>
         PubsubKubernetesError(
           AppError(

diff --git a/...ain/scala/org/broadinstitute/dsde/workbench/leonardo/util/AzurePubsubHandlerAlgebra.scala b/...ain/scala/org/broadinstitute/dsde/workbench/leonardo/util/AzurePubsubHandlerAlgebra.scala
@@ -50,7 +50,7 @@ trait AzurePubsubHandlerAlgebra[F[_]] {
   def deleteApp(appId: AppId,
                 appName: AppName,
                 workspaceId: WorkspaceId,
-                landingZoneResourcesOpt: Option[LandingZoneResources],
+                landingZoneResources: LandingZoneResources,
                 cloudContext: AzureCloudContext
   )(implicit
     ev: Ask[F, AppContext]

diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/util/AKSInterpreterSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/util/AKSInterpreterSpec.scala
@@ -9,22 +9,11 @@ import com.azure.resourcemanager.compute.models.{VirtualMachineScaleSet, Virtual
 import com.azure.resourcemanager.containerservice.models.KubernetesCluster
 import com.azure.resourcemanager.msi.MsiManager
 import com.azure.resourcemanager.msi.models.{Identities, Identity}
-import io.kubernetes.client.openapi.ApiClient
 import io.kubernetes.client.openapi.apis.CoreV1Api
-import io.kubernetes.client.openapi.models.{
-  V1Namespace,
-  V1NamespaceList,
-  V1NamespaceStatus,
-  V1ObjectMeta,
-  V1Pod,
-  V1PodList,
-  V1PodStatus,
-  V1Status
-}
+import io.kubernetes.client.openapi.models._
 import org.broadinstitute.dsde.workbench.azure._
 import org.broadinstitute.dsde.workbench.azure.mock.FakeAzureRelayService
 import org.broadinstitute.dsde.workbench.google2.KubernetesSerializableName.{NamespaceName, ServiceAccountName}
-import org.broadinstitute.dsde.workbench.google2.mock.MockKubernetesService
 import org.broadinstitute.dsde.workbench.google2.{NetworkName, SubnetworkName}
 import org.broadinstitute.dsde.workbench.leonardo.CommonTestData.{landingZoneResources, workspaceId}
 import org.broadinstitute.dsde.workbench.leonardo.KubernetesTestData.{makeApp, makeKubeCluster, makeNodepool}
@@ -36,11 +25,10 @@ import org.broadinstitute.dsde.workbench.leonardo.db.{KubernetesServiceDbQueries
 import org.broadinstitute.dsde.workbench.leonardo.http.ConfigReader
 import org.broadinstitute.dsp.Release
 import org.broadinstitute.dsp.mocks.MockHelm
-import org.mockito.ArgumentMatchers.{any, anyBoolean, anyInt, anyString}
+import org.mockito.ArgumentMatchers.{any, anyString}
 import org.mockito.Mockito.when
 import org.scalatest.flatspec.AnyFlatSpecLike
 import org.scalatestplus.mockito.MockitoSugar
-import scalacache.Cache
 
 import java.nio.file.Files
 import java.util.{Base64, UUID}
@@ -139,6 +127,7 @@ class AKSInterpreterSpec extends AnyFlatSpecLike with TestComponent with Leonard
       "config.batchNodesSubnetId=subnet1," +
       s"config.drsUrl=${ConfigReader.appConfig.drs.url}," +
       "relay.path=https://relay.com/app," +
+      "persistence.storageResourceGroup=mrg," +
       "persistence.storageAccount=storage," +
       "persistence.blobContainer=sc-container," +
       "persistence.leoAppInstanceName=app," +
@@ -195,7 +184,7 @@ class AKSInterpreterSpec extends AnyFlatSpecLike with TestComponent with Leonard
     val app = dbApp.get.app
 
     val deletion = for {
-      _ <- aksInterp.deleteApp(DeleteAKSAppParams(app.appName, workspaceId, Option(landingZoneResources), cloudContext))
+      _ <- aksInterp.deleteApp(DeleteAKSAppParams(app.appName, workspaceId, landingZoneResources, cloudContext))
       app <- KubernetesServiceDbQueries
         .getActiveFullAppByName(CloudContext.Azure(cloudContext), app.appName)
         .transaction

diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/util/AKSManualTest.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/util/AKSManualTest.scala
@@ -2,10 +2,8 @@ package org.broadinstitute.dsde.workbench.leonardo.util
 
 import cats.effect.std.Semaphore
 import cats.effect.{IO, Resource}
-import io.kubernetes.client.openapi.ApiClient
 import org.broadinstitute.dsde.workbench.azure._
 import org.broadinstitute.dsde.workbench.google2.KubernetesSerializableName.{NamespaceName, ServiceAccountName}
-import org.broadinstitute.dsde.workbench.google2.mock.MockKubernetesService
 import org.broadinstitute.dsde.workbench.leonardo.CloudContext.Azure
 import org.broadinstitute.dsde.workbench.leonardo.CommonTestData.landingZoneResources
 import org.broadinstitute.dsde.workbench.leonardo.KubernetesTestData.{makeApp, makeKubeCluster, makeNodepool}
@@ -35,7 +33,6 @@ import org.broadinstitute.dsde.workbench.model.WorkbenchEmail
 import org.broadinstitute.dsp.{ChartName, HelmInterpreter, Release}
 import org.scalatestplus.mockito.MockitoSugar.mock
 import org.typelevel.log4cats.slf4j.Slf4jLogger
-import scalacache.Cache
 
 import java.util.UUID
 import scala.concurrent.ExecutionContext