diff --git a/api/envoy/config/filter/network/thrift_proxy/v2alpha1/route.proto b/api/envoy/config/filter/network/thrift_proxy/v2alpha1/route.proto index f70523e57f2..7aab4257c80 100644 --- a/api/envoy/config/filter/network/thrift_proxy/v2alpha1/route.proto +++ b/api/envoy/config/filter/network/thrift_proxy/v2alpha1/route.proto @@ -27,11 +27,24 @@ message Route { RouteAction route = 2 [(validate.rules).message.required = true, (gogoproto.nullable) = false]; } -// [#comment:next free field: 2] +// [#comment:next free field: 4] message RouteMatch { - // If specified, the route must exactly match the request method name. As a special case, an - // empty string matches any request method name. - string method = 1; + oneof match_specifier { + option (validate.required) = true; + + // If specified, the route must exactly match the request method name. As a special case, an + // empty string matches any request method name. + string method_name = 1; + + // If specified, the route must have the service name as the request method name prefix. As a + // special case, an empty string matches any service name. Only relevant when service + // multiplexing. + string service_name = 2; + } + + // Inverts whatever matching is done in match_specifier. Cannot be combined with wildcard matching + // as that would result in routes never being matched. + bool invert = 3; } // [#comment:next free field: 2] 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 2340ece251d..9d2d892ace5 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc @@ -4,6 +4,8 @@ #include "envoy/upstream/cluster_manager.h" #include "envoy/upstream/thread_local_cluster.h" +#include "common/common/utility.h" + #include "extensions/filters/network/thrift_proxy/app_exception_impl.h" namespace Envoy { @@ -24,14 +26,45 @@ RouteConstSharedPtr RouteEntryImplBase::clusterEntry() const { return shared_fro MethodNameRouteEntryImpl::MethodNameRouteEntryImpl( const envoy::config::filter::network::thrift_proxy::v2alpha1::Route& route) - : RouteEntryImplBase(route), method_name_(route.match().method()) {} + : RouteEntryImplBase(route), method_name_(route.match().method_name()), + invert_(route.match().invert()) { + if (method_name_.empty() && invert_) { + throw EnvoyException("Cannot have an empty method name with inversion enabled"); + } +} RouteConstSharedPtr MethodNameRouteEntryImpl::matches(const MessageMetadata& metadata) const { - if (method_name_.empty()) { + bool matches = + method_name_.empty() || (metadata.hasMethodName() && metadata.methodName() == method_name_); + + if (matches ^ invert_) { return clusterEntry(); } - if (metadata.hasMethodName() && metadata.methodName() == method_name_) { + return nullptr; +} + +ServiceNameRouteEntryImpl::ServiceNameRouteEntryImpl( + const envoy::config::filter::network::thrift_proxy::v2alpha1::Route& route) + : RouteEntryImplBase(route), invert_(route.match().invert()) { + const std::string service_name = route.match().service_name(); + if (service_name.empty() && invert_) { + throw EnvoyException("Cannot have an empty service name with inversion enabled"); + } + + if (!service_name.empty() && !StringUtil::endsWith(service_name, ":")) { + service_name_ = service_name + ":"; + } else { + service_name_ = service_name; + } +} + +RouteConstSharedPtr ServiceNameRouteEntryImpl::matches(const MessageMetadata& metadata) const { + bool matches = service_name_.empty() || + (metadata.hasMethodName() && + StringUtil::startsWith(metadata.methodName().c_str(), service_name_)); + + if (matches ^ invert_) { return clusterEntry(); } @@ -40,8 +73,19 @@ RouteConstSharedPtr MethodNameRouteEntryImpl::matches(const MessageMetadata& met RouteMatcher::RouteMatcher( const envoy::config::filter::network::thrift_proxy::v2alpha1::RouteConfiguration& config) { + using envoy::config::filter::network::thrift_proxy::v2alpha1::RouteMatch; + for (const auto& route : config.routes()) { - routes_.emplace_back(new MethodNameRouteEntryImpl(route)); + switch (route.match().match_specifier_case()) { + case RouteMatch::MatchSpecifierCase::kMethodName: + routes_.emplace_back(new MethodNameRouteEntryImpl(route)); + break; + case RouteMatch::MatchSpecifierCase::kServiceName: + routes_.emplace_back(new ServiceNameRouteEntryImpl(route)); + break; + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } } } 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 8255f345480..2a4997efb16 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.h +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.h @@ -52,11 +52,27 @@ class MethodNameRouteEntryImpl : public RouteEntryImplBase { const std::string& methodName() const { return method_name_; } - // RoutEntryImplBase + // RouteEntryImplBase RouteConstSharedPtr matches(const MessageMetadata& metadata) const override; private: const std::string method_name_; + const bool invert_; +}; + +class ServiceNameRouteEntryImpl : public RouteEntryImplBase { +public: + ServiceNameRouteEntryImpl( + const envoy::config::filter::network::thrift_proxy::v2alpha1::Route& route); + + const std::string& serviceName() const { return service_name_; } + + // RouteEntryImplBase + RouteConstSharedPtr matches(const MessageMetadata& metadata) const override; + +private: + std::string service_name_; + const bool invert_; }; class RouteMatcher { 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 790931376f3..1710194965f 100644 --- a/test/extensions/filters/network/thrift_proxy/conn_manager_test.cc +++ b/test/extensions/filters/network/thrift_proxy/conn_manager_test.cc @@ -518,7 +518,7 @@ stat_prefix: test name: "routes" routes: - match: - method: name + method_name: name route: cluster: cluster )EOF"; diff --git a/test/extensions/filters/network/thrift_proxy/integration_test.cc b/test/extensions/filters/network/thrift_proxy/integration_test.cc index b50aa45e0af..d64fb0e9438 100644 --- a/test/extensions/filters/network/thrift_proxy/integration_test.cc +++ b/test/extensions/filters/network/thrift_proxy/integration_test.cc @@ -46,9 +46,18 @@ class ThriftConnManagerIntegrationTest route_config: name: "routes" routes: - - match: {} + - match: + service_name: "svcname" route: cluster: "cluster_0" + - match: + method_name: "execute" + route: + cluster: "cluster_1" + - match: + method_name: "poke" + route: + cluster: "cluster_2" )EOF"; } @@ -73,8 +82,7 @@ class ThriftConnManagerIntegrationTest preparePayloads(result_mode, "execute"); ASSERT(request_bytes_.length() > 0); ASSERT(response_bytes_.length() > 0); - - BaseIntegrationTest::initialize(); + initializeCommon(); } void initializeOneway() { @@ -84,6 +92,24 @@ class ThriftConnManagerIntegrationTest ASSERT(request_bytes_.length() > 0); ASSERT(response_bytes_.length() == 0); + initializeCommon(); + } + + // We allocate as many upstreams as there are clusters, with each upstream being allocated + // to clusters in the order they're defined in the bootstrap config. + void initializeCommon() { + setUpstreamCount(3); + + config_helper_.addConfigModifier([](envoy::config::bootstrap::v2::Bootstrap& bootstrap) { + auto* c1 = bootstrap.mutable_static_resources()->add_clusters(); + c1->MergeFrom(bootstrap.static_resources().clusters()[0]); + c1->set_name("cluster_1"); + + auto* c2 = bootstrap.mutable_static_resources()->add_clusters(); + c2->MergeFrom(bootstrap.static_resources().clusters()[0]); + c2->set_name("cluster_2"); + }); + BaseIntegrationTest::initialize(); } @@ -140,6 +166,20 @@ class ThriftConnManagerIntegrationTest } } + // Multiplexed requests are handled by the service name route match, + // while oneway's are handled by the "poke" method. All other requests + // are handled by "execute". + FakeUpstream* getExpectedUpstream(bool oneway) { + int upstreamIdx = 1; + if (multiplexed_) { + upstreamIdx = 0; + } else if (oneway) { + upstreamIdx = 2; + } + + return fake_upstreams_[upstreamIdx].get(); + } + std::string transport_; std::string protocol_; bool multiplexed_; @@ -176,7 +216,8 @@ TEST_P(ThriftConnManagerIntegrationTest, Success) { tcp_client->write(request_bytes_.toString()); FakeRawConnectionPtr fake_upstream_connection; - ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + FakeUpstream* expected_upstream = getExpectedUpstream(false); + ASSERT_TRUE(expected_upstream->waitForRawConnection(fake_upstream_connection)); std::string data; ASSERT_TRUE(fake_upstream_connection->waitForData(request_bytes_.length(), &data)); Buffer::OwnedImpl upstream_request(data); @@ -201,8 +242,9 @@ TEST_P(ThriftConnManagerIntegrationTest, IDLException) { IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0")); tcp_client->write(request_bytes_.toString()); + FakeUpstream* expected_upstream = getExpectedUpstream(false); FakeRawConnectionPtr fake_upstream_connection; - ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT_TRUE(expected_upstream->waitForRawConnection(fake_upstream_connection)); std::string data; ASSERT_TRUE(fake_upstream_connection->waitForData(request_bytes_.length(), &data)); Buffer::OwnedImpl upstream_request(data); @@ -227,8 +269,9 @@ TEST_P(ThriftConnManagerIntegrationTest, Exception) { IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0")); tcp_client->write(request_bytes_.toString()); + FakeUpstream* expected_upstream = getExpectedUpstream(false); FakeRawConnectionPtr fake_upstream_connection; - ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT_TRUE(expected_upstream->waitForRawConnection(fake_upstream_connection)); std::string data; ASSERT_TRUE(fake_upstream_connection->waitForData(request_bytes_.length(), &data)); Buffer::OwnedImpl upstream_request(data); @@ -253,8 +296,9 @@ TEST_P(ThriftConnManagerIntegrationTest, Oneway) { IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0")); tcp_client->write(request_bytes_.toString()); + FakeUpstream* expected_upstream = getExpectedUpstream(true); FakeRawConnectionPtr fake_upstream_connection; - ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT_TRUE(expected_upstream->waitForRawConnection(fake_upstream_connection)); std::string data; ASSERT_TRUE(fake_upstream_connection->waitForData(request_bytes_.length(), &data)); Buffer::OwnedImpl upstream_request(data); @@ -274,8 +318,9 @@ TEST_P(ThriftConnManagerIntegrationTest, OnewayEarlyClose) { tcp_client->write(request_bytes_.toString()); tcp_client->close(); + FakeUpstream* expected_upstream = getExpectedUpstream(true); FakeRawConnectionPtr fake_upstream_connection; - ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT_TRUE(expected_upstream->waitForRawConnection(fake_upstream_connection)); std::string data; ASSERT_TRUE(fake_upstream_connection->waitForData(request_bytes_.length(), &data)); Buffer::OwnedImpl upstream_request(data); diff --git a/test/extensions/filters/network/thrift_proxy/router_test.cc b/test/extensions/filters/network/thrift_proxy/router_test.cc index 93fb798460f..d1a83977fe8 100644 --- a/test/extensions/filters/network/thrift_proxy/router_test.cc +++ b/test/extensions/filters/network/thrift_proxy/router_test.cc @@ -688,16 +688,16 @@ TEST_P(ThriftRouterContainerTest, DecoderFilterCallbacks) { destroyRouter(); } -TEST(RouteMatcherTest, Route) { +TEST(RouteMatcherTest, RouteByMethodNameWithNoInversion) { const std::string yaml = R"EOF( name: config routes: - match: - method: "method1" + method_name: "method1" route: cluster: "cluster1" - match: - method: "method2" + method_name: "method2" route: cluster: "cluster2" )EOF"; @@ -724,15 +724,60 @@ name: config EXPECT_EQ("cluster2", route2->routeEntry()->clusterName()); } -TEST(RouteMatcherTest, RouteMatchAny) { +TEST(RouteMatcherTest, RouteByMethodNameWithInversion) { const std::string yaml = R"EOF( name: config routes: - match: - method: "method1" + method_name: "method1" route: cluster: "cluster1" - - match: {} + - match: + method_name: "method2" + invert: true + route: + cluster: "cluster2" +)EOF"; + + envoy::config::filter::network::thrift_proxy::v2alpha1::RouteConfiguration config = + parseRouteConfigurationFromV2Yaml(yaml); + + RouteMatcher matcher(config); + MessageMetadata metadata; + RouteConstSharedPtr route = matcher.route(metadata); + EXPECT_NE(nullptr, route); + EXPECT_EQ("cluster2", route->routeEntry()->clusterName()); + + metadata.setMethodName("unknown"); + route = matcher.route(metadata); + EXPECT_NE(nullptr, route); + EXPECT_EQ("cluster2", route->routeEntry()->clusterName()); + + metadata.setMethodName("METHOD1"); + route = matcher.route(metadata); + EXPECT_NE(nullptr, route); + EXPECT_EQ("cluster2", route->routeEntry()->clusterName()); + + metadata.setMethodName("method1"); + route = matcher.route(metadata); + EXPECT_NE(nullptr, route); + EXPECT_EQ("cluster1", route->routeEntry()->clusterName()); + + metadata.setMethodName("method2"); + route = matcher.route(metadata); + EXPECT_EQ(nullptr, route); +} + +TEST(RouteMatcherTest, RouteByAnyMethodNameWithNoInversion) { + const std::string yaml = R"EOF( +name: config +routes: + - match: + method_name: "method1" + route: + cluster: "cluster1" + - match: + method_name: "" route: cluster: "cluster2" )EOF"; @@ -763,6 +808,160 @@ name: config } } +TEST(RouteMatcherTest, RouteByAnyMethodNameWithInversion) { + const std::string yaml = R"EOF( +name: config +routes: + - match: + method_name: "" + invert: true + route: + cluster: "cluster2" +)EOF"; + + envoy::config::filter::network::thrift_proxy::v2alpha1::RouteConfiguration config = + parseRouteConfigurationFromV2Yaml(yaml); + + EXPECT_THROW(new RouteMatcher(config), EnvoyException); +} + +TEST(RouteMatcherTest, RouteByServiceNameWithNoInversion) { + const std::string yaml = R"EOF( +name: config +routes: + - match: + method_name: "method1" + route: + cluster: "cluster1" + - match: + service_name: "service2" + route: + cluster: "cluster2" +)EOF"; + + envoy::config::filter::network::thrift_proxy::v2alpha1::RouteConfiguration config = + parseRouteConfigurationFromV2Yaml(yaml); + + RouteMatcher matcher(config); + MessageMetadata metadata; + EXPECT_EQ(nullptr, matcher.route(metadata)); + metadata.setMethodName("unknown"); + EXPECT_EQ(nullptr, matcher.route(metadata)); + metadata.setMethodName("METHOD1"); + EXPECT_EQ(nullptr, matcher.route(metadata)); + + metadata.setMethodName("service2:method1"); + RouteConstSharedPtr route = matcher.route(metadata); + EXPECT_NE(nullptr, route); + EXPECT_EQ("cluster2", route->routeEntry()->clusterName()); + + metadata.setMethodName("service2:method2"); + RouteConstSharedPtr route2 = matcher.route(metadata); + EXPECT_NE(nullptr, route2); + EXPECT_EQ("cluster2", route2->routeEntry()->clusterName()); +} + +TEST(RouteMatcherTest, RouteByServiceNameWithInversion) { + const std::string yaml = R"EOF( +name: config +routes: + - match: + method_name: "method1" + route: + cluster: "cluster1" + - match: + service_name: "service2" + invert: true + route: + cluster: "cluster2" +)EOF"; + + envoy::config::filter::network::thrift_proxy::v2alpha1::RouteConfiguration config = + parseRouteConfigurationFromV2Yaml(yaml); + + RouteMatcher matcher(config); + MessageMetadata metadata; + RouteConstSharedPtr route = matcher.route(metadata); + EXPECT_NE(nullptr, route); + EXPECT_EQ("cluster2", route->routeEntry()->clusterName()); + + metadata.setMethodName("unknown"); + route = matcher.route(metadata); + EXPECT_NE(nullptr, route); + EXPECT_EQ("cluster2", route->routeEntry()->clusterName()); + + metadata.setMethodName("METHOD1"); + route = matcher.route(metadata); + EXPECT_NE(nullptr, route); + EXPECT_EQ("cluster2", route->routeEntry()->clusterName()); + + metadata.setMethodName("method1"); + route = matcher.route(metadata); + EXPECT_NE(nullptr, route); + EXPECT_EQ("cluster1", route->routeEntry()->clusterName()); + + metadata.setMethodName("service2:method1"); + route = matcher.route(metadata); + EXPECT_EQ(nullptr, route); +} + +TEST(RouteMatcherTest, RouteByAnyServiceNameWithNoInversion) { + const std::string yaml = R"EOF( +name: config +routes: + - match: + method_name: "method1" + route: + cluster: "cluster1" + - match: + service_name: "" + route: + cluster: "cluster2" +)EOF"; + + envoy::config::filter::network::thrift_proxy::v2alpha1::RouteConfiguration config = + parseRouteConfigurationFromV2Yaml(yaml); + + RouteMatcher matcher(config); + + { + MessageMetadata metadata; + metadata.setMethodName("method1"); + RouteConstSharedPtr route = matcher.route(metadata); + EXPECT_NE(nullptr, route); + EXPECT_EQ("cluster1", route->routeEntry()->clusterName()); + + metadata.setMethodName("anything"); + RouteConstSharedPtr route2 = matcher.route(metadata); + EXPECT_NE(nullptr, route2); + EXPECT_EQ("cluster2", route2->routeEntry()->clusterName()); + } + + { + MessageMetadata metadata; + RouteConstSharedPtr route2 = matcher.route(metadata); + EXPECT_NE(nullptr, route2); + EXPECT_EQ("cluster2", route2->routeEntry()->clusterName()); + } +} + +TEST(RouteMatcherTest, RouteByAnyServiceNameWithInversion) { + const std::string yaml = R"EOF( +name: config +routes: + - match: + service_name: "" + invert: true + route: + cluster: "cluster2" +)EOF"; + + envoy::config::filter::network::thrift_proxy::v2alpha1::RouteConfiguration config = + parseRouteConfigurationFromV2Yaml(yaml); + + EXPECT_THROW(new RouteMatcher(config), EnvoyException); +} + } // namespace Router } // namespace ThriftProxy } // namespace NetworkFilters