c-ares: add option for udp_max_queries by deveshkandpal1224 · Pull Request #33551 · envoyproxy/envoy

deveshkandpal1224 · 2024-04-16T06:49:27Z

Commit Message: Provides an option to set udp_max_queries in DNSResolverOptions to limit max UDP queries.
Additional Description: This change does following:

Implements the fix in this version of c-ares library that has been raised in this comment.
Updates cares_dns_resolver.proto to expose the option to pass udp_max_queries which can then be passed to c-ares.
Adds a unit test to validate that when udp_max_queries is set, ARES_OPT_UDP_MAX_QUERIES flag is set as well.

Risk Level: Low
Testing: Unit test
Docs Changes: n/a
Release Notes: updated
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

repokitteh-read-only · 2024-04-16T06:49:39Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @htuch

🐱

Caused by: #33551 was opened by deveshkandpal1224.

see: more, trace.

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

adisuissa · 2024-04-16T13:21:59Z

@yanjunxiang-google can you please take a first pass?

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

yanjunxiang-google · 2024-04-16T19:27:26Z

source/extensions/network/dns_resolver/cares/dns_impl.cc

    // 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) {


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?

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 ?

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

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

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

yanjunxiang-google · 2024-04-16T19:36:00Z

test/extensions/network/dns_resolver/cares/dns_impl_test.cc

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


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

@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!

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.

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.

yanjunxiang-google · 2024-04-16T19:36:38Z

LGTM in high level except a few nit comments.

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

yanjunxiang-google · 2024-04-18T15:24:50Z

LGTM

lizan · 2024-04-18T16:31:08Z

/lgtm api

@yanavlasov for cares DNS resolver owner.

rakeshdatta

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.

yanavlasov

/wait-any

yanavlasov · 2024-04-22T17:28:28Z

api/envoy/config/core/v3/resolver.proto

+
+  // 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;


Would it make sense to move this option in c-ares specific configuration: https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/network/dns_resolver/cares/v3/cares_dns_resolver.proto ?

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.

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

c-ares: add option for udp_max_queries

ae8bd25

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

deveshkandpal1224 requested review from mattklein123 and yanavlasov as code owners April 16, 2024 06:49

repokitteh-read-only bot added api deps Approval required for changes to Envoy's external dependencies labels Apr 16, 2024

repokitteh-read-only bot assigned lizan and htuch Apr 16, 2024

resolve conflicts

79ef702

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

deveshkandpal1224 added 2 commits April 16, 2024 11:03

fix whitespace

2e40135

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

fix line length issue

b97ad5a

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

yanjunxiang-google reviewed Apr 16, 2024

View reviewed changes

fix current.yaml for doc update

16a1761

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

yanjunxiang-google reviewed Apr 16, 2024

View reviewed changes

deveshkandpal1224 added 4 commits April 16, 2024 23:14

fix segfault in unit test

35d6a19

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

resolve conflict

5f0625a

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

fix format

3a117a9

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

add test for udp_max_queries

bc4470c

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

repokitteh-read-only bot removed the api label Apr 18, 2024

lizan assigned yanavlasov and unassigned lizan Apr 18, 2024

rakeshdatta reviewed Apr 18, 2024

View reviewed changes

deveshkandpal1224 mentioned this pull request Apr 22, 2024

c-ares: upgrade library to 1.20.1 #33711

Merged

yanavlasov reviewed Apr 22, 2024

View reviewed changes

repokitteh-read-only bot added the waiting:any label Apr 22, 2024

ramaraochavali mentioned this pull request Apr 24, 2024

DNS resolution failure results in UH / no healthy upstream #31992

Closed

repokitteh-read-only bot removed waiting:any labels Apr 26, 2024

deveshkandpal1224 added 2 commits April 27, 2024 22:44

fix merge conflicts

973c8ae

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

move udp_max_queries to cares

b59d5dc

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

repokitteh-read-only bot added the api label Apr 28, 2024

deveshkandpal1224 added 3 commits April 28, 2024 16:54

refactor and fix test

5da6454

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

fix current.yaml

f97933a

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

update api ref

392cc7e

Signed-off-by: deveshkandpal1224 <deveshkandpal1224@gmail.com>

yanavlasov approved these changes Apr 29, 2024

View reviewed changes

yanavlasov merged commit 048d9a2 into envoyproxy:main Apr 29, 2024

deveshkandpal1224 deleted the devesh24/upgrade-c-ares branch April 29, 2024 17:10

agrawroh mentioned this pull request Aug 1, 2024

Increased DNS Timeouts After Upgrading to v1.31.0 #35117

Closed

Conversation

deveshkandpal1224 commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 16, 2024

Uh oh!

adisuissa commented Apr 16, 2024

Uh oh!

yanjunxiang-google Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deveshkandpal1224 Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

deveshkandpal1224 Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

deveshkandpal1224 Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

rakeshdatta Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov Apr 29, 2024

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google commented Apr 16, 2024

Uh oh!

yanjunxiang-google commented Apr 18, 2024

Uh oh!

lizan commented Apr 18, 2024

Uh oh!

rakeshdatta left a comment

Choose a reason for hiding this comment

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

yanavlasov Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

deveshkandpal1224 Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

deveshkandpal1224 commented Apr 16, 2024 •

edited

Loading

yanjunxiang-google Apr 16, 2024 •

edited

Loading

deveshkandpal1224 Apr 16, 2024 •

edited

Loading

rakeshdatta Apr 18, 2024 •

edited

Loading