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

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

This patch clarifies the use of "/" as a prefix_rewrite in route and redirect prefix rewriting. And also a note on the use of trailing slashes as match value.

Risk Level: Low
Testing: add more input samples to RedirectPrefixRewrite test.
Docs Changes: Update route.proto doc regarding path_rewrite both for redirect and route.
Release Notes: N/A

Fixes #2956

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

api/envoy/api/v2/route/route.proto

@@ -353,6 +353,28 @@ 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.
+  //
+  // .. attention::
+  //
+  //   Pay careful attention to the use of trailing slashes in the
+  //   :ref:`route's match <envoy_api_field_route.Route.match>` 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
+  //   :ref:`Route <envoy_api_msg_route.Route>`, as shown by the below config entries:
+  //
+  //   .. code-block:: yaml
+  //
+  //     - match:
+  //         prefix: "/prefix/"
+  //       route:
+  //         prefix_rewrite: "/"
+  //     - match:
+  //         prefix: "/prefix"
+  //       route:
+  //         prefix_rewrite: "/"
+  //
+  //   Having above entries in the config, requests to */prefix* will be stripped to */*, while
+  //   requests to */prefix/etc* will be stripped to */etc*.
   string prefix_rewrite = 5;
 
   oneof host_rewrite_specifier {
@@ -566,6 +588,11 @@ 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.
+    //
+    // .. attention::
+    //
+    //   Pay attention to the use of trailing slashes as mentioned in
+    //   :ref:`RouteAction's prefix_rewrite <envoy_api_field_route.RouteAction.prefix_rewrite>`.
     string prefix_rewrite = 5;
   }
 

test/common/router/config_impl_test.cc

@@ -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;
@@ -3969,6 +3977,62 @@ name: AllRedirects
     redirect->rewritePathHeader(headers);
     EXPECT_EQ("https://redirect.lyft.com/https/prefix/", redirect->newPath(headers));
   }
+  {
+    // The following matches to the redirect action match value equals to `/ignore-this` instead of
+    // `/ignore-this/`.
+    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));
+  }
+  {
+    // The following matches to the redirect action match value equals to `/ignore-this/` instead of
+    // `/ignore-this`.
+    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));
+  }
+  {
+    // The same as previous test request, the following matches to the redirect action match value
+    // equals to `/ignore-this/` instead of `/ignore-this`.
+    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