GCS repo plugin update-able secure settings #30688

albertzaharovits · 2018-05-17T15:03:26Z

GoogleCloudStoragePlugin implements ReInitializablePlugin. Such plugins implement the reinit handler which receives a Settings object with the inner SecureSettings opened. The plugin has to recreate the clients and the code should not internally cache client references. Instead, client references are served from a cache, where they are built on demand using the latest settings.

This is the sibling of: #29319 #29134 and #28517

Overall progress is tracked by: #29135

sweet

elasticmachine · 2018-05-17T15:03:32Z

Pinging @elastic/es-distributed

tlrx

Thanks @albertzaharovits, it looks easier than I though. I left some comments and suggestions.

tlrx · 2018-05-18T14:57:00Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

        }
    }

+    private <R, E extends IOException> R storageAccess(CheckedFunction<Storage, R, E> storageFunction) throws IOException {


I understand why you did that but I find that the two storageAccess() just adds extra unnecessary noise. I think we could instead have a simple private safeClient() method that returns the Storage client to use, and later do things like:

SocketAccess.doPrivilegedIOException(() -> safeClient().get(bucketName));

I find this easier to read and to understand where the stack is cut for access control.

@tlrx I kinda think the extra noise is worth the trouble.

For one thing, trying to locally save the reference (that you get as an argument to the lambda) is an obvious code smell. We convey that you only borrow the reference as an argument to the lambda, you don't get to store it locally. In the S3 repo plugin the simple approach was vetoed, but there the client was closeable, so the IDE hinted a resource leak.
Secondly, using the noisy approach we are cutting the access control stack after the client has been built. That is because, if the client is not cached it has to be created, which generally speaking, does not require elevated privileges. In the simple approach the elevated privilege encompass the client creation too. This is however only a theoretical benefit, since currently we sadly elevate privileges when creating the client (to allow for the google default application credentials).

These were my thoughts, but sometimes I tend to complicate things.
Do you wish to sudo?

I don't want to sudo, I just want to express my concerns about this. GoogleCloudStorageService provides Storage clients to the plugin but they have to be used in a specific way and not be stored locally, and we try to circumvey this fact by are adding storageAccess() methods like this in the BlobStore implementation. I'm tempted to think that we should not expose Storage clients at all and just have GoogleCloudStorageService (or something else) that borrows a temporary client to execute the needed operation and just returns the result. So that we just keep Storage internal. WDYT?

Ok. So we could pack the GoogleCloudStorageService together with the clientName in another entity, and this would be responsible for handling the operations without exposing the Storage. Yet, I dislike this because I feel like this encapsulation of the client is already done at the BlobStore level and away from the BlobContainer; BlobContainer implements blob operations, abstracting away the client. This approach, if I got it right, will nest the client even further under and possibly stripping BlobStore or BlobContainer of their duties. In this matter, I think AzureBlobContainer is illustrative; AzureBlobStore and AzureBlobContainer could be collapsed into a single class. This is in part because the azure client is relatively high level and the encapsulation we are talking about is handled internally.

Writing this down, I am gravitating towards your suggestion. There still has to be someone that knows how to use the client (not to cache it locally) and adding another layer (entity or method receiving lambda) is without justification, BlobStore should handle it.

Unless you have other thoughts I will go ahead and have a simple private method inside the BlobStore that returns the Storage instance. Code inside the Blobstore is responsible for not caching the instance.

There is history around the azure plugin so I won't take this as an example.

Unless you have other thoughts I will go ahead and have a simple private method inside the BlobStore that returns the Storage instance. Code inside the Blobstore is responsible for not caching the instance.

👍

Let's do that and not block this PR. This is something we can still revisit later on.

tlrx · 2018-05-18T14:57:30Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

@@ -95,7 +120,7 @@ public void close() {
     */
    boolean doesBucketExist(String bucketName) {
        try {
-            final Bucket bucket = SocketAccess.doPrivilegedIOException(() -> storage.get(bucketName));
+            final Bucket bucket = storageAccess(storage -> storage.get(bucketName));


Following my previous comment, this could be:

SocketAccess.doPrivilegedIOException(() -> safeClient().get(bucketName));

tlrx · 2018-05-18T14:59:12Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

@@ -277,13 +300,14 @@ void deleteBlobs(Collection<String> blobNames) throws IOException {
            deleteBlob(blobNames.iterator().next());
            return;
        }
-        final List<BlobId> blobIdsToDelete = blobNames.stream().map(blobName -> BlobId.of(bucket, blobName)).collect(Collectors.toList());
-        final List<Boolean> deletedStatuses = SocketAccess.doPrivilegedIOException(() -> storage.delete(blobIdsToDelete));
+        final List<BlobId> blobIdsToDelete = blobNames.stream().map(blobName -> BlobId.of(bucketName, blobName)).collect(


Nit: can you make this fit on the same line as before?

will do. For the record this was not my IDE going wild, this was prompted by the 'bucket' -> 'bucketName' rename.

tlrx · 2018-05-18T14:59:36Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

@@ -299,20 +323,18 @@ void deleteBlobs(Collection<String> blobNames) throws IOException {
     * @param targetBlob new name of the blob in the same bucket


Nit: Can you fix the Javadoc?

tlrx · 2018-05-18T15:01:06Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

-            }
-        });
+        // There's no atomic "move" in GCS so we need to copy and delete
+        storageAccessConsumer(storage -> storage.copy(request).getResult());


This is where a safeClient() would be helpful, so that you have less chance that the underlying storage instance changed between the copy and delete calls

Since google forces us to use two calls for the move op they should not be surprised if we do it over different clients...
Anyhow, using the present approach we could extend the lambda to:

storageAccess(storage -> { storage.copy(request).getResult(); return storage.delete(sourceBlobId); });

and keep it over a single client

tlrx · 2018-05-18T15:06:12Z

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

+        this.storageService = createStorageService(settings);
+        // eagerly load client settings so that secure settings are readable (not closed)
+        final Map<String, GoogleCloudStorageClientSettings> clientsSettings = GoogleCloudStorageClientSettings.load(settings);
+        this.storageService.updateClientsSettings(clientsSettings);


Maybe we can fold this into the GoogleCloudStorageService class so that we only have

this.storageService = createStorageService(settings);

here?

Is this an acceptable simplification?

- final Map<String, GoogleCloudStorageClientSettings> clientsSettings = GoogleCloudStorageClientSettings.load(settings); - this.storageService.updateClientsSettings(clientsSettings); + reinit(settings); }

I really wish to avoid passing around Settings objects which should contain open SecureSettings members. GoogleCloudStorageClientSettings is the conveyor of secure settings and GoogleCloudStorageClientSettings#load is only called in the Plugin implementation.

tlrx · 2018-05-18T15:07:46Z

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

+        // secure settings should be readable
+        final Map<String, GoogleCloudStorageClientSettings> clientsSettings = GoogleCloudStorageClientSettings.load(settings);
+        this.storageService.updateClientsSettings(clientsSettings);
+        return true;


I don't see any documentation for the reinit() method and what it is supposed to return :(

Do you think we could simplify this too and have something like:

return storageService.refreshAndClearCache(settings); ?

I don't see any documentation for the reinit() method and what it is supposed to return :(

Well... reinit was vague from the start. I will document/rename/remove return type in a follow up PR, before merging to master, for all plugins. I will surely add you as a reviewer.

The same goes with storageService#updateClientSettings the name deserves better attention, but it should receive clientsSettings not the raw settings assuming the SecureSettings are open.

So, the renamings I will address in a follow-up PR, for all plugins (EC2, S3, Azure, GCS).

Ok, thanks.

tlrx · 2018-05-18T15:16:18Z

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

+     * @param clientsSettings the new settings used for building clients for subsequent requests
+     * @return previous settings which have been substituted
+     */
+    public synchronized Map<String, GoogleCloudStorageClientSettings>


We could have a refreshAndClearCache(final Settings settings) method or with a similar name that reflects the fact that settings are refresh and cached clients deleted.

We could also use GoogleCloudStorageClientSettings.load() here that already returns an immutable map.

public synchronized refreshAndClearCache(final Settings settings) { final Map<String, GoogleCloudStorageClientSettings> prevSettings = this.clientsSettings; this.clientsSettings = GoogleCloudStorageClientSettings.load(settings); ... }

What do you think? Unless not all the settings are passed when the plugin is reinitialized?

We could have a refreshAndClearCache(final Settings settings) method or with a similar name that reflects the fact that settings are refresh and cached clients deleted.

Agreed.

Again, I prefer to read secure settings in a GoogleCloudStorageClientSettings instance asap and pass that around, not the raw Settings object.

Makes sense, thanks. Do you think it could deserve a comment?

I have added comments inside GoogleCloudStoragePlugin#reinit.

// Secure settings should be readable inside this method. Duplicate client // settings in a format (`GoogleCloudStorageClientSettings`) that does not // require for the `SecureSettings` to be open. Pass that around (the // `GoogleCloudStorageClientSettings` instance) instead of the `Settings` // instance.

tlrx · 2018-05-18T15:17:40Z

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

+                return storage;
+            }
+            storage = SocketAccess.doPrivilegedIOException(() -> createClient(clientName));
+            clientsCache = MapBuilder.newMapBuilder(clientsCache).put(clientName, storage).immutableMap();


Why not using HashMap() directly?

Or maybe use a ConcurrentHashMap to simplify the code? You could use computeIfAbsent and the like?

Why not using HashMap() directly?

I suppose MapBuilder was created for the chaining syntax, plus making sure the result is immutable. It grew on me, don't ruin it for me now... 😛

@jaymode
I think ConcurrentHashMap is a behemoth we don't need.
In the common case the map contains a single entry and there is really no contention here, the "other" thread only clears the map. The volatile immutable map is the evolution to the volatile client reference.
If there is an occasion where a volatile reference to an immutable map trumps the concurrent map I think this must be it.
At some point (when I wished the reinit be blocking until all clients have been changed), I also pushed the concurrent map #28517 (comment)).

Do you wish to sudo?

I also pushed the concurrent map #28517 (comment))

My quick glance makes this seem like it is somewhat different as there was a read write lock external to that CHM?

The volatile immutable map is fine; this was just a suggestion to simplify the code.

My quick glance makes this seem like it is somewhat different as there was a read write lock external to that CHM?

Yes, unfortunately the comment tied the read write lock to the CHM; I remember they were not tied though... my bad for bringing this up.

I have been thinking and I reckon this synchronized block is necessary

synchronized (this) { storage = clientsCache.get(clientName); if (storage != null) { return storage; } storage = SocketAccess.doPrivilegedIOException(() -> createClient(clientName)); clientsCache = MapBuilder.newMapBuilder(clientsCache).put(clientName, storage).immutableMap(); return storage; }

because we need to keep the settings and the client references updates paired. For example, we might end up with one thread reading the settings (clientsCache.get(clientName)), another changing them, and then the first client will use stale settings to build a client. Now the settings don't reflect the client.
Not saying CHM will never fit, but it will not simplify this part here.

…-gcs

albertzaharovits · 2018-05-24T09:27:01Z

@tlrx and @jaymode this is now ready for your attention again.

tlrx

LGTM, left a minor comment

tlrx · 2018-05-24T12:34:09Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

+        try {
+            return storageService.client(clientName);
+        } catch (final Exception e) {
+            throw new IOException(e);


Nit: maybe add a message to the IOException?

jaymode

LGTM too

jaymode · 2018-05-24T16:31:52Z

...sitory-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java

     *
     * @param blobName name of the blob
-     * @return an InputStream
+     * @return the InputStream used to read blob's content


nit: s/read blob's/read the blob's

…-gcs

albertzaharovits added 3 commits May 17, 2018 13:06

WIP GoogleCloudStorageService and GoogleCloudStoragePlugin looking

4c6b0a2

sweet

src/main done :)

29847d3

aaand tests, done!

d31cc92

albertzaharovits added >feature :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 v6.4.0 labels May 17, 2018

albertzaharovits self-assigned this May 17, 2018

albertzaharovits requested review from tlrx and jaymode May 17, 2018 15:03

albertzaharovits mentioned this pull request May 17, 2018

Reloadable secure settings for plugins #29135

Closed

6 tasks

tlrx reviewed May 18, 2018

View reviewed changes

albertzaharovits added 10 commits May 21, 2018 16:14

Merge branch 'master' into update-secure-settings-gcs

2a31ea6

Fix javadocs

5eabce7

Use reinit to avoid code duplication

7212b4d

Move uses a single client reference

badea12

BlobIdsToDelete on a single line

86ca953

Merge branch 'reload-secure-store-action' into update-secure-settings…

8b839e1

…-gcs

Merge branch 'reload-secure-store-action' into update-secure-settings…

2505242

…-gcs

private method returns Storage instance

6fbf79d

Javadoc touches

285870d

Comment about reinit and client settings

229d598

tlrx approved these changes May 24, 2018

View reviewed changes

jaymode approved these changes May 24, 2018

View reviewed changes

albertzaharovits added 2 commits May 28, 2018 12:04

Merge branch 'reload-secure-store-action' into update-secure-settings…

147ef68

…-gcs

Nits

aec341c

albertzaharovits merged commit 000c585 into elastic:reload-secure-store-action May 28, 2018

albertzaharovits deleted the update-secure-settings-gcs branch May 28, 2018 09:20

albertzaharovits removed v6.4.0 v7.0.0 labels Jul 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCS repo plugin update-able secure settings #30688

GCS repo plugin update-able secure settings #30688

albertzaharovits commented May 17, 2018

elasticmachine commented May 17, 2018

tlrx left a comment

tlrx May 18, 2018

albertzaharovits May 21, 2018

tlrx May 23, 2018 •

edited

Loading

albertzaharovits May 23, 2018

tlrx May 23, 2018

tlrx May 18, 2018

tlrx May 18, 2018

albertzaharovits May 21, 2018

tlrx May 18, 2018

tlrx May 18, 2018

albertzaharovits May 21, 2018

tlrx May 18, 2018

albertzaharovits May 21, 2018

tlrx May 23, 2018

tlrx May 18, 2018

albertzaharovits May 21, 2018

tlrx May 23, 2018

tlrx May 18, 2018

albertzaharovits May 21, 2018 •

edited

Loading

tlrx May 23, 2018

albertzaharovits May 24, 2018

tlrx May 18, 2018

jaymode May 18, 2018

albertzaharovits May 21, 2018

jaymode May 21, 2018

albertzaharovits May 21, 2018

albertzaharovits commented May 24, 2018

tlrx left a comment

tlrx May 24, 2018

jaymode left a comment

jaymode May 24, 2018

		@@ -299,20 +323,18 @@ void deleteBlobs(Collection<String> blobNames) throws IOException {
		* @param targetBlob new name of the blob in the same bucket

GCS repo plugin update-able secure settings #30688

GCS repo plugin update-able secure settings #30688

Conversation

albertzaharovits commented May 17, 2018

elasticmachine commented May 17, 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

tlrx May 23, 2018 • edited Loading

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 May 21, 2018 • edited Loading

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 24, 2018

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx May 23, 2018 •

edited

Loading

albertzaharovits May 21, 2018 •

edited

Loading