From 428f5bc01c0e5ecfa45c425b42bb825662cd98bb Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Wed, 8 Nov 2017 16:17:41 -0800 Subject: [PATCH 1/4] Add support to specify redirect response code Signed-off-by: Constance Caramanolis --- bazel/repositories.bzl | 2 +- include/envoy/router/BUILD | 1 + include/envoy/router/router.h | 7 +++++++ source/common/http/utility.cc | 5 +++-- source/common/http/utility.h | 6 ++++-- source/common/router/BUILD | 1 + source/common/router/config_impl.cc | 4 +++- source/common/router/config_impl.h | 4 ++++ source/common/router/config_utility.cc | 18 ++++++++++++++++++ source/common/router/config_utility.h | 9 +++++++++ source/common/router/router.cc | 3 ++- test/common/router/router_test.cc | 15 +++++++++++++++ test/mocks/router/mocks.h | 1 + 13 files changed, 69 insertions(+), 7 deletions(-) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index c8c9e349ca7f6..e0f5e53f4e4ff 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -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 = [ diff --git a/include/envoy/router/BUILD b/include/envoy/router/BUILD index 2ac863630c10d..aa69ddd8b12e5 100644 --- a/include/envoy/router/BUILD +++ b/include/envoy/router/BUILD @@ -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", diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 8ea712a66b896..0d79cefeb3f24 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -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" @@ -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; }; /** diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 23025c316149a..f6112d1ae5ed7 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -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); diff --git a/source/common/http/utility.h b/source/common/http/utility.h index efad3e00dc157..edd4c6e1ff1a5 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -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 status_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. diff --git a/source/common/router/BUILD b/source/common/router/BUILD index dd839deaeee59..c274e1eb42ba2 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -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", diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 623f448cada66..bef1c2efc4381 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -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); diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 025d0819c216c..2a8958081094e 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -52,6 +52,8 @@ 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 { @@ -329,6 +331,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; @@ -474,6 +477,7 @@ class RouteEntryImplBase : public RouteEntry, const std::multimap opaque_config_; const DecoratorConstPtr decorator_; + const Http::Code redirect_response_code_; }; /** diff --git a/source/common/router/config_utility.cc b/source/common/router/config_utility.cc index 0beb9d363130e..6f374b073fb09 100644 --- a/source/common/router/config_utility.cc +++ b/source/common/router/config_utility.cc @@ -45,5 +45,23 @@ bool ConfigUtility::matchHeaders(const Http::HeaderMap& request_headers, return matches; } +Http::Code ConfigUtility::parseRedirectResponseCode( + 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 diff --git a/source/common/router/config_utility.h b/source/common/router/config_utility.h index e19686a4e9f84..280cde4b77964 100644 --- a/source/common/router/config_utility.h +++ b/source/common/router/config_utility.h @@ -4,6 +4,7 @@ #include #include +#include "envoy/http/codes.h" #include "envoy/json/json_object.h" #include "envoy/upstream/resource_manager.h" @@ -57,6 +58,14 @@ class ConfigUtility { */ static bool matchHeaders(const Http::HeaderMap& headers, const std::vector& request_headers); + + /** + * Returns the HTTP Status Code enum parsed from proto. + * @param code supplies the RedirectResponseCode enum support for Redirect Actions. + * @return Returns the Http::Code version of the RedirectResponseCode. + */ + static Http::Code + parseRedirectResponseCode(const envoy::api::v2::RedirectAction::RedirectResponseCode& code); }; } // namespace Router diff --git a/source/common/router/router.cc b/source/common/router/router.cc index da5164302f192..5ad6650dec91a 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -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; } diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 002ac6ea7b16d..e80e0e6802204 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -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"}}; @@ -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 route; diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 9387bb967c70c..bedae0a17e59d 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -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 { From 72a32ab0f3d052469a45d2ee50108c6b7f82d995 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Wed, 8 Nov 2017 21:08:08 -0800 Subject: [PATCH 2/4] Fix tiny nits Signed-off-by: Constance Caramanolis --- include/envoy/router/router.h | 2 +- source/common/router/config_impl.h | 1 - source/common/router/config_utility.h | 4 ++-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 0d79cefeb3f24..1b1efe00ac312 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -38,7 +38,7 @@ class RedirectEntry { /** * Returns the HTTP status code to use when redirecting a request. - * @return HTTP::Code the redirect response Code. + * @return Http::Code the redirect response Code. */ virtual Http::Code redirectResponseCode() const PURE; }; diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 2a8958081094e..600d7b1cca074 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -52,7 +52,6 @@ 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; } }; diff --git a/source/common/router/config_utility.h b/source/common/router/config_utility.h index 280cde4b77964..e52ffb49d8574 100644 --- a/source/common/router/config_utility.h +++ b/source/common/router/config_utility.h @@ -60,8 +60,8 @@ class ConfigUtility { const std::vector& request_headers); /** - * Returns the HTTP Status Code enum parsed from proto. - * @param code supplies the RedirectResponseCode enum support for Redirect Actions. + * 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 From 6e3a49ce25d970996528726807fda68bcd0ff78d Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 9 Nov 2017 10:21:36 -0800 Subject: [PATCH 3/4] Add test case and address comments. Signed-off-by: Constance Caramanolis --- source/common/http/utility.h | 2 +- test/common/router/config_impl_test.cc | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/source/common/http/utility.h b/source/common/http/utility.h index edd4c6e1ff1a5..c5ca55161bd69 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -144,7 +144,7 @@ class Utility { * Send a redirect response. * @param callbacks supplies the filter callbacks to use. * @param new_path supplies the redirect target. - * @param status_code supplies the response code to use. + * @param response_code supplies the response code to use. */ static void sendRedirect(StreamDecoderFilterCallbacks& callbacks, const std::string& new_path, Code response_code); diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index ff22f7a927d05..4476c6c9d1c6b 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -2927,6 +2927,21 @@ TEST(RoutEntryMetadataMatchTest, ParsesMetadata) { } } +TEST(ConfigUtility, ParseResponseCode) { + const std::vector> + 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)); + } +} + } // namespace } // namespace Router } // namespace Envoy From 13ced5ba8f48ee896ed44263d347d3e97649e0d8 Mon Sep 17 00:00:00 2001 From: Constance Caramanolis Date: Thu, 9 Nov 2017 15:10:09 -0800 Subject: [PATCH 4/4] Add test for redirect response from route configuration. Signed-off-by: Constance Caramanolis --- source/common/router/config_impl.cc | 2 +- test/common/router/config_impl_test.cc | 33 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index bef1c2efc4381..5202fed55bf75 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -688,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]) { diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 4476c6c9d1c6b..f2a2b633d2fff 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -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( { @@ -2942,6 +2948,33 @@ TEST(ConfigUtility, ParseResponseCode) { } } +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; + NiceMock 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