Send optional entity-body with direct responses by brian-pane · Pull Request #2429 · envoyproxy/envoy

brian-pane · 2018-01-23T04:25:03Z

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

Signed-off-by: Brian Pane bpane@pinterest.com

*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](envoyproxy/data-plane-api#393) Signed-off-by: Brian Pane <bpane@pinterest.com>

zuercher

Looks great modulo the ASAN error.

zuercher · 2018-01-23T19:43:04Z

source/common/router/config_utility.h

+   * Returns the content of the response body to send with direct responses from a route.
+   * @param route supplies the Route configuration.
+   * @return Optional<std::string> the response body in the route's direct_response if specified,
+   *         or the HTTP status code from the route's redirect if specified,


I don't think this line applies.

zuercher · 2018-01-23T20:02:11Z

source/common/router/router.cc

+        sendLocalReply(Http::Code::InternalServerError, "", false);
+        return Http::FilterHeadersStatus::StopIteration;
+      }
+      body = Filesystem::fileReadToEnd(path.value());


This may throw if path is a directory or exists but is not readable. I expect that probably turns into an internal server error elsewhere, but wanted to point it out in case that's not the desired behavior.

I would strongly advise not reading this file during serving. Instead, I would read it into memory during route config load, and then just sent the pre-read buffer. Length sanity checking, etc. can be handled at that point.

@mattklein123 are you proposing to watch for changes to the file (e.g., by adding an inotify fd to an existing epoll loop) and re-read when it changes?

I'm proposing just reading it once at config load time, and documenting very well the behavior. In general, I feel that this should be generally sufficient for the ways that people are likely to use this behavior. (More generally, I would prefer for Envoy to not become a generic web server, and if we go in that direction I feel that it should be done in a new filter and not as part of the router).

I think this is much easier to reason about from an error handling perspective.

My concern is whether this would set a bad precedent by being the first Envoy feature that requires a manual restart or reload to pick up a configuration change. Or do we already have that issue with TLS cert and key files?

There are ways to workaround this, for example to rename the file as part of operations, etc. IMO this is an OK compromise for this feature as long as it is well documented. We could add watching in the future as a separate feature if desired.

If we want to move forward w/ this implementation we definitely must catch exceptions and handle, as well as understand potential perf issues (though the file will likely be in cache).

Sounds good, I'll go with the simple read-at-startup approach for now.

I think read-at-startup is fine for this feature, brian you can ignore the rest of this :-P

That said, do we have a plan for files for v2 in general? This could be enabling pushing files via RPC, some config command which says "reload files and tell me if config is valid", disallowing files when you're getting live config or even documenting "rename rather than change contents of your files or you will shoot yourself in the foot". @htuch

@alyssawilk in general I think we do not currently have a great plan/roadmap for files. This is exemplified in #2241. IMO if we expect Envoy to really start heavily using files in substantial production deployments we need to put a lot more thought into the entire situation (e.g., there are definitely bugs in the current code where if someone deletes an xDS file after initial error checking Envoy will probably crash). We should probably track in a dedicated ticket.

Off the top of my head, I think an RPC to push file changes to Envoy would be a good thing. It would solve all the atomicity problems associated with people using cp rather than mv. In addition, if there were a RESTful push interface, it could provide a simpler alternative to the long-poll JSON-REST pull API.

And it would be nice to have a way to push TLS certificates and keys to Envoy without having to store them in a filesystem on the local host.

alyssawilk

Great addition, thanks!

alyssawilk · 2018-01-23T20:13:18Z

source/common/router/config_impl.h

  // Router::DirectResponseEntry
  std::string newPath(const Http::HeaderMap& headers) const override;
  Http::Code responseCode() const override { return Http::Code::MovedPermanently; }
+  Optional<std::string> responseBody() const override { return Optional<std::string>(); }


Any reason to not kill off the optional and insead return EMPTY_STRING by default? Especially as on config parsing we return an empty optional if the body is empty in any case.

alyssawilk · 2018-01-23T20:16:49Z

source/common/router/config_impl.cc

-      direct_response_code_(ConfigUtility::parseDirectResponseCode(route)) {
+      direct_response_code_(ConfigUtility::parseDirectResponseCode(route)),
+      direct_response_body_(ConfigUtility::parseDirectResponseBody(route)),
+      direct_response_file_(ConfigUtility::parseDirectResponseFilename(route)) {


I'd suggest checking for file existence and readability and throwing an error on failure.

alyssawilk · 2018-01-23T20:18:39Z

source/common/router/router.cc

+    if (path.valid()) {
+      const ssize_t kMaxFileSize = 4096;
+      ssize_t file_size = Filesystem::fileSize(path.value());
+      if (file_size < 0 || file_size > kMaxFileSize) {


While file size can change (so totally check here) it's probably worth best-effort validation of file size at config load time. Also is the limitation documented anywhere?

I'm planning to add documentation for the feature with a follow-up PR that adds the one remaining part of the implementation: support for sending the response_headers_to_add along with direct responses like these.

alyssawilk · 2018-01-23T20:28:24Z

test/common/router/router_test.cc

  EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value());
 }

+TEST_F(RouterTest, DirectResponseWithBody) {


Can we get an integration test for one of these, just to make sure end to end with bodies works smoothly?

Signed-off-by: Brian Pane <bpane@pinterest.com>

brian-pane · 2018-01-24T07:21:11Z

I'll add an integration test and debug the ASAN error tomorrow.

alyssawilk

Looking good! One last nit while you're working on tests

alyssawilk · 2018-01-24T16:44:56Z

source/common/router/router.cc

    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;


const std::string& body = route_->directResponseEntry()->responseBody(); ?

…the code a bit Signed-off-by: Brian Pane <bpane@pinterest.com>

alyssawilk

LGTM modulo the asan change - we can get a speedy review if it's a prereq for the tests passing for this one

alyssawilk · 2018-01-25T16:23:06Z

tools/bazel.rc

 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


this doesn't seem to logically belong with this PR. Do we need it and if so what do you think about splitting it into its own?

Ah, @htuch pointed me to the slack conversation I missed. Yeah, let's split this out and send it his way. We should be able to get both in today.

@alyssawilk @htuch I just posted PR 2452, which contains the google-protobuf ASAN fix in isolation. Once that change lands, I'll merge master into this PR to pick up the fix.

mattklein123

lgtm, few small drive by nits

mattklein123 · 2018-01-25T17:23:38Z

source/common/router/config_utility.cc

+    return EMPTY_STRING;
+  }
+  const auto& body = route.direct_response().body();
+  std::string filename = body.filename();


nit: const here and similar below

mattklein123 · 2018-01-25T17:24:04Z

source/common/router/config_utility.cc

+  const auto& body = route.direct_response().body();
+  std::string filename = body.filename();
+  if (!filename.empty()) {
+    static const ssize_t kMaxFileSize = 4096;


nit: in general we don't prefix constants with 'k'. MaxfileSize is fine.

mattklein123 · 2018-01-25T17:24:41Z

source/common/router/config_utility.cc

+  if (!inline_bytes.empty()) {
+    return inline_bytes;
+  }
+  std::string inline_string = body.inline_string();


nit: you don't really need the extra logic below here. At this point you can just return inline_string.

Signed-off-by: Brian Pane <bpane@pinterest.com>

PiotrSikora · 2018-01-26T02:17:36Z

source/common/router/config_utility.cc

+    return inline_bytes;
+  }
+  return body.inline_string();
+}


Sorry for being late to the party, but:

Why did everybody think that 4KB limit is a good idea?

Iff it's a good idea, then why does it only apply to response body from file, but not to response body inlined in the config?

cc @mattklein123 @alyssawilk

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?

@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.

BTW, PR 2458 now contains a fix to apply the same 4KB limit to both inline and file-sourced response bodies.

* add Stackdriver access logger * address comments * fix test * format * try to remove DEBUG macro to make mac happy * sigh, remove the test * address comment * clean up * add back macos test. add -c opt to circle build args * address comment * format * switch back to request queue * remove deconstructor, flush should happen in RootContext onDone. * clean up

Description: Per our documentation, the following should be true "While breaking on a C++ function, Android Studio should present the source file and highlight the line where the breakpoint hit with all scope information". That feature did not work - I didn't confirm this but it's possible that the functionality was broken when envoyproxy/envoy-mobile#2184 was merged. Anyway, the issue was that https://github.com/envoyproxy/envoy-mobile/blob/d8cd0d96bbd38c4fe4133b3b4322f6a8095e0a68/.bazelrc#L44 config setting was not applied when Envoy Mobile was built for the runs of example apps. This has been fixed by making run configurations that are used by Envoy Mobile example apps specify `--config=dbg` setting explicitly. Risk Level: None Testing: Confirmed that Android Studio opens the right file (and highlights the right line) when EnvoyMobile breakpoint is hit. Docs Changes: N/A Release Notes: N/A Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

mattklein123 assigned zuercher and alyssawilk Jan 23, 2018

zuercher reviewed Jan 23, 2018

View reviewed changes

alyssawilk reviewed Jan 23, 2018

View reviewed changes

Read response body from file only during config load

7a6404f

Signed-off-by: Brian Pane <bpane@pinterest.com>

alyssawilk reviewed Jan 24, 2018

View reviewed changes

Add integration test, fix ASAN/protobuf false positive, and clean up …

b0fe87b

…the code a bit Signed-off-by: Brian Pane <bpane@pinterest.com>

zuercher previously approved these changes Jan 25, 2018

View reviewed changes

alyssawilk reviewed Jan 25, 2018

View reviewed changes

mattklein123 reviewed Jan 25, 2018

View reviewed changes

brian-pane mentioned this pull request Jan 25, 2018

bazel: fix protobuf UBSAN errors #2452

Merged

Brian Pane added 2 commits January 25, 2018 18:31

Small code cleanups

0094f29

Signed-off-by: Brian Pane <bpane@pinterest.com>

Merge remote-tracking branch 'upstream/master' into direct-response/2315

f7c9678

Signed-off-by: Brian Pane <bpane@pinterest.com>

brian-pane dismissed zuercher’s stale review via f7c9678 January 25, 2018 19:05

alyssawilk approved these changes Jan 25, 2018

View reviewed changes

alyssawilk merged commit 921530a into envoyproxy:master Jan 25, 2018

brian-pane deleted the direct-response/2315 branch January 25, 2018 21:59

PiotrSikora reviewed Jan 26, 2018

View reviewed changes

Conversation

brian-pane commented Jan 23, 2018

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brian-pane Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brian-pane commented Jan 24, 2018

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

brian-pane Jan 25, 2018 •

edited

Loading