diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index b48a78e5cf3b..0a21ae3fa591 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -364,16 +364,19 @@ Http::WebSocketProxyPtr RouteEntryImplBase::createWebSocketProxy( void RouteEntryImplBase::finalizeRequestHeaders(Http::HeaderMap& headers, const RequestInfo::RequestInfo& request_info, bool insert_envoy_original_path) const { - UNREFERENCED_PARAMETER(insert_envoy_original_path); // Append user-specified request headers in the following order: route-level headers, // virtual host level headers and finally global connection manager level headers. request_headers_parser_->evaluateHeaders(headers, request_info); vhost_.requestHeaderParser().evaluateHeaders(headers, request_info); vhost_.globalRouteConfig().requestHeaderParser().evaluateHeaders(headers, request_info); - if (host_rewrite_.empty()) { - return; + if (!host_rewrite_.empty()) { + headers.Host()->value(host_rewrite_); + } + + // Handle path rewrite + if (!getPathRewrite().empty()) { + rewritePathHeader(headers, insert_envoy_original_path); } - headers.Host()->value(host_rewrite_); } void RouteEntryImplBase::finalizeResponseHeaders( @@ -399,7 +402,7 @@ RouteEntryImplBase::loadRuntimeData(const envoy::api::v2::route::RouteMatch& rou void RouteEntryImplBase::finalizePathHeader(Http::HeaderMap& headers, const std::string& matched_path, bool insert_envoy_original_path) const { - const auto& rewrite = (isRedirect()) ? prefix_rewrite_redirect_ : prefix_rewrite_; + const auto& rewrite = getPathRewrite(); if (rewrite.empty()) { return; } @@ -602,14 +605,6 @@ PrefixRouteEntryImpl::PrefixRouteEntryImpl(const VirtualHostImpl& vhost, Server::Configuration::FactoryContext& factory_context) : RouteEntryImplBase(vhost, route, factory_context), prefix_(route.match().prefix()) {} -void PrefixRouteEntryImpl::finalizeRequestHeaders(Http::HeaderMap& headers, - const RequestInfo::RequestInfo& request_info, - bool insert_envoy_original_path) const { - RouteEntryImplBase::finalizeRequestHeaders(headers, request_info, insert_envoy_original_path); - - finalizePathHeader(headers, prefix_, insert_envoy_original_path); -} - void PrefixRouteEntryImpl::rewritePathHeader(Http::HeaderMap& headers, bool insert_envoy_original_path) const { finalizePathHeader(headers, prefix_, insert_envoy_original_path); @@ -629,14 +624,6 @@ PathRouteEntryImpl::PathRouteEntryImpl(const VirtualHostImpl& vhost, Server::Configuration::FactoryContext& factory_context) : RouteEntryImplBase(vhost, route, factory_context), path_(route.match().path()) {} -void PathRouteEntryImpl::finalizeRequestHeaders(Http::HeaderMap& headers, - const RequestInfo::RequestInfo& request_info, - bool insert_envoy_original_path) const { - RouteEntryImplBase::finalizeRequestHeaders(headers, request_info, insert_envoy_original_path); - - finalizePathHeader(headers, path_, insert_envoy_original_path); -} - void PathRouteEntryImpl::rewritePathHeader(Http::HeaderMap& headers, bool insert_envoy_original_path) const { finalizePathHeader(headers, path_, insert_envoy_original_path); @@ -681,19 +668,14 @@ void RegexRouteEntryImpl::rewritePathHeader(Http::HeaderMap& headers, bool insert_envoy_original_path) const { const Http::HeaderString& path = headers.Path()->value(); const char* query_string_start = Http::Utility::findQueryStringStart(path); + // TODO(yuval-k): This ASSERT can happen if the path was changed by a filter without clearing the + // route cache. We should consider if ASSERT-ing is the desired behavior in this case. ASSERT(std::regex_match(path.c_str(), query_string_start, regex_)); std::string matched_path(path.c_str(), query_string_start); finalizePathHeader(headers, matched_path, insert_envoy_original_path); } -void RegexRouteEntryImpl::finalizeRequestHeaders(Http::HeaderMap& headers, - const RequestInfo::RequestInfo& request_info, - bool insert_envoy_original_path) const { - RouteEntryImplBase::finalizeRequestHeaders(headers, request_info, insert_envoy_original_path); - rewritePathHeader(headers, insert_envoy_original_path); -} - RouteConstSharedPtr RegexRouteEntryImpl::matches(const Http::HeaderMap& headers, uint64_t random_value) const { if (RouteEntryImplBase::matchRoute(headers, random_value)) { diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index cf7dfd386686..d56e6a5aa6c0 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -342,6 +342,14 @@ class RouteEntryImplBase : public RouteEntry, bool include_vh_rate_limits_; RouteConstSharedPtr clusterEntry(const Http::HeaderMap& headers, uint64_t random_value) const; + + /** + * returns the correct path rewrite string for this route. + */ + const std::string& getPathRewrite() const { + return (isRedirect()) ? prefix_rewrite_redirect_ : prefix_rewrite_; + } + void finalizePathHeader(Http::HeaderMap& headers, const std::string& matched_path, bool insert_envoy_original_path) const; const HeaderParser& requestHeaderParser() const { return *request_headers_parser_; }; @@ -532,11 +540,6 @@ class PrefixRouteEntryImpl : public RouteEntryImplBase { PrefixRouteEntryImpl(const VirtualHostImpl& vhost, const envoy::api::v2::route::Route& route, Server::Configuration::FactoryContext& factory_context); - // Router::RouteEntry - void finalizeRequestHeaders(Http::HeaderMap& headers, - const RequestInfo::RequestInfo& request_info, - bool insert_envoy_original_path) const override; - // Router::PathMatchCriterion const std::string& matcher() const override { return prefix_; } PathMatchType matchType() const override { return PathMatchType::Prefix; } @@ -559,11 +562,6 @@ class PathRouteEntryImpl : public RouteEntryImplBase { PathRouteEntryImpl(const VirtualHostImpl& vhost, const envoy::api::v2::route::Route& route, Server::Configuration::FactoryContext& factory_context); - // Router::RouteEntry - void finalizeRequestHeaders(Http::HeaderMap& headers, - const RequestInfo::RequestInfo& request_info, - bool insert_envoy_original_path) const override; - // Router::PathMatchCriterion const std::string& matcher() const override { return path_; } PathMatchType matchType() const override { return PathMatchType::Exact; } @@ -586,11 +584,6 @@ class RegexRouteEntryImpl : public RouteEntryImplBase { RegexRouteEntryImpl(const VirtualHostImpl& vhost, const envoy::api::v2::route::Route& route, Server::Configuration::FactoryContext& factory_context); - // Router::RouteEntry - void finalizeRequestHeaders(Http::HeaderMap& headers, - const RequestInfo::RequestInfo& request_info, - bool insert_envoy_original_path) const override; - // Router::PathMatchCriterion const std::string& matcher() const override { return regex_str_; } PathMatchType matchType() const override { return PathMatchType::Regex; } diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index e766980acfd0..164944b70fdf 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -4340,6 +4340,38 @@ name: foo } } +TEST(RouteConfigurationV2, RegexPrefixWithNoRewriteWorksWhenPathChanged) { + + // setup regex route entry. the regex is trivial, thats ok as we only want to test that + // path change works. + std::string RegexRewrite = R"EOF( +name: RegexNoMatch +virtual_hosts: + - name: regex + domains: [regex.lyft.com] + routes: + - match: { regex: "/regex"} + route: { cluster: some-cluster } + )EOF"; + + NiceMock factory_context; + ConfigImpl config(parseRouteConfigurationFromV2Yaml(RegexRewrite), factory_context, true); + + { + // Get our regex route entry + Http::TestHeaderMapImpl headers = genRedirectHeaders("regex.lyft.com", "/regex", true, false); + const RouteEntry* route_entry = config.route(headers, 0)->routeEntry(); + + // simulate a filter changing the path + headers.remove(":path"); + headers.addCopy(":path", "/not-the-original-regex"); + + // no re-write was specified; so this should not throw + NiceMock request_info; + EXPECT_NO_THROW(route_entry->finalizeRequestHeaders(headers, request_info, false)); + } +} + class PerFilterConfigsTest : public testing::Test { public: PerFilterConfigsTest() : factory_(), registered_factory_(factory_) {}