[#6050] http: add edge_accept_request_id field

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

@@ -25,7 +25,7 @@ import "gogoproto/gogo.proto";
 // [#protodoc-title: HTTP connection manager]
 // HTTP connection manager :ref:`configuration overview <config_http_conn_man>`.
 
-// [#comment:next free field: 32]
+// [#comment:next free field: 33]
 message HttpConnectionManager {
   enum CodecType {
     option (gogoproto.goproto_enum_prefix) = false;
@@ -287,6 +287,12 @@ message HttpConnectionManager {
   // is not desired it can be disabled.
   google.protobuf.BoolValue generate_request_id = 15;
 
+  // Whether the connection manager will keep the :ref:`x-request-id
+  // <config_http_conn_man_headers_x-request-id>` header if passed for a request that is edge
+  // (Edge request is the request from external clients to front Envoy) and not reset it, which
+  // is the current Envoy behaviour. This defaults to false.
+  bool preserve_external_request_id = 32;
+
   // How to handle the :ref:`config_http_conn_man_headers_x-forwarded-client-cert` (XFCC) HTTP
   // header.
   enum ForwardClientCertDetails {

docs/root/intro/version_history.rst

@@ -24,6 +24,7 @@ Version history
 * http: fixed a bug where large unbufferable responses were not tracked in stats and logs correctly.
 * http: fixed a crashing bug where gRPC local replies would cause segfaults when upstream access logging was on.
 * http: mitigated a race condition with the :ref:`delayed_close_timeout<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.delayed_close_timeout>` where it could trigger while actively flushing a pending write buffer for a downstream connection.
+* http: added support for :ref:`preserve_external_request_id<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.preserve_external_request_id>` that represents whether the x-request-id should not be reset on edge entry inside mesh
 * http: changed `sendLocalReply` to send percent-encoded `GrpcMessage`.
 * jwt_authn: make filter's parsing of JWT more flexible, allowing syntax like ``jwt=eyJhbGciOiJS...ZFnFIw,extra=7,realm=123``
 * listener: added :ref:`source IP <envoy_api_field_listener.FilterChainMatch.source_prefix_ranges>`

source/common/http/conn_manager_config.h

@@ -213,6 +213,11 @@ class ConnectionManagerConfig {
    */
   virtual bool generateRequestId() PURE;
 
+  /**
+   * @return whether the x-request-id should not be reset on edge entry inside mesh
+   */
+  virtual bool preserveExternalRequestId() const PURE;
+
   /**
    * @return optional idle timeout for incoming connection manager connections.
    */

source/common/http/conn_manager_utility.cc

@@ -206,11 +206,13 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest
   }
 
   // Generate x-request-id for all edge requests, or if there is none.
-  if (config.generateRequestId() && (edge_request || !request_headers.RequestId())) {
+  if (config.generateRequestId()) {
     // TODO(PiotrSikora) PERF: Write UUID directly to the header map.
-    const std::string uuid = random.uuid();
-    ASSERT(!uuid.empty());
-    request_headers.insertRequestId().value(uuid);
+    if ((!config.preserveExternalRequestId() && edge_request) || !request_headers.RequestId()) {
+      const std::string uuid = random.uuid();
+      ASSERT(!uuid.empty());
+      request_headers.insertRequestId().value(uuid);
+    }
   }
 
   mutateXfccRequestHeader(request_headers, connection, config);

source/common/json/config_schemas.cc

@@ -352,7 +352,8 @@ const std::string Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA(R"EOF(
               "enum" : ["Subject", "SAN"]
           }
       },
-      "generate_request_id" : {"type" : "boolean"}
+      "generate_request_id" : {"type" : "boolean"},
+      "preserve_external_request_id" : {"type" : "boolean"}
     },
     "required" : ["codec_type", "stat_prefix", "filters"],
     "additionalProperties" : false

source/extensions/filters/network/http_connection_manager/config.cc

@@ -156,6 +156,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
       request_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, request_timeout, RequestTimeoutMs)),
       drain_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, drain_timeout, 5000)),
       generate_request_id_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, generate_request_id, true)),
+      preserve_external_request_id_(config.preserve_external_request_id()),
       date_provider_(date_provider),
       listener_stats_(Http::ConnectionManagerImpl::generateListenerStats(stats_prefix_,
                                                                          context_.listenerScope())),

source/extensions/filters/network/http_connection_manager/config.h

@@ -102,6 +102,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   std::chrono::milliseconds drainTimeout() override { return drain_timeout_; }
   FilterChainFactory& filterFactory() override { return *this; }
   bool generateRequestId() override { return generate_request_id_; }
+  bool preserveExternalRequestId() const override { return preserve_external_request_id_; }
   uint32_t maxRequestHeadersKb() const override { return max_request_headers_kb_; }
   absl::optional<std::chrono::milliseconds> idleTimeout() const override { return idle_timeout_; }
   std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; }
@@ -173,6 +174,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   Config::ConfigProviderPtr scoped_routes_config_provider_;
   std::chrono::milliseconds drain_timeout_;
   bool generate_request_id_;
+  const bool preserve_external_request_id_;
   Http::DateProvider& date_provider_;
   Http::ConnectionManagerListenerStats listener_stats_;
   const bool proxy_100_continue_;

source/server/http/admin.h

@@ -103,6 +103,7 @@ class AdminImpl : public Admin,
   std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); }
   Http::FilterChainFactory& filterFactory() override { return *this; }
   bool generateRequestId() override { return false; }
+  bool preserveExternalRequestId() const override { return false; }
   absl::optional<std::chrono::milliseconds> idleTimeout() const override { return idle_timeout_; }
   uint32_t maxRequestHeadersKb() const override { return max_request_headers_kb_; }
   std::chrono::milliseconds streamIdleTimeout() const override { return {}; }

test/common/http/conn_manager_impl_fuzz_test.cc

@@ -79,6 +79,7 @@ class FuzzConfig : public ConnectionManagerConfig {
   std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); }
   FilterChainFactory& filterFactory() override { return filter_factory_; }
   bool generateRequestId() override { return true; }
+  bool preserveExternalRequestId() const override { return false; }
   uint32_t maxRequestHeadersKb() const override { return max_request_headers_kb_; }
   absl::optional<std::chrono::milliseconds> idleTimeout() const override { return idle_timeout_; }
   std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; }
@@ -137,6 +138,7 @@ class FuzzConfig : public ConnectionManagerConfig {
   absl::optional<std::string> user_agent_;
   TracingConnectionManagerConfigPtr tracing_config_;
   bool proxy_100_continue_{true};
+  bool preserve_external_request_id_{false};
   Http::Http1Settings http1_settings_;
   Http::DefaultInternalAddressConfig internal_address_config_;
   bool normalize_path_{true};

test/common/http/conn_manager_impl_test.cc

@@ -247,6 +247,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan
   std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); }
   FilterChainFactory& filterFactory() override { return filter_factory_; }
   bool generateRequestId() override { return true; }
+  bool preserveExternalRequestId() const override { return false; }
   uint32_t maxRequestHeadersKb() const override { return max_request_headers_kb_; }
   absl::optional<std::chrono::milliseconds> idleTimeout() const override { return idle_timeout_; }
   std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; }
@@ -322,6 +323,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan
   Stats::IsolatedStoreImpl fake_listener_stats_;
   ConnectionManagerListenerStats listener_stats_;
   bool proxy_100_continue_ = false;
+  bool preserve_external_request_id_ = false;
   Http::Http1Settings http1_settings_;
   bool normalize_path_ = false;
   NiceMock<Network::MockClientConnection> upstream_conn_; // for websocket tests

test/common/http/conn_manager_utility_test.cc

@@ -36,7 +36,10 @@ class MockInternalAddressConfig : public Http::InternalAddressConfig {
 
 class MockConnectionManagerConfig : public ConnectionManagerConfig {
 public:
-  MockConnectionManagerConfig() { ON_CALL(*this, generateRequestId()).WillByDefault(Return(true)); }
+  MockConnectionManagerConfig() {
+    ON_CALL(*this, generateRequestId()).WillByDefault(Return(true));
+    ON_CALL(*this, preserveExternalRequestId()).WillByDefault(Return(false));
+  }
 
   // Http::ConnectionManagerConfig
   ServerConnectionPtr createCodec(Network::Connection& connection, const Buffer::Instance& instance,
@@ -51,6 +54,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig {
   MOCK_METHOD0(drainTimeout, std::chrono::milliseconds());
   MOCK_METHOD0(filterFactory, FilterChainFactory&());
   MOCK_METHOD0(generateRequestId, bool());
+  MOCK_CONST_METHOD0(preserveExternalRequestId, bool());
   MOCK_CONST_METHOD0(maxRequestHeadersKb, uint32_t());
   MOCK_CONST_METHOD0(idleTimeout, absl::optional<std::chrono::milliseconds>());
   MOCK_CONST_METHOD0(streamIdleTimeout, std::chrono::milliseconds());
@@ -1206,5 +1210,87 @@ TEST_F(ConnectionManagerUtilityTest, SanitizePathRelativePAth) {
   EXPECT_EQ(header_map.Path()->value().getStringView(), "/abc");
 }
 
+// test preserve_external_request_id true does not reset the passed requestId if passed
+TEST_F(ConnectionManagerUtilityTest, PreserveExternalRequestId) {
+  connection_.remote_address_ = std::make_shared<Network::Address::Ipv4Instance>("134.2.2.11");
+  ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
+  ON_CALL(config_, preserveExternalRequestId()).WillByDefault(Return(true));
+  TestHeaderMapImpl headers{{"x-request-id", "my-request-id"}, {"x-forwarded-for", "198.51.100.1"}};
+  EXPECT_EQ((MutateRequestRet{"134.2.2.11:0", false}),
+            callMutateRequestHeaders(headers, Protocol::Http2));
+  EXPECT_CALL(random_, uuid()).Times(0);
+  EXPECT_EQ("my-request-id", headers.get_("x-request-id"));
+}
+
+// test preserve_external_request_id true but generates new request id when not passed
+TEST_F(ConnectionManagerUtilityTest, PreseverExternalRequestIdNoReqId) {
+  connection_.remote_address_ = std::make_shared<Network::Address::Ipv4Instance>("134.2.2.11");
+  ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
+  ON_CALL(config_, preserveExternalRequestId()).WillByDefault(Return(true));
+  TestHeaderMapImpl headers{{"x-forwarded-for", "198.51.100.1"}};
+  EXPECT_EQ((MutateRequestRet{"134.2.2.11:0", false}),
+            callMutateRequestHeaders(headers, Protocol::Http2));
+  EXPECT_EQ(random_.uuid_, headers.get_(Headers::get().RequestId));
+}
+
+// test preserve_external_request_id true and no edge_request passing requestId should keep the
+// requestID
+TEST_F(ConnectionManagerUtilityTest, PreserveExternalRequestIdNoEdgeRequestKeepRequestId) {
+  ON_CALL(config_, preserveExternalRequestId()).WillByDefault(Return(true));
+  TestHeaderMapImpl headers{{"x-request-id", "myReqId"}};
+  EXPECT_EQ("myReqId", headers.get_(Headers::get().RequestId));
+}
+
+// test preserve_external_request_id true and no edge_request not passing requestId should generate
+// new request id
+TEST_F(ConnectionManagerUtilityTest, PreserveExternalRequestIdNoEdgeRequestGenerateNewRequestId) {
+  ON_CALL(config_, preserveExternalRequestId()).WillByDefault(Return(true));
+  TestHeaderMapImpl headers;
+  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_F(ConnectionManagerUtilityTest, NoPreserveExternalRequestIdEdgeRequestGenerateRequestId) {
+  ON_CALL(config_, preserveExternalRequestId()).WillByDefault(Return(false));
+  connection_.remote_address_ = std::make_shared<Network::Address::Ipv4Instance>("134.2.2.11");
+  // with request id
+  {
+
+    ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
+    TestHeaderMapImpl headers{{"x-forwarded-for", "198.51.100.1"},
+                              {"x-request-id", "my-request-id"}};
+    EXPECT_EQ((MutateRequestRet{"134.2.2.11:0", false}),
+              callMutateRequestHeaders(headers, Protocol::Http2));
+    EXPECT_EQ(random_.uuid_, headers.get_(Headers::get().RequestId));
+  }
+
+  // with no request id
+  {
+    TestHeaderMapImpl headers{{"x-forwarded-for", "198.51.100.1"}};
+    EXPECT_EQ((MutateRequestRet{"134.2.2.11:0", false}),
+              callMutateRequestHeaders(headers, Protocol::Http2));
+    EXPECT_EQ(random_.uuid_, headers.get_(Headers::get().RequestId));
+  }
+}
+
+// test preserve_external_request_id false not edge request
+TEST_F(ConnectionManagerUtilityTest, NoPreserveExternalRequestIdNoEdgeRequest) {
+  ON_CALL(config_, preserveExternalRequestId()).WillByDefault(Return(false));
+
+  // with no request id
+  {
+    TestHeaderMapImpl headers;
+    callMutateRequestHeaders(headers, Protocol::Http2);
+    EXPECT_EQ(random_.uuid_, headers.get_(Headers::get().RequestId));
+  }
+
+  // with request id
+  {
+    TestHeaderMapImpl headers{{"x-request-id", "my-request-id"}};
+    callMutateRequestHeaders(headers, Protocol::Http2);
+    EXPECT_EQ("my-request-id", headers.get_(Headers::get().RequestId));
+  }
+}
 } // namespace Http
 } // namespace Envoy