Backport: router: fix a bug where header mutations may not be processed properly (#40856) (#40904)

Commit Message: router: fix a bug where header mutations may not be
processed properly

Additional Description:

The #39534 introduced a bug
where the `response_headers_to_add`` may be processed multiple times for
local responses from the router filter.

The sendLocalReply method will call the `finalizeResponseHeaders()` and
the #39534 updated the code and
make the `finalizeResponseHeaders()` be called in the modify_headers_
callback. This finally resulted in this problem.

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

Signed-off-by: WangBaiping <[email protected]>
Signed-off-by: code <[email protected]>

Author: wbpcode

changelogs/current.yaml

@@ -8,6 +8,10 @@ minor_behavior_changes:
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*
+- area: http
+  change: |
+    Fixed a bug where the ``response_headers_to_add`` may be processed multiple times for the local responses from
+    the router filter.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

source/common/router/router.cc

@@ -455,8 +455,6 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
       modify_headers_from_upstream_lb_(headers);
     }
 
-    route_entry_->finalizeResponseHeaders(headers, callbacks_->streamInfo());
-
     if (attempt_count_ == 0 || !route_entry_->includeAttemptCountInResponse()) {
       return;
     }
@@ -1758,6 +1756,7 @@ void Filter::onUpstreamHeaders(uint64_t response_code, Http::ResponseHeaderMapPt
 
   // Modify response headers after we have set the final upstream info because we may need to
   // modify the headers based on the upstream host.
+  route_entry_->finalizeResponseHeaders(*headers, callbacks_->streamInfo());
   modify_headers_(*headers);
 
   if (end_stream) {

test/common/router/router_test.cc

@@ -811,7 +811,6 @@ TEST_F(RouterTest, NoHost) {
   EXPECT_CALL(callbacks_, encodeData(_, true));
   EXPECT_CALL(callbacks_.stream_info_,
               setResponseFlag(StreamInfo::CoreResponseFlag::NoHealthyUpstream));
-  EXPECT_CALL(callbacks_.route_->route_entry_, finalizeResponseHeaders(_, _));
 
   Http::TestRequestHeaderMapImpl headers;
   HttpTestUtility::addDefaultHeaders(headers);
@@ -846,7 +845,6 @@ TEST_F(RouterTest, MaintenanceMode) {
   EXPECT_CALL(callbacks_, encodeData(_, true));
   EXPECT_CALL(callbacks_.stream_info_,
               setResponseFlag(StreamInfo::CoreResponseFlag::UpstreamOverflow));
-  EXPECT_CALL(callbacks_.route_->route_entry_, finalizeResponseHeaders(_, _));
 
   Http::TestRequestHeaderMapImpl headers;
   HttpTestUtility::addDefaultHeaders(headers);
@@ -894,7 +892,6 @@ TEST_F(RouterTest, DropOverloadDropped) {
   EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false));
   EXPECT_CALL(callbacks_, encodeData(_, true));
   EXPECT_CALL(callbacks_.stream_info_, setResponseFlag(StreamInfo::CoreResponseFlag::DropOverLoad));
-  EXPECT_CALL(callbacks_.route_->route_entry_, finalizeResponseHeaders(_, _));
 
   Http::TestRequestHeaderMapImpl headers;
   HttpTestUtility::addDefaultHeaders(headers);
@@ -1226,7 +1223,6 @@ TEST_F(RouterTest, AllDebugConfig) {
   Http::TestResponseHeaderMapImpl response_headers{{":status", "204"},
                                                    {"x-envoy-not-forwarded", "true"}};
   EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true));
-  EXPECT_CALL(callbacks_.route_->route_entry_, finalizeResponseHeaders(_, _));
 
   Http::TestRequestHeaderMapImpl headers;
   HttpTestUtility::addDefaultHeaders(headers);
@@ -3332,7 +3328,6 @@ TEST_F(RouterTest, RetryNoneHealthy) {
   EXPECT_CALL(callbacks_, encodeData(_, true));
   EXPECT_CALL(callbacks_.stream_info_,
               setResponseFlag(StreamInfo::CoreResponseFlag::NoHealthyUpstream));
-  EXPECT_CALL(callbacks_.route_->route_entry_, finalizeResponseHeaders(_, _));
   router_->retry_state_->callback_();
   EXPECT_TRUE(verifyHostUpstreamStats(0, 1));
   // Pool failure for the first try, so only 1 upstream request was made.

test/integration/header_integration_test.cc

@@ -1053,6 +1053,45 @@ TEST_P(HeaderIntegrationTest, TestDynamicHeaders) {
       });
 }
 
+TEST_P(HeaderIntegrationTest, TestResponseHeadersOnlyBeHandledOnce) {
+  initializeFilter(HeaderMode::Append, false);
+  registerTestServerPorts({"http"});
+  codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http")));
+
+  auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{
+      {":method", "POST"},
+      {":path", "/vhost-and-route"},
+      {":scheme", "http"},
+      {":authority", "vhost-headers.com"},
+  });
+  auto response = std::move(encoder_decoder.second);
+
+  ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_));
+
+  ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));
+  ASSERT_TRUE(upstream_request_->waitForHeadersComplete());
+  ASSERT_TRUE(fake_upstream_connection_->close());
+  ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
+  ASSERT_TRUE(response->waitForEndStream());
+
+  if (downstream_protocol_ == Http::CodecType::HTTP1) {
+    ASSERT_TRUE(codec_client_->waitForDisconnect());
+  } else {
+    codec_client_->close();
+  }
+
+  EXPECT_FALSE(upstream_request_->complete());
+  EXPECT_EQ(0U, upstream_request_->bodyLength());
+
+  EXPECT_TRUE(response->complete());
+
+  Http::TestResponseHeaderMapImpl response_headers{response->headers()};
+  EXPECT_EQ(1, response_headers.get(Http::LowerCaseString("x-route-response")).size());
+  EXPECT_EQ("route", response_headers.get_("x-route-response"));
+  EXPECT_EQ(1, response_headers.get(Http::LowerCaseString("x-vhost-response")).size());
+  EXPECT_EQ("vhost", response_headers.get_("x-vhost-response"));
+}
+
 // Validates that XFF gets properly parsed.
 TEST_P(HeaderIntegrationTest, TestXFFParsing) {
   initializeFilter(HeaderMode::Replace, false);