Provide an Option to Use Path-Style-Access with S3 Repo (#41966)

* Provide an Option to Use Path-Style-Access with S3 Repo

* As discussed, added the option to use path style access back again and
deprecated it.
* Defaulted to `false`
* Added warning to docs

* Closes #41816

docs/plugins/repository-s3.asciidoc

@@ -145,6 +145,20 @@ 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 force the use of the path style access pattern. If `true`, the
+   path style access pattern will be used. If `false`, the access pattern will
+   be automatically determined by the AWS Java SDK (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`.
+
+[[repository-s3-path-style-deprecation]]
+NOTE: In versions `7.0`, `7.1`, `7.2` and `7.3` all bucket operations used the
+https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story/[now-deprecated]
+path style access pattern. If your deployment requires the path style access
+pattern then you should set this setting to `true` when upgrading.
+
 [float]
 [[repository-s3-compatible-services]]
 ===== S3-compatible services
@@ -381,10 +395,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

docs/reference/migration/migrate_8_0/snapshots.asciidoc

@@ -81,3 +81,18 @@ 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.4 the `repository-s3` plugin does not use the
+now-deprecated path-style access pattern by default. In versions 7.0, 7.1, 7.2
+and 7.3 the `repository-s3` plugin always used the path-style access pattern.
+This is a breaking change for deployments that only support path-style access
+but which are recognized as supporting DNS-style access by the AWS SDK. If your
+deployment only supports path-style access and is affected by this change then
+you must configure the S3 client setting `path_style_access` to `true`. 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] that the path-style access pattern is deprecated and will be
+unsupported on buckets created after September 30th 2020.

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java

@@ -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;
 
@@ -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;
+
     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 @@ final class S3ClientSettings {
         this.readTimeoutMillis = readTimeoutMillis;
         this.maxRetries = maxRetries;
         this.throttleRetries = throttleRetries;
+        this.pathStyleAccess = pathStyleAccess;
     }
 
     /**
@@ -162,6 +171,7 @@ final class S3ClientSettings {
             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 @@ final class S3ClientSettings {
             proxyPassword,
             newReadTimeoutMillis,
             newMaxRetries,
-            newThrottleRetries
+            newThrottleRetries,
+            usePathStyleAccess
         );
     }
 
@@ -270,7 +281,8 @@ final class S3ClientSettings {
                 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)
             );
         }
     }

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java

@@ -105,6 +105,7 @@ public class S3RepositoryPlugin extends Plugin implements RepositoryPlugin, Relo
             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);
     }

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java

@@ -153,9 +153,10 @@ class S3Service implements Closeable {
         //
         // 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();
     }
 

plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ClientSettingsTests.java

@@ -144,4 +144,11 @@ public class S3ClientSettingsTests extends ESTestCase {
             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));
+    }
 }