http: config options for adding custom request headers by rshriram · Pull Request #630 · envoyproxy/envoy

rshriram · 2017-03-27T16:34:30Z

This PR allows the user to insert additional request headers into the HTTP request before it is forwarded by Envoy to the upstream cluster.

There are three levels at which this can be done:

At http connection manager level (across virtual hosts)
At a single virtual host level (across routes)
On a per route basis

These roughly correspond to Nginx's proxy_set_headers directive that can be applied at http/server/location block granularities.

Note that headers cannot be overridden. If duplicate headers are provided in Connection Manager, Virtual host and the route, only the first occurrence of the header (i.e. in the connection manager level) is taken into account.

mattklein123 · 2017-03-27T16:36:47Z

@ccaraman please do first pass review.

ccaraman · 2017-03-27T20:33:26Z

source/common/http/conn_manager_utility.cc

General question: Why do we want to add these before scrubbing the headers?

I was chatting with Matt in Gitter. I am going to rework this a bit. I initially thought we could enforce some sort of rules on what headers the user can/cannot add. But the enforcement logic was messy. So, the decision was to have the code be simple. If users add bad configs (e.g., override authority/connection headers, etc.), the behavior will be undefined. We will mention this in the docs.

ccaraman · 2017-03-27T20:34:11Z

test/common/router/config_impl_test.cc

nit: incomplete sentence(this line and the next)

ccaraman · 2017-03-27T20:47:37Z

test/common/router/config_impl_test.cc

nit: full stop at the end of the sentence.

ccaraman · 2017-03-27T21:04:38Z

test/common/router/config_impl_test.cc

finalizeRequestHeaders doesn't invoke the added logic for request_headers_to_add. Please add a test to https://github.com/lyft/envoy/blob/master/test/common/http/conn_manager_utility_test.cc

rshriram · 2017-03-28T14:28:19Z

fixes #539

mattklein123 · 2017-03-28T22:10:42Z

source/common/router/config_impl.cc

I'm not super thrilled that we do one thing in the connection manager and two things here. I'm wondering if we should move at least adding request headers for the global route level here. (We can leave munging response headers in the connection manager). Thoughts?

rshriram · 2017-03-28T22:36:24Z

I agree. I was looking for a way to get a handle on the global config object so that I can add all three headers here. But I couldn't go beyond the virtual host. If there is already a way to do so, let me know. Otherwise, I can look into adding an interface accordingly.

On Tue, Mar 28, 2017 at 6:10 PM Matt Klein ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In source/common/router/config_impl.cc <#630 (comment)>: > @@ -154,6 +161,17 @@ bool RouteEntryImplBase::matchRoute(const Http::HeaderMap& headers, uint64_t ran const std::string& RouteEntryImplBase::clusterName() const { return cluster_name_; } void RouteEntryImplBase::finalizeRequestHeaders(Http::HeaderMap& headers) const { + // Add user-specified request headers from the vhost and then from the route. I'm not super thrilled that we do one thing in the connection manager and two things here. I'm wondering if we should move at least adding request headers for the global route level here. (We can leave munging response headers in the connection manager). Thoughts? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#630 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH0qd2xhMjV6WtSgINHkLtMP0pH5DWuXks5rqYVngaJpZM4MqhCS> .

-- ~shriram

mattklein123 · 2017-03-28T22:54:55Z

You don't need any interface changes as you are inside config_impl. Just have a reference chain that leads back to the ConfigImpl root (via virtual host impl).

rshriram · 2017-03-30T04:05:03Z

@mattklein123 @ccaraman I have moved the request header addition functionality completely into Config Impl (and removed all public interfaces as well). The hash map semantics of Http::HeaderMap are still unclear. It seems that the class refuses to overwrite headers that are already set -- this causes the test to fail. Any pointers?

ccaraman · 2017-03-31T21:41:08Z

docs/configuration/http_conn_man/route_config/route.rst

Can there only be one place that describes this order of precedence? That way you don't have to duplicate the wording.

ccaraman · 2017-03-31T21:51:01Z

docs/configuration/http_conn_man/route_config/route.rst

I don't agree with the ordering of precedence. The most specific rules (routes) should take precendence over the less specific rules virtual host and vh over HTTP Conn Man route configurations.
The order of

Route

Virtual Host

Route configuration within the HTTP Conn Man Filter.

I believe this to be better because at the HTTP Conn Man layer we have request headers to be added for all requests then particular vhs or routes can set particular more specific header values.

ccaraman · 2017-03-31T21:52:55Z

docs/configuration/http_conn_man/route_config/route_config.rst

Please add link to documentation that mentions the order of precedence.

ccaraman · 2017-03-31T21:56:50Z

source/common/router/config_impl.cc

does this need to be moved?

ccaraman · 2017-03-31T22:41:30Z

source/common/router/config_impl.cc

incomplete sentence and missing full stop on the next line before the If.

ccaraman · 2017-03-31T22:41:45Z

source/common/router/config_impl.cc

nit remove the comma before the and.

ccaraman · 2017-03-31T22:43:28Z

test/common/router/config_impl_test.cc

new line for ]

done & done. Sorry, I rebased and it ended showing up as bunch of new commits.

mattklein123 · 2017-04-03T15:40:57Z

docs/configuration/http_conn_man/route_config/route.rst

+  ]
+
+*Note:* In the presence of duplicate header keys, headers specified at the
+individual route-level take precedence over those specified at 


This is not exactly clear: For headers that are not in the O(1) set, the header will get appended multiple times. So there is precedent, but only in the ordering. This is kind of complicated, and not sure whether you want to try to describe this in the docs, or just rephrase to talk about the order of addition.

mattklein123 · 2017-04-03T15:42:23Z

source/common/router/config_impl.cc

 const std::string& RouteEntryImplBase::clusterName() const { return cluster_name_; }

 void RouteEntryImplBase::finalizeRequestHeaders(Http::HeaderMap& headers) const {
+  // For user-specified request headers, route-level headers of take precedence over


mattklein123 · 2017-04-03T15:43:41Z

source/common/router/config_impl.cc

-    VirtualHostSharedPtr virtual_host(
-        new VirtualHostImpl(*virtual_host_config, runtime, cm, validate_clusters));
+  for (const Json::ObjectPtr& virtual_host_config : json_config.getObjectArray("virtual_hosts")) {
+    VirtualHostSharedPtr virtual_host(new VirtualHostImpl(*virtual_host_config, global_http_config,


s/global_http_config/global_route_config

mattklein123 · 2017-04-03T15:44:15Z

source/common/router/config_impl.h

+  const std::list<std::pair<Http::LowerCaseString, std::string>>& requestHeadersToAdd() const {
+    return request_headers_to_add_;
+  }
+  const ConfigImpl& globalHttpConfig() const { return global_http_config_; }


globalRouteConfig() (just rename this similarly everywhere)

mattklein123 · 2017-04-03T20:37:15Z

This change LGTM. I will let @ccaraman give you final review/approve/merge.

…xy#630) (envoyproxy#630) Signed-off-by: wujie1993 <qq594jj@gmail.com>

Description: this PR updates Envoy Mobile's Envoy ref past the point where Envoy internally uses v3 config. I took the chance to update Envoy Mobile's config and delete deprecated fields. Note that SAN verification changes, and thus it was removed here. I opened an issue tracking #630. Further note that this ref update significantly increased the binary size. It is a priority for the team to investigate and trim the binary ahead of the v0.3 release (noted in #447) Risk Level: med boost to v3 config and deleted deprecated fields. Testing: passing liveliness in CI and locally. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Commit Message** Creating a generic type that includes OIDC field from envoy proxy gateway, and grant_type/aud. Embedding struct into `AWSOIDCExchangeToken` to avoid much refactoring. Reduces repetition when introducing OIDC for other providers --------- Signed-off-by: Aaron Choo <achoo30@bloomberg.net>

rshriram changed the title ~~Config options for setting custom request headers before forwarding (global)~~ Global config options for adding custom request headers Mar 27, 2017

ccaraman reviewed Mar 27, 2017

View reviewed changes

test/common/router/config_impl_test.cc Outdated

Copy link
Copy Markdown

Contributor

ccaraman Mar 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: incomplete sentence(this line and the next)

ccaraman reviewed Mar 27, 2017

View reviewed changes

test/common/router/config_impl_test.cc Outdated

Copy link
Copy Markdown

Contributor

ccaraman Mar 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: full stop at the end of the sentence.

ccaraman reviewed Mar 27, 2017

View reviewed changes

rshriram changed the title ~~Global config options for adding custom request headers~~ config options for adding custom request headers Mar 28, 2017

rshriram changed the title ~~config options for adding custom request headers~~ http: config options for adding custom request headers Mar 28, 2017

mattklein123 reviewed Mar 28, 2017

View reviewed changes

rshriram force-pushed the add_request_headers branch from a10a6ce to 0b06326 Compare March 29, 2017 20:50

rshriram force-pushed the add_request_headers branch from 96605f3 to 47cc088 Compare March 31, 2017 03:54

ccaraman reviewed Mar 31, 2017

View reviewed changes

source/common/router/config_impl.cc Outdated

Copy link
Copy Markdown

Contributor

ccaraman Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be moved?

ccaraman reviewed Mar 31, 2017

View reviewed changes

source/common/router/config_impl.cc Outdated

Copy link
Copy Markdown

Contributor

ccaraman Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit remove the comma before the and.

ccaraman reviewed Mar 31, 2017

View reviewed changes

mattklein123 closed this Apr 1, 2017

mattklein123 reopened this Apr 1, 2017

Shriram Rajagopalan added 6 commits April 2, 2017 22:14

request_headers_to_add in global route_config

c9bd281

allow adding request headers in vhosts and routes

2fda10a

docs

60c8830

request_headers_to_add in async_client

7066968

bug fixes in test

94663a0

remove public interfaces for request_headers_to_add

5b16213

Shriram Rajagopalan added 5 commits April 2, 2017 22:14

move global req header addition into ConfigImpl

43df9d1

rework header manipulation tests

79b941b

remove ability to overwrite request headers

acc56df

addressing PR comments

41c8715

addressing nits on comments

1f38764

rshriram force-pushed the add_request_headers branch from 1f0c986 to 1f38764 Compare April 3, 2017 02:21

mattklein123 reviewed Apr 3, 2017

View reviewed changes

renaming and doc fixes for http req_headers_to_add

dad30f6

ccaraman approved these changes Apr 4, 2017

View reviewed changes

mattklein123 merged commit f6c9e95 into envoyproxy:master Apr 4, 2017

mattklein123 mentioned this pull request Apr 5, 2017

Support adding/overwriting request headers before forwarding request #539

Closed

rshriram deleted the add_request_headers branch August 14, 2017 18:11

wolfguoliang pushed a commit to wolfguoliang/envoy that referenced this pull request Jan 23, 2021

zh-translation: docs/root/api-v3/config/filter/http/http.rst(envoypro…

a332fe3

…xy#630) (envoyproxy#630) Signed-off-by: wujie1993 <qq594jj@gmail.com>

GoVulnBot mentioned this pull request Feb 22, 2022

x/vulndb: potential Go vuln in github.com/envoyproxy/envoy: CVE-2022-21657 golang/vulndb#336

Closed

jba mentioned this pull request Feb 28, 2022

x/vulndb: potential Go vuln in github.com/envoyproxy/envoy: CVE-2022-21657 jba/nested-modules#186

Open

Conversation

rshriram commented Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Mar 27, 2017

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!

ccaraman Mar 27, 2017 • 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!

rshriram commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram commented Mar 28, 2017 via email

Uh oh!

mattklein123 commented Mar 28, 2017

Uh oh!

rshriram commented Mar 30, 2017

Uh oh!

ccaraman Mar 31, 2017 • 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!

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!

mattklein123 commented Apr 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rshriram commented Mar 27, 2017 •

edited

Loading

ccaraman Mar 27, 2017 •

edited

Loading

ccaraman Mar 31, 2017 •

edited

Loading