-
Notifications
You must be signed in to change notification settings - Fork 5.3k
router: add new path-splitting prefix matcher #20145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
04559b0
bb31a66
928235a
6bc04ba
17c404f
62988f1
e71d8d7
4697277
0b6527c
6a78c3e
0aa1804
4958eb7
aeb6f0e
dd8972a
2202337
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -727,6 +727,7 @@ enum class PathMatchType { | |
| Prefix, | ||
| Exact, | ||
| Regex, | ||
| PathSeparatedPrefix, | ||
| }; | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -92,6 +92,11 @@ RouteEntryImplBaseConstSharedPtr createAndValidateRoute( | |||
| route = std::make_shared<ConnectRouteEntryImpl>(vhost, route_config, optional_http_filters, | ||||
| factory_context, validator); | ||||
| break; | ||||
| case envoy::config::route::v3::RouteMatch::PathSpecifierCase::kPathSeparatedPrefix: { | ||||
| route = std::make_shared<PathSeparatedPrefixRouteEntryImpl>( | ||||
| vhost, route_config, optional_http_filters, factory_context, validator); | ||||
| break; | ||||
| } | ||||
| case envoy::config::route::v3::RouteMatch::PathSpecifierCase::PATH_SPECIFIER_NOT_SET: | ||||
| break; // throw the error below. | ||||
| } | ||||
|
|
@@ -1402,6 +1407,40 @@ RouteConstSharedPtr ConnectRouteEntryImpl::matches(const Http::RequestHeaderMap& | |||
| return nullptr; | ||||
| } | ||||
|
|
||||
| PathSeparatedPrefixRouteEntryImpl::PathSeparatedPrefixRouteEntryImpl( | ||||
| const VirtualHostImpl& vhost, const envoy::config::route::v3::Route& route, | ||||
| const OptionalHttpFilters& optional_http_filters, | ||||
| Server::Configuration::ServerFactoryContext& factory_context, | ||||
| ProtobufMessage::ValidationVisitor& validator) | ||||
| : RouteEntryImplBase(vhost, route, optional_http_filters, factory_context, validator), | ||||
| prefix_(route.match().path_separated_prefix()), | ||||
| path_matcher_(Matchers::PathMatcher::createPrefix(prefix_, !case_sensitive_)) {} | ||||
|
|
||||
| void PathSeparatedPrefixRouteEntryImpl::rewritePathHeader(Http::RequestHeaderMap& headers, | ||||
| bool insert_envoy_original_path) const { | ||||
| finalizePathHeader(headers, prefix_, insert_envoy_original_path); | ||||
| } | ||||
|
|
||||
| absl::optional<std::string> PathSeparatedPrefixRouteEntryImpl::currentUrlPathAfterRewrite( | ||||
| const Http::RequestHeaderMap& headers) const { | ||||
| return currentUrlPathAfterRewriteWithMatchedPath(headers, prefix_); | ||||
| } | ||||
|
|
||||
| RouteConstSharedPtr | ||||
| PathSeparatedPrefixRouteEntryImpl::matches(const Http::RequestHeaderMap& headers, | ||||
| const StreamInfo::StreamInfo& stream_info, | ||||
| uint64_t random_value) const { | ||||
| if (!RouteEntryImplBase::matchRoute(headers, stream_info, random_value)) { | ||||
| return nullptr; | ||||
| } | ||||
| absl::string_view path = Http::PathUtil::removeQueryAndFragment(headers.getPathValue()); | ||||
| if (path.size() >= prefix_.size() && path_matcher_->match(path) && | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The envoy/source/common/common/matchers.cc Line 126 in b77651d
Not sure whether worth to use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you are correct.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seemed like the most concise way to do it, without expanding the StringMatcher definition. |
||||
| (path.size() == prefix_.size() || path[prefix_.size()] == '/')) { | ||||
| return clusterEntry(headers, random_value); | ||||
| } | ||||
| return nullptr; | ||||
| } | ||||
|
|
||||
| VirtualHostImpl::VirtualHostImpl( | ||||
| const envoy::config::route::v3::VirtualHost& virtual_host, | ||||
| const OptionalHttpFilters& optional_http_filters, const ConfigImpl& global_route_config, | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| #include "source/common/common/logger.h" | ||
| #include "source/common/common/matchers.h" | ||
| #include "source/common/common/regex.h" | ||
| #include "source/common/http/path_utility.h" | ||
| #include "source/common/router/config_impl.h" | ||
|
|
||
| #include "absl/strings/match.h" | ||
|
|
@@ -154,6 +155,31 @@ class ConnectMatcherImpl : public BaseMatcherImpl { | |
| return false; | ||
| } | ||
| }; | ||
|
|
||
| class PathSeparatedPrefixMatcherImpl : public BaseMatcherImpl { | ||
| public: | ||
| PathSeparatedPrefixMatcherImpl(const RequirementRule& rule) | ||
| : BaseMatcherImpl(rule), prefix_(rule.match().path_separated_prefix()), | ||
| path_matcher_(Matchers::PathMatcher::createPrefix(prefix_, !case_sensitive_)) {} | ||
|
|
||
| bool matches(const Http::RequestHeaderMap& headers) const override { | ||
| if (!BaseMatcherImpl::matchRoute(headers)) { | ||
| return false; | ||
| } | ||
| absl::string_view path = Http::PathUtil::removeQueryAndFragment(headers.getPathValue()); | ||
| if (path.size() >= prefix_.size() && path_matcher_->match(path) && | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit unhappy we have duplicative logic here @qiwzhang. I've probably complained before and this is unrelated to this PR, so just an observation, but by having to copy+paste all our matching logic in two places, we are leaving open a significant opportunity for drift and check vs. use semantic differences between this and routing. |
||
| (path.size() == prefix_.size() || path[prefix_.size()] == '/')) { | ||
| ENVOY_LOG(debug, "Path-separated prefix requirement '{}' matched.", prefix_); | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private: | ||
| // prefix string | ||
| const std::string prefix_; | ||
| const Matchers::PathMatcherConstSharedPtr path_matcher_; | ||
| }; | ||
| } // namespace | ||
|
|
||
| MatcherConstPtr Matcher::create(const RequirementRule& rule) { | ||
|
|
@@ -166,6 +192,8 @@ MatcherConstPtr Matcher::create(const RequirementRule& rule) { | |
| return std::make_unique<RegexMatcherImpl>(rule); | ||
| case RouteMatch::PathSpecifierCase::kConnectMatcher: | ||
| return std::make_unique<ConnectMatcherImpl>(rule); | ||
| case RouteMatch::PathSpecifierCase::kPathSeparatedPrefix: | ||
| return std::make_unique<PathSeparatedPrefixMatcherImpl>(rule); | ||
| case RouteMatch::PathSpecifierCase::PATH_SPECIFIER_NOT_SET: | ||
| break; // Fall through to PANIC. | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.