Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix requester pays access by updating the google-cloud-nio library to the latest release #7700

Merged
merged 8 commits into from
Mar 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,13 @@ final scalaVersion = System.getProperty('scala.version', '2.11')
final hadoopVersion = System.getProperty('hadoop.version', '3.2.1')
final disqVersion = System.getProperty('disq.version','0.3.6')
final genomicsdbVersion = System.getProperty('genomicsdb.version','1.4.3')
final bigQueryVersion = System.getProperty('bigQuery.version', '1.117.1')
final guavaVersion = System.getProperty('guava.version', '27.1-jre')
final bigQueryVersion = System.getProperty('bigQuery.version', '2.9.0')
final guavaVersion = System.getProperty('guava.version', '31.0.1-jre')
final log4j2Version = System.getProperty('log4j2Version', '2.17.1')
final testNGVersion = '7.0.0'

// Using the shaded version to avoid conflicts between its protobuf dependency
// and that of Hadoop/Spark (either the one we reference explicitly, or the one
// provided by dataproc).
final googleCloudNioDependency = 'com.google.cloud:google-cloud-nio:0.117.0-alpha:shaded'
final googleCloudNioDependency = 'com.google.cloud:google-cloud-nio:0.123.23'


final baseJarName = 'gatk'
final secondaryBaseJarName = 'hellbender'
Expand Down Expand Up @@ -181,7 +179,7 @@ configurations.all {
force 'com.google.guava:guava:' + guavaVersion
// force the htsjdk version so we don't get a different one transitively
force 'com.github.samtools:htsjdk:' + htsjdkVersion
force 'com.google.protobuf:protobuf-java:3.8.0'
force 'com.google.protobuf:protobuf-java:3.19.4'
// force testng dependency so we don't pick up a different version via GenomicsDB
force 'org.testng:testng:' + testNGVersion
force 'org.broadinstitute:barclay:' + barclayVersion
Expand Down Expand Up @@ -267,12 +265,10 @@ dependencies {
implementation 'com.opencsv:opencsv:3.4'
implementation 'com.google.guava:guava:' + guavaVersion
implementation 'com.github.samtools:htsjdk:'+ htsjdkVersion
implementation(googleCloudNioDependency) {
transitive = false
}
implementation(googleCloudNioDependency)

implementation 'com.google.cloud:google-cloud-bigquery:' + bigQueryVersion
implementation 'com.google.cloud:google-cloud-bigquerystorage:1.5.1'
implementation 'com.google.cloud:google-cloud-bigquerystorage:2.9.1'

implementation "gov.nist.math.jama:gov.nist.math.jama:1.1.1"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.json.JsonMapper;
import com.google.common.util.concurrent.ThreadFactoryBuilder;

import org.apache.commons.io.Charsets;
Expand Down Expand Up @@ -226,11 +227,11 @@ private void checkMd5(final HtsgetResponse resp) {
}
}

private ObjectMapper getObjectMapper() {
final ObjectMapper mapper = new ObjectMapper();
mapper.enable(DeserializationFeature.UNWRAP_ROOT_VALUE);
mapper.configure(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES, true);
return mapper;
private JsonMapper getObjectMapper() {
return JsonMapper.builder()
.enable(DeserializationFeature.UNWRAP_ROOT_VALUE)
.configure(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES, true)
.build();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.broadinstitute.hellbender.utils.dragstr;

import com.google.common.io.Files;
import htsjdk.samtools.SAMSequenceDictionary;
import htsjdk.samtools.SAMSequenceDictionaryCodec;
import htsjdk.samtools.util.BufferedLineReader;
Expand All @@ -18,6 +17,7 @@
import org.broadinstitute.hellbender.tools.dragstr.STRDecimationTable;

import java.io.*;
import java.nio.file.Files;

/**
* Class to create and access STR table file contents.
Expand Down Expand Up @@ -89,7 +89,14 @@ public BinaryTableReader<DragstrLocus> locusReader() {
* @return never {@code null}.
*/
public static STRTableFile open(final GATKPath path) {
final File dir = Files.createTempDir();
File dir;
try {
dir = Files.createTempDirectory("STRTableFile").toFile();
}
catch ( IOException e ) {
throw new GATKException("Unable to create temp directory for STRTableFile", e);
}

try {
ZipUtils.unzip(path, dir, ESSENTIAL_FILES);
} catch (final Exception ex) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.broadinstitute.hellbender.utils.dragstr;

import com.google.common.io.Files;
import htsjdk.samtools.SAMSequenceDictionary;
import htsjdk.samtools.SAMSequenceDictionaryCodec;
import htsjdk.samtools.SAMSequenceRecord;
Expand All @@ -17,6 +16,7 @@
import org.broadinstitute.hellbender.utils.tsv.TableWriter;

import java.io.*;
import java.nio.file.Files;
import java.util.LinkedHashMap;
import java.util.Map;

Expand Down Expand Up @@ -79,7 +79,15 @@ public static STRTableFileBuilder newInstance(final SAMSequenceDictionary dictio
Utils.validateArg(maxRepeatLength >= 1, "max repeat length must be positive");
Utils.nonNull(decimationTable, "decimation table must not be negative");
Utils.nonNull(dictionary, "dictionary must not be negative");
final File tempDir = Files.createTempDir();

File tempDir;
try {
tempDir = Files.createTempDirectory("STRTableFileBuilder").toFile();
}
catch ( IOException e ) {
throw new GATKException("Unable to create temp directory for STRTableFileBuilder", e);
}

return new STRTableFileBuilder(tempDir, generateTextSitesFile, dictionary, decimationTable, maxPeriod, maxRepeatLength);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@
import org.broadinstitute.hellbender.utils.io.IOUtils;
import org.broadinstitute.http.nio.HttpFileSystemProvider;
import org.broadinstitute.http.nio.HttpsFileSystemProvider;
import shaded.cloud_nio.com.google.api.gax.retrying.RetrySettings;
import shaded.cloud_nio.com.google.auth.oauth2.GoogleCredentials;
import shaded.cloud_nio.com.google.cloud.http.HttpTransportOptions;
import shaded.cloud_nio.org.threeten.bp.Duration;
import com.google.api.gax.retrying.RetrySettings;
import com.google.auth.oauth2.GoogleCredentials;
import com.google.cloud.http.HttpTransportOptions;
import org.threeten.bp.Duration;

import java.io.*;
import java.net.URL;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
package org.broadinstitute.hellbender.utils.runtime;

import com.google.common.io.Files;
import org.apache.commons.io.IOUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.broadinstitute.hellbender.exceptions.GATKException;
import org.broadinstitute.hellbender.utils.Utils;

import java.io.*;
import java.nio.file.Files;
import java.util.Arrays;
import java.util.Random;
import java.util.concurrent.ExecutionException;
Expand Down Expand Up @@ -87,7 +87,13 @@ public File start() {
processStdinStream = getProcess().getOutputStream();

// create the fifo temp directory, and one FIFO to use for IPC signalling
fifoTempDir = Files.createTempDir();
try {
fifoTempDir = Files.createTempDirectory("StreamingProcessController").toFile();
}
catch ( IOException e ) {
throw new GATKException("Unable to create temp directory for StreamingProcessController", e);
}

ackFIFOFile = createFIFOFile(ACK_FIFO_FILE_NAME);
return ackFIFOFile;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import org.broadinstitute.hellbender.CommandLineProgramTest;
import org.broadinstitute.hellbender.exceptions.UserException;
import org.broadinstitute.hellbender.testutils.IntegrationTestSpec;
import shaded.cloud_nio.com.google.common.collect.Comparators;
import com.google.common.collect.Comparators;

import java.io.File;
import java.io.IOException;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
package org.broadinstitute.hellbender.utils;

import com.google.common.io.Files;
import org.apache.commons.io.FileUtils;
import org.broadinstitute.hellbender.engine.GATKPath;
import org.broadinstitute.hellbender.exceptions.GATKException;
import org.testng.Assert;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

import java.io.*;
import java.nio.file.Files;
import java.util.*;
import java.util.stream.IntStream;

Expand All @@ -29,9 +30,17 @@ public void testZipAndUnzipSingleFile() throws IOException {
Assert.assertFalse(zipFile.exists());
ZipUtils.zip(aBin, new GATKPath(zipFile.toString()));
Assert.assertTrue(zipFile.isFile());
final File unzipRoot = Files.createTempDir();

File unzipRoot;
try {
unzipRoot = Files.createTempDirectory("testZipAndUnzipSingleFile").toFile();
}
catch ( IOException e ) {
throw new GATKException("Unable to create temp directory for testZipAndUnzipSingleFile", e);
}
unzipRoot.delete();
Assert.assertFalse(unzipRoot.exists());

ZipUtils.unzip(new GATKPath(zipFile.toString()), unzipRoot);
Assert.assertTrue(unzipRoot.isDirectory());
final File[] subFiles = unzipRoot.listFiles();
Expand All @@ -51,9 +60,16 @@ public void testZipAndUnzipFullFile(final File root, final String[] files) throw
Assert.assertTrue(zipFile.exists());
Assert.assertTrue(zipFile.isFile());
// now unzip.
final File unzipRoot = Files.createTempDir();
File unzipRoot;
try {
unzipRoot = Files.createTempDirectory("zipAndUnzipFullFileData").toFile();
}
catch ( IOException e ) {
throw new GATKException("Unable to create temp directory for zipAndUnzipFullFileData", e);
}
FileUtils.deleteDirectory(unzipRoot);
Assert.assertFalse(unzipRoot.exists());

ZipUtils.unzip(new GATKPath(zipFile.toString()), unzipRoot);
Assert.assertTrue(unzipRoot.exists());
// now we compare the unzipped content with the original input content.
Expand All @@ -74,9 +90,16 @@ public void testZipFullAndUnzipSubset(final File root, final String[] files) thr
Assert.assertTrue(zipFile.exists());
Assert.assertTrue(zipFile.isFile());
// now unzip.
final File unzipRoot = Files.createTempDir();
File unzipRoot;
try {
unzipRoot = Files.createTempDirectory("testZipFullAndUnzipSubset").toFile();
}
catch ( IOException e ) {
throw new GATKException("Unable to create temp directory for testZipFullAndUnzipSubset", e);
}
FileUtils.deleteDirectory(unzipRoot);
Assert.assertFalse(unzipRoot.exists());

final Random rdn = new Random(Arrays.hashCode(files));
final String[] only = IntStream.range(0, files.length)
.filter(i -> rdn.nextBoolean())
Expand Down Expand Up @@ -113,7 +136,13 @@ public void testZipSubsetAndUnzipFull(final File root, final String[] files) thr
Assert.assertTrue(zipFile.exists());
Assert.assertTrue(zipFile.isFile());
// now unzip.
final File unzipRoot = Files.createTempDir();
File unzipRoot;
try {
unzipRoot = Files.createTempDirectory("testZipSubsetAndUnzipFull").toFile();
}
catch ( IOException e ) {
throw new GATKException("Unable to create temp directory for testZipSubsetAndUnzipFull", e);
}
FileUtils.deleteDirectory(unzipRoot);
Assert.assertFalse(unzipRoot.exists());

Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package org.broadinstitute.hellbender.utils.gcs;

import com.google.cloud.storage.StorageException;
import com.google.cloud.storage.contrib.nio.CloudStorageConfiguration;
import com.google.cloud.storage.contrib.nio.SeekableByteChannelPrefetcher;
import htsjdk.samtools.util.IOUtil;
import org.broadinstitute.hellbender.GATKBaseTest;
import org.broadinstitute.hellbender.engine.GATKPath;
import org.broadinstitute.hellbender.testutils.BaseTest;
import org.broadinstitute.hellbender.testutils.MiniClusterUtils;
import org.broadinstitute.hellbender.utils.config.ConfigFactory;
import org.broadinstitute.hellbender.utils.io.IOUtils;
Expand All @@ -25,12 +27,37 @@

public final class BucketUtilsUnitTest extends GATKBaseTest {

public static final String LINE_IN_REMOTE_TEXT_FILE = "The Project Gutenberg EBook of The Adventures of Sherlock Holmes";

/**
* This file is in a public requester pays bucket and is owned by the broad-gatk-test project. It must be owned by
* a different project than the service account doing the testing or the test may fail because it can access the
* file directly through alternative permissions.
*/
public static final String FILE_IN_REQUESTER_PAYS_BUCKET = "gs://hellbender-requester-pays-test/test/resources/nio/big.txt";

static {
setDefaultNioOptions();
}

private static void setDefaultNioOptions() {
BucketUtils.setGlobalNIODefaultOptions(
ConfigFactory.getInstance().getGATKConfig().gcsMaxRetries(),
ConfigFactory.getInstance().getGATKConfig().gcsProjectForRequesterPays());
}

private static void setNoProjectForRequesterPays() {
BucketUtils.setGlobalNIODefaultOptions(
ConfigFactory.getInstance().getGATKConfig().gcsMaxRetries(),
null);
}

private static void setRequesterPays(){
BucketUtils.setGlobalNIODefaultOptions(
ConfigFactory.getInstance().getGATKConfig().gcsMaxRetries(),
BaseTest.getGCPTestProject());
}

@Test(groups={"bucket"})
public void testIsGcsUrl(){
Assert.assertTrue(BucketUtils.isGcsUrl("gs://abucket/bucket"));
Expand Down Expand Up @@ -263,7 +290,7 @@ public void testCreateSignedUrl() throws IOException {
Assert.assertTrue(signed.contains("big.txt"), "path is missing blob name, "+ signed);
Assert.assertTrue(Files.exists(path), "path doesn't exist: " + signed);
try(final Stream<String> lines = Files.lines(path)) {
Assert.assertTrue(lines.anyMatch(line -> line.contains("The Project Gutenberg EBook of The Adventures of Sherlock Holmes")), "blob data is incorrect, " + signed);
Assert.assertTrue(lines.anyMatch(line -> line.contains(LINE_IN_REMOTE_TEXT_FILE)), "blob data is incorrect, " + signed);
}
}

Expand All @@ -275,4 +302,29 @@ public void testBucketPathToPublicHttpUrl(){
Assert.assertEquals(publicHttpLink, "https://storage.googleapis.com/hellbender/test/resources/nio/big.txt");
Assert.assertTrue(Files.exists(IOUtils.getPath(publicHttpLink)));
}

@Test(groups="cloud", singleThreaded=true)
public void testRequesterPays() throws IOException {

// IOUtils.getPath() triggers the requester pays check, it's not lazily delayed until trying to read
// the file.
try {
//Assert that this fails when no project is provided
setNoProjectForRequesterPays();
Assert.assertThrows(StorageException.class, () -> {
try( final Stream<String> lines = Files.lines(IOUtils.getPath(FILE_IN_REQUESTER_PAYS_BUCKET))) {
lines.anyMatch(line -> line.contains(LINE_IN_REMOTE_TEXT_FILE));
}
});

setRequesterPays();
try( final Stream<String> lines = Files.lines(IOUtils.getPath(FILE_IN_REQUESTER_PAYS_BUCKET))) {
Assert.assertTrue(lines.anyMatch(line -> line.contains(LINE_IN_REMOTE_TEXT_FILE)),
"Failed to read from file.");
}
} finally {
setDefaultNioOptions();
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public void writePrivateFile() throws IOException {
try (OutputStream os = Files.newOutputStream(outputPath)) {
os.write(42);
}
} catch (shaded.cloud_nio.com.google.api.client.http.HttpResponseException forbidden) {
} catch (com.google.api.client.http.HttpResponseException forbidden) {
helpDebugAuthError();
throw forbidden;
}
Expand Down