Fix Google Cloud requester pays access harder. #7716 (#7730)

* The previous attempt to fix requester pays didn't fix it in many cases. This incorporates a newer version of the NIO library with several patches to fix edge cases we were hitting. * googleapis/java-storage-nio#849 * googleapis/java-storage-nio#856 * googleapis/java-storage-nio#857 * upgrade com.google.cloud:google-cloud-nio:0.123.23 ->0.123.25 fixes #7716 Co-authored-by: David Roazen <droazen@broadinstitute.org>
broadinstitute · Apr 7, 2022 · 9d4fe23 · 9d4fe23
1 parent ea82339
commit 9d4fe23
Show file tree

Hide file tree

Showing 5 changed files with 154 additions and 3 deletions.
diff --git a/build.gradle b/build.gradle
@@ -73,8 +73,7 @@ final guavaVersion = System.getProperty('guava.version', '31.0.1-jre')
 final log4j2Version = System.getProperty('log4j2Version', '2.17.1')
 final testNGVersion = '7.0.0'
 
-final googleCloudNioDependency = 'com.google.cloud:google-cloud-nio:0.123.23'
-
+final googleCloudNioDependency = 'com.google.cloud:google-cloud-nio:0.123.25'
 
 final baseJarName = 'gatk'
 final secondaryBaseJarName = 'hellbender'

diff --git a/src/test/java/org/broadinstitute/hellbender/engine/RequesterPaysIntegrationTest.java b/src/test/java/org/broadinstitute/hellbender/engine/RequesterPaysIntegrationTest.java
@@ -0,0 +1,89 @@
+package org.broadinstitute.hellbender.engine;
+
+import com.google.cloud.storage.StorageException;
+import org.broadinstitute.hellbender.CommandLineProgramTest;
+import org.broadinstitute.hellbender.cmdline.StandardArgumentDefinitions;
+import org.broadinstitute.hellbender.exceptions.UserException;
+import org.broadinstitute.hellbender.testutils.ArgumentsBuilder;
+import org.broadinstitute.hellbender.testutils.IntegrationTestSpec;
+import org.broadinstitute.hellbender.tools.examples.ExampleReadWalkerWithVariants;
+import org.broadinstitute.hellbender.utils.io.IOUtils;
+import org.testng.Assert;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import java.io.File;
+import java.io.IOException;
+
+public class RequesterPaysIntegrationTest extends CommandLineProgramTest {
+
+    @Override
+    public String getTestedToolName() {
+        return ExampleReadWalkerWithVariants.class.getSimpleName();
+    }
+
+    @DataProvider
+    public Object[][] getRequesterPaysPaths() {
+
+        //Files here are paid for by the bucket owners
+        final String OWNER = getGCPTestInputPath() + "org/broadinstitute/hellbender/engine/RequesterPaysIntegrationTest/";
+
+        //Files here are requester pays
+        final String REQUESTER = getGCPRequesterPaysBucket() + "test/resources/nio/";
+
+        return new Object[][]{
+                {OWNER, OWNER, OWNER, OWNER, false},
+                {OWNER, OWNER, OWNER, REQUESTER, true},
+                {OWNER, OWNER, REQUESTER, OWNER, true},
+                {OWNER, OWNER, REQUESTER, REQUESTER, true},
+                {OWNER, REQUESTER, OWNER, OWNER, true},
+                {OWNER, REQUESTER, OWNER, REQUESTER, true},
+                {OWNER, REQUESTER, REQUESTER, OWNER, true},
+                {OWNER, REQUESTER, REQUESTER, REQUESTER, true},
+                {REQUESTER, OWNER, OWNER, OWNER, true},
+                {REQUESTER, OWNER, OWNER, REQUESTER, true},
+                {REQUESTER, OWNER, REQUESTER, OWNER, true},
+                {REQUESTER, OWNER, REQUESTER, REQUESTER, true},
+                {REQUESTER, REQUESTER, OWNER, OWNER, true},
+                {REQUESTER, REQUESTER, OWNER, REQUESTER, true},
+                {REQUESTER, REQUESTER, REQUESTER, OWNER, true},
+                {REQUESTER, REQUESTER, REQUESTER, REQUESTER, true},
+        };
+    }
+
+    // Disabled due to https://github.com/broadinstitute/gatk/issues/7763
+    @Test(dataProvider = "getRequesterPaysPaths", groups="cloud", enabled = false)
+    public void testMixedNormalAndRequesterPays(String referenceBase, String bamBase, String vcfBase,
+                                                  String intervalBase, boolean requiresRequesterPays) throws IOException {
+        final ArgumentsBuilder args = new ArgumentsBuilder();
+        final File output = IOUtils.createTempFile("out", ".txt");
+        args.addReference(referenceBase + "hg19mini.fasta")
+                        .addInput(bamBase + "reads_data_source_test1.bam"  )
+                        .addVCF(vcfBase + "example_variants_withSequenceDict.vcf")
+                        .addInterval(intervalBase + "hg19mini.all.interval_list")
+                        .addOutput(output)
+                        .add(StandardArgumentDefinitions.NIO_PROJECT_FOR_REQUESTER_PAYS_LONG_NAME, getGCPTestProject());
+                runCommandLine(args);
+        IntegrationTestSpec.assertEqualTextFiles(output, new File(packageRootTestDir+"engine/RequesterPaysIntegrationTest/expected_ExampleReadWalkerWithVariantsIntegrationTest_output.txt"));
+    }
+
+    // Disabled due to https://github.com/broadinstitute/gatk/issues/7763
+    @Test(dataProvider = "getRequesterPaysPaths", groups="cloud", enabled = false)
+    public void testWithoutRequesterPaysArgument(String referenceBase, String bamBase, String vcfBase,
+                                                  String intervalBase, boolean requiresRequesterPays) {
+        final ArgumentsBuilder args = new ArgumentsBuilder();
+        args.addReference(referenceBase + "hg19mini.fasta")
+                .addInput(bamBase + "reads_data_source_test1.bam"  )
+                .addVCF(vcfBase + "example_variants_withSequenceDict.vcf")
+                .addInterval(intervalBase + "hg19mini.all.interval_list")
+                .addOutput(IOUtils.createTempFile("out", ".txt"));
+        try{
+            runCommandLine(args);
+            Assert.assertFalse(requiresRequesterPays, "This shouldn't have reached here because it should if thrown.");
+        } catch (final UserException.CouldNotReadInputFile | StorageException ex){
+            if( !requiresRequesterPays){
+                Assert.fail("This should have thrown an exception.");
+            }
+        }
+    }
+}
diff --git a/src/test/java/org/broadinstitute/hellbender/utils/gcs/BucketUtilsUnitTest.java b/src/test/java/org/broadinstitute/hellbender/utils/gcs/BucketUtilsUnitTest.java
@@ -2,6 +2,7 @@
 
 import com.google.cloud.storage.StorageException;
 import com.google.cloud.storage.contrib.nio.CloudStorageConfiguration;
+import com.google.cloud.storage.contrib.nio.CloudStorageFileSystemProvider;
 import com.google.cloud.storage.contrib.nio.SeekableByteChannelPrefetcher;
 import htsjdk.samtools.util.IOUtil;
 import org.broadinstitute.hellbender.GATKBaseTest;
@@ -17,6 +18,8 @@
 import java.io.File;
 import java.io.FileWriter;
 import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.nio.channels.SeekableByteChannel;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -34,7 +37,7 @@ public final class BucketUtilsUnitTest extends GATKBaseTest {
      * 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";
+    public static final String FILE_IN_REQUESTER_PAYS_BUCKET = getGCPRequesterPaysBucket() + "test/resources/nio/big.txt";
 
     static {
         setDefaultNioOptions();

diff --git a/...esterPaysIntegrationTest/expected_ExampleReadWalkerWithVariantsIntegrationTest_output.txt b/...esterPaysIntegrationTest/expected_ExampleReadWalkerWithVariantsIntegrationTest_output.txt
@@ -0,0 +1,42 @@
+Read at 1:200-275:
+ACCCTAACCCTAACCCTAACCCTAACCATAACCCTAAGACTAACCCTAAACCTAACCCTCATAATCGAAATACAAC
+Overlapping variant at 1:200-200: Ref: G* Alt(s): [A]
+Overlapping variant at 1:203-206: Ref: GGGG* Alt(s): [G]
+
+Read at 1:205-280:
+ACCCTAACCCTAACCCTAACCCTAACCATAACCCTAAGACTAACCCTAAACCTAACCCTCATAATCGAAATACAAC
+Overlapping variant at 1:203-206: Ref: GGGG* Alt(s): [G]
+Overlapping variant at 1:280-280: Ref: G* Alt(s): [A]
+
+Read at 1:210-285:
+ACCCTAACCCTAACCCTAACCCTAACCATAACCCTAAGACTAACCCTAAACCTAACCCTCATAATCGAAATACAAC
+Overlapping variant at 1:280-280: Ref: G* Alt(s): [A]
+Overlapping variant at 1:284-286: Ref: GGG* Alt(s): [G]
+Overlapping variant at 1:285-285: Ref: G* Alt(s): [A]
+
+Read at 1:1000-1075:
+ACCCTAACCCTAACCCTAACCCTAACCATAACCCTAAGACTAACCCTAAACCTAACCCTCATAATCGAAATACAAC
+Overlapping variant at 1:1000-1000: Ref: G* Alt(s): [A]
+
+Read at 1:1100-1175:
+ACCCTAACCCTAACCCTAACCCTAACCATAACCCTAAGACTAACCCTAAACCTAACCCTCATAATCGAAATACAAC
+
+Read at 2:500-575:
+ACCCTAACCCTAACCCTAACCCTAACCATAACCCTAAGACTAACCCTAAACCTAACCCTCATAATCGAAATACAAC
+
+Read at 2:550-625:
+ACCCTAACCCTAACCCTAACCCTAACCATAACCCTAAGACTAACCCTAAACCTAACCCTCATAATCGAAATACAAC
+
+Read at 2:650-725:
+ACCCTAACCCTAACCCTAACCCTAACCATAACCCTAAGACTAACCCTAAACCTAACCCTCATAATCGAAATACAAC
+
+Read at 3:300-375:
+ACCCTAACCCTAACCCTAACCCTAACCATAACCCTAAGACTAACCCTAAACCTAACCCTCATAATCGAAATACAAC
+
+Read at 3:400-475:
+ACCCTAACCCTAACCCTAACCCTAACCATAACCCTAAGACTAACCCTAAACCTAACCCTCATAATCGAAATACAAC
+
+Read at 4:700-775:
+ACCCTAACCCTAACCCTAACCCTAACCATAACCCTAAGACTAACCCTAAACCTAACCCTCATAATCGAAATACAAC
+Overlapping variant at 4:775-775: Ref: G* Alt(s): [A]
+
diff --git a/src/testUtils/java/org/broadinstitute/hellbender/testutils/BaseTest.java b/src/testUtils/java/org/broadinstitute/hellbender/testutils/BaseTest.java
@@ -1,5 +1,6 @@
 package org.broadinstitute.hellbender.testutils;
 
+import com.google.common.base.Strings;
 import htsjdk.samtools.util.Log;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -107,6 +108,23 @@ public void setTestVerbosity(){
 
     public static final Logger logger = LogManager.getLogger("org.broadinstitute.gatk");
 
+    /**
+     *  This is a public requester pays bucket 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 REQUESTER_PAYS_BUCKET = "gs://hellbender-requester-pays-test/";
+
+    /**
+     * A publicly readable GCS bucket set as requester pays, this should not be owned by the same project that is set
+     * as {@link BaseTest#getGCPTestProject()} or the tests for requester pays access may be invalid.
+     * @return HELLBENDER_REQUESTER_PAYS_BUCKET env. var if defined, {@value BaseTest#REQUESTER_PAYS_BUCKET}.
+     */
+    public static final String getGCPRequesterPaysBucket(){
+        final String valueFromEnvironment = System.getenv("HELLBENDER_REQUESTER_PAYS_BUCKET");
+        return Strings.isNullOrEmpty(valueFromEnvironment) ? REQUESTER_PAYS_BUCKET : valueFromEnvironment;
+    }
+
     /**
      * name of the google cloud project that stores the data and will run the code
      * @return HELLBENDER_TEST_PROJECT env. var if defined, throws otherwise.