envoy: [bug] Client x-request-id Mangled by RequestIdExtension::setTraceStatus
Description
When providing an x-request-id header in the request (rather than have it injected by Envoy), a UUID has its 14th byte modified. This results in a different UUID than provided by the caller, breaking the x-request-id passed along a call chain.
At a cursory glance, I believe the code is here: request_id_extension_uuid_impl.cc
void UUIDRequestIDExtension::setTraceStatus(RequestHeaderMap& request_headers, TraceStatus status) {
if (request_headers.RequestId() == nullptr) {
return;
}
absl::string_view uuid_view = request_headers.getRequestIdValue();
if (uuid_view.length() != Runtime::RandomGeneratorImpl::UUID_LENGTH) {
return;
}
std::string uuid(uuid_view);
switch (status) {
case TraceStatus::Forced:
uuid[TRACE_BYTE_POSITION] = TRACE_FORCED;
break;
case TraceStatus::Client:
uuid[TRACE_BYTE_POSITION] = TRACE_CLIENT;
break;
case TraceStatus::Sampled:
uuid[TRACE_BYTE_POSITION] = TRACE_SAMPLED;
break;
case TraceStatus::NoTrace:
uuid[TRACE_BYTE_POSITION] = NO_TRACE;
break;
}
request_headers.setRequestId(uuid);
}
This code doesn’t distinguish either:
- UUID vs non-UUID string (of length 36)
- UUID generated internally vs provided in original request
Illustrative flow:
- Request created by client, with ‘x-request-id’ header containing value ‘aaaaaaaa-bbbb-4ccc-dddd-eeeeeeeeeeee’
- Request is received by Envoy, which sets Tracing Status on the 14th byte
- Request now has ‘x-request-id’ header containing value ‘aaaaaaaa-bbbb-9ccc-dddd-eeeeeeeeeeee’ (or ‘bccc’, ‘eccc’)
Ask
Ideally, setTraceStatus() should only update trace status on valid UUIDs generated by Envoy itself, and should not update values provided/retained from caller.
Config
Seems to occur either naturally (when request is deemed to be x-request-internal) or when using the ‘preserve_external_request_id’ connection manager flag.
References
I believe it caused the behaviour seen #https://github.com/envoyproxy/envoy/issues/6050
Examples/Reproduce
Used with lds.yaml:
- name: envoy.http_connection_manager
typed_config:
"@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager
server_name: 'example'
server_header_transformation: PASS_THROUGH
generate_request_id: true
preserve_external_request_id: true
Example - Request populated ‘x-request-id’, len != 36
curl -v -H "x-request-id: chris-request-id" https://echotest
> GET / HTTP/1.1
> Host: echotest
> User-Agent: curl/7.64.1
> Accept: */*
> x-request-id: chris-request-id
>
< HTTP/1.1 200 OK
< server: openresty/1.11.2.5
< date: Wed, 10 Jun 2020 00:19:18 GMT
< content-type: text/plain
< x-envoy-upstream-service-time: 0
< transfer-encoding: chunked
<
GET / HTTP/1.1
host: echotest
user-agent: curl/7.64.1
accept: */*
x-request-id: chris-request-id
x-forwarded-proto: https
x-envoy-expected-rq-timeout-ms: 30000
x-b3-traceid: 2157a3a019156213
x-b3-spanid: 89a206d07d499cda
x-b3-parentspanid: 2157a3a019156213
x-b3-sampled: 0
content-length: 0
Example - Request populated 'x-request-id, UUID with 14th byte == 4 (no trace)
curl -v -H "x-request-id: b6aa0744-49e1-4f5d-b5a8-750aaaf6c205" https://echotest
> GET / HTTP/1.1
> Host: echotest
> User-Agent: curl/7.64.1
> Accept: */*
> x-request-id: b6aa0744-49e1-4f5d-b5a8-750aaaf6c205
>
< HTTP/1.1 200 OK
< server: openresty/1.11.2.5
< date: Wed, 10 Jun 2020 00:12:49 GMT
< content-type: text/plain
< x-envoy-upstream-service-time: 0
< transfer-encoding: chunked
<
GET / HTTP/1.1
host: echotest
user-agent: curl/7.64.1
accept: */*
x-request-id: b6aa0744-49e1-9f5d-b5a8-750aaaf6c205
x-forwarded-proto: https
x-envoy-expected-rq-timeout-ms: 30000
x-b3-traceid: 9a837832ae4fac75
x-b3-spanid: f4468cd7b2b7db97
x-b3-parentspanid: 9a837832ae4fac75
x-b3-sampled: 1
content-length: 0
(extraneous -v fields removed)
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Reactions: 13
- Comments: 24 (11 by maintainers)
Commits related to this issue
- request_id: allow disabling trace reason packing 1) Promote the default UUID request_id implementation to an actual extension that is required in the build and wire up all documentation. 2) Add... — committed to envoyproxy/envoy by deleted user 3 years ago
- request_id: allow disabling trace reason packing 1) Promote the default UUID request_id implementation to an actual extension that is required in the build and wire up all documentation. 2) Add... — committed to envoyproxy/envoy by deleted user 3 years ago
- request_id: allow disabling trace reason packing 1) Promote the default UUID request_id implementation to an actual extension that is required in the build and wire up all documentation. 2) Add... — committed to envoyproxy/envoy by deleted user 3 years ago
- request_id: allow disabling trace reason packing (#15248) 1) Promote the default UUID request_id implementation to an actual extension that is required in the build and wire up all documentat... — committed to envoyproxy/envoy by deleted user 3 years ago
- request_id: allow disabling trace reason packing (#15248) 1) Promote the default UUID request_id implementation to an actual extension that is required in the build and wire up all documentatio... — committed to envoyproxy/data-plane-api by deleted user 3 years ago
- request_id: allow disabling trace reason packing (#15248) 1) Promote the default UUID request_id implementation to an actual extension that is required in the build and wire up all documentat... — committed to rexengineering/istio-envoy by deleted user 3 years ago
I don’t think anyone is working on this currently. I will try to get this done since it’s a frequent request.
@Brunomachadob yes that’s correct. Let me know if it doesn’t work for you.
Yes, this is my plan. I’m going to allow configuration to be passed to the built in UUID request ID extension. Initially I will just add configuration to disable trace status packing, but eventually we could also allow the header name itself to be configured. I think this will satisfy all of the problems that are being reported here.
+1 to allow the behavior to be configurable, -1 to a new header. Context propagation is a disaster and I would rather not make it more complicated for the common case as @cbrisket mentions.