router: implement append/override of request/response headers to add (fixes #2003) by zuercher · Pull Request #2025 · envoyproxy/envoy

zuercher · 2017-11-08T19:16:11Z

Modifies the Http::HeaderMap interface and implementation to allow replace semantics for non-static headers. Uses the new functionality to implement the append flag in envoy::v2::api::HeaderValueOption (with the caveat that the default behavior is append, which requires a change to the protobuf comment).

Fixes #2003

Risk Level: Low
The default path (append) is unchanged.

Testing:
Added unit tests with full code coverage over the modified code.

Release Notes:
Note change to default value of HeaderValueOption's append and that header override is now possible.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

…ixes envoyproxy#2003) Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

mattklein123

cool stuff. few comments.

mattklein123 · 2017-11-09T00:06:36Z

include/envoy/http/header_map.h

+   *        the lifetime of any request/response using the string (since a codec may optimize for
+   *        zero copy).
+   */
+  HeaderString& operator=(HeaderString&& rhs);


Do we explicitly need the assignment operator? Would prefer not having compiler surprises here. Seems like we can basically just use the move function below? (Also, not sure comment about lifetime is accurate, the impl seems to handle all cases).

mattklein123 · 2017-11-09T00:07:04Z

include/envoy/http/header_map.h


+  /**
+   * Set the header value by moving data into it.
+   */


setMove for consistency?

mattklein123 · 2017-11-09T00:07:58Z

source/common/http/header_map_impl.cc

  string_length_ = ref_value.size();
 }

+HeaderString& HeaderString::operator=(HeaderString&& move_value) {


This is basically the same code as the move constructor. Can we put code in common utility function and just call freeDynamic() followed by that function?

mattklein123 · 2017-11-09T00:10:18Z

source/common/http/header_map_impl.h


+  /**
+   * Set a header via full move.
+   */


Does this need to be public?

No, in which case it doesn't even need to exist.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

mattklein123 · 2017-11-09T19:18:53Z

include/envoy/http/header_map.h


-  /**
-   * Set the header value by moving data into it.
-   */


FYI I didn't mean for you to revert all the perf stuff you did. I was just saying to remove HeaderString& operator=, rename this to setMove(), and then just call setMove() wherever you need it. I'm fine either way though. LMK.

Apparently I didn't ctrl-c that push fast enough.

Anyway, I decided this is easier to understand and arguably more correct (the rule is route headers before vhost headers before global headers, so they should come out in the right order even in the face of set-after-append).

It's slightly slower in that it always adds a new HeaderEntry, but I think that's probably swamped by having to iterate over the headers to remove duplicate keys.

That said, we could have both if I undid this last commit and then changed the updateByKey search to iterate over the headers in reverse order.

OK sounds good. The upside I can see about the update code that you had is it would update in place and not effect ordering. What you have now will move it to the back of the list which is arguably not as good. I don't think it matters that much though.

I'm fine either way, just LMK if you are going to change again.

mattklein123 · 2017-11-09T21:42:43Z

include/envoy/http/header_map.h

+   *
+   * @param key specifies the name of the header to set; it WILL be copied.
+   * @param value specifies the value of the header to set; it WILL be copied.
+   */


Are the two setCopy() calls added used anywhere?

The string version is used in TestHeaderMapImpl to provide a convenience function for tests.

Couldn't the test version just do remove followed by add? (I guess now that you removed update, couldn't it all just work that way)?

It seems to me that having explicit method that do this work allows for future changes to optimize the set path, even if it's just remove/add under the covers now. But if you want, I can yank out all the set methods and move the remove calls into the router.

You already wrote the optimized code but then you deleted it. 😉 I don't care either way. I mostly want to make sure that if we have functions they are actually used. If you want to keep the ones that are called but for example remove the setCopy stuff that isn't used other than test (and implement that in terms of remove/add) that's fine. LMK.

Ok. I'll remove the unused ones.

htuch

LGTM, thanks for this additional header mutation support.

htuch · 2017-11-13T02:44:05Z

include/envoy/http/header_map.h

+   *
+   * @param key specifies the name of the header to set; it WILL NOT be copied.
+   * @param value specifies the value of the header to set; it WILL NOT be copied.
+   */


It might be less verbose to just make this a bool arg on the existing add*, but I don't think it's a big deal.

htuch · 2017-11-13T02:50:04Z

source/common/router/config_impl.cc

+namespace {
+
+Router::HeaderAddition
+make_header_to_add(const envoy::api::v2::HeaderValueOption& header_value_option) {


Nit: makeHeaderToAdd(..).

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

htuch · 2017-11-15T18:03:26Z

test/common/router/config_impl_test.cc

              ContainerEq(config.responseHeadersToRemove()));
 }

+TEST(RouteMatcherTest, TestAddRemoveReqRespHeadersWithAppendFalse) {


Nit: I like one liners above tests explaining what they do.

mattklein123

LGTM other than tiny nits.

mattklein123 · 2017-11-15T18:08:28Z

source/common/router/req_header_formatter.h


  virtual const std::string format(const Envoy::AccessLog::RequestInfo& request_info) const PURE;
+
+  /*


Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

* Improve performance by removing MD5 for check cache keys (envoyproxy#2002) * Improve performance by removing MD5 for check cache keys Signed-off-by: Wayne Zhang <qiwzhang@google.com> * not to allocate memory from stack Signed-off-by: Wayne Zhang <qiwzhang@google.com> * Make debug string readable Signed-off-by: Wayne Zhang <qiwzhang@google.com> * alts: remove ALTS (envoyproxy#2003) Signed-off-by: Lizan Zhou <lizan@tetrate.io> * Use std::hash for check cache. (envoyproxy#2009) Signed-off-by: Wayne Zhang <qiwzhang@google.com> * Remove tests to compare signature values (envoyproxy#2015) Signed-off-by: Wayne Zhang <qiwzhang@google.com> * update sample envoy config to latest version (envoyproxy#2016) * Add a new TCP cluster rewrite filter (envoyproxy#2017) * Add a new TCP cluster rewrite filter This commit adds a new TCP cluster rewrite filter which allows users to rewrite TCP cluster names obtained via TLS SNI by matching via regex configuration. Signed-off-by: Venil Noronha <veniln@vmware.com> * Make TCP cluster rewrite stackable on SNI filter This commit updates the TCP Cluster Rewrite filter to be stackable on the SNI Cluster filter. Signed-off-by: Venil Noronha <veniln@vmware.com> * Update TCP Cluster Rewrite filter name (envoyproxy#2019) This commit updates the TCP Cluster Rewrite filter name to envoy.filters.network.tcp_cluster_rewrite. Signed-off-by: Venil Noronha <veniln@vmware.com> * Enable TCP Cluster Rewrite filter registration (envoyproxy#2021) This commit enables the static registration of the TCP Cluster Rewrite filter by updating the build configuration. Signed-off-by: Venil Noronha <veniln@vmware.com> * Update Envoy SHA to 4ef8562 (envoyproxy#2023) Envoy /server_info API was inconsistent intermittently causing errors on a Proxy update on Istio. This update will bring in the API fix to Istio. Signed-off-by: Venil Noronha <veniln@vmware.com> * add proxy postsubmit periodic (envoyproxy#2025)

* Improve performance by removing MD5 for check cache keys (envoyproxy#2002) * Improve performance by removing MD5 for check cache keys Signed-off-by: Wayne Zhang <qiwzhang@google.com> * not to allocate memory from stack Signed-off-by: Wayne Zhang <qiwzhang@google.com> * Make debug string readable Signed-off-by: Wayne Zhang <qiwzhang@google.com> * alts: remove ALTS (envoyproxy#2003) Signed-off-by: Lizan Zhou <lizan@tetrate.io> * Use std::hash for check cache. (envoyproxy#2009) Signed-off-by: Wayne Zhang <qiwzhang@google.com> * Remove tests to compare signature values (envoyproxy#2015) Signed-off-by: Wayne Zhang <qiwzhang@google.com> * update sample envoy config to latest version (envoyproxy#2016) * Add a new TCP cluster rewrite filter (envoyproxy#2017) * Add a new TCP cluster rewrite filter This commit adds a new TCP cluster rewrite filter which allows users to rewrite TCP cluster names obtained via TLS SNI by matching via regex configuration. Signed-off-by: Venil Noronha <veniln@vmware.com> * Make TCP cluster rewrite stackable on SNI filter This commit updates the TCP Cluster Rewrite filter to be stackable on the SNI Cluster filter. Signed-off-by: Venil Noronha <veniln@vmware.com> * Update TCP Cluster Rewrite filter name (envoyproxy#2019) This commit updates the TCP Cluster Rewrite filter name to envoy.filters.network.tcp_cluster_rewrite. Signed-off-by: Venil Noronha <veniln@vmware.com> * Enable TCP Cluster Rewrite filter registration (envoyproxy#2021) This commit enables the static registration of the TCP Cluster Rewrite filter by updating the build configuration. Signed-off-by: Venil Noronha <veniln@vmware.com> * Update Envoy SHA to 4ef8562 (envoyproxy#2023) Envoy /server_info API was inconsistent intermittently causing errors on a Proxy update on Istio. This update will bring in the API fix to Istio. Signed-off-by: Venil Noronha <veniln@vmware.com> * add proxy postsubmit periodic (envoyproxy#2025) * Update Envoy SHA to c41fa71 (envoyproxy#2029) * Update Envoy SHA Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com> * Fix format. Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com> * bazel: Allow to distdir all dependencies (envoyproxy#2034) To use --distdir option of Bazel (which allows to use previously fetched tarballs instead of downloading dependencies during build), all dependencies should use http instead of git and need to have sha256 sums specified. Signed-off-by: Michal Rostecki <mrostecki@suse.de> * bazel: Remove BoringSSL repository (envoyproxy#2035) Pull request envoyproxy#2002 removed signature calculation which was using BoringSSL as a dependency. BoringSSL is not needed anymore. Signed-off-by: Michal Rostecki <mrostecki@suse.de> * Update Envoy SHA to fcc68f1 (envoyproxy#2037) * Update Envoy SHA to fcc68f1 Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com> * Update SHA256 Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: JP Simard <jp@jpsim.com>

zuercher added 5 commits November 7, 2017 10:49

http: modify HeaderMap to provide replacement semantics

b73e512

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

router: implement append/override of request/response headers to add (f…

6684ab4

…ixes envoyproxy#2003) Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

replace std::tuple with a struct

5816b1e

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

Merge branch 'master' into fix-2003-header-append

4cd59c0

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

fix my lazy merge

b7709cd

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

mattklein123 reviewed Nov 9, 2017

View reviewed changes

ditch set-in-place for header map, remove unused methods

2c8ae3a

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

mattklein123 reviewed Nov 9, 2017

View reviewed changes

htuch reviewed Nov 13, 2017

View reviewed changes

zuercher added 4 commits November 13, 2017 10:11

fix function name

7e21f95

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

Merge branch 'master' into fix-2003-header-append

2cad40f

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

remove unused set methods on HeaderMap

6f46d0b

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

Merge branch 'master' into fix-2003-header-append

4282a17

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

htuch previously approved these changes Nov 15, 2017

View reviewed changes

mattklein123 reviewed Nov 15, 2017

View reviewed changes

fix nits

b092b4d

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher dismissed htuch’s stale review via b092b4d November 15, 2017 18:54

another comment

c634fd2

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

mattklein123 approved these changes Nov 15, 2017

View reviewed changes

mattklein123 merged commit 9f7f8f5 into envoyproxy:master Nov 15, 2017

zuercher deleted the fix-2003-header-append branch December 8, 2017 18:27

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

add proxy postsubmit periodic (envoyproxy#2025)

32e134a

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

fix (#2025)

bc116a5

Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: JP Simard <jp@jpsim.com>

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

fix (#2025)

c4860f9

Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: JP Simard <jp@jpsim.com>


		virtual const std::string format(const Envoy::AccessLog::RequestInfo& request_info) const PURE;

		/*

Conversation

zuercher commented Nov 8, 2017

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!

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!

htuch 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants