-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Provide an Option to Use Path-Style-Access with S3 Repo #41966
Changes from 2 commits
713698a
13e3311
4530b79
897516a
5c85170
ad1ead9
22ecda1
0979112
3110d3c
9582749
42fa7e4
af3f0a1
c865f8d
c13bf0c
f76a14e
a2b8d8e
5ba5ac4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,6 +95,10 @@ final class S3ClientSettings { | |
static final Setting.AffixSetting<Boolean> USE_THROTTLE_RETRIES_SETTING = Setting.affixKeySetting(PREFIX, "use_throttle_retries", | ||
key -> Setting.boolSetting(key, ClientConfiguration.DEFAULT_THROTTLE_RETRIES, Property.NodeScope)); | ||
|
||
/** Whether the s3 client should use path style access. */ | ||
static final Setting.AffixSetting<Boolean> USE_PATH_STYLE_ACCESS = Setting.affixKeySetting(PREFIX, "path_style_access", | ||
key -> Setting.boolSetting(key, false, Property.NodeScope, Property.Deprecated)); | ||
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. Now that AWS changed course on this one and has announced continued support for this access pattern for existing/older buckets (https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story), maybe there's no need to change the default here before |
||
|
||
/** Credentials to authenticate with s3. */ | ||
final S3BasicCredentials credentials; | ||
|
||
|
@@ -127,9 +131,13 @@ final class S3ClientSettings { | |
/** Whether the s3 client should use an exponential backoff retry policy. */ | ||
final boolean throttleRetries; | ||
|
||
/** Whether the s3 client should use path style access. */ | ||
final boolean pathStyleAccess; | ||
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. The AWS SDK uses this as a ternary setting (true, false, null), with null being the default, which enables the path-style setting based on the configured endpoint. I think we should do treat this setting exactly the same way, allowing path-style setting both being explicitly enabled and disabled. 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. Sounds good, so we break behavior after all by changing the default to 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. It's unclear what the behaviour of |
||
|
||
private S3ClientSettings(S3BasicCredentials credentials, String endpoint, Protocol protocol, | ||
String proxyHost, int proxyPort, String proxyUsername, String proxyPassword, | ||
int readTimeoutMillis, int maxRetries, boolean throttleRetries) { | ||
int readTimeoutMillis, int maxRetries, boolean throttleRetries, | ||
boolean pathStyleAccess) { | ||
this.credentials = credentials; | ||
this.endpoint = endpoint; | ||
this.protocol = protocol; | ||
|
@@ -140,6 +148,7 @@ private S3ClientSettings(S3BasicCredentials credentials, String endpoint, Protoc | |
this.readTimeoutMillis = readTimeoutMillis; | ||
this.maxRetries = maxRetries; | ||
this.throttleRetries = throttleRetries; | ||
this.pathStyleAccess = pathStyleAccess; | ||
} | ||
|
||
/** | ||
|
@@ -162,6 +171,7 @@ S3ClientSettings refine(RepositoryMetaData metadata) { | |
getRepoSettingOrDefault(READ_TIMEOUT_SETTING, normalizedSettings, TimeValue.timeValueMillis(readTimeoutMillis)).millis()); | ||
final int newMaxRetries = getRepoSettingOrDefault(MAX_RETRIES_SETTING, normalizedSettings, maxRetries); | ||
final boolean newThrottleRetries = getRepoSettingOrDefault(USE_THROTTLE_RETRIES_SETTING, normalizedSettings, throttleRetries); | ||
final boolean usePathStyleAccess = getRepoSettingOrDefault(USE_PATH_STYLE_ACCESS, normalizedSettings, pathStyleAccess); | ||
final S3BasicCredentials newCredentials; | ||
if (checkDeprecatedCredentials(repoSettings)) { | ||
newCredentials = loadDeprecatedCredentials(repoSettings); | ||
|
@@ -183,7 +193,8 @@ S3ClientSettings refine(RepositoryMetaData metadata) { | |
proxyPassword, | ||
newReadTimeoutMillis, | ||
newMaxRetries, | ||
newThrottleRetries | ||
newThrottleRetries, | ||
usePathStyleAccess | ||
); | ||
} | ||
|
||
|
@@ -270,7 +281,8 @@ static S3ClientSettings getClientSettings(final Settings settings, final String | |
proxyPassword.toString(), | ||
Math.toIntExact(getConfigValue(settings, clientName, READ_TIMEOUT_SETTING).millis()), | ||
getConfigValue(settings, clientName, MAX_RETRIES_SETTING), | ||
getConfigValue(settings, clientName, USE_THROTTLE_RETRIES_SETTING) | ||
getConfigValue(settings, clientName, USE_THROTTLE_RETRIES_SETTING), | ||
getConfigValue(settings, clientName, USE_PATH_STYLE_ACCESS) | ||
); | ||
} | ||
} | ||
|
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.
only those that are not on AWS and that do not use IP-address based endpoint.