[repository-s3] remove endpointOverride and let AWS SDK V2 S3 determine the s3 url based on bucket name or arn provided

[cwperks]

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)

PUT /_snapshot/s3_standard_bucket
{
  "type": "s3",
  "settings": {
    "bucket": "my-standard-bucket",
    "region": "us-east-1",
    "base_path": "opensearch/snapshots"
  }
}

Access Point ARN:

PUT /_snapshot/s3_access_point_arn
{
  "type": "s3",
  "settings": {
    "bucket": "arn:aws:s3:us-west-2:111122223333:accesspoint/my-ap",
    "region": "us-west-2",
    "base_path": "snapshots"
  }

S3 Outposts access point ARN as bucket

PUT /_snapshot/s3_outposts_arn
{
  "type": "s3",
  "settings": {
    "bucket": "arn:aws:s3-outposts:us-east-1:111122223333:outpost/op-0123456789abcdef/accesspoint/my-op-ap",
    "region": "us-east-1",
    "base_path": "snapshots"
  }
}

S3 compatible endpoint (contains endpoint override)

PUT /_snapshot/s3_minio
{
  "type": "s3",
  "settings": {
    "bucket": "minio-bucket",
    "region": "us-east-1",
    "endpoint": "http://localhost:9000",
    "protocol": "http",
    "path_style_access": true,
    "base_path": "snapshots"
  }

Related Issues

Attempt to resolve:

• [BUG] Automatic S3 endpoint routing #19900
• [Feature Request] Support for S3 Outposts buckets #19897

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.

Summary by CodeRabbit

• Bug Fixes

  • Fixed S3 repository endpoint handling to apply overrides only when explicitly configured.
  • Improved automatic endpoint resolution for S3 ARNs, including proper support for S3 Outposts and Access Points.

• Tests

  • Added test coverage for S3 endpoint resolution behavior across different bucket types and configurations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

This 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

Cohort / File(s)	Summary
Documentation CHANGELOG.md	Added entry documenting removal of unconditional endpointOverride for repository-s3, noting AWS SDK V2 S3 will determine the S3 URL based on bucket name or ARN provided.
Core Implementation plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java	Introduced resolveEndpointOverride() method that returns Optional<URI> and conditionally applies endpoint overrides only when explicitly configured; normalizes missing schemes to https://. Modified buildClient() to use the new method via ifPresent() instead of unconditional override application. Added Optional import.
Unit Tests plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ServiceTests.java	Added three test methods: testResolveEndpointOverrideAbsentWhenEndpointNotProvided(), testResolveEndpointOverrideAddsSchemeWhenMissing(), and testResolveEndpointOverridePreservesExplicitScheme() to validate endpoint override resolution behavior.
Integration/ARN Tests plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ArnEndpointResolutionTests.java	New test class validating AWS SDK V2 endpoint resolution for different bucket types using a no-network strategy; includes CapturingInterceptor to observe endpoint and signing service resolution, and three test scenarios covering Outposts ARN, Access Point ARN, and regular bucket endpoints.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 The endpoints now flow free and smart,
No forced overrides play their part!
AWS SDK picks the right domain,
From ARNs and buckets, the path is plain—
Dynamic routing, the rabbit's delight! 🌟

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: removing hardcoded endpointOverride and delegating S3 URL determination to AWS SDK V2 based on bucket name or ARN.
Description check	✅ Passed	The description includes a detailed explanation of the change, practical examples of snapshot repository configurations for different bucket types, and references related GitHub issues. The checklist is present but incomplete.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

❌ 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?

[github-actions]

✅ Gradle check result for cdcb47e: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.33%. Comparing base (68c2d14) to head (8096a37).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...org/opensearch/repositories/s3/S3AsyncService.java	85.71%	0 Missing and 1 partial ⚠️
...java/org/opensearch/repositories/s3/S3Service.java	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20345      +/-   ##
============================================
- Coverage     73.33%   73.33%   -0.01%     
+ Complexity    72004    71970      -34     
============================================
  Files          5793     5793              
  Lines        329110   329114       +4     
  Branches      47402    47400       -2     
============================================
- Hits         241364   241354      -10     
- Misses        68394    68402       +8     
- Partials      19352    19358       +6     

☔ 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.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between e9b5bd1 and cdcb47e.

📒 Files selected for processing (4)

• CHANGELOG.md
• plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java
• plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ArnEndpointResolutionTests.java
• plugins/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.java
• plugins/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.

[cwperks]

@ashking94 @reta @andrross wdyt of the changes in this PR? I see we have another similar PR open to address a similar issue: #20386

[github-actions]

❌ Gradle check result for 139d818: null

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?

[reta]

@ashking94 @reta @andrross wdyt of the changes in this PR? I see we have another similar PR open to address a similar issue: #20386

Thanks @cwperks , I think the change looks reasonable, thank you. If few minor comments, plus we may need to update the documentation for S3 plugin to highlight new resolution strategy (and document the fallback to use s3.amazonaws.com as an override in case old behavior is required for some reasons).

[github-actions]

❌ Gradle check result for 8096a37: 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]

❌ Gradle check result for 8096a37: 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]

✅ Gradle check result for 8096a37: SUCCESS