router: Implement https_redirect in RedirectAction #2479
Conversation
* envoyproxy#2371 Signed-off-by: Jamie Hewland <[email protected]>
Signed-off-by: Jamie Hewland <[email protected]>
There was a problem hiding this comment.
Nice change! Only adding this for v2 is totally fine :-)
One thought while I'm in here but otherwise, @junr03 mind doing a first pass for style and I'll do one final pass?
| @@ -2171,17 +2171,17 @@ TEST(RouteMatcherTest, DirectResponse) { | |||
| "domains": ["redirect.lyft.com"], | |||
| "routes": [ | |||
| { | |||
| "prefix": "/foo", | |||
There was a problem hiding this comment.
Sorry, I'm probably missing something obvious, but why change these?
It's generally helpful when changing code to leave the existing tests as is to make it clear there's no change to existing functionality.
There was a problem hiding this comment.
It seemed to me to be better to have the path prefixes mean something, especially as more kinds of redirects were added.
So there would be...
/host => host_redirect
/path => path_redirect
/host_path => host_redirect + path_redirect
...
...instead of foo/bar/baz which don't mean anything. That was slightly complicated by /host being a prefix of /host_path so ordering matters now 😕.
I can try rework or go back to a minimal change?
There was a problem hiding this comment.
Fair enough - I'll take a close look at diffs when I do my pass, so please just consider it for future PRs.
There was a problem hiding this comment.
Thanks for the explanation - it is good clean up :-)
There was a problem hiding this comment.
Looks great overall. I suspect this may have merge conflicts with #2315, mind doing a merge with master?
source/common/router/config_impl.cc
Outdated
| return fmt::format("{}://{}{}", headers.ForwardedProto()->value().c_str(), final_host, | ||
| final_path); | ||
| if (https_redirect_) { | ||
| final_scheme = "https"; |
There was a problem hiding this comment.
minor prefernce for Http::Headers::get().SchemeValues.Https
There was a problem hiding this comment.
Some notes here (which I hope you'll see since GitHub has hidden the thread):
- I did make this change
My C++ is pretty rusty, but I'm not sure why this method usesIt's to prevent copies + thechar*rather than juststd::string. Could save a bunch of.c_str()s?HeaderStringtype doesn't have an easy way to get astd::string.- There is another place in this file where the
"https"string literal is used.
| config.route(headers, 0)->directResponseEntry()->newPath(headers)); | ||
| } | ||
| { | ||
| Http::TestHeaderMapImpl headers = | ||
| genRedirectHeaders("redirect.lyft.com", "/bar", true, false); | ||
| EXPECT_EQ("https://redirect.lyft.com/new_bar", | ||
| genRedirectHeaders("redirect.lyft.com", "/path", false, false); |
There was a problem hiding this comment.
Looking at these more carefully I believe we've lost test coverage that https scheme is preserved in the non-redirect path. Let me know if I'm missing a test, otherwise please leave the tests which previously had true for ssl true?
Signed-off-by: Jamie Hewland <[email protected]>
Signed-off-by: Jamie Hewland <[email protected]>
Signed-off-by: Jamie Hewland <[email protected]>
|
It looks like the tests were broken when #2307 was merged. |
|
indeed, sorry, #2490 should fix once it lands. |
|
Can you merge master to pickup #2490? |
Signed-off-by: Jamie Hewland <[email protected]>
| - match: { prefix: /foo } | ||
| - match: { prefix: /host_path_https } | ||
| redirect: { host_redirect: new.lyft.com, path_redirect: /new_path, https_redirect: true } | ||
| - match: { prefix: /host_path } |
There was a problem hiding this comment.
based on the previous test coverage on host and path redirect combined, then host and path independently, would it be beneficial to do all chose two combinations here? i.e add the missing host_https path_https redirects?
Signed-off-by: Jamie Hewland <[email protected]>
Signed-off-by: Jamie Hewland <[email protected]>
Signed-off-by: Jamie Hewland <[email protected]>
…xy#2479) * use different alpn override for different upstream protocol * update dependent * address comment
Signed-off-by: JP Simard <[email protected]>
Signed-off-by: JP Simard <[email protected]>
Description:
This implements the
https_redirectfield toRedirectActionto redirect requests to thehttpsscheme.Risk Level: Low
Testing:
Tests added to
test/common/router/config_test_impl.cc. I only implemented these for the v2 API. I hope that's ok?Docs Changes:
envoyproxy/data-plane-api#426
Release Notes:
Added