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

Most upvoted comments

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.

Or we could rework this UUIDRequestIDExtension to work with any configured header, by default it would be x-request-id and we could override it via config.

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.