Skip to content
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.

Sign up for GitHub

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

Jump to bottom

router: Implement https_redirect in RedirectAction #2479

Merged
merged 9 commits into from
Feb 1, 2018
Merged

router: Implement https_redirect in RedirectAction #2479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a96387d
Implement https_redirect in RedirectAction
Jan 30, 2018
dba65c5
Add a release note for https_redirect in RedirectAction
Jan 30, 2018
165b58d
Use https constant from Http::Headers
Jan 31, 2018
f898729
Keep testing that host/path redirects work for https URLs
Jan 31, 2018
b571212
Merge branch 'master' into https-redirect
Jan 31, 2018
0c803ad
Merge branch 'master' into https-redirect
Jan 31, 2018
33a1baa
Merge branch 'master' into https-redirect
Feb 1, 2018
22addc4
Use exact path matching so matcher order doesn't matter
Feb 1, 2018
e203b8c
Add tests for host/path and https redirects
Feb 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RAW_RELEASE_NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,4 @@ final version.
* Added support for direct responses -- i.e., sending a preconfigured HTTP response without proxying anywhere.
* Added DOWNSTREAM_LOCAL_ADDRESS, DOWNSTREAM_LOCAL_ADDRESS_WITHOUT_PORT header formatters, and
DOWNSTREAM_LOCAL_ADDRESS access log formatter.
* Added support for HTTPS redirects on specific routes.
15 changes: 11 additions & 4 deletions source/common/router/config_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,8 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost,
timeout_(PROTOBUF_GET_MS_OR_DEFAULT(route.route(), timeout, DEFAULT_ROUTE_TIMEOUT_MS)),
runtime_(loadRuntimeData(route.match())), loader_(loader),
host_redirect_(route.redirect().host_redirect()),
path_redirect_(route.redirect().path_redirect()), retry_policy_(route.route()),
path_redirect_(route.redirect().path_redirect()),
https_redirect_(route.redirect().https_redirect()), retry_policy_(route.route()),
rate_limit_policy_(route.route().rate_limits()), shadow_policy_(route.route()),
priority_(ConfigUtility::parsePriority(route.route().priority())),
total_cluster_weight_(
Expand Down Expand Up @@ -388,6 +389,7 @@ std::string RouteEntryImplBase::newPath(const Http::HeaderMap& headers) const {

const char* final_host;
const char* final_path;
const char* final_scheme;
if (!host_redirect_.empty()) {
final_host = host_redirect_.c_str();
} else {
Expand All @@ -402,9 +404,14 @@ std::string RouteEntryImplBase::newPath(const Http::HeaderMap& headers) const {
final_path = headers.Path()->value().c_str();
}

ASSERT(headers.ForwardedProto());
return fmt::format("{}://{}{}", headers.ForwardedProto()->value().c_str(), final_host,
final_path);
if (https_redirect_) {
final_scheme = Http::Headers::get().SchemeValues.Https.c_str();
} else {
ASSERT(headers.ForwardedProto());
final_scheme = headers.ForwardedProto()->value().c_str();
}

return fmt::format("{}://{}{}", final_scheme, final_host, final_path);
}

std::multimap<std::string, std::string>
Expand Down
1 change: 1 addition & 0 deletions source/common/router/config_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ class RouteEntryImplBase : public RouteEntry,
Runtime::Loader& loader_;
const std::string host_redirect_;
const std::string path_redirect_;
const bool https_redirect_;
const RetryPolicyImpl retry_policy_;
const RateLimitPolicyImpl rate_limit_policy_;
const ShadowPolicyImpl shadow_policy_;
Expand Down
64 changes: 48 additions & 16 deletions test/common/router/config_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2201,17 +2201,17 @@ TEST(RouteMatcherTest, DirectResponse) {
"domains": ["redirect.lyft.com"],
"routes": [
{
"prefix": "/foo",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough - I'll take a close look at diffs when I do my pass, so please just consider it for future PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 🙏🏼

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation - it is good clean up :-)

"path": "/host",
"host_redirect": "new.lyft.com"
},
{
"prefix": "/bar",
"path_redirect": "/new_bar"
"path": "/path",
"path_redirect": "/new_path"
},
{
"prefix": "/baz",
"path": "/host_path",
"host_redirect": "new.lyft.com",
"path_redirect": "/new_baz"
"path_redirect": "/new_path"
}
]
}
Expand Down Expand Up @@ -2241,12 +2241,20 @@ name: foo
- name: redirect
domains: [redirect.lyft.com]
routes:
- match: { prefix: /foo }
- match: { path: /host }
redirect: { host_redirect: new.lyft.com }
- match: { prefix: /bar }
redirect: { path_redirect: /new_bar }
- match: { prefix: /baz }
redirect: { host_redirect: new.lyft.com, path_redirect: /new_baz }
- match: { path: /path }
redirect: { path_redirect: /new_path }
- match: { path: /https }
redirect: { https_redirect: true }
- match: { path: /host_path }
redirect: { host_redirect: new.lyft.com, path_redirect: /new_path }
- match: { path: /host_https }
redirect: { host_redirect: new.lyft.com, https_redirect: true }
- match: { path: /path_https }
redirect: { path_redirect: /new_path, https_redirect: true }
- match: { path: /host_path_https }
redirect: { host_redirect: new.lyft.com, path_redirect: /new_path, https_redirect: true }
- name: direct
domains: [direct.example.com]
routes:
Expand Down Expand Up @@ -2292,20 +2300,20 @@ name: foo
}
{
Http::TestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/foo", false, false);
EXPECT_EQ("http://new.lyft.com/foo",
genRedirectHeaders("redirect.lyft.com", "/host", false, false);
EXPECT_EQ("http://new.lyft.com/host",
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", true, false);
EXPECT_EQ("https://redirect.lyft.com/new_path",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
}
{
Http::TestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/baz", true, false);
EXPECT_EQ("https://new.lyft.com/new_baz",
genRedirectHeaders("redirect.lyft.com", "/host_path", true, false);
EXPECT_EQ("https://new.lyft.com/new_path",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
}
if (!test_v2) {
Expand Down Expand Up @@ -2341,6 +2349,30 @@ name: foo
genRedirectHeaders("direct.example.com", "/other", true, false);
EXPECT_EQ(nullptr, config.route(headers, 0)->directResponseEntry());
}
{
Http::TestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/https", false, false);
EXPECT_EQ("https://redirect.lyft.com/https",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
}
{
Http::TestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/host_https", false, false);
EXPECT_EQ("https://new.lyft.com/host_https",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
}
{
Http::TestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/path_https", false, false);
EXPECT_EQ("https://redirect.lyft.com/new_path",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
}
{
Http::TestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/host_path_https", false, false);
EXPECT_EQ("https://new.lyft.com/new_path",
config.route(headers, 0)->directResponseEntry()->newPath(headers));
}
};

NiceMock<Runtime::MockLoader> runtime;
Expand Down