dns_cache: tracking ttl but not yet using it by alyssawilk · Pull Request #17951 · envoyproxy/envoy

alyssawilk · 2021-09-01T17:35:25Z

Fixing the dns cache to not fake resolution
tracking TTL and staleness
flushing ttl and last resolution time to disk

This shouldn't have a functional change (apart from what we cache to disk, which isn't yet used) but I have mild preference for keeping the refactor separate from resolution + lifetime changes.

Risk Level: medium (refactoring code)
Testing: updated unit tests, integration test
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist

Sweet!

RyanTheOptimist · 2021-09-01T18:19:28Z

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h

    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(uint64_t time) { stale_at_time_ = time; }


Is there by any chance a data type we can use here which expresses the semantics here? For example is this a time_t (seconds since the UNIX epoch) or is it more of a TTL (a time delta)? Perhaps one of the std::chrono types? Oh, or maybe just SystemTime directly?

RyanTheOptimist · 2021-09-01T18:24:22Z

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

+  uint64_t ms_since_epoch =
+      std::chrono::duration_cast<std::chrono::milliseconds>(now.time_since_epoch()).count();
+  uint64_t stale_at = ms_since_epoch + std::chrono::duration_cast<std::chrono::seconds>(response.front().ttl_).count();
+  primary_host_info->host_info_->updateStale(stale_at);


nit: It might make sense for this method to take the ttl directly and do the work in the method of coverting the ttl to whatever the internal representation is of "now + ttl"?

RyanTheOptimist · 2021-09-01T18:33:51Z

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

+    Network::Address::InstanceConstSharedPtr address;
+    const auto parts = StringUtil::splitToken(value, "|");
+    std::chrono::seconds ttl(0);
+    if (parts.size() == 3) {


I might be misreading this code but it seems like up on line 452, we write 3 things into the cache value. Down here, we spilt on | and ensure that we have 2 parts. This all makes sense. But I only see code looking at parts[0] (line 478) and parts[1] (line 483) but I don't see a reference to parts[2]. Is this expected?

yeah I wasn't using part two yet. I was going to do in the PR where we start using stale, but I've worked it into this one with the time updates.
I'll say I'm not 100% on the conversion back and forth but it's hard to test until we have stale expirey which is coming in the next PR!

RyanTheOptimist · 2021-09-01T18:42:49Z

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

+  uint64_t ms_since_epoch =
+              std::chrono::duration_cast<std::chrono::milliseconds>(now.time_since_epoch()).count();
+  const std::string value = absl::StrCat(address->asString(), "|", ttl.count(), "|",
+                                         ms_since_epoch);


Not for this PR, but it seems like we could add a wrapper around KeyValueStore in which the Value is allowed to be a type which implements an interfaces that has methods like std::string serialize() and bool deserialise(const std::string&). Then this wrapper class could be something like: TypedKeyValueStore which would have a vanilla KeyValueStore as a member. When reading from / writing to the store, it would transparently transform the Value types to/from strings.

/shrug. Maybe something to think about down the road...

RyanTheOptimist · 2021-09-01T18:46:06Z

test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc

  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(*store, addOrUpdate("foo.com", ContainsRegex("10.0.0.1:80|30|\\d*)")));


Instead of using a Regex, could we use the actual value? Maybe that's hard cause we're using the "real" clock and not a mock clock? (If so, would it be possible to use a mock clock? Probably!)

We can now!

RyanTheOptimist · 2021-09-01T18:46:54Z

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

      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());


Would it be valid to check that this is 0 since the cached entry won't be re-queried?

I think it might race becasue it reregisters for refresh and I'm not sure how long it'll take

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist

Looks great!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-09-08T14:39:11Z

/wait on ci

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp

Some nits but otherwise LGTM

snowp · 2021-09-10T15:03:08Z

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h

  void addCacheEntry(const std::string& host,
-                     const Network::Address::InstanceConstSharedPtr& address);
+                     const Network::Address::InstanceConstSharedPtr& address,
+                     const std::chrono::seconds& ttl);


I think normally we'd pass this by value since it's just a wrapper around an integer

snowp · 2021-09-10T15:05:17Z

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h

    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) {


nit: avoid const value params

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-09-13T13:47:57Z

Matt can you pick up with snow out?

mattklein123 · 2021-09-13T19:10:19Z

@snowp is still in today. @snowp if it's just a final pass can you review today? Otherwise I can take it over.

snowp

Thanks!

dns_cache: tracking ttl but not yet using it

fdf2fc5

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned RyanTheOptimist Sep 1, 2021

alyssawilk requested a review from mattklein123 as a code owner September 1, 2021 17:35

ci

e8e3020

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist reviewed Sep 1, 2021

View reviewed changes

comments

e2b3815

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist previously approved these changes Sep 2, 2021

View reviewed changes

Merge branch 'main' into dns_ttl

b150b55

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned snowp Sep 8, 2021

repokitteh-read-only bot added the waiting label Sep 8, 2021

test fix

3ded0b7

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed RyanTheOptimist’s stale review via 3ded0b7 September 8, 2021 16:09

alyssawilk requested a review from zuercher as a code owner September 8, 2021 16:09

repokitteh-read-only bot removed the waiting label Sep 8, 2021

alyssawilk added 3 commits September 8, 2021 14:49

Merge branch 'main' into dns_ttl

70553bc

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

asan fix

02e9e53

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

fix gcc build?

054f2ef

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp reviewed Sep 10, 2021

View reviewed changes

comments

24c13f3

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned mattklein123 Sep 13, 2021

Merge branch 'main' into dns_ttl

b8cd3b1

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp approved these changes Sep 13, 2021

View reviewed changes

snowp merged commit c51e30c into envoyproxy:main Sep 13, 2021

Conversation

alyssawilk commented Sep 1, 2021

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Sep 8, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Sep 13, 2021

Uh oh!

mattklein123 commented Sep 13, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants