From d4a57a80dc7e8e153cdb38eca72c1c2683e3211d Mon Sep 17 00:00:00 2001
From: Janet Gainer-Dewar <jdewar@broadinstitute.org>
Date: Mon, 26 Sep 2022 14:05:09 -0400
Subject: [PATCH 1/6] Give blob SAS tokens write permission
---
.../scala/cromwell/filesystems/blob/BlobFileSystemManager.scala | 1 +
1 file changed, 1 insertion(+)
diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala
index 3b8f5149055..7ed79b8edce 100644
--- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala
+++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala
@@ -123,6 +123,7 @@ case class NativeBlobTokenGenerator(container: BlobContainerName, endpoint: Endp
.setReadPermission(true)
.setCreatePermission(true)
.setListPermission(true)
+ .setWritePermission(true)
def generateAccessToken: Try[AzureSasCredential] = for {
From f41ff6bb13f6bc8b23865e9169db34f57d3b766d Mon Sep 17 00:00:00 2001
From: Janet Gainer-Dewar <jdewar@broadinstitute.org>
Date: Mon, 26 Sep 2022 14:36:35 -0400
Subject: [PATCH 2/6] Case class wrapper for subscription id
---
.../filesystems/blob/BlobFileSystemManager.scala | 13 +++++++++----
.../filesystems/blob/BlobPathBuilderFactory.scala | 7 ++++---
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala
index 7ed79b8edce..a3e6f33572a 100644
--- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala
+++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala
@@ -77,10 +77,15 @@ case class BlobFileSystemManager(
sealed trait BlobTokenGenerator {def generateAccessToken: Try[AzureSasCredential]}
object BlobTokenGenerator {
- def createBlobTokenGenerator(container: BlobContainerName, endpoint: EndpointURL, subscription: Option[String]): BlobTokenGenerator = {
+ def createBlobTokenGenerator(container: BlobContainerName, endpoint: EndpointURL, subscription: Option[SubscriptionId]): BlobTokenGenerator = {
createBlobTokenGenerator(container, endpoint, None, None, subscription)
}
- def createBlobTokenGenerator(container: BlobContainerName, endpoint: EndpointURL, workspaceId: Option[WorkspaceId], workspaceManagerURL: Option[WorkspaceManagerURL], subscription: Option[String]): BlobTokenGenerator = {
+ def createBlobTokenGenerator(container: BlobContainerName,
+ endpoint: EndpointURL,
+ workspaceId: Option[WorkspaceId],
+ workspaceManagerURL: Option[WorkspaceManagerURL],
+ subscription: Option[SubscriptionId]
+ ): BlobTokenGenerator = {
(container: BlobContainerName, endpoint: EndpointURL, workspaceId, workspaceManagerURL) match {
case (container, endpoint, None, None) =>
NativeBlobTokenGenerator(container, endpoint, subscription)
@@ -100,13 +105,13 @@ case class WSMBlobTokenGenerator(container: BlobContainerName, endpoint: Endpoin
def generateAccessToken: Try[AzureSasCredential] = Failure(new NotImplementedError)
}
-case class NativeBlobTokenGenerator(container: BlobContainerName, endpoint: EndpointURL, subscription: Option[String] = None) extends BlobTokenGenerator {
+case class NativeBlobTokenGenerator(container: BlobContainerName, endpoint: EndpointURL, subscription: Option[SubscriptionId] = None) extends BlobTokenGenerator {
private val azureProfile = new AzureProfile(AzureEnvironment.AZURE)
private def azureCredentialBuilder = new DefaultAzureCredentialBuilder()
.authorityHost(azureProfile.getEnvironment.getActiveDirectoryEndpoint)
.build
- private def authenticateWithSubscription(sub: String) = AzureResourceManager.authenticate(azureCredentialBuilder, azureProfile).withSubscription(sub)
+ private def authenticateWithSubscription(sub: SubscriptionId) = AzureResourceManager.authenticate(azureCredentialBuilder, azureProfile).withSubscription(sub.value)
private def authenticateWithDefaultSubscription = AzureResourceManager.authenticate(azureCredentialBuilder, azureProfile).withDefaultSubscription()
private def azure = subscription.map(authenticateWithSubscription(_)).getOrElse(authenticateWithDefaultSubscription)
diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala
index c93f751b706..e6d269a07b0 100644
--- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala
+++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala
@@ -10,18 +10,19 @@ import scala.concurrent.{ExecutionContext, Future}
final case class BlobFileSystemConfig(config: Config)
+final case class SubscriptionId(value: String) {override def toString: String = value}
final case class BlobContainerName(value: String) {override def toString: String = value}
final case class StorageAccountName(value: String) {override def toString: String = value}
final case class EndpointURL(value: String) {override def toString: String = value}
final case class WorkspaceId(value: String) {override def toString: String = value}
final case class WorkspaceManagerURL(value: String) {override def toString: String = value}
final case class BlobPathBuilderFactory(globalConfig: Config, instanceConfig: Config, singletonConfig: BlobFileSystemConfig) extends PathBuilderFactory {
- val subscription: Option[String] = instanceConfig.as[Option[String]]("subscription")
+ val subscription: Option[SubscriptionId] = instanceConfig.as[Option[String]]("subscription").map(SubscriptionId)
val container: BlobContainerName = BlobContainerName(instanceConfig.as[String]("container"))
val endpoint: EndpointURL = EndpointURL(instanceConfig.as[String]("endpoint"))
- val workspaceId: Option[WorkspaceId] = instanceConfig.as[Option[String]]("workspace-id").map(WorkspaceId(_))
+ val workspaceId: Option[WorkspaceId] = instanceConfig.as[Option[String]]("workspace-id").map(WorkspaceId)
val expiryBufferMinutes: Long = instanceConfig.as[Option[Long]]("expiry-buffer-minutes").getOrElse(10)
- val workspaceManagerURL: Option[WorkspaceManagerURL] = singletonConfig.config.as[Option[String]]("workspace-manager-url").map(WorkspaceManagerURL(_))
+ val workspaceManagerURL: Option[WorkspaceManagerURL] = singletonConfig.config.as[Option[String]]("workspace-manager-url").map(WorkspaceManagerURL)
val blobTokenGenerator: BlobTokenGenerator = BlobTokenGenerator.createBlobTokenGenerator(
container, endpoint, workspaceId, workspaceManagerURL, subscription)
From aacfd8132d31b48ae377e007af36d1c6d446d73b Mon Sep 17 00:00:00 2001
From: Janet Gainer-Dewar <jdewar@broadinstitute.org>
Date: Sun, 2 Oct 2022 14:40:20 -0400
Subject: [PATCH 3/6] Resolve duplicate container name in absolute BlobPath
---
.../filesystems/blob/BlobPathBuilder.scala | 23 +++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
index 3e69ce2a7bd..162b30c9d10 100644
--- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
+++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
@@ -67,19 +67,34 @@ class BlobPathBuilder(container: BlobContainerName, endpoint: EndpointURL)(priva
override def name: String = "Azure Blob Storage"
}
+object BlobPath {
+ // The Azure NIO library uses `{containerName}:` as the root of the path.
+ // This doesn't work well for our need to easily transfer back and forth
+ // to and from the blob URL format. This method removes anything up to and including
+ // the first colon, to create a path string useful for working with BlobPath.
+ // This is safe because the NIO library enforces no colons except to mark
+ // the root container name.
+ private def nioPathString(nioPath: NioPath): String = {
+ val pathStr = nioPath.toString
+ pathStr.substring(pathStr.indexOf(":")+1)
+ }
+}
+
case class BlobPath private[blob](pathString: String, endpoint: EndpointURL, container: BlobContainerName)(private val fsm: BlobFileSystemManager) extends Path {
override def nioPath: NioPath = findNioPath(pathString)
- override protected def newPath(nioPath: NioPath): Path = BlobPath(nioPath.toString, endpoint, container)(fsm)
+ override protected def newPath(nioPath: NioPath): Path =
+ BlobPath(BlobPath.nioPathString(nioPath), endpoint, container)(fsm)
- override def pathAsString: String = List(endpoint, container, nioPath.toString).mkString("/")
+ override def pathAsString: String = List(endpoint, container, pathString.stripPrefix("/")).mkString("/")
//This is purposefully an unprotected get because if the endpoint cannot be parsed this should fail loudly rather than quietly
- override def pathWithoutScheme: String = parseURI(endpoint.value).map(_.getHost + "/" + container + "/" + nioPath.toString).get
+ override def pathWithoutScheme: String = parseURI(endpoint.value).map(u => List(u.getHost, container, pathString).mkString("/")).get
private def findNioPath(path: String): NioPath = (for {
fileSystem <- fsm.retrieveFilesystem()
- nioPath = fileSystem.getPath(path)
+ // The Azure NIO library uses `{container}:` to represent the root of the path
+ nioPath = fileSystem.getPath(s"${container.value}:", path)
// This is purposefully an unprotected get because the NIO API needing an unwrapped path object.
// If an error occurs the api expects a thrown exception
} yield nioPath).get
From f2cba321fce7568a6a41d7403f4740955011010a Mon Sep 17 00:00:00 2001
From: Janet Gainer-Dewar <jdewar@broadinstitute.org>
Date: Sun, 2 Oct 2022 14:40:55 -0400
Subject: [PATCH 4/6] Ignored test demonstrating correct absolute path
generation
---
.../filesystems/blob/BlobPathBuilderSpec.scala | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala
index 9975065a3e2..e4b7aeb6ef8 100644
--- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala
+++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala
@@ -91,4 +91,17 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar {
val fileText = (is.readAllBytes.map(_.toChar)).mkString
fileText should include ("This is my test file!!!! Did it work?")
}
+
+ ignore should "resolve a path without duplicating container name" in {
+ val endpoint = BlobPathBuilderSpec.buildEndpoint("coaexternalstorage")
+ val store = BlobContainerName("inputs")
+ val blobTokenGenerator = NativeBlobTokenGenerator(store, endpoint)
+ val fsm: BlobFileSystemManager = BlobFileSystemManager(store, endpoint, 10, blobTokenGenerator)
+
+ val rootString = List(endpoint.value, store.value, "cromwell-execution").mkString("/")
+ val blobRoot: BlobPath = new BlobPathBuilder(store, endpoint)(fsm) build rootString getOrElse fail()
+ blobRoot.toAbsolutePath.pathAsString should equal ("https://coaexternalstorage.blob.core.windows.net/inputs/cromwell-execution")
+ val otherFile = blobRoot.resolve("test/inputFile.txt")
+ otherFile.toAbsolutePath.pathAsString should equal ("https://coaexternalstorage.blob.core.windows.net/inputs/cromwell-execution/test/inputFile.txt")
+ }
}
From a2b10f9d60538f1638b2ff2c0da522bd0ee8e63d Mon Sep 17 00:00:00 2001
From: Janet Gainer-Dewar <jdewar@broadinstitute.org>
Date: Wed, 5 Oct 2022 13:41:00 -0400
Subject: [PATCH 5/6] Update
filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala
Co-authored-by: Brian Reilly <breilly@broadinstitute.org>
---
.../scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala
index e4b7aeb6ef8..671e7b2d35b 100644
--- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala
+++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala
@@ -98,7 +98,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar {
val blobTokenGenerator = NativeBlobTokenGenerator(store, endpoint)
val fsm: BlobFileSystemManager = BlobFileSystemManager(store, endpoint, 10, blobTokenGenerator)
- val rootString = List(endpoint.value, store.value, "cromwell-execution").mkString("/")
+ val rootString = s"${endpoint.value}/${store.value}/cromwell-execution"
val blobRoot: BlobPath = new BlobPathBuilder(store, endpoint)(fsm) build rootString getOrElse fail()
blobRoot.toAbsolutePath.pathAsString should equal ("https://coaexternalstorage.blob.core.windows.net/inputs/cromwell-execution")
val otherFile = blobRoot.resolve("test/inputFile.txt")
From 26eca2f10f184a134f524d80ec6cbc7892a105b5 Mon Sep 17 00:00:00 2001
From: Janet Gainer-Dewar <jdewar@broadinstitute.org>
Date: Wed, 5 Oct 2022 13:54:56 -0400
Subject: [PATCH 6/6] PR feedback
---
.../cromwell/filesystems/blob/BlobPathBuilder.scala | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
index 162b30c9d10..1be6bb8e13b 100644
--- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
+++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala
@@ -78,13 +78,19 @@ object BlobPath {
val pathStr = nioPath.toString
pathStr.substring(pathStr.indexOf(":")+1)
}
+
+ def apply(nioPath: NioPath,
+ endpoint: EndpointURL,
+ container: BlobContainerName,
+ fsm: BlobFileSystemManager): BlobPath = {
+ BlobPath(nioPathString(nioPath), endpoint, container)(fsm)
+ }
}
case class BlobPath private[blob](pathString: String, endpoint: EndpointURL, container: BlobContainerName)(private val fsm: BlobFileSystemManager) extends Path {
override def nioPath: NioPath = findNioPath(pathString)
- override protected def newPath(nioPath: NioPath): Path =
- BlobPath(BlobPath.nioPathString(nioPath), endpoint, container)(fsm)
+ override protected def newPath(nioPath: NioPath): Path = BlobPath(nioPath, endpoint, container, fsm)
override def pathAsString: String = List(endpoint, container, pathString.stripPrefix("/")).mkString("/")