Skip to content

Commit

Permalink
dns: preserve custom resolver after channel destruction (envoyproxy#1…
Browse files Browse the repository at this point in the history 
…3820)

Signed-off-by: Jose Nino <[email protected]>
  • Loading branch information
@junr03
junr03 authored Oct 30, 2020
1 parent c36abec commit 3a32d23
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 42 deletions.
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ Bug Fixes
---------
*Changes expected to improve the state of the world and are unlikely to have negative effects*

* dns: fix a bug where custom resolvers provided in configuration were not preserved after network issues.
* http: fixed URL parsing for HTTP/1.1 fully qualified URLs and connect requests containing IPv6 addresses.
* http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests.
* tls: fix read resumption after triggering buffer high-watermark and all remaining request/response bytes are stored in the SSL connection's internal buffers.
Expand Down
65 changes: 37 additions & 28 deletions source/common/network/dns_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,41 +25,43 @@ DnsResolverImpl::DnsResolverImpl(
const bool use_tcp_for_dns_lookups)
: dispatcher_(dispatcher),
timer_(dispatcher.createTimer([this] { onEventCallback(ARES_SOCKET_BAD, 0); })),
use_tcp_for_dns_lookups_(use_tcp_for_dns_lookups) {

use_tcp_for_dns_lookups_(use_tcp_for_dns_lookups),
resolvers_csv_(maybeBuildResolversCsv(resolvers)) {
AresOptions options = defaultAresOptions();
initializeChannel(&options.options_, options.optmask_);

if (!resolvers.empty()) {
std::vector<std::string> resolver_addrs;
resolver_addrs.reserve(resolvers.size());
for (const auto& resolver : resolvers) {
// This should be an IP address (i.e. not a pipe).
if (resolver->ip() == nullptr) {
ares_destroy(channel_);
throw EnvoyException(
fmt::format("DNS resolver '{}' is not an IP address", resolver->asString()));
}
// Note that the ip()->port() may be zero if the port is not fully specified by the
// Address::Instance.
// resolver->asString() is avoided as that format may be modified by custom
// Address::Instance implementations in ways that make the <port> not a simple
// integer. See https://github.com/envoyproxy/envoy/pull/3366.
resolver_addrs.push_back(fmt::format(resolver->ip()->ipv6() ? "[{}]:{}" : "{}:{}",
resolver->ip()->addressAsString(),
resolver->ip()->port()));
}
const std::string resolvers_csv = absl::StrJoin(resolver_addrs, ",");
int result = ares_set_servers_ports_csv(channel_, resolvers_csv.c_str());
RELEASE_ASSERT(result == ARES_SUCCESS, "");
}
}

DnsResolverImpl::~DnsResolverImpl() {
timer_->disableTimer();
ares_destroy(channel_);
}

absl::optional<std::string> DnsResolverImpl::maybeBuildResolversCsv(
const std::vector<Network::Address::InstanceConstSharedPtr>& resolvers) {
if (resolvers.empty()) {
return absl::nullopt;
}

std::vector<std::string> resolver_addrs;
resolver_addrs.reserve(resolvers.size());
for (const auto& resolver : resolvers) {
// This should be an IP address (i.e. not a pipe).
if (resolver->ip() == nullptr) {
throw EnvoyException(
fmt::format("DNS resolver '{}' is not an IP address", resolver->asString()));
}
// Note that the ip()->port() may be zero if the port is not fully specified by the
// Address::Instance.
// resolver->asString() is avoided as that format may be modified by custom
// Address::Instance implementations in ways that make the <port> not a simple
// integer. See https://github.com/envoyproxy/envoy/pull/3366.
resolver_addrs.push_back(fmt::format(resolver->ip()->ipv6() ? "[{}]:{}" : "{}:{}",
resolver->ip()->addressAsString(),
resolver->ip()->port()));
}
return {absl::StrJoin(resolver_addrs, ",")};
}

DnsResolverImpl::AresOptions DnsResolverImpl::defaultAresOptions() {
AresOptions options{};

Expand All @@ -72,11 +74,19 @@ DnsResolverImpl::AresOptions DnsResolverImpl::defaultAresOptions() {
}

void DnsResolverImpl::initializeChannel(ares_options* options, int optmask) {
dirty_channel_ = false;

options->sock_state_cb = [](void* arg, os_fd_t fd, int read, int write) {
static_cast<DnsResolverImpl*>(arg)->onAresSocketStateChange(fd, read, write);
};
options->sock_state_cb_data = this;
ares_init_options(&channel_, options, optmask | ARES_OPT_SOCK_STATE_CB);

// Ensure that the channel points to custom resolvers, if they exist.
if (resolvers_csv_.has_value()) {
int result = ares_set_servers_ports_csv(channel_, resolvers_csv_->c_str());
RELEASE_ASSERT(result == ARES_SUCCESS, "");
}
}

void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, int timeouts,
Expand Down Expand Up @@ -236,12 +246,11 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name,

// @see DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback for why this is done.
if (dirty_channel_) {
dirty_channel_ = false;
ares_destroy(channel_);

AresOptions options = defaultAresOptions();
initializeChannel(&options.options_, options.optmask_);
}

std::unique_ptr<PendingResolution> pending_resolution(
new PendingResolution(*this, callback, dispatcher_, channel_, dns_name));
if (dns_lookup_family == DnsLookupFamily::Auto) {
Expand Down
4 changes: 4 additions & 0 deletions source/common/network/dns_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
int optmask_;
};

static absl::optional<std::string>
maybeBuildResolversCsv(const std::vector<Network::Address::InstanceConstSharedPtr>& resolvers);

// Callback for events on sockets tracked in events_.
void onEventCallback(os_fd_t fd, uint32_t events);
// c-ares callback when a socket state changes, indicating that libevent
Expand All @@ -105,6 +108,7 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
bool dirty_channel_{};
const bool use_tcp_for_dns_lookups_;
absl::node_hash_map<int, Event::FileEventPtr> events_;
const absl::optional<std::string> resolvers_csv_;
};

} // namespace Network
Expand Down
75 changes: 61 additions & 14 deletions test/common/network/dns_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -434,18 +434,22 @@ class DnsImplTest : public testing::TestWithParam<Address::IpVersion> {
: api_(Api::createApiForTest()), dispatcher_(api_->allocateDispatcher("test_thread")) {}

void SetUp() override {
resolver_ = dispatcher_->createDnsResolver({}, use_tcp_for_dns_lookups());

// Instantiate TestDnsServer and listen on a random port on the loopback address.
server_ = std::make_unique<TestDnsServer>(*dispatcher_);
socket_ = std::make_shared<Network::TcpListenSocket>(
Network::Test::getCanonicalLoopbackAddress(GetParam()), nullptr, true);
listener_ = dispatcher_->createListener(socket_, *server_, true, ENVOY_TCP_BACKLOG_SIZE);

if (setResolverInConstructor()) {
resolver_ = dispatcher_->createDnsResolver({socket_->localAddress()}, useTcpForDnsLookups());
} else {
resolver_ = dispatcher_->createDnsResolver({}, useTcpForDnsLookups());
}

// Point c-ares at the listener with no search domains and TCP-only.
peer_ = std::make_unique<DnsResolverImplPeer>(dynamic_cast<DnsResolverImpl*>(resolver_.get()));
if (tcp_only()) {
peer_->resetChannelTcpOnly(zero_timeout());
if (tcpOnly()) {
peer_->resetChannelTcpOnly(zeroTimeout());
}
ares_set_servers_ports_csv(peer_->channel(), socket_->localAddress()->asString().c_str());
}
Expand Down Expand Up @@ -539,9 +543,10 @@ class DnsImplTest : public testing::TestWithParam<Address::IpVersion> {

protected:
// Should the DnsResolverImpl use a zero timeout for c-ares queries?
virtual bool zero_timeout() const { return false; }
virtual bool tcp_only() const { return true; }
virtual bool use_tcp_for_dns_lookups() const { return false; }
virtual bool zeroTimeout() const { return false; }
virtual bool tcpOnly() const { return true; }
virtual bool useTcpForDnsLookups() const { return false; }
virtual bool setResolverInConstructor() const { return false; }
std::unique_ptr<TestDnsServer> server_;
std::unique_ptr<DnsResolverImplPeer> peer_;
Network::MockConnectionHandler connection_handler_;
Expand Down Expand Up @@ -579,7 +584,7 @@ TEST_P(DnsImplTest, DestructCallback) {

// This simulates destruction thanks to another query setting the dirty_channel_ bit, thus causing
// a subsequent result to call ares_destroy.
peer_->resetChannelTcpOnly(zero_timeout());
peer_->resetChannelTcpOnly(zeroTimeout());
ares_set_servers_ports_csv(peer_->channel(), socket_->localAddress()->asString().c_str());

dispatcher_->run(Event::Dispatcher::RunType::Block);
Expand Down Expand Up @@ -704,8 +709,8 @@ TEST_P(DnsImplTest, DestroyChannelOnRefused) {
EXPECT_FALSE(peer_->isChannelDirty());

// Reset the channel to point to the TestDnsServer, and make sure resolution is healthy.
if (tcp_only()) {
peer_->resetChannelTcpOnly(zero_timeout());
if (tcpOnly()) {
peer_->resetChannelTcpOnly(zeroTimeout());
}
ares_set_servers_ports_csv(peer_->channel(), socket_->localAddress()->asString().c_str());

Expand Down Expand Up @@ -878,7 +883,7 @@ TEST_P(DnsImplTest, PendingTimerEnable) {

class DnsImplZeroTimeoutTest : public DnsImplTest {
protected:
bool zero_timeout() const override { return true; }
bool zeroTimeout() const override { return true; }
};

// Parameterize the DNS test server socket address.
Expand All @@ -898,8 +903,8 @@ TEST_P(DnsImplZeroTimeoutTest, Timeout) {

class DnsImplAresFlagsForTcpTest : public DnsImplTest {
protected:
bool tcp_only() const override { return false; }
bool use_tcp_for_dns_lookups() const override { return true; }
bool tcpOnly() const override { return false; }
bool useTcpForDnsLookups() const override { return true; }
};

// Parameterize the DNS test server socket address.
Expand All @@ -923,7 +928,7 @@ TEST_P(DnsImplAresFlagsForTcpTest, TcpLookupsEnabled) {

class DnsImplAresFlagsForUdpTest : public DnsImplTest {
protected:
bool tcp_only() const override { return false; }
bool tcpOnly() const override { return false; }
};

// Parameterize the DNS test server socket address.
Expand All @@ -945,5 +950,47 @@ TEST_P(DnsImplAresFlagsForUdpTest, UdpLookupsEnabled) {
ares_destroy_options(&opts);
}

class DnsImplCustomResolverTest : public DnsImplTest {
bool tcpOnly() const override { return false; }
bool useTcpForDnsLookups() const override { return true; }
bool setResolverInConstructor() const override { return true; }
};

// Parameterize the DNS test server socket address.
INSTANTIATE_TEST_SUITE_P(IpVersions, DnsImplCustomResolverTest,
testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
TestUtility::ipTestParamsToString);

TEST_P(DnsImplCustomResolverTest, CustomResolverValidAfterChannelDestruction) {
ASSERT_FALSE(peer_->isChannelDirty());
server_->addHosts("some.good.domain", {"201.134.56.7"}, RecordType::A);
server_->setRefused(true);

EXPECT_NE(nullptr,
resolveWithExpectations("some.good.domain", DnsLookupFamily::V4Only,
DnsResolver::ResolutionStatus::Failure, {}, {}, absl::nullopt));
dispatcher_->run(Event::Dispatcher::RunType::Block);
// The c-ares channel should be dirty because the TestDnsServer replied with return code REFUSED;
// This test, and the way the TestDnsServerQuery is setup, relies on the fact that Envoy's
// c-ares channel is configured **without** the ARES_FLAG_NOCHECKRESP flag. This causes c-ares to
// discard packets with REFUSED, and thus Envoy receives ARES_ECONNREFUSED due to the code here:
// https://github.com/c-ares/c-ares/blob/d7e070e7283f822b1d2787903cce3615536c5610/ares_process.c#L654
// If that flag needs to be set, or c-ares changes its handling this test will need to be updated
// to create another condition where c-ares invokes onAresGetAddrInfoCallback with status ==
// ARES_ECONNREFUSED.
EXPECT_TRUE(peer_->isChannelDirty());

server_->setRefused(false);

// The next query destroys, and re-initializes the channel. Furthermore, because the test dns
// server's address was passed as a custom resolver on construction, the new channel should still
// point to the test dns server, and the query should succeed.
EXPECT_NE(nullptr, resolveWithExpectations("some.good.domain", DnsLookupFamily::Auto,
DnsResolver::ResolutionStatus::Success,
{"201.134.56.7"}, {}, absl::nullopt));
dispatcher_->run(Event::Dispatcher::RunType::Block);
EXPECT_FALSE(peer_->isChannelDirty());
}

} // namespace Network
} // namespace Envoy

0 comments on commit 3a32d23

Please sign in to comment.