router: send response_headers_to_add with direct responses by brian-pane · Pull Request #2458 · envoyproxy/envoy

brian-pane · 2018-01-26T00:25:35Z

Description:
Send any configured response_headers_to_add with direct responses.

Risk Level: Medium

Testing: New integration test included

Docs Changes: envoyproxy/data-plane-api#436

Release Notes: Included in this PR

Fixes: #2315

API Changes: envoyproxy/data-plane-api#393

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

*Description*: Send any configured `response_headers_to_add` with direct responses. *Risk Level*: Medium *Testing*: New integration test included *Docs Changes*: TODO *Release Notes*: Included in this PR *Fixes*: #2315 *API Changes*: envoyproxy/data-plane-api#393 Signed-off-by: Brian Pane <bpane@pinterest.com>

brian-pane · 2018-01-26T00:26:36Z

I still need to write the documentation for this feature. I'll make a doc PR and link to it here.

PiotrSikora · 2018-01-26T00:44:47Z

source/common/router/router.cc

      config_.stats_.rq_redirect_.inc();
-      Http::Utility::sendRedirect(*callbacks_, route_->directResponseEntry()->newPath(headers),
-                                  response_code);
+      Http::Utility::sendRedirect(*callbacks_, direct_response->newPath(headers), response_code);


Is there a reason why 3xx responses don't get extra headers and response body?

That's for backward compatibility. The implementations of the new direct-response feature and the existing redirect feature were merged in PR2335, at the request of the reviewers. Thus, if we were to send the extra headers with 3xx responses, it could change the behavior of users' existing Envoy configurations.

Suggestion here (without full review): I still think we could probably pull the trigger and just do it in all cases and no one would notice, but if we feel that we shouldn't do that, I would put the fact that we are deprecating the old behavior in DEPRECATED.md and put a comment here. We can remove the guard in the next release.

Hmm, sending the headers and response body in all cases would definitely yield a more consistent feature. I'll update this PR to make 3xx work the same way as all other response statuses.

…mit to both file and inline response bodies Signed-off-by: Brian Pane <bpane@pinterest.com>

mattklein123

Generally looks great to me. A few minor comments. Thank you!

mattklein123 · 2018-01-26T22:58:35Z

include/envoy/router/router.h

+  virtual ~ResponseEntry() {}
+
+  /**
+   * Do potentially destructive header transforms on response headers prior to forwarding. For


"For example" ?

mattklein123 · 2018-01-26T23:00:49Z

source/common/router/router.cc

-      return Http::FilterHeadersStatus::StopIteration;
-    }
+  const auto* direct_response = route_->directResponseEntry();
+  const auto& request_headers = headers;


why do you need this variable? Is this just for the capture? can you just capture the headers reference directly?

The lambda has its own function parameter named headers, so this is just an alias to prevent a name collision.

ah OK. FYI you can use C++14 lambda capture expression to define this variable inline in the capture which is a bit cleaner.

mattklein123 · 2018-01-26T23:02:22Z

source/common/router/router.cc

+                                                  bool end_stream) -> void {
+          const auto new_path = direct_response->newPath(request_headers);
+          if (!new_path.empty()) {
+            headers->addCopy(Http::LowerCaseString("location"), new_path);


You can do this using O(1) headers by doing a variant of headers->insertLocation()...

It looks like Location isn't configured as one of the O(1) headers in header_map.h. I suppose I could add it, but it looks like each new header configured for O(1) access adds another pointer field to the HeaderMapImpl ... is it okay to increase the size of that structure?

Ah OK, sorry. It's not a big deal either way. But you can use the constant that is already in Http::Headers and then addReferenceKey

mattklein123 · 2018-01-26T23:03:36Z

test/common/router/config_impl_test.cc

+  EXPECT_EQ(Http::Code::OK, direct_response->responseCode());
+  EXPECT_STREQ("content", direct_response->responseBody().c_str());
+
+  std::string response_body(4097, 'A');


nit: I might split this into a separate test with a more obvious name.

mattklein123 · 2018-01-26T23:03:52Z

test/common/router/config_impl_test.cc

  }
 }

+TEST(RouteConfigurationV2, DirectResponse) {


For new tests please add a // comment with a brief description of what the test does.

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

alyssawilk

Looks good - only two questions on my end.

alyssawilk · 2018-01-29T21:16:04Z

source/common/router/router.cc

-    auto response_code = route_->directResponseEntry()->responseCode();
-    if (response_code >= Http::Code::MultipleChoices && response_code < Http::Code::BadRequest) {
-      config_.stats_.rq_redirect_.inc();
-      Http::Utility::sendRedirect(*callbacks_, route_->directResponseEntry()->newPath(headers),


can we remove sendRedirect with this PR?

alyssawilk · 2018-01-29T21:18:54Z

source/common/router/router.cc

-    sendLocalReply(route_->directResponseEntry()->responseCode(),
-                   route_->directResponseEntry()->responseBody(), false);
-    // TODO(brian-pane) support sending response_headers_to_add.
+    Http::Utility::sendLocalReply(


This is very similar to the existing Filter::sendLocalReply. Could we extend that instead?

Yes, I think I have a way to make that work. Update coming soon.

I removed the overloaded parameter from Http::Filter::sendLocalReply and replaced it with a more general callback hook to modify the headers. That made the couple of existing calls to that method more verbose, but I think it's a net improvement.

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

alyssawilk · 2018-01-30T14:36:30Z

Did you miss one user of the function? It looks like some of the builds are unhappy.
Also you'll need a master merge at some point but no hurry on that one.

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

mattklein123 · 2018-01-30T16:09:37Z

source/common/router/router.cc

+            ->void {
+              const auto new_path = direct_response->newPath(request_headers);
+              if (!new_path.empty()) {
+                response_headers.addCopy(Http::LowerCaseString("location"), new_path);


My previous comment on this still needs action just in case it got lost (switch to addReferenceKey and use the constant in the headers class).

There isn't a constant for location in the headers class:

$ egrep -ir location include/envoy/http $ echo $? 1 $

Oh, wait you mean the other headers class, the one in source/common/http.

Yes, sorry, was going to provide you link. Basically just look at how it's being used in the other redirect code.

…eaders/2315 Conflicts: RAW_RELEASE_NOTES.md source/common/router/config_utility.cc Signed-off-by: Brian Pane <bpane@pinterest.com>

alyssawilk

Thanks for the refactor and adding comments! LGTM modulo one final nit.

alyssawilk · 2018-01-30T18:11:33Z

source/common/router/router.h

+   * @param code supplies the HTTP status code.
+   * @param body supplies the response body (empty string if no body is needed).
+   * @param add_headers supplies an optional callback function that can add additional
+   *                    headers to the response.


that can alter the headers in the response
since it could also theoretically remove or just change, yeah?

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

mattklein123

very nice, thanks.

* add usage of stackdriver logger and test * add stat prefix * add metrics and only export at server * remove timeseries.go * fix test due to merge conflict

Signed-off-by: JP Simard <jp@jpsim.com>

PiotrSikora reviewed Jan 26, 2018

View reviewed changes

send extra headers and response body with redirects, and apply 4KB li…

81a27d0

…mit to both file and inline response bodies Signed-off-by: Brian Pane <bpane@pinterest.com>

brian-pane mentioned this pull request Jan 26, 2018

Send optional entity-body with direct responses #2429

Merged

mattklein123 reviewed Jan 26, 2018

View reviewed changes

Brian Pane added 2 commits January 27, 2018 00:04

miscellaneous cleanups

fa6b535

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

formatting fix

9b86f7e

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

alyssawilk assigned mattklein123 and alyssawilk Jan 29, 2018

alyssawilk reviewed Jan 29, 2018

View reviewed changes

Brian Pane added 2 commits January 30, 2018 01:44

remove sendRedirect and refactor Filter::sendLocalReply

99b716f

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

generalize sendLocalReply to share some common code

c039235

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

fix a clang compilation error

f8675ca

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

mattklein123 reviewed Jan 30, 2018

View reviewed changes

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

7e61113

…eaders/2315 Conflicts: RAW_RELEASE_NOTES.md source/common/router/config_utility.cc Signed-off-by: Brian Pane <bpane@pinterest.com>

alyssawilk reviewed Jan 30, 2018

View reviewed changes

update comments and use constant for location header

e92be89

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

alyssawilk approved these changes Jan 30, 2018

View reviewed changes

mattklein123 approved these changes Jan 30, 2018

View reviewed changes

mattklein123 merged commit 788faa2 into envoyproxy:master Jan 30, 2018

jpsim pushed a commit that referenced this pull request Nov 28, 2022

Update Envoy (#2458)

8cecd15

Signed-off-by: JP Simard <jp@jpsim.com>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

Update Envoy (#2458)

626fed2

Signed-off-by: JP Simard <jp@jpsim.com>

Conversation

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

Uh oh!

Uh oh!

brian-pane commented Jan 26, 2018

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!

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!

brian-pane Jan 26, 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!

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!

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!

alyssawilk commented Jan 30, 2018

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!

alyssawilk left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brian-pane commented Jan 26, 2018 •

edited

Loading

brian-pane Jan 26, 2018 •

edited

Loading