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.

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

redirect: add support to specify response code #2030

Merged
merged 4 commits into from
Nov 9, 2017
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ def envoy_api_deps(skip_targets):
native.git_repository(
name = "envoy_api",
remote = REPO_LOCATIONS["data-plane-api"],
commit = "4d18e6d236a6476782076b217cd62d43c30a7dfe",
commit = "971fb1b70f419348a1ac2273508237b7ebd08cf5",
)

api_bind_targets = [
Expand Down
1 change: 1 addition & 0 deletions include/envoy/router/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ envoy_cc_library(
"//include/envoy/access_log:access_log_interface",
"//include/envoy/common:optional",
"//include/envoy/http:codec_interface",
"//include/envoy/http:codes_interface",
"//include/envoy/http:header_map_interface",
"//include/envoy/tracing:http_tracer_interface",
"//include/envoy/upstream:resource_manager_interface",
Expand Down
7 changes: 7 additions & 0 deletions include/envoy/router/router.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "envoy/access_log/access_log.h"
#include "envoy/common/optional.h"
#include "envoy/http/codec.h"
#include "envoy/http/codes.h"
#include "envoy/http/header_map.h"
#include "envoy/tracing/http_tracer.h"
#include "envoy/upstream/resource_manager.h"
Expand All @@ -34,6 +35,12 @@ class RedirectEntry {
* @return std::string the redirect URL.
*/
virtual std::string newPath(const Http::HeaderMap& headers) const PURE;

/**
* Returns the HTTP status code to use when redirecting a request.
* @return Http::Code the redirect response Code.
*/
virtual Http::Code redirectResponseCode() const PURE;
};

/**
Expand Down
5 changes: 3 additions & 2 deletions source/common/http/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,10 @@ void Utility::sendLocalReply(
}
}

void Utility::sendRedirect(StreamDecoderFilterCallbacks& callbacks, const std::string& new_path) {
void Utility::sendRedirect(StreamDecoderFilterCallbacks& callbacks, const std::string& new_path,
Code response_code) {
HeaderMapPtr response_headers{
new HeaderMapImpl{{Headers::get().Status, std::to_string(enumToInt(Code::MovedPermanently))},
new HeaderMapImpl{{Headers::get().Status, std::to_string(enumToInt(response_code))},
{Headers::get().Location, new_path}}};

callbacks.encodeHeaders(std::move(response_headers), true);
Expand Down
6 changes: 4 additions & 2 deletions source/common/http/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -141,11 +141,13 @@ class Utility {
const bool& is_reset, Code response_code, const std::string& body_text);

/**
* Send a redirect response (301).
* Send a redirect response.
* @param callbacks supplies the filter callbacks to use.
* @param new_path supplies the redirect target.
* @param response_code supplies the response code to use.
*/
static void sendRedirect(StreamDecoderFilterCallbacks& callbacks, const std::string& new_path);
static void sendRedirect(StreamDecoderFilterCallbacks& callbacks, const std::string& new_path,
Code response_code);

/**
* Retrieves the last address in x-forwarded-for header. If it isn't set, returns empty string.
Expand Down
1 change: 1 addition & 0 deletions source/common/router/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ envoy_cc_library(
hdrs = ["config_utility.h"],
external_deps = ["envoy_rds"],
deps = [
"//include/envoy/http:codes_interface",
"//include/envoy/upstream:resource_manager_interface",
"//source/common/common:assert_lib",
"//source/common/common:empty_string",
Expand Down
6 changes: 4 additions & 2 deletions source/common/router/config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,9 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost,
rate_limit_policy_(route.route().rate_limits()), shadow_policy_(route.route()),
priority_(ConfigUtility::parsePriority(route.route().priority())),
request_headers_parser_(RequestHeaderParser::parse(route.route().request_headers_to_add())),
opaque_config_(parseOpaqueConfig(route)), decorator_(parseDecorator(route)) {
opaque_config_(parseOpaqueConfig(route)), decorator_(parseDecorator(route)),
redirect_response_code_(
ConfigUtility::parseRedirectResponseCode(route.redirect().response_code())) {
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 @@ -686,7 +688,7 @@ RouteMatcher::RouteMatcher(const envoy::api::v2::RouteConfiguration& route_confi
for (const std::string& domain : virtual_host_config.domains()) {
if ("*" == domain) {
if (default_virtual_host_) {
throw EnvoyException(fmt::format("Only a single single wildcard domain is permitted"));
throw EnvoyException(fmt::format("Only a single wildcard domain is permitted"));
}
default_virtual_host_ = virtual_host;
} else if (domain.size() > 0 && '*' == domain[0]) {
Expand Down
3 changes: 3 additions & 0 deletions source/common/router/config_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class SslRedirector : public RedirectEntry {
public:
// Router::RedirectEntry
std::string newPath(const Http::HeaderMap& headers) const override;
Http::Code redirectResponseCode() const override { return Http::Code::MovedPermanently; }
};

class SslRedirectRoute : public Route {
Expand Down Expand Up @@ -329,6 +330,7 @@ class RouteEntryImplBase : public RouteEntry,

// Router::RedirectEntry
std::string newPath(const Http::HeaderMap& headers) const override;
Http::Code redirectResponseCode() const override { return redirect_response_code_; }

// Router::Route
const RedirectEntry* redirectEntry() const override;
Expand Down Expand Up @@ -474,6 +476,7 @@ class RouteEntryImplBase : public RouteEntry,
const std::multimap<std::string, std::string> opaque_config_;

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

/**
Expand Down
18 changes: 18 additions & 0 deletions source/common/router/config_utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,23 @@ bool ConfigUtility::matchHeaders(const Http::HeaderMap& request_headers,
return matches;
}

Http::Code ConfigUtility::parseRedirectResponseCode(
Copy link
Member

Choose a reason for hiding this comment

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

please make sure you have coverage over all of this. There should be some tests in config_impl_test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test case in config_impl_test.cc

Copy link
Member

Choose a reason for hiding this comment

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

You need an actual test that loads a route table, gets the route, and makes sure a code is set. You don't have any test to make sure that that part is working. This should be in config_impl_test also. This may require converting one of the tests over to v2 YAML like we have been doing lately in other tests.

const envoy::api::v2::RedirectAction::RedirectResponseCode& code) {
switch (code) {
case envoy::api::v2::RedirectAction::MOVED_PERMANENTLY:
return Http::Code::MovedPermanently;
case envoy::api::v2::RedirectAction::FOUND:
return Http::Code::Found;
case envoy::api::v2::RedirectAction::SEE_OTHER:
return Http::Code::SeeOther;
case envoy::api::v2::RedirectAction::TEMPORARY_REDIRECT:
return Http::Code::TemporaryRedirect;
case envoy::api::v2::RedirectAction::PERMANENT_REDIRECT:
return Http::Code::PermanentRedirect;
default:
NOT_IMPLEMENTED;
}
}

} // namespace Router
} // namespace Envoy
9 changes: 9 additions & 0 deletions source/common/router/config_utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <string>
#include <vector>

#include "envoy/http/codes.h"
#include "envoy/json/json_object.h"
#include "envoy/upstream/resource_manager.h"

Expand Down Expand Up @@ -57,6 +58,14 @@ class ConfigUtility {
*/
static bool matchHeaders(const Http::HeaderMap& headers,
const std::vector<HeaderData>& request_headers);

/**
* Returns the redirect HTTP Status Code enum parsed from proto.
* @param code supplies the RedirectResponseCode enum.
* @return Returns the Http::Code version of the RedirectResponseCode.
*/
static Http::Code
parseRedirectResponseCode(const envoy::api::v2::RedirectAction::RedirectResponseCode& code);
};

} // namespace Router
Expand Down
3 changes: 2 additions & 1 deletion source/common/router/router.cc
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e
// 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));
Http::Utility::sendRedirect(*callbacks_, route_->redirectEntry()->newPath(headers),
route_->redirectEntry()->redirectResponseCode());
return Http::FilterHeadersStatus::StopIteration;
}

Expand Down
48 changes: 48 additions & 0 deletions test/common/router/config_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ envoy::api::v2::RouteConfiguration parseRouteConfigurationFromJson(const std::st
return route_config;
}

envoy::api::v2::RouteConfiguration parseRouteConfigurationFromV2Yaml(const std::string& yaml) {
envoy::api::v2::RouteConfiguration route_config;
MessageUtil::loadFromYaml(yaml, route_config);
return route_config;
}

TEST(RouteMatcherTest, TestRoutes) {
std::string json = R"EOF(
{
Expand Down Expand Up @@ -2927,6 +2933,48 @@ TEST(RoutEntryMetadataMatchTest, ParsesMetadata) {
}
}

TEST(ConfigUtility, ParseResponseCode) {
const std::vector<std::pair<envoy::api::v2::RedirectAction::RedirectResponseCode, Http::Code>>
test_set = {std::make_pair(envoy::api::v2::RedirectAction::MOVED_PERMANENTLY,
Http::Code::MovedPermanently),
std::make_pair(envoy::api::v2::RedirectAction::FOUND, Http::Code::Found),
std::make_pair(envoy::api::v2::RedirectAction::SEE_OTHER, Http::Code::SeeOther),
std::make_pair(envoy::api::v2::RedirectAction::TEMPORARY_REDIRECT,
Http::Code::TemporaryRedirect),
std::make_pair(envoy::api::v2::RedirectAction::PERMANENT_REDIRECT,
Http::Code::PermanentRedirect)};
for (const auto& test_case : test_set) {
EXPECT_EQ(test_case.second, ConfigUtility::parseRedirectResponseCode(test_case.first));
}
}

TEST(RouteConfigurationV2, RedirectCode) {
std::string yaml = R"EOF(
name: foo
virtual_hosts:
- name: redirect
domains: [redirect.lyft.com]
routes:
- match: { prefix: "/"}
redirect: { host_redirect: new.lyft.com, response_code: TEMPORARY_REDIRECT }

)EOF";

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

EXPECT_EQ(nullptr, config.route(genRedirectHeaders("www.foo.com", "/foo", true, true), 0));

{
Http::TestHeaderMapImpl headers = genRedirectHeaders("redirect.lyft.com", "/foo", false, false);
EXPECT_EQ("http://new.lyft.com/foo",
config.route(headers, 0)->redirectEntry()->newPath(headers));
EXPECT_EQ(Http::Code::TemporaryRedirect,
config.route(headers, 0)->redirectEntry()->redirectResponseCode());
}
}

} // namespace
} // namespace Router
} // namespace Envoy
15 changes: 15 additions & 0 deletions test/common/router/router_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1405,6 +1405,7 @@ TEST_F(RouterTest, AltStatName) {
TEST_F(RouterTest, Redirect) {
MockRedirectEntry redirect;
EXPECT_CALL(redirect, newPath(_)).WillOnce(Return("hello"));
EXPECT_CALL(redirect, redirectResponseCode()).WillOnce(Return(Http::Code::MovedPermanently));
EXPECT_CALL(*callbacks_.route_, redirectEntry()).WillRepeatedly(Return(&redirect));

Http::TestHeaderMapImpl response_headers{{":status", "301"}, {"location", "hello"}};
Expand All @@ -1415,6 +1416,20 @@ TEST_F(RouterTest, Redirect) {
EXPECT_TRUE(verifyHostUpstreamStats(0, 0));
}

TEST_F(RouterTest, RedirectFound) {
MockRedirectEntry redirect;
EXPECT_CALL(redirect, newPath(_)).WillOnce(Return("hello"));
EXPECT_CALL(redirect, redirectResponseCode()).WillOnce(Return(Http::Code::Found));
EXPECT_CALL(*callbacks_.route_, redirectEntry()).WillRepeatedly(Return(&redirect));

Http::TestHeaderMapImpl response_headers{{":status", "302"}, {"location", "hello"}};
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
1 change: 1 addition & 0 deletions test/mocks/router/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class MockRedirectEntry : public RedirectEntry {

// Router::Config
MOCK_CONST_METHOD1(newPath, std::string(const Http::HeaderMap& headers));
MOCK_CONST_METHOD0(redirectResponseCode, Http::Code());
};

class TestCorsPolicy : public CorsPolicy {
Expand Down