router: switch to use new QueryParametersMulti type (#29385)

Signed-off-by: Neal Turett <neal.turett@datadoghq.com>

api/envoy/config/route/v3/route_components.proto

@@ -619,7 +619,8 @@ message RouteMatch {
   // match. The router will check the query string from the ``path`` header
   // against all the specified query parameters. If the number of specified
   // query parameters is nonzero, they all must match the ``path`` header's
-  // query string for a match to occur.
+  // query string for a match to occur.  In the event query parameters are
+  // repeated, only the first value for each key will be considered.
   //
   // .. note::
   //

envoy/http/query_params.h

@@ -6,6 +6,7 @@
 
 #include "absl/container/btree_map.h"
 #include "absl/strings/string_view.h"
+#include "absl/types/optional.h"
 #include "header_map.h"
 
 namespace Envoy {
@@ -29,6 +30,7 @@ class QueryParamsMulti {
   void overwrite(absl::string_view key, absl::string_view value);
   std::string toString();
   std::string replaceQueryString(const HeaderString& path);
+  absl::optional<std::string> getFirstValue(absl::string_view key) const;
 
   const absl::btree_map<std::string, std::vector<std::string>>& data() { return data_; }
 

source/common/http/utility.cc

@@ -32,6 +32,7 @@
 #include "absl/strings/str_cat.h"
 #include "absl/strings/str_split.h"
 #include "absl/strings/string_view.h"
+#include "absl/types/optional.h"
 #include "quiche/http2/adapter/http2_protocol.h"
 
 namespace Envoy {
@@ -609,6 +610,15 @@ void Utility::QueryParamsMulti::overwrite(absl::string_view key, absl::string_vi
   this->data_[key] = std::vector<std::string>{std::string(value)};
 }
 
+absl::optional<std::string> Utility::QueryParamsMulti::getFirstValue(absl::string_view key) const {
+  auto it = this->data_.find(key);
+  if (it == this->data_.end()) {
+    return std::nullopt;
+  }
+
+  return absl::optional<std::string>{it->second.at(0)};
+}
+
 absl::string_view Utility::findQueryStringStart(const HeaderString& path) {
   absl::string_view path_str = path.getStringView();
   size_t query_offset = path_str.find('?');

source/common/router/config_impl.cc

@@ -787,8 +787,8 @@ bool RouteEntryImplBase::matchRoute(const Http::RequestHeaderMap& headers,
     return false;
   }
   if (!config_query_parameters_.empty()) {
-    Http::Utility::QueryParams query_parameters =
-        Http::Utility::parseQueryString(headers.getPathValue());
+    auto query_parameters =
+        Http::Utility::QueryParamsMulti::parseQueryString(headers.getPathValue());
     matches &= ConfigUtility::matchQueryParams(query_parameters, config_query_parameters_);
     if (!matches) {
       return false;

source/common/router/config_utility.cc

@@ -38,16 +38,19 @@ ConfigUtility::QueryParameterMatcher::QueryParameterMatcher(
     : name_(config.name()), matcher_(maybeCreateStringMatcher(config)) {}
 
 bool ConfigUtility::QueryParameterMatcher::matches(
-    const Http::Utility::QueryParams& request_query_params) const {
-  auto query_param = request_query_params.find(name_);
-  if (query_param == request_query_params.end()) {
+    const Http::Utility::QueryParamsMulti& request_query_params) const {
+  // This preserves the legacy behavior of ignoring all but the first value for a given key
+  auto data = request_query_params.getFirstValue(name_);
+  if (!data.has_value()) {
     return false;
-  } else if (!matcher_.has_value()) {
-    // Present match.
+  }
+
+  if (!matcher_.has_value()) {
+    // Present check
     return true;
-  } else {
-    return matcher_.value().match(query_param->second);
   }
+
+  return matcher_.value().match(data.value());
 }
 
 Upstream::ResourcePriority
@@ -63,7 +66,7 @@ ConfigUtility::parsePriority(const envoy::config::core::v3::RoutingPriority& pri
 }
 
 bool ConfigUtility::matchQueryParams(
-    const Http::Utility::QueryParams& query_params,
+    const Http::Utility::QueryParamsMulti& query_params,
     const std::vector<QueryParameterMatcherPtr>& config_query_params) {
   for (const auto& config_query_param : config_query_params) {
     if (!config_query_param->matches(query_params)) {

source/common/router/config_utility.h

@@ -39,7 +39,7 @@ class ConfigUtility {
      * @param request_query_params supplies the parsed query parameters from a request.
      * @return bool true if a match for this QueryParameterMatcher exists in request_query_params.
      */
-    bool matches(const Http::Utility::QueryParams& request_query_params) const;
+    bool matches(const Http::Utility::QueryParamsMulti& request_query_params) const;
 
   private:
     const std::string name_;
@@ -62,7 +62,7 @@ class ConfigUtility {
    * @return bool true if all the query params (and values) in the config_params are found in the
    *         query_params
    */
-  static bool matchQueryParams(const Http::Utility::QueryParams& query_params,
+  static bool matchQueryParams(const Http::Utility::QueryParamsMulti& query_params,
                                const std::vector<QueryParameterMatcherPtr>& config_query_params);
 
   /**

source/common/router/router_ratelimit.cc

@@ -257,8 +257,8 @@ QueryParameterValueMatchAction::QueryParameterValueMatchAction(
 bool QueryParameterValueMatchAction::populateDescriptor(
     RateLimit::DescriptorEntry& descriptor_entry, const std::string&,
     const Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo&) const {
-  Http::Utility::QueryParams query_parameters =
-      Http::Utility::parseAndDecodeQueryString(headers.getPathValue());
+  Http::Utility::QueryParamsMulti query_parameters =
+      Http::Utility::QueryParamsMulti::parseAndDecodeQueryString(headers.getPathValue());
   if (expect_match_ ==
       ConfigUtility::matchQueryParams(query_parameters, action_query_parameters_)) {
     descriptor_entry = {descriptor_key_, descriptor_value_};

source/extensions/filters/http/jwt_authn/matcher.cc

@@ -42,8 +42,8 @@ class BaseMatcherImpl : public Matcher, public Logger::Loggable<Logger::Id::jwt>
 
     matches &= Http::HeaderUtility::matchHeaders(headers, config_headers_);
     if (!config_query_parameters_.empty()) {
-      Http::Utility::QueryParams query_parameters =
-          Http::Utility::parseQueryString(headers.getPathValue());
+      Http::Utility::QueryParamsMulti query_parameters =
+          Http::Utility::QueryParamsMulti::parseQueryString(headers.getPathValue());
       matches &= ConfigUtility::matchQueryParams(query_parameters, config_query_parameters_);
     }
     return matches;

test/common/http/utility_test.cc

@@ -298,6 +298,9 @@ TEST(HttpUtility, testQueryParamModification) {
   EXPECT_EQ(params.toString(), "?a=1&a=2&b=3&c=4");
   params.overwrite("b", "foo");
   EXPECT_EQ(params.toString(), "?a=1&a=2&b=foo&c=4");
+  EXPECT_EQ(params.getFirstValue("a").value(), "1");
+  EXPECT_EQ(params.getFirstValue("b").value(), "foo");
+  EXPECT_FALSE(params.getFirstValue("d").has_value());
   params.remove("b");
   EXPECT_EQ(params.toString(), "?a=1&a=2&c=4");
   params.overwrite("a", "bar");

test/common/router/config_impl_test.cc

@@ -2704,6 +2704,20 @@ TEST_F(RouteMatcherTest, QueryParamMatchedRouting) {
               config.route(headers, 0)->routeEntry()->clusterName());
   }
 
+  { // Repeated parameter - match should only track the first, and match
+    Http::TestRequestHeaderMapImpl headers =
+        genHeaders("example.com", "/?debug3=foo&debug3=bar", "GET");
+    EXPECT_EQ("local_service_with_string_match_query_parameter",
+              config.route(headers, 0)->routeEntry()->clusterName());
+  }
+
+  { // Repeated parameter - match should only track the first, and not match
+    Http::TestRequestHeaderMapImpl headers =
+        genHeaders("example.com", "/?debug3=bar&debug3=foo", "GET");
+    EXPECT_EQ("local_service_without_query_parameters",
+              config.route(headers, 0)->routeEntry()->clusterName());
+  }
+
   {
     Http::TestRequestHeaderMapImpl headers = genHeaders("example.com", "/?debug=2", "GET");
     EXPECT_EQ("local_service_with_valueless_query_parameter",