dfp: Don't filter IP address families from the DNS cache (envoyproxy#39037)

In Mobile, whenever there is a network change, there's a check to see if
there is IPv6 connectivity on the current network. If there is no IPv6
connectivity and the `dns_cache_set_ip_version_to_remove` runtime guard
is true, the IPv6 addresses would get filtered out from the DNS lookup
response.

This is problematic because if there is a network change and the device
now has IPv6 connectivity, the DNS cache will have filtered out the IPv6
addresses already and won't be able to use them to connect to the peer.

This change fixes the issue by storing all resolved IP addresses in the
DNS cache, and only filtering out the IP address families that don't
have connectivity in the DFP cluster. This has the further benefit of
avoiding having to re-resolve DNS host entries whenever there is a
network change.

The change is guarded by
`envoy.reloadable_features.dns_cache_filter_unusable_ip_version`, which
defaults to false.

Risk Level: low (runtime guarded)
Testing: Unit/Integration
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Runtime guard:
envoy.reloadable_features.dns_cache_filter_unusable_ip_version

---------

Signed-off-by: Ali Beyad <abeyad@google.com>

Author: abeyad

mobile/library/common/internal_engine.cc

@@ -381,7 +381,9 @@ void InternalEngine::handleNetworkChange(const int network_type, const bool has_
   envoy_netconf_t configuration =
       Network::ConnectivityManagerImpl::setPreferredNetwork(network_type);
   if (Runtime::runtimeFeatureEnabled(
-          "envoy.reloadable_features.dns_cache_set_ip_version_to_remove")) {
+          "envoy.reloadable_features.dns_cache_set_ip_version_to_remove") ||
+      Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.dns_cache_filter_unusable_ip_version")) {
     // The IP version to remove flag must be set first before refreshing the DNS cache so that
     // the DNS cache will be updated with whether or not the IPv6 addresses will need to be
     // removed.

mobile/test/common/integration/client_integration_test.cc

@@ -1452,6 +1452,21 @@ TEST_P(ClientIntegrationTest, OnNetworkChanged) {
   }
 }
 
+// This test is simply to test the IPv6 connectivity check and DNS refresh and make sure the code
+// doesn't crash. It doesn't really test the actual network change event, but it does ensure that
+// requests/responses still work in the presence of IP version filtering.
+TEST_P(ClientIntegrationTest, OnNetworkChangedFilterUnsableIps) {
+  builder_.addRuntimeGuard("dns_cache_set_ip_version_to_remove", false);
+  builder_.addRuntimeGuard("dns_cache_filter_unusable_ip_version", true);
+  builder_.setDisableDnsRefreshOnNetworkChange(true);
+  initialize();
+  internalEngine()->onDefaultNetworkChanged(1);
+  basicTest();
+  if (upstreamProtocol() == Http::CodecType::HTTP1) {
+    ASSERT_EQ(cc_.on_complete_received_byte_count_, 67);
+  }
+}
+
 // This test is simply to test the IPv6 connectivity check and DNS refresh and make sure the code
 // doesn't crash. It doesn't really test the actual network change event.
 TEST_P(ClientIntegrationTest, OnNetworkChangeEvent) {

source/common/runtime/runtime_features.cc

@@ -139,6 +139,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_reresolve_if_no_connections);
 FALSE_RUNTIME_GUARD(envoy_restart_features_xds_failover_support);
 // TODO(fredyw): evaluate and either make this a config knob or remove.
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_dns_cache_set_ip_version_to_remove);
+// TODO(abeyad): evaluate and either make this the default or remove.
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_dns_cache_filter_unusable_ip_version);
 // TODO(alyssawilk): evaluate and make this a config knob or remove.
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_reset_brokenness_on_nework_change);
 // TODO(alyssawilk): evaluate and make this a config knob or remove.

source/extensions/clusters/dynamic_forward_proxy/cluster.cc

@@ -288,17 +288,15 @@ absl::Status Cluster::addOrUpdateHost(
       //                     cluster would create multiple logical hosts based on those addresses.
       //                     We will leave this is a follow up depending on need.
       ASSERT(host_info == host_map_it->second.shared_host_info_);
-      ASSERT(host_map_it->second.shared_host_info_->address() !=
-             host_map_it->second.logical_host_->address());
       ENVOY_LOG(debug, "updating dfproxy cluster host address '{}'", host);
       host_map_it->second.logical_host_->setNewAddresses(
-          host_info->address(), host_info->addressList(), dummy_lb_endpoint_);
+          host_info->address(), host_info->addressList(/*filtered=*/true), dummy_lb_endpoint_);
       return absl::OkStatus();
     }
 
     ENVOY_LOG(debug, "adding new dfproxy cluster host '{}'", host);
     auto host_or_error = Upstream::LogicalHost::create(
-        info(), std::string{host}, host_info->address(), host_info->addressList(),
+        info(), std::string{host}, host_info->address(), host_info->addressList(/*filtered=*/true),
         dummy_locality_lb_endpoint_, dummy_lb_endpoint_, nullptr, time_source_);
     RETURN_IF_NOT_OK_REF(host_or_error.status());
 

source/extensions/common/dynamic_forward_proxy/dns_cache.h

@@ -50,8 +50,13 @@ class DnsHostInfo {
   /**
    * Returns the host's currently resolved address. These addresses may change periodically due to
    * async re-resolution.
+   *
+   * If `filtered` is true and the runtime guard
+   * `envoy.reloadable_features.dns_cache_filter_unusable_ip_version` is true, return a filtered
+   * list where the IP addresses of IP families unsupported on the current network are removed.
    */
-  virtual std::vector<Network::Address::InstanceConstSharedPtr> addressList() const PURE;
+  virtual std::vector<Network::Address::InstanceConstSharedPtr>
+  addressList(bool filtered) const PURE;
 
   /**
    * Returns the host that was actually resolved via DNS. If port was originally specified it will
@@ -282,6 +287,11 @@ class DnsCache {
    */
   virtual void setIpVersionToRemove(absl::optional<Network::Address::IpVersion> ip_version) PURE;
 
+  /**
+   * Gets the `IpVersion` addresses to be removed from the DNS response.
+   */
+  virtual absl::optional<Network::Address::IpVersion> getIpVersionToRemove() PURE;
+
   /**
    * Stops the DNS cache background tasks by canceling the pending queries and stopping the timeout
    * and refresh timers. This function can be useful when the network is unavailable, such as when

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

@@ -345,8 +345,40 @@ void DnsCacheImpl::forceRefreshHosts() {
 }
 
 void DnsCacheImpl::setIpVersionToRemove(absl::optional<Network::Address::IpVersion> ip_version) {
+  bool has_changed = false;
+  {
+    absl::MutexLock lock{&ip_version_to_remove_lock_};
+    has_changed = ip_version_to_remove_ != ip_version;
+    ip_version_to_remove_ = ip_version;
+  }
+
+  if (has_changed && Runtime::runtimeFeatureEnabled(
+                         "envoy.reloadable_features.dns_cache_filter_unusable_ip_version")) {
+    // The IP version to remove has changed, so we need to refresh all logical hosts in the DFP
+    // cluster so they filter out the unsupported/unusable IP addresses from their address list.
+    absl::ReaderMutexLock reader_lock{&primary_hosts_lock_};
+    for (auto& primary_host : primary_hosts_) {
+      for (auto* callbacks : update_callbacks_) {
+        auto status = callbacks->callbacks_.onDnsHostAddOrUpdate(primary_host.first,
+                                                                 primary_host.second->host_info_);
+        if (!status.ok()) {
+          // TODO(abeyad): Do something better with a failure status.
+          ENVOY_LOG(warn, "Failed to update DFP host after IP version update due to {}",
+                    status.message());
+        }
+      }
+    }
+    ENVOY_LOG(debug, "refresh all {} logical hosts in host map, unsupported IP version {}",
+              primary_hosts_.size(),
+              ip_version.has_value()
+                  ? (*ip_version == Network::Address::IpVersion::v4 ? "v4" : "v6")
+                  : "none");
+  }
+}
+
+absl::optional<Network::Address::IpVersion> DnsCacheImpl::getIpVersionToRemove() {
   absl::MutexLock lock{&ip_version_to_remove_lock_};
-  ip_version_to_remove_ = ip_version;
+  return ip_version_to_remove_;
 }
 
 void DnsCacheImpl::stop() {
@@ -464,13 +496,6 @@ void DnsCacheImpl::finishResolve(const std::string& host,
     }
   }
 
-  // If the DNS resolver successfully resolved with an empty response list, the dns cache does not
-  // update. This ensures that a potentially previously resolved address does not stabilize back to
-  // 0 hosts.
-  const auto new_address =
-      !response.empty() ? Network::Utility::getAddressWithPort(
-                              *(response.front().addrInfo().address_), primary_host_info->port_)
-                        : nullptr;
   auto address_list = DnsUtils::generateAddressList(response, primary_host_info->port_);
   // Only the change the address if:
   // 1) The new address is valid &&
@@ -487,10 +512,14 @@ void DnsCacheImpl::finishResolve(const std::string& host,
   }
   std::chrono::seconds dns_ttl =
       std::chrono::duration_cast<std::chrono::seconds>(refresh_interval_);
-  if (new_address) {
+
+  // If the DNS resolver successfully resolved with an empty response list, the dns cache does not
+  // update. This ensures that a potentially previously resolved address does not stabilize back to
+  // 0 hosts.
+  if (!address_list.empty()) {
     // Update the cache entry and staleness any time the ttl changes.
     if (!from_cache) {
-      addCacheEntry(host, new_address, address_list, response.front().addrInfo().ttl_);
+      addCacheEntry(host, address_list, response.front().addrInfo().ttl_);
     }
     // Arbitrarily cap DNS re-resolution at min_refresh_interval_ to avoid constant DNS queries.
     dns_ttl = std::max<std::chrono::seconds>(
@@ -499,22 +528,23 @@ void DnsCacheImpl::finishResolve(const std::string& host,
     primary_host_info->host_info_->updateStale(resolution_time.value(), dns_ttl);
   }
 
-  bool changed_to_non_null_address =
-      (new_address != nullptr &&
-       (current_address == nullptr || *current_address != *new_address ||
-        DnsUtils::listChanged(address_list, primary_host_info->host_info_->addressList())));
+  bool should_update_cache =
+      !address_list.empty() &&
+      DnsUtils::listChanged(address_list,
+                            primary_host_info->host_info_->addressList(/*filtered=*/false));
   // If this was a proxy lookup it's OK to send a null address resolution as
   // long as this isn't a transition from non-null to null address.
-  bool proxying_and_didnt_unresolve = is_proxy_lookup && !current_address;
+  should_update_cache |= is_proxy_lookup && !current_address;
 
-  if (changed_to_non_null_address || proxying_and_didnt_unresolve) {
+  if (should_update_cache) {
+    primary_host_info->host_info_->setAddresses(std::move(address_list), details_with_maybe_trace,
+                                                status);
     ENVOY_LOG_EVENT(debug, "dns_cache_update_address",
                     "host '{}' address has changed from {} to {}", host,
                     current_address ? current_address->asStringView() : "<empty>",
-                    new_address ? new_address->asStringView() : "<empty>");
-    primary_host_info->host_info_->setAddresses(new_address, std::move(address_list));
-    primary_host_info->host_info_->setDetails(details_with_maybe_trace);
-    primary_host_info->host_info_->setResolutionStatus(status);
+                    primary_host_info->host_info_->address()
+                        ? primary_host_info->host_info_->address()->asStringView()
+                        : "<empty>");
 
     absl::Status host_status = runAddUpdateCallbacks(host, primary_host_info->host_info_);
     ENVOY_BUG(host_status.ok(),
@@ -616,8 +646,7 @@ DnsCacheImpl::PrimaryHostInfo::PrimaryHostInfo(DnsCacheImpl& parent,
     : parent_(parent), port_(port),
       refresh_timer_(parent.main_thread_dispatcher_.createTimer(refresh_timer_cb)),
       timeout_timer_(parent.main_thread_dispatcher_.createTimer(timeout_timer_cb)),
-      host_info_(std::make_shared<DnsHostInfoImpl>(parent.main_thread_dispatcher_.timeSource(),
-                                                   host_to_resolve, is_ip_address)),
+      host_info_(std::make_shared<DnsHostInfoImpl>(parent, host_to_resolve, is_ip_address)),
       failure_backoff_strategy_(
           Config::Utility::prepareDnsRefreshStrategy<
               envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig>(
@@ -632,23 +661,18 @@ DnsCacheImpl::PrimaryHostInfo::~PrimaryHostInfo() {
 }
 
 void DnsCacheImpl::addCacheEntry(
-    const std::string& host, const Network::Address::InstanceConstSharedPtr& address,
+    const std::string& host,
     const std::vector<Network::Address::InstanceConstSharedPtr>& address_list,
     const std::chrono::seconds ttl) {
-  if (!key_value_store_) {
+  if (!key_value_store_ || address_list.empty()) {
     return;
   }
   MonotonicTime now = main_thread_dispatcher_.timeSource().monotonicTime();
   uint64_t seconds_since_epoch =
       std::chrono::duration_cast<std::chrono::seconds>(now.time_since_epoch()).count();
-  std::string value;
-  if (address_list.empty()) {
-    value = absl::StrCat(address->asString(), "|", ttl.count(), "|", seconds_since_epoch);
-  } else {
-    value = absl::StrJoin(address_list, "\n", [&](std::string* out, const auto& addr) {
-      absl::StrAppend(out, addr->asString(), "|", ttl.count(), "|", seconds_since_epoch);
-    });
-  }
+  std::string value = absl::StrJoin(address_list, "\n", [&](std::string* out, const auto& addr) {
+    absl::StrAppend(out, addr->asString(), "|", ttl.count(), "|", seconds_since_epoch);
+  });
   key_value_store_->addOrUpdate(host, value, absl::nullopt);
 }
 
@@ -730,6 +754,116 @@ void DnsCacheImpl::loadCacheEntries(
   key_value_store_->iterate(load);
 }
 
+DnsCacheImpl::DnsHostInfoImpl::DnsHostInfoImpl(DnsCacheImpl& parent,
+                                               absl::string_view resolved_host, bool is_ip_address)
+    : parent_(parent), resolved_host_(resolved_host), is_ip_address_(is_ip_address),
+      stale_at_time_(parent_.main_thread_dispatcher_.timeSource().monotonicTime()) {
+  touch();
+}
+
+Network::Address::InstanceConstSharedPtr DnsCacheImpl::DnsHostInfoImpl::address() const {
+  const bool filter_unusable_ips = Runtime::runtimeFeatureEnabled(
+      "envoy.reloadable_features.dns_cache_filter_unusable_ip_version");
+  absl::optional<Network::Address::IpVersion> ip_version_to_remove = parent_.getIpVersionToRemove();
+  absl::ReaderMutexLock lock{&resolve_lock_};
+  for (const auto& address : address_list_) {
+    // If not filtering unusable IPs, OR if there is no IP version to remove, OR if the address is
+    // not of the IP family to remove, use the address. This means if the
+    // `dns_cache_filter_unusable_ip_version` feature is off OR there is no set IP family to remove,
+    // the first address in the list will automatically be returned.
+    if (!filter_unusable_ips || !ip_version_to_remove ||
+        address->ip()->version() != *ip_version_to_remove) {
+      return address;
+    }
+  }
+  // If no address was returned yet, return the first address in the list, if any.
+  return !address_list_.empty() ? address_list_.front() : nullptr;
+}
+
+std::vector<Network::Address::InstanceConstSharedPtr>
+DnsCacheImpl::DnsHostInfoImpl::addressList(const bool filtered) const {
+  if (filtered && Runtime::runtimeFeatureEnabled(
+                      "envoy.reloadable_features.dns_cache_filter_unusable_ip_version")) {
+    auto ip_version_to_remove = parent_.getIpVersionToRemove();
+    if (ip_version_to_remove.has_value()) {
+      std::vector<Network::Address::InstanceConstSharedPtr> ret;
+      absl::ReaderMutexLock lock{&resolve_lock_};
+      for (const auto& address : address_list_) {
+        if (address->ip()->version() != *ip_version_to_remove) {
+          ret.push_back(address);
+        }
+      }
+      return ret;
+    }
+  }
+  std::vector<Network::Address::InstanceConstSharedPtr> ret;
+  absl::ReaderMutexLock lock{&resolve_lock_};
+  ret = address_list_;
+  return ret;
+}
+
+const std::string& DnsCacheImpl::DnsHostInfoImpl::resolvedHost() const { return resolved_host_; }
+
+bool DnsCacheImpl::DnsHostInfoImpl::isIpAddress() const { return is_ip_address_; }
+
+void DnsCacheImpl::DnsHostInfoImpl::touch() {
+  last_used_time_ = parent_.main_thread_dispatcher_.timeSource().monotonicTime().time_since_epoch();
+}
+
+void DnsCacheImpl::DnsHostInfoImpl::updateStale(MonotonicTime resolution_time,
+                                                std::chrono::seconds ttl) {
+  stale_at_time_ = resolution_time + ttl;
+}
+
+bool DnsCacheImpl::DnsHostInfoImpl::isStale() {
+  return parent_.main_thread_dispatcher_.timeSource().monotonicTime() >
+         static_cast<MonotonicTime>(stale_at_time_);
+}
+
+void DnsCacheImpl::DnsHostInfoImpl::setAddresses(
+    std::vector<Network::Address::InstanceConstSharedPtr>&& list, absl::string_view details,
+    Network::DnsResolver::ResolutionStatus resolution_status) {
+  absl::WriterMutexLock lock{&resolve_lock_};
+  address_list_ = std::move(list);
+  details_ = details;
+  resolution_status_ = resolution_status;
+}
+
+void DnsCacheImpl::DnsHostInfoImpl::setDetails(absl::string_view details) {
+  absl::WriterMutexLock lock{&resolve_lock_};
+  details_ = details;
+}
+
+std::string DnsCacheImpl::DnsHostInfoImpl::details() {
+  absl::ReaderMutexLock lock{&resolve_lock_};
+  return details_;
+}
+
+std::chrono::steady_clock::duration DnsCacheImpl::DnsHostInfoImpl::lastUsedTime() const {
+  return last_used_time_.load();
+}
+
+bool DnsCacheImpl::DnsHostInfoImpl::firstResolveComplete() const {
+  absl::ReaderMutexLock lock{&resolve_lock_};
+  return first_resolve_complete_;
+}
+
+void DnsCacheImpl::DnsHostInfoImpl::setFirstResolveComplete() {
+  absl::WriterMutexLock lock{&resolve_lock_};
+  first_resolve_complete_ = true;
+}
+
+void DnsCacheImpl::DnsHostInfoImpl::setResolutionStatus(
+    Network::DnsResolver::ResolutionStatus resolution_status) {
+  absl::WriterMutexLock lock{&resolve_lock_};
+  resolution_status_ = resolution_status;
+}
+
+Network::DnsResolver::ResolutionStatus DnsCacheImpl::DnsHostInfoImpl::resolutionStatus() const {
+  absl::WriterMutexLock lock{&resolve_lock_};
+  return resolution_status_;
+}
+
 } // namespace DynamicForwardProxy
 } // namespace Common
 } // namespace Extensions