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.

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

dns: add dns resolver implementation based on Apple APIs #13074

Merged
merged 48 commits into from
Sep 30, 2020
Merged
Show file tree
Hide file tree
Changes from 46 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
6b72142
earliest rough draft
Sep 11, 2020
afb5731
Merge branch 'master' into apple-dns
Sep 11, 2020
3f6adce
done
Sep 12, 2020
8e3e44e
additional comments
Sep 12, 2020
204f3dd
another error
Sep 13, 2020
b5ecbdb
added by accident
Sep 13, 2020
6fb5a14
fmt
Sep 13, 2020
3ecd18a
comments
Sep 14, 2020
1c972cc
comments
Sep 18, 2020
d90ab49
Merge branch 'master' into apple-dns
Sep 18, 2020
09c8946
WIP tests
Sep 22, 2020
875458a
WIP tests
Sep 22, 2020
44a1ce6
just run mac
Sep 22, 2020
25cc48f
WIP tests
Sep 22, 2020
2797de8
less expectations
Sep 22, 2020
1001842
fixed crash
Sep 22, 2020
5c3ab10
cleanup
Sep 22, 2020
eb85d38
compilation
Sep 22, 2020
5f79bd3
fmt
Sep 22, 2020
6ff5a29
fmt
Sep 22, 2020
8374eb6
fmt
Sep 23, 2020
3fd1cc9
remove stale target
Sep 23, 2020
a5477a9
runtime control
Sep 23, 2020
c1facb7
fix
Sep 23, 2020
04e2861
updates
Sep 24, 2020
b220407
word
Sep 24, 2020
7167cb6
Revert "runtime control"
Sep 24, 2020
8062ad0
update
Sep 24, 2020
29749aa
release notes
Sep 24, 2020
2bada07
comments
Sep 25, 2020
896d88a
runtime change
Sep 26, 2020
2332f23
fmt
Sep 26, 2020
7516419
fmt
Sep 26, 2020
ddf8456
fix runtime for tests
Sep 26, 2020
41ba439
Merge branch 'master' into apple-dns
Sep 26, 2020
72cc832
comments
Sep 26, 2020
9286227
wording on docs
Sep 26, 2020
c3bbdd6
more docs
Sep 26, 2020
9a6120d
more docs
Sep 26, 2020
4491132
Merge branch 'master' into apple-dns
Sep 28, 2020
2d04495
build comment
Sep 28, 2020
b533f2c
double quote
Sep 28, 2020
4800d0d
shellcheck
Sep 28, 2020
fd2521f
all test target
Sep 28, 2020
6737b8f
additional tests
Sep 28, 2020
6f14724
comments
Sep 28, 2020
40b40e4
verbose assert message
Sep 30, 2020
80fb3f7
switch
Sep 30, 2020
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
6 changes: 6 additions & 0 deletions api/envoy/api/v2/cluster.proto
Original file line number Diff line number Diff line change
Expand Up @@ -677,10 +677,16 @@ message Cluster {
// :ref:`STRICT_DNS<envoy_api_enum_value_Cluster.DiscoveryType.STRICT_DNS>`
// and :ref:`LOGICAL_DNS<envoy_api_enum_value_Cluster.DiscoveryType.LOGICAL_DNS>`
// this setting is ignored.
// 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.
repeated core.Address dns_resolvers = 18;

// [#next-major-version: Reconcile DNS options in a single message.]
// Always use TCP queries instead of UDP queries for DNS lookups.
// Setting this value causes failure if the
// ``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 = 45;

// If specified, outlier detection will be enabled for this upstream cluster.
Expand Down
3 changes: 3 additions & 0 deletions api/envoy/config/bootstrap/v2/bootstrap.proto
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,9 @@ message Bootstrap {
// when :ref:`dns_resolvers <envoy_api_field_Cluster.dns_resolvers>` and
// :ref:`use_tcp_for_dns_lookups <envoy_api_field_Cluster.use_tcp_for_dns_lookups>` are
// specified.
// Setting this value causes failure if the
// ``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 = 20;
}

Expand Down
3 changes: 3 additions & 0 deletions api/envoy/config/bootstrap/v3/bootstrap.proto
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,9 @@ message Bootstrap {
// when :ref:`dns_resolvers <envoy_api_field_config.cluster.v3.Cluster.dns_resolvers>` and
// :ref:`use_tcp_for_dns_lookups <envoy_api_field_config.cluster.v3.Cluster.use_tcp_for_dns_lookups>` are
// specified.
// Setting this value causes failure if the
// ``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 = 20;

// Specifies optional bootstrap extensions to be instantiated at startup time.
Expand Down
3 changes: 3 additions & 0 deletions api/envoy/config/bootstrap/v4alpha/bootstrap.proto

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

6 changes: 6 additions & 0 deletions api/envoy/config/cluster/v3/cluster.proto
Original file line number Diff line number Diff line change
Expand Up @@ -829,10 +829,16 @@ message Cluster {
// :ref:`STRICT_DNS<envoy_api_enum_value_config.cluster.v3.Cluster.DiscoveryType.STRICT_DNS>`
// and :ref:`LOGICAL_DNS<envoy_api_enum_value_config.cluster.v3.Cluster.DiscoveryType.LOGICAL_DNS>`
// this setting is ignored.
// 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.
repeated core.v3.Address dns_resolvers = 18;

// [#next-major-version: Reconcile DNS options in a single message.]
// Always use TCP queries instead of UDP queries for DNS lookups.
// Setting this value causes failure if the
// ``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 = 45;

// If specified, outlier detection will be enabled for this upstream cluster.
Expand Down
6 changes: 6 additions & 0 deletions api/envoy/config/cluster/v4alpha/cluster.proto

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

Original file line number Diff line number Diff line change
Expand Up @@ -98,5 +98,8 @@ message DnsCacheConfig {

// [#next-major-version: Reconcile DNS options in a single message.]
// Always use TCP queries instead of UDP queries for DNS lookups.
// Setting this value causes failure if the
// ``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;
}
7 changes: 5 additions & 2 deletions ci/mac_ci_steps.sh
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,13 @@ BAZEL_BUILD_OPTIONS=(
if [[ $# -gt 0 ]]; then
TEST_TARGETS=$*
else
TEST_TARGETS=//test/integration/...
TEST_TARGETS='//test/integration/...'
fi

if [[ "$TEST_TARGETS" == "//test/..." || "$TEST_TARGETS" == "//test/integration/..." ]]; then
bazel build "${BAZEL_BUILD_OPTIONS[@]}" //source/exe:envoy-static
fi
bazel test "${BAZEL_BUILD_OPTIONS[@]}" ${TEST_TARGETS}
bazel test "${BAZEL_BUILD_OPTIONS[@]}" "${TEST_TARGETS}"

# Additionally run macOS specific test suites
bazel test "${BAZEL_BUILD_OPTIONS[@]}" //test/common/network:apple_dns_impl_test
2 changes: 1 addition & 1 deletion ci/run_clang_tidy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ function exclude_win32_impl() {
# Do not run clang-tidy against macOS impl
# TODO: We should run clang-tidy against macOS impl for completeness
function exclude_macos_impl() {
grep -v source/common/filesystem/kqueue/
grep -v source/common/filesystem/kqueue/ | grep -v source/common/network/apple_dns_impl | grep -v test/common/network/apple_dns_impl_test
}

# Do not run incremental clang-tidy on check_format testdata files.
Expand Down
2 changes: 2 additions & 0 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ Incompatible Behavior Changes
*Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required*

* build: added visibility rules for upstream. If these cause visibility related breakage, see notes in //BUILD.
* dns: ``envoy.restart_features.use_apple_api_for_dns_lookups`` is on by default. This flag only affects Apple platforms (macOS, iOS). It is incompatible to have the runtime flag set to true at the same time as specifying the ``use_tcp_for_dns_lookups`` option or custom dns resolvers. Doing so will cause failure.
* watchdog: added two guarddogs, breaking the aggregated stats for the single guarddog system. The aggregated stats for the guarddogs will have the following prefixes: `main_thread` and `workers`. Concretely, anything monitoring `server.watchdog_miss` and `server.watchdog_mega_miss` will need to be updated.

Minor Behavior Changes
Expand Down Expand Up @@ -80,6 +81,7 @@ New Features
* build: enable building envoy :ref:`arm64 images <arm_binaries>` by buildx tool in x86 CI platform.
* cluster: added new :ref:`connection_pool_per_downstream_connection <envoy_v3_api_field_config.cluster.v3.Cluster.connection_pool_per_downstream_connection>` flag, which enable creation of a new connection pool for each downstream connection.
* decompressor filter: reports compressed and uncompressed bytes in trailers.
* dns: added support for doing DNS resolution using Apple's DnsService APIs in Apple platforms (macOS, iOS). This feature is ON by default, and is only configurable via the ``envoy.restart_features.use_apple_api_for_dns_lookups`` runtime key. Note that this value is latched during server startup and changing the runtime key is a no-op during the lifetime of the process.
* dns_filter: added support for answering :ref:`service record<envoy_v3_api_msg_data.dns.v3.DnsTable.DnsService>` queries.
* dynamic_forward_proxy: added :ref:`use_tcp_for_dns_lookups<envoy_v3_api_field_extensions.common.dynamic_forward_proxy.v3.DnsCacheConfig.use_tcp_for_dns_lookups>` option to use TCP for DNS lookups in order to match the DNS options for :ref:`Clusters<envoy_v3_api_msg_config.cluster.v3.Cluster>`.
* ext_authz filter: added support for emitting dynamic metadata for both :ref:`HTTP <config_http_filters_ext_authz_dynamic_metadata>` and :ref:`network <config_network_filters_ext_authz_dynamic_metadata>` filters.
Expand Down
6 changes: 6 additions & 0 deletions generated_api_shadow/envoy/api/v2/cluster.proto

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

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

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

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

6 changes: 6 additions & 0 deletions generated_api_shadow/envoy/config/cluster/v3/cluster.proto

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

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

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

7 changes: 5 additions & 2 deletions source/common/event/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,14 @@ envoy_cc_library(
"//source/common/common:assert_lib",
"//source/common/common:thread_lib",
"//source/common/filesystem:watcher_lib",
"//source/common/network:connection_lib",
"//source/common/network:dns_lib",
"//source/common/network:connection_lib",
"//source/common/network:listener_lib",
"//source/common/runtime:runtime_features_lib",
],
] + select({
"//bazel:apple": ["//source/common/network:apple_dns_lib"],
"//conditions:default": [],
}),
)

envoy_cc_library(
Expand Down
19 changes: 19 additions & 0 deletions source/common/event/dispatcher_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,18 @@
#include "common/network/dns_impl.h"
#include "common/network/tcp_listener_impl.h"
#include "common/network/udp_listener_impl.h"
#include "common/runtime/runtime_features.h"

#include "event2/event.h"

#ifdef ENVOY_HANDLE_SIGNALS
#include "common/signal/signal_action.h"
#endif

#ifdef __APPLE__
#include "common/network/apple_dns_impl.h"
#endif

namespace Envoy {
namespace Event {

Expand Down Expand Up @@ -121,6 +126,20 @@ Network::DnsResolverSharedPtr DispatcherImpl::createDnsResolver(
const std::vector<Network::Address::InstanceConstSharedPtr>& resolvers,
const bool use_tcp_for_dns_lookups) {
ASSERT(isThreadSafe());
#ifdef __APPLE__
static bool use_apple_api_for_dns_lookups =
Runtime::runtimeFeatureEnabled("envoy.restart_features.use_apple_api_for_dns_lookups");
if (use_apple_api_for_dns_lookups) {
RELEASE_ASSERT(
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we be rejecting the config earlier in a more graceful way? If so, shouldn't these be ASSERTs?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this should be exceptions that fail during config load.

Copy link
Member Author

Choose a reason for hiding this comment

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

It all comes down to snapping the runtime once by making it static local state here vs checking earlier in callers of this function.

I guess I could change this function to return nullptr on failure, and have callers deal with the return value?

Copy link
Member

Choose a reason for hiding this comment

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

I believe the DNS resolver is created during config load time in all cases. I think you can just throw an exception here.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's the case for all xDS, assuming this also holds for dynamic forward proxy, SG.

Copy link
Member Author

Choose a reason for hiding this comment

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

It holds for the dynamic proxy. Let me check in the UDP filter. If that's the case, I'll update with an exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

@htuch @mattklein123 unfortunately, the UDP DNS filter creates a resolver (underneath its own wrapper) for every filter instance construction:

resolver_ = std::make_unique<DnsFilterResolver>(resolver_callback_, config->resolvers(),

So can't throw an exception as is (and per @htuch the assert is not great either).

The underlying resolver in the UDP DNS filter should definitely be shared, and I can work to untangle. However, I don't want to block this PR too much (we need it to be able to run envoy mobile on ios 14 devices). How do you feel about landing this PR with a warn log in place of the assert. Then I can work on untangling the filter situation, and subsequently upgrading the warn log here into an exception?

Copy link
Member

@mattklein123 mattklein123 Sep 29, 2020

Choose a reason for hiding this comment

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

I would actually rather you just leave the RELEASE_ASSERT and make sure the error message is good so the OSX user can flip the runtime flag, then come back and clean it up. So just leave a TODO for that? @htuch?

resolvers.empty(),
"defining custom resolvers is not possible when using Apple APIs for DNS resolution. "
"Apple's API only allows overriding DNS resolvers via system settings.");
RELEASE_ASSERT(!use_tcp_for_dns_lookups,
"using TCP for DNS lookups is not possible when using Apple APIs for DNS "
"resolution. Apple' API only uses UDP for DNS resolution");
return Network::DnsResolverSharedPtr{new Network::AppleDnsResolverImpl(*this)};
}
#endif
return Network::DnsResolverSharedPtr{
new Network::DnsResolverImpl(*this, resolvers, use_tcp_for_dns_lookups)};
}
Expand Down
21 changes: 21 additions & 0 deletions source/common/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,27 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "apple_dns_lib",
srcs = select({
"//bazel:apple": ["apple_dns_impl.cc"],
"//conditions:default": [],
}),
hdrs = select({
"//bazel:apple": ["apple_dns_impl.h"],
"//conditions:default": [],
}),
deps = [
":address_lib",
":utility_lib",
"//include/envoy/event:dispatcher_interface",
"//include/envoy/event:file_event_interface",
"//include/envoy/network:dns_interface",
"//source/common/common:assert_lib",
"//source/common/common:linked_object",
],
)

envoy_cc_library(
name = "dns_lib",
srcs = ["dns_impl.cc"],
Expand Down
Loading