router: remove preserve_query_string_in_path_redirects (#15335)

mattklein123 · web-flow · commit 1ef5634b07c0 · 2021-03-08T09:15:04.000-05:00
Fixes #14648 Risk Level: Low Testing: Existing tests Docs Changes: N/A Release Notes: Added Platform Specific Features: N/A Fixes #14648 Signed-off-by: Matt Klein <mklein@lyft.com>
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -97,6 +97,7 @@ Removed Config or Runtime
 * http: removed legacy date header overwriting logic and runtime guard `envoy.reloadable_features.preserve_upstream_date deprecation`.
 * listener: removed legacy runtime guard `envoy.reloadable_features.listener_in_place_filterchain_update`.
 * router: removed `envoy.reloadable_features.consume_all_retry_headers` and legacy code path.
+* router: removed `envoy.reloadable_features.preserve_query_string_in_path_redirects` and legacy code path.
 
 New Features
 ------------
diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc
@@ -334,8 +334,6 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost,
                          : ""),
       path_redirect_(route.redirect().path_redirect()),
       path_redirect_has_query_(path_redirect_.find('?') != absl::string_view::npos),
-      enable_preserve_query_in_path_redirects_(Runtime::runtimeFeatureEnabled(
-          "envoy.reloadable_features.preserve_query_string_in_path_redirects")),
       https_redirect_(route.redirect().https_redirect()),
       using_new_timeouts_(route.route().has_max_stream_duration()),
       prefix_rewrite_redirect_(route.redirect().prefix_rewrite()),
@@ -475,7 +473,7 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost,
     regex_rewrite_redirect_substitution_ = rewrite_spec.substitution();
   }
 
-  if (enable_preserve_query_in_path_redirects_ && path_redirect_has_query_ && strip_query_) {
+  if (path_redirect_has_query_ && strip_query_) {
     ENVOY_LOG(warn,
               "`strip_query` is set to true, but `path_redirect` contains query string and it will "
               "not be stripped: {}",
@@ -759,44 +757,30 @@ std::string RouteEntryImplBase::newPath(const Http::RequestHeaderMap& headers) c
   }
 
   std::string final_path_value;
-  if (enable_preserve_query_in_path_redirects_) {
-    if (!path_redirect_.empty()) {
-      // The path_redirect query string, if any, takes precedence over the request's query string,
-      // and it will not be stripped regardless of `strip_query`.
-      if (path_redirect_has_query_) {
-        final_path = path_redirect_.c_str();
-      } else {
-        const absl::string_view current_path = headers.getPathValue();
-        const size_t path_end = current_path.find('?');
-        const bool current_path_has_query = path_end != absl::string_view::npos;
-        if (current_path_has_query) {
-          final_path_value = path_redirect_;
-          final_path_value.append(current_path.data() + path_end, current_path.length() - path_end);
-          final_path = final_path_value;
-        } else {
-          final_path = path_redirect_.c_str();
-        }
-      }
+  if (!path_redirect_.empty()) {
+    // The path_redirect query string, if any, takes precedence over the request's query string,
+    // and it will not be stripped regardless of `strip_query`.
+    if (path_redirect_has_query_) {
+      final_path = path_redirect_.c_str();
     } else {
-      final_path = headers.getPathValue();
-    }
-    if (!path_redirect_has_query_ && strip_query_) {
-      const size_t path_end = final_path.find('?');
-      if (path_end != absl::string_view::npos) {
-        final_path = final_path.substr(0, path_end);
+      const absl::string_view current_path = headers.getPathValue();
+      const size_t path_end = current_path.find('?');
+      const bool current_path_has_query = path_end != absl::string_view::npos;
+      if (current_path_has_query) {
+        final_path_value = path_redirect_;
+        final_path_value.append(current_path.data() + path_end, current_path.length() - path_end);
+        final_path = final_path_value;
+      } else {
+        final_path = path_redirect_.c_str();
       }
     }
   } else {
-    if (!path_redirect_.empty()) {
-      final_path = path_redirect_.c_str();
-    } else {
-      final_path = headers.getPathValue();
-      if (strip_query_) {
-        const size_t path_end = final_path.find("?");
-        if (path_end != absl::string_view::npos) {
-          final_path = final_path.substr(0, path_end);
-        }
-      }
+    final_path = headers.getPathValue();
+  }
+  if (!path_redirect_has_query_ && strip_query_) {
+    const size_t path_end = final_path.find('?');
+    if (path_end != absl::string_view::npos) {
+      final_path = final_path.substr(0, path_end);
     }
   }
 
diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h
@@ -812,7 +812,6 @@ class RouteEntryImplBase : public RouteEntry,
   const std::string port_redirect_;
   const std::string path_redirect_;
   const bool path_redirect_has_query_;
-  const bool enable_preserve_query_in_path_redirects_;
   const bool https_redirect_;
   const bool using_new_timeouts_;
   const std::string prefix_rewrite_redirect_;
diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -78,7 +78,6 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.http2_skip_encoding_empty_trailers",
     "envoy.reloadable_features.overload_manager_disable_keepalive_drain_http2",
     "envoy.reloadable_features.prefer_quic_kernel_bpf_packet_routing",
-    "envoy.reloadable_features.preserve_query_string_in_path_redirects",
     "envoy.reloadable_features.remove_forked_chromium_url",
     "envoy.reloadable_features.require_ocsp_response_for_must_staple_certs",
     "envoy.reloadable_features.stop_faking_paths",
diff --git a/test/common/router/BUILD b/test/common/router/BUILD
@@ -455,7 +455,6 @@ envoy_cc_benchmark_binary(
         "//source/common/router:config_lib",
         "//test/mocks/server:instance_mocks",
         "//test/mocks/stream_info:stream_info_mocks",
-        "//test/test_common:test_runtime_lib",
         "//test/test_common:utility_lib",
         "@envoy_api//envoy/config/route/v3:pkg_cc_proto",
     ],
diff --git a/test/common/router/config_impl_speed_test.cc b/test/common/router/config_impl_speed_test.cc
@@ -6,7 +6,6 @@
 
 #include "test/mocks/server/instance.h"
 #include "test/mocks/stream_info/mocks.h"
-#include "test/test_common/test_runtime.h"
 #include "test/test_common/utility.h"
 
 #include "benchmark/benchmark.h"
@@ -88,9 +87,6 @@ static RouteConfiguration genRouteConfig(benchmark::State& state,
  */
 static void bmRouteTableSize(benchmark::State& state, RouteMatch::PathSpecifierCase match_type) {
   // Setup router for benchmarking.
-  TestScopedRuntime scoped_runtime;
-  Runtime::LoaderSingleton::getExisting()->mergeValues(
-      {{"envoy.reloadable_features.preserve_query_string_in_path_redirects", "false"}});
   Api::ApiPtr api = Api::createApiForTest();
   NiceMock<Server::Configuration::MockServerFactoryContext> factory_context;
   NiceMock<Envoy::StreamInfo::MockStreamInfo> stream_info;
diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc
@@ -6711,67 +6711,6 @@ TEST_F(RouteConfigurationV2, RedirectRegexRewrite) {
   }
 }
 
-TEST_F(RouteConfigurationV2, PathRedirectQueryNotPreserved) {
-  TestScopedRuntime scoped_runtime;
-  Runtime::LoaderSingleton::getExisting()->mergeValues(
-      {{"envoy.reloadable_features.preserve_query_string_in_path_redirects", "false"}});
-
-  std::string yaml = R"EOF(
-virtual_hosts:
-  - name: redirect
-    domains: [redirect.lyft.com]
-    routes:
-      - match: { path: "/path/redirect/"}
-        redirect: { path_redirect: "/new/path-redirect/" }
-      - match: { path: "/path/redirect/strip-query/true"}
-        redirect: { path_redirect: "/new/path-redirect/", strip_query: "true" }
-      - match: { path: "/path/redirect/query"}
-        redirect: { path_redirect: "/new/path-redirect?foo=1" }
-      - match: { path: "/path/redirect/query-with-strip"}
-        redirect: { path_redirect: "/new/path-redirect?foo=2", strip_query: "true" }
-  )EOF";
-
-  TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true);
-  EXPECT_EQ(nullptr, config.route(genRedirectHeaders("www.foo.com", "/foo", true, true), 0));
-
-  {
-    Http::TestRequestHeaderMapImpl headers =
-        genRedirectHeaders("redirect.lyft.com", "/path/redirect/?lang=eng&con=US", true, false);
-    EXPECT_EQ("https://redirect.lyft.com/new/path-redirect/",
-              config.route(headers, 0)->directResponseEntry()->newPath(headers));
-  }
-  {
-    Http::TestRequestHeaderMapImpl headers = genRedirectHeaders(
-        "redirect.lyft.com", "/path/redirect/strip-query/true?lang=eng&con=US", true, false);
-    EXPECT_EQ("https://redirect.lyft.com/new/path-redirect/",
-              config.route(headers, 0)->directResponseEntry()->newPath(headers));
-  }
-  {
-    Http::TestRequestHeaderMapImpl headers =
-        genRedirectHeaders("redirect.lyft.com", "/path/redirect/query", true, false);
-    EXPECT_EQ("https://redirect.lyft.com/new/path-redirect?foo=1",
-              config.route(headers, 0)->directResponseEntry()->newPath(headers));
-  }
-  {
-    Http::TestRequestHeaderMapImpl headers =
-        genRedirectHeaders("redirect.lyft.com", "/path/redirect/query?bar=1", true, false);
-    EXPECT_EQ("https://redirect.lyft.com/new/path-redirect?foo=1",
-              config.route(headers, 0)->directResponseEntry()->newPath(headers));
-  }
-  {
-    Http::TestRequestHeaderMapImpl headers =
-        genRedirectHeaders("redirect.lyft.com", "/path/redirect/query-with-strip", true, false);
-    EXPECT_EQ("https://redirect.lyft.com/new/path-redirect?foo=2",
-              config.route(headers, 0)->directResponseEntry()->newPath(headers));
-  }
-  {
-    Http::TestRequestHeaderMapImpl headers = genRedirectHeaders(
-        "redirect.lyft.com", "/path/redirect/query-with-strip?bar=1", true, false);
-    EXPECT_EQ("https://redirect.lyft.com/new/path-redirect?foo=2",
-              config.route(headers, 0)->directResponseEntry()->newPath(headers));
-  }
-}
-
 // Test to check Strip Query for redirect messages
 TEST_F(RouteConfigurationV2, RedirectStripQuery) {
   std::string yaml = R"EOF(
