Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WM-2454: Private GitHub support on describe api #7365

Merged
merged 34 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
4ec82f2
Wiring for github token lookups [WIP]
cjllanwarne Feb 12, 2024
953fb13
Compiling [WIP]
cjllanwarne Feb 12, 2024
5d88d0b
Working in local testing
cjllanwarne Feb 13, 2024
19c1097
Remove unwanted logging
cjllanwarne Feb 13, 2024
a1fd14d
Unused import
cjllanwarne Feb 13, 2024
9cbf10a
Make vending optional. Extract interface out of impl
cjllanwarne Feb 14, 2024
3483604
Undo name change which breaks tests
cjllanwarne Feb 14, 2024
6bbea51
Fix compile issues
cjllanwarne Feb 14, 2024
19ae950
Tidy the await.result, albeit without fixing the world :(
cjllanwarne Feb 15, 2024
19205a4
Retry on 404
cjllanwarne Feb 15, 2024
645da1d
Add tests for new ImportResolver behavior
cjllanwarne Feb 15, 2024
4964e9e
Add tests for new GithubAuthVendingSupport
cjllanwarne Feb 15, 2024
12b2f0e
lint
cjllanwarne Feb 15, 2024
46ea66d
Undo pointless name change to http resolvers
cjllanwarne Feb 16, 2024
af3effd
Remove duplication of status code
cjllanwarne Feb 16, 2024
6bfffcc
Merge branch 'develop' into cjl_private_github_support_api
salonishah11 Feb 16, 2024
c627c1c
Merge branch 'develop' into cjl_private_github_support_api
jgainerdewar Feb 20, 2024
7e7fd59
add access-token back to config
salonishah11 Feb 20, 2024
1c6440b
Merge branch 'develop' into cjl_private_github_support_api
salonishah11 Feb 20, 2024
49f8326
PR feedback
salonishah11 Feb 20, 2024
0de17e7
reduce maxInitializationAttempts to 3
salonishah11 Feb 21, 2024
debfc8f
revert maxInitializationAttempts commit; try updating NonEmptyList
salonishah11 Feb 21, 2024
ae2c89c
move restart_abort_jes test to restart suite
salonishah11 Feb 22, 2024
656975f
add ShutDown command
salonishah11 Feb 22, 2024
109323d
increase stop-io-activity for tests to 60 mins
salonishah11 Feb 22, 2024
d6ce18d
experiement - comment out ShutDownCommand and see if other GHA jobs fail
salonishah11 Feb 22, 2024
09b5249
experiment take 2 - should compile before commiting
salonishah11 Feb 22, 2024
1f0097e
Merge branch 'develop' into cjl_private_github_support_api
aednichols Feb 23, 2024
008b396
experiment - move stop-io-activity to centaur's reference.conf
salonishah11 Feb 23, 2024
a66feb7
Merge branch 'cjl_private_github_support_api' of https://github.com/b…
salonishah11 Feb 23, 2024
a27bd41
experiment - move stop-io-activity to build_application.inc.conf
salonishah11 Feb 23, 2024
e4a7fe4
put ShutDown command handling back; update comments
salonishah11 Feb 23, 2024
7aad252
better wording
salonishah11 Feb 23, 2024
832f07d
Merge branch 'develop' into cjl_private_github_support_api
salonishah11 Feb 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion core/src/main/resources/reference.conf
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ docker {
throttle {
number-of-requests = 1000
per = 60 seconds
}
}
num-threads = 10
}
google {
Expand Down Expand Up @@ -571,6 +571,16 @@ services {
# Default - run within the Cromwell JVM
class = "cromwell.services.womtool.impl.WomtoolServiceInCromwellActor"
}
GithubAuthVending {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick - it seems like kebab-case is more standard in this file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgainerdewar it seems the configs in this section are already in kebab-case. Am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the GithubAuthVending :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saloni pointed out that the others services configured here also use camelcase, so my case inconsistency grumpiness is out of scope for this PR.

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"
}
}
}

include required(classpath("reference_database.inc.conf"))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@

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.auth.GithubAuthVendingSupport
import cromwell.services.womtool.WomtoolServiceMessages.{
DescribeFailure,
DescribeRequest,
Expand All @@ -20,7 +22,7 @@
import scala.concurrent.ExecutionContext
import scala.util.{Failure, Success}

trait WomtoolRouteSupport extends WebServiceUtils {
trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport {

implicit def actorRefFactory: ActorRefFactory
implicit val ec: ExecutionContext
Expand All @@ -33,17 +35,25 @@
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))

Check warning on line 40 in engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala

View check run for this annotation

Codecov / codecov/patch

engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala#L40

Added line #L40 was not covered by tests
case _ => List.empty
}
onComplete(materializeFormData(formData)(timeout, materializer, ec)) {
case Success(data) =>
validateAndSubmitRequest(data, authProviders)
case Failure(e) =>
e.failRequest(StatusCodes.InternalServerError)

Check warning on line 47 in engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala

View check run for this annotation

Codecov / codecov/patch

engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala#L47

Added line #L47 was not covered by tests
}
}
}
}
}

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)
Expand All @@ -66,7 +76,7 @@
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._
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
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._
Expand All @@ -22,11 +23,11 @@
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 {
Expand All @@ -37,6 +38,15 @@
resolvedImportRecord: ResolvedImportRecord
)

trait ImportAuthProvider {
def validHosts: List[String]
def authHeader(): Future[Map[String, String]]
}

trait GithubImportAuthProvider extends ImportAuthProvider {
override def validHosts: List[String] = List("github.com", "githubusercontent.com", "raw.githubusercontent.com")

Check warning on line 47 in languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala

View check run for this annotation

Codecov / codecov/patch

languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala#L47

Added line #L47 was not covered by tests
}

trait ImportResolver {
def name: String
protected def innerResolver(path: String, currentResolvers: List[ImportResolver]): Checked[ResolvedImportBundle]
Expand Down Expand Up @@ -179,20 +189,23 @@
}
}

case class HttpResolver(relativeTo: Option[String], headers: Map[String, String], hostAllowlist: Option[List[String]])
extends ImportResolver {
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 = 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] = {
val rootWithoutFilename = newRoot.split('/').init.mkString("", "/", "/")
List(
HttpResolver(relativeTo = Some(canonicalize(rootWithoutFilename)), headers, hostAllowlist)
HttpResolver(relativeTo = Some(canonicalize(rootWithoutFilename)), headers, hostAllowlist, authProviders)
)
}

Expand All @@ -211,8 +224,13 @@
case None => true
}

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])

Check warning on line 230 in languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala

View check run for this annotation

Codecov / codecov/patch

languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala#L228-L230

Added lines #L228 - L230 were not covered by tests

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"

Expand All @@ -227,21 +245,37 @@
}).contextualizeErrors(s"download $toLookup")
}

private def getUri(toLookup: WorkflowSource): Either[NonEmptyList[WorkflowSource], ResolvedImportBundle] = {
implicit val sttpBackend = HttpResolver.sttpBackend()
val responseIO: IO[Response[WorkflowSource]] = sttp.get(uri"$toLookup").headers(headers).send()

// temporary situation to get functionality working before
private def getUri(toLookup: WorkflowUrl): Checked[ResolvedImportBundle] = {
// Temporary situation to get functionality working before
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad we are refreshing the last-modified date of the "temporary situation" 🙂

// starting in on async-ifying the entire WdlNamespace flow
val result: Checked[WorkflowSource] = Await.result(responseIO.unsafeToFuture(), 15.seconds).body.leftMap { e =>
NonEmptyList(e.toString.trim, List.empty)
// 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 (unauthedAttempt.code == StatusCodes.NotFound) {
val authHeaders = Await.result(fetchAuthHeaders(uri"$toLookup"), 5.seconds)
if (authHeaders.nonEmpty) {
getUriInner(toLookup, authHeaders)

Check warning on line 257 in languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala

View check run for this annotation

Codecov / codecov/patch

languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala#L255-L257

Added lines #L255 - L257 were not covered by tests
} else {
unauthedAttempt

Check warning on line 259 in languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala

View check run for this annotation

Codecov / codecov/patch

languageFactories/language-factory-core/src/main/scala/cromwell/languages/util/ImportResolver.scala#L259

Added line #L259 was not covered by tests
}
} else {
unauthedAttempt
}

result map {
result.body.leftMap { e =>
NonEmptyList.of(e.trim)
} map {
ResolvedImportBundle(_, newResolverList(toLookup), ResolvedImportRecord(toLookup))
}
}

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()
Await.result(responseIO.unsafeToFuture(), 15.seconds)
}

override def cleanupIfNecessary(): ErrorOr[Unit] = ().validNel

override def hashKey: ErrorOr[String] = relativeTo.toString.md5Sum.validNel
Expand All @@ -252,15 +286,18 @@
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]] =
if (allowListEnabled)
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 {
Expand Down
Loading
Loading