New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
http: config options for adding custom request headers #630
Conversation
|
@ccaraman please do first pass review. |
| @@ -28,6 +28,12 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea | |||
| const Router::Config& route_config, | |||
| Runtime::RandomGenerator& random, | |||
| Runtime::Loader& runtime) { | |||
| // Add user-specified headers to the request before scrubbing the request headers. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question: Why do we want to add these before scrubbing the headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| @@ -279,6 +284,23 @@ TEST(RouteMatcherTest, TestRoutes) { | |||
| Http::LowerCaseString("x-envoy-virtual-cluster")}), | |||
| ContainerEq(config.responseHeadersToRemove())); | |||
|
|
|||
| // Request header manipulation testing | |||
| { | |||
| // At route_config level | |||
There was a problem hiding this comment.
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)
| @@ -279,6 +284,23 @@ TEST(RouteMatcherTest, TestRoutes) { | |||
| Http::LowerCaseString("x-envoy-virtual-cluster")}), | |||
| ContainerEq(config.responseHeadersToRemove())); | |||
|
|
|||
| // Request header manipulation testing | |||
There was a problem hiding this comment.
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.
| {Http::LowerCaseString("Connection"), "Keep-Alive"}})), | ||
| ContainerEq(config.requestHeadersToAdd())); | ||
|
|
||
| Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/host/rewrite/me", "GET"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
fixes #539 |
source/common/router/config_impl.cc
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
|
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
|
|
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). |
a10a6ce
to
0b06326
Compare
|
@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? |
96605f3
to
47cc088
Compare
| Adding custom request headers | ||
| ----------------------------- | ||
|
|
||
| Custom request headers can be added to a request that matches a specific route. The headers are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can there only be one place that describes this order of precedence? That way you don't have to duplicate the wording.
| {"key": "header2", "value": "value2"} | ||
| ] | ||
|
|
||
| *Note:* In the presence of duplicate header keys, headers specified at the global |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| {"key": "header2", "value": "value2"} | ||
| ] | ||
|
|
||
| *Note:* In the presence of duplicate header keys, only the first header is taken into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add link to documentation that mentions the order of precedence.
| @@ -533,8 +564,6 @@ VirtualHostImpl::virtualClusterFromEntries(const Http::HeaderMap& headers) const | |||
|
|
|||
| ConfigImpl::ConfigImpl(const Json::Object& config, Runtime::Loader& runtime, | |||
| Upstream::ClusterManager& cm, bool validate_clusters) { | |||
| route_matcher_.reset(new RouteMatcher(config, runtime, cm, validate_clusters)); | |||
There was a problem hiding this comment.
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?
source/common/router/config_impl.cc
Outdated
| @@ -154,6 +161,20 @@ 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 route-level headers first, followed by virtual host level headers | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incomplete sentence and missing full stop on the next line before the If.
source/common/router/config_impl.cc
Outdated
| @@ -154,6 +161,20 @@ 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 route-level headers first, followed by virtual host level headers | |||
| // and finally connection manager level headers If there are two headers with same | |||
| // name, we consider only the first one, and ignore the rest. | |||
There was a problem hiding this comment.
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.
| "prefix": "/", | ||
| "cluster": "instant-server", | ||
| "timeout_ms": 30000 | ||
| }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line for ]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done & done. Sorry, I rebased and it ended showing up as bunch of new commits.
1f0c986
to
1f38764
Compare
| ] | ||
|
|
||
| *Note:* In the presence of duplicate header keys, headers specified at the | ||
| individual route-level take precedence over those specified at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
source/common/router/config_impl.cc
Outdated
| @@ -154,6 +161,20 @@ 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 { | |||
| // For user-specified request headers, route-level headers of take precedence over | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo "of"
source/common/router/config_impl.cc
Outdated
| 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/global_http_config/global_route_config
source/common/router/config_impl.h
Outdated
| const std::list<std::pair<Http::LowerCaseString, std::string>>& requestHeadersToAdd() const { | ||
| return request_headers_to_add_; | ||
| } | ||
| const ConfigImpl& globalHttpConfig() const { return global_http_config_; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
globalRouteConfig() (just rename this similarly everywhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
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>
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>
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>
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>
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:
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.