Refactor route matching (envoyproxy#341)

include/envoy/router/router.h

@@ -201,32 +201,39 @@ class RouteEntry {
 };
 
 /**
- * The router configuration.
+ * An interface that holds a RedirectEntry or a RouteEntry for a request.
  */
-class Config {
+class Route {
 public:
-  virtual ~Config() {}
+  virtual ~Route() {}
 
   /**
-   * Based on the incoming HTTP request headers, determine whether a redirect should take place.
-   * @param headers supplies the request headers.
-   * @param random_value supplies the random seed to use if a runtime choice is required. This
-   *        allows stable choices between calls if desired.
    * @return the redirect entry or nullptr if there is no redirect needed for the request.
    */
-  virtual const RedirectEntry* redirectRequest(const Http::HeaderMap& headers,
-                                               uint64_t random_value) const PURE;
+  virtual const RedirectEntry* redirectEntry() const PURE;
+
+  /**
+   * @return the route entry or nullptr if there is no matching route for the request.
+   */
+  virtual const RouteEntry* routeEntry() const PURE;
+};
+
+/**
+ * The router configuration.
+ */
+class Config {
+public:
+  virtual ~Config() {}
 
   /**
-   * Based on the incoming HTTP request headers, choose the target route to send the remainder
-   * of the request to.
+   * Based on the incoming HTTP request headers, determine the target route (containing either a
+   * route entry or a redirect entry) for the request.
    * @param headers supplies the request headers.
    * @param random_value supplies the random seed to use if a runtime choice is required. This
    *        allows stable choices between calls if desired.
    * @return the route or nullptr if there is no matching route for the request.
    */
-  virtual const RouteEntry* routeForRequest(const Http::HeaderMap& headers,
-                                            uint64_t random_value) const PURE;
+  virtual const Route* route(const Http::HeaderMap& headers, uint64_t random_value) const PURE;
 
   /**
    * Return a list of headers that will be cleaned from any requests that are not from an internal
@@ -266,19 +273,12 @@ class StableRouteTable {
   virtual ~StableRouteTable() {}
 
   /**
-   * Based on the incoming HTTP request headers, determine whether a redirect should take place.
-   * @param headers supplies the request headers.
-   * @return the redirect entry or nullptr if there is no redirect needed for the request.
-   */
-  virtual const RedirectEntry* redirectRequest(const Http::HeaderMap& headers) const PURE;
-
-  /**
-   * Based on the incoming HTTP request headers, choose the target route to send the remainder
-   * of the request to.
+   * Based on the incoming HTTP request headers, determine the target route (containing either a
+   * route entry or a redirect entry) for the request.
    * @param headers supplies the request headers.
    * @return the route or nullptr if there is no matching route for the request.
    */
-  virtual const RouteEntry* routeForRequest(const Http::HeaderMap& headers) const PURE;
+  virtual const Route* route(const Http::HeaderMap& headers) const PURE;
 };
 
 } // Router

source/common/grpc/http1_bridge_filter.cc

@@ -95,18 +95,23 @@ Http::FilterTrailersStatus Http1BridgeFilter::encodeTrailers(Http::HeaderMap& tr
 }
 
 void Http1BridgeFilter::setupStatTracking(const Http::HeaderMap& headers) {
-  const Router::RouteEntry* route = decoder_callbacks_->routeTable().routeForRequest(headers);
+  const Router::Route* route = decoder_callbacks_->routeTable().route(headers);
   if (!route) {
     return;
   }
 
+  const Router::RouteEntry* route_entry = route->routeEntry();
+  if (!route_entry) {
+    return;
+  }
+
   std::vector<std::string> parts = StringUtil::split(headers.Path()->value().c_str(), '/');
   if (parts.size() != 2) {
     return;
   }
 
   // TODO: Cluster may not exist.
-  cluster_ = cm_.get(route->clusterName());
+  cluster_ = cm_.get(route_entry->clusterName());
   grpc_service_ = parts[0];
   grpc_method_ = parts[1];
   do_stat_tracking_ = true;

source/common/http/async_client_impl.h

@@ -125,6 +125,17 @@ class AsyncRequestImpl final : public AsyncClient::Request,
     Optional<std::chrono::milliseconds> timeout_;
   };
 
+  struct RouteImpl : public Router::Route {
+    RouteImpl(const std::string& cluster_name, const Optional<std::chrono::milliseconds>& timeout)
+        : route_entry_(cluster_name, timeout) {}
+
+    // Router::Route
+    const Router::RedirectEntry* redirectEntry() const override { return nullptr; }
+    const Router::RouteEntry* routeEntry() const override { return &route_entry_; }
+
+    RouteEntryImpl route_entry_;
+  };
+
   void cleanup();
   void failDueToClientDestroy();
   void onComplete();
@@ -147,12 +158,7 @@ class AsyncRequestImpl final : public AsyncClient::Request,
   void encodeTrailers(HeaderMapPtr&& trailers) override;
 
   // Router::StableRouteTable
-  const Router::RedirectEntry* redirectRequest(const Http::HeaderMap&) const override {
-    return nullptr;
-  }
-  const Router::RouteEntry* routeForRequest(const Http::HeaderMap&) const override {
-    return &route_;
-  }
+  const Router::Route* route(const Http::HeaderMap&) const override { return &route_; }
 
   MessagePtr request_;
   AsyncClientImpl& parent_;
@@ -162,7 +168,7 @@ class AsyncRequestImpl final : public AsyncClient::Request,
   Router::ProdFilter router_;
   std::function<void()> reset_callback_;
   AccessLog::RequestInfoImpl request_info_;
-  RouteEntryImpl route_;
+  RouteImpl route_;
   bool complete_{};
 
   friend class AsyncClientImpl;

source/common/http/conn_manager_impl.h

@@ -251,13 +251,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     const std::string& downstreamAddress() override;
 
     // Router::StableRouteTable
-    const Router::RedirectEntry* redirectRequest(const HeaderMap& headers) const {
-      return parent_.connection_manager_.config_.routeConfig().redirectRequest(headers,
-                                                                               parent_.stream_id_);
-    }
-    const Router::RouteEntry* routeForRequest(const HeaderMap& headers) const {
-      return parent_.connection_manager_.config_.routeConfig().routeForRequest(headers,
-                                                                               parent_.stream_id_);
+    const Router::Route* route(const HeaderMap& headers) const {
+      return parent_.connection_manager_.config_.routeConfig().route(headers, parent_.stream_id_);
     }
 
     ActiveStream& parent_;

source/common/http/filter/ratelimit.cc

@@ -19,19 +19,21 @@ FilterHeadersStatus Filter::decodeHeaders(HeaderMap& headers, bool) {
     return FilterHeadersStatus::Continue;
   }
 
-  const Router::RouteEntry* route = callbacks_->routeTable().routeForRequest(headers);
-  if (route) {
+  const Router::Route* route = callbacks_->routeTable().route(headers);
+  if (route && route->routeEntry()) {
+    const Router::RouteEntry* route_entry = route->routeEntry();
+
     // TODO: Cluster may not exist.
-    cluster_ = config_->cm().get(route->clusterName());
+    cluster_ = config_->cm().get(route_entry->clusterName());
 
     std::vector<::RateLimit::Descriptor> descriptors;
 
     // Get all applicable rate limit policy entries for the route.
-    populateRateLimitDescriptors(route->rateLimitPolicy(), descriptors, route, headers);
+    populateRateLimitDescriptors(route_entry->rateLimitPolicy(), descriptors, route_entry, headers);
 
     // Get all applicable rate limit policy entries for the virtual host.
-    populateRateLimitDescriptors(route->virtualHost().rateLimitPolicy(), descriptors, route,
-                                 headers);
+    populateRateLimitDescriptors(route_entry->virtualHost().rateLimitPolicy(), descriptors,
+                                 route_entry, headers);
 
     if (!descriptors.empty()) {
       state_ = State::Calling;

source/common/router/config_impl.cc

@@ -175,6 +175,24 @@ std::string RouteEntryImplBase::newPath(const Http::HeaderMap& headers) const {
                      final_path);
 }
 
+const RedirectEntry* RouteEntryImplBase::redirectEntry() const {
+  // A route for a request can exclusively be a route entry or a redirect entry.
+  if (isRedirect()) {
+    return this;
+  } else {
+    return nullptr;
+  }
+}
+
+const RouteEntry* RouteEntryImplBase::routeEntry() const {
+  // A route for a request can exclusively be a route entry or a redirect entry.
+  if (isRedirect()) {
+    return nullptr;
+  } else {
+    return this;
+  }
+}
+
 PrefixRouteEntryImpl::PrefixRouteEntryImpl(const VirtualHostImpl& vhost, const Json::Object& route,
                                            Runtime::Loader& loader)
     : RouteEntryImplBase(vhost, route, loader), prefix_(route.getString("prefix")) {}
@@ -307,26 +325,19 @@ RouteMatcher::RouteMatcher(const Json::Object& config, Runtime::Loader& runtime,
   }
 }
 
-const RedirectEntry* VirtualHostImpl::redirectFromEntries(const Http::HeaderMap& headers,
-                                                          uint64_t random_value) const {
-  // First we check to see if we have any vhost level SSL requirements.
+const Route* VirtualHostImpl::getRouteFromEntries(const Http::HeaderMap& headers,
+                                                  uint64_t random_value) const {
+  // First check for ssl redirect.
   if (ssl_requirements_ == SslRequirements::ALL && headers.ForwardedProto()->value() != "https") {
-    return &SSL_REDIRECTOR;
+    return &SSL_REDIRECT_ROUTE;
   } else if (ssl_requirements_ == SslRequirements::EXTERNAL_ONLY &&
              headers.ForwardedProto()->value() != "https" && !headers.EnvoyInternalRequest()) {
-    return &SSL_REDIRECTOR;
-  } else {
-    // See if there is a route level redirect that we need to do. We search for a route entry
-    // and see if it has redirect information on it.
-    return routeFromEntries(headers, true, random_value);
+    return &SSL_REDIRECT_ROUTE;
   }
-}
 
-const RouteEntryImplBase* VirtualHostImpl::routeFromEntries(const Http::HeaderMap& headers,
-                                                            bool redirect,
-                                                            uint64_t random_value) const {
+  // Check for a route that matches the request.
   for (const RouteEntryImplBasePtr& route : routes_) {
-    if (redirect == route->isRedirect() && route->matches(headers, random_value)) {
+    if (route->matches(headers, random_value)) {
       return route.get();
     }
   }
@@ -350,28 +361,18 @@ const VirtualHostImpl* RouteMatcher::findVirtualHost(const Http::HeaderMap& head
   return nullptr;
 }
 
-const RedirectEntry* RouteMatcher::redirectRequest(const Http::HeaderMap& headers,
-                                                   uint64_t random_value) const {
-  const VirtualHostImpl* virtual_host = findVirtualHost(headers);
-  if (virtual_host) {
-    return virtual_host->redirectFromEntries(headers, random_value);
-  } else {
-    return nullptr;
-  }
-}
-
-const RouteEntry* RouteMatcher::routeForRequest(const Http::HeaderMap& headers,
-                                                uint64_t random_value) const {
+const Route* RouteMatcher::route(const Http::HeaderMap& headers, uint64_t random_value) const {
   const VirtualHostImpl* virtual_host = findVirtualHost(headers);
   if (virtual_host) {
-    return virtual_host->routeFromEntries(headers, false, random_value);
+    return virtual_host->getRouteFromEntries(headers, random_value);
   } else {
     return nullptr;
   }
 }
 
 const VirtualHostImpl::CatchAllVirtualCluster VirtualHostImpl::VIRTUAL_CLUSTER_CATCH_ALL;
-const SslRedirector VirtualHostImpl::SSL_REDIRECTOR;
+const SslRedirector SslRedirectRoute::SSL_REDIRECTOR;
+const SslRedirectRoute VirtualHostImpl::SSL_REDIRECT_ROUTE;
 
 const VirtualCluster*
 VirtualHostImpl::virtualClusterFromEntries(const Http::HeaderMap& headers) const {

source/common/router/config_impl.h

@@ -40,6 +40,16 @@ class SslRedirector : public RedirectEntry {
   std::string newPath(const Http::HeaderMap& headers) const override;
 };
 
+class SslRedirectRoute : public Route {
+public:
+  // Router::Route
+  const RedirectEntry* redirectEntry() const override { return &SSL_REDIRECTOR; }
+  const RouteEntry* routeEntry() const override { return nullptr; }
+
+private:
+  static const SslRedirector SSL_REDIRECTOR;
+};
+
 /**
  * Utility routines for loading route configuration and matching runtime request headers.
  */
@@ -80,10 +90,7 @@ class VirtualHostImpl : public VirtualHost {
   VirtualHostImpl(const Json::Object& virtual_host, Runtime::Loader& runtime,
                   Upstream::ClusterManager& cm);
 
-  const RedirectEntry* redirectFromEntries(const Http::HeaderMap& headers,
-                                           uint64_t random_value) const;
-  const RouteEntryImplBase* routeFromEntries(const Http::HeaderMap& headers, bool redirect,
-                                             uint64_t random_value) const;
+  const Route* getRouteFromEntries(const Http::HeaderMap& headers, uint64_t random_value) const;
   bool usesRuntime() const;
   const VirtualCluster* virtualClusterFromEntries(const Http::HeaderMap& headers) const;
 
@@ -118,7 +125,7 @@ class VirtualHostImpl : public VirtualHost {
   };
 
   static const CatchAllVirtualCluster VIRTUAL_CLUSTER_CATCH_ALL;
-  static const SslRedirector SSL_REDIRECTOR;
+  static const SslRedirectRoute SSL_REDIRECT_ROUTE;
 
   const std::string name_;
   std::vector<RouteEntryImplBasePtr> routes_;
@@ -164,7 +171,7 @@ class ShadowPolicyImpl : public ShadowPolicy {
 /**
  * Base implementation for all route entries.
  */
-class RouteEntryImplBase : public RouteEntry, public Matchable, public RedirectEntry {
+class RouteEntryImplBase : public RouteEntry, public Matchable, public RedirectEntry, public Route {
 public:
   RouteEntryImplBase(const VirtualHostImpl& vhost, const Json::Object& route,
                      Runtime::Loader& loader);
@@ -191,6 +198,10 @@ class RouteEntryImplBase : public RouteEntry, public Matchable, public RedirectE
   // Router::Matchable
   bool matches(const Http::HeaderMap& headers, uint64_t random_value) const override;
 
+  // Router::Route
+  const RedirectEntry* redirectEntry() const override;
+  const RouteEntry* routeEntry() const override;
+
 protected:
   const bool case_sensitive_;
   const std::string prefix_rewrite_;
@@ -267,8 +278,7 @@ class RouteMatcher {
 public:
   RouteMatcher(const Json::Object& config, Runtime::Loader& runtime, Upstream::ClusterManager& cm);
 
-  const RedirectEntry* redirectRequest(const Http::HeaderMap& headers, uint64_t random_value) const;
-  const RouteEntry* routeForRequest(const Http::HeaderMap& headers, uint64_t random_value) const;
+  const Route* route(const Http::HeaderMap& headers, uint64_t random_value) const;
   bool usesRuntime() const { return uses_runtime_; }
 
 private:
@@ -287,14 +297,8 @@ class ConfigImpl : public Config {
   ConfigImpl(const Json::Object& config, Runtime::Loader& runtime, Upstream::ClusterManager& cm);
 
   // Router::Config
-  const RedirectEntry* redirectRequest(const Http::HeaderMap& headers,
-                                       uint64_t random_value) const override {
-    return route_matcher_->redirectRequest(headers, random_value);
-  }
-
-  const RouteEntry* routeForRequest(const Http::HeaderMap& headers,
-                                    uint64_t random_value) const override {
-    return route_matcher_->routeForRequest(headers, random_value);
+  const Route* route(const Http::HeaderMap& headers, uint64_t random_value) const override {
+    return route_matcher_->route(headers, random_value);
   }
 
   const std::list<Http::LowerCaseString>& internalOnlyHeaders() const override {
@@ -325,13 +329,7 @@ class ConfigImpl : public Config {
 class NullConfigImpl : public Config {
 public:
   // Router::Config
-  const RedirectEntry* redirectRequest(const Http::HeaderMap&, uint64_t) const override {
-    return nullptr;
-  }
-
-  const RouteEntry* routeForRequest(const Http::HeaderMap&, uint64_t) const override {
-    return nullptr;
-  }
+  const Route* route(const Http::HeaderMap&, uint64_t) const override { return nullptr; }
 
   const std::list<Http::LowerCaseString>& internalOnlyHeaders() const override {
     return internal_only_headers_;