request id: minor optimization or fix to the request id logic (#36773)

Commit Message: request id: minor optimization or fix to the request id
logic
Additional Description: 

This PR did some minor optimization (or fix) to the request logic:
1. If `pack_trace_reason` is set to `false`, then the Envoy will not try
to read the trace reason from the request id. This ensure the read and
write operation of the trace reason be consistant.
2. If `pack_trace_reason` is set to `true` and an external request id is
used (by setting the `preserve_external_request_id` to true), then the
trace reason from the external request id will be cleared. This ensure
the trace reason from the external request id will not be used.

Risk Level: low.
Testing: unit.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

---------

Signed-off-by: wangbaiping/wbpcode <wangbaiping@bytedance.com>
Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

changelogs/current.yaml

@@ -21,6 +21,14 @@ minor_behavior_changes:
   change: |
     The NaN and Infinity values of float will be serialized to ``null`` and ``"inf"`` respectively in the
     metadata (``DYNAMIC_METADATA``, ``CLUSTER_METADATA``, etc.) formatter.
+- area: http
+  change: |
+    If the :ref:`pack_trace_reason <envoy_v3_api_field_extensions.request_id.uuid.v3.UuidRequestIdConfig.pack_trace_reason>`
+    is set to false, Envoy will not parse the trace reason from the ``x-request-id`` header to ensure reads and writes of
+    trace reason be consistant.
+    If the :ref:`pack_trace_reason <envoy_v3_api_field_extensions.request_id.uuid.v3.UuidRequestIdConfig.pack_trace_reason>`
+    is set to true and external ``x-request-id`` value is used, the trace reason in the external request id will not
+    be trusted and will be cleared.
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*

envoy/http/request_id_extension.h

@@ -38,9 +38,11 @@ class RequestIDExtension {
    * Directly set a request ID into the provided request headers. Override any previous request ID
    * if any.
    * @param request_headers supplies the incoming request headers for setting a request ID.
-   * @param force specifies if a new request ID should be forcefully set if one is already present.
+   * @param edge_request whether the request is an edge request.
+   * @param keep_external_id whether to preserve the request ID from external requests.
    */
-  virtual void set(Http::RequestHeaderMap& request_headers, bool force) PURE;
+  virtual void set(Http::RequestHeaderMap& request_headers, bool edge_request,
+                   bool keep_external_id) PURE;
 
   /**
    * Preserve request ID in response headers if any is set in the request headers.

source/common/http/conn_manager_utility.cc

@@ -278,11 +278,8 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m
 
   // Generate x-request-id for all edge requests, or if there is none.
   if (config.generateRequestId()) {
-    auto rid_extension = config.requestIDExtension();
-    // Unconditionally set a request ID if we are allowed to override it from
-    // the edge. Otherwise just ensure it is set.
-    const bool force_set = !config.preserveExternalRequestId() && edge_request;
-    rid_extension->set(request_headers, force_set);
+    config.requestIDExtension()->set(request_headers, edge_request,
+                                     config.preserveExternalRequestId());
   }
 
   if (connection.connecting() && request_headers.get(Headers::get().EarlyData).empty()) {

source/extensions/request_id/uuid/config.cc

@@ -11,15 +11,36 @@ namespace Envoy {
 namespace Extensions {
 namespace RequestId {
 
-void UUIDRequestIDExtension::set(Http::RequestHeaderMap& request_headers, bool force) {
-  if (!force && request_headers.RequestId()) {
+void UUIDRequestIDExtension::set(Http::RequestHeaderMap& request_headers, bool edge_request,
+                                 bool keep_external_id) {
+  const Http::HeaderEntry* request_id_header = request_headers.RequestId();
+
+  // No request ID then set new one anyway.
+  if (request_id_header == nullptr) {
+    request_headers.setRequestId(random_.uuid());
     return;
   }
 
-  // TODO(PiotrSikora) PERF: Write UUID directly to the header map.
-  std::string uuid = random_.uuid();
-  ASSERT(!uuid.empty());
-  request_headers.setRequestId(uuid);
+  // There is request ID already set and this is not an edge request. Then this is trusted
+  // request ID. Do nothing.
+  if (!edge_request) {
+    return;
+  }
+
+  // There is request ID already set and this is an edge request. Then this is ID may cannot
+  // be trusted.
+
+  if (!keep_external_id) {
+    // If we are not keeping external request ID, then set new one anyway.
+    request_headers.setRequestId(random_.uuid());
+    return;
+  }
+
+  // If we are keeping external request ID, and `pack_trace_reason` is enabled, then clear
+  // the trace reason in the external request ID.
+  if (pack_trace_reason_) {
+    setTraceReason(request_headers, Tracing::Reason::NotTraceable);
+  }
 }
 
 void UUIDRequestIDExtension::setInResponse(Http::ResponseHeaderMap& response_headers,
@@ -57,7 +78,9 @@ UUIDRequestIDExtension::getInteger(const Http::RequestHeaderMap& request_headers
 
 Tracing::Reason
 UUIDRequestIDExtension::getTraceReason(const Http::RequestHeaderMap& request_headers) {
-  if (request_headers.RequestId() == nullptr) {
+  // If the request ID is not present or the pack trace reason is not enabled, return
+  // NotTraceable directly.
+  if (!pack_trace_reason_ || request_headers.RequestId() == nullptr) {
     return Tracing::Reason::NotTraceable;
   }
   absl::string_view uuid = request_headers.getRequestIdValue();

source/extensions/request_id/uuid/config.h

@@ -28,7 +28,8 @@ class UUIDRequestIDExtension : public Envoy::Http::RequestIDExtension {
   bool packTraceReason() { return pack_trace_reason_; }
 
   // Envoy::Http::RequestIDExtension
-  void set(Envoy::Http::RequestHeaderMap& request_headers, bool force) override;
+  void set(Envoy::Http::RequestHeaderMap& request_headers, bool edge_request,
+           bool keep_external_id) override;
   void setInResponse(Envoy::Http::ResponseHeaderMap& response_headers,
                      const Envoy::Http::RequestHeaderMap& request_headers) override;
   absl::optional<absl::string_view>

test/common/http/conn_manager_utility_test.cc

@@ -43,9 +43,10 @@ class MockRequestIDExtension : public RequestIDExtension {
 public:
   explicit MockRequestIDExtension(Random::RandomGenerator& random)
       : real_(Extensions::RequestId::UUIDRequestIDExtension::defaultInstance(random)) {
-    ON_CALL(*this, set(_, _))
-        .WillByDefault([this](Http::RequestHeaderMap& request_headers, bool force) {
-          return real_->set(request_headers, force);
+    ON_CALL(*this, set(_, _, _))
+        .WillByDefault([this](Http::RequestHeaderMap& request_headers, bool keep_external_id,
+                              bool edge_request) {
+          return real_->set(request_headers, keep_external_id, edge_request);
         });
     ON_CALL(*this, setInResponse(_, _))
         .WillByDefault([this](Http::ResponseHeaderMap& response_headers,
@@ -71,7 +72,7 @@ class MockRequestIDExtension : public RequestIDExtension {
     ON_CALL(*this, useRequestIdForTraceSampling()).WillByDefault(Return(true));
   }
 
-  MOCK_METHOD(void, set, (Http::RequestHeaderMap&, bool));
+  MOCK_METHOD(void, set, (Http::RequestHeaderMap&, bool, bool));
   MOCK_METHOD(void, setInResponse, (Http::ResponseHeaderMap&, const Http::RequestHeaderMap&));
   MOCK_METHOD(absl::optional<absl::string_view>, get, (const Http::RequestHeaderMap&), (const));
   MOCK_METHOD(absl::optional<uint64_t>, getInteger, (const Http::RequestHeaderMap&), (const));
@@ -2026,8 +2027,7 @@ TEST_F(ConnectionManagerUtilityTest, PreserveExternalRequestId) {
   ON_CALL(config_, preserveExternalRequestId()).WillByDefault(Return(true));
   TestRequestHeaderMapImpl headers{{"x-request-id", "my-request-id"},
                                    {"x-forwarded-for", "198.51.100.1"}};
-  EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), false));
-  EXPECT_CALL(*request_id_extension_, set(_, true)).Times(0);
+  EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), true, true));
   EXPECT_EQ((MutateRequestRet{"134.2.2.11:0", false, Tracing::Reason::NotTraceable}),
             callMutateRequestHeaders(headers, Protocol::Http2));
   EXPECT_CALL(random_, uuid()).Times(0);
@@ -2041,8 +2041,7 @@ TEST_F(ConnectionManagerUtilityTest, PreseverExternalRequestIdNoReqId) {
   ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
   ON_CALL(config_, preserveExternalRequestId()).WillByDefault(Return(true));
   TestRequestHeaderMapImpl headers{{"x-forwarded-for", "198.51.100.1"}};
-  EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), false));
-  EXPECT_CALL(*request_id_extension_, set(_, true)).Times(0);
+  EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), true, true));
   EXPECT_EQ((MutateRequestRet{"134.2.2.11:0", false, Tracing::Reason::NotTraceable}),
             callMutateRequestHeaders(headers, Protocol::Http2));
   EXPECT_EQ(random_.uuid_, headers.get_(Headers::get().RequestId));
@@ -2053,8 +2052,7 @@ TEST_F(ConnectionManagerUtilityTest, PreseverExternalRequestIdNoReqId) {
 TEST_F(ConnectionManagerUtilityTest, PreserveExternalRequestIdNoEdgeRequestKeepRequestId) {
   ON_CALL(config_, preserveExternalRequestId()).WillByDefault(Return(true));
   TestRequestHeaderMapImpl headers{{"x-request-id", "myReqId"}};
-  EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), false));
-  EXPECT_CALL(*request_id_extension_, set(_, true)).Times(0);
+  EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), false, true));
   callMutateRequestHeaders(headers, Protocol::Http2);
   EXPECT_EQ("myReqId", headers.get_(Headers::get().RequestId));
 }
@@ -2064,13 +2062,12 @@ TEST_F(ConnectionManagerUtilityTest, PreserveExternalRequestIdNoEdgeRequestKeepR
 TEST_F(ConnectionManagerUtilityTest, PreserveExternalRequestIdNoEdgeRequestGenerateNewRequestId) {
   ON_CALL(config_, preserveExternalRequestId()).WillByDefault(Return(true));
   TestRequestHeaderMapImpl headers;
-  EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), false));
-  EXPECT_CALL(*request_id_extension_, set(_, true)).Times(0);
+  EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), false, true));
   callMutateRequestHeaders(headers, Protocol::Http2);
   EXPECT_EQ(random_.uuid_, headers.get_(Headers::get().RequestId));
 }
 
-// test preserve_external_request_id false edge request generates new request id
+// Test preserve_external_request_id false and edge_request true.
 TEST_F(ConnectionManagerUtilityTest, NoPreserveExternalRequestIdEdgeRequestGenerateRequestId) {
   ON_CALL(config_, preserveExternalRequestId()).WillByDefault(Return(false));
   connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(
@@ -2081,8 +2078,7 @@ TEST_F(ConnectionManagerUtilityTest, NoPreserveExternalRequestIdEdgeRequestGener
     ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
     TestRequestHeaderMapImpl headers{{"x-forwarded-for", "198.51.100.1"},
                                      {"x-request-id", "my-request-id"}};
-    EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), true));
-    EXPECT_CALL(*request_id_extension_, set(_, false)).Times(0);
+    EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), true, false));
     EXPECT_EQ((MutateRequestRet{"134.2.2.11:0", false, Tracing::Reason::NotTraceable}),
               callMutateRequestHeaders(headers, Protocol::Http2));
     EXPECT_EQ(random_.uuid_, headers.get_(Headers::get().RequestId));
@@ -2091,32 +2087,29 @@ TEST_F(ConnectionManagerUtilityTest, NoPreserveExternalRequestIdEdgeRequestGener
   // with no request id
   {
     TestRequestHeaderMapImpl headers{{"x-forwarded-for", "198.51.100.1"}};
-    EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), true));
-    EXPECT_CALL(*request_id_extension_, set(_, false)).Times(0);
+    EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), true, false));
     EXPECT_EQ((MutateRequestRet{"134.2.2.11:0", false, Tracing::Reason::NotTraceable}),
               callMutateRequestHeaders(headers, Protocol::Http2));
     EXPECT_EQ(random_.uuid_, headers.get_(Headers::get().RequestId));
   }
 }
 
-// test preserve_external_request_id false not edge request
+// Test preserve_external_request_id false and edge_request false.
 TEST_F(ConnectionManagerUtilityTest, NoPreserveExternalRequestIdNoEdgeRequest) {
   ON_CALL(config_, preserveExternalRequestId()).WillByDefault(Return(false));
 
   // with no request id
   {
     TestRequestHeaderMapImpl headers;
-    EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), false));
-    EXPECT_CALL(*request_id_extension_, set(_, true)).Times(0);
+    EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), false, false));
     callMutateRequestHeaders(headers, Protocol::Http2);
     EXPECT_EQ(random_.uuid_, headers.get_(Headers::get().RequestId));
   }
 
   // with request id
   {
     TestRequestHeaderMapImpl headers{{"x-request-id", "my-request-id"}};
-    EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), false));
-    EXPECT_CALL(*request_id_extension_, set(_, true)).Times(0);
+    EXPECT_CALL(*request_id_extension_, set(testing::Ref(headers), false, false));
     callMutateRequestHeaders(headers, Protocol::Http2);
     EXPECT_EQ("my-request-id", headers.get_(Headers::get().RequestId));
   }

test/extensions/filters/network/http_connection_manager/config_test.cc

@@ -2399,7 +2399,7 @@ class TestRequestIDExtension : public Http::RequestIDExtension {
   TestRequestIDExtension(const test::http_connection_manager::CustomRequestIDExtension& config)
       : config_(config) {}
 
-  void set(Http::RequestHeaderMap&, bool) override {}
+  void set(Http::RequestHeaderMap&, bool, bool) override {}
   void setInResponse(Http::ResponseHeaderMap&, const Http::RequestHeaderMap&) override {}
   absl::optional<absl::string_view> get(const Http::RequestHeaderMap&) const override {
     return absl::nullopt;

test/extensions/request_id/uuid/config_test.cc

@@ -19,30 +19,96 @@ TEST(UUIDRequestIDExtensionTest, SetRequestID) {
   testing::StrictMock<Random::MockRandomGenerator> random;
   UUIDRequestIDExtension uuid_utils(envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(),
                                     random);
-  Http::TestRequestHeaderMapImpl request_headers;
 
-  EXPECT_CALL(random, uuid()).WillOnce(Return("first-request-id"));
-  uuid_utils.set(request_headers, true);
-  EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId));
+  {
+    // edge_request: true, keep_external_id: false.
+
+    Http::TestRequestHeaderMapImpl request_headers;
+
+    // Without request ID.
+    EXPECT_CALL(random, uuid()).WillOnce(Return("first-request-id"));
+    uuid_utils.set(request_headers, true, false);
+    EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId));
+
+    // With request ID. Previous one will be overwritten.
+    EXPECT_CALL(random, uuid()).WillOnce(Return("second-request-id"));
+    uuid_utils.set(request_headers, true, false);
+    EXPECT_EQ("second-request-id", request_headers.get_(Http::Headers::get().RequestId));
+  }
+
+  {
+    // edge_request: true, keep_external_id: true.
+
+    Http::TestRequestHeaderMapImpl request_headers;
+
+    // Without request ID.
+    EXPECT_CALL(random, uuid()).WillOnce(Return("first-request-id"));
+    uuid_utils.set(request_headers, true, true);
+    EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId));
+
+    // With request ID. Previous one will be kept.
+    EXPECT_CALL(random, uuid()).Times(0);
+    uuid_utils.set(request_headers, true, true);
+    EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId));
+  }
+
+  {
+    // edge_request: false, keep_external_id: false.
+
+    Http::TestRequestHeaderMapImpl request_headers;
+
+    // Without request ID.
+    EXPECT_CALL(random, uuid()).WillOnce(Return("first-request-id"));
+    uuid_utils.set(request_headers, false, false);
+    EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId));
+
+    // With request ID. Previous one will be kept.
+    EXPECT_CALL(random, uuid()).Times(0);
+    uuid_utils.set(request_headers, false, false);
+    EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId));
+  }
 
-  EXPECT_CALL(random, uuid()).WillOnce(Return("second-request-id"));
-  uuid_utils.set(request_headers, true);
-  EXPECT_EQ("second-request-id", request_headers.get_(Http::Headers::get().RequestId));
+  {
+    // edge_request: false, keep_external_id: true.
+
+    Http::TestRequestHeaderMapImpl request_headers;
+
+    // Without request ID.
+    EXPECT_CALL(random, uuid()).WillOnce(Return("first-request-id"));
+    uuid_utils.set(request_headers, false, true);
+    EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId));
+
+    // With request ID. Previous one will be kept.
+    EXPECT_CALL(random, uuid()).Times(0);
+    uuid_utils.set(request_headers, false, true);
+    EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId));
+  }
 }
 
-TEST(UUIDRequestIDExtensionTest, EnsureRequestID) {
-  testing::StrictMock<Random::MockRandomGenerator> random;
+TEST(UUIDRequestIDExtensionTest, ClearExternalTraceReason) {
+  testing::NiceMock<Random::MockRandomGenerator> random;
   UUIDRequestIDExtension uuid_utils(envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(),
                                     random);
-  Http::TestRequestHeaderMapImpl request_headers;
 
-  EXPECT_CALL(random, uuid()).WillOnce(Return("first-request-id"));
-  uuid_utils.set(request_headers, false);
-  EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId));
+  std::string uuid_with_trace_reason = random.uuid_;
+
+  uuid_with_trace_reason[14] = 'b'; // 'b' means TRACE_CLIENT.
+
+  // edge_request: true, keep_external_id: true.
+
+  Http::TestRequestHeaderMapImpl request_headers{{
+      "x-request-id",
+      uuid_with_trace_reason,
+  }};
+
+  std::string expected_uuid_with_trace_reason = uuid_with_trace_reason;
+  expected_uuid_with_trace_reason[14] = '4'; // 'b' means NO_TRACE.
 
   EXPECT_CALL(random, uuid()).Times(0);
-  uuid_utils.set(request_headers, false);
-  EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId));
+  uuid_utils.set(request_headers, true, true);
+
+  // External request ID will be kept but the trace reason will be cleared.
+  EXPECT_EQ(expected_uuid_with_trace_reason, request_headers.get_(Http::Headers::get().RequestId));
 }
 
 TEST(UUIDRequestIDExtensionTest, PreserveRequestIDInResponse) {
@@ -194,15 +260,18 @@ TEST(UUIDRequestIDExtensionTest, SetTraceStatusPackingDisabled) {
   config.mutable_pack_trace_reason()->set_value(false);
   UUIDRequestIDExtension uuid_utils(config, random);
 
+  std::string uuid_with_trace_reason = random.uuid();
+  uuid_with_trace_reason[14] = 'b'; // 'b' means TRACE_CLIENT.
+
   Http::TestRequestHeaderMapImpl request_headers;
-  request_headers.setRequestId(random.uuid());
-  const std::string request_id = std::string(request_headers.getRequestIdValue());
+  request_headers.setRequestId(uuid_with_trace_reason);
+
   EXPECT_EQ(Tracing::Reason::NotTraceable, uuid_utils.getTraceReason(request_headers));
-  EXPECT_EQ(request_id, request_headers.getRequestIdValue());
+  EXPECT_EQ(uuid_with_trace_reason, request_headers.getRequestIdValue());
 
   uuid_utils.setTraceReason(request_headers, Tracing::Reason::Sampling);
   EXPECT_EQ(Tracing::Reason::NotTraceable, uuid_utils.getTraceReason(request_headers));
-  EXPECT_EQ(request_id, request_headers.getRequestIdValue());
+  EXPECT_EQ(uuid_with_trace_reason, request_headers.getRequestIdValue());
 }
 
 } // namespace RequestId