From 3f6957db9ebe13a5278d1c58fa2ea487ef9c47ac Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Tue, 12 Apr 2022 19:16:59 +0000 Subject: [PATCH 1/2] Revert "router: Remove PathMatchType and PathMatchCriterion (#20649)" This reverts commit 7014ce6fd31ef7a2f6a2083d99d4b85bbe55b6c6. Signed-off-by: Adi Suissa-Peleg --- envoy/router/router.h | 34 +++++++++++++++++++ source/common/http/async_client_impl.cc | 2 ++ source/common/http/async_client_impl.h | 9 +++++ source/common/router/config_impl.h | 25 ++++++++++++++ source/common/router/delegating_route_impl.cc | 4 +++ source/common/router/delegating_route_impl.h | 1 + test/common/http/async_client_impl_test.cc | 3 ++ test/common/http/conn_manager_impl_test.cc | 5 +++ test/common/router/config_impl_test.cc | 19 +++++++++++ test/mocks/router/mocks.cc | 8 +++++ test/mocks/router/mocks.h | 15 ++++++++ 11 files changed, 125 insertions(+) diff --git a/envoy/router/router.h b/envoy/router/router.h index 9b9dfbb1eb46f..c74fba658e1db 100644 --- a/envoy/router/router.h +++ b/envoy/router/router.h @@ -719,6 +719,35 @@ class TlsContextMatchCriteria { using TlsContextMatchCriteriaConstPtr = std::unique_ptr; +/** + * Type of path matching that a route entry uses. + */ +enum class PathMatchType { + None, + Prefix, + Exact, + Regex, + PathSeparatedPrefix, +}; + +/** + * Criterion that a route entry uses for matching a particular path. + */ +class PathMatchCriterion { +public: + virtual ~PathMatchCriterion() = default; + + /** + * @return PathMatchType type of path match. + */ + virtual PathMatchType matchType() const PURE; + + /** + * @return const std::string& the string with which to compare paths. + */ + virtual const std::string& matcher() const PURE; +}; + /** * Base class for all route typed metadata factories. */ @@ -922,6 +951,11 @@ class RouteEntry : public ResponseEntry { */ virtual const TlsContextMatchCriteria* tlsContextMatchCriteria() const PURE; + /** + * @return const PathMatchCriterion& the match criterion for this route. + */ + virtual const PathMatchCriterion& pathMatchCriterion() const PURE; + /** * True if the virtual host this RouteEntry belongs to is configured to include the attempt * count header. diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 1401626701110..1352861406a77 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -25,6 +25,8 @@ const AsyncStreamImpl::NullVirtualHost AsyncStreamImpl::RouteEntryImpl::virtual_ const AsyncStreamImpl::NullRateLimitPolicy AsyncStreamImpl::NullVirtualHost::rate_limit_policy_; const AsyncStreamImpl::NullConfig AsyncStreamImpl::NullVirtualHost::route_configuration_; const std::multimap AsyncStreamImpl::RouteEntryImpl::opaque_config_; +const AsyncStreamImpl::NullPathMatchCriterion + AsyncStreamImpl::RouteEntryImpl::path_match_criterion_; const absl::optional AsyncStreamImpl::RouteEntryImpl::connect_config_nullopt_; const std::list AsyncStreamImpl::NullConfig::internal_only_headers_; diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index d53f19ba9e55f..e68856ade3498 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -169,6 +169,11 @@ class AsyncStreamImpl : public AsyncClient::Stream, static const NullConfig route_configuration_; }; + struct NullPathMatchCriterion : public Router::PathMatchCriterion { + Router::PathMatchType matchType() const override { return Router::PathMatchType::None; } + const std::string& matcher() const override { return EMPTY_STRING; } + }; + struct RouteEntryImpl : public Router::RouteEntry { RouteEntryImpl( AsyncClientImpl& parent, const absl::optional& timeout, @@ -265,6 +270,9 @@ class AsyncStreamImpl : public AsyncClient::Stream, bool autoHostRewrite() const override { return false; } bool appendXfh() const override { return false; } bool includeVirtualHostRateLimits() const override { return true; } + const Router::PathMatchCriterion& pathMatchCriterion() const override { + return path_match_criterion_; + } const absl::optional& connectConfig() const override { return connect_config_nullopt_; @@ -283,6 +291,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, static const std::vector shadow_policies_; static const NullVirtualHost virtual_host_; static const std::multimap opaque_config_; + static const NullPathMatchCriterion path_match_criterion_; Router::RouteEntry::UpgradeMap upgrade_map_; const std::string& cluster_name_; diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index a22e7b01a7f4c..c226277d261ab 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -497,6 +497,7 @@ class RouteEntryImplBase : public RouteEntry, public Matchable, public DirectResponseEntry, public Route, + public PathMatchCriterion, public std::enable_shared_from_this, Logger::Loggable { public: @@ -586,6 +587,7 @@ class RouteEntryImplBase : public RouteEntry, bool includeVirtualHostRateLimits() const override { return include_vh_rate_limits_; } const envoy::config::core::v3::Metadata& metadata() const override { return metadata_; } const Envoy::Config::TypedMetadata& typedMetadata() const override { return typed_metadata_; } + const PathMatchCriterion& pathMatchCriterion() const override { return *this; } bool includeAttemptCountInRequest() const override { return vhost_.includeAttemptCountInRequest(); } @@ -747,6 +749,9 @@ class RouteEntryImplBase : public RouteEntry, const Envoy::Config::TypedMetadata& typedMetadata() const override { return parent_->typedMetadata(); } + const PathMatchCriterion& pathMatchCriterion() const override { + return parent_->pathMatchCriterion(); + } bool includeAttemptCountInRequest() const override { return parent_->includeAttemptCountInRequest(); @@ -983,6 +988,10 @@ class PrefixRouteEntryImpl : public RouteEntryImplBase { Server::Configuration::ServerFactoryContext& factory_context, ProtobufMessage::ValidationVisitor& validator); + // Router::PathMatchCriterion + const std::string& matcher() const override { return prefix_; } + PathMatchType matchType() const override { return PathMatchType::Prefix; } + // Router::Matchable RouteConstSharedPtr matches(const Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo& stream_info, @@ -1011,6 +1020,10 @@ class PathRouteEntryImpl : public RouteEntryImplBase { Server::Configuration::ServerFactoryContext& factory_context, ProtobufMessage::ValidationVisitor& validator); + // Router::PathMatchCriterion + const std::string& matcher() const override { return path_; } + PathMatchType matchType() const override { return PathMatchType::Exact; } + // Router::Matchable RouteConstSharedPtr matches(const Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo& stream_info, @@ -1039,6 +1052,10 @@ class RegexRouteEntryImpl : public RouteEntryImplBase { Server::Configuration::ServerFactoryContext& factory_context, ProtobufMessage::ValidationVisitor& validator); + // Router::PathMatchCriterion + const std::string& matcher() const override { return regex_str_; } + PathMatchType matchType() const override { return PathMatchType::Regex; } + // Router::Matchable RouteConstSharedPtr matches(const Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo& stream_info, @@ -1067,6 +1084,10 @@ class ConnectRouteEntryImpl : public RouteEntryImplBase { Server::Configuration::ServerFactoryContext& factory_context, ProtobufMessage::ValidationVisitor& validator); + // Router::PathMatchCriterion + const std::string& matcher() const override { return EMPTY_STRING; } + PathMatchType matchType() const override { return PathMatchType::None; } + // Router::Matchable RouteConstSharedPtr matches(const Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo& stream_info, @@ -1093,6 +1114,10 @@ class PathSeparatedPrefixRouteEntryImpl : public RouteEntryImplBase { Server::Configuration::ServerFactoryContext& factory_context, ProtobufMessage::ValidationVisitor& validator); + // Router::PathMatchCriterion + const std::string& matcher() const override { return prefix_; } + PathMatchType matchType() const override { return PathMatchType::PathSeparatedPrefix; } + // Router::Matchable RouteConstSharedPtr matches(const Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo& stream_info, diff --git a/source/common/router/delegating_route_impl.cc b/source/common/router/delegating_route_impl.cc index b134738b649f8..e04febfd6cb94 100644 --- a/source/common/router/delegating_route_impl.cc +++ b/source/common/router/delegating_route_impl.cc @@ -150,6 +150,10 @@ const TlsContextMatchCriteria* DelegatingRouteEntry::tlsContextMatchCriteria() c return base_route_->routeEntry()->tlsContextMatchCriteria(); } +const PathMatchCriterion& DelegatingRouteEntry::pathMatchCriterion() const { + return base_route_->routeEntry()->pathMatchCriterion(); +} + bool DelegatingRouteEntry::includeAttemptCountInRequest() const { return base_route_->routeEntry()->includeAttemptCountInRequest(); } diff --git a/source/common/router/delegating_route_impl.h b/source/common/router/delegating_route_impl.h index aa3a3e30e871a..f2667f5837c43 100644 --- a/source/common/router/delegating_route_impl.h +++ b/source/common/router/delegating_route_impl.h @@ -103,6 +103,7 @@ class DelegatingRouteEntry : public Router::RouteEntry { const std::multimap& opaqueConfig() const override; bool includeVirtualHostRateLimits() const override; const TlsContextMatchCriteria* tlsContextMatchCriteria() const override; + const PathMatchCriterion& pathMatchCriterion() const override; bool includeAttemptCountInRequest() const override; bool includeAttemptCountInResponse() const override; const UpgradeMap& upgradeMap() const override; diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index b6589ce98f9d5..5216c97a4e03b 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -1674,6 +1674,9 @@ TEST_F(AsyncClientImplTest, RdsGettersTest) { ASSERT_NE(nullptr, route); auto route_entry = route->routeEntry(); ASSERT_NE(nullptr, route_entry); + auto& path_match_criterion = route_entry->pathMatchCriterion(); + EXPECT_EQ("", path_match_criterion.matcher()); + EXPECT_EQ(Router::PathMatchType::None, path_match_criterion.matchType()); const auto& route_config = route_entry->virtualHost().routeConfig(); EXPECT_EQ("", route_config.name()); EXPECT_EQ(0, route_config.internalOnlyHeaders().size()); diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index c832637905c28..e0b57657f92dc 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -1074,6 +1074,11 @@ TEST_F(HttpConnectionManagerImplTest, DelegatingRouteEntryAllCalls) { EXPECT_EQ(default_route->routeEntry()->tlsContextMatchCriteria(), delegating_route_foo->routeEntry()->tlsContextMatchCriteria()); + EXPECT_EQ(default_route->routeEntry()->pathMatchCriterion().matcher(), + delegating_route_foo->routeEntry()->pathMatchCriterion().matcher()); + EXPECT_EQ(default_route->routeEntry()->pathMatchCriterion().matchType(), + delegating_route_foo->routeEntry()->pathMatchCriterion().matchType()); + EXPECT_EQ(default_route->routeEntry()->includeAttemptCountInRequest(), delegating_route_foo->routeEntry()->includeAttemptCountInRequest()); EXPECT_EQ(default_route->routeEntry()->includeAttemptCountInResponse(), diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 12e33346299f6..416fd4cb95f95 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -3114,6 +3114,7 @@ TEST_F(RouteMatcherTest, ClusterHeader) { route->routeEntry()->virtualCluster(headers); route->routeEntry()->virtualHost(); route->routeEntry()->virtualHost().rateLimitPolicy(); + route->routeEntry()->pathMatchCriterion(); route->routeEntry()->hedgePolicy(); route->routeEntry()->maxGrpcTimeout(); route->routeEntry()->grpcTimeoutOffset(); @@ -6787,6 +6788,16 @@ TEST_F(RouteConfigurationV2, DirectResponseTooLarge) { EnvoyException, "response body size is 4097 bytes; maximum is 4096"); } +void checkPathMatchCriterion(const Route* route, const std::string& expected_matcher, + PathMatchType expected_type) { + ASSERT_NE(nullptr, route); + const auto route_entry = route->routeEntry(); + ASSERT_NE(nullptr, route_entry); + const auto& match_criterion = route_entry->pathMatchCriterion(); + EXPECT_EQ(expected_matcher, match_criterion.matcher()); + EXPECT_EQ(expected_type, match_criterion.matchType()); +} + // Test loading broken config throws EnvoyException. TEST_F(RouteConfigurationV2, BrokenTypedMetadata) { const std::string yaml = R"EOF( @@ -6831,7 +6842,15 @@ name: foo factory_context_.cluster_manager_.initializeClusters({"ww2", "www2"}, {}); const TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); + checkPathMatchCriterion(config.route(genHeaders("www.foo.com", "/regex", "GET"), 0).get(), + "/rege[xy]", PathMatchType::Regex); + checkPathMatchCriterion(config.route(genHeaders("www.foo.com", "/exact-path", "GET"), 0).get(), + "/exact-path", PathMatchType::Exact); + checkPathMatchCriterion( + config.route(genHeaders("www.foo.com", "/path/separated", "GET"), 0).get(), "/path/separated", + PathMatchType::PathSeparatedPrefix); const auto route = config.route(genHeaders("www.foo.com", "/", "GET"), 0); + checkPathMatchCriterion(route.get(), "/", PathMatchType::Prefix); const auto route_entry = route->routeEntry(); const auto& metadata = route->metadata(); diff --git a/test/mocks/router/mocks.cc b/test/mocks/router/mocks.cc index 75af9bba0a751..d5e8d21f0763f 100644 --- a/test/mocks/router/mocks.cc +++ b/test/mocks/router/mocks.cc @@ -87,6 +87,13 @@ MockMetadataMatchCriteria::~MockMetadataMatchCriteria() = default; MockTlsContextMatchCriteria::MockTlsContextMatchCriteria() = default; MockTlsContextMatchCriteria::~MockTlsContextMatchCriteria() = default; +MockPathMatchCriterion::MockPathMatchCriterion() { + ON_CALL(*this, matchType()).WillByDefault(ReturnPointee(&type_)); + ON_CALL(*this, matcher()).WillByDefault(ReturnPointee(&matcher_)); +} + +MockPathMatchCriterion::~MockPathMatchCriterion() = default; + MockRouteEntry::MockRouteEntry() { ON_CALL(*this, clusterName()).WillByDefault(ReturnRef(cluster_name_)); ON_CALL(*this, opaqueConfig()).WillByDefault(ReturnRef(opaque_config_)); @@ -100,6 +107,7 @@ MockRouteEntry::MockRouteEntry() { ON_CALL(*this, virtualCluster(_)).WillByDefault(Return(&virtual_cluster_)); ON_CALL(*this, virtualHost()).WillByDefault(ReturnRef(virtual_host_)); ON_CALL(*this, includeVirtualHostRateLimits()).WillByDefault(Return(true)); + ON_CALL(*this, pathMatchCriterion()).WillByDefault(ReturnRef(path_match_criterion_)); ON_CALL(*this, upgradeMap()).WillByDefault(ReturnRef(upgrade_map_)); ON_CALL(*this, hedgePolicy()).WillByDefault(ReturnRef(hedge_policy_)); ON_CALL(*this, routeName()).WillByDefault(ReturnRef(route_name_)); diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index d2fce4a4f3cef..af39348b5fc31 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -329,6 +329,19 @@ class MockTlsContextMatchCriteria : public TlsContextMatchCriteria { MOCK_METHOD(const absl::optional&, validated, (), (const)); }; +class MockPathMatchCriterion : public PathMatchCriterion { +public: + MockPathMatchCriterion(); + ~MockPathMatchCriterion() override; + + // Router::PathMatchCriterion + MOCK_METHOD(PathMatchType, matchType, (), (const)); + MOCK_METHOD(const std::string&, matcher, (), (const)); + + PathMatchType type_; + std::string matcher_; +}; + class MockRouteEntry : public RouteEntry { public: MockRouteEntry(); @@ -375,6 +388,7 @@ class MockRouteEntry : public RouteEntry { MOCK_METHOD(const CorsPolicy*, corsPolicy, (), (const)); MOCK_METHOD(absl::optional, currentUrlPathAfterRewrite, (const Http::RequestHeaderMap&), (const)); + MOCK_METHOD(const PathMatchCriterion&, pathMatchCriterion, (), (const)); MOCK_METHOD(bool, includeAttemptCountInRequest, (), (const)); MOCK_METHOD(bool, includeAttemptCountInResponse, (), (const)); MOCK_METHOD(const absl::optional&, connectConfig, (), (const)); @@ -395,6 +409,7 @@ class MockRouteEntry : public RouteEntry { MockMetadataMatchCriteria metadata_matches_criteria_; MockTlsContextMatchCriteria tls_context_matches_criteria_; TestCorsPolicy cors_policy_; + testing::NiceMock path_match_criterion_; UpgradeMap upgrade_map_; absl::optional connect_config_; }; From 98720c128f78ff30f266ece5c9338030701b1892 Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Tue, 12 Apr 2022 19:22:19 +0000 Subject: [PATCH 2/2] Adding a comment that explains how it can be used Signed-off-by: Adi Suissa-Peleg --- envoy/router/router.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/envoy/router/router.h b/envoy/router/router.h index c74fba658e1db..ec746aef070b2 100644 --- a/envoy/router/router.h +++ b/envoy/router/router.h @@ -732,6 +732,8 @@ enum class PathMatchType { /** * Criterion that a route entry uses for matching a particular path. + * Extensions can use this to gain better insights of chosen route paths, + * see: https://github.com/envoyproxy/envoy/pull/2531. */ class PathMatchCriterion { public: