Skip to content

Commit 940b441

Browse files
DaveCTurnerDaveCTurner
authored
Make S3 custom query parameter optional (elastic#128189)
Today Elasticsearch will record the purpose for each request to S3 using a custom query parameter[^1]. 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. Backport of elastic#128043 to `8.17` [^1]: https://docs.aws.amazon.com/AmazonS3/latest/userguide/LogFormat.html#LogFormatCustom
1 parent 1e25d82 commit 940b441

File tree

8 files changed

+271
-16
lines changed

8 files changed

+271
-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: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@ protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) {
172172
.put(S3ClientSettings.ENDPOINT_SETTING.getConcreteSettingForNamespace("test").getKey(), httpServerUrl())
173173
// Disable request throttling because some random values in tests might generate too many failures for the S3 client
174174
.put(S3ClientSettings.USE_THROTTLE_RETRIES_SETTING.getConcreteSettingForNamespace("test").getKey(), false)
175+
.put(S3ClientSettings.ADD_PURPOSE_CUSTOM_QUERY_PARAMETER.getConcreteSettingForNamespace("test").getKey(), "true")
175176
.put(super.nodeSettings(nodeOrdinal, otherSettings))
176177
.setSecureSettings(secureSettings);
177178

@@ -495,19 +496,13 @@ public void testMultipartUploadCleanup() {
495496
blobStore.bucket(),
496497
blobStore.blobContainer(repository.basePath().add("test-multipart-upload")).path().buildAsString() + danglingBlobName
497498
);
498-
initiateMultipartUploadRequest.putCustomQueryParameter(
499-
S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE,
500-
OperationPurpose.SNAPSHOT_DATA.getKey()
501-
);
499+
blobStore.addPurposeQueryParameter(OperationPurpose.SNAPSHOT_DATA, initiateMultipartUploadRequest);
502500
final var multipartUploadResult = clientRef.client().initiateMultipartUpload(initiateMultipartUploadRequest);
503501

504502
final var listMultipartUploadsRequest = new ListMultipartUploadsRequest(blobStore.bucket()).withPrefix(
505503
repository.basePath().buildAsString()
506504
);
507-
listMultipartUploadsRequest.putCustomQueryParameter(
508-
S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE,
509-
OperationPurpose.SNAPSHOT_DATA.getKey()
510-
);
505+
blobStore.addPurposeQueryParameter(OperationPurpose.SNAPSHOT_DATA, listMultipartUploadsRequest);
511506
assertEquals(
512507
List.of(multipartUploadResult.getUploadId()),
513508
clientRef.client()

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

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -928,7 +928,7 @@ ActionListener<Void> getMultipartUploadCleanupListener(int maxUploads, RefCounti
928928
try (var clientReference = blobStore.clientReference()) {
929929
final var bucket = blobStore.bucket();
930930
final var request = new ListMultipartUploadsRequest(bucket).withPrefix(keyPath).withMaxUploads(maxUploads);
931-
request.putCustomQueryParameter(S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE, OperationPurpose.SNAPSHOT_DATA.getKey());
931+
blobStore.addPurposeQueryParameter(OperationPurpose.SNAPSHOT_DATA, request);
932932
final var multipartUploadListing = SocketAccess.doPrivileged(() -> clientReference.client().listMultipartUploads(request));
933933
final var multipartUploads = multipartUploadListing.getMultipartUploads();
934934
if (multipartUploads.isEmpty()) {
@@ -968,10 +968,7 @@ private ActionListener<Void> newMultipartUploadCleanupListener(
968968
public void onResponse(Void unused) {
969969
try (var clientReference = blobStore.clientReference()) {
970970
for (final var abortMultipartUploadRequest : abortMultipartUploadRequests) {
971-
abortMultipartUploadRequest.putCustomQueryParameter(
972-
S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE,
973-
OperationPurpose.SNAPSHOT_DATA.getKey()
974-
);
971+
blobStore.addPurposeQueryParameter(OperationPurpose.SNAPSHOT_DATA, abortMultipartUploadRequest);
975972
try {
976973
SocketAccess.doPrivilegedVoid(() -> clientReference.client().abortMultipartUpload(abortMultipartUploadRequest));
977974
logger.info(

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,8 @@ class S3BlobStore implements BlobStore {
9191

9292
private final int bulkDeletionBatchSize;
9393

94+
private final boolean addPurposeCustomQueryParameter;
95+
9496
S3BlobStore(
9597
S3Service service,
9698
String bucket,
@@ -115,7 +117,7 @@ class S3BlobStore implements BlobStore {
115117
this.snapshotExecutor = threadPool.executor(ThreadPool.Names.SNAPSHOT);
116118
this.s3RepositoriesMetrics = s3RepositoriesMetrics;
117119
this.bulkDeletionBatchSize = S3Repository.DELETION_BATCH_SIZE_SETTING.get(repositoryMetadata.settings());
118-
120+
this.addPurposeCustomQueryParameter = service.settings(repositoryMetadata).addPurposeCustomQueryParameter;
119121
}
120122

121123
RequestMetricCollector getMetricCollector(Operation operation, OperationPurpose purpose) {
@@ -524,6 +526,14 @@ static void configureRequestForMetrics(
524526
OperationPurpose purpose
525527
) {
526528
request.setRequestMetricCollector(blobStore.getMetricCollector(operation, purpose));
527-
request.putCustomQueryParameter(CUSTOM_QUERY_PARAMETER_PURPOSE, purpose.getKey());
529+
blobStore.addPurposeQueryParameter(purpose, request);
530+
}
531+
532+
public void addPurposeQueryParameter(OperationPurpose purpose, AmazonWebServiceRequest request) {
533+
if (addPurposeCustomQueryParameter || purpose == OperationPurpose.REPOSITORY_ANALYSIS) {
534+
// REPOSITORY_ANALYSIS is a strict check for 100% S3 compatibility, including custom query parameter support, so is always added
535+
request.putCustomQueryParameter(CUSTOM_QUERY_PARAMETER_PURPOSE, purpose.getKey());
536+
}
528537
}
538+
529539
}

‎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
@@ -174,6 +174,13 @@ final class S3ClientSettings {
174174
key -> new Setting<>(key, "", Function.identity(), Property.NodeScope)
175175
);
176176

177+
/** Whether to include the {@code x-purpose} custom query parameter in all requests. */
178+
static final Setting.AffixSetting<Boolean> ADD_PURPOSE_CUSTOM_QUERY_PARAMETER = Setting.affixKeySetting(
179+
PREFIX,
180+
"add_purpose_custom_query_parameter",
181+
key -> Setting.boolSetting(key, false, Property.NodeScope)
182+
);
183+
177184
/** Credentials to authenticate with s3. */
178185
final S3BasicCredentials credentials;
179186

@@ -218,6 +225,9 @@ final class S3ClientSettings {
218225
/** Whether chunked encoding should be disabled or not. */
219226
final boolean disableChunkedEncoding;
220227

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

@@ -239,6 +249,7 @@ private S3ClientSettings(
239249
boolean throttleRetries,
240250
boolean pathStyleAccess,
241251
boolean disableChunkedEncoding,
252+
boolean addPurposeCustomQueryParameter,
242253
String region,
243254
String signerOverride
244255
) {
@@ -256,6 +267,7 @@ private S3ClientSettings(
256267
this.throttleRetries = throttleRetries;
257268
this.pathStyleAccess = pathStyleAccess;
258269
this.disableChunkedEncoding = disableChunkedEncoding;
270+
this.addPurposeCustomQueryParameter = addPurposeCustomQueryParameter;
259271
this.region = region;
260272
this.signerOverride = signerOverride;
261273
}
@@ -290,6 +302,11 @@ S3ClientSettings refine(Settings repositorySettings) {
290302
normalizedSettings,
291303
disableChunkedEncoding
292304
);
305+
final boolean newAddPurposeCustomQueryParameter = getRepoSettingOrDefault(
306+
ADD_PURPOSE_CUSTOM_QUERY_PARAMETER,
307+
normalizedSettings,
308+
addPurposeCustomQueryParameter
309+
);
293310
final S3BasicCredentials newCredentials;
294311
if (checkDeprecatedCredentials(repositorySettings)) {
295312
newCredentials = loadDeprecatedCredentials(repositorySettings);
@@ -310,6 +327,7 @@ S3ClientSettings refine(Settings repositorySettings) {
310327
&& Objects.equals(credentials, newCredentials)
311328
&& newPathStyleAccess == pathStyleAccess
312329
&& newDisableChunkedEncoding == disableChunkedEncoding
330+
&& newAddPurposeCustomQueryParameter == addPurposeCustomQueryParameter
313331
&& Objects.equals(region, newRegion)
314332
&& Objects.equals(signerOverride, newSignerOverride)) {
315333
return this;
@@ -329,6 +347,7 @@ S3ClientSettings refine(Settings repositorySettings) {
329347
newThrottleRetries,
330348
newPathStyleAccess,
331349
newDisableChunkedEncoding,
350+
newAddPurposeCustomQueryParameter,
332351
newRegion,
333352
newSignerOverride
334353
);
@@ -438,6 +457,7 @@ static S3ClientSettings getClientSettings(final Settings settings, final String
438457
getConfigValue(settings, clientName, USE_THROTTLE_RETRIES_SETTING),
439458
getConfigValue(settings, clientName, USE_PATH_STYLE_ACCESS),
440459
getConfigValue(settings, clientName, DISABLE_CHUNKED_ENCODING),
460+
getConfigValue(settings, clientName, ADD_PURPOSE_CUSTOM_QUERY_PARAMETER),
441461
getConfigValue(settings, clientName, REGION),
442462
getConfigValue(settings, clientName, SIGNER_OVERRIDE)
443463
);
@@ -466,6 +486,7 @@ public boolean equals(final Object o) {
466486
&& Objects.equals(proxyUsername, that.proxyUsername)
467487
&& Objects.equals(proxyPassword, that.proxyPassword)
468488
&& Objects.equals(disableChunkedEncoding, that.disableChunkedEncoding)
489+
&& Objects.equals(addPurposeCustomQueryParameter, that.addPurposeCustomQueryParameter)
469490
&& Objects.equals(region, that.region)
470491
&& Objects.equals(signerOverride, that.signerOverride);
471492
}
@@ -486,6 +507,7 @@ public int hashCode() {
486507
maxConnections,
487508
throttleRetries,
488509
disableChunkedEncoding,
510+
addPurposeCustomQueryParameter,
489511
region,
490512
signerOverride
491513
);

‎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
@@ -131,6 +131,7 @@ public List<Setting<?>> getSettings() {
131131
S3ClientSettings.USE_THROTTLE_RETRIES_SETTING,
132132
S3ClientSettings.USE_PATH_STYLE_ACCESS,
133133
S3ClientSettings.SIGNER_OVERRIDE,
134+
S3ClientSettings.ADD_PURPOSE_CUSTOM_QUERY_PARAMETER,
134135
S3ClientSettings.REGION,
135136
S3Service.REPOSITORY_S3_CAS_TTL_SETTING,
136137
S3Service.REPOSITORY_S3_CAS_ANTI_CONTENTION_DELAY_SETTING,

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

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

0 commit comments

Comments
 (0)