add response/request header options at route level by derekargueta · Pull Request #3838 · envoyproxy/envoy

derekargueta · 2018-07-11T01:13:04Z

Description:
This adds the ability to specify response_headers_to_* and request_headers_to_add at the route level, for #3520
Risk Level: low
Testing: updated unit tests
Docs Changes: added
Fixes #Issue: #3520

Signed-off-by: Derek Argueta <dereka@pinterest.com>

dio

Hi, @derekargueta thank you for doing this. While the tests are failing (see: #3838 (comment)), I think this PR is on the right direction to provide the fix for the corresponding issue.

We also need to add a release note entry in version_history.rst. And potentially deprecation note if we want to deprecate this setting from route action level.

Some comments:

dio · 2018-07-11T19:10:57Z

api/envoy/api/v2/route/route.proto

    // through the enclosing :ref:`envoy_api_msg_route.RouteAction`.
    // Headers specified at this level are applied before headers from the enclosing
-    // :ref:`envoy_api_msg_route.RouteAction`,
+    // :ref:`envoy_api_msg_route.RouteAction`, :ref:`envoy_api_msg_Route`,


I think we need to change :ref:`envoy_api_msg_Route` to :ref:`envoy_api_msg_route.Route`. Optionally adding Route label should be fine too i.e. :ref:`Router <envoy_api_msg_route.Route>`. The same everywhere else.

dio · 2018-07-11T19:11:51Z

api/envoy/api/v2/route/route.proto

  //
  //   Headers can be specified using *response_headers_to_add* in
-  //   :ref:`envoy_api_msg_RouteConfiguration`.
+  //   :ref:`envoy_api_msg_RouteConfiguration` or :ref:`envoy_msg_api_Route`.


Change this to :ref:`envoy_api_msg_route.Route`.

dio · 2018-07-11T19:22:53Z

source/common/router/config_impl.h

-  const HeaderParser& requestHeaderParser() const { return *request_headers_parser_; };
-  const HeaderParser& responseHeaderParser() const { return *response_headers_parser_; };
+  const HeaderParser& requestHeaderParser() const { return *route_action_request_headers_parser_; };
+  const HeaderParser& responseHeaderParser() const { return *route_action_response_headers_parser_; };


This change for these two getters is reasonable since we used to work on RouteAction level. Do you think we need to have getters for the Route level too?

dio · 2018-07-11T19:22:59Z

source/common/router/config_impl.cc

+      route_action_response_headers_parser_(
+          HeaderParser::configure(route.route().response_headers_to_add(),
+                                  route.route().response_headers_to_remove())),
      request_headers_parser_(HeaderParser::configure(route.route().request_headers_to_add())),


It seems we try to configure parsers with route.route().request_headers_to_add() twice here, hence the relevant integration tests are failed.

Do you want to configure request_headers_parser_ by route.request_headers_to_add() instead? And regarding that, I think the newly added fields to Route (request_headers_to_add, response_headers_to_add, and response_headers_to_remove) needs to be accomodated in the test.

dio · 2018-07-11T19:23:38Z

test/common/router/config_impl_test.cc

 }

-// Validates behavior of request_headers_to_add at router, vhost, and route levels.
+// Validates behavior of request_headers_to_add at router, vhost, and route action levels.


As mentioned above, we need to add tests for the route level too.

dio · 2018-07-11T19:28:02Z

source/common/router/config_impl.cc

@@ -371,6 +376,7 @@ void RouteEntryImplBase::finalizeRequestHeaders(Http::HeaderMap& headers,
  // Append user-specified request headers in the following order: route-level headers,


Modify the comment to add "route action level" to the sequence (route-level headers, route-action-level, ...).

dio · 2018-07-11T19:30:30Z

source/common/router/config_impl.cc


 void RouteEntryImplBase::finalizeResponseHeaders(
    Http::HeaderMap& headers, const RequestInfo::RequestInfo& request_info) const {
+  route_action_response_headers_parser_->evaluateHeaders(headers, request_info);


Probably adding a comment the same fashion with finalizeRequestHeaders here will add clarity.

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta · 2018-07-13T22:20:35Z

@dio thanks for the review, tests coming up!

Signed-off-by: Derek Argueta <dereka@pinterest.com>

dio · 2018-07-14T00:31:34Z

Cool. @derekargueta please let me know when you think we can take another pass.

Reminder: add an entry regarding this change in https://github.com/envoyproxy/envoy/blob/master/docs/root/intro/version_history.rst 🙂

Thank you!

dio · 2018-07-17T12:59:20Z

@derekargueta Sorry, could you fix the DCO check? Thank you.

https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#fixing-dco

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta · 2018-07-20T22:56:02Z

@dio sorry for the sluggish response - I've added a deprecation note to DEPRECATED.md. Please let me know if I should add/change anything else. Thanks!

dio

Thanks, @derekargueta. Just a question and one more small thing. Thanks!

dio · 2018-07-20T23:24:34Z

DEPRECATED.md

  [HttpConnectionManager](https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto)
  instead.
+* Use of `response_headers_to_*` and `request_headers_to_add` are deprecated at the `RouteAction`
+  level. Please use the configuration options at the `Route` level.


In addition to this, please add [deprecated = true] annotation to the relevant fields inside RouteAction. e.g. for

envoy/api/envoy/api/v2/route/route.proto

Line 502 in 598f5c9

repeated core.HeaderValueOption request_headers_to_add = 12;

make it repeated core.HeaderValueOption request_headers_to_add = 12 [deprecated = true]; like what we have here for use_data_plane_proto in rls.proto

dio · 2018-07-21T00:29:57Z

source/common/router/config_impl.h

+  const HeaderParser& requestHeaderParser() const { return *route_action_request_headers_parser_; };
+  const HeaderParser& responseHeaderParser() const {
+    return *route_action_response_headers_parser_;
+  };


Sorry missed this in the previous pass, if we remove response_headers_to_* and request_headers_to_add at RouteAction level (since it is deprecated in this PR) what will be returned in these two getters above? @derekargueta do you know which part of Envoy makes use of these two getters?

From git grep, these 2 getters aren't used. The only reference to them in the code is in source/common/router/config_impl.cc, lines 401-402 and 382-383, and it's for these methods of the same name in the ConfigImpl and VirtualHostImpl classes. Compilation/tests seem fine with these two methods commented out. I only changed these to return route_action_...headers_parser to be consistent, but can update or remove these methods.

Should we just get rid of them?

Signed-off-by: Derek Argueta <dereka@pinterest.com>

dio

Thanks, @derekargueta! LGTM.

mattklein123

LGTM, small request. Thank you!

mattklein123 · 2018-07-22T17:03:13Z

api/envoy/api/v2/route/route.proto

    // through the enclosing :ref:`envoy_api_msg_route.RouteAction`.
    // Headers specified at this level are applied before headers from the enclosing
-    // :ref:`envoy_api_msg_route.RouteAction`,
+    // :ref:`envoy_api_msg_route.RouteAction`, :ref:`envoy_api_msg_route.Route`,


Since RouteAction is deprecated just remove from docs now? (Any similar things need to be fixed?). Basically I would just like to make it as simple as possible to do the follow up deprecation change, so anything you can do now to make it less painful/error prone later would be great.

Is there an annotation to exclude a protobuf field from showing up in the generated docs?

envoy/api/envoy/service/discovery/v2/sds.proto

Line 9 in 1fdb638

// [#not-implemented-hide:] Not configuration. Workaround c++ protobuf issue with importing

htuch

LGTM also, modulo one minor comment.

htuch · 2018-07-23T00:47:32Z

source/common/router/config_impl.h

+  const HeaderParser& requestHeaderParser() const { return *route_action_request_headers_parser_; };
+  const HeaderParser& responseHeaderParser() const {
+    return *route_action_response_headers_parser_;
+  };


Should we just get rid of them?

zuercher

I added some comments on the protobuf docs.

zuercher · 2018-07-23T17:10:03Z

api/envoy/api/v2/route/route.proto

  // Specifies a set of headers that will be added to requests matching this
  // route. Headers specified at this level are applied before headers from the
-  // enclosing :ref:`envoy_api_msg_route.VirtualHost` and
+  // enclosing :ref:`envoy_api_msg_route.Route` and


Shouldn't this say from the enclosing Route, VirtualHost, and RouteConfiguration (with appropriate links)?

zuercher · 2018-07-23T17:11:56Z

api/envoy/api/v2/route/route.proto

-  // :ref:`envoy_api_msg_RouteConfiguration`. For more information, including
-  // details on header value syntax, see the documentation on
+  // headers from the enclosing :ref:`envoy_api_msg_route.VirtualHost`,
+  // :ref:`envoy_api_msg_RouteConfiguration`, and :ref:`envoy_api_msg_route.Route`. For more


Maybe put Route before VirtualHost (preserving the order in which they're applied)?

zuercher · 2018-07-23T17:12:32Z

api/envoy/api/v2/route/route.proto

  //
  //   Headers can be specified using *response_headers_to_add* in
-  //   :ref:`envoy_api_msg_RouteConfiguration`.
+  //   :ref:`envoy_api_msg_RouteConfiguration` or :ref:`envoy_api_msg_route.Route`.


Add VirtualHost to the places where response headers can be added.

Signed-off-by: Derek Argueta <dereka@pinterest.com>

dio · 2018-07-25T01:07:33Z

api/envoy/api/v2/route/route.proto

    // through the enclosing :ref:`envoy_api_msg_route.RouteAction`.
    // Headers specified at this level are applied before headers from the enclosing
-    // :ref:`envoy_api_msg_route.RouteAction`,
+    // :ref:`envoy_api_msg_route.Route`, :ref:`envoy_api_msg_route.Route`,


Duplicated ref:`envoy_api_msg_route.Route` ?

htuch

LGTM modulo remaining nit.

Signed-off-by: Derek Argueta <dereka@pinterest.com>

htuch

Thanks!

While there, move header modification in some tests from route action level (deprecated in envoyproxy#3838) to route level. *Risk Level*: Low *Testing*: bazel test //test/... *Docs Changes*: Added *Release Notes*: Added Fixes envoyproxy#4249. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

While there, move header modification in some tests from route action level (deprecated in #3838) to route level. *Risk Level*: Low *Testing*: bazel test //test/... *Docs Changes*: Added *Release Notes*: Added Fixes #4249. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

add request/response header options at route level

bc8e0b7

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta changed the title ~~Response headers~~ add response/request header options at route level Jul 11, 2018

htuch assigned ggreenway and htuch Jul 11, 2018

htuch requested a review from ggreenway July 11, 2018 02:41

htuch assigned dio and unassigned ggreenway Jul 11, 2018

htuch requested review from dio and removed request for ggreenway July 11, 2018 16:57

dio suggested changes Jul 11, 2018

View reviewed changes

derekargueta added 2 commits July 13, 2018 14:40

code review feedback

17c05ba

Signed-off-by: Derek Argueta <dereka@pinterest.com>

formatting

490599b

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta added 2 commits July 13, 2018 15:56

add test case

a08d3ff

Signed-off-by: Derek Argueta <dereka@pinterest.com>

better test data

39c1d0e

Signed-off-by: Derek Argueta <dereka@pinterest.com>

add version note

a2cf8eb

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta force-pushed the response_headers branch from e03ae0e to a2cf8eb Compare July 20, 2018 22:50

derekargueta added 2 commits July 20, 2018 17:53

Merge remote-tracking branch 'upstream/master' into response_headers

c6f1c9c

add deprecation note

4e85f30

Signed-off-by: Derek Argueta <dereka@pinterest.com>

dio suggested changes Jul 20, 2018

View reviewed changes

dio reviewed Jul 21, 2018

View reviewed changes

add deprecate annotation to API

fb3a7f8

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta force-pushed the response_headers branch from e7ae3b7 to fb3a7f8 Compare July 22, 2018 08:23

dio previously approved these changes Jul 22, 2018

View reviewed changes

mattklein123 reviewed Jul 22, 2018

View reviewed changes

htuch reviewed Jul 23, 2018

View reviewed changes

zuercher reviewed Jul 23, 2018

View reviewed changes

derekargueta added 2 commits July 23, 2018 15:42

remove unused getters

40a383d

Signed-off-by: Derek Argueta <dereka@pinterest.com>

documentation fixes

7c4a305

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta dismissed dio’s stale review via 7c4a305 July 24, 2018 22:47

derekargueta added 2 commits July 24, 2018 15:49

merge master

c4ac263

Signed-off-by: Derek Argueta <dereka@pinterest.com>

hide deprecated fields from docs

247d593

Signed-off-by: Derek Argueta <dereka@pinterest.com>

dio reviewed Jul 25, 2018

View reviewed changes

htuch reviewed Jul 25, 2018

View reviewed changes

fix duplicate route reference

64b1f89

Signed-off-by: Derek Argueta <dereka@pinterest.com>

dio approved these changes Jul 25, 2018

View reviewed changes

htuch approved these changes Jul 25, 2018

View reviewed changes

zuercher approved these changes Jul 25, 2018

View reviewed changes

zuercher merged commit ec0179a into envoyproxy:master Jul 25, 2018

PiotrSikora mentioned this pull request Sep 15, 2018

http: add support for removing request headers. #4431

Merged

mattklein123 mentioned this pull request Oct 5, 2018

[v1.8.0 (Oct 4, 2018) deprecation] Remove features marked deprecated in #3838 #4616

Closed

derekargueta deleted the response_headers branch November 6, 2018 19:28

derekargueta mentioned this pull request Mar 28, 2019

router: remove route-action level header manipulation #6408

Merged

		@@ -371,6 +376,7 @@ void RouteEntryImplBase::finalizeRequestHeaders(Http::HeaderMap& headers,
		// Append user-specified request headers in the following order: route-level headers,

Conversation

derekargueta commented Jul 11, 2018

Uh oh!

dio left a comment • 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!

dio Jul 11, 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!

dio Jul 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekargueta commented Jul 13, 2018

Uh oh!

dio commented Jul 14, 2018

Uh oh!

dio commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekargueta commented Jul 20, 2018

Uh oh!

dio left a comment • 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!

dio Jul 21, 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!

dio left a comment

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!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

htuch left a comment

Choose a reason for hiding this comment

dio left a comment •

edited

Loading

dio Jul 11, 2018 •

edited

Loading

dio Jul 11, 2018 •

edited

Loading

dio commented Jul 17, 2018 •

edited

Loading

dio left a comment •

edited

Loading

dio Jul 21, 2018 •

edited

Loading