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 5 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
16 changes: 12 additions & 4 deletions docs/plugins/repository-s3.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,18 @@ 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 false, DNS style access will
Copy link
Contributor

Choose a reason for hiding this comment

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

this sentence reads a bit funny with the if false not being delimited

be used. Defaults to letting the AWS S3 SDK decide the right access pattern dynamically.
Copy link
Contributor

Choose a reason for hiding this comment

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

AWS Java SDK

Copy link
Contributor

Choose a reason for hiding this comment

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

s/right access pattern/path style access/

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a link to the relevant SDK docs


NOTE: This setting is deprecated and only intended as a stop-gap solution to provide
compatibility with alternative S3 implementations that do not provide compatibility with the domain style access pattern.
AWS S3 will stop supporting the path style access pattern for new buckets from
https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story[September 30th, 2020].
Any setups using the path style access pattern should be switched to the domain style access pattern as soon as possible to ensure
continued compatibility with the S3 repository plugin.
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically it depends on how long AWS is going to support path-style-access on their Java SDK. I couldn't find any information on that. I'm not sure whether we should word this here as strongly, but instead point out that we rely on the SDK and that if that configuration option is going away, we will not be able to support this anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't toning it down a little inconsistent with us now making a breaking change in 7.x?
My thinking was that we're doing this breaking change because we fear that future versions of the SDK won't support path style access. If we don't think this is an imminent enough issue to strongly push users towards adjusting their environment asap then why make a breaking change and not just wait for 8.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't toning it down a little inconsistent with us now making a breaking change in 7.x?

It's just going back to the pre-7.0 behavior, which was the more sensible default, especially given the current strategy of Amazon not to support path-style access in the future anymore, and there being no way to explicitly force-disable it in the SDK (i.e. it would still use path-style access with IP-based endpoint). This here makes it sound like we would break Minio support with path-style access at any moment in time, which sounds perhaps a bit overdramatic.


[float]
[[repository-s3-compatible-services]]
===== S3-compatible services
Expand Down Expand Up @@ -381,10 +393,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 deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we should deprecate this at this time. AWS's SDK is going to support path-style access for a very long time to come (even after Sept. 2020, given that the SDK will still be used to connect to older repos).

Copy link
Member Author

Choose a reason for hiding this comment

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

Same point as in the above, why make a breaking change then (that we know for a fact will break some environments)?

Copy link
Contributor

Choose a reason for hiding this comment

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

The focus of this section should be on the fact that the behavior changes between 7.2- and 7.3+, going back to the 6.x behavior, and that the 7.0-7.2 behavior can be restored by setting this new setting. The deprecation of that new setting is secondary. As said, given that we don't have a time frame for removing this setting, I would prefer to defer the deprecation to a later time.

Previously the S3 repository plugin was exclusively using the path style access pattern. This is a breaking
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps qualify this by "In 7.0, 7.1 and 7.2"

change for deployments that do not also allow for the DNS style access pattern. For short-term compatibility with these deployments users
Copy link
Contributor

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.

must configure the S3 client setting `path_style_access` to `true` to retain the previous behaviour.

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 @@ -21,6 +21,7 @@

import com.amazonaws.ClientConfiguration;
import com.amazonaws.Protocol;
import com.amazonaws.services.s3.AmazonS3Builder;
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
import org.elasticsearch.common.settings.SecureSetting;
import org.elasticsearch.common.settings.SecureString;
Expand Down Expand Up @@ -95,6 +96,12 @@ 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<S3ClientSettings.PathStyleAccess> USE_PATH_STYLE_ACCESS = Setting.affixKeySetting(
PREFIX, "path_style_access",
key -> new Setting<>(key, s -> "default", s -> Boolean.parseBoolean(s) ? PathStyleAccess.ENABLED : PathStyleAccess.DISABLED,
Copy link
Contributor

Choose a reason for hiding this comment

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

this PR is missing tests, and I doubt that the parsing here is correct given that it never yields PathStyleAccess.DEFAULT

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea was to simply only allow parsing true and false here and let the DEFAULT represent the setting not being set (I didn't see much value in allowing to explicitly set "default" or "null"). Not a good idea? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

We can make this a boolean setting (see my comment above) and use S3ClientOptions.DEFAULT_PATH_STYLE_ACCESS, which is false, as the default value. We should also add a test to make sure this default never changes.

Property.NodeScope, Property.Deprecated));
Copy link
Contributor

Choose a reason for hiding this comment

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

As said above, I don't think we should log deprecation warnings.


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

Expand Down Expand Up @@ -127,9 +134,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 PathStyleAccess pathStyleAccess;

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

/**
Expand All @@ -162,6 +174,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 PathStyleAccess usePathStyleAccess = getRepoSettingOrDefault(USE_PATH_STYLE_ACCESS, normalizedSettings, pathStyleAccess);
final S3BasicCredentials newCredentials;
if (checkDeprecatedCredentials(repoSettings)) {
newCredentials = loadDeprecatedCredentials(repoSettings);
Expand All @@ -183,7 +196,8 @@ S3ClientSettings refine(RepositoryMetaData metadata) {
proxyPassword,
newReadTimeoutMillis,
newMaxRetries,
newThrottleRetries
newThrottleRetries,
usePathStyleAccess
);
}

Expand Down Expand Up @@ -270,7 +284,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 Expand Up @@ -314,4 +329,22 @@ private static <T> T getRepoSettingOrDefault(Setting.AffixSetting<T> setting, Se
}
return defaultValue;
}

/**
* Settings for path style access behavior.
*/
enum PathStyleAccess {
/**
* Let SDK decide whether to use path style access.
*/
DEFAULT,
/**
* Force use of path style access, i.e set {@link AmazonS3Builder#setPathStyleAccessEnabled(Boolean)} to {@code true}.
*/
ENABLED,
/**
* Don't use path style access, i.e set {@link AmazonS3Builder#setPathStyleAccessEnabled(Boolean)} to {@code false}.
*/
DISABLED
}
}
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,8 +153,15 @@ 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 == S3ClientSettings.PathStyleAccess.ENABLED) {
builder.enablePathStyleAccess();
} else if (clientSettings.pathStyleAccess == S3ClientSettings.PathStyleAccess.DISABLED) {
builder.setPathStyleAccessEnabled(false);
} else {
assert clientSettings.pathStyleAccess == S3ClientSettings.PathStyleAccess.DEFAULT;
assert builder.isPathStyleAccessEnabled() == null;
}

return builder.build();
}
Expand Down