Skip to content

Commit b26a277

Browse files
DaveCTurnerDaveCTurner
authored
Make S3 custom query parameter optional (elastic#128185)
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.18` [^1]: https://docs.aws.amazon.com/AmazonS3/latest/userguide/LogFormat.html#LogFormatCustom
1 parent 6b052e7 commit b26a277

File tree

9 files changed

+279
-18
lines changed

9 files changed

+279
-18
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/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 & 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) {
@@ -523,6 +525,14 @@ static void configureRequestForMetrics(
523525
OperationPurpose purpose
524526
) {
525527
request.setRequestMetricCollector(blobStore.getMetricCollector(operation, purpose));
526-
request.putCustomQueryParameter(CUSTOM_QUERY_PARAMETER_PURPOSE, purpose.getKey());
528+
blobStore.addPurposeQueryParameter(purpose, request);
529+
}
530+
531+
public void addPurposeQueryParameter(OperationPurpose purpose, AmazonWebServiceRequest request) {
532+
if (addPurposeCustomQueryParameter || purpose == OperationPurpose.REPOSITORY_ANALYSIS) {
533+
// REPOSITORY_ANALYSIS is a strict check for 100% S3 compatibility, including custom query parameter support, so is always added
534+
request.putCustomQueryParameter(CUSTOM_QUERY_PARAMETER_PURPOSE, purpose.getKey());
535+
}
527536
}
537+
528538
}

‎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,

0 commit comments

Comments
 (0)