From ddd3d9bd8a8a1943bc20bb54eb1230b8157a0d79 Mon Sep 17 00:00:00 2001 From: kuochunghsu Date: Tue, 29 Mar 2022 17:41:07 -0700 Subject: [PATCH 01/11] Reject thrift router config if cluster/weighted clusters/mirror cluster doesnt exist Signed-off-by: kuochunghsu --- envoy/rds/config_traits.h | 5 +- source/common/rds/common/config_traits_impl.h | 7 +- .../rds/route_config_update_receiver_impl.cc | 5 +- .../rds/route_config_update_receiver_impl.h | 1 + .../rds/static_route_config_provider_impl.cc | 3 +- source/common/router/rds_impl.cc | 2 +- .../route_config_update_receiver_impl.cc | 7 +- .../route_config_update_receiver_impl.h | 17 +- .../network/thrift_proxy/router/config.h | 6 +- .../thrift_proxy/router/router_impl.cc | 57 +++++- .../network/thrift_proxy/router/router_impl.h | 4 +- test/common/rds/rds_test.cc | 4 +- .../filters/network/thrift_proxy/BUILD | 2 + .../thrift_proxy/route_matcher_test.cc | 180 +++++++----------- 14 files changed, 166 insertions(+), 134 deletions(-) diff --git a/envoy/rds/config_traits.h b/envoy/rds/config_traits.h index b7d79018cb5b6..57aca9e341b77 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" @@ -54,7 +55,9 @@ class ConfigTraits { * to the corresponding route configuration class. * @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/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..b29a6002822d4 100644 --- a/source/extensions/filters/network/thrift_proxy/router/config.h +++ b/source/extensions/filters/network/thrift_proxy/router/config.h @@ -27,8 +27,10 @@ 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) + : route_matcher_(std::make_unique(config, context, validate_clusters_default)) { + } // 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..d3604227fd4b4 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,36 @@ RouteEntryImplBase::RouteEntryImplBase( } } +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 cluster '{}'", cluster_name_)); + } + } else if (!weighted_clusters_.empty()) { + for (const WeightedClusterEntrySharedPtr& cluster : weighted_clusters_) { + if (!cluster->clusterName().empty()) { + if (!cluster_info_maps.hasCluster(cluster->clusterName())) { + throw EnvoyException( + fmt::format("route: unknown weighted cluster '{}'", cluster->clusterName())); + } + } + // For weighted clusters with `cluster_header_name`, we only verify that this field is + // not empty because the cluster name is not set yet at config time (hence the validation + // here). + else if (cluster->clusterHeader().get().empty()) { + throw EnvoyException("route: unknown weighted cluster with no cluster_header field"); + } + } + } +} + std::vector> RouteEntryImplBase::buildMirrorPolicies( const envoy::extensions::filters::network::thrift_proxy::v3::RouteAction& route) { std::vector> policies{}; @@ -169,20 +199,41 @@ 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, + Server::Configuration::ServerFactoryContext& factory_context, bool validate_clusters) { + absl::optional validation_clusters; + if (validate_clusters) { + validation_clusters = factory_context.clusterManager().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()) { + ASSERT(!mirror_policy->clusterName().empty()); + if (!validation_clusters->hasCluster(mirror_policy->clusterName())) { + throw EnvoyException( + fmt::format("route: unknown 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..1a57cd8fee324 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,8 @@ class ServiceNameRouteEntryImpl : public RouteEntryImplBase { class RouteMatcher { public: - RouteMatcher(const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration&); + RouteMatcher(const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration&, + Server::Configuration::ServerFactoryContext&, bool); 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/BUILD b/test/extensions/filters/network/thrift_proxy/BUILD index 4751bd789f988..017c0fc31687b 100644 --- a/test/extensions/filters/network/thrift_proxy/BUILD +++ b/test/extensions/filters/network/thrift_proxy/BUILD @@ -311,10 +311,12 @@ envoy_extension_cc_test( srcs = ["route_matcher_test.cc"], extension_names = ["envoy.filters.network.thrift_proxy"], deps = [ + ":mocks", ":utility_lib", "//source/extensions/filters/network/thrift_proxy/router:config", "//source/extensions/filters/network/thrift_proxy/router:router_interface", "//source/extensions/filters/network/thrift_proxy/router:router_lib", + "//test/mocks/server:factory_context_mocks", "//test/test_common:utility_lib", "@envoy_api//envoy/config/filter/thrift/router/v2alpha1:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/network/thrift_proxy/v3:pkg_cc_proto", 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..bf521434426b6 100644 --- a/test/extensions/filters/network/thrift_proxy/route_matcher_test.cc +++ b/test/extensions/filters/network/thrift_proxy/route_matcher_test.cc @@ -7,7 +7,9 @@ #include "source/extensions/filters/network/thrift_proxy/router/config.h" #include "source/extensions/filters/network/thrift_proxy/router/router_impl.h" +#include "test/extensions/filters/network/thrift_proxy/mocks.h" #include "test/extensions/filters/network/thrift_proxy/utility.h" +#include "test/mocks/server/instance.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -19,15 +21,32 @@ 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, context_, false); + } + + RouteMatcher createMatcher(const std::string& yaml) { + envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration 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) { + NiceMock context_; +}; + +TEST_F(ThriftRouteMatcherTest, RouteByMethodNameWithNoInversion) { const std::string yaml = R"EOF( name: config routes: @@ -41,10 +60,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 +79,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 +94,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 +120,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 +134,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 +157,7 @@ name: config } } -TEST(ThriftRouteMatcherTest, RouteByAnyMethodNameWithInversion) { +TEST_F(ThriftRouteMatcherTest, RouteByAnyMethodNameWithInversion) { const std::string yaml = R"EOF( name: config routes: @@ -158,13 +168,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 +185,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 +204,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 +219,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 +245,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 +259,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 +282,7 @@ name: config } } -TEST(ThriftRouteMatcherTest, RouteByAnyServiceNameWithInversion) { +TEST_F(ThriftRouteMatcherTest, RouteByAnyServiceNameWithInversion) { const std::string yaml = R"EOF( name: config routes: @@ -295,13 +293,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 +310,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 +325,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 +341,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 +361,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 +376,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 +396,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 +409,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 +430,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 +444,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 +464,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 +478,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 +503,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 +513,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 +538,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 +566,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 +592,7 @@ name: config } } -TEST(ThriftRouteMatcherTest, WeightedClusterMissingWeight) { +TEST_F(ThriftRouteMatcherTest, WeightedClusterMissingWeight) { const std::string yaml = R"EOF( name: config routes: @@ -636,12 +608,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 +630,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 +669,7 @@ name: config } } -TEST(ThriftRouteMatcherTest, WeightedClusterMetadataMatch) { +TEST_F(ThriftRouteMatcherTest, WeightedClusterMetadataMatch) { const std::string yaml = R"EOF( name: config routes: @@ -732,9 +700,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 +757,7 @@ name: config } } -TEST(ThriftRouteMatcherTest, WeightedClusterRouteActionMetadataMatchMerged) { +TEST_F(ThriftRouteMatcherTest, WeightedClusterRouteActionMetadataMatchMerged) { const std::string yaml = R"EOF( name: config routes: @@ -821,9 +787,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 +861,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 +881,7 @@ TEST(ThriftRouteMatcherTest, ClusterHeaderMetadataMatch) { action2->set_cluster("cluster2"); } - RouteMatcher matcher(config); + auto matcher = createMatcher(config); // match with metadata { @@ -963,7 +927,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 +943,7 @@ TEST(RouteMatcherTest, WeightedClusterWithStripServiceEnabled) { action->set_strip_service_name(true); } - RouteMatcher matcher(config); + auto matcher = createMatcher(config); MessageMetadata metadata; metadata.setMethodName("method1"); @@ -988,7 +952,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 +963,7 @@ TEST(RouteMatcherTest, ClusterHeaderWithStripServiceEnabled) { action->set_strip_service_name(true); } - RouteMatcher matcher(config); + auto matcher = createMatcher(config); MessageMetadata metadata; metadata.setMethodName("method1"); From 48751adfe8880c5c3d904495efa75e85ee2876f9 Mon Sep 17 00:00:00 2001 From: kuochunghsu Date: Wed, 30 Mar 2022 10:05:54 -0700 Subject: [PATCH 02/11] Only expose cluster information to `RouteMatcher` Signed-off-by: kuochunghsu --- .../filters/network/thrift_proxy/router/config.h | 8 ++++++-- .../filters/network/thrift_proxy/router/router_impl.cc | 7 ++----- .../filters/network/thrift_proxy/router/router_impl.h | 6 ++++-- test/extensions/filters/network/thrift_proxy/BUILD | 2 -- .../filters/network/thrift_proxy/route_matcher_test.cc | 9 ++------- 5 files changed, 14 insertions(+), 18 deletions(-) diff --git a/source/extensions/filters/network/thrift_proxy/router/config.h b/source/extensions/filters/network/thrift_proxy/router/config.h index b29a6002822d4..7ab5006ee24bc 100644 --- a/source/extensions/filters/network/thrift_proxy/router/config.h +++ b/source/extensions/filters/network/thrift_proxy/router/config.h @@ -28,8 +28,12 @@ class ConfigImpl : public Config, Logger::Loggable { public: ConfigImpl( const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& config, - Server::Configuration::ServerFactoryContext& context, bool validate_clusters_default) - : route_matcher_(std::make_unique(config, context, validate_clusters_default)) { + 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 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 d3604227fd4b4..4c3b29949b1b7 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,7 @@ RouteEntryImplBase::RouteEntryImplBase( } } +// Similiar 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 @@ -200,11 +201,7 @@ RouteConstSharedPtr ServiceNameRouteEntryImpl::matches(const MessageMetadata& me RouteMatcher::RouteMatcher( const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& config, - Server::Configuration::ServerFactoryContext& factory_context, bool validate_clusters) { - absl::optional validation_clusters; - if (validate_clusters) { - validation_clusters = factory_context.clusterManager().clusters(); - } + const absl::optional& validation_clusters) { using envoy::extensions::filters::network::thrift_proxy::v3::RouteMatch; for (const auto& route : config.routes()) { 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 1a57cd8fee324..653df2934317f 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.h +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.h @@ -185,8 +185,10 @@ class ServiceNameRouteEntryImpl : public RouteEntryImplBase { class RouteMatcher { public: - RouteMatcher(const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration&, - Server::Configuration::ServerFactoryContext&, bool); + // validation_clusters = absl::nullpopt 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/extensions/filters/network/thrift_proxy/BUILD b/test/extensions/filters/network/thrift_proxy/BUILD index 017c0fc31687b..4751bd789f988 100644 --- a/test/extensions/filters/network/thrift_proxy/BUILD +++ b/test/extensions/filters/network/thrift_proxy/BUILD @@ -311,12 +311,10 @@ envoy_extension_cc_test( srcs = ["route_matcher_test.cc"], extension_names = ["envoy.filters.network.thrift_proxy"], deps = [ - ":mocks", ":utility_lib", "//source/extensions/filters/network/thrift_proxy/router:config", "//source/extensions/filters/network/thrift_proxy/router:router_interface", "//source/extensions/filters/network/thrift_proxy/router:router_lib", - "//test/mocks/server:factory_context_mocks", "//test/test_common:utility_lib", "@envoy_api//envoy/config/filter/thrift/router/v2alpha1:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/network/thrift_proxy/v3:pkg_cc_proto", 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 bf521434426b6..2b5b305c56ffb 100644 --- a/test/extensions/filters/network/thrift_proxy/route_matcher_test.cc +++ b/test/extensions/filters/network/thrift_proxy/route_matcher_test.cc @@ -7,9 +7,7 @@ #include "source/extensions/filters/network/thrift_proxy/router/config.h" #include "source/extensions/filters/network/thrift_proxy/router/router_impl.h" -#include "test/extensions/filters/network/thrift_proxy/mocks.h" #include "test/extensions/filters/network/thrift_proxy/utility.h" -#include "test/mocks/server/instance.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -25,12 +23,11 @@ class ThriftRouteMatcherTest : public testing::Test { protected: RouteMatcher createMatcher( const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& route) { - return RouteMatcher(route, context_, false); + return RouteMatcher(route, absl::nullopt); } RouteMatcher createMatcher(const std::string& yaml) { - envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration config = - parseRouteConfigurationFromV3Yaml(yaml); + auto config = parseRouteConfigurationFromV3Yaml(yaml); return createMatcher(config); } @@ -42,8 +39,6 @@ class ThriftRouteMatcherTest : public testing::Test { TestUtility::validate(route_config); return route_config; } - - NiceMock context_; }; TEST_F(ThriftRouteMatcherTest, RouteByMethodNameWithNoInversion) { From 6c84567131fc33d0d10c27482c627fca255fce9f Mon Sep 17 00:00:00 2001 From: kuochunghsu Date: Wed, 30 Mar 2022 11:17:55 -0700 Subject: [PATCH 03/11] Fix router ratelimit test Signed-off-by: kuochunghsu --- .../thrift_proxy/router_ratelimit_test.cc | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) 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..332597c3f4afe 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,19 @@ 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 +88,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 +106,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 +126,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 +162,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 +220,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 = From 6ac01958ef405954ef7aa14a3240c28d74b95110 Mon Sep 17 00:00:00 2001 From: kuochunghsu Date: Wed, 30 Mar 2022 12:39:56 -0700 Subject: [PATCH 04/11] Add unit test and address rgs1's comment Signed-off-by: kuochunghsu --- .../thrift_proxy/router/router_impl.cc | 14 +- .../network/thrift_proxy/conn_manager_test.cc | 156 ++++++++++++++++-- .../thrift_proxy/router_ratelimit_test.cc | 3 +- 3 files changed, 153 insertions(+), 20 deletions(-) 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 4c3b29949b1b7..f2efa4a51b31a 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc @@ -59,21 +59,21 @@ void RouteEntryImplBase::validateClusters( // 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 cluster '{}'", 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->clusterName().empty()) { if (!cluster_info_maps.hasCluster(cluster->clusterName())) { throw EnvoyException( - fmt::format("route: unknown weighted cluster '{}'", cluster->clusterName())); + fmt::format("route: unknown thrift weighted cluster '{}'", cluster->clusterName())); } } // For weighted clusters with `cluster_header_name`, we only verify that this field is // not empty because the cluster name is not set yet at config time (hence the validation // here). else if (cluster->clusterHeader().get().empty()) { - throw EnvoyException("route: unknown weighted cluster with no cluster_header field"); + throw EnvoyException("route: unknown thrift weighted cluster with no cluster_header field"); } } } @@ -220,16 +220,16 @@ RouteMatcher::RouteMatcher( if (validation_clusters) { // Throw exception for unknown clusters. route_entry->validateClusters(*validation_clusters); + for (const auto& mirror_policy : route_entry->requestMirrorPolicies()) { - ASSERT(!mirror_policy->clusterName().empty()); if (!validation_clusters->hasCluster(mirror_policy->clusterName())) { - throw EnvoyException( - fmt::format("route: unknown shadow cluster '{}'", 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. + // Now we pass the validation. Add the route to the table. routes_.emplace_back(route_entry); } } 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..d2841dc6591da 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,23 @@ class ThriftConnectionManagerTest : public testing::Test { void initializeFilter() { initializeFilter(""); } - void initializeFilter(const std::string& yaml) { + void initializeFilter(const std::string& yaml) { initializeFilter(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 +109,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 +740,7 @@ stat_prefix: test cluster: cluster )EOF"; - initializeFilter(yaml); + initializeFilter(yaml, {"cluster"}); writeFramedBinaryMessage(buffer_, MessageType::Oneway, 0x0F); ThriftFilters::DecoderFilterCallbacks* callbacks{}; @@ -1849,7 +1860,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 +1904,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 +1936,127 @@ 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: cluster +)EOF"; + + EXPECT_THROW_WITH_REGEX(initializeFilter(yaml), EnvoyException, "unknown thrift cluster"); +} + +TEST_F(ThriftConnectionManagerTest, UnknownCluster2) { + 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: + cluster: unknown_cluster +)EOF"; + + EXPECT_THROW_WITH_REGEX(initializeFilter(yaml, {"cluster"}), EnvoyException, + "unknown thrift cluster"); +} + +TEST_F(ThriftConnectionManagerTest, UnknownCluster3) { + const std::string yaml = R"EOF( +transport: FRAMED +protocol: BINARY +stat_prefix: test +route_config: + name: "routes" + routes: + - match: + method_name: name + route: + cluster: unknown_cluster + - match: + method_name: name2 + route: + cluster: cluster +)EOF"; + + EXPECT_THROW_WITH_REGEX(initializeFilter(yaml, {"cluster"}), 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("thrift"); + cluster1->mutable_weight()->set_value(50); + auto* cluster2 = action->mutable_weighted_clusters()->add_clusters(); + cluster2->set_name("unknown_cluster"); + cluster2->mutable_weight()->set_value(50); + } + // Intentionally miss "unknown_cluster" cluster. + EXPECT_THROW_WITH_REGEX(initializeFilter(config, {"thrift"}), EnvoyException, + "unknown thrift weighted cluster"); +} + +TEST_F(ThriftConnectionManagerTest, UnknownWeightedCluster2) { + 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("unknown_cluster"); + cluster1->mutable_weight()->set_value(1); + auto* cluster2 = action->mutable_weighted_clusters()->add_clusters(); + cluster2->set_name("cluster"); + cluster2->mutable_weight()->set_value(100); + } + // Intentionally miss "unknown_cluster" cluster. + EXPECT_THROW_WITH_REGEX(initializeFilter(config, {"cluster"}), EnvoyException, + "unknown thrift weighted cluster"); +} + +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/router_ratelimit_test.cc b/test/extensions/filters/network/thrift_proxy/router_ratelimit_test.cc index 332597c3f4afe..2dc22c8be60b2 100644 --- a/test/extensions/filters/network/thrift_proxy/router_ratelimit_test.cc +++ b/test/extensions/filters/network/thrift_proxy/router_ratelimit_test.cc @@ -40,7 +40,8 @@ class ThriftRateLimitConfigurationTest : public testing::Test { 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) { + void initialize(envoy::extensions::filters::network::thrift_proxy::v3::ThriftProxy& config, + const std::vector& cluster_names) { initializeClusters(cluster_names); initialize(config); } From 8b2beb83d4310148b48133cb165f24333a17aeb3 Mon Sep 17 00:00:00 2001 From: kuochunghsu Date: Wed, 30 Mar 2022 21:03:00 -0700 Subject: [PATCH 05/11] address tkovacs-2's comment Signed-off-by: kuochunghsu --- .../network/thrift_proxy/conn_manager_test.cc | 72 +++---------------- 1 file changed, 8 insertions(+), 64 deletions(-) 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 d2841dc6591da..0638a1a29a8a7 100644 --- a/test/extensions/filters/network/thrift_proxy/conn_manager_test.cc +++ b/test/extensions/filters/network/thrift_proxy/conn_manager_test.cc @@ -1947,53 +1947,16 @@ stat_prefix: test - match: method_name: name route: - cluster: cluster -)EOF"; - - EXPECT_THROW_WITH_REGEX(initializeFilter(yaml), EnvoyException, "unknown thrift cluster"); -} - -TEST_F(ThriftConnectionManagerTest, UnknownCluster2) { - 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 + cluster: cluster1 - match: method_name: name2 route: - cluster: unknown_cluster + cluster: cluster2 )EOF"; - EXPECT_THROW_WITH_REGEX(initializeFilter(yaml, {"cluster"}), EnvoyException, + EXPECT_THROW_WITH_REGEX(initializeFilter(yaml, {"cluster1"}), EnvoyException, "unknown thrift cluster"); -} - -TEST_F(ThriftConnectionManagerTest, UnknownCluster3) { - const std::string yaml = R"EOF( -transport: FRAMED -protocol: BINARY -stat_prefix: test -route_config: - name: "routes" - routes: - - match: - method_name: name - route: - cluster: unknown_cluster - - match: - method_name: name2 - route: - cluster: cluster -)EOF"; - - EXPECT_THROW_WITH_REGEX(initializeFilter(yaml, {"cluster"}), EnvoyException, + EXPECT_THROW_WITH_REGEX(initializeFilter(yaml, {"cluster2"}), EnvoyException, "unknown thrift cluster"); } @@ -2006,34 +1969,15 @@ TEST_F(ThriftConnectionManagerTest, UnknownWeightedCluster) { route->mutable_match()->set_method_name("foo"); auto* action = route->mutable_route(); auto* cluster1 = action->mutable_weighted_clusters()->add_clusters(); - cluster1->set_name("thrift"); + cluster1->set_name("cluster1"); cluster1->mutable_weight()->set_value(50); auto* cluster2 = action->mutable_weighted_clusters()->add_clusters(); - cluster2->set_name("unknown_cluster"); + cluster2->set_name("cluster2"); cluster2->mutable_weight()->set_value(50); } - // Intentionally miss "unknown_cluster" cluster. - EXPECT_THROW_WITH_REGEX(initializeFilter(config, {"thrift"}), EnvoyException, + EXPECT_THROW_WITH_REGEX(initializeFilter(config, {"cluster1"}), EnvoyException, "unknown thrift weighted cluster"); -} - -TEST_F(ThriftConnectionManagerTest, UnknownWeightedCluster2) { - 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("unknown_cluster"); - cluster1->mutable_weight()->set_value(1); - auto* cluster2 = action->mutable_weighted_clusters()->add_clusters(); - cluster2->set_name("cluster"); - cluster2->mutable_weight()->set_value(100); - } - // Intentionally miss "unknown_cluster" cluster. - EXPECT_THROW_WITH_REGEX(initializeFilter(config, {"cluster"}), EnvoyException, + EXPECT_THROW_WITH_REGEX(initializeFilter(config, {"cluster2"}), EnvoyException, "unknown thrift weighted cluster"); } From 308ed340a9573249464e7628d7b25ca1efcf8424 Mon Sep 17 00:00:00 2001 From: kuochunghsu Date: Wed, 30 Mar 2022 21:57:17 -0700 Subject: [PATCH 06/11] fix nit Signed-off-by: kuochunghsu --- .../filters/network/thrift_proxy/router/router_impl.cc | 2 +- .../filters/network/thrift_proxy/router/router_impl.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 f2efa4a51b31a..30c04ab4a8a7d 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc @@ -48,7 +48,7 @@ RouteEntryImplBase::RouteEntryImplBase( } } -// Similiar validation procedure with Envoy::Router::RouteEntryImplBase::validateCluster +// 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 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 653df2934317f..480f86ced3b56 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.h +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.h @@ -185,7 +185,7 @@ class ServiceNameRouteEntryImpl : public RouteEntryImplBase { class RouteMatcher { public: - // validation_clusters = absl::nullpopt means that clusters are not validated. + // 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); From 7f383906b9b23a50bd913ada2773ce56566e9d0f Mon Sep 17 00:00:00 2001 From: kuochunghsu Date: Sun, 3 Apr 2022 17:52:23 -0700 Subject: [PATCH 07/11] Address Tamas and Harvey's comments Signed-off-by: kuochunghsu --- envoy/rds/config_traits.h | 4 ++++ .../filters/network/thrift_proxy/conn_manager_test.cc | 11 +++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/envoy/rds/config_traits.h b/envoy/rds/config_traits.h index 57aca9e341b77..23aca7ed6e0fb 100644 --- a/envoy/rds/config_traits.h +++ b/envoy/rds/config_traits.h @@ -53,6 +53,10 @@ 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. * @throw EnvoyException if the new config can't be applied of. */ virtual ConfigConstSharedPtr createConfig(const Protobuf::Message& rc, 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 0638a1a29a8a7..d9d4befe57933 100644 --- a/test/extensions/filters/network/thrift_proxy/conn_manager_test.cc +++ b/test/extensions/filters/network/thrift_proxy/conn_manager_test.cc @@ -85,9 +85,7 @@ class ThriftConnectionManagerTest : public testing::Test { void initializeFilter() { initializeFilter(""); } - void initializeFilter(const std::string& yaml) { initializeFilter(yaml, {}); } - - void initializeFilter(const std::string& yaml, const std::vector& cluster_names) { + 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"); @@ -101,7 +99,7 @@ class ThriftConnectionManagerTest : public testing::Test { initializeFilter(config, cluster_names); } void initializeFilter(envoy::extensions::filters::network::thrift_proxy::v3::ThriftProxy& config, - const std::vector& cluster_names) { + const std::vector& cluster_names = {}) { // Destroy any existing filter first. filter_ = nullptr; @@ -1954,6 +1952,8 @@ stat_prefix: test 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, @@ -1975,6 +1975,9 @@ TEST_F(ThriftConnectionManagerTest, UnknownWeightedCluster) { 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, From f434f4c426756cb4b4f03cc65e503eda595f731f Mon Sep 17 00:00:00 2001 From: kuochunghsu Date: Sun, 3 Apr 2022 18:54:36 -0700 Subject: [PATCH 08/11] clang format and remove redundant ref Signed-off-by: kuochunghsu --- .../filters/network/thrift_proxy/router/router_impl.cc | 2 +- .../filters/network/thrift_proxy/router/router_impl.h | 2 +- .../filters/network/thrift_proxy/conn_manager_test.cc | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) 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 30c04ab4a8a7d..d992c308c8481 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc @@ -201,7 +201,7 @@ RouteConstSharedPtr ServiceNameRouteEntryImpl::matches(const MessageMetadata& me RouteMatcher::RouteMatcher( const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& config, - const absl::optional& validation_clusters) { + const absl::optional validation_clusters) { using envoy::extensions::filters::network::thrift_proxy::v3::RouteMatch; for (const auto& route : config.routes()) { 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 480f86ced3b56..748bf716fc778 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.h +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.h @@ -188,7 +188,7 @@ class RouteMatcher { // 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); + const absl::optional validation_clusters); RouteConstSharedPtr route(const MessageMetadata& metadata, uint64_t random_value) const; 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 d9d4befe57933..0d05a6e771392 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,8 @@ class ThriftConnectionManagerTest : public testing::Test { void initializeFilter() { initializeFilter(""); } - void initializeFilter(const std::string& yaml, const std::vector& cluster_names = {}) { + 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"); @@ -1952,8 +1953,7 @@ stat_prefix: test cluster: cluster2 )EOF"; - EXPECT_THROW_WITH_REGEX(initializeFilter(yaml), EnvoyException, - "unknown thrift cluster"); + 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, From 6a962915df39ec5d745c5f2e5ded66ec29e20203 Mon Sep 17 00:00:00 2001 From: kuochunghsu Date: Sun, 3 Apr 2022 19:08:04 -0700 Subject: [PATCH 09/11] add comment to state the current status Signed-off-by: kuochunghsu --- envoy/rds/config_traits.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/envoy/rds/config_traits.h b/envoy/rds/config_traits.h index 23aca7ed6e0fb..18ab097c3c71a 100644 --- a/envoy/rds/config_traits.h +++ b/envoy/rds/config_traits.h @@ -56,7 +56,9 @@ class ConfigTraits { * @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. + * 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, From b05112dec23f26f12bf935e71a5abe124f6eaade Mon Sep 17 00:00:00 2001 From: kuochunghsu Date: Mon, 4 Apr 2022 16:47:10 -0700 Subject: [PATCH 10/11] remove dead code and add test coverage Signed-off-by: kuochunghsu --- .../thrift_proxy/router/router_impl.cc | 16 ++++--------- .../network/thrift_proxy/router/router_impl.h | 2 +- .../network/thrift_proxy/conn_manager_test.cc | 24 +++++++++++++++++++ 3 files changed, 29 insertions(+), 13 deletions(-) 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 d992c308c8481..cd220bcf495bf 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc @@ -63,17 +63,9 @@ void RouteEntryImplBase::validateClusters( } } else if (!weighted_clusters_.empty()) { for (const WeightedClusterEntrySharedPtr& cluster : weighted_clusters_) { - if (!cluster->clusterName().empty()) { - if (!cluster_info_maps.hasCluster(cluster->clusterName())) { - throw EnvoyException( - fmt::format("route: unknown thrift weighted cluster '{}'", cluster->clusterName())); - } - } - // For weighted clusters with `cluster_header_name`, we only verify that this field is - // not empty because the cluster name is not set yet at config time (hence the validation - // here). - else if (cluster->clusterHeader().get().empty()) { - throw EnvoyException("route: unknown thrift weighted cluster with no cluster_header field"); + if (!cluster_info_maps.hasCluster(cluster->clusterName())) { + throw EnvoyException( + fmt::format("route: unknown thrift weighted cluster '{}'", cluster->clusterName())); } } } @@ -201,7 +193,7 @@ RouteConstSharedPtr ServiceNameRouteEntryImpl::matches(const MessageMetadata& me RouteMatcher::RouteMatcher( const envoy::extensions::filters::network::thrift_proxy::v3::RouteConfiguration& config, - const absl::optional validation_clusters) { + const absl::optional& validation_clusters) { using envoy::extensions::filters::network::thrift_proxy::v3::RouteMatch; for (const auto& route : config.routes()) { 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 748bf716fc778..480f86ced3b56 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.h +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.h @@ -188,7 +188,7 @@ class RouteMatcher { // 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); + const absl::optional& validation_clusters); RouteConstSharedPtr route(const MessageMetadata& metadata, uint64_t random_value) const; 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 0d05a6e771392..a21080162bf12 100644 --- a/test/extensions/filters/network/thrift_proxy/conn_manager_test.cc +++ b/test/extensions/filters/network/thrift_proxy/conn_manager_test.cc @@ -1984,6 +1984,30 @@ TEST_F(ThriftConnectionManagerTest, UnknownWeightedCluster) { "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 From 08dc3740a9ee1d0b28b911c37c9e1d62c733a3c7 Mon Sep 17 00:00:00 2001 From: kuochunghsu Date: Tue, 5 Apr 2022 19:09:42 -0700 Subject: [PATCH 11/11] add dep lib Signed-off-by: kuochunghsu --- envoy/rds/BUILD | 12 +++++++++++- envoy/router/BUILD | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) 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/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",