allow setting headers#709
Conversation
|
Hi @itaysk. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
.. |
|
|
||
| // HTTP headers to set (overwrite) before forwarding a request to the | ||
| // destination service. | ||
| map<string, string> set_request_headers = 17; |
There was a problem hiding this comment.
Instead of this, I suggest creating a new message like this:
headers:
Request:
Set
Remove
Response:
Set
Remove
Do you think it’s sufficient to always set, for the most common use case? Append simply buys you the ability to have duplicate headers while common expectation is to set or unset
There was a problem hiding this comment.
your suggestion makes alot of sense. both the reorg into one spec level field, and to always set by default. But can we eliminate the case for headers with multiple values? (which is allowable by HTTP: https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2)
There was a problem hiding this comment.
We could always have Set, Add, Remove. Something like this:
message HeaderOperations {
map<string, string> set
map<string, string> add
map<string, string> remove
}
message Headers {
HeaderOperations request
HeaderOperations response
}
Headers headers
There was a problem hiding this comment.
Yes this is what I meant. I'll make the necessary changes.
There was a problem hiding this comment.
@rshriram I assume that now append_{request,reqponse}_headers should be marked as deprecated?
There was a problem hiding this comment.
@rshriram additionally, doesn't it make sense to nest the messages like:
message Headers {
HeaderOperations request
HeaderOperations response
message HeaderOperations {
...
}
}
WDYT?
There was a problem hiding this comment.
Yep.. go for it.. and please mark the others as deprecated
There was a problem hiding this comment.
@rshriram updated based on our discussion
|
Tests are failing, related to protolock which I'm not entirely familiar with. before committing I ran |
|
OK please disregard my prev message, I did some reading on the build scripts and protolock and now understand I needed to commit the |
|
/ok-to-test |
|
Do you have the implementation ready for this change? If so, I would push this to release-1.1 branch.. If not, then stick with master |
|
Yes I already wrote the core implementation, just need to finalize the auxiliary code like tests. I can have it ready for tomorrow. |
|
changed base to |
| // Apppend the given values to the the headers specified by keys | ||
| // (will create a comma seperated list of values) | ||
| map<string, string> add = 2; | ||
| // Remove a the specified headers |
There was a problem hiding this comment.
typo: a .. "Remove the specified headers"
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: itaysk, rshriram The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
for #708