thrift: Reject router config if cluster/weighted clusters/mirror clus…

…ter doesn't exist for static route config provider (#20577) * Reject thrift router config if cluster/weighted clusters/mirror cluster don't exist * Only expose cluster information to `RouteMatcher` * Fix router ratelimit test, add unit test, add test coverage * address rgs1's , tkovacs-2's comment, Tamas and Harvey's comments * remove redundant ref, dead code and * add comment to state the current status * add dep lib Signed-off-by: kuochunghsu <[email protected]>
envoyproxy · Apr 7, 2022 · 456e449 · 456e449
1 parent 0a587f2
commit 456e449
Show file tree

Hide file tree

Showing 17 changed files with 306 additions and 154 deletions.
diff --git a/envoy/rds/BUILD b/envoy/rds/BUILD
@@ -9,11 +9,21 @@ licenses(["notice"])  # Apache 2
 envoy_package()
 
 envoy_cc_library(
-    name = "rds_interface",
+    name = "rds_config_interface",
     hdrs = [
         "config.h",
+    ],
+)
+
+envoy_cc_library(
+    name = "rds_interface",
+    hdrs = [
         "config_traits.h",
         "route_config_provider.h",
         "route_config_update_receiver.h",
     ],
+    deps = [
+        ":rds_config_interface",
+        "//envoy/server:factory_context_interface",
+    ],
 )
diff --git a/envoy/rds/config_traits.h b/envoy/rds/config_traits.h
@@ -4,6 +4,7 @@
 
 #include "envoy/common/pure.h"
 #include "envoy/rds/config.h"
+#include "envoy/server/factory_context.h"
 
 #include "source/common/protobuf/protobuf.h"
 
@@ -52,9 +53,17 @@ class ConfigTraits {
    * guaranteed to match with the return value of ProtoTraits::resourceType.
    * Both dynamic or static cast can be applied to downcast the message
    * to the corresponding route configuration class.
+   * @param rc supplies the RouteConfiguration.
+   * @param context supplies the context of the server factory.
+   * @param validate_clusters_default specifies whether the clusters that the route
+   *    table refers to will be validated by the cluster manager. Currently thrift
+   *    route config provider manager validates the clusters for static route config
+   *    by default but doesn't validate the clusters for TRDS.
    * @throw EnvoyException if the new config can't be applied of.
    */
-  virtual ConfigConstSharedPtr createConfig(const Protobuf::Message& rc) const PURE;
+  virtual ConfigConstSharedPtr createConfig(const Protobuf::Message& rc,
+                                            Server::Configuration::ServerFactoryContext& context,
+                                            bool validate_clusters_default) const PURE;
 };
 
 } // namespace Rds

diff --git a/envoy/router/BUILD b/envoy/router/BUILD
@@ -74,7 +74,7 @@ envoy_cc_library(
         "//envoy/http:conn_pool_interface",
         "//envoy/http:hash_policy_interface",
         "//envoy/http:header_map_interface",
-        "//envoy/rds:rds_interface",
+        "//envoy/rds:rds_config_interface",
         "//envoy/tcp:conn_pool_interface",
         "//envoy/tracing:http_tracer_interface",
         "//envoy/upstream:resource_manager_interface",

diff --git a/source/common/rds/common/config_traits_impl.h b/source/common/rds/common/config_traits_impl.h
@@ -19,9 +19,12 @@ class ConfigTraitsImpl : public ConfigTraits {
     return std::make_shared<const NullConfigImpl>();
   }
 
-  ConfigConstSharedPtr createConfig(const Protobuf::Message& rc) const override {
+  ConfigConstSharedPtr createConfig(const Protobuf::Message& rc,
+                                    Server::Configuration::ServerFactoryContext& context,
+                                    bool validate_clusters_default) const override {
     ASSERT(dynamic_cast<const RouteConfiguration*>(&rc));
-    return std::make_shared<const ConfigImpl>(static_cast<const RouteConfiguration&>(rc));
+    return std::make_shared<const ConfigImpl>(static_cast<const RouteConfiguration&>(rc), context,
+                                              validate_clusters_default);
   }
 };
 

diff --git a/source/common/rds/route_config_update_receiver_impl.cc b/source/common/rds/route_config_update_receiver_impl.cc
@@ -8,14 +8,15 @@ namespace Rds {
 RouteConfigUpdateReceiverImpl::RouteConfigUpdateReceiverImpl(
     ConfigTraits& config_traits, ProtoTraits& proto_traits,
     Server::Configuration::ServerFactoryContext& factory_context)
-    : config_traits_(config_traits), proto_traits_(proto_traits),
+    : config_traits_(config_traits), proto_traits_(proto_traits), factory_context_(factory_context),
       time_source_(factory_context.timeSource()),
       route_config_proto_(proto_traits_.createEmptyProto()), last_config_hash_(0ull),
       config_(config_traits_.createNullConfig()) {}
 
 void RouteConfigUpdateReceiverImpl::updateConfig(
     std::unique_ptr<Protobuf::Message>&& route_config_proto) {
-  config_ = config_traits_.createConfig(*route_config_proto);
+  config_ = config_traits_.createConfig(*route_config_proto, factory_context_,
+                                        false /* not validate unknown cluster */);
   // If the above create config doesn't raise exception, update the
   // other cached config entries.
   route_config_proto_ = std::move(route_config_proto);

diff --git a/source/common/rds/route_config_update_receiver_impl.h b/source/common/rds/route_config_update_receiver_impl.h
@@ -32,6 +32,7 @@ class RouteConfigUpdateReceiverImpl : public RouteConfigUpdateReceiver {
 private:
   ConfigTraits& config_traits_;
   ProtoTraits& proto_traits_;
+  Server::Configuration::ServerFactoryContext& factory_context_;
   TimeSource& time_source_;
   ProtobufTypes::MessagePtr route_config_proto_;
   uint64_t last_config_hash_;

diff --git a/source/common/rds/static_route_config_provider_impl.cc b/source/common/rds/static_route_config_provider_impl.cc
@@ -11,7 +11,8 @@ StaticRouteConfigProviderImpl::StaticRouteConfigProviderImpl(
     RouteConfigProviderManager& route_config_provider_manager)
     : route_config_proto_(
           cloneProto(route_config_provider_manager.protoTraits(), route_config_proto)),
-      config_(config_traits.createConfig(*route_config_proto_)),
+      config_(config_traits.createConfig(*route_config_proto_, factory_context,
+                                         true /* validate unknown cluster */)),
       last_updated_(factory_context.timeSource().systemTime()),
       config_info_(ConfigInfo{*route_config_proto_, ""}),
       route_config_provider_manager_(route_config_provider_manager) {}

diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc
@@ -254,7 +254,7 @@ RouteConfigProviderPtr RouteConfigProviderManagerImpl::createStaticRouteConfigPr
     ProtobufMessage::ValidationVisitor& validator) {
   auto provider = manager_.addStaticProvider(
       [&optional_http_filters, &factory_context, &validator, &route_config, this]() {
-        ConfigTraitsImpl config_traits(optional_http_filters, factory_context, validator, true);
+        ConfigTraitsImpl config_traits(optional_http_filters, validator);
         return std::make_unique<StaticRouteConfigProviderImpl>(route_config, config_traits,
                                                                factory_context, manager_);
       });

diff --git a/source/common/router/route_config_update_receiver_impl.cc b/source/common/router/route_config_update_receiver_impl.cc
@@ -39,11 +39,14 @@ Rds::ConfigConstSharedPtr ConfigTraitsImpl::createNullConfig() const {
   return std::make_shared<NullConfigImpl>();
 }
 
-Rds::ConfigConstSharedPtr ConfigTraitsImpl::createConfig(const Protobuf::Message& rc) const {
+Rds::ConfigConstSharedPtr
+ConfigTraitsImpl::createConfig(const Protobuf::Message& rc,
+                               Server::Configuration::ServerFactoryContext& factory_context,
+                               bool validate_clusters_default) const {
   ASSERT(dynamic_cast<const envoy::config::route::v3::RouteConfiguration*>(&rc));
   return std::make_shared<ConfigImpl>(
       static_cast<const envoy::config::route::v3::RouteConfiguration&>(rc), optional_http_filters_,
-      factory_context_, validator_, validate_clusters_default_);
+      factory_context, validator_, validate_clusters_default);
 }
 
 bool RouteConfigUpdateReceiverImpl::onRdsUpdate(const Protobuf::Message& rc,

diff --git a/source/common/router/route_config_update_receiver_impl.h b/source/common/router/route_config_update_receiver_impl.h
@@ -21,29 +21,26 @@ namespace Router {
 class ConfigTraitsImpl : public Rds::ConfigTraits {
 public:
   ConfigTraitsImpl(const OptionalHttpFilters& optional_http_filters,
-                   Server::Configuration::ServerFactoryContext& factory_context,
-                   ProtobufMessage::ValidationVisitor& validator, bool validate_clusters_default)
-      : optional_http_filters_(optional_http_filters), factory_context_(factory_context),
-        validator_(validator), validate_clusters_default_(validate_clusters_default) {}
+                   ProtobufMessage::ValidationVisitor& validator)
+      : optional_http_filters_(optional_http_filters), validator_(validator) {}
 
   Rds::ConfigConstSharedPtr createNullConfig() const override;
-  Rds::ConfigConstSharedPtr createConfig(const Protobuf::Message& rc) const override;
+  Rds::ConfigConstSharedPtr createConfig(const Protobuf::Message& rc,
+                                         Server::Configuration::ServerFactoryContext& context,
+                                         bool validate_clusters_default) const override;
 
 private:
   const OptionalHttpFilters optional_http_filters_;
-  Server::Configuration::ServerFactoryContext& factory_context_;
   ProtobufMessage::ValidationVisitor& validator_;
-  bool validate_clusters_default_;
 };
 
 class RouteConfigUpdateReceiverImpl : public RouteConfigUpdateReceiver {
 public:
   RouteConfigUpdateReceiverImpl(Rds::ProtoTraits& proto_traits,
                                 Server::Configuration::ServerFactoryContext& factory_context,
                                 const OptionalHttpFilters& optional_http_filters)
-      : config_traits_(optional_http_filters, factory_context,
-                       factory_context.messageValidationContext().dynamicValidationVisitor(),
-                       false),
+      : config_traits_(optional_http_filters,
+                       factory_context.messageValidationContext().dynamicValidationVisitor()),
         base_(config_traits_, proto_traits, factory_context), last_vhds_config_hash_(0ul),
         vhds_virtual_hosts_(
             std::make_unique<std::map<std::string, envoy::config::route::v3::VirtualHost>>()),

diff --git a/source/extensions/filters/network/thrift_proxy/router/config.h b/source/extensions/filters/network/thrift_proxy/router/config.h
@@ -27,8 +27,14 @@ class RouterFilterConfig
 class ConfigImpl : public Config, Logger::Loggable<Logger::Id::config> {
 public:
   ConfigImpl(
-      const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& config)
-      : route_matcher_(std::make_unique<RouteMatcher>(config)) {}
+      const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& config,
+      Server::Configuration::ServerFactoryContext& context, bool validate_clusters_default) {
+    absl::optional<Upstream::ClusterManager::ClusterInfoMaps> validation_clusters;
+    if (validate_clusters_default) {
+      validation_clusters = context.clusterManager().clusters();
+    }
+    route_matcher_ = std::make_unique<RouteMatcher>(config, validation_clusters);
+  }
 
   // Config
   RouteConstSharedPtr route(const MessageMetadata& metadata, uint64_t random_value) const override {

diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc
@@ -48,6 +48,29 @@ RouteEntryImplBase::RouteEntryImplBase(
   }
 }
 
+// Similar validation procedure with Envoy::Router::RouteEntryImplBase::validateCluster
+void RouteEntryImplBase::validateClusters(
+    const Upstream::ClusterManager::ClusterInfoMaps& cluster_info_maps) const {
+  // Currently, we verify that the cluster exists in the CM if we have an explicit cluster or
+  // weighted cluster rule. We obviously do not verify a cluster_header rule. This means that
+  // trying to use all CDS clusters with a static route table will not work. In the upcoming RDS
+  // change we will make it so that dynamically loaded route tables do *not* perform CM checks.
+  // In the future we might decide to also have a config option that turns off checks for static
+  // route tables. This would enable the all CDS with static route table case.
+  if (!cluster_name_.empty()) {
+    if (!cluster_info_maps.hasCluster(cluster_name_)) {
+      throw EnvoyException(fmt::format("route: unknown thrift cluster '{}'", cluster_name_));
+    }
+  } else if (!weighted_clusters_.empty()) {
+    for (const WeightedClusterEntrySharedPtr& cluster : weighted_clusters_) {
+      if (!cluster_info_maps.hasCluster(cluster->clusterName())) {
+        throw EnvoyException(
+            fmt::format("route: unknown thrift weighted cluster '{}'", cluster->clusterName()));
+      }
+    }
+  }
+}
+
 std::vector<std::shared_ptr<RequestMirrorPolicy>> RouteEntryImplBase::buildMirrorPolicies(
     const envoy::extensions::filters::network::thrift_proxy::v3::RouteAction& route) {
   std::vector<std::shared_ptr<RequestMirrorPolicy>> policies{};
@@ -169,20 +192,37 @@ RouteConstSharedPtr ServiceNameRouteEntryImpl::matches(const MessageMetadata& me
 }
 
 RouteMatcher::RouteMatcher(
-    const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& config) {
+    const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& config,
+    const absl::optional<Upstream::ClusterManager::ClusterInfoMaps>& validation_clusters) {
   using envoy::extensions::filters::network::thrift_proxy::v3::RouteMatch;
 
   for (const auto& route : config.routes()) {
+    RouteEntryImplBaseConstSharedPtr route_entry;
     switch (route.match().match_specifier_case()) {
     case RouteMatch::MatchSpecifierCase::kMethodName:
-      routes_.emplace_back(new MethodNameRouteEntryImpl(route));
+      route_entry = std::make_shared<MethodNameRouteEntryImpl>(route);
       break;
     case RouteMatch::MatchSpecifierCase::kServiceName:
-      routes_.emplace_back(new ServiceNameRouteEntryImpl(route));
+      route_entry = std::make_shared<ServiceNameRouteEntryImpl>(route);
       break;
     case RouteMatch::MatchSpecifierCase::MATCH_SPECIFIER_NOT_SET:
       PANIC_DUE_TO_CORRUPT_ENUM;
     }
+
+    if (validation_clusters) {
+      // Throw exception for unknown clusters.
+      route_entry->validateClusters(*validation_clusters);
+
+      for (const auto& mirror_policy : route_entry->requestMirrorPolicies()) {
+        if (!validation_clusters->hasCluster(mirror_policy->clusterName())) {
+          throw EnvoyException(fmt::format("route: unknown thrift shadow cluster '{}'",
+                                           mirror_policy->clusterName()));
+        }
+      }
+    }
+
+    // Now we pass the validation. Add the route to the table.
+    routes_.emplace_back(route_entry);
   }
 }
 

diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.h b/source/extensions/filters/network/thrift_proxy/router/router_impl.h
@@ -52,6 +52,7 @@ class RouteEntryImplBase : public RouteEntry,
 public:
   RouteEntryImplBase(const envoy::extensions::filters::network::thrift_proxy::v3::Route& route);
 
+  void validateClusters(const Upstream::ClusterManager::ClusterInfoMaps& cluster_info_maps) const;
   // Router::RouteEntry
   const std::string& clusterName() const override;
   const Envoy::Router::MetadataMatchCriteria* metadataMatchCriteria() const override {
@@ -184,7 +185,10 @@ class ServiceNameRouteEntryImpl : public RouteEntryImplBase {
 
 class RouteMatcher {
 public:
-  RouteMatcher(const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration&);
+  // validation_clusters = absl::nullopt means that clusters are not validated.
+  RouteMatcher(
+      const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& config,
+      const absl::optional<Upstream::ClusterManager::ClusterInfoMaps>& validation_clusters);
 
   RouteConstSharedPtr route(const MessageMetadata& metadata, uint64_t random_value) const;
 

diff --git a/test/common/rds/rds_test.cc b/test/common/rds/rds_test.cc
@@ -53,7 +53,9 @@ class RdsTestBase : public testing::Test {
 class TestConfig : public Config {
 public:
   TestConfig() = default;
-  TestConfig(const envoy::config::route::v3::RouteConfiguration& rc) : rc_(rc) {}
+  TestConfig(const envoy::config::route::v3::RouteConfiguration& rc,
+             Server::Configuration::ServerFactoryContext&, bool)
+      : rc_(rc) {}
   const std::string* route(const std::string& name) const {
     for (const auto& virtual_host_config : rc_.virtual_hosts()) {
       if (virtual_host_config.name() == name) {