Update secure settings for the repository azure repository plugin #29319

albertzaharovits · 2018-03-30T18:22:44Z

Make secure settings for the repository-azure plugin updateable. This is the equivalent of #29134 and #28517 for the ec2-discovery and s3-repository, respectively. The meta issue tracking the progress of all plugins is #29135 .

The approach is similar to the previously mentioned plugins, but it is different in an important way. It is similar because AzureRepositoryPlugin now implements the ReInitializablePlugin interface, and there is a 'client service' dictionary of clients that permits changing the settings for the clients that it builds internally.
The difference stems from the azure cloud client (CloudBlobClient) being a wrapper over java.net.HttpURLConnection . This has two implications. One, the client is not Closeable and, two, it is not thread safe, so it is not cache-able across threads. Over the web, in their code samples, (I couldn't point to a clear statement inside the docs) it is recommended to just create a new instance of the client and not to reuse them. This is in contrast to both AmazonEC2 and AmazonS3 clients that have to shutdown to be collected and are thread safe. Consequently AwsEc2Service and AwsS3Service interfaces, which return a "reference counted wrapper", are different to AzureStorageService#client(String) which returns a new client on each invocation! In the interest of the pattern we could have the same reference counted wrapper that would synchronize an internal CloudBlobClient instance, yet I think this is intricate code-wise and trades performance (due to contention over a single HttpURLConnection). Premature optimization is the root of all evil, so I could be swayed here, but just sticking to the letter of the pattern is a week reason IMO.

…ttings

elasticmachine · 2018-03-30T18:22:46Z

Pinging @elastic/es-distributed

jaymode

This is looking pretty good. I left a few comments.

jaymode · 2018-04-04T16:32:56Z

plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureBlobStore.java

-    {
-        return this.client.doesContainerExist(this.clientName, this.locMode, container);
+    public boolean doesContainerExist() {
+        logger.trace("containerExists({})", container);


is this trace logging necessary?

Agreed. I have removed it. Trace logging statements have been sprinkled without much restrain. Surely some nasty bug fighting went on here 🐛

jaymode · 2018-04-04T16:47:09Z

plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureBlobStore.java

        } catch (URISyntaxException | StorageException e) {
-            logger.warn("can not remove [{}] in container {{}}: {}", keyPath, container, e.getMessage());
+            logger.warn("cannot access [{}] in container {{}}: {}", keyPath, container, e.getMessage());
+            throw new IOException(e);
        }
    }

    @Override
    public void close() {


not something you changed but I think we can remove this override?

Looks like it's needed to implement Closeable#close.

jaymode · 2018-04-04T16:48:15Z

plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureBlobStore.java

+        logger.trace("containerExists({})", container);
+        try {
+            return service.doesContainerExist(clientName, container);
+        } catch (URISyntaxException | StorageException e) {


every other method throws these exceptions, is there a reason why this one cannot also throw?

I have adjusted it to also throw exception.

I guess the original reasoning was that boolean exists should swallow the exception and return false, making code simpler and presumably because if indeed there is a connection problem the exception will be rethrown later, when trying the create/access associated operation. Yet, this is not consistent across the plugin code, and your suggestion is indeed in tone with the surrounding.

jaymode · 2018-04-04T16:51:27Z

...pository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureRepositoryPlugin.java

+        if (clientsSettings.isEmpty()) {
+            throw new IllegalArgumentException("If you want to use an azure repository, you need to define a client configuration.");
+        }
+        assert clientsSettings.containsKey("default") : "always have 'default'";


Can we make this an exception instead of an assert?

I have moved the check as suggested in the below comment, but kept the assert there since it would truly be a code problem instead of an external one.

jaymode · 2018-04-04T16:56:20Z

...sitory-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageServiceImpl.java

-        if (storageSettings.isEmpty()) {
-            // If someone did not register any settings, they basically can't use the plugin
+        // eagerly load client settings so that secure settings are read
+        final Map<String, AzureStorageSettings> clientsSettings = AzureStorageSettings.load(settings);


how about moving the validation (empty and default checks) into AzureStorageSettings#load so it is not duplicated in the plugin reinit as well?

I have moved the validation inside the AzureStorageSettings#load method, where settings are constructed.

I have contemplated this duplication before commiting it. I have tried to keep settings validation closer to the "use-point" instead of "construction point" since, in principle, they might change in other layers (which is not the case here). This was the closest I could get, with only duplication, because of the override location mode mechanism which temporarily updates with invalid settings (empty).

jaymode · 2018-04-04T16:57:47Z

...sitory-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageServiceImpl.java

-
-        if (client == null) {
-            throw new IllegalArgumentException("Can not find an azure client named [" + azureStorageSettings.getAccount() + "]");
+        logger.trace(() -> new ParameterizedMessage("creating new Azure storage client using account [{}], endpoint suffix [{}]",


Is this trace logging necessary?

jaymode · 2018-04-04T17:59:35Z

...epository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageSettings.java

@@ -140,15 +153,33 @@ public Proxy getProxy() {
        return proxy;
    }

+    public String getConnectionString() {


Should we precompute and store this in a member variable? It does not look like it will change as when we reinit we get a new object.

I have renamed it to buildConnectionString.

I slightly prefer to keep AzureStorageSettings with only primal members, no derivated/computed ones. It's only a taste decision, because if the connection string would have been build several times, or there would be other such fields, then I would go for it and taint the struct with cached members 😛

…ttings

s1monw

I looked over it and I think it LGTM thanks for doing it albert

s1monw · 2018-04-05T12:34:58Z

...epository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageSettings.java

@@ -140,15 +153,33 @@ public Proxy getProxy() {
        return proxy;
    }

+    public String buildConnectionString() {


s1monw · 2018-04-05T12:41:38Z

...sitory-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageServiceImpl.java

+        final long timeout = azureStorageSettings.getTimeout().getMillis();
+        if (timeout > 0) {
+            if (timeout > Integer.MAX_VALUE) {
+                throw new IllegalArgumentException("Timeout [" + azureStorageSettings.getTimeout() + "] exceeds 2,147,483,647ms.");


albertzaharovits · 2018-04-05T13:08:21Z

I appreciate the feedbacks ! Thank you 👍

@jaymode I have addressed all your comments. Could take another glance at the look of things?

jaymode

LGTM

albertzaharovits added 9 commits March 28, 2018 18:23

WIP

ef2a7de

More WIP

f4283be

Only tests to go

deb79ce

Tests untested :)

478100b

Gradlew test is kicking ****

014f2ef

Logger trace

9cbd24f

All tests are green

247c5f7

Settings reinit tests

a9f8cdd

Merge branch 'reload-secure-store-action' into update-azure-secure-se…

16b5a63

…ttings

albertzaharovits added >enhancement review :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 v6.3.0 labels Mar 30, 2018

albertzaharovits self-assigned this Mar 30, 2018

albertzaharovits requested a review from s1monw March 30, 2018 18:22

albertzaharovits mentioned this pull request Mar 30, 2018

Reloadable secure settings for plugins #29135

Closed

6 tasks

albertzaharovits assigned jaymode Apr 4, 2018

albertzaharovits requested a review from jaymode April 4, 2018 12:51

albertzaharovits unassigned jaymode Apr 4, 2018

jaymode requested changes Apr 4, 2018

View reviewed changes

albertzaharovits added 2 commits April 5, 2018 15:23

Merge branch 'reload-secure-store-action' into update-azure-secure-se…

97fc266

…ttings

renames

4d2694f

s1monw approved these changes Apr 5, 2018

View reviewed changes

jaymode approved these changes Apr 5, 2018

View reviewed changes

albertzaharovits merged commit 057e388 into elastic:reload-secure-store-action Apr 6, 2018

albertzaharovits deleted the update-azure-secure-settings branch April 6, 2018 07:00

albertzaharovits mentioned this pull request May 17, 2018

GCS repo plugin update-able secure settings #30688

Merged

albertzaharovits added >feature and removed v6.3.0 v7.0.0 review >enhancement labels Jul 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update secure settings for the repository azure repository plugin #29319

Update secure settings for the repository azure repository plugin #29319

albertzaharovits commented Mar 30, 2018

elasticmachine commented Mar 30, 2018

jaymode left a comment

jaymode Apr 4, 2018

albertzaharovits Apr 5, 2018

jaymode Apr 4, 2018

albertzaharovits Apr 5, 2018 •

edited

Loading

jaymode Apr 4, 2018

albertzaharovits Apr 5, 2018

jaymode Apr 4, 2018

albertzaharovits Apr 5, 2018

jaymode Apr 4, 2018

albertzaharovits Apr 5, 2018

jaymode Apr 4, 2018

albertzaharovits Apr 5, 2018

jaymode Apr 4, 2018

albertzaharovits Apr 5, 2018

s1monw left a comment

s1monw Apr 5, 2018

s1monw Apr 5, 2018

albertzaharovits commented Apr 5, 2018

jaymode left a comment

Update secure settings for the repository azure repository plugin #29319

Update secure settings for the repository azure repository plugin #29319

Conversation

albertzaharovits commented Mar 30, 2018

elasticmachine commented Mar 30, 2018

jaymode 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

albertzaharovits Apr 5, 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

s1monw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Apr 5, 2018

jaymode left a comment

Choose a reason for hiding this comment

albertzaharovits Apr 5, 2018 •

edited

Loading