Update s3 secure settings #28517

albertzaharovits · 2018-02-05T09:29:22Z

Secure settings for the S3 repository plugin are update-able.
AWSS3 clients have to be requested from the InternalAwsS3Service service for each use. These client instances cannot be stored and used at a later time because they can change if their settings change. The client request and client update logic is protected by a read-write lock.

TODOs:

fix existing tests
add tests for reload credentials
~~fail if removing settings in use by the repository ?~~
~~keep existing settings override from cluster settings ?~~

s1monw

I like it. I left some comments

s1monw · 2018-02-13T10:07:00Z

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/InternalAwsS3Service.java

-
-    private final Map<String, S3ClientSettings> clientsSettings;
+    private final ReadWriteLock lock;
+    private final ConcurrentHashMap<String, AmazonS3> clientsCache;


lets make this an immutable map instead. That way we don't need a read-write lock and can just make the method that changes the reference synchronized. Also for the AmazonS3Wrapper we can just use a reference counter. We already have the infrastructure for it in org.elasticsearch.common.util.concurrent.AbstractRefCounted lemme know if you need help with this.

s1monw · 2018-02-13T10:07:47Z

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/InternalAwsS3Service.java

+    public void updateClientSettings(Settings settings) {
+        lock.writeLock().lock();
+        try {
+            // clear previously cached clients


++ I like that we completely destroy it once we are refreshing the settings.

s1monw · 2018-02-13T10:09:36Z

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java

        } catch (AmazonClientException e) {
            throw new IOException("Exception when deleting blob [" + blobName + "]", e);
        }
    }

    @Override
    public Map<String, BlobMetaData> listBlobsByPrefix(@Nullable String blobNamePrefix) throws IOException {
-        return AccessController.doPrivileged((PrivilegedAction<Map<String, BlobMetaData>>) () -> {


can we please not change the way we use AccessController here. We can do this in a sep. change but this is a cause of trouble and hard to understand why it's needed if at all

albertzaharovits · 2018-02-16T11:56:17Z

@s1monw I have addressed your feedback.
The client cache is a volatile reference to an immutable map.
Also, AmazonS3Wrapperinterface extends RefCounted and Releasable. The implementation extends AbstractRefCount. The Releasable#close methods is a proxy for RefCounted#decRef so it's not really required ,but it's nice because it allows the usage inside try/with blocks.

albertzaharovits · 2018-02-16T15:03:58Z

@s1monw this is ready for another review round. Can you please take a look?
If you agree with the approach I will update the tests as well.

s1monw

left some comments. The approach is good IMO

s1monw · 2018-02-19T11:36:31Z

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/InternalAwsS3Service.java

-            return client;
+    public synchronized void updateClientSettings(Settings settings) {
+        // the clients will shutdown when they will not be used anymore
+        for (final AmazonS3Wrapper clientWrapper : clientsCache.values()) {


can u use Releasables#close here instead of a loop.

s1monw · 2018-02-19T11:41:01Z

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/InternalAwsS3Service.java

+        // clear previously cached clients
+        clientsCache = Collections.unmodifiableMap(emptyMap());
+        // reload secure settings
+        clientsSettings = Collections.unmodifiableMap(S3ClientSettings.load(settings));


leave a comment here that we built the clients lazily

s1monw · 2018-02-19T11:43:20Z

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/InternalAwsS3Service.java

        }
+        // clear previously cached clients
+        clientsCache = Collections.unmodifiableMap(emptyMap());


Collections.emptyMap() is already immutable

s1monw · 2018-02-19T11:44:40Z

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/InternalAwsS3Service.java

@@ -174,4 +166,30 @@ public void refresh() {
            SocketAccess.doPrivilegedVoid(credentials::refresh);
        }
    }
+
+    private static class InternalAmazonS3Wrapper extends AbstractRefCounted implements AmazonS3Wrapper {


I don't think we need the AmazonS3Wrapper interface just make this class public and name it AmazonS3Wrapper

I'd call it AmazonS3Reference

albertzaharovits · 2018-03-12T09:52:35Z

@s1monw I think this deserves another round of review. May you look into it, please?

There are quite a few changes, few of them overflowing more than you'd like on a bad day.
Most changes were required to separate client settings parsing (from node secure settings and cluster state) from building and caching the clients using those settings. I think the client cache allowing for update-able settings (InternalAwsS3Service) can be cherry-pick-ed in other places now (it will require some generics enrichment at least).
Other minor changes are for testing purposes (RepositoryCredentialsTests) since some of the new functionality is buried deeper and required some mocking to be constructed.

Lastly, I think tests are patchy and it is not clear what code paths they cover but I also think changes there fit in a separate PR(s).

s1monw

LGTM

albertzaharovits self-assigned this Feb 5, 2018

albertzaharovits requested a review from s1monw February 5, 2018 09:29

albertzaharovits added :Internal >feature v6.3.0 v7.0.0 labels Feb 5, 2018

dadoonet added the :Plugin Repository S3 label Feb 5, 2018

s1monw requested changes Feb 13, 2018

View reviewed changes

clintongormley added :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed :Plugin Repository S3 labels Feb 14, 2018

albertzaharovits closed this Feb 16, 2018

albertzaharovits force-pushed the update-s3-secure-settings branch from ee9f4e4 to ef49ac2 Compare February 16, 2018 11:08

albertzaharovits added 3 commits February 16, 2018 13:11

Read write lock

26acd44

Hook update handle to awsService

8389c7e

volatile immutable map and RefCount

469828c

albertzaharovits reopened this Feb 16, 2018

albertzaharovits force-pushed the update-s3-secure-settings branch from e42e019 to 469828c Compare February 16, 2018 11:35

s1monw reviewed Feb 19, 2018

View reviewed changes

AmazonS3Reference, Javadoc & other feedback

5ae961c

albertzaharovits force-pushed the update-s3-secure-settings branch from 86db918 to 5ae961c Compare February 20, 2018 12:17

albertzaharovits added 7 commits February 21, 2018 14:31

Fix simple tests

92fe976

Still working on tests

caf9a97

Tests compile, will they blow up?

1602eb3

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

97f4360

Fix checkstyle

81cfb9c

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

22168ee

Fix tests but they turned up a twisted contraption

5af6542

albertzaharovits added 4 commits March 10, 2018 13:34

Tests for reload and many other test fixes

076ab66

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

beca7c7

gradle check

b623bb8

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

a52e100

s1monw approved these changes Mar 12, 2018

View reviewed changes

albertzaharovits merged commit 3db5ae0 into elastic:reload-secure-store-action Mar 13, 2018

albertzaharovits deleted the update-s3-secure-settings branch March 19, 2018 08:54

This was referenced Mar 19, 2018

Update ec2 secure settings #29134

Merged

Reloadable secure settings for plugins #29135

Closed

albertzaharovits mentioned this pull request Mar 30, 2018

Update secure settings for the repository azure repository plugin #29319

Merged

albertzaharovits mentioned this pull request May 17, 2018

GCS repo plugin update-able secure settings #30688

Merged

scottsom mentioned this pull request Jun 29, 2018

6.3.0 Release Notes for Updatable Secure Settings Incorrect #31695

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update s3 secure settings #28517

Update s3 secure settings #28517

albertzaharovits commented Feb 5, 2018 •

edited

Loading

s1monw left a comment

s1monw Feb 13, 2018

s1monw Feb 13, 2018

s1monw Feb 13, 2018

albertzaharovits commented Feb 16, 2018

albertzaharovits commented Feb 16, 2018 •

edited

Loading

s1monw left a comment

s1monw Feb 19, 2018

s1monw Feb 19, 2018

s1monw Feb 19, 2018

s1monw Feb 19, 2018

s1monw Feb 19, 2018

albertzaharovits commented Mar 12, 2018 •

edited

Loading

s1monw left a comment

Update s3 secure settings #28517

Update s3 secure settings #28517

Conversation

albertzaharovits commented Feb 5, 2018 • edited Loading

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

Choose a reason for hiding this comment

albertzaharovits commented Feb 16, 2018

albertzaharovits commented Feb 16, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Mar 12, 2018 • edited Loading

s1monw left a comment

Choose a reason for hiding this comment

albertzaharovits commented Feb 5, 2018 •

edited

Loading

albertzaharovits commented Feb 16, 2018 •

edited

Loading

albertzaharovits commented Mar 12, 2018 •

edited

Loading