Remove GCS Bucket Exists Check (#60899) (#60917)

Same as #43288 for GCS. We don't need to do the bucket exists check before using the repo, that just needlessly increases the necessary permissions for using the GCS repository.
elastic · Aug 11, 2020 · 0813c44 · 0813c44
1 parent d52d003
commit 0813c44
Show file tree

Hide file tree

Showing 6 changed files with 12 additions and 63 deletions.
diff --git a/...ory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java b/...ory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java
@@ -25,7 +25,6 @@
 import com.google.cloud.storage.Blob;
 import com.google.cloud.storage.BlobId;
 import com.google.cloud.storage.BlobInfo;
-import com.google.cloud.storage.Bucket;
 import com.google.cloud.storage.Storage;
 import com.google.cloud.storage.Storage.BlobListOption;
 import com.google.cloud.storage.StorageBatch;
@@ -39,7 +38,6 @@
 import org.elasticsearch.common.blobstore.BlobMetadata;
 import org.elasticsearch.common.blobstore.BlobPath;
 import org.elasticsearch.common.blobstore.BlobStore;
-import org.elasticsearch.common.blobstore.BlobStoreException;
 import org.elasticsearch.common.blobstore.DeleteResult;
 import org.elasticsearch.common.blobstore.support.PlainBlobMetadata;
 import org.elasticsearch.common.collect.MapBuilder;
@@ -114,9 +112,6 @@ class GoogleCloudStorageBlobStore implements BlobStore {
         this.storageService = storageService;
         this.stats = new GoogleCloudStorageOperationsStats(bucketName);
         this.bufferSize = bufferSize;
-        if (doesBucketExist(bucketName) == false) {
-            throw new BlobStoreException("Bucket [" + bucketName + "] does not exist");
-        }
     }
 
     private Storage client() throws IOException {
@@ -133,21 +128,6 @@ public void close() {
         storageService.closeRepositoryClient(repositoryName);
     }
 
-    /**
-     * Return true iff the given bucket exists
-     *
-     * @param bucketName name of the bucket
-     * @return true iff the bucket exists
-     */
-    private boolean doesBucketExist(String bucketName) {
-        try {
-            final Bucket bucket = SocketAccess.doPrivilegedIOException(() -> client().get(bucketName));
-            return bucket != null;
-        } catch (final Exception e) {
-            throw new BlobStoreException("Unable to check if bucket [" + bucketName + "] exists", e);
-        }
-    }
-
     /**
      * List blobs in the specific bucket under the specified path. The path root is removed.
      *
@@ -171,7 +151,7 @@ Map<String, BlobMetadata> listBlobsByPrefix(String path, String prefix) throws I
         final String pathPrefix = buildKey(path, prefix);
         final MapBuilder<String, BlobMetadata> mapBuilder = MapBuilder.newMapBuilder();
         SocketAccess.doPrivilegedVoidIOException(
-            () -> client().get(bucketName).list(BlobListOption.currentDirectory(), BlobListOption.prefix(pathPrefix)).iterateAll().forEach(
+            () -> client().list(bucketName, BlobListOption.currentDirectory(), BlobListOption.prefix(pathPrefix)).iterateAll().forEach(
                 blob -> {
                     assert blob.getName().startsWith(path);
                     if (blob.isDirectory() == false) {
@@ -186,7 +166,7 @@ Map<String, BlobContainer> listChildren(BlobPath path) throws IOException {
         final String pathStr = path.buildAsString();
         final MapBuilder<String, BlobContainer> mapBuilder = MapBuilder.newMapBuilder();
         SocketAccess.doPrivilegedVoidIOException
-            (() -> client().get(bucketName).list(BlobListOption.currentDirectory(), BlobListOption.prefix(pathStr)).iterateAll().forEach(
+            (() -> client().list(bucketName, BlobListOption.currentDirectory(), BlobListOption.prefix(pathStr)).iterateAll().forEach(
                 blob -> {
                     if (blob.isDirectory()) {
                         assert blob.getName().startsWith(pathStr);
@@ -378,7 +358,7 @@ private void writeBlobMultipart(BlobInfo blobInfo, InputStream inputStream, long
     DeleteResult deleteDirectory(String pathStr) throws IOException {
         return SocketAccess.doPrivilegedIOException(() -> {
             DeleteResult deleteResult = DeleteResult.ZERO;
-            Page<Blob> page = client().get(bucketName).list(BlobListOption.prefix(pathStr));
+            Page<Blob> page = client().list(bucketName, BlobListOption.prefix(pathStr));
             do {
                 final Collection<String> blobsToDelete = new ArrayList<>();
                 final AtomicLong blobsDeleted = new AtomicLong(0L);

diff --git a/.../java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainerRetriesTests.java b/.../java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainerRetriesTests.java
@@ -22,7 +22,6 @@
 import com.google.cloud.http.HttpTransportOptions;
 import com.google.cloud.storage.StorageException;
 import com.google.cloud.storage.StorageOptions;
-import com.sun.net.httpserver.HttpContext;
 import com.sun.net.httpserver.HttpHandler;
 import fixture.gcs.FakeOAuth2HttpHandler;
 import org.apache.http.HttpStatus;
@@ -57,7 +56,6 @@
 import java.net.SocketTimeoutException;
 import java.util.Arrays;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
@@ -157,21 +155,9 @@ StorageOptions createStorageOptions(final GoogleCloudStorageClientSettings clien
         };
         service.refreshAndClearCache(GoogleCloudStorageClientSettings.load(clientSettings.build()));
 
-        final List<HttpContext> httpContexts = Arrays.asList(
-            // Auth
-            httpServer.createContext("/token", new FakeOAuth2HttpHandler()),
-            // Does bucket exists?
-            httpServer.createContext("/storage/v1/b/bucket", safeHandler(exchange -> {
-                byte[] response = ("{\"kind\":\"storage#bucket\",\"name\":\"bucket\",\"id\":\"0\"}").getBytes(UTF_8);
-                exchange.getResponseHeaders().add("Content-Type", "application/json; charset=utf-8");
-                exchange.sendResponseHeaders(HttpStatus.SC_OK, response.length);
-                exchange.getResponseBody().write(response);
-            }))
-        );
-
+        httpServer.createContext("/token", new FakeOAuth2HttpHandler());
         final GoogleCloudStorageBlobStore blobStore = new GoogleCloudStorageBlobStore("bucket", client, "repo", service,
             randomIntBetween(1, 8) * 1024);
-        httpContexts.forEach(httpContext -> httpServer.removeContext(httpContext));
 
         return new GoogleCloudStorageBlobContainer(BlobPath.cleanPath(), blobStore);
     }

diff --git a/...t/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java b/...t/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java
@@ -38,7 +38,6 @@
 import org.elasticsearch.common.blobstore.BlobContainer;
 import org.elasticsearch.common.blobstore.BlobPath;
 import org.elasticsearch.common.blobstore.BlobStore;
-import org.elasticsearch.common.blobstore.BlobStoreException;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.io.Streams;
 import org.elasticsearch.common.regex.Regex;
@@ -52,7 +51,6 @@
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.repositories.RepositoriesService;
 import org.elasticsearch.repositories.Repository;
-import org.elasticsearch.repositories.RepositoryException;
 import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
 import org.elasticsearch.repositories.blobstore.ESMockAPIBasedRepositoryIntegTestCase;
 import org.junit.BeforeClass;
@@ -74,8 +72,6 @@
 import static org.elasticsearch.repositories.gcs.GoogleCloudStorageClientSettings.TOKEN_URI_SETTING;
 import static org.elasticsearch.repositories.gcs.GoogleCloudStorageRepository.BUCKET;
 import static org.elasticsearch.repositories.gcs.GoogleCloudStorageRepository.CLIENT_NAME;
-import static org.hamcrest.Matchers.instanceOf;
-import static org.hamcrest.Matchers.is;
 
 @SuppressForbidden(reason = "this test uses a HttpServer to emulate a Google Cloud Storage endpoint")
 public class GoogleCloudStorageBlobStoreRepositoryTests extends ESMockAPIBasedRepositoryIntegTestCase {
@@ -215,16 +211,6 @@ public void testWriteReadLarge() throws IOException {
         }
     }
 
-    public void testBucketDoesNotExist() {
-        RepositoryException ex = expectThrows(RepositoryException.class, () ->
-                client().admin().cluster().preparePutRepository("invalid")
-                        .setType(repositoryType())
-                        .setVerify(true)
-                        .setSettings(Settings.builder().put(repositorySettings()).put("bucket", "missing")).get());
-        assertThat(ex.getCause(), instanceOf(BlobStoreException.class));
-        assertThat(ex.getCause().getMessage(), is("Bucket [missing] does not exist"));
-    }
-
     public static class TestGoogleCloudStoragePlugin extends GoogleCloudStoragePlugin {
 
         public TestGoogleCloudStoragePlugin(Settings settings) {

diff --git a/...ins/repository-gcs/src/test/resources/rest-api-spec/test/repository_gcs/20_repository.yml b/...ins/repository-gcs/src/test/resources/rest-api-spec/test/repository_gcs/20_repository.yml
@@ -192,7 +192,7 @@ setup:
 "Register a repository with a non existing bucket":
 
   - do:
-      catch: /repository_exception/
+      catch: /repository_verification_exception/
       snapshot.create_repository:
         repository: repository
         body:
@@ -205,7 +205,7 @@ setup:
 "Register a repository with a non existing client":
 
   - do:
-      catch: /repository_exception/
+      catch: /repository_verification_exception/
       snapshot.create_repository:
         repository: repository
         body:

diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java
@@ -1203,7 +1203,7 @@ public String startVerification() {
                 }
                 return seed;
             }
-        } catch (IOException exp) {
+        } catch (Exception exp) {
             throw new RepositoryVerificationException(metadata.name(), "path " + basePath() + " is not accessible on master node", exp);
         }
     }
@@ -1214,7 +1214,7 @@ public void endVerification(String seed) {
             try {
                 final String testPrefix = testBlobPrefix(seed);
                 blobStore().blobContainer(basePath().add(testPrefix)).delete();
-            } catch (IOException exp) {
+            } catch (Exception exp) {
                 throw new RepositoryVerificationException(metadata.name(), "cannot delete test data at " + basePath(), exp);
             }
         }
@@ -2152,7 +2152,7 @@ public void verify(String seed, DiscoveryNode localNode) {
         if (isReadOnly()) {
             try {
                 latestIndexBlobId();
-            } catch (IOException e) {
+            } catch (Exception e) {
                 throw new RepositoryVerificationException(metadata.name(), "path " + basePath() +
                     " is not accessible on node " + localNode, e);
             }
@@ -2163,7 +2163,7 @@ public void verify(String seed, DiscoveryNode localNode) {
                 try (InputStream stream = bytes.streamInput()) {
                     testBlobContainer.writeBlob("data-" + localNode.getId() + ".dat", stream, bytes.length(), true);
                 }
-            } catch (IOException exp) {
+            } catch (Exception exp) {
                 throw new RepositoryVerificationException(metadata.name(), "store location [" + blobStore() +
                     "] is not accessible on the node [" + localNode + "]", exp);
             }
@@ -2178,7 +2178,7 @@ public void verify(String seed, DiscoveryNode localNode) {
                     "] cannot be accessed on the node [" + localNode + "]. " +
                     "This might indicate that the store [" + blobStore() + "] is not shared between this node and the master node or " +
                     "that permissions on the store don't allow reading files written by the master node", e);
-            } catch (IOException e) {
+            } catch (Exception e) {
                 throw new RepositoryVerificationException(metadata.name(), "Failed to verify repository", e);
             }
         }

diff --git a/test/fixtures/gcs-fixture/src/main/java/fixture/gcs/GoogleCloudStorageHttpHandler.java b/test/fixtures/gcs-fixture/src/main/java/fixture/gcs/GoogleCloudStorageHttpHandler.java
@@ -132,10 +132,7 @@ public void handle(final HttpExchange exchange) throws IOException {
 
             } else if (Regex.simpleMatch("GET /storage/v1/b/" + bucket + "*", request)) {
                 // GET Bucket https://cloud.google.com/storage/docs/json_api/v1/buckets/get
-                byte[] response = ("{\"kind\":\"storage#bucket\",\"name\":\""+ bucket + "\",\"id\":\"0\"}").getBytes(UTF_8);
-                exchange.getResponseHeaders().add("Content-Type", "application/json; charset=utf-8");
-                exchange.sendResponseHeaders(RestStatus.OK.getStatus(), response.length);
-                exchange.getResponseBody().write(response);
+                throw new AssertionError("Should not call get bucket API");
 
             } else if (Regex.simpleMatch("GET /download/storage/v1/b/" + bucket + "/o/*", request)) {
                 // Download Object https://cloud.google.com/storage/docs/request-body