Repository GCS plugin new client library #30168

albertzaharovits · 2018-04-26T09:05:48Z

This does away with the com.google.api-client:google-api-client:1.23 and replaces it with com.google.cloud:google-cloud-storage:1.27.0 . It also changes security permissions for this plugin.

The API of the new library is indeed easier to use, but the configuration requires uncovering internals which most likely will change in the new versions (because the internals are actually the old library that will go away). This should be easy to change when it will happen, although some configuration options might go away (eg applicationName or projectId)

DISCLAIMER: This is not turnkey ready. The REST integ test ./gradlew :plugins:repository-gcs:integTest does not work so I have tested the plugin manually with my Google Cloud account. I have tested it using the Service Account JSON file.

The reason the integration test does not work is that the Service Account JSON file, that we contrive to direct the cluster to the mock cloud, does not allow to override the oauth/token endpoint, which is the first endpoint called in the flow. So, even if the subsequent API calls should be compatible with our existing mock, to obtain the token, there is a call to 'googleapis' which obviously gets 'unauthorized'. I don't think this is intended, just a sloppiness in the transition.

Another worry is that the Application Default Credentials used to authenticate when ES runs on a VM provisioned inside google cloud, reads files from various places (ENV vars, user home, ... other heuristics), so a snap-in security-permission.policy is not an option. We could mimic what we do with our config dir, but in this case we would have to duplicate their credentials search logic.

Closes: #29259

elasticmachine · 2018-04-26T09:05:50Z

Pinging @elastic/es-distributed

albertzaharovits · 2018-05-08T14:36:21Z

Thank you Tanguy for the prompt and diligent review! I have addressed it all.

These rogue permissions have been removed:

permission java.lang.RuntimePermission "accessClassInPackage.sun.misc";
permission java.net.URLPermission "https://www.googleapis.com/-", "*:*";
permission java.net.URLPermission "https://accounts.google.com/-", "*:*";

Apologies, I don't recall what the errors were when they came to be.

I haven't however adopted the MockStorage you propose:

I think we can get rid of the specific permission by using a different implementation of MockStorage > that does not use mockito and does not require the additionnal permission.
I implemented it here: tlrx/elasticsearch@1f40ea5, I think this is how MockStorage should be.

Just to check I understand, generally speaking we should avoid writing classes inside foreign packages and use reflection to gain access to hidden members. However, this special case (com.google.cloud.storage.StorageRpcOptionUtils) was mandated by the goal to get rid of the suppressAccessChecks permission, assuming the permission was required only by the test code. Is this correct? If the permission is required by the src/main code, and therefore cannot be removed, are classes written outside the org.elasticsearch considered a "code smell"?

tlrx

Thanks @albertzaharovits. I did more research today and I think we can get rid of the magic deletes.size < 3 trick using an appropriate solution that does not require hacks in plugin's code to address integration tests issues. I also did many comments I'd like to see addressed.

There are still two things that worry me and I'd like to do more research about them:

writeBlobMultipart() loads the whole blob in heap before uploading it. This will produce many allocations and we might want to use the deprecated method based on stream or maybe use BytesStreamOutput... wdyt?
the resumable/multipart code in the fixture looks a bit messy, I'd like to see if we can use UTF8 or plain bytes arrays and mutualize code with the batch request handler

Concerning the MockStorage point I've added my opinion and I don't remember any similar situation before. I don't have a strong feeling so I asked for core/infra team opinion :)

Again, I think this is going in the right direction and thanks a lot for all you work!

tlrx · 2018-05-09T14:13:46Z

plugins/repository-gcs/qa/google-cloud-storage/build.gradle

-                "  \"token_uri\": \"http://${googleCloudStorageFixture.addressAndPort}/o/oauth2/token\",\n" +
-                '  "auth_provider_x509_cert_url": "https://www.googleapis.com/oauth2/v1/certs",\n' +
-                '  "client_x509_cert_url": "https://www.googleapis.com/robot/v1/metadata/x509/integration_test%40appspot.gserviceaccount.com"\n' +
+                '  "client_id": "123456789101112130594"\n' +


Now the fixture URL is not required anymore, we can remove the dependsOn googleCloudStorageFixture

tlrx · 2018-05-09T14:14:39Z

...d-storage/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageTestServer.java

+                if (bucket == null) {
+                    return newError(RestStatus.NOT_FOUND, "bucket not found");
+                }
+                if (bucket.objects.put(objectName, EMPTY_BYTE) == null) {


Can you make EMPTY_BYTE final?

Also, there is some unused code in this class (ctor, handle() method), can you clean this while you are here?

done and done

tlrx · 2018-05-09T14:15:22Z

...d-storage/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageTestServer.java

+                 * KEwE3bU4TuyetBgQIghmUw
+                 * --__END_OF_PART__--
+                 */
+                String boundary = "__END_OF_PART__";


Can't we reuse/mutualize the code with the batch request handler here?

I have pondered it too, but I think they should be kept separate. The "theoretical" reason is that for the multipart/related content type the entities are tied together. The first entity contains metadata about the data from the second entity. There is an implied order, and the number of entities is fixed. For the multipart/mixed all entities are homogeneous.
The "practical" reason is that parsing these is intricate and grouping them together will make them more so.

The only common denominator I see is parsing the headers of each part.

Ok, let's keep as it is.

tlrx · 2018-05-09T14:15:47Z

...d-storage/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageTestServer.java

+                    }
+                }
+                // Read line by line ?both? parts of the multipart
+                try (BufferedReader reader = new BufferedReader(new InputStreamReader(inputStreamBody, StandardCharsets.ISO_8859_1))) {


Ok. I'll look at this because I'm a bit surprised.

tlrx · 2018-05-09T14:17:23Z

...d-storage/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageTestServer.java

+        // Rewrite Object
+        //
+        // https://cloud.google.com/storage/docs/json_api/v1/objects/rewrite
+        handlers.insert("POST " + endpoint + "/storage/v1/b/{srcBucket}/o/{src}/rewriteTo/b/{destBucket}/o/{dest}",


This code is identical to the copyTo handler, maybe we could use a path pattern like:
/storage/v1/b/{srcBucket}/o/{src}/{action}/b/{destBucket}/o/{dest}

And use the value of {action} to differentiate the response?

I have done so although, it looks like copyTo has fallen into disgrace in favor of rewriteTo but I cannot guarantee it's not used anymore, or that it will not be used again in a minor version update of the lib.

tlrx · 2018-05-09T14:41:41Z

...epository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStoragePlugin.java

-            return null;
-        });
-    }
-


Thanks for this cleanup!

tlrx · 2018-05-09T14:43:56Z

...pository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java

     * @return a Client instance that can be used to manage Storage objects
     */
-    public Storage createClient(final String clientName) throws Exception {
+    public Storage createClient(final String clientName) throws GeneralSecurityException, IOException {


We are OK to only declare Exception instead of declaring all the possible thrown exceptions if they do not require any special handling by the callers.

tlrx · 2018-05-09T14:47:28Z

...pository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageService.java

-        Storage.Builder storage = new Storage.Builder(transport, JacksonFactory.getDefaultInstance(), requestInitializer);
-        if (Strings.hasLength(clientSettings.getApplicationName())) {
-            storage.setApplicationName(clientSettings.getApplicationName());
+        final NetHttpTransport netHttpTransport = GoogleNetHttpTransport.newTrustedTransport();


I think we could workaround the Google SDK issue concerning non propagated custom endpoint by doing something like:

final HttpTransport httpTransport = createHttpTransport(clientSettings.getHost());

and add a static method like the following which is in charge of checking and rewriting URLS that do not point to the right endpoint:

static HttpTransport createHttpTransport(final String endpoint) throws Exception { final NetHttpTransport.Builder builder = new NetHttpTransport.Builder(); builder.trustCertificates(GoogleUtils.getCertificateTrustStore()); if (Strings.hasLength(endpoint)) { final URL endpointUrl = URI.create(endpoint).toURL(); builder.setConnectionFactory(new DefaultConnectionFactory() { @Override public HttpURLConnection openConnection(final URL originalUrl) throws IOException { URL url = originalUrl; if ((originalUrl.getHost().equals(endpointUrl.getHost()) == false) || (originalUrl.getPort() != endpointUrl.getPort()) || (originalUrl.getProtocol().equals(endpointUrl.getProtocol())) == false) { url = new URL(endpointUrl.getProtocol(), endpointUrl.getHost(), endpointUrl.getPort(), originalUrl.getFile()); } return super.openConnection(url); } }); } return builder.build(); }

This way, production code is not forced to have a magic trick like deletes.size() < 3. Also, production code should almost always use the default connection factory provided by the Google SDK.

I have coded smth similar. I have not copied it verbatim because URL#getFile is a forbiddenApi and a URI conversion was required.

private static HttpTransport createHttpTransport(final String endpoint) throws Exception { final NetHttpTransport.Builder builder = new NetHttpTransport.Builder(); // requires java.lang.RuntimePermission "setFactory" builder.trustCertificates(GoogleUtils.getCertificateTrustStore()); if (Strings.hasLength(endpoint)) { final URL endpointUrl = URI.create(endpoint).toURL(); builder.setConnectionFactory(new DefaultConnectionFactory() { @Override public HttpURLConnection openConnection(final URL originalUrl) throws IOException { // test if the URL is built correctly, ie following the `host` setting if (originalUrl.getHost().equals(endpointUrl.getHost()) && originalUrl.getPort() == endpointUrl.getPort() && originalUrl.getProtocol().equals(endpointUrl.getProtocol())) { return super.openConnection(originalUrl); } // override connection URLs because some don't follow the config. See // https://github.com/GoogleCloudPlatform/google-cloud-java/issues/3254 and // https://github.com/GoogleCloudPlatform/google-cloud-java/issues/3255 URI originalUri; try { originalUri = originalUrl.toURI(); } catch (final URISyntaxException e) { throw new RuntimeException(e); } String overridePath = "/"; if (originalUri.getRawPath() != null) { overridePath = originalUri.getRawPath(); } if (originalUri.getRawQuery() != null) { overridePath += "?" + originalUri.getRawQuery(); } return super.openConnection( new URL(endpointUrl.getProtocol(), endpointUrl.getHost(), endpointUrl.getPort(), overridePath)); } }); } return builder.build(); }

tlrx · 2018-05-09T15:03:17Z

...d-storage/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageTestServer.java

            }
        });

        // Insert Object (upload)
        //
        // https://cloud.google.com/storage/docs/json_api/v1/how-tos/resumable-upload
        handlers.insert("PUT " + endpoint + "/upload/storage/v1/b/{bucket}/o", (params, headers, body) -> {
-            String objectId = params.get("upload_id");
+            final String objectId = params.get("upload_id");


Do we still need this handler?

Yes it is still required when resumable upload is used.

tlrx · 2018-05-09T15:21:44Z

plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/MockStorage.java

+            optionMethod.setAccessible(true);
+            assert StorageRpc.Option.PREFIX.equals(optionMethod.invoke(prefixFilter)) : "Only the prefix filter is mocked";
+            final Method valueMethod = BlobListOption.class.getSuperclass().getDeclaredMethod("getValue");
+            valueMethod.setAccessible(true);


@albertzaharovits and I talked about this, and we figured out that this is not the only place where the permission is needed but the main code requires it too.

I still think that we should not add a specific permission for tests in this case because it can hide other places where a security exception could be thrown where in fact we should work to remove this permission in the main plugin. I also think that in this case Mockito makes things harder to maintain and to read where we can instead use POJOs to mock the Google Storage but with this option we have to pay the price of adding two utility classed in foreign packages like com.google.storage. It's like to choose between two bad options but this is an interesting point and I'd like to know @elastic/es-core-infra opinion

albertzaharovits · 2018-05-15T08:51:44Z

@tlrx I have profiled the _snapshot API with w/ & w/o the multipart update trick. I have attached the flight recordings (profile data, in unpretentious lingo).
resumable & mutipart:
https://drive.google.com/file/d/1KZc4Ua2r8oAttrSMvso7oTD8EirPV1_1/view?usp=sharing
https://drive.google.com/file/d/1wDuTTnqtDYdpZ1TJr3j1sWTaT9BOMnDQ/view?usp=sharing
resumable only:
https://drive.google.com/file/d/1Va_iTVwFrP99Iy96pMGITJrfwxGdxP2a/view?usp=sharing
https://drive.google.com/file/d/1krSQPOiJnpLc4b3bTbXXYHQ3Q3GxtA8n/view?usp=sharing
The VM is looping PUT _snapshot and DELETE _snapshot for the geonames index (from the rally track) with true GCS credentials. I cannot eye any difference between thee two methods. It might be that the bottleneck is outside the VM.
I have collected the timings too, but they seem un-illustrative, they have variance and it does not appear to be any statistical difference between the snapshot times. I can share them if the profile data is not enough.

Curious if you spot something on the profiling data!

tlrx

LGTM, thanks @albertzaharovits for going through this change!

I left some minor comments I'd like to be addressed before merging but they do not require another review.

tlrx · 2018-05-15T09:25:46Z

docs/plugins/repository-gcs.asciidoc

+`project_id`::
+
+    The Google Cloud project id. This will be automatically infered from the credentials file but
+    can be specified explicitly. For example, it can be used to switch between projects  when the


Nit: extra space

tlrx · 2018-05-15T09:26:14Z

plugins/repository-gcs/build.gradle

@@ -17,43 +17,214 @@
 * under the License.
 */

+import org.elasticsearch.gradle.test.AntFixture


Nit: can you remove this import? It seems to be unused

tlrx · 2018-05-15T09:29:33Z

...d-storage/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageTestServer.java

+                if (bucket == null) {
+                    return newError(RestStatus.NOT_FOUND, "bucket not found");
+                }
+                if (bucket.objects.put(objectName, EMPTY_BYTE) == null) {


Can you change this to putIfAbsent()?

tlrx · 2018-05-15T09:30:00Z

...d-storage/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageTestServer.java

+                 * KEwE3bU4TuyetBgQIghmUw
+                 * --__END_OF_PART__--
+                 */
+                String boundary = "__END_OF_PART__";


Ok, let's keep as it is.

tlrx · 2018-05-15T09:30:53Z

...d-storage/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageTestServer.java

            }
        });

        // Insert Object (upload)
        //
        // https://cloud.google.com/storage/docs/json_api/v1/how-tos/resumable-upload
        handlers.insert("PUT " + endpoint + "/upload/storage/v1/b/{bucket}/o", (params, headers, body) -> {
-            String objectId = params.get("upload_id");
+            final String objectId = params.get("upload_id");


tlrx · 2018-05-15T09:41:26Z

...y-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageClientSettings.java


-    /** An override for the Storage endpoint to connect to. */
+    /**


Nit: use the same doc formatting as the other settings

tlrx · 2018-05-15T09:41:58Z

...y-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageClientSettings.java

@@ -66,14 +80,24 @@

    /** Name used by the client when it uses the Google Cloud JSON API. **/
    static final Setting.AffixSetting<String> APPLICATION_NAME_SETTING = Setting.affixKeySetting(PREFIX, "application_name",
-        key -> new Setting<>(key, "repository-gcs", s -> s, Setting.Property.NodeScope));
+            key -> new Setting<>(key, "repository-gcs", Function.identity(), Setting.Property.NodeScope,


nit: can you revert this identation?

tlrx · 2018-05-15T09:42:35Z

...y-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageClientSettings.java

+    /**
+     * The Storage endpoint URL the client should talk to, or null string to use the
+     * default
+     **/


Nit: same doc formating?

tlrx · 2018-05-15T09:43:22Z

...y-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageClientSettings.java

        return endpoint;
    }

+    public String getProjectId() {
+        return Strings.hasLength(projectId) ? projectId : (credential != null ? credential.getProjectId() : null);


Can you add a test for this?

tlrx · 2018-05-15T09:49:52Z

.../src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageClientSettingsTests.java

@@ -121,6 +136,14 @@ private static GoogleCloudStorageClientSettings randomClient(final String client
            endpoint = ENDPOINT_SETTING.getDefault(Settings.EMPTY);
        }

+        String projectId;


I think the endpoint setting should be randomized between valid URLs like http://www.elastic.co, https://www.elastic.co:443, https://www.googleapis.com etc

…ngs-to-true * elastic/master: [DOCS] Restores 7.0.0 release notes and highlights Remove assert statements from field caps documentation. (elastic#30601) Repository GCS plugin new client library (elastic#30168) HLRestClient: Follow-up for put index template api (elastic#30592) Unmute IndexUpgradeIT tests [DOCS] Remove references to changelog and to highlights Side-step pending deletes check (elastic#30571) [DOCS] Remove references to removed changelog Revert "Mute ML upgrade test (elastic#30458)" [ML] Adjust BWC version following backport of elastic#30125 [Docs] Improve section detailing translog usage (elastic#30573) [Tests] Relax allowed delta in extended_stats aggregation (elastic#30569) [ML] Reverse engineer Grok patterns from categorization results (elastic#30125) Update build file due to doc file rename Remove the changelog (elastic#30593) Fix issue with finishing handshake in ssl driver (elastic#30580) Fail if reading from closed KeyStoreWrapper (elastic#30394) Silence IndexUpgradeIT test failures. (elastic#30430)

This does away with the deprecated `com.google.api-client:google-api-client:1.23` and replaces it with `com.google.cloud:google-cloud-storage:1.28.0`. It also changes security permissions for the repository-gcs plugin. Closes: #29259

tlrx · 2018-05-16T07:54:13Z

I looked at the profiling results from @albertzaharovits and I didn't see any strong differences except that the tests using only the resumable upload method show more socket I/O, which is expected because this method is supposed to execute more requests. I didn't see any differences in total execution time when snapshotting a large index using resumable and resumable+multipart using the "new" client.

I also compared the snapshot execution time with the "old" client and this "new" client and didn't see any performance change.

Of course, we should have real benchmarks for this.

albertzaharovits added 20 commits April 17, 2018 09:29

src/main completed

bce5c01

Mock is completed

dda51f4

WIP

962f7c8

Merge branch 'master' into gcs-new-libs

4b91b89

Patchy deps compile

ec285a6

Dependency hell is hell

a8bf229

Dumb byte to int bug

c5de4d6

More WIP

a825d71

X content error

13f039a

Merge branch 'master' into gcs-new-libs

6c7e36d

Dreadfull mock reader bug

c2be359

Unit and Integ tests finally budge

ac1dd1c

Authentication failed 401

8933681

Storage scopes

87d782a

Closer

b1cbf70

Permissions WIP

ee8755a

Licenses & some deps curated

5a12d76

Merge branch 'master' into gcs-new-libs

effd879

Client initializer unit tests

a5eb4c8

Remove cached field prefetching

63dee21

albertzaharovits added >enhancement review :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 v6.4.0 labels Apr 26, 2018

albertzaharovits self-assigned this Apr 26, 2018

albertzaharovits requested review from rjernst and tlrx April 26, 2018 09:05

Merge branch 'master' into gcs-new-libs

d03ed35

tlrx requested changes May 9, 2018

View reviewed changes

albertzaharovits added 17 commits May 10, 2018 11:46

Revert HOST / ENDPOINT setting

ccd7bcd

Remove unused in mock

5f3364c

Collapse rewriteTo and copyTo actions

d3e9fbe

HttpConnectionFactory

505f007

Renames

25f5023

Mock with foreign packages

8f0c139

Merge branch 'master' into gcs-new-libs

0f3b2d9

Reformatting

824def8

GoogleCloudStorageService override URL path

0a73dbc

Nit: bucket != null

1da918c

Documented project id

b3a1006

Merge branch 'master' into gcs-new-libs

ae8ee33

Merge branch 'master' into gcs-new-libs

bda0e8d

Merge branch 'master' into gcs-new-libs

2c67ab4

Merge branch 'master' into gcs-new-libs

e6f9f3b

Minor trimmings

e99c69e

Merge branch 'master' into gcs-new-libs

54075fe

tlrx approved these changes May 15, 2018

View reviewed changes

albertzaharovits added 2 commits May 15, 2018 16:04

Addressed feedback

85d9bd9

Merge branch 'master' into gcs-new-libs

4716031

albertzaharovits merged commit 801973f into elastic:master May 15, 2018

ywelsch mentioned this pull request May 16, 2018

User proper write-once semantics for GCS repository #30438

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019


		/** An override for the Storage endpoint to connect to. */
		/**

Repository GCS plugin new client library #30168

Repository GCS plugin new client library #30168

Conversation

albertzaharovits commented Apr 26, 2018

elasticmachine commented Apr 26, 2018

albertzaharovits commented May 8, 2018

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented May 15, 2018

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented May 16, 2018