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

docs: clarify the use of "/" as a prefix_rewrite #3160

Merged
merged 11 commits into from
Apr 30, 2018
Merged

docs: clarify the use of "/" as a prefix_rewrite #3160

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
65d01cc
utility: add normalize URL's path helper
dio Apr 22, 2018
ae407fd
config: normalize URL path after prefix rewrite
dio Apr 22, 2018
64bf6ad
Remove partial support for url normalization
dio Apr 23, 2018
a88cebb
Add examples as notes
dio Apr 23, 2018
e5bf03b
Fix comment
dio Apr 23, 2018
9541fb6
Remove normalize path util
dio Apr 25, 2018
dc34878
Add more samples for RedirectPrefixRewrite
dio Apr 25, 2018
ddf2a2e
Add more input samples
dio Apr 25, 2018
d46c26e
Update docs
dio Apr 25, 2018
b47c282
Update doc and comments
dio Apr 26, 2018
a4c60d5
Add example of config entries
dio Apr 30, 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
66 changes: 66 additions & 0 deletions api/envoy/api/v2/route/route.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,39 @@ message RouteAction {
// Indicates that during forwarding, the matched prefix (or path) should be
// swapped with this value. This option allows application URLs to be rooted
// at a different path from those exposed at the reverse proxy layer.
//
// .. note::
//
// Take a careful attention to the use of trailing slash in match prefix value. A common
Copy link
Member

Choose a reason for hiding this comment

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

I think if you write something like the following, you can delete the examples.

"Pay careful attention to the use of trailing slashes in the RouteMatch's prefix value. Stripping a prefix from a path requires multiple Routes to handle all cases. For example, rewriting /prefix to / and /prefix/etc to /etc cannot be done in a single Route."

Copy link
Member Author

@dio dio Apr 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @zuercher, I have updated this PR as suggested.

// use-case is to have a rule to rewrite e.g. :code:`scheme://host/prefix` and
// :code:`scheme://host/prefix/ok/1/2/3` to the root path of the host (:code:`scheme://host/`)
// and :code:`scheme://host/ok/1/2/3` respectively.
//
// The following config example exhibits that behavior.
//
// .. code-block:: yaml
//
// - match:
// prefix: "/prefix/"
// route:
// prefix_rewrite: "/"
// - match:
// prefix: "/prefix"
// route:
// prefix_rewrite: "/"
//
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the examples below will be clearer if the two prefix-substitutions above use different match strings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you clarify?

Copy link
Member Author

@dio dio Apr 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The point of this is to tell that to achieve such behavior (rewriting by replacing a segment of a URL, in this case: "prefix" and attach the rest to the root path) is by declaring two match rules (with and without trailing "/", in this case "/prefix" and "/prefix/"). The below example tells that if we remove the rule with "match" value with trailing slash, it gives a different result (rewrites "/prefix/ok" to "//ok").

// If the match prefix value with trailing slash is removed, i.e.
//
// .. code-block:: yaml
//
// match:
// prefix: "/prefix"
// route:
// prefix_rewrite: "/"
//
// it rewrites :code:`/prefix` to :code:`/`, but :code:`/prefix/ok` to :code:`//ok`, which
// most of the time is not the intended result. However, it can be useful for the case of
// replacing substring of a path segment. For example to rewrite :code:`/prefixok` to :code:`/ok`.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/replacing substring/replacing a substring/

string prefix_rewrite = 5;

oneof host_rewrite_specifier {
Expand Down Expand Up @@ -571,6 +604,39 @@ message RedirectAction {
// Indicates that during redirection, the matched prefix (or path)
// should be swapped with this value. This option allows redirect URLs be dynamically created
// based on the request.
//
// .. note::
//
// Take a careful attention to the use of trailing slash in match prefix value. A common
// use-case is to have a rule to rewrite e.g. :code:`scheme://host/prefix` and
// :code:`scheme://host/prefix/ok/1/2/3` to the root path of the host (:code:`scheme://host/`)
// and :code:`scheme://host/ok/1/2/3` respectively.
//
// The following config example exhibits that behavior.
//
// .. code-block:: yaml
//
// - match:
// prefix: "/prefix/"
// redirect:
// prefix_rewrite: "/"
// - match:
// prefix: "/prefix"
// redirect:
// prefix_rewrite: "/"
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't have the whole context for this file, but I wonder if you could point to the first instance of this example rather than repeating it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me see

//
// If the match prefix value with trailing slash is removed, i.e.
//
// .. code-block:: yaml
//
// match:
// prefix: "/prefix"
// redirect:
// prefix_rewrite: "/"
//
// it rewrites :code:`/prefix` to :code:`/`, but :code:`/prefix/ok` to :code:`//ok`, which
// most of the time is not the intended result. However, it can be useful for the case of
// replacing substring of a path segment. For example to rewrite :code:`/prefixok` to :code:`/ok`.
string prefix_rewrite = 5;
}

Expand Down
58 changes: 58 additions & 0 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 @@ -3925,6 +3925,14 @@ name: AllRedirects
redirect: { prefix_rewrite: "/new/regex-prefix/" }
- match: { prefix: "/http/prefix"}
redirect: { prefix_rewrite: "/https/prefix" , https_redirect: true }
- match: { prefix: "/ignore-this/"}
redirect: { prefix_rewrite: "/" }
- match: { prefix: "/ignore-this"}
redirect: { prefix_rewrite: "/" }
- match: { prefix: "/ignore-substring"}
redirect: { prefix_rewrite: "/" }
- match: { prefix: "/service-hello/"}
redirect: { prefix_rewrite: "/" }
)EOF";

NiceMock<Runtime::MockLoader> runtime;
Expand Down Expand Up @@ -3969,6 +3977,56 @@ name: AllRedirects
redirect->rewritePathHeader(headers);
EXPECT_EQ("https://redirect.lyft.com/https/prefix/", redirect->newPath(headers));
}
{
Http::TestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/ignore-this", false, false);
const DirectResponseEntry* redirect = config.route(headers, 0)->directResponseEntry();
redirect->rewritePathHeader(headers);
EXPECT_EQ("http://redirect.lyft.com/", redirect->newPath(headers));
}
{
Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth some comments here to describe what how these two cases are different.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, will add some comments here.

Http::TestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/ignore-this/", false, false);
const DirectResponseEntry* redirect = config.route(headers, 0)->directResponseEntry();
redirect->rewritePathHeader(headers);
EXPECT_EQ("http://redirect.lyft.com/", redirect->newPath(headers));
}
{
Http::TestHeaderMapImpl headers = genRedirectHeaders(
"redirect.lyft.com", "/ignore-this/however/use/the/rest/of/this/path", false, false);
const DirectResponseEntry* redirect = config.route(headers, 0)->directResponseEntry();
redirect->rewritePathHeader(headers);
EXPECT_EQ("http://redirect.lyft.com/however/use/the/rest/of/this/path",
redirect->newPath(headers));
}
{
Http::TestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/ignore-this/use/", false, false);
const DirectResponseEntry* redirect = config.route(headers, 0)->directResponseEntry();
redirect->rewritePathHeader(headers);
EXPECT_EQ("http://redirect.lyft.com/use/", redirect->newPath(headers));
}
{
Http::TestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/ignore-substringto/use/", false, false);
const DirectResponseEntry* redirect = config.route(headers, 0)->directResponseEntry();
redirect->rewritePathHeader(headers);
EXPECT_EQ("http://redirect.lyft.com/to/use/", redirect->newPath(headers));
}
{
Http::TestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/ignore-substring-to/use/", false, false);
const DirectResponseEntry* redirect = config.route(headers, 0)->directResponseEntry();
redirect->rewritePathHeader(headers);
EXPECT_EQ("http://redirect.lyft.com/-to/use/", redirect->newPath(headers));
}
{
Http::TestHeaderMapImpl headers =
genRedirectHeaders("redirect.lyft.com", "/service-hello/a/b/c", false, false);
const DirectResponseEntry* redirect = config.route(headers, 0)->directResponseEntry();
redirect->rewritePathHeader(headers);
EXPECT_EQ("http://redirect.lyft.com/a/b/c", redirect->newPath(headers));
}
}

// Test to check Strip Query for redirect messages
Expand Down