diff --git a/docs/changelog/126843.yaml b/docs/changelog/126843.yaml index 77d3916c31955..1497f75b2ea74 100644 --- a/docs/changelog/126843.yaml +++ b/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. diff --git a/modules/repository-s3/src/javaRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3ExplicitProtocolRestIT.java b/modules/repository-s3/src/javaRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3ExplicitProtocolRestIT.java new file mode 100644 index 0000000000000..bc4bac1209cd1 --- /dev/null +++ b/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 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; + } +} diff --git a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java index 48ec20992ec20..797a16240f338 100644 --- a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java +++ b/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 UNUSED_PROTOCOL_SETTING = Setting.affixKeySetting( + static final Setting.AffixSetting 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, diff --git a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java index 64295a7249ed6..43520bb123647 100644 --- a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java +++ b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java @@ -131,7 +131,7 @@ public List> 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, diff --git a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java index 07dfb332cbf8c..82f0ea5964963 100644 --- a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java +++ b/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() ); } diff --git a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ClientSettingsTests.java b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ClientSettingsTests.java index 3de070934bdbd..5e7f083de8eb2 100644 --- a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ClientSettingsTests.java +++ b/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)); diff --git a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java b/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java index d4cb72dd04257..dd18932cea7d3 100644 --- a/modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java +++ b/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(); } }