-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Send optional entity-body with direct responses #2429
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
Changes from 2 commits
9aa8efd
7a6404f
b0fe87b
0094f29
f7c9678
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| #include <vector> | ||
|
|
||
| #include "common/common/assert.h" | ||
| #include "common/filesystem/filesystem_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Router { | ||
|
|
@@ -98,6 +99,38 @@ Optional<Http::Code> ConfigUtility::parseDirectResponseCode(const envoy::api::v2 | |
| return Optional<Http::Code>(); | ||
| } | ||
|
|
||
| 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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: in general we don't prefix constants with 'k'. |
||
| if (!Filesystem::fileExists(filename)) { | ||
| throw EnvoyException(fmt::format("response body file {} does not exist", filename)); | ||
| } | ||
| ssize_t size = Filesystem::fileSize(filename); | ||
| if (size < 0) { | ||
| throw EnvoyException(fmt::format("cannot determine size of response body file {}", 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(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: you don't really need the extra logic below here. At this point you can just return |
||
| if (!inline_string.empty()) { | ||
| return inline_string; | ||
| } | ||
| return EMPTY_STRING; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for being late to the party, but:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The exact number was chosen by @brian-pane. He can comment. It's probably arbitrary. The main idea here is to provide some sanity check to avoid people from streaming huge files which won't reliably work without a lot of effort around buffering, flow control, etc. We should probably apply the same limit whether inline or by file I agree. @brian-pane can you do a follow up?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @PiotrSikora the idea of using a fixed limit arose from a Slack discussion in #envoy-dev on Jan 5th. My original plan was to use sendfile, but Matt had (quite reasonable) concerns about how much complexity that would add to the proxy core, so we settled on reading the file into memory but limiting its size.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, PR 2458 now contains a fix to apply the same 4KB limit to both inline and file-sourced response bodies. |
||
|
|
||
| Http::Code ConfigUtility::parseClusterNotFoundResponseCode( | ||
| const envoy::api::v2::RouteAction::ClusterNotFoundResponseCode& code) { | ||
| switch (code) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,13 @@ 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. const std::string& body = route_->directResponseEntry()->responseBody(); ? |
||
| Optional<std::string> inline_body = route_->directResponseEntry()->responseBody(); | ||
| if (inline_body.valid()) { | ||
| body = inline_body.value(); | ||
| } | ||
| sendLocalReply(route_->directResponseEntry()->responseCode(), body, false); | ||
| // TODO(brian-pane) support sending response_headers_to_add. | ||
| return Http::FilterHeadersStatus::StopIteration; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| #include <string> | ||
|
|
||
| #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" | ||
|
|
@@ -15,6 +16,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 +30,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,8 +1555,9 @@ TEST_F(RouterTest, RedirectFound) { | |
| } | ||
|
|
||
| TEST_F(RouterTest, DirectResponse) { | ||
| MockDirectResponseEntry direct_response; | ||
| NiceMock<MockDirectResponseEntry> 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"}}; | ||
|
|
@@ -1565,6 +1569,24 @@ TEST_F(RouterTest, DirectResponse) { | |
| EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value()); | ||
| } | ||
|
|
||
| TEST_F(RouterTest, DirectResponseWithBody) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we get an integration test for one of these, just to make sure end to end with bodies works smoothly? |
||
| NiceMock<MockDirectResponseEntry> direct_response; | ||
| EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); | ||
| 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{ | ||
| {":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(RouterFilterUtilityTest, finalTimeout) { | ||
| { | ||
| NiceMock<MockRouteEntry> route; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: const here and similar below