Skip to content

Conversation

@cwperks
Copy link
Member

@cwperks cwperks commented Sep 2, 2025

Description

This PR fixes an issue where the repository-s3 plugin is not working in OpenSearch 3.x with s3-compatible repositories. The reason its not working is due to the mandatory inclusion of checksum trailing headers

See also: #19124

The gist of this fix is in S3Service and S3AsyncService to build the S3Client with the following settings:

final S3ClientBuilder builder = S3Client.builder()
    .requestChecksumCalculation(RequestChecksumCalculation.WHEN_REQUIRED)
    .responseChecksumValidation(ResponseChecksumValidation.WHEN_REQUIRED)
    .addPlugin(LegacyMd5Plugin.create());

This will add the trailing checksum headers when required by the repository and optionally leave them out if the repository does not require these headers.

Related Issues

Resolves #18240

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Craig Perkins <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2025

❌ Gradle check result for 6d099aa: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2025

❌ Gradle check result for a4beb3b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2025

✅ Gradle check result for d693b10: SUCCESS

@codecov
Copy link

codecov bot commented Sep 2, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.28%. Comparing base (6c4f3d8) to head (eab0f71).
⚠️ Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
...org/opensearch/repositories/s3/S3AsyncService.java 50.00% 1 Missing and 1 partial ⚠️
...g/opensearch/repositories/s3/S3ClientSettings.java 80.00% 0 Missing and 2 partials ⚠️
...java/org/opensearch/repositories/s3/S3Service.java 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #19220      +/-   ##
============================================
+ Coverage     72.88%   73.28%   +0.39%     
- Complexity    69841    70210     +369     
============================================
  Files          5673     5673              
  Lines        320756   320774      +18     
  Branches      46370    46371       +1     
============================================
+ Hits         233796   235087    +1291     
+ Misses        68102    66986    -1116     
+ Partials      18858    18701     -157     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@willyborankin
Copy link
Contributor

willyborankin commented Sep 17, 2025

@cwperks, thank you for the fix. The fix works just fine for Oracle Storage and some others not so popular storages.
WDYT to introduce a new boolean settings for the S3 client: s3.client.<client>.legacy_md5_checksum_calculation
and add LegacyMd5Plugin on demand both for sync and async clients?

@cwperks
Copy link
Member Author

cwperks commented Sep 18, 2025

@cwperks, thank you for the fix. The fix works just fine for Oracle Storage and some others not so popular storages. WDYT to introduce a new boolean settings for the S3 client: s3.client.<client>.legacy_md5_checksum_calculation and add LegacyMd5Plugin on demand both for sync and async clients?

I think that makes sense. I pushed a change for that and added tests as well. Provided the gradle check passes I will take this out of draft. Thank you for confirming the patch works.

Signed-off-by: Craig Perkins <[email protected]>
This reverts commit 6023298.

Signed-off-by: Craig Perkins <[email protected]>
@github-actions
Copy link
Contributor

❌ Gradle check result for eab0f71: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Contributor

❕ Gradle check result for eab0f71: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link
Member

@mch2 mch2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lgtm, thanks for the fix @cwperks

@github-actions
Copy link
Contributor

✅ Gradle check result for eab0f71: SUCCESS

@cwperks cwperks merged commit 488149f into opensearch-project:main Sep 23, 2025
42 of 44 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Storage Project Board Sep 23, 2025
vinaykpud pushed a commit to vinaykpud/OpenSearch that referenced this pull request Sep 26, 2025
…iling headers (opensearch-project#19220)

* Update CHANGELOG to use correct comparison link of 3.2..main

Signed-off-by: Craig Perkins <[email protected]>

* Fix repository-s3 plugin to work with s3-compatible blob stores

Signed-off-by: Craig Perkins <[email protected]>

* Apply to S3AsyncService as well

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG

Signed-off-by: Craig Perkins <[email protected]>

* Remove LegacyMd5Plugin from S3AsyncService

Signed-off-by: Craig Perkins <[email protected]>

* Fix tests related to sdk upgrade

Signed-off-by: Craig Perkins <[email protected]>

* Address review comments

Signed-off-by: Craig Perkins <[email protected]>

* Update sha files

Signed-off-by: Craig Perkins <[email protected]>

* Revert "Update sha files"

This reverts commit 6023298.

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Plugins Storage:Remote Storage:Snapshots Storage Issues and PRs relating to data and metadata storage v3.3.0

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

[BUG] Snapshot repositories don't work with S3 compatible storage in 3.0.0

3 participants