DNS Cache keeping resolved-but-refreshed-with-error entries forever when disable_dns_refresh_on_failure=true

[skandalfo]

Title: DNS Cache keeping resolved-but-refreshed-with-error entries forever when disable_dns_refresh_on_failure=true

Description:

During a production deployment we observed the dynamic_forward_proxy module performing connections to an old IP address for a particular hostname past its entry TTL.

We've traced this to an issue where when disable_dns_refresh_on_failure=true has been configured, an already-resolved entry in the DNS Cache that fails to resolve during a further refresh is never purged from the cache, and remains being used forever with the last successfully-resolved IP address.

This has been verified on v1.37.5, but the bug is likely still extant at HEAD.

Analysis:

1. The cache will just return cached entries as-is without checking for any kind of expiration unless someone explicitly forces a refresh:

   envoy/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

   Lines 128 to 166 in cd142b9

   	DnsCacheImpl::LoadDnsCacheEntryResult
   	DnsCacheImpl::loadDnsCacheEntryWithForceRefresh(absl::string_view raw_host, uint16_t default_port,
   	bool is_proxy_lookup, bool force_refresh,
   	LoadDnsCacheEntryCallbacks& callbacks) {
   	std::string host = DnsHostInfo::normalizeHostForDfp(raw_host, default_port);
   	ENVOY_LOG(debug, "thread local lookup for host '{}' {}", host,
   	is_proxy_lookup ? "proxy mode " : "");
   	ThreadLocalHostInfo& tls_host_info = *tls_slot_;
   	bool is_overflow = false;
   	std::optional<DnsHostInfoSharedPtr> host_info = std::nullopt;
   	bool ignore_cached_entries = force_refresh;
   	{
   	absl::ReaderMutexLock read_lock{primary_hosts_lock_};
   	is_overflow = primary_hosts_.size() >= max_hosts_;
   	auto tls_host = primary_hosts_.find(host);
   	if (tls_host != primary_hosts_.end() && tls_host->second->host_info_->firstResolveComplete()) {
   	host_info = tls_host->second->host_info_;
   	}
   	}
   	if (host_info) {
   	ENVOY_LOG(debug, "cache hit for host '{}'", host);
   	if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.reresolve_null_addresses") &&
   	!is_proxy_lookup && *host_info && (*host_info)->address() == nullptr) {
   	ENVOY_LOG(debug, "ignoring null address cache hit for miss for host '{}'", host);
   	ignore_cached_entries = true;
   	}
   	if (config_.disable_dns_refresh_on_failure() && !is_proxy_lookup &&
   	(*host_info)->resolutionStatus() == Network::DnsResolver::ResolutionStatus::Failure) {
   	ENVOY_LOG(debug, "ignoring failed address cache hit for miss for host '{}'", host);
   	ignore_cached_entries = true;
   	}
   	if (!ignore_cached_entries) {
   	return {LoadDnsCacheEntryStatus::InCache, nullptr, host_info};
   	}
   	}

2. The "normal" way to expire something is when a refresh timer calls onReResolveAlarm() and hits this:

   envoy/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

   Lines 323 to 326 in cd142b9

   	if ((now_duration - last_used_time) >= host_ttl_) {
   	ENVOY_LOG(debug, "host='{}' TTL expired, removing", host);
   	removeHost(host, primary_host, true);
   	} else {

3. The entry's last usage time is updated externally (we can assume the problematic item was in use continually, hence not expiring):

   envoy/source/extensions/clusters/dynamic_forward_proxy/cluster.cc

   Line 513 in 2dc52eb

   host_it->second.shared_host_info_->touch();

4. When the first resolution completes normally (first attempt) the refresh timer is re-armed:

   envoy/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

   Line 614 in cd142b9

   primary_host_info->refresh_timer_->enableTimer(dns_ttl);

5. If a subsequent refresh fails, as there already was an address resolved, we don't update resolutionStatus, but leave it containing the previously successful state:

   envoy/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

   Lines 540 to 548 in cd142b9

   	// Only the change the address if:
   	// 1) The new address is valid &&
   	// 2a) The host doesn't yet have an address ||
   	// 2b) The host has a changed address.
   	//
   	// This means that once a host gets an address it will stick even in the case of a subsequent
   	// resolution failure.
   	bool address_changed = false;
   	auto current_address = primary_host_info->host_info_->address();

   and

   envoy/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

   Lines 594 to 600 in cd142b9

   	} else if (current_address == nullptr) {
   	// We only set details here if current address is null because but
   	// non-null->null resolutions we don't update the address so will use a
   	// previously resolved address + details.
   	primary_host_info->host_info_->setDetails(details_with_maybe_trace);
   	primary_host_info->host_info_->setResolutionStatus(status);
   	}

6. So the code won't ignore the cached entry, but will keep returning it:

   envoy/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

   Lines 158 to 162 in cd142b9

   	if (config_.disable_dns_refresh_on_failure() && !is_proxy_lookup &&
   	(*host_info)->resolutionStatus() == Network::DnsResolver::ResolutionStatus::Failure) {
   	ENVOY_LOG(debug, "ignoring failed address cache hit for miss for host '{}'", host);
   	ignore_cached_entries = true;
   	}

7. Also, if we got the failure result, and disable_dns_refresh_on_failure is set, we won't re-arm the refresh timer in any code path:

   envoy/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

   Lines 617 to 646 in cd142b9

   	} else {
   	if (!config_.disable_dns_refresh_on_failure()) {
   	// Cap the failure backoff so the next re-resolve alarm fires no later than when the host
   	// becomes eligible for eviction. Without this cap, a touch() that lands just before
   	// onReResolveAlarm leaves the host active and schedules the next check at the full backoff,
   	// which can exceed host_ttl by a large margin.
   	const auto now = main_thread_dispatcher_.timeSource().monotonicTime().time_since_epoch();
   	const auto elapsed = now - primary_host_info->host_info_->lastUsedTime();
   	const std::chrono::milliseconds raw_backoff_ms(
   	primary_host_info->failure_backoff_strategy_->nextBackOffMs());
   	std::chrono::milliseconds refresh_interval(raw_backoff_ms);
   	if (elapsed >= host_ttl_) {
   	refresh_interval = std::chrono::milliseconds(0);
   	} else {
   	const auto until_eviction =
   	std::chrono::duration_cast<std::chrono::milliseconds>(host_ttl_ - elapsed);
   	refresh_interval = std::min(refresh_interval, until_eviction);
   	}
   	// Floor the result at min_refresh_interval_ (dns_min_refresh_rate) to prevent arming
   	// a ms-scale alarm that can kick rapid-fire resolves and race with dispatcher/resolver
   	// teardown in integration tests (observed as a LeakSanitizer leak in
   	// proxy_filter_integration_test DoubleResolution).
   	refresh_interval =
   	std::max(refresh_interval,
   	std::chrono::duration_cast<std::chrono::milliseconds>(min_refresh_interval_));
   	primary_host_info->refresh_timer_->enableTimer(refresh_interval);
   	ENVOY_LOG(debug, "DNS refresh rate reset for host '{}', (failure) raw={} ms armed={} ms",
   	host, raw_backoff_ms.count(), refresh_interval.count());
   	}
   	}

   (if we hadn't set disable_dns_refresh_on_failure it would be rearmed in

   envoy/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

   Line 642 in cd142b9

   primary_host_info->refresh_timer_->enableTimer(refresh_interval);

   )

Repro steps:

1. Set up a dynamic_forward_proxy configuration where disable_dns_refresh_on_failure has been set to true. Set host_ttl to a test-friendly small period, like 60s.
2. Perform proxying to some resolved address, ideally using a DNS name under the tester's control that returns a small TTL (say 60s) to help with testing. Keep proxy requests coming in in a loop so that the DNS cache entry never becomes stale due to lack of use.
3. Deny the host access to the DNS resolver (e.g. using an iptables filter like sudo iptables -A OUTPUT -p udp --dport 53 -j DROP).
4. Verify connections keep being performed to the already-resolved IP address past the 60s TTL.
5. Change the DNS name to resolve to a different IP address.
6. Re-enable the host access to the DNS resolver (e.g.: sudo iptables -D OUTPUT -p udp --dport 53 -j DROP)
7. Verify that connections still keep being performed to the old IP address, and not the new one.

Note: The Envoy_collect tool
gathers a tarball with debug logs, config and the following admin
endpoints: /stats, /clusters and /server_info. Please note if there are
privacy concerns, sanitize the data prior to sharing the tarball/pasting.

Admin and Stats Output:

Include the admin output for the following endpoints: /stats,
/clusters, /routes, /server_info. For more information, refer to the
admin endpoint documentation.

Note: If there are privacy concerns, sanitize the data prior to
sharing.

Config:

Include the config used to configure Envoy.

Logs:

Include the access logs and the Envoy logs.

Note: If there are privacy concerns, sanitize the data prior to
sharing.

Call Stack:

If the Envoy binary is crashing, a call stack is required.
Please refer to the Bazel Stack trace documentation.

Patch that fixes the issue, based on v1.37.5:

diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc
index e245ce66e7..d8cdbb5752 100644
--- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc
+++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc
@@ -561,7 +561,15 @@ void DnsCacheImpl::finishResolve(const std::string& host,
     ENVOY_LOG(debug, "DNS refresh rate reset for host '{}', refresh rate {} ms", host,
               dns_ttl.count() * 1000);
   } else {
-    if (!config_.disable_dns_refresh_on_failure()) {
+    if (config_.disable_dns_refresh_on_failure()) {
+      // Force a fresh attempt at resolution next time by removing the cache
+      // entry completely. Otherwise, if this is a failed refresh for a host
+      // that had been successfully resolved before, we'd be returning the
+      // last resolved address forever, as we're not rearming the refresh
+      // timer, which is the only mechanism that can expire unused entries.
+      removeHost(host, *primary_host_info, true);
+      ENVOY_LOG(debug, "Removing host '{} from DNS cache, as it failed resolution on refresh", host);
+    } else {
       const uint64_t refresh_interval =
           primary_host_info->failure_backoff_strategy_->nextBackOffMs();
       primary_host_info->refresh_timer_->enableTimer(std::chrono::milliseconds(refresh_interval));