diff --git a/google-cloud-nio/pom.xml b/google-cloud-nio/pom.xml index 6463539b..5ea390cd 100644 --- a/google-cloud-nio/pom.xml +++ b/google-cloud-nio/pom.xml @@ -71,6 +71,12 @@ truth test + + com.google.auth + google-auth-library-credentials + test + + org.mockito mockito-core diff --git a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java index 8c1a13d7..47183181 100644 --- a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java +++ b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static java.util.Objects.requireNonNull; import com.google.api.gax.paging.Page; import com.google.cloud.storage.Bucket; @@ -25,7 +26,11 @@ import com.google.cloud.storage.StorageException; import com.google.cloud.storage.StorageOptions; import com.google.common.base.Strings; +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; import com.google.common.collect.ImmutableSet; +import com.google.common.util.concurrent.UncheckedExecutionException; import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; @@ -38,10 +43,9 @@ import java.nio.file.attribute.FileTime; import java.nio.file.attribute.UserPrincipalLookupService; import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.concurrent.ExecutionException; import javax.annotation.CheckReturnValue; import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; @@ -66,8 +70,21 @@ public final class CloudStorageFileSystem extends FileSystem { private final CloudStorageFileSystemProvider provider; private final String bucket; private final CloudStorageConfiguration config; - private static final Map> - CONFIG_TO_PROVIDERS_MAP = new HashMap<>(); + private static final LoadingCache + PROVIDER_CACHE_BY_CONFIG = + CacheBuilder.newBuilder() + .build( + new CacheLoader() { + @Override + public CloudStorageFileSystemProvider load(ProviderCacheKey key) { + CloudStorageConfiguration config = key.cloudStorageConfiguration; + StorageOptions storageOptions = key.storageOptions; + String userProject = config.userProject(); + return (storageOptions == null) + ? new CloudStorageFileSystemProvider(userProject) + : new CloudStorageFileSystemProvider(userProject, storageOptions); + } + }); // Users can change this: then this affects every filesystem object created // later, including via SPI. This is meant to be done only once, at the beginning @@ -144,32 +161,7 @@ public static CloudStorageFileSystem forBucket(String bucket) { */ @CheckReturnValue public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfiguration config) { - checkArgument( - !bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket); - checkNotNull(config); - return new CloudStorageFileSystem( - getCloudStorageFileSystemProvider(config, null), bucket, config); - } - - private static CloudStorageFileSystemProvider getCloudStorageFileSystemProvider( - CloudStorageConfiguration config, StorageOptions storageOptions) { - CloudStorageFileSystemProvider newProvider = - (storageOptions == null) - ? new CloudStorageFileSystemProvider(config.userProject()) - : new CloudStorageFileSystemProvider(config.userProject(), storageOptions); - Set existingProviders = CONFIG_TO_PROVIDERS_MAP.get(config); - if (existingProviders == null) { - existingProviders = new HashSet<>(); - } else { - for (CloudStorageFileSystemProvider existiningProvider : existingProviders) { - if (existiningProvider.equals(newProvider)) { - return existiningProvider; - } - } - } - existingProviders.add(newProvider); - CONFIG_TO_PROVIDERS_MAP.put(config, existingProviders); - return newProvider; + return forBucket(bucket, config, null); } /** @@ -192,8 +184,16 @@ public static CloudStorageFileSystem forBucket( String bucket, CloudStorageConfiguration config, @Nullable StorageOptions storageOptions) { checkArgument( !bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket); - return new CloudStorageFileSystem( - getCloudStorageFileSystemProvider(config, storageOptions), bucket, checkNotNull(config)); + checkNotNull(config); + CloudStorageFileSystemProvider result; + ProviderCacheKey providerCacheKey = new ProviderCacheKey(config, storageOptions); + try { + result = PROVIDER_CACHE_BY_CONFIG.get(providerCacheKey); + } catch (ExecutionException | UncheckedExecutionException e) { + throw new IllegalStateException( + "Unable to resolve CloudStorageFileSystemProvider for the provided configuration", e); + } + return new CloudStorageFileSystem(result, bucket, config); } CloudStorageFileSystem( @@ -335,4 +335,50 @@ public String toString() { throw new AssertionError(e); } } + + /** + * In order to cache a {@link CloudStorageFileSystemProvider} we track the config used to + * instantiate that provider. This class creates an immutable key encapsulating the config to + * allow reliable resolution from the cache. + */ + private static final class ProviderCacheKey { + private final CloudStorageConfiguration cloudStorageConfiguration; + @Nullable private final StorageOptions storageOptions; + + public ProviderCacheKey( + CloudStorageConfiguration cloudStorageConfiguration, + @Nullable StorageOptions storageOptions) { + this.cloudStorageConfiguration = + requireNonNull(cloudStorageConfiguration, "cloudStorageConfiguration must be non null"); + this.storageOptions = storageOptions; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof ProviderCacheKey)) { + return false; + } + ProviderCacheKey that = (ProviderCacheKey) o; + return cloudStorageConfiguration.equals(that.cloudStorageConfiguration) + && Objects.equals(storageOptions, that.storageOptions); + } + + @Override + public int hashCode() { + return Objects.hash(cloudStorageConfiguration, storageOptions); + } + + @Override + public String toString() { + return "ConfigTuple{" + + "cloudStorageConfiguration=" + + cloudStorageConfiguration + + ", storageOptions=" + + storageOptions + + '}'; + } + } } diff --git a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java index 35b2e50f..28198495 100644 --- a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java +++ b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java @@ -23,12 +23,16 @@ import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertSame; +import com.google.api.gax.rpc.internal.QuotaProjectIdHidingCredentials; +import com.google.auth.Credentials; +import com.google.cloud.NoCredentials; import com.google.cloud.storage.StorageOptions; import com.google.cloud.storage.contrib.nio.testing.LocalStorageHelper; import com.google.cloud.testing.junit4.MultipleAttemptsRule; import com.google.common.testing.EqualsTester; import com.google.common.testing.NullPointerTester; import java.io.IOException; +import java.lang.reflect.Field; import java.net.URI; import java.nio.channels.SeekableByteChannel; import java.nio.file.FileSystem; @@ -374,6 +378,46 @@ public void testDifferentProvider() throws IOException { assertEquals("new-bucket", destFileSystem.bucket()); } } + + // port of test from + // https://github.com/broadinstitute/cromwell/pull/6491/files#diff-758dbbe823e71cc26fee7bc89cd5c434dfb76e604d51005b8327db59aab96068R300-R336 + @Test + public void ensureMultipleInstancesDoNotCorruptCredentials() throws Exception { + + CloudStorageConfiguration config = + CloudStorageConfiguration.builder() + .permitEmptyPathComponents(true) + .stripPrefixSlash(true) + .usePseudoDirectories(true) + .build(); + + Credentials noCredentials = NoCredentials.getInstance(); + Credentials saCredentials = new QuotaProjectIdHidingCredentials(noCredentials); + + StorageOptions noOptions = + StorageOptions.newBuilder() + .setProjectId("public-project") + .setCredentials(noCredentials) + .build(); + + StorageOptions saOptions = + StorageOptions.newBuilder() + .setProjectId("private-project") + .setCredentials(saCredentials) + .build(); + + CloudStorageFileSystem noFs = + CloudStorageFileSystem.forBucket("public-bucket", config, noOptions); + CloudStorageFileSystem saFs = + CloudStorageFileSystem.forBucket("private-bucket", config, saOptions); + + CloudStoragePath noPath = noFs.getPath("public-file"); + CloudStoragePath saPath = saFs.getPath("private-file"); + + assertThat(credentialsForPath(noPath)).isEqualTo(noCredentials); + assertThat(credentialsForPath(saPath)).isEqualTo(saCredentials); + } + /** * Delete the given directory and all of its contents if non-empty. * @@ -402,4 +446,19 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOEx private void assertMatches(FileSystem fs, PathMatcher matcher, String toMatch, boolean expected) { assertThat(matcher.matches(fs.getPath(toMatch).getFileName())).isEqualTo(expected); } + + private static Credentials credentialsForPath(Path p) + throws NoSuchFieldException, IllegalAccessException { + CloudStorageFileSystemProvider cloudFilesystemProvider = + (CloudStorageFileSystemProvider) p.getFileSystem().provider(); + Field storageOptionsField = + cloudFilesystemProvider.getClass().getDeclaredField("storageOptions"); + storageOptionsField.setAccessible(true); + StorageOptions storageOptions = + (StorageOptions) storageOptionsField.get(cloudFilesystemProvider); + Field credentialsField = + storageOptions.getClass().getSuperclass().getDeclaredField("credentials"); + credentialsField.setAccessible(true); + return (Credentials) credentialsField.get(storageOptions); + } } diff --git a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageIsDirectoryTest.java b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageIsDirectoryTest.java index ebd201d8..16647fb0 100644 --- a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageIsDirectoryTest.java +++ b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageIsDirectoryTest.java @@ -32,6 +32,7 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TestName; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -40,12 +41,17 @@ public class CloudStorageIsDirectoryTest { @Rule public final MultipleAttemptsRule multipleAttemptsRule = new MultipleAttemptsRule(3); + @Rule public final TestName testName = new TestName(); + private StorageOptions mockOptions; private Storage mockStorage; @Before public void before() { - mockOptions = mock(StorageOptions.class); + mockOptions = + mock( + StorageOptions.class, + String.format("storage-options-mock_%s", testName.getMethodName())); mockStorage = mock(Storage.class); when(mockOptions.getService()).thenReturn(mockStorage); CloudStorageFileSystemProvider.setStorageOptions(mockOptions); @@ -54,7 +60,7 @@ public void before() { @Test public void testIsDirectoryNoUserProject() { CloudStorageFileSystem fs = - CloudStorageFileSystem.forBucket("bucket", CloudStorageConfiguration.DEFAULT); + CloudStorageFileSystem.forBucket("bucket", CloudStorageConfiguration.DEFAULT, mockOptions); when(mockStorage.get(BlobId.of("bucket", "test", null))) .thenThrow(new IllegalArgumentException()); Page pages = mock(Page.class); @@ -74,7 +80,9 @@ public void testIsDirectoryNoUserProject() { public void testIsDirectoryWithUserProject() { CloudStorageFileSystem fs = CloudStorageFileSystem.forBucket( - "bucket", CloudStorageConfiguration.builder().userProject("project-id").build()); + "bucket", + CloudStorageConfiguration.builder().userProject("project-id").build(), + mockOptions); when(mockStorage.get(BlobId.of("bucket", "test", null))) .thenThrow(new IllegalArgumentException()); Page pages = mock(Page.class);