router: fixes a regression where headers were not available in request_headers_to_add

changelogs/current.yaml

@@ -82,6 +82,12 @@ bug_fixes:
   change: |
     c-ares resolver: add optional ``reinit_channel_on_timeout`` to reinitialize
     the resolver after DNS timeouts.
+- area: router
+  change: |
+    Fixed a regression where router-set headers (e.g., ``x-envoy-expected-rq-timeout-ms``, ``x-envoy-attempt-count``)
+    were not accessible in ``request_headers_to_add`` configuration on the initial request. Headers configured via
+    ``request_headers_to_add`` can now reference router-set headers using formatters like
+    ``%REQ(x-envoy-expected-rq-timeout-ms)%``.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

source/common/router/config_impl.cc

@@ -906,6 +906,8 @@ void RouteEntryImplBase::finalizeRequestHeaders(Http::RequestHeaderMap& headers,
                                                 const Formatter::Context& context,
                                                 const StreamInfo::StreamInfo& stream_info,
                                                 bool keep_original_host_or_path) const {
+  // Apply header transformations configured via request_headers_to_add first.
+  // This is important because host/path rewriting may depend on headers added here.
   for (const HeaderParser* header_parser : getRequestHeaderParsers(
            /*specificity_ascend=*/vhost_->globalRouteConfig().mostSpecificHeaderMutationsWins())) {
     // Later evaluated header parser wins.

source/common/router/config_impl.h

@@ -612,6 +612,7 @@ class RouteEntryImplBase : public RouteEntryAndRoute,
   void finalizeRequestHeaders(Http::RequestHeaderMap& headers, const Formatter::Context& context,
                               const StreamInfo::StreamInfo& stream_info,
                               bool keep_original_host_or_path) const override;
+
   Http::HeaderTransforms requestHeaderTransforms(const StreamInfo::StreamInfo& stream_info,
                                                  bool do_formatting = true) const override;
   void finalizeResponseHeaders(Http::ResponseHeaderMap& headers, const Formatter::Context& context,

source/common/router/router.cc

@@ -600,7 +600,24 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
   attempt_count_++;
   callbacks_->streamInfo().setAttemptCount(attempt_count_);
 
-  // Finalize the request headers before the host selection.
+  // Set hedging params before finalizeRequestHeaders so timeout calculation is correct.
+  hedging_params_ = FilterUtility::finalHedgingParams(*route_entry_, headers);
+
+  // Calculate timeout and set x-envoy-expected-rq-timeout-ms before finalizeRequestHeaders.
+  // This allows request_headers_to_add to reference the timeout header.
+  timeout_ = FilterUtility::finalTimeout(*route_entry_, headers, !config_->suppress_envoy_headers_,
+                                         grpc_request_, hedging_params_.hedge_on_per_try_timeout_,
+                                         config_->respect_expected_rq_timeout_);
+
+  // Set x-envoy-attempt-count before finalizeRequestHeaders so it can be referenced.
+  include_attempt_count_in_request_ = route_entry_->includeAttemptCountInRequest();
+  if (include_attempt_count_in_request_) {
+    headers.setEnvoyAttemptCount(attempt_count_);
+  }
+
+  // Finalize request headers (host/path rewriting + request_headers_to_add) before host selection.
+  // At this point, router-set headers (x-envoy-expected-rq-timeout-ms, x-envoy-attempt-count)
+  // are already set, so request_headers_to_add can reference them and affect load balancing.
   const Formatter::Context formatter_context(&headers, {}, {}, {}, {}, &callbacks_->activeSpan());
   route_entry_->finalizeRequestHeaders(headers, formatter_context, callbacks_->streamInfo(),
                                        !config_->suppress_envoy_headers_);
@@ -755,12 +772,7 @@ bool Filter::continueDecodeHeaders(Upstream::ThreadLocalCluster* cluster,
     return false;
   }
 
-  hedging_params_ = FilterUtility::finalHedgingParams(*route_entry_, headers);
-
-  timeout_ = FilterUtility::finalTimeout(*route_entry_, headers, !config_->suppress_envoy_headers_,
-                                         grpc_request_, hedging_params_.hedge_on_per_try_timeout_,
-                                         config_->respect_expected_rq_timeout_);
-
+  // Handle additional header processing.
   const Http::HeaderEntry* header_max_stream_duration_entry =
       headers.EnvoyUpstreamStreamDurationMs();
   if (header_max_stream_duration_entry) {
@@ -769,17 +781,12 @@ bool Filter::continueDecodeHeaders(Upstream::ThreadLocalCluster* cluster,
     headers.removeEnvoyUpstreamStreamDurationMs();
   }
 
-  // If this header is set with any value, use an alternate response code on timeout
+  // If this header is set with any value, use an alternate response code on timeout.
   if (headers.EnvoyUpstreamRequestTimeoutAltResponse()) {
     timeout_response_code_ = Http::Code::NoContent;
     headers.removeEnvoyUpstreamRequestTimeoutAltResponse();
   }
 
-  include_attempt_count_in_request_ = route_entry_->includeAttemptCountInRequest();
-  if (include_attempt_count_in_request_) {
-    headers.setEnvoyAttemptCount(attempt_count_);
-  }
-
   FilterUtility::setUpstreamScheme(
       headers, callbacks_->streamInfo().downstreamAddressProvider().sslConnection() != nullptr,
       host->transportSocketFactory().sslCtx() != nullptr,
@@ -2241,7 +2248,7 @@ void Filter::continueDoRetry(bool can_send_early_data, bool can_use_http3,
   }
 
   // The request timeouts only account for time elapsed since the downstream request completed
-  // which might not have happened yet (in which case zero time has elapsed.)
+  // which might not have happened yet, in which case zero time has elapsed.
   std::chrono::milliseconds elapsed_time = std::chrono::milliseconds::zero();
 
   if (DateUtil::timePointValid(downstream_request_complete_time_)) {

test/common/router/config_impl_test.cc

@@ -7812,6 +7812,48 @@ TEST_F(CustomRequestHeadersTest, CustomHeaderWrongFormat) {
                EnvoyException);
 }
 
+// Validate that request_headers_to_add can reference router-set headers like
+// x-envoy-expected-rq-timeout-ms and x-envoy-attempt-count on the initial request.
+TEST_F(CustomRequestHeadersTest, RequestHeadersCanReferenceRouterSetHeaders) {
+  const std::string yaml = R"EOF(
+virtual_hosts:
+- name: www2
+  domains:
+  - www.lyft.com
+  routes:
+  - match:
+      prefix: "/endpoint"
+    route:
+      cluster: www2
+      timeout: 5s
+    request_headers_to_add:
+    - header:
+        key: x-timeout-copy
+        value: "%REQ(x-envoy-expected-rq-timeout-ms)%"
+    - header:
+        key: x-attempt-copy
+        value: "%REQ(x-envoy-attempt-count)%"
+  )EOF";
+  NiceMock<Envoy::StreamInfo::MockStreamInfo> stream_info;
+  factory_context_.cluster_manager_.initializeClusters({"www2"}, {});
+  TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true,
+                        creation_status_);
+  Http::TestRequestHeaderMapImpl headers = genHeaders("www.lyft.com", "/endpoint", "GET");
+  const RouteEntry* route_entry = config.route(headers, 0)->routeEntry();
+
+  // Simulate router filter setting these headers before calling finalizeRequestHeaders.
+  // This mimics the fix where router-set headers are added before finalizeRequestHeaders.
+  headers.addCopy(Http::LowerCaseString("x-envoy-expected-rq-timeout-ms"), "5000");
+  headers.addCopy(Http::LowerCaseString("x-envoy-attempt-count"), "1");
+
+  auto formatter_context = Formatter::Context().setRequestHeaders(headers);
+  route_entry->finalizeRequestHeaders(headers, formatter_context, stream_info, true);
+
+  // Verify that request_headers_to_add was able to reference router-set headers.
+  EXPECT_EQ("5000", headers.get_("x-timeout-copy"));
+  EXPECT_EQ("1", headers.get_("x-attempt-copy"));
+}
+
 TEST(MetadataMatchCriteriaImpl, Create) {
   auto v1 = Protobuf::Value();
   v1.set_string_value("v1");

test/common/router/router_test.cc

@@ -1327,6 +1327,61 @@ TEST_F(RouterTest, EnvoyAttemptCountInResponseNotOverwritten) {
       /* expected_count */ 123);
 }
 
+// Validate that router-set headers like x-envoy-expected-rq-timeout-ms are accessible in
+// request_headers_to_add configuration.
+TEST_F(RouterTest, RouterSetHeadersAccessibleInRequestHeadersToAdd) {
+  NiceMock<Http::MockRequestEncoder> encoder;
+  Http::ResponseDecoder* response_decoder = nullptr;
+
+  EXPECT_CALL(cm_.thread_local_cluster_.conn_pool_, newStream(_, _, _))
+      .WillOnce(
+          Invoke([&](Http::ResponseDecoder& decoder, Http::ConnectionPool::Callbacks& callbacks,
+                     const Http::ConnectionPool::Instance::StreamOptions&)
+                     -> Http::ConnectionPool::Cancellable* {
+            response_decoder = &decoder;
+            callbacks.onPoolReady(encoder, cm_.thread_local_cluster_.conn_pool_.host_,
+                                  upstream_stream_info_, Http::Protocol::Http10);
+            return nullptr;
+          }));
+
+  expectResponseTimerCreate();
+
+  // Set up finalizeRequestHeaders to simulate request_headers_to_add with a reference to
+  // x-envoy-expected-rq-timeout-ms. This will be called AFTER router-set headers are added.
+  EXPECT_CALL(callbacks_.route_->route_entry_, finalizeRequestHeaders(_, _, _, _))
+      .WillOnce(Invoke([](Http::RequestHeaderMap& headers, const Formatter::Context&,
+                          const StreamInfo::StreamInfo&, bool) {
+        // Simulate request_headers_to_add configuration:
+        // - header:
+        //     key: x-timeout
+        //     value: '%REQ(x-envoy-expected-rq-timeout-ms)%'
+        //   append_action: ADD_IF_ABSENT
+        const auto timeout_header =
+            headers.get(Http::LowerCaseString("x-envoy-expected-rq-timeout-ms"));
+        if (!timeout_header.empty()) {
+          headers.addCopy(Http::LowerCaseString("x-timeout"),
+                          timeout_header[0]->value().getStringView());
+        }
+      }));
+
+  Http::TestRequestHeaderMapImpl headers;
+  HttpTestUtility::addDefaultHeaders(headers);
+  router_->decodeHeaders(headers, true);
+
+  // Verify that x-envoy-expected-rq-timeout-ms was set by the router.
+  EXPECT_FALSE(headers.get_("x-envoy-expected-rq-timeout-ms").empty());
+
+  // Verify that our request_headers_to_add logic was able to copy it to x-timeout.
+  // This verifies the fix: finalizeRequestHeaders is called AFTER router-set headers.
+  EXPECT_FALSE(headers.get_("x-timeout").empty());
+  EXPECT_EQ(headers.get_("x-envoy-expected-rq-timeout-ms"), headers.get_("x-timeout"));
+
+  Http::ResponseHeaderMapPtr response_headers(
+      new Http::TestResponseHeaderMapImpl{{":status", "200"}});
+  response_decoder->decodeHeaders(std::move(response_headers), true);
+  EXPECT_TRUE(verifyHostUpstreamStats(1, 0));
+}
+
 // Validate that x-envoy-attempt-count is present in local replies after an upstream attempt is
 // made.
 TEST_F(RouterTest, EnvoyAttemptCountInResponsePresentWithLocalReply) {