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
6 changes: 6 additions & 0 deletions api/envoy/config/core/v3/resolver.proto
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ package envoy.config.core.v3;

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

import "google/protobuf/wrappers.proto";

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

Expand All @@ -22,6 +24,10 @@ message DnsResolverOptions {

// Do not use the default search domains; only query hostnames as-is or as aliases.
bool no_default_search_domain = 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 = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was in two minds thinking that if tomorrow there was an option for apple dns resolver, it could be leveraged. But happy to move this to c-ares if that makes more sense.

}

// DNS resolution configuration which includes the underlying dns resolver addresses and options.
Expand Down
6 changes: 3 additions & 3 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -218,12 +218,12 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "c-ares",
project_desc = "C library for asynchronous DNS requests",
project_url = "https://c-ares.haxx.se/",
version = "1.19.1",
sha256 = "321700399b72ed0e037d0074c629e7741f6b2ec2dda92956abe3e9671d3e268e",
version = "1.20.0",
sha256 = "cde8433e9bf6c6a0d9e7e69947745ee649256d76009d6c23b9555f84c5c13988",
strip_prefix = "c-ares-{version}",
urls = ["https://github.com/c-ares/c-ares/releases/download/cares-{underscore_version}/c-ares-{version}.tar.gz"],
use_category = ["dataplane_core", "controlplane"],
release_date = "2023-05-22",
release_date = "2023-10-07",
cpe = "cpe:2.3:a:c-ares_project:c-ares:*",
license = "c-ares",
license_url = "https://github.com/c-ares/c-ares/blob/cares-{underscore_version}/LICENSE.md",
Expand Down
4 changes: 4 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,10 @@ new_features:
- area: rbac
change: |
Added additional statistics for rules and shadow rules to the RBAC HTTP filter.
- area: cares
change: |
Added :ref:`udp_max_queries<envoy_v3_api_field_config.core.v3.DnsResolverOptions.udp_max_queries>`
option to limit the number of UDP queries.

deprecated:
- area: listener
Expand Down
10 changes: 9 additions & 1 deletion 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 @@ -92,6 +94,12 @@ DnsResolverImpl::AresOptions DnsResolverImpl::defaultAresOptions() {
options.options_.flags |= ARES_FLAG_NOSEARCH;
}

if (dns_resolver_options_.has_udp_max_queries()) {
options.optmask_ |= ARES_OPT_FLAGS;
options.options_.flags |= ARES_OPT_UDP_MAX_QUERIES;
options.options_.udp_max_queries = dns_resolver_options_.udp_max_queries().value();
}

return options;
}

Expand Down Expand Up @@ -194,7 +202,7 @@ void DnsResolverImpl::AddrInfoPendingResolution::onAresGetAddrInfoCallback(
//
// The channel cannot be destroyed and reinitialized here because that leads to a c-ares
// segfault.
if (status == ARES_ECONNREFUSED) {
if (status == ARES_ECONNREFUSED || status == ARES_EREFUSED) {
Copy link
Contributor

@yanjunxiang-google yanjunxiang-google Apr 16, 2024

Choose a reason for hiding this comment

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

nit: This change is for updating c-ares library to 1.20, but have nothing to do with the udp_max_queries option support. Does it make sense to separate it?

Copy link
Contributor Author

@deveshkandpal1224 deveshkandpal1224 Apr 16, 2024

Choose a reason for hiding this comment

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

DestroyChannelOnRefused and CustomResolverValidAfterChannelDestruction test fail in dns_impl_test.cc after upgrading the c-ares library to 1.20 if this isn't added, while udp_max_queries is supported in 1.20 so kind of tied together if that makes sense ? Or do you suggest splitting this into 2 PRs ?

Copy link
Member

Choose a reason for hiding this comment

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

Can you split into two PRs? It will make it easier to review the dependency updates vs. the UDP query specific change proposed. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure I can split it. Let me send out a PR for library update.

parent_.dirty_channel_ = true;
}
}
Expand Down
30 changes: 30 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 @@ -2089,5 +2089,35 @@ TEST_P(DnsImplCustomResolverTest, CustomResolverValidAfterChannelDestruction) {
EXPECT_FALSE(peer_->isChannelDirty());
}

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

// 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((opts.flags & ARES_OPT_UDP_MAX_QUERIES) == ARES_OPT_UDP_MAX_QUERIES);
EXPECT_NE(nullptr,
resolveWithUnreferencedParameters("root.cname.domain", DnsLookupFamily::Auto, true));
ares_destroy_options(&opts);
}

} // namespace Network
} // namespace Envoy
Loading