diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 24ce924c6e999..17cf907224caa 100644 --- a/changelogs/current.yaml +++ b/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 ` diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index fad5054e78f3a..d0697bdc3002c 100644 --- a/source/common/router/config_impl.cc +++ b/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. diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 9b3275cc73dfe..6b32a55175b93 100644 --- a/source/common/router/config_impl.h +++ b/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, diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 686989d10a8d6..07bfede7fbfa7 100644 --- a/source/common/router/router.cc +++ b/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_)) { diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index c352745ab47b0..4d4d621b1409f 100644 --- a/test/common/router/config_impl_test.cc +++ b/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 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"); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 256e8591f544d..187137b2d6940 100644 --- a/test/common/router/router_test.cc +++ b/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 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) {