-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Streamline S3 Repository- and Client-Settings #37393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
original-brownbear
merged 18 commits into
elastic:master
from
original-brownbear:repo-setting-fixes
Jan 30, 2019
Merged
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
377dd26
Streamline S3 Repository- and Client-Settings
original-brownbear 63d5b9b
Merge remote-tracking branch 'elastic/master' into repo-setting-fixes
original-brownbear 485f7bd
remove dead field
original-brownbear 3f92d32
Merge remote-tracking branch 'elastic/master' into repo-setting-fixes
original-brownbear 8f2a85b
CR: remove redundant reloading of settings and use in cache key
original-brownbear ea35e5c
Merge remote-tracking branch 'elastic/master' into repo-setting-fixes
original-brownbear 093d7aa
start test
original-brownbear 31c9a26
Merge remote-tracking branch 'elastic/master' into repo-setting-fixes
original-brownbear 494a964
remove dead code
original-brownbear 6d8d52f
Merge remote-tracking branch 'elastic/master' into repo-setting-fixes
original-brownbear dbf927e
test
original-brownbear 38771fb
settings IT
original-brownbear 5691bd0
unittest
original-brownbear 0a58ae9
Merge remote-tracking branch 'elastic/master' into repo-setting-fixes
original-brownbear 5ae7d4e
Merge remote-tracking branch 'elastic/master' into repo-setting-fixes
original-brownbear 46acd77
Add some documentation
original-brownbear 4673763
Merge remote-tracking branch 'elastic/master' into repo-setting-fixes
original-brownbear b157fa5
CR: fix jdocs
original-brownbear File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
62 changes: 62 additions & 0 deletions
62
...ins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BasicCredentials.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| /* | ||
| * Licensed to Elasticsearch under one or more contributor | ||
| * license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright | ||
| * ownership. Elasticsearch licenses this file to you under | ||
| * the Apache License, Version 2.0 (the "License"); you may | ||
| * not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.elasticsearch.repositories.s3; | ||
|
|
||
| import com.amazonaws.auth.AWSCredentials; | ||
|
|
||
| import java.util.Objects; | ||
|
|
||
| class S3BasicCredentials implements AWSCredentials { | ||
|
|
||
| private final String accessKey; | ||
|
|
||
| private final String secretKey; | ||
|
|
||
| S3BasicCredentials(String accessKey, String secretKey) { | ||
| this.accessKey = accessKey; | ||
| this.secretKey = secretKey; | ||
| } | ||
|
|
||
| @Override | ||
| public final String getAWSAccessKeyId() { | ||
| return accessKey; | ||
| } | ||
|
|
||
| @Override | ||
| public final String getAWSSecretKey() { | ||
| return secretKey; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(final Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (o == null || getClass() != o.getClass()) { | ||
| return false; | ||
| } | ||
| final S3BasicCredentials that = (S3BasicCredentials) o; | ||
| return accessKey.equals(that.accessKey) && secretKey.equals(that.secretKey); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(accessKey, secretKey); | ||
| } | ||
| } |
57 changes: 57 additions & 0 deletions
57
...ository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BasicSessionCredentials.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| /* | ||
| * Licensed to Elasticsearch under one or more contributor | ||
| * license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright | ||
| * ownership. Elasticsearch licenses this file to you under | ||
| * the Apache License, Version 2.0 (the "License"); you may | ||
| * not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.elasticsearch.repositories.s3; | ||
|
|
||
| import com.amazonaws.auth.AWSSessionCredentials; | ||
|
|
||
| import java.util.Objects; | ||
|
|
||
| final class S3BasicSessionCredentials extends S3BasicCredentials implements AWSSessionCredentials { | ||
|
|
||
| private final String sessionToken; | ||
|
|
||
| S3BasicSessionCredentials(String accessKey, String secretKey, String sessionToken) { | ||
| super(accessKey, secretKey); | ||
| this.sessionToken = sessionToken; | ||
| } | ||
|
|
||
| @Override | ||
| public String getSessionToken() { | ||
| return sessionToken; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(final Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (o == null || getClass() != o.getClass()) { | ||
| return false; | ||
| } | ||
| final S3BasicSessionCredentials that = (S3BasicSessionCredentials) o; | ||
| return sessionToken.equals(that.sessionToken) && | ||
| getAWSAccessKeyId().equals(that.getAWSAccessKeyId()) && | ||
| getAWSSecretKey().equals(that.getAWSSecretKey()); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(sessionToken, getAWSAccessKeyId(), getAWSSecretKey()); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,10 +25,13 @@ | |
| import com.amazonaws.services.s3.model.ObjectListing; | ||
| import com.amazonaws.services.s3.model.S3ObjectSummary; | ||
| import com.amazonaws.services.s3.model.StorageClass; | ||
| import org.elasticsearch.cluster.metadata.RepositoryMetaData; | ||
| import org.elasticsearch.common.blobstore.BlobContainer; | ||
| import org.elasticsearch.common.blobstore.BlobPath; | ||
| import org.elasticsearch.common.blobstore.BlobStore; | ||
| import org.elasticsearch.common.blobstore.BlobStoreException; | ||
| import org.elasticsearch.common.collect.Tuple; | ||
| import org.elasticsearch.common.settings.Settings; | ||
| import org.elasticsearch.common.unit.ByteSizeValue; | ||
|
|
||
| import java.io.IOException; | ||
|
|
@@ -39,8 +42,6 @@ class S3BlobStore implements BlobStore { | |
|
|
||
| private final S3Service service; | ||
|
|
||
| private final String clientName; | ||
|
|
||
| private final String bucket; | ||
|
|
||
| private final ByteSizeValue bufferSize; | ||
|
|
@@ -51,15 +52,18 @@ class S3BlobStore implements BlobStore { | |
|
|
||
| private final StorageClass storageClass; | ||
|
|
||
| S3BlobStore(S3Service service, String clientName, String bucket, boolean serverSideEncryption, | ||
| ByteSizeValue bufferSize, String cannedACL, String storageClass) { | ||
| private final Tuple<RepositoryMetaData, Settings> settingsKey; | ||
|
|
||
| S3BlobStore(S3Service service, String bucket, boolean serverSideEncryption, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This constructor could be simplified now, making all the setting extraction from the metadata happen here. I just left that out of this step because it requires some noisy changes in the tests. |
||
| ByteSizeValue bufferSize, String cannedACL, String storageClass, | ||
| RepositoryMetaData repositoryMetaData, Settings settings) { | ||
| this.service = service; | ||
| this.clientName = clientName; | ||
| this.bucket = bucket; | ||
| this.serverSideEncryption = serverSideEncryption; | ||
| this.bufferSize = bufferSize; | ||
| this.cannedACL = initCannedACL(cannedACL); | ||
| this.storageClass = initStorageClass(storageClass); | ||
| settingsKey = new Tuple<>(repositoryMetaData, settings); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -68,7 +72,7 @@ public String toString() { | |
| } | ||
|
|
||
| public AmazonS3Reference clientReference() { | ||
| return service.client(clientName); | ||
| return service.client(settingsKey); | ||
| } | ||
|
|
||
| public String bucket() { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is perhaps a bit too coarse-grained as settings key? Should it just be repository metadata that's relevant for establishing connections?
Also, why are the settings (i.e. 2nd part of tuple) part of this key? These are static / fixed at node startup, so there's no need to cache them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ywelsch
I think I was a little paranoid here with reloads from the secret store but on second thought, that's wrong it's all handled by
refreshAndClearCachejust fine :). I'll simplify the key.Maybe in theory, but does it matter in practice? Creating a new repo client isn't absurdly expensive and updating the repo settings is a rare event? I agree we could create another key here but it seems like it's just a bunch of extra code complexity and then if we ever add a new relevant (for establishing the connection) setting we have to maintain this as well => maybe the rare case of needlessly creating a new client isn't a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if with this approach we never share clients between repo definitions (as these repos will probably be different in some way). Not sure how many repo definitions we typically have in clusters though and whether it matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait :) I can't take credit for it now, but I think I actually handled this just fine already :D
The actual client instances are cached by their
S3ClientSettingswhich only contain stuff needed for instantiating the clients.So
org.elasticsearch.repositories.s3.S3Service#clientsCacheshould deduplicate clients just fine. All we get are some redundant settings instances inorg.elasticsearch.repositories.s3.S3Service#derivedClientSettingswhere two different repo-metadata keys could point to equal settings instances.