Handle route_entry_->metadataMatchCriteria() == nullptr

Also: * add a test for the above * add default return value for MockRequestInfo.dynamicMetadata Signed-off-by: Raul Gutierrez Segales <[email protected]>
envoyproxy · htuch · May 4, 2018 · Apr 30, 2018 · Apr 30, 2018 · Apr 30, 2018
commit b261a237ae9ef38bb7fed4725fa3ba676f747e06
diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h
@@ -350,7 +350,7 @@ class MetadataMatchCriteria {
   metadataMatchCriteria() const PURE;
 
   /**
-   * Creates a new MetadataMatchCriteriaImpl, merging existing
+   * Creates a new MetadataMatchCriteria, 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.

diff --git a/source/common/router/router.h b/source/common/router/router.h
@@ -24,6 +24,7 @@
 #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 {
@@ -168,8 +169,12 @@ class Filter : Logger::Loggable<Logger::Id::router>,
       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()) {
-        metadata_match_ =
-            route_entry_->metadataMatchCriteria()->mergeMatchCriteria(filter_it->second);
+        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();

diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc
@@ -107,6 +107,54 @@ 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
+    val.set_string_value("v3.1");
+    auto& fields_map = *request_struct.mutable_fields();
+    fields_map["version"] = 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(), 1);
+                    auto it = match.begin();
+                    EXPECT_EQ((*it)->name(), "version");
+                    EXPECT_EQ((*it)->value().value().string_value(), "v3.1");
+                    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_;
@@ -469,9 +517,6 @@ TEST_F(RouterTest, AddMultipleCookies) {
 TEST_F(RouterTest, MetadataNoOp) { EXPECT_EQ(nullptr, router_.metadataMatchCriteria()); }
 
 TEST_F(RouterTest, MetadataMatchCriteria) {
-  envoy::api::v2::core::Metadata metadata;
-
-  EXPECT_CALL(callbacks_.request_info_, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata));
   ON_CALL(callbacks_.route_->route_entry_, metadataMatchCriteria())
       .WillByDefault(Return(&callbacks_.route_->route_entry_.metadata_matches_criteria_));
   EXPECT_CALL(cm_, httpConnPoolForCluster(_, _, _, _))
@@ -495,49 +540,14 @@ TEST_F(RouterTest, MetadataMatchCriteria) {
 }
 
 TEST_F(RouterTest, MetadataMatchCriteriaFromRequest) {
-  envoy::api::v2::core::Metadata metadata;
-  ProtobufWkt::Struct request_struct, route_struct;
-  ProtobufWkt::Value val;
-  MetadataMatchCriteriaImpl route_entry_matches(route_struct);
-
-  // populate metadata like RequestInfo.setDynamicMetadata() would
-  val.set_string_value("v3.1");
-  auto& fields_map = *request_struct.mutable_fields();
-  fields_map["version"] = val;
-  (*metadata.mutable_filter_metadata())[Envoy::Config::MetadataFilters::get().ENVOY_LB] =
-      request_struct;
-
-  EXPECT_CALL(callbacks_.request_info_, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata));
-  ON_CALL(callbacks_.route_->route_entry_, metadataMatchCriteria())
-      .WillByDefault(Return(&route_entry_matches));
-
-  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(), 1);
-            auto it = match.begin();
-            EXPECT_EQ((*it)->name(), "version");
-            EXPECT_EQ((*it)->value().value().string_value(), "v3.1");
-            return &cm_.conn_pool_;
-          }));
-  EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_));
-  expectResponseTimerCreate();
-
-  Http::TestHeaderMapImpl headers;
-  HttpTestUtility::addDefaultHeaders(headers);
-  router_.decodeHeaders(headers, true);
+  verifyMetadataMatchCriteriaFromRequest(true);
+}
 
-  // When the router filter gets reset we should cancel the pool request.
-  EXPECT_CALL(cancellable_, cancel());
-  router_.onDestroy();
+TEST_F(RouterTest, MetadataMatchCriteriaFromRequestNoRouteEntryMatch) {
+  verifyMetadataMatchCriteriaFromRequest(false);
 }
 
 TEST_F(RouterTest, NoMetadataMatchCriteria) {
-  envoy::api::v2::core::Metadata metadata;
-
-  EXPECT_CALL(callbacks_.request_info_, dynamicMetadata()).WillRepeatedly(ReturnRef(metadata));
   ON_CALL(callbacks_.route_->route_entry_, metadataMatchCriteria()).WillByDefault(Return(nullptr));
   EXPECT_CALL(cm_, httpConnPoolForCluster(_, _, _, _))
       .WillOnce(

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