S3 Repository: Eagerly load static settings#23910
Merged
rjernst merged 5 commits intoelastic:masterfrom Apr 11, 2017
Merged
Conversation
The S3 repostiory has many levels of settings it looks at to create a repository, and these settings were read at repository creation time. This meant secure settings like access and secret keys had to be available after node construction. This change makes setting loading for every except repository level settings eager, so that secure settings can be stashed, and the keystore can once again be closed after bootstrapping the node is complete.
dakrone
approved these changes
Apr 11, 2017
Member
dakrone
left a comment
There was a problem hiding this comment.
Looks good to me in general, I left some comments though
| } else { | ||
| static String findEndpoint(Logger logger, S3ClientSettings clientSettings, Settings repositorySettings) { | ||
| String endpoint = getRepoValue(repositorySettings, S3Repository.Repository.ENDPOINT_SETTING, clientSettings.endpoint); | ||
| if (Strings.isNullOrEmpty(endpoint) == false) { |
Member
There was a problem hiding this comment.
I think this could just be if (Strings.hasText(endpoint)) { ?
Member
Author
There was a problem hiding this comment.
This already existed, but I modified it as you suggested.
| return getValue(repositorySettings, globalSettings, repositorySetting, globalSetting); | ||
| /** Returns the value for a given setting from the repository, or returns the fallback value. */ | ||
| private static <T> T getRepoValue(Settings repositorySettings, Setting<T> repositorySetting, T fallback) { | ||
| if (repositorySetting.exists(repositorySettings)) { |
Member
There was a problem hiding this comment.
It's too bad our settings don't mimic the Map getOrDefault to make this one operation
| key -> new Setting<>(key, S3Repository.Repositories.ENDPOINT_SETTING, s -> s.toLowerCase(Locale.ROOT), | ||
| Setting.Property.NodeScope)); | ||
|
|
||
| /** The protocol to use to connec to to s3. */ |
| SecureString proxyUsername = getConfigValue(settings, clientName, PROXY_USERNAME_SETTING, CLOUD_S3.PROXY_USERNAME_SETTING); | ||
| SecureString proxyPassword = getConfigValue(settings, clientName, PROXY_PASSWORD_SETTING, CLOUD_S3.PROXY_PASSWORD_SETTING)) { | ||
| BasicAWSCredentials credentials = null; | ||
| if (accessKey.length() != 0) { |
Member
There was a problem hiding this comment.
I think this block would be simpler as:
if (accessKey.length() == 0) {
throw new IllegalArgumentException("Missing access key for s3 client [" + clientName + "]");
}
if (secretKey.length() == 0) {
throw new IllegalArgumentException("Missing secret key for s3 client [" + clientName + "]");
}
final BasicAWSCredentials credentials = new BasicAWSCredentials(accessKey.toString(), secretKey.toString());
return new S3ClientSettings(...);Instead of the nested if statements and double-negative-ish if statements
Member
Author
There was a problem hiding this comment.
Unfortunately that won't work. The current block allows both access key and secret key to be missing, meaning continue to use the builtin credential provider. What is there ensures if one is set, the other is set as well, otherwise an error.
| getConfigValue(settings, clientName, PROXY_PORT_SETTING, AwsS3Service.CLOUD_S3.PROXY_PORT_SETTING), | ||
| proxyUsername.toString(), | ||
| proxyPassword.toString(), | ||
| (int)getConfigValue(settings, clientName, READ_TIMEOUT_SETTING, AwsS3Service.CLOUD_S3.READ_TIMEOUT).millis() |
Member
There was a problem hiding this comment.
I assume this is returning a long? Is it okay if the read timeout is -128731 or something like that?
Member
Author
There was a problem hiding this comment.
This is the exact same behavior as the existing settings, but I think you are right that there is not validation. We should be using the Setting.timeSetting variant that takes a min value. I will open a followup to fix these settings for both s3 and ec2.
| public S3RepositoryPlugin(Settings settings) { | ||
| // eagerly load client settings so that secure settings are read | ||
| clientsSettings = S3ClientSettings.load(settings); | ||
| assert clientsSettings.isEmpty() == false; // always at least have "default" |
Member
There was a problem hiding this comment.
Might as well make the comment for this assert the actual assert message
rjernst
added a commit
that referenced
this pull request
Apr 12, 2017
The S3 repostiory has many levels of settings it looks at to create a repository, and these settings were read at repository creation time. This meant secure settings like access and secret keys had to be available after node construction. This change makes setting loading for every except repository level settings eager, so that secure settings can be stashed, and the keystore can once again be closed after bootstrapping the node is complete.
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Apr 13, 2017
* master: (41 commits) Remove awaits fix from evil JNA native tests Correct handling of default and array settings Build: Switch jna dependency to an elastic version (elastic#24081) fix CategoryContextMappingTests compilation bugs testConcurrentGetAndSetOnPrimary - fix a race condition between indexing and updating value map Allow different data types for category in Context suggester (elastic#23491) Restrict build info loading to ES jar, not any jar (elastic#24049) Remove more hidden file leniency from plugins Register error listener in evil logger tests Detect using logging before configuration [DOCS] Added note about Elastic Cloud to improve 'elastic aws' SERP results. Add version constant for 5.5 (elastic#24075) Add unit tests for NestedAggregator (elastic#24054) Add more debugging information to rethrottles Tests: Use random analyzer only on string fields in Match/MultiMatchBuilderTests Cleanup outdated comments for fixing up pom dependencies (elastic#24056) S3 Repository: Eagerly load static settings (elastic#23910) Reject duplicate settings on the command line Wildcard cluster names for cross cluster search (elastic#23985) Update scripts/security docs for sandboxed world (elastic#23977) ...
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The S3 repostiory has many levels of settings it looks at to create a
repository, and these settings were read at repository creation time.
This meant secure settings like access and secret keys had to be
available after node construction. This change makes setting loading for
every except repository level settings eager, so that secure settings
can be stashed, and the keystore can once again be closed after
bootstrapping the node is complete.