From 38b5b4b28d34ff73aa6a02410bd65ce7bd46651b Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Fri, 14 Jul 2023 15:48:32 -0400 Subject: [PATCH 01/36] Initial map rework --- .../storage/blob/nio/AzureFileSystem.java | 28 +++++++++---------- .../cloudsupport/azure/AzureUtils.scala | 1 + .../blob/BlobFileSystemManager.scala | 6 ++-- .../blob/BlobPathBuilderFactorySpec.scala | 11 ++++---- .../blob/BlobPathBuilderSpec.scala | 12 ++++---- 5 files changed, 30 insertions(+), 28 deletions(-) diff --git a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java index 6f981b1b45e..5b3d590ba8b 100644 --- a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java +++ b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java @@ -3,19 +3,6 @@ package com.azure.storage.blob.nio; -import com.azure.core.credential.AzureSasCredential; -import com.azure.core.http.HttpClient; -import com.azure.core.http.policy.HttpLogDetailLevel; -import com.azure.core.http.policy.HttpPipelinePolicy; -import com.azure.core.util.CoreUtils; -import com.azure.core.util.logging.ClientLogger; -import com.azure.storage.blob.BlobServiceClient; -import com.azure.storage.blob.BlobServiceClientBuilder; -import com.azure.storage.blob.implementation.util.BlobUserAgentModificationPolicy; -import com.azure.storage.common.StorageSharedKeyCredential; -import com.azure.storage.common.policy.RequestRetryOptions; -import com.azure.storage.common.policy.RetryPolicyType; - import java.io.IOException; import java.nio.file.FileStore; import java.nio.file.FileSystem; @@ -36,6 +23,19 @@ import java.util.regex.PatternSyntaxException; import java.util.stream.Collectors; +import com.azure.core.credential.AzureSasCredential; +import com.azure.core.http.HttpClient; +import com.azure.core.http.policy.HttpLogDetailLevel; +import com.azure.core.http.policy.HttpPipelinePolicy; +import com.azure.core.util.CoreUtils; +import com.azure.core.util.logging.ClientLogger; +import com.azure.storage.blob.BlobServiceClient; +import com.azure.storage.blob.BlobServiceClientBuilder; +import com.azure.storage.blob.implementation.util.BlobUserAgentModificationPolicy; +import com.azure.storage.common.StorageSharedKeyCredential; +import com.azure.storage.common.policy.RequestRetryOptions; +import com.azure.storage.common.policy.RetryPolicyType; + /** * Implement's Java's {@link FileSystem} interface for Azure Blob Storage. *

@@ -221,7 +221,7 @@ public FileSystemProvider provider() { @Override public void close() throws IOException { this.closed = true; - this.parentFileSystemProvider.closeFileSystem(this.getFileSystemUrl()); + this.parentFileSystemProvider.closeFileSystem(this.getFileSystemUrl() + "/" + defaultFileStore.name()); } /** diff --git a/cloudSupport/src/main/scala/cromwell/cloudsupport/azure/AzureUtils.scala b/cloudSupport/src/main/scala/cromwell/cloudsupport/azure/AzureUtils.scala index 09cf5f3869d..bdc6cd10d00 100644 --- a/cloudSupport/src/main/scala/cromwell/cloudsupport/azure/AzureUtils.scala +++ b/cloudSupport/src/main/scala/cromwell/cloudsupport/azure/AzureUtils.scala @@ -29,6 +29,7 @@ object AzureUtils { val azureProfile = new AzureProfile(AzureEnvironment.AZURE) def azureCredentialBuilder = new DefaultAzureCredentialBuilder() + .tenantId("fad90753-2022-4456-9b0a-c7e5b934e408") .authorityHost(azureProfile.getEnvironment.getActiveDirectoryEndpoint) .build 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 e50446ea294..a46d326a571 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala @@ -9,7 +9,7 @@ import common.validation.Validation._ import cromwell.cloudsupport.azure.AzureUtils import java.net.URI -import java.nio.file.{FileSystem, FileSystemNotFoundException, FileSystems} +import java.nio.file._ import java.time.temporal.ChronoUnit import java.time.{Duration, Instant, OffsetDateTime} import scala.jdk.CollectionConverters._ @@ -40,7 +40,7 @@ object BlobFileSystemManager { (AzureFileSystem.AZURE_STORAGE_SKIP_INITIAL_CONTAINER_CHECK, java.lang.Boolean.TRUE)) } def hasTokenExpired(tokenExpiry: Instant, buffer: Duration): Boolean = Instant.now.plus(buffer).isAfter(tokenExpiry) - def uri(endpoint: EndpointURL) = new URI("azb://?endpoint=" + endpoint) + def uri(endpoint: EndpointURL, container: BlobContainerName) = new URI("azb://?endpoint=" + endpoint + "/" + container.value) } class BlobFileSystemManager(val container: BlobContainerName, @@ -65,7 +65,7 @@ class BlobFileSystemManager(val container: BlobContainerName, private var expiry: Option[Instant] = initialExpiration def getExpiry: Option[Instant] = expiry - def uri: URI = BlobFileSystemManager.uri(endpoint) + def uri: URI = BlobFileSystemManager.uri(endpoint, container) def isTokenExpired: Boolean = expiry.exists(BlobFileSystemManager.hasTokenExpired(_, buffer)) def shouldReopenFilesystem: Boolean = isTokenExpired || expiry.isEmpty def retrieveFilesystem(): Try[FileSystem] = { diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala index 881cd3669a1..0e3e513100a 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala @@ -51,7 +51,8 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga it should "test that a filesystem gets closed correctly" in { val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") - val azureUri = BlobFileSystemManager.uri(endpoint) + val container = BlobContainerName("test") + val azureUri = BlobFileSystemManager.uri(endpoint, container) val fileSystems = mock[FileSystemAPI] val fileSystem = mock[FileSystem] when(fileSystems.getFileSystem(azureUri)).thenReturn(Try(fileSystem)) @@ -68,7 +69,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) val container = BlobContainerName("storageContainer") val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) - val azureUri = BlobFileSystemManager.uri(endpoint) + val azureUri = BlobFileSystemManager.uri(endpoint, container) val fileSystems = mock[FileSystemAPI] val blobTokenGenerator = mock[BlobSasTokenGenerator] @@ -93,7 +94,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) val container = BlobContainerName("storageContainer") val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) - val azureUri = BlobFileSystemManager.uri(endpoint) + val azureUri = BlobFileSystemManager.uri(endpoint,container) // Need a fake filesystem to supply the getFileSystem simulated try val dummyFileSystem = mock[FileSystem] @@ -121,7 +122,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) val container = BlobContainerName("storageContainer") val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) - val azureUri = BlobFileSystemManager.uri(endpoint) + val azureUri = BlobFileSystemManager.uri(endpoint, container) val fileSystems = mock[FileSystemAPI] when(fileSystems.getFileSystem(azureUri)).thenReturn(Failure(new FileSystemNotFoundException)) @@ -146,7 +147,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) val container = BlobContainerName("storageContainer") val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) - val azureUri = BlobFileSystemManager.uri(endpoint) + val azureUri = BlobFileSystemManager.uri(endpoint, container) val fileSystems = mock[FileSystemAPI] val blobTokenGenerator = mock[BlobSasTokenGenerator] 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 4012e241eb3..2e511ba31e6 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala @@ -100,7 +100,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { new BlobPathBuilder(store, endpoint)(fsm) } - ignore should "resolve an absolute path string correctly to a path" in { + it should "resolve an absolute path string correctly to a path" in { val builder = makeBlobPathBuilder(endpoint, store) val rootString = s"${endpoint.value}/${store.value}/cromwell-execution" val blobRoot: BlobPath = builder build rootString getOrElse fail() @@ -109,7 +109,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { otherFile.toAbsolutePath.pathAsString should equal ("https://coaexternalstorage.blob.core.windows.net/inputs/cromwell-execution/test/inputFile.txt") } - ignore should "build a blob path from a test string and read a file" in { + it should "build a blob path from a test string and read a file" in { val builder = makeBlobPathBuilder(endpoint, store) val endpointHost = BlobPathBuilder.parseURI(endpoint.value).map(_.getHost).getOrElse(fail("Could not parse URI")) val evalPath = "/test/inputFile.txt" @@ -125,7 +125,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { fileText should include ("This is my test file!!!! Did it work?") } - ignore should "build duplicate blob paths in the same filesystem" in { + it should "build duplicate blob paths in the same filesystem" in { val builder = makeBlobPathBuilder(endpoint, store) val evalPath = "/test/inputFile.txt" val testString = endpoint.value + "/" + store + evalPath @@ -138,7 +138,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { fileText should include ("This is my test file!!!! Did it work?") } - ignore should "resolve a path without duplicating container name" in { + it should "resolve a path without duplicating container name" in { val builder = makeBlobPathBuilder(endpoint, store) val rootString = s"${endpoint.value}/${store.value}/cromwell-execution" val blobRoot: BlobPath = builder build rootString getOrElse fail() @@ -147,7 +147,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { otherFile.toAbsolutePath.pathAsString should equal ("https://coaexternalstorage.blob.core.windows.net/inputs/cromwell-execution/test/inputFile.txt") } - ignore should "correctly remove a prefix from the blob path" in { + it should "correctly remove a prefix from the blob path" in { val builder = makeBlobPathBuilder(endpoint, store) val rootString = s"${endpoint.value}/${store.value}/cromwell-execution/" val execDirString = s"${endpoint.value}/${store.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/" @@ -160,7 +160,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { blobFile.pathStringWithoutPrefix(blobFile) should equal ("") } - ignore should "not change a path if it doesn't start with a prefix" in { + it should "not change a path if it doesn't start with a prefix" in { val builder = makeBlobPathBuilder(endpoint, store) val otherRootString = s"${endpoint.value}/${store.value}/foobar/" val fileString = s"${endpoint.value}/${store.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/stdout" From 30490d98b29b5820b352c1967ac97a7298b51a0e Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Mon, 17 Jul 2023 17:19:20 -0400 Subject: [PATCH 02/36] Move over changes from initial prototype --- .../storage/blob/nio/AzureFileSystem.java | 27 ++++ .../blob/BlobFileSystemManager.scala | 117 +++++++----------- .../filesystems/blob/BlobPathBuilder.scala | 34 ++--- .../blob/BlobPathBuilderFactory.scala | 21 +++- .../WorkspaceManagerApiClientProvider.scala | 47 ++++++- .../org.mockito.plugins.MockMaker | 1 + .../blob/BlobPathBuilderFactorySpec.scala | 85 +++++-------- .../blob/BlobPathBuilderSpec.scala | 38 ++---- 8 files changed, 195 insertions(+), 175 deletions(-) create mode 100644 filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker diff --git a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java index 5b3d590ba8b..c9b22070fb2 100644 --- a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java +++ b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java @@ -14,11 +14,16 @@ import java.nio.file.attribute.FileAttributeView; import java.nio.file.attribute.UserPrincipalLookupService; import java.nio.file.spi.FileSystemProvider; +import java.time.Duration; +import java.time.Instant; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.regex.PatternSyntaxException; import java.util.stream.Collectors; @@ -162,6 +167,7 @@ public final class AzureFileSystem extends FileSystem { private final Map fileStores; private FileStore defaultFileStore; private boolean closed; + private Instant expiry; AzureFileSystem(AzureFileSystemProvider parentFileSystemProvider, String endpoint, Map config) throws IOException { @@ -397,6 +403,7 @@ private BlobServiceClient buildBlobServiceClient(String endpoint, Map builder.credential((StorageSharedKeyCredential) config.get(AZURE_STORAGE_SHARED_KEY_CREDENTIAL)); } else if (config.containsKey(AZURE_STORAGE_SAS_TOKEN_CREDENTIAL)) { builder.credential((AzureSasCredential) config.get(AZURE_STORAGE_SAS_TOKEN_CREDENTIAL)); + this.setExpiryFromSAS((AzureSasCredential) config.get(AZURE_STORAGE_SAS_TOKEN_CREDENTIAL)); } else { throw LoggingUtility.logError(LOGGER, new IllegalArgumentException(String.format("No credentials were " + "provided. Please specify one of the following when constructing an AzureFileSystem: %s, %s.", @@ -489,4 +496,24 @@ Long getPutBlobThreshold() { Integer getMaxConcurrencyPerRequest() { return this.maxConcurrencyPerRequest; } + + public Optional getExpiry() { + return Optional.ofNullable(expiry); + } + + private void setExpiryFromSAS(AzureSasCredential token) { + List strings = Arrays.asList(token.getSignature().split("&")); + Optional expiryString = strings.stream() + .filter(s -> s.startsWith("se")) + .findFirst() + .map(s -> s.replaceFirst("se=","")) + .map(s -> s.replace("%3A", ":")); + this.expiry = expiryString.map(es -> Instant.parse(es)).orElse(null); + } + + public boolean isExpired(Duration buffer) { + return Optional.ofNullable(this.expiry) + .map(e -> Instant.now().plus(buffer).isAfter(e)) + .orElse(true); + } } 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 a46d326a571..3df5c94d39d 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala @@ -12,14 +12,15 @@ import java.net.URI import java.nio.file._ import java.time.temporal.ChronoUnit import java.time.{Duration, Instant, OffsetDateTime} +import java.util.UUID import scala.jdk.CollectionConverters._ -import scala.util.{Failure, Success, Try} +import scala.util.{Success, Try} // We encapsulate this functionality here so that we can easily mock it out, to allow for testing without // actually connecting to Blob storage. case class FileSystemAPI() { - def getFileSystem(uri: URI): Try[FileSystem] = Try(FileSystems.getFileSystem(uri)) - def newFileSystem(uri: URI, config: Map[String, Object]): FileSystem = FileSystems.newFileSystem(uri, config.asJava) + def getFileSystem(uri: URI): Try[AzureFileSystem] = Try( FileSystems.getFileSystem(uri).asInstanceOf[AzureFileSystem]) + def newFileSystem(uri: URI, config: Map[String, Object]): AzureFileSystem = FileSystems.newFileSystem(uri, config.asJava).asInstanceOf[AzureFileSystem] def closeFileSystem(uri: URI): Option[Unit] = getFileSystem(uri).toOption.map(_.close) } /** @@ -43,17 +44,12 @@ object BlobFileSystemManager { def uri(endpoint: EndpointURL, container: BlobContainerName) = new URI("azb://?endpoint=" + endpoint + "/" + container.value) } -class BlobFileSystemManager(val container: BlobContainerName, - val endpoint: EndpointURL, - val expiryBufferMinutes: Long, +class BlobFileSystemManager(val expiryBufferMinutes: Long, val blobTokenGenerator: BlobSasTokenGenerator, - val fileSystemAPI: FileSystemAPI = FileSystemAPI(), - private val initialExpiration: Option[Instant] = None) extends LazyLogging { + val fileSystemAPI: FileSystemAPI = FileSystemAPI()) extends LazyLogging { def this(config: BlobFileSystemConfig) = { this( - config.blobContainerName, - config.endpointURL, config.expiryBufferMinutes, BlobSasTokenGenerator.createBlobTokenGeneratorFromConfig(config) ) @@ -61,40 +57,36 @@ class BlobFileSystemManager(val container: BlobContainerName, def this(rawConfig: Config) = this(BlobFileSystemConfig(rawConfig)) - val buffer: Duration = Duration.of(expiryBufferMinutes, ChronoUnit.MINUTES) - private var expiry: Option[Instant] = initialExpiration + def buffer: Duration = Duration.of(expiryBufferMinutes, ChronoUnit.MINUTES) - def getExpiry: Option[Instant] = expiry - def uri: URI = BlobFileSystemManager.uri(endpoint, container) - def isTokenExpired: Boolean = expiry.exists(BlobFileSystemManager.hasTokenExpired(_, buffer)) - def shouldReopenFilesystem: Boolean = isTokenExpired || expiry.isEmpty - def retrieveFilesystem(): Try[FileSystem] = { + def retrieveFilesystem(endpoint: EndpointURL, container: BlobContainerName): Try[FileSystem] = { + val uri = BlobFileSystemManager.uri(endpoint, container) synchronized { - shouldReopenFilesystem match { - case false => fileSystemAPI.getFileSystem(uri).recoverWith { - // If no filesystem already exists, this will create a new connection, with the provided configs - case _: FileSystemNotFoundException => - logger.info(s"Creating new blob filesystem for URI $uri") - blobTokenGenerator.generateBlobSasToken.flatMap(generateFilesystem(uri, container, _)) + fileSystemAPI.getFileSystem(uri).recoverWith { + // If no filesystem already exists, this will create a new connection, with the provided configs + case _: FileSystemNotFoundException => { + logger.info(s"Creating new blob filesystem for URI $uri") + generateFilesystem(uri, container, endpoint) } - // If the token has expired, OR there is no token record, try to close the FS and regenerate - case true => + }.filter(!_.isExpired(buffer)).recoverWith { + case _ => { logger.info(s"Closing & regenerating token for existing blob filesystem at URI $uri") fileSystemAPI.closeFileSystem(uri) - blobTokenGenerator.generateBlobSasToken.flatMap(generateFilesystem(uri, container, _)) + generateFilesystem(uri, container, endpoint) + } } } } - private def generateFilesystem(uri: URI, container: BlobContainerName, token: AzureSasCredential): Try[FileSystem] = { - expiry = BlobFileSystemManager.parseTokenExpiry(token) - if (expiry.isEmpty) return Failure(new Exception("Could not reopen filesystem, no expiration found")) - Try(fileSystemAPI.newFileSystem(uri, BlobFileSystemManager.buildConfigMap(token, container))) + private def generateFilesystem(uri: URI, container: BlobContainerName, endpoint: EndpointURL): Try[AzureFileSystem] = { + blobTokenGenerator.generateBlobSasToken(endpoint, container) + .flatMap((token: AzureSasCredential) => { + Try(fileSystemAPI.newFileSystem(uri, BlobFileSystemManager.buildConfigMap(token, container))) + }) } - } -sealed trait BlobSasTokenGenerator { def generateBlobSasToken: Try[AzureSasCredential] } +sealed trait BlobSasTokenGenerator { def generateBlobSasToken(endpoint: EndpointURL, container: BlobContainerName): Try[AzureSasCredential] } object BlobSasTokenGenerator { /** @@ -122,16 +114,12 @@ object BlobSasTokenGenerator { // WSM-mediated mediated SAS token generator // parameterizing client instead of URL to make injecting mock client possible BlobSasTokenGenerator.createBlobTokenGenerator( - config.blobContainerName, - config.endpointURL, - wsmConfig.workspaceId, - wsmConfig.containerResourceId, wsmClient, wsmConfig.overrideWsmAuthToken ) }.getOrElse( // Native SAS token generator - BlobSasTokenGenerator.createBlobTokenGenerator(config.blobContainerName, config.endpointURL, config.subscriptionId) + BlobSasTokenGenerator.createBlobTokenGenerator(config.subscriptionId) ) /** @@ -146,10 +134,8 @@ object BlobSasTokenGenerator { * @return A NativeBlobTokenGenerator, able to produce a valid SAS token for accessing the provided blob * container and endpoint locally */ - def createBlobTokenGenerator(container: BlobContainerName, - endpoint: EndpointURL, - subscription: Option[SubscriptionId]): BlobSasTokenGenerator = { - NativeBlobSasTokenGenerator(container, endpoint, subscription) + def createBlobTokenGenerator(subscription: Option[SubscriptionId]): BlobSasTokenGenerator = { + NativeBlobSasTokenGenerator(subscription) } /** @@ -157,11 +143,6 @@ object BlobSasTokenGenerator { * to request a SAS token from the WSM to access the given blob container. If an overrideWsmAuthToken * is provided this is used instead. * - * @param container The BlobContainerName of the blob container to be accessed by the generated SAS token - * @param endpoint The EndpointURL containing the storage account of the blob container to be accessed by - * this SAS token - * @param workspaceId The WorkspaceId of the account to authenticate against - * @param containerResourceId The ContainterResourceId of the blob container as WSM knows it * @param workspaceManagerClient The client for making requests against WSM * @param overrideWsmAuthToken An optional WsmAuthToken used for authenticating against the WSM for a valid * SAS token to access the given container and endpoint. This is a dev only option that is only intended @@ -169,22 +150,14 @@ object BlobSasTokenGenerator { * @return A WSMBlobTokenGenerator, able to produce a valid SAS token for accessing the provided blob * container and endpoint that is managed by WSM */ - def createBlobTokenGenerator(container: BlobContainerName, - endpoint: EndpointURL, - workspaceId: WorkspaceId, - containerResourceId: ContainerResourceId, - workspaceManagerClient: WorkspaceManagerApiClientProvider, + def createBlobTokenGenerator(workspaceManagerClient: WorkspaceManagerApiClientProvider, overrideWsmAuthToken: Option[String]): BlobSasTokenGenerator = { - WSMBlobSasTokenGenerator(container, endpoint, workspaceId, containerResourceId, workspaceManagerClient, overrideWsmAuthToken) + WSMBlobSasTokenGenerator(workspaceManagerClient, overrideWsmAuthToken) } } -case class WSMBlobSasTokenGenerator(container: BlobContainerName, - endpoint: EndpointURL, - workspaceId: WorkspaceId, - containerResourceId: ContainerResourceId, - wsmClientProvider: WorkspaceManagerApiClientProvider, +case class WSMBlobSasTokenGenerator(wsmClientProvider: WorkspaceManagerApiClientProvider, overrideWsmAuthToken: Option[String]) extends BlobSasTokenGenerator { /** @@ -194,7 +167,7 @@ case class WSMBlobSasTokenGenerator(container: BlobContainerName, * * @return an AzureSasCredential for accessing a blob container */ - def generateBlobSasToken: Try[AzureSasCredential] = { + def generateBlobSasToken(endpoint: EndpointURL, container: BlobContainerName): Try[AzureSasCredential] = { val wsmAuthToken: Try[String] = overrideWsmAuthToken match { case Some(t) => Success(t) case None => AzureCredentials.getAccessToken(None).toTry @@ -202,21 +175,23 @@ case class WSMBlobSasTokenGenerator(container: BlobContainerName, for { wsmAuth <- wsmAuthToken - wsmClient = wsmClientProvider.getControlledAzureResourceApi(wsmAuth) - sasToken <- Try( // Java library throws - wsmClient.createAzureStorageContainerSasToken( - workspaceId.value, - containerResourceId.value, - null, - null, - null, - null - ).getToken) - } yield new AzureSasCredential(sasToken) + wsmAzureResourceClient = wsmClientProvider.getControlledAzureResourceApi(wsmAuth) + sa <- endpoint.storageAccountName + ids <- getWorkspaceAndContainerResourceId(sa, container, wsmAuth) + azureSasToken <- wsmAzureResourceClient.createAzureStorageContainerSasToken(ids._1, ids._2) + } yield azureSasToken + } + + def getWorkspaceAndContainerResourceId(storageAccountName: StorageAccountName, container: BlobContainerName, wsmAuth : String): Try[(UUID, UUID)] = { + val wsmResourceClient = wsmClientProvider.getResourceApi(wsmAuth) + for { + workspaceId <- container.workspaceId + resourceId <- wsmResourceClient.findContainerResourceId(workspaceId, container) + } yield (workspaceId, resourceId) } } -case class NativeBlobSasTokenGenerator(container: BlobContainerName, endpoint: EndpointURL, subscription: Option[SubscriptionId] = None) extends BlobSasTokenGenerator { +case class NativeBlobSasTokenGenerator(subscription: Option[SubscriptionId] = None) extends BlobSasTokenGenerator { private val bcsp = new BlobContainerSasPermission() .setReadPermission(true) .setCreatePermission(true) @@ -229,7 +204,7 @@ case class NativeBlobSasTokenGenerator(container: BlobContainerName, endpoint: E * * @return an AzureSasCredential for accessing a blob container */ - def generateBlobSasToken: Try[AzureSasCredential] = for { + def generateBlobSasToken(endpoint: EndpointURL, container: BlobContainerName): Try[AzureSasCredential] = for { bcc <- AzureUtils.buildContainerClientFromLocalEnvironment(container.toString, endpoint.toString, subscription.map(_.toString)) bsssv = new BlobServiceSasSignatureValues(OffsetDateTime.now.plusDays(1), bcsp) asc = new AzureSasCredential(bcc.generateSas(bsssv)) 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 9e7b230286c..91260b233fe 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala @@ -11,12 +11,13 @@ import scala.language.postfixOps import scala.util.{Failure, Success, Try} object BlobPathBuilder { - + val HOSTNAME_SUFFIX = ".blob.core.windows.net" sealed trait BlobPathValidation - case class ValidBlobPath(path: String) extends BlobPathValidation + case class ValidBlobPath(path: String, container: BlobContainerName, endpoint: EndpointURL) extends BlobPathValidation case class UnparsableBlobPath(errorMessage: Throwable) extends BlobPathValidation - def invalidBlobPathMessage(container: BlobContainerName, endpoint: EndpointURL) = s"Malformed Blob URL for this builder. Expecting a URL for a container $container and endpoint $endpoint" + def invalidBlobHostMessage(endpoint: EndpointURL) = s"Malformed Blob URL for this builder: The endpoint $endpoint doesn't contain the expected host string '{SA}.blob.core.windows.net/'" + def invalidBlobContainerMessage(endpoint: EndpointURL) = s"Malformed Blob URL for this builder: Could not parse container" def parseURI(string: String): Try[URI] = Try(URI.create(UrlEscapers.urlFragmentEscaper().escape(string))) def parseStorageAccount(uri: URI): Try[StorageAccountName] = uri.getHost.split("\\.").find(_.nonEmpty).map(StorageAccountName(_)) .map(Success(_)).getOrElse(Failure(new Exception("Could not parse storage account"))) @@ -39,28 +40,31 @@ object BlobPathBuilder { * * If the configured container and storage account do not match, the string is considered unparsable */ - def validateBlobPath(string: String, container: BlobContainerName, endpoint: EndpointURL): BlobPathValidation = { + def validateBlobPath(string: String): BlobPathValidation = { val blobValidation = for { testUri <- parseURI(string) - endpointUri <- parseURI(endpoint.value) + testEndpoint = EndpointURL(testUri.getScheme + "://" + testUri.getHost()) testStorageAccount <- parseStorageAccount(testUri) - endpointStorageAccount <- parseStorageAccount(endpointUri) - hasContainer = testUri.getPath.split("/").find(_.nonEmpty).contains(container.value) - hasEndpoint = testStorageAccount.equals(endpointStorageAccount) - blobPathValidation = (hasContainer && hasEndpoint) match { - case true => ValidBlobPath(testUri.getPath.replaceFirst("/" + container, "")) - case false => UnparsableBlobPath(new MalformedURLException(invalidBlobPathMessage(container, endpoint))) + testContainer = testUri.getPath.split("/").find(_.nonEmpty) + isBlobHost = testUri.getHost().contains(HOSTNAME_SUFFIX) && testUri.getScheme().contains("https") + blobPathValidation = (isBlobHost, testContainer) match { + case (true, Some(container)) => ValidBlobPath( + testUri.getPath.replaceFirst("/" + container, ""), + BlobContainerName(container), + testEndpoint) + case (false, _) => UnparsableBlobPath(new MalformedURLException(invalidBlobHostMessage(testEndpoint))) + case (true, None) => UnparsableBlobPath(new MalformedURLException(invalidBlobContainerMessage(testEndpoint))) } } yield blobPathValidation blobValidation recover { case t => UnparsableBlobPath(t) } get } } -class BlobPathBuilder(container: BlobContainerName, endpoint: EndpointURL)(private val fsm: BlobFileSystemManager) extends PathBuilder { +class BlobPathBuilder()(private val fsm: BlobFileSystemManager) extends PathBuilder { def build(string: String): Try[BlobPath] = { - validateBlobPath(string, container, endpoint) match { - case ValidBlobPath(path) => Try(BlobPath(path, endpoint, container)(fsm)) + validateBlobPath(string) match { + case ValidBlobPath(path, container, endpoint) => Try(BlobPath(path, endpoint, container)(fsm)) case UnparsableBlobPath(errorMessage: Throwable) => Failure(errorMessage) } } @@ -106,7 +110,7 @@ case class BlobPath private[blob](pathString: String, endpoint: EndpointURL, con override def pathWithoutScheme: String = parseURI(endpoint.value).map(u => List(u.getHost, container, pathString.stripPrefix("/")).mkString("/")).get private def findNioPath(path: String): NioPath = (for { - fileSystem <- fsm.retrieveFilesystem() + fileSystem <- fsm.retrieveFilesystem(endpoint, container) // 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. 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 c263841dc8a..47245552dc2 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala @@ -8,11 +8,26 @@ import cromwell.core.path.PathBuilderFactory.PriorityBlob import java.util.UUID import scala.concurrent.{ExecutionContext, Future} +import scala.util.Try final case class SubscriptionId(value: UUID) {override def toString: String = value.toString} -final case class BlobContainerName(value: String) {override def toString: String = value} +final case class BlobContainerName(value: String) { + override def toString: String = value + lazy val workspaceId: Try[UUID] = { + Try(UUID.fromString(value.replaceFirst("sc-",""))) + } +} 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 EndpointURL(value: String) { + override def toString: String = value + lazy val storageAccountName : Try[StorageAccountName] = { + val sa = for { + host <- value.split("//").findLast(_.nonEmpty) + storageAccountName <- host.split("\\.").find(_.nonEmpty) + } yield StorageAccountName(storageAccountName) + sa.toRight(new Exception(s"Storage account name could not be parsed from $value")).toTry + } +} final case class WorkspaceId(value: UUID) {override def toString: String = value.toString} final case class ContainerResourceId(value: UUID) {override def toString: String = value.toString} final case class WorkspaceManagerURL(value: String) {override def toString: String = value} @@ -21,7 +36,7 @@ final case class BlobPathBuilderFactory(globalConfig: Config, instanceConfig: Co override def withOptions(options: WorkflowOptions)(implicit as: ActorSystem, ec: ExecutionContext): Future[BlobPathBuilder] = { Future { - new BlobPathBuilder(fsm.container, fsm.endpoint)(fsm) + new BlobPathBuilder()(fsm) } } diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/WorkspaceManagerApiClientProvider.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/WorkspaceManagerApiClientProvider.scala index a9f52d92a91..1db09d1d0cd 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/WorkspaceManagerApiClientProvider.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/WorkspaceManagerApiClientProvider.scala @@ -1,7 +1,14 @@ package cromwell.filesystems.blob -import bio.terra.workspace.api.ControlledAzureResourceApi +import bio.terra.workspace.api._ import bio.terra.workspace.client.ApiClient +import com.azure.core.credential.AzureSasCredential + +import java.util.UUID +import scala.util.Try +import scala.jdk.CollectionConverters._ +import bio.terra.workspace.model.ResourceType +import bio.terra.workspace.model.StewardshipType /** * Represents a way to get a client for interacting with workspace manager controlled resources. @@ -12,7 +19,8 @@ import bio.terra.workspace.client.ApiClient * For testing, create an anonymous subclass as in `org.broadinstitute.dsde.rawls.dataaccess.workspacemanager.HttpWorkspaceManagerDAOSpec` */ trait WorkspaceManagerApiClientProvider { - def getControlledAzureResourceApi(token: String): ControlledAzureResourceApi + def getControlledAzureResourceApi(token: String): WsmControlledAzureResourceApi + def getResourceApi(token: String): WsmResourceApi } class HttpWorkspaceManagerClientProvider(baseWorkspaceManagerUrl: WorkspaceManagerURL) extends WorkspaceManagerApiClientProvider { @@ -22,9 +30,40 @@ class HttpWorkspaceManagerClientProvider(baseWorkspaceManagerUrl: WorkspaceManag client } - def getControlledAzureResourceApi(token: String): ControlledAzureResourceApi = { + def getResourceApi(token: String): WsmResourceApi = { + val apiClient = getApiClient + apiClient.setAccessToken(token) + WsmResourceApi(new ResourceApi(apiClient)) + } + + def getControlledAzureResourceApi(token: String): WsmControlledAzureResourceApi = { val apiClient = getApiClient apiClient.setAccessToken(token) - new ControlledAzureResourceApi(apiClient) + WsmControlledAzureResourceApi(new ControlledAzureResourceApi(apiClient)) + } +} + +case class WsmResourceApi(resourcesApi : ResourceApi) { + def findContainerResourceId(workspaceId : UUID, container: BlobContainerName): Try[UUID] = { + for { + workspaceResources <- Try(resourcesApi.enumerateResources(workspaceId, 0, 1, ResourceType.AZURE_STORAGE_CONTAINER, StewardshipType.CONTROLLED).getResources()) + workspaceStorageContainerOption = workspaceResources.asScala.find(r => r.getMetadata().getName() == container.value) + workspaceStorageContainer <- workspaceStorageContainerOption.toRight(new Exception("No storage container found for this workspace")).toTry + resourceId = workspaceStorageContainer.getMetadata().getResourceId() + } yield resourceId + } +} +case class WsmControlledAzureResourceApi(controlledAzureResourceApi : ControlledAzureResourceApi) { + def createAzureStorageContainerSasToken(workspaceId: UUID, resourceId: UUID): Try[AzureSasCredential] = { + for { + sas <- Try(controlledAzureResourceApi.createAzureStorageContainerSasToken( + workspaceId, + resourceId, + null, + null, + null, + null + ).getToken) + } yield new AzureSasCredential(sas) } } diff --git a/filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker b/filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker new file mode 100644 index 00000000000..1f0955d450f --- /dev/null +++ b/filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker @@ -0,0 +1 @@ +mock-maker-inline diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala index 0e3e513100a..a2f71570f21 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala @@ -1,16 +1,17 @@ package cromwell.filesystems.blob import com.azure.core.credential.AzureSasCredential +import com.azure.storage.blob.nio.AzureFileSystem import common.mock.MockSugar import org.mockito.Mockito._ import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers -import java.nio.file.{FileSystem, FileSystemNotFoundException} +import java.nio.file.FileSystemNotFoundException import java.time.format.DateTimeFormatter import java.time.temporal.ChronoUnit import java.time.{Duration, Instant, ZoneId} -import scala.util.{Failure, Try} +import scala.util.{Failure, Success, Try} object BlobPathBuilderFactorySpec { @@ -54,7 +55,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga val container = BlobContainerName("test") val azureUri = BlobFileSystemManager.uri(endpoint, container) val fileSystems = mock[FileSystemAPI] - val fileSystem = mock[FileSystem] + val fileSystem = mock[AzureFileSystem] when(fileSystems.getFileSystem(azureUri)).thenReturn(Try(fileSystem)) when(fileSystems.closeFileSystem(azureUri)).thenCallRealMethod() @@ -64,53 +65,52 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga it should "test retrieveFileSystem with expired filesystem" in { val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") - val expiredToken = generateTokenExpiration(9L) + //val expiredToken = generateTokenExpiration(9L) val refreshedToken = generateTokenExpiration(69L) val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) val container = BlobContainerName("storageContainer") val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) val azureUri = BlobFileSystemManager.uri(endpoint, container) + //Mocking this final class requires the plugin Mock Maker Inline plugin, configured here + //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker + val azureFileSystem = mock[AzureFileSystem] + when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(true) val fileSystems = mock[FileSystemAPI] + when(fileSystems.getFileSystem(azureUri)).thenReturn(Success(azureFileSystem)) val blobTokenGenerator = mock[BlobSasTokenGenerator] - when(blobTokenGenerator.generateBlobSasToken).thenReturn(Try(sasToken)) + when(blobTokenGenerator.generateBlobSasToken(endpoint, container)).thenReturn(Try(sasToken)) - val fsm = new BlobFileSystemManager(container, endpoint, 10L, blobTokenGenerator, fileSystems, Some(expiredToken)) - fsm.getExpiry should contain(expiredToken) - fsm.isTokenExpired shouldBe true - fsm.retrieveFilesystem() + val fsm = new BlobFileSystemManager(10L, blobTokenGenerator, fileSystems) + fsm.retrieveFilesystem(endpoint, container) - fsm.getExpiry should contain(refreshedToken) - fsm.isTokenExpired shouldBe false - verify(fileSystems, never()).getFileSystem(azureUri) + verify(fileSystems, times(1)).getFileSystem(azureUri) verify(fileSystems, times(1)).newFileSystem(azureUri, configMap) verify(fileSystems, times(1)).closeFileSystem(azureUri) } it should "test retrieveFileSystem with an unexpired fileSystem" in { val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") - val initialToken = generateTokenExpiration(11L) + //val initialToken = generateTokenExpiration(11L) val refreshedToken = generateTokenExpiration(71L) val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) val container = BlobContainerName("storageContainer") val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) val azureUri = BlobFileSystemManager.uri(endpoint,container) - // Need a fake filesystem to supply the getFileSystem simulated try - val dummyFileSystem = mock[FileSystem] + //Mocking this final class requires the plugin Mock Maker Inline plugin, configured here + //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker + val azureFileSystem = mock[AzureFileSystem] + when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(false) val fileSystems = mock[FileSystemAPI] - when(fileSystems.getFileSystem(azureUri)).thenReturn(Try(dummyFileSystem)) + when(fileSystems.getFileSystem(azureUri)).thenReturn(Try(azureFileSystem)) val blobTokenGenerator = mock[BlobSasTokenGenerator] - when(blobTokenGenerator.generateBlobSasToken).thenReturn(Try(sasToken)) + when(blobTokenGenerator.generateBlobSasToken(endpoint, container)).thenReturn(Try(sasToken)) - val fsm = new BlobFileSystemManager(container, endpoint, 10L, blobTokenGenerator, fileSystems, Some(initialToken)) - fsm.getExpiry should contain(initialToken) - fsm.isTokenExpired shouldBe false - fsm.retrieveFilesystem() + val fsm = new BlobFileSystemManager(10L, blobTokenGenerator, fileSystems) + fsm.retrieveFilesystem(endpoint, container) - fsm.getExpiry should contain(initialToken) - fsm.isTokenExpired shouldBe false verify(fileSystems, times(1)).getFileSystem(azureUri) verify(fileSystems, never()).newFileSystem(azureUri, configMap) verify(fileSystems, never()).closeFileSystem(azureUri) @@ -124,44 +124,21 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) val azureUri = BlobFileSystemManager.uri(endpoint, container) + //Mocking this final class requires the plugin Mock Maker Inline plugin, configured here + //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker + val azureFileSystem = mock[AzureFileSystem] + when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(false) val fileSystems = mock[FileSystemAPI] when(fileSystems.getFileSystem(azureUri)).thenReturn(Failure(new FileSystemNotFoundException)) + when(fileSystems.newFileSystem(azureUri, configMap)).thenReturn(azureFileSystem) val blobTokenGenerator = mock[BlobSasTokenGenerator] - when(blobTokenGenerator.generateBlobSasToken).thenReturn(Try(sasToken)) + when(blobTokenGenerator.generateBlobSasToken(endpoint, container)).thenReturn(Try(sasToken)) - val fsm = new BlobFileSystemManager(container, endpoint, 10L, blobTokenGenerator, fileSystems, Some(refreshedToken)) - fsm.getExpiry.isDefined shouldBe true - fsm.isTokenExpired shouldBe false - fsm.retrieveFilesystem() + val fsm = new BlobFileSystemManager(0L, blobTokenGenerator, fileSystems) + fsm.retrieveFilesystem(endpoint, container) - fsm.getExpiry should contain(refreshedToken) - fsm.isTokenExpired shouldBe false verify(fileSystems, times(1)).getFileSystem(azureUri) verify(fileSystems, times(1)).newFileSystem(azureUri, configMap) verify(fileSystems, never()).closeFileSystem(azureUri) } - - it should "test retrieveFileSystem with an unknown filesystem" in { - val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") - val refreshedToken = generateTokenExpiration(71L) - val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) - val container = BlobContainerName("storageContainer") - val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) - val azureUri = BlobFileSystemManager.uri(endpoint, container) - - val fileSystems = mock[FileSystemAPI] - val blobTokenGenerator = mock[BlobSasTokenGenerator] - when(blobTokenGenerator.generateBlobSasToken).thenReturn(Try(sasToken)) - - val fsm = new BlobFileSystemManager(container, endpoint, 10L, blobTokenGenerator, fileSystems) - fsm.getExpiry.isDefined shouldBe false - fsm.isTokenExpired shouldBe false - fsm.retrieveFilesystem() - - fsm.getExpiry should contain(refreshedToken) - fsm.isTokenExpired shouldBe false - verify(fileSystems, never()).getFileSystem(azureUri) - verify(fileSystems, times(1)).newFileSystem(azureUri, configMap) - verify(fileSystems, times(1)).closeFileSystem(azureUri) - } } 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 2e511ba31e6..c9a97d00baa 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala @@ -18,41 +18,23 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { val container = BlobContainerName("container") val evalPath = "/path/to/file" val testString = endpoint.value + "/" + container + evalPath - BlobPathBuilder.validateBlobPath(testString, container, endpoint) match { - case BlobPathBuilder.ValidBlobPath(path) => path should equal(evalPath) + BlobPathBuilder.validateBlobPath(testString) match { + case BlobPathBuilder.ValidBlobPath(path, parsedContainer, parsedEndpoint) => { + path should equal(evalPath) + parsedContainer should equal(container) + parsedEndpoint should equal(endpoint) + } case BlobPathBuilder.UnparsableBlobPath(errorMessage) => fail(errorMessage) } } - it should "bad storage account fails causes URI to fail parse into a path" in { - val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") - val container = BlobContainerName("container") - val evalPath = "/path/to/file" - val testString = BlobPathBuilderSpec.buildEndpoint("badStorageAccount").value + container.value + evalPath - BlobPathBuilder.validateBlobPath(testString, container, endpoint) match { - case BlobPathBuilder.ValidBlobPath(path) => fail(s"Valid path: $path found when verifying mismatched storage account") - case BlobPathBuilder.UnparsableBlobPath(errorMessage) => errorMessage.getMessage should equal(BlobPathBuilder.invalidBlobPathMessage(container, endpoint)) - } - } - - it should "bad container fails causes URI to fail parse into a path" in { - val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") - val container = BlobContainerName("container") - val evalPath = "/path/to/file" - val testString = endpoint.value + "badContainer" + evalPath - BlobPathBuilder.validateBlobPath(testString, container, endpoint) match { - case BlobPathBuilder.ValidBlobPath(path) => fail(s"Valid path: $path found when verifying mismatched container") - case BlobPathBuilder.UnparsableBlobPath(errorMessage) => errorMessage.getMessage should equal(BlobPathBuilder.invalidBlobPathMessage(container, endpoint)) - } - } - it should "provide a readable error when getting an illegal nioPath" in { val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") val container = BlobContainerName("container") val evalPath = "/path/to/file" val exception = new Exception("Failed to do the thing") val fsm = mock[BlobFileSystemManager] - when(fsm.retrieveFilesystem()).thenReturn(Failure(exception)) + when(fsm.retrieveFilesystem(endpoint, container)).thenReturn(Failure(exception)) val path = BlobPath(evalPath, endpoint, container)(fsm) val testException = Try(path.nioPath).failed.toOption testException should contain(exception) @@ -95,9 +77,9 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { private val store: BlobContainerName = BlobContainerName("inputs") def makeBlobPathBuilder(blobEndpoint: EndpointURL, container: BlobContainerName): BlobPathBuilder = { - val blobTokenGenerator = NativeBlobSasTokenGenerator(container, blobEndpoint, Some(subscriptionId)) - val fsm = new BlobFileSystemManager(container, blobEndpoint, 10, blobTokenGenerator) - new BlobPathBuilder(store, endpoint)(fsm) + val blobTokenGenerator = NativeBlobSasTokenGenerator(Some(subscriptionId)) + val fsm = new BlobFileSystemManager(10, blobTokenGenerator) + new BlobPathBuilder()(fsm) } it should "resolve an absolute path string correctly to a path" in { From 73825d390519f021d913283b736214de76582e7e Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Mon, 17 Jul 2023 20:52:02 -0400 Subject: [PATCH 03/36] Some cleanup --- .../storage/blob/nio/AzureFileSystem.java | 10 ++++ .../blob/BlobFileSystemManager.scala | 58 +++++++++++++------ .../blob/BlobPathBuilderFactorySpec.scala | 20 ++----- 3 files changed, 54 insertions(+), 34 deletions(-) diff --git a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java index c9b22070fb2..b1b27ccac1a 100644 --- a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java +++ b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java @@ -72,6 +72,11 @@ public final class AzureFileSystem extends FileSystem { */ public static final String AZURE_STORAGE_SAS_TOKEN_CREDENTIAL = "AzureStorageSasTokenCredential"; + /** + * Expected type: String + */ + public static final String AZURE_STORAGE_PUBLIC_ACCESS_CREDENTIAL = "AzureStoragePublicAccessCredential"; + /** * Expected type: com.azure.core.http.policy.HttpLogLevelDetail */ @@ -404,6 +409,11 @@ private BlobServiceClient buildBlobServiceClient(String endpoint, Map } else if (config.containsKey(AZURE_STORAGE_SAS_TOKEN_CREDENTIAL)) { builder.credential((AzureSasCredential) config.get(AZURE_STORAGE_SAS_TOKEN_CREDENTIAL)); this.setExpiryFromSAS((AzureSasCredential) config.get(AZURE_STORAGE_SAS_TOKEN_CREDENTIAL)); + } else if (config.containsKey(AZURE_STORAGE_PUBLIC_ACCESS_CREDENTIAL)) { + // The Blob Service Client Builder requires at least one kind of authentication to make requests + // For public files however, this is unnecessary. This key-value pair is to denote the case + // explicitly when we supply a placeholder SAS credential to bypass this requirement. + builder.credential((AzureSasCredential) config.get(AZURE_STORAGE_PUBLIC_ACCESS_CREDENTIAL)); } else { throw LoggingUtility.logError(LOGGER, new IllegalArgumentException(String.format("No credentials were " + "provided. Please specify one of the following when constructing an AzureFileSystem: %s, %s.", 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 3df5c94d39d..eeabdd0762e 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala @@ -5,7 +5,6 @@ import com.azure.storage.blob.nio.AzureFileSystem import com.azure.storage.blob.sas.{BlobContainerSasPermission, BlobServiceSasSignatureValues} import com.typesafe.config.Config import com.typesafe.scalalogging.LazyLogging -import common.validation.Validation._ import cromwell.cloudsupport.azure.AzureUtils import java.net.URI @@ -14,7 +13,9 @@ import java.time.temporal.ChronoUnit import java.time.{Duration, Instant, OffsetDateTime} import java.util.UUID import scala.jdk.CollectionConverters._ -import scala.util.{Success, Try} +import scala.util.{Failure, Success, Try} + +import common.validation.Validation._ // We encapsulate this functionality here so that we can easily mock it out, to allow for testing without // actually connecting to Blob storage. @@ -35,13 +36,14 @@ object BlobFileSystemManager { instant = Instant.parse(expiryString) } yield instant - def buildConfigMap(credential: AzureSasCredential, container: BlobContainerName): Map[String, Object] = { - Map((AzureFileSystem.AZURE_STORAGE_SAS_TOKEN_CREDENTIAL, credential), + def buildConfigMap(keyValue: (String, AzureSasCredential), container: BlobContainerName): Map[String, Object] = { + Map((keyValue._1, keyValue._2), (AzureFileSystem.AZURE_STORAGE_FILE_STORES, container.value), (AzureFileSystem.AZURE_STORAGE_SKIP_INITIAL_CONTAINER_CHECK, java.lang.Boolean.TRUE)) } - def hasTokenExpired(tokenExpiry: Instant, buffer: Duration): Boolean = Instant.now.plus(buffer).isAfter(tokenExpiry) - def uri(endpoint: EndpointURL, container: BlobContainerName) = new URI("azb://?endpoint=" + endpoint + "/" + container.value) + def combinedEnpointContainerUri(endpoint: EndpointURL, container: BlobContainerName) = new URI("azb://?endpoint=" + endpoint + "/" + container.value) + + val PLACEHOLDER_TOKEN = new AzureSasCredential("this-is-a-public-sas") } class BlobFileSystemManager(val expiryBufferMinutes: Long, @@ -57,19 +59,21 @@ class BlobFileSystemManager(val expiryBufferMinutes: Long, def this(rawConfig: Config) = this(BlobFileSystemConfig(rawConfig)) - def buffer: Duration = Duration.of(expiryBufferMinutes, ChronoUnit.MINUTES) + val buffer: Duration = Duration.of(expiryBufferMinutes, ChronoUnit.MINUTES) def retrieveFilesystem(endpoint: EndpointURL, container: BlobContainerName): Try[FileSystem] = { - val uri = BlobFileSystemManager.uri(endpoint, container) + val uri = BlobFileSystemManager.combinedEnpointContainerUri(endpoint, container) synchronized { - fileSystemAPI.getFileSystem(uri).recoverWith { + fileSystemAPI.getFileSystem(uri).filter(!_.isExpired(buffer)).recoverWith { // If no filesystem already exists, this will create a new connection, with the provided configs case _: FileSystemNotFoundException => { logger.info(s"Creating new blob filesystem for URI $uri") generateFilesystem(uri, container, endpoint) } - }.filter(!_.isExpired(buffer)).recoverWith { - case _ => { + case _ : NoSuchElementException => { + // When the filesystem expires, the above filter results in a + // NoSuchElementException. If expired, close the filesystem, + // regenerate a new SAS token, and reopen the filesystem with the fresh token logger.info(s"Closing & regenerating token for existing blob filesystem at URI $uri") fileSystemAPI.closeFileSystem(uri) generateFilesystem(uri, container, endpoint) @@ -79,10 +83,29 @@ class BlobFileSystemManager(val expiryBufferMinutes: Long, } private def generateFilesystem(uri: URI, container: BlobContainerName, endpoint: EndpointURL): Try[AzureFileSystem] = { - blobTokenGenerator.generateBlobSasToken(endpoint, container) - .flatMap((token: AzureSasCredential) => { - Try(fileSystemAPI.newFileSystem(uri, BlobFileSystemManager.buildConfigMap(token, container))) - }) + // There are three cases here + // 1. Authorizing and opening a Terra filesystem + // 2. Accessing a public filesystem (including ones outside of Terra) + // 3. Failing to interact with a private non-Terra filesystem + + // This is handled by determining if the container name represents a Terra container, + // and attempting to get a SAS. + // If we cannot get a SAS we will return a Failure with the error + // Lastly if the filesystem is outside of Terra try as if the filesystem is public, + // and if unable to access it this way, we also return a failure with the error + val token = container.workspaceId match { + // This is a Terra blob container, we can try to get a SAS + case Success(_) => + blobTokenGenerator.generateBlobSasToken(endpoint, container) + .map((AzureFileSystem.AZURE_STORAGE_SAS_TOKEN_CREDENTIAL, _)) + // This is not a Terra blob container, if we can access it, it will be publicly accessible + // The blob client requires we supply a token, but it doesn't need to be a valid one since its unused + case Failure(_) => + Try(BlobFileSystemManager.PLACEHOLDER_TOKEN) + .map((AzureFileSystem.AZURE_STORAGE_PUBLIC_ACCESS_CREDENTIAL, _)) + } + // Try to use whatever token we found and open the filesystem. + token.flatMap((credentialKeyValue : (String, AzureSasCredential)) => Try(fileSystemAPI.newFileSystem(uri, BlobFileSystemManager.buildConfigMap(credentialKeyValue, container)))) } } @@ -176,13 +199,12 @@ case class WSMBlobSasTokenGenerator(wsmClientProvider: WorkspaceManagerApiClient for { wsmAuth <- wsmAuthToken wsmAzureResourceClient = wsmClientProvider.getControlledAzureResourceApi(wsmAuth) - sa <- endpoint.storageAccountName - ids <- getWorkspaceAndContainerResourceId(sa, container, wsmAuth) + ids <- getWorkspaceAndContainerResourceId(container, wsmAuth) azureSasToken <- wsmAzureResourceClient.createAzureStorageContainerSasToken(ids._1, ids._2) } yield azureSasToken } - def getWorkspaceAndContainerResourceId(storageAccountName: StorageAccountName, container: BlobContainerName, wsmAuth : String): Try[(UUID, UUID)] = { + def getWorkspaceAndContainerResourceId(container: BlobContainerName, wsmAuth : String): Try[(UUID, UUID)] = { val wsmResourceClient = wsmClientProvider.getResourceApi(wsmAuth) for { workspaceId <- container.workspaceId diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala index a2f71570f21..ba63451e2b7 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala @@ -38,22 +38,10 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga expiry should contain(expiryTime) } - it should "verify an unexpired token will be processed as unexpired" in { - val expiryTime = generateTokenExpiration(11L) - val expired = BlobFileSystemManager.hasTokenExpired(expiryTime, Duration.ofMinutes(10L)) - expired shouldBe false - } - - it should "test an expired token will be processed as expired" in { - val expiryTime = generateTokenExpiration(9L) - val expired = BlobFileSystemManager.hasTokenExpired(expiryTime, Duration.ofMinutes(10L)) - expired shouldBe true - } - it should "test that a filesystem gets closed correctly" in { val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") val container = BlobContainerName("test") - val azureUri = BlobFileSystemManager.uri(endpoint, container) + val azureUri = BlobFileSystemManager.combinedEnpointContainerUri(endpoint, container) val fileSystems = mock[FileSystemAPI] val fileSystem = mock[AzureFileSystem] when(fileSystems.getFileSystem(azureUri)).thenReturn(Try(fileSystem)) @@ -70,7 +58,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) val container = BlobContainerName("storageContainer") val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) - val azureUri = BlobFileSystemManager.uri(endpoint, container) + val azureUri = BlobFileSystemManager.combinedEnpointContainerUri(endpoint, container) //Mocking this final class requires the plugin Mock Maker Inline plugin, configured here //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker @@ -96,7 +84,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) val container = BlobContainerName("storageContainer") val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) - val azureUri = BlobFileSystemManager.uri(endpoint,container) + val azureUri = BlobFileSystemManager.combinedEnpointContainerUri(endpoint,container) //Mocking this final class requires the plugin Mock Maker Inline plugin, configured here //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker @@ -122,7 +110,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) val container = BlobContainerName("storageContainer") val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) - val azureUri = BlobFileSystemManager.uri(endpoint, container) + val azureUri = BlobFileSystemManager.combinedEnpointContainerUri(endpoint, container) //Mocking this final class requires the plugin Mock Maker Inline plugin, configured here //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker From 88ecd81e3bcf50bd69bc5248c6e42db11f91501e Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Tue, 18 Jul 2023 13:29:27 -0400 Subject: [PATCH 04/36] Modify SAS generation to account for public fs --- .../blob/BlobFileSystemManager.scala | 58 +++++-------- .../blob/BlobPathBuilderFactorySpec.scala | 87 +++++++++++++++++-- 2 files changed, 105 insertions(+), 40 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 eeabdd0762e..02ced8d62f6 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala @@ -36,10 +36,14 @@ object BlobFileSystemManager { instant = Instant.parse(expiryString) } yield instant - def buildConfigMap(keyValue: (String, AzureSasCredential), container: BlobContainerName): Map[String, Object] = { - Map((keyValue._1, keyValue._2), - (AzureFileSystem.AZURE_STORAGE_FILE_STORES, container.value), - (AzureFileSystem.AZURE_STORAGE_SKIP_INITIAL_CONTAINER_CHECK, java.lang.Boolean.TRUE)) + def buildConfigMap(credential: AzureSasCredential, container: BlobContainerName): Map[String, Object] = { + val sasTuple = if (credential == PLACEHOLDER_TOKEN) { + (AzureFileSystem.AZURE_STORAGE_PUBLIC_ACCESS_CREDENTIAL, PLACEHOLDER_TOKEN) + } else { + (AzureFileSystem.AZURE_STORAGE_SAS_TOKEN_CREDENTIAL, credential) + } + Map(sasTuple, (AzureFileSystem.AZURE_STORAGE_FILE_STORES, container.value), + (AzureFileSystem.AZURE_STORAGE_SKIP_INITIAL_CONTAINER_CHECK, java.lang.Boolean.TRUE)) } def combinedEnpointContainerUri(endpoint: EndpointURL, container: BlobContainerName) = new URI("azb://?endpoint=" + endpoint + "/" + container.value) @@ -83,29 +87,10 @@ class BlobFileSystemManager(val expiryBufferMinutes: Long, } private def generateFilesystem(uri: URI, container: BlobContainerName, endpoint: EndpointURL): Try[AzureFileSystem] = { - // There are three cases here - // 1. Authorizing and opening a Terra filesystem - // 2. Accessing a public filesystem (including ones outside of Terra) - // 3. Failing to interact with a private non-Terra filesystem - - // This is handled by determining if the container name represents a Terra container, - // and attempting to get a SAS. - // If we cannot get a SAS we will return a Failure with the error - // Lastly if the filesystem is outside of Terra try as if the filesystem is public, - // and if unable to access it this way, we also return a failure with the error - val token = container.workspaceId match { - // This is a Terra blob container, we can try to get a SAS - case Success(_) => - blobTokenGenerator.generateBlobSasToken(endpoint, container) - .map((AzureFileSystem.AZURE_STORAGE_SAS_TOKEN_CREDENTIAL, _)) - // This is not a Terra blob container, if we can access it, it will be publicly accessible - // The blob client requires we supply a token, but it doesn't need to be a valid one since its unused - case Failure(_) => - Try(BlobFileSystemManager.PLACEHOLDER_TOKEN) - .map((AzureFileSystem.AZURE_STORAGE_PUBLIC_ACCESS_CREDENTIAL, _)) - } - // Try to use whatever token we found and open the filesystem. - token.flatMap((credentialKeyValue : (String, AzureSasCredential)) => Try(fileSystemAPI.newFileSystem(uri, BlobFileSystemManager.buildConfigMap(credentialKeyValue, container)))) + blobTokenGenerator.generateBlobSasToken(endpoint, container) + .flatMap((token: AzureSasCredential) => { + Try(fileSystemAPI.newFileSystem(uri, BlobFileSystemManager.buildConfigMap(token, container))) + }) } } @@ -195,13 +180,18 @@ case class WSMBlobSasTokenGenerator(wsmClientProvider: WorkspaceManagerApiClient case Some(t) => Success(t) case None => AzureCredentials.getAccessToken(None).toTry } - - for { - wsmAuth <- wsmAuthToken - wsmAzureResourceClient = wsmClientProvider.getControlledAzureResourceApi(wsmAuth) - ids <- getWorkspaceAndContainerResourceId(container, wsmAuth) - azureSasToken <- wsmAzureResourceClient.createAzureStorageContainerSasToken(ids._1, ids._2) - } yield azureSasToken + container.workspaceId match { + // If this is a Terra workspace, request a token from WSM + case Success(_) => for { + wsmAuth <- wsmAuthToken + wsmAzureResourceClient = wsmClientProvider.getControlledAzureResourceApi(wsmAuth) + ids <- getWorkspaceAndContainerResourceId(container, wsmAuth) + azureSasToken <- wsmAzureResourceClient.createAzureStorageContainerSasToken(ids._1, ids._2) + } yield azureSasToken + // Otherwise assume that the container is public and use a placeholder + // SAS token to bypass the BlobClient authentication requirement + case Failure(_) => Try(BlobFileSystemManager.PLACEHOLDER_TOKEN) + } } def getWorkspaceAndContainerResourceId(container: BlobContainerName, wsmAuth : String): Try[(UUID, UUID)] = { diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala index ba63451e2b7..7683afde3b7 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala @@ -11,6 +11,7 @@ import java.nio.file.FileSystemNotFoundException import java.time.format.DateTimeFormatter import java.time.temporal.ChronoUnit import java.time.{Duration, Instant, ZoneId} +import java.util.UUID import scala.util.{Failure, Success, Try} @@ -51,12 +52,12 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga verify(fileSystem, times(1)).close() } - it should "test retrieveFileSystem with expired filesystem" in { + it should "test retrieveFileSystem with expired Terra filesystem" in { val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") //val expiredToken = generateTokenExpiration(9L) val refreshedToken = generateTokenExpiration(69L) val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) - val container = BlobContainerName("storageContainer") + val container = BlobContainerName("sc-" + UUID.randomUUID().toString()) val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) val azureUri = BlobFileSystemManager.combinedEnpointContainerUri(endpoint, container) @@ -77,12 +78,12 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga verify(fileSystems, times(1)).closeFileSystem(azureUri) } - it should "test retrieveFileSystem with an unexpired fileSystem" in { + it should "test retrieveFileSystem with an unexpired Terra fileSystem" in { val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") //val initialToken = generateTokenExpiration(11L) val refreshedToken = generateTokenExpiration(71L) val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) - val container = BlobContainerName("storageContainer") + val container = BlobContainerName("sc-" + UUID.randomUUID().toString()) val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) val azureUri = BlobFileSystemManager.combinedEnpointContainerUri(endpoint,container) @@ -104,11 +105,85 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga verify(fileSystems, never()).closeFileSystem(azureUri) } - it should "test retrieveFileSystem with an uninitialized filesystem" in { + it should "test retrieveFileSystem with an uninitialized Terra filesystem" in { val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") val refreshedToken = generateTokenExpiration(71L) val sasToken = BlobPathBuilderFactorySpec.buildExampleSasToken(refreshedToken) - val container = BlobContainerName("storageContainer") + val container = BlobContainerName("sc-" + UUID.randomUUID().toString()) + val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) + val azureUri = BlobFileSystemManager.combinedEnpointContainerUri(endpoint, container) + + //Mocking this final class requires the plugin Mock Maker Inline plugin, configured here + //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker + val azureFileSystem = mock[AzureFileSystem] + when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(false) + val fileSystems = mock[FileSystemAPI] + when(fileSystems.getFileSystem(azureUri)).thenReturn(Failure(new FileSystemNotFoundException)) + when(fileSystems.newFileSystem(azureUri, configMap)).thenReturn(azureFileSystem) + val blobTokenGenerator = mock[BlobSasTokenGenerator] + when(blobTokenGenerator.generateBlobSasToken(endpoint, container)).thenReturn(Try(sasToken)) + + val fsm = new BlobFileSystemManager(0L, blobTokenGenerator, fileSystems) + fsm.retrieveFilesystem(endpoint, container) + + verify(fileSystems, times(1)).getFileSystem(azureUri) + verify(fileSystems, times(1)).newFileSystem(azureUri, configMap) + verify(fileSystems, never()).closeFileSystem(azureUri) + } + + it should "test retrieveFileSystem with expired non-Terra filesystem" in { + val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") + val sasToken = BlobFileSystemManager.PLACEHOLDER_TOKEN + val container = BlobContainerName("sc-" + UUID.randomUUID().toString()) + val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) + val azureUri = BlobFileSystemManager.combinedEnpointContainerUri(endpoint, container) + + //Mocking this final class requires the plugin Mock Maker Inline plugin, configured here + //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker + val azureFileSystem = mock[AzureFileSystem] + when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(true) + val fileSystems = mock[FileSystemAPI] + when(fileSystems.getFileSystem(azureUri)).thenReturn(Success(azureFileSystem)) + val blobTokenGenerator = mock[BlobSasTokenGenerator] + when(blobTokenGenerator.generateBlobSasToken(endpoint, container)).thenReturn(Try(sasToken)) + + val fsm = new BlobFileSystemManager(10L, blobTokenGenerator, fileSystems) + fsm.retrieveFilesystem(endpoint, container) + + verify(fileSystems, times(1)).getFileSystem(azureUri) + verify(fileSystems, times(1)).newFileSystem(azureUri, configMap) + verify(fileSystems, times(1)).closeFileSystem(azureUri) + } + + it should "test retrieveFileSystem with an unexpired non-Terra fileSystem" in { + val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") + val sasToken = BlobFileSystemManager.PLACEHOLDER_TOKEN + val container = BlobContainerName("sc-" + UUID.randomUUID().toString()) + val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) + val azureUri = BlobFileSystemManager.combinedEnpointContainerUri(endpoint,container) + + //Mocking this final class requires the plugin Mock Maker Inline plugin, configured here + //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker + val azureFileSystem = mock[AzureFileSystem] + when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(false) + val fileSystems = mock[FileSystemAPI] + when(fileSystems.getFileSystem(azureUri)).thenReturn(Try(azureFileSystem)) + + val blobTokenGenerator = mock[BlobSasTokenGenerator] + when(blobTokenGenerator.generateBlobSasToken(endpoint, container)).thenReturn(Try(sasToken)) + + val fsm = new BlobFileSystemManager(10L, blobTokenGenerator, fileSystems) + fsm.retrieveFilesystem(endpoint, container) + + verify(fileSystems, times(1)).getFileSystem(azureUri) + verify(fileSystems, never()).newFileSystem(azureUri, configMap) + verify(fileSystems, never()).closeFileSystem(azureUri) + } + + it should "test retrieveFileSystem with an uninitialized non-Terra filesystem" in { + val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") + val sasToken = BlobFileSystemManager.PLACEHOLDER_TOKEN + val container = BlobContainerName("sc-" + UUID.randomUUID().toString()) val configMap = BlobFileSystemManager.buildConfigMap(sasToken, container) val azureUri = BlobFileSystemManager.combinedEnpointContainerUri(endpoint, container) From 073728875769e86b0aa518c1eb181da098ef8d90 Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Tue, 18 Jul 2023 14:55:53 -0400 Subject: [PATCH 05/36] Update some comments --- .../blob/BlobFileSystemManager.scala | 25 +++++++++++++------ 1 file changed, 18 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 02ced8d62f6..7204918359f 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala @@ -5,6 +5,7 @@ import com.azure.storage.blob.nio.AzureFileSystem import com.azure.storage.blob.sas.{BlobContainerSasPermission, BlobServiceSasSignatureValues} import com.typesafe.config.Config import com.typesafe.scalalogging.LazyLogging +import common.validation.Validation._ import cromwell.cloudsupport.azure.AzureUtils import java.net.URI @@ -15,8 +16,6 @@ import java.util.UUID import scala.jdk.CollectionConverters._ import scala.util.{Failure, Success, Try} -import common.validation.Validation._ - // We encapsulate this functionality here so that we can easily mock it out, to allow for testing without // actually connecting to Blob storage. case class FileSystemAPI() { @@ -37,6 +36,8 @@ object BlobFileSystemManager { } yield instant def buildConfigMap(credential: AzureSasCredential, container: BlobContainerName): Map[String, Object] = { + // Special handling is done here to provide a special key value pair if the placeholder token is provided + // This is due to the BlobClient requiring an auth token even for public blob paths. val sasTuple = if (credential == PLACEHOLDER_TOKEN) { (AzureFileSystem.AZURE_STORAGE_PUBLIC_ACCESS_CREDENTIAL, PLACEHOLDER_TOKEN) } else { @@ -76,8 +77,8 @@ class BlobFileSystemManager(val expiryBufferMinutes: Long, } case _ : NoSuchElementException => { // When the filesystem expires, the above filter results in a - // NoSuchElementException. If expired, close the filesystem, - // regenerate a new SAS token, and reopen the filesystem with the fresh token + // NoSuchElementException. If expired, close the filesystem + // and reopen the filesystem with the fresh token logger.info(s"Closing & regenerating token for existing blob filesystem at URI $uri") fileSystemAPI.closeFileSystem(uri) generateFilesystem(uri, container, endpoint) @@ -86,6 +87,15 @@ class BlobFileSystemManager(val expiryBufferMinutes: Long, } } + /** + * Create a new filesystem pointing to a particular container and storage account, + * generating a SAS token from WSM as needed + * + * @param uri a URI formatted to include the scheme, storage account endpoint and container + * @param container the container to open as a filesystem + * @param endpoint the endpoint containing the storage account for the container to open + * @return a try with either the successfully created filesystem, or a failure containing the exception + */ private def generateFilesystem(uri: URI, container: BlobContainerName, endpoint: EndpointURL): Try[AzureFileSystem] = { blobTokenGenerator.generateBlobSasToken(endpoint, container) .flatMap((token: AzureSasCredential) => { @@ -134,9 +144,6 @@ object BlobSasTokenGenerator { * Native SAS token generator, uses the DefaultAzureCredentialBuilder in the local environment * to produce a SAS token. * - * @param container The BlobContainerName of the blob container to be accessed by the generated SAS token - * @param endpoint The EndpointURL containing the storage account of the blob container to be accessed by - * this SAS token * @param subscription Optional subscription parameter to use for local authorization. * If one is not provided the default subscription is used * @return A NativeBlobTokenGenerator, able to produce a valid SAS token for accessing the provided blob @@ -172,6 +179,8 @@ case class WSMBlobSasTokenGenerator(wsmClientProvider: WorkspaceManagerApiClient * Generate a BlobSasToken by using the available authorization information * If an overrideWsmAuthToken is provided, use this in the wsmClient request * Else try to use the environment azure identity to request the SAS token + * @param endpoint The EndpointURL of the blob container to be accessed by the generated SAS token + * @param container The BlobContainerName of the blob container to be accessed by the generated SAS token * * @return an AzureSasCredential for accessing a blob container */ @@ -213,6 +222,8 @@ case class NativeBlobSasTokenGenerator(subscription: Option[SubscriptionId] = No /** * Generate a BlobSasToken by using the local environment azure identity * This will use a default subscription if one is not provided. + * @param endpoint The EndpointURL of the blob container to be accessed by the generated SAS token + * @param container The BlobContainerName of the blob container to be accessed by the generated SAS token * * @return an AzureSasCredential for accessing a blob container */ From dcb491207990ba2d0f2640b4d9ac8e3dee8c4333 Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Tue, 18 Jul 2023 15:13:22 -0400 Subject: [PATCH 06/36] Modify the NIO library to assume a single store in a filesystem --- .../blob/nio/AzureDirectoryStream.java | 14 ++++---- .../storage/blob/nio/AzureFileSystem.java | 36 ++++++++----------- .../com/azure/storage/blob/nio/AzurePath.java | 2 +- .../blob/BlobPathBuilderSpec.scala | 12 +++---- 4 files changed, 28 insertions(+), 36 deletions(-) diff --git a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureDirectoryStream.java b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureDirectoryStream.java index 917f712ddfc..817121e958e 100644 --- a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureDirectoryStream.java +++ b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureDirectoryStream.java @@ -3,12 +3,6 @@ package com.azure.storage.blob.nio; -import com.azure.core.util.logging.ClientLogger; -import com.azure.storage.blob.BlobContainerClient; -import com.azure.storage.blob.models.BlobItem; -import com.azure.storage.blob.models.BlobListDetails; -import com.azure.storage.blob.models.ListBlobsOptions; - import java.io.IOException; import java.nio.file.DirectoryIteratorException; import java.nio.file.DirectoryStream; @@ -18,6 +12,12 @@ import java.util.NoSuchElementException; import java.util.Set; +import com.azure.core.util.logging.ClientLogger; +import com.azure.storage.blob.BlobContainerClient; +import com.azure.storage.blob.models.BlobItem; +import com.azure.storage.blob.models.BlobListDetails; +import com.azure.storage.blob.models.ListBlobsOptions; + /** * A type for iterating over the contents of a directory. * @@ -88,7 +88,7 @@ private static class AzureDirectoryIterator implements Iterator { if (path.isRoot()) { String containerName = path.toString().substring(0, path.toString().length() - 1); AzureFileSystem afs = ((AzureFileSystem) path.getFileSystem()); - containerClient = ((AzureFileStore) afs.getFileStore(containerName)).getContainerClient(); + containerClient = ((AzureFileStore) afs.getFileStore()).getContainerClient(); } else { AzureResource azureResource = new AzureResource(path); listOptions.setPrefix(azureResource.getBlobClient().getBlobName() + AzureFileSystem.PATH_SEPARATOR); diff --git a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java index b1b27ccac1a..8c3fac74602 100644 --- a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java +++ b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java @@ -26,7 +26,6 @@ import java.util.Optional; import java.util.Set; import java.util.regex.PatternSyntaxException; -import java.util.stream.Collectors; import com.azure.core.credential.AzureSasCredential; import com.azure.core.http.HttpClient; @@ -169,7 +168,6 @@ public final class AzureFileSystem extends FileSystem { private final Long putBlobThreshold; private final Integer maxConcurrencyPerRequest; private final Integer downloadResumeRetries; - private final Map fileStores; private FileStore defaultFileStore; private boolean closed; private Instant expiry; @@ -192,7 +190,7 @@ public final class AzureFileSystem extends FileSystem { this.downloadResumeRetries = (Integer) config.get(AZURE_STORAGE_DOWNLOAD_RESUME_RETRIES); // Initialize and ensure access to FileStores. - this.fileStores = this.initializeFileStores(config); + this.defaultFileStore = this.initializeFileStore(config); } catch (RuntimeException e) { throw LoggingUtility.logError(LOGGER, new IllegalArgumentException("There was an error parsing the " + "configurations map. Please ensure all fields are set to a legal value of the correct type.", e)); @@ -293,9 +291,7 @@ public Iterable getRootDirectories() { If the file system was set to use all containers in the account, the account will be re-queried and the list may grow or shrink if containers were added or deleted. */ - return fileStores.keySet().stream() - .map(name -> this.getPath(name + AzurePath.ROOT_DIR_SUFFIX)) - .collect(Collectors.toList()); + return Arrays.asList(this.getPath(defaultFileStore.name() + AzurePath.ROOT_DIR_SUFFIX)); } /** @@ -315,7 +311,7 @@ public Iterable getFileStores() { If the file system was set to use all containers in the account, the account will be re-queried and the list may grow or shrink if containers were added or deleted. */ - return this.fileStores.values(); + return Arrays.asList(defaultFileStore); } /** @@ -447,23 +443,20 @@ private BlobServiceClient buildBlobServiceClient(String endpoint, Map return builder.buildClient(); } - private Map initializeFileStores(Map config) throws IOException { - String fileStoreNames = (String) config.get(AZURE_STORAGE_FILE_STORES); - if (CoreUtils.isNullOrEmpty(fileStoreNames)) { + private FileStore initializeFileStore(Map config) throws IOException { + String fileStoreName = (String) config.get(AZURE_STORAGE_FILE_STORES); + if (CoreUtils.isNullOrEmpty(fileStoreName)) { throw LoggingUtility.logError(LOGGER, new IllegalArgumentException("The list of FileStores cannot be " + "null.")); } Boolean skipConnectionCheck = (Boolean) config.get(AZURE_STORAGE_SKIP_INITIAL_CONTAINER_CHECK); Map fileStores = new HashMap<>(); - for (String fileStoreName : fileStoreNames.split(",")) { - FileStore fs = new AzureFileStore(this, fileStoreName, skipConnectionCheck); - if (this.defaultFileStore == null) { - this.defaultFileStore = fs; - } - fileStores.put(fileStoreName, fs); + FileStore fs = new AzureFileStore(this, fileStoreName, skipConnectionCheck); + if (this.defaultFileStore == null) { + this.defaultFileStore = fs; } - return fileStores; + return this.defaultFileStore; } @Override @@ -487,12 +480,11 @@ Path getDefaultDirectory() { return this.getPath(this.defaultFileStore.name() + AzurePath.ROOT_DIR_SUFFIX); } - FileStore getFileStore(String name) throws IOException { - FileStore store = this.fileStores.get(name); - if (store == null) { - throw LoggingUtility.logError(LOGGER, new IOException("Invalid file store: " + name)); + FileStore getFileStore() throws IOException { + if (this.defaultFileStore == null) { + throw LoggingUtility.logError(LOGGER, new IOException("FileStore not initialized")); } - return store; + return defaultFileStore; } Long getBlockSize() { diff --git a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzurePath.java b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzurePath.java index 9742af1f696..917895ba39e 100644 --- a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzurePath.java +++ b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzurePath.java @@ -735,7 +735,7 @@ public BlobClient toBlobClient() throws IOException { String fileStoreName = this.rootToFileStore(root.toString()); BlobContainerClient containerClient = - ((AzureFileStore) this.parentFileSystem.getFileStore(fileStoreName)).getContainerClient(); + ((AzureFileStore) this.parentFileSystem.getFileStore()).getContainerClient(); String blobName = this.withoutRoot(); if (blobName.isEmpty()) { 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 c9a97d00baa..e67e86570e3 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala @@ -82,7 +82,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { new BlobPathBuilder()(fsm) } - it should "resolve an absolute path string correctly to a path" in { + ignore should "resolve an absolute path string correctly to a path" in { val builder = makeBlobPathBuilder(endpoint, store) val rootString = s"${endpoint.value}/${store.value}/cromwell-execution" val blobRoot: BlobPath = builder build rootString getOrElse fail() @@ -91,7 +91,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { otherFile.toAbsolutePath.pathAsString should equal ("https://coaexternalstorage.blob.core.windows.net/inputs/cromwell-execution/test/inputFile.txt") } - it should "build a blob path from a test string and read a file" in { + ignore should "build a blob path from a test string and read a file" in { val builder = makeBlobPathBuilder(endpoint, store) val endpointHost = BlobPathBuilder.parseURI(endpoint.value).map(_.getHost).getOrElse(fail("Could not parse URI")) val evalPath = "/test/inputFile.txt" @@ -107,7 +107,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { fileText should include ("This is my test file!!!! Did it work?") } - it should "build duplicate blob paths in the same filesystem" in { + ignore should "build duplicate blob paths in the same filesystem" in { val builder = makeBlobPathBuilder(endpoint, store) val evalPath = "/test/inputFile.txt" val testString = endpoint.value + "/" + store + evalPath @@ -120,7 +120,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { fileText should include ("This is my test file!!!! Did it work?") } - it should "resolve a path without duplicating container name" in { + ignore should "resolve a path without duplicating container name" in { val builder = makeBlobPathBuilder(endpoint, store) val rootString = s"${endpoint.value}/${store.value}/cromwell-execution" val blobRoot: BlobPath = builder build rootString getOrElse fail() @@ -129,7 +129,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { otherFile.toAbsolutePath.pathAsString should equal ("https://coaexternalstorage.blob.core.windows.net/inputs/cromwell-execution/test/inputFile.txt") } - it should "correctly remove a prefix from the blob path" in { + ignore should "correctly remove a prefix from the blob path" in { val builder = makeBlobPathBuilder(endpoint, store) val rootString = s"${endpoint.value}/${store.value}/cromwell-execution/" val execDirString = s"${endpoint.value}/${store.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/" @@ -142,7 +142,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { blobFile.pathStringWithoutPrefix(blobFile) should equal ("") } - it should "not change a path if it doesn't start with a prefix" in { + ignore should "not change a path if it doesn't start with a prefix" in { val builder = makeBlobPathBuilder(endpoint, store) val otherRootString = s"${endpoint.value}/${store.value}/foobar/" val fileString = s"${endpoint.value}/${store.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/stdout" From bbf0d01857eeafc7bc70906f5b22217996f69cb4 Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Tue, 18 Jul 2023 15:16:07 -0400 Subject: [PATCH 07/36] Cleanup single store changes --- .../java/com/azure/storage/blob/nio/AzureFileSystem.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java index 8c3fac74602..381ed0289d7 100644 --- a/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java +++ b/azure-blob-nio/src/main/java/com/azure/storage/blob/nio/AzureFileSystem.java @@ -452,10 +452,7 @@ private FileStore initializeFileStore(Map config) throws IOException Boolean skipConnectionCheck = (Boolean) config.get(AZURE_STORAGE_SKIP_INITIAL_CONTAINER_CHECK); Map fileStores = new HashMap<>(); - FileStore fs = new AzureFileStore(this, fileStoreName, skipConnectionCheck); - if (this.defaultFileStore == null) { - this.defaultFileStore = fs; - } + this.defaultFileStore = new AzureFileStore(this, fileStoreName, skipConnectionCheck); return this.defaultFileStore; } From 4962cbfa4adc002c6e2bd4c18ee3f8f327ad5745 Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Wed, 19 Jul 2023 15:43:36 -0400 Subject: [PATCH 08/36] Fix bug with public filesystems that fit the 'Terra' pattern --- .../blob/BlobFileSystemManager.scala | 26 ++++++++++++------- .../WorkspaceManagerApiClientProvider.scala | 5 ++-- 2 files changed, 19 insertions(+), 12 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 7204918359f..318d9290bc3 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala @@ -1,5 +1,6 @@ package cromwell.filesystems.blob +import bio.terra.workspace.client.ApiException import com.azure.core.credential.AzureSasCredential import com.azure.storage.blob.nio.AzureFileSystem import com.azure.storage.blob.sas.{BlobContainerSasPermission, BlobServiceSasSignatureValues} @@ -191,24 +192,31 @@ case class WSMBlobSasTokenGenerator(wsmClientProvider: WorkspaceManagerApiClient } container.workspaceId match { // If this is a Terra workspace, request a token from WSM - case Success(_) => for { - wsmAuth <- wsmAuthToken - wsmAzureResourceClient = wsmClientProvider.getControlledAzureResourceApi(wsmAuth) - ids <- getWorkspaceAndContainerResourceId(container, wsmAuth) - azureSasToken <- wsmAzureResourceClient.createAzureStorageContainerSasToken(ids._1, ids._2) - } yield azureSasToken + case Success(workspaceId) => { + val wsmSasToken = for { + wsmAuth <- wsmAuthToken + wsmAzureResourceClient = wsmClientProvider.getControlledAzureResourceApi(wsmAuth) + resourceId <- getContainerResourceId(workspaceId, container, wsmAuth) + sasToken <- wsmAzureResourceClient.createAzureStorageContainerSasToken(workspaceId, resourceId) + } yield sasToken + wsmSasToken match { + case Success(value) => Success(value) + // If the storage account was still not found in WSM, this may be a public filesystem + case Failure(exception: ApiException) if exception.getCode == 404 => Try(BlobFileSystemManager.PLACEHOLDER_TOKEN) + case Failure(exception) => Failure(exception) + } + } // Otherwise assume that the container is public and use a placeholder // SAS token to bypass the BlobClient authentication requirement case Failure(_) => Try(BlobFileSystemManager.PLACEHOLDER_TOKEN) } } - def getWorkspaceAndContainerResourceId(container: BlobContainerName, wsmAuth : String): Try[(UUID, UUID)] = { + def getContainerResourceId(workspaceId: UUID, container: BlobContainerName, wsmAuth : String): Try[UUID] = { val wsmResourceClient = wsmClientProvider.getResourceApi(wsmAuth) for { - workspaceId <- container.workspaceId resourceId <- wsmResourceClient.findContainerResourceId(workspaceId, container) - } yield (workspaceId, resourceId) + } yield resourceId } } diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/WorkspaceManagerApiClientProvider.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/WorkspaceManagerApiClientProvider.scala index 1db09d1d0cd..fb1475f03be 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/WorkspaceManagerApiClientProvider.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/WorkspaceManagerApiClientProvider.scala @@ -2,13 +2,12 @@ package cromwell.filesystems.blob import bio.terra.workspace.api._ import bio.terra.workspace.client.ApiClient +import bio.terra.workspace.model.{ResourceType, StewardshipType} import com.azure.core.credential.AzureSasCredential import java.util.UUID -import scala.util.Try import scala.jdk.CollectionConverters._ -import bio.terra.workspace.model.ResourceType -import bio.terra.workspace.model.StewardshipType +import scala.util.Try /** * Represents a way to get a client for interacting with workspace manager controlled resources. From 5dc6ba9a6f4b2bb4353aa035163a7eaa40405476 Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Wed, 19 Jul 2023 16:18:26 -0400 Subject: [PATCH 09/36] Clean up --- .../blob/BlobFileSystemManager.scala | 26 ++++++------------- 1 file changed, 8 insertions(+), 18 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 318d9290bc3..2f85e91fac8 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala @@ -39,11 +39,9 @@ object BlobFileSystemManager { def buildConfigMap(credential: AzureSasCredential, container: BlobContainerName): Map[String, Object] = { // Special handling is done here to provide a special key value pair if the placeholder token is provided // This is due to the BlobClient requiring an auth token even for public blob paths. - val sasTuple = if (credential == PLACEHOLDER_TOKEN) { - (AzureFileSystem.AZURE_STORAGE_PUBLIC_ACCESS_CREDENTIAL, PLACEHOLDER_TOKEN) - } else { - (AzureFileSystem.AZURE_STORAGE_SAS_TOKEN_CREDENTIAL, credential) - } + val sasTuple = if (credential == PLACEHOLDER_TOKEN) (AzureFileSystem.AZURE_STORAGE_PUBLIC_ACCESS_CREDENTIAL, PLACEHOLDER_TOKEN) + else (AzureFileSystem.AZURE_STORAGE_SAS_TOKEN_CREDENTIAL, credential) + Map(sasTuple, (AzureFileSystem.AZURE_STORAGE_FILE_STORES, container.value), (AzureFileSystem.AZURE_STORAGE_SKIP_INITIAL_CONTAINER_CHECK, java.lang.Boolean.TRUE)) } @@ -132,10 +130,7 @@ object BlobSasTokenGenerator { // WSM-mediated mediated SAS token generator // parameterizing client instead of URL to make injecting mock client possible - BlobSasTokenGenerator.createBlobTokenGenerator( - wsmClient, - wsmConfig.overrideWsmAuthToken - ) + BlobSasTokenGenerator.createBlobTokenGenerator(wsmClient, wsmConfig.overrideWsmAuthToken) }.getOrElse( // Native SAS token generator BlobSasTokenGenerator.createBlobTokenGenerator(config.subscriptionId) @@ -193,17 +188,14 @@ case class WSMBlobSasTokenGenerator(wsmClientProvider: WorkspaceManagerApiClient container.workspaceId match { // If this is a Terra workspace, request a token from WSM case Success(workspaceId) => { - val wsmSasToken = for { + (for { wsmAuth <- wsmAuthToken wsmAzureResourceClient = wsmClientProvider.getControlledAzureResourceApi(wsmAuth) resourceId <- getContainerResourceId(workspaceId, container, wsmAuth) sasToken <- wsmAzureResourceClient.createAzureStorageContainerSasToken(workspaceId, resourceId) - } yield sasToken - wsmSasToken match { - case Success(value) => Success(value) + } yield sasToken).recoverWith { // If the storage account was still not found in WSM, this may be a public filesystem - case Failure(exception: ApiException) if exception.getCode == 404 => Try(BlobFileSystemManager.PLACEHOLDER_TOKEN) - case Failure(exception) => Failure(exception) + case exception: ApiException if exception.getCode == 404 => Try(BlobFileSystemManager.PLACEHOLDER_TOKEN) } } // Otherwise assume that the container is public and use a placeholder @@ -214,9 +206,7 @@ case class WSMBlobSasTokenGenerator(wsmClientProvider: WorkspaceManagerApiClient def getContainerResourceId(workspaceId: UUID, container: BlobContainerName, wsmAuth : String): Try[UUID] = { val wsmResourceClient = wsmClientProvider.getResourceApi(wsmAuth) - for { - resourceId <- wsmResourceClient.findContainerResourceId(workspaceId, container) - } yield resourceId + wsmResourceClient.findContainerResourceId(workspaceId, container) } } From cd83f2973ffaa1a524ea4a5a4c8863e0a3d78e08 Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Thu, 20 Jul 2023 09:59:26 -0400 Subject: [PATCH 10/36] Remove tenant --- .../src/main/scala/cromwell/cloudsupport/azure/AzureUtils.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/cloudSupport/src/main/scala/cromwell/cloudsupport/azure/AzureUtils.scala b/cloudSupport/src/main/scala/cromwell/cloudsupport/azure/AzureUtils.scala index bdc6cd10d00..09cf5f3869d 100644 --- a/cloudSupport/src/main/scala/cromwell/cloudsupport/azure/AzureUtils.scala +++ b/cloudSupport/src/main/scala/cromwell/cloudsupport/azure/AzureUtils.scala @@ -29,7 +29,6 @@ object AzureUtils { val azureProfile = new AzureProfile(AzureEnvironment.AZURE) def azureCredentialBuilder = new DefaultAzureCredentialBuilder() - .tenantId("fad90753-2022-4456-9b0a-c7e5b934e408") .authorityHost(azureProfile.getEnvironment.getActiveDirectoryEndpoint) .build From 3de31fdbf5bb3d3a041ff340596a64f8d625c8e0 Mon Sep 17 00:00:00 2001 From: Christian Freitas Date: Thu, 20 Jul 2023 15:53:31 -0400 Subject: [PATCH 11/36] Add initial AzureFileSystem spec --- .../blob/AzureFileSystemSpec.scala | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala new file mode 100644 index 00000000000..5530e4e6078 --- /dev/null +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala @@ -0,0 +1,25 @@ +package com.azure.storage.blob.nio; + +import cromwell.filesystems.blob._ +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.nio.file.spi.FileSystemProvider +import java.time.Instant +import scala.compat.java8.OptionConverters._ +import scala.jdk.CollectionConverters._ + +class AzureFileSystemSpec extends AnyFlatSpec with Matchers { + val now = Instant.now() + val container = BlobContainerName("testConainer") + val exampleSas = BlobPathBuilderFactorySpec.buildExampleSasToken(now) + val exampleConfig = BlobFileSystemManager.buildConfigMap(exampleSas, container) + val exampleEndpoint = BlobPathBuilderSpec.buildEndpoint("testStorageAccount") + val fileSystemProvider = FileSystemProvider.installedProviders().asScala.find(p => p.getScheme() == "azb") + it should "parse an expiration from a sas token" in { + val fs = fileSystemProvider.map(p => new AzureFileSystem(p.asInstanceOf[AzureFileSystemProvider], exampleEndpoint.value, exampleConfig.asJava)) + fs.nonEmpty shouldBe(true) + fs.flatMap(_.getExpiry().asScala) shouldBe(Some(now)) + fs.map(_.getFileStore().name()) shouldBe(Some(container.value)) + } +} From 5121e6ebe5d65e72178f812e196173151a718889 Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Tue, 25 Jul 2023 11:01:04 -0400 Subject: [PATCH 12/36] Test debugging --- .../scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala index 5530e4e6078..f5f25a3071f 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala @@ -17,6 +17,7 @@ class AzureFileSystemSpec extends AnyFlatSpec with Matchers { val exampleEndpoint = BlobPathBuilderSpec.buildEndpoint("testStorageAccount") val fileSystemProvider = FileSystemProvider.installedProviders().asScala.find(p => p.getScheme() == "azb") it should "parse an expiration from a sas token" in { + fileSystemProvider.nonEmpty shouldBe(true) val fs = fileSystemProvider.map(p => new AzureFileSystem(p.asInstanceOf[AzureFileSystemProvider], exampleEndpoint.value, exampleConfig.asJava)) fs.nonEmpty shouldBe(true) fs.flatMap(_.getExpiry().asScala) shouldBe(Some(now)) From 342c95602da7ce2554fc72fd7d1128b915f95aa4 Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Tue, 25 Jul 2023 13:45:42 -0400 Subject: [PATCH 13/36] Test debugging --- .../scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala index f5f25a3071f..019f571ec0c 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala @@ -18,6 +18,7 @@ class AzureFileSystemSpec extends AnyFlatSpec with Matchers { val fileSystemProvider = FileSystemProvider.installedProviders().asScala.find(p => p.getScheme() == "azb") it should "parse an expiration from a sas token" in { fileSystemProvider.nonEmpty shouldBe(true) + FileSystemProvider.installedProviders().asScala.map(_.getScheme) shouldBe List("azb") val fs = fileSystemProvider.map(p => new AzureFileSystem(p.asInstanceOf[AzureFileSystemProvider], exampleEndpoint.value, exampleConfig.asJava)) fs.nonEmpty shouldBe(true) fs.flatMap(_.getExpiry().asScala) shouldBe(Some(now)) From c884b3d6405f7427e529c16c24145be6510371b3 Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Tue, 25 Jul 2023 14:46:49 -0400 Subject: [PATCH 14/36] Test debugging --- .../scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala index 019f571ec0c..3515db9ecb8 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala @@ -17,7 +17,7 @@ class AzureFileSystemSpec extends AnyFlatSpec with Matchers { val exampleEndpoint = BlobPathBuilderSpec.buildEndpoint("testStorageAccount") val fileSystemProvider = FileSystemProvider.installedProviders().asScala.find(p => p.getScheme() == "azb") it should "parse an expiration from a sas token" in { - fileSystemProvider.nonEmpty shouldBe(true) + // fileSystemProvider.nonEmpty shouldBe(true) FileSystemProvider.installedProviders().asScala.map(_.getScheme) shouldBe List("azb") val fs = fileSystemProvider.map(p => new AzureFileSystem(p.asInstanceOf[AzureFileSystemProvider], exampleEndpoint.value, exampleConfig.asJava)) fs.nonEmpty shouldBe(true) From 95dd0b8e94e9c127970782efca6f9eb03c409b3d Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Tue, 25 Jul 2023 16:17:27 -0400 Subject: [PATCH 15/36] Does this work better? --- .../filesystems/blob/AzureFileSystemSpec.scala | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala index 3515db9ecb8..37ffcf60e00 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala @@ -4,7 +4,7 @@ import cromwell.filesystems.blob._ import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers -import java.nio.file.spi.FileSystemProvider +import java.nio.file.FileSystems import java.time.Instant import scala.compat.java8.OptionConverters._ import scala.jdk.CollectionConverters._ @@ -14,14 +14,12 @@ class AzureFileSystemSpec extends AnyFlatSpec with Matchers { val container = BlobContainerName("testConainer") val exampleSas = BlobPathBuilderFactorySpec.buildExampleSasToken(now) val exampleConfig = BlobFileSystemManager.buildConfigMap(exampleSas, container) - val exampleEndpoint = BlobPathBuilderSpec.buildEndpoint("testStorageAccount") - val fileSystemProvider = FileSystemProvider.installedProviders().asScala.find(p => p.getScheme() == "azb") + val exampleStorageEndpoint = BlobPathBuilderSpec.buildEndpoint("testStorageAccount") + val exampleCombinedEndpoint = BlobFileSystemManager.combinedEnpointContainerUri(exampleStorageEndpoint, container) + it should "parse an expiration from a sas token" in { - // fileSystemProvider.nonEmpty shouldBe(true) - FileSystemProvider.installedProviders().asScala.map(_.getScheme) shouldBe List("azb") - val fs = fileSystemProvider.map(p => new AzureFileSystem(p.asInstanceOf[AzureFileSystemProvider], exampleEndpoint.value, exampleConfig.asJava)) - fs.nonEmpty shouldBe(true) - fs.flatMap(_.getExpiry().asScala) shouldBe(Some(now)) - fs.map(_.getFileStore().name()) shouldBe(Some(container.value)) + val fs = FileSystems.newFileSystem(exampleCombinedEndpoint, exampleConfig.asJava).asInstanceOf[AzureFileSystem] + fs.getExpiry.asScala shouldBe Some(now) + fs.getFileStore.name() shouldBe container.value } } From 01cf7db8fa360f1885f9144aa8137f91e5b3ab7a Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Tue, 25 Jul 2023 16:55:33 -0400 Subject: [PATCH 16/36] What about now? --- .../cromwell/filesystems/blob/AzureFileSystemSpec.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala index 37ffcf60e00..2d8f68f9e54 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala @@ -1,6 +1,6 @@ -package com.azure.storage.blob.nio; +package cromwell.filesystems.blob -import cromwell.filesystems.blob._ +import com.azure.storage.blob.nio.AzureFileSystem import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers @@ -20,6 +20,6 @@ class AzureFileSystemSpec extends AnyFlatSpec with Matchers { it should "parse an expiration from a sas token" in { val fs = FileSystems.newFileSystem(exampleCombinedEndpoint, exampleConfig.asJava).asInstanceOf[AzureFileSystem] fs.getExpiry.asScala shouldBe Some(now) - fs.getFileStore.name() shouldBe container.value + fs.getFileStores.asScala.map(_.name()).exists(_ == container.value) shouldBe true } } From 5ec30fcd0f9d562caedd969263789ef64cb221af Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Wed, 26 Jul 2023 10:03:03 -0400 Subject: [PATCH 17/36] Change test build --- build.sbt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/build.sbt b/build.sbt index 6b2641e024f..248b0620c67 100644 --- a/build.sbt +++ b/build.sbt @@ -103,10 +103,11 @@ lazy val azureBlobNio = (project in file("azure-blob-nio")) lazy val azureBlobFileSystem = (project in file("filesystems/blob")) .withLibrarySettings("cromwell-azure-blobFileSystem", blobFileSystemDependencies) .dependsOn(core) - .dependsOn(core % "test->test") - .dependsOn(common % "test->test") .dependsOn(cloudSupport) .dependsOn(azureBlobNio) + .dependsOn(core % "test->test") + .dependsOn(common % "test->test") + .dependsOn(azureBlobNio % "test->test") lazy val awsS3FileSystem = (project in file("filesystems/s3")) .withLibrarySettings("cromwell-aws-s3filesystem", s3FileSystemDependencies) From 672ffda103ff9b070b9822f4fa0fe3594f45668b Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Tue, 1 Aug 2023 11:30:55 -0400 Subject: [PATCH 18/36] Testing --- .../cromwell/filesystems/blob/AzureFileSystemSpec.scala | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala index 2d8f68f9e54..4d006dc2b74 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala @@ -1,10 +1,11 @@ package cromwell.filesystems.blob -import com.azure.storage.blob.nio.AzureFileSystem +import com.azure.storage.blob.nio.{AzureFileSystem, AzureFileSystemProvider} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers import java.nio.file.FileSystems +import java.nio.file.spi.FileSystemProvider import java.time.Instant import scala.compat.java8.OptionConverters._ import scala.jdk.CollectionConverters._ @@ -16,8 +17,11 @@ class AzureFileSystemSpec extends AnyFlatSpec with Matchers { val exampleConfig = BlobFileSystemManager.buildConfigMap(exampleSas, container) val exampleStorageEndpoint = BlobPathBuilderSpec.buildEndpoint("testStorageAccount") val exampleCombinedEndpoint = BlobFileSystemManager.combinedEnpointContainerUri(exampleStorageEndpoint, container) + // FileSystemProvider.installedProviders() it should "parse an expiration from a sas token" in { + new AzureFileSystemProvider().getScheme shouldBe "azb" + FileSystemProvider.installedProviders().asScala.map(_.getScheme) shouldBe List("azb") val fs = FileSystems.newFileSystem(exampleCombinedEndpoint, exampleConfig.asJava).asInstanceOf[AzureFileSystem] fs.getExpiry.asScala shouldBe Some(now) fs.getFileStores.asScala.map(_.name()).exists(_ == container.value) shouldBe true From 86194e547739abf75acffa8c302e00b8e9c0ef03 Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Fri, 4 Aug 2023 10:41:09 -0400 Subject: [PATCH 19/36] Restore test to pre-debugging state and ignore --- .../filesystems/blob/AzureFileSystemSpec.scala | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala index 4d006dc2b74..2c475ce2441 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala @@ -1,11 +1,10 @@ package cromwell.filesystems.blob -import com.azure.storage.blob.nio.{AzureFileSystem, AzureFileSystemProvider} +import com.azure.storage.blob.nio.AzureFileSystem import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers import java.nio.file.FileSystems -import java.nio.file.spi.FileSystemProvider import java.time.Instant import scala.compat.java8.OptionConverters._ import scala.jdk.CollectionConverters._ @@ -17,11 +16,11 @@ class AzureFileSystemSpec extends AnyFlatSpec with Matchers { val exampleConfig = BlobFileSystemManager.buildConfigMap(exampleSas, container) val exampleStorageEndpoint = BlobPathBuilderSpec.buildEndpoint("testStorageAccount") val exampleCombinedEndpoint = BlobFileSystemManager.combinedEnpointContainerUri(exampleStorageEndpoint, container) - // FileSystemProvider.installedProviders() - it should "parse an expiration from a sas token" in { - new AzureFileSystemProvider().getScheme shouldBe "azb" - FileSystemProvider.installedProviders().asScala.map(_.getScheme) shouldBe List("azb") + + // This test is passing locally but not in CI, because it can't find a filesystem provider with the azb scheme. + // Some sort of build issue? + ignore should "parse an expiration from a sas token" in { val fs = FileSystems.newFileSystem(exampleCombinedEndpoint, exampleConfig.asJava).asInstanceOf[AzureFileSystem] fs.getExpiry.asScala shouldBe Some(now) fs.getFileStores.asScala.map(_.name()).exists(_ == container.value) shouldBe true From 8621e8c17d0417bcb8f4364cd4e199a5f7d1bcbc Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Fri, 4 Aug 2023 16:24:51 -0400 Subject: [PATCH 20/36] Remove unused config --- .../blob/BlobFileSystemConfig.scala | 17 +++---------- .../blob/BlobFileSystemConfigSpec.scala | 25 ++----------------- .../blob/BlobPathBuilderSpec.scala | 14 +++++------ 3 files changed, 12 insertions(+), 44 deletions(-) diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemConfig.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemConfig.scala index f68bf7f5176..c5467c78ffe 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemConfig.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemConfig.scala @@ -11,13 +11,9 @@ import java.util.UUID // WSM config is needed for accessing WSM-managed blob containers created in Terra workspaces. // If the identity executing Cromwell has native access to the blob container, this can be ignored. final case class WorkspaceManagerConfig(url: WorkspaceManagerURL, - workspaceId: WorkspaceId, - containerResourceId: ContainerResourceId, overrideWsmAuthToken: Option[String]) // dev-only -final case class BlobFileSystemConfig(endpointURL: EndpointURL, - blobContainerName: BlobContainerName, - subscriptionId: Option[SubscriptionId], +final case class BlobFileSystemConfig(subscriptionId: Option[SubscriptionId], expiryBufferMinutes: Long, workspaceManagerConfig: Option[WorkspaceManagerConfig]) @@ -26,8 +22,6 @@ object BlobFileSystemConfig { final val defaultExpiryBufferMinutes = 10L def apply(config: Config): BlobFileSystemConfig = { - val endpointURL = parseString(config, "endpoint").map(EndpointURL) - val blobContainer = parseString(config, "container").map(BlobContainerName) val subscriptionId = parseUUIDOpt(config, "subscription").map(_.map(SubscriptionId)) val expiryBufferMinutes = parseLongOpt(config, "expiry-buffer-minutes") @@ -37,17 +31,15 @@ object BlobFileSystemConfig { if (config.hasPath("workspace-manager")) { val wsmConf = config.getConfig("workspace-manager") val wsmURL = parseString(wsmConf, "url").map(WorkspaceManagerURL) - val workspaceId = parseUUID(wsmConf, "workspace-id").map(WorkspaceId) - val containerResourceId = parseUUID(wsmConf, "container-resource-id").map(ContainerResourceId) val overrideWsmAuthToken = parseStringOpt(wsmConf, "b2cToken") - (wsmURL, workspaceId, containerResourceId, overrideWsmAuthToken) + (wsmURL, overrideWsmAuthToken) .mapN(WorkspaceManagerConfig) .map(Option(_)) } else None.validNel - (endpointURL, blobContainer, subscriptionId, expiryBufferMinutes, wsmConfig) + (subscriptionId, expiryBufferMinutes, wsmConfig) .mapN(BlobFileSystemConfig.apply) .unsafe("Couldn't parse blob filesystem config") } @@ -58,9 +50,6 @@ object BlobFileSystemConfig { private def parseStringOpt(config: Config, path: String) = validate[Option[String]] { config.as[Option[String]](path) } - private def parseUUID(config: Config, path: String) = - validate[UUID] { UUID.fromString(config.as[String](path)) } - private def parseUUIDOpt(config: Config, path: String) = validate[Option[UUID]] { config.as[Option[String]](path).map(UUID.fromString) } diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobFileSystemConfigSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobFileSystemConfigSpec.scala index 607ad5606f7..68804113763 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobFileSystemConfigSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobFileSystemConfigSpec.scala @@ -5,14 +5,8 @@ import common.exception.AggregatedMessageException import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers -import java.util.UUID - class BlobFileSystemConfigSpec extends AnyFlatSpec with Matchers { - private val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") - private val container = BlobContainerName("storageContainer") - private val workspaceId = WorkspaceId(UUID.fromString("B0BAFE77-0000-0000-0000-000000000000")) - private val containerResourceId = ContainerResourceId(UUID.fromString("F00B4911-0000-0000-0000-000000000000")) private val workspaceManagerURL = WorkspaceManagerURL("https://wsm.example.com") private val b2cToken = "b0gus-t0ken" @@ -20,12 +14,8 @@ class BlobFileSystemConfigSpec extends AnyFlatSpec with Matchers { val config = BlobFileSystemConfig( ConfigFactory.parseString( s""" - |container = "$container" - |endpoint = "$endpoint" """.stripMargin) ) - config.blobContainerName should equal(container) - config.endpointURL should equal(endpoint) config.expiryBufferMinutes should equal(BlobFileSystemConfig.defaultExpiryBufferMinutes) } @@ -33,25 +23,17 @@ class BlobFileSystemConfigSpec extends AnyFlatSpec with Matchers { val config = BlobFileSystemConfig( ConfigFactory.parseString( s""" - |container = "$container" - |endpoint = "$endpoint" |expiry-buffer-minutes = "20" |workspace-manager { | url = "$workspaceManagerURL" - | workspace-id = "$workspaceId" - | container-resource-id = "$containerResourceId" | b2cToken = "$b2cToken" |} | """.stripMargin) ) - config.blobContainerName should equal(container) - config.endpointURL should equal(endpoint) config.expiryBufferMinutes should equal(20L) config.workspaceManagerConfig.isDefined shouldBe true config.workspaceManagerConfig.get.url shouldBe workspaceManagerURL - config.workspaceManagerConfig.get.workspaceId shouldBe workspaceId - config.workspaceManagerConfig.get.containerResourceId shouldBe containerResourceId config.workspaceManagerConfig.get.overrideWsmAuthToken.contains(b2cToken) shouldBe true } @@ -59,17 +41,14 @@ class BlobFileSystemConfigSpec extends AnyFlatSpec with Matchers { val rawConfig = ConfigFactory.parseString( s""" - |container = "$container" - |endpoint = "$endpoint" |expiry-buffer-minutes = "10" |workspace-manager { - | url = "$workspaceManagerURL" - | container-resource-id = "$containerResourceId" + | b2cToken = "$b2cToken" |} | """.stripMargin) val error = intercept[AggregatedMessageException](BlobFileSystemConfig(rawConfig)) - error.getMessage should include("No configuration setting found for key 'workspace-id'") + error.getMessage should include("No configuration setting found for key 'url'") } } 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 e67e86570e3..5e48544db15 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala @@ -76,14 +76,14 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { private val endpoint: EndpointURL = BlobPathBuilderSpec.buildEndpoint("coaexternalstorage") private val store: BlobContainerName = BlobContainerName("inputs") - def makeBlobPathBuilder(blobEndpoint: EndpointURL, container: BlobContainerName): BlobPathBuilder = { + def makeBlobPathBuilder: BlobPathBuilder = { val blobTokenGenerator = NativeBlobSasTokenGenerator(Some(subscriptionId)) val fsm = new BlobFileSystemManager(10, blobTokenGenerator) new BlobPathBuilder()(fsm) } ignore should "resolve an absolute path string correctly to a path" in { - val builder = makeBlobPathBuilder(endpoint, store) + val builder = makeBlobPathBuilder val rootString = s"${endpoint.value}/${store.value}/cromwell-execution" val blobRoot: BlobPath = builder build rootString getOrElse fail() blobRoot.toAbsolutePath.pathAsString should equal ("https://coaexternalstorage.blob.core.windows.net/inputs/cromwell-execution") @@ -92,7 +92,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } ignore should "build a blob path from a test string and read a file" in { - val builder = makeBlobPathBuilder(endpoint, store) + val builder = makeBlobPathBuilder val endpointHost = BlobPathBuilder.parseURI(endpoint.value).map(_.getHost).getOrElse(fail("Could not parse URI")) val evalPath = "/test/inputFile.txt" val testString = endpoint.value + "/" + store + evalPath @@ -108,7 +108,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } ignore should "build duplicate blob paths in the same filesystem" in { - val builder = makeBlobPathBuilder(endpoint, store) + val builder = makeBlobPathBuilder val evalPath = "/test/inputFile.txt" val testString = endpoint.value + "/" + store + evalPath val blobPath1: BlobPath = builder build testString getOrElse fail() @@ -121,7 +121,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } ignore should "resolve a path without duplicating container name" in { - val builder = makeBlobPathBuilder(endpoint, store) + val builder = makeBlobPathBuilder val rootString = s"${endpoint.value}/${store.value}/cromwell-execution" val blobRoot: BlobPath = builder build rootString getOrElse fail() blobRoot.toAbsolutePath.pathAsString should equal ("https://coaexternalstorage.blob.core.windows.net/inputs/cromwell-execution") @@ -130,7 +130,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } ignore should "correctly remove a prefix from the blob path" in { - val builder = makeBlobPathBuilder(endpoint, store) + val builder = makeBlobPathBuilder val rootString = s"${endpoint.value}/${store.value}/cromwell-execution/" val execDirString = s"${endpoint.value}/${store.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/" val fileString = s"${endpoint.value}/${store.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/stdout" @@ -143,7 +143,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } ignore should "not change a path if it doesn't start with a prefix" in { - val builder = makeBlobPathBuilder(endpoint, store) + val builder = makeBlobPathBuilder val otherRootString = s"${endpoint.value}/${store.value}/foobar/" val fileString = s"${endpoint.value}/${store.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/stdout" val otherBlobRoot: BlobPath = builder build otherRootString getOrElse fail() From 2dac0ba949d114bbaa0b6c3e4c6ae78a8e6c5b1a Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Fri, 4 Aug 2023 16:26:00 -0400 Subject: [PATCH 21/36] Remove unused config --- src/ci/resources/tes_application.conf | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/ci/resources/tes_application.conf b/src/ci/resources/tes_application.conf index 5eb812a52d4..ae80ef1aa3b 100644 --- a/src/ci/resources/tes_application.conf +++ b/src/ci/resources/tes_application.conf @@ -8,16 +8,12 @@ filesystems { # One BFSM is shared across all BlobPathBuilders class = "cromwell.filesystems.blob.BlobFileSystemManager" config { - container: "cromwell" - endpoint: "https://.blob.core.windows.net" subscription: "00001111-2222-3333-aaaa-bbbbccccdddd" # WSM config is needed for accessing WSM-managed blob containers # created in Terra workspaces. workspace-manager { url: "https://workspace.dsde-dev.broadinstitute.org" - workspace-id: "00001111-2222-3333-aaaa-bbbbccccdddd" - container-resource-id: "00001111-2222-3333-aaaa-bbbbccccdddd" - b2cToken: "Zardoz" + b2cToken: "Zardoz" # no `Bearer`! } } } From da8a547b2370d98dfa5c71305acc84cce80d8e56 Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Mon, 14 Aug 2023 16:29:46 -0400 Subject: [PATCH 22/36] tiny change huge impact --- .../filesystems/blob/WorkspaceManagerApiClientProvider.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/WorkspaceManagerApiClientProvider.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/WorkspaceManagerApiClientProvider.scala index fb1475f03be..276738c98b6 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/WorkspaceManagerApiClientProvider.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/WorkspaceManagerApiClientProvider.scala @@ -45,7 +45,7 @@ class HttpWorkspaceManagerClientProvider(baseWorkspaceManagerUrl: WorkspaceManag case class WsmResourceApi(resourcesApi : ResourceApi) { def findContainerResourceId(workspaceId : UUID, container: BlobContainerName): Try[UUID] = { for { - workspaceResources <- Try(resourcesApi.enumerateResources(workspaceId, 0, 1, ResourceType.AZURE_STORAGE_CONTAINER, StewardshipType.CONTROLLED).getResources()) + workspaceResources <- Try(resourcesApi.enumerateResources(workspaceId, 0, 10, ResourceType.AZURE_STORAGE_CONTAINER, StewardshipType.CONTROLLED).getResources()) workspaceStorageContainerOption = workspaceResources.asScala.find(r => r.getMetadata().getName() == container.value) workspaceStorageContainer <- workspaceStorageContainerOption.toRight(new Exception("No storage container found for this workspace")).toTry resourceId = workspaceStorageContainer.getMetadata().getResourceId() From a3ff4d7f0c269fa9e100940a265647e963778866 Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Tue, 15 Aug 2023 15:03:36 -0400 Subject: [PATCH 23/36] remove unnecessary config change --- src/ci/resources/tes_application.conf | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/ci/resources/tes_application.conf b/src/ci/resources/tes_application.conf index ae80ef1aa3b..5eb812a52d4 100644 --- a/src/ci/resources/tes_application.conf +++ b/src/ci/resources/tes_application.conf @@ -8,12 +8,16 @@ filesystems { # One BFSM is shared across all BlobPathBuilders class = "cromwell.filesystems.blob.BlobFileSystemManager" config { + container: "cromwell" + endpoint: "https://.blob.core.windows.net" subscription: "00001111-2222-3333-aaaa-bbbbccccdddd" # WSM config is needed for accessing WSM-managed blob containers # created in Terra workspaces. workspace-manager { url: "https://workspace.dsde-dev.broadinstitute.org" - b2cToken: "Zardoz" # no `Bearer`! + workspace-id: "00001111-2222-3333-aaaa-bbbbccccdddd" + container-resource-id: "00001111-2222-3333-aaaa-bbbbccccdddd" + b2cToken: "Zardoz" } } } From 90bf473a159911820b8c44f6cc85bef5d8dc2558 Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Fri, 18 Aug 2023 11:34:14 -0400 Subject: [PATCH 24/36] merge but better --- .../blob/BlobPathBuilderSpec.scala | 36 ------------------- 1 file changed, 36 deletions(-) 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 2a3ba318c20..20c4c72d8bc 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala @@ -77,17 +77,6 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { private val endpoint: EndpointURL = BlobPathBuilderSpec.buildEndpoint("centaurtesting") private val container: BlobContainerName = BlobContainerName("test-blob") -<<<<<<< HEAD - def makeBlobPathBuilder: BlobPathBuilder = { - val blobTokenGenerator = NativeBlobSasTokenGenerator(Some(subscriptionId)) - val fsm = new BlobFileSystemManager(10, blobTokenGenerator) - new BlobPathBuilder()(fsm) - } - - ignore should "resolve an absolute path string correctly to a path" in { - val builder = makeBlobPathBuilder - val rootString = s"${endpoint.value}/${store.value}/cromwell-execution" -======= def makeBlobPathBuilder(blobEndpoint: EndpointURL, container: BlobContainerName): BlobPathBuilder = { val blobTokenGenerator = NativeBlobSasTokenGenerator(container, blobEndpoint, Some(subscriptionId)) @@ -130,20 +119,14 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { it should "resolve an absolute path string correctly to a path" in { val builder = makeBlobPathBuilder(endpoint, container) val rootString = s"${endpoint.value}/${container.value}/cromwell-execution" ->>>>>>> develop val blobRoot: BlobPath = builder build rootString getOrElse fail() blobRoot.toAbsolutePath.pathAsString should equal ("https://centaurtesting.blob.core.windows.net/test-blob/cromwell-execution") val otherFile = blobRoot.resolve("https://centaurtesting.blob.core.windows.net/test-blob/cromwell-execution/test/inputFile.txt") otherFile.toAbsolutePath.pathAsString should equal ("https://centaurtesting.blob.core.windows.net/test-blob/cromwell-execution/test/inputFile.txt") } -<<<<<<< HEAD - ignore should "build a blob path from a test string and read a file" in { - val builder = makeBlobPathBuilder -======= it should "build a blob path from a test string and read a file" in { val builder = makeBlobPathBuilder(endpoint, container) ->>>>>>> develop val endpointHost = BlobPathBuilder.parseURI(endpoint.value).map(_.getHost).getOrElse(fail("Could not parse URI")) val evalPath = "/test/inputFile.txt" val testString = endpoint.value + "/" + container + evalPath @@ -158,13 +141,8 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { fileText should include ("This is my test file!!!! Did it work?") } -<<<<<<< HEAD - ignore should "build duplicate blob paths in the same filesystem" in { - val builder = makeBlobPathBuilder -======= it should "build duplicate blob paths in the same filesystem" in { val builder = makeBlobPathBuilder(endpoint, container) ->>>>>>> develop val evalPath = "/test/inputFile.txt" val testString = endpoint.value + "/" + container + evalPath val blobPath1: BlobPath = builder build testString getOrElse fail() @@ -176,34 +154,20 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { fileText should include ("This is my test file!!!! Did it work?") } -<<<<<<< HEAD - ignore should "resolve a path without duplicating container name" in { - val builder = makeBlobPathBuilder - val rootString = s"${endpoint.value}/${store.value}/cromwell-execution" -======= it should "resolve a path without duplicating container name" in { val builder = makeBlobPathBuilder(endpoint, container) val rootString = s"${endpoint.value}/${container.value}/cromwell-execution" ->>>>>>> develop val blobRoot: BlobPath = builder build rootString getOrElse fail() blobRoot.toAbsolutePath.pathAsString should equal ("https://centaurtesting.blob.core.windows.net/test-blob/cromwell-execution") val otherFile = blobRoot.resolve("test/inputFile.txt") otherFile.toAbsolutePath.pathAsString should equal ("https://centaurtesting.blob.core.windows.net/test-blob/cromwell-execution/test/inputFile.txt") } -<<<<<<< HEAD - ignore should "correctly remove a prefix from the blob path" in { - val builder = makeBlobPathBuilder - val rootString = s"${endpoint.value}/${store.value}/cromwell-execution/" - val execDirString = s"${endpoint.value}/${store.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/" - val fileString = s"${endpoint.value}/${store.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/stdout" -======= it should "correctly remove a prefix from the blob path" in { val builder = makeBlobPathBuilder(endpoint, container) val rootString = s"${endpoint.value}/${container.value}/cromwell-execution/" val execDirString = s"${endpoint.value}/${container.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/" val fileString = s"${endpoint.value}/${container.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/stdout" ->>>>>>> develop val blobRoot: BlobPath = builder build rootString getOrElse fail() val execDir: BlobPath = builder build execDirString getOrElse fail() val blobFile: BlobPath = builder build fileString getOrElse fail() From a9b28f1dc5249545e6e8804af28201eef3e70c1c Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Fri, 18 Aug 2023 11:36:17 -0400 Subject: [PATCH 25/36] unignore --- .../scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala index 2c475ce2441..4e9cdd31ae2 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala @@ -20,7 +20,7 @@ class AzureFileSystemSpec extends AnyFlatSpec with Matchers { // This test is passing locally but not in CI, because it can't find a filesystem provider with the azb scheme. // Some sort of build issue? - ignore should "parse an expiration from a sas token" in { + it should "parse an expiration from a sas token" in { val fs = FileSystems.newFileSystem(exampleCombinedEndpoint, exampleConfig.asJava).asInstanceOf[AzureFileSystem] fs.getExpiry.asScala shouldBe Some(now) fs.getFileStores.asScala.map(_.name()).exists(_ == container.value) shouldBe true From fd9e7f43e7e00195401353b779ce8653f2625b74 Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Fri, 18 Aug 2023 12:24:22 -0400 Subject: [PATCH 26/36] still one import problem --- .../filesystems/blob/BlobFileSystemManager.scala | 10 +++++++--- 1 file changed, 7 insertions(+), 3 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 a0f2d4bcb47..1798a19df1d 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala @@ -2,7 +2,7 @@ package cromwell.filesystems.blob import bio.terra.workspace.client.ApiException import com.azure.core.credential.AzureSasCredential -import com.azure.storage.blob.nio.{AzureFileSystem, AzureFileSystemProvider} +import com.azure.storage.blob.nio.{AzureFileSystem, AzureFileSystemProvider, _} import com.azure.storage.blob.sas.{BlobContainerSasPermission, BlobServiceSasSignatureValues} import com.typesafe.config.Config import com.typesafe.scalalogging.LazyLogging @@ -11,6 +11,7 @@ import cromwell.cloudsupport.azure.{AzureCredentials, AzureUtils} import java.net.URI import java.nio.file._ +import java.nio.file.spi.FileSystemProvider import java.time.temporal.ChronoUnit import java.time.{Duration, Instant, OffsetDateTime} import java.util.UUID @@ -20,7 +21,7 @@ import scala.util.{Failure, Success, Try} // We encapsulate this functionality here so that we can easily mock it out, to allow for testing without // actually connecting to Blob storage. case class FileSystemAPI(private val provider: FileSystemProvider = new AzureFileSystemProvider()) { - def getFileSystem(uri: URI): Try[FileSystem] = Try(provider.getFileSystem(uri)) + def getFileSystem(uri: URI): Try[FileSystem] = Try(provider.getFileSystem(uri).asInstanceOf[AzureFileSystem]) def newFileSystem(uri: URI, config: Map[String, Object]): FileSystem = provider.newFileSystem(uri, config.asJava) def closeFileSystem(uri: URI): Option[Unit] = getFileSystem(uri).toOption.map(_.close) } @@ -99,7 +100,10 @@ class BlobFileSystemManager(val expiryBufferMinutes: Long, blobTokenGenerator.generateBlobSasToken(endpoint, container) .flatMap((token: AzureSasCredential) => { Try(fileSystemAPI.newFileSystem(uri, BlobFileSystemManager.buildConfigMap(token, container))) - }) + }) match { + case azureFilesystem : Try[AzureFileSystem] => azureFilesystem + case _ => Failure[AzureFileSystem](new FileSystemNotFoundException("Generated filesystem was not an azure filesystem")) + } } } From 12250574c08185451765acfb932787f0021dd84e Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Fri, 18 Aug 2023 12:44:55 -0400 Subject: [PATCH 27/36] `AzureFileSystem` is-a `FileSystem` and has-a `isExpired` --- .../scala/cromwell/filesystems/blob/BlobFileSystemManager.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 1798a19df1d..7a9f0a68a9f 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala @@ -21,7 +21,7 @@ import scala.util.{Failure, Success, Try} // We encapsulate this functionality here so that we can easily mock it out, to allow for testing without // actually connecting to Blob storage. case class FileSystemAPI(private val provider: FileSystemProvider = new AzureFileSystemProvider()) { - def getFileSystem(uri: URI): Try[FileSystem] = Try(provider.getFileSystem(uri).asInstanceOf[AzureFileSystem]) + def getFileSystem(uri: URI): Try[AzureFileSystem] = Try(provider.getFileSystem(uri).asInstanceOf[AzureFileSystem]) def newFileSystem(uri: URI, config: Map[String, Object]): FileSystem = provider.newFileSystem(uri, config.asJava) def closeFileSystem(uri: URI): Option[Unit] = getFileSystem(uri).toOption.map(_.close) } From c2df9e540125aaee3e51af81085c2ed233599b03 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Fri, 18 Aug 2023 15:05:58 -0400 Subject: [PATCH 28/36] Revert "`AzureFileSystem` is-a `FileSystem` and has-a `isExpired`" This reverts commit 12250574c08185451765acfb932787f0021dd84e. --- .../scala/cromwell/filesystems/blob/BlobFileSystemManager.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 7a9f0a68a9f..1798a19df1d 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala @@ -21,7 +21,7 @@ import scala.util.{Failure, Success, Try} // We encapsulate this functionality here so that we can easily mock it out, to allow for testing without // actually connecting to Blob storage. case class FileSystemAPI(private val provider: FileSystemProvider = new AzureFileSystemProvider()) { - def getFileSystem(uri: URI): Try[AzureFileSystem] = Try(provider.getFileSystem(uri).asInstanceOf[AzureFileSystem]) + def getFileSystem(uri: URI): Try[FileSystem] = Try(provider.getFileSystem(uri).asInstanceOf[AzureFileSystem]) def newFileSystem(uri: URI, config: Map[String, Object]): FileSystem = provider.newFileSystem(uri, config.asJava) def closeFileSystem(uri: URI): Option[Unit] = getFileSystem(uri).toOption.map(_.close) } From 4b890a6cc2964dfec8812bc0b4b4b6aa564c13f7 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Fri, 18 Aug 2023 15:19:12 -0400 Subject: [PATCH 29/36] Revert "Revert "`AzureFileSystem` is-a `FileSystem` and has-a `isExpired`"" This reverts commit c2df9e540125aaee3e51af81085c2ed233599b03. --- .../scala/cromwell/filesystems/blob/BlobFileSystemManager.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 1798a19df1d..7a9f0a68a9f 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala @@ -21,7 +21,7 @@ import scala.util.{Failure, Success, Try} // We encapsulate this functionality here so that we can easily mock it out, to allow for testing without // actually connecting to Blob storage. case class FileSystemAPI(private val provider: FileSystemProvider = new AzureFileSystemProvider()) { - def getFileSystem(uri: URI): Try[FileSystem] = Try(provider.getFileSystem(uri).asInstanceOf[AzureFileSystem]) + def getFileSystem(uri: URI): Try[AzureFileSystem] = Try(provider.getFileSystem(uri).asInstanceOf[AzureFileSystem]) def newFileSystem(uri: URI, config: Map[String, Object]): FileSystem = provider.newFileSystem(uri, config.asJava) def closeFileSystem(uri: URI): Option[Unit] = getFileSystem(uri).toOption.map(_.close) } From 8a8a5058b1cc734295487d570f0205f149c688f7 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Fri, 18 Aug 2023 15:32:38 -0400 Subject: [PATCH 30/36] `AzureFileSystem` is-a `FileSystem` and has-a `isExpired` --- .../filesystems/blob/BlobFileSystemManager.scala | 15 ++++++--------- .../blob/BlobPathBuilderFactorySpec.scala | 14 +++++++------- 2 files changed, 13 insertions(+), 16 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 7a9f0a68a9f..6b6088c7689 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobFileSystemManager.scala @@ -2,7 +2,7 @@ package cromwell.filesystems.blob import bio.terra.workspace.client.ApiException import com.azure.core.credential.AzureSasCredential -import com.azure.storage.blob.nio.{AzureFileSystem, AzureFileSystemProvider, _} +import com.azure.storage.blob.nio.{AzureFileSystem, AzureFileSystemProvider} import com.azure.storage.blob.sas.{BlobContainerSasPermission, BlobServiceSasSignatureValues} import com.typesafe.config.Config import com.typesafe.scalalogging.LazyLogging @@ -20,9 +20,9 @@ import scala.util.{Failure, Success, Try} // We encapsulate this functionality here so that we can easily mock it out, to allow for testing without // actually connecting to Blob storage. -case class FileSystemAPI(private val provider: FileSystemProvider = new AzureFileSystemProvider()) { +case class AzureFileSystemAPI(private val provider: FileSystemProvider = new AzureFileSystemProvider()) { def getFileSystem(uri: URI): Try[AzureFileSystem] = Try(provider.getFileSystem(uri).asInstanceOf[AzureFileSystem]) - def newFileSystem(uri: URI, config: Map[String, Object]): FileSystem = provider.newFileSystem(uri, config.asJava) + def newFileSystem(uri: URI, config: Map[String, Object]): Try[AzureFileSystem] = Try(provider.newFileSystem(uri, config.asJava).asInstanceOf[AzureFileSystem]) def closeFileSystem(uri: URI): Option[Unit] = getFileSystem(uri).toOption.map(_.close) } /** @@ -53,7 +53,7 @@ object BlobFileSystemManager { class BlobFileSystemManager(val expiryBufferMinutes: Long, val blobTokenGenerator: BlobSasTokenGenerator, - val fileSystemAPI: FileSystemAPI = FileSystemAPI()) extends LazyLogging { + val fileSystemAPI: AzureFileSystemAPI = AzureFileSystemAPI()) extends LazyLogging { def this(config: BlobFileSystemConfig) = { this( @@ -99,11 +99,8 @@ class BlobFileSystemManager(val expiryBufferMinutes: Long, private def generateFilesystem(uri: URI, container: BlobContainerName, endpoint: EndpointURL): Try[AzureFileSystem] = { blobTokenGenerator.generateBlobSasToken(endpoint, container) .flatMap((token: AzureSasCredential) => { - Try(fileSystemAPI.newFileSystem(uri, BlobFileSystemManager.buildConfigMap(token, container))) - }) match { - case azureFilesystem : Try[AzureFileSystem] => azureFilesystem - case _ => Failure[AzureFileSystem](new FileSystemNotFoundException("Generated filesystem was not an azure filesystem")) - } + fileSystemAPI.newFileSystem(uri, BlobFileSystemManager.buildConfigMap(token, container)) + }) } } diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala index 7683afde3b7..9fd16468317 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala @@ -43,7 +43,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") val container = BlobContainerName("test") val azureUri = BlobFileSystemManager.combinedEnpointContainerUri(endpoint, container) - val fileSystems = mock[FileSystemAPI] + val fileSystems = mock[AzureFileSystemAPI] val fileSystem = mock[AzureFileSystem] when(fileSystems.getFileSystem(azureUri)).thenReturn(Try(fileSystem)) when(fileSystems.closeFileSystem(azureUri)).thenCallRealMethod() @@ -65,7 +65,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker val azureFileSystem = mock[AzureFileSystem] when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(true) - val fileSystems = mock[FileSystemAPI] + val fileSystems = mock[AzureFileSystemAPI] when(fileSystems.getFileSystem(azureUri)).thenReturn(Success(azureFileSystem)) val blobTokenGenerator = mock[BlobSasTokenGenerator] when(blobTokenGenerator.generateBlobSasToken(endpoint, container)).thenReturn(Try(sasToken)) @@ -91,7 +91,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker val azureFileSystem = mock[AzureFileSystem] when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(false) - val fileSystems = mock[FileSystemAPI] + val fileSystems = mock[AzureFileSystemAPI] when(fileSystems.getFileSystem(azureUri)).thenReturn(Try(azureFileSystem)) val blobTokenGenerator = mock[BlobSasTokenGenerator] @@ -117,7 +117,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker val azureFileSystem = mock[AzureFileSystem] when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(false) - val fileSystems = mock[FileSystemAPI] + val fileSystems = mock[AzureFileSystemAPI] when(fileSystems.getFileSystem(azureUri)).thenReturn(Failure(new FileSystemNotFoundException)) when(fileSystems.newFileSystem(azureUri, configMap)).thenReturn(azureFileSystem) val blobTokenGenerator = mock[BlobSasTokenGenerator] @@ -142,7 +142,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker val azureFileSystem = mock[AzureFileSystem] when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(true) - val fileSystems = mock[FileSystemAPI] + val fileSystems = mock[AzureFileSystemAPI] when(fileSystems.getFileSystem(azureUri)).thenReturn(Success(azureFileSystem)) val blobTokenGenerator = mock[BlobSasTokenGenerator] when(blobTokenGenerator.generateBlobSasToken(endpoint, container)).thenReturn(Try(sasToken)) @@ -166,7 +166,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker val azureFileSystem = mock[AzureFileSystem] when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(false) - val fileSystems = mock[FileSystemAPI] + val fileSystems = mock[AzureFileSystemAPI] when(fileSystems.getFileSystem(azureUri)).thenReturn(Try(azureFileSystem)) val blobTokenGenerator = mock[BlobSasTokenGenerator] @@ -191,7 +191,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga //at filesystems/blob/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker val azureFileSystem = mock[AzureFileSystem] when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(false) - val fileSystems = mock[FileSystemAPI] + val fileSystems = mock[AzureFileSystemAPI] when(fileSystems.getFileSystem(azureUri)).thenReturn(Failure(new FileSystemNotFoundException)) when(fileSystems.newFileSystem(azureUri, configMap)).thenReturn(azureFileSystem) val blobTokenGenerator = mock[BlobSasTokenGenerator] From 6e818c1ded1e3d3eb14781367467948b78aa2ee8 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Fri, 18 Aug 2023 16:31:58 -0400 Subject: [PATCH 31/36] `Try.get` in test --- .../filesystems/blob/BlobPathBuilderFactorySpec.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala index 9fd16468317..7a643cd9d57 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala @@ -119,7 +119,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(false) val fileSystems = mock[AzureFileSystemAPI] when(fileSystems.getFileSystem(azureUri)).thenReturn(Failure(new FileSystemNotFoundException)) - when(fileSystems.newFileSystem(azureUri, configMap)).thenReturn(azureFileSystem) + when(fileSystems.newFileSystem(azureUri, configMap).get).thenReturn(azureFileSystem) val blobTokenGenerator = mock[BlobSasTokenGenerator] when(blobTokenGenerator.generateBlobSasToken(endpoint, container)).thenReturn(Try(sasToken)) @@ -193,7 +193,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(false) val fileSystems = mock[AzureFileSystemAPI] when(fileSystems.getFileSystem(azureUri)).thenReturn(Failure(new FileSystemNotFoundException)) - when(fileSystems.newFileSystem(azureUri, configMap)).thenReturn(azureFileSystem) + when(fileSystems.newFileSystem(azureUri, configMap).get).thenReturn(azureFileSystem) val blobTokenGenerator = mock[BlobSasTokenGenerator] when(blobTokenGenerator.generateBlobSasToken(endpoint, container)).thenReturn(Try(sasToken)) From a0a0979f060ac66765b86fd54e79f06083a76f73 Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Mon, 21 Aug 2023 13:15:49 -0400 Subject: [PATCH 32/36] compile issue --- .../cromwell/filesystems/blob/BlobPathBuilderSpec.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 20c4c72d8bc..301bcfd0e06 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala @@ -79,9 +79,9 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { def makeBlobPathBuilder(blobEndpoint: EndpointURL, container: BlobContainerName): BlobPathBuilder = { - val blobTokenGenerator = NativeBlobSasTokenGenerator(container, blobEndpoint, Some(subscriptionId)) - val fsm = new BlobFileSystemManager(container, blobEndpoint, 10, blobTokenGenerator) - new BlobPathBuilder(container, blobEndpoint)(fsm) + val blobTokenGenerator = NativeBlobSasTokenGenerator(Some(subscriptionId)) + val fsm = new BlobFileSystemManager(10, blobTokenGenerator) + new BlobPathBuilder()(fsm) } it should "read md5 from small files <5g" in { From 9ced6fb1aeafc8ecf346b11b1b7acb408da016a7 Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Mon, 21 Aug 2023 14:19:38 -0400 Subject: [PATCH 33/36] definitley fix mock issue, maybe fix filesystem issue --- .../filesystems/blob/AzureFileSystemSpec.scala | 10 +++++----- .../filesystems/blob/BlobPathBuilderFactorySpec.scala | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala index 4e9cdd31ae2..12363bc3b48 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala @@ -1,10 +1,9 @@ package cromwell.filesystems.blob -import com.azure.storage.blob.nio.AzureFileSystem +import com.azure.storage.blob.nio.{AzureFileSystem, AzureFileSystemProvider} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers -import java.nio.file.FileSystems import java.time.Instant import scala.compat.java8.OptionConverters._ import scala.jdk.CollectionConverters._ @@ -21,8 +20,9 @@ class AzureFileSystemSpec extends AnyFlatSpec with Matchers { // This test is passing locally but not in CI, because it can't find a filesystem provider with the azb scheme. // Some sort of build issue? it should "parse an expiration from a sas token" in { - val fs = FileSystems.newFileSystem(exampleCombinedEndpoint, exampleConfig.asJava).asInstanceOf[AzureFileSystem] - fs.getExpiry.asScala shouldBe Some(now) - fs.getFileStores.asScala.map(_.name()).exists(_ == container.value) shouldBe true + val provider = new AzureFileSystemProvider() + val filesystem : AzureFileSystem = provider.newFileSystem(exampleCombinedEndpoint, exampleConfig.asJava).asInstanceOf[AzureFileSystem] + filesystem.getExpiry.asScala shouldBe Some(now) + filesystem.getFileStores.asScala.map(_.name()).exists(_ == container.value) shouldBe true } } diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala index 7a643cd9d57..c4ee102c58b 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderFactorySpec.scala @@ -119,7 +119,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(false) val fileSystems = mock[AzureFileSystemAPI] when(fileSystems.getFileSystem(azureUri)).thenReturn(Failure(new FileSystemNotFoundException)) - when(fileSystems.newFileSystem(azureUri, configMap).get).thenReturn(azureFileSystem) + when(fileSystems.newFileSystem(azureUri, configMap)).thenReturn(Try(azureFileSystem)) val blobTokenGenerator = mock[BlobSasTokenGenerator] when(blobTokenGenerator.generateBlobSasToken(endpoint, container)).thenReturn(Try(sasToken)) @@ -193,7 +193,7 @@ class BlobPathBuilderFactorySpec extends AnyFlatSpec with Matchers with MockSuga when(azureFileSystem.isExpired(Duration.ofMinutes(10L))).thenReturn(false) val fileSystems = mock[AzureFileSystemAPI] when(fileSystems.getFileSystem(azureUri)).thenReturn(Failure(new FileSystemNotFoundException)) - when(fileSystems.newFileSystem(azureUri, configMap).get).thenReturn(azureFileSystem) + when(fileSystems.newFileSystem(azureUri, configMap)).thenReturn(Try(azureFileSystem)) val blobTokenGenerator = mock[BlobSasTokenGenerator] when(blobTokenGenerator.generateBlobSasToken(endpoint, container)).thenReturn(Try(sasToken)) From 9e09bbab060d53eb379de9b7907f82e2a7a14272 Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Mon, 21 Aug 2023 15:38:37 -0400 Subject: [PATCH 34/36] all seems well. rename variable --- .../scala/cromwell/filesystems/blob/BlobPathBuilder.scala | 4 ++-- 1 file changed, 2 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 b37783de29d..3aa26eb3c11 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala @@ -12,7 +12,7 @@ import scala.language.postfixOps import scala.util.{Failure, Success, Try} object BlobPathBuilder { - val HOSTNAME_SUFFIX = ".blob.core.windows.net" + private val blobHostnameSuffix = ".blob.core.windows.net" sealed trait BlobPathValidation case class ValidBlobPath(path: String, container: BlobContainerName, endpoint: EndpointURL) extends BlobPathValidation case class UnparsableBlobPath(errorMessage: Throwable) extends BlobPathValidation @@ -47,7 +47,7 @@ object BlobPathBuilder { testEndpoint = EndpointURL(testUri.getScheme + "://" + testUri.getHost()) testStorageAccount <- parseStorageAccount(testUri) testContainer = testUri.getPath.split("/").find(_.nonEmpty) - isBlobHost = testUri.getHost().contains(HOSTNAME_SUFFIX) && testUri.getScheme().contains("https") + isBlobHost = testUri.getHost().contains(blobHostnameSuffix) && testUri.getScheme().contains("https") blobPathValidation = (isBlobHost, testContainer) match { case (true, Some(container)) => ValidBlobPath( testUri.getPath.replaceFirst("/" + container, ""), From 8e36d6fd759fb14265eeb3c07478732ffcd01929 Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Mon, 21 Aug 2023 15:41:52 -0400 Subject: [PATCH 35/36] remove unused params --- .../blob/BlobPathBuilderSpec.scala | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) 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 301bcfd0e06..a8ca7d58d6f 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala @@ -77,15 +77,14 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { private val endpoint: EndpointURL = BlobPathBuilderSpec.buildEndpoint("centaurtesting") private val container: BlobContainerName = BlobContainerName("test-blob") - def makeBlobPathBuilder(blobEndpoint: EndpointURL, - container: BlobContainerName): BlobPathBuilder = { + def makeBlobPathBuilder(): BlobPathBuilder = { val blobTokenGenerator = NativeBlobSasTokenGenerator(Some(subscriptionId)) val fsm = new BlobFileSystemManager(10, blobTokenGenerator) new BlobPathBuilder()(fsm) } it should "read md5 from small files <5g" in { - val builder = makeBlobPathBuilder(endpoint, container) + val builder = makeBlobPathBuilder() val evalPath = "/testRead.txt" val testString = endpoint.value + "/" + container + evalPath val blobPath1: BlobPath = (builder build testString).get @@ -93,7 +92,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } it should "read md5 from large files >5g" in { - val builder = makeBlobPathBuilder(endpoint, container) + val builder = makeBlobPathBuilder() val evalPath = "/Rocky-9.2-aarch64-dvd.iso" val testString = endpoint.value + "/" + container + evalPath val blobPath1: BlobPath = (builder build testString).get @@ -101,7 +100,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } it should "choose the root/metadata md5 over the native md5 for files that have both" in { - val builder = makeBlobPathBuilder(endpoint, container) + val builder = makeBlobPathBuilder() val evalPath = "/redundant_md5_test.txt" val testString = endpoint.value + "/" + container + evalPath val blobPath1: BlobPath = (builder build testString).get @@ -109,7 +108,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } it should "gracefully return `None` when neither hash is found" in { - val builder = makeBlobPathBuilder(endpoint, container) + val builder = makeBlobPathBuilder() val evalPath = "/no_md5_test.txt" val testString = endpoint.value + "/" + container + evalPath val blobPath1: BlobPath = (builder build testString).get @@ -117,7 +116,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } it should "resolve an absolute path string correctly to a path" in { - val builder = makeBlobPathBuilder(endpoint, container) + val builder = makeBlobPathBuilder() val rootString = s"${endpoint.value}/${container.value}/cromwell-execution" val blobRoot: BlobPath = builder build rootString getOrElse fail() blobRoot.toAbsolutePath.pathAsString should equal ("https://centaurtesting.blob.core.windows.net/test-blob/cromwell-execution") @@ -126,7 +125,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } it should "build a blob path from a test string and read a file" in { - val builder = makeBlobPathBuilder(endpoint, container) + val builder = makeBlobPathBuilder() val endpointHost = BlobPathBuilder.parseURI(endpoint.value).map(_.getHost).getOrElse(fail("Could not parse URI")) val evalPath = "/test/inputFile.txt" val testString = endpoint.value + "/" + container + evalPath @@ -142,7 +141,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } it should "build duplicate blob paths in the same filesystem" in { - val builder = makeBlobPathBuilder(endpoint, container) + val builder = makeBlobPathBuilder() val evalPath = "/test/inputFile.txt" val testString = endpoint.value + "/" + container + evalPath val blobPath1: BlobPath = builder build testString getOrElse fail() @@ -155,7 +154,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } it should "resolve a path without duplicating container name" in { - val builder = makeBlobPathBuilder(endpoint, container) + val builder = makeBlobPathBuilder() val rootString = s"${endpoint.value}/${container.value}/cromwell-execution" val blobRoot: BlobPath = builder build rootString getOrElse fail() blobRoot.toAbsolutePath.pathAsString should equal ("https://centaurtesting.blob.core.windows.net/test-blob/cromwell-execution") @@ -164,7 +163,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } it should "correctly remove a prefix from the blob path" in { - val builder = makeBlobPathBuilder(endpoint, container) + val builder = makeBlobPathBuilder() val rootString = s"${endpoint.value}/${container.value}/cromwell-execution/" val execDirString = s"${endpoint.value}/${container.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/" val fileString = s"${endpoint.value}/${container.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/stdout" @@ -177,7 +176,7 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } it should "not change a path if it doesn't start with a prefix" in { - val builder = makeBlobPathBuilder(endpoint, container) + val builder = makeBlobPathBuilder() val otherRootString = s"${endpoint.value}/${container.value}/foobar/" val fileString = s"${endpoint.value}/${container.value}/cromwell-execution/abc123/myworkflow/task1/def4356/execution/stdout" val otherBlobRoot: BlobPath = builder build otherRootString getOrElse fail() From 28692389ecbac445ffbbf45c428fc6240c886b3a Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Mon, 21 Aug 2023 17:08:15 -0400 Subject: [PATCH 36/36] remove stale comment --- .../scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala | 3 --- 1 file changed, 3 deletions(-) diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala index 12363bc3b48..e0463bab740 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/AzureFileSystemSpec.scala @@ -16,9 +16,6 @@ class AzureFileSystemSpec extends AnyFlatSpec with Matchers { val exampleStorageEndpoint = BlobPathBuilderSpec.buildEndpoint("testStorageAccount") val exampleCombinedEndpoint = BlobFileSystemManager.combinedEnpointContainerUri(exampleStorageEndpoint, container) - - // This test is passing locally but not in CI, because it can't find a filesystem provider with the azb scheme. - // Some sort of build issue? it should "parse an expiration from a sas token" in { val provider = new AzureFileSystemProvider() val filesystem : AzureFileSystem = provider.newFileSystem(exampleCombinedEndpoint, exampleConfig.asJava).asInstanceOf[AzureFileSystem]