Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dns: configuring a basic key value store to persist to disk #17745

Merged
merged 17 commits into from
Aug 25, 2021
Merged

dns: configuring a basic key value store to persist to disk #17745

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
7c3e296
dns: configuring a basic key value store.
alyssawilk Aug 2, 2021
ade78e0
hopefully fixing docs
alyssawilk Aug 17, 2021
253dbae
third try
alyssawilk Aug 17, 2021
7104f0f
tidy
alyssawilk Aug 18, 2021
ca9e396
clang
alyssawilk Aug 18, 2021
1c37b93
moves
alyssawilk Aug 18, 2021
d2b4fb0
fix docs build
alyssawilk Aug 19, 2021
ab188c0
Merge branch 'main' into dns_cache
alyssawilk Aug 19, 2021
f229567
and tidy
alyssawilk Aug 19, 2021
30eb561
review comments
alyssawilk Aug 19, 2021
a94bc2a
docs build
alyssawilk Aug 19, 2021
5dd50d7
more moves
alyssawilk Aug 23, 2021
51063a8
comments
alyssawilk Aug 23, 2021
2810696
Merge branch 'main' into dns_cache
alyssawilk Aug 24, 2021
7b2b206
windows exclude with TODO
alyssawilk Aug 24, 2021
e9fd018
Merge branch 'main' into dns_cache
alyssawilk Aug 24, 2021
a431763
fix bad merge
alyssawilk Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
review comments 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
  • Loading branch information
@alyssawilk
alyssawilk committed Aug 19, 2021
commit 30eb5610bae0c82ae74c717be45bd80f04a85b2e
1 change: 1 addition & 0 deletions api/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ proto_library(
"//envoy/extensions/clusters/dynamic_forward_proxy/v3:pkg",
"//envoy/extensions/clusters/redis/v3:pkg",
"//envoy/extensions/common/dynamic_forward_proxy/v3:pkg",
"//envoy/extensions/common/key_value/file_based/v3:pkg",
"//envoy/extensions/common/key_value/v3:pkg",
"//envoy/extensions/common/matching/v3:pkg",
"//envoy/extensions/common/ratelimit/v3:pkg",
Expand Down
2 changes: 1 addition & 1 deletion api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,5 +142,5 @@ message DnsCacheConfig {

// [#not-implemented-hide:]
// Configuration to flush the DNS cache to long term storage.
key_value.v3.KeyValueStoreConfig persistent_cache_config = 13;
key_value.v3.KeyValueStoreConfig key_value_config = 13;
}
2 changes: 1 addition & 1 deletion api/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions api/envoy/extensions/common/key_value/file_based/v3/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# DO NOT EDIT. This file is generated by tools/proto_format/proto_sync.py.

load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package")

licenses(["notice"]) # Apache 2

api_proto_package(
deps = ["@com_github_cncf_udpa//udpa/annotations:pkg"],
)
26 changes: 26 additions & 0 deletions api/envoy/extensions/common/key_value/file_based/v3/config.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
syntax = "proto3";

package envoy.extensions.common.key_value.file_based.v3;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>.


import "google/protobuf/any.proto";
import "google/protobuf/duration.proto";

import "udpa/annotations/status.proto";
import "validate/validate.proto";

option java_package = "io.envoyproxy.envoy.extensions.common.key_value.file_based.v3";
option java_outer_classname = "ConfigProto";
option java_multiple_files = true;
option (udpa.annotations.file_status).package_version_status = ACTIVE;

// [#alpha:]
// [#extension: envoy.common.key_value.file_based]
// This is configuration to flush a key value store out to disk.
message FileBasedKeyValueStoreConfig {
// The filename to read the keys and values from, and write the keys and
// values to.
string filename = 1 [(validate.rules).string = {min_len: 1}];

// The interval at which the key value store should be flushed to the file.
google.protobuf.Duration flush_interval = 2;
}
7 changes: 3 additions & 4 deletions api/envoy/extensions/common/key_value/v3/config.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
message KeyValueStoreConfig {
// [#extension-category: envoy.common.key_value]
config.core.v3.TypedExtensionConfig config = 1 [(validate.rules).message = {required: true}];

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

// [#alpha:]
Expand All @@ -35,4 +31,7 @@ message FileBasedKeyValueStoreConfig {
// The filename to read the keys and values from, and write the keys and
// values to.
string filename = 1 [(validate.rules).string = {min_len: 1}];

// The interval at which the key value store should be flushed to the file.
google.protobuf.Duration flush_interval = 2;
}
1 change: 1 addition & 0 deletions api/versioning/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ proto_library(
"//envoy/extensions/clusters/dynamic_forward_proxy/v3:pkg",
"//envoy/extensions/clusters/redis/v3:pkg",
"//envoy/extensions/common/dynamic_forward_proxy/v3:pkg",
"//envoy/extensions/common/key_value/file_based/v3:pkg",
"//envoy/extensions/common/key_value/v3:pkg",
"//envoy/extensions/common/matching/v3:pkg",
"//envoy/extensions/common/ratelimit/v3:pkg",
Expand Down
1 change: 1 addition & 0 deletions generated_api_shadow/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ proto_library(
"//envoy/extensions/clusters/dynamic_forward_proxy/v3:pkg",
"//envoy/extensions/clusters/redis/v3:pkg",
"//envoy/extensions/common/dynamic_forward_proxy/v3:pkg",
"//envoy/extensions/common/key_value/file_based/v3:pkg",
"//envoy/extensions/common/key_value/v3:pkg",
"//envoy/extensions/common/matching/v3:pkg",
"//envoy/extensions/common/ratelimit/v3:pkg",
Expand Down
2 changes: 1 addition & 1 deletion generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v4alpha/dns_cache.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions generated_api_shadow/envoy/extensions/common/key_value/file_based/v3/BUILD
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 26 additions & 0 deletions generated_api_shadow/envoy/extensions/common/key_value/file_based/v3/config.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 3 additions & 4 deletions generated_api_shadow/envoy/extensions/common/key_value/v3/config.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 6 additions & 4 deletions source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,12 +442,12 @@ void DnsCacheImpl::removeCacheEntry(const std::string& host) {

void DnsCacheImpl::loadCacheEntries(
const envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig& config) {
if (!config.has_persistent_cache_config()) {
if (!config.has_key_value_config()) {
return;
}
auto& factory = Config::Utility::getAndCheckFactory<KeyValueStoreFactory>(
config.persistent_cache_config().config());
key_value_store_ = factory.createStore(config.persistent_cache_config(), validation_visitor_,
auto& factory =
Config::Utility::getAndCheckFactory<KeyValueStoreFactory>(config.key_value_config().config());
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);
Expand All @@ -456,6 +456,8 @@ void DnsCacheImpl::loadCacheEntries(
}
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 */));
alyssawilk marked this conversation as resolved.
Show resolved Hide resolved
startCacheLoad(key, address->ip()->port());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

finishResolve(key, Network::DnsResolver::ResolutionStatus::Success, std::move(response), true);
Expand Down
1 change: 1 addition & 0 deletions source/extensions/common/key_value/file_based/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ envoy_cc_extension(
"//envoy/filesystem:filesystem_interface",
"//envoy/registry",
"//source/common/common:key_value_store_lib",
"@envoy_api//envoy/extensions/common/key_value/file_based/v3:pkg_cc_proto",
"@envoy_api//envoy/extensions/common/key_value/v3:pkg_cc_proto",
],
)
4 changes: 2 additions & 2 deletions source/extensions/common/key_value/file_based/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ KeyValueStorePtr FileBasedKeyValueStoreFactory::createStore(
const envoy::extensions::common::key_value::v3::KeyValueStoreConfig&>(config,
validation_visitor);
const auto file_config = MessageUtil::anyConvertAndValidate<
envoy::extensions::common::key_value::v3::FileBasedKeyValueStoreConfig>(
envoy::extensions::common::key_value::file_based::v3::FileBasedKeyValueStoreConfig>(
typed_config.config().typed_config(), validation_visitor);
auto seconds =
std::chrono::seconds(DurationUtil::durationToSeconds(typed_config.flush_interval()));
std::chrono::seconds(DurationUtil::durationToSeconds(file_config.flush_interval()));
return std::make_unique<FileBasedKeyValueStore>(dispatcher, seconds, file_system,
file_config.filename());
}
Expand Down
4 changes: 3 additions & 1 deletion source/extensions/common/key_value/file_based/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#include "envoy/common/key_value_store.h"
#include "envoy/extensions/common/key_value/file_based/v3/config.pb.h"
#include "envoy/extensions/common/key_value/file_based/v3/config.pb.validate.h"
#include "envoy/extensions/common/key_value/v3/config.pb.h"
#include "envoy/extensions/common/key_value/v3/config.pb.validate.h"

Expand Down Expand Up @@ -37,7 +39,7 @@ class FileBasedKeyValueStoreFactory : public KeyValueStoreFactory {
// TypedFactory
ProtobufTypes::MessagePtr createEmptyConfigProto() override {
return ProtobufTypes::MessagePtr{
new envoy::extensions::common::key_value::v3::FileBasedKeyValueStoreConfig()};
new envoy::extensions::common::key_value::file_based::v3::FileBasedKeyValueStoreConfig()};
}

std::string name() const override { return "envoy.common.key_value.file_based"; }
Expand Down
1 change: 1 addition & 0 deletions test/extensions/common/dynamic_forward_proxy/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ envoy_cc_test(
"@envoy_api//envoy/config/cluster/v3:pkg_cc_proto",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
"@envoy_api//envoy/extensions/common/dynamic_forward_proxy/v3:pkg_cc_proto",
"@envoy_api//envoy/extensions/common/key_value/file_based/v3:pkg_cc_proto",
],
)

Expand Down
6 changes: 3 additions & 3 deletions test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "envoy/config/cluster/v3/cluster.pb.h"
#include "envoy/config/core/v3/resolver.pb.h"
#include "envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.pb.h"
#include "envoy/extensions/common/key_value/file_based/v3/config.pb.validate.h"

#include "source/common/config/utility.h"
#include "source/common/network/resolver_impl.h"
Expand Down Expand Up @@ -1028,7 +1029,7 @@ TEST_F(DnsCacheImplTest, ResolveSuccessWithCaching) {
MockKeyValueStoreFactory factory;
EXPECT_CALL(factory, createEmptyConfigProto()).WillRepeatedly(Invoke([]() {
return std::make_unique<
envoy::extensions::common::key_value::v3::FileBasedKeyValueStoreConfig>();
envoy::extensions::common::key_value::file_based::v3::FileBasedKeyValueStoreConfig>();
}));
MockKeyValueStore* store{};
EXPECT_CALL(factory, createStore(_, _, _, _)).WillOnce(Invoke([&store]() {
Expand All @@ -1040,8 +1041,7 @@ TEST_F(DnsCacheImplTest, ResolveSuccessWithCaching) {
}));

Registry::InjectFactory<KeyValueStoreFactory> injector(factory);
config_.mutable_persistent_cache_config()->mutable_config()->set_name(
"mock_key_value_store_factory");
config_.mutable_key_value_config()->mutable_config()->set_name("mock_key_value_store_factory");

initialize();
InSequence s;
Expand Down
8 changes: 4 additions & 4 deletions test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ name: dynamic_forward_proxy
max_hosts: {}
dns_cache_circuit_breaker:
max_pending_requests: {}
persistent_cache_config:
key_value_config:
config:
name: envoy.common.key_value.file_based
typed_config:
"@type": type.googleapis.com/envoy.extensions.common.key_value.v3.FileBasedKeyValueStoreConfig
"@type": type.googleapis.com/envoy.extensions.common.key_value.file_based.v3.FileBasedKeyValueStoreConfig
filename: {}
)EOF",
Network::Test::ipVersionToDnsFamily(GetParam()),
Expand Down Expand Up @@ -83,11 +83,11 @@ name: envoy.clusters.dynamic_forward_proxy
max_hosts: {}
dns_cache_circuit_breaker:
max_pending_requests: {}
persistent_cache_config:
key_value_config:
config:
name: envoy.common.key_value.file_based
typed_config:
"@type": type.googleapis.com/envoy.extensions.common.key_value.v3.FileBasedKeyValueStoreConfig
"@type": type.googleapis.com/envoy.extensions.common.key_value.file_based.v3.FileBasedKeyValueStoreConfig
filename: {}
)EOF",
Network::Test::ipVersionToDnsFamily(GetParam()), max_hosts, max_pending_requests, filename);
Expand Down
2 changes: 1 addition & 1 deletion tools/extensions/extensions_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
"envoy.retry_host_predicates", "envoy.retry_priorities", "envoy.stats_sinks",
"envoy.thrift_proxy.filters", "envoy.tracers", "envoy.transport_sockets.downstream",
"envoy.transport_sockets.upstream", "envoy.tls.cert_validator", "envoy.upstreams",
"envoy.wasm.runtime", "envoy.key_value")
"envoy.wasm.runtime", "envoy.common.key_value")

EXTENSION_STATUS_VALUES = (
# This extension is stable and is expected to be production usable.
Expand Down