[Backport 3.5] [Bugfix] Remove X-Request-Id format restrictions and make size configurable

[finnegancarroll]

Description

Backport 80ce21c from #21048.

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.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit a750930.

Path	Line	Severity	Description
server/src/main/java/org/opensearch/common/util/RequestUtils.java	30	medium	Removing the hex-character restriction on X-Request-Id allows arbitrary characters including newlines, ANSI escape sequences, and control characters. If the request ID is written to logs without sanitization (a common pattern for correlation IDs), this opens a log injection vector. The change is functionally justified, but the absence of any character-set sanitization warrants review of all log sinks that consume request IDs.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 629706f)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Test Logic Error Line 54 tests that a 129-character request ID fails after setting newMax to 20, but the request ID length (129) is unrelated to the new maximum (20). The test should use requestWithId("a".repeat(newMax + 1)) to verify that IDs exceeding the new limit are rejected. // Was valid under default, now too long ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129))); assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]"));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 629706f

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Pass max length to constructor The setRequestIdMaxLength call immediately after construction could be avoided by passing the max length value directly to the RestController constructor. This would eliminate the need for a separate setter call and reduce the window where the field might have an incorrect value. server/src/main/java/org/opensearch/action/ActionModule.java [616-617] -restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService); -restController.setRequestIdMaxLength(HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings)); +restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService, HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings)); Suggestion importance[1-10]: 6 __ Why: This is a valid architectural improvement that would eliminate the separate setter call and reduce potential inconsistency. However, it would require modifying the RestController constructor signature, which is a more invasive change. The current approach is functional and the suggestion offers a cleaner design pattern.	Low
	Initialize from constructor settings parameter The requestIdMaxLength field should be initialized from the actual settings passed to the constructor rather than using the default value. This ensures consistency when the setting is configured at startup before the setRequestIdMaxLength method is called. server/src/main/java/org/opensearch/rest/RestController.java [103] -private volatile int requestIdMaxLength = HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.getDefault(Settings.EMPTY); +private volatile int requestIdMaxLength; Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that requestIdMaxLength is initialized with a default value rather than from constructor settings. However, the PR shows that setRequestIdMaxLength is called immediately after construction in ActionModule, making this a minor timing issue rather than a functional bug.	Low
	Increase minimum request ID length The minimum value of 16 characters may be too restrictive for legitimate use cases. Consider increasing the minimum to at least 32 characters to accommodate common UUID formats (with or without dashes) and provide more flexibility for request tracking systems. server/src/main/java/org/opensearch/http/HttpTransportSettings.java [262-269] public static final Setting<Integer> SETTING_HTTP_REQUEST_ID_MAX_LENGTH = intSetting( "http.request_id.max_length", 128, - 16, + 32, 1024, Setting.Property.Dynamic, Setting.Property.NodeScope ); Suggestion importance[1-10]: 4 __ Why: While increasing the minimum from 16 to 32 characters could accommodate UUID formats better, the current minimum of 16 is reasonable and functional. The suggestion is valid but represents a minor improvement in flexibility rather than addressing a critical issue.	Low

---

Previous suggestions

Suggestions up to commit 629706f

Category	Suggestion	Impact
General	Fix test validation logic The test expects a 129-character request ID to fail after setting newMax to 20, but the comment says "Was valid under default". A 129-character ID would fail under the default 128 limit. Use "a".repeat(newMax + 1) to properly test the new limit. modules/transport-netty4/src/javaRestTest/java/org/opensearch/rest/Netty4RequestIdIT.java [53-54] // Was valid under default, now too long -ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129))); +ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(newMax + 1))); Suggestion importance[1-10]: 8 __ Why: This is a valid bug in the test logic. The test uses "a".repeat(129) which would fail under both the default (128) and new (20) limits, making the test ineffective at verifying the dynamic setting change. Using "a".repeat(newMax + 1) would properly test that the new limit is enforced.	Medium
	Prevent initialization race condition The restController.setRequestIdMaxLength() is called before registering the settings update consumer. If the setting changes during initialization, there's a potential race condition. Consider setting the initial value after registering the consumer, or ensure thread-safe initialization order. server/src/main/java/org/opensearch/action/ActionModule.java [616-621] restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService); -restController.setRequestIdMaxLength(HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings)); clusterSettings.addSettingsUpdateConsumer( HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH, restController::setRequestIdMaxLength ); +restController.setRequestIdMaxLength(HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings)); Suggestion importance[1-10]: 4 __ Why: While the suggestion identifies a potential initialization ordering concern, the actual risk is minimal. The volatile field in RestController ensures visibility, and the initialization happens during module setup before concurrent access. The suggested reordering is a minor improvement but not critical.	Low
	Initialize field in constructor The requestIdMaxLength field is initialized with the default value but may not reflect the actual configured value from settings. This could cause validation to use incorrect limits until setRequestIdMaxLength() is called. Consider initializing this in the constructor with the actual settings value. server/src/main/java/org/opensearch/rest/RestController.java [103] -private volatile int requestIdMaxLength = HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.getDefault(Settings.EMPTY); +private volatile int requestIdMaxLength; Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that the field is initialized with a default value rather than the actual configured value. However, the PR code in ActionModule.java line 617 explicitly calls setRequestIdMaxLength() with the settings value immediately after construction, so this is already handled. The suggestion is valid but the impact is low.	Low

Suggestions up to commit 629706f

Category	Suggestion	Impact
Possible issue	Prevent initialization race condition The setRequestIdMaxLength is called immediately after RestController construction, but there's a potential race condition. If requests arrive before the setting is applied, they'll use the default value. Consider passing the max length as a constructor parameter to ensure it's set atomically during initialization. server/src/main/java/org/opensearch/action/ActionModule.java [616-621] -restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService); -restController.setRequestIdMaxLength(HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings)); +restController = new RestController( + headers, + restWrapper, + nodeClient, + circuitBreakerService, + usageService, + HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings) +); clusterSettings.addSettingsUpdateConsumer( HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH, restController::setRequestIdMaxLength ); Suggestion importance[1-10]: 3 __ Why: While the suggestion identifies a theoretical race condition, in practice this is unlikely to be an issue since RestController initialization happens during server startup before request handling begins. The volatile field in RestController ensures visibility. The suggested constructor change would require modifying the RestController constructor signature, which may have broader implications not shown in the diff.	Low

Suggestions up to commit 629706f

Category	Suggestion	Impact
Possible issue	Prevent race condition in settings initialization The restController.setRequestIdMaxLength() is called before registering the settings update consumer. If a concurrent settings update occurs between these two calls, the update might be missed. Consider registering the consumer before the initial value is set to ensure all updates are captured. server/src/main/java/org/opensearch/action/ActionModule.java [616-621] restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService); -restController.setRequestIdMaxLength(HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings)); clusterSettings.addSettingsUpdateConsumer( HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH, restController::setRequestIdMaxLength ); +restController.setRequestIdMaxLength(HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings)); Suggestion importance[1-10]: 3 __ Why: While the suggestion identifies a theoretical race condition, in practice this is unlikely to occur during initialization. The ActionModule constructor runs during node startup before concurrent settings updates would happen. The current order is acceptable, though the suggested order would be marginally safer.	Low
General	Initialize field in constructor with settings The requestIdMaxLength field is initialized with the default value but may not reflect the actual configured value from settings. This could cause validation to use incorrect limits until setRequestIdMaxLength() is called. Consider initializing this in the constructor with the actual settings value. server/src/main/java/org/opensearch/rest/RestController.java [103] -private volatile int requestIdMaxLength = HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.getDefault(Settings.EMPTY); +private volatile int requestIdMaxLength; Suggestion importance[1-10]: 2 __ Why: The suggestion misunderstands the initialization flow. The field is initialized with the default value as a fallback, and setRequestIdMaxLength() is called immediately after construction in ActionModule (line 617) with the actual settings value. The current approach is valid and provides a safe default.	Low

Suggestions up to commit a750930

Category	Suggestion	Impact
Possible issue	Restore cluster setting after test to prevent pollution The test testRequestIdAfterSettingUpdate updates a transient cluster setting but never restores it, which can cause test pollution for other tests running after it. The setting should be reset to its default value in a finally block or tearDown method to ensure test isolation. modules/transport-netty4/src/javaRestTest/java/org/opensearch/rest/Netty4RequestIdIT.java [42-60] public void testRequestIdAfterSettingUpdate() throws IOException { int newMax = 20; // Expect request is valid under default assertThat(requestWithId("a".repeat(128)).getStatusLine().getStatusCode(), equalTo(200)); // Update setting Request updateSettings = new Request("PUT", "/_cluster/settings"); updateSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": " + newMax + "}}"); client().performRequest(updateSettings); - // Was valid under default, now too long - ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129))); - ... + try { + // Was valid under default, now too long + ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129))); + assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); + assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]")); + + // ID at new size passes + assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200)); + } finally { + // Restore default setting + Request restoreSettings = new Request("PUT", "/_cluster/settings"); + restoreSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": null}}"); + client().performRequest(restoreSettings); + } } Suggestion importance[1-10]: 7 __ Why: The test modifies a transient cluster setting without restoring it, which can cause test pollution. Wrapping the test body in a try/finally block to reset the setting is a valid and important fix for test isolation.	Medium
	Initialize field from actual settings, not defaults The requestIdMaxLength field is initialized with the default value from Settings.EMPTY rather than from the actual node settings passed to the constructor. This means if a node is started with a non-default value configured in opensearch.yml, the initial value will be wrong until setRequestIdMaxLength is called from ActionModule. The field should be initialized lazily or the constructor should accept the initial value. server/src/main/java/org/opensearch/rest/RestController.java [103] -private volatile int requestIdMaxLength = HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.getDefault(Settings.EMPTY); +private volatile int requestIdMaxLength; Suggestion importance[1-10]: 4 __ Why: While the concern about using Settings.EMPTY for initialization is valid, the improved_code only removes the initializer without providing a replacement, making it incomplete. In practice, setRequestIdMaxLength is called immediately after construction in ActionModule, so the window is extremely small and only during server startup before any requests are served.	Low
General	Eliminate race condition in initial setting value There is a race condition between constructing RestController (which initializes requestIdMaxLength to the hardcoded default) and calling setRequestIdMaxLength. If requests arrive between these two calls, they will be validated against the default value rather than the configured value. Consider passing the initial max length directly to the RestController constructor to eliminate this window. server/src/main/java/org/opensearch/action/ActionModule.java [616-621] -restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService); -restController.setRequestIdMaxLength(HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings)); +restController = new RestController( + headers, restWrapper, nodeClient, circuitBreakerService, usageService, + HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.get(settings) +); clusterSettings.addSettingsUpdateConsumer( HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH, restController::setRequestIdMaxLength ); Suggestion importance[1-10]: 3 __ Why: The race condition window between construction and setRequestIdMaxLength is extremely narrow and occurs only during server startup before requests are served. The suggested constructor change would require modifying the RestController constructor signature, which is a larger refactor for a minimal practical benefit.	Low

Suggestions up to commit a750930

Category	Suggestion	Impact
Possible issue	Reset cluster setting after test completes The test testRequestIdAfterSettingUpdate changes a transient cluster setting but never resets it after the test completes. This can cause other tests in the suite to fail because the reduced max_length of 20 will persist for subsequent tests. Add a tearDown or a finally block to restore the setting to its default value after the test. modules/transport-netty4/src/javaRestTest/java/org/opensearch/rest/Netty4RequestIdIT.java [42-60] public void testRequestIdAfterSettingUpdate() throws IOException { int newMax = 20; // Expect request is valid under default assertThat(requestWithId("a".repeat(128)).getStatusLine().getStatusCode(), equalTo(200)); // Update setting Request updateSettings = new Request("PUT", "/_cluster/settings"); updateSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": " + newMax + "}}"); client().performRequest(updateSettings); - // Was valid under default, now too long - ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129))); - ... + try { + // Was valid under default, now too long + ResponseException e = expectThrows(ResponseException.class, () -> requestWithId("a".repeat(129))); + assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400)); + assertThat(e.getMessage(), containsString("exceeds maximum allowed length [" + newMax + "]")); + + // ID at new size passes + assertThat(requestWithId("a".repeat(newMax)).getStatusLine().getStatusCode(), equalTo(200)); + } finally { + // Restore default setting + Request restoreSettings = new Request("PUT", "/_cluster/settings"); + restoreSettings.setJsonEntity("{\"transient\": {\"http.request_id.max_length\": null}}"); + client().performRequest(restoreSettings); + } } Suggestion importance[1-10]: 7 __ Why: This is a valid concern - the transient setting change to max_length=20 persists after the test and could cause other tests to fail since they use "a".repeat(128) which would exceed the new limit. Wrapping the test body in a try/finally to restore the setting is a good practice for integration tests.	Medium
General	Validate setter input for max length The setRequestIdMaxLength method does not validate the incoming maxLength value. Although the setting itself enforces a minimum of 16 and maximum of 1024, this public setter could be called directly with invalid values (e.g., zero or negative), which would cause confusing behavior. Add a guard to ensure the value is positive. server/src/main/java/org/opensearch/rest/RestController.java [152-154] public void setRequestIdMaxLength(int maxLength) { + if (maxLength <= 0) { + throw new IllegalArgumentException("requestIdMaxLength must be positive, got: " + maxLength); + } this.requestIdMaxLength = maxLength; } Suggestion importance[1-10]: 2 __ Why: The setRequestIdMaxLength setter is only called via clusterSettings.addSettingsUpdateConsumer and the initial get(settings) call, both of which are already validated by the intSetting definition with min=16 and max=1024. Adding a redundant guard here provides minimal practical benefit.	Low

[finnegancarroll]

Code diff analyzer is failing with:
"""
Removing the hex-character restriction on X-Request-Id allows arbitrary characters including newlines, ANSI escape sequences, and control characters. If the request ID is written to logs without sanitization (a common pattern for correlation IDs), this opens a log injection vector. The change is functionally justified, but the absence of any character-set sanitization warrants review of all log sinks that consume request IDs.
"""

This was identified by AI in the upstream PR but now it seems to be hard failing CI.
#21048 (comment)
#21048 (comment)
#21048 (review)

[github-actions]

Persistent review updated to latest commit a750930

[github-actions]

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

Persistent review updated to latest commit a750930

[github-actions]

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

Persistent review updated to latest commit a750930

[github-actions]

Persistent review updated to latest commit a750930

[github-actions]

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

Persistent review updated to latest commit a750930

[github-actions]

✅ Gradle check result for a750930: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.32%. Comparing base (9258302) to head (629706f).
⚠️ Report is 1 commits behind head on 3.5.

Files with missing lines	Patch %	Lines
.../main/java/org/opensearch/rest/RestController.java	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                3.5   #21434      +/-   ##
============================================
- Coverage     73.35%   73.32%   -0.03%     
+ Complexity    71967    71929      -38     
============================================
  Files          5782     5777       -5     
  Lines        329128   329100      -28     
  Branches      47451    47449       -2     
============================================
- Hits         241442   241326     -116     
- Misses        68310    68414     +104     
+ Partials      19376    19360      -16     

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

[prudhvigodithi]

Thanks @finnegancarroll seeing the following error

A failure occurred while executing me.champeau.gradle.japicmp.JApiCmpWorkAction
   > Detected binary changes.
         - current: opensearch-3.5.1-SNAPSHOT.jar
         - baseline: opensearch-3.6.0.jar.

@cwperks

[cwperks]

Thanks @finnegancarroll seeing the following error

A failure occurred while executing me.champeau.gradle.japicmp.JApiCmpWorkAction
   > Detected binary changes.
         - current: opensearch-3.5.1-SNAPSHOT.jar
         - baseline: opensearch-3.6.0.jar.

@cwperks

Why does it show 3.6 here?

[jainankitk]

Seems the change is already part of 3.6. Don't understand the reason for failing breaking change validation

[cwperks]

@prudhvigodithi FYI I think #21529 would fix the version selection logic and make it branch agnostic.

[prudhvigodithi]

Thanks @cwperks, @finnegancarroll can you once fetch the upstream ? I will monitor the Gradle check failures and retry if they are flaky. Still waiting to merge this #21546.

[github-actions]

Persistent review updated to latest commit 629706f

[github-actions]

❌ Gradle check result for 629706f: 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 629706f: 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?

[github-actions]

Persistent review updated to latest commit 629706f

[github-actions]

❌ Gradle check result for 629706f: 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]

Persistent review updated to latest commit 629706f

[github-actions]

❌ Gradle check result for 629706f: 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]

Persistent review updated to latest commit 629706f

[github-actions]

✅ Gradle check result for 629706f: SUCCESS