From 9d58abb87293ec4eda888ee676f47402678d2a2c Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Fri, 5 Mar 2021 19:41:29 +0000 Subject: [PATCH] router: remove preserve_query_string_in_path_redirects Fixes https://github.com/envoyproxy/envoy/issues/14648 Signed-off-by: Matt Klein --- docs/root/version_history/current.rst | 1 + source/common/router/config_impl.cc | 58 +++++++------------ source/common/router/config_impl.h | 1 - source/common/runtime/runtime_features.cc | 1 - test/common/router/BUILD | 1 - test/common/router/config_impl_speed_test.cc | 4 -- test/common/router/config_impl_test.cc | 61 -------------------- 7 files changed, 22 insertions(+), 105 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 8c1b53e036d4..bb37f9832df5 100644 --- 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 index ac777ff099f2..0c9e896ee740 100644 --- 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 index 648b1e8e2c11..2e947ac7d667 100644 --- 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 index 264d3b2bdaa9..12056de2c1d5 100644 --- 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 index fea4e906a23a..858621a096c4 100644 --- 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 index a4192eb487f2..704aac46644b 100644 --- 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 factory_context; NiceMock stream_info; diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 69932d8b4ed2..a29e1d1fb052 100644 --- 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(