Make S3 custom query parameter optional #128043

DaveCTurner · 2025-05-12T21:38:31Z

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.

https://docs.aws.amazon.com/AmazonS3/latest/userguide/LogFormat.html#LogFormatCustom ↩

elasticsearchmachine · 2025-05-12T21:38:57Z

Hi @DaveCTurner, I've created a changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

elasticsearchmachine · 2025-05-13T04:41:42Z

Hi @DaveCTurner, I've updated the changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

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.

elasticsearchmachine · 2025-05-13T12:49:08Z

Hi @DaveCTurner, I've updated the changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

This reverts commit 81cef8c.

elasticsearchmachine · 2025-05-13T14:06:40Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

elasticsearchmachine · 2025-05-13T14:06:40Z

Hi @DaveCTurner, I've updated the changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

This reverts commit 6cca2b3.

elasticsearchmachine · 2025-05-13T14:07:35Z

Hi @DaveCTurner, I've updated the changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

This reverts commit b9da53e.

ywangd

LGTM

ywangd · 2025-05-20T02:27:54Z

...-s3/src/test/java/org/elasticsearch/repositories/s3/AddPurposeCustomQueryParameterTests.java

+            protected HttpHandler createHandler() {
+                return new AssertingHandler();
+            }


Should we make AssertingHandler delegate to the handler returned by super.createHandler() so that we don't miss the fix from #102976?

Sure see 6b91e9b. It doesn't really matter here, the test fails either way.

ywangd · 2025-05-20T02:30:24Z

...-s3/src/test/java/org/elasticsearch/repositories/s3/AddPurposeCustomQueryParameterTests.java

+                return new AssertingHandler();
+            }
+        };
+        httpFixture.apply(new Statement() {


Do we need to stop the fixture explicitly especially when the test fails?

That's handled within apply().

ywangd · 2025-05-20T02:40:56Z

...ava/org/elasticsearch/repositories/blobstore/testkit/analyze/S3RepositoryAnalysisRestIT.java

+                return;
+            }
+        }
+        repoAnalysisStarted.set(true);


I think it's more readable to move this statement to be the last statement inside the if (repoAnalysisStarted.get() == false) block.

Ok see 0e58906

ywangd · 2025-05-20T02:47:12Z

...ava/org/elasticsearch/repositories/blobstore/testkit/analyze/S3RepositoryAnalysisRestIT.java

+        if (request.path().startsWith("/bucket/base_path_integration_tests/temp-analysis-")) {
+            repoAnalysisStarted.set(true);
+        }
+        if (repoAnalysisStarted.get() == false) {


With #126593, the s3 fixture now runs with multiple threads. Is there any racing concern with checking the atomic boolean? I guess it's probably not an issue since operations before repo analysis are all sequential?

All the earlier operations relate to registering the repository and complete strictly before the start of repo analysis, regardless of concurrency in the handler.

elasticsearchmachine · 2025-05-20T05:10:39Z

Hi @DaveCTurner, I've updated the changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

This reverts commit f38515d.

elasticsearchmachine · 2025-05-20T07:18:08Z

💔 Backport failed

Status	Branch	Result
❌	8.19	Commit could not be cherrypicked due to conflicts
❌	8.17	Commit could not be cherrypicked due to conflicts
❌	8.18	Commit could not be cherrypicked due to conflicts
❌	9.0	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 128043

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. [^1]: https://docs.aws.amazon.com/AmazonS3/latest/userguide/LogFormat.html#LogFormatCustom Backport of elastic#128043 to `9.0`

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

DaveCTurner added >enhancement >breaking :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.19.0 v9.1.0 v8.17.7 v8.18.2 v9.0.2 labels May 12, 2025

elasticsearchmachine added the serverless-linked Added by automation, don't add manually label May 13, 2025

DaveCTurner force-pushed the 2025/05/12/optional-s3-custom-query-parameter branch from 3fa7c30 to 6814d98 Compare May 13, 2025 11:47

DaveCTurner added 2 commits May 13, 2025 12:57

CI poke

88c4c77

Update docs/changelog/128043.yaml

81cef8c

Revert "Update docs/changelog/128043.yaml"

3f1a8f4

This reverts commit 81cef8c.

DaveCTurner marked this pull request as ready for review May 13, 2025 14:06

DaveCTurner requested a review from ywangd May 13, 2025 14:06

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label May 13, 2025

Update docs/changelog/128043.yaml

6cca2b3

DaveCTurner added 2 commits May 13, 2025 15:06

Revert "Update docs/changelog/128043.yaml"

2512826

This reverts commit 6cca2b3.

Update docs/changelog/128043.yaml

b9da53e

Revert "Update docs/changelog/128043.yaml"

36cb13d

This reverts commit b9da53e.

ywangd approved these changes May 20, 2025

View reviewed changes

DaveCTurner added 2 commits May 20, 2025 05:59

Merge branch 'main' into 2025/05/12/optional-s3-custom-query-parameter

c148ba1

Rethrow errors properly

6b91e9b

No need to set repoAnalysisStarted again

0e58906

DaveCTurner added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) auto-backport Automatically create backport pull requests when merged labels May 20, 2025

Update docs/changelog/128043.yaml

f38515d

Revert "Update docs/changelog/128043.yaml"

9bfb6f0

This reverts commit f38515d.

elasticsearchmachine merged commit 18c6079 into elastic:main May 20, 2025
17 checks passed

DaveCTurner deleted the 2025/05/12/optional-s3-custom-query-parameter branch May 20, 2025 07:15

elasticsearchmachine added the backport pending label May 20, 2025

DaveCTurner mentioned this pull request May 20, 2025

Make S3 custom query parameter optional #128178

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make S3 custom query parameter optional #128043

Make S3 custom query parameter optional #128043

DaveCTurner commented May 12, 2025 •

edited

Loading

elasticsearchmachine commented May 12, 2025

elasticsearchmachine commented May 13, 2025

elasticsearchmachine commented May 13, 2025

elasticsearchmachine commented May 13, 2025

elasticsearchmachine commented May 13, 2025

elasticsearchmachine commented May 13, 2025

ywangd left a comment

ywangd May 20, 2025

DaveCTurner May 20, 2025

ywangd May 20, 2025

DaveCTurner May 20, 2025

ywangd May 20, 2025

DaveCTurner May 20, 2025

ywangd May 20, 2025

DaveCTurner May 20, 2025

elasticsearchmachine commented May 20, 2025

elasticsearchmachine commented May 20, 2025

Make S3 custom query parameter optional #128043

Make S3 custom query parameter optional #128043

Conversation

DaveCTurner commented May 12, 2025 • edited Loading

Footnotes

elasticsearchmachine commented May 12, 2025

elasticsearchmachine commented May 13, 2025

elasticsearchmachine commented May 13, 2025

elasticsearchmachine commented May 13, 2025

elasticsearchmachine commented May 13, 2025

elasticsearchmachine commented May 13, 2025

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented May 20, 2025

elasticsearchmachine commented May 20, 2025

💔 Backport failed

DaveCTurner commented May 12, 2025 •

edited

Loading