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

[finnegancarroll]

Description

Introduces dynamic http.request_id.max_length setting for configuring maximum length for X-Request-Id headers. Removes the alpha-numeric validation of X-Request-Id.

Related Issues

Resolves #20688

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 Reviewer Guide 🔍

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 Thread Safety The requestIdMaxLength field is declared volatile which handles visibility, but setRequestIdMaxLength is a public setter callable from outside. Verify that the cluster settings update consumer and any concurrent request processing cannot cause race conditions or inconsistent validation behavior during updates. private volatile int requestIdMaxLength = HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH.getDefault(Settings.EMPTY); private static final BytesReference FAVICON_RESPONSE; static { try (InputStream stream = RestController.class.getResourceAsStream("/config/favicon.ico")) { ByteArrayOutputStream out = new ByteArrayOutputStream(); Streams.copy(stream, out); FAVICON_RESPONSE = new BytesArray(out.toByteArray()); } catch (IOException e) { throw new AssertionError(e); } } private final PathTrie<RestMethodHandlers> handlers = new PathTrie<>(RestUtils.REST_DECODER); private final UnaryOperator<RestHandler> handlerWrapper; private final NodeClient client; private final CircuitBreakerService circuitBreakerService; /** Rest headers that are copied to internal requests made during a rest request. */ private final Set<RestHeaderDefinition> headersToCopy; private final UsageService usageService; public RestController( Set<RestHeaderDefinition> headersToCopy, UnaryOperator<RestHandler> handlerWrapper, NodeClient client, CircuitBreakerService circuitBreakerService, UsageService usageService ) { this.headersToCopy = headersToCopy; this.usageService = usageService; if (handlerWrapper == null) { handlerWrapper = h -> h; // passthrough if no wrapper set } this.handlerWrapper = handlerWrapper; this.client = client; this.circuitBreakerService = circuitBreakerService; registerHandlerNoWrap( RestRequest.Method.GET, "/favicon.ico", (request, channel, clnt) -> channel.sendResponse(new BytesRestResponse(RestStatus.OK, "image/x-icon", FAVICON_RESPONSE)) ); } public void setRequestIdMaxLength(int maxLength) { this.requestIdMaxLength = maxLength; } Missing Cleanup The integration test testRequestIdAfterSettingUpdate modifies a transient cluster setting (http.request_id.max_length) but does not restore the original value after the test. This could affect other tests running in the same cluster if test ordering matters. 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))); 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)); } Min Value Concern The minimum allowed value for http.request_id.max_length is 16. Verify that this minimum is sufficient and intentional, as very short request IDs could cause issues with uniqueness or compatibility with existing clients sending longer IDs. public static final Setting<Integer> SETTING_HTTP_REQUEST_ID_MAX_LENGTH = intSetting( "http.request_id.max_length", 128, 16, 1024, Setting.Property.Dynamic, Setting.Property.NodeScope );

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Restore cluster setting after test completes The test testRequestIdAfterSettingUpdate modifies a transient cluster setting but never restores it to its default value after the test completes. This can cause test pollution, where subsequent tests run with max_length=20 instead of the default 128, potentially causing unexpected failures. Add a finally block or a teardown step to reset the setting back to its default (or null) 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 - modifying transient cluster settings without cleanup can cause test pollution in integration tests. The testRequestIdAfterSettingUpdate test sets http.request_id.max_length to 20 but never restores it, which could affect other tests running in the same cluster. The improved code correctly wraps the test body in a try/finally block to reset the setting.	Medium
General	Validate input before updating max length The setRequestIdMaxLength method does not validate the maxLength parameter before setting it. Although the setting itself has min/max bounds (16–1024), this public setter could be called directly with an invalid value (e.g., zero or negative), potentially causing unexpected behavior in validateRequestId. 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 method is only called via the settings framework (HttpTransportSettings.SETTING_HTTP_REQUEST_ID_MAX_LENGTH) which already enforces bounds of 16–1024, making this extra validation largely redundant. The suggestion adds defensive programming for a scenario that is practically unreachable through normal usage.	Low

[jainankitk]

Closing in favor of #21096

[github-actions]

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