Skip to content

Commit b0f4ae8

Browse files
DaveCTurnerDaveCTurner
authored
Make S3 custom query parameter optional (#128180)
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 #128043 to `8.19` [^1]: https://docs.aws.amazon.com/AmazonS3/latest/userguide/LogFormat.html#LogFormatCustom
1 parent ace066d commit b0f4ae8

File tree

9 files changed

+273
-16
lines changed

9 files changed

+273
-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
@@ -165,6 +165,7 @@ protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) {
165165
final Settings.Builder builder = Settings.builder()
166166
.put(ThreadPool.ESTIMATED_TIME_INTERVAL_SETTING.getKey(), 0) // We have tests that verify an exact wait time
167167
.put(S3ClientSettings.ENDPOINT_SETTING.getConcreteSettingForNamespace("test").getKey(), httpServerUrl())
168+
.put(S3ClientSettings.ADD_PURPOSE_CUSTOM_QUERY_PARAMETER.getConcreteSettingForNamespace("test").getKey(), "true")
168169
.put(super.nodeSettings(nodeOrdinal, otherSettings))
169170
.setSecureSettings(secureSettings);
170171

‎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
@@ -1027,9 +1027,7 @@ ActionListener<Void> getMultipartUploadCleanupListener(int maxUploads, RefCounti
10271027
.prefix(keyPath)
10281028
.maxUploads(maxUploads)
10291029
// TODO adjust to use S3BlobStore.configureRequestForMetrics, adding metrics collection
1030-
.overrideConfiguration(
1031-
b -> b.putRawQueryParameter(S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE, OperationPurpose.SNAPSHOT_DATA.getKey())
1032-
)
1030+
.overrideConfiguration(b -> blobStore.addPurposeQueryParameter(OperationPurpose.SNAPSHOT_DATA, b))
10331031
.build();
10341032
final var multipartUploadListing = SocketAccess.doPrivileged(
10351033
() -> clientReference.client().listMultipartUploads(listMultipartUploadsRequest)
@@ -1060,12 +1058,7 @@ ActionListener<Void> getMultipartUploadCleanupListener(int maxUploads, RefCounti
10601058
.key(u.key())
10611059
.uploadId(u.uploadId())
10621060
// TODO adjust to use S3BlobStore.configureRequestForMetrics, adding metrics collection
1063-
.overrideConfiguration(
1064-
b -> b.putRawQueryParameter(
1065-
S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE,
1066-
OperationPurpose.SNAPSHOT_DATA.getKey()
1067-
)
1068-
)
1061+
.overrideConfiguration(b -> blobStore.addPurposeQueryParameter(OperationPurpose.SNAPSHOT_DATA, b))
10691062
.build()
10701063
)
10711064
);

‎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.http.HttpMetric;
@@ -92,6 +93,8 @@ class S3BlobStore implements BlobStore {
9293

9394
private final int bulkDeletionBatchSize;
9495

96+
private final boolean addPurposeCustomQueryParameter;
97+
9598
S3BlobStore(
9699
S3Service service,
97100
String bucket,
@@ -116,6 +119,7 @@ class S3BlobStore implements BlobStore {
116119
this.snapshotExecutor = threadPool.executor(ThreadPool.Names.SNAPSHOT);
117120
this.s3RepositoriesMetrics = s3RepositoriesMetrics;
118121
this.bulkDeletionBatchSize = S3Repository.DELETION_BATCH_SIZE_SETTING.get(repositoryMetadata.settings());
122+
this.addPurposeCustomQueryParameter = service.settings(repositoryMetadata).addPurposeCustomQueryParameter;
119123
}
120124

121125
MetricPublisher getMetricPublisher(Operation operation, OperationPurpose purpose) {
@@ -520,9 +524,17 @@ static void configureRequestForMetrics(
520524
Operation operation,
521525
OperationPurpose purpose
522526
) {
523-
request.overrideConfiguration(
524-
builder -> builder.metricPublishers(List.of(blobStore.getMetricPublisher(operation, purpose)))
525-
.putRawQueryParameter(CUSTOM_QUERY_PARAMETER_PURPOSE, purpose.getKey())
526-
);
527+
request.overrideConfiguration(builder -> {
528+
builder.metricPublishers(List.of(blobStore.getMetricPublisher(operation, purpose)));
529+
blobStore.addPurposeQueryParameter(purpose, builder);
530+
});
527531
}
532+
533+
public void addPurposeQueryParameter(OperationPurpose purpose, AwsRequestOverrideConfiguration.Builder builder) {
534+
if (addPurposeCustomQueryParameter || purpose == OperationPurpose.REPOSITORY_ANALYSIS) {
535+
// REPOSITORY_ANALYSIS is a strict check for 100% S3 compatibility, including custom query parameter support, so is always added
536+
builder.putRawQueryParameter(CUSTOM_QUERY_PARAMETER_PURPOSE, purpose.getKey());
537+
}
538+
}
539+
528540
}

‎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
@@ -182,6 +182,13 @@ final class S3ClientSettings {
182182
key -> Setting.simpleString(key, Property.NodeScope, Property.DeprecatedWarning)
183183
);
184184

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

@@ -223,6 +230,9 @@ final class S3ClientSettings {
223230
/** Whether chunked encoding should be disabled or not. */
224231
final boolean disableChunkedEncoding;
225232

233+
/** Whether to add the {@code x-purpose} custom query parameter to all requests. */
234+
final boolean addPurposeCustomQueryParameter;
235+
226236
/** Region to use for signing requests or empty string to use default. */
227237
final String region;
228238

@@ -240,6 +250,7 @@ private S3ClientSettings(
240250
int maxRetries,
241251
boolean pathStyleAccess,
242252
boolean disableChunkedEncoding,
253+
boolean addPurposeCustomQueryParameter,
243254
String region
244255
) {
245256
this.credentials = credentials;
@@ -255,6 +266,7 @@ private S3ClientSettings(
255266
this.maxRetries = maxRetries;
256267
this.pathStyleAccess = pathStyleAccess;
257268
this.disableChunkedEncoding = disableChunkedEncoding;
269+
this.addPurposeCustomQueryParameter = addPurposeCustomQueryParameter;
258270
this.region = region;
259271
}
260272

@@ -287,6 +299,11 @@ S3ClientSettings refine(Settings repositorySettings) {
287299
normalizedSettings,
288300
disableChunkedEncoding
289301
);
302+
final boolean newAddPurposeCustomQueryParameter = getRepoSettingOrDefault(
303+
ADD_PURPOSE_CUSTOM_QUERY_PARAMETER,
304+
normalizedSettings,
305+
addPurposeCustomQueryParameter
306+
);
290307
final AwsCredentials newCredentials;
291308
if (checkDeprecatedCredentials(repositorySettings)) {
292309
newCredentials = loadDeprecatedCredentials(repositorySettings);
@@ -305,6 +322,7 @@ S3ClientSettings refine(Settings repositorySettings) {
305322
&& Objects.equals(credentials, newCredentials)
306323
&& newPathStyleAccess == pathStyleAccess
307324
&& newDisableChunkedEncoding == disableChunkedEncoding
325+
&& newAddPurposeCustomQueryParameter == addPurposeCustomQueryParameter
308326
&& Objects.equals(region, newRegion)) {
309327
return this;
310328
}
@@ -322,6 +340,7 @@ S3ClientSettings refine(Settings repositorySettings) {
322340
newMaxRetries,
323341
newPathStyleAccess,
324342
newDisableChunkedEncoding,
343+
newAddPurposeCustomQueryParameter,
325344
newRegion
326345
);
327346
}
@@ -429,6 +448,7 @@ static S3ClientSettings getClientSettings(final Settings settings, final String
429448
getConfigValue(settings, clientName, MAX_RETRIES_SETTING),
430449
getConfigValue(settings, clientName, USE_PATH_STYLE_ACCESS),
431450
getConfigValue(settings, clientName, DISABLE_CHUNKED_ENCODING),
451+
getConfigValue(settings, clientName, ADD_PURPOSE_CUSTOM_QUERY_PARAMETER),
432452
getConfigValue(settings, clientName, REGION)
433453
);
434454
}
@@ -455,6 +475,7 @@ public boolean equals(final Object o) {
455475
&& Objects.equals(proxyUsername, that.proxyUsername)
456476
&& Objects.equals(proxyPassword, that.proxyPassword)
457477
&& Objects.equals(disableChunkedEncoding, that.disableChunkedEncoding)
478+
&& Objects.equals(addPurposeCustomQueryParameter, that.addPurposeCustomQueryParameter)
458479
&& Objects.equals(region, that.region);
459480
}
460481

@@ -473,6 +494,7 @@ public int hashCode() {
473494
maxRetries,
474495
maxConnections,
475496
disableChunkedEncoding,
497+
addPurposeCustomQueryParameter,
476498
region
477499
);
478500
}

‎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
@@ -143,6 +143,7 @@ public List<Setting<?>> getSettings() {
143143
S3ClientSettings.UNUSED_USE_THROTTLE_RETRIES_SETTING,
144144
S3ClientSettings.USE_PATH_STYLE_ACCESS,
145145
S3ClientSettings.UNUSED_SIGNER_OVERRIDE,
146+
S3ClientSettings.ADD_PURPOSE_CUSTOM_QUERY_PARAMETER,
146147
S3ClientSettings.REGION,
147148
S3Service.REPOSITORY_S3_CAS_TTL_SETTING,
148149
S3Service.REPOSITORY_S3_CAS_ANTI_CONTENTION_DELAY_SETTING,

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

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
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+
@Override
50+
protected Settings nodeSettings() {
51+
final var secureSettings = new MockSecureSettings();
52+
for (final var clientName : List.of("default", "with_purpose", "without_purpose")) {
53+
secureSettings.setString(
54+
S3ClientSettings.ACCESS_KEY_SETTING.getConcreteSettingForNamespace(clientName).getKey(),
55+
randomIdentifier()
56+
);
57+
secureSettings.setString(
58+
S3ClientSettings.SECRET_KEY_SETTING.getConcreteSettingForNamespace(clientName).getKey(),
59+
randomSecretKey()
60+
);
61+
}
62+
63+
return Settings.builder()
64+
.put(super.nodeSettings())
65+
.put(S3ClientSettings.REGION.getConcreteSettingForNamespace("default").getKey(), randomIdentifier())
66+
.put(S3ClientSettings.ADD_PURPOSE_CUSTOM_QUERY_PARAMETER.getConcreteSettingForNamespace("with_purpose").getKey(), "true")
67+
.put(S3ClientSettings.ADD_PURPOSE_CUSTOM_QUERY_PARAMETER.getConcreteSettingForNamespace("without_purpose").getKey(), "false")
68+
.setSecureSettings(secureSettings)
69+
.build();
70+
}
71+
72+
private static final Matcher<Iterable<? super String>> HAS_CUSTOM_QUERY_PARAMETER = hasItem(S3BlobStore.CUSTOM_QUERY_PARAMETER_PURPOSE);
73+
private static final Matcher<Iterable<? super String>> NO_CUSTOM_QUERY_PARAMETER = not(HAS_CUSTOM_QUERY_PARAMETER);
74+
75+
public void testCustomQueryParameterConfiguration() throws Throwable {
76+
final var indexName = randomIdentifier();
77+
createIndex(indexName);
78+
prepareIndex(indexName).setSource("foo", "bar").get();
79+
80+
final var bucket = randomIdentifier();
81+
final var basePath = randomIdentifier();
82+
83+
runCustomQueryParameterTest(bucket, basePath, null, Settings.EMPTY, NO_CUSTOM_QUERY_PARAMETER);
84+
runCustomQueryParameterTest(bucket, basePath, "default", Settings.EMPTY, NO_CUSTOM_QUERY_PARAMETER);
85+
runCustomQueryParameterTest(bucket, basePath, "without_purpose", Settings.EMPTY, NO_CUSTOM_QUERY_PARAMETER);
86+
runCustomQueryParameterTest(bucket, basePath, "with_purpose", Settings.EMPTY, HAS_CUSTOM_QUERY_PARAMETER);
87+
88+
final var falseRepositorySetting = Settings.builder().put("add_purpose_custom_query_parameter", false).build();
89+
final var trueRepositorySetting = Settings.builder().put("add_purpose_custom_query_parameter", true).build();
90+
for (final var clientName : new String[] { null, "default", "with_purpose", "without_purpose" }) {
91+
// client name doesn't matter if repository setting specified
92+
runCustomQueryParameterTest(bucket, basePath, clientName, falseRepositorySetting, NO_CUSTOM_QUERY_PARAMETER);
93+
runCustomQueryParameterTest(bucket, basePath, clientName, trueRepositorySetting, HAS_CUSTOM_QUERY_PARAMETER);
94+
}
95+
}
96+
97+
private void runCustomQueryParameterTest(
98+
String bucket,
99+
String basePath,
100+
String clientName,
101+
Settings extraRepositorySettings,
102+
Matcher<Iterable<? super String>> queryParamMatcher
103+
) throws Throwable {
104+
final var httpFixture = new S3HttpFixture(true, bucket, basePath, (key, token) -> true) {
105+
106+
@SuppressForbidden(reason = "this test uses a HttpServer to emulate an S3 endpoint")
107+
class AssertingHandler extends S3HttpHandler {
108+
AssertingHandler() {
109+
super(bucket, basePath);
110+
}
111+
112+
@SuppressForbidden(reason = "this test uses a HttpServer to emulate an S3 endpoint")
113+
@Override
114+
public void handle(HttpExchange exchange) throws IOException {
115+
try {
116+
assertThat(parseRequest(exchange).queryParameters().keySet(), queryParamMatcher);
117+
super.handle(exchange);
118+
} catch (Error e) {
119+
// HttpServer catches Throwable, so we must throw errors on another thread
120+
ExceptionsHelper.maybeDieOnAnotherThread(e);
121+
throw e;
122+
}
123+
}
124+
}
125+
126+
@SuppressForbidden(reason = "this test uses a HttpServer to emulate an S3 endpoint")
127+
@Override
128+
protected HttpHandler createHandler() {
129+
return new AssertingHandler();
130+
}
131+
};
132+
httpFixture.apply(new Statement() {
133+
@Override
134+
public void evaluate() {
135+
final var repoName = randomIdentifier();
136+
assertAcked(
137+
client().execute(
138+
TransportPutRepositoryAction.TYPE,
139+
new PutRepositoryRequest(TEST_REQUEST_TIMEOUT, TEST_REQUEST_TIMEOUT, repoName).type(S3Repository.TYPE)
140+
.settings(
141+
Settings.builder()
142+
.put("bucket", bucket)
143+
.put("base_path", basePath)
144+
.put("endpoint", httpFixture.getAddress())
145+
.put(clientName == null ? Settings.EMPTY : Settings.builder().put("client", clientName).build())
146+
.put(extraRepositorySettings)
147+
)
148+
)
149+
);
150+
151+
assertEquals(
152+
SnapshotState.SUCCESS,
153+
client().execute(
154+
TransportCreateSnapshotAction.TYPE,
155+
new CreateSnapshotRequest(TEST_REQUEST_TIMEOUT, repoName, randomIdentifier()).waitForCompletion(true)
156+
).actionGet(SAFE_AWAIT_TIMEOUT).getSnapshotInfo().state()
157+
);
158+
}
159+
}, Description.EMPTY).evaluate();
160+
}
161+
162+
}

0 commit comments

Comments
 (0)