diff --git a/envoy/rds/BUILD b/envoy/rds/BUILD index 9f20ca9bdff57..ab72d9d5776be 100644 --- 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 index b7d79018cb5b6..18ab097c3c71a 100644 --- 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 index 7e6077e5fb8d4..4761ca06eac48 100644 --- 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 index 30aa77890919b..01815a7dbc677 100644 --- 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(); } - 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(&rc)); - return std::make_shared(static_cast(rc)); + return std::make_shared(static_cast(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 index dee58fa4cbeeb..3d0122616b97e 100644 --- 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&& 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 index 27cd5c1383855..153dab4d491bf 100644 --- 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 index fe43895a0adfc..f6d1523d4af22 100644 --- 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 index 6313624de0496..c054738a078a8 100644 --- 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(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 index 488fa5f663c90..6b3f9f43f0a4a 100644 --- 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(); } -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(&rc)); return std::make_shared( static_cast(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 index fa9501b29a0ce..b28de9fb2664c 100644 --- a/source/common/router/route_config_update_receiver_impl.h +++ b/source/common/router/route_config_update_receiver_impl.h @@ -21,19 +21,17 @@ 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 { @@ -41,9 +39,8 @@ class RouteConfigUpdateReceiverImpl : public RouteConfigUpdateReceiver { 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>()), diff --git a/source/extensions/filters/network/thrift_proxy/router/config.h b/source/extensions/filters/network/thrift_proxy/router/config.h index f148c3b7cf480..7ab5006ee24bc 100644 --- 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 { public: ConfigImpl( - const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& config) - : route_matcher_(std::make_unique(config)) {} + const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& config, + Server::Configuration::ServerFactoryContext& context, bool validate_clusters_default) { + absl::optional validation_clusters; + if (validate_clusters_default) { + validation_clusters = context.clusterManager().clusters(); + } + route_matcher_ = std::make_unique(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 index 798fe0a971644..cd220bcf495bf 100644 --- 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> RouteEntryImplBase::buildMirrorPolicies( const envoy::extensions::filters::network::thrift_proxy::v3::RouteAction& route) { std::vector> 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& 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(route); break; case RouteMatch::MatchSpecifierCase::kServiceName: - routes_.emplace_back(new ServiceNameRouteEntryImpl(route)); + route_entry = std::make_shared(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 index 78afddfaf3970..480f86ced3b56 100644 --- 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& 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 index f493aa663b0af..629296dc08199 100644 --- 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) { diff --git a/test/extensions/filters/network/thrift_proxy/conn_manager_test.cc b/test/extensions/filters/network/thrift_proxy/conn_manager_test.cc index 362f2163bb7f2..a21080162bf12 100644 --- a/test/extensions/filters/network/thrift_proxy/conn_manager_test.cc +++ b/test/extensions/filters/network/thrift_proxy/conn_manager_test.cc @@ -85,7 +85,22 @@ class ThriftConnectionManagerTest : public testing::Test { void initializeFilter() { initializeFilter(""); } - void initializeFilter(const std::string& yaml) { + void initializeFilter(const std::string& yaml, + const std::vector& cluster_names = {}) { + envoy::extensions::filters::network::thrift_proxy::v3::ThriftProxy config; + if (yaml.empty()) { + config.set_stat_prefix("test"); + } else { + TestUtility::loadFromYaml(yaml, config); + TestUtility::validate(config); + } + + config.set_stat_prefix("test"); + + initializeFilter(config, cluster_names); + } + void initializeFilter(envoy::extensions::filters::network::thrift_proxy::v3::ThriftProxy& config, + const std::vector& cluster_names = {}) { // Destroy any existing filter first. filter_ = nullptr; @@ -93,16 +108,11 @@ class ThriftConnectionManagerTest : public testing::Test { counter->reset(); } - if (yaml.empty()) { - proto_config_.set_stat_prefix("test"); - } else { - TestUtility::loadFromYaml(yaml, proto_config_); - TestUtility::validate(proto_config_); - } + decoder_filter_ = std::make_shared>(); - proto_config_.set_stat_prefix("test"); + context_.server_factory_context_.cluster_manager_.initializeClusters(cluster_names, {}); - decoder_filter_ = std::make_shared>(); + proto_config_ = config; config_ = std::make_unique( proto_config_, context_, *route_config_provider_manager_, decoder_filter_, stats_); @@ -729,7 +739,7 @@ stat_prefix: test cluster: cluster )EOF"; - initializeFilter(yaml); + initializeFilter(yaml, {"cluster"}); writeFramedBinaryMessage(buffer_, MessageType::Oneway, 0x0F); ThriftFilters::DecoderFilterCallbacks* callbacks{}; @@ -1849,7 +1859,7 @@ stat_prefix: test cluster: cluster )EOF"; - initializeFilter(yaml); + initializeFilter(yaml, {"cluster"}); writeFramedBinaryMessage(buffer_, MessageType::Oneway, 0x0F); EXPECT_CALL(*decoder_filter_, passthroughSupported()).WillRepeatedly(Return(true)); @@ -1893,7 +1903,7 @@ stat_prefix: test cluster: cluster )EOF"; - initializeFilter(yaml); + initializeFilter(yaml, {"cluster"}); writeFramedBinaryMessage(buffer_, MessageType::Oneway, 0x0F); EXPECT_CALL(*decoder_filter_, passthroughSupported()).WillRepeatedly(Return(true)); @@ -1925,6 +1935,99 @@ stat_prefix: test EXPECT_EQ(nullptr, route); } +TEST_F(ThriftConnectionManagerTest, UnknownCluster) { + const std::string yaml = R"EOF( +transport: FRAMED +protocol: BINARY +stat_prefix: test +route_config: + name: "routes" + routes: + - match: + method_name: name + route: + cluster: cluster1 + - match: + method_name: name2 + route: + cluster: cluster2 +)EOF"; + + EXPECT_THROW_WITH_REGEX(initializeFilter(yaml), EnvoyException, "unknown thrift cluster"); + EXPECT_THROW_WITH_REGEX(initializeFilter(yaml, {"cluster1"}), EnvoyException, + "unknown thrift cluster"); + EXPECT_THROW_WITH_REGEX(initializeFilter(yaml, {"cluster2"}), EnvoyException, + "unknown thrift cluster"); +} + +TEST_F(ThriftConnectionManagerTest, UnknownWeightedCluster) { + envoy::extensions::filters::network::thrift_proxy::v3::ThriftProxy config; + { + auto* route_config = config.mutable_route_config(); + route_config->set_name("config"); + auto* route = route_config->add_routes(); + route->mutable_match()->set_method_name("foo"); + auto* action = route->mutable_route(); + auto* cluster1 = action->mutable_weighted_clusters()->add_clusters(); + cluster1->set_name("cluster1"); + cluster1->mutable_weight()->set_value(50); + auto* cluster2 = action->mutable_weighted_clusters()->add_clusters(); + cluster2->set_name("cluster2"); + cluster2->mutable_weight()->set_value(50); + } + + EXPECT_THROW_WITH_REGEX(initializeFilter(config), EnvoyException, + "unknown thrift weighted cluster"); + EXPECT_THROW_WITH_REGEX(initializeFilter(config, {"cluster1"}), EnvoyException, + "unknown thrift weighted cluster"); + EXPECT_THROW_WITH_REGEX(initializeFilter(config, {"cluster2"}), EnvoyException, + "unknown thrift weighted cluster"); +} + +TEST_F(ThriftConnectionManagerTest, WeightedClusterNotSpecified) { + const std::string yaml = R"EOF( + transport: FRAMED + protocol: BINARY + stat_prefix: test + route_config: + name: "routes" + routes: + - match: + method_name: name + route: + cluster: cluster + - match: + method_name: name2 + route: + weighted_clusters: + clusters: + - weight: 2000 + )EOF"; + + EXPECT_THROW_WITH_REGEX(initializeFilter(yaml, {"cluster"}), EnvoyException, + "Proto constraint validation failed"); +} + +TEST_F(ThriftConnectionManagerTest, UnknownMirrorPolicyCluster) { + const std::string yaml = R"EOF( +transport: FRAMED +protocol: BINARY +stat_prefix: test +route_config: + name: "routes" + routes: + - match: + method_name: name + route: + cluster: cluster + request_mirror_policies: + - cluster: unknown_cluster +)EOF"; + // Intentionally miss "unknown_cluster" cluster. + EXPECT_THROW_WITH_REGEX(initializeFilter(yaml, {"cluster"}), EnvoyException, + "unknown thrift shadow cluster"); +} + } // namespace ThriftProxy } // namespace NetworkFilters } // namespace Extensions diff --git a/test/extensions/filters/network/thrift_proxy/route_matcher_test.cc b/test/extensions/filters/network/thrift_proxy/route_matcher_test.cc index 4368eab78d72f..2b5b305c56ffb 100644 --- a/test/extensions/filters/network/thrift_proxy/route_matcher_test.cc +++ b/test/extensions/filters/network/thrift_proxy/route_matcher_test.cc @@ -19,15 +19,29 @@ namespace ThriftProxy { namespace Router { namespace { -envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration -parseRouteConfigurationFromV3Yaml(const std::string& yaml) { - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration route_config; - TestUtility::loadFromYaml(yaml, route_config); - TestUtility::validate(route_config); - return route_config; -} +class ThriftRouteMatcherTest : public testing::Test { +protected: + RouteMatcher createMatcher( + const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& route) { + return RouteMatcher(route, absl::nullopt); + } + + RouteMatcher createMatcher(const std::string& yaml) { + auto config = parseRouteConfigurationFromV3Yaml(yaml); + return createMatcher(config); + } + +private: + envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration + parseRouteConfigurationFromV3Yaml(const std::string& yaml) { + envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration route_config; + TestUtility::loadFromYaml(yaml, route_config); + TestUtility::validate(route_config); + return route_config; + } +}; -TEST(ThriftRouteMatcherTest, RouteByMethodNameWithNoInversion) { +TEST_F(ThriftRouteMatcherTest, RouteByMethodNameWithNoInversion) { const std::string yaml = R"EOF( name: config routes: @@ -41,10 +55,7 @@ name: config cluster: "cluster2" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; EXPECT_EQ(nullptr, matcher.route(metadata, 0)); metadata.setMethodName("unknown"); @@ -63,7 +74,7 @@ name: config EXPECT_EQ("cluster2", route2->routeEntry()->clusterName()); } -TEST(ThriftRouteMatcherTest, RouteByMethodNameWithInversion) { +TEST_F(ThriftRouteMatcherTest, RouteByMethodNameWithInversion) { const std::string yaml = R"EOF( name: config routes: @@ -78,10 +89,7 @@ name: config cluster: "cluster2" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; RouteConstSharedPtr route = matcher.route(metadata, 0); EXPECT_NE(nullptr, route); @@ -107,7 +115,7 @@ name: config EXPECT_EQ(nullptr, route); } -TEST(ThriftRouteMatcherTest, RouteByAnyMethodNameWithNoInversion) { +TEST_F(ThriftRouteMatcherTest, RouteByAnyMethodNameWithNoInversion) { const std::string yaml = R"EOF( name: config routes: @@ -121,10 +129,7 @@ name: config cluster: "cluster2" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); { MessageMetadata metadata; @@ -147,7 +152,7 @@ name: config } } -TEST(ThriftRouteMatcherTest, RouteByAnyMethodNameWithInversion) { +TEST_F(ThriftRouteMatcherTest, RouteByAnyMethodNameWithInversion) { const std::string yaml = R"EOF( name: config routes: @@ -158,13 +163,10 @@ name: config cluster: "cluster2" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - EXPECT_THROW(new RouteMatcher(config), EnvoyException); + EXPECT_THROW(createMatcher(yaml), EnvoyException); } -TEST(ThriftRouteMatcherTest, RouteByServiceNameWithNoInversion) { +TEST_F(ThriftRouteMatcherTest, RouteByServiceNameWithNoInversion) { const std::string yaml = R"EOF( name: config routes: @@ -178,10 +180,7 @@ name: config cluster: "cluster2" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; EXPECT_EQ(nullptr, matcher.route(metadata, 0)); metadata.setMethodName("unknown"); @@ -200,7 +199,7 @@ name: config EXPECT_EQ("cluster2", route2->routeEntry()->clusterName()); } -TEST(ThriftRouteMatcherTest, RouteByServiceNameWithInversion) { +TEST_F(ThriftRouteMatcherTest, RouteByServiceNameWithInversion) { const std::string yaml = R"EOF( name: config routes: @@ -215,10 +214,7 @@ name: config cluster: "cluster2" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; RouteConstSharedPtr route = matcher.route(metadata, 0); EXPECT_NE(nullptr, route); @@ -244,7 +240,7 @@ name: config EXPECT_EQ(nullptr, route); } -TEST(ThriftRouteMatcherTest, RouteByAnyServiceNameWithNoInversion) { +TEST_F(ThriftRouteMatcherTest, RouteByAnyServiceNameWithNoInversion) { const std::string yaml = R"EOF( name: config routes: @@ -258,10 +254,7 @@ name: config cluster: "cluster2" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); { MessageMetadata metadata; @@ -284,7 +277,7 @@ name: config } } -TEST(ThriftRouteMatcherTest, RouteByAnyServiceNameWithInversion) { +TEST_F(ThriftRouteMatcherTest, RouteByAnyServiceNameWithInversion) { const std::string yaml = R"EOF( name: config routes: @@ -295,13 +288,10 @@ name: config cluster: "cluster2" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - EXPECT_THROW(new RouteMatcher(config), EnvoyException); + EXPECT_THROW(createMatcher(yaml), EnvoyException); } -TEST(ThriftRouteMatcherTest, RouteByExactHeaderMatcher) { +TEST_F(ThriftRouteMatcherTest, RouteByExactHeaderMatcher) { const std::string yaml = R"EOF( name: config routes: @@ -315,10 +305,7 @@ name: config cluster: "cluster1" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; RouteConstSharedPtr route = matcher.route(metadata, 0); EXPECT_EQ(nullptr, route); @@ -333,7 +320,7 @@ name: config EXPECT_EQ("cluster1", route->routeEntry()->clusterName()); } -TEST(ThriftRouteMatcherTest, RouteByRegexHeaderMatcher) { +TEST_F(ThriftRouteMatcherTest, RouteByRegexHeaderMatcher) { const std::string yaml = R"EOF( name: config routes: @@ -349,10 +336,7 @@ name: config cluster: "cluster1" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; RouteConstSharedPtr route = matcher.route(metadata, 0); EXPECT_EQ(nullptr, route); @@ -372,7 +356,7 @@ name: config EXPECT_EQ("cluster1", route->routeEntry()->clusterName()); } -TEST(ThriftRouteMatcherTest, RouteByRangeHeaderMatcher) { +TEST_F(ThriftRouteMatcherTest, RouteByRangeHeaderMatcher) { const std::string yaml = R"EOF( name: config routes: @@ -387,10 +371,7 @@ name: config cluster: "cluster1" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; RouteConstSharedPtr route = matcher.route(metadata, 0); EXPECT_EQ(nullptr, route); @@ -410,7 +391,7 @@ name: config EXPECT_EQ("cluster1", route->routeEntry()->clusterName()); } -TEST(ThriftRouteMatcherTest, RouteByPresentHeaderMatcher) { +TEST_F(ThriftRouteMatcherTest, RouteByPresentHeaderMatcher) { const std::string yaml = R"EOF( name: config routes: @@ -423,10 +404,7 @@ name: config cluster: "cluster1" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; RouteConstSharedPtr route = matcher.route(metadata, 0); EXPECT_EQ(nullptr, route); @@ -447,7 +425,7 @@ name: config EXPECT_EQ("cluster1", route->routeEntry()->clusterName()); } -TEST(ThriftRouteMatcherTest, RouteByPrefixHeaderMatcher) { +TEST_F(ThriftRouteMatcherTest, RouteByPrefixHeaderMatcher) { const std::string yaml = R"EOF( name: config routes: @@ -461,10 +439,7 @@ name: config cluster: "cluster1" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; RouteConstSharedPtr route = matcher.route(metadata, 0); EXPECT_EQ(nullptr, route); @@ -484,7 +459,7 @@ name: config EXPECT_EQ("cluster1", route->routeEntry()->clusterName()); } -TEST(ThriftRouteMatcherTest, RouteBySuffixHeaderMatcher) { +TEST_F(ThriftRouteMatcherTest, RouteBySuffixHeaderMatcher) { const std::string yaml = R"EOF( name: config routes: @@ -498,10 +473,7 @@ name: config cluster: "cluster1" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; RouteConstSharedPtr route = matcher.route(metadata, 0); EXPECT_EQ(nullptr, route); @@ -526,7 +498,7 @@ name: config EXPECT_EQ("cluster1", route->routeEntry()->clusterName()); } -TEST(ThriftRouteMatcherTest, RouteByClusterHeader) { +TEST_F(ThriftRouteMatcherTest, RouteByClusterHeader) { const std::string yaml = R"EOF( name: config routes: @@ -536,10 +508,7 @@ name: config cluster_header: "x-cluster" )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; RouteConstSharedPtr route; @@ -564,7 +533,7 @@ name: config EXPECT_EQ("cluster1", route->routeEntry()->clusterName()); } -TEST(ThriftRouteMatcherTest, WeightedClusters) { +TEST_F(ThriftRouteMatcherTest, WeightedClusters) { const std::string yaml = R"EOF( name: config routes: @@ -592,9 +561,7 @@ name: config weight: 5000 )EOF"; - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; { @@ -620,7 +587,7 @@ name: config } } -TEST(ThriftRouteMatcherTest, WeightedClusterMissingWeight) { +TEST_F(ThriftRouteMatcherTest, WeightedClusterMissingWeight) { const std::string yaml = R"EOF( name: config routes: @@ -636,12 +603,10 @@ name: config weight: 5000 )EOF"; - const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - EXPECT_THROW(RouteMatcher m(config), EnvoyException); + EXPECT_THROW(createMatcher(yaml), EnvoyException); } -TEST(ThriftRouteMatcherTest, RouteActionMetadataMatch) { +TEST_F(ThriftRouteMatcherTest, RouteActionMetadataMatch) { const std::string yaml = R"EOF( name: config routes: @@ -660,9 +625,7 @@ name: config cluster: cluster2 )EOF"; - const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; // match with metadata @@ -701,7 +664,7 @@ name: config } } -TEST(ThriftRouteMatcherTest, WeightedClusterMetadataMatch) { +TEST_F(ThriftRouteMatcherTest, WeightedClusterMetadataMatch) { const std::string yaml = R"EOF( name: config routes: @@ -732,9 +695,7 @@ name: config k3: v3 )EOF"; - const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; metadata.setMethodName("method1"); ProtobufWkt::Value v1, v2, v3; @@ -791,7 +752,7 @@ name: config } } -TEST(ThriftRouteMatcherTest, WeightedClusterRouteActionMetadataMatchMerged) { +TEST_F(ThriftRouteMatcherTest, WeightedClusterRouteActionMetadataMatchMerged) { const std::string yaml = R"EOF( name: config routes: @@ -821,9 +782,7 @@ name: config k2: v3 )EOF"; - const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); - RouteMatcher matcher(config); + auto matcher = createMatcher(yaml); MessageMetadata metadata; metadata.setMethodName("method1"); ProtobufWkt::Value v1, v2, v3; @@ -897,7 +856,7 @@ name: config } // Test that the route entry has metadata match criteria when using a cluster header. -TEST(ThriftRouteMatcherTest, ClusterHeaderMetadataMatch) { +TEST_F(ThriftRouteMatcherTest, ClusterHeaderMetadataMatch) { envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config; { config.set_name("config"); @@ -917,7 +876,7 @@ TEST(ThriftRouteMatcherTest, ClusterHeaderMetadataMatch) { action2->set_cluster("cluster2"); } - RouteMatcher matcher(config); + auto matcher = createMatcher(config); // match with metadata { @@ -963,7 +922,7 @@ TEST(ThriftRouteMatcherTest, ClusterHeaderMetadataMatch) { } // Tests that weighted cluster route entries can be configured to strip the service name. -TEST(RouteMatcherTest, WeightedClusterWithStripServiceEnabled) { +TEST_F(ThriftRouteMatcherTest, WeightedClusterWithStripServiceEnabled) { envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config; { config.set_name("config"); @@ -979,7 +938,7 @@ TEST(RouteMatcherTest, WeightedClusterWithStripServiceEnabled) { action->set_strip_service_name(true); } - RouteMatcher matcher(config); + auto matcher = createMatcher(config); MessageMetadata metadata; metadata.setMethodName("method1"); @@ -988,7 +947,7 @@ TEST(RouteMatcherTest, WeightedClusterWithStripServiceEnabled) { } // Tests that dynamic route entries can be configured to strip the service name. -TEST(RouteMatcherTest, ClusterHeaderWithStripServiceEnabled) { +TEST_F(ThriftRouteMatcherTest, ClusterHeaderWithStripServiceEnabled) { envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config; { config.set_name("config"); @@ -999,7 +958,7 @@ TEST(RouteMatcherTest, ClusterHeaderWithStripServiceEnabled) { action->set_strip_service_name(true); } - RouteMatcher matcher(config); + auto matcher = createMatcher(config); MessageMetadata metadata; metadata.setMethodName("method1"); diff --git a/test/extensions/filters/network/thrift_proxy/router_ratelimit_test.cc b/test/extensions/filters/network/thrift_proxy/router_ratelimit_test.cc index 7a5d8c6c91b77..2dc22c8be60b2 100644 --- a/test/extensions/filters/network/thrift_proxy/router_ratelimit_test.cc +++ b/test/extensions/filters/network/thrift_proxy/router_ratelimit_test.cc @@ -37,6 +37,20 @@ class ThriftRateLimitConfigurationTest : public testing::Test { std::make_unique(factory_context_.admin_); } + void initializeClusters(const std::vector& cluster_names) { + factory_context_.server_factory_context_.cluster_manager_.initializeClusters(cluster_names, {}); + } + void initialize(envoy::extensions::filters::network::thrift_proxy::v3::ThriftProxy& config, + const std::vector& cluster_names) { + initializeClusters(cluster_names); + initialize(config); + } + + void initialize(const std::string& yaml, const std::vector& cluster_names) { + initializeClusters(cluster_names); + initialize(yaml); + } + void initialize(const std::string& yaml) { envoy::extensions::filters::network::thrift_proxy::v3::ThriftProxy config; TestUtility::loadFromYaml(yaml, config); @@ -75,8 +89,7 @@ TEST_F(ThriftRateLimitConfigurationTest, NoApplicableRateLimit) { - match: { method_name: "bar" } route: { cluster: thrift } )EOF"; - - initialize(yaml); + initialize(yaml, {"thrift"}); EXPECT_EQ(0U, config_->route(genMetadata("bar"), 0) ->routeEntry() @@ -94,7 +107,7 @@ TEST_F(ThriftRateLimitConfigurationTest, NoRateLimitPolicy) { route: { cluster: thrift } )EOF"; - initialize(yaml); + initialize(yaml, {"thrift"}); auto route = config_->route(genMetadata("bar"), 0)->routeEntry(); EXPECT_EQ(0U, route->rateLimitPolicy().getApplicableRateLimit(0).size()); @@ -114,7 +127,7 @@ TEST_F(ThriftRateLimitConfigurationTest, TestGetApplicableRateLimit) { - remote_address: {} )EOF"; - initialize(yaml); + initialize(yaml, {"thrift"}); auto route = config_->route(genMetadata("foo"), 0)->routeEntry(); EXPECT_FALSE(route->rateLimitPolicy().empty()); @@ -150,7 +163,7 @@ TEST_F(ThriftRateLimitConfigurationTest, Stages) { - source_cluster: {} )EOF"; - initialize(yaml); + initialize(yaml, {"thrift"}); auto route = config_->route(genMetadata("foo"), 0)->routeEntry(); std::vector> rate_limits = @@ -208,7 +221,7 @@ TEST_F(ThriftRateLimitConfigurationTest, WeightedClusterStages) { limit3->add_actions()->mutable_destination_cluster(); limit3->add_actions()->mutable_source_cluster(); } - initialize(config); + initialize(config, {"thrift", "thrift2"}); auto route = config_->route(genMetadata("foo"), 0)->routeEntry(); std::vector> rate_limits =