Update s3 secure settings#28517
Update s3 secure settings#28517albertzaharovits merged 15 commits intoelastic:reload-secure-store-actionfrom
Conversation
s1monw
left a comment
There was a problem hiding this comment.
I like it. I left some comments
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
++ I like that we completely destroy it once we are refreshing the settings.
There was a problem hiding this comment.
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
ee9f4e4 to
ef49ac2
Compare
e42e019 to
469828c
Compare
|
@s1monw I have addressed your feedback. |
|
@s1monw this is ready for another review round. Can you please take a look? |
s1monw
left a comment
There was a problem hiding this comment.
left some comments. The approach is good IMO
| 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()) { |
There was a problem hiding this comment.
can u use Releasables#close here instead of a loop.
| // clear previously cached clients | ||
| clientsCache = Collections.unmodifiableMap(emptyMap()); | ||
| // reload secure settings | ||
| clientsSettings = Collections.unmodifiableMap(S3ClientSettings.load(settings)); |
There was a problem hiding this comment.
leave a comment here that we built the clients lazily
| clientWrapper.decRef(); | ||
| } | ||
| // clear previously cached clients | ||
| clientsCache = Collections.unmodifiableMap(emptyMap()); |
There was a problem hiding this comment.
Collections.emptyMap() is already immutable
| } | ||
| } | ||
|
|
||
| private static class InternalAmazonS3Wrapper extends AbstractRefCounted implements AmazonS3Wrapper { |
There was a problem hiding this comment.
I don't think we need the AmazonS3Wrapper interface just make this class public and name it AmazonS3Wrapper
There was a problem hiding this comment.
I'd call it AmazonS3Reference
86db918 to
5ae961c
Compare
|
@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. 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). |
Secure settings for the S3 repository plugin are update-able.
AWSS3 clients have to be requested from the
InternalAwsS3Serviceservice 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:
fail if removing settings in use by the repository ?keep existing settings override from cluster settings ?