Skip to content

add response/request header options at route level #3838

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.

Sign up for GitHub

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

Merged
zuercher merged 14 commits into from
Jul 25, 2018
Merged

add response/request header options at route level #3838

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
bc8e0b7
add request/response header options at route level
derekargueta Jul 11, 2018
17c05ba
code review feedback
derekargueta Jul 13, 2018
490599b
formatting
derekargueta Jul 13, 2018
a08d3ff
add test case
derekargueta Jul 13, 2018
39c1d0e
better test data
derekargueta Jul 13, 2018
a2cf8eb
add version note
derekargueta Jul 16, 2018
c6f1c9c
Merge remote-tracking branch 'upstream/master' into response_headers
derekargueta Jul 20, 2018
4e85f30
add deprecation note
derekargueta Jul 20, 2018
fb3a7f8
add deprecate annotation to API
derekargueta Jul 22, 2018
40a383d
remove unused getters
derekargueta Jul 23, 2018
7c4a305
documentation fixes
derekargueta Jul 24, 2018
c4ac263
merge master
derekargueta Jul 24, 2018
247d593
hide deprecated fields from docs
derekargueta Jul 25, 2018
64b1f89
fix duplicate route reference
derekargueta Jul 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 26 additions & 6 deletions api/envoy/api/v2/route/route.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,26 @@ message Route {
// specific; see the :ref:`HTTP filter documentation <config_http_filters>` for
// if and how it is utilized.
map<string, google.protobuf.Struct> per_filter_config = 8;

// 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
// :ref:`envoy_api_msg_RouteConfiguration`. For more information, including details on
// header value syntax, see the documentation on :ref:`custom request headers
// <config_http_conn_man_headers_custom_request_headers>`.
repeated core.HeaderValueOption request_headers_to_add = 9;

// Specifies a set of headers that will be added to responses to requests
// matching this route. Headers specified at this level are applied before
// headers from the enclosing :ref:`envoy_api_msg_route.VirtualHost` and
// :ref:`envoy_api_msg_RouteConfiguration`. For more information, including
// details on header value syntax, see the documentation on
// :ref:`custom request headers <config_http_conn_man_headers_custom_request_headers>`.
repeated core.HeaderValueOption response_headers_to_add = 10;

// Specifies a list of HTTP headers that should be removed from each response
// to requests matching this route.
repeated string response_headers_to_remove = 11;
}

// Compared to the :ref:`cluster <envoy_api_field_route.RouteAction.cluster>` field that specifies a
Expand Down Expand Up @@ -186,7 +206,7 @@ message WeightedCluster {
// Specifies a list of headers to be added to responses when this cluster is selected
// 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`,

@dio dio Jul 11, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

// :ref:`envoy_api_msg_route.VirtualHost`, and
// :ref:`envoy_api_msg_RouteConfiguration`. For more information, including details on
// header value syntax, see the documentation on :ref:`custom request headers
Expand Down Expand Up @@ -471,17 +491,17 @@ message RouteAction {

// 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

@zuercher zuercher Jul 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

// :ref:`envoy_api_msg_RouteConfiguration`. For more information, including details on
// header value syntax, see the documentation on :ref:`custom request headers
// <config_http_conn_man_headers_custom_request_headers>`.
repeated core.HeaderValueOption request_headers_to_add = 12;

// Specifies a set of headers that will be added to responses to requests
// matching this route. Headers specified at this level are applied before
// headers from the enclosing :ref:`envoy_api_msg_route.VirtualHost` and
// :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`. For more
// information, including details on header value syntax, see the documentation on
// :ref:`custom request headers <config_http_conn_man_headers_custom_request_headers>`.
repeated core.HeaderValueOption response_headers_to_add = 18;

Expand Down Expand Up @@ -689,7 +709,7 @@ message DirectResponseAction {
// .. note::
//
// 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`.

@dio dio Jul 11, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

core.DataSource body = 2;
}

Expand Down
11 changes: 9 additions & 2 deletions source/common/router/config_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,14 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost,
priority_(ConfigUtility::parsePriority(route.route().priority())),
total_cluster_weight_(
PROTOBUF_GET_WRAPPED_OR_DEFAULT(route.route().weighted_clusters(), total_weight, 100UL)),
route_action_request_headers_parser_(
HeaderParser::configure(route.route().request_headers_to_add())),
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())),

@dio dio Jul 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

response_headers_parser_(HeaderParser::configure(route.route().response_headers_to_add(),
route.route().response_headers_to_remove())),
response_headers_parser_(HeaderParser::configure(route.response_headers_to_add(),
route.response_headers_to_remove())),
opaque_config_(parseOpaqueConfig(route)), decorator_(parseDecorator(route)),
direct_response_code_(ConfigUtility::parseDirectResponseCode(route)),
direct_response_body_(ConfigUtility::parseDirectResponseBody(route)),
Expand Down Expand Up @@ -371,6 +376,7 @@ void RouteEntryImplBase::finalizeRequestHeaders(Http::HeaderMap& headers,
// Append user-specified request headers in the following order: route-level headers,

@dio dio Jul 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

// virtual host level headers and finally global connection manager level headers.
request_headers_parser_->evaluateHeaders(headers, request_info);
route_action_request_headers_parser_->evaluateHeaders(headers, request_info);
vhost_.requestHeaderParser().evaluateHeaders(headers, request_info);
vhost_.globalRouteConfig().requestHeaderParser().evaluateHeaders(headers, request_info);
if (!host_rewrite_.empty()) {
Expand All @@ -385,6 +391,7 @@ void RouteEntryImplBase::finalizeRequestHeaders(Http::HeaderMap& headers,

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

@dio dio Jul 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

response_headers_parser_->evaluateHeaders(headers, request_info);
vhost_.responseHeaderParser().evaluateHeaders(headers, request_info);
vhost_.globalRouteConfig().responseHeaderParser().evaluateHeaders(headers, request_info);
Expand Down
6 changes: 4 additions & 2 deletions source/common/router/config_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,8 +358,8 @@ class RouteEntryImplBase : public RouteEntry,

void finalizePathHeader(Http::HeaderMap& headers, const std::string& matched_path,
bool insert_envoy_original_path) const;
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_; };

@dio dio Jul 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?


private:
struct RuntimeData {
Expand Down Expand Up @@ -530,6 +530,8 @@ class RouteEntryImplBase : public RouteEntry,
const uint64_t total_cluster_weight_;
std::unique_ptr<const HashPolicyImpl> hash_policy_;
MetadataMatchCriteriaConstPtr metadata_match_criteria_;
HeaderParserPtr route_action_request_headers_parser_;
HeaderParserPtr route_action_response_headers_parser_;
HeaderParserPtr request_headers_parser_;
HeaderParserPtr response_headers_parser_;
envoy::api::v2::core::Metadata metadata_;
Expand Down
41 changes: 23 additions & 18 deletions test/common/router/config_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ TEST(RouteMatcherTest, TestRoutesWithInvalidRegex) {
EnvoyException, "Invalid regex '\\^/\\(\\+invalid\\)':");
}

// 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.

@dio dio Jul 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

TEST(RouteMatcherTest, TestAddRemoveRequestHeaders) {
std::string json = R"EOF(
{
Expand All @@ -550,14 +550,14 @@ TEST(RouteMatcherTest, TestAddRemoveRequestHeaders) {
"request_headers_to_add": [
{"key": "x-global-header1", "value": "route-override"},
{"key": "x-vhost-header1", "value": "route-override"},
{"key": "x-route-header", "value": "route-new_endpoint"}
{"key": "x-route-action-header", "value": "route-new_endpoint"}
]
},
{
"path": "/",
"cluster": "root_www2",
"request_headers_to_add": [
{"key": "x-route-header", "value": "route-allpath"}
{"key": "x-route-action-header", "value": "route-allpath"}
]
},
{
Expand All @@ -577,7 +577,7 @@ TEST(RouteMatcherTest, TestAddRemoveRequestHeaders) {
"prefix": "/",
"cluster": "www2_staging",
"request_headers_to_add": [
{"key": "x-route-header", "value": "route-allprefix"}
{"key": "x-route-action-header", "value": "route-allprefix"}
]
}
]
Expand Down Expand Up @@ -627,7 +627,7 @@ TEST(RouteMatcherTest, TestAddRemoveRequestHeaders) {
route->finalizeRequestHeaders(headers, request_info, true);
EXPECT_EQ("route-override", headers.get_("x-global-header1"));
EXPECT_EQ("route-override", headers.get_("x-vhost-header1"));
EXPECT_EQ("route-new_endpoint", headers.get_("x-route-header"));
EXPECT_EQ("route-new_endpoint", headers.get_("x-route-action-header"));
}

// Multiple routes can have same route-level headers with different values.
Expand All @@ -637,7 +637,7 @@ TEST(RouteMatcherTest, TestAddRemoveRequestHeaders) {
route->finalizeRequestHeaders(headers, request_info, true);
EXPECT_EQ("vhost-override", headers.get_("x-global-header1"));
EXPECT_EQ("vhost1-www2", headers.get_("x-vhost-header1"));
EXPECT_EQ("route-allpath", headers.get_("x-route-header"));
EXPECT_EQ("route-allpath", headers.get_("x-route-action-header"));
}

// Multiple virtual hosts can have same virtual host level headers with different values.
Expand All @@ -647,7 +647,7 @@ TEST(RouteMatcherTest, TestAddRemoveRequestHeaders) {
route->finalizeRequestHeaders(headers, request_info, true);
EXPECT_EQ("global1", headers.get_("x-global-header1"));
EXPECT_EQ("vhost1-www2_staging", headers.get_("x-vhost-header1"));
EXPECT_EQ("route-allprefix", headers.get_("x-route-header"));
EXPECT_EQ("route-allprefix", headers.get_("x-route-action-header"));
}

// Global headers.
Expand All @@ -660,8 +660,8 @@ TEST(RouteMatcherTest, TestAddRemoveRequestHeaders) {
}
}

// Validates behavior of request_headers_to_add at router, vhost, and route levels when append
// is disabled.
// Validates behavior of request_headers_to_add at router, vhost, and route action levels when
// append is disabled.
TEST(RouteMatcherTest, TestRequestHeadersToAddWithAppendFalse) {
std::string yaml = R"EOF(
name: foo
Expand Down Expand Up @@ -691,7 +691,7 @@ name: foo
value: route-endpoint
append: false
- header:
key: x-route-header
key: x-route-action-header
value: route-endpoint
append: false
- match: { prefix: "/" }
Expand Down Expand Up @@ -719,7 +719,7 @@ name: foo
route->finalizeRequestHeaders(headers, request_info, true);
EXPECT_EQ("global", headers.get_("x-global-header"));
EXPECT_EQ("vhost-www2", headers.get_("x-vhost-header"));
EXPECT_EQ("route-endpoint", headers.get_("x-route-header"));
EXPECT_EQ("route-endpoint", headers.get_("x-route-action-header"));
}

// Global overrides virtual host.
Expand All @@ -734,7 +734,7 @@ name: foo
}

// Validates behavior of response_headers_to_add and response_headers_to_remove at router, vhost,
// and route levels.
// route, and route action levels.
TEST(RouteMatcherTest, TestAddRemoveResponseHeaders) {
std::string yaml = R"EOF(
name: foo
Expand All @@ -751,6 +751,10 @@ name: foo
response_headers_to_remove: ["x-vhost-remove"]
routes:
- match: { prefix: "/new_endpoint" }
response_headers_to_add:
- header:
key: x-route-header
value: route-override
route:
prefix_rewrite: "/api/new_endpoint"
cluster: www2
Expand All @@ -762,14 +766,14 @@ name: foo
key: x-vhost-header1
value: route-override
- header:
key: x-route-header
key: x-route-action-header
value: route-new_endpoint
- match: { path: "/" }
route:
cluster: root_www2
response_headers_to_add:
- header:
key: x-route-header
key: x-route-action-header
value: route-allpath
response_headers_to_remove: ["x-route-remove"]
- match: { prefix: "/" }
Expand All @@ -786,7 +790,7 @@ name: foo
cluster: www2_staging
response_headers_to_add:
- header:
key: x-route-header
key: x-route-action-header
value: route-allprefix
- name: default
domains: ["*"]
Expand Down Expand Up @@ -815,7 +819,8 @@ response_headers_to_remove: ["x-global-remove"]
route->finalizeResponseHeaders(headers, request_info);
EXPECT_EQ("route-override", headers.get_("x-global-header1"));
EXPECT_EQ("route-override", headers.get_("x-vhost-header1"));
EXPECT_EQ("route-new_endpoint", headers.get_("x-route-header"));
EXPECT_EQ("route-new_endpoint", headers.get_("x-route-action-header"));
EXPECT_EQ("route-override", headers.get_("x-route-header"));
}

// Multiple routes can have same route-level headers with different values.
Expand All @@ -826,7 +831,7 @@ response_headers_to_remove: ["x-global-remove"]
route->finalizeResponseHeaders(headers, request_info);
EXPECT_EQ("vhost-override", headers.get_("x-global-header1"));
EXPECT_EQ("vhost1-www2", headers.get_("x-vhost-header1"));
EXPECT_EQ("route-allpath", headers.get_("x-route-header"));
EXPECT_EQ("route-allpath", headers.get_("x-route-action-header"));
}

// Multiple virtual hosts can have same virtual host level headers with different values.
Expand All @@ -837,7 +842,7 @@ response_headers_to_remove: ["x-global-remove"]
route->finalizeResponseHeaders(headers, request_info);
EXPECT_EQ("global1", headers.get_("x-global-header1"));
EXPECT_EQ("vhost1-www2_staging", headers.get_("x-vhost-header1"));
EXPECT_EQ("route-allprefix", headers.get_("x-route-header"));
EXPECT_EQ("route-allprefix", headers.get_("x-route-action-header"));
}

// Global headers.
Expand Down