Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions docs/plugins/repository-s3.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,17 @@ settings belong in the `elasticsearch.yml` file.
Whether retries should be throttled (i.e. should back off). Must be `true`
or `false`. Defaults to `true`.

`path_style_access`::

Whether to use the path style access pattern. If `true`, the path style access pattern will be used. If set to`false`,
original-brownbear marked this conversation as resolved.
Show resolved Hide resolved
original-brownbear marked this conversation as resolved.
Show resolved Hide resolved
the AWS Java SDK decide whether to use the path style access pattern dynamically
original-brownbear marked this conversation as resolved.
Show resolved Hide resolved
(See https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/services/s3/AmazonS3Builder.html#setPathStyleAccessEnabled-java.lang.Boolean-[AWS documentation] for details).
Defaults to `false`.

NOTE: In versions `7.0` and `7.1`, all bucket operations were using the path style access pattern in every situation without an option to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7.0, 7.1 and 7.2

disable it. If your deployment requires path style access to be used you might need to manually configure this setting to `true` when
upgrading.

[float]
[[repository-s3-compatible-services]]
===== S3-compatible services
Expand Down Expand Up @@ -381,10 +392,6 @@ bucket, in this example, named "foo".
The bucket needs to exist to register a repository for snapshots. If you did not
create the bucket then the repository registration will fail.

Note: Starting in version 7.0, all bucket operations are using the path style
access pattern. In previous versions the decision to use virtual hosted style or
path style access was made by the AWS Java SDK.

[[repository-s3-aws-vpc]]
[float]
==== AWS VPC Bandwidth Settings
Expand Down
12 changes: 12 additions & 0 deletions docs/reference/migration/migrate_8_0/snapshots.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,15 @@ This change will affect both newly created repositories and existing repositorie
explicitly specified.

For more information on the compress option, see <<modules-snapshots>>

[float]
==== The S3 repository plugin uses the DNS style access pattern by default

Starting in version 7.3 using the path style access pattern with the S3 repository is not enabled by default.
In versions 7.0, 7.1, and 7.2 the S3 repository plugin was exclusively using the path style access pattern. This is a breaking
change for deployments that do not also allow for the DNS style but are recognized as supporting DNS style access by the AWS SDK.
If your deployment does only support path style access and is affected by this change you must configure the S3 client setting
`path_style_access` to `true` to return to the behaviour of always using path style access.
This breaking change was made necessary by
https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story[AWS's announcement] to no longer support
the path-style API past September 30th, 2020 for newly created buckets.
Original file line number Diff line number Diff line change
Expand Up @@ -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));

/** Credentials to authenticate with s3. */
final S3BasicCredentials credentials;

Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 null? See my comment above, this may not be necessary (at least in 7.x)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear what the behaviour of setPathStyleAccessEnabled(Boolean.FALSE) is from the SDK docs. I think we're doing the right thing here, either enabling it or leaving it unset.


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;
Expand All @@ -140,6 +148,7 @@ private S3ClientSettings(S3BasicCredentials credentials, String endpoint, Protoc
this.readTimeoutMillis = readTimeoutMillis;
this.maxRetries = maxRetries;
this.throttleRetries = throttleRetries;
this.pathStyleAccess = pathStyleAccess;
}

/**
Expand All @@ -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);
Expand All @@ -183,7 +193,8 @@ S3ClientSettings refine(RepositoryMetaData metadata) {
proxyPassword,
newReadTimeoutMillis,
newMaxRetries,
newThrottleRetries
newThrottleRetries,
usePathStyleAccess
);
}

Expand Down Expand Up @@ -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)
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ public List<Setting<?>> getSettings() {
S3ClientSettings.READ_TIMEOUT_SETTING,
S3ClientSettings.MAX_RETRIES_SETTING,
S3ClientSettings.USE_THROTTLE_RETRIES_SETTING,
S3ClientSettings.USE_PATH_STYLE_ACCESS,
S3Repository.ACCESS_KEY_SETTING,
S3Repository.SECRET_KEY_SETTING);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,10 @@ AmazonS3 buildClient(final S3ClientSettings clientSettings) {
//
// We do this because directly constructing the client is deprecated (was already deprecated in 1.1.223 too)
// so this change removes that usage of a deprecated API.
builder.withEndpointConfiguration(new AwsClientBuilder.EndpointConfiguration(endpoint, null))
.enablePathStyleAccess();

builder.withEndpointConfiguration(new AwsClientBuilder.EndpointConfiguration(endpoint, null));
if (clientSettings.pathStyleAccess) {
builder.enablePathStyleAccess();
}
return builder.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,4 +144,11 @@ public void testRefineWithRepoSettings() {
assertThat(credentials.getSessionToken(), is("session_token"));
}
}

public void testPathStyleAccessCanBeSet() {
final Map<String, S3ClientSettings> settings = S3ClientSettings.load(
Settings.builder().put("s3.client.other.path_style_access", true).build());
assertThat(settings.get("default").pathStyleAccess, is(false));
assertThat(settings.get("other").pathStyleAccess, is(true));
}
}