noop changes

Changes to add the Noop routenames being hit to access log (envoyproxy#78)

* Changes to add the Noop routenames being hit to access log
* Must keep flag on whether we hit noop in past
* Cant use dynamic metadata
* Testcases
* Fixing tests
* Adding config testcase and addressing Balas comments
* Dont indent line

Signed-off-by: pitiwari <pitiwari@ebay.com>

api/envoy/api/v2/route/route.proto

@@ -180,6 +180,9 @@ message Route {
 
     // Return an arbitrary HTTP response directly, without proxying.
     DirectResponseAction direct_response = 7;
+
+    // Return an noop action 
+    Noop  noop = 16;
   }
 
   // The Metadata field can be used to provide additional information
@@ -1062,6 +1065,13 @@ message DirectResponseAction {
   core.DataSource body = 2;
 }
 
+message Noop {
+      // If set to true, then when this route is hit, stream info (and hence access logs)
+      // will be populated with the name of this route. Only the NOOP routes that set this
+      // flag will be added to access logs.
+      bool add_route_name_to_stream_info = 1;
+}
+
 message Decorator {
   // The operation name associated with the request matched to this route. If tracing is
   // enabled, this information will be used as the span name reported for this request.

include/envoy/router/router.h

@@ -93,6 +93,10 @@ class DirectResponseEntry : public ResponseEntry {
    * @return std::string& the name of the route.
    */
   virtual const std::string& routeName() const PURE;
+
+  virtual  bool noop() const PURE;
+
+  virtual bool addRouteNameToStreamInfo() const PURE;
 };
 
 /**
@@ -766,6 +770,10 @@ class RouteEntry : public ResponseEntry {
    * @return std::string& the name of the route.
    */
   virtual const std::string& routeName() const PURE;
+
+  virtual  bool noop() const  PURE;
+
+  virtual bool addRouteNameToStreamInfo() const PURE;
 };
 
 /**
@@ -880,7 +888,7 @@ class Config {
    */
   virtual RouteConstSharedPtr route(const Http::HeaderMap& headers,
                                     const StreamInfo::StreamInfo& stream_info,
-                                    uint64_t random_value) const PURE;
+                                    uint64_t random_value, uint32_t &) const PURE;
 
   /**
    * Return a list of headers that will be cleaned from any requests that are not from an internal

include/envoy/stream_info/stream_info.h

@@ -217,6 +217,11 @@ class StreamInfo {
    * @return std::string& the name of the route.
    */
   virtual const std::string& getRouteName() const PURE;
+
+  virtual void setNoopRouteNames(std::string name) PURE;
+
+  virtual const std::string& getNoopRouteNames() const PURE;
+
   /**
    * @param bytes_received denotes number of bytes to add to total received bytes.
    */

source/common/access_log/access_log_formatter.cc

@@ -498,7 +498,12 @@ StreamInfoFormatter::StreamInfoFormatter(const std::string& field_name) {
         return UnspecifiedValueString;
       }
     };
-  } else {
+  } else if (field_name == "NOOP_ROUTE_NAME") {
+    field_extractor_ = [](const StreamInfo::StreamInfo& stream_info) {
+      std::string noop_route_names = stream_info.getNoopRouteNames();
+      return noop_route_names.empty() ? UnspecifiedValueString : noop_route_names;
+    };
+   }  else {
     throw EnvoyException(fmt::format("Not supported field in StreamInfo: {}", field_name));
   }
 }

source/common/http/async_client_impl.h

@@ -161,7 +161,7 @@ class AsyncStreamImpl : public AsyncClient::Stream,
 
   struct NullConfig : public Router::Config {
     Router::RouteConstSharedPtr route(const Http::HeaderMap&, const StreamInfo::StreamInfo&,
-                                      uint64_t) const override {
+                                      uint64_t, uint32_t &) const override {
       return nullptr;
     }
 
@@ -273,7 +273,9 @@ class AsyncStreamImpl : public AsyncClient::Stream,
     }
     const std::string& routeName() const override { return route_name_; }
     std::unique_ptr<const HashPolicyImpl> hash_policy_;
+    bool noop() const override { return false;}
     static const NullHedgePolicy hedge_policy_;
+    bool addRouteNameToStreamInfo() const override { return false;}
     static const NullRateLimitPolicy rate_limit_policy_;
     static const NullRetryPolicy retry_policy_;
     static const NullShadowPolicy shadow_policy_;

source/common/http/conn_manager_impl.cc

@@ -1302,7 +1302,7 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() {
       snapScopedRouteConfig();
     }
     if (snapped_route_config_ != nullptr) {
-      route = snapped_route_config_->route(*request_headers_, stream_info_, stream_id_);
+      route = snapped_route_config_->route(*request_headers_, stream_info_, stream_id_, route_index_);
     }
   }
   stream_info_.route_entry_ = route ? route->routeEntry() : nullptr;
@@ -2032,25 +2032,32 @@ Tracing::Span& ConnectionManagerImpl::ActiveStreamFilterBase::activeSpan() {
 Tracing::Config& ConnectionManagerImpl::ActiveStreamFilterBase::tracingConfig() { return parent_; }
 
 Upstream::ClusterInfoConstSharedPtr ConnectionManagerImpl::ActiveStreamFilterBase::clusterInfo() {
-  // NOTE: Refreshing route caches clusterInfo as well.
-  if (!parent_.cached_route_.has_value()) {
-    parent_.refreshCachedRoute();
-  }
-
+  if ( parent_.cached_route_.has_value()) {
+      if( parent_.cached_route_.value() != nullptr && parent_.cached_route_.value()->routeEntry() && parent_.cached_route_.value()->routeEntry()->noop()) {
+          parent_.refreshCachedRoute();
+       }
+  } else {
+     // NOTE: Refreshing route caches clusterInfo as well.
+     parent_.refreshCachedRoute();
+  } 
   return parent_.cached_cluster_info_.value();
 }
 
 Router::RouteConstSharedPtr ConnectionManagerImpl::ActiveStreamFilterBase::route() {
-  if (!parent_.cached_route_.has_value()) {
-    parent_.refreshCachedRoute();
-  }
-
+  if ( parent_.cached_route_.has_value()) {
+      if( parent_.cached_route_.value() != nullptr && parent_.cached_route_.value()->routeEntry() && parent_.cached_route_.value()->routeEntry()->noop()) {
+          parent_.refreshCachedRoute();
+       }
+  } else {
+     parent_.refreshCachedRoute();
+  } 
   return parent_.cached_route_.value();
 }
 
 void ConnectionManagerImpl::ActiveStreamFilterBase::clearRouteCache() {
   parent_.cached_route_ = absl::optional<Router::RouteConstSharedPtr>();
   parent_.cached_cluster_info_ = absl::optional<Upstream::ClusterInfoConstSharedPtr>();
+  parent_.route_index_ = 0;
 }
 
 Buffer::WatermarkBufferPtr ConnectionManagerImpl::ActiveStreamDecoderFilter::createBuffer() {

source/common/http/conn_manager_impl.h

@@ -631,6 +631,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     // Whether a filter has indicated that the response should be treated as a headers only
     // response.
     bool encoding_headers_only_{};
+    uint32_t route_index_{0};
     Network::Socket::OptionsSharedPtr upstream_options_;
   };
 

source/common/router/config_impl.cc

@@ -281,6 +281,8 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost,
       decorator_(parseDecorator(route)), route_tracing_(parseRouteTracing(route)),
       direct_response_code_(ConfigUtility::parseDirectResponseCode(route)),
       direct_response_body_(ConfigUtility::parseDirectResponseBody(route, factory_context.api())),
+      has_noop_(route.has_noop()),
+      add_route_name_to_stream_info_(route.has_noop() && route.noop().add_route_name_to_stream_info()),
       per_filter_configs_(route.typed_per_filter_config(), route.per_filter_config(),
                           factory_context, validator),
       route_name_(route.name()), time_source_(factory_context.dispatcher().timeSource()),
@@ -670,7 +672,7 @@ RouteConstSharedPtr RouteEntryImplBase::clusterEntry(const Http::HeaderMap& head
   // Gets the route object chosen from the list of weighted clusters
   // (if there is one) or returns self.
   if (weighted_clusters_.empty()) {
-    if (!cluster_name_.empty() || isDirectResponse()) {
+    if (!cluster_name_.empty() || isDirectResponse() || noop()) {
       return shared_from_this();
     } else {
       ASSERT(!cluster_header_name_.get().empty());
@@ -1032,14 +1034,13 @@ RouteMatcher::RouteMatcher(const envoy::api::v2::RouteConfiguration& route_confi
 
 RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const Http::HeaderMap& headers,
                                                          const StreamInfo::StreamInfo& stream_info,
-                                                         uint64_t random_value) const {
+                                                         uint64_t random_value, uint32_t &route_index) const {
   // No x-forwarded-proto header. This normally only happens when ActiveStream::decodeHeaders
   // bails early (as it rejects a request), so there is no routing is going to happen anyway.
   const auto* forwarded_proto_header = headers.ForwardedProto();
   if (forwarded_proto_header == nullptr) {
     return nullptr;
   }
-
   // First check for ssl redirect.
   if (ssl_requirements_ == SslRequirements::ALL && forwarded_proto_header->value() != "https") {
     return SSL_REDIRECT_ROUTE;
@@ -1048,16 +1049,17 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const Http::HeaderMap&
              !Http::HeaderUtility::isEnvoyInternalRequest(headers)) {
     return SSL_REDIRECT_ROUTE;
   }
-
   // Check for a route that matches the request.
-  for (const RouteEntryImplBaseConstSharedPtr& route : routes_) {
-    RouteConstSharedPtr route_entry = route->matches(headers, stream_info, random_value);
-    if (nullptr != route_entry) {
-      return route_entry;
-    }
-  }
 
-  return nullptr;
+  while( route_index <routes_.size() ) {
+       RouteEntryImplBaseConstSharedPtr route = routes_[route_index]; 
+       RouteConstSharedPtr route_entry = route->matches(headers, random_value);
+       route_index++;
+       if (nullptr != route_entry) {
+          return route_entry;
+       }
+  }
+    return nullptr;
 }
 
 const VirtualHostImpl* RouteMatcher::findVirtualHost(const Http::HeaderMap& headers) const {
@@ -1095,11 +1097,18 @@ const VirtualHostImpl* RouteMatcher::findVirtualHost(const Http::HeaderMap& head
 }
 
 RouteConstSharedPtr RouteMatcher::route(const Http::HeaderMap& headers,
+<<<<<<< d4fa470da75791976807d911333f066ecfcd09b8
                                         const StreamInfo::StreamInfo& stream_info,
                                         uint64_t random_value) const {
   const VirtualHostImpl* virtual_host = findVirtualHost(headers);
   if (virtual_host) {
     return virtual_host->getRouteFromEntries(headers, stream_info, random_value);
+=======
+                                        uint64_t random_value, uint32_t &route_index) const {
+  const VirtualHostImpl* virtual_host = findVirtualHost(headers);
+  if (virtual_host) {
+    return virtual_host->getRouteFromEntries(headers, random_value, route_index);
+>>>>>>> noop changes
   } else {
     return nullptr;
   }

source/common/router/config_impl.h

@@ -80,7 +80,8 @@ class SslRedirector : public DirectResponseEntry {
   Http::Code responseCode() const override { return Http::Code::MovedPermanently; }
   const std::string& responseBody() const override { return EMPTY_STRING; }
   const std::string& routeName() const override { return route_name_; }
-
+  bool noop() const { return false;} 
+  bool addRouteNameToStreamInfo() const { return false;}
 private:
   const std::string route_name_;
 };
@@ -95,7 +96,6 @@ class SslRedirectRoute : public Route {
   const RouteSpecificFilterConfig* perFilterConfig(const std::string&) const override {
     return nullptr;
   }
-
 private:
   static const SslRedirector SSL_REDIRECTOR;
 };
@@ -158,7 +158,7 @@ class VirtualHostImpl : public VirtualHost {
 
   RouteConstSharedPtr getRouteFromEntries(const Http::HeaderMap& headers,
                                           const StreamInfo::StreamInfo& stream_info,
-                                          uint64_t random_value) const;
+                                          uint64_t random_value, uint32_t &) const;
   const VirtualCluster* virtualClusterFromEntries(const Http::HeaderMap& headers) const;
   const ConfigImpl& globalRouteConfig() const { return global_route_config_; }
   const HeaderParser& requestHeaderParser() const { return *request_headers_parser_; }
@@ -445,7 +445,8 @@ class RouteEntryImplBase : public RouteEntry,
   void rewritePathHeader(Http::HeaderMap&, bool) const override {}
   Http::Code responseCode() const override { return direct_response_code_.value(); }
   const std::string& responseBody() const override { return direct_response_body_; }
-
+  bool noop() const override { return has_noop_;}
+  bool addRouteNameToStreamInfo() const override { return add_route_name_to_stream_info_;}
   // Router::Route
   const DirectResponseEntry* directResponseEntry() const override;
   const RouteEntry* routeEntry() const override;
@@ -483,6 +484,8 @@ class RouteEntryImplBase : public RouteEntry,
         : parent_(parent), cluster_name_(name) {}
 
     const std::string& routeName() const override { return parent_->routeName(); }
+    bool  noop() const override { return false; }
+    bool addRouteNameToStreamInfo() const override { return false;}
     // Router::RouteEntry
     const std::string& clusterName() const override { return cluster_name_; }
     Http::Code clusterNotFoundResponseCode() const override {
@@ -601,7 +604,8 @@ class RouteEntryImplBase : public RouteEntry,
     }
 
     const RouteSpecificFilterConfig* perFilterConfig(const std::string& name) const override;
-
+    bool noop() const override { return false;}
+    bool addRouteNameToStreamInfo() const override { return false;}
   private:
     const std::string runtime_key_;
     Runtime::Loader& loader_;
@@ -618,7 +622,6 @@ class RouteEntryImplBase : public RouteEntry,
 
   static std::multimap<std::string, std::string>
   parseOpaqueConfig(const envoy::api::v2::route::Route& route);
-
   static DecoratorConstPtr parseDecorator(const envoy::api::v2::route::Route& route);
 
   static RouteTracingConstPtr parseRouteTracing(const envoy::api::v2::route::Route& route);
@@ -688,6 +691,8 @@ class RouteEntryImplBase : public RouteEntry,
   const RouteTracingConstPtr route_tracing_;
   const absl::optional<Http::Code> direct_response_code_;
   std::string direct_response_body_;
+  bool has_noop_;
+  bool add_route_name_to_stream_info_;
   PerFilterConfigs per_filter_configs_;
   const std::string route_name_;
   TimeSource& time_source_;
@@ -784,7 +789,7 @@ class RouteMatcher {
                ProtobufMessage::ValidationVisitor& validator, bool validate_clusters);
 
   RouteConstSharedPtr route(const Http::HeaderMap& headers,
-                            const StreamInfo::StreamInfo& stream_info, uint64_t random_value) const;
+                            const StreamInfo::StreamInfo& stream_info, uint64_t random_value, uint32_t &) const;
 
 private:
   const VirtualHostImpl* findVirtualHost(const Http::HeaderMap& headers) const;
@@ -825,10 +830,8 @@ class ConfigImpl : public Config {
   const HeaderParser& responseHeaderParser() const { return *response_headers_parser_; };
 
   // Router::Config
-  RouteConstSharedPtr route(const Http::HeaderMap& headers,
-                            const StreamInfo::StreamInfo& stream_info,
-                            uint64_t random_value) const override {
-    return route_matcher_->route(headers, stream_info, random_value);
+  RouteConstSharedPtr route(const Http::HeaderMap& headers, const StreamInfo::StreamInfo& stream_info, uint64_t random_value, uint32_t &route_index) const override {
+    return route_matcher_->route(headers, random_value, route_index);
   }
 
   const std::list<Http::LowerCaseString>& internalOnlyHeaders() const override {
@@ -861,7 +864,7 @@ class NullConfigImpl : public Config {
 public:
   // Router::Config
   RouteConstSharedPtr route(const Http::HeaderMap&, const StreamInfo::StreamInfo&,
-                            uint64_t) const override {
+                            uint64_t, uint32_t &) const override {
     return nullptr;
   }
 

source/common/router/router.cc

@@ -370,6 +370,24 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e
 
   // Determine if there is a route entry or a direct response for the request.
   route_ = callbacks_->route();
+
+  // Contains all noop route names we hit while routing this request
+  std::string no_op_route_names("");
+  // if this is the noop route then keep on picking routes till we find non-noop route or run out of the routes
+  while (route_ && route_->routeEntry() && route_->routeEntry()->noop()) {
+         if (route_->routeEntry()->addRouteNameToStreamInfo()) {
+           no_op_route_names.append(route_->routeEntry()->routeName() + ";"); 
+         }
+         route_ = callbacks_->route();
+  }
+
+  ENVOY_STREAM_LOG(trace, "noop route names={}", *callbacks_, no_op_route_names);
+
+  // Add to access logs if we have encountered some noop routes
+  if (no_op_route_names.length() != 0) {
+    callbacks_->streamInfo().setNoopRouteNames(no_op_route_names);
+  }
+
   if (!route_) {
     config_.stats_.no_route_.inc();
     ENVOY_STREAM_LOG(debug, "no cluster match for URL '{}'", *callbacks_,

source/common/stream_info/stream_info_impl.h

@@ -132,6 +132,10 @@ struct StreamInfoImpl : public StreamInfo {
   void setRouteName(absl::string_view route_name) override {
     route_name_ = std::string(route_name);
   }
+
+  void setNoopRouteNames(std::string noop_route_names) { noop_route_names_ = noop_route_names; }
+
+  const std::string& getNoopRouteNames() const { return noop_route_names_; }
 
   const std::string& getRouteName() const override { return route_name_; }
 
@@ -244,6 +248,7 @@ struct StreamInfoImpl : public StreamInfo {
   envoy::api::v2::core::Metadata metadata_{};
   FilterStateImpl filter_state_{};
   std::string route_name_;
+  std::string noop_route_names_;
 
 private:
   uint64_t bytes_received_{};