From 4ec82f207f882b6fbaa1746707fc79fa2c1585bb Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Mon, 12 Feb 2024 16:14:56 -0500 Subject: [PATCH 01/28] Wiring for github token lookups [WIP] --- .../routes/WomtoolRouteSupport.scala | 35 +++++++----- .../languages/util/ImportResolver.scala | 31 +++++++--- .../services/GithubAuthVendingActor.scala | 56 +++++++++++++++++++ .../cromwell/services/womtool/Describer.scala | 6 +- .../womtool/WomtoolServiceMessages.scala | 3 +- .../impl/WomtoolServiceInCromwellActor.scala | 4 +- 6 files changed, 105 insertions(+), 30 deletions(-) create mode 100644 services/src/main/scala/cromwell/services/GithubAuthVendingActor.scala diff --git a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala index dec26a63dbf..9e58f8c32c5 100644 --- a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala +++ b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala @@ -1,26 +1,25 @@ package cromwell.webservice.routes -import akka.actor.{ActorRef, ActorRefFactory} +import akka.actor.{Actor, ActorRef, ActorRefFactory} +import akka.http.javadsl.model.headers.HttpCredentials import akka.http.scaladsl.model._ +import akka.http.scaladsl.model.headers.OAuth2BearerToken import akka.http.scaladsl.server.Directives._ import akka.http.scaladsl.server.Route import akka.pattern.ask import akka.stream.ActorMaterializer import akka.util.Timeout import cromwell.core.{WorkflowOptions, WorkflowSourceFilesCollection} -import cromwell.services.womtool.WomtoolServiceMessages.{ - DescribeFailure, - DescribeRequest, - DescribeResult, - DescribeSuccess -} +import cromwell.languages.util.ImportResolver.ImportAuthProvider +import cromwell.services.GithubAuthVendingActor.GithubAuthVendingSupport +import cromwell.services.womtool.WomtoolServiceMessages.{DescribeFailure, DescribeRequest, DescribeResult, DescribeSuccess} import cromwell.webservice.WebServiceUtils import cromwell.webservice.WebServiceUtils.EnhancedThrowable import scala.concurrent.ExecutionContext import scala.util.{Failure, Success} -trait WomtoolRouteSupport extends WebServiceUtils { +trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport { this: Actor => implicit def actorRefFactory: ActorRefFactory implicit val ec: ExecutionContext @@ -33,17 +32,23 @@ trait WomtoolRouteSupport extends WebServiceUtils { path("womtool" / Segment / "describe") { _ => post { entity(as[Multipart.FormData]) { formData: Multipart.FormData => - onComplete(materializeFormData(formData)) { - case Success(data) => - validateAndSubmitRequest(data) - case Failure(e) => - e.failRequest(StatusCodes.InternalServerError) + extractCredentials { creds => + val authProviders: List[ImportAuthProvider] = creds match { + case Some(OAuth2BearerToken(token)) => List(importAuthProvider(token)) + case _ => List.empty + } + onComplete(materializeFormData(formData)) { + case Success(data) => + validateAndSubmitRequest(data, authProviders) + case Failure(e) => + e.failRequest(StatusCodes.InternalServerError) + } } } } } - private def validateAndSubmitRequest(data: MaterializedFormData): Route = { + private def validateAndSubmitRequest(data: MaterializedFormData, importAuthProviders: List[ImportAuthProvider]): Route = { // TODO: move constants to WebServiceUtils, adopt in PartialWorkflowSources val workflowSource = data.get("workflowSource").map(_.utf8String) val workflowUrl = data.get("workflowUrl").map(_.utf8String) @@ -66,7 +71,7 @@ trait WomtoolRouteSupport extends WebServiceUtils { requestedWorkflowId = None ) - onComplete(serviceRegistryActor.ask(DescribeRequest(wsfc)).mapTo[DescribeResult]) { + onComplete(serviceRegistryActor.ask(DescribeRequest(wsfc, importAuthProviders)).mapTo[DescribeResult]) { case Success(response: DescribeSuccess) => import cromwell.services.womtool.models.WorkflowDescription.workflowDescriptionEncoder import de.heikoseeberger.akkahttpcirce.ErrorAccumulatingCirceSupport._ diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index e3690248b09..be8ce72a0fd 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -22,11 +22,11 @@ import java.nio.file.{Path => NioPath} import java.security.MessageDigest import cromwell.core.WorkflowId import wom.ResolvedImportRecord -import wom.core.WorkflowSource +import wom.core.{WorkflowSource, WorkflowUrl} import wom.values._ import scala.concurrent.duration._ -import scala.concurrent.{Await, ExecutionContext} +import scala.concurrent.{Await, ExecutionContext, Future} import scala.util.{Failure, Success, Try} object ImportResolver { @@ -37,6 +37,11 @@ object ImportResolver { resolvedImportRecord: ResolvedImportRecord ) + trait ImportAuthProvider { + def validHosts: List[String] + def authHeader(): Future[Map[String, String]] + } + trait ImportResolver { def name: String protected def innerResolver(path: String, currentResolvers: List[ImportResolver]): Checked[ResolvedImportBundle] @@ -179,7 +184,7 @@ object ImportResolver { } } - case class HttpResolver(relativeTo: Option[String], headers: Map[String, String], hostAllowlist: Option[List[String]]) + case class HttpResolver(relativeTo: Option[String], headers: Map[String, String], hostAllowlist: Option[List[String]], authProviders: List[ImportAuthProvider]) extends ImportResolver { import HttpResolver._ @@ -192,7 +197,7 @@ object ImportResolver { def newResolverList(newRoot: String): List[ImportResolver] = { val rootWithoutFilename = newRoot.split('/').init.mkString("", "/", "/") List( - HttpResolver(relativeTo = Some(canonicalize(rootWithoutFilename)), headers, hostAllowlist) + HttpResolver(relativeTo = Some(canonicalize(rootWithoutFilename)), headers, hostAllowlist, authProviders) ) } @@ -211,8 +216,14 @@ object ImportResolver { case None => true } + def authHeaders(uri: Uri): Map[String, String] = { + authProviders collectFirst { + case provider if provider.validHosts.contains(uri.host) => provider.authHeader() + } getOrElse Map.empty[String, String] + } + override def innerResolver(str: String, currentResolvers: List[ImportResolver]): Checked[ResolvedImportBundle] = - pathToLookup(str) flatMap { toLookup: WorkflowSource => + pathToLookup(str) flatMap { toLookup: WorkflowUrl => (Try { val uri: Uri = uri"$toLookup" @@ -227,9 +238,11 @@ object ImportResolver { }).contextualizeErrors(s"download $toLookup") } - private def getUri(toLookup: WorkflowSource): Either[NonEmptyList[WorkflowSource], ResolvedImportBundle] = { + private def getUri(toLookup: WorkflowUrl): Either[NonEmptyList[WorkflowSource], ResolvedImportBundle] = { implicit val sttpBackend = HttpResolver.sttpBackend() - val responseIO: IO[Response[WorkflowSource]] = sttp.get(uri"$toLookup").headers(headers).send() + + val authAmendedHeaders = headers ++ authHeaders(uri"$toLookup") + val responseIO: IO[Response[WorkflowSource]] = sttp.get(uri"$toLookup").headers(authAmendedHeaders).send() // temporary situation to get functionality working before // starting in on async-ifying the entire WdlNamespace flow @@ -252,7 +265,7 @@ object ImportResolver { import common.util.IntrospectableLazy import common.util.IntrospectableLazy._ - def apply(relativeTo: Option[String] = None, headers: Map[String, String] = Map.empty): HttpResolver = { + def apply(relativeTo: Option[String] = None, headers: Map[String, String] = Map.empty, authProviders: List[ImportAuthProvider] = List.empty): HttpResolver = { val config = ConfigFactory.load().getConfig("languages.WDL.http-allow-list") val allowListEnabled = config.as[Option[Boolean]]("enabled").getOrElse(false) val allowList: Option[List[String]] = @@ -260,7 +273,7 @@ object ImportResolver { config.as[Option[List[String]]]("allowed-http-hosts") else None - new HttpResolver(relativeTo, headers, allowList) + new HttpResolver(relativeTo, headers, allowList, authProviders) } val sttpBackend: IntrospectableLazy[SttpBackend[IO, Nothing]] = lazily { diff --git a/services/src/main/scala/cromwell/services/GithubAuthVendingActor.scala b/services/src/main/scala/cromwell/services/GithubAuthVendingActor.scala new file mode 100644 index 00000000000..fa89cf08c37 --- /dev/null +++ b/services/src/main/scala/cromwell/services/GithubAuthVendingActor.scala @@ -0,0 +1,56 @@ +package cromwell.services + +import akka.actor.{Actor, ActorRef, Props} +import akka.pattern.AskSupport +import akka.util.Timeout +import com.typesafe.config.Config +import com.typesafe.scalalogging.LazyLogging +import cromwell.core.Dispatcher.ServiceDispatcher +import cromwell.languages.util.ImportResolver.ImportAuthProvider +import cromwell.services.GithubAuthVendingActor.GithubAuthRequest +import cromwell.services.ServiceRegistryActor.ServiceRegistryMessage + +import scala.concurrent.{ExecutionContext, Future} +import scala.concurrent.duration.{Duration, DurationInt} + +class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, serviceRegistryActor: ActorRef) extends Actor with LazyLogging { + + override def receive: Receive = { + case GithubAuthRequest(terraToken, replyTo) => + replyTo ! GithubAuthVendingActor.GithubAuthVendingSuccess("access-token") + } +} + +object GithubAuthVendingActor { + + def props(serviceConfig: Config, globalConfig: Config, serviceRegistryActor: ActorRef): Props = + Props(new GithubAuthVendingActor(serviceConfig, globalConfig, serviceRegistryActor)) + .withDispatcher(ServiceDispatcher) + + sealed trait GithubAuthVendingMessage extends ServiceRegistryMessage { + override def serviceName: String = "GithubAuthVending" + } + + case class GithubAuthRequest(terraToken: String, replyTo: ActorRef) extends GithubAuthVendingMessage + + sealed trait GithubAuthVendingResponse extends GithubAuthVendingMessage + case class GithubAuthVendingSuccess(accessToken: String) extends GithubAuthVendingResponse + case class GithubAuthVendingFailure(error: Exception) extends GithubAuthVendingResponse + + trait GithubAuthVendingSupport extends AskSupport { + def serviceRegistry: ActorRef + implicit val timeout: Timeout = 10.seconds + implicit val ec: ExecutionContext + + def importAuthProvider(token: String): ImportAuthProvider = new ImportAuthProvider { + override def validHosts: List[String] = List("github.com") + override def authHeader(): Future[Map[String, String]] = { + serviceRegistry.ask(replyTo => GithubAuthRequest(token, replyTo)).mapTo[GithubAuthVendingResponse].flatMap { + case GithubAuthVendingSuccess(token) => Future.successful(Map("Authorization" -> s"Bearer ${token}")) + case GithubAuthVendingFailure(error) => Future.failed(error) + } + } + } + } + +} diff --git a/services/src/main/scala/cromwell/services/womtool/Describer.scala b/services/src/main/scala/cromwell/services/womtool/Describer.scala index 7b8116001da..6263b32c568 100644 --- a/services/src/main/scala/cromwell/services/womtool/Describer.scala +++ b/services/src/main/scala/cromwell/services/womtool/Describer.scala @@ -2,7 +2,7 @@ package cromwell.services.womtool import cats.data.Validated.{Invalid, Valid} import cromwell.core.WorkflowSourceFilesCollection -import cromwell.languages.util.ImportResolver.HttpResolver +import cromwell.languages.util.ImportResolver.{HttpResolver, ImportAuthProvider, ImportResolver} import cromwell.languages.util.{ImportResolver, LanguageFactoryUtil} import cromwell.languages.{LanguageFactory, ValidatedWomNamespace} import cromwell.services.womtool.WomtoolServiceMessages.{DescribeFailure, DescribeResult, DescribeSuccess} @@ -13,9 +13,9 @@ import wom.expression.NoIoFunctionSet object Describer { - def describeWorkflow(wsfc: WorkflowSourceFilesCollection): DescribeResult = { + def describeWorkflow(wsfc: WorkflowSourceFilesCollection, authProviders: List[ImportAuthProvider]): DescribeResult = { - val initialResolvers = List(HttpResolver(None, Map.empty)) + val initialResolvers: List[ImportResolver] = List(HttpResolver(None, Map.empty, authProviders)) // The HTTP resolver is used to pull down workflows submitted by URL LanguageFactoryUtil.findWorkflowSource(wsfc.workflowSource, wsfc.workflowUrl, initialResolvers) match { diff --git a/services/src/main/scala/cromwell/services/womtool/WomtoolServiceMessages.scala b/services/src/main/scala/cromwell/services/womtool/WomtoolServiceMessages.scala index ecbd41d5fc1..7ac0ec13d4a 100644 --- a/services/src/main/scala/cromwell/services/womtool/WomtoolServiceMessages.scala +++ b/services/src/main/scala/cromwell/services/womtool/WomtoolServiceMessages.scala @@ -1,6 +1,7 @@ package cromwell.services.womtool import cromwell.core.WorkflowSourceFilesCollection +import cromwell.languages.util.ImportResolver.ImportAuthProvider import cromwell.services.ServiceRegistryActor.ServiceRegistryMessage import cromwell.services.womtool.models.WorkflowDescription @@ -12,7 +13,7 @@ object WomtoolServiceMessages { override def serviceName: String = WomtoolServiceName } - case class DescribeRequest(filesCollection: WorkflowSourceFilesCollection) extends WomtoolServiceMessage + case class DescribeRequest(filesCollection: WorkflowSourceFilesCollection, authProviders: List[ImportAuthProvider]) extends WomtoolServiceMessage sealed trait DescribeResult extends WomtoolServiceMessage case class DescribeSuccess(description: WorkflowDescription) extends DescribeResult diff --git a/services/src/main/scala/cromwell/services/womtool/impl/WomtoolServiceInCromwellActor.scala b/services/src/main/scala/cromwell/services/womtool/impl/WomtoolServiceInCromwellActor.scala index ac1c748281d..d090024e629 100644 --- a/services/src/main/scala/cromwell/services/womtool/impl/WomtoolServiceInCromwellActor.scala +++ b/services/src/main/scala/cromwell/services/womtool/impl/WomtoolServiceInCromwellActor.scala @@ -18,11 +18,11 @@ class WomtoolServiceInCromwellActor(serviceConfig: Config, globalConfig: Config, implicit val ec: ExecutionContext = context.dispatcher override def receive: Receive = { - case DescribeRequest(filesCollection) => + case DescribeRequest(filesCollection, authProviders) => // We are consciously wrapping a Future around the Await.result way down in the HTTP import resolver until we can update the whole call hierarchy to async // https://doc.akka.io/docs/akka/2.5.16/actors.html?language=scala#ask-send-and-receive-future Future { - Describer.describeWorkflow(filesCollection) + Describer.describeWorkflow(filesCollection, authProviders) } pipeTo sender() () case ShutdownCommand => From 953fb13c91c316cae54d70419270cc2089bb7bcf Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Mon, 12 Feb 2024 18:40:37 -0500 Subject: [PATCH 02/28] Compiling [WIP] --- core/src/main/resources/reference.conf | 8 +++++- .../routes/WomtoolRouteSupport.scala | 10 +++---- .../routes/CromwellApiServiceSpec.scala | 2 +- .../languages/util/ImportResolver.scala | 2 +- .../languages/util/ImportResolverSpec.scala | 14 +++++----- .../NamespaceCacheSpec.scala | 2 +- .../impl}/GithubAuthVendingActor.scala | 16 +++++------ .../services/womtool/DescriberSpec.scala | 2 +- .../WomtoolServiceInCromwellActorSpec.scala | 28 +++++++++---------- 9 files changed, 43 insertions(+), 41 deletions(-) rename services/src/main/scala/cromwell/services/{ => auth/impl}/GithubAuthVendingActor.scala (82%) diff --git a/core/src/main/resources/reference.conf b/core/src/main/resources/reference.conf index 3fa11b458ff..b9c197be6b5 100644 --- a/core/src/main/resources/reference.conf +++ b/core/src/main/resources/reference.conf @@ -418,7 +418,7 @@ docker { throttle { number-of-requests = 1000 per = 60 seconds - } + } num-threads = 10 } google { @@ -571,6 +571,12 @@ services { # Default - run within the Cromwell JVM class = "cromwell.services.womtool.impl.WomtoolServiceInCromwellActor" } + GithubAuthVending { + class = "cromwell.services.auth.impl.GithubAuthVendingActor" + config { + access-token = "dummy-token" + } + } } include required(classpath("reference_database.inc.conf")) diff --git a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala index 9e58f8c32c5..ceac594eb03 100644 --- a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala +++ b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala @@ -1,17 +1,15 @@ package cromwell.webservice.routes -import akka.actor.{Actor, ActorRef, ActorRefFactory} -import akka.http.javadsl.model.headers.HttpCredentials +import akka.actor.{ActorRef, ActorRefFactory} import akka.http.scaladsl.model._ import akka.http.scaladsl.model.headers.OAuth2BearerToken import akka.http.scaladsl.server.Directives._ import akka.http.scaladsl.server.Route -import akka.pattern.ask import akka.stream.ActorMaterializer import akka.util.Timeout import cromwell.core.{WorkflowOptions, WorkflowSourceFilesCollection} import cromwell.languages.util.ImportResolver.ImportAuthProvider -import cromwell.services.GithubAuthVendingActor.GithubAuthVendingSupport +import cromwell.services.auth.impl.GithubAuthVendingActor.GithubAuthVendingSupport import cromwell.services.womtool.WomtoolServiceMessages.{DescribeFailure, DescribeRequest, DescribeResult, DescribeSuccess} import cromwell.webservice.WebServiceUtils import cromwell.webservice.WebServiceUtils.EnhancedThrowable @@ -19,7 +17,7 @@ import cromwell.webservice.WebServiceUtils.EnhancedThrowable import scala.concurrent.ExecutionContext import scala.util.{Failure, Success} -trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport { this: Actor => +trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport { implicit def actorRefFactory: ActorRefFactory implicit val ec: ExecutionContext @@ -37,7 +35,7 @@ trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport case Some(OAuth2BearerToken(token)) => List(importAuthProvider(token)) case _ => List.empty } - onComplete(materializeFormData(formData)) { + onComplete(materializeFormData(formData)(timeout, materializer, ec)) { case Success(data) => validateAndSubmitRequest(data, authProviders) case Failure(e) => diff --git a/engine/src/test/scala/cromwell/webservice/routes/CromwellApiServiceSpec.scala b/engine/src/test/scala/cromwell/webservice/routes/CromwellApiServiceSpec.scala index b7440bbe31c..9088879bc63 100644 --- a/engine/src/test/scala/cromwell/webservice/routes/CromwellApiServiceSpec.scala +++ b/engine/src/test/scala/cromwell/webservice/routes/CromwellApiServiceSpec.scala @@ -748,7 +748,7 @@ object CromwellApiServiceSpec { ) case oh => throw new Exception(s"Programmer Error! Unexpected case match: $oh") } - case DescribeRequest(sourceFiles) => + case DescribeRequest(sourceFiles, _) => sourceFiles.workflowSource match { case Some("fail to describe") => sender() ! DescribeFailure("as requested, failing to describe") diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index be8ce72a0fd..860da373ed1 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -218,7 +218,7 @@ object ImportResolver { def authHeaders(uri: Uri): Map[String, String] = { authProviders collectFirst { - case provider if provider.validHosts.contains(uri.host) => provider.authHeader() + case provider if provider.validHosts.contains(uri.host) => Await.result(provider.authHeader(), 10.seconds) } getOrElse Map.empty[String, String] } diff --git a/languageFactories/language-factory-core/src/test/scala/cromwell/languages/util/ImportResolverSpec.scala b/languageFactories/language-factory-core/src/test/scala/cromwell/languages/util/ImportResolverSpec.scala index 720a7f279af..d15318c6473 100644 --- a/languageFactories/language-factory-core/src/test/scala/cromwell/languages/util/ImportResolverSpec.scala +++ b/languageFactories/language-factory-core/src/test/scala/cromwell/languages/util/ImportResolverSpec.scala @@ -41,13 +41,13 @@ class ImportResolverSpec extends AnyFlatSpec with CromwellTimeoutSpec with Match } it should "resolve a path from no initial root" in { - val resolver = HttpResolver(None, Map.empty, None) + val resolver = HttpResolver(None, Map.empty, None, List.empty) val toResolve = resolver.pathToLookup("http://abc.com:8000/blah1/blah2.wdl") toResolve shouldBeValid "http://abc.com:8000/blah1/blah2.wdl" } it should "resolve a path and store the import in ResolvedImportRecord" in { - val resolver = HttpResolver(None, Map.empty, None) + val resolver = HttpResolver(None, Map.empty, None, List.empty) val importUri = "https://raw.githubusercontent.com/broadinstitute/cromwell/develop/centaur/src/main/resources/standardTestCases/hello/hello.wdl" val resolvedBundle = resolver.innerResolver(importUri, List(resolver)) @@ -64,14 +64,14 @@ class ImportResolverSpec extends AnyFlatSpec with CromwellTimeoutSpec with Match val pathEnd = "bob/loblaw/blah/blah.wdl" it should "allow any import when there is no allow list" in { - val resolver = HttpResolver(None, Map.empty, None) + val resolver = HttpResolver(None, Map.empty, None, List.empty) resolver.isAllowed(uri"https://my.favorite.wdls.com/$pathEnd") shouldBe true resolver.isAllowed(uri"http://some-garbage.whatever.eu/$pathEnd") shouldBe true resolver.isAllowed(uri"localhost:8080/my/secrets") shouldBe true } it should "allow any import that's on the allow list" in { - val resolver = HttpResolver(None, Map.empty, allowList) + val resolver = HttpResolver(None, Map.empty, allowList, List.empty) resolver.isAllowed(uri"https://my.favorite.wdls.com/$pathEnd") shouldBe true resolver.isAllowed(uri"http://anotherwdlsite.org/$pathEnd") shouldBe true resolver.isAllowed(uri"https://yetanotherwdlsite.org/$pathEnd") shouldBe false @@ -81,7 +81,7 @@ class ImportResolverSpec extends AnyFlatSpec with CromwellTimeoutSpec with Match } it should "allow nothing with an empty allow list" in { - val resolver = HttpResolver(None, Map.empty, Option(List.empty)) + val resolver = HttpResolver(None, Map.empty, Option(List.empty), List.empty) resolver.isAllowed(uri"https://my.favorite.wdls.com/$pathEnd") shouldBe false resolver.isAllowed(uri"http://anotherwdlsite.org/$pathEnd") shouldBe false resolver.isAllowed(uri"https://yetanotherwdlsite.org/$pathEnd") shouldBe false @@ -92,8 +92,8 @@ class ImportResolverSpec extends AnyFlatSpec with CromwellTimeoutSpec with Match behavior of "HttpResolver with a 'relativeTo' value" - val relativeHttpResolver = HttpResolver(relativeTo = Some("http://abc.com:8000/blah1/blah2/"), Map.empty, None) - val relativeToGithubHttpResolver = HttpResolver(relativeTo = Some(relativeToGithubRoot), Map.empty, None) + val relativeHttpResolver = HttpResolver(relativeTo = Some("http://abc.com:8000/blah1/blah2/"), Map.empty, None, List.empty) + val relativeToGithubHttpResolver = HttpResolver(relativeTo = Some(relativeToGithubRoot), Map.empty, None, List.empty) it should "resolve an absolute path from a different initial root" in { val pathToLookup = relativeHttpResolver.pathToLookup("http://def.org:8080/blah3.wdl") diff --git a/languageFactories/wdl-draft2/src/test/scala/languages.wdl.draft2/NamespaceCacheSpec.scala b/languageFactories/wdl-draft2/src/test/scala/languages.wdl.draft2/NamespaceCacheSpec.scala index eab4163410d..c50fb0df33a 100644 --- a/languageFactories/wdl-draft2/src/test/scala/languages.wdl.draft2/NamespaceCacheSpec.scala +++ b/languageFactories/wdl-draft2/src/test/scala/languages.wdl.draft2/NamespaceCacheSpec.scala @@ -61,7 +61,7 @@ class NamespaceCacheSpec extends AnyFlatSpec with CromwellTimeoutSpec with Befor ) var lookupCount = 0 - val countingResolver = new HttpResolver(None, Map.empty, None) { + val countingResolver = new HttpResolver(None, Map.empty, None, List.empty) { override def pathToLookup(str: String): Checked[String] = { lookupCount = lookupCount + 1 super.pathToLookup(str) diff --git a/services/src/main/scala/cromwell/services/GithubAuthVendingActor.scala b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala similarity index 82% rename from services/src/main/scala/cromwell/services/GithubAuthVendingActor.scala rename to services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala index fa89cf08c37..6af81aed254 100644 --- a/services/src/main/scala/cromwell/services/GithubAuthVendingActor.scala +++ b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala @@ -1,4 +1,4 @@ -package cromwell.services +package cromwell.services.auth.impl import akka.actor.{Actor, ActorRef, Props} import akka.pattern.AskSupport @@ -7,17 +7,15 @@ import com.typesafe.config.Config import com.typesafe.scalalogging.LazyLogging import cromwell.core.Dispatcher.ServiceDispatcher import cromwell.languages.util.ImportResolver.ImportAuthProvider -import cromwell.services.GithubAuthVendingActor.GithubAuthRequest import cromwell.services.ServiceRegistryActor.ServiceRegistryMessage - +import cromwell.services.auth.impl.GithubAuthVendingActor.GithubAuthRequest import scala.concurrent.{ExecutionContext, Future} -import scala.concurrent.duration.{Duration, DurationInt} class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, serviceRegistryActor: ActorRef) extends Actor with LazyLogging { override def receive: Receive = { - case GithubAuthRequest(terraToken, replyTo) => - replyTo ! GithubAuthVendingActor.GithubAuthVendingSuccess("access-token") + case GithubAuthRequest(_, replyTo) => + replyTo ! GithubAuthVendingActor.GithubAuthVendingSuccess(serviceConfig.getString("access-token")) } } @@ -38,14 +36,14 @@ object GithubAuthVendingActor { case class GithubAuthVendingFailure(error: Exception) extends GithubAuthVendingResponse trait GithubAuthVendingSupport extends AskSupport { - def serviceRegistry: ActorRef - implicit val timeout: Timeout = 10.seconds + def serviceRegistryActor: ActorRef + implicit val timeout: Timeout implicit val ec: ExecutionContext def importAuthProvider(token: String): ImportAuthProvider = new ImportAuthProvider { override def validHosts: List[String] = List("github.com") override def authHeader(): Future[Map[String, String]] = { - serviceRegistry.ask(replyTo => GithubAuthRequest(token, replyTo)).mapTo[GithubAuthVendingResponse].flatMap { + serviceRegistryActor.ask(replyTo => GithubAuthRequest(token, replyTo)).mapTo[GithubAuthVendingResponse].flatMap { case GithubAuthVendingSuccess(token) => Future.successful(Map("Authorization" -> s"Bearer ${token}")) case GithubAuthVendingFailure(error) => Future.failed(error) } diff --git a/services/src/test/scala/cromwell/services/womtool/DescriberSpec.scala b/services/src/test/scala/cromwell/services/womtool/DescriberSpec.scala index 47fba0684fc..31dcc07934b 100644 --- a/services/src/test/scala/cromwell/services/womtool/DescriberSpec.scala +++ b/services/src/test/scala/cromwell/services/womtool/DescriberSpec.scala @@ -73,7 +73,7 @@ class DescriberSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matchers { // // N.B. the `asJson` is highly significant as it exercises the entire serialization module and compares // the end product instead of an intermediate case class hierarchy - Describer.describeWorkflow(wsfc).asInstanceOf[DescribeSuccess].description.asJson shouldBe expectedJson + Describer.describeWorkflow(wsfc, List.empty).asInstanceOf[DescribeSuccess].description.asJson shouldBe expectedJson } } diff --git a/services/src/test/scala/cromwell/services/womtool/impl/WomtoolServiceInCromwellActorSpec.scala b/services/src/test/scala/cromwell/services/womtool/impl/WomtoolServiceInCromwellActorSpec.scala index 08b9955dcc1..29fd4a5970a 100644 --- a/services/src/test/scala/cromwell/services/womtool/impl/WomtoolServiceInCromwellActorSpec.scala +++ b/services/src/test/scala/cromwell/services/womtool/impl/WomtoolServiceInCromwellActorSpec.scala @@ -137,7 +137,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { val wsfc = wsfcConjurer(workflowSource = Option(TestData.wdlValid)) check( - DescribeRequest(wsfc), + DescribeRequest(wsfc, List.empty), DescribeSuccess( description = TestData.successfulDescription ) @@ -148,7 +148,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { val wsfc = wsfcConjurer(workflowSource = Option(TestData.wdlHttpImportValid)) check( - DescribeRequest(wsfc), + DescribeRequest(wsfc, List.empty), DescribeSuccess( description = TestData.successfulDescription ) @@ -160,7 +160,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { val wsfc = wsfcConjurer(workflowSource = Option(TestData.wdlValid), inputsJson = TestData.helloWorldInputs) check( - DescribeRequest(wsfc), + DescribeRequest(wsfc, List.empty), DescribeSuccess( description = TestData.successfulDescription ) @@ -172,7 +172,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { val wsfc = wsfcConjurer(workflowSource = Option(TestData.wdlValid), inputsJson = TestData.bogusInputs) check( - DescribeRequest(wsfc), + DescribeRequest(wsfc, List.empty), DescribeSuccess( description = WorkflowDescription( valid = false, @@ -192,7 +192,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { val wsfc = wsfcConjurer(workflowSource = Option(TestData.wdlValid), inputsJson = TestData.emptyInputs) check( - DescribeRequest(wsfc), + DescribeRequest(wsfc, List.empty), DescribeSuccess( description = WorkflowDescription( valid = false, @@ -212,7 +212,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { val wsfc = wsfcConjurer(workflowSource = Option(TestData.wdlValidNoInputs), inputsJson = TestData.emptyInputs) check( - DescribeRequest(wsfc), + DescribeRequest(wsfc, List.empty), DescribeSuccess( description = WorkflowDescription( valid = true, @@ -240,7 +240,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { val wsfc = wsfcConjurer(workflowSource = Option(TestData.wdlValidNoInputs), inputsJson = TestData.bogusInputs) check( - DescribeRequest(wsfc), + DescribeRequest(wsfc, List.empty), DescribeSuccess( description = WorkflowDescription( valid = false, @@ -262,7 +262,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { wsfcConjurer(workflowSource = Option(TestData.wdlValidDraft2NoInputs), inputsJson = TestData.bogusInputs) check( - DescribeRequest(wsfc), + DescribeRequest(wsfc, List.empty), DescribeSuccess( description = WorkflowDescription( valid = true, @@ -290,7 +290,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { val wsfc = wsfcConjurer(workflowUrl = Option(TestData.workflowUrlValid)) check( - DescribeRequest(wsfc), + DescribeRequest(wsfc, List.empty), DescribeSuccess( description = WorkflowDescription( valid = true, @@ -317,7 +317,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { val wsfc = wsfcConjurer() - check(DescribeRequest(wsfc), DescribeFailure("Either workflow source or url has to be supplied")) + check(DescribeRequest(wsfc, List.empty), DescribeFailure("Either workflow source or url has to be supplied")) } "return an error when both workflow URL and workflow source specified" in { @@ -325,7 +325,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { val wsfc = wsfcConjurer(workflowSource = Option(TestData.wdlInvalid), workflowUrl = Option(TestData.workflowUrlValid)) - check(DescribeRequest(wsfc), DescribeFailure("Both workflow source and url can't be supplied")) + check(DescribeRequest(wsfc, List.empty), DescribeFailure("Both workflow source and url can't be supplied")) } "return an error when the workflow URL is a 404" in { @@ -333,7 +333,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { val wsfc = wsfcConjurer(workflowUrl = Option(TestData.workflowUrlNotFound)) check( - DescribeRequest(wsfc), + DescribeRequest(wsfc, List.empty), DescribeFailure( "Failed to resolve 'https://raw.githubusercontent.com/broadinstitute/cromwell/develop/my_workflow' using resolver: 'http importer (no 'relative-to' origin)' (reason 1 of 1): Failed to download https://raw.githubusercontent.com/broadinstitute/cromwell/develop/my_workflow (reason 1 of 1): 404: Not Found" ) @@ -346,7 +346,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { // The error is from the OS network stack and differs between Mac and Linux (for { - result <- (womtoolActor ? DescribeRequest(wsfc)).mapTo[DescribeResult] + result <- (womtoolActor ? DescribeRequest(wsfc, List.empty)).mapTo[DescribeResult] _ = result should ( be( DescribeFailure( @@ -369,7 +369,7 @@ class WomtoolServiceInCromwellActorSpec extends ServicesSpec { // The HTTP resolver has figured out that you have not given it a URL and assumes it's a relative path check( - DescribeRequest(wsfc), + DescribeRequest(wsfc, List.empty), DescribeFailure( "Failed to resolve 'Zardoz' using resolver: 'http importer (no 'relative-to' origin)' (reason 1 of 1): Relative path" ) From 5d88d0b042e049df184be7f04b0991e63ca39b64 Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Tue, 13 Feb 2024 14:34:51 -0500 Subject: [PATCH 03/28] Working in local testing --- .../routes/WomtoolRouteSupport.scala | 4 +++- .../languages/util/ImportResolver.scala | 23 +++++++++++++++---- .../auth/impl/GithubAuthVendingActor.scala | 10 ++++---- 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala index ceac594eb03..dd6f1176c8a 100644 --- a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala +++ b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala @@ -7,6 +7,7 @@ import akka.http.scaladsl.server.Directives._ import akka.http.scaladsl.server.Route import akka.stream.ActorMaterializer import akka.util.Timeout +import com.typesafe.scalalogging.StrictLogging import cromwell.core.{WorkflowOptions, WorkflowSourceFilesCollection} import cromwell.languages.util.ImportResolver.ImportAuthProvider import cromwell.services.auth.impl.GithubAuthVendingActor.GithubAuthVendingSupport @@ -17,7 +18,7 @@ import cromwell.webservice.WebServiceUtils.EnhancedThrowable import scala.concurrent.ExecutionContext import scala.util.{Failure, Success} -trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport { +trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport with StrictLogging { implicit def actorRefFactory: ActorRefFactory implicit val ec: ExecutionContext @@ -31,6 +32,7 @@ trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport post { entity(as[Multipart.FormData]) { formData: Multipart.FormData => extractCredentials { creds => + logger.info(s"Received a POST request to /womtool/describe with creds: ${creds}") val authProviders: List[ImportAuthProvider] = creds match { case Some(OAuth2BearerToken(token)) => List(importAuthProvider(token)) case _ => List.empty diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index 860da373ed1..f45c4767570 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -10,6 +10,7 @@ import cats.syntax.validated._ import com.softwaremill.sttp._ import com.softwaremill.sttp.asynchttpclient.cats.AsyncHttpClientCatsBackend import com.typesafe.config.ConfigFactory +import com.typesafe.scalalogging.StrictLogging import common.Checked import common.transforms.CheckedAtoB import common.validation.ErrorOr._ @@ -185,13 +186,18 @@ object ImportResolver { } case class HttpResolver(relativeTo: Option[String], headers: Map[String, String], hostAllowlist: Option[List[String]], authProviders: List[ImportAuthProvider]) - extends ImportResolver { + extends ImportResolver with StrictLogging { import HttpResolver._ - override def name: String = relativeTo match { - case Some(relativeToPath) => s"http importer (relative to $relativeToPath)" - case None => "http importer (no 'relative-to' origin)" + override def name: String = { + val relativeToSuffix = relativeTo match { + case Some(relativeToPath) => s"(relative to $relativeToPath)" + case None => "(no 'relative-to' origin)" + } + val authProviderValidUrls = authProviders.flatMap(_.validHosts).mkString(", ") + val authProviderSuffix = if (authProviderValidUrls.nonEmpty) s"(authenticating for $authProviderValidUrls)" else "(without auth)" + s"HTTP resolver $relativeToSuffix $authProviderSuffix" } def newResolverList(newRoot: String): List[ImportResolver] = { @@ -217,8 +223,14 @@ object ImportResolver { } def authHeaders(uri: Uri): Map[String, String] = { + def checkAuthProvider(provider: ImportAuthProvider, uri: Uri): Boolean = { + val result = provider.validHosts.contains(uri.host) + logger.info(s"Validity of using 'auth provider for {${provider.validHosts.mkString(",")}} with uri: ${uri}': ${result}") + result + } + authProviders collectFirst { - case provider if provider.validHosts.contains(uri.host) => Await.result(provider.authHeader(), 10.seconds) + case provider if checkAuthProvider(provider, uri) => Await.result(provider.authHeader(), 10.seconds) } getOrElse Map.empty[String, String] } @@ -242,6 +254,7 @@ object ImportResolver { implicit val sttpBackend = HttpResolver.sttpBackend() val authAmendedHeaders = headers ++ authHeaders(uri"$toLookup") + logger.info(s"Fetching $toLookup with headers: $authAmendedHeaders") val responseIO: IO[Response[WorkflowSource]] = sttp.get(uri"$toLookup").headers(authAmendedHeaders).send() // temporary situation to get functionality working before diff --git a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala index 6af81aed254..1eca3875264 100644 --- a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala +++ b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala @@ -14,8 +14,8 @@ import scala.concurrent.{ExecutionContext, Future} class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, serviceRegistryActor: ActorRef) extends Actor with LazyLogging { override def receive: Receive = { - case GithubAuthRequest(_, replyTo) => - replyTo ! GithubAuthVendingActor.GithubAuthVendingSuccess(serviceConfig.getString("access-token")) + case GithubAuthRequest(_) => + sender() ! GithubAuthVendingActor.GithubAuthVendingSuccess(serviceConfig.getString("access-token")) } } @@ -29,7 +29,7 @@ object GithubAuthVendingActor { override def serviceName: String = "GithubAuthVending" } - case class GithubAuthRequest(terraToken: String, replyTo: ActorRef) extends GithubAuthVendingMessage + case class GithubAuthRequest(terraToken: String) extends GithubAuthVendingMessage sealed trait GithubAuthVendingResponse extends GithubAuthVendingMessage case class GithubAuthVendingSuccess(accessToken: String) extends GithubAuthVendingResponse @@ -41,9 +41,9 @@ object GithubAuthVendingActor { implicit val ec: ExecutionContext def importAuthProvider(token: String): ImportAuthProvider = new ImportAuthProvider { - override def validHosts: List[String] = List("github.com") + override def validHosts: List[String] = List("github.com", "githubusercontent.com", "raw.githubusercontent.com") override def authHeader(): Future[Map[String, String]] = { - serviceRegistryActor.ask(replyTo => GithubAuthRequest(token, replyTo)).mapTo[GithubAuthVendingResponse].flatMap { + serviceRegistryActor.ask(GithubAuthRequest(token)).mapTo[GithubAuthVendingResponse].flatMap { case GithubAuthVendingSuccess(token) => Future.successful(Map("Authorization" -> s"Bearer ${token}")) case GithubAuthVendingFailure(error) => Future.failed(error) } From 19c10977c4764aa96baa2de2827137ec321a4ca5 Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Tue, 13 Feb 2024 17:19:59 -0500 Subject: [PATCH 04/28] Remove unwanted logging --- .../cromwell/webservice/routes/WomtoolRouteSupport.scala | 3 +-- .../scala/cromwell/languages/util/ImportResolver.scala | 9 +-------- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala index dd6f1176c8a..4489e1f36cd 100644 --- a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala +++ b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala @@ -18,7 +18,7 @@ import cromwell.webservice.WebServiceUtils.EnhancedThrowable import scala.concurrent.ExecutionContext import scala.util.{Failure, Success} -trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport with StrictLogging { +trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport { implicit def actorRefFactory: ActorRefFactory implicit val ec: ExecutionContext @@ -32,7 +32,6 @@ trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport post { entity(as[Multipart.FormData]) { formData: Multipart.FormData => extractCredentials { creds => - logger.info(s"Received a POST request to /womtool/describe with creds: ${creds}") val authProviders: List[ImportAuthProvider] = creds match { case Some(OAuth2BearerToken(token)) => List(importAuthProvider(token)) case _ => List.empty diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index f45c4767570..2e9bd911f7a 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -223,14 +223,8 @@ object ImportResolver { } def authHeaders(uri: Uri): Map[String, String] = { - def checkAuthProvider(provider: ImportAuthProvider, uri: Uri): Boolean = { - val result = provider.validHosts.contains(uri.host) - logger.info(s"Validity of using 'auth provider for {${provider.validHosts.mkString(",")}} with uri: ${uri}': ${result}") - result - } - authProviders collectFirst { - case provider if checkAuthProvider(provider, uri) => Await.result(provider.authHeader(), 10.seconds) + case provider if provider.validHosts.contains(uri.host) => Await.result(provider.authHeader(), 10.seconds) } getOrElse Map.empty[String, String] } @@ -254,7 +248,6 @@ object ImportResolver { implicit val sttpBackend = HttpResolver.sttpBackend() val authAmendedHeaders = headers ++ authHeaders(uri"$toLookup") - logger.info(s"Fetching $toLookup with headers: $authAmendedHeaders") val responseIO: IO[Response[WorkflowSource]] = sttp.get(uri"$toLookup").headers(authAmendedHeaders).send() // temporary situation to get functionality working before From a1fd14d9b267d620622585de48393f17b02bee88 Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Tue, 13 Feb 2024 17:41:55 -0500 Subject: [PATCH 05/28] Unused import --- .../scala/cromwell/webservice/routes/WomtoolRouteSupport.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala index 4489e1f36cd..ceac594eb03 100644 --- a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala +++ b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala @@ -7,7 +7,6 @@ import akka.http.scaladsl.server.Directives._ import akka.http.scaladsl.server.Route import akka.stream.ActorMaterializer import akka.util.Timeout -import com.typesafe.scalalogging.StrictLogging import cromwell.core.{WorkflowOptions, WorkflowSourceFilesCollection} import cromwell.languages.util.ImportResolver.ImportAuthProvider import cromwell.services.auth.impl.GithubAuthVendingActor.GithubAuthVendingSupport From 9cbf10a8b4204f946a13bd703828802ab477ada3 Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Wed, 14 Feb 2024 17:29:20 -0500 Subject: [PATCH 06/28] Make vending optional. Extract interface out of impl --- core/src/main/resources/reference.conf | 2 +- .../services/auth/GithubAuthVending.scala | 18 +++++++++ .../auth/GithubAuthVendingSupport.scala | 28 +++++++++++++ .../auth/impl/GithubAuthVendingActor.scala | 39 ++++--------------- 4 files changed, 54 insertions(+), 33 deletions(-) create mode 100644 services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala create mode 100644 services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala diff --git a/core/src/main/resources/reference.conf b/core/src/main/resources/reference.conf index b9c197be6b5..54e56d4f33b 100644 --- a/core/src/main/resources/reference.conf +++ b/core/src/main/resources/reference.conf @@ -574,7 +574,7 @@ services { GithubAuthVending { class = "cromwell.services.auth.impl.GithubAuthVendingActor" config { - access-token = "dummy-token" + enabled = false } } } diff --git a/services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala b/services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala new file mode 100644 index 00000000000..345fbc33113 --- /dev/null +++ b/services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala @@ -0,0 +1,18 @@ +package cromwell.services.auth + +import cromwell.services.ServiceRegistryActor.ServiceRegistryMessage + +object GithubAuthVending { + sealed trait GithubAuthVendingMessage extends ServiceRegistryMessage { + override def serviceName: String = "GithubAuthVending" + } + + case class GithubAuthRequest(terraToken: String) extends GithubAuthVendingMessage + + sealed trait GithubAuthVendingResponse extends GithubAuthVendingMessage + case class GithubAuthTokenResponse(accessToken: String) extends GithubAuthVendingResponse + case object NoGithubAuthResponse extends GithubAuthVendingResponse + case class GithubAuthVendingFailure(error: Exception) extends GithubAuthVendingResponse + + +} diff --git a/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala b/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala new file mode 100644 index 00000000000..e55ff36f829 --- /dev/null +++ b/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala @@ -0,0 +1,28 @@ +package cromwell.services.auth + +import akka.actor.ActorRef +import akka.pattern.AskSupport +import akka.util.Timeout +import cromwell.languages.util.ImportResolver.ImportAuthProvider +import cromwell.services.auth.GithubAuthVending.{GithubAuthRequest, GithubAuthVendingFailure, GithubAuthVendingResponse, GithubAuthTokenResponse} + +import scala.concurrent.{ExecutionContext, Future} + +trait GithubAuthVendingSupport extends AskSupport { + + def serviceRegistryActor: ActorRef + + implicit val timeout: Timeout + implicit val ec: ExecutionContext + + def importAuthProvider(token: String): ImportAuthProvider = new ImportAuthProvider { + override def validHosts: List[String] = List("github.com", "githubusercontent.com", "raw.githubusercontent.com") + + override def authHeader(): Future[Map[String, String]] = { + serviceRegistryActor.ask(GithubAuthRequest(token)).mapTo[GithubAuthVendingResponse].flatMap { + case GithubAuthTokenResponse(token) => Future.successful(Map("Authorization" -> s"Bearer ${token}")) + case GithubAuthVendingFailure(error) => Future.failed(error) + } + } + } +} diff --git a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala index 1eca3875264..242b934d76f 100644 --- a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala +++ b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala @@ -6,16 +6,17 @@ import akka.util.Timeout import com.typesafe.config.Config import com.typesafe.scalalogging.LazyLogging import cromwell.core.Dispatcher.ServiceDispatcher -import cromwell.languages.util.ImportResolver.ImportAuthProvider -import cromwell.services.ServiceRegistryActor.ServiceRegistryMessage -import cromwell.services.auth.impl.GithubAuthVendingActor.GithubAuthRequest -import scala.concurrent.{ExecutionContext, Future} +import cromwell.services.auth.GithubAuthVending.{GithubAuthRequest, GithubAuthTokenResponse, NoGithubAuthResponse} class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, serviceRegistryActor: ActorRef) extends Actor with LazyLogging { + lazy val enabled = serviceConfig.getBoolean("enabled") + override def receive: Receive = { - case GithubAuthRequest(_) => - sender() ! GithubAuthVendingActor.GithubAuthVendingSuccess(serviceConfig.getString("access-token")) + case GithubAuthRequest(_) if enabled => + sender() ! GithubAuthTokenResponse(serviceConfig.getString("access-token")) + case _ => + sender() ! NoGithubAuthResponse } } @@ -25,30 +26,4 @@ object GithubAuthVendingActor { Props(new GithubAuthVendingActor(serviceConfig, globalConfig, serviceRegistryActor)) .withDispatcher(ServiceDispatcher) - sealed trait GithubAuthVendingMessage extends ServiceRegistryMessage { - override def serviceName: String = "GithubAuthVending" - } - - case class GithubAuthRequest(terraToken: String) extends GithubAuthVendingMessage - - sealed trait GithubAuthVendingResponse extends GithubAuthVendingMessage - case class GithubAuthVendingSuccess(accessToken: String) extends GithubAuthVendingResponse - case class GithubAuthVendingFailure(error: Exception) extends GithubAuthVendingResponse - - trait GithubAuthVendingSupport extends AskSupport { - def serviceRegistryActor: ActorRef - implicit val timeout: Timeout - implicit val ec: ExecutionContext - - def importAuthProvider(token: String): ImportAuthProvider = new ImportAuthProvider { - override def validHosts: List[String] = List("github.com", "githubusercontent.com", "raw.githubusercontent.com") - override def authHeader(): Future[Map[String, String]] = { - serviceRegistryActor.ask(GithubAuthRequest(token)).mapTo[GithubAuthVendingResponse].flatMap { - case GithubAuthVendingSuccess(token) => Future.successful(Map("Authorization" -> s"Bearer ${token}")) - case GithubAuthVendingFailure(error) => Future.failed(error) - } - } - } - } - } From 3483604b0a1d1a348676147fe36472a7874280d3 Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Wed, 14 Feb 2024 17:40:41 -0500 Subject: [PATCH 07/28] Undo name change which breaks tests --- .../main/scala/cromwell/languages/util/ImportResolver.scala | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index 2e9bd911f7a..4f8b2f64ede 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -194,10 +194,8 @@ object ImportResolver { case Some(relativeToPath) => s"(relative to $relativeToPath)" case None => "(no 'relative-to' origin)" } - val authProviderValidUrls = authProviders.flatMap(_.validHosts).mkString(", ") - val authProviderSuffix = if (authProviderValidUrls.nonEmpty) s"(authenticating for $authProviderValidUrls)" else "(without auth)" - s"HTTP resolver $relativeToSuffix $authProviderSuffix" + s"HTTP resolver $relativeToSuffix" } def newResolverList(newRoot: String): List[ImportResolver] = { From 6bbea511a28240d3fd1e062bea886af5515504e8 Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Wed, 14 Feb 2024 18:06:08 -0500 Subject: [PATCH 08/28] Fix compile issues --- .../scala/cromwell/webservice/routes/WomtoolRouteSupport.scala | 2 +- .../cromwell/services/auth/GithubAuthVendingSupport.scala | 3 ++- .../cromwell/services/auth/impl/GithubAuthVendingActor.scala | 2 -- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala index ceac594eb03..2cb4f8d0da3 100644 --- a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala +++ b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala @@ -9,7 +9,7 @@ import akka.stream.ActorMaterializer import akka.util.Timeout import cromwell.core.{WorkflowOptions, WorkflowSourceFilesCollection} import cromwell.languages.util.ImportResolver.ImportAuthProvider -import cromwell.services.auth.impl.GithubAuthVendingActor.GithubAuthVendingSupport +import cromwell.services.auth.GithubAuthVendingSupport import cromwell.services.womtool.WomtoolServiceMessages.{DescribeFailure, DescribeRequest, DescribeResult, DescribeSuccess} import cromwell.webservice.WebServiceUtils import cromwell.webservice.WebServiceUtils.EnhancedThrowable diff --git a/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala b/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala index e55ff36f829..c9168e56b0c 100644 --- a/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala +++ b/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala @@ -4,7 +4,7 @@ import akka.actor.ActorRef import akka.pattern.AskSupport import akka.util.Timeout import cromwell.languages.util.ImportResolver.ImportAuthProvider -import cromwell.services.auth.GithubAuthVending.{GithubAuthRequest, GithubAuthVendingFailure, GithubAuthVendingResponse, GithubAuthTokenResponse} +import cromwell.services.auth.GithubAuthVending.{GithubAuthRequest, GithubAuthTokenResponse, GithubAuthVendingFailure, GithubAuthVendingResponse, NoGithubAuthResponse} import scala.concurrent.{ExecutionContext, Future} @@ -21,6 +21,7 @@ trait GithubAuthVendingSupport extends AskSupport { override def authHeader(): Future[Map[String, String]] = { serviceRegistryActor.ask(GithubAuthRequest(token)).mapTo[GithubAuthVendingResponse].flatMap { case GithubAuthTokenResponse(token) => Future.successful(Map("Authorization" -> s"Bearer ${token}")) + case NoGithubAuthResponse => Future.successful(Map.empty) case GithubAuthVendingFailure(error) => Future.failed(error) } } diff --git a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala index 242b934d76f..8bf112754dd 100644 --- a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala +++ b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala @@ -1,8 +1,6 @@ package cromwell.services.auth.impl import akka.actor.{Actor, ActorRef, Props} -import akka.pattern.AskSupport -import akka.util.Timeout import com.typesafe.config.Config import com.typesafe.scalalogging.LazyLogging import cromwell.core.Dispatcher.ServiceDispatcher From 19ae950f6b8752d5234db42eb46ef48f365ebb60 Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Thu, 15 Feb 2024 15:49:01 -0500 Subject: [PATCH 09/28] Tidy the await.result, albeit without fixing the world :( --- .../cromwell/languages/util/ImportResolver.scala | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index 4f8b2f64ede..f1a2370e3b6 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -220,10 +220,10 @@ object ImportResolver { case None => true } - def authHeaders(uri: Uri): Map[String, String] = { + def fetchAuthHeaders(uri: Uri): Future[Map[String, String]] = { authProviders collectFirst { - case provider if provider.validHosts.contains(uri.host) => Await.result(provider.authHeader(), 10.seconds) - } getOrElse Map.empty[String, String] + case provider if provider.validHosts.contains(uri.host) => provider.authHeader() + } getOrElse Future.successful(Map.empty[String, String]) } override def innerResolver(str: String, currentResolvers: List[ImportResolver]): Checked[ResolvedImportBundle] = @@ -245,15 +245,15 @@ object ImportResolver { private def getUri(toLookup: WorkflowUrl): Either[NonEmptyList[WorkflowSource], ResolvedImportBundle] = { implicit val sttpBackend = HttpResolver.sttpBackend() - val authAmendedHeaders = headers ++ authHeaders(uri"$toLookup") - val responseIO: IO[Response[WorkflowSource]] = sttp.get(uri"$toLookup").headers(authAmendedHeaders).send() - - // temporary situation to get functionality working before + // Temporary situation to get functionality working before // starting in on async-ifying the entire WdlNamespace flow + // Note: this will cause the calling thread to block for up to 30 seconds + // (15 for the auth header lookup, 15 for the http request) + val authHeaders = Await.result(fetchAuthHeaders(uri"$toLookup"), 15.seconds) + val responseIO: IO[Response[WorkflowSource]] = sttp.get(uri"$toLookup").headers(headers ++ authHeaders).send() val result: Checked[WorkflowSource] = Await.result(responseIO.unsafeToFuture(), 15.seconds).body.leftMap { e => NonEmptyList(e.toString.trim, List.empty) } - result map { ResolvedImportBundle(_, newResolverList(toLookup), ResolvedImportRecord(toLookup)) } From 19205a4ff049e08cdfa71b9e1aa12ab6e50d696c Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Thu, 15 Feb 2024 16:40:57 -0500 Subject: [PATCH 10/28] Retry on 404 --- .../languages/util/ImportResolver.scala | 30 ++++++++++++++----- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index f1a2370e3b6..74e745d1e15 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -242,23 +242,37 @@ object ImportResolver { }).contextualizeErrors(s"download $toLookup") } - private def getUri(toLookup: WorkflowUrl): Either[NonEmptyList[WorkflowSource], ResolvedImportBundle] = { - implicit val sttpBackend = HttpResolver.sttpBackend() - + private def getUri(toLookup: WorkflowUrl): Checked[ResolvedImportBundle] = { // Temporary situation to get functionality working before // starting in on async-ifying the entire WdlNamespace flow // Note: this will cause the calling thread to block for up to 30 seconds // (15 for the auth header lookup, 15 for the http request) - val authHeaders = Await.result(fetchAuthHeaders(uri"$toLookup"), 15.seconds) - val responseIO: IO[Response[WorkflowSource]] = sttp.get(uri"$toLookup").headers(headers ++ authHeaders).send() - val result: Checked[WorkflowSource] = Await.result(responseIO.unsafeToFuture(), 15.seconds).body.leftMap { e => - NonEmptyList(e.toString.trim, List.empty) + val unauthedAttempt = getUriInner(toLookup, Map.empty) + val result = if (StatusCodes.NotFound == unauthedAttempt.code) { + val authHeaders = Await.result(fetchAuthHeaders(uri"$toLookup"), 15.seconds) + if (authHeaders.nonEmpty) { + getUriInner(toLookup, authHeaders) + } else { + unauthedAttempt + } + } else { + unauthedAttempt } - result map { + + result.body.leftMap { e => + NonEmptyList.of(s"${result.code}: ${e.trim}") + } map { ResolvedImportBundle(_, newResolverList(toLookup), ResolvedImportRecord(toLookup)) } } + private def getUriInner(toLookup: WorkflowUrl, authHeaders: Map[String, String]): Response[WorkflowSource] = { + implicit val sttpBackend = HttpResolver.sttpBackend() + + val responseIO: IO[Response[WorkflowSource]] = sttp.get(uri"$toLookup").headers(headers ++ authHeaders).send() + Await.result(responseIO.unsafeToFuture(), 15.seconds) + } + override def cleanupIfNecessary(): ErrorOr[Unit] = ().validNel override def hashKey: ErrorOr[String] = relativeTo.toString.md5Sum.validNel From 645da1d8e628885865370de9b56ead8ccbebec3f Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Thu, 15 Feb 2024 17:43:53 -0500 Subject: [PATCH 11/28] Add tests for new ImportResolver behavior --- .../languages/util/ImportResolver.scala | 6 +- .../languages/util/ImportResolverSpec.scala | 98 ++++++++++++++++++- .../auth/GithubAuthVendingSupport.scala | 6 +- 3 files changed, 104 insertions(+), 6 deletions(-) diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index 74e745d1e15..6a96b4f1bbe 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -43,6 +43,10 @@ object ImportResolver { def authHeader(): Future[Map[String, String]] } + trait GithubImportAuthProvider extends ImportAuthProvider { + override def validHosts: List[String] = List("github.com", "githubusercontent.com", "raw.githubusercontent.com") + } + trait ImportResolver { def name: String protected def innerResolver(path: String, currentResolvers: List[ImportResolver]): Checked[ResolvedImportBundle] @@ -266,7 +270,7 @@ object ImportResolver { } } - private def getUriInner(toLookup: WorkflowUrl, authHeaders: Map[String, String]): Response[WorkflowSource] = { + protected def getUriInner(toLookup: WorkflowUrl, authHeaders: Map[String, String]): Response[WorkflowSource] = { implicit val sttpBackend = HttpResolver.sttpBackend() val responseIO: IO[Response[WorkflowSource]] = sttp.get(uri"$toLookup").headers(headers ++ authHeaders).send() diff --git a/languageFactories/language-factory-core/src/test/scala/cromwell/languages/util/ImportResolverSpec.scala b/languageFactories/language-factory-core/src/test/scala/cromwell/languages/util/ImportResolverSpec.scala index d15318c6473..e03069e6c18 100644 --- a/languageFactories/language-factory-core/src/test/scala/cromwell/languages/util/ImportResolverSpec.scala +++ b/languageFactories/language-factory-core/src/test/scala/cromwell/languages/util/ImportResolverSpec.scala @@ -7,9 +7,12 @@ import common.assertion.CromwellTimeoutSpec import common.assertion.ErrorOrAssertions._ import cromwell.core.WorkflowId import cromwell.core.path.DefaultPath -import cromwell.languages.util.ImportResolver.{DirectoryResolver, HttpResolver} +import cromwell.languages.util.ImportResolver.{DirectoryResolver, GithubImportAuthProvider, HttpResolver, ImportAuthProvider} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers +import wom.core.{WorkflowSource, WorkflowUrl} + +import scala.concurrent.Future class ImportResolverSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matchers { behavior of "HttpResolver" @@ -155,6 +158,99 @@ class ImportResolverSpec extends AnyFlatSpec with CromwellTimeoutSpec with Match } } + behavior of "HttpResolver with an ImportAuthProvider" + + class RecordingHttpResolver(unauthedResponse: Response[String], authedResponse: Response[String], authProvider: ImportAuthProvider) extends HttpResolver(None, Map.empty, None, List(authProvider)) { + + case class RequestRecord(url: WorkflowUrl, headers: Map[String, String], response: Response[String]) + var requestRecords: List[RequestRecord] = List.empty + + override protected def getUriInner(toLookup: WorkflowUrl, authHeaders: Map[String, String]): Response[WorkflowSource] = { + val result = if (Uri.parse(toLookup).get.host == "raw.githubusercontent.com" && authHeaders.get("Authorization").contains("Bearer 1234")) { + authedResponse + } else if (headers.contains("Authorization")) { + throw new RuntimeException(s"Unexpected auth header applied") + } else { + unauthedResponse + } + + requestRecords = requestRecords :+ RequestRecord(toLookup, authHeaders, result) + result + } + } + + it should "lookup headers from auth provider after a 404 for valid host" in { + val unauthedResponse = new Response[String](Left("Not found or no permissions".getBytes), StatusCodes.NotFound, "NotFound", Nil, List.empty) + val authedResponse = new Response[String](Right("Hello World"), 200, "OK", Nil, List.empty) + val authProvider = new GithubImportAuthProvider { + override def authHeader() = Future.successful(Map("Authorization" -> "Bearer 1234")) + } + val resolver = new RecordingHttpResolver(unauthedResponse, authedResponse, authProvider) + val importUri = "https://raw.githubusercontent.com/broadinstitute/cromwell/develop/centaur/src/main/resources/standardTestCases/hello/hello.wdl" + val resolvedBundle = resolver.innerResolver(importUri, List(resolver)) + + resolvedBundle match { + case Left(e) => fail(s"Expected ResolvedImportBundle but got $e") + case Right(bundle) => { + bundle.resolvedImportRecord.importPath shouldBe importUri + bundle.source shouldBe "Hello World" + + resolver.requestRecords.size shouldBe 2 + resolver.requestRecords.head.url shouldBe importUri + resolver.requestRecords.head.headers shouldBe Map.empty + resolver.requestRecords(1).url shouldBe importUri + resolver.requestRecords(1).headers should be(Map(("Authorization", "Bearer 1234"))) + } + } + } + + it should "not lookup headers for urls which require no auth" in { + val unauthedResponse = new Response[String](Right("Hello World"), 200, "OK", Nil, List.empty) + val authedResponse = new Response[String](Left("Shouldn't be authed".getBytes), 500, "BAD", Nil, List.empty) + val authProvider = new GithubImportAuthProvider { + override def authHeader() = Future.failed(new Exception("Should not be called")) + } + val resolver = new RecordingHttpResolver(unauthedResponse, authedResponse, authProvider) + val importUri = "https://raw.githubusercontent.com/broadinstitute/cromwell/develop/centaur/src/main/resources/standardTestCases/hello/hello.wdl" + val resolvedBundle = resolver.innerResolver(importUri, List(resolver)) + + resolvedBundle match { + case Left(e) => fail(s"Expected ResolvedImportBundle but got $e") + case Right(bundle) => { + bundle.resolvedImportRecord.importPath shouldBe importUri + bundle.source shouldBe "Hello World" + + resolver.requestRecords.size shouldBe 1 + resolver.requestRecords.head.url shouldBe importUri + resolver.requestRecords.head.headers shouldBe Map.empty + } + } + } + + it should "not lookup headers for urls which failed with other errors" in { + val unauthedResponse = new Response[String](Left("Some other error".getBytes), StatusCodes.ServiceUnavailable, "ServiceUnavailable", Nil, List.empty) + val authedResponse = new Response[String](Left("Shouldn't be authed".getBytes), 500, "BAD", Nil, List.empty) + val authProvider = new GithubImportAuthProvider { + override def authHeader() = Future.failed(new Exception("Should not be called")) + } + val resolver = new RecordingHttpResolver(unauthedResponse, authedResponse, authProvider) + val importUri = "https://raw.githubusercontent.com/broadinstitute/cromwell/develop/centaur/src/main/resources/standardTestCases/hello/hello.wdl" + val resolvedBundle = resolver.innerResolver(importUri, List(resolver)) + + resolvedBundle match { + case Left(e) => { + e.length should be(1) + e.head.contains("Some other error") should be(true) + resolver.requestRecords.size shouldBe 1 + resolver.requestRecords.head.url shouldBe importUri + resolver.requestRecords.head.headers shouldBe Map.empty + } + case Right(bundle) => { + fail(s"Expected an error but got $bundle") + } + } + } + behavior of "directory resolver from root" val workingDirectory = sys.props("user.dir") diff --git a/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala b/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala index c9168e56b0c..37b1bcde155 100644 --- a/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala +++ b/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala @@ -3,7 +3,7 @@ package cromwell.services.auth import akka.actor.ActorRef import akka.pattern.AskSupport import akka.util.Timeout -import cromwell.languages.util.ImportResolver.ImportAuthProvider +import cromwell.languages.util.ImportResolver.{GithubImportAuthProvider, ImportAuthProvider} import cromwell.services.auth.GithubAuthVending.{GithubAuthRequest, GithubAuthTokenResponse, GithubAuthVendingFailure, GithubAuthVendingResponse, NoGithubAuthResponse} import scala.concurrent.{ExecutionContext, Future} @@ -15,9 +15,7 @@ trait GithubAuthVendingSupport extends AskSupport { implicit val timeout: Timeout implicit val ec: ExecutionContext - def importAuthProvider(token: String): ImportAuthProvider = new ImportAuthProvider { - override def validHosts: List[String] = List("github.com", "githubusercontent.com", "raw.githubusercontent.com") - + def importAuthProvider(token: String): ImportAuthProvider = new GithubImportAuthProvider { override def authHeader(): Future[Map[String, String]] = { serviceRegistryActor.ask(GithubAuthRequest(token)).mapTo[GithubAuthVendingResponse].flatMap { case GithubAuthTokenResponse(token) => Future.successful(Map("Authorization" -> s"Bearer ${token}")) From 4964e9eb167553debfa5a7ed2e515a7d78432bfe Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Thu, 15 Feb 2024 18:31:25 -0500 Subject: [PATCH 12/28] Add tests for new GithubAuthVendingSupport --- .../auth/GithubAuthVendingSupport.scala | 19 +++- .../auth/GithubAuthVendingSupportSpec.scala | 101 ++++++++++++++++++ 2 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 services/src/test/scala/cromwell/services/auth/GithubAuthVendingSupportSpec.scala diff --git a/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala b/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala index 37b1bcde155..babb423402e 100644 --- a/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala +++ b/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala @@ -1,14 +1,15 @@ package cromwell.services.auth import akka.actor.ActorRef -import akka.pattern.AskSupport +import akka.pattern.{AskSupport, AskTimeoutException} import akka.util.Timeout +import com.typesafe.scalalogging.StrictLogging import cromwell.languages.util.ImportResolver.{GithubImportAuthProvider, ImportAuthProvider} import cromwell.services.auth.GithubAuthVending.{GithubAuthRequest, GithubAuthTokenResponse, GithubAuthVendingFailure, GithubAuthVendingResponse, NoGithubAuthResponse} import scala.concurrent.{ExecutionContext, Future} -trait GithubAuthVendingSupport extends AskSupport { +trait GithubAuthVendingSupport extends AskSupport with StrictLogging { def serviceRegistryActor: ActorRef @@ -17,10 +18,20 @@ trait GithubAuthVendingSupport extends AskSupport { def importAuthProvider(token: String): ImportAuthProvider = new GithubImportAuthProvider { override def authHeader(): Future[Map[String, String]] = { - serviceRegistryActor.ask(GithubAuthRequest(token)).mapTo[GithubAuthVendingResponse].flatMap { + serviceRegistryActor + .ask(GithubAuthRequest(token)) + .mapTo[GithubAuthVendingResponse] + .recoverWith { + case e: AskTimeoutException => Future.failed(new Exception(s"Unable to resolve github auth token within allowed time", e)) + case e: Throwable => + // This "should" never happen. If it does, let's make it obvious and trigger our alerting: + logger.error("Programmer error: Unexpected failure to resolve github auth token", e) + Future.failed(new Exception("Failed to resolve github auth token", e)) + } + .flatMap { case GithubAuthTokenResponse(token) => Future.successful(Map("Authorization" -> s"Bearer ${token}")) case NoGithubAuthResponse => Future.successful(Map.empty) - case GithubAuthVendingFailure(error) => Future.failed(error) + case GithubAuthVendingFailure(error) => Future.failed(new Exception("Failed to resolve github auth token", error)) } } } diff --git a/services/src/test/scala/cromwell/services/auth/GithubAuthVendingSupportSpec.scala b/services/src/test/scala/cromwell/services/auth/GithubAuthVendingSupportSpec.scala new file mode 100644 index 00000000000..477fbbf5f72 --- /dev/null +++ b/services/src/test/scala/cromwell/services/auth/GithubAuthVendingSupportSpec.scala @@ -0,0 +1,101 @@ +package cromwell.services.auth + +import akka.actor.ActorRef +import akka.testkit.TestProbe +import akka.util.Timeout +import cromwell.core.TestKitSuite +import cromwell.services.ServiceRegistryActor.ServiceRegistryFailure +import cromwell.services.auth.GithubAuthVending.GithubAuthRequest +import cromwell.services.auth.GithubAuthVendingSupportSpec.TestGithubAuthVendingSupport +import org.scalatest.concurrent.Eventually +import org.scalatest.flatspec.AnyFlatSpecLike +import org.scalatest.matchers.should.Matchers + +import scala.concurrent.{Await, ExecutionContext, Future} +import scala.concurrent.duration._ + +class GithubAuthVendingSupportSpec extends TestKitSuite + with AnyFlatSpecLike + with Matchers + with Eventually { + + behavior of "GithubAuthVendingSupport" + + it should "send a message to the service registry and handle success response" in { + val serviceRegistryActor = TestProbe() + val testSupport = new TestGithubAuthVendingSupport(serviceRegistryActor.ref) + val provider = testSupport.importAuthProvider("user-token") + val authHeader: Future[Map[String, String]] = provider.authHeader() + + serviceRegistryActor.expectMsg(GithubAuthRequest("user-token")) + serviceRegistryActor.reply(GithubAuthVending.GithubAuthTokenResponse("github-token")) + + Await.result(authHeader, 10.seconds) should be(Map("Authorization" -> "Bearer github-token")) + } + + it should "handle 'no auth' responses" in { + val serviceRegistryActor = TestProbe() + val testSupport = new TestGithubAuthVendingSupport(serviceRegistryActor.ref) + val provider = testSupport.importAuthProvider("user-token") + val authHeader: Future[Map[String, String]] = provider.authHeader() + + serviceRegistryActor.expectMsg(GithubAuthRequest("user-token")) + serviceRegistryActor.reply(GithubAuthVending.NoGithubAuthResponse) + + Await.result(authHeader, 10.seconds) should be(Map.empty) + } + + it should "convert error responses in Future failures" in { + val serviceRegistryActor = TestProbe() + val testSupport = new TestGithubAuthVendingSupport(serviceRegistryActor.ref) + val provider = testSupport.importAuthProvider("user-token") + val authHeader: Future[Map[String, String]] = provider.authHeader() + + serviceRegistryActor.expectMsg(GithubAuthRequest("user-token")) + serviceRegistryActor.reply(GithubAuthVending.GithubAuthVendingFailure(new Exception("BOOM"))) + + eventually { + authHeader.isCompleted should be(true) + authHeader.value.get.failed.get.getMessage should be("Failed to resolve github auth token") + authHeader.value.get.failed.get.getCause.getMessage should be("BOOM") + } + } + + it should "handle timeouts" in { + val serviceRegistryActor = TestProbe() + val testSupport = new TestGithubAuthVendingSupport(serviceRegistryActor.ref, 1.millisecond) + val provider = testSupport.importAuthProvider("user-token") + val authHeader: Future[Map[String, String]] = provider.authHeader() + + eventually { + authHeader.isCompleted should be(true) + authHeader.value.get.failed.get.getMessage should be("Unable to resolve github auth token within allowed time") + authHeader.value.get.failed.get.getCause.getMessage should startWith("Ask timed out on") + } + } + + it should "handle ask failures" in { + val serviceRegistryActor = TestProbe() + val testSupport = new TestGithubAuthVendingSupport(serviceRegistryActor.ref) + val provider = testSupport.importAuthProvider("user-token") + val authHeader: Future[Map[String, String]] = provider.authHeader() + + serviceRegistryActor.expectMsg(GithubAuthRequest("user-token")) + serviceRegistryActor.reply(ServiceRegistryFailure("GithubAuthVending")) + + eventually { + authHeader.isCompleted should be(true) + authHeader.value.get.failed.get.getMessage should be("Failed to resolve github auth token") + // not the prettiest error message, but at least it should give us something to work with at debug time: + authHeader.value.get.failed.get.getCause.getMessage should startWith("Cannot cast") + } + } + +} + +object GithubAuthVendingSupportSpec { + class TestGithubAuthVendingSupport(val serviceRegistryActor: ActorRef, val timeout: Timeout = 10.seconds) extends GithubAuthVendingSupport { + override implicit val ec: ExecutionContext = ExecutionContext.global + } + +} From 12b2f0e612f526c3dab118857564b8cc7976271c Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Thu, 15 Feb 2024 18:34:40 -0500 Subject: [PATCH 13/28] lint --- .../routes/WomtoolRouteSupport.scala | 11 ++- .../languages/util/ImportResolver.scala | 16 ++-- .../languages/util/ImportResolverSpec.scala | 73 +++++++++++++------ .../services/auth/GithubAuthVending.scala | 1 - .../auth/GithubAuthVendingSupport.scala | 23 ++++-- .../auth/impl/GithubAuthVendingActor.scala | 4 +- .../womtool/WomtoolServiceMessages.scala | 3 +- .../auth/GithubAuthVendingSupportSpec.scala | 10 +-- 8 files changed, 93 insertions(+), 48 deletions(-) diff --git a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala index 2cb4f8d0da3..b073c3dae8d 100644 --- a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala +++ b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala @@ -10,7 +10,12 @@ import akka.util.Timeout import cromwell.core.{WorkflowOptions, WorkflowSourceFilesCollection} import cromwell.languages.util.ImportResolver.ImportAuthProvider import cromwell.services.auth.GithubAuthVendingSupport -import cromwell.services.womtool.WomtoolServiceMessages.{DescribeFailure, DescribeRequest, DescribeResult, DescribeSuccess} +import cromwell.services.womtool.WomtoolServiceMessages.{ + DescribeFailure, + DescribeRequest, + DescribeResult, + DescribeSuccess +} import cromwell.webservice.WebServiceUtils import cromwell.webservice.WebServiceUtils.EnhancedThrowable @@ -46,7 +51,9 @@ trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport } } - private def validateAndSubmitRequest(data: MaterializedFormData, importAuthProviders: List[ImportAuthProvider]): Route = { + private def validateAndSubmitRequest(data: MaterializedFormData, + importAuthProviders: List[ImportAuthProvider] + ): Route = { // TODO: move constants to WebServiceUtils, adopt in PartialWorkflowSources val workflowSource = data.get("workflowSource").map(_.utf8String) val workflowUrl = data.get("workflowUrl").map(_.utf8String) diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index 6a96b4f1bbe..b66ece9e23e 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -189,8 +189,12 @@ object ImportResolver { } } - case class HttpResolver(relativeTo: Option[String], headers: Map[String, String], hostAllowlist: Option[List[String]], authProviders: List[ImportAuthProvider]) - extends ImportResolver with StrictLogging { + case class HttpResolver(relativeTo: Option[String], + headers: Map[String, String], + hostAllowlist: Option[List[String]], + authProviders: List[ImportAuthProvider] + ) extends ImportResolver + with StrictLogging { import HttpResolver._ override def name: String = { @@ -224,11 +228,10 @@ object ImportResolver { case None => true } - def fetchAuthHeaders(uri: Uri): Future[Map[String, String]] = { + def fetchAuthHeaders(uri: Uri): Future[Map[String, String]] = authProviders collectFirst { case provider if provider.validHosts.contains(uri.host) => provider.authHeader() } getOrElse Future.successful(Map.empty[String, String]) - } override def innerResolver(str: String, currentResolvers: List[ImportResolver]): Checked[ResolvedImportBundle] = pathToLookup(str) flatMap { toLookup: WorkflowUrl => @@ -287,7 +290,10 @@ object ImportResolver { import common.util.IntrospectableLazy import common.util.IntrospectableLazy._ - def apply(relativeTo: Option[String] = None, headers: Map[String, String] = Map.empty, authProviders: List[ImportAuthProvider] = List.empty): HttpResolver = { + def apply(relativeTo: Option[String] = None, + headers: Map[String, String] = Map.empty, + authProviders: List[ImportAuthProvider] = List.empty + ): HttpResolver = { val config = ConfigFactory.load().getConfig("languages.WDL.http-allow-list") val allowListEnabled = config.as[Option[Boolean]]("enabled").getOrElse(false) val allowList: Option[List[String]] = diff --git a/languageFactories/language-factory-core/src/test/scala/cromwell/languages/util/ImportResolverSpec.scala b/languageFactories/language-factory-core/src/test/scala/cromwell/languages/util/ImportResolverSpec.scala index e03069e6c18..a451d0c8df6 100644 --- a/languageFactories/language-factory-core/src/test/scala/cromwell/languages/util/ImportResolverSpec.scala +++ b/languageFactories/language-factory-core/src/test/scala/cromwell/languages/util/ImportResolverSpec.scala @@ -7,7 +7,12 @@ import common.assertion.CromwellTimeoutSpec import common.assertion.ErrorOrAssertions._ import cromwell.core.WorkflowId import cromwell.core.path.DefaultPath -import cromwell.languages.util.ImportResolver.{DirectoryResolver, GithubImportAuthProvider, HttpResolver, ImportAuthProvider} +import cromwell.languages.util.ImportResolver.{ + DirectoryResolver, + GithubImportAuthProvider, + HttpResolver, + ImportAuthProvider +} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers import wom.core.{WorkflowSource, WorkflowUrl} @@ -95,7 +100,8 @@ class ImportResolverSpec extends AnyFlatSpec with CromwellTimeoutSpec with Match behavior of "HttpResolver with a 'relativeTo' value" - val relativeHttpResolver = HttpResolver(relativeTo = Some("http://abc.com:8000/blah1/blah2/"), Map.empty, None, List.empty) + val relativeHttpResolver = + HttpResolver(relativeTo = Some("http://abc.com:8000/blah1/blah2/"), Map.empty, None, List.empty) val relativeToGithubHttpResolver = HttpResolver(relativeTo = Some(relativeToGithubRoot), Map.empty, None, List.empty) it should "resolve an absolute path from a different initial root" in { @@ -160,19 +166,29 @@ class ImportResolverSpec extends AnyFlatSpec with CromwellTimeoutSpec with Match behavior of "HttpResolver with an ImportAuthProvider" - class RecordingHttpResolver(unauthedResponse: Response[String], authedResponse: Response[String], authProvider: ImportAuthProvider) extends HttpResolver(None, Map.empty, None, List(authProvider)) { + class RecordingHttpResolver(unauthedResponse: Response[String], + authedResponse: Response[String], + authProvider: ImportAuthProvider + ) extends HttpResolver(None, Map.empty, None, List(authProvider)) { case class RequestRecord(url: WorkflowUrl, headers: Map[String, String], response: Response[String]) var requestRecords: List[RequestRecord] = List.empty - override protected def getUriInner(toLookup: WorkflowUrl, authHeaders: Map[String, String]): Response[WorkflowSource] = { - val result = if (Uri.parse(toLookup).get.host == "raw.githubusercontent.com" && authHeaders.get("Authorization").contains("Bearer 1234")) { - authedResponse - } else if (headers.contains("Authorization")) { - throw new RuntimeException(s"Unexpected auth header applied") - } else { - unauthedResponse - } + override protected def getUriInner(toLookup: WorkflowUrl, + authHeaders: Map[String, String] + ): Response[WorkflowSource] = { + val result = + if ( + Uri.parse(toLookup).get.host == "raw.githubusercontent.com" && authHeaders + .get("Authorization") + .contains("Bearer 1234") + ) { + authedResponse + } else if (headers.contains("Authorization")) { + throw new RuntimeException(s"Unexpected auth header applied") + } else { + unauthedResponse + } requestRecords = requestRecords :+ RequestRecord(toLookup, authHeaders, result) result @@ -180,18 +196,24 @@ class ImportResolverSpec extends AnyFlatSpec with CromwellTimeoutSpec with Match } it should "lookup headers from auth provider after a 404 for valid host" in { - val unauthedResponse = new Response[String](Left("Not found or no permissions".getBytes), StatusCodes.NotFound, "NotFound", Nil, List.empty) + val unauthedResponse = new Response[String](Left("Not found or no permissions".getBytes), + StatusCodes.NotFound, + "NotFound", + Nil, + List.empty + ) val authedResponse = new Response[String](Right("Hello World"), 200, "OK", Nil, List.empty) val authProvider = new GithubImportAuthProvider { override def authHeader() = Future.successful(Map("Authorization" -> "Bearer 1234")) } val resolver = new RecordingHttpResolver(unauthedResponse, authedResponse, authProvider) - val importUri = "https://raw.githubusercontent.com/broadinstitute/cromwell/develop/centaur/src/main/resources/standardTestCases/hello/hello.wdl" + val importUri = + "https://raw.githubusercontent.com/broadinstitute/cromwell/develop/centaur/src/main/resources/standardTestCases/hello/hello.wdl" val resolvedBundle = resolver.innerResolver(importUri, List(resolver)) resolvedBundle match { case Left(e) => fail(s"Expected ResolvedImportBundle but got $e") - case Right(bundle) => { + case Right(bundle) => bundle.resolvedImportRecord.importPath shouldBe importUri bundle.source shouldBe "Hello World" @@ -200,7 +222,6 @@ class ImportResolverSpec extends AnyFlatSpec with CromwellTimeoutSpec with Match resolver.requestRecords.head.headers shouldBe Map.empty resolver.requestRecords(1).url shouldBe importUri resolver.requestRecords(1).headers should be(Map(("Authorization", "Bearer 1234"))) - } } } @@ -211,43 +232,47 @@ class ImportResolverSpec extends AnyFlatSpec with CromwellTimeoutSpec with Match override def authHeader() = Future.failed(new Exception("Should not be called")) } val resolver = new RecordingHttpResolver(unauthedResponse, authedResponse, authProvider) - val importUri = "https://raw.githubusercontent.com/broadinstitute/cromwell/develop/centaur/src/main/resources/standardTestCases/hello/hello.wdl" + val importUri = + "https://raw.githubusercontent.com/broadinstitute/cromwell/develop/centaur/src/main/resources/standardTestCases/hello/hello.wdl" val resolvedBundle = resolver.innerResolver(importUri, List(resolver)) resolvedBundle match { case Left(e) => fail(s"Expected ResolvedImportBundle but got $e") - case Right(bundle) => { + case Right(bundle) => bundle.resolvedImportRecord.importPath shouldBe importUri bundle.source shouldBe "Hello World" resolver.requestRecords.size shouldBe 1 resolver.requestRecords.head.url shouldBe importUri resolver.requestRecords.head.headers shouldBe Map.empty - } } } it should "not lookup headers for urls which failed with other errors" in { - val unauthedResponse = new Response[String](Left("Some other error".getBytes), StatusCodes.ServiceUnavailable, "ServiceUnavailable", Nil, List.empty) + val unauthedResponse = new Response[String](Left("Some other error".getBytes), + StatusCodes.ServiceUnavailable, + "ServiceUnavailable", + Nil, + List.empty + ) val authedResponse = new Response[String](Left("Shouldn't be authed".getBytes), 500, "BAD", Nil, List.empty) val authProvider = new GithubImportAuthProvider { override def authHeader() = Future.failed(new Exception("Should not be called")) } val resolver = new RecordingHttpResolver(unauthedResponse, authedResponse, authProvider) - val importUri = "https://raw.githubusercontent.com/broadinstitute/cromwell/develop/centaur/src/main/resources/standardTestCases/hello/hello.wdl" + val importUri = + "https://raw.githubusercontent.com/broadinstitute/cromwell/develop/centaur/src/main/resources/standardTestCases/hello/hello.wdl" val resolvedBundle = resolver.innerResolver(importUri, List(resolver)) resolvedBundle match { - case Left(e) => { + case Left(e) => e.length should be(1) e.head.contains("Some other error") should be(true) resolver.requestRecords.size shouldBe 1 resolver.requestRecords.head.url shouldBe importUri resolver.requestRecords.head.headers shouldBe Map.empty - } - case Right(bundle) => { + case Right(bundle) => fail(s"Expected an error but got $bundle") - } } } diff --git a/services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala b/services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala index 345fbc33113..c74df8c947f 100644 --- a/services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala +++ b/services/src/main/scala/cromwell/services/auth/GithubAuthVending.scala @@ -14,5 +14,4 @@ object GithubAuthVending { case object NoGithubAuthResponse extends GithubAuthVendingResponse case class GithubAuthVendingFailure(error: Exception) extends GithubAuthVendingResponse - } diff --git a/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala b/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala index babb423402e..a8d406578ae 100644 --- a/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala +++ b/services/src/main/scala/cromwell/services/auth/GithubAuthVendingSupport.scala @@ -5,7 +5,13 @@ import akka.pattern.{AskSupport, AskTimeoutException} import akka.util.Timeout import com.typesafe.scalalogging.StrictLogging import cromwell.languages.util.ImportResolver.{GithubImportAuthProvider, ImportAuthProvider} -import cromwell.services.auth.GithubAuthVending.{GithubAuthRequest, GithubAuthTokenResponse, GithubAuthVendingFailure, GithubAuthVendingResponse, NoGithubAuthResponse} +import cromwell.services.auth.GithubAuthVending.{ + GithubAuthRequest, + GithubAuthTokenResponse, + GithubAuthVendingFailure, + GithubAuthVendingResponse, + NoGithubAuthResponse +} import scala.concurrent.{ExecutionContext, Future} @@ -17,22 +23,23 @@ trait GithubAuthVendingSupport extends AskSupport with StrictLogging { implicit val ec: ExecutionContext def importAuthProvider(token: String): ImportAuthProvider = new GithubImportAuthProvider { - override def authHeader(): Future[Map[String, String]] = { + override def authHeader(): Future[Map[String, String]] = serviceRegistryActor .ask(GithubAuthRequest(token)) .mapTo[GithubAuthVendingResponse] .recoverWith { - case e: AskTimeoutException => Future.failed(new Exception(s"Unable to resolve github auth token within allowed time", e)) + case e: AskTimeoutException => + Future.failed(new Exception(s"Unable to resolve github auth token within allowed time", e)) case e: Throwable => // This "should" never happen. If it does, let's make it obvious and trigger our alerting: logger.error("Programmer error: Unexpected failure to resolve github auth token", e) Future.failed(new Exception("Failed to resolve github auth token", e)) } .flatMap { - case GithubAuthTokenResponse(token) => Future.successful(Map("Authorization" -> s"Bearer ${token}")) - case NoGithubAuthResponse => Future.successful(Map.empty) - case GithubAuthVendingFailure(error) => Future.failed(new Exception("Failed to resolve github auth token", error)) - } - } + case GithubAuthTokenResponse(token) => Future.successful(Map("Authorization" -> s"Bearer ${token}")) + case NoGithubAuthResponse => Future.successful(Map.empty) + case GithubAuthVendingFailure(error) => + Future.failed(new Exception("Failed to resolve github auth token", error)) + } } } diff --git a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala index 8bf112754dd..2f226cadaf1 100644 --- a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala +++ b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala @@ -6,7 +6,9 @@ import com.typesafe.scalalogging.LazyLogging import cromwell.core.Dispatcher.ServiceDispatcher import cromwell.services.auth.GithubAuthVending.{GithubAuthRequest, GithubAuthTokenResponse, NoGithubAuthResponse} -class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, serviceRegistryActor: ActorRef) extends Actor with LazyLogging { +class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, serviceRegistryActor: ActorRef) + extends Actor + with LazyLogging { lazy val enabled = serviceConfig.getBoolean("enabled") diff --git a/services/src/main/scala/cromwell/services/womtool/WomtoolServiceMessages.scala b/services/src/main/scala/cromwell/services/womtool/WomtoolServiceMessages.scala index 7ac0ec13d4a..d4743cbfe72 100644 --- a/services/src/main/scala/cromwell/services/womtool/WomtoolServiceMessages.scala +++ b/services/src/main/scala/cromwell/services/womtool/WomtoolServiceMessages.scala @@ -13,7 +13,8 @@ object WomtoolServiceMessages { override def serviceName: String = WomtoolServiceName } - case class DescribeRequest(filesCollection: WorkflowSourceFilesCollection, authProviders: List[ImportAuthProvider]) extends WomtoolServiceMessage + case class DescribeRequest(filesCollection: WorkflowSourceFilesCollection, authProviders: List[ImportAuthProvider]) + extends WomtoolServiceMessage sealed trait DescribeResult extends WomtoolServiceMessage case class DescribeSuccess(description: WorkflowDescription) extends DescribeResult diff --git a/services/src/test/scala/cromwell/services/auth/GithubAuthVendingSupportSpec.scala b/services/src/test/scala/cromwell/services/auth/GithubAuthVendingSupportSpec.scala index 477fbbf5f72..f9bc3e127f4 100644 --- a/services/src/test/scala/cromwell/services/auth/GithubAuthVendingSupportSpec.scala +++ b/services/src/test/scala/cromwell/services/auth/GithubAuthVendingSupportSpec.scala @@ -14,10 +14,7 @@ import org.scalatest.matchers.should.Matchers import scala.concurrent.{Await, ExecutionContext, Future} import scala.concurrent.duration._ -class GithubAuthVendingSupportSpec extends TestKitSuite - with AnyFlatSpecLike - with Matchers - with Eventually { +class GithubAuthVendingSupportSpec extends TestKitSuite with AnyFlatSpecLike with Matchers with Eventually { behavior of "GithubAuthVendingSupport" @@ -94,8 +91,9 @@ class GithubAuthVendingSupportSpec extends TestKitSuite } object GithubAuthVendingSupportSpec { - class TestGithubAuthVendingSupport(val serviceRegistryActor: ActorRef, val timeout: Timeout = 10.seconds) extends GithubAuthVendingSupport { - override implicit val ec: ExecutionContext = ExecutionContext.global + class TestGithubAuthVendingSupport(val serviceRegistryActor: ActorRef, val timeout: Timeout = 10.seconds) + extends GithubAuthVendingSupport { + implicit override val ec: ExecutionContext = ExecutionContext.global } } From 46ea66dcb3ac3c3a47978d6594fad9e3364bfadd Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Fri, 16 Feb 2024 10:37:02 -0500 Subject: [PATCH 14/28] Undo pointless name change to http resolvers --- .../scala/cromwell/languages/util/ImportResolver.scala | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index b66ece9e23e..666442b34eb 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -197,13 +197,9 @@ object ImportResolver { with StrictLogging { import HttpResolver._ - override def name: String = { - val relativeToSuffix = relativeTo match { - case Some(relativeToPath) => s"(relative to $relativeToPath)" - case None => "(no 'relative-to' origin)" - } - - s"HTTP resolver $relativeToSuffix" + override def name: String = relativeTo match { + case Some(relativeToPath) => s"http importer (relative to $relativeToPath)" + case None => "http importer (no 'relative-to' origin)" } def newResolverList(newRoot: String): List[ImportResolver] = { From af3effd6441f6f156f695e1c926c349c394c37b1 Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Fri, 16 Feb 2024 13:37:51 -0500 Subject: [PATCH 15/28] Remove duplication of status code --- .../src/main/scala/cromwell/languages/util/ImportResolver.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index 666442b34eb..4a83ab6e63e 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -263,7 +263,7 @@ object ImportResolver { } result.body.leftMap { e => - NonEmptyList.of(s"${result.code}: ${e.trim}") + NonEmptyList.of(e.trim) } map { ResolvedImportBundle(_, newResolverList(toLookup), ResolvedImportRecord(toLookup)) } From 7e7fd5955b644e1c16894cfc6128bee2050c2a71 Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Tue, 20 Feb 2024 15:40:17 -0500 Subject: [PATCH 16/28] add access-token back to config --- core/src/main/resources/reference.conf | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/src/main/resources/reference.conf b/core/src/main/resources/reference.conf index 54e56d4f33b..29723a07ded 100644 --- a/core/src/main/resources/reference.conf +++ b/core/src/main/resources/reference.conf @@ -575,6 +575,10 @@ services { class = "cromwell.services.auth.impl.GithubAuthVendingActor" config { enabled = false + # Notes: + # - don't include the 'Bearer' before the token + # - this config value should be removed when support for fetching tokens from ECM has been added to Cromwell + access-token = "dummy-token" } } } From 49f83260882dfd4a2dd4b96c459f0cc6bde870c6 Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Tue, 20 Feb 2024 17:03:21 -0500 Subject: [PATCH 17/28] PR feedback --- .../scala/cromwell/languages/util/ImportResolver.scala | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index 4a83ab6e63e..ee04b9e7ebf 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -248,11 +248,11 @@ object ImportResolver { private def getUri(toLookup: WorkflowUrl): Checked[ResolvedImportBundle] = { // Temporary situation to get functionality working before // starting in on async-ifying the entire WdlNamespace flow - // Note: this will cause the calling thread to block for up to 30 seconds - // (15 for the auth header lookup, 15 for the http request) + // Note: this will cause the calling thread to block for up to 20 seconds + // (5 for the auth header lookup, 15 for the http request) val unauthedAttempt = getUriInner(toLookup, Map.empty) - val result = if (StatusCodes.NotFound == unauthedAttempt.code) { - val authHeaders = Await.result(fetchAuthHeaders(uri"$toLookup"), 15.seconds) + val result = if (unauthedAttempt.code == StatusCodes.NotFound) { + val authHeaders = Await.result(fetchAuthHeaders(uri"$toLookup"), 5.seconds) if (authHeaders.nonEmpty) { getUriInner(toLookup, authHeaders) } else { From 0de17e785cb8d153a1a5ba5601b4d538ddfd0cb5 Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Wed, 21 Feb 2024 14:55:59 -0500 Subject: [PATCH 18/28] reduce maxInitializationAttempts to 3 --- .../src/main/scala/cromwell/engine/workflow/WorkflowActor.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/src/main/scala/cromwell/engine/workflow/WorkflowActor.scala b/engine/src/main/scala/cromwell/engine/workflow/WorkflowActor.scala index 8bf936fa88b..52d663f30d1 100644 --- a/engine/src/main/scala/cromwell/engine/workflow/WorkflowActor.scala +++ b/engine/src/main/scala/cromwell/engine/workflow/WorkflowActor.scala @@ -304,7 +304,7 @@ class WorkflowActor(workflowToStart: WorkflowToStart, protected val pathBuilderFactories: List[PathBuilderFactory] = EngineFilesystems.configuredPathBuilderFactories - protected val maxInitializationAttempts: Int = 16 + protected val maxInitializationAttempts: Int = 3 protected val initializationRetryInterval: FiniteDuration = 1.minute startWith(WorkflowUnstartedState, WorkflowActorData(initialStartableState)) From debfc8f9e02f09aabbb8453223a65226d1dba63d Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Wed, 21 Feb 2024 17:19:32 -0500 Subject: [PATCH 19/28] revert maxInitializationAttempts commit; try updating NonEmptyList --- .../src/main/scala/cromwell/engine/workflow/WorkflowActor.scala | 2 +- .../src/main/scala/cromwell/languages/util/ImportResolver.scala | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/engine/src/main/scala/cromwell/engine/workflow/WorkflowActor.scala b/engine/src/main/scala/cromwell/engine/workflow/WorkflowActor.scala index 52d663f30d1..8bf936fa88b 100644 --- a/engine/src/main/scala/cromwell/engine/workflow/WorkflowActor.scala +++ b/engine/src/main/scala/cromwell/engine/workflow/WorkflowActor.scala @@ -304,7 +304,7 @@ class WorkflowActor(workflowToStart: WorkflowToStart, protected val pathBuilderFactories: List[PathBuilderFactory] = EngineFilesystems.configuredPathBuilderFactories - protected val maxInitializationAttempts: Int = 3 + protected val maxInitializationAttempts: Int = 16 protected val initializationRetryInterval: FiniteDuration = 1.minute startWith(WorkflowUnstartedState, WorkflowActorData(initialStartableState)) diff --git a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala index ee04b9e7ebf..bdcd0c8b311 100644 --- a/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala +++ b/languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala @@ -263,7 +263,7 @@ object ImportResolver { } result.body.leftMap { e => - NonEmptyList.of(e.trim) + NonEmptyList(e.trim, List.empty) } map { ResolvedImportBundle(_, newResolverList(toLookup), ResolvedImportRecord(toLookup)) } From ae2c89c9402bf5066c0a7b5351dfa19c07c10e2e Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Thu, 22 Feb 2024 16:45:46 -0500 Subject: [PATCH 20/28] move restart_abort_jes test to restart suite --- .../resources/standardTestCases/abort.restart_abort_jes.test | 1 + 1 file changed, 1 insertion(+) diff --git a/centaur/src/main/resources/standardTestCases/abort.restart_abort_jes.test b/centaur/src/main/resources/standardTestCases/abort.restart_abort_jes.test index 80192065e6c..b064ba813d5 100644 --- a/centaur/src/main/resources/standardTestCases/abort.restart_abort_jes.test +++ b/centaur/src/main/resources/standardTestCases/abort.restart_abort_jes.test @@ -3,6 +3,7 @@ name: abort.restart_abort_jes testFormat: ScheduledAbortWithRestart callMark: scheduled_abort.aborted backends: [Papi] +tags: [restart] files { workflow: abort/scheduled_abort.wdl From 656975fcb6ada68f5ffa9aa0481191c60dafdfe7 Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Thu, 22 Feb 2024 17:14:50 -0500 Subject: [PATCH 21/28] add ShutDown command --- .../cromwell/services/auth/impl/GithubAuthVendingActor.scala | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala index 2f226cadaf1..2e1c68dded7 100644 --- a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala +++ b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala @@ -5,6 +5,7 @@ import com.typesafe.config.Config import com.typesafe.scalalogging.LazyLogging import cromwell.core.Dispatcher.ServiceDispatcher import cromwell.services.auth.GithubAuthVending.{GithubAuthRequest, GithubAuthTokenResponse, NoGithubAuthResponse} +import cromwell.util.GracefulShutdownHelper.ShutdownCommand class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, serviceRegistryActor: ActorRef) extends Actor @@ -15,6 +16,7 @@ class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, servic override def receive: Receive = { case GithubAuthRequest(_) if enabled => sender() ! GithubAuthTokenResponse(serviceConfig.getString("access-token")) + case ShutdownCommand => context stop self case _ => sender() ! NoGithubAuthResponse } From 109323d1bc86ddf989310070581945efce42974a Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Thu, 22 Feb 2024 17:36:00 -0500 Subject: [PATCH 22/28] increase stop-io-activity for tests to 60 mins --- centaur/src/main/resources/application.conf | 3 +++ 1 file changed, 3 insertions(+) diff --git a/centaur/src/main/resources/application.conf b/centaur/src/main/resources/application.conf index 0f81cc759fb..249bb40f915 100644 --- a/centaur/src/main/resources/application.conf +++ b/centaur/src/main/resources/application.conf @@ -2,3 +2,6 @@ akka.http.host-connection-pool.max-open-requests: 1024 akka.http.host-connection-pool.max-connections: 20 akka.http.host-connection-pool.min-connections: 10 +# This is set to 60 minutes so that if there is a "AskTimeoutException" during a coordinated shutdown, the tests +# fail instead of hiding it in logs because the GHA didn't run into its 2 hour limit +akka.coordinated-shutdown.phases.stop-io-activity.timeout: 60 minutes From d6ce18dedffb7fc1d3d1f46442b5073633dddd16 Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Thu, 22 Feb 2024 17:37:25 -0500 Subject: [PATCH 23/28] experiement - comment out ShutDownCommand and see if other GHA jobs fail --- .../cromwell/services/auth/impl/GithubAuthVendingActor.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala index 2e1c68dded7..6ea68c1ca02 100644 --- a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala +++ b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala @@ -16,7 +16,7 @@ class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, servic override def receive: Receive = { case GithubAuthRequest(_) if enabled => sender() ! GithubAuthTokenResponse(serviceConfig.getString("access-token")) - case ShutdownCommand => context stop self +// case ShutdownCommand => context stop self case _ => sender() ! NoGithubAuthResponse } From 09b52493c307222466f4193baf5bfde4e0b88d0b Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Thu, 22 Feb 2024 17:44:34 -0500 Subject: [PATCH 24/28] experiment take 2 - should compile before commiting --- .../cromwell/services/auth/impl/GithubAuthVendingActor.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala index 6ea68c1ca02..79e3a2e05fd 100644 --- a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala +++ b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala @@ -5,7 +5,6 @@ import com.typesafe.config.Config import com.typesafe.scalalogging.LazyLogging import cromwell.core.Dispatcher.ServiceDispatcher import cromwell.services.auth.GithubAuthVending.{GithubAuthRequest, GithubAuthTokenResponse, NoGithubAuthResponse} -import cromwell.util.GracefulShutdownHelper.ShutdownCommand class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, serviceRegistryActor: ActorRef) extends Actor From 008b396bf551f4c576cf134241e4bdbd050db5ff Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Fri, 23 Feb 2024 10:46:45 -0500 Subject: [PATCH 25/28] experiment - move stop-io-activity to centaur's reference.conf --- centaur/src/main/resources/application.conf | 3 --- centaur/src/main/resources/reference.conf | 4 ++++ 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/centaur/src/main/resources/application.conf b/centaur/src/main/resources/application.conf index 249bb40f915..0f81cc759fb 100644 --- a/centaur/src/main/resources/application.conf +++ b/centaur/src/main/resources/application.conf @@ -2,6 +2,3 @@ akka.http.host-connection-pool.max-open-requests: 1024 akka.http.host-connection-pool.max-connections: 20 akka.http.host-connection-pool.min-connections: 10 -# This is set to 60 minutes so that if there is a "AskTimeoutException" during a coordinated shutdown, the tests -# fail instead of hiding it in logs because the GHA didn't run into its 2 hour limit -akka.coordinated-shutdown.phases.stop-io-activity.timeout: 60 minutes diff --git a/centaur/src/main/resources/reference.conf b/centaur/src/main/resources/reference.conf index d570b5beef3..3bdb44156e2 100644 --- a/centaur/src/main/resources/reference.conf +++ b/centaur/src/main/resources/reference.conf @@ -93,4 +93,8 @@ centaur { } log-request-failures = false + + # This is set to 60 minutes so that if there is a "AskTimeoutException" during a coordinated shutdown, the tests + # fail instead of hiding it in logs because the GHA didn't run into its 2 hour limit + akka.coordinated-shutdown.phases.stop-io-activity.timeout = 60 minutes } From a27bd41cfc5c0a2f07ed7bed4ad33232ec47daab Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Fri, 23 Feb 2024 13:37:19 -0500 Subject: [PATCH 26/28] experiment - move stop-io-activity to build_application.inc.conf --- centaur/src/main/resources/reference.conf | 4 ---- src/ci/resources/build_application.inc.conf | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/centaur/src/main/resources/reference.conf b/centaur/src/main/resources/reference.conf index 3bdb44156e2..d570b5beef3 100644 --- a/centaur/src/main/resources/reference.conf +++ b/centaur/src/main/resources/reference.conf @@ -93,8 +93,4 @@ centaur { } log-request-failures = false - - # This is set to 60 minutes so that if there is a "AskTimeoutException" during a coordinated shutdown, the tests - # fail instead of hiding it in logs because the GHA didn't run into its 2 hour limit - akka.coordinated-shutdown.phases.stop-io-activity.timeout = 60 minutes } diff --git a/src/ci/resources/build_application.inc.conf b/src/ci/resources/build_application.inc.conf index 1a8a28857ae..61919fc5670 100644 --- a/src/ci/resources/build_application.inc.conf +++ b/src/ci/resources/build_application.inc.conf @@ -59,4 +59,8 @@ services.MetadataService.config.metadata-write-statistics { sub-workflow-bundling = true } +# This is set to 60 minutes so that if there is a "AskTimeoutException" during a coordinated shutdown, the tests +# fail instead of hiding it in logs because the GHA didn't run into its 2 hour limit +akka.coordinated-shutdown.phases.stop-io-activity.timeout = 60 minutes + include "cromwell_database.inc.conf" From e4a7fe408d2ca9b75e39e5a8ce92b5dce956a82f Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Fri, 23 Feb 2024 15:58:06 -0500 Subject: [PATCH 27/28] put ShutDown command handling back; update comments --- .../resources/standardTestCases/abort.restart_abort_jes.test | 1 - core/src/main/resources/reference.conf | 3 +++ .../cromwell/services/auth/impl/GithubAuthVendingActor.scala | 5 ++++- src/ci/resources/build_application.inc.conf | 4 ++-- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/centaur/src/main/resources/standardTestCases/abort.restart_abort_jes.test b/centaur/src/main/resources/standardTestCases/abort.restart_abort_jes.test index b064ba813d5..80192065e6c 100644 --- a/centaur/src/main/resources/standardTestCases/abort.restart_abort_jes.test +++ b/centaur/src/main/resources/standardTestCases/abort.restart_abort_jes.test @@ -3,7 +3,6 @@ name: abort.restart_abort_jes testFormat: ScheduledAbortWithRestart callMark: scheduled_abort.aborted backends: [Papi] -tags: [restart] files { workflow: abort/scheduled_abort.wdl diff --git a/core/src/main/resources/reference.conf b/core/src/main/resources/reference.conf index 29723a07ded..005ff5e692e 100644 --- a/core/src/main/resources/reference.conf +++ b/core/src/main/resources/reference.conf @@ -524,6 +524,9 @@ backend { } } +# Note: When adding a new actor that uses service registry pattern make sure that the new actor handles shutdown +# (i.e 'ShutdownCommand') even if the actor doesn't need to be graceful about shutting itself downing. +# See https://github.com/broadinstitute/cromwell/issues/2575 services { KeyValue { class = "cromwell.services.keyvalue.impl.SqlKeyValueServiceActor" diff --git a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala index 79e3a2e05fd..e0f4c9756a0 100644 --- a/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala +++ b/services/src/main/scala/cromwell/services/auth/impl/GithubAuthVendingActor.scala @@ -5,6 +5,7 @@ import com.typesafe.config.Config import com.typesafe.scalalogging.LazyLogging import cromwell.core.Dispatcher.ServiceDispatcher import cromwell.services.auth.GithubAuthVending.{GithubAuthRequest, GithubAuthTokenResponse, NoGithubAuthResponse} +import cromwell.util.GracefulShutdownHelper.ShutdownCommand class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, serviceRegistryActor: ActorRef) extends Actor @@ -15,7 +16,9 @@ class GithubAuthVendingActor(serviceConfig: Config, globalConfig: Config, servic override def receive: Receive = { case GithubAuthRequest(_) if enabled => sender() ! GithubAuthTokenResponse(serviceConfig.getString("access-token")) -// case ShutdownCommand => context stop self + // This service currently doesn't do any work on shutdown but the service registry pattern requires it + // (see https://github.com/broadinstitute/cromwell/issues/2575) + case ShutdownCommand => context stop self case _ => sender() ! NoGithubAuthResponse } diff --git a/src/ci/resources/build_application.inc.conf b/src/ci/resources/build_application.inc.conf index 61919fc5670..260e0463a14 100644 --- a/src/ci/resources/build_application.inc.conf +++ b/src/ci/resources/build_application.inc.conf @@ -59,8 +59,8 @@ services.MetadataService.config.metadata-write-statistics { sub-workflow-bundling = true } -# This is set to 60 minutes so that if there is a "AskTimeoutException" during a coordinated shutdown, the tests -# fail instead of hiding it in logs because the GHA didn't run into its 2 hour limit +# Make the shutdown timeout conspicuously long, so that if it hangs, it is an obvious problem in CI. +# See https://github.com/broadinstitute/cromwell/issues/2575 akka.coordinated-shutdown.phases.stop-io-activity.timeout = 60 minutes include "cromwell_database.inc.conf" From 7aad25234ed811108805b92ec56fbabe850ff4e0 Mon Sep 17 00:00:00 2001 From: Saloni Shah Date: Fri, 23 Feb 2024 16:03:44 -0500 Subject: [PATCH 28/28] better wording --- core/src/main/resources/reference.conf | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/core/src/main/resources/reference.conf b/core/src/main/resources/reference.conf index 005ff5e692e..c43dbce60ee 100644 --- a/core/src/main/resources/reference.conf +++ b/core/src/main/resources/reference.conf @@ -524,9 +524,8 @@ backend { } } -# Note: When adding a new actor that uses service registry pattern make sure that the new actor handles shutdown -# (i.e 'ShutdownCommand') even if the actor doesn't need to be graceful about shutting itself downing. -# See https://github.com/broadinstitute/cromwell/issues/2575 +# Note: When adding a new actor that uses service registry pattern make sure that the new actor handles the graceful +# shutdown command ('ShutdownCommand'). See https://github.com/broadinstitute/cromwell/issues/2575 services { KeyValue { class = "cromwell.services.keyvalue.impl.SqlKeyValueServiceActor"