Skip to content

test: refactoring how we add custom routes #6382

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

Merged
alyssawilk merged 2 commits into from
Mar 27, 2019
Merged

test: refactoring how we add custom routes #6382

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b6b949
test: refactoring how we add custom routes
alyssawilk Feb 26, 2019
93a193d
reviewer comments
alyssawilk Mar 26, 2019
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
38 changes: 14 additions & 24 deletions test/config/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,34 +337,24 @@ void ConfigHelper::setConnectTimeout(std::chrono::milliseconds timeout) {
connect_timeout_set_ = true;
}

void ConfigHelper::addRoute(
const std::string& domains, const std::string& prefix, const std::string& cluster,
bool validate_clusters, envoy::api::v2::route::RouteAction::ClusterNotFoundResponseCode code,
envoy::api::v2::route::VirtualHost::TlsRequirementType type,
envoy::api::v2::route::RetryPolicy retry_policy, bool include_attempt_count_header,
const absl::string_view upgrade,
envoy::api::v2::route::RouteAction::InternalRedirectAction internal_redirect_action) {
envoy::api::v2::route::VirtualHost ConfigHelper::createHost(const char* domain, const char* prefix,
const char* cluster) {
envoy::api::v2::route::VirtualHost virtual_host;
virtual_host.set_name(domain);
virtual_host.add_domains(domain);
virtual_host.add_routes()->mutable_match()->set_prefix(prefix);
auto* route = virtual_host.mutable_routes(0)->mutable_route();
route->set_cluster(cluster);
return virtual_host;
}

void ConfigHelper::addVirtualHost(envoy::api::v2::route::VirtualHost& vhost) {
RELEASE_ASSERT(!finalized_, "");
envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager hcm_config;
loadHttpConnectionManager(hcm_config);

auto* route_config = hcm_config.mutable_route_config();
route_config->mutable_validate_clusters()->set_value(validate_clusters);
auto route_config = hcm_config.mutable_route_config();
auto* virtual_host = route_config->add_virtual_hosts();
virtual_host->set_name(domains);
virtual_host->set_include_request_attempt_count(include_attempt_count_header);
virtual_host->add_domains(domains);
virtual_host->add_routes()->mutable_match()->set_prefix(prefix);
auto* route = virtual_host->mutable_routes(0)->mutable_route();
route->set_cluster(cluster);
route->set_cluster_not_found_response_code(code);
route->mutable_retry_policy()->Swap(&retry_policy);
if (!upgrade.empty()) {
route->add_upgrade_configs()->set_upgrade_type(std::string(upgrade));
}
route->set_internal_redirect_action(internal_redirect_action);
virtual_host->set_require_tls(type);

virtual_host->CopyFrom(vhost);
storeHttpConnectionManager(hcm_config);
}

Expand Down
15 changes: 4 additions & 11 deletions test/config/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,17 +104,10 @@ class ConfigHelper {
// Set the connect timeout on upstream connections.
void setConnectTimeout(std::chrono::milliseconds timeout);

// TODO(alyssawilk) this does not scale. Refactor.
// Add an additional route to the configuration.
void addRoute(const std::string& host, const std::string& route, const std::string& cluster,
bool validate_clusters,
envoy::api::v2::route::RouteAction::ClusterNotFoundResponseCode code,
envoy::api::v2::route::VirtualHost::TlsRequirementType type =
envoy::api::v2::route::VirtualHost::NONE,
envoy::api::v2::route::RetryPolicy retry_policy = {},
bool include_attempt_count_header = false, const absl::string_view upgrade = "",
const envoy::api::v2::route::RouteAction::InternalRedirectAction internal_action =
envoy::api::v2::route::RouteAction::PASS_THROUGH_INTERNAL_REDIRECT);
envoy::api::v2::route::VirtualHost createHost(const char* host, const char* route = "/",

@snowp snowp Mar 26, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: maybe call this createVHost or createVirtualHost?

const char* cluster = "cluster_0");

void addVirtualHost(envoy::api::v2::route::VirtualHost& vhost);

@snowp snowp Mar 26, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

any reason we're not passing this by const ref?

@alyssawilk alyssawilk Mar 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, I'd waffled about using Swap instead of CopyFrom and then forgot so got worst of both :-P
Good catch!


// Add an HTTP filter prior to existing filters.
void addFilter(const std::string& filter_yaml);
Expand Down
7 changes: 4 additions & 3 deletions test/integration/http_integration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,9 +579,10 @@ void HttpIntegrationTest::testRetry() {
// Tests that the x-envoy-attempt-count header is properly set on the upstream request
// and updated after the request is retried.
void HttpIntegrationTest::testRetryAttemptCountHeader() {
config_helper_.addRoute("host", "/test_retry", "cluster_0", false,
envoy::api::v2::route::RouteAction::NOT_FOUND,
envoy::api::v2::route::VirtualHost::NONE, {}, true);
auto host = config_helper_.createHost("host", "/test_retry");
host.set_include_request_attempt_count(true);
config_helper_.addVirtualHost(host);

initialize();
codec_client_ = makeHttpConnection(lookupPort("http"));
auto response =
Expand Down
18 changes: 9 additions & 9 deletions test/integration/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,9 +399,9 @@ TEST_P(IntegrationTest, BadPath) {
TEST_P(IntegrationTest, AbsolutePath) {
// Configure www.redirect.com to send a redirect, and ensure the redirect is
// encountered via absolute URL.
config_helper_.addRoute("www.redirect.com", "/", "cluster_0", true,
envoy::api::v2::route::RouteAction::SERVICE_UNAVAILABLE,
envoy::api::v2::route::VirtualHost::ALL);
auto host = config_helper_.createHost("www.redirect.com", "/");
host.set_require_tls(envoy::api::v2::route::VirtualHost::ALL);
config_helper_.addVirtualHost(host);
config_helper_.addConfigModifier(&setAllowAbsoluteUrl);

initialize();
Expand All @@ -415,9 +415,9 @@ TEST_P(IntegrationTest, AbsolutePath) {
TEST_P(IntegrationTest, AbsolutePathWithPort) {
// Configure www.namewithport.com:1234 to send a redirect, and ensure the redirect is
// encountered via absolute URL with a port.
config_helper_.addRoute("www.namewithport.com:1234", "/", "cluster_0", true,
envoy::api::v2::route::RouteAction::SERVICE_UNAVAILABLE,
envoy::api::v2::route::VirtualHost::ALL);
auto host = config_helper_.createHost("www.namewithport.com:1234", "/");
host.set_require_tls(envoy::api::v2::route::VirtualHost::ALL);
config_helper_.addVirtualHost(host);
config_helper_.addConfigModifier(&setAllowAbsoluteUrl);
initialize();
std::string response;
Expand All @@ -432,9 +432,9 @@ TEST_P(IntegrationTest, AbsolutePathWithoutPort) {
config_helper_.setDefaultHostAndRoute("foo.com", "/found");
// Set a matcher for www.namewithport.com:1234 and verify http://www.namewithport.com does not
// match
config_helper_.addRoute("www.namewithport.com:1234", "/", "cluster_0", true,
envoy::api::v2::route::RouteAction::SERVICE_UNAVAILABLE,
envoy::api::v2::route::VirtualHost::ALL);
auto host = config_helper_.createHost("www.namewithport.com:1234", "/");
host.set_require_tls(envoy::api::v2::route::VirtualHost::ALL);
config_helper_.addVirtualHost(host);
config_helper_.addConfigModifier(&setAllowAbsoluteUrl);
initialize();
std::string response;
Expand Down
56 changes: 32 additions & 24 deletions test/integration/protocol_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ using testing::Not;

namespace Envoy {

void setDoNotValidateRouteConfig(
envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) {
auto* route_config = hcm.mutable_route_config();
route_config->mutable_validate_clusters()->set_value(false);
};

// Tests for DownstreamProtocolIntegrationTest will be run with all protocols
// (H1/H2 downstream) but only H1 upstreams.
//
Expand Down Expand Up @@ -71,9 +77,11 @@ TEST_P(DownstreamProtocolIntegrationTest, RouterNotFoundBodyNoBuffer) {

// Add a route that uses unknown cluster (expect 404 Not Found).
TEST_P(DownstreamProtocolIntegrationTest, RouterClusterNotFound404) {
config_helper_.addRoute("foo.com", "/unknown", "unknown_cluster", false,
envoy::api::v2::route::RouteAction::NOT_FOUND,
envoy::api::v2::route::VirtualHost::NONE);
config_helper_.addConfigModifier(&setDoNotValidateRouteConfig);
auto host = config_helper_.createHost("foo.com", "/unknown", "unknown_cluster");
host.mutable_routes(0)->mutable_route()->set_cluster_not_found_response_code(
envoy::api::v2::route::RouteAction::NOT_FOUND);
config_helper_.addVirtualHost(host);
initialize();

BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest(
Expand All @@ -84,9 +92,11 @@ TEST_P(DownstreamProtocolIntegrationTest, RouterClusterNotFound404) {

// Add a route that uses unknown cluster (expect 503 Service Unavailable).
TEST_P(DownstreamProtocolIntegrationTest, RouterClusterNotFound503) {
config_helper_.addRoute("foo.com", "/unknown", "unknown_cluster", false,
envoy::api::v2::route::RouteAction::SERVICE_UNAVAILABLE,
envoy::api::v2::route::VirtualHost::NONE);
config_helper_.addConfigModifier(&setDoNotValidateRouteConfig);
auto host = config_helper_.createHost("foo.com", "/unknown", "unknown_cluster");
host.mutable_routes(0)->mutable_route()->set_cluster_not_found_response_code(
envoy::api::v2::route::RouteAction::SERVICE_UNAVAILABLE);
config_helper_.addVirtualHost(host);
initialize();

BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest(
Expand All @@ -97,9 +107,9 @@ TEST_P(DownstreamProtocolIntegrationTest, RouterClusterNotFound503) {

// Add a route which redirects HTTP to HTTPS, and verify Envoy sends a 301
TEST_P(ProtocolIntegrationTest, RouterRedirect) {
config_helper_.addRoute("www.redirect.com", "/", "cluster_0", true,
envoy::api::v2::route::RouteAction::SERVICE_UNAVAILABLE,
envoy::api::v2::route::VirtualHost::ALL);
auto host = config_helper_.createHost("www.redirect.com", "/");
host.set_require_tls(envoy::api::v2::route::VirtualHost::ALL);
config_helper_.addVirtualHost(host);
initialize();

BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest(
Expand Down Expand Up @@ -230,9 +240,9 @@ TEST_P(ProtocolIntegrationTest, Retry) {
// Tests that the x-envoy-attempt-count header is properly set on the upstream request
// and updated after the request is retried.
TEST_P(DownstreamProtocolIntegrationTest, RetryAttemptCountHeader) {
config_helper_.addRoute("host", "/test_retry", "cluster_0", false,
envoy::api::v2::route::RouteAction::NOT_FOUND,
envoy::api::v2::route::VirtualHost::NONE, {}, true);
auto host = config_helper_.createHost("host", "/test_retry");
host.set_include_request_attempt_count(true);
config_helper_.addVirtualHost(host);
initialize();
codec_client_ = makeHttpConnection(lookupPort("http"));
auto response =
Expand Down Expand Up @@ -280,13 +290,12 @@ TEST_P(DownstreamProtocolIntegrationTest, RetryPriority) {

Registry::InjectFactory<Upstream::RetryPriorityFactory> inject_factory(factory);

envoy::api::v2::route::RetryPolicy retry_policy;
retry_policy.mutable_retry_priority()->set_name(factory.name());

// Add route with custom retry policy
config_helper_.addRoute("host", "/test_retry", "cluster_0", false,
envoy::api::v2::route::RouteAction::NOT_FOUND,
envoy::api::v2::route::VirtualHost::NONE, retry_policy);
auto host = config_helper_.createHost("host", "/test_retry");
host.set_include_request_attempt_count(true);
auto retry_policy = host.mutable_routes(0)->mutable_route()->mutable_retry_policy();
retry_policy->mutable_retry_priority()->set_name(factory.name());
config_helper_.addVirtualHost(host);

// Use load assignments instead of static hosts. Necessary in order to use priorities.
config_helper_.addConfigModifier([](envoy::config::bootstrap::v2::Bootstrap& bootstrap) {
Expand Down Expand Up @@ -356,13 +365,12 @@ TEST_P(DownstreamProtocolIntegrationTest, RetryHostPredicateFilter) {
TestHostPredicateFactory predicate_factory;
Registry::InjectFactory<Upstream::RetryHostPredicateFactory> inject_factory(predicate_factory);

envoy::api::v2::route::RetryPolicy retry_policy;
retry_policy.add_retry_host_predicate()->set_name(predicate_factory.name());

// Add route with custom retry policy
config_helper_.addRoute("host", "/test_retry", "cluster_0", false,
envoy::api::v2::route::RouteAction::NOT_FOUND,
envoy::api::v2::route::VirtualHost::NONE, retry_policy);
auto host = config_helper_.createHost("host", "/test_retry");
host.set_include_request_attempt_count(true);
auto retry_policy = host.mutable_routes(0)->mutable_route()->mutable_retry_policy();
retry_policy->add_retry_host_predicate()->set_name(predicate_factory.name());
config_helper_.addVirtualHost(host);

// We want to work with a cluster with two hosts.
config_helper_.addConfigModifier([](envoy::config::bootstrap::v2::Bootstrap& bootstrap) {
Expand Down
16 changes: 7 additions & 9 deletions test/integration/redirect_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,13 @@ class RedirectIntegrationTest : public HttpProtocolIntegrationTest {
void initialize() override {
envoy::api::v2::route::RetryPolicy retry_policy;

config_helper_.addRoute("pass.through.internal.redirect", "/", "cluster_0", false,
envoy::api::v2::route::RouteAction::NOT_FOUND,
envoy::api::v2::route::VirtualHost::NONE, retry_policy, false, "",
envoy::api::v2::route::RouteAction::PASS_THROUGH_INTERNAL_REDIRECT);

config_helper_.addRoute("handle.internal.redirect", "/", "cluster_0", false,
envoy::api::v2::route::RouteAction::NOT_FOUND,
envoy::api::v2::route::VirtualHost::NONE, retry_policy, false, "",
envoy::api::v2::route::RouteAction::HANDLE_INTERNAL_REDIRECT);
auto pass_through = config_helper_.createHost("pass.through.internal.redirect");
config_helper_.addVirtualHost(pass_through);

auto handle = config_helper_.createHost("handle.internal.redirect");
handle.mutable_routes(0)->mutable_route()->set_internal_redirect_action(
envoy::api::v2::route::RouteAction::HANDLE_INTERNAL_REDIRECT);
config_helper_.addVirtualHost(handle);

HttpProtocolIntegrationTest::initialize();
}
Expand Down
6 changes: 3 additions & 3 deletions test/integration/websocket_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,9 +319,9 @@ TEST_P(WebsocketIntegrationTest, RouteSpecificUpgrade) {
foo_upgrade->set_upgrade_type("foo");
foo_upgrade->mutable_enabled()->set_value(false);
});
config_helper_.addRoute("host", "/websocket/test", "cluster_0", false,
envoy::api::v2::route::RouteAction::NOT_FOUND,
envoy::api::v2::route::VirtualHost::NONE, {}, false, "foo");
auto host = config_helper_.createHost("host", "/websocket/test");
host.mutable_routes(0)->mutable_route()->add_upgrade_configs()->set_upgrade_type("foo");
config_helper_.addVirtualHost(host);
initialize();

performUpgrade(upgradeRequestHeaders("foo", 0), upgradeResponseHeaders("foo"));
Expand Down