Skip to content

Commit f6651d6

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

File tree

9 files changed

+279
-17
lines changed

9 files changed

+279
-17
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
@@ -173,6 +173,7 @@ protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) {
173173
.put(S3ClientSettings.ENDPOINT_SETTING.getConcreteSettingForNamespace("test").getKey(), httpServerUrl())
174174
// Disable request throttling because some random values in tests might generate too many failures for the S3 client
175175
.put(S3ClientSettings.USE_THROTTLE_RETRIES_SETTING.getConcreteSettingForNamespace("test").getKey(), false)
176+
.put(S3ClientSettings.ADD_PURPOSE_CUSTOM_QUERY_PARAMETER.getConcreteSettingForNamespace("test").getKey(), "true")
176177
.put(super.nodeSettings(nodeOrdinal, otherSettings))
177178
.setSecureSettings(secureSettings);
178179

@@ -516,19 +517,13 @@ public void testMultipartUploadCleanup() {
516517
blobStore.bucket(),
517518
blobStore.blobContainer(repository.basePath().add("test-multipart-upload")).path().buildAsString() + danglingBlobName
518519
);
519-
initiateMultipartUploadRequest.putCustomQueryParameter(
520-
S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE,
521-
OperationPurpose.SNAPSHOT_DATA.getKey()
522-
);
520+
blobStore.addPurposeQueryParameter(OperationPurpose.SNAPSHOT_DATA, initiateMultipartUploadRequest);
523521
final var multipartUploadResult = clientRef.client().initiateMultipartUpload(initiateMultipartUploadRequest);
524522

525523
final var listMultipartUploadsRequest = new ListMultipartUploadsRequest(blobStore.bucket()).withPrefix(
526524
repository.basePath().buildAsString()
527525
);
528-
listMultipartUploadsRequest.putCustomQueryParameter(
529-
S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE,
530-
OperationPurpose.SNAPSHOT_DATA.getKey()
531-
);
526+
blobStore.addPurposeQueryParameter(OperationPurpose.SNAPSHOT_DATA, listMultipartUploadsRequest);
532527
assertEquals(
533528
List.of(multipartUploadResult.getUploadId()),
534529
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
@@ -966,7 +966,7 @@ ActionListener<Void> getMultipartUploadCleanupListener(int maxUploads, RefCounti
966966
try (var clientReference = blobStore.clientReference()) {
967967
final var bucket = blobStore.bucket();
968968
final var request = new ListMultipartUploadsRequest(bucket).withPrefix(keyPath).withMaxUploads(maxUploads);
969-
request.putCustomQueryParameter(S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE, OperationPurpose.SNAPSHOT_DATA.getKey());
969+
blobStore.addPurposeQueryParameter(OperationPurpose.SNAPSHOT_DATA, request);
970970
final var multipartUploadListing = SocketAccess.doPrivileged(() -> clientReference.client().listMultipartUploads(request));
971971
final var multipartUploads = multipartUploadListing.getMultipartUploads();
972972
if (multipartUploads.isEmpty()) {
@@ -1006,10 +1006,7 @@ private ActionListener<Void> newMultipartUploadCleanupListener(
10061006
public void onResponse(Void unused) {
10071007
try (var clientReference = blobStore.clientReference()) {
10081008
for (final var abortMultipartUploadRequest : abortMultipartUploadRequests) {
1009-
abortMultipartUploadRequest.putCustomQueryParameter(
1010-
S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE,
1011-
OperationPurpose.SNAPSHOT_DATA.getKey()
1012-
);
1009+
blobStore.addPurposeQueryParameter(OperationPurpose.SNAPSHOT_DATA, abortMultipartUploadRequest);
10131010
try {
10141011
SocketAccess.doPrivilegedVoid(() -> clientReference.client().abortMultipartUpload(abortMultipartUploadRequest));
10151012
logger.info(

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,8 @@ class S3BlobStore implements BlobStore {
9898

9999
private final TimeValue getRegisterRetryDelay;
100100

101+
private final boolean addPurposeCustomQueryParameter;
102+
101103
S3BlobStore(
102104
S3Service service,
103105
String bucket,
@@ -125,6 +127,7 @@ class S3BlobStore implements BlobStore {
125127
this.bulkDeletionBatchSize = S3Repository.DELETION_BATCH_SIZE_SETTING.get(repositoryMetadata.settings());
126128
this.retryThrottledDeleteBackoffPolicy = retryThrottledDeleteBackoffPolicy;
127129
this.getRegisterRetryDelay = S3Repository.GET_REGISTER_RETRY_DELAY.get(repositoryMetadata.settings());
130+
this.addPurposeCustomQueryParameter = service.settings(repositoryMetadata).addPurposeCustomQueryParameter;
128131
}
129132

130133
RequestMetricCollector getMetricCollector(Operation operation, OperationPurpose purpose) {
@@ -594,6 +597,14 @@ static void configureRequestForMetrics(
594597
OperationPurpose purpose
595598
) {
596599
request.setRequestMetricCollector(blobStore.getMetricCollector(operation, purpose));
597-
request.putCustomQueryParameter(CUSTOM_QUERY_PARAMETER_PURPOSE, purpose.getKey());
600+
blobStore.addPurposeQueryParameter(purpose, request);
598601
}
602+
603+
public void addPurposeQueryParameter(OperationPurpose purpose, AmazonWebServiceRequest request) {
604+
if (addPurposeCustomQueryParameter || purpose == OperationPurpose.REPOSITORY_ANALYSIS) {
605+
// REPOSITORY_ANALYSIS is a strict check for 100% S3 compatibility, including custom query parameter support, so is always added
606+
request.putCustomQueryParameter(CUSTOM_QUERY_PARAMETER_PURPOSE, purpose.getKey());
607+
}
608+
}
609+
599610
}

‎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
@@ -173,6 +173,13 @@ final class S3ClientSettings {
173173
key -> Setting.simpleString(key, Property.NodeScope)
174174
);
175175

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

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

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

@@ -238,6 +248,7 @@ private S3ClientSettings(
238248
boolean throttleRetries,
239249
boolean pathStyleAccess,
240250
boolean disableChunkedEncoding,
251+
boolean addPurposeCustomQueryParameter,
241252
String region,
242253
String signerOverride
243254
) {
@@ -255,6 +266,7 @@ private S3ClientSettings(
255266
this.throttleRetries = throttleRetries;
256267
this.pathStyleAccess = pathStyleAccess;
257268
this.disableChunkedEncoding = disableChunkedEncoding;
269+
this.addPurposeCustomQueryParameter = addPurposeCustomQueryParameter;
258270
this.region = region;
259271
this.signerOverride = signerOverride;
260272
}
@@ -289,6 +301,11 @@ S3ClientSettings refine(Settings repositorySettings) {
289301
normalizedSettings,
290302
disableChunkedEncoding
291303
);
304+
final boolean newAddPurposeCustomQueryParameter = getRepoSettingOrDefault(
305+
ADD_PURPOSE_CUSTOM_QUERY_PARAMETER,
306+
normalizedSettings,
307+
addPurposeCustomQueryParameter
308+
);
292309
final S3BasicCredentials newCredentials;
293310
if (checkDeprecatedCredentials(repositorySettings)) {
294311
newCredentials = loadDeprecatedCredentials(repositorySettings);
@@ -309,6 +326,7 @@ S3ClientSettings refine(Settings repositorySettings) {
309326
&& Objects.equals(credentials, newCredentials)
310327
&& newPathStyleAccess == pathStyleAccess
311328
&& newDisableChunkedEncoding == disableChunkedEncoding
329+
&& newAddPurposeCustomQueryParameter == addPurposeCustomQueryParameter
312330
&& Objects.equals(region, newRegion)
313331
&& Objects.equals(signerOverride, newSignerOverride)) {
314332
return this;
@@ -328,6 +346,7 @@ S3ClientSettings refine(Settings repositorySettings) {
328346
newThrottleRetries,
329347
newPathStyleAccess,
330348
newDisableChunkedEncoding,
349+
newAddPurposeCustomQueryParameter,
331350
newRegion,
332351
newSignerOverride
333352
);
@@ -437,6 +456,7 @@ static S3ClientSettings getClientSettings(final Settings settings, final String
437456
getConfigValue(settings, clientName, USE_THROTTLE_RETRIES_SETTING),
438457
getConfigValue(settings, clientName, USE_PATH_STYLE_ACCESS),
439458
getConfigValue(settings, clientName, DISABLE_CHUNKED_ENCODING),
459+
getConfigValue(settings, clientName, ADD_PURPOSE_CUSTOM_QUERY_PARAMETER),
440460
getConfigValue(settings, clientName, REGION),
441461
getConfigValue(settings, clientName, SIGNER_OVERRIDE)
442462
);
@@ -465,6 +485,7 @@ public boolean equals(final Object o) {
465485
&& Objects.equals(proxyUsername, that.proxyUsername)
466486
&& Objects.equals(proxyPassword, that.proxyPassword)
467487
&& Objects.equals(disableChunkedEncoding, that.disableChunkedEncoding)
488+
&& Objects.equals(addPurposeCustomQueryParameter, that.addPurposeCustomQueryParameter)
468489
&& Objects.equals(region, that.region)
469490
&& Objects.equals(signerOverride, that.signerOverride);
470491
}
@@ -485,6 +506,7 @@ public int hashCode() {
485506
maxConnections,
486507
throttleRetries,
487508
disableChunkedEncoding,
509+
addPurposeCustomQueryParameter,
488510
region,
489511
signerOverride
490512
);

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