router: allow path changes in regex routes. (envoyproxy#3537)

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
JimmyCYJ · Jun 5, 2018 · c585dfe · c585dfe
1 parent 6f92dc2
commit c585dfe
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 43 deletions.
diff --git 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
@@ -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
@@ -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<Server::Configuration::MockFactoryContext> 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<Envoy::RequestInfo::MockRequestInfo> request_info;
+    EXPECT_NO_THROW(route_entry->finalizeRequestHeaders(headers, request_info, false));
+  }
+}
+
 class PerFilterConfigsTest : public testing::Test {
 public:
   PerFilterConfigsTest() : factory_(), registered_factory_(factory_) {}