Skip to content

Commit db561c1

Browse files
DaveCTurnerywangd
DaveCTurner
authored and
ywangd
committed
Reinstate use of S3 protocol client setting (elastic#127744)
The `s3.client.CLIENT_NAME.protocol` setting became unused in elastic#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 elastic#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`.
1 parent 7fcf1b3 commit db561c1

File tree

7 files changed

+162
-25
lines changed

7 files changed

+162
-25
lines changed

‎docs/changelog/126843.yaml

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,7 @@ breaking:
4242
`com.amazonaws.sdk.ec2MetadataServiceEndpointOverride` system property.
4343
4444
* AWS SDK v2 does not permit specifying a choice between HTTP and HTTPS so
45-
the `s3.client.${CLIENT_NAME}.protocol` setting is deprecated and no longer
46-
has any effect.
45+
the `s3.client.${CLIENT_NAME}.protocol` setting is deprecated.
4746
4847
* AWS SDK v2 does not permit control over throttling for retries, so the
4948
the `s3.client.${CLIENT_NAME}.use_throttle_retries` setting is deprecated
@@ -81,9 +80,9 @@ breaking:
8180
* If applicable, discontinue use of the
8281
`com.amazonaws.sdk.ec2MetadataServiceEndpointOverride` system property.
8382
84-
* If applicable, specify that you wish to use the insecure HTTP protocol to
85-
access the S3 API by setting `s3.client.${CLIENT_NAME}.endpoint` to a URL
86-
which starts with `http://`.
83+
* If applicable, specify the protocol to use to access the S3 API by
84+
setting `s3.client.${CLIENT_NAME}.endpoint` to a URL which starts with
85+
`http://` or `https://`.
8786
8887
* If applicable, discontinue use of the `log-delivery-write` canned ACL.
8988

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

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
10+
package org.elasticsearch.repositories.s3;
11+
12+
import fixture.aws.DynamicRegionSupplier;
13+
import fixture.s3.S3HttpFixture;
14+
15+
import com.carrotsearch.randomizedtesting.annotations.ThreadLeakFilters;
16+
import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
17+
18+
import org.elasticsearch.test.cluster.ElasticsearchCluster;
19+
import org.elasticsearch.test.fixtures.testcontainers.TestContainersThreadFilter;
20+
import org.junit.ClassRule;
21+
import org.junit.rules.RuleChain;
22+
import org.junit.rules.TestRule;
23+
24+
import java.util.function.Supplier;
25+
26+
import static fixture.aws.AwsCredentialsUtils.fixedAccessKey;
27+
import static org.hamcrest.Matchers.startsWith;
28+
29+
@ThreadLeakFilters(filters = { TestContainersThreadFilter.class })
30+
@ThreadLeakScope(ThreadLeakScope.Scope.NONE) // https://github.com/elastic/elasticsearch/issues/102482
31+
public class RepositoryS3ExplicitProtocolRestIT extends AbstractRepositoryS3RestTestCase {
32+
33+
private static final String PREFIX = getIdentifierPrefix("RepositoryS3ExplicitProtocolRestIT");
34+
private static final String BUCKET = PREFIX + "bucket";
35+
private static final String BASE_PATH = PREFIX + "base_path";
36+
private static final String ACCESS_KEY = PREFIX + "access-key";
37+
private static final String SECRET_KEY = PREFIX + "secret-key";
38+
private static final String CLIENT = "explicit_protocol_client";
39+
40+
private static final Supplier<String> regionSupplier = new DynamicRegionSupplier();
41+
private static final S3HttpFixture s3Fixture = new S3HttpFixture(
42+
true,
43+
BUCKET,
44+
BASE_PATH,
45+
fixedAccessKey(ACCESS_KEY, regionSupplier, "s3")
46+
);
47+
48+
private static String getEndpoint() {
49+
final var s3FixtureAddress = s3Fixture.getAddress();
50+
assertThat(s3FixtureAddress, startsWith("http://"));
51+
return s3FixtureAddress.substring("http://".length());
52+
}
53+
54+
public static ElasticsearchCluster cluster = ElasticsearchCluster.local()
55+
.module("repository-s3")
56+
.systemProperty("aws.region", regionSupplier)
57+
.keystore("s3.client." + CLIENT + ".access_key", ACCESS_KEY)
58+
.keystore("s3.client." + CLIENT + ".secret_key", SECRET_KEY)
59+
.setting("s3.client." + CLIENT + ".endpoint", RepositoryS3ExplicitProtocolRestIT::getEndpoint)
60+
.setting("s3.client." + CLIENT + ".protocol", () -> "http")
61+
.build();
62+
63+
@ClassRule
64+
public static TestRule ruleChain = RuleChain.outerRule(s3Fixture).around(cluster);
65+
66+
@Override
67+
protected String getTestRestCluster() {
68+
return cluster.getHttpAddresses();
69+
}
70+
71+
@Override
72+
protected String getBucketName() {
73+
return BUCKET;
74+
}
75+
76+
@Override
77+
protected String getBasePath() {
78+
return BASE_PATH;
79+
}
80+
81+
@Override
82+
protected String getClientName() {
83+
return CLIENT;
84+
}
85+
}

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

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,10 @@ final class S3ClientSettings {
7777
key -> new Setting<>(key, "", s -> s.toLowerCase(Locale.ROOT), Property.NodeScope)
7878
);
7979

80-
/** Formerly the protocol to use to connect to s3, now unused. V2 AWS SDK can infer the protocol from {@link #endpoint}. */
80+
/** 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
81+
* {@code https://}. */
8182
@UpdateForV10(owner = UpdateForV10.Owner.DISTRIBUTED_COORDINATION) // no longer used, should be removed in v10
82-
static final Setting.AffixSetting<HttpScheme> UNUSED_PROTOCOL_SETTING = Setting.affixKeySetting(
83+
static final Setting.AffixSetting<HttpScheme> PROTOCOL_SETTING = Setting.affixKeySetting(
8384
PREFIX,
8485
"protocol",
8586
key -> new Setting<>(key, "https", s -> HttpScheme.valueOf(s.toUpperCase(Locale.ROOT)), Property.NodeScope, Property.Deprecated)
@@ -181,6 +182,9 @@ final class S3ClientSettings {
181182
/** Credentials to authenticate with s3. */
182183
final AwsCredentials credentials;
183184

185+
/** The scheme (HTTP or HTTPS) for talking to the endpoint, for use only if the endpoint doesn't contain an explicit scheme */
186+
final HttpScheme protocol;
187+
184188
/** The s3 endpoint the client should talk to, or empty string to use the default. */
185189
final String endpoint;
186190

@@ -221,6 +225,7 @@ final class S3ClientSettings {
221225

222226
private S3ClientSettings(
223227
AwsCredentials credentials,
228+
HttpScheme protocol,
224229
String endpoint,
225230
String proxyHost,
226231
int proxyPort,
@@ -235,6 +240,7 @@ private S3ClientSettings(
235240
String region
236241
) {
237242
this.credentials = credentials;
243+
this.protocol = protocol;
238244
this.endpoint = endpoint;
239245
this.proxyHost = proxyHost;
240246
this.proxyPort = proxyPort;
@@ -261,6 +267,7 @@ S3ClientSettings refine(Settings repositorySettings) {
261267
.put(repositorySettings)
262268
.normalizePrefix(PREFIX + PLACEHOLDER_CLIENT + '.')
263269
.build();
270+
final HttpScheme newProtocol = getRepoSettingOrDefault(PROTOCOL_SETTING, normalizedSettings, protocol);
264271
final String newEndpoint = getRepoSettingOrDefault(ENDPOINT_SETTING, normalizedSettings, endpoint);
265272

266273
final String newProxyHost = getRepoSettingOrDefault(PROXY_HOST_SETTING, normalizedSettings, proxyHost);
@@ -284,7 +291,8 @@ S3ClientSettings refine(Settings repositorySettings) {
284291
newCredentials = credentials;
285292
}
286293
final String newRegion = getRepoSettingOrDefault(REGION, normalizedSettings, region);
287-
if (Objects.equals(endpoint, newEndpoint)
294+
if (Objects.equals(protocol, newProtocol)
295+
&& Objects.equals(endpoint, newEndpoint)
288296
&& Objects.equals(proxyHost, newProxyHost)
289297
&& proxyPort == newProxyPort
290298
&& proxyScheme == newProxyScheme
@@ -299,6 +307,7 @@ S3ClientSettings refine(Settings repositorySettings) {
299307
}
300308
return new S3ClientSettings(
301309
newCredentials,
310+
newProtocol,
302311
newEndpoint,
303312
newProxyHost,
304313
newProxyPort,
@@ -405,6 +414,7 @@ static S3ClientSettings getClientSettings(final Settings settings, final String
405414
) {
406415
return new S3ClientSettings(
407416
S3ClientSettings.loadCredentials(settings, clientName),
417+
getConfigValue(settings, clientName, PROTOCOL_SETTING),
408418
getConfigValue(settings, clientName, ENDPOINT_SETTING),
409419
getConfigValue(settings, clientName, PROXY_HOST_SETTING),
410420
getConfigValue(settings, clientName, PROXY_PORT_SETTING),
@@ -435,6 +445,7 @@ public boolean equals(final Object o) {
435445
&& maxConnections == that.maxConnections
436446
&& maxRetries == that.maxRetries
437447
&& Objects.equals(credentials, that.credentials)
448+
&& Objects.equals(protocol, that.protocol)
438449
&& Objects.equals(endpoint, that.endpoint)
439450
&& Objects.equals(proxyHost, that.proxyHost)
440451
&& proxyScheme == that.proxyScheme
@@ -448,6 +459,7 @@ public boolean equals(final Object o) {
448459
public int hashCode() {
449460
return Objects.hash(
450461
credentials,
462+
protocol,
451463
endpoint,
452464
proxyHost,
453465
proxyPort,

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ public List<Setting<?>> getSettings() {
139139
S3ClientSettings.SECRET_KEY_SETTING,
140140
S3ClientSettings.SESSION_TOKEN_SETTING,
141141
S3ClientSettings.ENDPOINT_SETTING,
142-
S3ClientSettings.UNUSED_PROTOCOL_SETTING,
142+
S3ClientSettings.PROTOCOL_SETTING,
143143
S3ClientSettings.PROXY_HOST_SETTING,
144144
S3ClientSettings.PROXY_PORT_SETTING,
145145
S3ClientSettings.PROXY_SCHEME_SETTING,

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -307,14 +307,18 @@ protected S3ClientBuilder buildClientBuilder(S3ClientSettings clientSettings, Sd
307307
String endpoint = clientSettings.endpoint;
308308
if ((endpoint.startsWith("http://") || endpoint.startsWith("https://")) == false) {
309309
// The SDK does not know how to interpret endpoints without a scheme prefix and will error. Therefore, when the scheme is
310-
// absent, we'll supply HTTPS as a default to avoid errors.
310+
// absent, we'll look at the deprecated .protocol setting
311311
// See https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/client-configuration.html#client-config-other-diffs
312-
endpoint = "https://" + endpoint;
312+
endpoint = switch (clientSettings.protocol) {
313+
case HTTP -> "http://" + endpoint;
314+
case HTTPS -> "https://" + endpoint;
315+
};
313316
LOGGER.warn(
314317
"""
315-
found S3 client with endpoint [{}] that is missing a scheme, guessing it should use 'https://'; \
318+
found S3 client with endpoint [{}] that is missing a scheme, guessing it should be [{}]; \
316319
to suppress this warning, add a scheme prefix to the [{}] setting on this node""",
317320
clientSettings.endpoint,
321+
endpoint,
318322
S3ClientSettings.ENDPOINT_SETTING.getConcreteSettingForNamespace("CLIENT_NAME").getKey()
319323
);
320324
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ public void testThereIsADefaultClientByDefault() {
3737

3838
final S3ClientSettings defaultSettings = settings.get("default");
3939
assertThat(defaultSettings.credentials, nullValue());
40+
assertThat(defaultSettings.protocol, is(HttpScheme.HTTPS));
4041
assertThat(defaultSettings.endpoint, is(emptyString()));
4142
assertThat(defaultSettings.proxyHost, is(emptyString()));
4243
assertThat(defaultSettings.proxyPort, is(80));

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

Lines changed: 49 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
import software.amazon.awssdk.core.retry.conditions.RetryCondition;
1515
import software.amazon.awssdk.http.SdkHttpClient;
1616
import software.amazon.awssdk.regions.Region;
17-
import software.amazon.awssdk.services.s3.S3Client;
1817
import software.amazon.awssdk.services.s3.endpoints.S3EndpointParams;
1918
import software.amazon.awssdk.services.s3.endpoints.internal.DefaultS3EndpointProvider;
2019
import software.amazon.awssdk.services.s3.model.S3Exception;
@@ -278,20 +277,57 @@ public void testGetClientRegionFallbackToUsEast1() {
278277
}
279278

280279
public void testEndpointOverrideSchemeDefaultsToHttpsWhenNotSpecified() {
281-
final S3Service s3Service = new S3Service(
282-
mock(Environment.class),
283-
Settings.EMPTY,
284-
mock(ResourceWatcherService.class),
285-
() -> Region.of("es-test-region")
280+
final var endpointWithoutScheme = randomIdentifier() + ".ignore";
281+
final var clientName = randomIdentifier();
282+
assertThat(
283+
getEndpointUri(Settings.builder().put("s3.client." + clientName + ".endpoint", endpointWithoutScheme), clientName),
284+
equalTo(URI.create("https://" + endpointWithoutScheme))
285+
);
286+
}
287+
288+
public void testEndpointOverrideSchemeUsesHttpsIfHttpsProtocolSpecified() {
289+
final var endpointWithoutScheme = randomIdentifier() + ".ignore";
290+
final var clientName = randomIdentifier();
291+
assertThat(
292+
getEndpointUri(
293+
Settings.builder()
294+
.put("s3.client." + clientName + ".endpoint", endpointWithoutScheme)
295+
.put("s3.client." + clientName + ".protocol", "https"),
296+
clientName
297+
),
298+
equalTo(URI.create("https://" + endpointWithoutScheme))
286299
);
287-
final String endpointWithoutScheme = randomIdentifier() + ".ignore";
288-
S3Client s3Client = s3Service.buildClient(
289-
S3ClientSettings.getClientSettings(
290-
Settings.builder().put("s3.client.test-client.endpoint", endpointWithoutScheme).build(),
291-
"test-client"
300+
assertWarnings(Strings.format("""
301+
[s3.client.%s.protocol] setting was deprecated in Elasticsearch and will be removed in a future release. \
302+
See the breaking changes documentation for the next major version.""", clientName));
303+
}
304+
305+
public void testEndpointOverrideSchemeUsesHttpIfHttpProtocolSpecified() {
306+
final var endpointWithoutScheme = randomIdentifier() + ".ignore";
307+
final var clientName = randomIdentifier();
308+
assertThat(
309+
getEndpointUri(
310+
Settings.builder()
311+
.put("s3.client." + clientName + ".endpoint", endpointWithoutScheme)
312+
.put("s3.client." + clientName + ".protocol", "http"),
313+
clientName
292314
),
293-
mock(SdkHttpClient.class)
315+
equalTo(URI.create("http://" + endpointWithoutScheme))
294316
);
295-
assertThat(s3Client.serviceClientConfiguration().endpointOverride().get(), equalTo(URI.create("https://" + endpointWithoutScheme)));
317+
assertWarnings(Strings.format("""
318+
[s3.client.%s.protocol] setting was deprecated in Elasticsearch and will be removed in a future release. \
319+
See the breaking changes documentation for the next major version.""", clientName));
320+
}
321+
322+
private static URI getEndpointUri(Settings.Builder settings, String clientName) {
323+
return new S3Service(
324+
mock(Environment.class),
325+
Settings.EMPTY,
326+
mock(ResourceWatcherService.class),
327+
() -> Region.of(randomIdentifier())
328+
).buildClient(S3ClientSettings.getClientSettings(settings.build(), clientName), mock(SdkHttpClient.class))
329+
.serviceClientConfiguration()
330+
.endpointOverride()
331+
.get();
296332
}
297333
}

0 commit comments

Comments
 (0)