http: make HeaderHashMethod hashing all the header values

docs/root/version_history/current.rst

@@ -143,6 +143,7 @@ New Features
 * http: added new runtime config `envoy.reloadable_features.check_unsupported_typed_per_filter_config`, the default value is true. When the value is true, envoy will reject virtual host-specific typed per filter config when the filter doesn't support it.
 * http: added the ability to preserve HTTP/1 header case across the proxy. See the :ref:`header casing <config_http_conn_man_header_casing>` documentation for more information.
 * http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false.
+* http: hash multiple header values instead of only hash the first header value. It can be disabled by setting the `envoy.reloadable_features.hash_multiple_header_values` runtime key to false. See the :ref:`HashPolicy's Header configuration <envoy_v3_api_msg_config.route.v3.RouteAction.HashPolicy.Header>` for more information.
 * json: introduced new JSON parser (https://github.com/nlohmann/json) to replace RapidJSON. The new parser is disabled by default. To test the new RapidJSON parser, enable the runtime feature `envoy.reloadable_features.remove_legacy_json`.
 * kill_request: :ref:`Kill Request <config_http_filters_kill_request>` Now supports bidirection killing.
 * loadbalancer: added the ability to specify the hash_key for a host when using a consistent hashing loadbalancer (ringhash, maglev) using the :ref:`LbEndpoint.Metadata <envoy_api_field_endpoint.LbEndpoint.metadata>` e.g.: ``"envoy.lb": {"hash_key": "..."}``.

source/common/common/hash.h

@@ -25,6 +25,24 @@ class HashUtil {
     return XXH64(input.data(), input.size(), seed);
   }
 
+  /**
+   * Return 64-bit hash from the xxHash algorithm.
+   * @param input supplies the absl::InlinedVector<absl::string_view> to hash.
+   * @param seed supplies the hash seed which defaults to 0.
+   * See https://github.com/Cyan4973/xxHash for details.
+   */
+  static uint64_t xxHash64(absl::InlinedVector<absl::string_view, 1>& input, uint64_t seed = 0) {
+    uint64_t hash = seed;
+    if (input.size() == 0) {
+      hash = XXH64(input.data(), input.size(), hash);
+    } else {
+      for (auto& i : input) {
+        hash = XXH64(i.data(), i.size(), hash);
+      }
+    }
+    return hash;
+  }
+
   /**
    * TODO(gsagula): extend xxHash to handle case-insensitive.
    *

source/common/http/hash_policy.cc

@@ -1,10 +1,13 @@
 #include "common/http/hash_policy.h"
 
+#include <string>
+
 #include "envoy/config/route/v3/route_components.pb.h"
 
 #include "common/common/matchers.h"
 #include "common/common/regex.h"
 #include "common/http/utility.h"
+#include "common/runtime/runtime_features.h"
 
 #include "absl/strings/str_cat.h"
 
@@ -39,14 +42,39 @@ class HeaderHashMethod : public HashMethodImplBase {
                                     const StreamInfo::FilterStateSharedPtr) const override {
     absl::optional<uint64_t> hash;
 
-    // TODO(mattklein123): Potentially hash on all headers.
     const auto header = headers.get(header_name_);
     if (!header.empty()) {
+      const bool check_multiple_values =
+          Runtime::runtimeFeatureEnabled("envoy.reloadable_features.hash_multiple_header_values");
+      absl::InlinedVector<std::string, 1> rewritten_header_values;
+      absl::InlinedVector<absl::string_view, 1> header_values;
+
+      size_t num_headers_to_hash = 1;
+      if (check_multiple_values) {
+        num_headers_to_hash = header.size();
+        header_values.reserve(num_headers_to_hash);
+      }
+
+      for (size_t i = 0; i < num_headers_to_hash; i++) {
+        header_values.push_back(header[i]->value().getStringView());
+      }
+
       if (regex_rewrite_ != nullptr) {
-        hash = HashUtil::xxHash64(regex_rewrite_->replaceAll(header[0]->value().getStringView(),
-                                                             regex_rewrite_substitution_));
+        rewritten_header_values.reserve(num_headers_to_hash);
+        for (auto& value : header_values) {
+          rewritten_header_values.push_back(
+              regex_rewrite_->replaceAll(value, regex_rewrite_substitution_));
+          value = rewritten_header_values.back();
+        }
+      }
+
+      if (check_multiple_values && (num_headers_to_hash > 1)) {
+        // Ensure generating same hash value for different order header values.
+        // For example, generates the same hash value for {"foo","bar"} and {"bar","foo"}
+        std::sort(header_values.begin(), header_values.end());
+        hash = HashUtil::xxHash64(header_values);
       } else {
-        hash = HashUtil::xxHash64(header[0]->value().getStringView());
+        hash = HashUtil::xxHash64(header_values[0]);
       }
     }
     return hash;

source/common/runtime/runtime_features.cc

@@ -69,6 +69,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.enable_compression_without_content_length_header",
     "envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling",
     "envoy.reloadable_features.grpc_json_transcoder_adhere_to_buffer_limits",
+    "envoy.reloadable_features.hash_multiple_header_values",
     "envoy.reloadable_features.hcm_stream_error_on_invalid_message",
     "envoy.reloadable_features.health_check.graceful_goaway_handling",
     "envoy.reloadable_features.health_check.immediate_failure_exclude_from_cluster",

test/common/common/hash_test.cc

@@ -11,6 +11,13 @@ TEST(Hash, xxHash) {
   EXPECT_EQ(17241709254077376921U, HashUtil::xxHash64(""));
 }
 
+TEST(Hash, xxHashWithVector) {
+  absl::InlinedVector<absl::string_view, 1> v{"foo", "bar"};
+  EXPECT_EQ(17745830980996999794U, HashUtil::xxHash64(v));
+  absl::InlinedVector<absl::string_view, 1> empty_vector;
+  EXPECT_EQ(17241709254077376921U, HashUtil::xxHash64(empty_vector));
+}
+
 TEST(Hash, djb2CaseInsensitiveHash) {
   EXPECT_EQ(211616621U, HashUtil::djb2CaseInsensitiveHash("foo"));
   EXPECT_EQ(211611524U, HashUtil::djb2CaseInsensitiveHash("bar"));

test/common/http/async_client_impl_test.cc

@@ -314,7 +314,7 @@ TEST_F(AsyncClientImplTest, BasicHashPolicy) {
           Invoke([&](Upstream::ResourcePriority, auto,
                      Upstream::LoadBalancerContext* context) -> Http::ConnectionPool::Instance* {
             // this is the hash of :path header value "/"
-            EXPECT_EQ(16761507700594825962UL, context->computeHashKey().value());
+            EXPECT_NE(0, context->computeHashKey().value());
             return &cm_.thread_local_cluster_.conn_pool_;
           }));
 

test/common/router/config_impl_test.cc

@@ -2382,6 +2382,16 @@ class RouterMatcherHashPolicyTest : public testing::Test, public ConfigImplTestB
     return *config_;
   }
 
+  absl::optional<uint64_t> generateHash(const std::vector<absl::string_view>& header_values) {
+    Http::TestRequestHeaderMapImpl headers = genHeaders("www.lyft.com", "/foo", "GET");
+    for (auto& value : header_values) {
+      headers.addCopy("foo_header", std::string(value));
+    }
+    Router::RouteConstSharedPtr route = config().route(headers, 0);
+    return route->routeEntry()->hashPolicy()->generateHash(nullptr, headers, add_cookie_nop_,
+                                                           nullptr);
+  }
+
   envoy::config::route::v3::RouteConfiguration route_config_;
   Http::HashPolicy::AddCookieCallback add_cookie_nop_;
 
@@ -2390,6 +2400,9 @@ class RouterMatcherHashPolicyTest : public testing::Test, public ConfigImplTestB
 };
 
 TEST_F(RouterMatcherHashPolicyTest, HashHeaders) {
+  TestScopedRuntime scoped_runtime;
+  Runtime::LoaderSingleton::getExisting()->mergeValues(
+      {{"envoy.reloadable_features.hash_multiple_header_values", "false"}});
   firstRouteHashPolicy()->mutable_header()->set_header_name("foo_header");
   {
     Http::TestRequestHeaderMapImpl headers = genHeaders("www.lyft.com", "/foo", "GET");
@@ -2411,7 +2424,28 @@ TEST_F(RouterMatcherHashPolicyTest, HashHeaders) {
   }
 }
 
+TEST_F(RouterMatcherHashPolicyTest, HashHeadersWithMultipleValues) {
+  firstRouteHashPolicy()->mutable_header()->set_header_name("foo_header");
+  {
+    EXPECT_FALSE(generateHash({}));
+    EXPECT_TRUE(generateHash({"bar"}));
+
+    EXPECT_NE(0, generateHash({"bar", "foo"}));
+    EXPECT_EQ(generateHash({"bar", "foo"}), generateHash({"bar", "foo"})); // deterministic
+    EXPECT_EQ(generateHash({"bar", "foo"}), generateHash({"foo", "bar"})); // order independent
+    EXPECT_NE(generateHash({"abcd", "ef"}), generateHash({"abc", "def"}));
+  }
+  {
+    Http::TestRequestHeaderMapImpl headers = genHeaders("www.lyft.com", "/bar", "GET");
+    Router::RouteConstSharedPtr route = config().route(headers, 0);
+    EXPECT_EQ(nullptr, route->routeEntry()->hashPolicy());
+  }
+}
+
 TEST_F(RouterMatcherHashPolicyTest, HashHeadersRegexSubstitution) {
+  TestScopedRuntime scoped_runtime;
+  Runtime::LoaderSingleton::getExisting()->mergeValues(
+      {{"envoy.reloadable_features.hash_multiple_header_values", "false"}});
   // Apply a regex substitution before hashing.
   auto* header = firstRouteHashPolicy()->mutable_header();
   header->set_header_name(":path");
@@ -2432,6 +2466,26 @@ TEST_F(RouterMatcherHashPolicyTest, HashHeadersRegexSubstitution) {
   }
 }
 
+TEST_F(RouterMatcherHashPolicyTest, HashHeadersRegexSubstitutionWithMultipleValues) {
+  // Apply a regex substitution before hashing.
+  auto* header = firstRouteHashPolicy()->mutable_header();
+  header->set_header_name("foo_header");
+  auto* regex_spec = header->mutable_regex_rewrite();
+  regex_spec->set_substitution("\\1");
+  auto* pattern = regex_spec->mutable_pattern();
+  pattern->mutable_google_re2();
+  pattern->set_regex("^/(\\w+)$");
+  {
+    EXPECT_FALSE(generateHash({}));
+    EXPECT_TRUE(generateHash({"/bar"}));
+
+    EXPECT_NE(0, generateHash({"/bar", "/foo"}));
+    EXPECT_EQ(generateHash({"bar", "foo"}), generateHash({"/bar", "/foo"})); // deterministic
+    EXPECT_EQ(generateHash({"bar", "foo"}), generateHash({"/foo", "/bar"})); // order independent
+    EXPECT_NE(generateHash({"abcd", "ef"}), generateHash({"/abc", "/def"}));
+  }
+}
+
 class RouterMatcherCookieHashPolicyTest : public RouterMatcherHashPolicyTest {
 public:
   RouterMatcherCookieHashPolicyTest() {