dns_cache: tracking ttl but not yet using it

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

@@ -193,25 +193,28 @@ void DnsCacheImpl::startCacheLoad(const std::string& host, uint16_t default_port
     return;
   }
 
-  const auto host_attributes = Http::Utility::parseAuthority(host);
+  primary_host = createHost(host, default_port);
+  startResolve(host, *primary_host);
+}
 
+DnsCacheImpl::PrimaryHostInfo* DnsCacheImpl::createHost(const std::string& host,
+                                                        uint16_t default_port) {
+  const auto host_attributes = Http::Utility::parseAuthority(host);
   // TODO(mattklein123): Right now, the same host with different ports will become two
   // independent primary hosts with independent DNS resolutions. I'm not sure how much this will
   // matter, but we could consider collapsing these down and sharing the underlying DNS resolution.
   {
     absl::WriterMutexLock writer_lock{&primary_hosts_lock_};
-    primary_host = primary_hosts_
-                       // try_emplace() is used here for direct argument forwarding.
-                       .try_emplace(host, std::make_unique<PrimaryHostInfo>(
-                                              *this, std::string(host_attributes.host_),
-                                              host_attributes.port_.value_or(default_port),
-                                              host_attributes.is_ip_address_,
-                                              [this, host]() { onReResolve(host); },
-                                              [this, host]() { onResolveTimeout(host); }))
-                       .first->second.get();
+    return primary_hosts_
+        // try_emplace() is used here for direct argument forwarding.
+        .try_emplace(host,
+                     std::make_unique<PrimaryHostInfo>(
+                         *this, std::string(host_attributes.host_),
+                         host_attributes.port_.value_or(default_port),
+                         host_attributes.is_ip_address_, [this, host]() { onReResolve(host); },
+                         [this, host]() { onResolveTimeout(host); }))
+        .first->second.get();
   }
-
-  startResolve(host, *primary_host);
 }
 
 DnsCacheImpl::PrimaryHostInfo& DnsCacheImpl::getPrimaryHost(const std::string& host) {
@@ -287,9 +290,11 @@ void DnsCacheImpl::startResolve(const std::string& host, PrimaryHostInfo& host_i
 
 void DnsCacheImpl::finishResolve(const std::string& host,
                                  Network::DnsResolver::ResolutionStatus status,
-                                 std::list<Network::DnsResponse>&& response, bool from_cache) {
+                                 std::list<Network::DnsResponse>&& response,
+                                 absl::optional<MonotonicTime> resolution_time) {
   ASSERT(main_thread_dispatcher_.isThreadSafe());
   ENVOY_LOG(debug, "main thread resolve complete for host '{}'. {} results", host, response.size());
+  const bool from_cache = resolution_time.has_value();
 
   // Functions like this one that modify primary_hosts_ are only called in the main thread so we
   // know it is safe to use the PrimaryHostInfo pointers outside of the lock.
@@ -300,9 +305,19 @@ void DnsCacheImpl::finishResolve(const std::string& host,
     return primary_host_it->second.get();
   }();
 
-  const bool first_resolve = !primary_host_info->host_info_->firstResolveComplete();
-  primary_host_info->timeout_timer_->disableTimer();
-  primary_host_info->active_query_ = nullptr;
+  bool first_resolve = false;
+
+  if (!from_cache) {
+    first_resolve = !primary_host_info->host_info_->firstResolveComplete();
+    primary_host_info->timeout_timer_->disableTimer();
+    primary_host_info->active_query_ = nullptr;
+
+    if (status == Network::DnsResolver::ResolutionStatus::Failure) {
+      stats_.dns_query_failure_.inc();
+    } else {
+      stats_.dns_query_success_.inc();
+    }
+  }
 
   // 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
@@ -312,12 +327,6 @@ void DnsCacheImpl::finishResolve(const std::string& host,
                                                                       primary_host_info->port_)
                                : nullptr;
 
-  if (status == Network::DnsResolver::ResolutionStatus::Failure) {
-    stats_.dns_query_failure_.inc();
-  } else {
-    stats_.dns_query_success_.inc();
-  }
-
   // Only the change the address if:
   // 1) The new address is valid &&
   // 2a) The host doesn't yet have an address ||
@@ -328,26 +337,35 @@ void DnsCacheImpl::finishResolve(const std::string& host,
   bool address_changed = false;
   auto current_address = primary_host_info->host_info_->address();
   if (new_address != nullptr && (current_address == nullptr || *current_address != *new_address)) {
-    if (!from_cache) {
-      addCacheEntry(host, new_address);
-    }
-    // TODO(alyssawilk) don't immediately push cached entries to threads.
-    // Only serve stale entries if a configured resolve timeout has fired.
     ENVOY_LOG(debug, "host '{}' address has changed", host);
     primary_host_info->host_info_->setAddress(new_address);
     runAddUpdateCallbacks(host, primary_host_info->host_info_);
     address_changed = true;
     stats_.host_address_changed_.inc();
   }
 
-  if (first_resolve || address_changed) {
+  if (!resolution_time.has_value()) {
+    resolution_time = main_thread_dispatcher_.timeSource().monotonicTime();
+  }
+  if (new_address) {
+    // Always update the cache entry and staleness.
+    addCacheEntry(host, new_address, response.front().ttl_);
+    primary_host_info->host_info_->updateStale(resolution_time.value(), response.front().ttl_);
+  }
+
+  if (first_resolve) {
     primary_host_info->host_info_->setFirstResolveComplete();
+  }
+  if (first_resolve || address_changed) {
+    // TODO(alyssawilk) only notify threads of stale results after a resolution
+    // timeout.
     notifyThreads(host, primary_host_info->host_info_);
   }
 
   // Kick off the refresh timer.
   // TODO(mattklein123): Consider jitter here. It may not be necessary since the initial host
   // is populated dynamically.
+  // TODO(alyssawilk) also consider TTL here.
   if (status == Network::DnsResolver::ResolutionStatus::Success) {
     failure_backoff_strategy_->reset();
     primary_host_info->refresh_timer_->enableTimer(refresh_interval_);
@@ -424,12 +442,16 @@ DnsCacheImpl::PrimaryHostInfo::~PrimaryHostInfo() {
 }
 
 void DnsCacheImpl::addCacheEntry(const std::string& host,
-                                 const Network::Address::InstanceConstSharedPtr& address) {
+                                 const Network::Address::InstanceConstSharedPtr& address,
+                                 const std::chrono::seconds& ttl) {
   if (!key_value_store_) {
     return;
   }
-  // TODO(alyssawilk) cache data should include TTL, or some other indicator.
-  const std::string value = absl::StrCat(address->asString());
+  MonotonicTime now = main_thread_dispatcher_.timeSource().monotonicTime();
+  uint64_t seconds_since_epoch =
+      std::chrono::duration_cast<std::chrono::seconds>(now.time_since_epoch()).count();
+  const std::string value =
+      absl::StrCat(address->asString(), "|", ttl.count(), "|", seconds_since_epoch);
   key_value_store_->addOrUpdate(host, value);
 }
 
@@ -450,18 +472,42 @@ void DnsCacheImpl::loadCacheEntries(
   key_value_store_ = factory.createStore(config.key_value_config(), validation_visitor_,
                                          main_thread_dispatcher_, file_system_);
   KeyValueStore::ConstIterateCb load = [this](const std::string& key, const std::string& value) {
-    auto address = Network::Utility::parseInternetAddressAndPortNoThrow(value);
-    if (address == nullptr) {
+    Network::Address::InstanceConstSharedPtr address;
+    const auto parts = StringUtil::splitToken(value, "|");
+    std::chrono::seconds ttl(0);
+    absl::optional<MonotonicTime> resolution_time;
+    if (parts.size() == 3) {
+      address = Network::Utility::parseInternetAddressAndPortNoThrow(std::string(parts[0]));
+      if (address == nullptr) {
+        ENVOY_LOG(warn, "{} is not a valid address", parts[0]);
+      }
+      uint64_t ttl_int;
+      if (absl::SimpleAtoi(parts[1], &ttl_int) && ttl_int != 0) {
+        ttl = std::chrono::seconds(ttl_int);
+      } else {
+        ENVOY_LOG(warn, "{} is not a valid ttl", parts[1]);
+      }
+      uint64_t epoch_int;
+      if (absl::SimpleAtoi(parts[2], &epoch_int)) {
+        MonotonicTime now = main_thread_dispatcher_.timeSource().monotonicTime();
+        const std::chrono::seconds seconds_since_epoch =
+            std::chrono::duration_cast<std::chrono::seconds>(now.time_since_epoch());
+        resolution_time = main_thread_dispatcher_.timeSource().monotonicTime() -
+                          (seconds_since_epoch - std::chrono::seconds(epoch_int));
+      }
+    } else {
+      ENVOY_LOG(warn, "Incorrect number of tokens in the cache line");
+    }
+    if (address == nullptr || ttl == std::chrono::seconds(0) || !resolution_time.has_value()) {
       ENVOY_LOG(warn, "Unable to parse cache line '{}'", value);
       return KeyValueStore::Iterate::Break;
     }
     stats_.cache_load_.inc();
     std::list<Network::DnsResponse> response;
-    // TODO(alyssawilk) change finishResolve to actually use the TTL rather than
-    // putting 0 here, return the remaining TTL or indicate the result is stale.
-    response.emplace_back(Network::DnsResponse(address, std::chrono::seconds(0) /* ttl */));
-    startCacheLoad(key, address->ip()->port());
-    finishResolve(key, Network::DnsResolver::ResolutionStatus::Success, std::move(response), true);
+    createHost(key, address->ip()->port());
+    response.emplace_back(Network::DnsResponse(address, ttl));
+    finishResolve(key, Network::DnsResolver::ResolutionStatus::Success, std::move(response),
+                  resolution_time);
     return KeyValueStore::Iterate::Continue;
   };
   key_value_store_->iterate(load);

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h

@@ -112,6 +112,9 @@ class DnsCacheImpl : public DnsCache, Logger::Loggable<Logger::Id::forward_proxy
     const std::string& resolvedHost() const override { return resolved_host_; }
     bool isIpAddress() const override { return is_ip_address_; }
     void touch() final { last_used_time_ = time_source_.monotonicTime().time_since_epoch(); }
+    void updateStale(MonotonicTime resolution_time, const std::chrono::seconds ttl) {
+      stale_at_time_ = resolution_time + ttl;
+    }
 
     void setAddress(Network::Address::InstanceConstSharedPtr address) {
       absl::WriterMutexLock lock{&resolve_lock_};
@@ -141,6 +144,7 @@ class DnsCacheImpl : public DnsCache, Logger::Loggable<Logger::Id::forward_proxy
     // Using std::chrono::steady_clock::duration is required for compilation within an atomic vs.
     // using MonotonicTime.
     std::atomic<std::chrono::steady_clock::duration> last_used_time_;
+    std::atomic<MonotonicTime> stale_at_time_;
     bool first_resolve_complete_ ABSL_GUARDED_BY(resolve_lock_){false};
   };
 
@@ -177,7 +181,8 @@ class DnsCacheImpl : public DnsCache, Logger::Loggable<Logger::Id::forward_proxy
   void startResolve(const std::string& host, PrimaryHostInfo& host_info)
       ABSL_LOCKS_EXCLUDED(primary_hosts_lock_);
   void finishResolve(const std::string& host, Network::DnsResolver::ResolutionStatus status,
-                     std::list<Network::DnsResponse>&& response, bool from_cache = false);
+                     std::list<Network::DnsResponse>&& response,
+                     absl::optional<MonotonicTime> resolution_time = {});
   void runAddUpdateCallbacks(const std::string& host, const DnsHostInfoSharedPtr& host_info);
   void runRemoveCallbacks(const std::string& host);
   void notifyThreads(const std::string& host, const DnsHostInfoImplSharedPtr& resolved_info);
@@ -186,10 +191,12 @@ class DnsCacheImpl : public DnsCache, Logger::Loggable<Logger::Id::forward_proxy
   PrimaryHostInfo& getPrimaryHost(const std::string& host);
 
   void addCacheEntry(const std::string& host,
-                     const Network::Address::InstanceConstSharedPtr& address);
+                     const Network::Address::InstanceConstSharedPtr& address,
+                     const std::chrono::seconds& ttl);
   void removeCacheEntry(const std::string& host);
   void loadCacheEntries(
       const envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig& config);
+  PrimaryHostInfo* createHost(const std::string& host, uint16_t default_port);
 
   Event::Dispatcher& main_thread_dispatcher_;
   const Network::DnsLookupFamily dns_lookup_family_;

test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc

@@ -1025,6 +1025,9 @@ TEST(UtilityTest, PrepareDnsRefreshStrategy) {
 }
 
 TEST_F(DnsCacheImplTest, ResolveSuccessWithCaching) {
+  auto* time_source = new NiceMock<MockTimeSystem>();
+  dispatcher_.time_system_.reset(time_source);
+
   // Configure the cache.
   MockKeyValueStoreFactory factory;
   EXPECT_CALL(factory, createEmptyConfigProto()).WillRepeatedly(Invoke([]() {
@@ -1064,14 +1067,14 @@ TEST_F(DnsCacheImplTest, ResolveSuccessWithCaching) {
 
   EXPECT_CALL(*timeout_timer, disableTimer());
   // Make sure the store gets the first insert.
-  EXPECT_CALL(*store, addOrUpdate("foo.com", "10.0.0.1:80"));
   EXPECT_CALL(update_callbacks_,
               onDnsHostAddOrUpdate("foo.com", DnsHostInfoEquals("10.0.0.1:80", "foo.com", false)));
+  EXPECT_CALL(*store, addOrUpdate("foo.com", "10.0.0.1:80|30|0"));
   EXPECT_CALL(callbacks,
               onLoadDnsCacheComplete(DnsHostInfoEquals("10.0.0.1:80", "foo.com", false)));
   EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(60000), _));
   resolve_cb(Network::DnsResolver::ResolutionStatus::Success,
-             TestUtility::makeDnsResponse({"10.0.0.1"}));
+             TestUtility::makeDnsResponse({"10.0.0.1"}, std::chrono::seconds(30)));
 
   checkStats(1 /* attempt */, 1 /* success */, 0 /* failure */, 1 /* address changed */,
              1 /* added */, 0 /* removed */, 1 /* num hosts */);
@@ -1087,9 +1090,10 @@ TEST_F(DnsCacheImplTest, ResolveSuccessWithCaching) {
 
   // Address does not change.
   EXPECT_CALL(*timeout_timer, disableTimer());
+  EXPECT_CALL(*store, addOrUpdate("foo.com", "10.0.0.1:80|30|0"));
   EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(60000), _));
   resolve_cb(Network::DnsResolver::ResolutionStatus::Success,
-             TestUtility::makeDnsResponse({"10.0.0.1"}));
+             TestUtility::makeDnsResponse({"10.0.0.1"}, std::chrono::seconds(30)));
 
   checkStats(2 /* attempt */, 2 /* success */, 0 /* failure */, 1 /* address changed */,
              1 /* added */, 0 /* removed */, 1 /* num hosts */);
@@ -1105,15 +1109,31 @@ TEST_F(DnsCacheImplTest, ResolveSuccessWithCaching) {
 
   EXPECT_CALL(*timeout_timer, disableTimer());
   // Make sure the store gets the updated address.
-  EXPECT_CALL(*store, addOrUpdate("foo.com", "10.0.0.2:80"));
   EXPECT_CALL(update_callbacks_,
               onDnsHostAddOrUpdate("foo.com", DnsHostInfoEquals("10.0.0.2:80", "foo.com", false)));
+  EXPECT_CALL(*store, addOrUpdate("foo.com", "10.0.0.2:80|30|0"));
   EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(60000), _));
   resolve_cb(Network::DnsResolver::ResolutionStatus::Success,
-             TestUtility::makeDnsResponse({"10.0.0.2"}));
+             TestUtility::makeDnsResponse({"10.0.0.2"}, std::chrono::seconds(30)));
 
   checkStats(3 /* attempt */, 3 /* success */, 0 /* failure */, 2 /* address changed */,
              1 /* added */, 0 /* removed */, 1 /* num hosts */);
+
+  // Now do one more resolve, where the address does not change but the time
+  // does.
+
+  // Re-resolve timer.
+  EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(5000), nullptr));
+  EXPECT_CALL(*resolver_, resolve("foo.com", _, _))
+      .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_)));
+  resolve_timer->invokeCallback();
+
+  // Address does not change.
+  EXPECT_CALL(*timeout_timer, disableTimer());
+  EXPECT_CALL(*store, addOrUpdate("foo.com", "10.0.0.2:80|40|0"));
+  EXPECT_CALL(*resolve_timer, enableTimer(std::chrono::milliseconds(60000), _));
+  resolve_cb(Network::DnsResolver::ResolutionStatus::Success,
+             TestUtility::makeDnsResponse({"10.0.0.2"}, std::chrono::seconds(40)));
 }
 
 } // namespace

test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc

@@ -122,7 +122,8 @@ name: envoy.clusters.dynamic_forward_proxy
     if (write_cache_file_) {
       std::string host =
           fmt::format("localhost:{}", fake_upstreams_[0]->localAddress()->ip()->port());
-      std::string value = fake_upstreams_[0]->localAddress()->asString();
+      std::string value =
+          absl::StrCat(fake_upstreams_[0]->localAddress()->asString(), "|1000000|0");
       TestEnvironment::writeStringToFileForTest(
           "dns_cache.txt", absl::StrCat(host.length(), "\n", host, value.length(), "\n", value));
     }
@@ -359,7 +360,6 @@ TEST_P(ProxyFilterIntegrationTest, UseCacheFile) {
       sendRequestAndWaitForResponse(request_headers, 1024, default_response_headers_, 1024);
   checkSimpleRequestSuccess(1024, 1024, response.get());
   EXPECT_EQ(1, test_server_->counter("dns_cache.foo.cache_load")->value());
-  EXPECT_EQ(1, test_server_->counter("dns_cache.foo.dns_query_attempt")->value());
   EXPECT_EQ(1, test_server_->counter("dns_cache.foo.host_added")->value());
 }
 #endif

test/mocks/event/mocks.cc

@@ -19,6 +19,7 @@ namespace Event {
 MockDispatcher::MockDispatcher() : MockDispatcher("test_thread") {}
 
 MockDispatcher::MockDispatcher(const std::string& name) : name_(name) {
+  time_system_ = std::make_unique<GlobalTimeSystem>();
   ON_CALL(*this, initializeStats(_, _)).WillByDefault(Return());
   ON_CALL(*this, clearDeferredDeleteList()).WillByDefault(Invoke([this]() -> void {
     to_delete_.clear();

test/mocks/event/mocks.h

@@ -37,7 +37,7 @@ class MockDispatcher : public Dispatcher {
 
   // Dispatcher
   const std::string& name() override { return name_; }
-  TimeSource& timeSource() override { return time_system_; }
+  TimeSource& timeSource() override { return *time_system_; }
   Network::ServerConnectionPtr
   createServerConnection(Network::ConnectionSocketPtr&& socket,
                          Network::TransportSocketPtr&& transport_socket,
@@ -162,7 +162,7 @@ class MockDispatcher : public Dispatcher {
   MOCK_METHOD(void, updateApproximateMonotonicTime, ());
   MOCK_METHOD(void, shutdown, ());
 
-  GlobalTimeSystem time_system_;
+  std::unique_ptr<TimeSource> time_system_;
   std::list<DeferredDeletablePtr> to_delete_;
   testing::NiceMock<MockBufferFactory> buffer_factory_;
   bool allow_null_callback_{};