Skip to content

dynamic_forward_proxy: adding dns_resolvers to dns_cache used by the proxy filter #16294

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

Merged
mattklein123 merged 52 commits into from
May 27, 2021
Merged

dynamic_forward_proxy: adding dns_resolvers to dns_cache used by the proxy filter #16294

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
c152c7e
dynamic_forward_proxy: adding dns_resolvers to dns_cache used by the …
May 3, 2021
9fa85a7
fix test format
May 3, 2021
3988eb0
fix version history doc
May 3, 2021
026ca82
updating description for dns_resolvers option
May 4, 2021
6260b09
fixing memory leak issue
May 4, 2021
9d95c7b
add DnsResolver message
May 14, 2021
5501231
update dns_resolvers to resolvers
May 14, 2021
d3aaa31
Merge branch 'main' of https://github.com/envoyproxy/envoy into envoy…
May 14, 2021
a673969
Merge branch 'envoyproxy-main' into main
May 14, 2021
c03697b
fix release doc
May 14, 2021
7d41f09
moving DnsResolver to its own proto file
May 18, 2021
0677cf9
deps: update protobuf to 3.16.0 (#16390)
benjaminp May 16, 2021
2feca06
HCM: add support for IP detection extensions (#14855)
May 16, 2021
54af3ba
docs: comment config extension (#16406)
daixiang0 May 17, 2021
9390e7a
add defensive coding against None (for missing buildifier) in pre-com…
May 17, 2021
5c1ae91
docs: update rotation to include watching envoy-ci (#16463)
alyssawilk May 17, 2021
04cc847
docs: Fix subtitle format (#16521)
luckyxiaoqiang May 17, 2021
a9a03bc
redis cluster: fix ClusterSlot operator == (#16116)
gaoliangdut May 17, 2021
be652e9
http: cleaning up obsolete grpc args (#16525)
alyssawilk May 17, 2021
6be5d06
Fix bug in flaky test script (#16434)
May 17, 2021
6d4aa65
dependabot: Updates (#16499)
phlax May 18, 2021
6e99c81
tcp: switching to the new pool (#16465)
alyssawilk May 18, 2021
b67d82e
test: clean up upstream protocols (#16467)
alyssawilk May 18, 2021
2d7e299
disable giant request/response tests under TSAN (#16533)
danzh2010 May 18, 2021
e689756
Allow http route and cluster metadata to contain typed metadata in An…
yanjunxiang-google May 18, 2021
25e4ae0
grid: Plumb the AlternateProtocolCache down to the grid from the Upst…
RyanTheOptimist May 18, 2021
9d2d403
fix mac build (#16514)
ramaraochavali May 18, 2021
e9c44c7
Crash support: Restore crash context on filter's posted callback (#16…
KBaichoo May 18, 2021
7c44cc6
coverage: bumping numbers (#16522)
alyssawilk May 18, 2021
0602d8d
http: more tests for local reply and reset (#16526)
alyssawilk May 18, 2021
6af2490
fix doc
May 18, 2021
4e2a6e8
Merge branch 'main' of git://github.com/envoyproxy/envoy into envoypr…
May 18, 2021
3b41679
Merge branch 'envoyproxy-main' into main
May 18, 2021
f9a8326
missing protodoc-title
May 18, 2021
6234826
add resolver.proto to common messages doc
May 18, 2021
364258f
Merge branch 'envoyproxy:main' into main
ntgsx92 May 19, 2021
e91e5e0
fix release history format
May 19, 2021
8060ed8
fix comment
May 19, 2021
58c0500
Merge branch 'main' of git://github.com/envoyproxy/envoy into envoypr…
May 19, 2021
94c5bd3
Merge branch 'envoyproxy-main' into main
May 19, 2021
19201c5
Merge branch 'main' of https://github.com/ntgsx92/envoy into main
May 19, 2021
8a7d214
doc fix
May 19, 2021
6a3f7e5
feedback
May 20, 2021
e0f0df2
Merge branch 'envoyproxy:main' into main
ntgsx92 May 21, 2021
5eef9b3
feedback
May 26, 2021
80b95ea
Merge branch 'main' of https://github.com/ntgsx92/envoy into main
May 26, 2021
0412e3a
Merge branch 'main' of git://github.com/envoyproxy/envoy into main
May 26, 2021
69345fe
Merge branch 'main' of git://github.com/envoyproxy/envoy into envoypr…
May 26, 2021
e83289a
Merge branch 'envoyproxy-main' into main
May 26, 2021
004177f
Merge branch 'main' of https://github.com/ntgsx92/envoy into main
May 26, 2021
aaaea99
doc fix
May 26, 2021
ecd3229
fix test
May 26, 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
1 change: 1 addition & 0 deletions api/envoy/extensions/common/dynamic_forward_proxy/v3/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ api_proto_package(
deps = [
"//envoy/config/cluster/v3:pkg",
"//envoy/config/common/dynamic_forward_proxy/v2alpha:pkg",
"//envoy/config/core/v3:pkg",
"@com_github_cncf_udpa//udpa/annotations:pkg",
],
)
12 changes: 11 additions & 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 @@ -3,6 +3,7 @@ syntax = "proto3";
package envoy.extensions.common.dynamic_forward_proxy.v3;

import "envoy/config/cluster/v3/cluster.proto";
import "envoy/config/core/v3/address.proto";

import "google/protobuf/duration.proto";
import "google/protobuf/wrappers.proto";
Expand All @@ -27,7 +28,7 @@ message DnsCacheCircuitBreakers {

// Configuration for the dynamic forward proxy DNS cache. See the :ref:`architecture overview
// <arch_overview_http_dynamic_forward_proxy>` for more information.
// [#next-free-field: 9]
// [#next-free-field: 10]
message DnsCacheConfig {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.common.dynamic_forward_proxy.v2alpha.DnsCacheConfig";
Expand Down Expand Up @@ -101,4 +102,13 @@ message DnsCacheConfig {
// ``envoy.restart_features.use_apple_api_for_dns_lookups`` runtime value is true during
// server startup. Apple' API only uses UDP for DNS resolution.
bool use_tcp_for_dns_lookups = 8;

// If DNS resolvers are specified,
// DNS cache will perform DNS resolution via those resolvers.
// Setting this value causes failure if the
// ``envoy.restart_features.use_apple_api_for_dns_lookups`` runtime value is true during
// server startup. Apple's API only allows overriding DNS resolvers via system settings.
// If this setting is not specified, the value defaults to the default
// resolver, which uses /etc/resolv.conf for configuration.
repeated config.core.v3.Address dns_resolvers = 9;
Comment thread
ntgsx92 marked this conversation as resolved.
Outdated

@htuch htuch May 6, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How does this relate to #16237? Should this be a repeated DnsResolver and DnsResolver has both address and DnsLookupOptions?

@ntgsx92 ntgsx92 May 6, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for letting me know about this change.

Looks like we're going to have resolvers, use_tcp_for_dns_lookups and no_defalt_search_domain as the arguments for DnsResolverImpl after #16237 is merged.
Seems like a good time to encapsulate those fields into a single proto message like DnsResolver.

@suniltheta How do you feel about the above changes?

@suniltheta suniltheta May 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for letting me know about this PR. Just recently committed all of the changes for the PR #16237. A new protobuf message DnsResolverOptions in introduced which only include bool fields use_tcp_for_dns_lookups & no_default_search_domain.

Notice the name change from DnsLookupOptions to DnsResolverOptions.

Having said that if we move resolvers inside DnsResolverOptions, then we might have to take care of deprecating dns_resolvers present in config/cluster/v3/cluster.proto & dns_filter/v3alpha/dns_filter.proto.

I lean towards keeping the resolvers (repeated Address type) out of dedicated DnsResolverOptions. Only because if we combine we are looking at a slightly bloated PR and deprecation of a field. But I am also flexible in combining them, if doing so is the best option in the long run.

@htuch htuch May 6, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I would separate DnsResolverOptions and reference it from a DnsResolver message, which can deal with addressing and other concerns not related to resolution options.

@suniltheta suniltheta May 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So is this the format of the protobuf message that is being proposed?

FORMAT A:

└── DnsResolver
    ├── <repeated Address> dns_resolvers
    └── <DnsResolverOptions> dns_resolver_options
        ├── <bool> use_tcp_for_dns_lookups
        ├── <bool> no_default_search_domain
        └── /<any options added in future>/

After the merge of #16237 we do
dispatcher().createDnsResolver(resolvers, dns_resolver_options);

Instead we would be doing
dispatcher().createDnsResolver(dns_resolvers);
So when we call the function createDnsResolver we would be only passing the object of type DnsResolver. Here dns_resolvers will contain both resolvers and dns_resolver_options.

@htuch htuch May 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, fair point. Let's go with #16294 (comment) then.

@ntgsx92 ntgsx92 May 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@htuch What's the next step? @suniltheta and I discussed merging this PR as it is and make another PR to add the new DnsResolver message.

@htuch htuch May 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we're planning on immediately restructuring in the next PR that isn't so great. Can we at least introduce the DnsResolver message and place the list of addresses in there?

@suniltheta suniltheta May 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am on board with this idea as well. We can place the DnsResolver protobuf message in api/envoy/config/core/v3/base.proto or any other place which makes more sense in this PR. Move repeated Address dns_resolvers inside DnsResolver like mentioned in previous comment. Finally as part of #16237 PR we can move the DnsResolverOptions inside the already merged DnsResolver message.

@htuch htuch May 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

}
2 changes: 2 additions & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ Removed Config or Runtime
New Features
------------

* dynamic_forward_proxy: added :ref:`dns_resolvers<envoy_v3_api_field_extensions.common.dynamic_forward_proxy.v3.DnsCacheConfig.dns_resolvers>` option to the DNS cache config in order use custom DNS resolvers instead of the system default resolvers.

* metric service: added support for sending metric tags as labels. This can be enabled by setting the :ref:`emit_tags_as_labels <envoy_v3_api_field_config.metrics.v3.MetricsServiceConfig.emit_tags_as_labels>` field to true.
* udp_proxy: added :ref:`key <envoy_v3_api_msg_extensions.filters.udp.udp_proxy.v3.UdpProxyConfig.HashPolicy>` as another hash policy to support hash based routing on any given key.

Expand Down
1 change: 1 addition & 0 deletions generated_api_shadow/envoy/extensions/common/dynamic_forward_proxy/v3/BUILD
View file
Open in desktop

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

12 changes: 11 additions & 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.

22 changes: 20 additions & 2 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 @@ -4,6 +4,7 @@

#include "common/config/utility.h"
#include "common/http/utility.h"
#include "common/network/resolver_impl.h"
#include "common/network/utility.h"

// TODO(mattklein123): Move DNS family helpers to a smaller include.
Expand All @@ -20,8 +21,8 @@ DnsCacheImpl::DnsCacheImpl(
const envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig& config)
: main_thread_dispatcher_(main_thread_dispatcher),
dns_lookup_family_(Upstream::getDnsLookupFamilyFromEnum(config.dns_lookup_family())),
resolver_(main_thread_dispatcher.createDnsResolver({}, config.use_tcp_for_dns_lookups())),
tls_slot_(tls), scope_(root_scope.createScope(fmt::format("dns_cache.{}.", config.name()))),
resolver_(selectDnsResolver(config, main_thread_dispatcher)), tls_slot_(tls),
scope_(root_scope.createScope(fmt::format("dns_cache.{}.", config.name()))),
stats_(generateDnsCacheStats(*scope_)),
resource_manager_(*scope_, loader, config.name(), config.dns_cache_circuit_breaker()),
refresh_interval_(PROTOBUF_GET_MS_OR_DEFAULT(config, dns_refresh_rate, 60000)),
Expand All @@ -46,6 +47,23 @@ DnsCacheImpl::~DnsCacheImpl() {
}
}

Network::DnsResolverSharedPtr DnsCacheImpl::selectDnsResolver(
const envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig& config,
Event::Dispatcher& main_thread_dispatcher) {
if (!config.dns_resolvers().empty()) {
const auto& resolver_addrs = config.dns_resolvers();
std::vector<Network::Address::InstanceConstSharedPtr> resolvers;
resolvers.reserve(resolver_addrs.size());
for (const auto& resolver_addr : resolver_addrs) {
resolvers.push_back(Network::Address::resolveProtoAddress(resolver_addr));
}
const bool use_tcp_for_dns_lookups = config.use_tcp_for_dns_lookups();

@adisuissa adisuissa May 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can pass directly to the parameter.

@suniltheta suniltheta May 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @adisuissa, since the PR #16237 will follow this PR(16294) shortly, this line will be replaced by using dns_resolver_options. Can this PR get merged as it is?

return main_thread_dispatcher.createDnsResolver(resolvers, use_tcp_for_dns_lookups);
}

return main_thread_dispatcher.createDnsResolver({}, config.use_tcp_for_dns_lookups());
}

DnsCacheStats DnsCacheImpl::generateDnsCacheStats(Stats::Scope& scope) {
return {ALL_DNS_CACHE_STATS(POOL_COUNTER(scope), POOL_GAUGE(scope))};
}
Expand Down
3 changes: 3 additions & 0 deletions source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ class DnsCacheImpl : public DnsCache, Logger::Loggable<Logger::Id::forward_proxy
const envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig& config);
~DnsCacheImpl() override;
static DnsCacheStats generateDnsCacheStats(Stats::Scope& scope);
static Network::DnsResolverSharedPtr selectDnsResolver(
const envoy::extensions::common::dynamic_forward_proxy::v3::DnsCacheConfig& config,
Event::Dispatcher& main_thread_dispatcher);

// DnsCache
LoadDnsCacheEntryResult loadDnsCacheEntry(absl::string_view host, uint16_t default_port,
Expand Down
Loading