From 9aa8efd10355b91b558e8a20398bee07f09168ca Mon Sep 17 00:00:00 2001 From: Brian Pane Date: Mon, 22 Jan 2018 23:10:48 +0000 Subject: [PATCH 1/4] Send optional entity-body with direct responses *Description* This change, which is part 2 of a 3-part implementation of the new direct response feature, enables Envoy to include a body with a direct response. The body can be specified inline within the configuration or loaded from a file at request time. *Risk Level*: Medium *Testing*: Unit tests included *Docs Changes*: None yet. I'll make a docs PR once I've done the 3rd and last part of the implementation: support for adding headers to direct responses. *Release Notes*: N/A *Issue*: #2315 *API Changes*: [#393](https://github.com/envoyproxy/data-plane-api/pull/393) Signed-off-by: Brian Pane --- include/envoy/router/router.h | 14 ++++ source/common/filesystem/filesystem_impl.cc | 9 +++ source/common/filesystem/filesystem_impl.h | 8 ++ source/common/router/BUILD | 1 + source/common/router/config_impl.cc | 4 +- source/common/router/config_impl.h | 6 ++ source/common/router/config_utility.cc | 27 +++++++ source/common/router/config_utility.h | 18 +++++ source/common/router/router.cc | 20 ++++- .../common/filesystem/filesystem_impl_test.cc | 8 ++ test/common/router/BUILD | 1 + test/common/router/config_impl_test.cc | 38 ++++++++- test/common/router/router_test.cc | 77 ++++++++++++++++++- test/mocks/router/mocks.h | 2 + 14 files changed, 228 insertions(+), 5 deletions(-) diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 3a6bb91b79c6e..ef43f1c16e243 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -44,6 +44,20 @@ class DirectResponseEntry { * or an empty string otherwise. */ virtual std::string newPath(const Http::HeaderMap& headers) const PURE; + + /** + * Returns the response body to send with direct responses. + * @return Optional the response body specified in the route configuration, + * or an invalid Optional if no response body is specified. + */ + virtual Optional responseBody() const PURE; + + /** + * Returns the pathname of the file containing the response body to send with direct responses. + * @return Optional the response body pathname specified in the route configuration, + * or an invalid Optional if no response body pathname is specified. + */ + virtual Optional responseBodyFilename() const PURE; }; /** diff --git a/source/common/filesystem/filesystem_impl.cc b/source/common/filesystem/filesystem_impl.cc index d6431247c086e..1a42cb376cf8c 100644 --- a/source/common/filesystem/filesystem_impl.cc +++ b/source/common/filesystem/filesystem_impl.cc @@ -1,6 +1,7 @@ #include "common/filesystem/filesystem_impl.h" #include +#include #include #include @@ -39,6 +40,14 @@ bool directoryExists(const std::string& path) { return dir_exists; } +ssize_t fileSize(const std::string& path) { + struct stat info; + if (stat(path.c_str(), &info) != 0) { + return -1; + } + return info.st_size; +} + std::string fileReadToEnd(const std::string& path) { std::ios::sync_with_stdio(false); diff --git a/source/common/filesystem/filesystem_impl.h b/source/common/filesystem/filesystem_impl.h index fffda39fb2b60..2f936b49f9f8e 100644 --- a/source/common/filesystem/filesystem_impl.h +++ b/source/common/filesystem/filesystem_impl.h @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -41,6 +42,13 @@ bool fileExists(const std::string& path); */ bool directoryExists(const std::string& path); +/** + * @return ssize_t the size in bytes of the specified file, or -1 if the file size + * cannot be determined for any reason, including without limitation + * the non-existence of the file. + */ +ssize_t fileSize(const std::string& path); + /** * @return full file content as a string. * Be aware, this is not most highly performing file reading method. diff --git a/source/common/router/BUILD b/source/common/router/BUILD index b76d6d4ed526f..426d979c2b16b 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -157,6 +157,7 @@ envoy_cc_library( "//source/common/common:hex_lib", "//source/common/common:logger_lib", "//source/common/common:utility_lib", + "//source/common/filesystem:filesystem_lib", "//source/common/grpc:common_lib", "//source/common/http:codes_lib", "//source/common/http:header_map_lib", diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 2cc4849deba4c..c74341a61a465 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -237,7 +237,9 @@ 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)), - direct_response_code_(ConfigUtility::parseDirectResponseCode(route)) { + direct_response_code_(ConfigUtility::parseDirectResponseCode(route)), + direct_response_body_(ConfigUtility::parseDirectResponseBody(route)), + direct_response_file_(ConfigUtility::parseDirectResponseFilename(route)) { 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 0f6907aa5d3ff..2db1bdd015b71 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -54,6 +54,8 @@ class SslRedirector : public DirectResponseEntry { // Router::DirectResponseEntry std::string newPath(const Http::HeaderMap& headers) const override; Http::Code responseCode() const override { return Http::Code::MovedPermanently; } + Optional responseBody() const override { return Optional(); } + Optional responseBodyFilename() const override { return Optional(); } }; class SslRedirectRoute : public Route { @@ -334,6 +336,8 @@ class RouteEntryImplBase : public RouteEntry, // Router::DirectResponseEntry std::string newPath(const Http::HeaderMap& headers) const override; Http::Code responseCode() const override { return direct_response_code_.value(); } + Optional responseBody() const override { return direct_response_body_; } + Optional responseBodyFilename() const override { return direct_response_file_; } // Router::Route const DirectResponseEntry* directResponseEntry() const override; @@ -492,6 +496,8 @@ class RouteEntryImplBase : public RouteEntry, const DecoratorConstPtr decorator_; const Optional direct_response_code_; + const Optional direct_response_body_; + const Optional direct_response_file_; }; /** diff --git a/source/common/router/config_utility.cc b/source/common/router/config_utility.cc index 09acc121f64e5..c6ce5845a628d 100644 --- a/source/common/router/config_utility.cc +++ b/source/common/router/config_utility.cc @@ -98,6 +98,33 @@ Optional ConfigUtility::parseDirectResponseCode(const envoy::api::v2 return Optional(); } +Optional ConfigUtility::parseDirectResponseBody(const envoy::api::v2::Route& route) { + if (route.has_direct_response() && route.direct_response().has_body()) { + const auto& body = route.direct_response().body(); + std::string inline_bytes = body.inline_bytes(); + if (!inline_bytes.empty()) { + return inline_bytes; + } + std::string inline_string = body.inline_string(); + if (!inline_string.empty()) { + return inline_string; + } + } + return Optional(); +} + +Optional +ConfigUtility::parseDirectResponseFilename(const envoy::api::v2::Route& route) { + if (route.has_direct_response() && route.direct_response().has_body()) { + const auto& body = route.direct_response().body(); + std::string filename = body.filename(); + if (!filename.empty()) { + return filename; + } + } + return Optional(); +} + Http::Code ConfigUtility::parseClusterNotFoundResponseCode( const envoy::api::v2::RouteAction::ClusterNotFoundResponseCode& code) { switch (code) { diff --git a/source/common/router/config_utility.h b/source/common/router/config_utility.h index 9f2d3c2f578bd..9130f9725b765 100644 --- a/source/common/router/config_utility.h +++ b/source/common/router/config_utility.h @@ -114,6 +114,24 @@ class ConfigUtility { */ static Optional parseDirectResponseCode(const envoy::api::v2::Route& route); + /** + * Returns the content of the response body to send with direct responses from a route. + * @param route supplies the Route configuration. + * @return Optional the response body in 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 parseDirectResponseBody(const envoy::api::v2::Route& route); + + /** + * Returns the pathname of the file containing the response body to send with direct + * responses from a route. + * @param route supplies the Route configuration. + * @return Optional the response body filename in the route's direct_response + * if specified, or an empty Option otherwise. + */ + static Optional parseDirectResponseFilename(const envoy::api::v2::Route& route); + /** * Returns the HTTP Status Code enum parsed from proto. * @param code supplies the ClusterNotFoundResponseCode enum. diff --git a/source/common/router/router.cc b/source/common/router/router.cc index a8f0caf1ac3cd..93bc634a328f7 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -17,6 +17,7 @@ #include "common/common/empty_string.h" #include "common/common/enum_to_int.h" #include "common/common/utility.h" +#include "common/filesystem/filesystem_impl.h" #include "common/grpc/common.h" #include "common/http/codes.h" #include "common/http/header_map_impl.h" @@ -219,8 +220,23 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e return Http::FilterHeadersStatus::StopIteration; } config_.stats_.rq_direct_response_.inc(); - sendLocalReply(route_->directResponseEntry()->responseCode(), "", false); - // TODO(brian-pane) support sending a response body and response_headers_to_add. + std::string body; + Optional inline_body = route_->directResponseEntry()->responseBody(); + if (inline_body.valid()) { + body = inline_body.value(); + } + Optional path = route_->directResponseEntry()->responseBodyFilename(); + if (path.valid()) { + const ssize_t kMaxFileSize = 4096; + ssize_t file_size = Filesystem::fileSize(path.value()); + if (file_size < 0 || file_size > kMaxFileSize) { + sendLocalReply(Http::Code::InternalServerError, "", false); + return Http::FilterHeadersStatus::StopIteration; + } + body = Filesystem::fileReadToEnd(path.value()); + } + sendLocalReply(route_->directResponseEntry()->responseCode(), body, false); + // TODO(brian-pane) support sending response_headers_to_add. return Http::FilterHeadersStatus::StopIteration; } diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index 01f5852188207..ab2aebed9fa74 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/test/common/filesystem/filesystem_impl_test.cc @@ -47,6 +47,14 @@ TEST(FileSystemImpl, directoryExists) { EXPECT_FALSE(Filesystem::directoryExists("/dev/blahblah")); } +TEST(FileSystemImpl, fileSize) { + EXPECT_EQ(0, Filesystem::fileSize("/dev/null")); + EXPECT_EQ(-1, Filesystem::fileSize("/dev/blahblahblah")); + const std::string data = "test string\ntest"; + const std::string file_path = TestEnvironment::writeStringToFileForTest("test_envoy", data); + EXPECT_EQ(data.length(), Filesystem::fileSize(file_path)); +} + TEST(FileSystemImpl, fileReadToEndSuccess) { const std::string data = "test string\ntest"; const std::string file_path = TestEnvironment::writeStringToFileForTest("test_envoy", data); diff --git a/test/common/router/BUILD b/test/common/router/BUILD index d83657cd783ad..67cf9f34afa10 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -91,6 +91,7 @@ envoy_cc_test( "//test/mocks/runtime:runtime_mocks", "//test/mocks/ssl:ssl_mocks", "//test/mocks/upstream:upstream_mocks", + "//test/test_common:environment_lib", "//test/test_common:utility_lib", ], ) diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 72b552cd2e532..74fab81775c50 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -2217,7 +2217,20 @@ name: foo domains: [direct.example.com] routes: - match: { prefix: /gone } - direct_response: { status: 410 } + direct_response: + status: 410 + body: { inline_bytes: "RXhhbXBsZSB0ZXh0IDE=" } + - match: { prefix: /error } + direct_response: + status: 500 + body: { inline_string: "Example text 2" } + - match: { prefix: /no_body } + direct_response: + status: 200 + - match: { prefix: /static } + direct_response: + status: 200 + body: { filename: /etc/envoy/message } - match: { prefix: / } route: { cluster: www2 } )EOF"; @@ -2267,6 +2280,29 @@ name: foo Http::TestHeaderMapImpl headers = genRedirectHeaders("direct.example.com", "/gone", true, false); EXPECT_EQ(Http::Code::Gone, config.route(headers, 0)->directResponseEntry()->responseCode()); + EXPECT_EQ("Example text 1", + config.route(headers, 0)->directResponseEntry()->responseBody().value()); + } + { + Http::TestHeaderMapImpl headers = + genRedirectHeaders("direct.example.com", "/error", true, false); + EXPECT_EQ(Http::Code::InternalServerError, + config.route(headers, 0)->directResponseEntry()->responseCode()); + EXPECT_EQ("Example text 2", + config.route(headers, 0)->directResponseEntry()->responseBody().value()); + } + { + Http::TestHeaderMapImpl headers = + genRedirectHeaders("direct.example.com", "/no_body", true, false); + EXPECT_EQ(Http::Code::OK, config.route(headers, 0)->directResponseEntry()->responseCode()); + EXPECT_FALSE(config.route(headers, 0)->directResponseEntry()->responseBody().valid()); + } + { + Http::TestHeaderMapImpl headers = + genRedirectHeaders("direct.example.com", "/static", true, false); + EXPECT_EQ(Http::Code::OK, config.route(headers, 0)->directResponseEntry()->responseCode()); + EXPECT_EQ("/etc/envoy/message", + config.route(headers, 0)->directResponseEntry()->responseBodyFilename().value()); } { Http::TestHeaderMapImpl headers = diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 01135c40a92dc..622017212dc18 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -15,6 +15,7 @@ #include "test/mocks/runtime/mocks.h" #include "test/mocks/ssl/mocks.h" #include "test/mocks/upstream/mocks.h" +#include "test/test_common/environment.h" #include "test/test_common/printers.h" #include "test/test_common/utility.h" @@ -28,6 +29,7 @@ using testing::AtLeast; using testing::Invoke; using testing::MockFunction; using testing::NiceMock; +using testing::Ref; using testing::Return; using testing::ReturnRef; using testing::SaveArg; @@ -1552,7 +1554,7 @@ TEST_F(RouterTest, RedirectFound) { } TEST_F(RouterTest, DirectResponse) { - MockDirectResponseEntry direct_response; + NiceMock direct_response; EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); @@ -1565,6 +1567,79 @@ TEST_F(RouterTest, DirectResponse) { EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value()); } +TEST_F(RouterTest, DirectResponseWithBody) { + NiceMock direct_response; + EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); + EXPECT_CALL(direct_response, responseBody()) + .WillRepeatedly(Return(Optional("static response"))); + EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); + + Http::TestHeaderMapImpl response_headers{ + {":status", "200"}, {"content-length", "15"}, {"content-type", "text/plain"}}; + EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false)); + EXPECT_CALL(callbacks_, encodeData(_, true)); + Http::TestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); + EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value()); +} + +TEST_F(RouterTest, DirectResponseWithBodyFromFile) { + { + auto pathname = TestEnvironment::writeStringToFileForTest("response_body", + "content to deliver from a file"); + NiceMock direct_response; + EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); + EXPECT_CALL(direct_response, responseBodyFilename()) + .WillRepeatedly(Return(Optional(pathname))); + EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); + Http::TestHeaderMapImpl response_headers{ + {":status", "200"}, {"content-length", "30"}, {"content-type", "text/plain"}}; + EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false)); + EXPECT_CALL(callbacks_, encodeData(_, true)); + Http::TestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); + EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value()); + config_.stats_.rq_direct_response_.reset(); + } + { + // Nonexistent file, should return a 500. + NiceMock direct_response; + EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); + EXPECT_CALL(direct_response, responseBodyFilename()) + .WillRepeatedly(Return(Optional("nonexistent file"))); + EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); + Http::TestHeaderMapImpl response_headers{{":status", "500"}}; + EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); + Http::TestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); + EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value()); + config_.stats_.rq_direct_response_.reset(); + } + { + // File too big, should return a 500. + auto pathname = + TestEnvironment::writeStringToFileForTest("response_body", std::string(4097, '*')); + NiceMock direct_response; + EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); + EXPECT_CALL(direct_response, responseBodyFilename()) + .WillRepeatedly(Return(Optional(pathname))); + EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); + Http::TestHeaderMapImpl response_headers{{":status", "500"}}; + EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); + Http::TestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); + EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value()); + } +} + TEST(RouterFilterUtilityTest, finalTimeout) { { NiceMock route; diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 63fc1d9f3fe89..447a2bad81c49 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -35,6 +35,8 @@ class MockDirectResponseEntry : public DirectResponseEntry { // DirectResponseEntry MOCK_CONST_METHOD1(newPath, std::string(const Http::HeaderMap& headers)); MOCK_CONST_METHOD0(responseCode, Http::Code()); + MOCK_CONST_METHOD0(responseBody, Optional()); + MOCK_CONST_METHOD0(responseBodyFilename, Optional()); }; class TestCorsPolicy : public CorsPolicy { From 7a6404fc0df56f6f8da48b290a97080f06af89cf Mon Sep 17 00:00:00 2001 From: Brian Pane Date: Tue, 23 Jan 2018 21:54:15 +0000 Subject: [PATCH 2/4] Read response body from file only during config load Signed-off-by: Brian Pane --- include/envoy/router/router.h | 13 ++--- source/common/filesystem/filesystem_impl.h | 1 + source/common/router/BUILD | 1 + source/common/router/config_impl.cc | 3 +- source/common/router/config_impl.h | 12 ++--- source/common/router/config_utility.cc | 48 +++++++++-------- source/common/router/config_utility.h | 18 ++----- source/common/router/router.cc | 10 ---- test/common/router/BUILD | 1 + test/common/router/config_impl_test.cc | 18 ++++--- test/common/router/router_test.cc | 61 ++-------------------- test/mocks/router/mocks.h | 3 +- 12 files changed, 60 insertions(+), 129 deletions(-) diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index ef43f1c16e243..bb030bc9481c7 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -47,17 +47,10 @@ class DirectResponseEntry { /** * Returns the response body to send with direct responses. - * @return Optional the response body specified in the route configuration, - * or an invalid Optional if no response body is specified. + * @return std::string& the response body specified in the route configuration, + * or an empty string if no response body is specified. */ - virtual Optional responseBody() const PURE; - - /** - * Returns the pathname of the file containing the response body to send with direct responses. - * @return Optional the response body pathname specified in the route configuration, - * or an invalid Optional if no response body pathname is specified. - */ - virtual Optional responseBodyFilename() const PURE; + virtual const std::string& responseBody() const PURE; }; /** diff --git a/source/common/filesystem/filesystem_impl.h b/source/common/filesystem/filesystem_impl.h index 2f936b49f9f8e..6cf897d386944 100644 --- a/source/common/filesystem/filesystem_impl.h +++ b/source/common/filesystem/filesystem_impl.h @@ -51,6 +51,7 @@ ssize_t fileSize(const std::string& path); /** * @return full file content as a string. + * @throw EnvoyException if the file cannot be read. * Be aware, this is not most highly performing file reading method. */ std::string fileReadToEnd(const std::string& path); diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 426d979c2b16b..927a062e794b4 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -48,6 +48,7 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/common:empty_string", "//source/common/config:rds_json_lib", + "//source/common/filesystem:filesystem_lib", "//source/common/http:headers_lib", "//source/common/protobuf:utility_lib", ], diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index c74341a61a465..53587177f437d 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -238,8 +238,7 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, route.route().response_headers_to_remove())), opaque_config_(parseOpaqueConfig(route)), decorator_(parseDecorator(route)), direct_response_code_(ConfigUtility::parseDirectResponseCode(route)), - direct_response_body_(ConfigUtility::parseDirectResponseBody(route)), - direct_response_file_(ConfigUtility::parseDirectResponseFilename(route)) { + direct_response_body_(ConfigUtility::parseDirectResponseBody(route)) { 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 2db1bdd015b71..ccc3f9b8b4ee8 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -54,8 +54,7 @@ class SslRedirector : public DirectResponseEntry { // Router::DirectResponseEntry std::string newPath(const Http::HeaderMap& headers) const override; Http::Code responseCode() const override { return Http::Code::MovedPermanently; } - Optional responseBody() const override { return Optional(); } - Optional responseBodyFilename() const override { return Optional(); } + const std::string& responseBody() const override { return EMPTY_STRING; } }; class SslRedirectRoute : public Route { @@ -292,6 +291,9 @@ class RouteEntryImplBase : public RouteEntry, public Route, public std::enable_shared_from_this { public: + /** + * @throw EnvoyException with reason if the route configuration contains any errors + */ RouteEntryImplBase(const VirtualHostImpl& vhost, const envoy::api::v2::Route& route, Runtime::Loader& loader); @@ -336,8 +338,7 @@ class RouteEntryImplBase : public RouteEntry, // Router::DirectResponseEntry std::string newPath(const Http::HeaderMap& headers) const override; Http::Code responseCode() const override { return direct_response_code_.value(); } - Optional responseBody() const override { return direct_response_body_; } - Optional responseBodyFilename() const override { return direct_response_file_; } + const std::string& responseBody() const override { return direct_response_body_; } // Router::Route const DirectResponseEntry* directResponseEntry() const override; @@ -496,8 +497,7 @@ class RouteEntryImplBase : public RouteEntry, const DecoratorConstPtr decorator_; const Optional direct_response_code_; - const Optional direct_response_body_; - const Optional direct_response_file_; + std::string direct_response_body_; }; /** diff --git a/source/common/router/config_utility.cc b/source/common/router/config_utility.cc index c6ce5845a628d..6c4c43ce4f7fd 100644 --- a/source/common/router/config_utility.cc +++ b/source/common/router/config_utility.cc @@ -5,6 +5,7 @@ #include #include "common/common/assert.h" +#include "common/filesystem/filesystem_impl.h" namespace Envoy { namespace Router { @@ -98,31 +99,36 @@ Optional ConfigUtility::parseDirectResponseCode(const envoy::api::v2 return Optional(); } -Optional ConfigUtility::parseDirectResponseBody(const envoy::api::v2::Route& route) { - if (route.has_direct_response() && route.direct_response().has_body()) { - const auto& body = route.direct_response().body(); - std::string inline_bytes = body.inline_bytes(); - if (!inline_bytes.empty()) { - return inline_bytes; +std::string ConfigUtility::parseDirectResponseBody(const envoy::api::v2::Route& route) { + if (!route.has_direct_response() || !route.direct_response().has_body()) { + return EMPTY_STRING; + } + const auto& body = route.direct_response().body(); + std::string filename = body.filename(); + if (!filename.empty()) { + static const ssize_t kMaxFileSize = 4096; + if (!Filesystem::fileExists(filename)) { + throw EnvoyException(fmt::format("response body file {} does not exist", filename)); } - std::string inline_string = body.inline_string(); - if (!inline_string.empty()) { - return inline_string; + ssize_t size = Filesystem::fileSize(filename); + if (size < 0) { + throw EnvoyException(fmt::format("cannot determine size of response body file {}", filename)); } - } - return Optional(); -} - -Optional -ConfigUtility::parseDirectResponseFilename(const envoy::api::v2::Route& route) { - if (route.has_direct_response() && route.direct_response().has_body()) { - const auto& body = route.direct_response().body(); - std::string filename = body.filename(); - if (!filename.empty()) { - return filename; + if (size > kMaxFileSize) { + throw EnvoyException(fmt::format("response body file {} size is {} bytes; maximum is {}", + filename, kMaxFileSize)); } + return Filesystem::fileReadToEnd(filename); + } + std::string inline_bytes = body.inline_bytes(); + if (!inline_bytes.empty()) { + return inline_bytes; + } + std::string inline_string = body.inline_string(); + if (!inline_string.empty()) { + return inline_string; } - return Optional(); + return EMPTY_STRING; } Http::Code ConfigUtility::parseClusterNotFoundResponseCode( diff --git a/source/common/router/config_utility.h b/source/common/router/config_utility.h index 9130f9725b765..e6cf9edd97f5c 100644 --- a/source/common/router/config_utility.h +++ b/source/common/router/config_utility.h @@ -117,20 +117,12 @@ class ConfigUtility { /** * Returns the content of the response body to send with direct responses from a route. * @param route supplies the Route configuration. - * @return Optional the response body in 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 parseDirectResponseBody(const envoy::api::v2::Route& route); - - /** - * Returns the pathname of the file containing the response body to send with direct - * responses from a route. - * @param route supplies the Route configuration. - * @return Optional the response body filename in the route's direct_response - * if specified, or an empty Option otherwise. + * @return Optional the response body provided inline in the route's + * direct_response if specified, or the contents of the file named in the + * route's direct_response if specified, or an empty string otherwise. + * @throw EnvoyException if the route configuration contains an error. */ - static Optional parseDirectResponseFilename(const envoy::api::v2::Route& route); + static std::string parseDirectResponseBody(const envoy::api::v2::Route& route); /** * Returns the HTTP Status Code enum parsed from proto. diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 93bc634a328f7..3573827c64824 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -225,16 +225,6 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e if (inline_body.valid()) { body = inline_body.value(); } - Optional path = route_->directResponseEntry()->responseBodyFilename(); - if (path.valid()) { - const ssize_t kMaxFileSize = 4096; - ssize_t file_size = Filesystem::fileSize(path.value()); - if (file_size < 0 || file_size > kMaxFileSize) { - sendLocalReply(Http::Code::InternalServerError, "", false); - return Http::FilterHeadersStatus::StopIteration; - } - body = Filesystem::fileReadToEnd(path.value()); - } sendLocalReply(route_->directResponseEntry()->responseCode(), body, false); // TODO(brian-pane) support sending response_headers_to_add. return Http::FilterHeadersStatus::StopIteration; diff --git a/test/common/router/BUILD b/test/common/router/BUILD index 67cf9f34afa10..80d924b43f42b 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -20,6 +20,7 @@ envoy_cc_test( "//source/common/router:config_lib", "//test/mocks/runtime:runtime_mocks", "//test/mocks/upstream:upstream_mocks", + "//test/test_common:environment_lib", "//test/test_common:utility_lib", ], ) diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 74fab81775c50..d7df7ceaaae7e 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -15,6 +15,7 @@ #include "test/mocks/runtime/mocks.h" #include "test/mocks/upstream/mocks.h" +#include "test/test_common/environment.h" #include "test/test_common/printers.h" #include "test/test_common/utility.h" @@ -2188,6 +2189,9 @@ TEST(RouteMatcherTest, DirectResponse) { } )EOF"; + const auto pathname = + TestEnvironment::writeStringToFileForTest("direct_response_body", "Example text 3"); + // A superset of v1_json, with API v2 direct-response configuration added. static const std::string v2_yaml = R"EOF( name: foo @@ -2230,7 +2234,8 @@ name: foo - match: { prefix: /static } direct_response: status: 200 - body: { filename: /etc/envoy/message } + body: { filename: )EOF" + pathname + + R"EOF(} - match: { prefix: / } route: { cluster: www2 } )EOF"; @@ -2280,29 +2285,26 @@ name: foo Http::TestHeaderMapImpl headers = genRedirectHeaders("direct.example.com", "/gone", true, false); EXPECT_EQ(Http::Code::Gone, config.route(headers, 0)->directResponseEntry()->responseCode()); - EXPECT_EQ("Example text 1", - config.route(headers, 0)->directResponseEntry()->responseBody().value()); + EXPECT_EQ("Example text 1", config.route(headers, 0)->directResponseEntry()->responseBody()); } { Http::TestHeaderMapImpl headers = genRedirectHeaders("direct.example.com", "/error", true, false); EXPECT_EQ(Http::Code::InternalServerError, config.route(headers, 0)->directResponseEntry()->responseCode()); - EXPECT_EQ("Example text 2", - config.route(headers, 0)->directResponseEntry()->responseBody().value()); + EXPECT_EQ("Example text 2", config.route(headers, 0)->directResponseEntry()->responseBody()); } { Http::TestHeaderMapImpl headers = genRedirectHeaders("direct.example.com", "/no_body", true, false); EXPECT_EQ(Http::Code::OK, config.route(headers, 0)->directResponseEntry()->responseCode()); - EXPECT_FALSE(config.route(headers, 0)->directResponseEntry()->responseBody().valid()); + EXPECT_TRUE(config.route(headers, 0)->directResponseEntry()->responseBody().empty()); } { Http::TestHeaderMapImpl headers = genRedirectHeaders("direct.example.com", "/static", true, false); EXPECT_EQ(Http::Code::OK, config.route(headers, 0)->directResponseEntry()->responseCode()); - EXPECT_EQ("/etc/envoy/message", - config.route(headers, 0)->directResponseEntry()->responseBodyFilename().value()); + EXPECT_EQ("Example text 3", config.route(headers, 0)->directResponseEntry()->responseBody()); } { Http::TestHeaderMapImpl headers = diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 622017212dc18..ee92248a49d47 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -3,6 +3,7 @@ #include #include "common/buffer/buffer_impl.h" +#include "common/common/empty_string.h" #include "common/network/utility.h" #include "common/router/router.h" #include "common/upstream/upstream_impl.h" @@ -1556,6 +1557,7 @@ TEST_F(RouterTest, RedirectFound) { TEST_F(RouterTest, DirectResponse) { NiceMock direct_response; EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); + EXPECT_CALL(direct_response, responseBody()).WillRepeatedly(ReturnRef(EMPTY_STRING)); EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); Http::TestHeaderMapImpl response_headers{{":status", "200"}}; @@ -1570,8 +1572,8 @@ TEST_F(RouterTest, DirectResponse) { TEST_F(RouterTest, DirectResponseWithBody) { NiceMock direct_response; EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); - EXPECT_CALL(direct_response, responseBody()) - .WillRepeatedly(Return(Optional("static response"))); + const std::string response_body("static response"); + EXPECT_CALL(direct_response, responseBody()).WillRepeatedly(ReturnRef(response_body)); EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); Http::TestHeaderMapImpl response_headers{ @@ -1585,61 +1587,6 @@ TEST_F(RouterTest, DirectResponseWithBody) { EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value()); } -TEST_F(RouterTest, DirectResponseWithBodyFromFile) { - { - auto pathname = TestEnvironment::writeStringToFileForTest("response_body", - "content to deliver from a file"); - NiceMock direct_response; - EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); - EXPECT_CALL(direct_response, responseBodyFilename()) - .WillRepeatedly(Return(Optional(pathname))); - EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); - Http::TestHeaderMapImpl response_headers{ - {":status", "200"}, {"content-length", "30"}, {"content-type", "text/plain"}}; - EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false)); - EXPECT_CALL(callbacks_, encodeData(_, true)); - Http::TestHeaderMapImpl headers; - HttpTestUtility::addDefaultHeaders(headers); - router_.decodeHeaders(headers, true); - EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); - EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value()); - config_.stats_.rq_direct_response_.reset(); - } - { - // Nonexistent file, should return a 500. - NiceMock direct_response; - EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); - EXPECT_CALL(direct_response, responseBodyFilename()) - .WillRepeatedly(Return(Optional("nonexistent file"))); - EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); - Http::TestHeaderMapImpl response_headers{{":status", "500"}}; - EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); - Http::TestHeaderMapImpl headers; - HttpTestUtility::addDefaultHeaders(headers); - router_.decodeHeaders(headers, true); - EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); - EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value()); - config_.stats_.rq_direct_response_.reset(); - } - { - // File too big, should return a 500. - auto pathname = - TestEnvironment::writeStringToFileForTest("response_body", std::string(4097, '*')); - NiceMock direct_response; - EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); - EXPECT_CALL(direct_response, responseBodyFilename()) - .WillRepeatedly(Return(Optional(pathname))); - EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); - Http::TestHeaderMapImpl response_headers{{":status", "500"}}; - EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); - Http::TestHeaderMapImpl headers; - HttpTestUtility::addDefaultHeaders(headers); - router_.decodeHeaders(headers, true); - EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); - EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value()); - } -} - TEST(RouterFilterUtilityTest, finalTimeout) { { NiceMock route; diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 447a2bad81c49..8f6a64a3800c4 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -35,8 +35,7 @@ class MockDirectResponseEntry : public DirectResponseEntry { // DirectResponseEntry MOCK_CONST_METHOD1(newPath, std::string(const Http::HeaderMap& headers)); MOCK_CONST_METHOD0(responseCode, Http::Code()); - MOCK_CONST_METHOD0(responseBody, Optional()); - MOCK_CONST_METHOD0(responseBodyFilename, Optional()); + MOCK_CONST_METHOD0(responseBody, const std::string&()); }; class TestCorsPolicy : public CorsPolicy { From b0fe87b6f5e3c59d9814ad196aae042343a4f39b Mon Sep 17 00:00:00 2001 From: Brian Pane Date: Wed, 24 Jan 2018 17:40:42 +0000 Subject: [PATCH 3/4] Add integration test, fix ASAN/protobuf false positive, and clean up the code a bit Signed-off-by: Brian Pane --- source/common/router/router.cc | 8 ++------ test/integration/http_integration.cc | 9 +++++++-- tools/bazel.rc | 2 ++ 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 3573827c64824..f5209a6659b76 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -220,12 +220,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e return Http::FilterHeadersStatus::StopIteration; } config_.stats_.rq_direct_response_.inc(); - std::string body; - Optional inline_body = route_->directResponseEntry()->responseBody(); - if (inline_body.valid()) { - body = inline_body.value(); - } - sendLocalReply(route_->directResponseEntry()->responseCode(), body, false); + sendLocalReply(route_->directResponseEntry()->responseCode(), + route_->directResponseEntry()->responseBody(), false); // TODO(brian-pane) support sending response_headers_to_add. return Http::FilterHeadersStatus::StopIteration; } diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 554b0b34b37d0..d7f6ea65c1d2a 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -332,9 +332,11 @@ void HttpIntegrationTest::testRouterRedirect() { } void HttpIntegrationTest::testRouterDirectResponse() { + const std::string body = "Response body"; + const std::string file_path = TestEnvironment::writeStringToFileForTest("test_envoy", body); static const std::string domain("direct.example.com"); static const std::string prefix("/"); - static const Http::Code status(Http::Code::NoContent); + static const Http::Code status(Http::Code::OK); config_helper_.addConfigModifier( [&](envoy::api::v2::filter::network::HttpConnectionManager& hcm) -> void { auto* route_config = hcm.mutable_route_config(); @@ -344,13 +346,16 @@ void HttpIntegrationTest::testRouterDirectResponse() { virtual_host->add_routes()->mutable_match()->set_prefix(prefix); virtual_host->mutable_routes(0)->mutable_direct_response()->set_status( static_cast(status)); + virtual_host->mutable_routes(0)->mutable_direct_response()->mutable_body()->set_filename( + file_path); }); initialize(); BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest( lookupPort("http"), "GET", "/", "", downstream_protocol_, version_, "direct.example.com"); EXPECT_TRUE(response->complete()); - EXPECT_STREQ("204", response->headers().Status()->value().c_str()); + EXPECT_STREQ("200", response->headers().Status()->value().c_str()); + EXPECT_EQ(body, response->body()); } // Add a health check filter and verify correct behavior when draining. diff --git a/tools/bazel.rc b/tools/bazel.rc index 16a50c93a4b26..4a14b309faf36 100644 --- a/tools/bazel.rc +++ b/tools/bazel.rc @@ -13,6 +13,7 @@ build:asan --define tcmalloc=disabled build:asan --build_tag_filters=-no_asan build:asan --test_tag_filters=-no_asan build:asan --define signal_trace=disabled +build:asan --copt -DADDRESS_SANITIZER=1 # Clang 5.0 ASAN build:clang-asan --define ENVOY_CONFIG_ASAN=1 @@ -27,6 +28,7 @@ build:clang-asan --define tcmalloc=disabled build:clang-asan --build_tag_filters=-no_asan build:clang-asan --test_tag_filters=-no_asan build:clang-asan --define signal_trace=disabled +build:clang-asan --copt -DADDRESS_SANITIZER=1 build:clang-asan --test_env=ASAN_SYMBOLIZER_PATH # Clang 5.0 TSAN From 0094f29491e1ee655d6093bd21a73ad948587886 Mon Sep 17 00:00:00 2001 From: Brian Pane Date: Thu, 25 Jan 2018 17:43:01 +0000 Subject: [PATCH 4/4] Small code cleanups Signed-off-by: Brian Pane --- source/common/router/config_utility.cc | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/source/common/router/config_utility.cc b/source/common/router/config_utility.cc index 6c4c43ce4f7fd..fb40ed27d9430 100644 --- a/source/common/router/config_utility.cc +++ b/source/common/router/config_utility.cc @@ -104,9 +104,9 @@ std::string ConfigUtility::parseDirectResponseBody(const envoy::api::v2::Route& return EMPTY_STRING; } const auto& body = route.direct_response().body(); - std::string filename = body.filename(); + const std::string filename = body.filename(); if (!filename.empty()) { - static const ssize_t kMaxFileSize = 4096; + static const ssize_t MaxFileSize = 4096; if (!Filesystem::fileExists(filename)) { throw EnvoyException(fmt::format("response body file {} does not exist", filename)); } @@ -114,21 +114,17 @@ std::string ConfigUtility::parseDirectResponseBody(const envoy::api::v2::Route& if (size < 0) { throw EnvoyException(fmt::format("cannot determine size of response body file {}", filename)); } - if (size > kMaxFileSize) { + if (size > MaxFileSize) { throw EnvoyException(fmt::format("response body file {} size is {} bytes; maximum is {}", - filename, kMaxFileSize)); + filename, MaxFileSize)); } return Filesystem::fileReadToEnd(filename); } - std::string inline_bytes = body.inline_bytes(); + const std::string inline_bytes = body.inline_bytes(); if (!inline_bytes.empty()) { return inline_bytes; } - std::string inline_string = body.inline_string(); - if (!inline_string.empty()) { - return inline_string; - } - return EMPTY_STRING; + return body.inline_string(); } Http::Code ConfigUtility::parseClusterNotFoundResponseCode(