-
Notifications
You must be signed in to change notification settings - Fork 267
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
Add *_headers_to_add to ClusterWeight
#441
Conversation
Add request_headers_to_add and response_headers_to_add to ClusterWeight. Signed-off-by: Mandar U Jog <[email protected]>
/cc @zuercher |
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.
LGTM, we should hold off on any merge until #438 is resolved.
envoy/api/v2/route/route.proto
Outdated
@@ -208,6 +208,26 @@ message WeightedCluster { | |||
// cluster with metadata matching that set in metadata_match will be | |||
// considered. The filter name should be specified as *envoy.lb*. | |||
Metadata metadata_match = 3; | |||
|
|||
// Specifies a list of headers to be added to requests when this cluster is selected |
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 the not implemented tags unless the associated Envoy PR is going to get posted at the same time.
Signed-off-by: Mandar U Jog <[email protected]>
@htuch @mattklein123 Added |
LGTM. Holding until the refactor is merged. |
// :ref:`envoy_api_msg_route.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 HeaderValueOption response_headers_to_add = 5; |
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.
Should we go ahead an put response_headers_to_remove
in here?
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.
+1 might as well be consistent
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.
Added response_headers_to_remove
Now it feels like it should be a separate proto, there is plenty of repetition in this file.
message HeaderAction {
repeated HeaderValueOption request_headers_to_add = 1;
repeated HeaderValueOption response_headers_to_add = 2;
repeated string response_headers_to_remove = 3;
}
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.
Unfortunately, we can't refactor, as the other locations that this occurs in are frozen.
Also, |
Signed-off-by: Mandar U Jog <[email protected]>
1ddc30f
to
fe2dae9
Compare
@zuercher I will update |
@mattklein123 can this move forward now, #438 has merged. |
…clusters (#2765) Implements the request_headers_to_add, response_headers_to_add, and response_headers_to_remove fields added to weighted clusters by envoyproxy/data-plane-api#441. Risk Level: Low - no change in behavior without configuration changes Testing: unit and integration tests Docs Changes: envoyproxy/data-plane-api#531 Release Notes: updated Fixes: #2455 Signed-off-by: Stephan Zuercher [email protected]
Add request_headers_to_add and response_headers_to_add to ClusterWeight.
The directives are processed when the cluster in selected through the enclosing RouteAction.
reference: envoyproxy/envoy#2455
Signed-off-by: Mandar U Jog [email protected]