dns: configuring a basic key value store to persist to disk by alyssawilk · Pull Request #17745 · envoyproxy/envoy

alyssawilk · 2021-08-17T16:13:58Z

Risk Level: low (config guarded)
Testing: new unit, integration tests
Docs Changes: n/a (hidden)
Release Notes: n/a
Part of envoyproxy/envoy-mobile#1587

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

repokitteh-read-only · 2021-08-17T16:14:06Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17745 was opened by alyssawilk.

see: more, trace.

alyssawilk · 2021-08-17T16:40:43Z

/wait on CI

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

htuch

Would this overall feature make sense as part of the DNS extensibility being worked on in #17479?

I.e. we add a caching DNS resolver extension (that maybe wraps another DNS resolver like c-ares or platform specific)?

mattklein123 · 2021-08-18T03:48:14Z

I.e. we add a caching DNS resolver extension (that maybe wraps another DNS resolver like c-ares or platform specific)?

I think this would potentially be a nice solution to think about it. FWIW we had discussed something similar today for wrapping DNS in order to get DNS specific analytic events. cc @snowp

alyssawilk · 2021-08-18T12:28:19Z

I'm not sure what the ask is for this PR. I don't want this tied to DNS but I think it'd be reasonable to make sure the DNS refactor allows for all the platform resolvers to use the cache.

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

htuch · 2021-08-18T14:30:32Z

I'm suggesting the DNS resolver itself is a cache.

htuch · 2021-08-18T14:31:39Z

i.e. that we build DNS caching inside the DNS resolver framework. I'm not sure why we want to reuse the existing data plane caching framework for this.

api/envoy/extensions/cache/key_value_cache/v3/config.proto

RyanTheOptimist

A few small questions to get started...

RyanTheOptimist · 2021-08-18T14:47:41Z

source/extensions/cache/key_value_store/file_based/config.h

+//
+// All keys and values are flushed to a single file as
+// [length]\n[key][length]\n[value]
+class FileBasedKeyValueStore : public KeyValueStoreBase {


I was a bit surprised to see that the interface here is Envoy::KeyValueStore but the concrete class is Envoy::Extensions::Cache::KeyValueCache. I had expected them to both live in the same namespace, roughly. But maybe this is a typical way to lay things out?

yeah, per offline conversation with harvey I moved all the things to a location he prefered, so now it's KeyValue and then KeyValueStore which is at least a substring?

source/extensions/cache/key_value_store/file_based/config.cc

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

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

alyssawilk

FYI there's an outstanding issue with both clang tidy and docs build, but API should be ready for review. PTAL.

api/envoy/extensions/cache/key_value_cache/v3/config.proto

source/extensions/cache/key_value_store/file_based/config.cc

alyssawilk · 2021-08-19T00:03:46Z

source/extensions/cache/key_value_store/file_based/config.h

+//
+// All keys and values are flushed to a single file as
+// [length]\n[key][length]\n[value]
+class FileBasedKeyValueStore : public KeyValueStoreBase {


yeah, per offline conversation with harvey I moved all the things to a location he prefered, so now it's KeyValue and then KeyValueStore which is at least a substring?

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

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

htuch

API looks good modulo comments.

htuch · 2021-08-19T03:09:57Z

api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto

+
+  // [#not-implemented-hide:]
+  // Configuration to flush the DNS cache to long term storage.
+  key_value.v3.KeyValueStoreConfig persistent_cache_config = 13;


Nit on naming: given this is referencing an extension point, do we know that there will always be persistence guaranteed?

Good question. I can't think of a use case where we'd add a key-value store and not persist it. We already have a DNS cache so the point of this is to persist. That said, just because I can't think of a use case it certainly doesn't hurt to future-proof :-)

htuch · 2021-08-19T03:10:59Z

api/envoy/extensions/common/key_value/v3/config.proto

+
+  // The interval at which the key value store should be flushed to long term
+  // storage.
+  google.protobuf.Duration flush_interval = 2;


I guess this gets to my earlier question, do we know that all implementations will flush? Or will flushing be a property of a specific key-value story, e.g. the FileBasedKeyValueStoreConfig?

Ditto - current uses all involve persisting but can't hurt to move it to file based and just dup the single value for mobile based.

api/envoy/extensions/common/key_value/v3/config.proto

RyanTheOptimist

Looks good!

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

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

alyssawilk

looking at docs pr - must have failed a string somewhere

alyssawilk · 2021-08-19T15:02:57Z

api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto

+
+  // [#not-implemented-hide:]
+  // Configuration to flush the DNS cache to long term storage.
+  key_value.v3.KeyValueStoreConfig persistent_cache_config = 13;


Good question. I can't think of a use case where we'd add a key-value store and not persist it. We already have a DNS cache so the point of this is to persist. That said, just because I can't think of a use case it certainly doesn't hurt to future-proof :-)

alyssawilk · 2021-08-19T15:03:32Z

api/envoy/extensions/common/key_value/v3/config.proto

+
+  // The interval at which the key value store should be flushed to long term
+  // storage.
+  google.protobuf.Duration flush_interval = 2;


Ditto - current uses all involve persisting but can't hurt to move it to file based and just dup the single value for mobile based.

api/envoy/extensions/common/key_value/v3/config.proto

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

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

api/envoy/extensions/common/key_value/v3/config.proto

htuch · 2021-08-20T04:37:15Z

api/envoy/extensions/common/key_value/file_based/v3/config.proto

@@ -0,0 +1,27 @@
+syntax = "proto3";
+
+package envoy.extensions.common.key_value.file_based.v3;


Nit: this should be in api/envoy/extensions/key_value/filed_based/v3/config.proto and same package namespace.

The rationale is that the KeyValueConfig is a common extension point declaration reused across protos. The FileBasedKkeyValueStoreConfig is a key_value extension, and all extension classes have a root like api/envoy/extensions/<extension class>.

htuch

/lgtm api

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

alyssawilk · 2021-08-23T15:16:01Z

argh sorry, force pushed format fixes. Bad @alyssawilk no biscuit

snowp

Overall this looks pretty good, just a few nits and a question on the interaction between cache load and DNS resolution

api/envoy/extensions/cache/key_value_cache/v3/config.proto

snowp · 2021-08-17T20:07:07Z

envoy/common/key_value_store.h

Should we just pass ServerFactoryContext instead of specifically passing the file system? Might future proof against some other use cases

Unfortunately we can't - the DNS store is created both from ServerFactoryContext and from the cluster's TransportSocketFactoryContextImpl which have wide overlap but no common base class.
I have that filed in away in my clean up list but I think resolving it is out of scope for this PR

source/extensions/cache/key_value_store/file_based/config.cc

snowp · 2021-08-17T20:11:08Z

source/extensions/cache/key_value_store/file_based/config.h

make_unique

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

snowp · 2021-08-23T15:43:54Z

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

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


I'm not that familiar with this code, but it looks like this will kick off a DNS query? And then we immediately resolve it?

If so I wonder if this will cause unforeseen consequences, like having entries being resolved twice (once from cache and once from DNS) and confusing stats (e.g. stat increment for DNS resolution and cache load for same entry)

Thoughts?

it works today (see integration test) but I agree it's not great, hence the TODO :-)

What I want to have happen is be able to differentiate stale cached results from fresh. I do want to immediately kick off a resolve, but also have stale data in there, config for how long we wait for fresh data before we use stale data. I plan to do that in the follow up

For my understanding, would a DNS resolution then overwrite this value once it arrives?

yeah, fresh resolve always overwrites older data (if it's different)
in this case it'd be the same so not trigger any changes.

snowp · 2021-08-23T15:48:04Z

test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc

+  MockKeyValueStoreFactory factory;
+  EXPECT_CALL(factory, createEmptyConfigProto()).WillRepeatedly(Invoke([]() {
+    return std::make_unique<
+        envoy::extensions::key_value::file_based::v3::FileBasedKeyValueStoreConfig>();
+  }));
+  MockKeyValueStore* store{};
+  EXPECT_CALL(factory, createStore(_, _, _, _)).WillOnce(Invoke([&store]() {
+    auto ret = std::make_unique<NiceMock<MockKeyValueStore>>();
+    store = ret.get();
+    // Make sure there's an attempt to load from the key value store.
+    EXPECT_CALL(*store, iterate);
+    return ret;
+  }));


Should this be implemented on the MockKeyValueStoreFactory itself? Having them in the test case here is a bit distracting from what the test is actually testing

maybe? Honestly I regularly get bit by expectations in mock timer where I don't understand what's going on, so I'm hesitant to add complexity like that to the mock base classes

Sure not a big deal, just what I usually see done with mocks. Agree that MockTimer is especially confusing :)

snowp · 2021-08-23T15:48:58Z

test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc

+  InSequence s;
+  ASSERT(store != nullptr);
+
+  // Make sure the store gets the first insert.


It's a bit hard to follow how the below code block verifies anything with the store, maybe make that clearer with comments? (I think it's L1067 but there is a lot of boiler plate here)

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

alyssawilk · 2021-08-24T13:21:19Z

@davinci26 @wrowe any ideas on the windows failure? I've commented out with TODO to unblock this but I'm wondering if you have ideas what went wrong. For example are there known gotchas with file paths on windows CI?

snowp

LG! We definitely need to figure out the Windows thing, but I'm okay with doing that in a follow up

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

davinci26 · 2021-08-24T16:24:11Z

@alyssawilk I am not sure, I will have to debug locally but it is super weird because the same test passes on windows with clang.

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

alyssawilk · 2021-08-25T13:38:50Z

coverage flake unrelated: from a combination of quic refactor and setec patches
I've got #17846 to hep so I'm going to merge over it.

* main: config: fix dfp config validation (envoyproxy#17835) docs: updating where meetings are uploaded (envoyproxy#17832) h2: moving a comment (envoyproxy#17846) quiche: early fail listener config with both quic and connection_balencer (envoyproxy#17834) dns: configuring a basic key value store to persist to disk (envoyproxy#17745) quic: fix receiving STOP_SENDING (envoyproxy#17815) tooling: Add Github release manager (envoyproxy#17741) tooling: Use upstream pytest-patches (envoyproxy#17809) Remove `hidden_envoy_deprecated_use_http2` (envoyproxy#17805) kafka: produce request for mesh-filter (envoyproxy#17818) Signed-off-by: Michael Puncel <mpuncel@squareup.com>

Clean up inspired by #17745 Risk Level: low (interface refactor) Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

dns: configuring a basic key value store.

7c3e296

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

alyssawilk assigned snowp Aug 17, 2021

alyssawilk requested review from lizan and mattklein123 as code owners August 17, 2021 16:13

repokitteh-read-only bot added the api label Aug 17, 2021

repokitteh-read-only bot assigned htuch Aug 17, 2021

repokitteh-read-only bot added the waiting label Aug 17, 2021

hopefully fixing docs

ade78e0

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

repokitteh-read-only bot removed the waiting label Aug 17, 2021

alyssawilk assigned RyanTheOptimist Aug 17, 2021

alyssawilk force-pushed the dns_cache branch from 8bef2eb to b94f7cf Compare August 17, 2021 19:57

third try

253dbae

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

alyssawilk force-pushed the dns_cache branch from b94f7cf to 253dbae Compare August 17, 2021 20:23

htuch reviewed Aug 18, 2021

View reviewed changes

tidy

7104f0f

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

htuch reviewed Aug 18, 2021

View reviewed changes

api/envoy/extensions/cache/key_value_cache/v3/config.proto Outdated Show resolved Hide resolved

RyanTheOptimist reviewed Aug 18, 2021

View reviewed changes

alyssawilk added 2 commits August 18, 2021 11:20

clang

ca9e396

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

moves

1c37b93

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

alyssawilk commented Aug 19, 2021

View reviewed changes

alyssawilk added 3 commits August 18, 2021 21:04

fix docs build

d2b4fb0

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

Merge branch 'main' into dns_cache

ab188c0

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

and tidy

f229567

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

htuch reviewed Aug 19, 2021

View reviewed changes

RyanTheOptimist previously approved these changes Aug 19, 2021

View reviewed changes

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc Show resolved Hide resolved

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc Show resolved Hide resolved

review comments

30eb561

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

alyssawilk dismissed RyanTheOptimist’s stale review via 30eb561 August 19, 2021 16:06

alyssawilk commented Aug 19, 2021

View reviewed changes

docs build

a94bc2a

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

htuch reviewed Aug 20, 2021

View reviewed changes

htuch reviewed Aug 23, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Aug 23, 2021

more moves

5dd50d7

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

alyssawilk force-pushed the dns_cache branch from 3a9ec41 to 5dd50d7 Compare August 23, 2021 15:15

snowp suggested changes Aug 23, 2021

View reviewed changes

alyssawilk added 3 commits August 23, 2021 12:43

comments

51063a8

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

Merge branch 'main' into dns_cache

2810696

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

windows exclude with TODO

7b2b206

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

snowp previously approved these changes Aug 24, 2021

View reviewed changes

Merge branch 'main' into dns_cache

e9fd018

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

fix bad merge

a431763

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

alyssawilk dismissed snowp’s stale review via a431763 August 24, 2021 18:19

alyssawilk merged commit 16879db into envoyproxy:main Aug 25, 2021

alyssawilk mentioned this pull request Sep 13, 2021

factory: cleaning up factory APIs to allow code sharing #18096

Merged

alyssawilk deleted the dns_cache branch August 4, 2022 01:09

		@@ -0,0 +1,27 @@
		syntax = "proto3";

		package envoy.extensions.common.key_value.file_based.v3;

Conversation

alyssawilk commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 17, 2021

Uh oh!

alyssawilk commented Aug 17, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Aug 18, 2021

Uh oh!

alyssawilk commented Aug 18, 2021

Uh oh!

htuch commented Aug 18, 2021

Uh oh!

htuch commented Aug 18, 2021

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch 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!

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alyssawilk 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Aug 23, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Aug 17, 2021 •

edited

Loading