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 all 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.
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk Jan 10, 2018

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

@brian-pane brian-pane Jan 10, 2018

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

@alyssawilk alyssawilk Jan 10, 2018

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

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Jan 10, 2018 •
edited
Loading

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

@brian-pane brian-pane Jan 10, 2018

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

@mattklein123 mattklein123 Jan 10, 2018

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.
*/
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
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
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()) {
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 direct_response_code_.valid(); }

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_.value(); }

// 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 Optional<Http::Code> direct_response_code_;
};

/**
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(
}
}

Optional<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 Optional<Http::Code>();
}

Http::Code ConfigUtility::parseClusterNotFoundResponseCode(
const envoy::api::v2::RouteAction::ClusterNotFoundResponseCode& code) {
switch (code) {
Expand Down
10 changes: 10 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 @@ -4,6 +4,7 @@
#include <string>
#include <vector>

#include "envoy/common/optional.h"
#include "envoy/http/codes.h"
#include "envoy/json/json_object.h"
#include "envoy/upstream/resource_manager.h"
Expand Down Expand Up @@ -103,6 +104,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 Optional<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 an empty Option otherwise.
*/
static Optional<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
19 changes: 13 additions & 6 deletions source/common/router/router.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e
// that get handled by earlier filters.
config_.stats_.rq_total_.inc();

// Determine if there is a route entry or a redirect for the request.
// Determine if there is a route entry or a direct response for the request.
route_ = callbacks_->route();
if (!route_) {
config_.stats_.no_route_.inc();
Expand All @@ -209,11 +209,18 @@ 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();
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk Jan 11, 2018

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.

Copy link
Copy Markdown
Contributor Author

@brian-pane brian-pane Jan 11, 2018

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)?

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Jan 12, 2018

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;
}
config_.stats_.rq_direct_response_.inc();
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Jan 12, 2018

Choose a reason for hiding this comment

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

please add an expectation in one of the tests that checks this stat gets incremented.

Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk Jan 16, 2018

Choose a reason for hiding this comment

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

Oh, and I almost forgot - we should update docs for this. Please add an envoy-API pull updating router_filter.rst and note it in your PR description

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Jan 16, 2018

Choose a reason for hiding this comment

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

@brian-pane you plan on finishing up docs in a follow up, right?

Copy link
Copy Markdown
Contributor Author

@brian-pane brian-pane Jan 16, 2018

Choose a reason for hiding this comment

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

Yes, I was planning to make a followup PR that adds the remainder of this feature (adding optional response headers and body), with the docs linked to that new PR.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Jan 16, 2018

Choose a reason for hiding this comment

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

OK that works. Will merge this.

sendLocalReply(route_->directResponseEntry()->responseCode(), "", false);
// TODO(brian-pane) support sending a response body and response_headers_to_add.
return Http::FilterHeadersStatus::StopIteration;
}

Expand Down
1 change: 1 addition & 0 deletions source/common/router/router.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ namespace Router {
COUNTER(no_route) \
COUNTER(no_cluster) \
COUNTER(rq_redirect) \
COUNTER(rq_direct_response) \
COUNTER(rq_total)
// clang-format on

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