-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
S3 Repository: Eagerly load static settings #23910
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.
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.
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) { |
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 think this could just be if (Strings.hasText(endpoint)) {
?
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 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)) { |
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.
It's too bad our settings don't mimic the Map
getOrDefault
to make this one operation
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.
Indeed...
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. */ |
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.
connec to to -> connect to
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) { |
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 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
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.
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() |
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 assume this is returning a long? Is it okay if the read timeout is -128731 or something like that?
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 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" |
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.
Might as well make the comment for this assert the actual assert message
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.
* 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) ...
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.