Fix missing repository-s3 client settings registration#20376
Fix missing repository-s3 client settings registration#20376Gautam-aman wants to merge 10 commits intoopensearch-project:mainfrom
Conversation
📝 WalkthroughWalkthroughAdded a new accessor Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java (1)
717-748: Misplaced Javadoc comment.The Javadoc at lines 717-720 documents
IrsaCredentialsbut now incorrectly precedesgetAllClientSettings(). Move the new method above the Javadoc block or relocate the comment.🔎 Proposed fix to relocate the method
+ static Collection<Setting<?>> getAllClientSettings() { + return List.of( + ACCESS_KEY_SETTING, + SECRET_KEY_SETTING, + SESSION_TOKEN_SETTING, + ENDPOINT_SETTING, + PROTOCOL_SETTING, + PROXY_TYPE_SETTING, + PROXY_HOST_SETTING, + PROXY_PORT_SETTING, + PROXY_USERNAME_SETTING, + PROXY_PASSWORD_SETTING, + READ_TIMEOUT_SETTING, + REQUEST_TIMEOUT_SETTING, + CONNECTION_TIMEOUT_SETTING, + CONNECTION_TTL_SETTING, + MAX_CONNECTIONS_SETTING, + MAX_SYNC_CONNECTIONS_SETTING, + CONNECTION_ACQUISITION_TIMEOUT, + MAX_PENDING_CONNECTION_ACQUIRES, + MAX_RETRIES_SETTING, + USE_THROTTLE_RETRIES_SETTING, + USE_PATH_STYLE_ACCESS, + SIGNER_OVERRIDE, + REGION, + ROLE_ARN_SETTING, + IDENTITY_TOKEN_FILE_SETTING, + ROLE_SESSION_NAME_SETTING, + LEGACY_MD5_CHECKSUM_CALCULATION, + DISABLE_CHUNKED_ENCODING + ); + } + /** * Class to store IAM Roles for Service Accounts (IRSA) credentials * See please: https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts.html */ - - static Collection<Setting<?>> getAllClientSettings() { - return List.of( - ACCESS_KEY_SETTING, - ... - ); - } - - static class IrsaCredentials {
🤖 Fix all issues with AI Agents
In
@plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java:
- Around line 722-746: getAllClientSettings() is missing seven client settings
so they aren't registered; update the return List.of(...) in
getAllClientSettings to include REQUEST_TIMEOUT_SETTING,
CONNECTION_TIMEOUT_SETTING, CONNECTION_TTL_SETTING, MAX_CONNECTIONS_SETTING,
MAX_SYNC_CONNECTIONS_SETTING, CONNECTION_ACQUISITION_TIMEOUT, and
MAX_PENDING_CONNECTION_ACQUIRES along with the existing settings so the method
truly returns all client settings and prevents "unknown setting" errors.
🧹 Nitpick comments (1)
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ClientSettingsTests.java (1)
97-111: Well-designed test that will catch the missing settings bug.This reflection-based test correctly validates that
getAllClientSettings()returns all declared staticSettingfields. When run, it will fail due to the 7 missing settings ingetAllClientSettings().Consider adding a descriptive message to the assertion to help diagnose failures:
🔎 Optional: add assertion message
assertEquals( + "getAllClientSettings() must return all static Setting fields declared in S3ClientSettings", Set.copyOf(reflectedSettings), Set.copyOf(S3ClientSettings.getAllClientSettings()) );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.javaplugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.javaplugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ClientSettingsTests.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/S3ClientSettingsTests.java
🧬 Code graph analysis (2)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java (1)
S3ClientSettings(61-791)
plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ClientSettingsTests.java (1)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java (1)
S3ClientSettings(61-791)
⏰ 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
🔇 Additional comments (2)
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java (1)
55-55: Wildcard import is acceptable for internal consistency.This change aligns with the pattern used elsewhere in the codebase, though explicit imports are generally preferred for clarity.
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java (1)
346-366: Clean refactor delegating toS3ClientSettings.getAllClientSettings().The separation of client-level and repository-level settings improves maintainability. However, this will only work correctly once the missing settings in
getAllClientSettings()are added.
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java
Show resolved
Hide resolved
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.Set; | ||
| import java.util.*; |
There was a problem hiding this comment.
OpenSearch forbids wildcard imports. Can we add the imports back in here, please?
|
I’ve opened a PR addressing this using the suggested approach for consolidating |
|
❌ Gradle check result for b4c1dfa: 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? |
|
Good catch thanks for flagging this. |
|
❌ Gradle check result for e82002c: 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? |
|
Thanks I’m looking into the Jenkins failure now. Once I identify whether it’s related to this change or a flaky test, |
|
❌ Gradle check result for db78647: 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? |
|
@Gautam-aman -- can you please run |
|
I’ve run ./gradlew spotlessApply from the repository root and pushed the formatting fixes. |
|
❌ Gradle check result for 3027042: 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? |
Gautam-aman
force-pushed
the
fix-s3-client-settings
branch
from
3027042 to
f03659a
Compare
|
❌ Gradle check result for f03659a: 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? |
|
The Gradle failures appear to be from unapproved workflows. I’ve already run ./gradlew spotlessApply from the repo root and pushed the results. Happy to re-run or adjust once workflows are approved. |
|
The latest failure is on Retrying the build. Incidentally, the Changelog Verifier doesn't seem to like the changelog fragment. It's still expecting a change to CHANGELOG.md. |
|
❌ Gradle check result for f03659a: 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? |
|
I’ve pushed the latest updates: The remaining Gradle failure appears to be the known flaky test mentioned above. Happy to rebase or adjust if anything else is needed. |
|
❌ Gradle check result for 246be81: 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? |
Signed-off-by: Aman Gautam <amangautam2128@gmail.com>
Signed-off-by: Aman Gautam <amangautam2128@gmail.com>
Signed-off-by: Aman Gautam <amangautam2128@gmail.com>
Signed-off-by: Aman Gautam <amangautam2128@gmail.com>
Signed-off-by: Aman Gautam <amangautam2128@gmail.com>
Gautam-aman
force-pushed
the
fix-s3-client-settings
branch
from
246be81 to
6610bb9
Compare
Signed-off-by: Aman Gautam <amangautam2128@gmail.com>
Gautam-aman
force-pushed
the
fix-s3-client-settings
branch
from
6610bb9 to
f9a059d
Compare
|
Added missing Signed-off-by to the final commit to satisfy DCO. |
|
❌ Gradle check result for f9a059d: 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? |
Again, this changelog fragment thing isn't how the project works (unless something has changed recently that I've missed). You need to edit the CHANGELOG.md file. The error from the Changelog Verifier step says: |
Signed-off-by: Aman Gautam <amangautam2128@gmail.com>
|
Thanks I’ve updated CHANGELOG.md under [Unreleased 3.x] → Fixed with this change. |
Signed-off-by: Aman Gautam <amangautam2128@gmail.com>
Signed-off-by: Aman Gautam <amangautam2128@gmail.com>
Signed-off-by: Aman Gautam <amangautam2128@gmail.com>
|
Changelog updated under Unreleased 3.x |
|
❌ Gradle check result for 9f9cc77: 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? |
|
Ahh... I hadn't seen this output from Sorry -- that's a miss on my part. |
|
❌ Gradle check result for 9f9cc77: 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? |
|
This looks like a known CI / Spotless infrastructure issue rather than a code or formatting problem in this PR. No Java sources in opensearch-x-content were modified here. |
|
This PR is stalled because it has been open for 30 days with no activity. |
3 participants
Problem
S3 client settings such as
s3.client.default.disable_chunked_encodingwere definedbut not consistently registered, causing OpenSearch to fail with
unknown settingerrors during startup.
Solution
S3ClientSettings.getAllClientSettings()as a single source of truthS3RepositoryPluginto rely on it instead of maintaining a duplicate listTesting
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.