Skip to content
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.

Sign up for GitHub

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

rds: expose path match criterion via route entry #2531

Merged
merged 4 commits into from
Feb 6, 2018
Merged

rds: expose path match criterion via route entry #2531

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f210831
Expose path match criterion via route entry
mrice32 Feb 4, 2018
507cb1b
Merge branch 'master' into route_match_type
mrice32 Feb 5, 2018
acdf74c
Fixed compile error
mrice32 Feb 5, 2018
c272cf0
Corrected const-ness in test
mrice32 Feb 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions include/envoy/router/router.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,34 @@ class MetadataMatchCriteria {
metadataMatchCriteria() const PURE;
};

/**
* Type of path matching that a route entry uses.
*/
enum class PathMatchType {
None,
Prefix,
Exact,
Regex,
};

/**
* Criterion that a route entry uses for matching a particular path.
*/
class PathMatchCriterion {
public:
virtual ~PathMatchCriterion() {}

/**
* @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;
};

/**
* An individual resolved route entry.
*/
Expand Down Expand Up @@ -421,6 +449,11 @@ class RouteEntry : public ResponseEntry {
* this route.
*/
virtual const envoy::api::v2::core::Metadata& metadata() const PURE;

/**
* @return const PathMatchCriterion& the match criterion for this route.
*/
virtual const PathMatchCriterion& pathMatchCriterion() const PURE;
};

/**
Expand Down
2 changes: 2 additions & 0 deletions source/common/http/async_client_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ const AsyncStreamImpl::NullRateLimitPolicy AsyncStreamImpl::NullVirtualHost::rat
const AsyncStreamImpl::NullConfig AsyncStreamImpl::NullVirtualHost::route_configuration_;
const std::multimap<std::string, std::string> AsyncStreamImpl::RouteEntryImpl::opaque_config_;
const envoy::api::v2::core::Metadata AsyncStreamImpl::RouteEntryImpl::metadata_;
const AsyncStreamImpl::NullPathMatchCriterion
AsyncStreamImpl::RouteEntryImpl::path_match_criterion_;
const std::list<LowerCaseString> AsyncStreamImpl::NullConfig::internal_only_headers_;

AsyncClientImpl::AsyncClientImpl(const Upstream::ClusterInfo& cluster, Stats::Store& stats_store,
Expand Down
9 changes: 9 additions & 0 deletions source/common/http/async_client_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,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(const std::string& cluster_name,
const Optional<std::chrono::milliseconds>& timeout)
Expand Down Expand Up @@ -191,13 +196,17 @@ class AsyncStreamImpl : public AsyncClient::Stream,
bool useWebSocket() const override { return false; }
bool includeVirtualHostRateLimits() const override { return true; }
const envoy::api::v2::core::Metadata& metadata() const override { return metadata_; }
const Router::PathMatchCriterion& pathMatchCriterion() const override {
return path_match_criterion_;
}

static const NullRateLimitPolicy rate_limit_policy_;
static const NullRetryPolicy retry_policy_;
static const NullShadowPolicy shadow_policy_;
static const NullVirtualHost virtual_host_;
static const std::multimap<std::string, std::string> opaque_config_;
static const envoy::api::v2::core::Metadata metadata_;
static const NullPathMatchCriterion path_match_criterion_;

const std::string& cluster_name_;
Optional<std::chrono::milliseconds> timeout_;
Expand Down
3 changes: 2 additions & 1 deletion source/common/router/config_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,8 @@ RegexRouteEntryImpl::RegexRouteEntryImpl(const VirtualHostImpl& vhost,
const envoy::api::v2::route::Route& route,
Runtime::Loader& loader)
: RouteEntryImplBase(vhost, route, loader),
regex_(RegexUtil::parseRegex(route.match().regex().c_str())) {}
regex_(RegexUtil::parseRegex(route.match().regex().c_str())),
regex_str_(route.match().regex()) {}

void RegexRouteEntryImpl::finalizeRequestHeaders(
Http::HeaderMap& headers, const RequestInfo::RequestInfo& request_info) const {
Expand Down
18 changes: 18 additions & 0 deletions source/common/router/config_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ class RouteEntryImplBase : public RouteEntry,
public Matchable,
public DirectResponseEntry,
public Route,
public PathMatchCriterion,
public std::enable_shared_from_this<RouteEntryImplBase> {
public:
/**
Expand Down Expand Up @@ -335,6 +336,7 @@ class RouteEntryImplBase : public RouteEntry,
}
bool includeVirtualHostRateLimits() const override { return include_vh_rate_limits_; }
const envoy::api::v2::core::Metadata& metadata() const override { return metadata_; }
const PathMatchCriterion& pathMatchCriterion() const override { return *this; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if I've forgotten how to read C++ :) Can you explain why this is needed (or even how it passes type checking?).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so the inheritance structure is a little convoluted here. RouteEntryImplBase inherits from RouteEntry, Matchable, and now PathMatchCriterion (among others). It has three derived classes (PrefixRouteEntryImpl, PathRouteEntryImpl, and RegexRouteEntryImpl) that implement the Matchable and PathMatchCriterion abstract methods since they implement all of the match-type specific logic. So, for the pathMatchCriterion() method (part of the RouteEntry interface), it just returns itself as the PathMatchCriterion since it implements that interface. Does that help clarify things?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it ever need to be directly instantiated (i.e. be concrete)? Or do we only use derived classes? If the latter, it seems we don't need to define it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of the class was to group the idea of a route match type and a matcher since one is needed to interpret the other (like a virtualized POD struct) - as opposed to polluting the RouteEntry interface (which is already starting to feel a little big) with a routeMatchType() method and a routeMatchString() method or something. I'm happy to promote them to the RouteEntry interface if that seems like a net gain, though.


// Router::DirectResponseEntry
std::string newPath(const Http::HeaderMap& headers) const override;
Expand Down Expand Up @@ -409,6 +411,9 @@ class RouteEntryImplBase : public RouteEntry,
return parent_->includeVirtualHostRateLimits();
}
const envoy::api::v2::core::Metadata& metadata() const override { return parent_->metadata(); }
const PathMatchCriterion& pathMatchCriterion() const override {
return parent_->pathMatchCriterion();
}

// Router::Route
const DirectResponseEntry* directResponseEntry() const override { return nullptr; }
Expand Down Expand Up @@ -513,6 +518,10 @@ class PrefixRouteEntryImpl : public RouteEntryImplBase {
void finalizeRequestHeaders(Http::HeaderMap& headers,
const RequestInfo::RequestInfo& request_info) const override;

// Router::PathMatchCriterion
const std::string& matcher() const override { return prefix_; }
PathMatchType matchType() const override { return PathMatchType::Prefix; }

// Router::Matchable
RouteConstSharedPtr matches(const Http::HeaderMap& headers, uint64_t random_value) const override;

Expand All @@ -532,6 +541,10 @@ class PathRouteEntryImpl : public RouteEntryImplBase {
void finalizeRequestHeaders(Http::HeaderMap& headers,
const RequestInfo::RequestInfo& request_info) const override;

// Router::PathMatchCriterion
const std::string& matcher() const override { return path_; }
PathMatchType matchType() const override { return PathMatchType::Exact; }

// Router::Matchable
RouteConstSharedPtr matches(const Http::HeaderMap& headers, uint64_t random_value) const override;

Expand All @@ -551,11 +564,16 @@ class RegexRouteEntryImpl : public RouteEntryImplBase {
void finalizeRequestHeaders(Http::HeaderMap& headers,
const RequestInfo::RequestInfo& request_info) const override;

// Router::PathMatchCriterion
const std::string& matcher() const override { return regex_str_; }
PathMatchType matchType() const override { return PathMatchType::Regex; }

// Router::Matchable
RouteConstSharedPtr matches(const Http::HeaderMap& headers, uint64_t random_value) const override;

private:
const std::regex regex_;
const std::string regex_str_;
};

/**
Expand Down
6 changes: 5 additions & 1 deletion test/common/http/async_client_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,7 @@ TEST_F(AsyncClientImplTest, WatermarkCallbacks) {
EXPECT_CALL(stream_callbacks_, onReset());
}

TEST_F(AsyncClientImplTest, NullRouteConfigTest) {
TEST_F(AsyncClientImplTest, RdsGettersTest) {
TestHeaderMapImpl headers;
HttpTestUtility::addDefaultHeaders(headers);
AsyncClient::Stream* stream =
Expand All @@ -872,8 +872,12 @@ TEST_F(AsyncClientImplTest, NullRouteConfigTest) {
Http::StreamDecoderFilterCallbacks* filter_callbacks =
static_cast<Http::AsyncStreamImpl*>(stream);
auto route = filter_callbacks->route();
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());
Expand Down
22 changes: 21 additions & 1 deletion test/common/router/config_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3773,13 +3773,27 @@ name: foo
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);
auto route_entry = route->routeEntry();
ASSERT_NE(nullptr, route_entry);
auto& match_criterion = route_entry->pathMatchCriterion();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: const etc.

EXPECT_EQ(expected_matcher, match_criterion.matcher());
EXPECT_EQ(expected_type, match_criterion.matchType());
}

TEST(RouteConfigurationV2, RouteConfigGetters) {
std::string yaml = R"EOF(
name: foo
virtual_hosts:
- name: bar
domains: ["*"]
routes:
- match: { regex: "/rege[xy]" }
route: { cluster: ww2 }
- match: { path: "/exact-path" }
route: { cluster: ww2 }
- match: { prefix: "/"}
route: { cluster: www2 }
metadata: { filter_metadata: { com.bar.foo: { baz: test_value } } }
Expand All @@ -3789,8 +3803,14 @@ name: foo
NiceMock<Upstream::MockClusterManager> cm;
ConfigImpl config(parseRouteConfigurationFromV2Yaml(yaml), runtime, cm, true);

auto* route_entry = config.route(genHeaders("www.foo.com", "/", "GET"), 0)->routeEntry();
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);
auto route = config.route(genHeaders("www.foo.com", "/", "GET"), 0);
checkPathMatchCriterion(route.get(), "/", PathMatchType::Prefix);

auto route_entry = route->routeEntry();
const auto& metadata = route_entry->metadata();

EXPECT_EQ("test_value",
Expand Down
1 change: 1 addition & 0 deletions test/mocks/router/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ class MockRouteEntry : public RouteEntry {
MOCK_CONST_METHOD0(includeVirtualHostRateLimits, bool());
MOCK_CONST_METHOD0(corsPolicy, const CorsPolicy*());
MOCK_CONST_METHOD0(metadata, const envoy::api::v2::core::Metadata&());
MOCK_CONST_METHOD0(pathMatchCriterion, const PathMatchCriterion&());

std::string cluster_name_{"fake_cluster"};
std::multimap<std::string, std::string> opaque_config_;
Expand Down