Implement host_rewrite_path option #12440
Conversation
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
|
@dio thank you for the review. Addressed your comments. |
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
source/common/router/config_impl.cc
Outdated
| @@ -287,6 +287,13 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, | |||
| ? absl::optional<Http::LowerCaseString>(Http::LowerCaseString( | |||
| route.route().host_rewrite_header())) | |||
| : absl::nullopt), | |||
| host_rewrite_path_( | |||
There was a problem hiding this comment.
nit: suggest using a "regex" in the naming so it's clear this is a regex
There was a problem hiding this comment.
IMHO that would make it less clear what the regex is being executed on. It seems like other options names suggest where the Host header rewrite is coming from, (e.g. host_rewrite_literal, host_rewrite_header) not how the result is constructed.
With host_rewrite_regex you could imaging the regex being exectuted on some other header, or on the original Host name.
Having said that, happy to change if you think regex's better.
There was a problem hiding this comment.
Or, you mean host_rewrite_path_regex, not host_rewrite_regex?
There was a problem hiding this comment.
Haha yep, definitely agree that path should be included so it's clear. On my own first glance though the naming didn't line up to my understanding that this was a compiled regex matcher. But no worries either way (header file's got the type anyway)
There was a problem hiding this comment.
ok. changing to host_rewrite_path_regex, thank you.
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
|
Merged master in to resolve a conflict. |
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
| host_rewrite_path_regex_substitution_( | ||
| route.route().has_host_rewrite_path_regex() | ||
| ? route.route().host_rewrite_path_regex().substitution() | ||
| : ""), |
There was a problem hiding this comment.
We usually set it as EMPTY_STRING here. But I think we can do cleanup later.
|
/lgtm api |
This implements a
host_rewrite_pathoption for rewriting the Host header based on path. See rational in the linked issue.Note: the regex is executed on the path with query/fragment stripped. This is analogues to what
regex_rewriteoption is doing.Commit Message: Implement host_rewrite_path option
Risk Level: Low
Testing: added unit tests
Docs Changes: document the new option in proto file
Release Notes: added to current.rst
Fixes #12430
Signed-off-by: Petr Pchelko ppchelko@wikimedia.org