diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 95944beddb2b0..b0e3955f6d7d0 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -64,9 +64,11 @@ class ResponseEntry { * process them later. Note: do not use unless you are sure that there will be no route * modifications later in the filter chain. * @param stream_info holds additional information about the request. + * @param do_formatting whether or not to evaluate configured transformations; if false, returns + * original values instead. */ - virtual Http::HeaderTransforms - responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info) const PURE; + virtual Http::HeaderTransforms responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info, + bool do_formatting = true) const PURE; }; /** diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 5aea06d97056b..fee5b63f3adbf 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -237,7 +237,8 @@ class AsyncStreamImpl : public AsyncClient::Stream, bool) const override {} void finalizeResponseHeaders(Http::ResponseHeaderMap&, const StreamInfo::StreamInfo&) const override {} - Http::HeaderTransforms responseHeaderTransforms(const StreamInfo::StreamInfo&) const override { + Http::HeaderTransforms responseHeaderTransforms(const StreamInfo::StreamInfo&, + bool) const override { return {}; } const HashPolicy* hashPolicy() const override { return hash_policy_.get(); } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 4994bc5d070e7..1c085856c9664 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -624,24 +624,29 @@ void RouteEntryImplBase::finalizeResponseHeaders(Http::ResponseHeaderMap& header } Http::HeaderTransforms -RouteEntryImplBase::responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info) const { +RouteEntryImplBase::responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info, + bool do_formatting) const { Http::HeaderTransforms transforms; if (!vhost_.globalRouteConfig().mostSpecificHeaderMutationsWins()) { // Append user-specified request headers from most to least specific: route-level headers, // virtual host level headers and finally global connection manager level headers. - mergeTransforms(transforms, response_headers_parser_->getHeaderTransforms(stream_info)); - mergeTransforms(transforms, vhost_.responseHeaderParser().getHeaderTransforms(stream_info)); - mergeTransforms( - transforms, - vhost_.globalRouteConfig().responseHeaderParser().getHeaderTransforms(stream_info)); + mergeTransforms(transforms, + response_headers_parser_->getHeaderTransforms(stream_info, do_formatting)); + mergeTransforms(transforms, + vhost_.responseHeaderParser().getHeaderTransforms(stream_info, do_formatting)); + mergeTransforms(transforms, + vhost_.globalRouteConfig().responseHeaderParser().getHeaderTransforms( + stream_info, do_formatting)); } else { // Most specific mutations (route-level) take precedence by being applied // last: if a header is specified at all levels, the last one applied wins. - mergeTransforms( - transforms, - vhost_.globalRouteConfig().responseHeaderParser().getHeaderTransforms(stream_info)); - mergeTransforms(transforms, vhost_.responseHeaderParser().getHeaderTransforms(stream_info)); - mergeTransforms(transforms, response_headers_parser_->getHeaderTransforms(stream_info)); + mergeTransforms(transforms, + vhost_.globalRouteConfig().responseHeaderParser().getHeaderTransforms( + stream_info, do_formatting)); + mergeTransforms(transforms, + vhost_.responseHeaderParser().getHeaderTransforms(stream_info, do_formatting)); + mergeTransforms(transforms, + response_headers_parser_->getHeaderTransforms(stream_info, do_formatting)); } return transforms; } @@ -1044,9 +1049,10 @@ RouteEntryImplBase::WeightedClusterEntry::WeightedClusterEntry( } Http::HeaderTransforms RouteEntryImplBase::WeightedClusterEntry::responseHeaderTransforms( - const StreamInfo::StreamInfo& stream_info) const { - auto transforms = response_headers_parser_->getHeaderTransforms(stream_info); - mergeTransforms(transforms, DynamicRouteEntry::responseHeaderTransforms(stream_info)); + const StreamInfo::StreamInfo& stream_info, bool do_formatting) const { + auto transforms = response_headers_parser_->getHeaderTransforms(stream_info, do_formatting); + mergeTransforms(transforms, + DynamicRouteEntry::responseHeaderTransforms(stream_info, do_formatting)); return transforms; } diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index b88f1932666d5..f2b19b6aa5dfe 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -96,7 +96,8 @@ class SslRedirector : public DirectResponseEntry { // Router::DirectResponseEntry void finalizeResponseHeaders(Http::ResponseHeaderMap&, const StreamInfo::StreamInfo&) const override {} - Http::HeaderTransforms responseHeaderTransforms(const StreamInfo::StreamInfo&) const override { + Http::HeaderTransforms responseHeaderTransforms(const StreamInfo::StreamInfo&, + bool) const override { return {}; } std::string newPath(const Http::RequestHeaderMap& headers) const override; @@ -501,8 +502,8 @@ class RouteEntryImplBase : public RouteEntry, bool insert_envoy_original_path) const override; void finalizeResponseHeaders(Http::ResponseHeaderMap& headers, const StreamInfo::StreamInfo& stream_info) const override; - Http::HeaderTransforms - responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info) const override; + Http::HeaderTransforms responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info, + bool do_formatting = true) const override; const Http::HashPolicy* hashPolicy() const override { return hash_policy_.get(); } const HedgePolicy& hedgePolicy() const override { return hedge_policy_; } @@ -636,9 +637,9 @@ class RouteEntryImplBase : public RouteEntry, const StreamInfo::StreamInfo& stream_info) const override { return parent_->finalizeResponseHeaders(headers, stream_info); } - Http::HeaderTransforms - responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info) const override { - return parent_->responseHeaderTransforms(stream_info); + Http::HeaderTransforms responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info, + bool do_formatting = true) const override { + return parent_->responseHeaderTransforms(stream_info, do_formatting); } const CorsPolicy* corsPolicy() const override { return parent_->corsPolicy(); } @@ -765,8 +766,8 @@ class RouteEntryImplBase : public RouteEntry, response_headers_parser_->evaluateHeaders(headers, stream_info); DynamicRouteEntry::finalizeResponseHeaders(headers, stream_info); } - Http::HeaderTransforms - responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info) const override; + Http::HeaderTransforms responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info, + bool do_formatting = true) const override; const RouteSpecificFilterConfig* perFilterConfig(const std::string& name) const override; diff --git a/source/common/router/delegating_route_impl.cc b/source/common/router/delegating_route_impl.cc index af4c61304c698..bdc8ce1dde02f 100644 --- a/source/common/router/delegating_route_impl.cc +++ b/source/common/router/delegating_route_impl.cc @@ -25,8 +25,9 @@ void DelegatingRouteEntry::finalizeResponseHeaders( } Http::HeaderTransforms -DelegatingRouteEntry::responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info) const { - return base_route_->routeEntry()->responseHeaderTransforms(stream_info); +DelegatingRouteEntry::responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info, + bool do_formatting) const { + return base_route_->routeEntry()->responseHeaderTransforms(stream_info, do_formatting); } const std::string& DelegatingRouteEntry::clusterName() const { diff --git a/source/common/router/delegating_route_impl.h b/source/common/router/delegating_route_impl.h index ebe791c37c755..53721b394c53d 100644 --- a/source/common/router/delegating_route_impl.h +++ b/source/common/router/delegating_route_impl.h @@ -46,8 +46,8 @@ class DelegatingRouteEntry : public Router::RouteEntry { // Router::ResponseEntry void finalizeResponseHeaders(Http::ResponseHeaderMap& headers, const StreamInfo::StreamInfo& stream_info) const override; - Http::HeaderTransforms - responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info) const override; + Http::HeaderTransforms responseHeaderTransforms(const StreamInfo::StreamInfo& stream_info, + bool do_formatting = true) const override; // Router::RouteEntry const std::string& clusterName() const override; diff --git a/source/common/router/header_parser.cc b/source/common/router/header_parser.cc index e05460bfb0c17..5a46d92d7c152 100644 --- a/source/common/router/header_parser.cc +++ b/source/common/router/header_parser.cc @@ -296,17 +296,25 @@ void HeaderParser::evaluateHeaders(Http::HeaderMap& headers, } } -Http::HeaderTransforms -HeaderParser::getHeaderTransforms(const StreamInfo::StreamInfo& stream_info) const { +Http::HeaderTransforms HeaderParser::getHeaderTransforms(const StreamInfo::StreamInfo& stream_info, + bool do_formatting) const { Http::HeaderTransforms transforms; for (const auto& [key, entry] : headers_to_add_) { - const std::string value = entry.formatter_->format(stream_info); - if (!value.empty()) { + if (do_formatting) { + const std::string value = entry.formatter_->format(stream_info); + if (!value.empty()) { + if (entry.formatter_->append()) { + transforms.headers_to_append.push_back({key, value}); + } else { + transforms.headers_to_overwrite.push_back({key, value}); + } + } + } else { if (entry.formatter_->append()) { - transforms.headers_to_append.push_back({key, value}); + transforms.headers_to_append.push_back({key, entry.original_value_}); } else { - transforms.headers_to_overwrite.push_back({key, value}); + transforms.headers_to_overwrite.push_back({key, entry.original_value_}); } } } diff --git a/source/common/router/header_parser.h b/source/common/router/header_parser.h index 8e4d5548fce03..03b1dc65b00c3 100644 --- a/source/common/router/header_parser.h +++ b/source/common/router/header_parser.h @@ -55,8 +55,11 @@ class HeaderParser { * Same as evaluateHeaders, but returns the modifications that would have been made rather than * modifying an existing HeaderMap. * @param stream_info contains additional information about the request. + * @param do_formatting whether or not to evaluate configured transformations; if false, returns + * original values instead. */ - Http::HeaderTransforms getHeaderTransforms(const StreamInfo::StreamInfo& stream_info) const; + Http::HeaderTransforms getHeaderTransforms(const StreamInfo::StreamInfo& stream_info, + bool do_formatting = true) const; protected: HeaderParser() = default; diff --git a/source/docs/response_header_transforms.md b/source/docs/response_header_transforms.md index 33390129ebc75..0d0ca01116b11 100644 --- a/source/docs/response_header_transforms.md +++ b/source/docs/response_header_transforms.md @@ -26,3 +26,6 @@ Http::FilterHeadersStatus MyFilter::decodeHeaders( "local_reply"); } ``` + +If you want to retrieve the original values rather than the formatted ones, pass +`/*do_formatting=*/false` as the second argument to `responseHeaderTransforms`. diff --git a/test/common/router/BUILD b/test/common/router/BUILD index 858621a096c4d..eabd2de01e82d 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -29,6 +29,7 @@ envoy_cc_test_library( "//source/common/http:header_map_lib", "//source/common/http:headers_lib", "//source/common/router:config_lib", + "//source/common/router:string_accessor_lib", "//source/common/stream_info:filter_state_lib", "//test/extensions/filters/http/common:empty_http_filter_config_lib", "//test/fuzz:utility_lib", diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index c465afe150e6a..fccd56bf0e577 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -18,6 +18,7 @@ #include "common/http/headers.h" #include "common/network/address_impl.h" #include "common/router/config_impl.h" +#include "common/router/string_accessor_impl.h" #include "common/stream_info/filter_state_impl.h" #include "test/common/router/route_fuzz.pb.h" @@ -1789,6 +1790,52 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersAppendMostSpecificWins) { Http::LowerCaseString("x-vhost-remove"))); } +TEST_F(RouteMatcherTest, TestResponseHeaderTransformsDoFormatting) { + factory_context_.cluster_manager_.initializeClusters({"default"}, {}); + const std::string yaml = R"EOF( +virtual_hosts: + - name: default + domains: ["*"] + routes: + - match: + prefix: "/" + route: + cluster: "default" +response_headers_to_add: + - header: + key: x-has-variable + value: "%PER_REQUEST_STATE(testing)%" + append: false +)EOF"; + NiceMock stream_info; + + Envoy::StreamInfo::FilterStateSharedPtr filter_state( + std::make_shared( + Envoy::StreamInfo::FilterState::LifeSpan::FilterChain)); + filter_state->setData("testing", std::make_unique("test_value"), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); + ON_CALL(stream_info, filterState()).WillByDefault(ReturnRef(filter_state)); + ON_CALL(Const(stream_info), filterState()).WillByDefault(ReturnRef(*filter_state)); + + TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); + + Http::TestRequestHeaderMapImpl req_headers = + genHeaders("www.lyft.com", "/new_endpoint/foo", "GET"); + const RouteEntry* route = config.route(req_headers, 0)->routeEntry(); + Http::TestResponseHeaderMapImpl headers; + route->finalizeResponseHeaders(headers, stream_info); + + auto transforms = route->responseHeaderTransforms(stream_info, /*do_formatting=*/true); + EXPECT_THAT(transforms.headers_to_overwrite, + ElementsAre(Pair(Http::LowerCaseString("x-has-variable"), "test_value"))); + + transforms = route->responseHeaderTransforms(stream_info, /*do_formatting=*/false); + EXPECT_THAT( + transforms.headers_to_overwrite, + ElementsAre(Pair(Http::LowerCaseString("x-has-variable"), "%PER_REQUEST_STATE(testing)%"))); +} + TEST_F(RouteMatcherTest, TestAddGlobalResponseHeaderRemoveFromRoute) { const std::string yaml = R"EOF( virtual_hosts: diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index 6ffb945b39486..47385fb9dad99 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -1438,7 +1438,7 @@ response_headers_to_remove: ["x-foo-header"] EXPECT_EQ("bar", header_map.get_("x-foo-header")); } -TEST(HeaderParserTest, GetHeaderTransforms) { +TEST(HeaderParserTest, GetHeaderTransformsWithFormatting) { const std::string yaml = R"EOF( match: { prefix: "/new_endpoint" } route: @@ -1452,6 +1452,10 @@ match: { prefix: "/new_endpoint" } key: "x-bar-header" value: "bar" append: false + - header: + key: "x-per-request-header" + value: "%PER_REQUEST_STATE(testing)%" + append: false response_headers_to_remove: ["x-baz-header"] )EOF"; @@ -1460,11 +1464,67 @@ response_headers_to_remove: ["x-baz-header"] HeaderParser::configure(route.response_headers_to_add(), route.response_headers_to_remove()); NiceMock stream_info; + Envoy::StreamInfo::FilterStateSharedPtr filter_state( + std::make_shared( + Envoy::StreamInfo::FilterState::LifeSpan::FilterChain)); + filter_state->setData("testing", std::make_unique("test_value"), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); + ON_CALL(stream_info, filterState()).WillByDefault(ReturnRef(filter_state)); + ON_CALL(Const(stream_info), filterState()).WillByDefault(ReturnRef(*filter_state)); + auto transforms = resp_header_parser->getHeaderTransforms(stream_info); EXPECT_THAT(transforms.headers_to_append, ElementsAre(Pair(Http::LowerCaseString("x-foo-header"), "foo"))); EXPECT_THAT(transforms.headers_to_overwrite, - ElementsAre(Pair(Http::LowerCaseString("x-bar-header"), "bar"))); + ElementsAre(Pair(Http::LowerCaseString("x-bar-header"), "bar"), + Pair(Http::LowerCaseString("x-per-request-header"), "test_value"))); + EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-baz-header"))); +} + +TEST(HeaderParserTest, GetHeaderTransformsOriginalValues) { + const std::string yaml = R"EOF( +match: { prefix: "/new_endpoint" } +route: + cluster: www2 +response_headers_to_add: + - header: + key: "x-foo-header" + value: "foo" + append: true + - header: + key: "x-bar-header" + value: "bar" + append: false + - header: + key: "x-per-request-header" + value: "%PER_REQUEST_STATE(testing)%" + append: false +response_headers_to_remove: ["x-baz-header"] +)EOF"; + + const auto route = parseRouteFromV3Yaml(yaml); + HeaderParserPtr response_header_parser = + HeaderParser::configure(route.response_headers_to_add(), route.response_headers_to_remove()); + NiceMock stream_info; + + Envoy::StreamInfo::FilterStateSharedPtr filter_state( + std::make_shared( + Envoy::StreamInfo::FilterState::LifeSpan::FilterChain)); + filter_state->setData("testing", std::make_unique("test_value"), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); + ON_CALL(stream_info, filterState()).WillByDefault(ReturnRef(filter_state)); + ON_CALL(Const(stream_info), filterState()).WillByDefault(ReturnRef(*filter_state)); + + auto transforms = + response_header_parser->getHeaderTransforms(stream_info, /*do_formatting=*/false); + EXPECT_THAT(transforms.headers_to_append, + ElementsAre(Pair(Http::LowerCaseString("x-foo-header"), "foo"))); + EXPECT_THAT(transforms.headers_to_overwrite, + ElementsAre(Pair(Http::LowerCaseString("x-bar-header"), "bar"), + Pair(Http::LowerCaseString("x-per-request-header"), + "%PER_REQUEST_STATE(testing)%"))); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-baz-header"))); } diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 13f56a239ef5e..59b21d481f554 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -50,7 +50,7 @@ class MockDirectResponseEntry : public DirectResponseEntry { (Http::ResponseHeaderMap & headers, const StreamInfo::StreamInfo& stream_info), (const)); MOCK_METHOD(Http::HeaderTransforms, responseHeaderTransforms, - (const StreamInfo::StreamInfo& stream_info), (const)); + (const StreamInfo::StreamInfo& stream_info, bool do_formatting), (const)); MOCK_METHOD(std::string, newPath, (const Http::RequestHeaderMap& headers), (const)); MOCK_METHOD(void, rewritePathHeader, (Http::RequestHeaderMap & headers, bool insert_envoy_original_path), (const)); @@ -361,7 +361,7 @@ class MockRouteEntry : public RouteEntry { (Http::ResponseHeaderMap & headers, const StreamInfo::StreamInfo& stream_info), (const)); MOCK_METHOD(Http::HeaderTransforms, responseHeaderTransforms, - (const StreamInfo::StreamInfo& stream_info), (const)); + (const StreamInfo::StreamInfo& stream_info, bool do_formatting), (const)); MOCK_METHOD(const Http::HashPolicy*, hashPolicy, (), (const)); MOCK_METHOD(const HedgePolicy&, hedgePolicy, (), (const)); MOCK_METHOD(const Router::MetadataMatchCriteria*, metadataMatchCriteria, (), (const));