Allowlist tracestate header on remote server port
#112649
Conversation
|
Hi @n1v0lg, I've created a changelog YAML for you. |
n1v0lg
added
v8.15.2
auto-backport
| import java.util.function.Consumer; | ||
|
|
||
| @SuppressForbidden(reason = "Uses an HTTP server for testing") | ||
| public class ConsumingTestServer extends ExternalResource { |
There was a problem hiding this comment.
This is largely a duplicate of https://github.com/elastic/elasticsearch/blob/main/test/external-modules/apm-integration/src/javaRestTest/java/org/elasticsearch/test/apmintegration/RecordingApmServer.java -- due to module structure, I could not simply rely on that class though. I can pull this test util class further up where both APM and RCS 2.0 testing have access to it; it's quite trivial though and might need more clean up to make it a generic test util, so opting for test code duplication here.
| public static TestRule clusterRule = RuleChain.outerRule(mockApmServer).around(fulfillingCluster).around(queryCluster); | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public void testTracingCrossCluster() throws Exception { |
There was a problem hiding this comment.
Note: even though the PR is a fix for the tracestate header I'm not asserting anything specific around tracestate here; the purpose of this test is to exercise and ensure that tracing works with RCS 2.0 end to end without honing in any too many specific details.
(Without the prod code fix in this PR, this test fails with a header check failure, as expected).
…lasticsearch into rcs-allowlist-tracestate-header
|
Pinging @elastic/es-security (Team:Security) |
|
@elasticmachine update branch |
n1v0lg
added
the
auto-merge
|
@elasticmachine update branch |
The [`tracestate` header](https://www.elastic.co/guide/en/apm/agent/rum-js/current/distributed-tracing-guide.html#enable-tracestate) is an HTTP header used for distributed tracing; it's a valid header to persist in cross cluster requests and should therefore be allowlisted in the remote server port header check. Note: due to implementation details, `tracestate` today may be set on the fulfilling cluster (instead of arriving across the wire) _before_ the header check. Not allowing the header therefore can lead to failures to connect clusters (elastic#112552). This PR allowlists the header to allow tracing with RCS 2.0. As a separate follow up, we may furthermore change behavior around sending the header from the query cluster to the fulfilling cluster (which we don't today). This is pending further discussion. Closes: elastic#112552
The [`tracestate` header](https://www.elastic.co/guide/en/apm/agent/rum-js/current/distributed-tracing-guide.html#enable-tracestate) is an HTTP header used for distributed tracing; it's a valid header to persist in cross cluster requests and should therefore be allowlisted in the remote server port header check. Note: due to implementation details, `tracestate` today may be set on the fulfilling cluster (instead of arriving across the wire) _before_ the header check. Not allowing the header therefore can lead to failures to connect clusters (#112552). This PR allowlists the header to allow tracing with RCS 2.0. As a separate follow up, we may furthermore change behavior around sending the header from the query cluster to the fulfilling cluster (which we don't today). This is pending further discussion. Closes: #112552 Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
The
tracestateheader is an HTTP header used for distributed tracing; it's a valid header to persist in cross cluster requests and should therefore be allowlisted in the remote server port header check.Note: due to implementation details,
tracestatetoday may be set on the fulfilling cluster (instead of arriving across the wire) before the header check. Not allowing the header therefore can lead to failures to connect clusters (#112552).This PR allowlists the header to allow tracing with RCS 2.0.
As a separate follow up, we may furthermore change behavior around sending the header from the query cluster to the fulfilling cluster (which we don't today). This is pending further discussion.
Closes: #112552