Update Transport.assertDefaultThreadContext to allow all Task.REQUEST_HEADERS

[cwperks]

Description

This PR fixes test failures seen in the security repo.

  java.lang.AssertionError: expected empty context but was {X-Opaque-Id=testOpaqueId, X-Request-Id=abcd1234abcd1234abcd1234abcd1234}
  on opensearch[node_utest...][transport_worker][T#1]
      at org.opensearch.transport.Transports.assertDefaultThreadContext(Transports.java:86)
      at org.opensearch.transport.netty4.Netty4MessageChannelHandler.write(Netty4MessageChannelHandler.java:116)
      at org.opensearch.transport.netty4.Netty4TcpChannel.sendMessage(Netty4TcpChannel.java:161)
      at org.opensearch.transport.TcpChannel.sendMessage(TcpChannel.java:96)
      at org.opensearch.transport.OutboundHandler.sendBytes(OutboundHandler.java:86)
      at org.opensearch.transport.nativeprotocol.NativeOutboundHandler.sendMessage(NativeOutboundHandler.java:187)
      at org.opensearch.transport.nativeprotocol.NativeOutboundHandler.sendRequest(NativeOutboundHandler.java:121)
      at org.opensearch.transport.TcpTransport$NodeChannels.sendRequest(TcpTransport.java:374)
      at org.opensearch.transport.TransportService.sendRequestInternal(TransportService.java:1070)
      at org.opensearch.security.transport.SecurityInterceptor.sendRequestDecorate(SecurityInterceptor.java:295)

Related Issues

Resolves CI failures like: https://github.com/cwperks/security/actions/runs/25255296838/job/74053773192?pr=92

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 🔍

(Review updated until commit 8f642c5)

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 Coverage Gap The test testAssertDefaultThreadContextRejectsNonTaskRequestHeaders only verifies that a non-Task header is rejected, but there is no test for an empty context (which should always pass), nor a test that mixes allowed and disallowed headers together. Consider adding a test for an empty context and a mixed-header scenario to ensure full coverage of the assertion logic. public void testAssertDefaultThreadContextRejectsNonTaskRequestHeaders() { final ThreadContext threadContext = new ThreadContext(Settings.EMPTY); threadContext.putHeader("custom-header", "value"); expectThrows(AssertionError.class, () -> Transports.assertDefaultThreadContext(threadContext)); } Assertion Behavior The new assertion uses Task.REQUEST_HEADERS.containsAll(requestHeaders.keySet()), which allows any header defined in Task.REQUEST_HEADERS. If new headers are added to Task.REQUEST_HEADERS in the future, they will automatically be permitted on transport threads without any explicit review of whether that is safe. It is worth validating that all current and future entries in Task.REQUEST_HEADERS are indeed safe to propagate on transport threads. final Map<String, String> requestHeaders = threadContext.getRequestHeadersOnly(); assert requestHeaders.isEmpty() || Task.REQUEST_HEADERS.containsAll(requestHeaders.keySet()) : "expected empty context but was " + requestHeaders + " on " + Thread.currentThread().getName();

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 8f642c5
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard test against disabled assertions The assertDefaultThreadContext method uses Java assert statements, which are only active when assertions are enabled via the -ea JVM flag. If assertions are disabled at runtime, the method will always return true without throwing AssertionError, making this test unreliable. Consider verifying that assertions are enabled in the test environment, or restructure the test to account for this behavior. server/src/test/java/org/opensearch/transport/TransportsTests.java [30] +// Ensure assertions are enabled; if not, the assert in assertDefaultThreadContext won't fire +assumeTrue("Assertions must be enabled for this test", TransportsTests.class.desiredAssertionStatus()); expectThrows(AssertionError.class, () -> Transports.assertDefaultThreadContext(threadContext)); Suggestion importance[1-10]: 5 __ Why: The concern about assert statements being disabled is valid and could make the test unreliable in environments without -ea. However, OpenSearch test infrastructure typically runs with assertions enabled, making this a minor concern. The assumeTrue approach is a reasonable mitigation.	Low
General	Test all allowed request headers exhaustively The test only checks two specific headers from Task.REQUEST_HEADERS, but does not verify that all headers in Task.REQUEST_HEADERS are individually allowed. If Task.REQUEST_HEADERS contains additional headers beyond X_OPAQUE_ID and X_REQUEST_ID, those would not be tested. Consider iterating over all Task.REQUEST_HEADERS to ensure complete coverage. server/src/test/java/org/opensearch/transport/TransportsTests.java [18-24] public void testAssertDefaultThreadContextAllowsTaskRequestHeaders() { - final ThreadContext threadContext = new ThreadContext(Settings.EMPTY); - threadContext.putHeader(Task.X_OPAQUE_ID, "opaque-id"); - threadContext.putHeader(Task.X_REQUEST_ID, "1234567890abcdef1234567890abcdef"); - - assertTrue(Transports.assertDefaultThreadContext(threadContext)); + for (String header : Task.REQUEST_HEADERS) { + final ThreadContext threadContext = new ThreadContext(Settings.EMPTY); + threadContext.putHeader(header, "test-value"); + assertTrue("Header " + header + " should be allowed", Transports.assertDefaultThreadContext(threadContext)); + } } Suggestion importance[1-10]: 4 __ Why: The suggestion to iterate over all Task.REQUEST_HEADERS improves test coverage completeness. However, the current test already covers the two most common headers (X_OPAQUE_ID and X_REQUEST_ID), and the improvement is incremental rather than critical.	Low

---

Previous suggestions

Suggestions up to commit 96a7f1d

Category	Suggestion	Impact
General	Verify assertions are enabled for test reliability The assertDefaultThreadContext method uses Java assert statements, which are only active when assertions are enabled via the -ea JVM flag. If assertions are disabled at runtime, the method will always return true without throwing AssertionError, making this test unreliable. Ensure assertions are enabled in the test environment, or verify that OpenSearchTestCase already enables assertions. server/src/test/java/org/opensearch/transport/TransportsTests.java [30] +// Ensure assertions are enabled; OpenSearchTestCase should enable them, but verify this is the case. +// If not guaranteed, consider: assert false : "assertions must be enabled"; expectThrows(AssertionError.class, () -> Transports.assertDefaultThreadContext(threadContext)); Suggestion importance[1-10]: 4 __ Why: This is a valid concern about assertion enablement, but OpenSearchTestCase typically enables assertions in the test environment. The suggestion only asks to verify existing behavior and the improved_code is essentially the same as existing_code with added comments, limiting its impact.	Low

[github-actions]

❌ Gradle check result for 96a7f1d: 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 8f642c5

[github-actions]

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

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

❕ Gradle check result for 8f642c5: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.42%. Comparing base (e089d06) to head (8f642c5).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21462      +/-   ##
============================================
+ Coverage     73.38%   73.42%   +0.03%     
- Complexity    74353    74393      +40     
============================================
  Files          5966     5967       +1     
  Lines        338131   338183      +52     
  Branches      48740    48751      +11     
============================================
+ Hits         248139   248295     +156     
+ Misses        70247    70108     -139     
- Partials      19745    19780      +35     

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