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

Same as https://github.com/elastic/elasticsearch/pull/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.
This commit is contained in:
Armin Braun 2020-08-11 09:54:27 +02:00 committed by GitHub
parent d51eae6e9f
commit 3e2dfc6eac
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 12 additions and 63 deletions

View File

@ -38,7 +38,6 @@ import org.elasticsearch.common.SuppressForbidden;
import org.elasticsearch.common.blobstore.BlobContainer; import org.elasticsearch.common.blobstore.BlobContainer;
import org.elasticsearch.common.blobstore.BlobPath; import org.elasticsearch.common.blobstore.BlobPath;
import org.elasticsearch.common.blobstore.BlobStore; import org.elasticsearch.common.blobstore.BlobStore;
import org.elasticsearch.common.blobstore.BlobStoreException;
import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.io.Streams; import org.elasticsearch.common.io.Streams;
import org.elasticsearch.common.regex.Regex; import org.elasticsearch.common.regex.Regex;
@ -52,7 +51,6 @@ import org.elasticsearch.indices.recovery.RecoverySettings;
import org.elasticsearch.plugins.Plugin; import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.repositories.RepositoriesService; import org.elasticsearch.repositories.RepositoriesService;
import org.elasticsearch.repositories.Repository; import org.elasticsearch.repositories.Repository;
import org.elasticsearch.repositories.RepositoryException;
import org.elasticsearch.repositories.blobstore.BlobStoreRepository; import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
import org.elasticsearch.repositories.blobstore.ESMockAPIBasedRepositoryIntegTestCase; import org.elasticsearch.repositories.blobstore.ESMockAPIBasedRepositoryIntegTestCase;
import org.junit.BeforeClass; import org.junit.BeforeClass;
@ -74,8 +72,6 @@ import static org.elasticsearch.repositories.gcs.GoogleCloudStorageClientSetting
import static org.elasticsearch.repositories.gcs.GoogleCloudStorageClientSettings.TOKEN_URI_SETTING; 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.BUCKET;
import static org.elasticsearch.repositories.gcs.GoogleCloudStorageRepository.CLIENT_NAME; 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") @SuppressForbidden(reason = "this test uses a HttpServer to emulate a Google Cloud Storage endpoint")
public class GoogleCloudStorageBlobStoreRepositoryTests extends ESMockAPIBasedRepositoryIntegTestCase { public class GoogleCloudStorageBlobStoreRepositoryTests extends ESMockAPIBasedRepositoryIntegTestCase {
@ -215,16 +211,6 @@ public class GoogleCloudStorageBlobStoreRepositoryTests extends ESMockAPIBasedRe
} }
} }
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 static class TestGoogleCloudStoragePlugin extends GoogleCloudStoragePlugin {
public TestGoogleCloudStoragePlugin(Settings settings) { public TestGoogleCloudStoragePlugin(Settings settings) {

View File

@ -25,7 +25,6 @@ import com.google.cloud.WriteChannel;
import com.google.cloud.storage.Blob; import com.google.cloud.storage.Blob;
import com.google.cloud.storage.BlobId; import com.google.cloud.storage.BlobId;
import com.google.cloud.storage.BlobInfo; import com.google.cloud.storage.BlobInfo;
import com.google.cloud.storage.Bucket;
import com.google.cloud.storage.Storage; import com.google.cloud.storage.Storage;
import com.google.cloud.storage.Storage.BlobListOption; import com.google.cloud.storage.Storage.BlobListOption;
import com.google.cloud.storage.StorageBatch; import com.google.cloud.storage.StorageBatch;
@ -39,7 +38,6 @@ import org.elasticsearch.common.blobstore.BlobContainer;
import org.elasticsearch.common.blobstore.BlobMetadata; import org.elasticsearch.common.blobstore.BlobMetadata;
import org.elasticsearch.common.blobstore.BlobPath; import org.elasticsearch.common.blobstore.BlobPath;
import org.elasticsearch.common.blobstore.BlobStore; import org.elasticsearch.common.blobstore.BlobStore;
import org.elasticsearch.common.blobstore.BlobStoreException;
import org.elasticsearch.common.blobstore.DeleteResult; import org.elasticsearch.common.blobstore.DeleteResult;
import org.elasticsearch.common.blobstore.support.PlainBlobMetadata; import org.elasticsearch.common.blobstore.support.PlainBlobMetadata;
import org.elasticsearch.common.collect.MapBuilder; import org.elasticsearch.common.collect.MapBuilder;
@ -114,9 +112,6 @@ class GoogleCloudStorageBlobStore implements BlobStore {
this.storageService = storageService; this.storageService = storageService;
this.stats = new GoogleCloudStorageOperationsStats(bucketName); this.stats = new GoogleCloudStorageOperationsStats(bucketName);
this.bufferSize = bufferSize; this.bufferSize = bufferSize;
if (doesBucketExist(bucketName) == false) {
throw new BlobStoreException("Bucket [" + bucketName + "] does not exist");
}
} }
private Storage client() throws IOException { private Storage client() throws IOException {
@ -133,21 +128,6 @@ class GoogleCloudStorageBlobStore implements BlobStore {
storageService.closeRepositoryClient(repositoryName); 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. * List blobs in the specific bucket under the specified path. The path root is removed.
* *
@ -171,7 +151,7 @@ class GoogleCloudStorageBlobStore implements BlobStore {
final String pathPrefix = buildKey(path, prefix); final String pathPrefix = buildKey(path, prefix);
final MapBuilder<String, BlobMetadata> mapBuilder = MapBuilder.newMapBuilder(); final MapBuilder<String, BlobMetadata> mapBuilder = MapBuilder.newMapBuilder();
SocketAccess.doPrivilegedVoidIOException( SocketAccess.doPrivilegedVoidIOException(
() -> client().get(bucketName).list(BlobListOption.currentDirectory(), BlobListOption.prefix(pathPrefix)).iterateAll().forEach( () -> client().list(bucketName, BlobListOption.currentDirectory(), BlobListOption.prefix(pathPrefix)).iterateAll().forEach(
blob -> { blob -> {
assert blob.getName().startsWith(path); assert blob.getName().startsWith(path);
if (blob.isDirectory() == false) { if (blob.isDirectory() == false) {
@ -186,7 +166,7 @@ class GoogleCloudStorageBlobStore implements BlobStore {
final String pathStr = path.buildAsString(); final String pathStr = path.buildAsString();
final MapBuilder<String, BlobContainer> mapBuilder = MapBuilder.newMapBuilder(); final MapBuilder<String, BlobContainer> mapBuilder = MapBuilder.newMapBuilder();
SocketAccess.doPrivilegedVoidIOException SocketAccess.doPrivilegedVoidIOException
(() -> client().get(bucketName).list(BlobListOption.currentDirectory(), BlobListOption.prefix(pathStr)).iterateAll().forEach( (() -> client().list(bucketName, BlobListOption.currentDirectory(), BlobListOption.prefix(pathStr)).iterateAll().forEach(
blob -> { blob -> {
if (blob.isDirectory()) { if (blob.isDirectory()) {
assert blob.getName().startsWith(pathStr); assert blob.getName().startsWith(pathStr);
@ -378,7 +358,7 @@ class GoogleCloudStorageBlobStore implements BlobStore {
DeleteResult deleteDirectory(String pathStr) throws IOException { DeleteResult deleteDirectory(String pathStr) throws IOException {
return SocketAccess.doPrivilegedIOException(() -> { return SocketAccess.doPrivilegedIOException(() -> {
DeleteResult deleteResult = DeleteResult.ZERO; DeleteResult deleteResult = DeleteResult.ZERO;
Page<Blob> page = client().get(bucketName).list(BlobListOption.prefix(pathStr)); Page<Blob> page = client().list(bucketName, BlobListOption.prefix(pathStr));
do { do {
final Collection<String> blobsToDelete = new ArrayList<>(); final Collection<String> blobsToDelete = new ArrayList<>();
final AtomicLong blobsDeleted = new AtomicLong(0L); final AtomicLong blobsDeleted = new AtomicLong(0L);

View File

@ -22,7 +22,6 @@ import com.google.api.gax.retrying.RetrySettings;
import com.google.cloud.http.HttpTransportOptions; import com.google.cloud.http.HttpTransportOptions;
import com.google.cloud.storage.StorageException; import com.google.cloud.storage.StorageException;
import com.google.cloud.storage.StorageOptions; import com.google.cloud.storage.StorageOptions;
import com.sun.net.httpserver.HttpContext;
import com.sun.net.httpserver.HttpHandler; import com.sun.net.httpserver.HttpHandler;
import fixture.gcs.FakeOAuth2HttpHandler; import fixture.gcs.FakeOAuth2HttpHandler;
import org.apache.http.HttpStatus; import org.apache.http.HttpStatus;
@ -58,7 +57,6 @@ import java.net.InetSocketAddress;
import java.net.SocketTimeoutException; import java.net.SocketTimeoutException;
import java.util.Arrays; import java.util.Arrays;
import java.util.HashMap; import java.util.HashMap;
import java.util.List;
import java.util.Locale; import java.util.Locale;
import java.util.Map; import java.util.Map;
import java.util.Objects; import java.util.Objects;
@ -165,21 +163,9 @@ public class GoogleCloudStorageBlobContainerRetriesTests extends AbstractBlobCon
}; };
service.refreshAndClearCache(GoogleCloudStorageClientSettings.load(clientSettings.build())); service.refreshAndClearCache(GoogleCloudStorageClientSettings.load(clientSettings.build()));
final List<HttpContext> httpContexts = Arrays.asList( httpServer.createContext("/token", new FakeOAuth2HttpHandler());
// 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);
}))
);
final GoogleCloudStorageBlobStore blobStore = new GoogleCloudStorageBlobStore("bucket", client, "repo", service, final GoogleCloudStorageBlobStore blobStore = new GoogleCloudStorageBlobStore("bucket", client, "repo", service,
randomIntBetween(1, 8) * 1024); randomIntBetween(1, 8) * 1024);
httpContexts.forEach(httpContext -> httpServer.removeContext(httpContext));
return new GoogleCloudStorageBlobContainer(BlobPath.cleanPath(), blobStore); return new GoogleCloudStorageBlobContainer(BlobPath.cleanPath(), blobStore);
} }

View File

@ -192,7 +192,7 @@ setup:
"Register a repository with a non existing bucket": "Register a repository with a non existing bucket":
- do: - do:
catch: /repository_exception/ catch: /repository_verification_exception/
snapshot.create_repository: snapshot.create_repository:
repository: repository repository: repository
body: body:
@ -205,7 +205,7 @@ setup:
"Register a repository with a non existing client": "Register a repository with a non existing client":
- do: - do:
catch: /repository_exception/ catch: /repository_verification_exception/
snapshot.create_repository: snapshot.create_repository:
repository: repository repository: repository
body: body:

View File

@ -1193,7 +1193,7 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp
} }
return seed; return seed;
} }
} catch (IOException exp) { } catch (Exception exp) {
throw new RepositoryVerificationException(metadata.name(), "path " + basePath() + " is not accessible on master node", exp); throw new RepositoryVerificationException(metadata.name(), "path " + basePath() + " is not accessible on master node", exp);
} }
} }
@ -1204,7 +1204,7 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp
try { try {
final String testPrefix = testBlobPrefix(seed); final String testPrefix = testBlobPrefix(seed);
blobStore().blobContainer(basePath().add(testPrefix)).delete(); blobStore().blobContainer(basePath().add(testPrefix)).delete();
} catch (IOException exp) { } catch (Exception exp) {
throw new RepositoryVerificationException(metadata.name(), "cannot delete test data at " + basePath(), exp); throw new RepositoryVerificationException(metadata.name(), "cannot delete test data at " + basePath(), exp);
} }
} }
@ -2170,7 +2170,7 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp
if (isReadOnly()) { if (isReadOnly()) {
try { try {
latestIndexBlobId(); latestIndexBlobId();
} catch (IOException e) { } catch (Exception e) {
throw new RepositoryVerificationException(metadata.name(), "path " + basePath() + throw new RepositoryVerificationException(metadata.name(), "path " + basePath() +
" is not accessible on node " + localNode, e); " is not accessible on node " + localNode, e);
} }
@ -2181,7 +2181,7 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp
try (InputStream stream = bytes.streamInput()) { try (InputStream stream = bytes.streamInput()) {
testBlobContainer.writeBlob("data-" + localNode.getId() + ".dat", stream, bytes.length(), true); testBlobContainer.writeBlob("data-" + localNode.getId() + ".dat", stream, bytes.length(), true);
} }
} catch (IOException exp) { } catch (Exception exp) {
throw new RepositoryVerificationException(metadata.name(), "store location [" + blobStore() + throw new RepositoryVerificationException(metadata.name(), "store location [" + blobStore() +
"] is not accessible on the node [" + localNode + "]", exp); "] is not accessible on the node [" + localNode + "]", exp);
} }
@ -2196,7 +2196,7 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp
"] cannot be accessed on the node [" + 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 " + "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); "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); throw new RepositoryVerificationException(metadata.name(), "Failed to verify repository", e);
} }
} }

View File

@ -132,10 +132,7 @@ public class GoogleCloudStorageHttpHandler implements HttpHandler {
} else if (Regex.simpleMatch("GET /storage/v1/b/" + bucket + "*", request)) { } else if (Regex.simpleMatch("GET /storage/v1/b/" + bucket + "*", request)) {
// GET Bucket https://cloud.google.com/storage/docs/json_api/v1/buckets/get // 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); throw new AssertionError("Should not call get bucket API");
exchange.getResponseHeaders().add("Content-Type", "application/json; charset=utf-8");
exchange.sendResponseHeaders(RestStatus.OK.getStatus(), response.length);
exchange.getResponseBody().write(response);
} else if (Regex.simpleMatch("GET /download/storage/v1/b/" + bucket + "/o/*", request)) { } else if (Regex.simpleMatch("GET /download/storage/v1/b/" + bucket + "/o/*", request)) {
// Download Object https://cloud.google.com/storage/docs/request-body // Download Object https://cloud.google.com/storage/docs/request-body