Skip to content

Commit 6814d98

Browse files
DaveCTurnerDaveCTurner
committed
Make S3 custom query parameter optional
Today Elasticsearch will record the purpose for each request to S3 using a custom query parameter. This isn't believed to be necessary outside of the ECH/ECE/ECK/... managed services, and it adds rather a lot to the request logs, so with this commit we make the feature optional and disabled by default.
1 parent 7e22bb5 commit 6814d98

File tree

9 files changed

+266
-16
lines changed

9 files changed

+266
-16
lines changed

‎docs/changelog/128043.yaml

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
pr: 128043
2+
summary: Make S3 custom query parameter optional
3+
area: Snapshot/Restore
4+
type: breaking
5+
issues: []
6+
breaking:
7+
title: Make S3 custom query parameter optional
8+
area: Cluster and node setting
9+
details: >-
10+
Earlier versions of Elasticsearch would record the purpose of each S3 API
11+
call using the `?x-purpose=` custom query parameter. This isn't believed to
12+
be necessary outside of the ECH/ECE/ECK/... managed services, and it adds
13+
rather a lot to the request logs, so with this change we make the feature
14+
optional and disabled by default.
15+
impact: >-
16+
If you wish to reinstate the old behaviour on a S3 repository, set
17+
`s3.client.${CLIENT_NAME}.add_purpose_custom_query_parameter` to `true`
18+
for the relevant client.
19+
notable: false

‎modules/repository-s3/src/internalClusterTest/java/org/elasticsearch/repositories/s3/S3BlobStoreRepositoryTests.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@ protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) {
167167
final Settings.Builder builder = Settings.builder()
168168
.put(ThreadPool.ESTIMATED_TIME_INTERVAL_SETTING.getKey(), 0) // We have tests that verify an exact wait time
169169
.put(S3ClientSettings.ENDPOINT_SETTING.getConcreteSettingForNamespace("test").getKey(), httpServerUrl())
170+
.put(S3ClientSettings.ADD_PURPOSE_CUSTOM_QUERY_PARAMETER.getConcreteSettingForNamespace("test").getKey(), "true")
170171
.put(super.nodeSettings(nodeOrdinal, otherSettings))
171172
.setSecureSettings(secureSettings);
172173

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,12 @@ private Request getRegisterRequest(UnaryOperator<Settings> settingsUnaryOperator
5959
.put("canned_acl", "private")
6060
.put("storage_class", "standard")
6161
.put("disable_chunked_encoding", randomBoolean())
62+
.put(
63+
randomFrom(
64+
Settings.EMPTY,
65+
Settings.builder().put("add_purpose_custom_query_parameter", randomBoolean()).build()
66+
)
67+
)
6268
.build()
6369
)
6470
)
@@ -183,8 +189,10 @@ private void testNonexistentClient(Boolean readonly) throws Exception {
183189
final var responseObjectPath = ObjectPath.createFromResponse(responseException.getResponse());
184190
assertThat(responseObjectPath.evaluate("error.type"), equalTo("repository_verification_exception"));
185191
assertThat(responseObjectPath.evaluate("error.reason"), containsString("is not accessible on master node"));
186-
assertThat(responseObjectPath.evaluate("error.caused_by.type"), equalTo("illegal_argument_exception"));
187-
assertThat(responseObjectPath.evaluate("error.caused_by.reason"), containsString("Unknown s3 client name"));
192+
assertThat(responseObjectPath.evaluate("error.caused_by.type"), equalTo("repository_exception"));
193+
assertThat(responseObjectPath.evaluate("error.caused_by.reason"), containsString("cannot create blob store"));
194+
assertThat(responseObjectPath.evaluate("error.caused_by.caused_by.type"), equalTo("illegal_argument_exception"));
195+
assertThat(responseObjectPath.evaluate("error.caused_by.caused_by.reason"), containsString("Unknown s3 client name"));
188196
}
189197

190198
public void testNonexistentSnapshot() throws Exception {

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

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1153,9 +1153,7 @@ ActionListener<Void> getMultipartUploadCleanupListener(int maxUploads, RefCounti
11531153
.prefix(keyPath)
11541154
.maxUploads(maxUploads)
11551155
// TODO adjust to use S3BlobStore.configureRequestForMetrics, adding metrics collection
1156-
.overrideConfiguration(
1157-
b -> b.putRawQueryParameter(S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE, OperationPurpose.SNAPSHOT_DATA.getKey())
1158-
)
1156+
.overrideConfiguration(b -> blobStore.addPurposeQueryParameter(OperationPurpose.SNAPSHOT_DATA, b))
11591157
.build();
11601158
final var multipartUploadListing = clientReference.client().listMultipartUploads(listMultipartUploadsRequest);
11611159
final var multipartUploads = multipartUploadListing.uploads();
@@ -1184,12 +1182,7 @@ ActionListener<Void> getMultipartUploadCleanupListener(int maxUploads, RefCounti
11841182
.key(u.key())
11851183
.uploadId(u.uploadId())
11861184
// TODO adjust to use S3BlobStore.configureRequestForMetrics, adding metrics collection
1187-
.overrideConfiguration(
1188-
b -> b.putRawQueryParameter(
1189-
S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE,
1190-
OperationPurpose.SNAPSHOT_DATA.getKey()
1191-
)
1192-
)
1185+
.overrideConfiguration(b -> blobStore.addPurposeQueryParameter(OperationPurpose.SNAPSHOT_DATA, b))
11931186
.build()
11941187
)
11951188
);

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

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
package org.elasticsearch.repositories.s3;
1111

1212
import software.amazon.awssdk.awscore.AwsRequest;
13+
import software.amazon.awssdk.awscore.AwsRequestOverrideConfiguration;
1314
import software.amazon.awssdk.core.exception.SdkException;
1415
import software.amazon.awssdk.core.metrics.CoreMetric;
1516
import software.amazon.awssdk.core.retry.RetryUtils;
@@ -102,6 +103,8 @@ class S3BlobStore implements BlobStore {
102103

103104
private final TimeValue getRegisterRetryDelay;
104105

106+
private final boolean addPurposeCustomQueryParameter;
107+
105108
S3BlobStore(
106109
S3Service service,
107110
String bucket,
@@ -131,6 +134,7 @@ class S3BlobStore implements BlobStore {
131134
this.bulkDeletionBatchSize = S3Repository.DELETION_BATCH_SIZE_SETTING.get(repositoryMetadata.settings());
132135
this.retryThrottledDeleteBackoffPolicy = retryThrottledDeleteBackoffPolicy;
133136
this.getRegisterRetryDelay = S3Repository.GET_REGISTER_RETRY_DELAY.get(repositoryMetadata.settings());
137+
this.addPurposeCustomQueryParameter = service.settings(repositoryMetadata).addPurposeCustomQueryParameter;
134138
}
135139

136140
MetricPublisher getMetricPublisher(Operation operation, OperationPurpose purpose) {
@@ -600,9 +604,17 @@ static void configureRequestForMetrics(
600604
Operation operation,
601605
OperationPurpose purpose
602606
) {
603-
request.overrideConfiguration(
604-
builder -> builder.metricPublishers(List.of(blobStore.getMetricPublisher(operation, purpose)))
605-
.putRawQueryParameter(CUSTOM_QUERY_PARAMETER_PURPOSE, purpose.getKey())
606-
);
607+
request.overrideConfiguration(builder -> {
608+
builder.metricPublishers(List.of(blobStore.getMetricPublisher(operation, purpose)));
609+
blobStore.addPurposeQueryParameter(purpose, builder);
610+
});
607611
}
612+
613+
public void addPurposeQueryParameter(OperationPurpose purpose, AwsRequestOverrideConfiguration.Builder builder) {
614+
if (addPurposeCustomQueryParameter || purpose == OperationPurpose.REPOSITORY_ANALYSIS) {
615+
// REPOSITORY_ANALYSIS is a strict check for 100% S3 compatibility, including custom query parameter support, so is always added
616+
builder.putRawQueryParameter(CUSTOM_QUERY_PARAMETER_PURPOSE, purpose.getKey());
617+
}
618+
}
619+
608620
}

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,13 @@ final class S3ClientSettings {
179179
key -> Setting.simpleString(key, Property.NodeScope, Property.Deprecated)
180180
);
181181

182+
/** Whether to include the {@code x-purpose} custom query parameter in all requests. */
183+
static final Setting.AffixSetting<Boolean> ADD_PURPOSE_CUSTOM_QUERY_PARAMETER = Setting.affixKeySetting(
184+
PREFIX,
185+
"add_purpose_custom_query_parameter",
186+
key -> Setting.boolSetting(key, false, Property.NodeScope)
187+
);
188+
182189
/** Credentials to authenticate with s3. */
183190
final AwsCredentials credentials;
184191

@@ -220,6 +227,9 @@ final class S3ClientSettings {
220227
/** Whether chunked encoding should be disabled or not. */
221228
final boolean disableChunkedEncoding;
222229

230+
/** Whether to add the {@code x-purpose} custom query parameter to all requests. */
231+
final boolean addPurposeCustomQueryParameter;
232+
223233
/** Region to use for signing requests or empty string to use default. */
224234
final String region;
225235

@@ -237,6 +247,7 @@ private S3ClientSettings(
237247
int maxRetries,
238248
boolean pathStyleAccess,
239249
boolean disableChunkedEncoding,
250+
boolean addPurposeCustomQueryParameter,
240251
String region
241252
) {
242253
this.credentials = credentials;
@@ -252,6 +263,7 @@ private S3ClientSettings(
252263
this.maxRetries = maxRetries;
253264
this.pathStyleAccess = pathStyleAccess;
254265
this.disableChunkedEncoding = disableChunkedEncoding;
266+
this.addPurposeCustomQueryParameter = addPurposeCustomQueryParameter;
255267
this.region = region;
256268
}
257269

@@ -284,6 +296,11 @@ S3ClientSettings refine(Settings repositorySettings) {
284296
normalizedSettings,
285297
disableChunkedEncoding
286298
);
299+
final boolean newAddPurposeCustomQueryParameter = getRepoSettingOrDefault(
300+
ADD_PURPOSE_CUSTOM_QUERY_PARAMETER,
301+
normalizedSettings,
302+
addPurposeCustomQueryParameter
303+
);
287304
final AwsCredentials newCredentials;
288305
if (checkDeprecatedCredentials(repositorySettings)) {
289306
newCredentials = loadDeprecatedCredentials(repositorySettings);
@@ -302,6 +319,7 @@ S3ClientSettings refine(Settings repositorySettings) {
302319
&& Objects.equals(credentials, newCredentials)
303320
&& newPathStyleAccess == pathStyleAccess
304321
&& newDisableChunkedEncoding == disableChunkedEncoding
322+
&& newAddPurposeCustomQueryParameter == addPurposeCustomQueryParameter
305323
&& Objects.equals(region, newRegion)) {
306324
return this;
307325
}
@@ -319,6 +337,7 @@ S3ClientSettings refine(Settings repositorySettings) {
319337
newMaxRetries,
320338
newPathStyleAccess,
321339
newDisableChunkedEncoding,
340+
newAddPurposeCustomQueryParameter,
322341
newRegion
323342
);
324343
}
@@ -426,6 +445,7 @@ static S3ClientSettings getClientSettings(final Settings settings, final String
426445
getConfigValue(settings, clientName, MAX_RETRIES_SETTING),
427446
getConfigValue(settings, clientName, USE_PATH_STYLE_ACCESS),
428447
getConfigValue(settings, clientName, DISABLE_CHUNKED_ENCODING),
448+
getConfigValue(settings, clientName, ADD_PURPOSE_CUSTOM_QUERY_PARAMETER),
429449
getConfigValue(settings, clientName, REGION)
430450
);
431451
}
@@ -452,6 +472,7 @@ public boolean equals(final Object o) {
452472
&& Objects.equals(proxyUsername, that.proxyUsername)
453473
&& Objects.equals(proxyPassword, that.proxyPassword)
454474
&& Objects.equals(disableChunkedEncoding, that.disableChunkedEncoding)
475+
&& Objects.equals(addPurposeCustomQueryParameter, that.addPurposeCustomQueryParameter)
455476
&& Objects.equals(region, that.region);
456477
}
457478

@@ -470,6 +491,7 @@ public int hashCode() {
470491
maxRetries,
471492
maxConnections,
472493
disableChunkedEncoding,
494+
addPurposeCustomQueryParameter,
473495
region
474496
);
475497
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ public List<Setting<?>> getSettings() {
123123
S3ClientSettings.UNUSED_USE_THROTTLE_RETRIES_SETTING,
124124
S3ClientSettings.USE_PATH_STYLE_ACCESS,
125125
S3ClientSettings.UNUSED_SIGNER_OVERRIDE,
126+
S3ClientSettings.ADD_PURPOSE_CUSTOM_QUERY_PARAMETER,
126127
S3ClientSettings.REGION,
127128
S3Service.REPOSITORY_S3_CAS_TTL_SETTING,
128129
S3Service.REPOSITORY_S3_CAS_ANTI_CONTENTION_DELAY_SETTING,

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

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
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.s3.S3HttpFixture;
13+
import fixture.s3.S3HttpHandler;
14+
15+
import com.sun.net.httpserver.HttpExchange;
16+
import com.sun.net.httpserver.HttpHandler;
17+
18+
import org.elasticsearch.action.admin.cluster.repositories.put.PutRepositoryRequest;
19+
import org.elasticsearch.action.admin.cluster.repositories.put.TransportPutRepositoryAction;
20+
import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotRequest;
21+
import org.elasticsearch.action.admin.cluster.snapshots.create.TransportCreateSnapshotAction;
22+
import org.elasticsearch.common.settings.MockSecureSettings;
23+
import org.elasticsearch.common.settings.Settings;
24+
import org.elasticsearch.common.util.CollectionUtils;
25+
import org.elasticsearch.core.SuppressForbidden;
26+
import org.elasticsearch.plugins.Plugin;
27+
import org.elasticsearch.snapshots.SnapshotState;
28+
import org.elasticsearch.test.ESSingleNodeTestCase;
29+
import org.hamcrest.Matcher;
30+
import org.junit.runner.Description;
31+
import org.junit.runners.model.Statement;
32+
33+
import java.io.IOException;
34+
import java.util.Collection;
35+
import java.util.List;
36+
37+
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
38+
import static org.hamcrest.Matchers.hasItem;
39+
import static org.hamcrest.Matchers.not;
40+
41+
public class AddPurposeCustomQueryParameterTests extends ESSingleNodeTestCase {
42+
43+
@Override
44+
protected Collection<Class<? extends Plugin>> getPlugins() {
45+
return CollectionUtils.appendToCopyNoNullElements(super.getPlugins(), S3RepositoryPlugin.class);
46+
}
47+
48+
@Override
49+
protected Settings nodeSettings() {
50+
final var secureSettings = new MockSecureSettings();
51+
for (final var clientName : List.of("default", "with_purpose", "without_purpose")) {
52+
secureSettings.setString(
53+
S3ClientSettings.ACCESS_KEY_SETTING.getConcreteSettingForNamespace(clientName).getKey(),
54+
randomIdentifier()
55+
);
56+
secureSettings.setString(
57+
S3ClientSettings.SECRET_KEY_SETTING.getConcreteSettingForNamespace(clientName).getKey(),
58+
randomSecretKey()
59+
);
60+
}
61+
62+
return Settings.builder()
63+
.put(super.nodeSettings())
64+
.put(S3ClientSettings.REGION.getConcreteSettingForNamespace("default").getKey(), randomIdentifier())
65+
.put(S3ClientSettings.ADD_PURPOSE_CUSTOM_QUERY_PARAMETER.getConcreteSettingForNamespace("with_purpose").getKey(), "true")
66+
.put(S3ClientSettings.ADD_PURPOSE_CUSTOM_QUERY_PARAMETER.getConcreteSettingForNamespace("without_purpose").getKey(), "false")
67+
.setSecureSettings(secureSettings)
68+
.build();
69+
}
70+
71+
private static final Matcher<Iterable<? super String>> HAS_CUSTOM_QUERY_PARAMETER = hasItem(S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE);
72+
private static final Matcher<Iterable<? super String>> NO_CUSTOM_QUERY_PARAMETER = not(HAS_CUSTOM_QUERY_PARAMETER);
73+
74+
public void testCustomQueryParameterConfiguration() throws Throwable {
75+
final var indexName = randomIdentifier();
76+
createIndex(indexName);
77+
prepareIndex(indexName).setSource("foo", "bar").get();
78+
79+
final var bucket = randomIdentifier();
80+
final var basePath = randomIdentifier();
81+
82+
runCustomQueryParameterTest(bucket, basePath, null, Settings.EMPTY, NO_CUSTOM_QUERY_PARAMETER);
83+
runCustomQueryParameterTest(bucket, basePath, "default", Settings.EMPTY, NO_CUSTOM_QUERY_PARAMETER);
84+
runCustomQueryParameterTest(bucket, basePath, "without_purpose", Settings.EMPTY, NO_CUSTOM_QUERY_PARAMETER);
85+
runCustomQueryParameterTest(bucket, basePath, "with_purpose", Settings.EMPTY, HAS_CUSTOM_QUERY_PARAMETER);
86+
87+
final var falseRepositorySetting = Settings.builder().put("add_purpose_custom_query_parameter", false).build();
88+
final var trueRepositorySetting = Settings.builder().put("add_purpose_custom_query_parameter", true).build();
89+
for (final var clientName : new String[] { null, "default", "with_purpose", "without_purpose" }) {
90+
// client name doesn't matter if repository setting specified
91+
runCustomQueryParameterTest(bucket, basePath, clientName, falseRepositorySetting, NO_CUSTOM_QUERY_PARAMETER);
92+
runCustomQueryParameterTest(bucket, basePath, clientName, trueRepositorySetting, HAS_CUSTOM_QUERY_PARAMETER);
93+
}
94+
}
95+
96+
private void runCustomQueryParameterTest(
97+
String bucket,
98+
String basePath,
99+
String clientName,
100+
Settings extraRepositorySettings,
101+
Matcher<Iterable<? super String>> queryParamMatcher
102+
) throws Throwable {
103+
final var httpFixture = new S3HttpFixture(true, bucket, basePath, (key, token) -> true) {
104+
105+
@SuppressForbidden(reason = "this test uses a HttpServer to emulate an S3 endpoint")
106+
class AssertingHandler extends S3HttpHandler {
107+
AssertingHandler() {
108+
super(bucket, basePath);
109+
}
110+
111+
@SuppressForbidden(reason = "this test uses a HttpServer to emulate an S3 endpoint")
112+
@Override
113+
public void handle(HttpExchange exchange) throws IOException {
114+
assertThat(parseRequest(exchange).queryParameters().keySet(), queryParamMatcher);
115+
super.handle(exchange);
116+
}
117+
}
118+
119+
@SuppressForbidden(reason = "this test uses a HttpServer to emulate an S3 endpoint")
120+
@Override
121+
protected HttpHandler createHandler() {
122+
return new AssertingHandler();
123+
}
124+
};
125+
httpFixture.apply(new Statement() {
126+
@Override
127+
public void evaluate() {
128+
final var repoName = randomIdentifier();
129+
assertAcked(
130+
client().execute(
131+
TransportPutRepositoryAction.TYPE,
132+
new PutRepositoryRequest(TEST_REQUEST_TIMEOUT, TEST_REQUEST_TIMEOUT, repoName).type(S3Repository.TYPE)
133+
.settings(
134+
Settings.builder()
135+
.put("bucket", bucket)
136+
.put("base_path", basePath)
137+
.put("endpoint", httpFixture.getAddress())
138+
.put(clientName == null ? Settings.EMPTY : Settings.builder().put("client", clientName).build())
139+
.put(extraRepositorySettings)
140+
)
141+
)
142+
);
143+
144+
assertEquals(
145+
SnapshotState.SUCCESS,
146+
client().execute(
147+
TransportCreateSnapshotAction.TYPE,
148+
new CreateSnapshotRequest(TEST_REQUEST_TIMEOUT, repoName, randomIdentifier()).waitForCompletion(true)
149+
).actionGet(SAFE_AWAIT_TIMEOUT).getSnapshotInfo().state()
150+
);
151+
}
152+
}, Description.EMPTY).evaluate();
153+
}
154+
155+
}

0 commit comments

Comments
 (0)