Skip to content

router: add support for direct (non-proxied) http responses. #2335

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
mattklein123 merged 8 commits into from
Jan 16, 2018
Merged

router: add support for direct (non-proxied) http responses. #2335

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
942d251
Add support for direct (non-proxied) HTTP responses.
Jan 9, 2018
eec6775
Fix formatting
Jan 10, 2018
3f04d58
Merge the redirect and direct-response implementations
Jan 10, 2018
dfc0b8d
Fix edit error that caused a merge conflict
Jan 11, 2018
ea91a05
Cleanups: remove v1 API support and addDirectResponse function
Jan 11, 2018
77a170f
Simplify the test code and add instrumentation.
Jan 11, 2018
49b2732
Clean up JSON schema, use Optional<Http::Code>, and add counter test.
Jan 12, 2018
e309720
Add YAML test
Jan 16, 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
29 changes: 15 additions & 14 deletions include/envoy/router/router.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,24 +23,25 @@ namespace Envoy {
namespace Router {

/**
* A routing primitive that creates a redirect path.
* A routing primitive that specifies a direct (non-proxied) HTTP response.
*/
class RedirectEntry {
class DirectResponseEntry {
public:
virtual ~RedirectEntry() {}
virtual ~DirectResponseEntry() {}

/**
* Returns the redirect path based on the request headers.
* @param headers supplies the request headers.
* @return std::string the redirect URL.
* Returns the HTTP status code to return.
* @return Http::Code the response Code.
*/
virtual std::string newPath(const Http::HeaderMap& headers) const PURE;
virtual Http::Code responseCode() const PURE;

/**
* Returns the HTTP status code to use when redirecting a request.
* @return Http::Code the redirect response Code.
* Returns the redirect path based on the request headers.
* @param headers supplies the request headers.
* @return std::string the redirect URL if this DirectResponseEntry is a redirect,
* or an empty string otherwise.
*/
virtual Http::Code redirectResponseCode() const PURE;
virtual std::string newPath(const Http::HeaderMap& headers) const PURE;
};

/**
Expand Down Expand Up @@ -416,16 +417,16 @@ class Decorator {
typedef std::unique_ptr<const Decorator> DecoratorConstPtr;

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

@alyssawilk alyssawilk Jan 10, 2018

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?

@brian-pane brian-pane Jan 10, 2018

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.

@alyssawilk alyssawilk Jan 10, 2018

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 •
edited
Loading

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?

@brian-pane brian-pane Jan 10, 2018

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 :(

@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.

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 DirectResponseEntry or RouteEntry for a request.
*/
class Route {
public:
virtual ~Route() {}

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

@alyssawilk alyssawilk Jan 10, 2018

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 RedirectEntry* redirectEntry() const PURE;
virtual const DirectResponseEntry* directResponseEntry() const PURE;

/**
* @return the route entry or nullptr if there is no matching route for the request.
Expand All @@ -449,7 +450,7 @@ class Config {

/**
* Based on the incoming HTTP request headers, determine the target route (containing either a
* route entry or a redirect entry) for the request.
* route entry or a direct response entry) for the request.
* @param headers supplies the request headers.
* @param random_value supplies the random seed to use if a runtime choice is required. This
* allows stable choices between calls if desired.
Expand Down
17 changes: 12 additions & 5 deletions source/common/config/rds_json.cc
View file
Open in desktop
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;

@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.

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
2 changes: 1 addition & 1 deletion source/common/http/async_client_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ class AsyncStreamImpl : public AsyncClient::Stream,
: route_entry_(cluster_name, timeout) {}

// 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
View file
Open in desktop
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"},

@mattklein123 mattklein123 Jan 12, 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.

del?

"prefix_rewrite" : {"type" : "string"},
"host_rewrite" : {"type" : "string"},
"auto_host_rewrite" : {"type" : "boolean"},
Expand Down
19 changes: 10 additions & 9 deletions source/common/router/config_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,7 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost,
response_headers_parser_(HeaderParser::configure(route.route().response_headers_to_add(),
route.route().response_headers_to_remove())),
opaque_config_(parseOpaqueConfig(route)), decorator_(parseDecorator(route)),
redirect_response_code_(
ConfigUtility::parseRedirectResponseCode(route.redirect().response_code())) {
direct_response_code_(ConfigUtility::parseDirectResponseCode(route)) {
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 @@ -425,18 +424,20 @@ DecoratorConstPtr RouteEntryImplBase::parseDecorator(const envoy::api::v2::Route
return ret;
}

const RedirectEntry* RouteEntryImplBase::redirectEntry() const {
// A route for a request can exclusively be a route entry or a redirect entry.
if (isRedirect()) {
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()) {

@alyssawilk alyssawilk Jan 10, 2018

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 +449,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 +490,7 @@ RouteConstSharedPtr RouteEntryImplBase::clusterEntry(const Http::HeaderMap& head
}

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

Expand Down
23 changes: 12 additions & 11 deletions source/common/router/config_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,19 @@ class RouteEntryImplBase;
typedef std::shared_ptr<const RouteEntryImplBase> RouteEntryImplBaseConstSharedPtr;

/**
* Redirect entry that does an SSL redirect.
* Direct response entry that does an SSL redirect.
*/
class SslRedirector : public RedirectEntry {
class SslRedirector : public DirectResponseEntry {
public:
// Router::RedirectEntry
// Router::DirectResponseEntry
std::string newPath(const Http::HeaderMap& headers) const override;
Http::Code redirectResponseCode() const override { return Http::Code::MovedPermanently; }
Http::Code responseCode() const override { return Http::Code::MovedPermanently; }
};

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

Expand Down Expand Up @@ -286,14 +286,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 @@ -329,12 +330,12 @@ class RouteEntryImplBase : public RouteEntry,
}
bool includeVirtualHostRateLimits() const override { return include_vh_rate_limits_; }

// Router::RedirectEntry
// Router::DirectResponseEntry
std::string newPath(const Http::HeaderMap& headers) const override;
Http::Code redirectResponseCode() const override { return redirect_response_code_; }
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 @@ -402,7 +403,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 @@ -487,7 +488,7 @@ class RouteEntryImplBase : public RouteEntry,
const std::multimap<std::string, std::string> opaque_config_;

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

@mattklein123 mattklein123 Jan 12, 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.

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
9 changes: 9 additions & 0 deletions source/common/router/config_utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,15 @@ Http::Code ConfigUtility::parseRedirectResponseCode(
}
}

Http::Code ConfigUtility::parseDirectResponseCode(const envoy::api::v2::Route& route) {
if (route.has_redirect()) {
return parseRedirectResponseCode(route.redirect().response_code());
} else if (route.has_direct_response()) {
return static_cast<Http::Code>(route.direct_response().status());
}
return static_cast<Http::Code>(0);
}

Http::Code ConfigUtility::parseClusterNotFoundResponseCode(
const envoy::api::v2::RouteAction::ClusterNotFoundResponseCode& code) {
switch (code) {
Expand Down
9 changes: 9 additions & 0 deletions source/common/router/config_utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,15 @@ class ConfigUtility {
static Http::Code
parseRedirectResponseCode(const envoy::api::v2::RedirectAction::RedirectResponseCode& code);

/**
* Returns the HTTP Status Code enum parsed from the route's redirect or direct_response.
* @param route supplies the Route configuration.
* @return Http::Code the HTTP status from the route's direct_response if specified,
* or the HTTP status code from the route's redirect if specified,
* or zero by default.
*/
static Http::Code parseDirectResponseCode(const envoy::api::v2::Route& route);

/**
* Returns the HTTP Status Code enum parsed from proto.
* @param code supplies the ClusterNotFoundResponseCode enum.
Expand Down
24 changes: 19 additions & 5 deletions source/common/router/router.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,25 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e
return Http::FilterHeadersStatus::StopIteration;
}

// Determine if there is a redirect for the request.
if (route_->redirectEntry()) {
config_.stats_.rq_redirect_.inc();
Http::Utility::sendRedirect(*callbacks_, route_->redirectEntry()->newPath(headers),
route_->redirectEntry()->redirectResponseCode());
// Determine if there is a direct response for the request.
if (route_->directResponseEntry()) {
auto response_code = route_->directResponseEntry()->responseCode();
if (response_code >= Http::Code::MultipleChoices && response_code < Http::Code::BadRequest) {
config_.stats_.rq_redirect_.inc();

@alyssawilk alyssawilk Jan 11, 2018

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 think we probably want a direct response stat which supplements (and hopefully eventually replaces) the redirect stat.

@brian-pane brian-pane Jan 11, 2018

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 added a counter of direct responses. Is there a precedent in Envoy for also having counters per response status or per response code family (2xx, 3xx, etc)?

@mattklein123 mattklein123 Jan 12, 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.

We already have split out codes for all downstream responses. IMO that's probably sufficient.

Http::Utility::sendRedirect(*callbacks_, route_->directResponseEntry()->newPath(headers),
response_code);
return Http::FilterHeadersStatus::StopIteration;
}
Http::Utility::sendLocalReply(

@alyssawilk alyssawilk Jan 11, 2018

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)

@brian-pane brian-pane Jan 11, 2018

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;
}

Expand Down
5 changes: 3 additions & 2 deletions test/common/http/filter/cors_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class CorsFilterTest : public testing::Test {
Buffer::OwnedImpl data_;
TestHeaderMapImpl request_headers_;
std::unique_ptr<Router::TestCorsPolicy> cors_policy_;
Router::MockRedirectEntry redirect_entry_;
Router::MockDirectResponseEntry direct_response_entry_;
};

TEST_F(CorsFilterTest, RequestWithoutOrigin) {
Expand Down Expand Up @@ -329,7 +329,8 @@ TEST_F(CorsFilterTest, EncodeWithNonMatchingOrigin) {
}

TEST_F(CorsFilterTest, RedirectRoute) {
ON_CALL(*decoder_callbacks_.route_, redirectEntry()).WillByDefault(Return(&redirect_entry_));
ON_CALL(*decoder_callbacks_.route_, directResponseEntry())
.WillByDefault(Return(&direct_response_entry_));

EXPECT_EQ(FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers_, false));
EXPECT_EQ(FilterDataStatus::Continue, filter_.decodeData(data_, false));
Expand Down
Loading