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

c-ares: add option for udp_max_queries #33551

Merged
merged 14 commits into from
Apr 29, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ package envoy.extensions.network.dns_resolver.cares.v3;
import "envoy/config/core/v3/address.proto";
import "envoy/config/core/v3/resolver.proto";

import "google/protobuf/wrappers.proto";

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

Expand All @@ -18,6 +20,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// [#extension: envoy.network.dns_resolver.cares]

// Configuration for c-ares DNS resolver.
// [#next-free-field: 6]
message CaresDnsResolverConfig {
// A list of dns resolver addresses.
// :ref:`use_resolvers_as_fallback<envoy_v3_api_field_extensions.network.dns_resolver.cares.v3.CaresDnsResolverConfig.use_resolvers_as_fallback>`
Expand All @@ -41,4 +44,8 @@ message CaresDnsResolverConfig {

// Configuration of DNS resolver option flags which control the behavior of the DNS resolver.
config.core.v3.DnsResolverOptions dns_resolver_options = 2;

// This option allows for number of UDP based DNS queries to be capped. Note, this
// is only applicable to c-ares DNS resolver currently.
google.protobuf.UInt32Value udp_max_queries = 5;
}
4 changes: 4 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,9 @@ new_features:
change: |
Added :ref:`Filter State Input <envoy_v3_api_msg_extensions.matching.common_inputs.network.v3.FilterStateInput>`
for matching http input based on filter state objects.
- area: cares
change: |
Added :ref:`udp_max_queries<envoy_v3_api_field_extensions.network.dns_resolver.cares.v3.CaresDnsResolverConfig.udp_max_queries>`
option to limit the number of UDP queries.
deprecated:
9 changes: 9 additions & 0 deletions source/extensions/network/dns_resolver/cares/dns_impl.cc
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "source/extensions/network/dns_resolver/cares/dns_impl.h"

#include <ares.h>

#include <chrono>
#include <cstdint>
#include <list>
Expand Down Expand Up @@ -33,6 +35,8 @@ DnsResolverImpl::DnsResolverImpl(
timer_(dispatcher.createTimer([this] { onEventCallback(ARES_SOCKET_BAD, 0); })),
dns_resolver_options_(config.dns_resolver_options()),
use_resolvers_as_fallback_(config.use_resolvers_as_fallback()),
udp_max_queries_(
static_cast<uint32_t>(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, udp_max_queries, 0))),
resolvers_csv_(maybeBuildResolversCsv(resolvers)),
filter_unroutable_families_(config.filter_unroutable_families()),
scope_(root_scope.createScope("dns.cares.")), stats_(generateCaresDnsResolverStats(*scope_)) {
Expand Down Expand Up @@ -92,6 +96,11 @@ DnsResolverImpl::AresOptions DnsResolverImpl::defaultAresOptions() {
options.options_.flags |= ARES_FLAG_NOSEARCH;
}

if (udp_max_queries_ > 0) {
options.optmask_ |= ARES_OPT_UDP_MAX_QUERIES;
options.options_.udp_max_queries = udp_max_queries_;
}

return options;
}

Expand Down
1 change: 1 addition & 0 deletions source/extensions/network/dns_resolver/cares/dns_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I

absl::node_hash_map<int, Event::FileEventPtr> events_;
const bool use_resolvers_as_fallback_;
const uint32_t udp_max_queries_;
const absl::optional<std::string> resolvers_csv_;
const bool filter_unroutable_families_;
Stats::ScopeSharedPtr scope_;
Expand Down
32 changes: 32 additions & 0 deletions test/extensions/network/dns_resolver/cares/dns_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,7 @@ class DnsImplTest : public testing::TestWithParam<Address::IpVersion> {
}

cares.set_filter_unroutable_families(filterUnroutableFamilies());
cares.set_allocated_udp_max_queries(udpMaxQueries());

// Copy over the dns_resolver_options_.
cares.mutable_dns_resolver_options()->MergeFrom(dns_resolver_options);
Expand Down Expand Up @@ -947,6 +948,7 @@ class DnsImplTest : public testing::TestWithParam<Address::IpVersion> {
virtual void updateDnsResolverOptions(){};
virtual bool setResolverInConstructor() const { return false; }
virtual bool filterUnroutableFamilies() const { return false; }
virtual ProtobufWkt::UInt32Value* udpMaxQueries() const { return 0; }
Stats::TestUtil::TestStore stats_store_;
NiceMock<Runtime::MockLoader> runtime_;
std::unique_ptr<TestDnsServer> server_;
Expand Down Expand Up @@ -2090,5 +2092,35 @@ TEST_P(DnsImplCustomResolverTest, CustomResolverValidAfterChannelDestruction) {
EXPECT_FALSE(peer_->isChannelDirty());
}

class DnsImplAresFlagsForMaxUdpQueriesinTest : public DnsImplTest {
protected:
bool tcpOnly() const override { return false; }
ProtobufWkt::UInt32Value* udpMaxQueries() const override {
auto udp_max_queries = std::make_unique<ProtobufWkt::UInt32Value>();
udp_max_queries->set_value(100);
return dynamic_cast<ProtobufWkt::UInt32Value*>(udp_max_queries.release());
}
};

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

// Validate that c_ares flag `ARES_OPT_UDP_MAX_QUERIES` is set when UInt32 property
// `udp_max_queries` is set.
TEST_P(DnsImplAresFlagsForMaxUdpQueriesinTest, UdpMaxQueriesIsSet) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:
Is there a way to verify the UdpMaxQueries number actually is enforced by the c-ares library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yanjunxiang-google - do you mean by checking opts.udp_max_queries or something more involved ? I tried poking in that area but couldn't find anything obvious - if you have suggestions - happy to look into it!

Copy link

@rakeshdatta rakeshdatta Apr 18, 2024

Choose a reason for hiding this comment

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

I was pushing c'ares team for this change : c-ares/c-ares#444

I believe you can test like this:

  • set this value as n
  • keep pumping traffic.
  • Dont turn on dns over tcp
  • Sniffing ('ngrep -W single -l -q -d any -i "" udp port 53') the DNS packets
  • The src port should keep changing after every n DNS resolution requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

This change should be ok as is, since we rely on c-ares to test the behavior of the ARES_OPT_UDP_MAX_QUERIES flag. I do not think it is important to validate that this flag is working in c-ares.

server_->addCName("root.cname.domain", "result.cname.domain");
server_->addHosts("result.cname.domain", {"201.134.56.7"}, RecordType::A);
ares_options opts{};
int optmask = 0;
EXPECT_EQ(ARES_SUCCESS, ares_save_options(peer_->channel(), &opts, &optmask));
EXPECT_TRUE((optmask & ARES_OPT_UDP_MAX_QUERIES) == ARES_OPT_UDP_MAX_QUERIES);
EXPECT_TRUE(opts.udp_max_queries == 100);
EXPECT_NE(nullptr,
resolveWithUnreferencedParameters("root.cname.domain", DnsLookupFamily::Auto, true));
ares_destroy_options(&opts);
}

} // namespace Network
} // namespace Envoy