diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9ecf0d6e48ce5..25a9c70373363 100644 --- a/changelogs/current.yaml +++ b/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 ` diff --git a/source/common/router/router.cc b/source/common/router/router.cc index a01ff69ee1942..2835d054cc600 100644 --- a/source/common/router/router.cc +++ b/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) { diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 2ebcd49eba84d..a130993e5c874 100644 --- a/test/common/router/router_test.cc +++ b/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. diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index f3b316444dda0..00594883f1d31 100644 --- a/test/integration/header_integration_test.cc +++ b/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);