From 29127dc4ba8cd6dae095bcf128092555ff61ed42 Mon Sep 17 00:00:00 2001 From: Rohit Agrawal Date: Sat, 1 Nov 2025 00:40:40 -0700 Subject: [PATCH 1/4] router: fixes a regression where headers were not available in request_headers_to_add Signed-off-by: Rohit Agrawal --- changelogs/current.yaml | 5 ++ envoy/router/router.h | 29 ++++++++++ source/common/http/null_route_impl.h | 4 ++ source/common/router/config_impl.cc | 35 ++++++++---- source/common/router/config_impl.h | 25 ++++++++ source/common/router/delegating_route_impl.cc | 14 +++++ source/common/router/delegating_route_impl.h | 6 ++ source/common/router/router.cc | 19 ++++++- test/common/router/router_2_test.cc | 2 +- test/common/router/router_test.cc | 57 ++++++++++++++++++- test/mocks/router/mocks.h | 16 ++++++ 11 files changed, 197 insertions(+), 15 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index ba47153c66eb6..3cb2848764226 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -82,6 +82,11 @@ 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. 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 ` diff --git a/envoy/router/router.h b/envoy/router/router.h index 8fae0877c62a4..f716a8bc80ea2 100644 --- a/envoy/router/router.h +++ b/envoy/router/router.h @@ -964,6 +964,35 @@ class RouteEntry : public ResponseEntry { const StreamInfo::StreamInfo& stream_info, bool keep_original_host_or_path) const PURE; + /** + * Finalizes host and path headers including host rewriting and path rewriting. + * This should be called early in the request processing flow (before host selection). + * This method is part of the implementation of finalizeRequestHeaders, split to allow + * proper ordering of header transformations. + * @param headers supplies the request headers to finalize. + * @param context supplies the formatter context. + * @param stream_info supplies the stream info. + * @param keep_original_host_or_path whether to preserve original host/path in x-envoy-original-* + * headers. + */ + virtual void finalizeHostAndPath(Http::RequestHeaderMap& headers, + const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info, + bool keep_original_host_or_path) const PURE; + + /** + * Applies request header transformations configured via request_headers_to_add. + * This should be called late in the request processing flow (after router-set headers). + * This method is part of the implementation of finalizeRequestHeaders, split to allow + * proper ordering of header transformations. + * @param headers supplies the request headers to transform. + * @param context supplies the formatter context. + * @param stream_info supplies the stream info. + */ + virtual void applyRequestHeaderTransforms(Http::RequestHeaderMap& headers, + const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info) const PURE; + /** * Returns the request header transforms that would be applied if finalizeRequestHeaders were * called now. This is useful if you want to obtain request header transforms which was or will be diff --git a/source/common/http/null_route_impl.h b/source/common/http/null_route_impl.h index 9efc26e281c51..9fa1833866e7d 100644 --- a/source/common/http/null_route_impl.h +++ b/source/common/http/null_route_impl.h @@ -136,6 +136,10 @@ struct RouteEntryImpl : public Router::RouteEntry { } void finalizeRequestHeaders(Http::RequestHeaderMap&, const Formatter::Context&, const StreamInfo::StreamInfo&, bool) const override {} + void finalizeHostAndPath(Http::RequestHeaderMap&, const Formatter::Context&, + const StreamInfo::StreamInfo&, bool) const override {} + void applyRequestHeaderTransforms(Http::RequestHeaderMap&, const Formatter::Context&, + const StreamInfo::StreamInfo&) const override {} Http::HeaderTransforms requestHeaderTransforms(const StreamInfo::StreamInfo&, bool) const override { return {}; diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index fad5054e78f3a..5060e37ce1e7a 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -902,16 +902,10 @@ void RouteEntryImplBase::finalizeHostHeader(Http::RequestHeaderMap& headers, Http::Utility::updateAuthority(headers, hostname, append_xfh_, keep_old_host); } -void RouteEntryImplBase::finalizeRequestHeaders(Http::RequestHeaderMap& headers, - const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info, - bool keep_original_host_or_path) const { - for (const HeaderParser* header_parser : getRequestHeaderParsers( - /*specificity_ascend=*/vhost_->globalRouteConfig().mostSpecificHeaderMutationsWins())) { - // Later evaluated header parser wins. - header_parser->evaluateHeaders(headers, context, stream_info); - } - +void RouteEntryImplBase::finalizeHostAndPath(Http::RequestHeaderMap& headers, + const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info, + bool keep_original_host_or_path) const { // Restore the port if this was a CONNECT request. // Note this will restore the port for HTTP/2 CONNECT-upgrades as well as as HTTP/1.1 style // CONNECT requests. @@ -930,6 +924,27 @@ void RouteEntryImplBase::finalizeRequestHeaders(Http::RequestHeaderMap& headers, finalizePathHeader(headers, context, stream_info, keep_original_host_or_path); } +void RouteEntryImplBase::applyRequestHeaderTransforms( + Http::RequestHeaderMap& headers, const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info) const { + for (const HeaderParser* header_parser : getRequestHeaderParsers( + /*specificity_ascend=*/vhost_->globalRouteConfig().mostSpecificHeaderMutationsWins())) { + // Later evaluated header parser wins. + header_parser->evaluateHeaders(headers, context, stream_info); + } +} + +void RouteEntryImplBase::finalizeRequestHeaders(Http::RequestHeaderMap& headers, + const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info, + bool keep_original_host_or_path) const { + // Apply host and path transformations first. + finalizeHostAndPath(headers, context, stream_info, keep_original_host_or_path); + + // Then apply header transformations configured via request_headers_to_add. + applyRequestHeaderTransforms(headers, context, stream_info); +} + void RouteEntryImplBase::finalizeResponseHeaders(Http::ResponseHeaderMap& headers, const Formatter::Context& context, const StreamInfo::StreamInfo& stream_info) const { diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 9b3275cc73dfe..c91468cabaa70 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -612,6 +612,31 @@ 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; + + /** + * Finalizes host and path headers including host rewriting and path rewriting. + * This should be called early in the request processing flow (before host selection). + * @param headers supplies the request headers to finalize. + * @param context supplies the formatter context. + * @param stream_info supplies the stream info. + * @param keep_original_host_or_path whether to preserve original host/path in x-envoy-original-* + * headers. + */ + void finalizeHostAndPath(Http::RequestHeaderMap& headers, const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info, + bool keep_original_host_or_path) const override; + + /** + * Applies request header transformations configured via request_headers_to_add. + * This should be called late in the request processing flow (after router-set headers). + * @param headers supplies the request headers to transform. + * @param context supplies the formatter context. + * @param stream_info supplies the stream info. + */ + void applyRequestHeaderTransforms(Http::RequestHeaderMap& headers, + const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info) 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, diff --git a/source/common/router/delegating_route_impl.cc b/source/common/router/delegating_route_impl.cc index 9f6e938e0c189..ce5181efe97e9 100644 --- a/source/common/router/delegating_route_impl.cc +++ b/source/common/router/delegating_route_impl.cc @@ -43,6 +43,20 @@ void DelegatingRouteEntry::finalizeRequestHeaders(Http::RequestHeaderMap& header insert_envoy_original_path); } +void DelegatingRouteEntry::finalizeHostAndPath(Http::RequestHeaderMap& headers, + const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info, + bool keep_original_host_or_path) const { + return base_route_entry_->finalizeHostAndPath(headers, context, stream_info, + keep_original_host_or_path); +} + +void DelegatingRouteEntry::applyRequestHeaderTransforms( + Http::RequestHeaderMap& headers, const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info) const { + return base_route_entry_->applyRequestHeaderTransforms(headers, context, stream_info); +} + Http::HeaderTransforms DelegatingRouteEntry::requestHeaderTransforms(const StreamInfo::StreamInfo& stream_info, bool do_formatting) const { diff --git a/source/common/router/delegating_route_impl.h b/source/common/router/delegating_route_impl.h index 7ee4ab7c57050..e40e72515102d 100644 --- a/source/common/router/delegating_route_impl.h +++ b/source/common/router/delegating_route_impl.h @@ -94,6 +94,12 @@ class DelegatingRouteEntry : public DelegatingRouteBase { void finalizeRequestHeaders(Http::RequestHeaderMap& headers, const Formatter::Context& context, const StreamInfo::StreamInfo& stream_info, bool insert_envoy_original_path) const override; + void finalizeHostAndPath(Http::RequestHeaderMap& headers, const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info, + bool keep_original_host_or_path) const override; + void applyRequestHeaderTransforms(Http::RequestHeaderMap& headers, + const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info) const override; Http::HeaderTransforms requestHeaderTransforms(const StreamInfo::StreamInfo& stream_info, bool do_formatting = true) const override; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 686989d10a8d6..a1c24ea9d154e 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -600,10 +600,11 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, attempt_count_++; callbacks_->streamInfo().setAttemptCount(attempt_count_); - // Finalize the request headers before the host selection. + // Finalize host and path headers before host selection. + // Note: request_headers_to_add will be applied later after router-set headers are added. const Formatter::Context formatter_context(&headers, {}, {}, {}, {}, &callbacks_->activeSpan()); - route_entry_->finalizeRequestHeaders(headers, formatter_context, callbacks_->streamInfo(), - !config_->suppress_envoy_headers_); + route_entry_->finalizeHostAndPath(headers, formatter_context, callbacks_->streamInfo(), + !config_->suppress_envoy_headers_); // Fetch a connection pool for the upstream cluster. const auto& upstream_http_protocol_options = cluster_->upstreamHttpProtocolOptions(); @@ -780,6 +781,12 @@ bool Filter::continueDecodeHeaders(Upstream::ThreadLocalCluster* cluster, headers.setEnvoyAttemptCount(attempt_count_); } + // Apply request_headers_to_add transformations now that router-set headers are present. + const Formatter::Context header_transform_context(&headers, {}, {}, {}, {}, + &callbacks_->activeSpan()); + route_entry_->applyRequestHeaderTransforms(headers, header_transform_context, + callbacks_->streamInfo()); + FilterUtility::setUpstreamScheme( headers, callbacks_->streamInfo().downstreamAddressProvider().sslConnection() != nullptr, host->transportSocketFactory().sslCtx() != nullptr, @@ -2254,6 +2261,12 @@ void Filter::continueDoRetry(bool can_send_early_data, bool can_use_http3, *downstream_headers_, !config_->suppress_envoy_headers_, grpc_request_, hedging_params_.hedge_on_per_try_timeout_); + // Apply request_headers_to_add transformations now that all router-set headers are present. + const Formatter::Context header_transform_context(downstream_headers_, {}, {}, {}, {}, + &callbacks_->activeSpan()); + route_entry_->applyRequestHeaderTransforms(*downstream_headers_, header_transform_context, + callbacks_->streamInfo()); + UpstreamRequest* upstream_request_tmp = upstream_request.get(); LinkedList::moveIntoList(std::move(upstream_request), upstream_requests_); upstream_requests_.front()->acceptHeadersFromRouter( diff --git a/test/common/router/router_2_test.cc b/test/common/router/router_2_test.cc index 3d53a3e69e105..0fe7a78cafd48 100644 --- a/test/common/router/router_2_test.cc +++ b/test/common/router/router_2_test.cc @@ -30,7 +30,7 @@ TEST_F(RouterTestSuppressEnvoyHeaders, Http1Upstream) { Http::TestRequestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); - EXPECT_CALL(callbacks_.route_->route_entry_, finalizeRequestHeaders(_, _, _, false)) + EXPECT_CALL(callbacks_.route_->route_entry_, finalizeHostAndPath(_, _, _, false)) .WillOnce(Invoke([this](Http::RequestHeaderMap& headers, const Formatter::Context& context, const StreamInfo::StreamInfo&, bool) { EXPECT_EQ(context.requestHeaders().ptr(), &headers); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 256e8591f544d..f074f73442b25 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -476,7 +476,7 @@ TEST_F(RouterTest, Http1Upstream) { Http::TestRequestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); - EXPECT_CALL(callbacks_.route_->route_entry_, finalizeRequestHeaders(_, _, _, true)) + EXPECT_CALL(callbacks_.route_->route_entry_, finalizeHostAndPath(_, _, _, true)) .WillOnce(Invoke([this](Http::RequestHeaderMap& headers, const Formatter::Context& context, const StreamInfo::StreamInfo&, bool) { EXPECT_EQ(context.requestHeaders().ptr(), &headers); @@ -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 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 applyRequestHeaderTransforms 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_, applyRequestHeaderTransforms(_, _, _)) + .WillOnce(Invoke([](Http::RequestHeaderMap& headers, const Formatter::Context&, + const StreamInfo::StreamInfo&) { + // 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: applyRequestHeaderTransforms 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) { diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 930ce50ecf831..4427c67ef1831 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -421,6 +421,14 @@ class MockRouteEntry : public RouteEntry { (Http::RequestHeaderMap & headers, const Formatter::Context& context, const StreamInfo::StreamInfo& stream_info, bool insert_envoy_original_path), (const)); + MOCK_METHOD(void, finalizeHostAndPath, + (Http::RequestHeaderMap & headers, const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info, bool keep_original_host_or_path), + (const)); + MOCK_METHOD(void, applyRequestHeaderTransforms, + (Http::RequestHeaderMap & headers, const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info), + (const)); MOCK_METHOD(Http::HeaderTransforms, requestHeaderTransforms, (const StreamInfo::StreamInfo& stream_info, bool do_formatting), (const)); MOCK_METHOD(void, finalizeResponseHeaders, @@ -546,6 +554,14 @@ class MockRoute : public RouteEntryAndRoute { (Http::RequestHeaderMap & headers, const Formatter::Context& context, const StreamInfo::StreamInfo& stream_info, bool insert_envoy_original_path), (const)); + MOCK_METHOD(void, finalizeHostAndPath, + (Http::RequestHeaderMap & headers, const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info, bool keep_original_host_or_path), + (const)); + MOCK_METHOD(void, applyRequestHeaderTransforms, + (Http::RequestHeaderMap & headers, const Formatter::Context& context, + const StreamInfo::StreamInfo& stream_info), + (const)); MOCK_METHOD(Http::HeaderTransforms, requestHeaderTransforms, (const StreamInfo::StreamInfo& stream_info, bool do_formatting), (const)); MOCK_METHOD(void, finalizeResponseHeaders, From 00ae4c58dfad199153ecd6de4acd4e2ed5887674 Mon Sep 17 00:00:00 2001 From: Rohit Agrawal Date: Sun, 2 Nov 2025 10:48:26 -0800 Subject: [PATCH 2/4] addressed comments from @wbpcode Signed-off-by: Rohit Agrawal --- envoy/router/router.h | 29 ----- source/common/http/null_route_impl.h | 4 - source/common/router/config_impl.cc | 37 ++---- source/common/router/config_impl.h | 24 ---- source/common/router/delegating_route_impl.cc | 14 --- source/common/router/delegating_route_impl.h | 6 - source/common/router/router.cc | 105 +++++++++--------- test/common/router/config_impl_test.cc | 81 ++++++++++++++ test/common/router/router_2_test.cc | 2 +- test/common/router/router_test.cc | 10 +- test/mocks/router/mocks.h | 16 --- 11 files changed, 152 insertions(+), 176 deletions(-) diff --git a/envoy/router/router.h b/envoy/router/router.h index f716a8bc80ea2..8fae0877c62a4 100644 --- a/envoy/router/router.h +++ b/envoy/router/router.h @@ -964,35 +964,6 @@ class RouteEntry : public ResponseEntry { const StreamInfo::StreamInfo& stream_info, bool keep_original_host_or_path) const PURE; - /** - * Finalizes host and path headers including host rewriting and path rewriting. - * This should be called early in the request processing flow (before host selection). - * This method is part of the implementation of finalizeRequestHeaders, split to allow - * proper ordering of header transformations. - * @param headers supplies the request headers to finalize. - * @param context supplies the formatter context. - * @param stream_info supplies the stream info. - * @param keep_original_host_or_path whether to preserve original host/path in x-envoy-original-* - * headers. - */ - virtual void finalizeHostAndPath(Http::RequestHeaderMap& headers, - const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info, - bool keep_original_host_or_path) const PURE; - - /** - * Applies request header transformations configured via request_headers_to_add. - * This should be called late in the request processing flow (after router-set headers). - * This method is part of the implementation of finalizeRequestHeaders, split to allow - * proper ordering of header transformations. - * @param headers supplies the request headers to transform. - * @param context supplies the formatter context. - * @param stream_info supplies the stream info. - */ - virtual void applyRequestHeaderTransforms(Http::RequestHeaderMap& headers, - const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info) const PURE; - /** * Returns the request header transforms that would be applied if finalizeRequestHeaders were * called now. This is useful if you want to obtain request header transforms which was or will be diff --git a/source/common/http/null_route_impl.h b/source/common/http/null_route_impl.h index 9fa1833866e7d..9efc26e281c51 100644 --- a/source/common/http/null_route_impl.h +++ b/source/common/http/null_route_impl.h @@ -136,10 +136,6 @@ struct RouteEntryImpl : public Router::RouteEntry { } void finalizeRequestHeaders(Http::RequestHeaderMap&, const Formatter::Context&, const StreamInfo::StreamInfo&, bool) const override {} - void finalizeHostAndPath(Http::RequestHeaderMap&, const Formatter::Context&, - const StreamInfo::StreamInfo&, bool) const override {} - void applyRequestHeaderTransforms(Http::RequestHeaderMap&, const Formatter::Context&, - const StreamInfo::StreamInfo&) const override {} Http::HeaderTransforms requestHeaderTransforms(const StreamInfo::StreamInfo&, bool) const override { return {}; diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 5060e37ce1e7a..d0697bdc3002c 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -902,10 +902,18 @@ void RouteEntryImplBase::finalizeHostHeader(Http::RequestHeaderMap& headers, Http::Utility::updateAuthority(headers, hostname, append_xfh_, keep_old_host); } -void RouteEntryImplBase::finalizeHostAndPath(Http::RequestHeaderMap& headers, - const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info, - bool keep_original_host_or_path) const { +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. + header_parser->evaluateHeaders(headers, context, stream_info); + } + // Restore the port if this was a CONNECT request. // Note this will restore the port for HTTP/2 CONNECT-upgrades as well as as HTTP/1.1 style // CONNECT requests. @@ -924,27 +932,6 @@ void RouteEntryImplBase::finalizeHostAndPath(Http::RequestHeaderMap& headers, finalizePathHeader(headers, context, stream_info, keep_original_host_or_path); } -void RouteEntryImplBase::applyRequestHeaderTransforms( - Http::RequestHeaderMap& headers, const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info) const { - for (const HeaderParser* header_parser : getRequestHeaderParsers( - /*specificity_ascend=*/vhost_->globalRouteConfig().mostSpecificHeaderMutationsWins())) { - // Later evaluated header parser wins. - header_parser->evaluateHeaders(headers, context, stream_info); - } -} - -void RouteEntryImplBase::finalizeRequestHeaders(Http::RequestHeaderMap& headers, - const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info, - bool keep_original_host_or_path) const { - // Apply host and path transformations first. - finalizeHostAndPath(headers, context, stream_info, keep_original_host_or_path); - - // Then apply header transformations configured via request_headers_to_add. - applyRequestHeaderTransforms(headers, context, stream_info); -} - void RouteEntryImplBase::finalizeResponseHeaders(Http::ResponseHeaderMap& headers, const Formatter::Context& context, const StreamInfo::StreamInfo& stream_info) const { diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index c91468cabaa70..6b32a55175b93 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -613,30 +613,6 @@ class RouteEntryImplBase : public RouteEntryAndRoute, const StreamInfo::StreamInfo& stream_info, bool keep_original_host_or_path) const override; - /** - * Finalizes host and path headers including host rewriting and path rewriting. - * This should be called early in the request processing flow (before host selection). - * @param headers supplies the request headers to finalize. - * @param context supplies the formatter context. - * @param stream_info supplies the stream info. - * @param keep_original_host_or_path whether to preserve original host/path in x-envoy-original-* - * headers. - */ - void finalizeHostAndPath(Http::RequestHeaderMap& headers, const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info, - bool keep_original_host_or_path) const override; - - /** - * Applies request header transformations configured via request_headers_to_add. - * This should be called late in the request processing flow (after router-set headers). - * @param headers supplies the request headers to transform. - * @param context supplies the formatter context. - * @param stream_info supplies the stream info. - */ - void applyRequestHeaderTransforms(Http::RequestHeaderMap& headers, - const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info) 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, diff --git a/source/common/router/delegating_route_impl.cc b/source/common/router/delegating_route_impl.cc index ce5181efe97e9..9f6e938e0c189 100644 --- a/source/common/router/delegating_route_impl.cc +++ b/source/common/router/delegating_route_impl.cc @@ -43,20 +43,6 @@ void DelegatingRouteEntry::finalizeRequestHeaders(Http::RequestHeaderMap& header insert_envoy_original_path); } -void DelegatingRouteEntry::finalizeHostAndPath(Http::RequestHeaderMap& headers, - const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info, - bool keep_original_host_or_path) const { - return base_route_entry_->finalizeHostAndPath(headers, context, stream_info, - keep_original_host_or_path); -} - -void DelegatingRouteEntry::applyRequestHeaderTransforms( - Http::RequestHeaderMap& headers, const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info) const { - return base_route_entry_->applyRequestHeaderTransforms(headers, context, stream_info); -} - Http::HeaderTransforms DelegatingRouteEntry::requestHeaderTransforms(const StreamInfo::StreamInfo& stream_info, bool do_formatting) const { diff --git a/source/common/router/delegating_route_impl.h b/source/common/router/delegating_route_impl.h index e40e72515102d..7ee4ab7c57050 100644 --- a/source/common/router/delegating_route_impl.h +++ b/source/common/router/delegating_route_impl.h @@ -94,12 +94,6 @@ class DelegatingRouteEntry : public DelegatingRouteBase { void finalizeRequestHeaders(Http::RequestHeaderMap& headers, const Formatter::Context& context, const StreamInfo::StreamInfo& stream_info, bool insert_envoy_original_path) const override; - void finalizeHostAndPath(Http::RequestHeaderMap& headers, const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info, - bool keep_original_host_or_path) const override; - void applyRequestHeaderTransforms(Http::RequestHeaderMap& headers, - const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info) const override; Http::HeaderTransforms requestHeaderTransforms(const StreamInfo::StreamInfo& stream_info, bool do_formatting = true) const override; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index a1c24ea9d154e..ae42ff0da6cb6 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -600,11 +600,27 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, attempt_count_++; callbacks_->streamInfo().setAttemptCount(attempt_count_); - // Finalize host and path headers before host selection. - // Note: request_headers_to_add will be applied later after router-set headers are added. + // 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_->finalizeHostAndPath(headers, formatter_context, callbacks_->streamInfo(), - !config_->suppress_envoy_headers_); + route_entry_->finalizeRequestHeaders(headers, formatter_context, callbacks_->streamInfo(), + !config_->suppress_envoy_headers_); // Fetch a connection pool for the upstream cluster. const auto& upstream_http_protocol_options = cluster_->upstreamHttpProtocolOptions(); @@ -756,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) { @@ -770,23 +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_); - } - - // Apply request_headers_to_add transformations now that router-set headers are present. - const Formatter::Context header_transform_context(&headers, {}, {}, {}, {}, - &callbacks_->activeSpan()); - route_entry_->applyRequestHeaderTransforms(headers, header_transform_context, - callbacks_->streamInfo()); - FilterUtility::setUpstreamScheme( headers, callbacks_->streamInfo().downstreamAddressProvider().sslConnection() != nullptr, host->transportSocketFactory().sslCtx() != nullptr, @@ -2193,6 +2193,35 @@ void Filter::doRetry(bool can_send_early_data, bool can_use_http3, TimeoutRetry return; } + // Set router-set headers before host selection so request_headers_to_add can reference them. + if (include_attempt_count_in_request_) { + downstream_headers_->setEnvoyAttemptCount(attempt_count_); + } + + if (include_timeout_retry_header_in_request_) { + downstream_headers_->setEnvoyIsTimeoutRetry(is_timeout_retry == TimeoutRetry::Yes ? "true" + : "false"); + } + + // Update timeout headers for the retry attempt. + std::chrono::milliseconds elapsed_time = std::chrono::milliseconds::zero(); + if (DateUtil::timePointValid(downstream_request_complete_time_)) { + Event::Dispatcher& dispatcher = callbacks_->dispatcher(); + elapsed_time = std::chrono::duration_cast( + dispatcher.timeSource().monotonicTime() - downstream_request_complete_time_); + } + + FilterUtility::setTimeoutHeaders(elapsed_time.count(), timeout_, *route_entry_, + *downstream_headers_, !config_->suppress_envoy_headers_, + grpc_request_, hedging_params_.hedge_on_per_try_timeout_); + + // Apply request_headers_to_add now that router-set headers are present. + const Formatter::Context header_transform_context(downstream_headers_, {}, {}, {}, {}, + &callbacks_->activeSpan()); + route_entry_->finalizeRequestHeaders(*downstream_headers_, header_transform_context, + callbacks_->streamInfo(), + !config_->suppress_envoy_headers_); + callbacks_->streamInfo().downstreamTiming().setValue( "envoy.router.host_selection_start_ms", callbacks_->dispatcher().timeSource().monotonicTime()); @@ -2223,7 +2252,8 @@ void Filter::doRetry(bool can_send_early_data, bool can_use_http3, TimeoutRetry } void Filter::continueDoRetry(bool can_send_early_data, bool can_use_http3, - TimeoutRetry is_timeout_retry, Upstream::HostConstSharedPtr&& host, + TimeoutRetry is_timeout_retry [[maybe_unused]], + Upstream::HostConstSharedPtr&& host, Upstream::ThreadLocalCluster& cluster, absl::optional host_selection_details) { callbacks_->streamInfo().downstreamTiming().setValue( @@ -2238,35 +2268,6 @@ void Filter::continueDoRetry(bool can_send_early_data, bool can_use_http3, *this, std::move(generic_conn_pool), can_send_early_data, can_use_http3, allow_multiplexed_upstream_half_close_ /*enable_half_close*/); - if (include_attempt_count_in_request_) { - downstream_headers_->setEnvoyAttemptCount(attempt_count_); - } - - if (include_timeout_retry_header_in_request_) { - downstream_headers_->setEnvoyIsTimeoutRetry(is_timeout_retry == TimeoutRetry::Yes ? "true" - : "false"); - } - - // 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.) - std::chrono::milliseconds elapsed_time = std::chrono::milliseconds::zero(); - - if (DateUtil::timePointValid(downstream_request_complete_time_)) { - Event::Dispatcher& dispatcher = callbacks_->dispatcher(); - elapsed_time = std::chrono::duration_cast( - dispatcher.timeSource().monotonicTime() - downstream_request_complete_time_); - } - - FilterUtility::setTimeoutHeaders(elapsed_time.count(), timeout_, *route_entry_, - *downstream_headers_, !config_->suppress_envoy_headers_, - grpc_request_, hedging_params_.hedge_on_per_try_timeout_); - - // Apply request_headers_to_add transformations now that all router-set headers are present. - const Formatter::Context header_transform_context(downstream_headers_, {}, {}, {}, {}, - &callbacks_->activeSpan()); - route_entry_->applyRequestHeaderTransforms(*downstream_headers_, header_transform_context, - callbacks_->streamInfo()); - UpstreamRequest* upstream_request_tmp = upstream_request.get(); LinkedList::moveIntoList(std::move(upstream_request), upstream_requests_); upstream_requests_.front()->acceptHeadersFromRouter( diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index c352745ab47b0..7eb36e9ef3833 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -7812,6 +7812,87 @@ 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. +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 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 request_headers_to_add. + 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")); +} + +// Validate that request_headers_to_add can reference router-set headers in retry scenarios. +TEST_F(CustomRequestHeadersTest, RequestHeadersCanReferenceRouterSetHeadersOnRetry) { + const std::string yaml = R"EOF( +virtual_hosts: +- name: www2 + domains: + - www.lyft.com + routes: + - match: + prefix: "/endpoint" + route: + cluster: www2 + timeout: 5s + retry_policy: + retry_on: "5xx" + num_retries: 3 + request_headers_to_add: + - header: + key: x-retry-attempt + value: "Attempt %REQ(x-envoy-attempt-count)% with timeout %REQ(x-envoy-expected-rq-timeout-ms)%ms" + )EOF"; + NiceMock 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 retry attempt where attempt count is incremented. + headers.addCopy(Http::LowerCaseString("x-envoy-expected-rq-timeout-ms"), "4500"); + headers.addCopy(Http::LowerCaseString("x-envoy-attempt-count"), "2"); + + auto formatter_context = Formatter::Context().setRequestHeaders(headers); + route_entry->finalizeRequestHeaders(headers, formatter_context, stream_info, true); + + // Verify that request_headers_to_add properly formatted the composite string. + EXPECT_EQ("Attempt 2 with timeout 4500ms", headers.get_("x-retry-attempt")); +} + TEST(MetadataMatchCriteriaImpl, Create) { auto v1 = Protobuf::Value(); v1.set_string_value("v1"); diff --git a/test/common/router/router_2_test.cc b/test/common/router/router_2_test.cc index 0fe7a78cafd48..3d53a3e69e105 100644 --- a/test/common/router/router_2_test.cc +++ b/test/common/router/router_2_test.cc @@ -30,7 +30,7 @@ TEST_F(RouterTestSuppressEnvoyHeaders, Http1Upstream) { Http::TestRequestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); - EXPECT_CALL(callbacks_.route_->route_entry_, finalizeHostAndPath(_, _, _, false)) + EXPECT_CALL(callbacks_.route_->route_entry_, finalizeRequestHeaders(_, _, _, false)) .WillOnce(Invoke([this](Http::RequestHeaderMap& headers, const Formatter::Context& context, const StreamInfo::StreamInfo&, bool) { EXPECT_EQ(context.requestHeaders().ptr(), &headers); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index f074f73442b25..187137b2d6940 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -476,7 +476,7 @@ TEST_F(RouterTest, Http1Upstream) { Http::TestRequestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); - EXPECT_CALL(callbacks_.route_->route_entry_, finalizeHostAndPath(_, _, _, true)) + EXPECT_CALL(callbacks_.route_->route_entry_, finalizeRequestHeaders(_, _, _, true)) .WillOnce(Invoke([this](Http::RequestHeaderMap& headers, const Formatter::Context& context, const StreamInfo::StreamInfo&, bool) { EXPECT_EQ(context.requestHeaders().ptr(), &headers); @@ -1346,11 +1346,11 @@ TEST_F(RouterTest, RouterSetHeadersAccessibleInRequestHeadersToAdd) { expectResponseTimerCreate(); - // Set up applyRequestHeaderTransforms to simulate request_headers_to_add with a reference to + // 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_, applyRequestHeaderTransforms(_, _, _)) + EXPECT_CALL(callbacks_.route_->route_entry_, finalizeRequestHeaders(_, _, _, _)) .WillOnce(Invoke([](Http::RequestHeaderMap& headers, const Formatter::Context&, - const StreamInfo::StreamInfo&) { + const StreamInfo::StreamInfo&, bool) { // Simulate request_headers_to_add configuration: // - header: // key: x-timeout @@ -1372,7 +1372,7 @@ TEST_F(RouterTest, RouterSetHeadersAccessibleInRequestHeadersToAdd) { 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: applyRequestHeaderTransforms is called AFTER router-set headers. + // 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")); diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 4427c67ef1831..930ce50ecf831 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -421,14 +421,6 @@ class MockRouteEntry : public RouteEntry { (Http::RequestHeaderMap & headers, const Formatter::Context& context, const StreamInfo::StreamInfo& stream_info, bool insert_envoy_original_path), (const)); - MOCK_METHOD(void, finalizeHostAndPath, - (Http::RequestHeaderMap & headers, const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info, bool keep_original_host_or_path), - (const)); - MOCK_METHOD(void, applyRequestHeaderTransforms, - (Http::RequestHeaderMap & headers, const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info), - (const)); MOCK_METHOD(Http::HeaderTransforms, requestHeaderTransforms, (const StreamInfo::StreamInfo& stream_info, bool do_formatting), (const)); MOCK_METHOD(void, finalizeResponseHeaders, @@ -554,14 +546,6 @@ class MockRoute : public RouteEntryAndRoute { (Http::RequestHeaderMap & headers, const Formatter::Context& context, const StreamInfo::StreamInfo& stream_info, bool insert_envoy_original_path), (const)); - MOCK_METHOD(void, finalizeHostAndPath, - (Http::RequestHeaderMap & headers, const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info, bool keep_original_host_or_path), - (const)); - MOCK_METHOD(void, applyRequestHeaderTransforms, - (Http::RequestHeaderMap & headers, const Formatter::Context& context, - const StreamInfo::StreamInfo& stream_info), - (const)); MOCK_METHOD(Http::HeaderTransforms, requestHeaderTransforms, (const StreamInfo::StreamInfo& stream_info, bool do_formatting), (const)); MOCK_METHOD(void, finalizeResponseHeaders, From 310382de895a382240a86367cc374bd14c4a3c3f Mon Sep 17 00:00:00 2001 From: Rohit Agrawal Date: Sun, 2 Nov 2025 10:51:25 -0800 Subject: [PATCH 3/4] format Signed-off-by: Rohit Agrawal --- source/common/router/router.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index ae42ff0da6cb6..27c0945d085a2 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -2200,7 +2200,7 @@ void Filter::doRetry(bool can_send_early_data, bool can_use_http3, TimeoutRetry if (include_timeout_retry_header_in_request_) { downstream_headers_->setEnvoyIsTimeoutRetry(is_timeout_retry == TimeoutRetry::Yes ? "true" - : "false"); + : "false"); } // Update timeout headers for the retry attempt. @@ -2217,10 +2217,9 @@ void Filter::doRetry(bool can_send_early_data, bool can_use_http3, TimeoutRetry // Apply request_headers_to_add now that router-set headers are present. const Formatter::Context header_transform_context(downstream_headers_, {}, {}, {}, {}, - &callbacks_->activeSpan()); + &callbacks_->activeSpan()); route_entry_->finalizeRequestHeaders(*downstream_headers_, header_transform_context, - callbacks_->streamInfo(), - !config_->suppress_envoy_headers_); + callbacks_->streamInfo(), !config_->suppress_envoy_headers_); callbacks_->streamInfo().downstreamTiming().setValue( "envoy.router.host_selection_start_ms", From d724880457f1b8607e71260c11bd8a86ec49676f Mon Sep 17 00:00:00 2001 From: Rohit Agrawal Date: Sun, 2 Nov 2025 22:31:53 -0800 Subject: [PATCH 4/4] addressed comments from @wbpcode Signed-off-by: Rohit Agrawal --- changelogs/current.yaml | 5 ++- source/common/router/router.cc | 54 ++++++++++++-------------- test/common/router/config_impl_test.cc | 43 +------------------- 3 files changed, 29 insertions(+), 73 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 0534dc9e0a636..17cf907224caa 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -85,8 +85,9 @@ bug_fixes: - 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. Headers configured via ``request_headers_to_add`` - can now reference router-set headers using formatters like ``%REQ(x-envoy-expected-rq-timeout-ms)%``. + 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 ` diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 27c0945d085a2..07bfede7fbfa7 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -2193,34 +2193,6 @@ void Filter::doRetry(bool can_send_early_data, bool can_use_http3, TimeoutRetry return; } - // Set router-set headers before host selection so request_headers_to_add can reference them. - if (include_attempt_count_in_request_) { - downstream_headers_->setEnvoyAttemptCount(attempt_count_); - } - - if (include_timeout_retry_header_in_request_) { - downstream_headers_->setEnvoyIsTimeoutRetry(is_timeout_retry == TimeoutRetry::Yes ? "true" - : "false"); - } - - // Update timeout headers for the retry attempt. - std::chrono::milliseconds elapsed_time = std::chrono::milliseconds::zero(); - if (DateUtil::timePointValid(downstream_request_complete_time_)) { - Event::Dispatcher& dispatcher = callbacks_->dispatcher(); - elapsed_time = std::chrono::duration_cast( - dispatcher.timeSource().monotonicTime() - downstream_request_complete_time_); - } - - FilterUtility::setTimeoutHeaders(elapsed_time.count(), timeout_, *route_entry_, - *downstream_headers_, !config_->suppress_envoy_headers_, - grpc_request_, hedging_params_.hedge_on_per_try_timeout_); - - // Apply request_headers_to_add now that router-set headers are present. - const Formatter::Context header_transform_context(downstream_headers_, {}, {}, {}, {}, - &callbacks_->activeSpan()); - route_entry_->finalizeRequestHeaders(*downstream_headers_, header_transform_context, - callbacks_->streamInfo(), !config_->suppress_envoy_headers_); - callbacks_->streamInfo().downstreamTiming().setValue( "envoy.router.host_selection_start_ms", callbacks_->dispatcher().timeSource().monotonicTime()); @@ -2251,8 +2223,7 @@ void Filter::doRetry(bool can_send_early_data, bool can_use_http3, TimeoutRetry } void Filter::continueDoRetry(bool can_send_early_data, bool can_use_http3, - TimeoutRetry is_timeout_retry [[maybe_unused]], - Upstream::HostConstSharedPtr&& host, + TimeoutRetry is_timeout_retry, Upstream::HostConstSharedPtr&& host, Upstream::ThreadLocalCluster& cluster, absl::optional host_selection_details) { callbacks_->streamInfo().downstreamTiming().setValue( @@ -2267,6 +2238,29 @@ void Filter::continueDoRetry(bool can_send_early_data, bool can_use_http3, *this, std::move(generic_conn_pool), can_send_early_data, can_use_http3, allow_multiplexed_upstream_half_close_ /*enable_half_close*/); + if (include_attempt_count_in_request_) { + downstream_headers_->setEnvoyAttemptCount(attempt_count_); + } + + if (include_timeout_retry_header_in_request_) { + downstream_headers_->setEnvoyIsTimeoutRetry(is_timeout_retry == TimeoutRetry::Yes ? "true" + : "false"); + } + + // 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. + std::chrono::milliseconds elapsed_time = std::chrono::milliseconds::zero(); + + if (DateUtil::timePointValid(downstream_request_complete_time_)) { + Event::Dispatcher& dispatcher = callbacks_->dispatcher(); + elapsed_time = std::chrono::duration_cast( + dispatcher.timeSource().monotonicTime() - downstream_request_complete_time_); + } + + FilterUtility::setTimeoutHeaders(elapsed_time.count(), timeout_, *route_entry_, + *downstream_headers_, !config_->suppress_envoy_headers_, + grpc_request_, hedging_params_.hedge_on_per_try_timeout_); + UpstreamRequest* upstream_request_tmp = upstream_request.get(); LinkedList::moveIntoList(std::move(upstream_request), upstream_requests_); upstream_requests_.front()->acceptHeadersFromRouter( diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 7eb36e9ef3833..4d4d621b1409f 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -7813,7 +7813,7 @@ TEST_F(CustomRequestHeadersTest, CustomHeaderWrongFormat) { } // Validate that request_headers_to_add can reference router-set headers like -// x-envoy-expected-rq-timeout-ms and x-envoy-attempt-count. +// 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: @@ -7842,7 +7842,7 @@ TEST_F(CustomRequestHeadersTest, RequestHeadersCanReferenceRouterSetHeaders) { 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 request_headers_to_add. + // 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"); @@ -7854,45 +7854,6 @@ TEST_F(CustomRequestHeadersTest, RequestHeadersCanReferenceRouterSetHeaders) { EXPECT_EQ("1", headers.get_("x-attempt-copy")); } -// Validate that request_headers_to_add can reference router-set headers in retry scenarios. -TEST_F(CustomRequestHeadersTest, RequestHeadersCanReferenceRouterSetHeadersOnRetry) { - const std::string yaml = R"EOF( -virtual_hosts: -- name: www2 - domains: - - www.lyft.com - routes: - - match: - prefix: "/endpoint" - route: - cluster: www2 - timeout: 5s - retry_policy: - retry_on: "5xx" - num_retries: 3 - request_headers_to_add: - - header: - key: x-retry-attempt - value: "Attempt %REQ(x-envoy-attempt-count)% with timeout %REQ(x-envoy-expected-rq-timeout-ms)%ms" - )EOF"; - NiceMock 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 retry attempt where attempt count is incremented. - headers.addCopy(Http::LowerCaseString("x-envoy-expected-rq-timeout-ms"), "4500"); - headers.addCopy(Http::LowerCaseString("x-envoy-attempt-count"), "2"); - - auto formatter_context = Formatter::Context().setRequestHeaders(headers); - route_entry->finalizeRequestHeaders(headers, formatter_context, stream_info, true); - - // Verify that request_headers_to_add properly formatted the composite string. - EXPECT_EQ("Attempt 2 with timeout 4500ms", headers.get_("x-retry-attempt")); -} - TEST(MetadataMatchCriteriaImpl, Create) { auto v1 = Protobuf::Value(); v1.set_string_value("v1");