Use dynamicMetadata to construct metadataMatchCriteria() (#3254)

This enables filter writers to construct metadataMatchCriteria() from a RequestInfo (e.g.: header value). This is useful given that currently routes need to be statically matched when using metadata, e.g.: --- match: prefix: / route: cluster: cluster-name metadata_match: filter_metadata: envoy.lb: version: '1.0' For setups in which RDS isn't used and routes cannot be constructed dynamically, a filter can be used to match which endpoints should be used. So a filter could do something like: auto header_value = std::string(header_entry->value().getStringView()); auto keyval = MessageUtil::keyValueStruct("version", header_value); callbacks_->requestInfo().setDynamicMetadata("envoy.lb", keyval); to determine the version for this request. Signed-off-by: Raul Gutierrez Segales <[email protected]>
envoyproxy · May 4, 2018 · 2d25067 · 2d25067
1 parent 45de3fb
commit 2d25067
Show file tree

Hide file tree

Showing 17 changed files with 136 additions and 25 deletions.
diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h
@@ -334,6 +334,9 @@ class MetadataMatchCriterion {
 
 typedef std::shared_ptr<const MetadataMatchCriterion> MetadataMatchCriterionConstSharedPtr;
 
+class MetadataMatchCriteria;
+typedef std::unique_ptr<const MetadataMatchCriteria> MetadataMatchCriteriaConstPtr;
+
 class MetadataMatchCriteria {
 public:
   virtual ~MetadataMatchCriteria() {}
@@ -345,6 +348,17 @@ class MetadataMatchCriteria {
    */
   virtual const std::vector<MetadataMatchCriterionConstSharedPtr>&
   metadataMatchCriteria() const PURE;
+
+  /**
+   * Creates a new MetadataMatchCriteria, merging existing
+   * metadata criteria with the provided criteria. The result criteria is the
+   * combination of both sets of criteria, with those from the metadata_matches
+   * ProtobufWkt::Struct taking precedence.
+   * @param metadata_matches supplies the new criteria.
+   * @return MetadataMatchCriteriaConstPtr the result criteria.
+   */
+  virtual MetadataMatchCriteriaConstPtr
+  mergeMatchCriteria(const ProtobufWkt::Struct& metadata_matches) const PURE;
 };
 
 /**

diff --git a/include/envoy/upstream/load_balancer.h b/include/envoy/upstream/load_balancer.h
@@ -30,7 +30,7 @@ class LoadBalancerContext {
    * @return Router::MetadataMatchCriteria* metadata for use in selecting a subset of hosts
    *         during load balancing.
    */
-  virtual const Router::MetadataMatchCriteria* metadataMatchCriteria() const PURE;
+  virtual const Router::MetadataMatchCriteria* metadataMatchCriteria() PURE;
 
   /**
    * @return const Network::Connection* the incoming connection or nullptr to use during load

diff --git a/source/common/http/websocket/ws_handler_impl.h b/source/common/http/websocket/ws_handler_impl.h
@@ -35,7 +35,7 @@ class WsHandlerImpl : public Extensions::NetworkFilters::TcpProxy::TcpProxyFilte
                 Network::ReadFilterCallbacks* read_callbacks);
 
   // Upstream::LoadBalancerContext
-  const Router::MetadataMatchCriteria* metadataMatchCriteria() const override {
+  const Router::MetadataMatchCriteria* metadataMatchCriteria() override {
     return route_entry_.metadataMatchCriteria();
   }
 

diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h
@@ -258,14 +258,8 @@ class MetadataMatchCriteriaImpl : public MetadataMatchCriteria {
   MetadataMatchCriteriaImpl(const ProtobufWkt::Struct& metadata_matches)
       : metadata_match_criteria_(extractMetadataMatchCriteria(nullptr, metadata_matches)){};
 
-  /**
-   * Creates a new MetadataMatchCriteriaImpl, merging existing
-   * metadata criteria this criteria. The result criteria is the
-   * combination of both sets of criteria, with those from the
-   * ProtobufWkt::Struct taking precedence.
-   */
-  MetadataMatchCriteriaImplConstPtr
-  mergeMatchCriteria(const ProtobufWkt::Struct& metadata_matches) const {
+  MetadataMatchCriteriaConstPtr
+  mergeMatchCriteria(const ProtobufWkt::Struct& metadata_matches) const override {
     return MetadataMatchCriteriaImplConstPtr(
         new MetadataMatchCriteriaImpl(extractMetadataMatchCriteria(this, metadata_matches)));
   }
@@ -497,7 +491,7 @@ class RouteEntryImplBase : public RouteEntry,
     const std::string runtime_key_;
     Runtime::Loader& loader_;
     const uint64_t cluster_weight_;
-    MetadataMatchCriteriaImplConstPtr cluster_metadata_match_criteria_;
+    MetadataMatchCriteriaConstPtr cluster_metadata_match_criteria_;
     HeaderParserPtr request_headers_parser_;
     HeaderParserPtr response_headers_parser_;
     PerFilterConfigs per_filter_configs_;
@@ -541,7 +535,7 @@ class RouteEntryImplBase : public RouteEntry,
   std::vector<WeightedClusterEntrySharedPtr> weighted_clusters_;
   const uint64_t total_cluster_weight_;
   std::unique_ptr<const HashPolicyImpl> hash_policy_;
-  MetadataMatchCriteriaImplConstPtr metadata_match_criteria_;
+  MetadataMatchCriteriaConstPtr metadata_match_criteria_;
   HeaderParserPtr request_headers_parser_;
   HeaderParserPtr response_headers_parser_;
   envoy::api::v2::core::Metadata metadata_;

diff --git a/source/common/router/router.h b/source/common/router/router.h
@@ -21,8 +21,10 @@
 #include "common/common/hash.h"
 #include "common/common/hex.h"
 #include "common/common/logger.h"
+#include "common/config/well_known_names.h"
 #include "common/http/utility.h"
 #include "common/request_info/request_info_impl.h"
+#include "common/router/config_impl.h"
 
 namespace Envoy {
 namespace Router {
@@ -161,8 +163,27 @@ class Filter : Logger::Loggable<Logger::Id::router>,
     }
     return {};
   }
-  const Router::MetadataMatchCriteria* metadataMatchCriteria() const override {
+  const Router::MetadataMatchCriteria* metadataMatchCriteria() override {
     if (route_entry_) {
+      // Have we been called before? If so, there's no need to recompute because
+      // by the time this method is called for the first time, route_entry_ should
+      // not change anymore.
+      if (metadata_match_ != nullptr) {
+        return metadata_match_.get();
+      }
+
+      // The request's metadata, if present, takes precedence over the route's.
+      const auto& request_metadata = callbacks_->requestInfo().dynamicMetadata().filter_metadata();
+      const auto filter_it = request_metadata.find(Envoy::Config::MetadataFilters::get().ENVOY_LB);
+      if (filter_it != request_metadata.end()) {
+        if (route_entry_->metadataMatchCriteria() != nullptr) {
+          metadata_match_ =
+              route_entry_->metadataMatchCriteria()->mergeMatchCriteria(filter_it->second);
+        } else {
+          metadata_match_ = std::make_unique<Router::MetadataMatchCriteriaImpl>(filter_it->second);
+        }
+        return metadata_match_.get();
+      }
       return route_entry_->metadataMatchCriteria();
     }
     return nullptr;
@@ -343,6 +364,7 @@ class Filter : Logger::Loggable<Logger::Id::router>,
   MonotonicTime downstream_request_complete_time_;
   uint32_t buffer_limit_{0};
   bool stream_destroyed_{};
+  MetadataMatchCriteriaConstPtr metadata_match_;
 
   // list of cookies to add to upstream headers
   std::vector<std::string> downstream_set_cookies_;

diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h
@@ -166,7 +166,7 @@ class InstanceImpl : public Instance {
     // TODO(danielhochman): convert to HashUtil::xxHash64 when we have a migration strategy.
     // Upstream::LoadBalancerContext
     absl::optional<uint64_t> computeHashKey() override { return hash_key_; }
-    const Router::MetadataMatchCriteria* metadataMatchCriteria() const override { return nullptr; }
+    const Router::MetadataMatchCriteria* metadataMatchCriteria() override { return nullptr; }
     const Network::Connection* downstreamConnection() const override { return nullptr; }
 
     const absl::optional<uint64_t> hash_key_;

diff --git a/source/extensions/filters/network/tcp_proxy/tcp_proxy.h b/source/extensions/filters/network/tcp_proxy/tcp_proxy.h
@@ -154,7 +154,7 @@ class TcpProxyFilter : public Network::ReadFilter,
 
   // Upstream::LoadBalancerContext
   absl::optional<uint64_t> computeHashKey() override { return {}; }
-  const Router::MetadataMatchCriteria* metadataMatchCriteria() const override {
+  const Router::MetadataMatchCriteria* metadataMatchCriteria() override {
     if (config_) {
       return config_->metadataMatchCriteria();
     }

diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc
@@ -3575,7 +3575,7 @@ TEST(MetadataMatchCriteriaImpl, Merge) {
   mutable_fields->insert({"b++", v2});
   mutable_fields->insert({"c", v3});
 
-  MetadataMatchCriteriaImplConstPtr matches = parent_matches.mergeMatchCriteria(metadata_struct);
+  MetadataMatchCriteriaConstPtr matches = parent_matches.mergeMatchCriteria(metadata_struct);
 
   EXPECT_EQ(matches->metadataMatchCriteria().size(), 4);
   auto it = matches->metadataMatchCriteria().begin();

diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc
@@ -4,7 +4,10 @@
 
 #include "common/buffer/buffer_impl.h"
 #include "common/common/empty_string.h"
+#include "common/config/metadata.h"
+#include "common/config/well_known_names.h"
 #include "common/network/utility.h"
+#include "common/router/config_impl.h"
 #include "common/router/router.h"
 #include "common/tracing/http_tracer_impl.h"
 #include "common/upstream/upstream_impl.h"
@@ -104,6 +107,72 @@ class RouterTestBase : public testing::Test {
     return AssertionSuccess();
   }
 
+  void verifyMetadataMatchCriteriaFromRequest(bool route_entry_has_match) {
+    ProtobufWkt::Struct request_struct, route_struct;
+    ProtobufWkt::Value val;
+
+    // Populate metadata like RequestInfo.setDynamicMetadata() would.
+    auto& fields_map = *request_struct.mutable_fields();
+    val.set_string_value("v3.1");
+    fields_map["version"] = val;
+    val.set_string_value("devel");
+    fields_map["stage"] = val;
+    (*callbacks_.request_info_.metadata_
+          .mutable_filter_metadata())[Envoy::Config::MetadataFilters::get().ENVOY_LB] =
+        request_struct;
+
+    // Populate route entry's metadata which will be overridden.
+    val.set_string_value("v3.0");
+    fields_map = *request_struct.mutable_fields();
+    fields_map["version"] = val;
+    MetadataMatchCriteriaImpl route_entry_matches(route_struct);
+
+    if (route_entry_has_match) {
+      ON_CALL(callbacks_.route_->route_entry_, metadataMatchCriteria())
+          .WillByDefault(Return(&route_entry_matches));
+    } else {
+      ON_CALL(callbacks_.route_->route_entry_, metadataMatchCriteria())
+          .WillByDefault(Return(nullptr));
+    }
+
+    EXPECT_CALL(cm_, httpConnPoolForCluster(_, _, _, _))
+        .WillOnce(
+            Invoke([&](const std::string&, Upstream::ResourcePriority, Http::Protocol,
+                       Upstream::LoadBalancerContext* context) -> Http::ConnectionPool::Instance* {
+              auto match = context->metadataMatchCriteria()->metadataMatchCriteria();
+              EXPECT_EQ(match.size(), 2);
+              auto it = match.begin();
+
+              // Note: metadataMatchCriteria() keeps its entries sorted, so the order for checks
+              // below matters.
+
+              // `stage` was only set by the request, not by the route entry.
+              EXPECT_EQ((*it)->name(), "stage");
+              EXPECT_EQ((*it)->value().value().string_value(), "devel");
+              it++;
+
+              // `version` should be what came from the request, overriding the route entry.
+              EXPECT_EQ((*it)->name(), "version");
+              EXPECT_EQ((*it)->value().value().string_value(), "v3.1");
+
+              // When metadataMatchCriteria() is computed from dynamic metadata, the result should
+              // be cached.
+              EXPECT_EQ(context->metadataMatchCriteria(), context->metadataMatchCriteria());
+
+              return &cm_.conn_pool_;
+            }));
+    EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_));
+    expectResponseTimerCreate();
+
+    Http::TestHeaderMapImpl headers;
+    HttpTestUtility::addDefaultHeaders(headers);
+    router_.decodeHeaders(headers, true);
+
+    // When the router filter gets reset we should cancel the pool request.
+    EXPECT_CALL(cancellable_, cancel());
+    router_.onDestroy();
+  }
+
   std::string upstream_zone_{"to_az"};
   envoy::api::v2::core::Locality upstream_locality_;
   Stats::IsolatedStoreImpl stats_store_;
@@ -466,8 +535,6 @@ TEST_F(RouterTest, AddMultipleCookies) {
 TEST_F(RouterTest, MetadataNoOp) { EXPECT_EQ(nullptr, router_.metadataMatchCriteria()); }
 
 TEST_F(RouterTest, MetadataMatchCriteria) {
-  MockMetadataMatchCriteria matches;
-
   ON_CALL(callbacks_.route_->route_entry_, metadataMatchCriteria())
       .WillByDefault(Return(&callbacks_.route_->route_entry_.metadata_matches_criteria_));
   EXPECT_CALL(cm_, httpConnPoolForCluster(_, _, _, _))
@@ -490,6 +557,14 @@ TEST_F(RouterTest, MetadataMatchCriteria) {
   router_.onDestroy();
 }
 
+TEST_F(RouterTest, MetadataMatchCriteriaFromRequest) {
+  verifyMetadataMatchCriteriaFromRequest(true);
+}
+
+TEST_F(RouterTest, MetadataMatchCriteriaFromRequestNoRouteEntryMatch) {
+  verifyMetadataMatchCriteriaFromRequest(false);
+}
+
 TEST_F(RouterTest, NoMetadataMatchCriteria) {
   ON_CALL(callbacks_.route_->route_entry_, metadataMatchCriteria()).WillByDefault(Return(nullptr));
   EXPECT_CALL(cm_, httpConnPoolForCluster(_, _, _, _))

diff --git a/test/common/upstream/load_balancer_benchmark.cc b/test/common/upstream/load_balancer_benchmark.cc
@@ -101,7 +101,7 @@ class TestLoadBalancerContext : public LoadBalancerContext {
 public:
   // Upstream::LoadBalancerContext
   absl::optional<uint64_t> computeHashKey() override { return hash_key_; }
-  const Router::MetadataMatchCriteria* metadataMatchCriteria() const override { return nullptr; }
+  const Router::MetadataMatchCriteria* metadataMatchCriteria() override { return nullptr; }
   const Network::Connection* downstreamConnection() const override { return nullptr; }
 
   absl::optional<uint64_t> hash_key_;

diff --git a/test/common/upstream/maglev_lb_test.cc b/test/common/upstream/maglev_lb_test.cc
@@ -12,7 +12,7 @@ class TestLoadBalancerContext : public LoadBalancerContext {
 
   // Upstream::LoadBalancerContext
   absl::optional<uint64_t> computeHashKey() override { return hash_key_; }
-  const Router::MetadataMatchCriteria* metadataMatchCriteria() const override { return nullptr; }
+  const Router::MetadataMatchCriteria* metadataMatchCriteria() override { return nullptr; }
   const Network::Connection* downstreamConnection() const override { return nullptr; }
 
   absl::optional<uint64_t> hash_key_;

diff --git a/test/common/upstream/original_dst_cluster_test.cc b/test/common/upstream/original_dst_cluster_test.cc
@@ -38,7 +38,7 @@ class TestLoadBalancerContext : public LoadBalancerContext {
   // Upstream::LoadBalancerContext
   absl::optional<uint64_t> computeHashKey() override { return 0; }
   const Network::Connection* downstreamConnection() const override { return connection_; }
-  const Router::MetadataMatchCriteria* metadataMatchCriteria() const override { return nullptr; }
+  const Router::MetadataMatchCriteria* metadataMatchCriteria() override { return nullptr; }
 
   absl::optional<uint64_t> hash_key_;
   const Network::Connection* connection_;

diff --git a/test/common/upstream/ring_hash_lb_test.cc b/test/common/upstream/ring_hash_lb_test.cc
@@ -28,7 +28,7 @@ class TestLoadBalancerContext : public LoadBalancerContext {
 
   // Upstream::LoadBalancerContext
   absl::optional<uint64_t> computeHashKey() override { return hash_key_; }
-  const Router::MetadataMatchCriteria* metadataMatchCriteria() const override { return nullptr; }
+  const Router::MetadataMatchCriteria* metadataMatchCriteria() override { return nullptr; }
   const Network::Connection* downstreamConnection() const override { return nullptr; }
 
   absl::optional<uint64_t> hash_key_;

diff --git a/test/common/upstream/subset_lb_test.cc b/test/common/upstream/subset_lb_test.cc
@@ -78,6 +78,11 @@ class TestMetadataMatchCriteria : public Router::MetadataMatchCriteria {
     return matches_;
   }
 
+  Router::MetadataMatchCriteriaConstPtr
+  mergeMatchCriteria(const ProtobufWkt::Struct&) const override {
+    return nullptr;
+  }
+
 private:
   std::vector<Router::MetadataMatchCriterionConstSharedPtr> matches_;
 };
@@ -92,9 +97,7 @@ class TestLoadBalancerContext : public LoadBalancerContext {
   // Upstream::LoadBalancerContext
   absl::optional<uint64_t> computeHashKey() override { return {}; }
   const Network::Connection* downstreamConnection() const override { return nullptr; }
-  const Router::MetadataMatchCriteria* metadataMatchCriteria() const override {
-    return matches_.get();
-  }
+  const Router::MetadataMatchCriteria* metadataMatchCriteria() override { return matches_.get(); }
 
 private:
   const std::shared_ptr<Router::MetadataMatchCriteria> matches_;

diff --git a/test/mocks/request_info/mocks.cc b/test/mocks/request_info/mocks.cc
@@ -40,6 +40,7 @@ MockRequestInfo::MockRequestInfo()
   ON_CALL(*this, responseCode()).WillByDefault(ReturnPointee(&response_code_));
   ON_CALL(*this, bytesReceived()).WillByDefault(ReturnPointee(&bytes_received_));
   ON_CALL(*this, bytesSent()).WillByDefault(ReturnPointee(&bytes_sent_));
+  ON_CALL(*this, dynamicMetadata()).WillByDefault(ReturnRef(metadata_));
 }
 
 MockRequestInfo::~MockRequestInfo() {}

diff --git a/test/mocks/request_info/mocks.h b/test/mocks/request_info/mocks.h
@@ -73,6 +73,7 @@ class MockRequestInfo : public RequestInfo {
   absl::optional<uint32_t> response_code_;
   uint64_t bytes_received_{};
   uint64_t bytes_sent_{};
+  envoy::api::v2::core::Metadata metadata_;
 };
 
 } // namespace RequestInfo

diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h
@@ -189,6 +189,7 @@ class MockMetadataMatchCriteria : public MetadataMatchCriteria {
   // Router::MetadataMatchCriteria
   MOCK_CONST_METHOD0(metadataMatchCriteria,
                      const std::vector<MetadataMatchCriterionConstSharedPtr>&());
+  MOCK_CONST_METHOD1(mergeMatchCriteria, MetadataMatchCriteriaConstPtr(const ProtobufWkt::Struct&));
 };
 
 class MockPathMatchCriterion : public PathMatchCriterion {