router: make IP hash policy ignore downstream port (envoyproxy#3457)

Downstream clients initiate connections using ephemeral ports, which are
included in the Address::Instance::asString implementation. Including
the ports breaks client IP hashing for repeated short-lived connections,
though long-lived HTTP2 connections are unaffected. This patch changes
the IP hash policy to ignore ports and hash only on the IP address.

Signed-off-by: Alex Konradi <akonradi@google.com>

docs/root/intro/version_history.rst

@@ -72,6 +72,8 @@ Version history
   already been proxied downstream when the timeout occurs. Previously, the response would be reset 
   leading to either an HTTP/2 reset or an HTTP/1 closed connection and a partial response. Now, the 
   timeout will be ignored and the response will continue to proxy up to the global request timeout. 
+* router: changed the behavior of :ref:`source IP routing <envoy_api_field_route.RouteAction.HashPolicy.ConnectionProperties.source_ip>`
+  to ignore the source port.
 * sockets: added :ref:`capture transport socket extension <operations_traffic_capture>` to support
   recording plain text traffic and PCAP generation.
 * sockets: added `IP_FREEBIND` socket option support for :ref:`listeners

include/envoy/router/router.h

@@ -310,17 +310,17 @@ class HashPolicy {
       AddCookieCallback;
 
   /**
-   * @param downstream_address contains the address of the connected client host, or an
-   * empty string if the request is initiated from within this host
+   * @param downstream_address is the address of the connected client host, or nullptr if the
+   * request is initiated from within this host
    * @param headers stores the HTTP headers for the stream
    * @param add_cookie is called to add a set-cookie header on the reply sent to the downstream
    * host
    * @return absl::optional<uint64_t> an optional hash value to route on. A hash value might not be
    * returned if for example the specified HTTP header does not exist.
    */
-  virtual absl::optional<uint64_t> generateHash(const std::string& downstream_address,
-                                                const Http::HeaderMap& headers,
-                                                AddCookieCallback add_cookie) const PURE;
+  virtual absl::optional<uint64_t>
+  generateHash(const Network::Address::Instance* downstream_address, const Http::HeaderMap& headers,
+               AddCookieCallback add_cookie) const PURE;
 };
 
 class MetadataMatchCriterion {

source/common/router/config_impl.cc

@@ -77,7 +77,8 @@ class HeaderHashMethod : public HashPolicyImpl::HashMethod {
 public:
   HeaderHashMethod(const std::string& header_name) : header_name_(header_name) {}
 
-  absl::optional<uint64_t> evaluate(const std::string&, const Http::HeaderMap& headers,
+  absl::optional<uint64_t> evaluate(const Network::Address::Instance*,
+                                    const Http::HeaderMap& headers,
                                     const HashPolicy::AddCookieCallback) const override {
     absl::optional<uint64_t> hash;
 
@@ -96,7 +97,8 @@ class CookieHashMethod : public HashPolicyImpl::HashMethod {
 public:
   CookieHashMethod(const std::string& key, long ttl) : key_(key), ttl_(ttl) {}
 
-  absl::optional<uint64_t> evaluate(const std::string&, const Http::HeaderMap& headers,
+  absl::optional<uint64_t> evaluate(const Network::Address::Instance*,
+                                    const Http::HeaderMap& headers,
                                     const HashPolicy::AddCookieCallback add_cookie) const override {
     absl::optional<uint64_t> hash;
     std::string value = Http::Utility::parseCookieValue(headers, key_);
@@ -118,13 +120,21 @@ class CookieHashMethod : public HashPolicyImpl::HashMethod {
 
 class IpHashMethod : public HashPolicyImpl::HashMethod {
 public:
-  absl::optional<uint64_t> evaluate(const std::string& downstream_addr, const Http::HeaderMap&,
+  absl::optional<uint64_t> evaluate(const Network::Address::Instance* downstream_addr,
+                                    const Http::HeaderMap&,
                                     const HashPolicy::AddCookieCallback) const override {
-    absl::optional<uint64_t> hash;
-    if (!downstream_addr.empty()) {
-      hash = HashUtil::xxHash64(downstream_addr);
+    if (downstream_addr == nullptr) {
+      return absl::nullopt;
     }
-    return hash;
+    auto* downstream_ip = downstream_addr->ip();
+    if (downstream_ip == nullptr) {
+      return absl::nullopt;
+    }
+    const auto& downstream_addr_str = downstream_ip->addressAsString();
+    if (downstream_addr_str.empty()) {
+      return absl::nullopt;
+    }
+    return HashUtil::xxHash64(downstream_addr_str);
   }
 };
 
@@ -155,9 +165,10 @@ HashPolicyImpl::HashPolicyImpl(
   }
 }
 
-absl::optional<uint64_t> HashPolicyImpl::generateHash(const std::string& downstream_addr,
-                                                      const Http::HeaderMap& headers,
-                                                      const AddCookieCallback add_cookie) const {
+absl::optional<uint64_t>
+HashPolicyImpl::generateHash(const Network::Address::Instance* downstream_addr,
+                             const Http::HeaderMap& headers,
+                             const AddCookieCallback add_cookie) const {
   absl::optional<uint64_t> hash;
   for (const HashMethodPtr& hash_impl : hash_impls_) {
     const absl::optional<uint64_t> new_hash =

source/common/router/config_impl.h

@@ -222,14 +222,14 @@ class HashPolicyImpl : public HashPolicy {
                      hash_policy);
 
   // Router::HashPolicy
-  absl::optional<uint64_t> generateHash(const std::string& downstream_addr,
+  absl::optional<uint64_t> generateHash(const Network::Address::Instance* downstream_addr,
                                         const Http::HeaderMap& headers,
                                         const AddCookieCallback add_cookie) const override;
 
   class HashMethod {
   public:
     virtual ~HashMethod() {}
-    virtual absl::optional<uint64_t> evaluate(const std::string& downstream_addr,
+    virtual absl::optional<uint64_t> evaluate(const Network::Address::Instance* downstream_addr,
                                               const Http::HeaderMap& headers,
                                               const AddCookieCallback add_cookie) const PURE;
   };

source/common/router/router.h

@@ -155,7 +155,7 @@ class Filter : Logger::Loggable<Logger::Id::router>,
       auto hash_policy = route_entry_->hashPolicy();
       if (hash_policy) {
         return hash_policy->generateHash(
-            callbacks_->requestInfo().downstreamRemoteAddress()->asString(), *downstream_headers_,
+            callbacks_->requestInfo().downstreamRemoteAddress().get(), *downstream_headers_,
             [this](const std::string& key, std::chrono::seconds max_age) {
               return addDownstreamSetCookie(key, max_age);
             });
@@ -376,5 +376,5 @@ class ProdFilter : public Filter {
                                  Upstream::ResourcePriority priority) override;
 };
 
-} // Router
+} // namespace Router
 } // namespace Envoy