Remove hard‑coded S3 endpoint override to support FIPS‑compliant access #20386
Conversation
github-actions
bot
added
enhancement
📝 WalkthroughWalkthroughRemoved the hardcoded default S3 endpoint from S3Service; endpoint override is now applied only when Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Use AWS_REGION and AWS_USE_FIPS_ENDPOINT to deduce Signed-off-by: Hongyu Yan <hongyu.yan@laserfiche.com>
![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
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gradle-check
| if (Strings.hasLength(clientSettings.endpoint)) | ||
| { | ||
| 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; | ||
| } | ||
| logger.debug("using endpoint [{}] and region [{}]", endpoint, clientSettings.region); | ||
| builder.endpointOverride(URI.create(endpoint)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Verify the current code at lines 222-232 in S3Service.java
echo "=== S3Service.java lines 220-240 ==="
sed -n '220,240p' plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java
# Check for DEFAULT_S3_ENDPOINT references (should be removed/not found)
echo -e "\n=== DEFAULT_S3_ENDPOINT references ==="
rg "DEFAULT_S3_ENDPOINT" --type java
# Find S3Service test files
echo -e "\n=== S3Service test files ==="
fd "S3Service.*Test" --type f
# List all test files in repository-s3 plugin
echo -e "\n=== Test files in repository-s3 plugin ==="
find plugins/repository-s3/src/test -name "*.java" -type f | head -20Repository: opensearch-project/OpenSearch
Length of output: 3673
🌐 Web query:
AWS SDK for Java v2 FIPS endpoints support AWS_USE_FIPS_ENDPOINT environment variable
💡 Result:
Yes. The AWS SDK for Java v2 supports using FIPS endpoints via the AWS_USE_FIPS_ENDPOINT environment variable (also configurable via the shared config setting use_fips_endpoint or the JVM system property aws.useFipsEndpoint). Default is false. [1][2]
Sources:
- AWS SDKs and Tools — Dual-stack and FIPS endpoints. [1]
- AWS IoT Greengrass FIPS docs (mentions AWS_USE_FIPS_ENDPOINT / aws.useFipsEndpoint for Java clients). [2]
🏁 Script executed:
# Check S3AsyncService to understand its role
echo "=== S3AsyncService.java endpoint handling ==="
rg -B5 -A5 "DEFAULT_S3_ENDPOINT" plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3AsyncService.java
# Check class hierarchy/relationship
echo -e "\n=== S3Service class declaration ==="
head -30 plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java
echo -e "\n=== S3AsyncService class declaration ==="
head -30 plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3AsyncService.java
# Review S3ServiceTests to check endpoint test coverage
echo -e "\n=== S3ServiceTests endpoint tests ==="
rg -B2 -A8 "endpoint|endpointOverride" plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ServiceTests.java
# Check if S3AsyncServiceTests has similar tests
echo -e "\n=== S3AsyncServiceTests endpoint tests ==="
rg -B2 -A8 "endpoint|DEFAULT_S3_ENDPOINT" plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3AsyncServiceTests.javaRepository: opensearch-project/OpenSearch
Length of output: 8936
🏁 Script executed:
# Check if this PR modified S3AsyncService as well
echo "=== Checking for S3AsyncService changes in context ==="
git log --oneline -1
# Look at related methods and initialization pattern
echo -e "\n=== S3Service initialization code ==="
sed -n '200,250p' plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.javaRepository: opensearch-project/OpenSearch
Length of output: 2681
Critical issue: S3AsyncService inconsistency must be addressed for consistency.
The conditional endpoint override logic in S3Service is sound, but this change is incomplete. S3AsyncService still applies DEFAULT_S3_ENDPOINT = "s3.amazonaws.com" via a ternary operator fallback, creating inconsistent behavior between the sync and async S3 clients. Both should follow the same endpoint resolution pattern to support FIPS and other AWS SDK defaults.
Additionally, test coverage for the no-endpoint scenario is missing. All existing tests explicitly set endpoints; there are no tests verifying S3Service behavior when clientSettings.endpoint is null or empty (which would now rely on AWS SDK defaults instead of the previously hardcoded "s3.amazonaws.com").
This is a breaking change: existing deployments without explicit endpoint configuration will switch from using s3.amazonaws.com to AWS SDK's default endpoint resolution. Ensure:
- S3AsyncService is updated to match S3Service behavior (remove DEFAULT_S3_ENDPOINT and make endpoint override conditional)
- Tests cover both endpoint-provided and no-endpoint scenarios for S3Service
- Verify FIPS endpoints work correctly when no explicit endpoint is set (AWS SDK v2 supports
AWS_USE_FIPS_ENDPOINTenvironment variable)
|
❌ Gradle check result for 70d59c1: 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? |
|
@Hongyu-Yan-lf I opened up a similar PR a few days ago: #20345 Would the changes in that PR work for this as well? |
Thanks for pointing me to that PR! It looks like it addresses the same issue and should resolve the problem I reported. Feel free to close the issue and my PR. Really appreciate your help! |
2 participants
Fixes an issue where repository-s3 always applies a hard‑coded endpoint override, even when endpoint is not configured. This causes AWS SDK errors in FIPS mode:
Related Issues
Resolves #20385
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.