-
Notifications
You must be signed in to change notification settings - Fork 5.1k
route: add substitution formatter for path/host rewrite #41468
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
route: add substitution formatter for path/host rewrite #41468
Conversation
Signed-off-by: WangBaiping <[email protected]>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Http::TestRequestHeaderMapImpl headers = | ||
genHeaders("api.lyft.com", "/rewrite-host-with-header-value", "GET"); | ||
const RouteEntry* route_entry = config.route(headers, 0)->routeEntry(); | ||
EXPECT_FALSE(route_entry->currentUrlPathAfterRewrite(headers).has_value()); |
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.
We needn't test test this everywhere because the finalizeRequestHeaders
will call this method to generate new path anyway.
// | ||
// If the final output of the path rewrite is empty, then the update will be ignored and the | ||
// original path will be preserved. | ||
string path_rewrite = 45; |
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 should be a general solution for path/host rewriting because it could cover almost all exist features but provide more features, flexibility and extensibility.
Signed-off-by: WangBaiping <[email protected]>
/retest |
@wbpcode Thank you so much for adding this feature. We have a similar use-case where we want to rewrite the paths based on another incoming header from the request. For example, the requests comes with a path With this feature, I could simply do something like |
…th-host-rewrite Signed-off-by: code <[email protected]>
/retest |
Re-assign this to @abeyad to give a API review because seems Mark is focus on gRPC recently. |
Sorry for the noise. I think of this require a senior maintainer to review the code anyway. So, let Matt to cover both the API and code directly. Matt, thanks 🙏 |
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.
Nice!
Commit Message: route: add substitution formatter for path/host rewrite
Additional Description:
This PR add substitution formatting support to host/path rewrite. Now we can rewrite the host/path in same way like rewriting other headers in
header_mutation
orrequest_headers_to_add
.The substitution is used for logging, header mutation, rate limiting, now, it also could be used for host/path header rewriting.
Risk Level: low.
Testing: unit.
Docs Changes: n/a.
Release Notes: added.
Platform Specific Features: n/a.