[repository-s3] remove endpointOverride and let AWS SDK V2 S3 determine the s3 url based on bucket name or arn provided #20345
Conversation
…ne the s3 url based on bucket name or arn provided Signed-off-by: Craig Perkins <cwperx@amazon.com>
📝 WalkthroughWalkthroughThis PR refactors S3 endpoint override handling in the repository-s3 plugin to conditionally apply explicit endpoint configurations while allowing AWS SDK V2 to dynamically determine S3 URLs based on bucket names or ARNs when no override is specified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
|
❌ Gradle check result for cdcb47e: 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? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20345 +/- ##
============================================
- Coverage 73.26% 73.22% -0.04%
- Complexity 71715 71722 +7
============================================
Files 5785 5785
Lines 328140 328142 +2
Branches 47270 47269 -1
============================================
- Hits 240399 240297 -102
- Misses 68459 68563 +104
Partials 19282 19282 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java (1)
259-263: Consider validating for unexpected URI schemes.The current logic prepends the protocol only when http:// or https:// is not present. If a user accidentally provides an endpoint with a different scheme (e.g., "s3://..."), it would result in "https://s3://..." which is invalid.
While S3 endpoints should always be HTTP(S) URLs, consider adding validation to reject endpoints with non-HTTP schemes upfront for clearer error messages.
🔎 Proposed enhancement
String endpoint = clientSettings.endpoint; +// Check for unexpected schemes before processing +if (endpoint.contains("://") && !endpoint.startsWith("http://") && !endpoint.startsWith("https://")) { + throw new IllegalArgumentException("Invalid endpoint scheme. S3 endpoints must use http:// or https://, but got: " + endpoint); +} if ((endpoint.startsWith("http://") || endpoint.startsWith("https://")) == false) { // Manually add the schema to the endpoint to work around https://github.com/aws/aws-sdk-java/issues/2274 // TODO: Remove this once fixed in the AWS SDK endpoint = clientSettings.protocol.toString() + "://" + endpoint; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdplugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.javaplugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ArnEndpointResolutionTests.javaplugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ServiceTests.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T18:40:08.452Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Applied to files:
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ArnEndpointResolutionTests.javaplugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ServiceTests.java
🔇 Additional comments (13)
CHANGELOG.md (1)
28-28: LGTM!The changelog entry accurately documents the change and follows the correct format.
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java (2)
98-98: LGTM!The Optional import is necessary for the new endpoint resolution method.
225-228: LGTM!The conditional endpoint override application correctly preserves dynamic SDK resolution for ARN-based buckets while still supporting explicit endpoint configuration for S3-compatible services.
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ServiceTests.java (4)
39-41: LGTM!The imports are necessary for the new test methods.
89-101: LGTM!The test correctly verifies that no endpoint override is returned when the endpoint setting is absent.
103-119: LGTM!The test correctly verifies scheme addition. The comment appropriately documents the assumption about the default protocol.
121-130: LGTM!The test correctly verifies that explicit schemes are preserved, which is essential for S3-compatible services like MinIO.
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ArnEndpointResolutionTests.java (6)
29-40: LGTM!The class documentation clearly explains the no-network testing strategy, and the dummy credentials are appropriate for endpoint resolution tests.
44-71: LGTM!The CapturingInterceptor correctly extracts the endpoint URI and signing service name from the AWS Signature V4 Authorization header. The parsing logic accurately targets the service name in the credential scope.
73-104: LGTM!The helper methods effectively implement the no-network testing strategy. The failing HTTP client ensures tests verify endpoint resolution logic without making actual network calls.
108-140: LGTM!The test correctly verifies that the AWS SDK resolves Outposts ARN buckets to s3-outposts endpoints and uses the correct signing service. The comment on line 126 appropriately emphasizes not setting endpointOverride to ensure testing SDK behavior.
142-167: LGTM!The test correctly verifies AWS SDK resolution for Access Point ARNs and validates that access points use the standard "s3" signing service (unlike Outposts which use "s3-outposts").
169-195: LGTM!The test correctly verifies that regular bucket names resolve to standard S3 endpoints. The defensive assertions (checking what the endpoint is NOT) make the test resilient to SDK version differences and regional variations.
| Optional<URI> resolveEndpointOverride(final S3ClientSettings clientSettings) { | ||
| if (Strings.hasLength(clientSettings.endpoint) == false) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| String endpoint = clientSettings.endpoint; | ||
| if ((endpoint.startsWith("http://") || endpoint.startsWith("https://")) == false) { | ||
| // Manually add the schema to the endpoint to work around https://github.com/aws/aws-sdk-java/issues/2274 | ||
| // TODO: Remove this once fixed in the AWS SDK | ||
| endpoint = clientSettings.protocol.toString() + "://" + endpoint; | ||
| } | ||
|
|
||
| // Use URI.create for simplicity; if your codebase prefers checked handling, swap to new URI(endpoint). | ||
| return Optional.of(URI.create(endpoint)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AWS SDK Java issue #2274 endpoint scheme
💡 Result:
I can't find an AWS SDK for Java issue numbered 2274. Did you mean the related aws-sdk-java-v2 bug about endpoint scheme handling (issue #5831)? That issue describes S3ClientBuilder throwing a URI/scheme error when an endpoint without "http(s)://" is supplied; the SDK docs require endpointOverride to be a URI with scheme. See the GitHub issue and the SDK docs: [1], [2].
Sources:
- GitHub issue: Null Check for Endpoint Scheme does not work in AWS SDK 2.x S3ClientBuilder ([Segment Replication] Adding check to avoid parallel replication events taking place #5831). [1]
- AWS SDK for Java v2 endpointOverride documentation. [2]
🌐 Web query:
AWS SDK Java S3 endpoint override scheme requirement v2
💡 Result:
Yes — in AWS SDK for Java v2 the endpointOverride must be a java.net.URI that includes a scheme (e.g., "https://" or "http://"). If you pass a host-only string (no scheme) the S3 client builder will fail with "The URI scheme of endpointOverride must not be null." Use URI.create("https://your-endpoint") when calling endpointOverride.
Minimal example:
S3Client s3 = S3Client.builder()
.region(Region.US_WEST_2)
.endpointOverride(URI.create("https://my-custom-s3-endpoint.example.com"))
.build();
Sources: AWS SDK docs and community reports. [1][2][3]
Sources
[1] AWS SDK for Java v2 — Configuring client endpoints (endpointOverride example).
[2] GitHub issue discussing scheme requirement/behavior in aws-sdk-java-v2.
[3] StackOverflow: "The URI scheme of endpointOverride must not be null" (example and resolution).
Update the TODO comment with the correct AWS SDK issue reference.
The workaround is still necessary—AWS SDK v2 requires endpointOverride to be a URI with a scheme. However, the referenced issue #2274 appears to be incorrect or outdated. Update the comment to reference AWS SDK Java v2 issue #5831 (or the equivalent issue in your SDK version) that describes the scheme requirement for endpointOverride.
🤖 Prompt for AI Agents
In
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java
around lines 253 to 267, the TODO references the wrong AWS SDK issue (#2274);
update that comment to reference the correct AWS SDK for Java v2 issue (e.g.
#5831) that documents the requirement for endpointOverride to include a scheme,
and optionally include the issue URL or SDK version to make the rationale clear;
do not change code behavior—only replace the issue number/text in the TODO
comment to point to the correct issue.
1 participant
Description
This PR removes the hardcoding of endpointOverride in the repository-s3 plugin and delegating this to the AWS SDK based on the bucket provided (arn or not).
Here are some examples of creating a snapshot repository:
Standard Bucket (No ARN, name of the bucket + region)
Access Point ARN:
S3 Outposts access point ARN as bucket
S3 compatible endpoint (contains endpoint override)
Related Issues
Attempt to resolve:
Check List
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.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.