From 8c71074e6a56800f9edb3b5bcacf627996fd991f Mon Sep 17 00:00:00 2001 From: Yonghao Yu Date: Wed, 14 Feb 2024 14:32:49 -0500 Subject: [PATCH 01/21] From original PR https://github.com/DataBiosphere/leonardo/pull/4232 --- .../leonardo/http/appRoutesModels.scala | 3 + .../workbench/leonardo/kubernetesModels.scala | 2 + http/src/main/resources/swagger/api-docs.yaml | 56 +++++++++++++++ .../workbench/leonardo/db/AppComponent.scala | 5 ++ .../leonardo/http/api/AppRoutes.scala | 19 +++++- .../leonardo/http/api/AppV2Routes.scala | 11 +++ .../leonardo/http/service/AppService.scala | 7 ++ .../http/service/LeoAppServiceInterp.scala | 68 +++++++++++++++++++ 8 files changed, 170 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala b/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala index 69720391dcc..a6ae1c5b442 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala @@ -39,6 +39,9 @@ final case class CreateAppRequest(kubernetesRuntimeConfig: Option[KubernetesRunt autodeleteEnabled: Option[Boolean] ) +final case class UpdateAppRequest(autodeleteThreshold: Option[Int], + autodeleteEnabled: Option[Boolean]) + final case class GetAppResponse( workspaceId: Option[WorkspaceId], appName: AppName, diff --git a/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/kubernetesModels.scala b/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/kubernetesModels.scala index a75136cfb25..475e78dab25 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/kubernetesModels.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/kubernetesModels.scala @@ -536,6 +536,8 @@ object AppStatus { val monitoredStatuses: Set[AppStatus] = Set(Deleting, Provisioning) + val updatableStatuses: Set[AppStatus] = Set(Running, Stopped) + implicit class EnrichedDiskStatus(status: AppStatus) { def isDeletable: Boolean = deletableStatuses contains status diff --git a/http/src/main/resources/swagger/api-docs.yaml b/http/src/main/resources/swagger/api-docs.yaml index ff87ed7d29f..a942fd704fd 100644 --- a/http/src/main/resources/swagger/api-docs.yaml +++ b/http/src/main/resources/swagger/api-docs.yaml @@ -1594,6 +1594,42 @@ paths: application/json: schema: $ref: "#/components/schemas/ErrorReport" + patch: + summary: Updates the configuration of an app + description: In order to update the configuration of an app, it must first be ready + operationId: updateApp + tags: + - apps + parameters: + - in: path + name: googleProject + description: googleProject + required: true + schema: + type: string + - in: path + name: appName + description: appName + required: true + schema: + type: string + requestBody: + $ref: "#/components/requestBodies/UpdateAppRequest" + responses: + "202": + description: App update request accepted + "400": + description: Bad Request + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorReport" + "500": + description: Internal Error + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorReport" delete: summary: Deletes an existing app in the given project description: deletes an App @@ -2914,6 +2950,14 @@ components: machineSize: "Standard_DS1_v2" disk: { labels: {}, name: "disk1", size: 50 } autopauseThreshold: 15 + UpdateAppRequest: + content: + application/json: + schema: + $ref: "#/components/schemas/UpdateAppRequest" + example: + authdeleteThrehold: 15 + authdeleteThreholdEnabled: true UpdateAppsRequest: content: application/json: @@ -4110,6 +4154,18 @@ components: type: boolean description: Whether to turn on autodelete + UpdateAppRequest: + description: a request to update an app + type: object + properties: + autodeleteThreshold: + type: integer + description: The number of minutes of idle time to elapse before the app is + deleted in minute. When autodeleteEnabled is true, a positive integer is required + autodeleteEnabled: + type: boolean + description: Whether to turn on autodelete + UpdateAppsRequest: description: a request to update a specific set of apps (v1 or v2) required: diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/AppComponent.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/AppComponent.scala index 8f9c33b8a09..70a7b8e906f 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/AppComponent.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/AppComponent.scala @@ -302,6 +302,11 @@ object appQuery extends TableQuery(new AppTable(_)) { .map(_.status) .update(status) + def updateAutodelete(id: AppId, autodeleteEnabled: Boolean, autodeleteThreshold: Option[Int]): DBIO[Int] = + getByIdQuery(id) + .map(x => (x.autodeleteEnabled, x.autodeleteThreshold)) + .update(autodeleteEnabled, autodeleteThreshold) + def markAsErrored(id: AppId): DBIO[Int] = getByIdQuery(id) .map(x => (x.status, x.diskId)) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala index 6ef038626f1..d5444dcd3bb 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala @@ -14,7 +14,8 @@ import io.opencensus.scala.akka.http.TracingDirective.traceRequestForService import org.broadinstitute.dsde.workbench.leonardo.http.api.AppV2Routes.{ createAppDecoder, getAppResponseEncoder, - listAppResponseEncoder + listAppResponseEncoder, + updateAppRequestDecoder } import org.broadinstitute.dsde.workbench.leonardo.http.service.AppService import org.broadinstitute.dsde.workbench.model.UserInfo @@ -71,6 +72,13 @@ class AppRoutes(kubernetesService: AppService[IO], userInfoDirectives: UserInfoD ) ) } ~ + patch { + entity(as[UpdateAppRequest]) { req => + complete( + updateAppHandler(userInfo, googleProject, appName, req) + ) + } + } ~ delete { parameterMap { params => complete( @@ -167,6 +175,15 @@ class AppRoutes(kubernetesService: AppService[IO], userInfoDirectives: UserInfoD resp <- ctx.span.fold(apiCall)(span => spanResource[IO](span, "listApp").use(_ => apiCall)) } yield StatusCodes.OK -> resp + private[api] def updateAppHandler(userInfo: UserInfo, googleProject: GoogleProject, appName: AppName, req: UpdateAppRequest)(implicit + ev: Ask[IO, AppContext] + ): IO[ToResponseMarshallable] = + for { + ctx <- ev.ask[AppContext] + apiCall = kubernetesService.updateApp(userInfo, CloudContext.Gcp(googleProject), appName, req) + _ <- ctx.span.fold(apiCall)(span => spanResource[IO](span, "updateApp").use(_ => apiCall)) + } yield StatusCodes.Accepted + private[api] def deleteAppHandler(userInfo: UserInfo, googleProject: GoogleProject, appName: AppName, diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppV2Routes.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppV2Routes.scala index f64945d3e52..b7816f29dae 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppV2Routes.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppV2Routes.scala @@ -212,6 +212,17 @@ object AppV2Routes { ) } + implicit val updateAppRequestDecoder: Decoder[UpdateAppRequest] = + Decoder.instance { x => + for { + adtm <- x.downField("autodeleteThreshold").as[Option[Int]] + adte <- x.downField("autodeleteEnabled").as[Option[Boolean]] + } yield UpdateAppRequest( + adtm, + adte + ) + } + implicit val nameKeyEncoder: KeyEncoder[ServiceName] = KeyEncoder.encodeKeyString.contramap(_.value) implicit val listAppResponseEncoder: Encoder[ListAppResponse] = Encoder.forProduct16( diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppService.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppService.scala index 0a6ccec179f..f400f11b325 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppService.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppService.scala @@ -29,6 +29,13 @@ trait AppService[F[_]] { params: Map[String, String] )(implicit as: Ask[F, AppContext]): F[Vector[ListAppResponse]] + def updateApp( + userInfo: UserInfo, + cloudContext: CloudContext.Gcp, + appName: AppName, + req: UpdateAppRequest, + )(implicit as: Ask[F, AppContext]): F[Unit] + def deleteApp(userInfo: UserInfo, cloudContext: CloudContext.Gcp, appName: AppName, deleteDisk: Boolean)(implicit as: Ask[F, AppContext] ): F[Unit] diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index ee4fa821be4..3b2c2d2d70b 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -669,6 +669,61 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, .raiseError[Unit](AppNotFoundByWorkspaceIdException(workspaceId, appName, ctx.traceId, "permission denied")) } yield GetAppResponse.fromDbResult(app, Config.proxyConfig.proxyUrlBase) + override def updateApp(userInfo: UserInfo, cloudContext: CloudContext.Gcp, appName: AppName, req: UpdateAppRequest)( + implicit as: Ask[F, AppContext] + ): F[Unit] = + for { + ctx <- as.ask + // throw 403 if no project-level permission + hasProjectPermission <- authProvider.isUserProjectReader( + cloudContext, + userInfo + ) + _ <- F.raiseWhen(!hasProjectPermission)(ForbiddenError(userInfo.userEmail, Some(ctx.traceId))) + + appOpt <- KubernetesServiceDbQueries + .getActiveFullAppByName(cloudContext, appName) + .transaction + appResult <- F.fromOption( + appOpt, + AppNotFoundException(cloudContext, appName, ctx.traceId, "No active app found in DB") + ) + tags = Map("appType" -> appResult.app.appType.toString) + _ <- metrics.incrementCounter("updateApp", 1, tags) + listOfPermissions <- authProvider.getActions(appResult.app.samResourceId, userInfo) + + // throw 404 if no GetAppStatus permission + hasPermission = listOfPermissions.toSet.contains(AppAction.UpdateApp) + _ <- + if (hasPermission) F.unit + else + F.raiseError[Unit]( + AppNotFoundException(cloudContext, appName, ctx.traceId, "Permission Denied") + ) + + + canUpdate = AppStatus.updatableStatuses.contains(appResult.app.status) + _ <- + if (canUpdate) F.unit + else + F.raiseError[Unit]( + AppCannotBeStoppedException(cloudContext, appName, appResult.app.status, ctx.traceId) + ) + + // auto delete + autodeleteEnabled = req.autodeleteEnabled.getOrElse(false) + autodeleteThreshold = req.autodeleteThreshold.getOrElse(0) + _ <- Either.cond(!(autodeleteEnabled && autodeleteThreshold <= 0), + (), + BadRequestException("autodeleteThreshold should be a positive value", Some(ctx.traceId)) + ) + _ <- + if (appResult.app.autodeleteEnabled != autodeleteEnabled || appResult.app.autodeleteThreshold != req.autodeleteThreshold) + appQuery.updateAutodelete(appResult.app.id, autodeleteEnabled, req.autodeleteThreshold).transaction.void + else Async[F].unit + + } yield () + override def createAppV2(userInfo: UserInfo, workspaceId: WorkspaceId, appName: AppName, req: CreateAppRequest)( implicit as: Ask[F, AppContext] ): F[Unit] = @@ -1649,6 +1704,19 @@ case class AppAlreadyExistsException(cloudContext: CloudContext, appName: AppNam traceId = Some(traceId) ) +case class AppCannotBeUpdatedException(cloudContext: CloudContext, + appName: AppName, + status: AppStatus, + traceId: TraceId, + extraMsg: String = "" + ) extends LeoException( + s"App ${cloudContext.asStringWithProvider}/${appName.value} cannot be updated in ${status} status." + + (if (status == AppStatus.Stopped) " Please start the app first." else ""), + StatusCodes.Conflict, + traceId = Some(traceId), + extraMessageInLogging = extraMsg +) + case class AppCannotBeDeletedException(cloudContext: CloudContext, appName: AppName, status: AppStatus, From d989c2e0c9f799997680ec702652d976e5bf39f2 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Mon, 20 May 2024 17:28:54 -0400 Subject: [PATCH 02/21] UpdateAppConfig and tests --- .../leonardo/http/appRoutesModels.scala | 3 +- .../workbench/leonardo/kubernetesModels.scala | 2 - http/src/main/resources/swagger/api-docs.yaml | 26 +-- .../workbench/leonardo/db/AppComponent.scala | 9 +- .../leonardo/http/api/AppRoutes.scala | 42 +++- .../leonardo/http/api/AppV2Routes.scala | 11 - .../leonardo/http/service/AppService.scala | 12 +- .../http/service/LeoAppServiceInterp.scala | 98 +++++---- .../http/service/AppServiceInterpSpec.scala | 205 +++++++++++++++++- .../http/service/MockAppService.scala | 5 + 10 files changed, 317 insertions(+), 96 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala b/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala index a6ae1c5b442..10ccf0ca43e 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala @@ -39,8 +39,7 @@ final case class CreateAppRequest(kubernetesRuntimeConfig: Option[KubernetesRunt autodeleteEnabled: Option[Boolean] ) -final case class UpdateAppRequest(autodeleteThreshold: Option[Int], - autodeleteEnabled: Option[Boolean]) +final case class UpdateAppConfigRequest(autodeleteEnabled: Option[Boolean], autodeleteThreshold: Option[Int]) final case class GetAppResponse( workspaceId: Option[WorkspaceId], diff --git a/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/kubernetesModels.scala b/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/kubernetesModels.scala index 475e78dab25..a75136cfb25 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/kubernetesModels.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/kubernetesModels.scala @@ -536,8 +536,6 @@ object AppStatus { val monitoredStatuses: Set[AppStatus] = Set(Deleting, Provisioning) - val updatableStatuses: Set[AppStatus] = Set(Running, Stopped) - implicit class EnrichedDiskStatus(status: AppStatus) { def isDeletable: Boolean = deletableStatuses contains status diff --git a/http/src/main/resources/swagger/api-docs.yaml b/http/src/main/resources/swagger/api-docs.yaml index a942fd704fd..33994be9574 100644 --- a/http/src/main/resources/swagger/api-docs.yaml +++ b/http/src/main/resources/swagger/api-docs.yaml @@ -1596,8 +1596,8 @@ paths: $ref: "#/components/schemas/ErrorReport" patch: summary: Updates the configuration of an app - description: In order to update the configuration of an app, it must first be ready - operationId: updateApp + description: Updates the configuration of an app. Currently limited to autodeletion config. + operationId: updateAppConfig tags: - apps parameters: @@ -1614,10 +1614,10 @@ paths: schema: type: string requestBody: - $ref: "#/components/requestBodies/UpdateAppRequest" + $ref: "#/components/requestBodies/UpdateAppConfigRequest" responses: "202": - description: App update request accepted + description: App configuration update request accepted "400": description: Bad Request content: @@ -2950,14 +2950,14 @@ components: machineSize: "Standard_DS1_v2" disk: { labels: {}, name: "disk1", size: 50 } autopauseThreshold: 15 - UpdateAppRequest: + UpdateAppConfigRequest: content: application/json: schema: - $ref: "#/components/schemas/UpdateAppRequest" + $ref: "#/components/schemas/UpdateAppConfigRequest" example: - authdeleteThrehold: 15 - authdeleteThreholdEnabled: true + autodeleteEnabled: true + autodeleteThreshold: 60 UpdateAppsRequest: content: application/json: @@ -4154,17 +4154,17 @@ components: type: boolean description: Whether to turn on autodelete - UpdateAppRequest: - description: a request to update an app + UpdateAppConfigRequest: + description: a request to update the configuration of an app type: object properties: + autodeleteEnabled: + type: boolean + description: Whether to turn on autodelete autodeleteThreshold: type: integer description: The number of minutes of idle time to elapse before the app is deleted in minute. When autodeleteEnabled is true, a positive integer is required - autodeleteEnabled: - type: boolean - description: Whether to turn on autodelete UpdateAppsRequest: description: a request to update a specific set of apps (v1 or v2) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/AppComponent.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/AppComponent.scala index 70a7b8e906f..23d7bdc8eef 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/AppComponent.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/AppComponent.scala @@ -302,10 +302,11 @@ object appQuery extends TableQuery(new AppTable(_)) { .map(_.status) .update(status) - def updateAutodelete(id: AppId, autodeleteEnabled: Boolean, autodeleteThreshold: Option[Int]): DBIO[Int] = - getByIdQuery(id) - .map(x => (x.autodeleteEnabled, x.autodeleteThreshold)) - .update(autodeleteEnabled, autodeleteThreshold) + def updateAutodeleteEnabled(id: AppId, autodeleteEnabled: Boolean): DBIO[Int] = + getByIdQuery(id).map(_.autodeleteEnabled).update(autodeleteEnabled) + + def updateAutodeleteThreshold(id: AppId, autodeleteThreshold: Option[Int]): DBIO[Int] = + getByIdQuery(id).map(_.autodeleteThreshold).update(autodeleteThreshold) def markAsErrored(id: AppId): DBIO[Int] = getByIdQuery(id) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala index d5444dcd3bb..e4d67d1d66d 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala @@ -5,17 +5,17 @@ package api import akka.http.scaladsl.marshalling.ToResponseMarshallable import akka.http.scaladsl.model.StatusCodes import akka.http.scaladsl.server -import akka.http.scaladsl.server.Directives.{pathEndOrSingleSlash, _} +import akka.http.scaladsl.server.Directives._ import cats.effect.IO import cats.mtl.Ask import de.heikoseeberger.akkahttpcirce.ErrorAccumulatingCirceSupport._ import io.circe.Decoder import io.opencensus.scala.akka.http.TracingDirective.traceRequestForService +import org.broadinstitute.dsde.workbench.leonardo.http.api.AppRoutes.updateAppConfigRequestDecoder import org.broadinstitute.dsde.workbench.leonardo.http.api.AppV2Routes.{ createAppDecoder, getAppResponseEncoder, - listAppResponseEncoder, - updateAppRequestDecoder + listAppResponseEncoder } import org.broadinstitute.dsde.workbench.leonardo.http.service.AppService import org.broadinstitute.dsde.workbench.model.UserInfo @@ -73,9 +73,9 @@ class AppRoutes(kubernetesService: AppService[IO], userInfoDirectives: UserInfoD ) } ~ patch { - entity(as[UpdateAppRequest]) { req => + entity(as[UpdateAppConfigRequest]) { req => complete( - updateAppHandler(userInfo, googleProject, appName, req) + updateAppConfigHandler(userInfo, googleProject, appName, req) ) } } ~ @@ -175,12 +175,28 @@ class AppRoutes(kubernetesService: AppService[IO], userInfoDirectives: UserInfoD resp <- ctx.span.fold(apiCall)(span => spanResource[IO](span, "listApp").use(_ => apiCall)) } yield StatusCodes.OK -> resp - private[api] def updateAppHandler(userInfo: UserInfo, googleProject: GoogleProject, appName: AppName, req: UpdateAppRequest)(implicit - ev: Ask[IO, AppContext] - ): IO[ToResponseMarshallable] = + private[api] def updateAppConfigHandler(userInfo: UserInfo, + googleProject: GoogleProject, + appName: AppName, + req: UpdateAppConfigRequest + )(implicit ev: Ask[IO, AppContext]): IO[ToResponseMarshallable] = + for { + _ <- foldSpan(kubernetesService.updateAppConfig(userInfo, CloudContext.Gcp(googleProject), appName, req), + "updateAppConfig" + ) + } yield StatusCodes.Accepted + + // TODO: this pattern is repeated over 20x + // ctx <- ev.ask[AppContext] + // apiCall = whatever + // _ <- ctx.span.fold(apiCall)(span => spanResource[IO](span, "methodName").use(_ => apiCall)) + // + // My intuition is that it's not necessary for all developers to have a deep understanding of what this is doing, + // so I want to abstract this implementation detail away for better ergonomics/approachability. Thoughts? + private def foldSpan(apiCall: IO[Unit], apiName: String)(implicit ev: Ask[IO, AppContext]): IO[Unit] = for { ctx <- ev.ask[AppContext] - apiCall = kubernetesService.updateApp(userInfo, CloudContext.Gcp(googleProject), appName, req) + apiCall = kubernetesService.updateAppConfig(userInfo, CloudContext.Gcp(googleProject), appName, req) _ <- ctx.span.fold(apiCall)(span => spanResource[IO](span, "updateApp").use(_ => apiCall)) } yield StatusCodes.Accepted @@ -231,4 +247,12 @@ object AppRoutes { case _ => Right(NumNodepools.apply(n)) } ) + + implicit val updateAppConfigRequestDecoder: Decoder[UpdateAppConfigRequest] = + Decoder.instance { x => + for { + enabled <- x.downField("autodeleteEnabled").as[Option[Boolean]] + threshold <- x.downField("autodeleteThreshold").as[Option[Int]] + } yield UpdateAppConfigRequest(enabled, threshold) + } } diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppV2Routes.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppV2Routes.scala index b7816f29dae..f64945d3e52 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppV2Routes.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppV2Routes.scala @@ -212,17 +212,6 @@ object AppV2Routes { ) } - implicit val updateAppRequestDecoder: Decoder[UpdateAppRequest] = - Decoder.instance { x => - for { - adtm <- x.downField("autodeleteThreshold").as[Option[Int]] - adte <- x.downField("autodeleteEnabled").as[Option[Boolean]] - } yield UpdateAppRequest( - adtm, - adte - ) - } - implicit val nameKeyEncoder: KeyEncoder[ServiceName] = KeyEncoder.encodeKeyString.contramap(_.value) implicit val listAppResponseEncoder: Encoder[ListAppResponse] = Encoder.forProduct16( diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppService.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppService.scala index f400f11b325..88553d9788f 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppService.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppService.scala @@ -29,12 +29,12 @@ trait AppService[F[_]] { params: Map[String, String] )(implicit as: Ask[F, AppContext]): F[Vector[ListAppResponse]] - def updateApp( - userInfo: UserInfo, - cloudContext: CloudContext.Gcp, - appName: AppName, - req: UpdateAppRequest, - )(implicit as: Ask[F, AppContext]): F[Unit] + def updateAppConfig( + userInfo: UserInfo, + cloudContext: CloudContext.Gcp, + appName: AppName, + req: UpdateAppConfigRequest + )(implicit as: Ask[F, AppContext]): F[Unit] def deleteApp(userInfo: UserInfo, cloudContext: CloudContext.Gcp, appName: AppName, deleteDisk: Boolean)(implicit as: Ask[F, AppContext] diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index 3b2c2d2d70b..de2221cf0b8 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -669,11 +669,54 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, .raiseError[Unit](AppNotFoundByWorkspaceIdException(workspaceId, appName, ctx.traceId, "permission denied")) } yield GetAppResponse.fromDbResult(app, Config.proxyConfig.proxyUrlBase) - override def updateApp(userInfo: UserInfo, cloudContext: CloudContext.Gcp, appName: AppName, req: UpdateAppRequest)( - implicit as: Ask[F, AppContext] - ): F[Unit] = + // Autodeletion config logic requires a positive integer for threshold if enabled is true. + // For simplicity we can enforce this by rejecting the following scenarios: + // 1. The request includes a threshold which is invalid + // 2. The request includes enabled=true but no threshold is provided, and the existing DB threshold is invalid + private def validateUpdateAppConfigRequest(req: UpdateAppConfigRequest, dbApp: App)(implicit + as: Ask[F, AppContext] + ): F[Unit] = for { + ctx <- as.ask + + invalidThresholdRequest = req.autodeleteThreshold.exists(_ <= 0) + _ <- F.raiseWhen(invalidThresholdRequest)( + BadRequestException("invalid value for autodeleteThreshold", Some(ctx.traceId)) + ) + + wantEnableButDbInvalid = req.autodeleteEnabled.getOrElse(false) && dbApp.autodeleteThreshold.exists(_ <= 0) + _ <- F.raiseWhen(wantEnableButDbInvalid)( + BadRequestException( + "when enabling autodelete without passing an explicit autodeleteThreshold, the existing config must be valid", + Some(ctx.traceId) + ) + ) + } yield () + + private def updateAppConfigInternal(appId: AppId, validatedChanges: UpdateAppConfigRequest): F[Unit] = for { + _ <- validatedChanges.autodeleteEnabled.fold(F.unit)(enabled => + appQuery + .updateAutodeleteEnabled(appId, enabled) + .transaction + .void + ) + + // note: does not clear the threshold if None. This only sets defined thresholds. + _ <- validatedChanges.autodeleteThreshold.fold(F.unit)(threshold => + appQuery + .updateAutodeleteThreshold(appId, Some(threshold)) + .transaction + .void + ) + } yield () + + override def updateAppConfig(userInfo: UserInfo, + cloudContext: CloudContext.Gcp, + appName: AppName, + req: UpdateAppConfigRequest + )(implicit as: Ask[F, AppContext]): F[Unit] = for { ctx <- as.ask + // throw 403 if no project-level permission hasProjectPermission <- authProvider.isUserProjectReader( cloudContext, @@ -688,40 +731,18 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, appOpt, AppNotFoundException(cloudContext, appName, ctx.traceId, "No active app found in DB") ) + tags = Map("appType" -> appResult.app.appType.toString) - _ <- metrics.incrementCounter("updateApp", 1, tags) + _ <- metrics.incrementCounter("updateAppConfig", 1, tags) listOfPermissions <- authProvider.getActions(appResult.app.samResourceId, userInfo) - // throw 404 if no GetAppStatus permission + // throw 404 if no UpdateApp permission hasPermission = listOfPermissions.toSet.contains(AppAction.UpdateApp) - _ <- - if (hasPermission) F.unit - else - F.raiseError[Unit]( - AppNotFoundException(cloudContext, appName, ctx.traceId, "Permission Denied") - ) - - - canUpdate = AppStatus.updatableStatuses.contains(appResult.app.status) - _ <- - if (canUpdate) F.unit - else - F.raiseError[Unit]( - AppCannotBeStoppedException(cloudContext, appName, appResult.app.status, ctx.traceId) - ) - - // auto delete - autodeleteEnabled = req.autodeleteEnabled.getOrElse(false) - autodeleteThreshold = req.autodeleteThreshold.getOrElse(0) - _ <- Either.cond(!(autodeleteEnabled && autodeleteThreshold <= 0), - (), - BadRequestException("autodeleteThreshold should be a positive value", Some(ctx.traceId)) - ) - _ <- - if (appResult.app.autodeleteEnabled != autodeleteEnabled || appResult.app.autodeleteThreshold != req.autodeleteThreshold) - appQuery.updateAutodelete(appResult.app.id, autodeleteEnabled, req.autodeleteThreshold).transaction.void - else Async[F].unit + _ <- F.raiseWhen(!hasPermission)(AppNotFoundException(cloudContext, appName, ctx.traceId, "Permission Denied")) + // confirm that the combination of the request and the existing DB values result in a valid configuration + _ <- validateUpdateAppConfigRequest(req, appResult.app) + _ <- updateAppConfigInternal(appResult.app.id, req) } yield () override def createAppV2(userInfo: UserInfo, workspaceId: WorkspaceId, appName: AppName, req: CreateAppRequest)( @@ -1704,19 +1725,6 @@ case class AppAlreadyExistsException(cloudContext: CloudContext, appName: AppNam traceId = Some(traceId) ) -case class AppCannotBeUpdatedException(cloudContext: CloudContext, - appName: AppName, - status: AppStatus, - traceId: TraceId, - extraMsg: String = "" - ) extends LeoException( - s"App ${cloudContext.asStringWithProvider}/${appName.value} cannot be updated in ${status} status." + - (if (status == AppStatus.Stopped) " Please start the app first." else ""), - StatusCodes.Conflict, - traceId = Some(traceId), - extraMessageInLogging = extraMsg -) - case class AppCannotBeDeletedException(cloudContext: CloudContext, appName: AppName, status: AppStatus, diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala index bfe40282bb0..511af2c7eef 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala @@ -15,7 +15,7 @@ import org.broadinstitute.dsde.workbench.leonardo.AppRestore.{GalaxyRestore, Oth import org.broadinstitute.dsde.workbench.leonardo.CommonTestData._ import org.broadinstitute.dsde.workbench.leonardo.KubernetesTestData._ import org.broadinstitute.dsde.workbench.leonardo.SamResourceId.AppSamResourceId -import org.broadinstitute.dsde.workbench.leonardo.TestUtils.appContext +import org.broadinstitute.dsde.workbench.leonardo.TestUtils.{appContext, defaultMockitoAnswer} import org.broadinstitute.dsde.workbench.leonardo.auth.AllowlistAuthProvider import org.broadinstitute.dsde.workbench.leonardo.config.Config.leoKubernetesConfig import org.broadinstitute.dsde.workbench.leonardo.config.{Config, CustomAppConfig, CustomApplicationAllowListConfig} @@ -36,7 +36,7 @@ import org.broadinstitute.dsde.workbench.leonardo.monitor.{ } import org.broadinstitute.dsde.workbench.leonardo.util.QueueFactory import org.broadinstitute.dsde.workbench.model.google.GoogleProject -import org.broadinstitute.dsde.workbench.model.{TraceId, WorkbenchEmail} +import org.broadinstitute.dsde.workbench.model.{TraceId, UserInfo, WorkbenchEmail} import org.broadinstitute.dsde.workbench.util2.messaging.CloudPublisher import org.broadinstitute.dsp.{ChartName, ChartVersion} import org.http4s.Uri @@ -49,7 +49,7 @@ import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.prop.TableDrivenPropertyChecks._ import org.scalatestplus.mockito.MockitoSugar import org.typelevel.log4cats.StructuredLogger - +import org.scalatest.prop.TableDrivenPropertyChecks.forAll import java.time.Instant import scala.concurrent.ExecutionContext.Implicits.global @@ -3107,7 +3107,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le thrown.appType shouldBe AppType.HailBatch } - "checkIfAppCreationIsAllowedAndIsAoU" should "enable IntraNodeVisibility if customApp check is disabled" in { + "checkIfAppCreationIsAllowed" should "enable IntraNodeVisibility if customApp check is disabled" in { val interp = makeInterp(QueueFactory.makePublisherQueue(), enableCustomAppCheckFlag = false) val res = interp.checkIfAppCreationIsAllowed(userEmail, project, Uri.unsafeFromString("https://dummy")) @@ -3161,4 +3161,201 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le .get .isInstanceOf[ForbiddenError] shouldBe true } + + def setupAppWithConfig(autodeleteEnabled: Boolean, autodeleteThreshold: Option[Int]): AppName = { + val appName = AppName("pong") + val createReq = createAppRequest.copy( + diskConfig = Some(PersistentDiskRequest(diskName, None, None, Map.empty)), + autodeleteEnabled = Some(autodeleteEnabled), + autodeleteThreshold = autodeleteThreshold + ) + + appServiceInterp + .createApp(userInfo, cloudContextGcp, appName, createReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + val createdApp = appServiceInterp + .getApp(userInfo, cloudContextGcp, appName) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + // confirm assumptions + + createdApp.autodeleteEnabled shouldBe autodeleteEnabled + createdApp.autodeleteThreshold shouldBe autodeleteThreshold + + appName + } + + "updateAppConfig" should "allow enabling autodeletion by specifying both fields" in isolatedDbTest { + val initialAutodeleteEnabled = false + val appName = setupAppWithConfig(initialAutodeleteEnabled, None) + + val autodeleteThreshold = 1000 + val updateReq = UpdateAppConfigRequest(Some(true), Some(autodeleteThreshold)) + appServiceInterp + .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + // confirm the update + + val updatedApp = appServiceInterp + .getApp(userInfo, cloudContextGcp, appName) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + updatedApp.autodeleteEnabled shouldBe true + updatedApp.autodeleteThreshold shouldBe Some(autodeleteThreshold) + } + + it should "allow enabling autodeletion by setting autodeleteEnabled=true if the existing config has a valid autodeleteThreshold" in isolatedDbTest { + val initialAutodeleteEnabled = false + val autodeleteThreshold = 1000 + val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + + val updateReq = UpdateAppConfigRequest(Some(true), None) + appServiceInterp + .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + // confirm the update + + val updatedApp = appServiceInterp + .getApp(userInfo, cloudContextGcp, appName) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + updatedApp.autodeleteEnabled shouldBe true + updatedApp.autodeleteThreshold shouldBe Some(autodeleteThreshold) + } + + it should "reject enabling autodeletion by setting autodeleteEnabled=true if the existing config has an invalid autodeleteThreshold" in isolatedDbTest { + val initialAutodeleteEnabled = false + val badAutodeleteThreshold = 0 + val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(badAutodeleteThreshold)) + + val updateReq = UpdateAppConfigRequest(Some(true), None) + a[BadRequestException] should be thrownBy { + appServiceInterp + .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + } + } + + it should "reject enabling autodeletion if the threshold in the request is invalid" in isolatedDbTest { + val initialAutodeleteEnabled = false + val appName = setupAppWithConfig(initialAutodeleteEnabled, None) + + val badAutodeleteThreshold = 0 + val updateReq = UpdateAppConfigRequest(Some(true), Some(badAutodeleteThreshold)) + a[BadRequestException] should be thrownBy { + appServiceInterp + .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + } + } + + it should "reject disabling autodeletion if the threshold in the request is invalid" in isolatedDbTest { + val initialAutodeleteEnabled = true + val autodeleteThreshold = 1000 + val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + + val badAutodeleteThreshold = 0 + val updateReq = UpdateAppConfigRequest(Some(false), Some(badAutodeleteThreshold)) + a[BadRequestException] should be thrownBy { + appServiceInterp + .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + } + } + + it should "reject autodeletionEnabled=None if the threshold in the request is invalid" in isolatedDbTest { + val initialAutodeleteEnabled = true + val autodeleteThreshold = 1000 + val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + + val badAutodeleteThreshold = 0 + val updateReq = UpdateAppConfigRequest(None, Some(badAutodeleteThreshold)) + a[BadRequestException] should be thrownBy { + appServiceInterp + .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + } + } + + it should "allow disabling autodeletion by setting enabled = false" in isolatedDbTest { + val initialAutodeleteEnabled = true + val autodeleteThreshold = 1000 + val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + + val updateReq = UpdateAppConfigRequest(Some(false), None) + appServiceInterp + .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + // confirm the update + + val updatedApp = appServiceInterp + .getApp(userInfo, cloudContextGcp, appName) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + updatedApp.autodeleteEnabled shouldBe false + updatedApp.autodeleteThreshold shouldBe Some(autodeleteThreshold) + } + + it should "allow disabling autodeletion by setting enabled = false and specifying a new autodeleteThreshold" in isolatedDbTest { + val initialAutodeleteEnabled = true + val autodeleteThreshold = 1000 + val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + + val newThreshold = 2000 + val updateReq = UpdateAppConfigRequest(Some(false), Some(newThreshold)) + appServiceInterp + .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + // confirm the update + + val updatedApp = appServiceInterp + .getApp(userInfo, cloudContextGcp, appName) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + updatedApp.autodeleteEnabled shouldBe false + updatedApp.autodeleteThreshold shouldBe Some(newThreshold) + } + + it should "allow changing autodeletion threshold" in isolatedDbTest { + val initialAutodeleteEnabled = true + val autodeleteThreshold = 1000 + val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + + val newAutodeleteThreshold = 2000 + val updateReq = UpdateAppConfigRequest(Some(true), Some(newAutodeleteThreshold)) + appServiceInterp + .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + // confirm the update + + val updatedApp = appServiceInterp + .getApp(userInfo, cloudContextGcp, appName) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + updatedApp.autodeleteEnabled shouldBe true + updatedApp.autodeleteThreshold shouldBe Some(newAutodeleteThreshold) + } + + it should "error if app not found" in isolatedDbTest { + val appName = AppName("app1") + val createDiskConfig = PersistentDiskRequest(diskName, None, None, Map.empty) + val appReq = createAppRequest.copy(diskConfig = Some(createDiskConfig)) + + appServiceInterp + .createApp(userInfo, cloudContextGcp, appName, appReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + val wrongApp = AppName("wrongApp") + an[AppNotFoundException] should be thrownBy { + appServiceInterp + .updateAppConfig(userInfo, cloudContextGcp, wrongApp, UpdateAppConfigRequest(None, None)) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + } + } } diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/MockAppService.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/MockAppService.scala index 53e9c6dc022..b8a58eff5d4 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/MockAppService.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/MockAppService.scala @@ -80,6 +80,11 @@ class MockAppService extends AppService[IO] { ): IO[Unit] = IO.unit + override def updateAppConfig(userInfo: UserInfo, + cloudContext: CloudContext.Gcp, + appName: AppName, + req: UpdateAppConfigRequest + )(implicit as: Ask[IO, AppContext]): IO[Unit] = IO.unit } object MockAppService extends MockAppService From b8ac4af033c03bf5b557e7aa92da6b7ebdb17312 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Tue, 21 May 2024 11:11:40 -0400 Subject: [PATCH 03/21] foldSpan --- .../dsde/workbench/leonardo/http/api/AppRoutes.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala index e4d67d1d66d..fc67edea6d7 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala @@ -18,6 +18,7 @@ import org.broadinstitute.dsde.workbench.leonardo.http.api.AppV2Routes.{ listAppResponseEncoder } import org.broadinstitute.dsde.workbench.leonardo.http.service.AppService +import org.broadinstitute.dsde.workbench.leonardo.http.spanResource import org.broadinstitute.dsde.workbench.model.UserInfo import org.broadinstitute.dsde.workbench.model.google.GoogleProject import org.broadinstitute.dsde.workbench.openTelemetry.OpenTelemetryMetrics @@ -196,9 +197,8 @@ class AppRoutes(kubernetesService: AppService[IO], userInfoDirectives: UserInfoD private def foldSpan(apiCall: IO[Unit], apiName: String)(implicit ev: Ask[IO, AppContext]): IO[Unit] = for { ctx <- ev.ask[AppContext] - apiCall = kubernetesService.updateAppConfig(userInfo, CloudContext.Gcp(googleProject), appName, req) - _ <- ctx.span.fold(apiCall)(span => spanResource[IO](span, "updateApp").use(_ => apiCall)) - } yield StatusCodes.Accepted + _ <- ctx.span.fold(apiCall)(span => spanResource[IO](span, apiName).use(_ => apiCall)) + } yield () private[api] def deleteAppHandler(userInfo: UserInfo, googleProject: GoogleProject, From 0929dfa2bf129a1221f6bc275c15d32fe5397a59 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Thu, 23 May 2024 16:41:11 -0400 Subject: [PATCH 04/21] add test --- .../http/service/AppServiceInterpSpec.scala | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala index 511af2c7eef..72acbe4f7ba 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala @@ -3321,7 +3321,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le updatedApp.autodeleteThreshold shouldBe Some(newThreshold) } - it should "allow changing autodeletion threshold" in isolatedDbTest { + it should "allow changing autodeletion threshold when enableAutodelete=true" in isolatedDbTest { val initialAutodeleteEnabled = true val autodeleteThreshold = 1000 val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) @@ -3342,6 +3342,27 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le updatedApp.autodeleteThreshold shouldBe Some(newAutodeleteThreshold) } + it should "allow changing autodeletion threshold when enableAutodelete=None" in isolatedDbTest { + val initialAutodeleteEnabled = true + val autodeleteThreshold = 1000 + val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + + val newAutodeleteThreshold = 2000 + val updateReq = UpdateAppConfigRequest(None, Some(newAutodeleteThreshold)) + appServiceInterp + .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + // confirm the update + + val updatedApp = appServiceInterp + .getApp(userInfo, cloudContextGcp, appName) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + + updatedApp.autodeleteEnabled shouldBe true + updatedApp.autodeleteThreshold shouldBe Some(newAutodeleteThreshold) + } + it should "error if app not found" in isolatedDbTest { val appName = AppName("app1") val createDiskConfig = PersistentDiskRequest(diskName, None, None, Map.empty) From 4b41ea8081b76bd9f02d1c8c233c61f8210a2980 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Thu, 23 May 2024 17:19:18 -0400 Subject: [PATCH 05/21] logic fix --- .../leonardo/http/service/LeoAppServiceInterp.scala | 9 +++++++-- .../leonardo/http/service/AppServiceInterpSpec.scala | 12 ++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index de2221cf0b8..934e1c174a6 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -678,13 +678,18 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, ): F[Unit] = for { ctx <- as.ask + // 1. The request includes a threshold which is invalid invalidThresholdRequest = req.autodeleteThreshold.exists(_ <= 0) _ <- F.raiseWhen(invalidThresholdRequest)( BadRequestException("invalid value for autodeleteThreshold", Some(ctx.traceId)) ) - wantEnableButDbInvalid = req.autodeleteEnabled.getOrElse(false) && dbApp.autodeleteThreshold.exists(_ <= 0) - _ <- F.raiseWhen(wantEnableButDbInvalid)( + // 2. The request includes enabled=true but no threshold is provided, and the existing DB threshold is invalid + wantEnableWithoutThresholdButDbInvalid = req.autodeleteEnabled.getOrElse(false) && + req.autodeleteThreshold.isEmpty && + dbApp.autodeleteThreshold.getOrElse(0) <= 0 + + _ <- F.raiseWhen(wantEnableWithoutThresholdButDbInvalid)( BadRequestException( "when enabling autodelete without passing an explicit autodeleteThreshold, the existing config must be valid", Some(ctx.traceId) diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala index 72acbe4f7ba..127fe7957b9 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala @@ -3239,6 +3239,18 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le } } + it should "reject enabling autodeletion by setting autodeleteEnabled=true if the existing config has a missing autodeleteThreshold" in isolatedDbTest { + val initialAutodeleteEnabled = false + val appName = setupAppWithConfig(initialAutodeleteEnabled, None) + + val updateReq = UpdateAppConfigRequest(Some(true), None) + a[BadRequestException] should be thrownBy { + appServiceInterp + .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) + } + } + it should "reject enabling autodeletion if the threshold in the request is invalid" in isolatedDbTest { val initialAutodeleteEnabled = false val appName = setupAppWithConfig(initialAutodeleteEnabled, None) From 117fff42a6f6471efb5741b4259f5c7cc843c971 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Thu, 23 May 2024 17:46:26 -0400 Subject: [PATCH 06/21] formatting --- .../http/service/LeoAppServiceInterp.scala | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index 934e1c174a6..1e4a30613da 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -723,25 +723,18 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, ctx <- as.ask // throw 403 if no project-level permission - hasProjectPermission <- authProvider.isUserProjectReader( - cloudContext, - userInfo - ) + hasProjectPermission <- authProvider.isUserProjectReader(cloudContext, userInfo) _ <- F.raiseWhen(!hasProjectPermission)(ForbiddenError(userInfo.userEmail, Some(ctx.traceId))) - appOpt <- KubernetesServiceDbQueries - .getActiveFullAppByName(cloudContext, appName) - .transaction - appResult <- F.fromOption( - appOpt, - AppNotFoundException(cloudContext, appName, ctx.traceId, "No active app found in DB") + appOpt <- KubernetesServiceDbQueries.getActiveFullAppByName(cloudContext, appName).transaction + appResult <- F.fromOption(appOpt, + AppNotFoundException(cloudContext, appName, ctx.traceId, "No active app found in DB") ) - tags = Map("appType" -> appResult.app.appType.toString) - _ <- metrics.incrementCounter("updateAppConfig", 1, tags) - listOfPermissions <- authProvider.getActions(appResult.app.samResourceId, userInfo) + _ <- metrics.incrementCounter("updateAppConfig", 1, Map("appType" -> appResult.app.appType.toString)) // throw 404 if no UpdateApp permission + listOfPermissions <- authProvider.getActions(appResult.app.samResourceId, userInfo) hasPermission = listOfPermissions.toSet.contains(AppAction.UpdateApp) _ <- F.raiseWhen(!hasPermission)(AppNotFoundException(cloudContext, appName, ctx.traceId, "Permission Denied")) From 3ef3bac0507003fa85ea8bb42cc4b2d919b250ef Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Fri, 24 May 2024 10:46:35 -0400 Subject: [PATCH 07/21] UpdateAppConfigRequest description --- http/src/main/resources/swagger/api-docs.yaml | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/http/src/main/resources/swagger/api-docs.yaml b/http/src/main/resources/swagger/api-docs.yaml index 33994be9574..628101e6308 100644 --- a/http/src/main/resources/swagger/api-docs.yaml +++ b/http/src/main/resources/swagger/api-docs.yaml @@ -4155,16 +4155,22 @@ components: description: Whether to turn on autodelete UpdateAppConfigRequest: - description: a request to update the configuration of an app + description: A request to update the configuration of an app type: object properties: autodeleteEnabled: type: boolean - description: Whether to turn on autodelete + description: > + Optional: Whether to turn on autodelete. When autodeleteEnabled is set to true but there is no + autodeleteThreshold supplied in this request, the existing app's configuration will be used. If the + existing configuration does not have an autodeleteThreshold with a positive integer, this would result in + an invalid autodelete configuration, and therefore this update request would be rejected. autodeleteThreshold: type: integer - description: The number of minutes of idle time to elapse before the app is - deleted in minute. When autodeleteEnabled is true, a positive integer is required + description: > + Optional: The number of minutes of idle time to elapse before the app is deleted. This must be a positive + integer. Note: deletion timing is not exact. The app will be deleted a short period of time after this + threshold time has elapsed. UpdateAppsRequest: description: a request to update a specific set of apps (v1 or v2) From 56d8aa24f8c53616398314626acdeb0c3dd7550f Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Fri, 24 May 2024 10:48:05 -0400 Subject: [PATCH 08/21] update validateUpdateAppConfigRequest comment --- .../workbench/leonardo/http/service/LeoAppServiceInterp.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index 1e4a30613da..ac29401a09d 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -671,14 +671,14 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, // Autodeletion config logic requires a positive integer for threshold if enabled is true. // For simplicity we can enforce this by rejecting the following scenarios: - // 1. The request includes a threshold which is invalid + // 1. The request includes a threshold which is invalid (0 or negative) // 2. The request includes enabled=true but no threshold is provided, and the existing DB threshold is invalid private def validateUpdateAppConfigRequest(req: UpdateAppConfigRequest, dbApp: App)(implicit as: Ask[F, AppContext] ): F[Unit] = for { ctx <- as.ask - // 1. The request includes a threshold which is invalid + // 1. The request includes a threshold which is invalid (0 or negative) invalidThresholdRequest = req.autodeleteThreshold.exists(_ <= 0) _ <- F.raiseWhen(invalidThresholdRequest)( BadRequestException("invalid value for autodeleteThreshold", Some(ctx.traceId)) From 2476ced017ca09b14d97e536bd6288a03737b444 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Fri, 24 May 2024 11:45:56 -0400 Subject: [PATCH 09/21] traverse --- .../workbench/leonardo/http/service/LeoAppServiceInterp.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index ac29401a09d..fdf2b48b5fc 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -698,7 +698,7 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, } yield () private def updateAppConfigInternal(appId: AppId, validatedChanges: UpdateAppConfigRequest): F[Unit] = for { - _ <- validatedChanges.autodeleteEnabled.fold(F.unit)(enabled => + _ <- validatedChanges.autodeleteEnabled.traverse(enabled => appQuery .updateAutodeleteEnabled(appId, enabled) .transaction @@ -706,7 +706,7 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, ) // note: does not clear the threshold if None. This only sets defined thresholds. - _ <- validatedChanges.autodeleteThreshold.fold(F.unit)(threshold => + _ <- validatedChanges.autodeleteThreshold.traverse(threshold => appQuery .updateAutodeleteThreshold(appId, Some(threshold)) .transaction From cbcb60c143b203510a535e9fa371a068091f8ae2 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Fri, 24 May 2024 16:37:21 -0400 Subject: [PATCH 10/21] rename UpdateAppConfig to UpdateApp --- .../leonardo/http/appRoutesModels.scala | 2 +- http/src/main/resources/swagger/api-docs.yaml | 10 ++-- .../leonardo/http/api/AppRoutes.scala | 14 +++--- .../leonardo/http/service/AppService.scala | 4 +- .../http/service/LeoAppServiceInterp.scala | 10 ++-- .../http/service/AppServiceInterpSpec.scala | 48 +++++++++---------- .../http/service/MockAppService.scala | 4 +- 7 files changed, 45 insertions(+), 47 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala b/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala index 10ccf0ca43e..095f222fbf7 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala @@ -39,7 +39,7 @@ final case class CreateAppRequest(kubernetesRuntimeConfig: Option[KubernetesRunt autodeleteEnabled: Option[Boolean] ) -final case class UpdateAppConfigRequest(autodeleteEnabled: Option[Boolean], autodeleteThreshold: Option[Int]) +final case class UpdateAppRequest(autodeleteEnabled: Option[Boolean], autodeleteThreshold: Option[Int]) final case class GetAppResponse( workspaceId: Option[WorkspaceId], diff --git a/http/src/main/resources/swagger/api-docs.yaml b/http/src/main/resources/swagger/api-docs.yaml index 628101e6308..a3936295020 100644 --- a/http/src/main/resources/swagger/api-docs.yaml +++ b/http/src/main/resources/swagger/api-docs.yaml @@ -1597,7 +1597,7 @@ paths: patch: summary: Updates the configuration of an app description: Updates the configuration of an app. Currently limited to autodeletion config. - operationId: updateAppConfig + operationId: updateApp tags: - apps parameters: @@ -1614,7 +1614,7 @@ paths: schema: type: string requestBody: - $ref: "#/components/requestBodies/UpdateAppConfigRequest" + $ref: "#/components/requestBodies/UpdateAppRequest" responses: "202": description: App configuration update request accepted @@ -2950,11 +2950,11 @@ components: machineSize: "Standard_DS1_v2" disk: { labels: {}, name: "disk1", size: 50 } autopauseThreshold: 15 - UpdateAppConfigRequest: + UpdateAppRequest: content: application/json: schema: - $ref: "#/components/schemas/UpdateAppConfigRequest" + $ref: "#/components/schemas/UpdateAppRequest" example: autodeleteEnabled: true autodeleteThreshold: 60 @@ -4154,7 +4154,7 @@ components: type: boolean description: Whether to turn on autodelete - UpdateAppConfigRequest: + UpdateAppRequest: description: A request to update the configuration of an app type: object properties: diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala index fc67edea6d7..361adcba252 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala @@ -11,7 +11,6 @@ import cats.mtl.Ask import de.heikoseeberger.akkahttpcirce.ErrorAccumulatingCirceSupport._ import io.circe.Decoder import io.opencensus.scala.akka.http.TracingDirective.traceRequestForService -import org.broadinstitute.dsde.workbench.leonardo.http.api.AppRoutes.updateAppConfigRequestDecoder import org.broadinstitute.dsde.workbench.leonardo.http.api.AppV2Routes.{ createAppDecoder, getAppResponseEncoder, @@ -22,6 +21,7 @@ import org.broadinstitute.dsde.workbench.leonardo.http.spanResource import org.broadinstitute.dsde.workbench.model.UserInfo import org.broadinstitute.dsde.workbench.model.google.GoogleProject import org.broadinstitute.dsde.workbench.openTelemetry.OpenTelemetryMetrics +import org.broadinstitute.dsde.workbench.leonardo.http.api.AppRoutes.updateAppConfigDecoder class AppRoutes(kubernetesService: AppService[IO], userInfoDirectives: UserInfoDirectives)(implicit metrics: OpenTelemetryMetrics[IO] @@ -74,7 +74,7 @@ class AppRoutes(kubernetesService: AppService[IO], userInfoDirectives: UserInfoD ) } ~ patch { - entity(as[UpdateAppConfigRequest]) { req => + entity(as[UpdateAppRequest]) { req => complete( updateAppConfigHandler(userInfo, googleProject, appName, req) ) @@ -179,12 +179,10 @@ class AppRoutes(kubernetesService: AppService[IO], userInfoDirectives: UserInfoD private[api] def updateAppConfigHandler(userInfo: UserInfo, googleProject: GoogleProject, appName: AppName, - req: UpdateAppConfigRequest + req: UpdateAppRequest )(implicit ev: Ask[IO, AppContext]): IO[ToResponseMarshallable] = for { - _ <- foldSpan(kubernetesService.updateAppConfig(userInfo, CloudContext.Gcp(googleProject), appName, req), - "updateAppConfig" - ) + _ <- foldSpan(kubernetesService.updateApp(userInfo, CloudContext.Gcp(googleProject), appName, req), "updateApp") } yield StatusCodes.Accepted // TODO: this pattern is repeated over 20x @@ -248,11 +246,11 @@ object AppRoutes { } ) - implicit val updateAppConfigRequestDecoder: Decoder[UpdateAppConfigRequest] = + implicit val updateAppConfigDecoder: Decoder[UpdateAppRequest] = Decoder.instance { x => for { enabled <- x.downField("autodeleteEnabled").as[Option[Boolean]] threshold <- x.downField("autodeleteThreshold").as[Option[Int]] - } yield UpdateAppConfigRequest(enabled, threshold) + } yield UpdateAppRequest(enabled, threshold) } } diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppService.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppService.scala index 88553d9788f..42ec6f4468a 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppService.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppService.scala @@ -29,11 +29,11 @@ trait AppService[F[_]] { params: Map[String, String] )(implicit as: Ask[F, AppContext]): F[Vector[ListAppResponse]] - def updateAppConfig( + def updateApp( userInfo: UserInfo, cloudContext: CloudContext.Gcp, appName: AppName, - req: UpdateAppConfigRequest + req: UpdateAppRequest )(implicit as: Ask[F, AppContext]): F[Unit] def deleteApp(userInfo: UserInfo, cloudContext: CloudContext.Gcp, appName: AppName, deleteDisk: Boolean)(implicit diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index fdf2b48b5fc..01649a7c002 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -673,7 +673,7 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, // For simplicity we can enforce this by rejecting the following scenarios: // 1. The request includes a threshold which is invalid (0 or negative) // 2. The request includes enabled=true but no threshold is provided, and the existing DB threshold is invalid - private def validateUpdateAppConfigRequest(req: UpdateAppConfigRequest, dbApp: App)(implicit + private def validateUpdateAppConfigRequest(req: UpdateAppRequest, dbApp: App)(implicit as: Ask[F, AppContext] ): F[Unit] = for { ctx <- as.ask @@ -697,7 +697,7 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, ) } yield () - private def updateAppConfigInternal(appId: AppId, validatedChanges: UpdateAppConfigRequest): F[Unit] = for { + private def updateAppConfigInternal(appId: AppId, validatedChanges: UpdateAppRequest): F[Unit] = for { _ <- validatedChanges.autodeleteEnabled.traverse(enabled => appQuery .updateAutodeleteEnabled(appId, enabled) @@ -714,10 +714,10 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, ) } yield () - override def updateAppConfig(userInfo: UserInfo, + override def updateApp(userInfo: UserInfo, cloudContext: CloudContext.Gcp, appName: AppName, - req: UpdateAppConfigRequest + req: UpdateAppRequest )(implicit as: Ask[F, AppContext]): F[Unit] = for { ctx <- as.ask @@ -731,7 +731,7 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, AppNotFoundException(cloudContext, appName, ctx.traceId, "No active app found in DB") ) - _ <- metrics.incrementCounter("updateAppConfig", 1, Map("appType" -> appResult.app.appType.toString)) + _ <- metrics.incrementCounter("updateApp", 1, Map("appType" -> appResult.app.appType.toString)) // throw 404 if no UpdateApp permission listOfPermissions <- authProvider.getActions(appResult.app.samResourceId, userInfo) diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala index 127fe7957b9..0015d3d0721 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala @@ -3186,14 +3186,14 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le appName } - "updateAppConfig" should "allow enabling autodeletion by specifying both fields" in isolatedDbTest { + "updateApp" should "allow enabling autodeletion by specifying both fields" in isolatedDbTest { val initialAutodeleteEnabled = false val appName = setupAppWithConfig(initialAutodeleteEnabled, None) val autodeleteThreshold = 1000 - val updateReq = UpdateAppConfigRequest(Some(true), Some(autodeleteThreshold)) + val updateReq = UpdateAppRequest(Some(true), Some(autodeleteThreshold)) appServiceInterp - .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .updateApp(userInfo, cloudContextGcp, appName, updateReq) .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) // confirm the update @@ -3211,9 +3211,9 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le val autodeleteThreshold = 1000 val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) - val updateReq = UpdateAppConfigRequest(Some(true), None) + val updateReq = UpdateAppRequest(Some(true), None) appServiceInterp - .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .updateApp(userInfo, cloudContextGcp, appName, updateReq) .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) // confirm the update @@ -3231,10 +3231,10 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le val badAutodeleteThreshold = 0 val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(badAutodeleteThreshold)) - val updateReq = UpdateAppConfigRequest(Some(true), None) + val updateReq = UpdateAppRequest(Some(true), None) a[BadRequestException] should be thrownBy { appServiceInterp - .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .updateApp(userInfo, cloudContextGcp, appName, updateReq) .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } } @@ -3243,10 +3243,10 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le val initialAutodeleteEnabled = false val appName = setupAppWithConfig(initialAutodeleteEnabled, None) - val updateReq = UpdateAppConfigRequest(Some(true), None) + val updateReq = UpdateAppRequest(Some(true), None) a[BadRequestException] should be thrownBy { appServiceInterp - .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .updateApp(userInfo, cloudContextGcp, appName, updateReq) .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } } @@ -3256,10 +3256,10 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le val appName = setupAppWithConfig(initialAutodeleteEnabled, None) val badAutodeleteThreshold = 0 - val updateReq = UpdateAppConfigRequest(Some(true), Some(badAutodeleteThreshold)) + val updateReq = UpdateAppRequest(Some(true), Some(badAutodeleteThreshold)) a[BadRequestException] should be thrownBy { appServiceInterp - .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .updateApp(userInfo, cloudContextGcp, appName, updateReq) .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } } @@ -3270,10 +3270,10 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) val badAutodeleteThreshold = 0 - val updateReq = UpdateAppConfigRequest(Some(false), Some(badAutodeleteThreshold)) + val updateReq = UpdateAppRequest(Some(false), Some(badAutodeleteThreshold)) a[BadRequestException] should be thrownBy { appServiceInterp - .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .updateApp(userInfo, cloudContextGcp, appName, updateReq) .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } } @@ -3284,10 +3284,10 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) val badAutodeleteThreshold = 0 - val updateReq = UpdateAppConfigRequest(None, Some(badAutodeleteThreshold)) + val updateReq = UpdateAppRequest(None, Some(badAutodeleteThreshold)) a[BadRequestException] should be thrownBy { appServiceInterp - .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .updateApp(userInfo, cloudContextGcp, appName, updateReq) .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } } @@ -3297,9 +3297,9 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le val autodeleteThreshold = 1000 val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) - val updateReq = UpdateAppConfigRequest(Some(false), None) + val updateReq = UpdateAppRequest(Some(false), None) appServiceInterp - .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .updateApp(userInfo, cloudContextGcp, appName, updateReq) .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) // confirm the update @@ -3318,9 +3318,9 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) val newThreshold = 2000 - val updateReq = UpdateAppConfigRequest(Some(false), Some(newThreshold)) + val updateReq = UpdateAppRequest(Some(false), Some(newThreshold)) appServiceInterp - .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .updateApp(userInfo, cloudContextGcp, appName, updateReq) .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) // confirm the update @@ -3339,9 +3339,9 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) val newAutodeleteThreshold = 2000 - val updateReq = UpdateAppConfigRequest(Some(true), Some(newAutodeleteThreshold)) + val updateReq = UpdateAppRequest(Some(true), Some(newAutodeleteThreshold)) appServiceInterp - .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .updateApp(userInfo, cloudContextGcp, appName, updateReq) .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) // confirm the update @@ -3360,9 +3360,9 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) val newAutodeleteThreshold = 2000 - val updateReq = UpdateAppConfigRequest(None, Some(newAutodeleteThreshold)) + val updateReq = UpdateAppRequest(None, Some(newAutodeleteThreshold)) appServiceInterp - .updateAppConfig(userInfo, cloudContextGcp, appName, updateReq) + .updateApp(userInfo, cloudContextGcp, appName, updateReq) .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) // confirm the update @@ -3387,7 +3387,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le val wrongApp = AppName("wrongApp") an[AppNotFoundException] should be thrownBy { appServiceInterp - .updateAppConfig(userInfo, cloudContextGcp, wrongApp, UpdateAppConfigRequest(None, None)) + .updateApp(userInfo, cloudContextGcp, wrongApp, UpdateAppRequest(None, None)) .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) } } diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/MockAppService.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/MockAppService.scala index b8a58eff5d4..ff6b128cfe5 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/MockAppService.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/MockAppService.scala @@ -80,10 +80,10 @@ class MockAppService extends AppService[IO] { ): IO[Unit] = IO.unit - override def updateAppConfig(userInfo: UserInfo, + override def updateApp(userInfo: UserInfo, cloudContext: CloudContext.Gcp, appName: AppName, - req: UpdateAppConfigRequest + req: UpdateAppRequest )(implicit as: Ask[IO, AppContext]): IO[Unit] = IO.unit } From 88f55ef7a49353036a5ec16f8462b94517133736 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Fri, 24 May 2024 16:44:54 -0400 Subject: [PATCH 11/21] lint --- .../leonardo/http/service/LeoAppServiceInterp.scala | 8 +++----- .../workbench/leonardo/http/service/MockAppService.scala | 8 +++----- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index 01649a7c002..60df8c04465 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -714,11 +714,9 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, ) } yield () - override def updateApp(userInfo: UserInfo, - cloudContext: CloudContext.Gcp, - appName: AppName, - req: UpdateAppRequest - )(implicit as: Ask[F, AppContext]): F[Unit] = + override def updateApp(userInfo: UserInfo, cloudContext: CloudContext.Gcp, appName: AppName, req: UpdateAppRequest)( + implicit as: Ask[F, AppContext] + ): F[Unit] = for { ctx <- as.ask diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/MockAppService.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/MockAppService.scala index ff6b128cfe5..d027799ea79 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/MockAppService.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/MockAppService.scala @@ -80,11 +80,9 @@ class MockAppService extends AppService[IO] { ): IO[Unit] = IO.unit - override def updateApp(userInfo: UserInfo, - cloudContext: CloudContext.Gcp, - appName: AppName, - req: UpdateAppRequest - )(implicit as: Ask[IO, AppContext]): IO[Unit] = IO.unit + override def updateApp(userInfo: UserInfo, cloudContext: CloudContext.Gcp, appName: AppName, req: UpdateAppRequest)( + implicit as: Ask[IO, AppContext] + ): IO[Unit] = IO.unit } object MockAppService extends MockAppService From 1be057ce842d0b0e09695abc5f707559fe04366a Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Tue, 28 May 2024 10:22:08 -0400 Subject: [PATCH 12/21] comment --- http/src/main/resources/swagger/api-docs.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http/src/main/resources/swagger/api-docs.yaml b/http/src/main/resources/swagger/api-docs.yaml index a3936295020..b09004b5ebb 100644 --- a/http/src/main/resources/swagger/api-docs.yaml +++ b/http/src/main/resources/swagger/api-docs.yaml @@ -1617,7 +1617,7 @@ paths: $ref: "#/components/requestBodies/UpdateAppRequest" responses: "202": - description: App configuration update request accepted + description: App update request accepted "400": description: Bad Request content: From 26aba7d72c68039729c10ebd8299ee5838b8d719 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Tue, 28 May 2024 11:10:04 -0400 Subject: [PATCH 13/21] rv foldspan --- .../leonardo/http/api/AppRoutes.scala | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala index 361adcba252..44b2ca844be 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala @@ -181,22 +181,16 @@ class AppRoutes(kubernetesService: AppService[IO], userInfoDirectives: UserInfoD appName: AppName, req: UpdateAppRequest )(implicit ev: Ask[IO, AppContext]): IO[ToResponseMarshallable] = - for { - _ <- foldSpan(kubernetesService.updateApp(userInfo, CloudContext.Gcp(googleProject), appName, req), "updateApp") - } yield StatusCodes.Accepted - - // TODO: this pattern is repeated over 20x - // ctx <- ev.ask[AppContext] - // apiCall = whatever - // _ <- ctx.span.fold(apiCall)(span => spanResource[IO](span, "methodName").use(_ => apiCall)) - // - // My intuition is that it's not necessary for all developers to have a deep understanding of what this is doing, - // so I want to abstract this implementation detail away for better ergonomics/approachability. Thoughts? - private def foldSpan(apiCall: IO[Unit], apiName: String)(implicit ev: Ask[IO, AppContext]): IO[Unit] = for { ctx <- ev.ask[AppContext] - _ <- ctx.span.fold(apiCall)(span => spanResource[IO](span, apiName).use(_ => apiCall)) - } yield () + apiCall = kubernetesService.updateApp( + userInfo, + CloudContext.Gcp(googleProject), + appName, + req + ) + _ <- ctx.span.fold(apiCall)(span => spanResource[IO](span, "updateApp").use(_ => apiCall)) + } yield StatusCodes.Accepted private[api] def deleteAppHandler(userInfo: UserInfo, googleProject: GoogleProject, From fb796b66064b5805c29d77117c87a499ad594dbd Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Tue, 28 May 2024 12:04:40 -0400 Subject: [PATCH 14/21] WIP tests run --- .../leonardo/http/api/AppRoutes.scala | 14 ++++---- .../http/service/LeoAppServiceInterp.scala | 34 ++++++++++++++----- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala index 44b2ca844be..4e1933a51e3 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala @@ -21,7 +21,7 @@ import org.broadinstitute.dsde.workbench.leonardo.http.spanResource import org.broadinstitute.dsde.workbench.model.UserInfo import org.broadinstitute.dsde.workbench.model.google.GoogleProject import org.broadinstitute.dsde.workbench.openTelemetry.OpenTelemetryMetrics -import org.broadinstitute.dsde.workbench.leonardo.http.api.AppRoutes.updateAppConfigDecoder +import org.broadinstitute.dsde.workbench.leonardo.http.api.AppRoutes.updateAppDecoder class AppRoutes(kubernetesService: AppService[IO], userInfoDirectives: UserInfoDirectives)(implicit metrics: OpenTelemetryMetrics[IO] @@ -76,7 +76,7 @@ class AppRoutes(kubernetesService: AppService[IO], userInfoDirectives: UserInfoD patch { entity(as[UpdateAppRequest]) { req => complete( - updateAppConfigHandler(userInfo, googleProject, appName, req) + updateAppHandler(userInfo, googleProject, appName, req) ) } } ~ @@ -176,10 +176,10 @@ class AppRoutes(kubernetesService: AppService[IO], userInfoDirectives: UserInfoD resp <- ctx.span.fold(apiCall)(span => spanResource[IO](span, "listApp").use(_ => apiCall)) } yield StatusCodes.OK -> resp - private[api] def updateAppConfigHandler(userInfo: UserInfo, - googleProject: GoogleProject, - appName: AppName, - req: UpdateAppRequest + private[api] def updateAppHandler(userInfo: UserInfo, + googleProject: GoogleProject, + appName: AppName, + req: UpdateAppRequest )(implicit ev: Ask[IO, AppContext]): IO[ToResponseMarshallable] = for { ctx <- ev.ask[AppContext] @@ -240,7 +240,7 @@ object AppRoutes { } ) - implicit val updateAppConfigDecoder: Decoder[UpdateAppRequest] = + implicit val updateAppDecoder: Decoder[UpdateAppRequest] = Decoder.instance { x => for { enabled <- x.downField("autodeleteEnabled").as[Option[Boolean]] diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index 60df8c04465..2892072f35b 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -673,7 +673,7 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, // For simplicity we can enforce this by rejecting the following scenarios: // 1. The request includes a threshold which is invalid (0 or negative) // 2. The request includes enabled=true but no threshold is provided, and the existing DB threshold is invalid - private def validateUpdateAppConfigRequest(req: UpdateAppRequest, dbApp: App)(implicit + private def validateUpdateAppRequest(req: UpdateAppRequest, dbApp: App)(implicit as: Ask[F, AppContext] ): F[Unit] = for { ctx <- as.ask @@ -697,7 +697,7 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, ) } yield () - private def updateAppConfigInternal(appId: AppId, validatedChanges: UpdateAppRequest): F[Unit] = for { + private def updateAppInternal(appId: AppId, validatedChanges: UpdateAppRequest): F[Unit] = for { _ <- validatedChanges.autodeleteEnabled.traverse(enabled => appQuery .updateAutodeleteEnabled(appId, enabled) @@ -737,8 +737,11 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, _ <- F.raiseWhen(!hasPermission)(AppNotFoundException(cloudContext, appName, ctx.traceId, "Permission Denied")) // confirm that the combination of the request and the existing DB values result in a valid configuration - _ <- validateUpdateAppConfigRequest(req, appResult.app) - _ <- updateAppConfigInternal(appResult.app.id, req) +// resolvedAutodeleteEnabled = req.autodeleteEnabled.getOrElse(appResult.app.autodeleteEnabled) + resolvedAutodeleteThreshold = req.autodeleteThreshold.orElse(appResult.app.autodeleteThreshold) + _ <- F.fromEither(validateAutodelete(resolvedAutodeleteThreshold, ctx.traceId)) +// _ <- validateUpdateAppRequest(req, appResult.app) + _ <- updateAppInternal(appResult.app.id, req) } yield () override def createAppV2(userInfo: UserInfo, workspaceId: WorkspaceId, appName: AppName, req: CreateAppRequest)( @@ -1501,13 +1504,15 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, if (req.appType == AppType.Allowed) Some(config.leoKubernetesConfig.allowedAppConfig.numOfReplicas) else None + autodeleteEnabled = req.autodeleteEnabled.getOrElse(false) autodeleteThreshold = req.autodeleteThreshold.getOrElse(0) - _ <- Either.cond(!(autodeleteEnabled && autodeleteThreshold <= 0), - (), - BadRequestException("autodeleteThreshold should be a positive value", Some(ctx.traceId)) - ) +// _ <- Either.cond(!(autodeleteEnabled && autodeleteThreshold <= 0), +// (), +// BadRequestException("autodeleteThreshold should be a positive value", Some(ctx.traceId)) +// ) + _ <- validateAutodelete(req.autodeleteThreshold, ctx.traceId) } yield SaveApp( App( AppId(-1), @@ -1536,11 +1541,22 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, req.sourceWorkspaceId, numOfReplicas, req.autodeleteThreshold, - autodeleteEnabled + req.autodeleteEnabled.getOrElse(false) ) ) } + private[service] def validateAutodelete( + autodeleteThreshold: Option[Int], + traceId: TraceId + ): Either[LeoException, Unit] = + for { + _ <- Either.cond(!autodeleteThreshold.exists(_ <= 0), + (), + BadRequestException("autodeleteThreshold should be a positive value", Some(traceId)) + ) + } yield () + private def getVisibleApps(allClusters: List[KubernetesCluster], userInfo: UserInfo)(implicit as: Ask[F, AppContext] ): F[Option[List[AppSamResourceId]]] = for { From 09220982f6e7a1662590efec1687c623630c3ba3 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Tue, 28 May 2024 13:17:53 -0400 Subject: [PATCH 15/21] common logic --- .../http/service/LeoAppServiceInterp.scala | 62 +++++-------------- .../http/service/AppServiceInterpSpec.scala | 13 ---- 2 files changed, 17 insertions(+), 58 deletions(-) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index 2892072f35b..0c905e48289 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -5,7 +5,6 @@ package service import akka.http.scaladsl.model.StatusCodes import bio.terra.workspace.model.{IamRole, WorkspaceDescription} import cats.Parallel -import cats.data.NonEmptyList import cats.effect.Async import cats.effect.std.Queue import cats.mtl.Ask @@ -669,34 +668,6 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, .raiseError[Unit](AppNotFoundByWorkspaceIdException(workspaceId, appName, ctx.traceId, "permission denied")) } yield GetAppResponse.fromDbResult(app, Config.proxyConfig.proxyUrlBase) - // Autodeletion config logic requires a positive integer for threshold if enabled is true. - // For simplicity we can enforce this by rejecting the following scenarios: - // 1. The request includes a threshold which is invalid (0 or negative) - // 2. The request includes enabled=true but no threshold is provided, and the existing DB threshold is invalid - private def validateUpdateAppRequest(req: UpdateAppRequest, dbApp: App)(implicit - as: Ask[F, AppContext] - ): F[Unit] = for { - ctx <- as.ask - - // 1. The request includes a threshold which is invalid (0 or negative) - invalidThresholdRequest = req.autodeleteThreshold.exists(_ <= 0) - _ <- F.raiseWhen(invalidThresholdRequest)( - BadRequestException("invalid value for autodeleteThreshold", Some(ctx.traceId)) - ) - - // 2. The request includes enabled=true but no threshold is provided, and the existing DB threshold is invalid - wantEnableWithoutThresholdButDbInvalid = req.autodeleteEnabled.getOrElse(false) && - req.autodeleteThreshold.isEmpty && - dbApp.autodeleteThreshold.getOrElse(0) <= 0 - - _ <- F.raiseWhen(wantEnableWithoutThresholdButDbInvalid)( - BadRequestException( - "when enabling autodelete without passing an explicit autodeleteThreshold, the existing config must be valid", - Some(ctx.traceId) - ) - ) - } yield () - private def updateAppInternal(appId: AppId, validatedChanges: UpdateAppRequest): F[Unit] = for { _ <- validatedChanges.autodeleteEnabled.traverse(enabled => appQuery @@ -737,10 +708,9 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, _ <- F.raiseWhen(!hasPermission)(AppNotFoundException(cloudContext, appName, ctx.traceId, "Permission Denied")) // confirm that the combination of the request and the existing DB values result in a valid configuration -// resolvedAutodeleteEnabled = req.autodeleteEnabled.getOrElse(appResult.app.autodeleteEnabled) + resolvedAutodeleteEnabled = req.autodeleteEnabled.getOrElse(appResult.app.autodeleteEnabled) resolvedAutodeleteThreshold = req.autodeleteThreshold.orElse(appResult.app.autodeleteThreshold) - _ <- F.fromEither(validateAutodelete(resolvedAutodeleteThreshold, ctx.traceId)) -// _ <- validateUpdateAppRequest(req, appResult.app) + _ <- F.fromEither(validateAutodelete(resolvedAutodeleteEnabled, resolvedAutodeleteThreshold, ctx.traceId)) _ <- updateAppInternal(appResult.app.id, req) } yield () @@ -1506,13 +1476,7 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, else None autodeleteEnabled = req.autodeleteEnabled.getOrElse(false) - autodeleteThreshold = req.autodeleteThreshold.getOrElse(0) - -// _ <- Either.cond(!(autodeleteEnabled && autodeleteThreshold <= 0), -// (), -// BadRequestException("autodeleteThreshold should be a positive value", Some(ctx.traceId)) -// ) - _ <- validateAutodelete(req.autodeleteThreshold, ctx.traceId) + _ <- validateAutodelete(autodeleteEnabled, req.autodeleteThreshold, ctx.traceId) } yield SaveApp( App( AppId(-1), @@ -1541,21 +1505,29 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, req.sourceWorkspaceId, numOfReplicas, req.autodeleteThreshold, - req.autodeleteEnabled.getOrElse(false) + autodeleteEnabled ) ) } - private[service] def validateAutodelete( - autodeleteThreshold: Option[Int], - traceId: TraceId - ): Either[LeoException, Unit] = + private[service] def validateAutodelete(autodeleteEnabled: Boolean, + autodeleteThreshold: Option[Int], + traceId: TraceId + ): Either[LeoException, Unit] = { + val invalidThreshold = autodeleteThreshold.exists(_ <= 0) + val wantEnabledButThresholdMissing = autodeleteEnabled && autodeleteThreshold.isEmpty for { - _ <- Either.cond(!autodeleteThreshold.exists(_ <= 0), + _ <- Either.cond(!invalidThreshold, (), BadRequestException("autodeleteThreshold should be a positive value", Some(traceId)) ) + _ <- Either.cond( + !wantEnabledButThresholdMissing, + (), + BadRequestException("when enabling autodelete, an autodeleteThreshold must be present", Some(traceId)) + ) } yield () + } private def getVisibleApps(allClusters: List[KubernetesCluster], userInfo: UserInfo)(implicit as: Ask[F, AppContext] diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala index 0015d3d0721..a6908306525 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala @@ -3226,19 +3226,6 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le updatedApp.autodeleteThreshold shouldBe Some(autodeleteThreshold) } - it should "reject enabling autodeletion by setting autodeleteEnabled=true if the existing config has an invalid autodeleteThreshold" in isolatedDbTest { - val initialAutodeleteEnabled = false - val badAutodeleteThreshold = 0 - val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(badAutodeleteThreshold)) - - val updateReq = UpdateAppRequest(Some(true), None) - a[BadRequestException] should be thrownBy { - appServiceInterp - .updateApp(userInfo, cloudContextGcp, appName, updateReq) - .unsafeRunSync()(cats.effect.unsafe.IORuntime.global) - } - } - it should "reject enabling autodeletion by setting autodeleteEnabled=true if the existing config has a missing autodeleteThreshold" in isolatedDbTest { val initialAutodeleteEnabled = false val appName = setupAppWithConfig(initialAutodeleteEnabled, None) From eebf6476abaf07a9ad10a91399eb957890aea0a9 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Tue, 28 May 2024 13:37:21 -0400 Subject: [PATCH 16/21] oops --- .../workbench/leonardo/http/service/LeoAppServiceInterp.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index 0c905e48289..e53c3b988d8 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -5,6 +5,7 @@ package service import akka.http.scaladsl.model.StatusCodes import bio.terra.workspace.model.{IamRole, WorkspaceDescription} import cats.Parallel +import cats.data.NonEmptyList import cats.effect.Async import cats.effect.std.Queue import cats.mtl.Ask From 5383d0762f465121346d23bb1e5ce2ac64168153 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Tue, 28 May 2024 13:40:46 -0400 Subject: [PATCH 17/21] rm spurious --- .../dsde/workbench/leonardo/http/api/AppRoutes.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala index 4e1933a51e3..ea6b60ac304 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala @@ -17,7 +17,6 @@ import org.broadinstitute.dsde.workbench.leonardo.http.api.AppV2Routes.{ listAppResponseEncoder } import org.broadinstitute.dsde.workbench.leonardo.http.service.AppService -import org.broadinstitute.dsde.workbench.leonardo.http.spanResource import org.broadinstitute.dsde.workbench.model.UserInfo import org.broadinstitute.dsde.workbench.model.google.GoogleProject import org.broadinstitute.dsde.workbench.openTelemetry.OpenTelemetryMetrics From 0ac9141379fddf80201b23ff16ad32a3bd487135 Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Tue, 28 May 2024 13:51:39 -0400 Subject: [PATCH 18/21] spacing --- .../workbench/leonardo/http/service/LeoAppServiceInterp.scala | 2 ++ 1 file changed, 2 insertions(+) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index e53c3b988d8..181eb2b30a2 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -1517,11 +1517,13 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, ): Either[LeoException, Unit] = { val invalidThreshold = autodeleteThreshold.exists(_ <= 0) val wantEnabledButThresholdMissing = autodeleteEnabled && autodeleteThreshold.isEmpty + for { _ <- Either.cond(!invalidThreshold, (), BadRequestException("autodeleteThreshold should be a positive value", Some(traceId)) ) + _ <- Either.cond( !wantEnabledButThresholdMissing, (), From fff4483a4a114cc5a56e0a57760a21401252e9aa Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Tue, 28 May 2024 13:53:29 -0400 Subject: [PATCH 19/21] rename --- .../http/service/AppServiceInterpSpec.scala | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala index a6908306525..fbaa3be3cf4 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala @@ -3162,7 +3162,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le .isInstanceOf[ForbiddenError] shouldBe true } - def setupAppWithConfig(autodeleteEnabled: Boolean, autodeleteThreshold: Option[Int]): AppName = { + def createAppForAutodeleteTests(autodeleteEnabled: Boolean, autodeleteThreshold: Option[Int]): AppName = { val appName = AppName("pong") val createReq = createAppRequest.copy( diskConfig = Some(PersistentDiskRequest(diskName, None, None, Map.empty)), @@ -3188,7 +3188,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le "updateApp" should "allow enabling autodeletion by specifying both fields" in isolatedDbTest { val initialAutodeleteEnabled = false - val appName = setupAppWithConfig(initialAutodeleteEnabled, None) + val appName = createAppForAutodeleteTests(initialAutodeleteEnabled, None) val autodeleteThreshold = 1000 val updateReq = UpdateAppRequest(Some(true), Some(autodeleteThreshold)) @@ -3209,7 +3209,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le it should "allow enabling autodeletion by setting autodeleteEnabled=true if the existing config has a valid autodeleteThreshold" in isolatedDbTest { val initialAutodeleteEnabled = false val autodeleteThreshold = 1000 - val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + val appName = createAppForAutodeleteTests(initialAutodeleteEnabled, Some(autodeleteThreshold)) val updateReq = UpdateAppRequest(Some(true), None) appServiceInterp @@ -3228,7 +3228,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le it should "reject enabling autodeletion by setting autodeleteEnabled=true if the existing config has a missing autodeleteThreshold" in isolatedDbTest { val initialAutodeleteEnabled = false - val appName = setupAppWithConfig(initialAutodeleteEnabled, None) + val appName = createAppForAutodeleteTests(initialAutodeleteEnabled, None) val updateReq = UpdateAppRequest(Some(true), None) a[BadRequestException] should be thrownBy { @@ -3240,7 +3240,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le it should "reject enabling autodeletion if the threshold in the request is invalid" in isolatedDbTest { val initialAutodeleteEnabled = false - val appName = setupAppWithConfig(initialAutodeleteEnabled, None) + val appName = createAppForAutodeleteTests(initialAutodeleteEnabled, None) val badAutodeleteThreshold = 0 val updateReq = UpdateAppRequest(Some(true), Some(badAutodeleteThreshold)) @@ -3254,7 +3254,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le it should "reject disabling autodeletion if the threshold in the request is invalid" in isolatedDbTest { val initialAutodeleteEnabled = true val autodeleteThreshold = 1000 - val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + val appName = createAppForAutodeleteTests(initialAutodeleteEnabled, Some(autodeleteThreshold)) val badAutodeleteThreshold = 0 val updateReq = UpdateAppRequest(Some(false), Some(badAutodeleteThreshold)) @@ -3268,7 +3268,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le it should "reject autodeletionEnabled=None if the threshold in the request is invalid" in isolatedDbTest { val initialAutodeleteEnabled = true val autodeleteThreshold = 1000 - val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + val appName = createAppForAutodeleteTests(initialAutodeleteEnabled, Some(autodeleteThreshold)) val badAutodeleteThreshold = 0 val updateReq = UpdateAppRequest(None, Some(badAutodeleteThreshold)) @@ -3282,7 +3282,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le it should "allow disabling autodeletion by setting enabled = false" in isolatedDbTest { val initialAutodeleteEnabled = true val autodeleteThreshold = 1000 - val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + val appName = createAppForAutodeleteTests(initialAutodeleteEnabled, Some(autodeleteThreshold)) val updateReq = UpdateAppRequest(Some(false), None) appServiceInterp @@ -3302,7 +3302,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le it should "allow disabling autodeletion by setting enabled = false and specifying a new autodeleteThreshold" in isolatedDbTest { val initialAutodeleteEnabled = true val autodeleteThreshold = 1000 - val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + val appName = createAppForAutodeleteTests(initialAutodeleteEnabled, Some(autodeleteThreshold)) val newThreshold = 2000 val updateReq = UpdateAppRequest(Some(false), Some(newThreshold)) @@ -3323,7 +3323,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le it should "allow changing autodeletion threshold when enableAutodelete=true" in isolatedDbTest { val initialAutodeleteEnabled = true val autodeleteThreshold = 1000 - val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + val appName = createAppForAutodeleteTests(initialAutodeleteEnabled, Some(autodeleteThreshold)) val newAutodeleteThreshold = 2000 val updateReq = UpdateAppRequest(Some(true), Some(newAutodeleteThreshold)) @@ -3344,7 +3344,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le it should "allow changing autodeletion threshold when enableAutodelete=None" in isolatedDbTest { val initialAutodeleteEnabled = true val autodeleteThreshold = 1000 - val appName = setupAppWithConfig(initialAutodeleteEnabled, Some(autodeleteThreshold)) + val appName = createAppForAutodeleteTests(initialAutodeleteEnabled, Some(autodeleteThreshold)) val newAutodeleteThreshold = 2000 val updateReq = UpdateAppRequest(None, Some(newAutodeleteThreshold)) From 102e8afa3896f06b3d8d1e15987978fe4405fafd Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Tue, 28 May 2024 14:50:12 -0400 Subject: [PATCH 20/21] getUpdateAppTransaction --- .../leonardo/http/service/LeoAppServiceInterp.scala | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala index 181eb2b30a2..8a582de19eb 100644 --- a/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala +++ b/http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala @@ -29,6 +29,7 @@ import org.broadinstitute.dsde.workbench.leonardo.JsonCodec._ import org.broadinstitute.dsde.workbench.leonardo.SamResourceId._ import org.broadinstitute.dsde.workbench.leonardo.config._ import org.broadinstitute.dsde.workbench.leonardo.dao.{WsmApiClientProvider, WsmDao} +import org.broadinstitute.dsde.workbench.leonardo.db.DBIOInstances.dbioInstance import org.broadinstitute.dsde.workbench.leonardo.db.KubernetesServiceDbQueries.getActiveFullAppByWorkspaceIdAndAppName import org.broadinstitute.dsde.workbench.leonardo.db._ import org.broadinstitute.dsde.workbench.leonardo.http.service.LeoAppServiceInterp.{ @@ -669,22 +670,18 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, .raiseError[Unit](AppNotFoundByWorkspaceIdException(workspaceId, appName, ctx.traceId, "permission denied")) } yield GetAppResponse.fromDbResult(app, Config.proxyConfig.proxyUrlBase) - private def updateAppInternal(appId: AppId, validatedChanges: UpdateAppRequest): F[Unit] = for { + private def getUpdateAppTransaction(appId: AppId, validatedChanges: UpdateAppRequest): F[Unit] = (for { _ <- validatedChanges.autodeleteEnabled.traverse(enabled => appQuery .updateAutodeleteEnabled(appId, enabled) - .transaction - .void ) // note: does not clear the threshold if None. This only sets defined thresholds. _ <- validatedChanges.autodeleteThreshold.traverse(threshold => appQuery .updateAutodeleteThreshold(appId, Some(threshold)) - .transaction - .void ) - } yield () + } yield ()).transaction override def updateApp(userInfo: UserInfo, cloudContext: CloudContext.Gcp, appName: AppName, req: UpdateAppRequest)( implicit as: Ask[F, AppContext] @@ -712,7 +709,7 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, resolvedAutodeleteEnabled = req.autodeleteEnabled.getOrElse(appResult.app.autodeleteEnabled) resolvedAutodeleteThreshold = req.autodeleteThreshold.orElse(appResult.app.autodeleteThreshold) _ <- F.fromEither(validateAutodelete(resolvedAutodeleteEnabled, resolvedAutodeleteThreshold, ctx.traceId)) - _ <- updateAppInternal(appResult.app.id, req) + _ <- getUpdateAppTransaction(appResult.app.id, req) } yield () override def createAppV2(userInfo: UserInfo, workspaceId: WorkspaceId, appName: AppName, req: CreateAppRequest)( From b995b359c20db1dedbb836642efbe193140dd4ff Mon Sep 17 00:00:00 2001 From: Joel Thibault Date: Thu, 30 May 2024 16:16:58 -0400 Subject: [PATCH 21/21] trivial change to re-trigger tests --- .../workbench/leonardo/http/service/AppServiceInterpSpec.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala index fbaa3be3cf4..db7776005dd 100644 --- a/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala +++ b/http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala @@ -3163,7 +3163,7 @@ class AppServiceInterpTest extends AnyFlatSpec with AppServiceInterpSpec with Le } def createAppForAutodeleteTests(autodeleteEnabled: Boolean, autodeleteThreshold: Option[Int]): AppName = { - val appName = AppName("pong") + val appName = AppName("Microsoft Excel") val createReq = createAppRequest.copy( diskConfig = Some(PersistentDiskRequest(diskName, None, None, Map.empty)), autodeleteEnabled = Some(autodeleteEnabled),