Reinstate use of S3 protocol client setting (#127744)

The `s3.client.CLIENT_NAME.protocol` setting became unused in #126843 as
it is inapplicable in the v2 SDK. However, the v2 SDK requires the
`s3.client.CLIENT_NAME.endpoint` setting to be a URL that includes a
scheme, so in #127489 we prepend a `https://` to the endpoint if needed.
This commit generalizes this slightly so that we prepend `http://` if
the endpoint has no scheme and the `.protocol` setting is set to `http`.

Author: DaveCTurner

docs/changelog/126843.yaml

@@ -42,8 +42,7 @@ breaking:
       `com.amazonaws.sdk.ec2MetadataServiceEndpointOverride` system property.
 
     * AWS SDK v2 does not permit specifying a choice between HTTP and HTTPS so
-      the `s3.client.${CLIENT_NAME}.protocol` setting is deprecated and no longer
-      has any effect.
+      the `s3.client.${CLIENT_NAME}.protocol` setting is deprecated.
 
     * AWS SDK v2 does not permit control over throttling for retries, so the
       the `s3.client.${CLIENT_NAME}.use_throttle_retries` setting is deprecated
@@ -81,9 +80,9 @@ breaking:
     * If applicable, discontinue use of the
       `com.amazonaws.sdk.ec2MetadataServiceEndpointOverride` system property.
 
-    * If applicable, specify that you wish to use the insecure HTTP protocol to
-      access the S3 API by setting `s3.client.${CLIENT_NAME}.endpoint` to a URL
-      which starts with `http://`.
+    * If applicable, specify the protocol to use to access the S3 API by
+      setting `s3.client.${CLIENT_NAME}.endpoint` to a URL which starts with
+      `http://` or `https://`.
 
     * If applicable, discontinue use of the `log-delivery-write` canned ACL.
 

modules/repository-s3/src/javaRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3ExplicitProtocolRestIT.java

@@ -0,0 +1,85 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the "Elastic License
+ * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
+ * Public License v 1"; you may not use this file except in compliance with, at
+ * your election, the "Elastic License 2.0", the "GNU Affero General Public
+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.repositories.s3;
+
+import fixture.aws.DynamicRegionSupplier;
+import fixture.s3.S3HttpFixture;
+
+import com.carrotsearch.randomizedtesting.annotations.ThreadLeakFilters;
+import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
+
+import org.elasticsearch.test.cluster.ElasticsearchCluster;
+import org.elasticsearch.test.fixtures.testcontainers.TestContainersThreadFilter;
+import org.junit.ClassRule;
+import org.junit.rules.RuleChain;
+import org.junit.rules.TestRule;
+
+import java.util.function.Supplier;
+
+import static fixture.aws.AwsCredentialsUtils.fixedAccessKey;
+import static org.hamcrest.Matchers.startsWith;
+
+@ThreadLeakFilters(filters = { TestContainersThreadFilter.class })
+@ThreadLeakScope(ThreadLeakScope.Scope.NONE) // https://github.com/elastic/elasticsearch/issues/102482
+public class RepositoryS3ExplicitProtocolRestIT extends AbstractRepositoryS3RestTestCase {
+
+    private static final String PREFIX = getIdentifierPrefix("RepositoryS3ExplicitProtocolRestIT");
+    private static final String BUCKET = PREFIX + "bucket";
+    private static final String BASE_PATH = PREFIX + "base_path";
+    private static final String ACCESS_KEY = PREFIX + "access-key";
+    private static final String SECRET_KEY = PREFIX + "secret-key";
+    private static final String CLIENT = "explicit_protocol_client";
+
+    private static final Supplier<String> regionSupplier = new DynamicRegionSupplier();
+    private static final S3HttpFixture s3Fixture = new S3HttpFixture(
+        true,
+        BUCKET,
+        BASE_PATH,
+        fixedAccessKey(ACCESS_KEY, regionSupplier, "s3")
+    );
+
+    private static String getEndpoint() {
+        final var s3FixtureAddress = s3Fixture.getAddress();
+        assertThat(s3FixtureAddress, startsWith("http://"));
+        return s3FixtureAddress.substring("http://".length());
+    }
+
+    public static ElasticsearchCluster cluster = ElasticsearchCluster.local()
+        .module("repository-s3")
+        .systemProperty("aws.region", regionSupplier)
+        .keystore("s3.client." + CLIENT + ".access_key", ACCESS_KEY)
+        .keystore("s3.client." + CLIENT + ".secret_key", SECRET_KEY)
+        .setting("s3.client." + CLIENT + ".endpoint", RepositoryS3ExplicitProtocolRestIT::getEndpoint)
+        .setting("s3.client." + CLIENT + ".protocol", () -> "http")
+        .build();
+
+    @ClassRule
+    public static TestRule ruleChain = RuleChain.outerRule(s3Fixture).around(cluster);
+
+    @Override
+    protected String getTestRestCluster() {
+        return cluster.getHttpAddresses();
+    }
+
+    @Override
+    protected String getBucketName() {
+        return BUCKET;
+    }
+
+    @Override
+    protected String getBasePath() {
+        return BASE_PATH;
+    }
+
+    @Override
+    protected String getClientName() {
+        return CLIENT;
+    }
+}

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

@@ -77,9 +77,10 @@ final class S3ClientSettings {
         key -> new Setting<>(key, "", s -> s.toLowerCase(Locale.ROOT), Property.NodeScope)
     );
 
-    /** Formerly the protocol to use to connect to s3, now unused. V2 AWS SDK can infer the protocol from {@link #endpoint}. */
+    /** The protocol to use to connect to s3, now only used if {@link #endpoint} is not a proper URI that starts with {@code http://} or
+     * {@code https://}. */
     @UpdateForV10(owner = UpdateForV10.Owner.DISTRIBUTED_COORDINATION) // no longer used, should be removed in v10
-    static final Setting.AffixSetting<HttpScheme> UNUSED_PROTOCOL_SETTING = Setting.affixKeySetting(
+    static final Setting.AffixSetting<HttpScheme> PROTOCOL_SETTING = Setting.affixKeySetting(
         PREFIX,
         "protocol",
         key -> new Setting<>(key, "https", s -> HttpScheme.valueOf(s.toUpperCase(Locale.ROOT)), Property.NodeScope, Property.Deprecated)
@@ -181,6 +182,9 @@ final class S3ClientSettings {
     /** Credentials to authenticate with s3. */
     final AwsCredentials credentials;
 
+    /** The scheme (HTTP or HTTPS) for talking to the endpoint, for use only if the endpoint doesn't contain an explicit scheme */
+    final HttpScheme protocol;
+
     /** The s3 endpoint the client should talk to, or empty string to use the default. */
     final String endpoint;
 
@@ -221,6 +225,7 @@ final class S3ClientSettings {
 
     private S3ClientSettings(
         AwsCredentials credentials,
+        HttpScheme protocol,
         String endpoint,
         String proxyHost,
         int proxyPort,
@@ -235,6 +240,7 @@ private S3ClientSettings(
         String region
     ) {
         this.credentials = credentials;
+        this.protocol = protocol;
         this.endpoint = endpoint;
         this.proxyHost = proxyHost;
         this.proxyPort = proxyPort;
@@ -261,6 +267,7 @@ S3ClientSettings refine(Settings repositorySettings) {
             .put(repositorySettings)
             .normalizePrefix(PREFIX + PLACEHOLDER_CLIENT + '.')
             .build();
+        final HttpScheme newProtocol = getRepoSettingOrDefault(PROTOCOL_SETTING, normalizedSettings, protocol);
         final String newEndpoint = getRepoSettingOrDefault(ENDPOINT_SETTING, normalizedSettings, endpoint);
 
         final String newProxyHost = getRepoSettingOrDefault(PROXY_HOST_SETTING, normalizedSettings, proxyHost);
@@ -284,7 +291,8 @@ S3ClientSettings refine(Settings repositorySettings) {
             newCredentials = credentials;
         }
         final String newRegion = getRepoSettingOrDefault(REGION, normalizedSettings, region);
-        if (Objects.equals(endpoint, newEndpoint)
+        if (Objects.equals(protocol, newProtocol)
+            && Objects.equals(endpoint, newEndpoint)
             && Objects.equals(proxyHost, newProxyHost)
             && proxyPort == newProxyPort
             && proxyScheme == newProxyScheme
@@ -299,6 +307,7 @@ S3ClientSettings refine(Settings repositorySettings) {
         }
         return new S3ClientSettings(
             newCredentials,
+            newProtocol,
             newEndpoint,
             newProxyHost,
             newProxyPort,
@@ -405,6 +414,7 @@ static S3ClientSettings getClientSettings(final Settings settings, final String
         ) {
             return new S3ClientSettings(
                 S3ClientSettings.loadCredentials(settings, clientName),
+                getConfigValue(settings, clientName, PROTOCOL_SETTING),
                 getConfigValue(settings, clientName, ENDPOINT_SETTING),
                 getConfigValue(settings, clientName, PROXY_HOST_SETTING),
                 getConfigValue(settings, clientName, PROXY_PORT_SETTING),
@@ -435,6 +445,7 @@ public boolean equals(final Object o) {
             && maxConnections == that.maxConnections
             && maxRetries == that.maxRetries
             && Objects.equals(credentials, that.credentials)
+            && Objects.equals(protocol, that.protocol)
             && Objects.equals(endpoint, that.endpoint)
             && Objects.equals(proxyHost, that.proxyHost)
             && proxyScheme == that.proxyScheme
@@ -448,6 +459,7 @@ public boolean equals(final Object o) {
     public int hashCode() {
         return Objects.hash(
             credentials,
+            protocol,
             endpoint,
             proxyHost,
             proxyPort,

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

@@ -131,7 +131,7 @@ public List<Setting<?>> getSettings() {
             S3ClientSettings.SECRET_KEY_SETTING,
             S3ClientSettings.SESSION_TOKEN_SETTING,
             S3ClientSettings.ENDPOINT_SETTING,
-            S3ClientSettings.UNUSED_PROTOCOL_SETTING,
+            S3ClientSettings.PROTOCOL_SETTING,
             S3ClientSettings.PROXY_HOST_SETTING,
             S3ClientSettings.PROXY_PORT_SETTING,
             S3ClientSettings.PROXY_SCHEME_SETTING,

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

@@ -257,14 +257,18 @@ protected S3ClientBuilder buildClientBuilder(S3ClientSettings clientSettings, Sd
             String endpoint = clientSettings.endpoint;
             if ((endpoint.startsWith("http://") || endpoint.startsWith("https://")) == false) {
                 // The SDK does not know how to interpret endpoints without a scheme prefix and will error. Therefore, when the scheme is
-                // absent, we'll supply HTTPS as a default to avoid errors.
+                // absent, we'll look at the deprecated .protocol setting
                 // See https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/client-configuration.html#client-config-other-diffs
-                endpoint = "https://" + endpoint;
+                endpoint = switch (clientSettings.protocol) {
+                    case HTTP -> "http://" + endpoint;
+                    case HTTPS -> "https://" + endpoint;
+                };
                 LOGGER.warn(
                     """
-                        found S3 client with endpoint [{}] that is missing a scheme, guessing it should use 'https://'; \
+                        found S3 client with endpoint [{}] that is missing a scheme, guessing it should be [{}]; \
                         to suppress this warning, add a scheme prefix to the [{}] setting on this node""",
                     clientSettings.endpoint,
+                    endpoint,
                     S3ClientSettings.ENDPOINT_SETTING.getConcreteSettingForNamespace("CLIENT_NAME").getKey()
                 );
             }

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

@@ -34,6 +34,7 @@ public void testThereIsADefaultClientByDefault() {
 
         final S3ClientSettings defaultSettings = settings.get("default");
         assertThat(defaultSettings.credentials, nullValue());
+        assertThat(defaultSettings.protocol, is(HttpScheme.HTTPS));
         assertThat(defaultSettings.endpoint, is(emptyString()));
         assertThat(defaultSettings.proxyHost, is(emptyString()));
         assertThat(defaultSettings.proxyPort, is(80));

modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java

@@ -14,7 +14,6 @@
 import software.amazon.awssdk.core.retry.conditions.RetryCondition;
 import software.amazon.awssdk.http.SdkHttpClient;
 import software.amazon.awssdk.regions.Region;
-import software.amazon.awssdk.services.s3.S3Client;
 import software.amazon.awssdk.services.s3.endpoints.S3EndpointParams;
 import software.amazon.awssdk.services.s3.endpoints.internal.DefaultS3EndpointProvider;
 import software.amazon.awssdk.services.s3.model.S3Exception;
@@ -223,20 +222,57 @@ public void testGetClientRegionFallbackToUsEast1() {
     }
 
     public void testEndpointOverrideSchemeDefaultsToHttpsWhenNotSpecified() {
-        final S3Service s3Service = new S3Service(
-            mock(Environment.class),
-            Settings.EMPTY,
-            mock(ResourceWatcherService.class),
-            () -> Region.of("es-test-region")
+        final var endpointWithoutScheme = randomIdentifier() + ".ignore";
+        final var clientName = randomIdentifier();
+        assertThat(
+            getEndpointUri(Settings.builder().put("s3.client." + clientName + ".endpoint", endpointWithoutScheme), clientName),
+            equalTo(URI.create("https://" + endpointWithoutScheme))
+        );
+    }
+
+    public void testEndpointOverrideSchemeUsesHttpsIfHttpsProtocolSpecified() {
+        final var endpointWithoutScheme = randomIdentifier() + ".ignore";
+        final var clientName = randomIdentifier();
+        assertThat(
+            getEndpointUri(
+                Settings.builder()
+                    .put("s3.client." + clientName + ".endpoint", endpointWithoutScheme)
+                    .put("s3.client." + clientName + ".protocol", "https"),
+                clientName
+            ),
+            equalTo(URI.create("https://" + endpointWithoutScheme))
         );
-        final String endpointWithoutScheme = randomIdentifier() + ".ignore";
-        S3Client s3Client = s3Service.buildClient(
-            S3ClientSettings.getClientSettings(
-                Settings.builder().put("s3.client.test-client.endpoint", endpointWithoutScheme).build(),
-                "test-client"
+        assertWarnings(Strings.format("""
+            [s3.client.%s.protocol] setting was deprecated in Elasticsearch and will be removed in a future release. \
+            See the breaking changes documentation for the next major version.""", clientName));
+    }
+
+    public void testEndpointOverrideSchemeUsesHttpIfHttpProtocolSpecified() {
+        final var endpointWithoutScheme = randomIdentifier() + ".ignore";
+        final var clientName = randomIdentifier();
+        assertThat(
+            getEndpointUri(
+                Settings.builder()
+                    .put("s3.client." + clientName + ".endpoint", endpointWithoutScheme)
+                    .put("s3.client." + clientName + ".protocol", "http"),
+                clientName
             ),
-            mock(SdkHttpClient.class)
+            equalTo(URI.create("http://" + endpointWithoutScheme))
         );
-        assertThat(s3Client.serviceClientConfiguration().endpointOverride().get(), equalTo(URI.create("https://" + endpointWithoutScheme)));
+        assertWarnings(Strings.format("""
+            [s3.client.%s.protocol] setting was deprecated in Elasticsearch and will be removed in a future release. \
+            See the breaking changes documentation for the next major version.""", clientName));
+    }
+
+    private static URI getEndpointUri(Settings.Builder settings, String clientName) {
+        return new S3Service(
+            mock(Environment.class),
+            Settings.EMPTY,
+            mock(ResourceWatcherService.class),
+            () -> Region.of(randomIdentifier())
+        ).buildClient(S3ClientSettings.getClientSettings(settings.build(), clientName), mock(SdkHttpClient.class))
+            .serviceClientConfiguration()
+            .endpointOverride()
+            .get();
     }
 }