Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
21 changes: 20 additions & 1 deletion include/envoy/router/router.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,20 @@ class RedirectEntry {
virtual Http::Code redirectResponseCode() const PURE;
};

/**
* A routing primitive that specifies a direct (non-proxied) HTTP response .

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.

response.

*/
class DirectResponseEntry {
public:
virtual ~DirectResponseEntry() {}

/**
* Returns the HTTP status code to return.
* @return Http::Code the response Code.
*/
virtual Http::Code responseCode() const PURE;
};

/**
* CorsPolicy for Route and VirtualHost.
*/
Expand Down Expand Up @@ -416,7 +430,7 @@ class Decorator {
typedef std::unique_ptr<const Decorator> DecoratorConstPtr;

/**
* An interface that holds a RedirectEntry or a RouteEntry for a request.

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.

I'd prefer this replace RedirectEntry rather than be a slightly duplicate thing.
newPath could be optional, and expected for redirect responses. I'm neutral if it should be done here or in a follow-up but maybe add a TODO?

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.

Thanks for the suggestion; combining RedirectEntry and DirectResponseEntry sounds good to me. The combined code will have to deal with some incompatibilities between the two -- e.g., under the current data plane API, redirects aren't allowed to send the response_headers_to_add, whereas direct responses are required to -- but it will probably still be cleaner than the current diff.

I'll merge the implementations post an update to this PR later today.

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.

cool. hopefully if we do some sanity checks with an IsRedirectReturnCode() in the right places we'll guard against bad configs/code

@mattklein123 mattklein123 Jan 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see any reason redirects cannot also populate response_headers_to_add. That seems pretty useful to me. Thoughts?

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.

Adding response_headers_to_add to redirects would change the behavior of existing configs. If we were designing the config schema from scratch, I’d advocate for sending the headers with redirects. But at this point it would impact backward compatibility. Unless we added a “send_headers_with_redirects” flag that defaults to false, I suppose :(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, fair enough. I kind of think we could "just do it" and document it in this case as a change. But that's me. Fine to block/TODO for now if you think that would be best.

* An interface that holds a RedirectEntry, DirectResponseEntry, or RouteEntry for a request.
*/
class Route {
public:
Expand All @@ -427,6 +441,11 @@ class Route {
*/
virtual const RedirectEntry* redirectEntry() const PURE;

/**
* @return the direct response entry or nullptr if there is no direct response for the request.

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.

@return DirectResponseEntry*

*/
virtual const DirectResponseEntry* directResponseEntry() const PURE;

/**
* @return the route entry or nullptr if there is no matching route for the request.
*/
Expand Down
17 changes: 12 additions & 5 deletions source/common/config/rds_json.cc
Original file line number Diff line number Diff line change
Expand Up @@ -236,15 +236,22 @@ void RdsJson::translateRoute(const Json::Object& json_route, envoy::api::v2::Rou
throw EnvoyException("Redirect route entries must not have WebSockets set");
}
}
bool has_direct_response = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All the v1 code can be dropped, right?

if (json_route.hasObject("status") || json_route.hasObject("body")) {
has_direct_response = true;
auto* direct_response = route.mutable_direct_response();
direct_response->set_status(json_route.getInteger("status"));
}
const bool has_cluster = json_route.hasObject("cluster") ||
json_route.hasObject("cluster_header") ||
json_route.hasObject("weighted_clusters");

if (has_cluster && has_redirect) {
throw EnvoyException("routes must be either redirects or cluster targets");
} else if (!has_cluster && !has_redirect) {
throw EnvoyException(
"routes must have redirect or one of cluster/cluster_header/weighted_clusters");
if ((has_cluster && (has_redirect || has_direct_response)) ||
(has_redirect && has_direct_response)) {
throw EnvoyException("routes must be either redirects, direct responses, or cluster targets");
} else if (!has_cluster && !has_redirect && !has_direct_response) {
throw EnvoyException("routes must have redirect, direct response, or one of "
"cluster/cluster_header/weighted_clusters");
} else if (has_cluster) {
auto* action = route.mutable_route();

Expand Down
1 change: 1 addition & 0 deletions source/common/http/async_client_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ class AsyncStreamImpl : public AsyncClient::Stream,

// Router::Route
const Router::RedirectEntry* redirectEntry() const override { return nullptr; }
const Router::DirectResponseEntry* directResponseEntry() const override { return nullptr; }
const Router::RouteEntry* routeEntry() const override { return &route_entry_; }
const Router::Decorator* decorator() const override { return nullptr; }

Expand Down
1 change: 1 addition & 0 deletions source/common/json/config_schemas.cc
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,7 @@ const std::string Json::Schema::ROUTE_ENTRY_CONFIGURATION_SCHEMA(R"EOF(
"weighted_clusters": {"$ref" : "#/definitions/weighted_clusters"},
"host_redirect" : {"type" : "string"},
"path_redirect" : {"type" : "string"},
"status": {"type": "integer"},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

del?

"prefix_rewrite" : {"type" : "string"},
"host_rewrite" : {"type" : "string"},
"auto_host_rewrite" : {"type" : "boolean"},
Expand Down
25 changes: 19 additions & 6 deletions source/common/router/config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,8 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost,
route.route().response_headers_to_remove())),
opaque_config_(parseOpaqueConfig(route)), decorator_(parseDecorator(route)),
redirect_response_code_(
ConfigUtility::parseRedirectResponseCode(route.redirect().response_code())) {
ConfigUtility::parseRedirectResponseCode(route.redirect().response_code())),
direct_response_code_(static_cast<Http::Code>(route.direct_response().status())) {
if (route.route().has_metadata_match()) {
const auto filter_it = route.route().metadata_match().filter_metadata().find(
Envoy::Config::MetadataFilters::get().ENVOY_LB);
Expand Down Expand Up @@ -426,17 +427,29 @@ DecoratorConstPtr RouteEntryImplBase::parseDecorator(const envoy::api::v2::Route
}

const RedirectEntry* RouteEntryImplBase::redirectEntry() const {
// A route for a request can exclusively be a route entry or a redirect entry.
// A route for a request can exclusively be a route entry, a direct response entry,
// or a redirect entry.
if (isRedirect()) {
return this;
} else {
return nullptr;
}
}

const DirectResponseEntry* RouteEntryImplBase::directResponseEntry() const {
// A route for a request can exclusively be a route entry, a direct response entry,
// or a redirect entry.
if (isDirectResponse()) {

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.

Yeah, I think this is confusing, since a redirect entry is a direct response too

return this;
} else {
return nullptr;
}
}

const RouteEntry* RouteEntryImplBase::routeEntry() const {
// A route for a request can exclusively be a route entry or a redirect entry.
if (isRedirect()) {
// A route for a request can exclusively be a route entry, a direct response entry,
// or a redirect entry.
if (isRedirect() || isDirectResponse()) {
return nullptr;
} else {
return this;
Expand All @@ -448,7 +461,7 @@ RouteConstSharedPtr RouteEntryImplBase::clusterEntry(const Http::HeaderMap& head
// Gets the route object chosen from the list of weighted clusters
// (if there is one) or returns self.
if (weighted_clusters_.empty()) {
if (!cluster_name_.empty() || isRedirect()) {
if (!cluster_name_.empty() || isRedirect() || isDirectResponse()) {
return shared_from_this();
} else {
ASSERT(!cluster_header_name_.get().empty());
Expand Down Expand Up @@ -489,7 +502,7 @@ RouteConstSharedPtr RouteEntryImplBase::clusterEntry(const Http::HeaderMap& head
}

void RouteEntryImplBase::validateClusters(Upstream::ClusterManager& cm) const {
if (isRedirect()) {
if (isRedirect() || isDirectResponse()) {
return;
}

Expand Down
9 changes: 9 additions & 0 deletions source/common/router/config_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class SslRedirectRoute : public Route {
public:
// Router::Route
const RedirectEntry* redirectEntry() const override { return &SSL_REDIRECTOR; }
const DirectResponseEntry* directResponseEntry() const override { return nullptr; }
const RouteEntry* routeEntry() const override { return nullptr; }
const Decorator* decorator() const override { return nullptr; }

Expand Down Expand Up @@ -287,13 +288,15 @@ class DecoratorImpl : public Decorator {
class RouteEntryImplBase : public RouteEntry,
public Matchable,
public RedirectEntry,
public DirectResponseEntry,
public Route,
public std::enable_shared_from_this<RouteEntryImplBase> {
public:
RouteEntryImplBase(const VirtualHostImpl& vhost, const envoy::api::v2::Route& route,
Runtime::Loader& loader);

bool isRedirect() const { return !host_redirect_.empty() || !path_redirect_.empty(); }
bool isDirectResponse() const { return static_cast<int>(direct_response_code_) != 0; }

bool matchRoute(const Http::HeaderMap& headers, uint64_t random_value) const;
void validateClusters(Upstream::ClusterManager& cm) const;
Expand Down Expand Up @@ -333,8 +336,12 @@ class RouteEntryImplBase : public RouteEntry,
std::string newPath(const Http::HeaderMap& headers) const override;
Http::Code redirectResponseCode() const override { return redirect_response_code_; }

// Router::DirectResponseEntry
Http::Code responseCode() const override { return direct_response_code_; }

// Router::Route
const RedirectEntry* redirectEntry() const override;
const DirectResponseEntry* directResponseEntry() const override;
const RouteEntry* routeEntry() const override;
const Decorator* decorator() const override { return decorator_.get(); }

Expand Down Expand Up @@ -403,6 +410,7 @@ class RouteEntryImplBase : public RouteEntry,

// Router::Route
const RedirectEntry* redirectEntry() const override { return nullptr; }
const DirectResponseEntry* directResponseEntry() const override { return nullptr; }
const RouteEntry* routeEntry() const override { return this; }
const Decorator* decorator() const override { return nullptr; }

Expand Down Expand Up @@ -488,6 +496,7 @@ class RouteEntryImplBase : public RouteEntry,

const DecoratorConstPtr decorator_;
const Http::Code redirect_response_code_;
const Http::Code direct_response_code_;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: I might consider making this Optional<Http::Code>. I think it would be slightly clearer in all the places you set and consume it.

};

/**
Expand Down
15 changes: 15 additions & 0 deletions source/common/router/router.cc
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,21 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e
return Http::FilterHeadersStatus::StopIteration;
}

// Determine if there is a direct response for the request.
if (route_->directResponseEntry()) {
Http::Utility::sendLocalReply(

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.

Take a look at callers of sendLocalReply - I think you can simplify the arguments a bit (and if I'm incorrect please educate me! :-P)

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 see, this method is in the same class that provides a friendly wrapper around sendLocalReply. I'll switch to that.

[this](Http::HeaderMapPtr&& headers, bool end_stream) -> void {
// TODO(brian-pane) add response_headers_to_add to the headers here
callbacks_->encodeHeaders(std::move(headers), end_stream);
},
[this](Buffer::Instance& data, bool end_stream) -> void {
callbacks_->encodeData(data, end_stream);
},
stream_destroyed_, route_->directResponseEntry()->responseCode(), "");
// TODO(brian-pane) support sending a response body.
return Http::FilterHeadersStatus::StopIteration;
}

// A route entry matches for the request.
route_entry_ = route_->routeEntry();
Upstream::ThreadLocalCluster* cluster = config_.cm_.get(route_entry_->clusterName());
Expand Down
4 changes: 4 additions & 0 deletions source/common/ssl/context_config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ ServerContextConfigImpl::ServerContextConfigImpl(const envoy::api::v2::Downstrea
validateAndAppendKey(ret, datasource.inline_bytes());
break;
}
case envoy::api::v2::DataSource::kInlineString: {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we remove from this PR?

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.

I’ll move this to a separate PR

validateAndAppendKey(ret, datasource.inline_string());
break;
}
default:
throw EnvoyException(fmt::format("Unexpected DataSource::specifier_case(): {}",
datasource.specifier_case()));
Expand Down
42 changes: 39 additions & 3 deletions test/common/router/config_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2113,6 +2113,41 @@ TEST(RouteMatcherTest, Redirect) {
}
}

TEST(RouteMatcherTest, DirectResponse) {
std::string json = R"EOF(
{
"virtual_hosts": [
{
"name": "www",
"domains": ["www.example.com"],
"routes": [
{
"prefix": "/direct",
"status": 200
},
{
"prefix": "/",
"cluster": "www"
}
]
}
]
}
)EOF";

NiceMock<Runtime::MockLoader> runtime;
NiceMock<Upstream::MockClusterManager> cm;
ConfigImpl config(parseRouteConfigurationFromJson(json), runtime, cm, true);

EXPECT_EQ("www", config.route(genHeaders("www.example.com", "/example", "GET"), 0)
->routeEntry()
->clusterName());
EXPECT_EQ(static_cast<Http::Code>(200),
config.route(genHeaders("www.example.com", "/direct", "GET"), 0)
->directResponseEntry()
->responseCode());
}

TEST(RouteMatcherTest, ExclusiveRouteEntryOrRedirectEntry) {
std::string json = R"EOF(
{
Expand Down Expand Up @@ -2726,9 +2761,10 @@ TEST(BadHttpRouteConfigurationsTest, BadRouteEntryConfigNoRedirectNoClusters) {
NiceMock<Runtime::MockLoader> runtime;
NiceMock<Upstream::MockClusterManager> cm;

EXPECT_THROW_WITH_MESSAGE(
ConfigImpl(parseRouteConfigurationFromJson(json), runtime, cm, true), EnvoyException,
"routes must have redirect or one of cluster/cluster_header/weighted_clusters")
EXPECT_THROW_WITH_MESSAGE(ConfigImpl(parseRouteConfigurationFromJson(json), runtime, cm, true),
EnvoyException,
"routes must have redirect, direct response, or "
"one of cluster/cluster_header/weighted_clusters")
}

TEST(RouteMatcherTest, TestOpaqueConfig) {
Expand Down
13 changes: 13 additions & 0 deletions test/common/router/router_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1476,6 +1476,19 @@ TEST_F(RouterTest, RedirectFound) {
EXPECT_TRUE(verifyHostUpstreamStats(0, 0));
}

TEST_F(RouterTest, DirectResponse) {
MockDirectResponseEntry direct_response;
EXPECT_CALL(direct_response, responseCode()).WillOnce(Return(Http::Code::OK));
EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response));

Http::TestHeaderMapImpl response_headers{{":status", "200"}};
EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true));
Http::TestHeaderMapImpl headers;
HttpTestUtility::addDefaultHeaders(headers);
router_.decodeHeaders(headers, true);
EXPECT_TRUE(verifyHostUpstreamStats(0, 0));
}

TEST(RouterFilterUtilityTest, finalTimeout) {
{
NiceMock<MockRouteEntry> route;
Expand Down
24 changes: 21 additions & 3 deletions test/common/ssl/context_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -248,23 +248,41 @@ TEST_F(SslServerContextImplTicketTest, TicketKeyNone) {
EXPECT_NO_THROW(loadConfigV2(cfg));
}

TEST_F(SslServerContextImplTicketTest, TicketKeyInlineSuccess) {
TEST_F(SslServerContextImplTicketTest, TicketKeyInlineBytesSuccess) {
envoy::api::v2::DownstreamTlsContext cfg;
cfg.mutable_session_ticket_keys()->add_keys()->set_inline_bytes(std::string(80, '\0'));
EXPECT_NO_THROW(loadConfigV2(cfg));
}

TEST_F(SslServerContextImplTicketTest, TicketKeyInlineFailTooBig) {
TEST_F(SslServerContextImplTicketTest, TicketKeyInlineStringSuccess) {
envoy::api::v2::DownstreamTlsContext cfg;
cfg.mutable_session_ticket_keys()->add_keys()->set_inline_string(std::string(80, '\0'));
EXPECT_NO_THROW(loadConfigV2(cfg));
}

TEST_F(SslServerContextImplTicketTest, TicketKeyInlineBytesFailTooBig) {
envoy::api::v2::DownstreamTlsContext cfg;
cfg.mutable_session_ticket_keys()->add_keys()->set_inline_bytes(std::string(81, '\0'));
EXPECT_THROW(loadConfigV2(cfg), EnvoyException);
}

TEST_F(SslServerContextImplTicketTest, TicketKeyInlineFailTooSmall) {
TEST_F(SslServerContextImplTicketTest, TicketKeyInlineStringFailTooBig) {
envoy::api::v2::DownstreamTlsContext cfg;
cfg.mutable_session_ticket_keys()->add_keys()->set_inline_string(std::string(81, '\0'));
EXPECT_THROW(loadConfigV2(cfg), EnvoyException);
}

TEST_F(SslServerContextImplTicketTest, TicketKeyInlineBytesFailTooSmall) {
envoy::api::v2::DownstreamTlsContext cfg;
cfg.mutable_session_ticket_keys()->add_keys()->set_inline_bytes(std::string(79, '\0'));
EXPECT_THROW(loadConfigV2(cfg), EnvoyException);
}

TEST_F(SslServerContextImplTicketTest, TicketKeyInlineStringFailTooSmall) {
envoy::api::v2::DownstreamTlsContext cfg;
cfg.mutable_session_ticket_keys()->add_keys()->set_inline_string(std::string(79, '\0'));
EXPECT_THROW(loadConfigV2(cfg), EnvoyException);
}

} // namespace Ssl
} // namespace Envoy
17 changes: 17 additions & 0 deletions test/config/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,23 @@ void ConfigHelper::addRoute(const std::string& domains, const std::string& prefi
storeHttpConnectionManager(hcm_config);
}

void ConfigHelper::addDirectResponse(const std::string& domains, const std::string& prefix,
Http::Code status, const std::string&) {
RELEASE_ASSERT(!finalized_);
envoy::api::v2::filter::network::HttpConnectionManager hcm_config;
loadHttpConnectionManager(hcm_config);

auto* route_config = hcm_config.mutable_route_config();
auto* virtual_host = route_config->add_virtual_hosts();
virtual_host->set_name(domains);
virtual_host->add_domains(domains);
virtual_host->add_routes()->mutable_match()->set_prefix(prefix);
virtual_host->mutable_routes(0)->mutable_direct_response()->set_status(
static_cast<uint32_t>(status));

storeHttpConnectionManager(hcm_config);
}

void ConfigHelper::addFilter(const std::string& config) {
RELEASE_ASSERT(!finalized_);
envoy::api::v2::filter::network::HttpConnectionManager hcm_config;
Expand Down
6 changes: 6 additions & 0 deletions test/config/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
#include <string>
#include <vector>

#include "envoy/http/codes.h"

#include "common/network/address_impl.h"

#include "api/base.pb.h"
Expand Down Expand Up @@ -67,6 +69,10 @@ class ConfigHelper {
bool validate_clusters, envoy::api::v2::RouteAction::ClusterNotFoundResponseCode code,
envoy::api::v2::VirtualHost::TlsRequirementType type = envoy::api::v2::VirtualHost::NONE);

// Add an additional route with a direct (non-proxied) response.
void addDirectResponse(const std::string& domains, const std::string& prefix,
Http::Code responseCode, const std::string& body);

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.

Do you think this will be used widely? If not I'd just suggest doing this as with void addConfigModifier(HttpModifierFunction function); in testRouterDirectResponse


// Add an HTTP filter prior to existing filters.
void addFilter(const std::string& filter_yaml);

Expand Down
Loading