dns cache: do not runRemoveCallbacks if runAddUpdateCallbacks has no…

…t fired (envoyproxy#10003)

Signed-off-by: Jose Nino <jnino@lyft.com>

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

@@ -120,7 +120,12 @@ void DnsCacheImpl::onReResolve(const std::string& host) {
             primary_host_it->second->host_info_->last_used_time_.load().count());
   if (now_duration - primary_host_it->second->host_info_->last_used_time_.load() > host_ttl_) {
     ENVOY_LOG(debug, "host='{}' TTL expired, removing", host);
-    runRemoveCallbacks(host);
+    // If the host has no address then that means that the DnsCacheImpl has never
+    // runAddUpdateCallbacks for this host, and thus the callback targets are not aware of it.
+    // Therefore, runRemoveCallbacks should only be ran if the host's address != nullptr.
+    if (primary_host_it->second->host_info_->address_) {
+      runRemoveCallbacks(host);
+    }
     primary_hosts_.erase(primary_host_it);
     updateTlsHostsMap();
   } else {

test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc

@@ -354,6 +354,7 @@ TEST_F(DnsCacheImplTest, ResolveFailure) {
 
   MockLoadDnsCacheEntryCallbacks callbacks;
   Network::DnsResolver::ResolveCb resolve_cb;
+  Event::MockTimer* resolve_timer = new Event::MockTimer(&dispatcher_);
   EXPECT_CALL(*resolver_, resolve("foo.com", _, _))
       .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_)));
   auto result = dns_cache_->loadDnsCacheEntry("foo.com", 80, callbacks);
@@ -364,13 +365,25 @@ TEST_F(DnsCacheImplTest, ResolveFailure) {
 
   EXPECT_CALL(update_callbacks_, onDnsHostAddOrUpdate(_, _)).Times(0);
   EXPECT_CALL(callbacks, onLoadDnsCacheComplete());
+  EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(60000), _));
   resolve_cb(TestUtility::makeDnsResponse({}));
   checkStats(1 /* attempt */, 0 /* success */, 1 /* failure */, 0 /* address changed */,
              1 /* added */, 0 /* removed */, 1 /* num hosts */);
 
   result = dns_cache_->loadDnsCacheEntry("foo.com", 80, callbacks);
   EXPECT_EQ(DnsCache::LoadDnsCacheEntryStatus::InCache, result.status_);
   EXPECT_EQ(result.handle_, nullptr);
+
+  // Re-resolve with ~5m passed. This is not realistic as we would have re-resolved many times
+  // during this period but it's good enough for the test.
+  simTime().sleep(std::chrono::milliseconds(300001));
+  // Because resolution failed for the host, onDnsHostAddOrUpdate was not called.
+  // Therefore, onDnsHostRemove should not be called either.
+  EXPECT_CALL(update_callbacks_, onDnsHostRemove(_)).Times(0);
+  resolve_timer->invokeCallback();
+  // DnsCacheImpl state is updated accordingly: the host is removed.
+  checkStats(1 /* attempt */, 0 /* success */, 1 /* failure */, 0 /* address changed */,
+             1 /* added */, 1 /* removed */, 0 /* num hosts */);
 }
 
 // Cancel a cache load before the resolve completes.